more gracefully handle incomplete uploads

[tyrasd]

When an upload is aborted mid-way (e.g. by closing the browser tab or because of a network connection issue), iD will ask to restore data in the next mapping session. This can then lead to duplicates, like in this example: object (first changeset), duplicate (second changeset)

Steps to reproduce:

• draw some new features
• hit upload
• close the browser tab during the upload process
• restart iD
• when asked, choose to restore the local data
• hit upload again

These are actions to mitigate this:

• Show the warning that a browser window should not be closed during an active upload.
• If an upload fails during upload: remember that an upload attempt was made and conflate the to be restored data with the freshly fetched OSM data (skipping features which would be duplicates of already existing ones).
• Check and potentially tweak when exactly the locally cached map changes are cleared (should be: right after the upload changeset is closed).

[UKChris-osm]

Could iD provide a checksum on the proposed edits, then compare that checksum again once the upload is completed? If the checksum isn't confirmed, could iD then revert the initial attempt to update OSM, then try again fully in a fresh changeset?

1. iD creates a checksum.
2. iD uploads the checksum.
3. iD uploads the changeset.
4. iD confirms the changeset upload via the checksum.
5. If any interruptions occur, and the checksum isn't confirmed, the database server, or iD, reverts the changeset.
6. iD returns to 1. and tries again if the users selects "restore local edits".

[bryceco]

iD knows the ID of the changeset it last tried to upload. It can fetch that upload and if changes_count is zero then it knows to upload again, otherwise the upload succeeded and it can clear the modified objects.

[bhousel]

Back with #5200 I fixed this exact issue by clearing the user's storage once the changeset was inflight, see 930e865

If this isn't how iD works anymore, I guess you should put that code back in.

[tyrasd]

Thanks @bhousel for the additional background.

If this isn't how iD works anymore

As far as I can tell, this is still how it works (modulo some small changes like the switch to oauth2). From what I can see, your fix did unfortunately never reliably/completely fix the issue: I was able to reproduce the bug also in iD version 2.11 (which was the first one to have the fix from 930e865) using the same steps mentioned above, resulting in https://api06.dev.openstreetmap.org/node/4332777649 and https://api06.dev.openstreetmap.org/node/4332777650 🤷 Also, from what I can see, the logic in your fix seems to be slightly flawed: isChangesetInflight() will also be true during the time the PUT /changetset/create request is pending (which can take a while on a slow network for example). At this point, iD will not have actually uploaded anything yet to the OSM API. Clearing the history at that point will result in a users changes to be lost completely, which is arguably even worse than uploading duplicates. The same argument also applies during the POST phase of the upload, where iD cannot know whether the contents have been received on the API side of the network. Am I overlooking something that prevents that from occurring?

I think we need to replace this with a more robust solution. I really like the approach outlined by @bryceco 👍

[bhousel]

Clearing the history at that point will result in a users changes to be lost completely, which is arguably even worse than uploading duplicates. Or am I overlooking something that prevents that from occurring?

No, you're not overlooking anything - that is the intent.

I guess if we wanted to be more conservative, we could relocate the history to a different localstorage key temporarily and delete it for good once the changeset is known to be uploaded, or relocate it back if we detect an API error. (Honestly we should not even be using localStorage for this anymore - serializing the history is slow and we regularly hit the size limit of what we can store in there).

The thing we are really trying to account for is the situation where a user hits the upload button and then immediately Ctrl-W closes the tab, and opens a new one to keep editing as fast as possible. Once the tab is unloaded, our code is gone and there's nothing we can do about it - this is why the history needs to go away ASAP, and it doesn't make much sense trying to cleverly handle slow connections and API failures.

[simonpoole]

I think we need to replace this with a more robust solution. I really like the approach outlined by @bryceco 👍

@tyrasd There is a longish discussion on this very topic here openstreetmap/openstreetmap-website#2201 Short version while it definitely can be made more robust, fixing it completely would require an API add on. Not a reason not to do what is possible now, but at least I have been a bit wary of this, because the situation in which it occurs (aka flaky networks) will potentially make the fix flaky too.

PS: as iD doesn't do chunk uploads the situation is slightly simpler than for JOSM and Vespucci.