perf(storage): task #59 read-vs-spill fairness — off-loop manifest fsync + spill-write pacing - #361
Merged
Merged
Conversation
…c thread (task #59 lever 1) Root cause of the residual task #59 tail (205ms cold-GET during spill flood, G2 re-run): apply_completion_vec ran ONE DURABLE manifest commit (up to 2 fsyncs) PER SPILLED FILE on the shard event-loop thread. strace decomposition (2026-07-16, tmp/t59-decompose.sh + tmp/t59-strace.sh): each shard-N thread spent 1.0-2.1s of an 8s flood window blocked in fsync (163-167 calls, single calls up to 1.0s) — every connection on the shard stalls behind each one. PING p50 through a fresh connection measured 103ms under flood AND under post-flood drain; raw device preads only p50=4ms — the loop, not the disk, was the dominant stall. Fix: split ShardManifest into loop-owned RAM state + ManifestIo (file handle, dual-slot alternation, reopen bookkeeping). enable_deferred_sync() moves ManifestIo onto a per-shard manifest-sync-{id} thread: - commit() keeps EXACTLY its old blocking durability (send + ack) — the checkpoint protocol and the no-AOF durable batch spill path, where the manifest IS the durability record, are unchanged. - commit_deferred() (new) ships a complete root snapshot and returns immediately; consecutive queued snapshots coalesce to the newest (each is a full root at a higher epoch). Used ONLY by the spill-completion apply path, which runs exclusively under --appendonly yes where AOF replay + the orphan sweep reconstruct anything a lost manifest commit recorded. - apply_completion_vec: one deferred commit per drained batch (was: durable commit per file), manifest add_file + cold-index inserts stay RAM-only. - Shutdown flushes pending commits and reclaims the handle inline (shutdown_deferred) before the event loop exits, both runtime arms. Crash-safety invariant (overflow pages sync'd BEFORE the root that points at them; dual-slot atomic root flip) is preserved verbatim — persist() is the old commit() body running on a different thread. TDD (mirrors cold_read's TEST_INJECT_DELAY_MS pattern): - deferred_commit_does_not_block_caller_and_reaches_disk — 200ms injected sync delay, caller returns <100ms, entry durable after flush (fails by construction against the old synchronous path). - durable_commit_blocks_until_persisted — commit() still waits and is immediately reopenable from a second handle. - queued_deferred_commits_coalesce_to_fewer_persists — 20 queued commits coalesce, final superset snapshot lands. - shutdown_reclaims_io_and_inline_commits_still_work. Full lib suite: 4343 passed / 0 failed. Part of task #59 (read-vs-spill fairness); lever 2 (spill pacing) follows. author: Tin Dang <tindang.ht97@gmail.com>
…readers (task #59 lever 2) Even with all fsyncs off the event loop (lever 1), a cold pread still waits behind the spill writer's continuous multi-MB pwrite+fsync stream at the device level: raw 64KB O_DIRECT reads measured p99 208ms during a G2-shape SET flood vs 6ms idle (tmp/t59-decompose.sh, 2026-07-16). Fix: after each durable batch flush, the spill thread consults COLD_READS_INFLIGHT (a Relaxed atomic incremented by RAII guard in BOTH the async cold-read pool path and the synchronous MGET/MULTI/Lua read path, cancel-safe via Drop) and yields one 4ms quantum when a reader is waiting. Guardrails (spill_pace_after_flush, pure decision fn): - reads_inflight == 0 -> never pause (zero idle cost) - request backlog >= REQUEST_QUEUE_CAP/2 -> never pause: a paced spill thread must not push eviction into try_send failure, which surfaces as -OOM to write clients; full-speed drain wins over read latency there - shutdown drain path never paces IO-priority (ioprio_set) deliberately NOT included: nix 0.31 has no wrapper, a raw libc syscall would add a new unsafe block (requires explicit approval per UNSAFE_POLICY), and it is only worth proposing if the pacing + lever-1 numbers still miss the target. Measure first. Tests: 3 unit tests on the decision function (idle-free, backlog-cutoff, quantum bounds) + RAII guard count/release test. Full lib suite 4347/0. author: Tin Dang <tindang.ht97@gmail.com>
author: Tin Dang <tindang.ht97@gmail.com>
author: Tin Dang <tindang.ht97@gmail.com>
…ag + MOON_BIN-blind spawn The 4 tests in cmd_flush_dbsize_debug_memory.rs have been silently broken since 1d26618 removed the --persistence-dir CLI flag (June 2026): the spawned server exits with status 2 (clap arg error) and the suite either fails or self-skips when target/release/moon is absent — which is why CI (no release binary) stayed green while any run with a built binary went red. Two fixes: - Pass --dir (the current flag) instead of the removed --persistence-dir. - Route binary discovery through common::find_moon_binary() so MOON_BIN is honored; the previous hardcoded target/release/moon fallback is the documented stale-binary trap on shared checkouts (OrbStack VM exec's a host Mach-O via proxy and the server never accepts in-VM). Verified on moon-dev: 4/4 green in 0.7s with MOON_BIN pinned to the tokio release-fast binary. Refs: unmasked while running the task #59 gate battery author: Tin Dang <tindang.ht97@gmail.com>
…memory_doctor Running the task #59 gate battery on a near-full dev volume (/Volumes/Games at <1% free) unmasked four latently environment-fragile suites: - mem_watchdog, oom_bypass_closure, spsc_two_db: root their server dirs on the repo volume, so the 5%-free diskfull write pause shadowed every assertion with MOONERR diskfull. These suites test the RSS watchdog, the -OOM eviction gate, and COPY/MOVE routing — not the disk guard — so they now pass --disk-free-min-pct 0, per the established harness convention (gotcha_diskfull_guard_gutted_crash_tests). - memory_doctor_response: hardcoded target/release/moon finder ignored MOON_BIN — on a shared macOS/Linux checkout the VM exec'd a stale host Mach-O via OrbStack's proxy and spawn_listening timed out after 30s. Now routes through common::find_moon_binary(). All four suites green on moon-dev with MOON_BIN pinned: 16/16 in <8s (previously 13 failures + one 30s timeout). author: Tin Dang <tindang.ht97@gmail.com>
… battery author: Tin Dang <tindang.ht97@gmail.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change moves manifest persistence to per-shard deferred sync threads, coalesces spill-completion commits, adds cold-read-aware spill pacing, fixes BGSAVE startup trigger handling, and updates integration test launch and crash synchronization. ChangesPersistence and spill coordination
BGSAVE startup handling
Integration test harness corrections
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShardEventLoop
participant ManifestSyncAgent
participant ManifestIo
participant SpillThread
participant ColdReadPool
ShardEventLoop->>ManifestSyncAgent: enable deferred sync
SpillThread->>ManifestSyncAgent: submit deferred manifest batch
ManifestSyncAgent->>ManifestIo: persist coalesced root
ColdReadPool->>SpillThread: expose in-flight read count
SpillThread->>SpillThread: pace after flush when readers are waiting
ShardEventLoop->>ManifestSyncAgent: shutdown and flush
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/persistence/manifest.rs (2)
301-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe shared test knob uses a poisoned standard mutex. Replace it with
parking_lot::Mutexand remove every.unwrap().
src/persistence/manifest.rs#L301-L302: define the knob withparking_lot::Mutex.src/persistence/manifest_sync.rs#L192-L193: acquire with.lock().src/persistence/manifest_sync.rs#L222-L223: acquire with.lock().src/persistence/manifest_sync.rs#L251-L252: acquire with.lock().src/persistence/manifest_sync.rs#L290-L291: acquire with.lock().As per coding guidelines, “Use
parking_lot::RwLockandparking_lot::Mutexinstead ofstd::synclocks; do not use poisoned-lock unwrap patterns.”🤖 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/persistence/manifest.rs` around lines 301 - 302, The shared TEST_SYNC_KNOB_LOCK should use parking_lot::Mutex instead of std::sync::Mutex, and all lock acquisitions must remove poisoned-lock unwrap patterns. Update the definition in src/persistence/manifest.rs#L301-L302, and replace each corresponding acquisition with .lock() in src/persistence/manifest_sync.rs#L192-L193, `#L222-L223`, `#L251-L252`, and `#L290-L291`.Source: Coding guidelines
898-1075: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove
ManifestIointo a dedicated module.This file now exceeds 1,800 lines, and the newly extracted I/O implementation is already a cohesive module boundary.
As per coding guidelines, “No single Rust file should exceed 1500 lines.”
🤖 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/persistence/manifest.rs` around lines 898 - 1075, Move the complete ManifestIo implementation, including persist and compact and its associated helpers/state, from manifest.rs into a dedicated persistence I/O module. Update module declarations, imports, visibility, and all call sites so ManifestIo remains available through the existing API without changing behavior. Keep manifest.rs below the 1,500-line guideline.Source: Coding guidelines
🤖 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/persistence/manifest_sync.rs`:
- Around line 67-82: The manifest-sync startup path currently drops ManifestIo
when thread creation fails, disabling durable inline operation. Update
manifest_sync::spawn to return the retained I/O alongside startup failure,
preserve IoBackend::Inline in manifest initialization when startup fails, and
update the event-loop startup handling to continue using inline durability
rather than disabling manifest persistence; apply these changes in
src/persistence/manifest_sync.rs:67-82, src/persistence/manifest.rs:541-548, and
src/shard/event_loop.rs:637-638.
- Around line 258-269: Update the test setup around TEST_INJECT_SYNC_DELAY_MS
and the burst loop to wait until TEST_PERSIST_COUNT advances from the captured
before value, confirming the first persist has entered the injected delay before
enqueueing the remaining requests. Keep the delay enabled during that
synchronization, then enqueue the rest of the burst and reset it afterward so
the coalescing assertion is deterministic.
- Around line 17-20: Update commit_deferred and its manifest-sync queueing to
use a bounded(1) wake channel with non-blocking sends, retaining only the latest
snapshot when the worker is behind. Remove the blocking flume::Sender::send
behavior for deferred commits while preserving the existing coalescing and
sync-worker processing semantics.
In `@src/storage/tiered/spill_thread.rs`:
- Around line 731-740: Update reader_inflight_guard_counts_and_releases to avoid
asserting exact values of the process-global COLD_READS_INFLIGHT counter; test
ColdReadInflightGuard with an injected or local counter instead, or serialize
all tests that access the global counter.
---
Nitpick comments:
In `@src/persistence/manifest.rs`:
- Around line 301-302: The shared TEST_SYNC_KNOB_LOCK should use
parking_lot::Mutex instead of std::sync::Mutex, and all lock acquisitions must
remove poisoned-lock unwrap patterns. Update the definition in
src/persistence/manifest.rs#L301-L302, and replace each corresponding
acquisition with .lock() in src/persistence/manifest_sync.rs#L192-L193,
`#L222-L223`, `#L251-L252`, and `#L290-L291`.
- Around line 898-1075: Move the complete ManifestIo implementation, including
persist and compact and its associated helpers/state, from manifest.rs into a
dedicated persistence I/O module. Update module declarations, imports,
visibility, and all call sites so ManifestIo remains available through the
existing API without changing behavior. Keep manifest.rs below the 1,500-line
guideline.
🪄 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: 0923bc95-c734-4798-b131-fd9551ef1964
📒 Files selected for processing (14)
CHANGELOG.mdsrc/persistence/manifest.rssrc/persistence/manifest_sync.rssrc/persistence/mod.rssrc/shard/event_loop.rssrc/shard/persistence_tick.rssrc/storage/tiered/cold_read.rssrc/storage/tiered/cold_read_pool.rssrc/storage/tiered/spill_thread.rstests/cmd_flush_dbsize_debug_memory.rstests/mem_watchdog.rstests/memory_doctor_response.rstests/oom_bypass_closure.rstests/spsc_two_db.rs
… commits can never block Review finding on PR #361, confirmed real: commit_deferred used a blocking flume send into a bounded(64) request queue. The worker's try_recv drain keeps up with any live burst, but commits arriving while the worker is INSIDE a stalled persist (saturated-disk fsync, seconds are realistic) pile up with no drainer — at 64 deep the shard thread blocks on send, re-creating exactly the stall the agent exists to remove. Replace the queue with a latest-snapshot slot (parking_lot::Mutex) plus a bounded(1) wake-token channel: - commit_deferred: replace slot + try_send token — never blocks. Full is fine (state was written before the send, the pending round observes it); Disconnected still fails loudly. - commit_durable: same handoff, then blocks on its ack as before. Acks are pushed with the slot write and taken with it; firing after a newer snapshot persists remains correct (a newer root strictly supersedes). - Coalescing is now by construction: the slot holds only the newest root. Red/green: new test burst_of_deferred_commits_never_blocks_the_caller primes the worker into a 400ms-stalled persist, then bursts 100 deferred commits — the old agent blocks (FAILED, caller waits out the persist), the new agent absorbs it in <200ms. All 5 manifest_sync unit tests, the 46 crash_matrix_cross_plane kill-9 tests, cold_orphan_sweep, and manifest_gc_tombstones pass. refs #361 author: Tin Dang <tindang.ht97@gmail.com>
…BGSAVE no longer hangs The listener accepts clients as soon as the FASTEST shard's event loop is up, so a BGSAVE (or auto-save) can broadcast its snapshot epoch while a slower shard is still initializing. Each shard seeded its epoch cursor from the watch channel's CURRENT value at loop start (`last_snapshot_epoch = snapshot_trigger_rx.borrow()`), silently swallowing such a pending trigger: that shard never snapshotted, BGSAVE_SHARDS_REMAINING never reached zero, and rdb_bgsave_in_progress stuck at 1 forever — every later BGSAVE refused as already-in-progress until restart. Pre-existing (not introduced by this branch); surfaced as a ~15%/run crash_matrix_cross_plane flake under full-suite CPU contention, where a stack sample of a stuck instance showed all four shard threads idle in kevent (state-machine hang, not a blocked thread) and INFO showed rdb_last_save_time:0 — the FIRST BGSAVE of the instance's life had hung. Fix: start the cursor at 0. Epochs are per-process and start at 0, so 0 is always "nothing seen yet"; a trigger that raced startup is honored on the shard's first tick. Red/green: new test-only fault injection MOON_TEST_SLOW_SHARD_START_MS delays every non-zero shard's loop entry, making the race deterministic — tests/bgsave_startup_race.rs times out at 10s on the pre-fix cursor every run and completes in ~1s post-fix. 18 full crash-matrix suite runs after the fix: zero BGSAVE hangs (was the dominant failure mode). refs #361 author: Tin Dang <tindang.ht97@gmail.com>
… kill-9 cross_plane_mq_isolated synced durability on a marker pushed to ORDERSQ and then killed -9 — but the DLQ queue lives on a different shard with an independent WAL stream that flushes on its own 1ms tick. Under full-suite CPU contention that tick can slip past the kill, so the cell measured cross-shard flush-timing luck instead of DLQ durability: autopsy of a failing run's data dir showed the dlqtestq bytes (create + both pushes + DLQ routing) absent from EVERY shard's WAL at kill time (2/12 suite runs; 0/30 isolated runs). Wait for the DLQ queue's own bytes to appear in a WAL before crashing — the product contract under test (DLQ routing recorded in the WAL survives kill-9) is unchanged and now tested exactly. 15 suite runs after the fix: zero mq failures. refs #361 author: Tin Dang <tindang.ht97@gmail.com>
…istic tests Three remaining review findings on PR #361, all verified real: 1. A failed manifest-sync thread spawn (thread limit, EAGAIN) destroyed the working inline backend: the ManifestIo moved into the dropped closure, leaving a manifest that could never commit again until restart. spawn() now hands the io to the thread over a channel and returns Result<Self, (ManifestIo, io::Error)> — on failure enable_deferred_sync keeps IoBackend::Inline and logs, degrading to on-loop fsyncs instead of no durability at all. 2. queued_deferred_commits_coalesce_to_fewer_persists raced its own delay knob: the injected persist stall was reset right after the burst, so scheduling could let the worker miss it and weaken the coalescing bound into flakiness. The test now primes the worker INSIDE a stalled persist before bursting and keeps the delay armed through the shutdown flush. 3. reader_inflight_guard_counts_and_releases asserted exact values on the process-global COLD_READS_INFLIGHT while sibling tests in the same binary exercise the cold-read path concurrently. The guard gains ColdReadInflightGuard::on(&'static AtomicUsize) and the test verifies the RAII mechanics on its own local counter. refs #361 author: Tin Dang <tindang.ht97@gmail.com>
Contributor
Author
Crash-matrix flake investigation (follow-up on this branch)While validating the review-round fixes, Mode A — BGSAVE stuck forever (dominant; FIXED in c4cd94c)A stack sample of a stuck instance showed all four shard threads idle in kevent — not a blocked thread, a state-machine hang. Mode C — MQ DLQ entries "lost" across kill-9 (FIXED in 0598d86, harness)Autopsy of a failing run's data dir: the DLQ queue's bytes (create + pushes + routing) were absent from every shard's WAL at kill time — the cell synced durability on a marker pushed to a different shard's queue, then killed -9. Per-shard WAL streams flush independently on each shard's 1ms tick; under full-suite CPU contention the DLQ shard's tick slips past the kill. The cell now also waits for the DLQ queue's own bytes before crashing. Product contract unchanged and now tested exactly. (0/30 isolated runs failed; only suite-parallel runs raced.) Mode B — rare ConnectionReset (~1/18, OPEN)Server-side stderr shows no panic; different cells each time (kv legacy, mixedpassc legacy, graphdrop s1). Not attributable to this branch with current evidence (a 6% rate is compatible with the clean 14-run baseline). Tracking separately in #365. Validation18 full-suite runs with all fixes: 0 BGSAVE hangs, 0 mq failures (runs after the harness fix compiled in), 1 mode-B reset. Plus: 46/46 crash matrix (single run), manifest_sync unit tests 5/5, |
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)
CHANGELOG.md (1)
55-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the five-suite scope.
This wording can imply that all five suites both switched to
MOON_BINresolution and added--disk-free-min-pct 0. The detailed description indicates these fixes were split across the suites; say “across five suites” to avoid overstating the per-suite changes.🤖 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 `@CHANGELOG.md` around lines 55 - 63, Clarify the changelog entry’s opening to say the fixes occurred across five integration suites, rather than implying every suite received both changes. Preserve the existing breakdown identifying which suites use MOON_BIN and which receive --disk-free-min-pct 0.
♻️ Duplicate comments (1)
src/persistence/manifest_sync.rs (1)
303-316: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSynchronize on persist entry instead of sleeping.
A loaded scheduler can delay the worker beyond 50 ms, so both tests may enqueue their bursts before the injected stall begins and falsely pass. Wait with a deadline until
TEST_PERSIST_COUNTadvances from its baseline.This repeats the earlier synchronization finding, which remains applicable to the current fixed-sleep implementation.
Also applies to: 350-359
🤖 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/persistence/manifest_sync.rs` around lines 303 - 316, Replace the fixed 50 ms sleep in the deferred-persist test around TEST_INJECT_SYNC_DELAY_MS with deadline-based polling that waits until TEST_PERSIST_COUNT advances from its recorded baseline, then enqueue the burst. Apply the same synchronization change to the corresponding block around the second test location, preserving the existing stall and shutdown flow.
🧹 Nitpick comments (1)
tests/bgsave_startup_race.rs (1)
71-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImprove test robustness by using
common::spawn_listening.The manual process spawn and
Conn::openloop can hang for 30 seconds if the child process fails to bind the port or crashes at startup (e.g., due to a port collision in parallel CI). Using thecommon::spawn_listeninghelper handles this cleanly by detecting child exits and retrying with a fresh port.🛠️ Proposed refactor
- let port = common::reserve_port(); - let mut child = std::process::Command::new(find_moon_binary()); - child - .args([ - "--port", - &port.to_string(), - "--dir", - &dir.path().to_string_lossy(), - "--shards", - "4", - "--appendonly", - "yes", - "--disk-free-min-pct", - "0", - ]) - // Shards 1-3 enter their event loops ~500ms late; shard 0 (and the - // listener) come up immediately, so the BGSAVE below lands inside - // the startup window. - .env("MOON_TEST_SLOW_SHARD_START_MS", "500") - .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()); - let mut child = child.spawn().expect("spawn moon"); + let (mut child, port) = common::spawn_listening(|port| { + let mut child = std::process::Command::new(find_moon_binary()); + child + .args([ + "--port", + &port.to_string(), + "--dir", + &dir.path().to_string_lossy(), + "--shards", + "4", + "--appendonly", + "yes", + "--disk-free-min-pct", + "0", + ]) + // Shards 1-3 enter their event loops ~500ms late; shard 0 (and the + // listener) come up immediately, so the BGSAVE below lands inside + // the startup window. + .env("MOON_TEST_SLOW_SHARD_START_MS", "500") + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()); + child.spawn().expect("spawn moon") + });🤖 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 `@tests/bgsave_startup_race.rs` around lines 71 - 92, Replace the manual process construction and startup waiting in the BGSAVE race test with common::spawn_listening, preserving the existing Moon arguments, environment configuration, and child-process handle usage. Ensure the helper supplies the selected listening port and handles startup failures by detecting child exits and retrying with a fresh port.
🤖 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/persistence/manifest_sync.rs`:
- Around line 153-156: Update ManifestSync::shutdown to join the worker thread
before waiting on gb_rx, then clear the stored shutdown sender in SyncShared
before calling gb_rx.recv(). Preserve the existing reclaimed ManifestIo return
behavior while ensuring a panicked worker cannot leave the receiver blocked by
an unconsumed sender.
In `@tests/crash_matrix_cross_plane/scenarios.rs`:
- Around line 478-485: Update the synchronization after the DLQ operation to
wait for a unique payload from the later DLQ-routing write, rather than
dlqtestq.as_bytes(), which also occurs in queue creation. Use the DLQ
operation’s distinctive bytes or the owning shard’s WAL position with
harness::wait_for_wal_v3_bytes_any_shard, while preserving the crash timing and
durability check.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 55-63: Clarify the changelog entry’s opening to say the fixes
occurred across five integration suites, rather than implying every suite
received both changes. Preserve the existing breakdown identifying which suites
use MOON_BIN and which receive --disk-free-min-pct 0.
---
Duplicate comments:
In `@src/persistence/manifest_sync.rs`:
- Around line 303-316: Replace the fixed 50 ms sleep in the deferred-persist
test around TEST_INJECT_SYNC_DELAY_MS with deadline-based polling that waits
until TEST_PERSIST_COUNT advances from its recorded baseline, then enqueue the
burst. Apply the same synchronization change to the corresponding block around
the second test location, preserving the existing stall and shutdown flow.
---
Nitpick comments:
In `@tests/bgsave_startup_race.rs`:
- Around line 71-92: Replace the manual process construction and startup waiting
in the BGSAVE race test with common::spawn_listening, preserving the existing
Moon arguments, environment configuration, and child-process handle usage.
Ensure the helper supplies the selected listening port and handles startup
failures by detecting child exits and retrying with a fresh port.
🪄 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: 20901d03-ca24-4ecb-bdc2-3b3a1c3b17aa
📒 Files selected for processing (8)
CHANGELOG.mdsrc/persistence/manifest.rssrc/persistence/manifest_sync.rssrc/shard/event_loop.rssrc/storage/tiered/cold_read_pool.rssrc/storage/tiered/spill_thread.rstests/bgsave_startup_race.rstests/crash_matrix_cross_plane/scenarios.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/storage/tiered/cold_read_pool.rs
- src/persistence/manifest.rs
- src/storage/tiered/spill_thread.rs
Comment on lines
+153
to
+156
| pub(crate) fn shutdown(mut self) -> Option<ManifestIo> { | ||
| let (gb_tx, gb_rx) = flume::bounded::<ManifestIo>(1); | ||
| self.shared.lock().shutdown = Some(gb_tx); | ||
| let _ = self.notify(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Join the worker before waiting for reclaimed I/O.
If the worker panics, shutdown stores gb_tx in SyncShared and then blocks forever on gb_rx.recv() because that sender remains alive. Join first, then remove any unconsumed shutdown sender before receiving.
Proposed fix
pub(crate) fn shutdown(mut self) -> Option<ManifestIo> {
let (gb_tx, gb_rx) = flume::bounded::<ManifestIo>(1);
self.shared.lock().shutdown = Some(gb_tx);
let _ = self.notify();
- let io = gb_rx.recv().ok();
if let Some(j) = self.join.take() {
let _ = j.join();
}
- io
+ // Drop the sender if the worker exited before consuming it.
+ self.shared.lock().shutdown.take();
+ gb_rx.recv().ok()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub(crate) fn shutdown(mut self) -> Option<ManifestIo> { | |
| let (gb_tx, gb_rx) = flume::bounded::<ManifestIo>(1); | |
| self.shared.lock().shutdown = Some(gb_tx); | |
| let _ = self.notify(); | |
| pub(crate) fn shutdown(mut self) -> Option<ManifestIo> { | |
| let (gb_tx, gb_rx) = flume::bounded::<ManifestIo>(1); | |
| self.shared.lock().shutdown = Some(gb_tx); | |
| let _ = self.notify(); | |
| if let Some(j) = self.join.take() { | |
| let _ = j.join(); | |
| } | |
| // Drop the sender if the worker exited before consuming it. | |
| self.shared.lock().shutdown.take(); | |
| gb_rx.recv().ok() | |
| } |
🤖 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/persistence/manifest_sync.rs` around lines 153 - 156, Update
ManifestSync::shutdown to join the worker thread before waiting on gb_rx, then
clear the stored shutdown sender in SyncShared before calling gb_rx.recv().
Preserve the existing reclaimed ManifestIo return behavior while ensuring a
panicked worker cannot leave the receiver blocked by an unconsumed sender.
Comment on lines
+478
to
+485
| // The marker proves ORDERSQ's shard flushed its WAL stream — it says | ||
| // nothing about the shard that owns the DLQ queue: per-shard WAL streams | ||
| // flush independently on each shard's own 1ms tick, and under CPU | ||
| // starvation (full-suite parallelism) that tick can slip past this kill. | ||
| // Sync on the DLQ queue's own bytes before crashing, or this cell | ||
| // measures cross-shard flush-timing luck instead of DLQ durability | ||
| // (observed: dlqtestq absent from EVERY WAL at kill time, 2/12 runs). | ||
| harness::wait_for_wal_v3_bytes_any_shard(&dir, cfg.shards, dlqtestq.as_bytes()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Wait for a unique DLQ write. dlqtestq.as_bytes() already appears in the queue-creation WAL record, so this byte search can succeed before the later DLQ-routing records are flushed. Use a payload that only appears in the DLQ operation, or wait on the owning shard’s WAL position instead.
🤖 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 `@tests/crash_matrix_cross_plane/scenarios.rs` around lines 478 - 485, Update
the synchronization after the DLQ operation to wait for a unique payload from
the later DLQ-routing write, rather than dlqtestq.as_bytes(), which also occurs
in queue creation. Use the DLQ operation’s distinctive bytes or the owning
shard’s WAL position with harness::wait_for_wal_v3_bytes_any_shard, while
preserving the crash timing and durability check.
tests/bgsave_startup_race.rs failed in CI (runtime-tokio): the tokio shard loop's snapshot-finalize path called finalize_snapshot_success/error but — unlike the monoio arm — never called bgsave_shard_done, so BGSAVE_SHARDS_REMAINING stayed at num_shards and rdb_bgsave_in_progress stuck at 1 after EVERY sharded BGSAVE under runtime-tokio. Pre-existing and CI-invisible until now: the crash suites that await BGSAVE are `#[ignore]`d (run only in the local VM battery, which uses monoio), so no CI test had ever waited for a tokio BGSAVE to complete. Mirror the monoio arm: bgsave_shard_done(true/false) after finalize. Safe for auto-save-triggered snapshots — the counter ignores calls at 0. Red: CI Check job, 3/3 retries timing out at the 10s deadline (and the same locally with --features runtime-tokio,jemalloc). Green: 0.99s local tokio run; monoio leg unaffected (1.35s). refs #361 author: Tin Dang <tindang.ht97@gmail.com>
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 19, 2026
…eset (#393) Patch release folding the post-v0.8.0 perf/correctness train (#361–#392) plus a single-shard tuning preset (broadened --profile standalone + conf/moon-standalone.conf). O3 contention governor makes --io-busy-poll-us deploy-safe on any host. Adversarial review caught + fixed a conf-file arena-cap false-claim before merge. Gate: crash-matrix nightly + ITERS=20 soak green on RC.
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
Task #59 — close the read-vs-spill fairness gap: under a sustained spill flood, cold-plane GETs (and every other command on the same shard) inherited multi-hundred-ms tails because the shard event-loop thread was doing durable manifest commits (up to 2 fsyncs) once per spilled file. strace over an 8s flood window: 163–224 fsync calls per shard thread, 0.68–2.1s cumulative block time, single fsyncs up to 1.0s.
Two levers, both in this PR:
src/persistence/manifest_sync.rs):ShardManifestsplits into loop-owned RAM state + a per-shardmanifest-sync-{id}thread that owns the file handle. Spill-completion commits are deferred and coalescing (newest-root-wins; acks only fire after a persist that supersedes them — superset durability). This is correct because the async spill path only runs under--appendonly yes, where AOF replay + the orphan sweep reconstruct anything a lost manifest commit recorded.commit()keeps its exact blocking durability for the checkpoint protocol and the no-AOF durable batch path. The crash-safety write order (overflow pages fsync'd before the root that references them; dual-slot atomic root flip) is preserved verbatim, now single-threaded inside the agent.src/storage/tiered/spill_thread.rs): after each durable batch flush, the spill writer yields a 4ms quantum only while a cold read is in flight (COLD_READS_INFLIGHTRAII signal from both the async read pool and the sync MGET/MULTI/Lua paths) and never when the request backlog is deep (≥ 2048 = half queue cap — pacing must not push evictiontry_sendinto-OOM).Deferred (needs explicit approval for new
unsafe):ioprio_seton the spill thread — rawlibcsyscall, nonix 0.31wrapper. Measure-first call: the structural fix already removed all loop stalls.A/B evidence (moon-dev VM, artifact-free raw-socket probes + per-thread strace)
manifest-sync-*)The fix leg ran on a ~3× slower disk window and still halved the maxima. Residual tail is raw device queueing on the shared VM vdisk (single fsyncs up to 0.75s observed at device level); production NVMe should approach the sub-10ms goal.
Measurement disclosure: the published G2 "205ms worst cold GET" contained a ~100ms client-side constant — fresh
redis-cliexecs major-page-fault their binary back in under VM page-cache pressure (invisible to strace). The A/B above uses raw/dev/tcpprobes timed in-process. Bench protocol note added to project memory.Gate battery (repl/persistence rule: parity + kill-9 + A/B)
crash_matrix_cross_plane: 46/46 cells green (--ignored --test-threads=1, monoio binary)--no-default-features --features runtime-tokio,jemalloc,MOON_NO_URING=1): 167 test binaries, 0 failures (3549 lib + integration)--release): 166 test binaries, 0 failurescargo clippy -- -D warnings(default + tokio configs, incl.--tests),cargo fmt --check: cleanmanifest_synctests (deferred non-blocking, durable blocking, coalescing, shutdown-reclaim) + 4 spill-pacing testsDrive-by: 5 latently broken test harnesses (unmasked by the gates)
These failures pre-date the branch and were invisible in CI:
cmd_flush_dbsize_debug_memory: still passed--persistence-dir, removed in June by 1d26618 — the server exits 2 before accepting; CI stayed green only because the suite self-skips whentarget/release/moonis absent. Now passes--dirand routes binary discovery throughcommon::find_moon_binary().memory_doctor_response: MOON_BIN-blind hardcoded finder → stale host Mach-O via OrbStack proxy → 30s spawn timeout. Samefind_moon_binary()fix.mem_watchdog,oom_bypass_closure,spsc_two_db: rooted server dirs on the repo volume; at <5% free the diskfull write pause shadowed every assertion withMOONERR diskfull. These suites test the RSS watchdog / OOM gate / COPY-MOVE routing — not the disk guard — so they now pass--disk-free-min-pct 0per the established harness convention.Follow-up candidate (not this PR): ~15 more test files still hardcode
target/release/moon.Summary by CodeRabbit
--disk-free-min-pct 0.