Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry activating photo in note on timeout #2102

Closed
peternewman opened this issue Sep 21, 2020 · 17 comments
Closed

Retry activating photo in note on timeout #2102

peternewman opened this issue Sep 21, 2020 · 17 comments

Comments

@peternewman
Copy link
Collaborator

How to Reproduce
I'm unsure, as future and subsequent uploads succeeded, although this is the only double upload I've ever tried.
https://www.openstreetmap.org/note/2355202

Can you for example see any missing images?

My Internet may have been a bit flaky at the time, but presumably it should retry?

Versions affected
SC 23.0-beta1 Android 9

@westnordost
Copy link
Member

According to the cleanup log, these two photos have never been activated. So, they were successfully uploaded but then StreetComplete did not call the backend to activate the pictures or failed to do so because of the flaky connection, more likely.

I figure it could make sense to retry once or twice if the activate call failed because of a timeout.

@westnordost westnordost changed the title Photo upload failed Retry activating photo in note on timeout Sep 21, 2020
@peternewman
Copy link
Collaborator Author

Sounds good. What's the algorithm for quest answers, I don't (think) I have issues with them, but perhaps I've only noticed this because its more obvious.

For anyone else in this scenario, this may recover your photos if they actually uploaded:
curl -d '{"osm_note_id": 2355202}' --verbose https://westnordost.de/streetcomplete/photo-upload/activate.php

I got:
{"found_photos":2,"activated_photos":0}

So I'm guessing they'd been expired elsewhere; indeed looks like they probably go after 24 hours. I don't know how tight your storage is, would it be worth bumping that expiry lifetime until this is fixed, so people can potentially recover them?

@westnordost
Copy link
Member

I think 24h is fine.

What's the algorithm for quest answers

It is tried once to upload them. If any fails (timeout), the whole upload fails. Quest answers that haven't been uploaded yet, are dropped.

Just kidding, quest answers that haven't been uploaded yet remain in local storage until such time as the upload was successful.

@peternewman
Copy link
Collaborator Author

I think 24h is fine.

I just mean with this current issue 24 hours isn't necessarily enough time to check and realise an image hasn't uploaded (and hence save it). I often only action my notes after a day or two.

It is tried once to upload them. If any fails (timeout), the whole upload fails.
quest answers that haven't been uploaded yet remain in local storage until such time as the upload was successful.

Could activation switch to the same model then (as it seems really rock solid for quests), I guess with a separate queue?

@westnordost
Copy link
Member

Could activation switch to the same model then (as it seems really rock solid for quests), I guess with a separate queue?

No, it can't. Each quest answer is uploaded in exactly one HTTPS request. Either it fails (timeout) or not. For note upload,

  • first each image is uploaded. If any of this fails, if I remember correctly, the whole note upload fails and must try again later (same as for quests)
  • then, the actual note is created / commented. Again, if this fails, the whole note upload fails and must try again later (same as for quests)
  • then, the photos are activated. If this fails, we cannot simply let the note upload fail because the note has already been posted publicly. This is why if the photo activation fails, it fails silently.

@peternewman
Copy link
Collaborator Author

peternewman commented Sep 22, 2020

Could that not be considered as three separate processes though (or do the quest uploads happen in parallel)?

So when you add a note with photos it would add three or more things onto the note queue:
Upload note image 1
Upload note image n
Create note
Activate note images

Then if uploading the image failed, the whole lot would stop. If it succeeded but the note failed, it would retry the note next time, if both succeeded but it didn't activate, it would retry that next time, and hopefully within 24 hours.

Rather than just trying say twice, then silently failing to activate leaving a rather useless note.

@mnalis
Copy link
Member

mnalis commented Nov 30, 2020

* first each image is uploaded. If any of this fails, if I remember correctly, the whole note upload fails and must try again later (same as for quests)

* then, the actual note is created / commented. Again, if this fails, the whole note upload fails and must try again later (same as for quests)

* then, the photos are activated. If this fails, we cannot simply let the note upload fail because the note has already been posted publicly. This is why if the photo activation fails, it fails silently.

Buy why is problematic 3rd step (Activation) even needed? Couldn't photos be activated (whatever that entails) automatically in the first step when images are uploaded? Than there would be no chance for disappearing photos.

@matkoniecz
Copy link
Member

matkoniecz commented Nov 30, 2020

Couldn't photos be activated (whatever that entails) automatically in the first step when images are uploaded?

Then anyone would be able to upload images and it would be necessary to monitor them. See #1030 #1161 for two cases of external services that were used by SC earlier and were killed by rampant abuse.

@mnalis
Copy link
Member

mnalis commented Nov 30, 2020

Ah, thanks for that information, didn't think of that. However, it is still possible to abuse it by also spamming an OSM Notes service, and calling activation link, right? So what was done was raising the barrier for abuse. Perhaps it could be done just as easily by requiring the image posting URL to have a specific HTTP header - then the wannabe abuser would have to use/write some nonstandard client for uploading the photo (and if they're willing to write a nonstandard client to abuse the service, adding calling 3 URLs instead of one is not a big difference)...

But I do agree that possible abuse is big issue, so the above protection might be too weak. But, having unreliable photos is also problematic for users (even if it happens rarely - I've noticed I myself started to retype important info from picture, just in case it gets lost again, which leads to annoying overhead - something which I never did before or and still don't do when using OsmAnd AVNotes).

So perhaps something like peternewman method idea to put "activating notes" as a separate entry in queue after the Note gets submitted to OSM, so it gets retried, and pictures don't get lost...

@matkoniecz
Copy link
Member

matkoniecz commented Nov 30, 2020

Ah, thanks for that information, didn't think of that. However, it is still possible to abuse it by also spamming an OSM Notes service, and calling activation link, right?

Yes, but once note is closed photos will be deleted. What makes it even less attractive to abusers who will do something else.

@westnordost westnordost added this to the Elements first class milestone Feb 28, 2021
@westnordost
Copy link
Member

This is implemented in the dev branch now.

How has this been done?

Note creations and note comments (and answering "can't answer" on quests) are all persisted as NoteEdits . Amongst other fields of course, they have the following fields: synced and imagesNeedActivation.

synced is set to true after the upload of images plus the creation of the note (comment). If the NoteEdit does not contain images, imagesNeedActivation is false from the start. If it does contain images, it is true. The uploader will thus first upload the note proper, set synced to true. Then right after, also activate the image and set imagesNeedActivation to true. However, if that fails, the whole upload fails (before, an error in this step was ignored).

When the (next) upload is triggered, it will first check if there are any (leftover) note edits where synced is true but imagesNeedActivation is also true from the last upload attempt and will try to activate these first before uploading the new notes.

@westnordost
Copy link
Member

currently, the timeout (the time in which the uploaded image will reside in an inaccesible folder on the server before it is deleted because it is not activated) is set to 1 hour. Does it make sense to increase that timeout?

@matkoniecz
Copy link
Member

It is possible to be on a trip with half-working internet and and with losing images in notes (happened to me at least once), in such case it may be easily far more time than one hour.

Is timeout of 24h or 72h safe?

@mnalis
Copy link
Member

mnalis commented Mar 21, 2021

Yes, I'd also vote for at least 24h (or preferably 72h). Mobile net could be very flakey (or daily data limit kick in) and easily remain unreachable until user gets back to wifi (or to more populated cellular areas).

Only downside to longer timeout (if I understand it correctly) is that possible (doomed to be futile) attempts of abuse would use storage space on server for a longer period of time? So unless the server is critically low on disk space and such abuse attempts are rampant, increasing the timeout should not be a problem IMHO.

@peternewman
Copy link
Collaborator Author

Only downside to longer timeout (if I understand it correctly) is that possible (doomed to be futile) attempts of abuse would use storage space on server for a longer period of time? So unless the server is critically low on disk space and such abuse attempts are rampant, increasing the timeout should not be a problem IMHO.

I don't know what the stats are like and hence how much of an issue storage is, but like @mnalis I'd favour prioritising transient space for these entirely unseen images over ones in closed notes where someone has likely acted on them (or they can be re-opened if closed accidentally), e.g. dropping them from 7 to 6 days (for example, from memory) would presumably give a huge boost of space to cover these at risk images if there was a storage squeeze?

@mnalis
Copy link
Member

mnalis commented Apr 25, 2021

It happened to me again in StreetComplete v31.3 (via f-droid).
I opened note with picture during the day with mobile data (possibly flakey) and auto-upload enabled all the time: https://www.openstreetmap.org/note/2637554
but later that night opening note and following link to https://westnordost.de/p/38709.jpg gives "404 Not Found"

Luckily I was able to utilize (above mentioned) hack by @peternewman, and it worked:

% curl -d '{"osm_note_id": 2637554}'  https://westnordost.de/streetcomplete/photo-upload/activate.php
{"found_photos":1,"activated_photos":1}

Just wanted to check, that should be fixed in next major version? Or was it supposed to fixed even in 31.3 ?

@westnordost
Copy link
Member

no, in v32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants