fix: fsync before acking durable writes so the server can't report content it never persisted - #148
Open
lehuan5062 wants to merge 6 commits into
Open
Conversation
The server's local packstore write path never called fsync, and the only periodic flush skipped fsync too, so a put could be acked as PayloadStoredDurable while the bytes existed only in an unsynced OS buffer. A crash in that window loses content that was reported durable. Adds a background durable-flush ticker (lore-storage) that fsyncs each group's packstore on a fixed interval and wakes writers waiting for a durable confirmation, bounded to ~2 ticks worst case with no per-write sleep. lore-server enables it at a 15ms interval; clients that only cache remote-durable content leave it disabled (default) at no cost. Also includes: leader_body no longer swallows upload errors before failing a commit, remote_put_retry retries Disconnected in addition to SlowDown, follower_future gained a require_durable check, and a new repository push-content/verify path to detect and repair content that is locally cached but not actually durable on the remote. Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com>
The previous commit added the repository push-content CLI command but missed its Reference doc entry and smoke-test coverage, both required by this repo's coding/doc standards. Regenerate docs/reference/lore-cli-commands.md via its documented `lore --markdown-help` command, which also picks up unrelated drift (--gc renamed to --no-gc, new --cache flag) already introduced by an earlier, separate commit. Add a repository_push_content wrapper to the test harness and smoke tests covering the no-op cases (scan with nothing pending, explicit address already durable). Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com>
Stable cargo fmt silently skips group_imports/imports_granularity since they are nightly-only, so a misplaced std::sync::Mutex import slipped through. Caught by cargo +nightly fmt --all --check. Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com>
docs/developing/code-standards/comments.md: write each comment about the thing it documents, not the specific consumers or use cases that call it. Several comments in the durability fix named `repository push-content` by name as the reason bytes stay recoverable, or described lore-server/client behavior baked into a generic settings doc; rephrase to describe the behavior itself. Also trims a comment that re-explained the FlushBarrier mechanism already documented on the type and setting, per "keep comments as short as possible." No behavior change. Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com>
node_path already returns an owned String, so mapping it through to_string() was a redundant Display round-trip; drop the map step entirely instead of just swapping to clone(). Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.com>
lore-cli-commands.md regeneration left trailing spaces on empty-description subcommand lines and an extra trailing blank line; CI's pre-commit hooks (trailing-whitespace, end-of-file-fixer) already computed the fix, applied verbatim here. Signed-off-by: Huân Lê-Vương <65440815+lehuan5062@users.noreply.github.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
A commit published a revision referencing content that existed nowhere: not on the remote, and not in the committing machine's local pack storage either, despite that machine's store index flagging the fragment
PayloadStoredDurable. This is real data loss, not a bookkeeping error.Root cause is on the server.
handle_putdoes awaitImmutableStore::putbefore acking, but the local packstore write path underneath it (PackStore::store) writes payload bytes with a plain positional write and never callsfsync. The only automatic flush,flush_delayedon a 5s timer, explicitly skips fsync as well; a realsync_data = trueflush happens only on graceful shutdown or GC. So a put is acked as durable while the bytes exist only in the page cache, the client discards its local copy on the strength of that ack, and a crash anywhere in that window loses content that was reported durable.A second, independent failure mode sits on the client:
leader_bodyswallowed upload errors, so an upload that failed or timed out still let the commit proceed to branch publish.Change
Sync before the ack, batched rather than per-write:
durable_flush_loop) fsyncs every group's packstore on a fixed interval and bumps that group'sFlushBarriergeneration. Astore()call writing fresh payload bytes under aPayloadStoredDurableclaim snapshots the generation and waits for the ticker to pass it, so there is no sleep and no spawned task in the write path itself.FlushBarrier::wait_pastwaits for the generation to advance by 2, not 1. A tick already in flight when the writer sets its dirty bit can complete without having observed it; ticks are sequential, so at most one can be non-covering and the next is guaranteed to sync the write. Waiting for +1 would reintroduce the bug on that interleaving.PackStore::flush_allis cheap when clean (onecompare_exchangeper packfile, no I/O), so the ticker walks all groups every tick without per-group pending-writer tracking.durable_flush_tick_mssetting.0disables the ticker entirely and is the default, so clients that only cache a remote-durable copy pay nothing; lore-server sets 15ms, bounding the worst-case unsynced window to roughly 30ms.Client write path, for the swallowed-error mode:
leader_bodynow returns the upload error instead of discarding it, after still writing the local entry non-durable with the payload cached as a safety net, so a failed upload fails the commit before branch publish.remote_put_retrybounded-retriesDisconnectedin addition toSlowDown, invalidating the session between attempts.follower_futuretakes arequire_durableflag so a follower whose own write is remote-coupled does not accept a local-only entry as success.Recovery for content that is still cached locally but was never confirmed durable:
lore repository push-contentcommand (explicit addresses, or--scanto sweep the local store), andrepository verifyextended to check content durability against the remote rather than structural integrity alone.Validation
cargo test -p lore-storage-> 169 passed, including two new regression tests:durable_put_waits_for_batched_fsync_before_returning(a durable write does not return until the ticker has confirmed it) andnon_durable_batch_window_skips_fsync_wait(tick interval 0 spawns no task and adds no wait).cargo check --workspace --all-targets-> clean.cargo clippy --all-targets -- -D warnings --no-deps-> clean;cargo +nightly fmt --all --check-> clean.Smoke tests:
scripts/test/test_push_content.py-> 2 passed;scripts/test/test_verify.py-> 11 passed as a regression check.docs/reference/lore-cli-commands.mdregenerated fromlore --markdown-help. That diff also picks up unrelated drift left by an earlier commit (--gcrenamed to--no-gc, new--cacheflag).Not covered, stated plainly: the original incident's address is unrecoverable and no test restores it. There is no crash-injection test that kills the server inside the unsynced window, so the regression tests assert the barrier ordering that closes the window rather than the crash itself.
AI assistance disclosure
I used an LLM (Claude Code) to help with implementation, code comments, and documentation on this change, and to help word things clearly as a non-native English speaker. I reviewed and tested all the code myself for its correctness, security, and license compliance.