fix(persistence): latch a degraded state when the data dir vanishes — no per-tick ENOENT error loop (#366)#369
Merged
Conversation
… no per-tick ENOENT error loop (#366) Deleting --dir under a running server (operator rm -rf, tmp-cleaner sweep, mount loss) used to leave the per-shard persistence tick retrying its file operations on ENOENT forever: WAL flush at the 1ms cadence, the WAL-overflow directory scan warning once per second per shard, checkpoints failing every tick. Observed in the wild as an idle 4-shard server at ~667% CPU (sys:user ~50:1) with a wedged PING. Design-for-failure, mirroring the MOONERR diskfull guard: - DiskMonitor's existing 5s statvfs poll now distinguishes ENOENT/ENOTDIR on the monitored path (deterministic "the dir is gone") from transient errors (which keep retaining previous state, as before), and latches a new `dir_lost` flag with ONE loud ERROR log. The latch feeds the existing write-stall gate: writes are refused with a distinct `MOONERR dirmissing` message at both dispatch sites; reads and PING keep serving. - The persistence tick consults `is_dir_lost()` (one relaxed atomic load) and skips WAL flush, checkpoint tick, WAL-overflow scan, and auto-save entirely while latched — zero per-tick syscalls, zero per-tick log lines. Manual BGSAVE still fails one attempt loudly through the existing finalize-error path (fan-in counter intact). - Self-heal: the next poll that sees the path again clears the latch and resumes writes, warning explicitly that data accepted since the deletion is not guaranteed durable until restart (WAL fds point at unlinked inodes). - The latch only arms when persistence is configured (an in-memory server losing its CWD has nothing to protect) and works with --disk-free-min-pct 0: poll_global now runs whenever either the free-space guard or the dir guard is active. Measured on the incident configuration (4 shards, appendonly yes): SET → MOONERR dirmissing within one poll, GET/PING unaffected, CPU flat at the idle baseline, stderr frozen after the single ERROR line (pre-fix: +4 warn lines/s forever). Red/green: tests/dir_deleted_degraded.rs pins the four contract points (refusal ≤ one poll, responsiveness while latched, quiesced log+CPU, self-heal) and failed at the refusal assertion pre-fix; two unit tests pin the latch transitions and the disarmed-guard default. Gates: fmt, clippy (default + tokio,jemalloc), 4369 lib tests, integration test green under monoio (macOS), tokio (macOS), and Linux VM. Closes #366 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? |
📝 WalkthroughWalkthroughThe server now latches a degraded state when its persistence directory disappears, refuses writes with ChangesDirectory-loss degraded mode
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Filesystem
participant DiskMonitor
participant WriteHandler
participant PersistenceTick
Filesystem-->>DiskMonitor: PathMissing
DiskMonitor->>DiskMonitor: Latch dir_lost
WriteHandler->>DiskMonitor: Check write state
DiskMonitor-->>WriteHandler: dir_lost=true
WriteHandler-->>WriteHandler: Return MOONERR dirmissing
PersistenceTick->>DiskMonitor: Check dir_lost
DiskMonitor-->>PersistenceTick: Skip persistence work
Filesystem-->>DiskMonitor: Directory restored
DiskMonitor->>DiskMonitor: Clear dir_lost
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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/shard/disk_monitor.rs`:
- Around line 155-166: In the successful `statvfs` branch of the disk-monitor
update logic, do not clear `dir_lost` or resume writes unless
`self.monitored_path` still exists as a directory; validate it with metadata
before the `dir_lost.swap(false, Ordering::Release)` transition. Preserve the
latch and paused state when the path is a regular file or otherwise invalid, and
retain the existing warning only for a genuinely restored directory.
- Around line 393-397: Update the non-Unix `query_free_bytes` stub to check the
supplied path with portable metadata and verify it is a directory; return
`QueryError::PathMissing` when metadata fails or the path is not a directory,
otherwise preserve the existing unlimited free-space result.
In `@src/shard/persistence_tick.rs`:
- Around line 117-129: Update the epoch-handling logic around
last_snapshot_epoch so the data-directory-missing early return applies only to
auto-save triggers. Preserve manual BGSAVE triggers for their existing
snapshot/error coordination path, or explicitly fail and complete BGSAVE before
returning; only advance last_snapshot_epoch without snapshot state for auto-save
epochs.
In `@tests/dir_deleted_degraded.rs`:
- Around line 74-84: Make the CPU-time probe fail explicitly when the ps command
fails or produces empty output. In the elapsed-time parsing block, validate
out.status, require non-empty trimmed stdout, and replace the unwrap_or(0.0)
fallback with strict field parsing that propagates or asserts on malformed
values, preventing invalid output from becoming 0ms.
🪄 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: b1607409-7abe-4049-ab50-81bcae566fb0
📒 Files selected for processing (7)
CHANGELOG.mdsrc/main.rssrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_sharded/dispatch.rssrc/shard/disk_monitor.rssrc/shard/persistence_tick.rstests/dir_deleted_degraded.rs
Comment on lines
+155
to
166
| Ok((free, total)) => { | ||
| if self.dir_guard && self.dir_lost.swap(false, Ordering::Release) { | ||
| tracing::warn!( | ||
| path = %self.monitored_path.display(), | ||
| "disk_monitor: data directory reappeared — writes resume; \ | ||
| data accepted since the deletion is NOT guaranteed durable \ | ||
| until the server is restarted (WAL fds point at unlinked \ | ||
| inodes)", | ||
| ); | ||
| } | ||
| self.free_bytes.store(free, Ordering::Relaxed); | ||
| self.update_paused(free, total); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant sections.
ast-grep outline src/shard/disk_monitor.rs --view expanded || true
echo
echo "---- relevant lines ----"
sed -n '1,260p' src/shard/disk_monitor.rs | cat -n
echo
echo "---- search for dir_lost / statvfs / metadata checks ----"
rg -n "dir_lost|statvfs|metadata\\(|is_dir\\(|mounted|mountpoint|fstatfs|statfs|realpath" src/shard/disk_monitor.rsRepository: pilotspace/moon
Length of output: 17354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the disk monitor implementation around the latch update and any fallback.
wc -l src/shard/disk_monitor.rs
sed -n '1,220p' src/shard/disk_monitor.rs | cat -nRepository: pilotspace/moon
Length of output: 11233
Keep dir_lost latched until the path is still the monitored directory. statvfs can still succeed for a regular file or after a mountpoint loses its backing mount, so this branch can clear the latch and resume writes even though the persistence target is no longer valid. Check metadata().is_dir() and, if mount loss matters, the original filesystem identity before clearing.
🤖 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/disk_monitor.rs` around lines 155 - 166, In the successful
`statvfs` branch of the disk-monitor update logic, do not clear `dir_lost` or
resume writes unless `self.monitored_path` still exists as a directory; validate
it with metadata before the `dir_lost.swap(false, Ordering::Release)`
transition. Preserve the latch and paused state when the path is a regular file
or otherwise invalid, and retain the existing warning only for a genuinely
restored directory.
Comment on lines
+393
to
+397
| /// Non-Unix stub: monitoring is a no-op, writes are never paused by disk and | ||
| /// the dir-lost latch never engages. | ||
| #[cfg(not(unix))] | ||
| fn query_free_bytes(_path: &Path) -> Option<(u64, u64)> { | ||
| Some((u64::MAX, u64::MAX)) | ||
| fn query_free_bytes(_path: &Path) -> Result<(u64, u64), QueryError> { | ||
| Ok((u64::MAX, u64::MAX)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the non-Unix implementation plus nearby logic.
ast-grep outline src/shard/disk_monitor.rs --view expanded || true
wc -l src/shard/disk_monitor.rs
sed -n '1,260p' src/shard/disk_monitor.rs
printf '\n----\n'
sed -n '260,460p' src/shard/disk_monitor.rs
printf '\n----\n'
sed -n '460,620p' src/shard/disk_monitor.rs
# Find all uses of the directory-loss / PathMissing path.
rg -n "PathMissing|dir-lost|directory|query_free_bytes|monitoring is a no-op|degraded mode|pause" src/shard -SRepository: pilotspace/moon
Length of output: 45832
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether Windows / non-Unix is treated as a supported runtime target.
rg -n "#\[cfg\(not\(unix\)\)\]|windows|win32|microsoft|non-unix|target_os = \"windows\"" src Cargo.toml README.md -S
# Inspect the persistence tick / hot-path gating that depends on dir-lost.
sed -n '100,170p' src/shard/persistence_tick.rs
printf '\n----\n'
sed -n '220,260p' src/shard/persistence_tick.rs
printf '\n----\n'
sed -n '1110,1275p' src/shard/persistence_tick.rs
printf '\n----\n'
sed -n '1,120p' src/shard/segment_stall.rsRepository: pilotspace/moon
Length of output: 32760
Implement non-Unix directory-loss detection.
The #[cfg(not(unix))] stub always reports a healthy path, so query_free_bytes can never surface PathMissing and is_dir_lost() stays false on supported non-Unix targets. Add a portable metadata/is_dir() check and return PathMissing when --dir is missing or no longer a directory, or the persistence tick will keep retrying doomed filesystem work after the directory disappears.
🤖 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/disk_monitor.rs` around lines 393 - 397, Update the non-Unix
`query_free_bytes` stub to check the supplied path with portable metadata and
verify it is a directory; return `QueryError::PathMissing` when metadata fails
or the path is not a directory, otherwise preserve the existing unlimited
free-space result.
Comment on lines
117
to
+129
| if new_epoch > *last_snapshot_epoch && snapshot_state.is_none() { | ||
| *last_snapshot_epoch = new_epoch; | ||
| // #366: consume the epoch but skip the save while the data directory | ||
| // is missing — the snapshot temp file can't be created, and retrying | ||
| // per save-point trigger just spams one doomed attempt per epoch. | ||
| if crate::shard::disk_monitor::is_dir_lost() { | ||
| tracing::warn!( | ||
| "Shard {}: auto-save epoch {} skipped — data directory missing", | ||
| shard_id, | ||
| new_epoch | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not consume manual BGSAVE triggers through the auto-save skip path.
This watch channel drives both auto-save and BGSAVE. The code advances last_snapshot_epoch, then returns without creating snapshot state or completing/failing BGSAVE coordination, potentially leaving BGSAVE permanently in progress and bypassing its existing error path.
Distinguish trigger origin, or explicitly fail and complete manual BGSAVE while only consuming auto-save epochs.
🤖 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/persistence_tick.rs` around lines 117 - 129, Update the
epoch-handling logic around last_snapshot_epoch so the data-directory-missing
early return applies only to auto-save triggers. Preserve manual BGSAVE triggers
for their existing snapshot/error coordination path, or explicitly fail and
complete BGSAVE before returning; only advance last_snapshot_epoch without
snapshot state for auto-save epochs.
Comment on lines
+74
to
+84
| let out = std::process::Command::new("ps") | ||
| .args(["-o", "cputime=", "-p", &pid.to_string()]) | ||
| .output() | ||
| .expect("ps"); | ||
| let s = String::from_utf8_lossy(&out.stdout); | ||
| let s = s.trim().replace('-', ":"); // DD-HH:MM:SS → DD:HH:MM:SS | ||
| let mut secs = 0.0f64; | ||
| for field in s.split(':') { | ||
| secs = secs * 60.0 + field.parse::<f64>().unwrap_or(0.0); | ||
| } | ||
| (secs * 1000.0) as u64 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail the test when the CPU probe cannot be parsed.
A failed ps invocation or changed output format currently becomes 0ms, making the spin regression assertion pass falsely. Check out.status, require non-empty output, and reject malformed fields instead of using unwrap_or(0.0).
🤖 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/dir_deleted_degraded.rs` around lines 74 - 84, Make the CPU-time probe
fail explicitly when the ps command fails or produces empty output. In the
elapsed-time parsing block, validate out.status, require non-empty trimmed
stdout, and replace the unwrap_or(0.0) fallback with strict field parsing that
propagates or asserts on malformed values, preventing invalid output from
becoming 0ms.
…review round (B1, S1-class, S3, N1, N2) Adversarial review of the dir-lost latch found one blocker and three gaps; all fixed here, two skipped with reasons (PR thread). B1 (blocker) — shallow heal re-armed the original bug UNLATCHED: the heal check is statvfs on the top-level --dir only, but WalWriterV3 creates <dir>/shard-N/wal-v3/ exactly once at startup and open_new_segment() only `create(true)`s the LEAF file. An operator answering "MOONERR dirmissing" with `mkdir -p <dir>` cleared the latch while the nested WAL dir stayed missing — the first post-heal segment rotation then failed ENOENT on every 1ms tick, reproducing issue #366 with the latch already released. Two independent layers: - open_new_segment() now runs fs::create_dir_all(&wal_dir) first — rotation self-repairs a vanished parent (rotations are rare; one mkdir syscall is free). Unit-pinned by test_rotation_self_repairs_deleted_wal_dir (tiny 256-byte segment, delete the wal dir under an open writer, force rotation, assert it recreates the dir and lands the new segment). - flush_wal_v3_if_needed() arms a 1s backoff on ANY flush failure (flush_backing_off/note_flush_failure/clear_flush_backoff on WalWriterV3) — no future failure class (EACCES, quota, …) can loop at tick cadence. Healthy-path cost: one Option discriminant check. Unit-pinned by test_flush_backoff_arms_and_clears. S3 — disk-offload without AOF/save never armed the guard: spill and the vector warm tier write to the data dir even with appendonly=no. init_global now arms on persistence OR disk_offload_enabled(), and check_warm_transitions (10s cadence segment writer) gates on is_dir_lost() like the other tick functions. N1 — module doc no longer claims clean-unmount coverage (mountpoint dir survives an unmount; catching it needs an st_dev comparison — follow-up material). N2 — QueryError variants are only constructed under cfg(unix); added cfg_attr(not(unix), allow(dead_code)) so the main-push-only Windows Check job can't fail on the dead-variant lint. Skipped with reason: - S2 (explicit BGSAVE not fast-failed while latched): the existing finalize-error path already fails the one attempt cleanly and keeps the BGSAVE fan-in counter correct; a fast-fail reply would need its own counter accounting and risks the stuck-in-progress bug class. - S1 residual (expiry/eviction WAL appends growing the buffer during an outage): append-time flush errors predate this PR (same class as diskfull) and are now bounded by the 1s backoff; full write-path gating of internal ops is follow-up scope. author: Tin Dang <tindang.ht97@gmail.com>
Contributor
Author
|
Adversarial review round applied in 5b01285 (verdict was BLOCK on B1; all findings triaged):
Local gates on the amended head: fmt, clippy (default + tokio,jemalloc), 4371 lib tests, integration test green. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/persistence/wal_v3/segment.rs (1)
580-587: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
clear_flush_backoffby unconditionally assigningNone.Checking
is_some()introduces an unnecessary branch. Directly assigningNoneis cleaner and slightly more efficient.♻️ Proposed refactor
/// Clear the backoff after a successful flush. #[inline] pub fn clear_flush_backoff(&mut self) { - if self.flush_backoff_until.is_some() { - self.flush_backoff_until = None; - } + self.flush_backoff_until = None; }🤖 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/wal_v3/segment.rs` around lines 580 - 587, simplify clear_flush_backoff by directly assigning None to flush_backoff_until, removing the redundant is_some conditional while preserving its existing behavior.
🤖 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.
Nitpick comments:
In `@src/persistence/wal_v3/segment.rs`:
- Around line 580-587: simplify clear_flush_backoff by directly assigning None
to flush_backoff_until, removing the redundant is_some conditional while
preserving its existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9098232-815c-41f1-9e25-f54e85b11bf8
📒 Files selected for processing (5)
CHANGELOG.mdsrc/main.rssrc/persistence/wal_v3/segment.rssrc/shard/disk_monitor.rssrc/shard/persistence_tick.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main.rs
- src/shard/persistence_tick.rs
- CHANGELOG.md
- src/shard/disk_monitor.rs
Contributor
Author
|
B1 scenario proven end-to-end against the amended binary (2 shards,
Pre-B1-fix this step 3 would have ENOENT-looped the 1ms tick with the latch already cleared. Linux VM re-run on the amended head: 28 segment unit tests + integration test green. |
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 17, 2026
…ows Check red on main (#370) Windows Check (main-push-only, PR-invisible) failed 3/3 tries on test_dir_lost_latches_pauses_and_self_heals after #369 merged: the non-unix query_free_bytes stub always returns Ok, so the dir-lost latch never engages on Windows BY DESIGN — the test's dir_lost() assertion can't hold there. Gate the latch-behavior test with cfg(unix); the disarmed-guard sibling stays cross-platform (its expectations hold under the stub). Same Windows-gate treatment the integration suite already had (#![cfg(unix)]). author: Tin Dang <tindang.ht97@gmail.com> Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem (#366)
Deleting
--dirunder a running server left the per-shard persistence tick retrying its file operations on ENOENT forever — observed in the wild as an idle 4-shard server at ~667% CPU (sys:user ~50:1) with a wedged PING. Even in the mildest reproduction, every shard warn-logged the WAL-overflow scan once per second, forever.Fix — design-for-failure, mirroring the diskfull guard
DiskMonitor: the existing 5s statvfs poll now distinguishesENOENT/ENOTDIRon the monitored path (deterministic "the dir is gone") from transient errors (unchanged: retain previous state). Latch = ONE loudERRORlog.MOONERR dirmissingmessage (both dispatch runtimes); reads and PING keep serving.is_dir_lost()(one relaxed atomic load) — zero per-tick syscalls, zero per-tick log lines while latched. Manual BGSAVE still fails one attempt loudly through the existing finalize-error path (BGSAVE fan-in counter untouched).--disk-free-min-pct 0(poll_globalnow runs whenever either guard is active).Measured (incident config: 4 shards, appendonly yes)
rm -rf $dir+OKforever (silent data-loss risk)MOONERR dirmissingwithin one pollERRORlineTests
tests/dir_deleted_degraded.rspins refusal ≤ one poll, responsiveness while latched, quiesced log+CPU, and self-heal — failed at the refusal assertion pre-fix.disk_monitor.Notes
moon.lockflock is held on the unlinked inode, so double-start protection on the recreated dir is weakened until restart.Closes #366
Summary by CodeRabbit
MOONERR dirmissing; reads andPINGstay responsive.