fix(cache): make disk ranged reads revision-atomic#384
Conversation
worstell
force-pushed
the
worstell/disk-open-revision-atomic
branch
from
19b3e78 to
e0e3741
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0e3741dba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // TestDiskOpenRevisionAtomic asserts that Open never returns one revision's | ||
| // bytes paired with another revision's ETag while the same key is concurrently | ||
| // replaced (#348). The invariant checked mirrors production: the ETag returned | ||
| // with a body must equal the hash of that body. Run with -race for best | ||
| // coverage. |
There was a problem hiding this comment.
The local /workspace/cachew/AGENTS.md instructions say, “NEVER add comments to top-level tests.” This new doc block is attached directly to a top-level test function, so it violates the repository’s documented review rules; please remove it or encode the context in the test/helper names instead.
Useful? React with 👍 / 👎.
Disk.Open opened the file descriptor and read the ETag from BoltDB without synchronisation, while diskWriter.Close renamed the body then wrote metadata as two separate steps. A replacement interleaving those reads returned one revision's bytes with another's ETag. With ParallelGet this passes the chunk-0 consistency check yet splices old and new bytes across later chunks, so per-chunk ETag pinning did not actually bind the served body to the returned metadata. Guard each key's body swap with a striped RWMutex: Open captures the (descriptor, headers) pair under a read lock, and Close/Delete perform the rename/remove plus metadata write under the write lock. os.Open binds the descriptor to a stable inode, so the captured pair stays revision-consistent for the descriptor's lifetime. Closes #348 Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Elizabeth Worstell <worstell@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-019f3d90-8c5a-757c-87ff-308404f3b2bd
worstell
force-pushed
the
worstell/disk-open-revision-atomic
branch
from
e0e3741 to
2b6d686
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b6d6867bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // locks serialises each key's body swap against reads so Open captures a | ||
| // revision-consistent (file descriptor, headers) pair. Striped to keep | ||
| // cross-key contention negligible. Shared across namespaced copies (the | ||
| // slice header is copied, the mutexes are not). |
There was a problem hiding this comment.
The local /workspace/cachew/AGENTS.md says to “ONLY add comments for relatively large blocks of code, 20+ lines or more” and only when the code is not obvious. This new private-field comment is attached just to the locks field rather than a 20+ line block, and the same pattern appears in other newly added private-helper/short-section comments in this patch, so it violates the repository’s documented commenting rule; please remove these small-block comments or make the code self-explanatory without them.
Useful? React with 👍 / 👎.
1 participant
Disk.Openread the file descriptor and the ETag from BoltDB without synchronisation, whilediskWriter.Closerenamed the body then wrote metadata as two separate steps. A concurrent replacement interleaving those reads could return one revision's bytes with another's ETag. WithParallelGetthis passes the chunk-0 consistency check but splices old and new bytes across later chunks, so per-chunk ETag pinning did not actually bind the served body to the returned metadata.Guard each key's body swap with a striped
RWMutex:Opencaptures the(descriptor, headers)pair under a read lock, andClose/Deleteperform the rename/remove plus metadata write under the write lock.os.Openbinds the descriptor to a stable inode, so the captured pair stays revision-consistent for the descriptor's lifetime.Adds
TestDiskOpenRevisionAtomic, which asserts the returned ETag re-hashes to the returned body under concurrent replacement (fails onmain, passes with the fix).Closes #348