perf(persistence): local-leg group commit + BITOP/COPY/DEL/UNLINK coordinator leg durability - #213
Merged
Merged
Conversation
…commit under appendfsync=always The cross-shard coordinator's LOCAL-leg persist (co-located MSET/MSETNX and scattered-MSET local slices, shipped in v3-4 Finding 1) called try_send_append_durable, which under appendfsync=always awaits one fsync ack PER COMMAND bounded by --aof-fsync-timeout-ms (default 2000ms). A pipeline of coordinated writes stacked these serially — the measured 2000-3000ms always far-tail (tmp/V3-4-GCLOUD-BENCH.md), carried as the [HIGH] follow-up into v3-5. Fix: local legs now use the same fire-and-forget-then-barrier contract the remote SPSC legs have used since the H1-BARRIER fix: - New AofWriterPool::send_append_group enqueues under bounded backpressure and returns immediately; Ok(true) means Always policy — the caller owes a barrier. EverySec/No are unchanged (writer loop owns the fsync cadence). - persist_local_leg switches to it and reports needs-barrier up through coordinate_mset/coordinate_msetnx/coordinate_multi_key. - Both connection handlers (monoio + sharded) collect the response indexes of barrier-pending local-leg writes and issue ONE fsync_barrier(local shard) per pipeline batch, BEFORE response serialization — +OK still implies confirmed durability, but a batch of N coordinated writes costs 1 awaited fsync instead of N. - On barrier failure every affected response is overwritten with AOF_FSYNC_ERR — never a false +OK (design-for-failure preserved). Tests: 3 new red-proven pool unit tests (send_append_group must not await the per-write ack; everysec needs no barrier; dead writer errors); v3-4 coordinator_local_leg_durability crash-recovery suite green on the new path (local legs still persist and replay); full lib suite 3656 pass; clippy clean on default + tokio,jemalloc. Absolute tail magnitude needs a GCE run (OrbStack fsync is near-free); tracked in v3-5. author: Tin Dang
The cross-shard coordinator's in-process legs for BITOP (dest write),
COPY (dst write + PEXPIRE TTL restore), and multi-key DEL/UNLINK (both
the co-located fast path and the scattered local slice) executed in
memory but never appended to the owning shard's AOF — the carried v3-4
follow-up. Failure modes on kill-9 + restart:
- DEL/UNLINK: deleted keys RESURRECTED from their seed writes (the seed
MSET is in the AOF, the local-leg delete never was).
- BITOP/COPY: results whose dest/dst owner == the connection's own shard
silently vanished. Remote legs were always durable (MultiExecute ->
wal_append_and_fanout).
Fix: all four now persist through the same persist_local_leg
group-commit path as MSET/MSETNX (previous commit):
- New run_on_owner_persist mirrors run_on_owner but appends the command
to my_shard's AOF when the owner is local and execution succeeded —
used by BITOP's whole-command forward + synthesized DEL/SET dest legs
and COPY's whole-command forward + SET/PEXPIRE dst legs. Replay-safe:
each persisted command covers only keys owned by my_shard.
- coordinate_multi_del_or_exists persists DEL/UNLINK on the co-located
fast path (whole command) and the scattered local slice (synthesized
over only local keys), skipped when n=0 (nothing removed replays
identically without a record). EXISTS/TOUCH stay read-only.
- All legs ride the batch-end fsync barrier under appendfsync=always
via local_barrier_pending (previous commit's plumbing).
Tests: 4 new red-proven crash-recovery tests in
coordinator_local_leg_durability.rs (DEL scatter resurrection, UNLINK
co-located fast-path resurrection, BITOP dest legs both shapes, COPY dst
legs both shapes) — deterministic via per-shard {hash-tags} regardless
of which shard the connection lands on; suite 7/7 green post-fix. Full
lib suite 3656 pass; clippy clean both feature sets.
author: Tin Dang
…s sleep The cluster_* tests spawned an in-process cluster server, slept a fixed 100ms, then connected with a no-retry unwrap (integration.rs:242). Under full-suite parallelism the listener thread can take longer to bind -> "Connection refused" flakes (observed repeatedly in CI-parity runs; pass in isolation). Replace the sleep with a bounded connect-poll (10s), the same pattern the binary-spawning suites use. author: Tin Dang
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds group-commit durability for coordinator-local write legs, threads barrier tracking through multi-key coordination and connection handling, and adds restart regression coverage plus changelog and startup readiness updates. ChangesCoordinator local-leg durability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Coordinator
participant AofWriterPool
participant Shared
Client->>Coordinator: multi-key write
Coordinator->>AofWriterPool: enqueue local leg append
Coordinator->>Shared: resolve_local_leg_barrier
Shared->>AofWriterPool: fsync_barrier(shard_id)
Shared-->>Coordinator: update responses or clear pending indexes
Coordinator-->>Client: send batch responses
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoCoordinator local-leg group commit + BITOP/COPY/DEL/UNLINK AOF durability
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
40 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shard/coordinator.rs (1)
1192-1266: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftLocal-leg persist failure aborts before remaining shard groups are dispatched.
In the scatter branch,
persist_local_leg(...).await(and its earlyreturnonErr(())) runs inside thefor (shard_id, key_args) in &groupsloop, before all groups have been dispatched and beforepending_shardsis awaited. Ifmy_shard's slice happens before a higher-shard-id group in the BTreeMap ordering and the local AOF enqueue fails, the function returns immediately — the remaining (not-yet-iterated) shard groups never get theirspsc_sendDEL/UNLINK dispatched at all, whilemy_shard's own keys were already deleted in memory and any earlier (lower-shard-id) groups were already fire-and-forget dispatched. The client gets a singleAOF_FSYNC_ERR, but the actual keyspace ends up partially deleted across shards with no way to tell which keys were affected.
coordinate_msetavoids this by deferring its local-leg persist call until after the full group loop and thepending_shardsawait loop.coordinate_multi_del_or_existsshould follow the same pattern: capture the synthesized local DEL/UNLINK parts during the loop, dispatch every group unconditionally, await allpending_shards, and only then attemptpersist_local_leg(returning the error afterward if it fails).🔧 Proposed restructuring
let mut total_count: i64 = 0; let mut pending_shards: Vec<channel::OneshotReceiver<Vec<Frame>>> = Vec::new(); + let mut local_delete_parts: Option<Vec<Frame>> = None; for (shard_id, key_args) in &groups { if *shard_id == my_shard { let mut selected = db_index; let result = crate::shard::slice::with_shard_db(db_index, |db| { db.refresh_now_from_cache(cached_clock); cmd_dispatch(db, cmd, key_args, &mut selected, db_count) }); if let DispatchResult::Response(Frame::Integer(n)) = result { total_count += n; if is_delete && n > 0 { let mut parts: Vec<Frame> = Vec::with_capacity(key_args.len() + 1); parts.push(Frame::BulkString(Bytes::from(cmd_upper.clone()))); parts.extend_from_slice(key_args); - let serialized = - crate::persistence::aof::serialize_command(&Frame::Array(parts.into())); - match persist_local_leg(aof_pool, repl_state, my_shard, serialized).await { - Ok(needs_barrier) => *local_barrier_pending |= needs_barrier, - Err(()) => { - return Frame::Error(Bytes::from_static( - crate::persistence::aof::AOF_FSYNC_ERR, - )); - } - } + local_delete_parts = Some(parts); } } } else { ... spsc_send(dispatch_tx, my_shard, *shard_id, msg, spsc_notifiers).await; pending_shards.push(reply_rx); } } for reply_rx in pending_shards { match reply_rx.recv().await { ... } } + if let Some(parts) = local_delete_parts { + let serialized = crate::persistence::aof::serialize_command(&Frame::Array(parts.into())); + match persist_local_leg(aof_pool, repl_state, my_shard, serialized).await { + Ok(needs_barrier) => *local_barrier_pending |= needs_barrier, + Err(()) => { + return Frame::Error(Bytes::from_static(crate::persistence::aof::AOF_FSYNC_ERR)); + } + } + } + Frame::Integer(total_count)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shard/coordinator.rs` around lines 1192 - 1266, The scatter path in coordinate_multi_del_or_exists is returning from persist_local_leg too early inside the shard loop, which can prevent later shard groups from being dispatched. Refactor the loop to always finish collecting/disptaching all groups and awaiting pending_shards first, while capturing the local DEL/UNLINK payload for my_shard during the loop. Then perform the persist_local_leg call after the loop (as coordinate_mset does), and only return AOF_FSYNC_ERR afterward if that persist fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/conn/handler_monoio/mod.rs`:
- Around line 1816-1831: Move the local-leg fsync barrier in
handler_monoio::mod::handle so it runs before any early-return PSYNC handling
and before the blocking-command flush/break path, not only at end-of-batch.
Ensure the same barrier logic that currently drains local_leg_write_idxs and
uses ctx.aof_pool.fsync_barrier(ctx.shard_id) is executed for MSET/MSETNX local
legs before those exits can skip it, so acknowledgements are only serialized
after durability is confirmed.
In `@src/server/conn/handler_sharded/mod.rs`:
- Around line 1719-1735: Batch-end durability barrier is using stale
local_leg_write_idxs after the blocking-command early flush path replaces
responses. In handler_sharded::mod.rs, make sure any pending AOF fsync barrier
for local write legs is resolved before the blocking-commands branch flushes
responses and resets the vector, then clear local_leg_write_idxs so later code
cannot index into the new buffer. Use the existing fsync_barrier logic around
the responses/local_leg_write_idxs handling in the per-frame loop, especially
the blocking-command branch and the final barrier block near the end of batch
processing.
---
Outside diff comments:
In `@src/shard/coordinator.rs`:
- Around line 1192-1266: The scatter path in coordinate_multi_del_or_exists is
returning from persist_local_leg too early inside the shard loop, which can
prevent later shard groups from being dispatched. Refactor the loop to always
finish collecting/disptaching all groups and awaiting pending_shards first,
while capturing the local DEL/UNLINK payload for my_shard during the loop. Then
perform the persist_local_leg call after the loop (as coordinate_mset does), and
only return AOF_FSYNC_ERR afterward if that persist fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4af9ed81-0668-4713-a812-f3d9cb558f92
📒 Files selected for processing (8)
CHANGELOG.mdsrc/persistence/aof/pool.rssrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/shard/coordinator.rstests/coordinator_local_leg_durability.rstests/integration.rs
…shes (PR #213 review) Review findings from PR #213 (CodeRabbit + Qodo), all addressed: 1. [CRITICAL, CodeRabbit] The batch-end local-leg barrier could be skipped or corrupted by mid-batch early flushes: - blocking commands (BLPOP...) flush accumulated responses then REPLACE the vec — pending barrier indexes became stale (panic or misattributed AOF_FSYNC_ERR onto the blocking response) and the flushed +OK escaped without confirmed durability; - PSYNC hijack (monoio) and SUBSCRIBE-entry (both runtimes) flush early with the same durability leak. Fix: new shared::resolve_local_leg_barrier(pool, shard, idxs, responses) — always drains, patches AOF_FSYNC_ERR on barrier failure — called at the batch end AND before every early flush (blocking, SUBSCRIBE entry, PSYNC) in both handlers. 2. [Qodo bug] run_on_owner_persist logged no-op writes (COPY SET..NX refusal, DEL of absent dest, PEXPIRE on vanished key) — costing a needless barrier fsync and risking AOF_FSYNC_ERR on a command that wrote nothing. Fix: persist_if predicate per call site (BITOP forward always mutates; DEL only n>0; COPY only :1; SET dst only +OK; PEXPIRE only :1). 3. [Qodo rule] #[allow(clippy::too_many_arguments)] now carries its justification; CHANGELOG latency numbers now state their Linux/GCE measurement context. Tests: coordinator_local_leg_durability 7/7, full lib suite 3656 pass, clippy clean both feature sets. The barrier-failure arm of the early flush paths is not black-box testable (requires an AOF writer dying mid-pipeline); covered by the shared helper's single code path + the existing fsync_barrier unit tests. author: Tin Dang
…commit # Conflicts: # CHANGELOG.md
4 tasks
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 7, 2026
…mmit (#239) Under `appendfsync always`, all three connection handlers awaited one fsync ack PER COMMAND for plain local writes (try_send_append_durable). A 16-deep pipeline therefore paid 16 serialized fsync round-trips per connection, while Redis fsyncs once per event-loop iteration — measured as an 8x SET deficit at P16 (Moon 5.2k vs Redis 44k ops/s, GCE c3-standard-8; tmp/MOON-VS-REDIS-DURABILITY.md). The writer-side group commit (one fsync per drained batch) already existed, and cross-shard writes + coordinator local legs already used the fire-and-forget + barrier contract from PR #213. This change extends that contract to plain local writes: - handler_monoio / handler_sharded: local writes, MOVE, and COPY enqueue via send_append_group(); successful responses join local_leg_write_idxs and the existing end-of-batch resolve_local_leg_barrier() (one fsync_barrier per pipelined batch) converts them to AOF_FSYNC_ERR on failure — never a silent +OK. - handler_single: flush_with_aof_ack collects barrier indexes and issues ONE fsync_barrier(0) for the whole flush; the inline pre-SUBSCRIBE path and single-shard GRAPH.* WAL-record loop use the same pattern. - The AOF writer processes its channel in order, so an acked zero-length AppendSync barrier proves every prior Append durable: the H1 fsync-before-ack guarantee is unchanged. - everysec/no policies unaffected (send_append_group returns Ok(false), no barrier joined). Validation: - crash_matrix_per_shard_aof --ignored: 3/3 green (SIGKILL under appendfsync always still recovers 100% of acked writes). - flush_with_aof_ack H1 ordering test updated to the batch protocol (Append then AppendSync barrier; ack still gates the response). - fmt, clippy x2 (default + tokio,jemalloc), cargo test --lib x2 (monoio 3864 / tokio 3143), cargo test --no-run x2: all green. author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
TinDang97
added a commit
that referenced
this pull request
Sep 3, 2026
…sing Executes all three steps of moon#660. A decision taken 2026-07-10 to flip `--disk-offload` from default-on to opt-in was recorded and never carried out; #660 filed that gap. `--disk-offload` now defaults to `disable`. Nothing about the tier changed and nothing is deprecated. `--disk-offload enable` turns it on, existing offload files are left untouched, and they are picked up again when it is re-enabled. `value_parser` now restricts the flag to `enable`/`disable`: only the exact string `enable` turns the tier on, so a typo used to mean "silently off" and would now mean "silently without the tier you upgraded specifically to keep". The reason is not a double-write conflict with the WAL -- spilled segments are independently self-durable and recover on their own. It is RECONCILIATION. Recovery runs Phase 3 (rebuild cold_index from the manifest) then Phase 4 (WAL replay on top, hot shadowing cold), and every bug found in that seam so far has been silent-data-loss class: DEL/FLUSH resurrection and expired-cold leak (#212), BITOP/COPY/DEL/UNLINK resurrection (#213), a spill completion resurrecting a DEL'd key (#459). Each was caught by soak or adversarial review, none by a proof. Step 2 supplies the proof. `tests/cold_reconciliation_property_660.rs` drives seeded random SET/SET PX/DEL/UNLINK/COPY/FLUSHDB sequences over a deliberately small keyspace under real memory pressure, then checks every key the sequence ever touched against a model -- live, and again after SIGKILL and a full Phase-3/Phase-4 recovery. The three failure shapes are named separately because all three have shipped: resurrection, expired-cold leak, lost/stale write. It refuses a vacuous pass, prints the seed, and MOON_660_SEEDS=<n> replays one case. No proptest dependency: a durability default is not the place to also widen the supply chain. Mutation-proved -- gutting `Database::remove_cold_only` reddens it with "RESURRECTION ... prop:key:004 ... v4-19-xxxx", on SEED 5 rather than seed 1. That is recorded in the file: whether a sequence happens to delete a key while it is cold is what the allkeys-lru victim choice decides, so shrinking the seed sweep to save wall-clock would quietly cost most of the file's power. Three of its own bugs were found by measurement while building it, each fixed at the cause rather than tuned around: the non-vacuity guard fired correctly when 4.9 MiB of filler never crossed an 8 MiB cap; raising it to 16 MiB produced -OOM, whose fix is that a REFUSED write must not be applied to the model (`accepted`), so correctness no longer depends on picking a filler size that never trips the cap; and a COPY mismatch that looked like the #610 cold-tier class replayed ALONE and passed, making it a race between the model's clock and the server's on a 300 ms TTL -- the prediction is now asserted only when neither key is volatile, which keeps the cold-source case that would actually catch #610. The flip's blast radius, measured by running all 268 test binaries against it: 264 green, and the one substantive red was `vector_db_isolation`. `vector_persist_dir_for` resolves the index-metadata directory to the disk-offload dir when the tier is on and to `persistence_dir` otherwise, and `persistence_dir` is None under `--appendonly no` with no `--save`. So a server with NO durability configured still persisted its FT.* index definitions, purely because the tier defaulted on. Measured on one binary with the flag explicit on both sides of a restart: --disk-offload disable --appendonly yes -> index survives --disk-offload enable --appendonly no -> index survives --disk-offload disable --appendonly no -> index LOST Only the third row changes, and it is the row where the operator asked for no durability at all, so the new behaviour is the consistent one. It was simply invisible before. That suite now pins `--disk-offload enable` explicitly -- preserving the environment it was written against rather than quietly re-pointing it at a different persistence path -- and a new `ft_index_survives_restart_without_disk_offload` pins the row an upgrading deployment actually lands on (tier off, AOF on), which nothing covered. Mutation-proved: make the non-offload arm of `vector_persist_dir_for` return None and it reddens. Docs carry both operator impacts: the memory one (a server that relied on the default now holds its keyspace in RAM and evicts, or answers -OOM under noeviction, where it previously spilled) and the FT one. BREAKING CHANGE: `--disk-offload` now defaults to `disable`. Pass `--disk-offload enable` to keep the previous behaviour. Refs: #660, #212, #213, #459 author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 4, 2026
…sing Executes all three steps of moon#660. A decision taken 2026-07-10 to flip `--disk-offload` from default-on to opt-in was recorded and never carried out; #660 filed that gap. `--disk-offload` now defaults to `disable`. Nothing about the tier changed and nothing is deprecated. `--disk-offload enable` turns it on, existing offload files are left untouched, and they are picked up again when it is re-enabled. `value_parser` now restricts the flag to `enable`/`disable`: only the exact string `enable` turns the tier on, so a typo used to mean "silently off" and would now mean "silently without the tier you upgraded specifically to keep". The reason is not a double-write conflict with the WAL -- spilled segments are independently self-durable and recover on their own. It is RECONCILIATION. Recovery runs Phase 3 (rebuild cold_index from the manifest) then Phase 4 (WAL replay on top, hot shadowing cold), and every bug found in that seam so far has been silent-data-loss class: DEL/FLUSH resurrection and expired-cold leak (#212), BITOP/COPY/DEL/UNLINK resurrection (#213), a spill completion resurrecting a DEL'd key (#459). Each was caught by soak or adversarial review, none by a proof. Step 2 supplies the proof. `tests/cold_reconciliation_property_660.rs` drives seeded random SET/SET PX/DEL/UNLINK/COPY/FLUSHDB sequences over a deliberately small keyspace under real memory pressure, then checks every key the sequence ever touched against a model -- live, and again after SIGKILL and a full Phase-3/Phase-4 recovery. The three failure shapes are named separately because all three have shipped: resurrection, expired-cold leak, lost/stale write. It refuses a vacuous pass, prints the seed, and MOON_660_SEEDS=<n> replays one case. No proptest dependency: a durability default is not the place to also widen the supply chain. Mutation-proved -- gutting `Database::remove_cold_only` reddens it with "RESURRECTION ... prop:key:004 ... v4-19-xxxx", on SEED 5 rather than seed 1. That is recorded in the file: whether a sequence happens to delete a key while it is cold is what the allkeys-lru victim choice decides, so shrinking the seed sweep to save wall-clock would quietly cost most of the file's power. Three of its own bugs were found by measurement while building it, each fixed at the cause rather than tuned around: the non-vacuity guard fired correctly when 4.9 MiB of filler never crossed an 8 MiB cap; raising it to 16 MiB produced -OOM, whose fix is that a REFUSED write must not be applied to the model (`accepted`), so correctness no longer depends on picking a filler size that never trips the cap; and a COPY mismatch that looked like the #610 cold-tier class replayed ALONE and passed, making it a race between the model's clock and the server's on a 300 ms TTL -- the prediction is now asserted only when neither key is volatile, which keeps the cold-source case that would actually catch #610. The flip's blast radius, measured by running all 268 test binaries against it: 264 green, and the one substantive red was `vector_db_isolation`. `vector_persist_dir_for` resolves the index-metadata directory to the disk-offload dir when the tier is on and to `persistence_dir` otherwise, and `persistence_dir` is None under `--appendonly no` with no `--save`. So a server with NO durability configured still persisted its FT.* index definitions, purely because the tier defaulted on. Measured on one binary with the flag explicit on both sides of a restart: --disk-offload disable --appendonly yes -> index survives --disk-offload enable --appendonly no -> index survives --disk-offload disable --appendonly no -> index LOST Only the third row changes, and it is the row where the operator asked for no durability at all, so the new behaviour is the consistent one. It was simply invisible before. That suite now pins `--disk-offload enable` explicitly -- preserving the environment it was written against rather than quietly re-pointing it at a different persistence path -- and a new `ft_index_survives_restart_without_disk_offload` pins the row an upgrading deployment actually lands on (tier off, AOF on), which nothing covered. Mutation-proved: make the non-offload arm of `vector_persist_dir_for` return None and it reddens. Docs carry both operator impacts: the memory one (a server that relied on the default now holds its keyspace in RAM and evicts, or answers -OOM under noeviction, where it previously spilled) and the FT one. BREAKING CHANGE: `--disk-offload` now defaults to `disable`. Pass `--disk-offload enable` to keep the previous behaviour. Refs: #660, #212, #213, #459 author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 4, 2026
…-offload Splits moon#660 in two and lands only the half that is safe on its own. WHAT LANDS `tests/cold_reconciliation_property_660.rs` — the proof #660 records as the one piece of work worth doing regardless of what happens to the default. Disk offload is a two-source-of-truth durability path, and the hazard is RECONCILIATION, not a double-write conflict with the WAL: recovery runs Phase 3 (rebuild `cold_index` from the manifest) then Phase 4 (WAL replay on top, hot shadowing cold). Every bug found in that seam so far has been silent-data-loss class (#212, #213, #459), and every one was caught by soak or adversarial review — never by a proof that the invariant holds in general. A seeded generator drives writes, deletes and expiries under real memory pressure and asserts the server's answer for every key matches a model, both live and after SIGKILL + full recovery. Failures are named by shape (RESURRECTION, EXPIRED-COLD LEAK, LOST WRITE); `MOON_660_SEEDS` replays any seed. It earned its keep immediately — it is what surfaced the COPY/BITOP single-shard durability bug as a deterministic 3-of-3 CI failure instead of a soak-only ghost. `--disk-offload` also gains `value_parser = ["enable", "disable"]`. Only the exact string `enable` ever turned the tier on, so `--disk-offload enabled` silently meant "without the tier". WHAT IS HELD BACK, AND WHY The default flip to `disable` is NOT here. It is a breaking change with a SILENT failure mode: nothing detects existing offload state when the tier is off. `disk_offload_base` is None, the v3 recovery branch is skipped entirely, `cold_index` is never built, and the operator gets no warning, no error and no INFO field — just a smaller keyspace. `docs/versioning.md` documents upgrade as "stop, swap the binary, start", which against that change is a silent keyspace shrink. The flip needs a startup check that REFUSES to start when offload state exists with the tier off, five doc updates, and a runbook before it can ship. Held on its own branch and tracked; nothing about that work is blocked by this commit, and this commit is what makes the flip provable when it comes. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 4, 2026
…-offload Splits moon#660 in two and lands only the half that is safe on its own. WHAT LANDS `tests/cold_reconciliation_property_660.rs` — the proof #660 records as the one piece of work worth doing regardless of what happens to the default. Disk offload is a two-source-of-truth durability path, and the hazard is RECONCILIATION, not a double-write conflict with the WAL: recovery runs Phase 3 (rebuild `cold_index` from the manifest) then Phase 4 (WAL replay on top, hot shadowing cold). Every bug found in that seam so far has been silent-data-loss class (#212, #213, #459), and every one was caught by soak or adversarial review — never by a proof that the invariant holds in general. A seeded generator drives writes, deletes and expiries under real memory pressure and asserts the server's answer for every key matches a model, both live and after SIGKILL + full recovery. Failures are named by shape (RESURRECTION, EXPIRED-COLD LEAK, LOST WRITE); `MOON_660_SEEDS` replays any seed. It earned its keep immediately — it is what surfaced the COPY/BITOP single-shard durability bug as a deterministic 3-of-3 CI failure instead of a soak-only ghost. `--disk-offload` also gains `value_parser = ["enable", "disable"]`. Only the exact string `enable` ever turned the tier on, so `--disk-offload enabled` silently meant "without the tier". WHAT IS HELD BACK, AND WHY The default flip to `disable` is NOT here. It is a breaking change with a SILENT failure mode: nothing detects existing offload state when the tier is off. `disk_offload_base` is None, the v3 recovery branch is skipped entirely, `cold_index` is never built, and the operator gets no warning, no error and no INFO field — just a smaller keyspace. `docs/versioning.md` documents upgrade as "stop, swap the binary, start", which against that change is a silent keyspace shrink. The flip needs a startup check that REFUSES to start when offload state exists with the tier off, five doc updates, and a runbook before it can ship. Held on its own branch and tracked; nothing about that work is blocked by this commit, and this commit is what makes the flip provable when it comes. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 4, 2026
…enable (#812) * perf(server): let inline writes run under the default --disk-offload enable `can_inline_writes` carried the term `ctx.spill_sender.is_none()`. `--disk-offload` defaults to `enable`, which spawns a per-shard `SpillThread` and hands every connection a live sender — so that term was false out of the box and the inline `SET` fast path never ran in the shipped default. (`GET` was unaffected: `can_inline_reads` never carried the term.) The term was a CONFIG predicate standing in for a STATE one. What a live sender changes is eviction ROUTING, and only that: with one, `run_write_eviction_gate` builds `EvictionRun::async_spill`, whose victims are handed to the `SpillThread` under `--appendonly yes`; the inline path can only build `EvictionRun::plain`, whose victims are DELETED. Inlining a write while eviction fires would silently substitute a drop for a spill. So the gate now enforces the actual invariant — the inline write path may run only when eviction provably will not fire — answered per-write by the lock-free `inline_write_can_skip_eviction` pre-gate, hoisted ABOVE the point where the command bytes leave `read_buf` so the bail-out is a true "not handled" rather than a silently lost write. Also adds `!conn.in_cross_txn()`. That defect is INTRODUCED here, not exposed: inside an open TXN the generic leg captures an undo record and a write intent before dispatching and `try_inline_dispatch` does neither, so `SET k original; TXN BEGIN; SET k modified; TXN ABORT; GET k` answered "modified" — an acked abort that rolls back nothing. Found by security review of this branch before it was opened. Corrects the `run_write_eviction_gate` doc, which claimed the no-manifest plain-drop fallback is taken for EVERY write past `maxmemory` under `--disk-offload enable`. It is taken only under `--appendonly no`; with `--appendonly yes` the `AsyncSpill` arm ignores the manifest and spills. New suite `tests/inline_write_spill_gate_660.rs` — 13 tests over --shards 1 and 4, cfg-gated to `runtime-monoio` because `local_inline` is permanently 0 under tokio. Every claim carries a reddening mutation that was applied, observed and reverted; the table in the file header names each one. Refs #660 author: Tin Dang * fix(server): stop inline writes bypassing the write-stall refusal Widening `can_inline_writes` (previous commit) let a plain `SET` reach the inline path in the shipped default. `segment_stall::stall_refusal` — the only producer of `-MOONERR memfull: writes paused until memory pressure recovers`, the MA12 disk-free refusal, and the moon#718 segment-stall refusal — has exactly two call sites, both in GENERIC dispatch. `try_inline_dispatch` has none, so an inlined write answered `+OK` for a write the server had already committed to refusing under memory or disk pressure. Caught by the existing suite, not by review: merge-base 7678156 passes `tests/mem_watchdog.rs` (cases A and B) and `tests/compaction_escape_hatch_718.rs`; the widened gate failed all three. The fix bails to generic dispatch rather than answering the error inline. `stall_refusal` is not a plain boolean — it exempts the commands that are a stall's own remedy (#718's escape hatch) and distinguishes its three sources. Re-deriving that on this path is exactly the drift the shared helper exists to prevent, so the whole decision defers to the leg that owns it. Placed with the eviction pre-gate, above the point where the command bytes leave `read_buf`, so the bail-out is a true "not handled" and not a lost write. Reads are untouched: this is the SET-only branch, and `mem_watchdog` case B asserts GET stays answerable while memfull is engaged. Cost is three Relaxed AtomicBool loads, all false on an unstalled server. Refs #660 author: Tin Dang * fix(server): close four more inline-write gaps found by review Adversarial review of this branch found two further obligations the generic write leg carries and `try_inline_dispatch` did not, and a performance review found two more. Same class as the three already fixed here: an enforcement or side effect that lives in the generic frame loop, which the inline block `continue`s past whenever it consumed the whole buffer. 1. CLIENT PAUSE was bypassed. Measured on one binary under `CLIENT PAUSE 3000 WRITE`: inline SET returned in 0.027s, generic SET (MONITOR attached) in 2.999s, HSET in 2.002s. The pause worked; the inline leg escaped it, so writes landed during a window an operator believes is frozen for failover or backup. Gated on a new lock-free `client_pause::pause_possibly_active()` hint rather than `check_pause` itself, because the latter takes a global RwLock read and a global lock on the write path is what this fast path exists to avoid. It BAILS to generic dispatch, which owns mode/expiry/duration. 2. The -LOADING gate was bypassed. Across a restart with a 40k-document FT index, 6/6 probes while loading:1 answered +OK here and -LOADING on main. 3. Inlined commands were invisible to total_commands_processed (200 plain SETs moved it by 0). `this_thread_commands` is the adaptive idle park's (#373) activity signal, so a shard serving only inlined commands read zero commands/tick and could be classified idle under full load. 4. The shard's cached clock was never refreshed on the inline path. Idle 10s, then SET followed immediately by OBJECT IDLETIME answered 18. Under allkeys-lru every inline-written key carried a frozen stamp, degrading victim selection exactly under memory pressure. Fixed at the same per-batch cadence the generic leg already uses. Also corrects two claims this branch made that measurement did not support: - g2's assertion demanded that ZERO writes inline during an eviction window and blamed any slip on victims being "plain-dropped instead of spilled". The Linux gate caught it: 5 of 2000 inlined. Both halves were wrong. The gate reads PUBLISHED hints that lag under rapid growth, and such a write SKIPS eviction rather than resolving it -- no EvictionRun::plain is ever built, so no drop occurs. g2 now asserts the real safety property (plain drops must not dominate tiering) and bounds the slip at 1% instead of forbidding it. Its window also runs until it has proved its own precondition, after the fixed window tiered on macOS but not on the Linux host (spilled 351 -> 351). - the stall gate's cost comment claimed "three Relaxed AtomicBool loads". Measured: ~7 loads, 2 Acquire, ~4 cache lines, 2 pointer chases, because two of the three sources reach state through OnceLock + an Arc chase. Two deterministic unit guards added for (1) and (2), each proved to redden by deleting its pre-gate. The suite stays 13/13; lib tests 147. Refs #660 author: Tin Dang * test(server): close the review findings on the inline-write gate A test-integrity pass over moon#660 found that the two most recent fixes had shipped with NO guard at all: deleting both the cached-clock refresh and the inline command-counter left the entire suite green. Two more findings were that G1 passed unchanged with inline writes disabled outright, and that `spill_sender_active: true` -- the operand that makes the bail-out fire -- had no unit coverage, every existing call site passing `false`. GROUP 7 adds the missing guards, both mutation-proved rather than assumed: deleting `refresh_now_from_cache` makes a key written microseconds ago report `OBJECT IDLETIME 5s`; deleting `record_inline_commands` advances `total_commands_processed` by 0 while the inline counter climbs by 200. G1 gains an inline-path CONTROL, and `test_inline_set_bails_only_when_a_spill_ sender_is_live` drives `needs_eviction` deterministically by publishing a 1-byte maxmemory so the two calls differ only in the operand under test. The twenty-two unit tests that drive `try_inline_dispatch` are now serialised, because the CLIENT PAUSE guard mutates process-global state the others read. Two claims are WITHDRAWN rather than defended. G2's slip bound now applies at --shards 1 only. The elastic budget lets a lone hot shard borrow its idle siblings' headroom, so at --shards 4 it spends most of a window legitimately under budget and inlines most of it -- measured on the Linux gate at 7,719 of 8,000 writes, with at most 15 plain drops against 156 spills. That is the pre-gate working, and the 1% ceiling asserted against it was wrong, not the server. G2's safety assertion, the one guarding against silent data loss, still runs at both shard counts. G3 is likewise documented as what it is: `remove_cold_only` is unreachable from `try_inline_dispatch`, so G3 reddens identically on merge-base -- a cold-plane guard riding this fixture, not evidence for the inline change -- and it is its RESTART assertion, not its live one, that carries the guard. Two harness defects the same gate surfaced are fixed. `spawn_moon` reserved its admin port OUTSIDE the retry loop, so a held admin port made moon exit at start-up while `spawn_listening` -- which polls the child exactly once, moon#811 -- handed back the corpse; the observable was `read_line` panicking with a bare "read byte". The crash-restart leg had no readiness check at all, only `Client::connect`, which proves a listener accepts and, under SO_REUSEPORT, not even that the peer is the process just spawned. Both now wait for a real +PONG, and `read_line` reports which server died and what it had already sent. Source changes are comment-only: the cross-txn block cited two line numbers that had drifted ~120 lines, and claimed the term closes the MVCC snapshot-visibility hole when only the undo half is demonstrated -- the inline READ path bypasses that filter independently, present identically on merge-base (moon#807). Refs: #660, #807, #811 author: Tin Dang * fix(server): publish the CLIENT PAUSE hint under the lock that clears it Review of PR #812 found a real race in the lock-free pause hint this branch added, and it is the bad direction: a paused write escaping the pause. `pause()` stored `PAUSE_ANY = true` BEFORE acquiring the `PAUSE` write lock, while `unpause()` and `expire_if_needed()` cleared it while holding that lock. A concurrent clear could therefore land in between: 1. `pause()` stores `PAUSE_ANY = true`, then blocks on `PAUSE.write()`. 2. `unpause()` / `expire_if_needed()` takes the lock, clears `active`, and stores `PAUSE_ANY = false`. 3. `pause()` acquires the lock and sets `active = true`. leaving `active == true` with the hint reading `false` for the WHOLE pause window -- so `pause_possibly_active()` answers false and every inline `SET` skips its pause pre-gate during a window an operator believes is frozen. The doc comment asserted this "cannot happen"; it could. Publishing the store inside the lock serialises all three writers and makes the claim true. `test_pause_hint_survives_a_clear_racing_an_activation` makes that interleaving deterministic rather than hoping a stress loop hits it: the test thread holds the write lock so `pause()` blocks exactly where the race lives, lands the clear first, then releases. Mutation-proved -- moving the store back above the lock reddens it with "CLIENT PAUSE is active but the lock-free hint reads false". Also from the same review, two flake sources: * The pause tests and the inline-dispatch tests each had their OWN mutex, and two disjoint mutexes are not serialisation. `test_pause_and_check`'s `pause(5000, ..)` could run concurrently with an inline SET and redden its CONTROL assertion for a reason nothing in its body mentions. There is now one `pause_test_lock()`, next to the state it guards, taken by both modules. * G3's overwrite step demanded a strict `+OK` from writes issued after the filler, where this file's own doc records the inline path answering the fail-loud `-MOONERR AOF backpressure` in roughly 1 run in 3. Those keys are DEL'd immediately after and carry no durability assertion, so the step needs only that the command was answered: `assert_filler_accepted`. `write_probes` stays strict deliberately -- it runs on an empty server before any filler, and its durability IS asserted downstream. Refs: #660 author: Tin Dang * test(storage): add the #660 reconciliation proof, and validate --disk-offload Splits moon#660 in two and lands only the half that is safe on its own. WHAT LANDS `tests/cold_reconciliation_property_660.rs` — the proof #660 records as the one piece of work worth doing regardless of what happens to the default. Disk offload is a two-source-of-truth durability path, and the hazard is RECONCILIATION, not a double-write conflict with the WAL: recovery runs Phase 3 (rebuild `cold_index` from the manifest) then Phase 4 (WAL replay on top, hot shadowing cold). Every bug found in that seam so far has been silent-data-loss class (#212, #213, #459), and every one was caught by soak or adversarial review — never by a proof that the invariant holds in general. A seeded generator drives writes, deletes and expiries under real memory pressure and asserts the server's answer for every key matches a model, both live and after SIGKILL + full recovery. Failures are named by shape (RESURRECTION, EXPIRED-COLD LEAK, LOST WRITE); `MOON_660_SEEDS` replays any seed. It earned its keep immediately — it is what surfaced the COPY/BITOP single-shard durability bug as a deterministic 3-of-3 CI failure instead of a soak-only ghost. `--disk-offload` also gains `value_parser = ["enable", "disable"]`. Only the exact string `enable` ever turned the tier on, so `--disk-offload enabled` silently meant "without the tier". WHAT IS HELD BACK, AND WHY The default flip to `disable` is NOT here. It is a breaking change with a SILENT failure mode: nothing detects existing offload state when the tier is off. `disk_offload_base` is None, the v3 recovery branch is skipped entirely, `cold_index` is never built, and the operator gets no warning, no error and no INFO field — just a smaller keyspace. `docs/versioning.md` documents upgrade as "stop, swap the binary, start", which against that change is a silent keyspace shrink. The flip needs a startup check that REFUSES to start when offload state exists with the tier off, five doc updates, and a runbook before it can ship. Held on its own branch and tracked; nothing about that work is blocked by this commit, and this commit is what makes the flip provable when it comes. author: Tin Dang
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
2 participants
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
Two v3-5 write-path-durability items, both red/green TDD-proven, plus a test-harness de-flake.
1. Coordinator local legs ride group commit under
appendfsync=always(666d32c)persist_local_leg(co-located MSET/MSETNX, scattered-MSET local slices) awaited one fsync ack per command, each bounded by--aof-fsync-timeout-ms(default 2000ms) — a pipeline of coordinated writes stacked these serially into the measured 2000–3000msalwaysfar-tail (tmp/V3-4-GCLOUD-BENCH.md; the carried [HIGH] follow-up).Local legs now use the same fire-and-forget-then-barrier contract as the remote SPSC legs:
AofWriterPool::send_append_group— bounded-backpressure enqueue, never awaits the per-write ack;Ok(true)= caller owes a barrier (Alwaysonly).fsync_barrier(local shard)per pipeline batch, before response serialization —+OKstill implies confirmed durability; batch of N coordinated writes = 1 awaited fsync instead of N.MOONERR AOF fsync— never a false+OK.everysec/nounchanged.2. BITOP/COPY/DEL/UNLINK local legs now persist (d513e8d)
The coordinator's in-process legs for BITOP (dest write), COPY (dst write + PEXPIRE), and multi-key DEL/UNLINK (co-located fast path AND scattered local slice) executed in memory but never reached the AOF (carried v3-4 follow-up). Failure modes on kill-9 + restart: deleted keys resurrected from their seed writes; BITOP/COPY results on the connection's own shard vanished.
run_on_owner_persistmirrorsrun_on_ownerbut appends the executed command to the local shard's AOF on success — replay-safe (each persisted command covers only locally-owned keys).3. Cluster test harness de-flake (1fd4ac7, test-only)
start_cluster_serverslept a fixed 100ms then connected with a no-retry unwrap — "Connection refused" flakes under full-suite parallelism (observed repeatedly in CI-parity runs, pass in isolation). Replaced with a bounded connect-poll; integration suite now 3/3 green at full parallelism.Testing
send_append_groupmust not await the per-write ack / everysec no-barrier / dead-writer errors)coordinator_local_leg_durability.rs(DEL scatter resurrection, UNLINK fast-path resurrection, BITOP dest both shapes, COPY dst both shapes) — deterministic via per-shard{hash-tags}; suite 7/7 green post-fix-D warningsboth feature sets, monoio--releasefull suite, tokio full suite (zero failures, honest exit code)Follow-up (not in this PR)
always-tail magnitude requires a GCE bench (OrbStack fsync is near-free) — happy to run the A/B on requestSummary by CodeRabbit
Performance
Bug Fixes
Tests