fix(server): zombie/CPU hardening wave 1 — busy-poll idle disengage, shard-panic fail-fast, SIGTERM matrix#216
Merged
Merged
Conversation
CI flake (Check job, shards=4): a connection accepted while the server
is still bringing up its per-shard SO_REUSEPORT listeners can be RESET
mid-read; wait_ready's retry loop only survived wrong ANSWERS — the
first PING read panicked inside read_line's expect ("read byte:
Connection reset by peer"). The readiness probe now uses a fallible
try_ping and retries with a FRESH connection on any I/O error or
non-PONG answer within the same 30s budget.
author: Tin Dang
…l x write-storm RSS/CPU/OOM review item 1 (zombie lens) flagged the detached monoio per-shard accept task as the suspected root cause of the documented SIGTERM+SO_REUSEPORT bench hangs. Before touching that path (its current shape deliberately dodges an io_uring cancel/resubmit race), the hang needs a reproducer — this extends the existing sigterm suite from 2 cases (shards=1 idle) to 7: - shards 1/4 idle, shards 1/4 with a held-open verified connection - shards=2 with --io-busy-poll-us 40 (the zombie-CPU-eater composition) - bench shape: 16 threads pipelining SETs, appendonly=yes, SIGTERM mid-storm, asserting clean exit code 0 within 10s Result at HEAD: 7/7 PASS on macOS kqueue, Linux io_uring, and Linux epoll (OrbStack, kernel 6.17) — the hang does NOT reproduce. The accept task structurally never observes the shutdown token (MEASURED), but runtime teardown collects it cleanly in every tested composition. Disposition: keep this matrix as the permanent regression guard; defer any accept-task change until a real reproducer exists (GCE pinned-core composition remains untested). The kill -9 backstops in bench harnesses stay until then. author: Tin Dang
…ervers RSS/CPU/OOM review item 2 (CPU lens): the poll-mode park (--io-busy-poll-us / MOON_EPOLL_SPIN_US, vendored monoio legacy driver) burned its full budget on EVERY park with zero connections and zero work, forever — an idle 4-shard server at 200us measured ~2.4s CPU per 3s wall (RED test), and an orphaned stray server becomes the documented "zombie CPU eater". The spin exists to delete wake latency UNDER TRAFFIC, so the driver now tracks the last real event per thread (readiness fd events, skip-notify probe hits, foreign-waker eventfd — NOT bare timer expiries) and skips the spin window once nothing has arrived for MOON_SPIN_IDLE_DISENGAGE_US (default 10ms; 0 restores the old always-spin behavior). The next event arrives via the normal wake path (one-time ~us penalty) and re-arms the spin, so steady request traffic never disengages — the GCE p=1 win path is structurally untouched (re-validation on pinned GCE cores queued with the next bench pass). Considered and rejected: gating on the shard's connection count — a conn-less shard still serves cross-shard SPSC work, and the skip-notify handshake relies on it spinning; a conn gate would re-create the multi-shard p=1 regression the spin was built to fix. Red/green: tests/busy_poll_idle.rs (Linux /proc CPU-time measurement, idle burn < 0.5s/3s bound + post-idle traffic recovery), RED-verified 2.4s burn before the patch; SIGTERM matrix unaffected (7/7 both VM drivers). author: Tin Dang
RSS/CPU/OOM review item 7 (zombie lens): shard threads are joined only at shutdown and the join result was discarded, so a shard panic during normal operation left an N-1-shard server listening and answering while every key owned by the dead shard was silently gone — worse than a crash for a database. - A panic hook (installed right after tracing init) aborts the whole process when the panicking thread is a `shard-*` thread; the default hook still runs first so the message + backtrace print. Connection/ aux/test threads keep normal unwind behavior. - The shutdown join loop now logs a panicked shard instead of swallowing it (belt-and-braces; the hook normally aborts first). - New `DEBUG PANIC` subcommand (Redis parity) panics the executing shard thread — the crash-handling test trigger. Red/green: tests/shard_panic_abort.rs (shards 1 and 4, DEBUG PANIC → whole process must die within 5s) — RED-verified as a live zombie on both cases before the hook; green on monoio AND tokio runtimes. author: Tin Dang
CHANGELOG [Unreleased] entry for the wave-1 PR + proper SAFETY comment form on the busy-poll test sysconf call (audit-unsafe format). 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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds shard-fail-fast panic handling, a ChangesZombie/CPU hardening wave 1
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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 |
PR Summary by QodoZombie CPU hardening: fail-fast shard panics + SIGTERM matrix + idle busy-poll tests
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
50 rules 1. libc::sysconf unsafe unapproved
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+79
to
+82
| // SAFETY: sysconf(_SC_CLK_TCK) reads a static kernel constant; it takes | ||
| // no pointers and touches no shared state. | ||
| let hz = unsafe { libc::sysconf(libc::_SC_CLK_TCK) } as f64; | ||
| (utime + stime) as f64 / hz |
There was a problem hiding this comment.
1. libc::sysconf unsafe unapproved 📘 Rule violation ≡ Correctness
The new test introduces an unsafe call to libc::sysconf that is not listed under the repo’s pre-approved unsafe patterns, so it must be avoided or explicitly approved per policy. Keeping unapproved unsafe blocks increases the UB audit surface and violates the unsafe governance requirements.
Agent Prompt
## Issue description
A new `unsafe { libc::sysconf(libc::_SC_CLK_TCK) }` call was added in an integration test. The unsafe policy requires new unsafe usage to match an approved pattern or be removed in favor of a safe alternative.
## Issue Context
`tests/busy_poll_idle.rs` computes CPU seconds from `/proc/<pid>/stat` and needs the system clock tick rate (HZ). The repo already depends on `nix` on Linux, which provides a safe `sysconf` wrapper.
## Fix Focus Areas
- tests/busy_poll_idle.rs[70-83]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
+29
| fn moon_binary() -> std::path::PathBuf { | ||
| std::path::PathBuf::from(env!("CARGO_BIN_EXE_moon")) | ||
| } |
There was a problem hiding this comment.
2. moon_binary() ignores moon_bin 📘 Rule violation ▣ Testability
New integration tests spawn the server binary via env!("CARGO_BIN_EXE_moon") instead of requiring
an explicit MOON_BIN setting. This violates the test harness rule and can cause CI/local runs to
use unintended binaries or paths.
Agent Prompt
## Issue description
Integration tests added in this PR start the server without requiring `MOON_BIN` to be explicitly set.
## Issue Context
The compliance requirement is that integration tests must explicitly use `MOON_BIN` for server binaries (and fail fast if it is missing) rather than relying on build-output discovery like `env!("CARGO_BIN_EXE_moon")`.
## Fix Focus Areas
- tests/busy_poll_idle.rs[27-29]
- tests/shard_panic_abort.rs[21-23]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+268
to
+274
| /// `DEBUG PANIC` — deliberately panic the executing shard thread (Redis | ||
| /// parity: a crash-handling test aid). The process-level panic policy | ||
| /// (fail-fast abort, installed in main) is what a client observes; the | ||
| /// return type exists only for the signature. | ||
| fn debug_panic() -> Frame { | ||
| panic!("DEBUG PANIC requested by client"); | ||
| } |
There was a problem hiding this comment.
3. Remote abort via debug panic 🐞 Bug ⛨ Security
DEBUG PANIC unconditionally panics on the shard thread, and the new global panic hook aborts the whole process for any shard-* panic, making it a client-triggerable full server outage when DEBUG is permitted. Under the default unrestricted ACL user, DEBUG is allowed, so this is reachable in default configurations.
Agent Prompt
## Issue description
`DEBUG PANIC` currently allows any client who can run `DEBUG` to crash the server (panic on shard thread → panic hook aborts process). This is an operational DoS risk in default/unrestricted ACL configurations.
## Issue Context
- `DEBUG PANIC` panics unconditionally.
- `main` installs a panic hook that aborts the whole process for `shard-*` thread panics.
- Default ACL user is unrestricted (AllAllowed / +@all), and `@all` includes `debug`.
## Fix Focus Areas
- Add an explicit runtime config/CLI flag (default **disabled**) required to enable `DEBUG PANIC`, and return an error when disabled.
- Alternatively (or additionally), compile-gate it behind a feature (e.g. `cfg(feature = "debug-panic")`) used only by tests.
- Update tests that rely on `DEBUG PANIC` to pass the enable flag / feature.
### Code references
- src/command/server_admin.rs[268-274]
- src/main.rs[96-116]
- src/acl/table.rs[44-62]
- src/acl/rules.rs[221-257]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/sigterm_shutdown.rs (1)
147-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHeld-connection setup leaks the server process on failure.
Unlike the readiness path (Lines 136-143), which kills and reaps
childbefore panicking, theHeldOpenarm panics via.expect(...)on connect/write/read without terminating the spawned server. A setup failure here leaves a livemoonprocess holdingport, which can cascade into flaky/failing subsequent tests — precisely the zombie/CI-hygiene problem this PR targets.Consider reaping the child on each failure (e.g., a small
bail(child, msg)helper that runschild.kill(); child.wait();then panics) so the held-conn arm cleans up like the readiness path does.🤖 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/sigterm_shutdown.rs` around lines 147 - 161, The HeldOpen branch in sigterm_shutdown.rs can panic on connect/write/read without cleaning up the spawned server process, unlike the readiness path. Update the held-connection setup around conn, ConnAtSigterm::HeldOpen, and the TcpStream operations to ensure child is killed and waited on before any panic/expect failure, ideally by using a small helper like bail(child, msg) or equivalent cleanup wrapper so failures do not leave a live moon process holding the port.tests/busy_poll_idle.rs (1)
85-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider pinning the disengage/spin env vars for deterministic runs.
Both tests depend on the process defaults
MOON_SPIN_IDLE_DISENGAGE_US=10000and an unsetMOON_EPOLL_SPIN_US. If a CI shell or developer env preseeds either (e.g.MOON_SPIN_IDLE_DISENGAGE_US=0to disable disengage, or a large window), test 1 will burn CPU and fail spuriously. Passing them explicitly on the spawned command's env makes the assertion self-contained.♻️ Example: pin env on spawn
let child = Command::new(moon_binary()) + .env_remove("MOON_EPOLL_SPIN_US") + .env("MOON_SPIN_IDLE_DISENGAGE_US", "10000") .args([Also applies to: 133-153
🤖 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/busy_poll_idle.rs` around lines 85 - 105, The busy-poll idle tests rely on ambient process defaults for spin/disengage timing, which can make them nondeterministic under different CI or developer environments. Update the spawned Moon process setup in the busy-poll test cases, including the `busy_poll_idle_server_does_not_burn_cpu` flow and the other affected test mentioned in the review, to explicitly set the `MOON_SPIN_IDLE_DISENGAGE_US` and `MOON_EPOLL_SPIN_US` environment variables on the `Command` before `spawn()`. This keeps the assertions self-contained and ensures the behavior of the test harness is stable regardless of the surrounding shell environment.src/command/server_admin.rs (1)
151-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit test coverage for the new
PANICclassification branch.
classify_debuggained aPANICbranch but there's no visible unit test assertingclassify_debug(b"PANIC")routes toDebugCall::Panic(a#[should_panic]unit test fordebug_panic()itself, or a targeted test ofclassify_debug, would suffice without needing the full integration-test process-abort path). As per coding guidelines, "New commands must have at least one unit test and one consistency test entry."🤖 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/command/server_admin.rs` around lines 151 - 207, Add unit test coverage for the new DEBUG PANIC path in classify_debug and/or debug_panic. The missing piece is an assertion that classify_debug with a PANIC subcommand returns DebugCall::Panic, with a #[should_panic] test for debug_panic() also acceptable; use the classify_debug and debug_panic symbols to locate the change and keep the test focused on this new branch.Source: Coding guidelines
src/main.rs (1)
96-118: 🚀 Performance & Scalability | 🔵 TrivialFail-fast scope limited to
shard-threads only.Solid, targeted fix for the described zombie-shard problem. Note that other critical background threads spawned in this file (
aof-writer,aof-writer-{sid},auto-save) are not covered by this hook — a panic there still unwinds normally and could leave the server serving traffic without durability/auto-save silently. If that's out of scope for wave 1 (per the PR title), consider tracking it as a follow-up wave.🤖 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/main.rs` around lines 96 - 118, The panic hook in main currently aborts only for threads whose names start with shard-, so other critical background threads created in this file can still unwind silently and leave the server in an unsafe state. Update the hook logic around std::panic::set_hook in main.rs to also cover the durability-related thread names used here, such as aof-writer, aof-writer-{sid}, and auto-save, while keeping the existing default_hook behavior and abort message. Use the existing thread-name check in the hook to broaden the fail-fast scope without affecting unrelated threads.
🤖 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/command/server_admin.rs`:
- Around line 151-207: Add unit test coverage for the new DEBUG PANIC path in
classify_debug and/or debug_panic. The missing piece is an assertion that
classify_debug with a PANIC subcommand returns DebugCall::Panic, with a
#[should_panic] test for debug_panic() also acceptable; use the classify_debug
and debug_panic symbols to locate the change and keep the test focused on this
new branch.
In `@src/main.rs`:
- Around line 96-118: The panic hook in main currently aborts only for threads
whose names start with shard-, so other critical background threads created in
this file can still unwind silently and leave the server in an unsafe state.
Update the hook logic around std::panic::set_hook in main.rs to also cover the
durability-related thread names used here, such as aof-writer, aof-writer-{sid},
and auto-save, while keeping the existing default_hook behavior and abort
message. Use the existing thread-name check in the hook to broaden the fail-fast
scope without affecting unrelated threads.
In `@tests/busy_poll_idle.rs`:
- Around line 85-105: The busy-poll idle tests rely on ambient process defaults
for spin/disengage timing, which can make them nondeterministic under different
CI or developer environments. Update the spawned Moon process setup in the
busy-poll test cases, including the `busy_poll_idle_server_does_not_burn_cpu`
flow and the other affected test mentioned in the review, to explicitly set the
`MOON_SPIN_IDLE_DISENGAGE_US` and `MOON_EPOLL_SPIN_US` environment variables on
the `Command` before `spawn()`. This keeps the assertions self-contained and
ensures the behavior of the test harness is stable regardless of the surrounding
shell environment.
In `@tests/sigterm_shutdown.rs`:
- Around line 147-161: The HeldOpen branch in sigterm_shutdown.rs can panic on
connect/write/read without cleaning up the spawned server process, unlike the
readiness path. Update the held-connection setup around conn,
ConnAtSigterm::HeldOpen, and the TcpStream operations to ensure child is killed
and waited on before any panic/expect failure, ideally by using a small helper
like bail(child, msg) or equivalent cleanup wrapper so failures do not leave a
live moon process holding the port.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73587799-a4ac-4d20-9408-edba66720bf3
📒 Files selected for processing (8)
CHANGELOG.mdsrc/command/server_admin.rssrc/main.rstests/busy_poll_idle.rstests/shard_panic_abort.rstests/sigterm_shutdown.rstests/vector_del_unindex.rsvendor/monoio/src/driver/legacy/mod.rs
Under the tokio runtime --io-busy-poll-us is an explicit no-op (main.rs warns and skips the spin wiring), so the CI tokio Check job was measuring a debug-build tokio server's unrelated idle-tick cost (0.94s/3s at 4 shards) against a bound written for spin burn. The test now compiles only under runtime-monoio, where the vendored legacy driver spin it validates actually exists. Follow-up noted: tokio idle cost itself is worth a look in the hygiene batch. author: Tin Dang
This was referenced Jul 6, 2026
Merged
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
Wave 1 of the RSS/CPU/OOM review fixes (
tmp/RSS-CPU-REVIEW.md, items 1, 2, 7) — the "zombie killer" bundle: an idle server must cost ~zero CPU, a broken server must die loudly, and SIGTERM behavior is now pinned by a regression matrix.Busy-poll idle disengage (review item 2, HIGH)
--io-busy-poll-usburned its full spin budget on EVERY park with zero connections and zero work, forever — an idle 4-shard server at 200µs measured ~2.4s CPU per 3s wall (RED test); an orphaned stray server is the documented "zombie CPU eater". The vendored monoio legacy driver now tracks the last real event per thread (readiness events, skip-notify probe hits, foreign-waker eventfd — NOT bare timer expiries) and skips the spin window afterMOON_SPIN_IDLE_DISENGAGE_US(default 10ms;0= old always-spin) of quiet. The next event re-arms it via the normal wake path.--io-busy-poll-us 40, ARM 1.19–1.21× / x86 1.65–1.66× vs Redis) is structurally untouched; worst case is one normal-wake latency on the first request after a >10ms idle gap. A pinned-core GCE re-validation is queued with the next bench pass.tests/busy_poll_idle.rs(Linux/procCPU-time measurement, idle burn <0.5s/3s + post-idle traffic recovery), RED-verified at 2.4s before the patch.Shard-panic fail-fast (review item 7, MED)
Shard threads are joined only at shutdown and the join result was discarded — a shard panic during normal operation left an N-1-shard server silently answering while every key owned by the dead shard was gone. Now:
shard-*thread panics (default hook runs first, so message + backtrace still print; other threads keep unwind semantics).DEBUG PANICsubcommand (Redis parity) as the crash-handling test trigger.tests/shard_panic_abort.rs(shards 1 and 4) — RED-verified as a live zombie on both runtimes before the hook.SIGTERM regression matrix (review item 1, HIGH — non-repro documented)
The review flagged the detached monoio per-shard accept task (never observes the shutdown token) as the suspected root of the documented SIGTERM+SO_REUSEPORT bench hangs. Before touching that path (its shape deliberately dodges an io_uring cancel/resubmit race), the hang needed a reproducer. The suite grew from 2 cases to 7: shards 1/4 × idle/held-open connections, busy-poll composition, and the bench shape (16 threads pipelining SETs,
appendonly yes, SIGTERM mid-storm).Result: 7/7 PASS on macOS kqueue, Linux io_uring, and Linux epoll — the hang does not reproduce at HEAD. Disposition: the matrix stays as the permanent guard; the accept-task change is deferred until a real reproducer exists (GCE pinned-core composition untested). Bench-harness
kill -9backstops stay until then.Test-harness hardening
wait_ready-style readiness probes (vector_del_unindex + the sigterm suite) retry with a fresh connection on mid-startup connection resets — kills the CI flake where per-shard SO_REUSEPORT listeners accept-then-reset during init (Checkjob, "read byte: Connection reset by peer").Validation
-D warnings(default ANDruntime-tokio,jemalloc) clean; unsafe/unwrap audits pass.Notes for reviewers
vendor/monoio/src/driver/legacy/mod.rs(thread-local state + 4 call sites ininner_park), zero host-side coupling.DEBUG PANICexecutes on whichever shard thread runs the command — at shards=1 that's the only shard; the fail-fast hook is what a client observes.author: Tin Dang
Summary by CodeRabbit
New Features
DEBUG PANICcommand to trigger fail-fast panic behavior.Bug Fixes
Tests