Repository navigation
Keep pending uploads and server-side renames when an item's local ID is replaced - #503
Open
leifrossau wants to merge 2 commits into
Open
leifrossau wants to merge 2 commits into
leifrossau wants to merge 2 commits into
Conversation
When a new file is uploaded for the first time, the server can report it in a delta before the upload loop has noticed that the upload completed. applyDelta then finds the item by name and calls MoveID to replace its local ID with the server ID. MoveID removed the old ID with DeleteID, which also cancels the item's upload. If the file was written again while the first upload was running, the upload queued for that rewrite was dropped: the new content stayed local only and the server kept the old content, with nothing left to retry the upload. MoveID now moves a pending upload to the new ID instead of cancelling it. The upload loop is the only goroutine that touches its sessions, so MoveID asks it to move the session and waits for it, before the inode is moved. A session created from the inode after that already has the new ID and replaces the moved one as usual. A moved session targets the item by its ID, so it does not create a copy of the file if the item was renamed on the server in the meantime. The upload loop moves inodes itself when an upload produced a new ID. It cannot wait for itself, so it moves the session directly and uses the inode-only part of MoveID. This also means it no longer queues a cancellation for the old ID on every first upload of a file. UploadSession.Upload now reads the session ID once under the lock, since it can change while an upload is running. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If a new file was renamed or moved on the server after its first upload, but before the upload loop noticed that the upload completed, the delta for it could not be matched to the local item by name. applyDelta then added a second inode for the server ID with the new name and parent. When the upload loop moved the local item to the server ID, MoveID overwrote that inode with the local one, so the file kept its old name and parent locally, and the server ID was listed twice in the parent. When an inode already exists under the new ID, MoveID now applies its name and parent to the item being moved and removes it, instead of overwriting it. The item is removed from its old parent before it is added under the new ID, so it ends up listed once, in the right parent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Problem
When a new file is uploaded for the first time, the server can report it in a delta before the upload loop notices that the upload completed. Two things go wrong in that window:
applyDeltamatches the item by name and callsMoveID(localID, serverID), which usesDeleteID→CancelUpload. If the file was written again while the first upload was running, that second queued upload is dropped: the new content stays local only and the server keeps the old content.applyDeltaadds a second inode under the server ID; the upload loop's laterMoveIDoverwrites it with the local one, so the file keeps its old name and the server ID is listed twice in the parent.Fix
MoveIDmoves a pending upload to the new ID instead of cancelling it. Only the upload loop touches its sessions, soMoveIDsends a request on a newmoveQueueand waits for it before moving the inode. The session is re-keyed in memory and in bolt and from then on uploads by item ID; if a session for the new ID is already queued, it is newer and wins.DeleteIDis split intodeleteID(no upload cancel) andDeleteID;moveID, the inode-only part ofMoveID, usesdeleteID.UploadSession.Uploadreads the session ID once under the lock, since the ID can now change while an upload runs.moveIDapplies its name and parent to the moved inode and removes it instead of overwriting it.Testing
fs/move_id_offline_test.go:TestMoveIDKeepsPendingUploadandTestMoveIDMergesInodeAddedByDelta(both fail on the old code), andTestUploadCompletionMovesID(no deadlock in the upload loop's own path).go build ./...andgo vet ./...pass. I could not run the live-account suite.Notes for reviewers
MoveUploadblocks until the upload loop handles it and must not be called from the upload loop; callers (applyDelta,remoteID) hold no inode locks at that point.MovePath(renames) still cancels pending uploads throughDeleteID— same kind of bug; I'll send a small follow-up once this is in.🤖 Generated with Claude Code