Skip to content

Don't block GetAttr/Lookup/ReadDirPlus while a file downloads in Open() - #500

Open
leifrossau wants to merge 1 commit into
jstaf:masterfrom
leifrossau:pr/open-without-inode-lock
Open

leifrossau wants to merge 1 commit into
jstaf:masterfrom
leifrossau:pr/open-without-inode-lock

Conversation

@leifrossau

Copy link
Copy Markdown

Problem

Filesystem.Open holds the inode's write lock for the whole hash and download. GetAttr, Lookup and ReadDirPlus on that inode block on the same lock, so a file manager freezes while one large file downloads.

Fix

  • Read the remote hash under a short RLock and update the size under a short Lock; never hold the inode lock while hashing or downloading.
  • Serialize concurrent Opens of the same ID with a per-ID download lock (a map of channels), so the same file isn't downloaded twice.
  • Before installing the download, re-check under the inode lock that the inode is still the one registered for the ID and has no local changes.
  • If the inode was replaced during the download (e.g. a delta racing a Lookup), retry with the current inode instead of returning success with an empty cache file; ENOENT if it is gone.

Testing

go build ./... and go vet ./... pass. The integration tests need a live account, so I haven't run them on this branch. The same change has been running in my fork, where an offline test suite against a mock Graph server covers the replaced-inode retry. Manual check: open a large file and run ls in the same directory while it downloads.

Notes for reviewers

  • A hash mismatch still re-downloads and replaces the cache file, as before.
  • A write during the download is protected by the post-download hasChanges check.

🤖 Generated with Claude Code

Open() held the inode's write lock across hashing and the whole network
download, so GetAttr, Lookup and ReadDirPlus on that inode blocked until the
download finished. File managers freeze while a large file is being opened.

The inode lock is now only taken briefly to read the remote hash and to
update the size. Concurrent Opens of the same file are serialized by a
per-ID download lock instead, so they still don't download the same file
twice. After the download the content is only installed if the inode is
still the one registered for that ID and has no local changes. If the inode
was replaced in the meantime (e.g. by a delta racing a Lookup), Open() retries
with the current inode, or returns ENOENT if the item is gone, rather than
returning success with an empty cache file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant