test(loading): kill the server on an unwinding panic, not on the last line - #717
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe loading-state test now owns spawned servers through ChangesLoading-state server cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change is localized to test cleanup behavior and documentation, with no actionable merge-blocking risk remaining beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, fix, regression test, platform scope, and broader impact. However, it does not follow the repository template because it omits the required Summary, Checklist, Performance Impact, and Notes headings and content. Resolution Update the description to include all template sections. Add the required checklist with current results, state the performance impact or write "None", and include any relevant notes. Preserve the existing technical explanation under the Summary or Notes sections. Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 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 |
Merge-bar recordSame override as #715 and for the same measured reason: the self-hosted runner is offline Covered instead by a full Remaining gap, stated plainly: no io_uring coverage. macOS ran monoio on kqueue. |
6045d50 to
ce07829
Compare
… line `tests/loading_state_476.rs` called `kill(child)` at the end of each test body. A failing `assert!` unwinds past that line, so the server was reparented to init and left running. This is not hypothetical. After a round of deliberately failing mutation runs of this file, five orphans were left behind: ~170% CPU each, 834% combined, for 7.5 hours. They answered nothing on their ports and spent the time in syscalls (13:42 system time vs 0:09 user). The wasted cores are the smaller half of the cost — every wall-clock-sensitive test run afterwards measured a machine under a third of a load it could not see. The child is now owned by a `ServerGuard` that kills and reaps on drop. `kill_now()` stays for the one place that needs the store CLOSED rather than merely doomed, before it is reopened. The regression test panics on purpose inside `catch_unwind` and then asserts on the PID, not the port: a dead server frees its port either way, so a connect-refused would prove nothing about whether anything is still running. Verified load-bearing — swapping the guard for `mem::forget` fails it with "server pid N survived a panicking test". Refs: #476 author: Tin Dang
`Check (Windows)` runs `cargo nextest run`, so this suite runs there too,
and `Command::new("ps")` cannot spawn on Windows. The liveness probe read
that spawn failure as "not alive" via `.unwrap_or(false)` — the assertion
would have passed on Windows without checking anything, which is the exact
shape of guarantee this test exists to refute.
Two changes:
* `#[cfg(unix)]` on the test — the guarantee is only worth what its
platform-specific primitive is worth, so it declares the platform
instead of quietly degrading on the others.
* the probe now `expect`s, so a `ps` that cannot RUN is an error rather
than a green test.
Both mutations fail as they must:
* probe binary renamed -> panics with "`ps` must be available to probe
process liveness: NotFound" instead of passing
* `mem::forget(guard)` -> "server pid 4554 survived a panicking test"
author: Tin Dang
ce07829 to
6390645
Compare
|
The earlier Note also: moon-dev has since been rebuilt and its runner is back online, so |
… from the last line of a passing test (#720) * test(harness): own the server through Drop in the 14 spawn_listening suites moon#713. A test that fails must not leave its server running. The kill sits on the last line of the test body, and a failing assert unwinds straight past it — measured on tests/loading_state_476.rs (fixed in #717): five orphans, ~170% CPU each, 834% combined, for 7.5 hours, answering nothing on their ports and spending the time in syscalls. That file was not special. 26 integration suites spawn a real moon and none of them owned the child through a Drop. Adds `common::ServerGuard` and `common::spawn_listening_guarded`, so the guarantee is written once and tested once rather than re-argued per file, and converts the 14 suites that already go through `common::spawn_listening`. The guard's shape is dictated by what these suites actually do: * `as_mut()` — crash and shutdown suites need the real Child for try_wait, stdio and custom readiness loops. * `kill_now()` is idempotent — crash-recovery suites SIGKILL on purpose and then restart, so "already reaped" is normal, and a second reap must not aim a kill at a pid the OS may have recycled. * `take()` — a suite that genuinely needs the Child back takes the duty to reap with it; the guard keeps only the pid, for assertions. Restarted servers are guarded too, not just the first spawn: a second server orphans exactly as well as the first. tests/server_guard_contract.rs pins all three behaviours against a real moon process, asserting on the PID rather than the port — a dead server's port frees up either way, so connection-refused proves nothing about whether anything is still running. Unix-only, deliberately: Windows runs this suite and `Command::new("ps")` cannot spawn there, so a probe that failed to RUN would report "not alive" and pass vacuously. Verification: - 14 converted suites + the contract: 31 passed, 0 failed; plus the 12 #[ignore]d crash/restart tests run explicitly, 12 passed, 0 failed. - Mutations, each failing its test: mem::forget(guard) and a no-op Drop both leave the server alive ("survived a panicking test"); a take() that does not transfer ownership trips sg3. - The two leak mutations left real orphans on the machine, which is the bug demonstrating itself. author: Tin Dang * test(harness): guard the remaining 12 direct-spawn suites, delete dead sigkill copies Batch 1 converted the 14 suites that spawn through `common::spawn_listening`. These 12 build their own `Command::spawn()` chain, so they get `ServerGuard::new` wrapped around the spawn expression directly: aof_fsync_err_subscribe_ordering, aof_multidb_kill9, aof_toplevel_multishard_refusal, cold_shadow_overwrite_resurrection, cold_tier_observability, crash_matrix_per_shard_bgrewriteaof, crash_recovery_cold_del_resurrection, crash_recovery_disk_offload_no_aof, crash_recovery_orphan_sweep_readiness, instance_lock, jepsen_lite, wal_group_commit That completes the set: 26 of 26 suites that spawn a real moon now reap it from `Drop`, on the unwind path as well as the success path. Seven file-local `fn sigkill(child: &mut Child)` copies are now dead — `ServerGuard::kill_now` supersedes them — and are deleted along with the `use std::process::Child` imports they were the last user of. `aof_fsync_err_subscribe_ordering` was already reaping correctly via `catch_unwind`/`resume_unwind`; it is converted anyway so the pattern is uniform, and its hand-rolled kill+wait pair collapses to `kill_now()`. Verified against a freshly built binary pinned via MOON_BIN, all 27 suites (26 converted + the contract suite): 44 passed / 0 failed in the normal run, 34 passed / 0 failed under `-- --ignored` (the crash and restart tests, which is where an orphan would actually be produced). `pgrep -f target/release/moon` is 0 before and 0 after both runs. `cargo check --all-targets` is clean, no warnings. Refs #713 author: Tin Dang
1 participant
Replaces #714, which I opened from the wrong head branch (it showed #712's already-merged diff).
What happened
tests/loading_state_476.rsput itskill(child)on the last line of the test body. A failingassert!unwinds straight past it, so the server is reparented to init. After one afternoon ofdeliberately failing mutation runs of this file:
five orphans, ~170% CPU each — 834% combined — for 7.5 hours, answering nothing on their
ports and spending the time in syscalls (13:42 system vs 0:09 user on one thread). The cost is not
only the cores: every wall-clock-sensitive test that ran afterwards ran on a machine under
invisible load.
Fix
A
ServerGuard(Option<Child>)owns the child and kills it inDrop, so an unwinding panic reapsit.
kill_now()consumes the guard where the test needs the server gone rather than merelydoomed (the store must be closed before it is reopened);
terminate()is idempotent viaOption::take, so the explicit kill and the drop cannot double-kill.The regression test, and why it checks the pid
It panics deliberately inside
catch_unwindand then asserts on the process, not the port —a dead server's port frees up either way, so a connect-refused proves nothing about whether
anything is still running.
#[cfg(unix)], deliberately:Check (Windows)runscargo nextest run,Command::new("ps")cannot spawn there, and the first version read that spawn failure as "not alive" through
.unwrap_or(false)— it would have passed on Windows without checking anything, which is theexact shape of guarantee this test exists to refute. The probe now
expects, so apsthatcannot run is an error rather than a green test.
Mutations (each fails as it must)
mem::forget(guard)server pid 4554 survived a panicking test✗`ps` must be available to probe process liveness: NotFound✗Wider exposure
Measured across the suite: 147 test files spawn a server, 30 contain no
impl Dropat all —tracked in #713 (the count there is corrected, with the method; two earlier numbers were wrong).
Summary by CodeRabbit
Bug Fixes
Documentation