fix(gateway): reserve launchd shutdown teardown headroom - #821
Merged
Merged
Conversation
Kyzcreig
force-pushed
the
wt/t_942691a4
branch
from
September 21, 2026 10:57
d424ed3 to
f075f2d
Compare
Collaborator
Author
FleetReviewReviewed with 2 of 3 model families — anthropic unavailable. Confidence: 2/5 Findings
FleetReview provenance · models: B=gpt-5.6-sol, D=grok-4.6 · cost: $0.00 · duration: 11m 08s · rounds: 1 · files examined: 7 |
Collaborator
Author
FleetReviewReviewed with 2 of 3 model families — anthropic unavailable. Confidence: 2/5 Findings
FleetReview provenance · models: B=gpt-5.6-sol, D=grok-4.6, F=gpt-5.6-sol, G=grok-4.6 · cost: $1.82 · duration: 13m 20s · rounds: 1 · files examined: 7 |
Kyzcreig
force-pushed
the
wt/t_942691a4
branch
from
September 21, 2026 15:16
f075f2d to
ba4cbfd
Compare
Measure and persist post-drain teardown time, cap launchd drains by at least 15 seconds or the prior measurement, and hard-exit before launchd's SIGKILL deadline. Deduplicate restart-loop boot accounting and label its reason class.\n\nVerified: scripts/run_tests.sh over focused restart/lifecycle suites (379 passed, 1 skipped), plus the per-boot dedup regression.
The drain cap sized the teardown window against launchd's SIGKILL wall, but the shutdown watchdog hard-exits LAUNCHD_HARD_EXIT_RESERVE_S earlier. The two reserves therefore overlapped: at clamp 60 a 45s drain left only 5s before os._exit for a teardown allocated 15s, and a recorded 22s teardown got 12s. A busy stop could hard-exit mid-persistence, which is the state.db corruption class this work exists to close. resolve_launchd_capped_drain now subtracts the hard-exit reserve as well, so the drain ends a full teardown reserve before the watchdog that actually fires. Measured (clamp/cfg/last_teardown -> drain, watchdog, teardown window): 60/50/None -> 35, 50, 15s; 60/50/22 -> 28, 50, 22s; 50/50/None -> 25, 40, 15s; 30/50/None -> 5, 20, 15s. SIGKILL slack stays 10s in every row. A live ExitTimeOut at or below the fixed reserve (e.g. 8s) previously produced a zero-second watchdog, hard-exiting the instant shutdown began and skipping all drain AND persistence. Short budgets now fall back to a proportional reserve: clamp 8 -> watchdog 6.0, clamp 4 -> 3.0. Verified: 7 new cases RED on f075f2d, GREEN after. Each arithmetic change mutation-checked independently -- reverting the additive cap reddens 6 cases, reverting the short-budget fallback reddens 2. Focused suites 47 passed / 1 skipped; broader shutdown+restart surface (10 files) 111 passed / 4 skipped. Ruff clean.
read_last_teardown_seconds validated only `value >= 0.0`, which `inf` satisfies. The value is read off disk at boot and flows straight into the teardown reserve, so a corrupt or hand-edited gateway.teardown.json made that reserve unbounded and silently drove the next shutdown's drain budget to zero (measured: last_teardown=inf -> drain 0.0). Rejecting non-finite input is input validation, not a minimum-drain floor -- the floor was explicitly withdrawn by review as contrary to the specified arithmetic, and this does not reintroduce it. A real completed teardown can never measure inf; finite values are untouched (22.0 still round-trips to 22.0). Tests exercise the real file path rather than the parameter, which was the gap called out in review. Mutation-checked: removing the isfinite guard reddens the Infinity case (NaN/-Infinity were already caught by the existing comparison). Focused suites: 51 passed, 1 skipped.
FleetReview round 3 on PR #821 raised three P1s against 8cead1e. P1 #1 "watchdog starves teardown" — REFUTED BY MEASUREMENT, with a regression test so it cannot become true silently. The premise is that the inner leash is "drain plus a small grace", so a 35s cap would arm a ~40s watchdog and leave persistence ~5s. DEFAULT_SHUTDOWN_WATCHDOG_GRACE_S is 60.0, not small, so under launchd the inner term never wins the min() and the armed deadline is clamp - LAUNCHD_HARD_EXIT_RESERVE_S. Measured at the production clamp (60): drain 35 -> armed 50 (window 15, need 15); drain 28 with a 22s record -> armed 50 (window 22, need 22). The P1's own proposed assertion passes by construction, both sides being clamp - 10. The arming arithmetic is extracted to resolve_armed_shutdown_watchdog_delay so the invariant is measured against the expression gateway.run really arms with, at both arming sites (live arm + diagnostic snapshot). test_stop_arms_the_watchdog_at_the_hard_exit_deadline drives the real stop() and reads the wall-clock delay handed to arm_shutdown_watchdog — not a call count, not re-derived arithmetic. The grace branch does bind on a clamp far above the drain (clamp > drain + 70); clamp=300 is pinned too, so shrinking the grace to the "small grace" the P1 assumed turns the production row red instead of flipping it quietly. P1 #3 "unbounded teardown reserve" — FIXED at the read/record boundary, not with a drain floor (Argus's floor withdrawal stands). Two bounds: * record_teardown_timing(budgeted=...) marks a stop that actually ran under a supervisor deadline. An unconstrained stop (hermes gateway stop, Ctrl+C, foreground) can legitimately take far longer than any launchd budget; reading that back as the reserve zeroes the next drain. Unbudgeted samples are still written for diagnostics but never read back. Legacy records with no provenance field are not trusted. * read_last_teardown_seconds(max_seconds=...) rejects a sample larger than resolve_max_actionable_teardown_reserve_s(clamp) — the window that exists before the hard exit. At clamp 60 a 55s record drove the drain to 0.0 and stayed poisoned if that stop hard-exited before recording a new sample; it now degrades to "no measurement" (drain 35.0) and is replaced by the next real measurement. P1 #2 "cron drain eats teardown" is DUPLICATE-OF t_f753b2b5 / PR #835 and is not fixed here; the stale `# 45` comment in the cron-leash test is corrected to `# 35` and cites the card. Also replaces test_stop_path_arms_via_the_shared_resolver, which read GatewayRunner.stop's source with inspect.getsource — banned outright by AGENTS.md ("Never read source code in tests") — with the behavioral stop()-driving test above. Verified: - Mutation-checked, each applied to source and reverted: arm from resolve_shutdown_watchdog_delay(drain, grace_s=5.0) -> armed 33.0 vs 50.0, 1 failed; drop the budgeted guard -> 2 failed; drop the max_seconds ceiling -> 2 failed. - Invariant sweep over clamp {None,1,4,8,10,20,30,45,50,60,90,120,300,600} x configured {5,20,30,50,180,600} x last_teardown {None,0,5,22,45,58, 1e9,inf} = 672 combos through the production boot read: 0 violations of window >= max(15, measured) wherever the drain is non-zero; the 240 residuals are all the specified nonnegative saturation (drain == 0); armed < clamp everywhere; non-launchd passthrough intact. - Focused: 46 + 18 pass. Broad shutdown/restart surface, 19 files: 383 passed, 0 failed, 4 skipped. - ruff clean on all changed files; git diff --check clean. Not verified and not claimed: the live safe-restart at host load >= 20 with 5 active turns ending in a clean exit status, the gateway-exit-diag teardown line on a real shutdown, merge-queue landing, deployment, the external fleet-config-lint headroom assertion, and the upstream PR.
Kyzcreig
force-pushed
the
wt/t_942691a4
branch
from
September 21, 2026 19:07
8cead1e to
ad9c323
Compare
Collaborator
Author
FleetReviewReviewed with 2 of 3 model families — openai unavailable. Confidence: 3/5 Findings
FleetReview provenance · models: C=claude-code-opus-5, D=grok-4.6, G=grok-4.6 · cost: $26.02 · duration: 28m 30s · rounds: 1 · files examined: 7 |
This was referenced Sep 21, 2026
Kyzcreig
added a commit
that referenced
this pull request
Sep 23, 2026
…not per resume scan The 2026-09-21 restart_loop.json entries at 02:52:02, 02:54:06 and 02:55:17 were not three gateway boots. The watchdog recorded PID 99910 continuously across that whole span (02:50:41 through 02:56:59). What actually happened is that _schedule_resume_pending_sessions calls restart_loop_guard.check_and_record whenever candidates exist, and the startup platform pass, the startup-wide pass and a Discord reconnect each recorded the SAME process as a fresh boot -- falsely tripping the threshold inside a single boot. Fix at the module layer: the guard now derives a stable per-process boot identity (pid + process-local monotonic import stamp, refreshed after a fork) and persists it alongside the timestamps as boot_ids. A repeated scan carrying an identity already in the ledger returns the existing chain without appending. That covers every caller of check_and_record, survives into the state file, and makes restart_loop.json forensically honest about what was one boot. Removes the per-runner _restart_loop_guard_recorded_this_boot flag added in #821. It lived on the GatewayRunner instance, so it only deduplicated scans routed through that one object and never reached the disk ledger. Keeping both mechanisms double-suppressed the scans: with the instance flag in place, the module dedupe could be deleted outright and the regression tests stayed green (measured -- mutation left both files passing). The guard now owns the dedupe and the caller invokes it unconditionally. #821's test is re-pointed rather than dropped: it previously mocked out check_and_record / is_restart_loop_tripped and asserted the call pattern, which could not observe the ledger at all. It now asserts the real observable -- one entry on disk after repeated scans. Verified: * scripts/run_tests.sh over tests/gateway/test_restart_cascade.py, tests/hermes_cli/test_gateway_restart_loop.py and tests/gateway/test_auto_continue_interrupted_turns.py: 431 passed, 0 failed, exit 0. * Mutation proof that both regression tests actually gate the bug: disabling the dedupe (`if False and identity in stored_ids`) turns test_resume_rescans_record_one_gateway_boot RED at `assert 3 == 1` and test_restart_loop_guard_records_at_most_once_per_gateway_boot RED at `assert 2 == 1`. * Cross-process control on the real default identity (no boot_id injection), 3 interpreters x 3 resume scans each: pid=71517 scans=[False,False,False] ledger=1 pid=71634 scans=[False,False,False] ledger=2 pid=71662 scans=[True,True,True] ledger=3 9 scans -> 3 entries, 3 unique boot ids, trips on the 3rd genuine boot. * ruff: All checks passed. git diff --check: clean.
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
1 participant
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
ExitTimeOut - max(15s, last measured post-drain teardown)state/gateway.teardown.jsonand appendgateway.shutdown_teardown_timingtogateway-exit-diag.logos._exitbyExitTimeOut - 10s, even when persistence/teardown wedgesIncident attribution
The supposed breaker boots 2 and 3 were not KeepAlive respawns. The one replacement process started at 02:51:34; its phased resume scanner recorded the same boot at 02:52:02, 02:54:06, and 02:55:17. The per-process dedup fixes that false chain.
Verification
git diff --check: cleanAcceptance note
The hardware safe-restart under load is intentionally left to post-review deployment: this PR changes shutdown semantics and must not be deployed from an unreviewed worker branch.