test(loading): kill the server on an unwinding panic, not on the last line - #714
test(loading): kill the server on an unwinding panic, not on the last line#714TinDang97 wants to merge 1 commit into
Conversation
`register_warm_segments` attributes a recovered warm segment to its owning index by reading exactly one file — mvcc.mpf, via `peek_key_hashes`. When that file is missing there is no evidence, no owner can be chosen, and the directory was left on disk with a warning. Nothing ever removed it, so every subsequent restart re-read it and re-warned about it. Reproduced by stripping mvcc.mpf from 6 of 12 warm segments and restarting three times: 6 warnings each restart, 6/6 directories still present, DBSIZE unchanged at 2400. The warning was permanent and the disk cost with it. On the store that prompted moon#546 this was 897 directories. Such a directory is what a crash mid-creation, or a GC pass that emptied a directory without removing it, leaves behind. It can never be attached to an index, and whatever keys it held are recovered by the keyspace rescan regardless. It is now removed, counted, and reported as `retired as orphans with no mvcc.mpf` in the startup summary. Once the directory is gone the existing #546a pass retires its manifest entry on the next boot, so the sequence converges instead of trading one permanent warning for another. Retirement is gated strictly on ErrorKind::NotFound. Every other IO error keeps the previous warn-and-leave behavior: a transient EIO or a permissions problem can sit over perfectly good vectors, and deleting on those would turn a recoverable blip into data loss. A present-but-unparseable mvcc.mpf — truncated or corrupt — does not error at all; it mmaps and parses to zero ids (measured), so it takes the existing no-owner path and is likewise kept. Three tests, all mutation-checked. The retirement test was red before the fix. The "must not delete on any other error" test uses a directory named mvcc.mpf, because that is what actually produces a non-NotFound error — an earlier draft used a zero-length file, and widening the gate to `if true` still passed it, which is how the vacuity was caught. With the real fixture, widening the gate fails it. Refs: #546 author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
|
Closing: this PR was opened from the wrong head branch. Its head is Reopened correctly as the PR from |
1 participant
Fixes the leak reported in #713 for the one file that caused it.
What happened
tests/loading_state_476.rskilled its server on the last line of each test body:After an afternoon of deliberately-failing mutation runs of this file, five servers were left reparented to init:
834% CPU combined, for 7.5 hours. They answered nothing on their ports and spent the time in syscalls — one thread showed
13:42system against0:09user. Their data dirs still existed, so this is not the dirless spin of #369.The wasted cores are the smaller half of the cost. Several
ci-localruns and a stack of wall-clock-sensitive tests executed on a machine carrying a third of a load that nothing in their output disclosed.The fix
The child is owned by a
ServerGuardthat kills and reaps onDrop, so an unwinding panic takes the server with it.kill_now()remains for the one site that needs the store closed rather than merely doomed, before it is reopened.The regression test
lst476_a_panicking_test_does_not_orphan_its_serverpanics on purpose insidecatch_unwindand 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 — that distinction is what makes the test non-vacuous.Verified load-bearing rather than assumed: replacing the guard with
mem::forgetfails it withScope
This PR fixes one file. #713 tracks the other 21 files that call
.kill()without aDropguard, and suggests promoting a single guard intotests/common/mod.rsso the correct thing is also the default thing. (117 of 122 files already guard correctly.)