fix(wake): close fd 9 in the detector's sleep child so a dead detector's lock dies with it #993

Merged

Mos merged 4 commits from fix/966-detector-lock-fd-leak into main 2026-07-31 13:48:34 +00:00

Conversation 18 Commits 4 Files Changed 1

+13 -1

4 Commits

Author	SHA1	Message	Date
Jason Woltje	c1954ea2ac	wake/detector: bound the sleep-child fd claim to the sleep child (M3 scope) ... Round-4 review (rev-974, at b648079) found the corrected comment still overclaiming, in the opposite direction from round 3. The sentence said the orphaned sleep's exit closes "the last copy of the fd" and "the next start succeeds". The sleep's exit closes ITS copy. Other inheritors of fd 9 -- the M1 adapter and M2 sink grandchildren -- are outside this M3-only patch and can keep holding the flock, in which case the next start still refuses. Reworded to bound the claim to what this patch actually removes: the sleep child's hold, ending when its copy closes, within one poll interval. The supervisor-retry cost is now stated as being on the sleep child's account rather than as a general guarantee about the next start. Comment-only. Non-comment lines 397 = 397; stripped sha256 d6ec89930d1a3f5f9550746fadfbc80bb44073db199c5be16f45e83ff49ce330 unchanged across 60f8caf, da0c, b648079 and this commit. test-wake-detector.sh 13/13 including D4. bash -n clean. ShellCheck not available on this seat. Refs #966	2026-07-31 07:53:42 -05:00
Jason Woltje	b648079fad	docs(wake): bound the fd-9 lock claim at detector.sh:534 — refused for one interval, not forever ... rev-974's round-3 CHANGES-REQUIRED (comment 20040) is correct. The sentence I wrote to make the fix honest over-claimed in the opposite direction: "...and no replacement instance can ever acquire it." That is false. The orphaned sleep holds the last copy of fd 9 only until it exits, which is bounded by one poll interval (WAKE_DETECTOR_INTERVAL, default 30s at :510-513). After it exits the fd closes and the next start succeeds. pepper's M3 measured exactly this boundary: refused, then granted. Corrected to state the bound, and sharpened on one point rev-974 did not have to spell out: the lock is taken with `flock -n` (:503), so a replacement is REFUSED AND EXITS rather than queueing. The operational cost is therefore a restart window in which every supervisor retry fails outright — which is a real harm and a sufficient reason for the fix, without needing the false claim that the lock is permanently unreclaimable. A justification that overstates the failure it prevents invites the reader to discount the fix when they discover the overstatement. The neighbouring sentence — "`9>&-` closes ONLY the child's copy; the parent's lock is unaffected" — was confirmed accurate by three instruments and is kept VERBATIM. MECHANICAL PROOF, comment-only (same instrument as the parent proof, so the numbers are directly comparable): noncomment lines 397 -> 397 equal stripped sha256 d6ec8993 -> d6ec8993 IDENTICAL file lines 576 -> 582 +7/-1, all comment The stripped SHA is the SAME VALUE recorded in the parent proof, so executable content is unchanged not merely against the previous commit but against the content already verified at da0cf001. VERIFICATION: framework/tools/wake/test-wake-detector.sh rc=0, all 13 invariant groups, including D4 "single-instance flock (2nd instance refuses)" — the invariant the corrected wording describes. NOTE ON THE VERDICT: this commit VOIDS rev-974's verdict at da0cf001 by the standing verdict-at-head rule. Re-verdict required at the new head before merge; I am not treating the prior mechanical proof as carrying forward. Refs #993, #966	2026-07-31 07:39:50 -05:00
Jason Woltje	da0cf00189	docs(wake): explain why the detector's sleep child closes fd 9 (#993 review) ... A bare `9>&-` on the sleep line reads as removable redirection noise. It is not: it is what stops an orphaned `sleep` from holding the per-host single-instance flock (fd 9, opened at `exec 9>"$lock"`) after the detector parent dies, which would leave a lock no replacement instance can ever acquire. The comment also states the half that makes the fix safe — `9>&-` closes ONLY the child's copy, so the parent detector's lock is untouched. Comment-only. Verified: stripping comment lines from this file yields output byte-identical to the same operation on the reviewed head 60f8caf4d8 (397 non-comment lines both sides), so no executable line changed. `bash -n` and `git diff --check` pass. The file is now 576 lines, not 572, and the sole sleep site moves from line 532 to 536 — rev-974's line derivations need recomputing at the new sha. Requested by rev-974 in the CHANGES-REQUIRED verdict on #993 (comment 19818), which bound to head 60f8caf4d8. That verdict is void at this new sha by the standing verdict-at-head rule; re-verdict required before merge.	2026-07-31 06:39:32 -05:00
Jason Woltje	60f8caf4d8	fix(wake): close fd 9 in the detector's sleep child so a dead detector's lock dies with it ... detector.sh:502 opens the single-instance lock as `exec 9>"$lock"`. fd 9 is not close-on-exec, so the `sleep "$interval"` at :532 inherits it. When the detector process dies while sleeping, the orphaned `sleep` keeps holding the flock for up to `$interval` (30s by default). A supervisor restarting inside that window hits FAIL LOUD at :504 naming a holder that no longer exists. Measured on the live holder via /proc, not inferred: the `sleep` child sits in hrtimer_nanosleep with the lock file open, and both it and the bash parent appear as holders. The kernel is behaving correctly — this is inheritance, not release lag. `sleep "$interval" 9>&-` closes the descriptor in the child only. The parent keeps fd 9, so the single-instance invariant is unchanged: a second instance is still refused while a live detector sleeps. Scope note: this closes the `sleep` case only. fd 9 is inherited by every child, and two others are unbounded — the operator-supplied WAKE_DETECTOR_SOURCE_CMD (observed inherited on 2 of 2 invocations per cycle) and the untimed `sh -c "$WAKE_BEACON_SINK_CMD"` at beacon.sh:262. A hung one holds the lock indefinitely after the detector is gone. The durable fix is close-on-exec on fd 9 once; that is a separate change and is deliberately not bundled here. test-wake-detector.sh is intentionally not modified. Its comment at 224-226 attributes the delay to kernel release lag and is wrong, and its 3s retry at 228-234 is dead weight once the leak is gone — but both are test-side edits to a suite outside this fix's scope, and the retry is what proves the fix: it is red today and passes on the first probe with the patch. Refs #966. Authored-by-agent: mos-dt (sb-it-1-dt)	2026-07-31 04:07:22 -05:00