Repository navigation
Fix project save bugs that wedged locks, lost updates and stored "null" - #73
Merged
Merged
Conversation
The upload and update wrappers unlocked only after their handler returned, so a request that threw kept the project's lock forever and every later save to it hung until a restart. PUT /projects/:id with credits set to a string did that: it failed on .assert(array). Both wrappers now run their handler through runLocked, which releases the lock with defer on every exit path. The upload hands its lock to history preparation and back; that hand-back is deferred too, and the history lock it takes in between is released by defer, so the upload still owns exactly one lock when it unwinds. Request input that was asserted is now checked and refused with a 400: credits, tags and collaborators must be lists of objects, compatibility must be an object, and the asset check needs an assets list.
collectProjectExtensions runs before the upload checks the archive for project.json, and it opened the file outside its catch, so an sb3 without one threw a 500. It now answers the 400 "sb3 has no project.json" that uploadProjectFiles already uses, and a failed open of project.json is caught like a failed read.
PUT /projects/:id locked project-upload:<id> while uploads, remixes, pull merges, snapshots and history generation lock project-history-<id>, so an update and an upload could each load the project, and the one that saved last dropped the other's change. Updates, and the vanity URL lapse that edits project records, now take projectSaveLock(id), which is project-history-<id>. Nothing that runs under an update takes that lock again for the same project, and the only other project lock an update touches is a tryLock on the project holding a requested vanity URL, so the shared lock adds no wait that could deadlock. The smoke test renames a project while a large upload to it is in flight; the stored record must keep both changes.
Fields that were sent as null, or left out of an object, were read with
toStr() and became the word "null": {title: null} saved the title
"null", a credit without a role got the role "null", a product
without an icon was refused for its icon, an item without a visual got
the HTTPS URL error, a null tag was saved, a teammate without a name
was looked up as the user "null", and a null vanity URL or group tag
was treated as the slug or group "null".
They are now read as empty: a null title is refused as blank, null text
fields are cleared, a missing role, icon, description or grant is
empty, a missing price is 0, null tags and nameless teammates are
skipped, and a null vanity URL or group tag clears it. Valid requests
read the same values as before.
Chunks are stored from index 0, but assembleUploadPart and removeUploadPartChunks looped from 1, so completing a chunked part always failed with a 500 and its chunk files were left behind. Trying an unfinished part also deleted it: consumeStagedUploadPart treated the missing assembled file as expiry before checking whether the part was complete. workspaceToken and mergeTreeToken uploads could not work. Both loops now cover indexes 0 to count - 1, and an unfinished part is refused as incomplete and kept so it can still complete. The smoke test stages a workspace in two chunks and a merge tree in one, tries each before it is complete, completes them, and saves with the tokens; reusing the workspace token is refused.
storeProjectExtensions copied record.urls and record.projects with .assert(array), appended to the copies and saved the record without them, so extension records never listed a URL or a project. It now writes both lists back before saving. Records saved before this keep empty lists until the projects using them are saved again. Nothing depends on those lists for correctness: blocking an extension scans the project files, and gallery records keep their gallery flag, so the admin extension list is the only place that shows the gap.
The other failure paths after the history archive is stored remove the new workspace blob, but a failure to store extension sources only released the storage hold, so the blob stayed in storage with nothing pointing at it. That path now removes the new workspace layers too, keeping the ones the project already had.
The uploaded thumbnail was written, and the uploader's creating day recorded, before the project JSON was stored and the project record saved. When either of those failed the upload answered 500 but the project already showed the new thumbnail and the streak had counted it. Both now happen right after the project record is saved, before the share card is warmed and the response is built, so a saved upload answers exactly as before, thumbUrl included.
discoverUserIdentity checked for a 404 inside if resp.status == 0, so that branch never ran. A 404 fell through to the body, which has no id or username, and the lookup answered the user id "null": a team member or new owner who had deleted their Rotur account was saved as the user "null", and a transfer to them gave the project away. Only a 200 profile is now a found user. A 404 refreshes the identity cached for that username as the branch intended, and every other status is not found, as a failed request already was.
Updates now look up teammates, the new owner, plan benefits, group permissions, vanity holders and the storage hold on an unlocked copy of the project, then reload it under the save lock and apply the update with those answers. If the project changed so that it needs an answer that wasn't looked up, the update starts again, and gives up with a 409 after three tries. The response is built after the lock is released. Since updates share the save lock with uploads, history writers and the pending project.json flush on reads, a slow Rotur no longer holds those up while an update waits on it. A Rotur that can't be reached or answers with an error now gives a 503 for teammates and transfers, instead of saying the user wasn't found.
The vanity lapse loop and the check for a held vanity URL now take the project save lock with a deferred unlock, so an error can't leave another project's saves locked until restart.
Records stored before the URL and project lists were written back still have empty lists. Bumping the usage migration to version 2 runs it once more, which fills them in. Running it again changes nothing.
Moving a project into a group asked Rotur for group rights during the locked pass. It now goes through the recorded answers like the other lookups.
A 0, 408, 429 or 5xx from the profile lookup is a 503; any other answer, such as a 400 or 403 for a malformed or banned username, means the user wasn't found. Only a 404 refreshes the stored identity.
Each update try now releases its storage holds in a deferred block, so an error can't leave them reserved until they expire. A saved transfer still keeps the hold it settles. Teammate and owner lookups that got a real answer carry over to the next try, so a busy project doesn't ask Rotur for them again.
Views never waited for the save lock, so the step now times a second update and an upload of the same project instead. It also covers a banned username.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These save-path bugs were found while splitting the upload and update handlers (#72). The worst ones could wedge a project or silently lose data in production.
PUT /v1/projects/:idwith{"credits": "x"}, returned 500 and left that project's lock held. Every later save hung until the server restarted. Both handlers now run throughrunLocked, which unlocks withdefer. Malformed credits, tags, collaborators, compatibility and asset-check bodies are 400s.project-upload:<id>vsproject-history-<id>), so a rename during an upload could be lost. They now shareprojectSaveLock(id).From review:
rotur_unavailablewhen Rotur is unreachable, times out, rate-limits or returns 5xx. A 400, 403 or 404 means not found.Verification
osl testpasses 56, with new tests for the lock wrapper, upload parts, extensions, identity lookups, update lookups, and the policy and team rules.There is no visible change.