djmitche
commented
9 years ago The race condition is this:
- user submits an upload with a novel digest
- user gets a PUT URL valid for 60s
- user uploads a valid file
- after 60s have elapsed, user submits a complete notification
check_pending_uploadbegins checking the file, since thetooltool_pending_uploadrow indicates the URL is expired- user immediately submits another upload, before the check is complete (a window of several seconds for a large file)
- the second upload returns a new PUT URL valid for another 60s
check_pending_uploadcompletes, adds the file to the database as valid, and deletes thetooltool_pending_uploadrow
This actually isn't a huge problem, since the client will also check the digest on download -- but it'd be nice to not have this issue.
One easy solution is for check_pending_upload to check the expires timestamp on the pending upload before deleting it -- if it's changed, it should abort the whole task (to be tried again). However, this just narrows the race window (and makes the issue more sensitive to DB replication delay and ACID compliance).
The better solution is probably to have uploads go to a different object, and then use the COPY operation to copy them to the destination file before validating. Then users can never write to a file while it is being validated.
Since this isn't a showstopper bug, I'd like to defer that fix to another pull req and get this one merged (after correcting any other review comments).
rail
This is a lot of commits, but the result is cohesive.
Blueprint
The blueprint handles tooltool files as a combination of "upload batches" and "files". Files have visibility levels, and are automatically replicated between regions. Upload batches have collections of files, an author, a message, and an upload date.
Downloads are handled by 302'ing to a signed S3 URL for the file in the preferred region or a random region. Uploads are only performed for files not already present, and are similarly performed via signed S3 URLs. Once the uploads are complete and the URL is expired, the blueprint verifies the upload (including checking the digest) and makes it available for download. It's careful to not verify an upload while there's an upload pending (actually, there's a race condition here now that I think about it.. I'll work on that)
UI
The UI lets users search for files and uploads. It can eventually grow an administrative interface to change file visibility, delete files, etc. (#8).
Client
The client is updated to handle the completely new upload system, and to handle token authentication for downloads. It should continue to work with the existing tooltool repositories, although I haven't tested this.