fix(sf-335): keep Batched as production WAL fsync default + reconcile durability docs - #102
Merged
Conversation
…urability doc-comments + boot warn + reproducible measurement - Decision (evidence-gated, /xask-validated): keep WalFsyncPolicy::Batched as the production default. A direct WalWriter::append microbenchmark shows PerOp costs ~40-45x single-partition append throughput and adds ~5ms p50 latency (macOS F_FULLFSYNC full device barrier) vs Batched — too high to impose by default on a single-node demo-tier server. PerOp stays one env var away (TOPGUN_WAL_FSYNC_POLICY=per_op) for acked-implies-durable. - Rewrite write_behind.rs doc-comment: the old 'Do NOT change to PerOp' text misframed PerOp as risky when Batched is the non-durable one under kill -9. - Rewrite WalFsyncPolicy enum + variant doc-comments: drop 'good balance of safety' weasel wording; state the kill -9 window truthfully per variant. - Add a startup warn! when the Batched default is active on a durable backend so the crash-loss caveat is impossible to miss at boot. - Add an #[ignore]-d measure_fsync_policy_append_cost bench test making the PerOp-vs-Batched-vs-None measurement reproducible on demand.
… claims with the Batched default - CLAUDE.md Production Defaults: add TOPGUN_WAL_FSYNC_POLICY row + a 'what the default guarantees under kill -9' paragraph (Batched is NOT acked==durable; per_op is, at ~40x throughput cost on macOS — Linux penalty smaller, not yet measured). Note the boot warn + startup-log field. Keep the write-behind ~1s flush note (TODO-339) as a distinct layer. - deploy/performance.mdx: add the env-var row + surface the per_op escape hatch in the durability note (audit found zero user-facing overclaims to retract; this adds the how-to-get-durability guidance).
…rement assert
- Broaden the boot warn to also fire (more strongly) when TOPGUN_WAL_FSYNC_POLICY=none
on a durable backend: None never fsyncs, so acked writes are not crash-safe at all —
strictly worse than the Batched default it previously warned about, yet was silent.
PerOp stays silent (acked == durable). (xreview MED)
- Measurement test: assert all policies measured >0 ops before computing the ratio
(no div-by-zero/infx), and tighten the sanity floor from 'perop <= none*1.5' to
'perop <= none' so a silently-skipped PerOp fsync (PerOp >= None) trips it. (xreview LOW x2)
Rejected (verified against source): xreview HIGH (env parse unwired) — parse site
write_behind.rs:217-231 exists + tested :2416-2452 (SPEC-330); xreview MED (!Null != durable)
— StorageBackend is {Null,Redb,Postgres}, both non-Null variants are durable, matches the
existing :840/:842 idiom.
… (append≠fsync-durable)
Deploying topgun with
|
| Latest commit: |
dd8af7c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f9d02da8.topgun-f45.pages.dev |
| Branch Preview URL: | https://fix-sf-335-wal-fsync-default.topgun-f45.pages.dev |
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
1 participant
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.
Summary
Makes the production-default WAL fsync-policy call and closes the durability claim/behavior gap (HN-blocker, from TODO-554).
Decision (evidence-gated,
/xask-validated): KEEPWalFsyncPolicy::Batchedas the demo-tier default. The durability chain (SPEC-331/332/333a/b) was proven underPerOp, but the shipped default isBatched— which acks a write after appending its WAL frame and fsyncs only on a ~10 ms group-commit timer (or 100-frame flush). Akill -9inside that window drops acked-but-unfsynced writes. Rather than pay a ~40× throughput cost by default, we keepBatchedand make its non-durability truthful and loud everywhere, withTOPGUN_WAL_FSYNC_POLICY=per_opas the acked-implies-durable opt-out.Changes
wal/mod.rs—WalFsyncPolicyenum + per-variant doc-comments rewritten to state exactkill -9behavior (dropped "good balance of safety" weasel wording); added an#[ignore]d reproduciblemeasure_fsync_policy_append_costmicrobenchmark.write_behind.rs—wal_fsync_policyfield doc rewritten (dropped the backwards "Do NOT change toPerOp"; states the non-durable window +per_opopt-out). Default value unchanged.topgun_server.rs— bootwarn!for the non-durable defaults (Batched,None) on a durable (non-Null) backend, so the crash-durability caveat is visible at startup. Startup log already surfaceswal_fsync_policy.CLAUDE.md— Production Defaults documentsTOPGUN_WAL_FSYNC_POLICY+ a "WAL fsync durability" section stating the exactkill -9guarantee; de-overpromised the acked-write note.performance.mdx— env-var row + reconciled durability note (surfaces theper_opescape hatch).baseline.jsonuntouched (default stayedBatched; no threshold flip).Measurement
Direct
WalWriter::appendmicrobenchmark (the load-harness runs onNullDataStore— no WAL — so it cannot measure the fsync delta):Batched ≈ 40× PerOp throughput — but this is a macOS
F_FULLFSYNCartifact; on Linuxfdatasync/NVMe the penalty is materially smaller (unmeasured — measure before quoting publicly).Verification
cargo build --release -p topgun-server✓ ·cargo clippy --all-targets --all-features -p topgun-server -- -D warnings= 0 ✓ ·cargo fmt --check✓ ·cargo test --release -p topgun-server --lib= 1529 passed / 0 failed / 1 ignored ✓pnpm -r build·pnpm lint·pnpm format:check·pnpm --filter apps-docs-astro build✓/xask(keep-Batched concurred) +/xreview(execution + review-gate passes; all findings reconciled).Scope fence
Does NOT re-implement SPEC-330 env/parse/fatal plumbing; does NOT touch TODO-339 shutdown-drain; does NOT change the Batched fsync mechanism (10 ms timer / 100-frame threshold). 3 Rust source files ≤ 5-file cap.