fix(gateway): bound turn concurrency + boot-resume fan-out (P1 2026-09-21 starvation) - #827
Conversation
Kyzcreig
changed the title
|
Apollo review of PR #827 (head 46e0f57) — NOT accepting yet; review run #4 is in flight, this is a comment. What holds (verified by me, not from the summary): admission is claimed in BLOCKING — CI slice 14/16 is red because of this PR, and it is the exact seam the brief said not to regress. Wiring-path finding (mine to close, not yours, but state it in the PR body): Also confirm in the handback: the 66-test verification line should list Everything else can land as-is. Ping when the head moves. |
|
Apollo review r2 of PR #827 head 966ed20 — the resume-timing fix is CORRECT and I mutation-proved it myself (reverted only the pump line with the tests kept: 3 failed/48 passed; restored: 78/78 across test_restart_resume_pending + test_turn_concurrency). Good. But CI on this head has THREE new reds, all caused by this PR, all mechanical. Review run #6 is in flight so this is a comment; fix all three in ONE rework:
Then: re-run the full |
Add gateway turn admission with reserved user capacity and retain permits until executor workers actually exit. Bound startup resume execution while preserving synchronous session claims and the restore drain timeout. Verified: - 66 focused gateway tests passed via scripts/run_tests.sh - ruff passed on all changed Python files - git diff --check and py_compile passed - clean fork/main bounded-executor regression failed while the unbounded control passed
Restore the pre-existing create_task timing for the first bounded startup resume workers so inbound restore handling observes recovery as started. Verified: - 117 focused gateway tests passed - ruff check gateway/turn_admission.py - python3 -m py_compile gateway/turn_admission.py - git diff --check
Normalize runtime concurrency values before constructing semaphores, preserve direct handler callers without generation state, and update the atomic-write ratchet for renamed admitted handlers. Verified: - 184 focused gateway tests passed across 10 files - ruff check passed on changed Python files - py_compile passed on changed Python files - git diff --check passed
7c9072f to
17888bd
Compare
FleetReview
Reviewed with 1 of 3 model families — anthropic, openai unavailable. Confidence: 1/5 Findings
FleetReview provenance · models: C=claude-code-opus-5, D=grok-4.6, G=grok-4.6 · cost: $33.62 · duration: 37m 46s · rounds: 2 · files examined: 7 |
|
Apollo r3 on head 17888bd (rebased): FleetReview's two P1s (slot leak on cancel in TurnAdmission.slot; bare-Future cancel contract in StartupResumePool.submit) are CONFIRMED real by reading the code; the red slice-9/14 test_charge_only_after_dispatch[cancel] is the second one. Round-4 rework dispatched (see kanban). Not landing this head. |
…me cancel owns its task Two FleetReview P1s on 17888bd. P1-A slot leak on cancel. TurnAdmission.slot() is an async generator, so a raise BEFORE its first yield skips __aexit__ entirely and the pre-yield region is the ONLY release path. The old code awaited the notice task after total_acquired=True; that await sat behind ack() -> adapter.send following a 15 s wait, a wide window in which a CancelledError permanently burned one of `cap` slots. At zero, every turn blocks in acquire forever — the starvation this gate exists to prevent. Pre-yield now releases internal/total (and unwinds in_flight/_owners) on ANY BaseException and re-raises; the notice is cancelled and reaped detached via add_done_callback, never awaited inside the critical section. P1-B broken cancel contract. StartupResumePool.submit() handed back a bare Future, which marks itself done the instant it is cancelled even while the admitted resume task keeps running. gateway/run.py's shutdown path reads exactly that done/cancelled state to tell "never started -> cancel + re-mark the session" from "in progress -> leave it alone", so a running resume could be re-marked and restored a second time on top of the original turn. Admitted entries now get _AdmittedResumeHandle, whose cancel() delegates to the inner task and which stays pending until the task's done callback resolves it. Queued entries keep the plain Future contract. Verified (venv python3.11, PYTHONPATH=worktree, HERMES_HOME=/tmp): - 8-file focused set: 176 passed, 0 failed. - tests/gateway/test_resume_cap_hardening.py::test_charge_only_after_dispatch [cancel] (#801's red test on this head) green, file unedited. - Mutation-proof P1-A: delete the pre-yield except-BaseException release -> test_pre_yield_failure_releases_acquired_permits[False,True] RED (total._value 2 != 3, the burned slot). - Mutation-proof P1-B: submit() back to a bare Future -> test_admitted_resume_handle_cancel_delegates_to_its_task RED AND test_charge_only_after_dispatch[cancel] RED (assert 2 == 0). - 4 round-3 guard mutants re-run, all still RED: cap coercion, startup coercion, legacy contract, stop-guard. Class sweep (AST over every .py): exactly 2 async context managers acquire a permit before yield. The other, hermes_cli/session_db_heavy_gate.py session_db_heavy_read_slot, has zero awaits between acquire and yield, so it has no cancellation-delivery point and is not an instance of this class. ruff check, py_compile, git diff --check all clean.
Round 4 — both FleetReview P1s closed. Head
|
| # | Mutant | Result |
|---|---|---|
| P1-A | delete the pre-yield except BaseException: release |
test_pre_yield_failure_releases_acquired_permits[False] + [True] RED — assert 2 == 3 on total._value, i.e. the burned slot itself |
| P1-B | submit() back to a bare create_future() |
test_admitted_resume_handle_cancel_delegates_to_its_task RED and test_resume_cap_hardening.py::test_charge_only_after_dispatch[cancel] RED (assert 2 == 0) — confirming the fix is what turns slice 9/14 green |
| r3-1 | cap coercion removed | RED (test_runtime_non_integer_turn_cap_is_unbounded_and_warns_once / test_proxy_mode) |
| r3-2 | startup coercion → or 3 |
RED (test_runtime_non_integer_startup_resume_cap_defaults_to_three) |
| r3-3 | legacy contract guard removed | RED (test_direct_handler_without_generation_state_keeps_legacy_contract / test_fast_command) |
| r3-4 | stop-guard removed | RED (test_queued_handler_drops_generation_invalidated_while_waiting) |
Every mutant was reverted and the file byte-compared back to the original before the next one.
Class sweep (P1-A's shape, not just its site)
AST pass over every .py in the tree for async context managers that acquire a permit before their first yield: exactly 2 sites. gateway/turn_admission.py::slot (acquire@68, yield@100, an await at 70 between them — the bug) and hermes_cli/session_db_heavy_gate.py::session_db_heavy_read_slot (acquire@136, yield@167, zero awaits between them). Cancellation is only delivered at an await, so the second site has no delivery point and is not an instance of this class. No other file needs the fix.
Also clean: ruff check, py_compile, git diff --check on both changed files. Only gateway/turn_admission.py and tests/gateway/test_turn_concurrency.py are touched this round; no pre-existing test file was edited.
Note for deploy (unchanged from r2): gateway.max_concurrent_turns must be SET in ~/.hermes/config.yaml — absent = unbounded = this fix is inert.
|
🤖 merged-by: apollo · lane: t_6745cd39 · gate: BYPASS: no FleetReview run exists for 043eae7 and the router is degraded (t_8d3c4eeb); argus artifact review stands in · why: argus r1 APPROVED (artifact lens): both FleetReview P1s (slot leak on cancel; bare-Future resume cancel contract) REPRODUCED on pre-fix head 17888bd and closed on 043eae7; 9 mutants RED incl #801's [cancel] test; CI 16/16 + 'All required checks pass'. FleetReview has no record and nothing queued for this head (router at ~50% escalated, t_8d3c4eeb). |
|
🤖 merged-by: apollo · lane: t_6745cd39 · gate: BYPASS: no FleetReview run exists for 043eae7; router degraded (t_8d3c4eeb); argus artifact review stands in · why: argus r1 APPROVED (artifact lens): both FleetReview P1s reproduced pre-fix + closed on 043eae7; 9 mutants RED; CI 16/16. No FR run exists for this head; router degraded (t_8d3c4eeb). |
… raises session_db_heavy_read_slot is an @asynccontextmanager that acquires its semaphore permit before the first yield. A generator that raises before its first yield never runs __aexit__, so the try/finally around the yield never executes and the pre-yield window is the ONLY release path. Any raise in that window burned a permit permanently; cap repeats killed the gate and every dashboard/TUI session-list read shed SessionDBHeavyReadBusy until the process restarted. Second member of the class PR #827 fixed at gateway/turn_admission.py. #827's sweep cleared this site with an await-only discriminator ("a cancel is only delivered at an await, and this window has none") — but #827's own regression test injects a SYNCHRONOUS raise, so the class is "the pre-yield region raises for ANY reason". Reproduced on the live file before fixing (cap=2, _record_stats injected to raise): RAISE arm final=1 entered_body=False; STARVATION arm final=0 healthy_caller_served=False. After the fix: final=2 / final=2 / served. Also lands the enforcement mechanism, not just the patch: scripts/check_preyield_permit_release.py asks the right question — "does any statement between the acquire and the first yield have a raise path (Call/Attribute/Subscript/Await), and is it covered by a try that releases?" — so a third such context manager cannot land unguarded. Its DOES NOT COVER section states the boundaries (acquire-failure path, release machinery, alias releases) rather than hiding them. Verified: - 12/12 tests/scripts/test_preyield_permit_release.py pass - mutants, each restored byte-identical after: A release deleted from the new guard -> RED (3 tests) B _RAISE_CAPABLE narrowed to (ast.Await,) -> RED (3 tests) C except Exception accepted as sufficient -> RED (1 test) D decorator discovery broken -> RED (7 tests) - guard run against gateway/turn_admission.py at 043eae7 (post-#827) -> PASS; at its pre-fix parent -> FAIL at the known leak line - ruff clean on all three files - tests/test_web_server_sessiondb_eventloop.py + tests/scripts/ : 89 passed, 2 failed — both failures reproduce on the unmodified base (ramscratch HERMES_HOME sandbox-guard + manifest nodeid collection), unrelated to this change
… raises session_db_heavy_read_slot is an @asynccontextmanager that acquires its semaphore permit before the first yield. A generator that raises before its first yield never runs __aexit__, so the try/finally around the yield never executes and the pre-yield window is the ONLY release path. Any raise in that window burned a permit permanently; cap repeats killed the gate and every dashboard/TUI session-list read shed SessionDBHeavyReadBusy until the process restarted. Second member of the class PR #827 fixed at gateway/turn_admission.py. #827's sweep cleared this site with an await-only discriminator ("a cancel is only delivered at an await, and this window has none") — but #827's own regression test injects a SYNCHRONOUS raise, so the class is "the pre-yield region raises for ANY reason". Reproduced on the live file before fixing (cap=2, _record_stats injected to raise): RAISE arm final=1 entered_body=False; STARVATION arm final=0 healthy_caller_served=False. After the fix: final=2 / final=2 / served. Also lands the enforcement mechanism, not just the patch: scripts/check_preyield_permit_release.py asks the right question — "does any statement between the acquire and the first yield have a raise path (Call/Attribute/Subscript/Await), and is it covered by a try that releases?" — so a third such context manager cannot land unguarded. Its DOES NOT COVER section states the boundaries (acquire-failure path, release machinery, alias releases) rather than hiding them. Verified: - 12/12 tests/scripts/test_preyield_permit_release.py pass - mutants, each restored byte-identical after: A release deleted from the new guard -> RED (3 tests) B _RAISE_CAPABLE narrowed to (ast.Await,) -> RED (3 tests) C except Exception accepted as sufficient -> RED (1 test) D decorator discovery broken -> RED (7 tests) - guard run against gateway/turn_admission.py at 043eae7 (post-#827) -> PASS; at its pre-fix parent -> FAIL at the known leak line - ruff clean on all three files - tests/test_web_server_sessiondb_eventloop.py + tests/scripts/ : 89 passed, 2 failed — both failures reproduce on the unmodified base (ramscratch HERMES_HOME sandbox-guard + manifest nodeid collection), unrelated to this change
1 participant
Incident
On 2026-09-21 from 02:52–03:07 PT, the default gateway became effectively deaf for 8–10 minutes per message after three interrupted restarts. A live
py-spycapture on pid 99910 measured:run_conversationframes in one interpreterEach boot scheduled about 11 restart resumes at once, while Kanban injections, subagent work, and live user turns also entered the unbounded gateway executor.
Summary
0preserves legacy unbounded behavior.max(1, cap - 2)slots.gateway.max_concurrent_turnsandgateway.startup_resume_concurrencysettings.max_concurrent_sessionsremains the separate new-session rejection gate. Nesteddelegate_taskagents remain governed by delegation limits and do not traverse this gateway admission path.Fail-before proof
Against clean
fork/mainatd13c5f25ca, the new real-executor regression was copied unchanged into a temporary worktree and run with both parameter variants:The bounded case therefore fails on the pre-change production path, while the explicit unbounded control preserves legacy behavior.
Verification
Round-3 hardening (
7c9072f) — three CI regressions this PR introduced, now closedEach was reproduced RED on the previous head
966ed208with the current tests, then GREEN here:test_no_atomic_write_reachable_from_loop.py). The admission splitrenamed two pre-existing reachable roots. The baseline entries were renamed in place
(
_handle_message_with_agent→_handle_message_with_agent_admitted,_run_agent_inner→_run_agent_admitted). No entries added; allowlist not widened.RED on
966ed208:1 failed.test_proxy_mode.py)._get_turn_admission()fed a non-intconfig value straight into
asyncio.Semaphore(TypeErroron aMagicMockcap). Both configboundaries are now normalized at runtime: only a positive
intis bounded;None/0/bool/any non-int → unbounded for turns, →
3forstartup_resume_concurrency; a present-but-invalidvalue logs once at WARNING. Covered by
test_runtime_non_integer_turn_cap_is_unbounded_and_warns_onceandtest_runtime_non_integer_startup_resume_cap_defaults_to_three.RED on
966ed208:TypeError: '<' not supported between instances of 'object' and 'int'.test_fast_command.py, 8 failures). The new outer_handle_message_with_agentgeneration guard treated a direct caller with no session state asstale and returned
Nonebefore the existing fail-closed routing ran. The guard now only applieswhen generation state actually exists (
_peek_session_state(...) is not None), so queued turnsinvalidated by
/stopwhile waiting for admission are still dropped(
test_queued_handler_drops_generation_invalidated_while_waiting) while direct/internal callerskeep the legacy contract (
test_direct_handler_without_generation_state_keeps_legacy_contract).RED on
966ed208:8 failed.None of the three pre-existing test files were edited except the ratchet baseline rename, which the
ratchet's own docstring prescribes.
Also passed:
ruff checkon all changed Python filespython3 -m py_compileon all changed Python filesgit diff --checkThe new tests cover bounded and unbounded real executor behavior, internal/user partitioning, admission-before-lease ordering, delayed one-time user acknowledgment, cancellation retention, FIFO boot-resume throttling for 4 and 7 entries, restore-drain timeout preservation, config coercion/precedence, and ten repeated admission bursts returning permits and ownership state to baseline.
Rollback and deployment
gateway.max_concurrent_turns: 0to restore unbounded turn execution.gateway.startup_resume_concurrencyto a sufficiently large positive value to approximate the prior eager boot-resume fan-out.This PR does not edit the live
~/.hermes/config.yamland does not restart the gateway. The fix is not live until Apollo merges it, configures the limits, and reloads the gateway throughsafe-restart.py. Existing installs must setgateway.max_concurrent_turns; an absent key remains unbounded for legacy compatibility.Remaining risk
The cap controls gateway-originated turns only; nested delegated agents intentionally remain outside this layer. The default of 8 is a new-install recommendation, while existing raw configs that omit the key remain unbounded until configured.