fix(buffers): flush the ledger when the disk_v2 writer rolls files#25768
fix(buffers): flush the ledger when the disk_v2 writer rolls files#25768graphcareful wants to merge 1 commit into
Conversation
Rolling to a new data file creates and fsyncs the file (and its directory entry) but only advances the writer file ID in the in-memory (mmap) ledger -- there was no ledger flush tied to that advance. A crash between creating the new file and the next periodic ledger flush could leave the file present on disk while the persisted ledger still pointed at the previous writer file. On restart that desync wedges the buffer: the writer, resuming on the old file, eventually tries to roll into the new one, finds it already exists and is non-empty, and blocks forever waiting for the reader to delete it, while the reader -- seeing equal reader/writer file IDs per the stale ledger -- waits on the writer instead of rolling forward. Neither side makes progress. Flush the ledger immediately after advancing the writer file ID, before any record is written into the new file. That maintains the invariant that no non-empty data file ever exists ahead of the durable ledger's writer file ID: the only state a crash can leave ahead of the ledger is a freshly-created empty file, which the roll logic already reclaims. Cost is one msync per data-file roll, i.e. once per max_data_file_size of throughput. No unit test accompanies this fix, and an attempted one was removed rather than kept: the ledger is an mmap-backed file, so an in-process reopen still observes the un-msync'd dirty pages -- "no flush" cannot be simulated without a real crash that drops those pages -- and there is no test hook to set the writer file ID to manufacture the lost-advance state (only record-ID setters exist). The attempted test therefore passed with and without the fix (verified by reverting the fix), making it a false guard. This behavior is validated by the crash-injecting Antithesis suite instead. Also adds a green reader-robustness regression test covering reopen with the reader head ahead of durable data. (cherry picked from commit 8f829c6)
graphcareful
force-pushed
the
fix/disk-v2-flush-ledger-on-roll
branch
from
395ca8e to
bda6459
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: bda6459c48
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // non-empty data file ever exists ahead of the durable writer file ID: the only | ||
| // state a crash can leave ahead of the ledger is an empty file, which the roll | ||
| // logic already reclaims. | ||
| self.ledger.flush()?; |
There was a problem hiding this comment.
When ledger.flush() returns an error here, this exits after self.writer has already been installed for the new data file and the in-memory writer file id has been advanced. If the caller retries the write after that I/O error, ensure_ready_for_write will see the writer is already open and proceed to write records without retrying the failed ledger flush, so a crash can again leave a non-empty data file ahead of the durable writer file id. Please leave the writer unopened, or otherwise force this flush to be retried, until the file-id advance is durably persisted.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes a durability gap in the
disk_v2buffer that could deadlock it after a crash during a data-file roll. When the writer rolled to a new data file it created and fsynced the file (and its directory entry) but only advanced the writer file ID in the in-memory (mmap) ledger, with no ledger flush tied to that advance. A crash between creating the new file and the next periodic ledger flush could leave the file present on disk while the persisted ledger still pointed at the previous writer file.On restart that desync deadlocked the buffer: the writer, resuming on the old file, would eventually try to roll into the new one, find it already exists and is non-empty, and block forever waiting for the reader to delete it, while the reader — seeing equal reader/writer file IDs per the stale ledger — waited on the writer. Neither side made progress.
This flushes the ledger immediately after advancing the writer file ID and before any record is written into the new file, preserving the invariant that no non-empty data file ever exists ahead of the durable ledger's writer file ID. Also adds a reader-robustness regression test covering reopen with the reader head ahead of durable data.
How did you test this PR?
cargo check -p vector-buffers --tests, plus the added regression test. The lost-advance crash window itself is validated by the crash-injecting Antithesis suite (the mmap ledger makes it impossible to simulate a dropped msync in-process).Change Type
Is this a breaking change?
Does this PR include user facing changes?