Conversation
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.
…n deadline
Cron's shutdown allowance ignored the adaptive teardown reserve. The chat
drain shrinks by max(15s, last measured teardown), but
resolve_cron_drain_budget subtracted only the fixed 10s
CRON_DRAIN_CLEANUP_RESERVE_S and was never given last_teardown_s, so
cron's deadline stayed pinned at clamp-10 no matter what teardown the host
had measured:
last_teardown | chat_drain | cron_due | left before SIGKILL | needed
None | 45.0 | 50.0 | 10.0 | 15.0
22.0 | 38.0 | 50.0 | 10.0 | 22.0
58.0 | 2.0 | 50.0 | 10.0 | 58.0
_drain_active_agents._still_draining() returns True on the cron branch
independently of the chat deadline, so in-flight cron work genuinely held
the drain open to cron_deadline after the shortened chat drain expired —
leaving less time before launchd's SIGKILL than the teardown the machine
had demonstrated it needs.
Both allowances now come off ONE absolute deadline. New
resolve_launchd_drain_deadline_s() returns the armed hard-exit wall less
max(cleanup_reserve, last_teardown_s); resolve_launchd_capped_drain is
re-expressed in terms of it (behaviour unchanged), resolve_cron_drain_budget
takes it as deadline_s, and the run.py call site threads it in instead of
min()-ing against the RAW clamp (60, not 60-10-teardown) which was never
the binding term.
Preserved: the cron floor still only EXTENDS the wait for an operator's
long restart_drain_timeout, and zero-drain saturation stays a real zero —
no minimum-drain floor is introduced anywhere.
Verified (worktree pinned, .venv py3.11):
- new tests/gateway/test_cron_drain_teardown_reserve.py, 23 tests, incl. a
hermetic stop-path regression that drives the real GatewayRunner.stop()
with active cron work AND a live launchd clamp, asserting the COMPOSED
budget leaves >= the measured reserve before the hard exit.
- 150 passed / 0 failed / 2 skipped across 14 shutdown/restart/cron files.
- Mutation-checked, each independently: drop deadline_s at the run.py call
site -> 6 red; stop threading last_teardown_s -> 5 red; make
resolve_cron_drain_budget ignore deadline_s -> 13 red. Source restored
clean after each.
- Reconciled the pre-existing
test_cron_leash_under_launchd_cannot_exceed_exit_timeout, which asserted
the leash-only allowance independently of the armed watchdog and was
green throughout the defect: scoped its docstring and added the
composed-is-strictly-tighter assertion.
- ruff clean.
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: $16.19 · duration: 28m 26s · rounds: 2 · files examined: 4 |
Kyzcreig
added a commit
that referenced
this pull request
Sep 21, 2026
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
This was referenced Sep 21, 2026
Collaborator
Author
|
Superseded (boil-ocean 09-25, Apollo-verified on fork/main af43fd6): launchd teardown reserve landed as d6897c6 (#821); non-finite persisted teardown guard is lifecycle_ledger.py:135 math.isfinite; the cron-drain-from-shared-deadline defect was reworked into resolve_cron_drain_budget(deadline_s=...) via 466734c/28d47a6 (#838/#861). Card t_f753b2b5 is done. Rebase would mean re-implementing this PR's API on top of #861's single-deadline design. |
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.
🔴 STACKED ON #821 — base is
wt/t_942691a4. Merge #821 FIRST. This PR builds onresolve_launchd_capped_drain'slast_teardown_sparameter, which arrives in #821.Carved off Argus's round-3 review of #821 (kanban
t_f753b2b5). PRE-EXISTING behavior, byte-identical ond13c5f25ca— not a regression from #821.The defect
#821 made the CHAT drain adaptive: it shrinks by
max(15s, last_teardown_s). Cron's allowance was not given the same input.resolve_cron_drain_budgetsubtracted only the fixedCRON_DRAIN_CLEANUP_RESERVE_S = 10.0and never receivedlast_teardown_s, so cron's deadline stayed pinned atclamp - 10regardless of what teardown the host had actually measured.Measured against the real helpers (clamp 60, configured chat drain 50, cron_cfg 600, elapsed 1s):
The chat drain correctly shrinks while
cron_duenever moves off 50.Mechanism, not arithmetic-only.
_drain_active_agents._still_draining()returnsTrueon the cron branch independently of the chatdeadline:so in-flight cron work genuinely holds the drain open to
cron_deadlineafter the shortened chat drain has expired — leaving less time before launchd's SIGKILL than the teardown the machine demonstrated it needs.The fix
Both allowances are now derived from ONE absolute deadline.
resolve_launchd_drain_deadline_s()— the armed hard-exit wall lessmax(cleanup_reserve_s, last_teardown_s).resolve_launchd_capped_drainre-expressed in terms of it (behaviour unchanged; it was already computing this inline).resolve_cron_drain_budgettakes it asdeadline_s.gateway/run.pycall site threads it through instead ofmin()-ing against the raw clamp (60, not 60−10−teardown), which was never the binding term.Preserved deliberately (both withdrawn twice on the parent card as contrary to the specified arithmetic):
restart_drain_timeout.Verification
Run from the worktree with the repo pinned on
sys.path,.venvpy3.11.tests/gateway/test_cron_drain_teardown_reserve.py— 23 tests, including a hermetic stop-path regression that drives the realGatewayRunner.stop()with active cron work AND a live launchd clamp, asserting the composed budget leaves >= the measured reserve before the armed hard exit. Parametrized over the whole teardown table above.deadline_sat therun.pycall site → 6 redlast_teardown_sinto the deadline → 5 redresolve_cron_drain_budgetignoredeadline_s→ 13 redruff checkclean on all four touched files.Reconciled, not trusted
test_cron_leash_under_launchd_cannot_exceed_exit_timeout(the pre-existing leash test) asserted the allowance independently of the armed watchdog and was green throughout the entire defect — its<= 60.0is not the safety property. Its docstring is now scoped to say so, and it carries an assertion that the composed (deadline-threaded) budget is strictly tighter than the leash-only one.Not verified / not claimed
gateway-exit-diag.logteardown line observed.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.