fix(gateway): release the heavy-read permit when the pre-yield region raises (+ class guard) - #860
Merged
Merged
Conversation
Kyzcreig
force-pushed
the
fix/session-db-heavy-preyield-leak
branch
from
September 22, 2026 05:59
3daf0ed to
3b7bedb
Compare
… 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
Kyzcreig
force-pushed
the
fix/session-db-heavy-preyield-leak
branch
from
September 22, 2026 06:02
3b7bedb to
a9dd9ba
Compare
Collaborator
Author
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: $8.88 · duration: 42m 29s · rounds: 3 · files examined: 3 |
Collaborator
Author
FleetReview
Reviewed with 2 of 3 model families — openai unavailable. Confidence: 1/5 Findings
FleetReview provenance · models: C=claude-code-opus-5, D=grok-4.6, G=grok-4.6 · cost: $18.87 · duration: 42m 15s · rounds: 2 · files examined: 3 |
Collaborator
Author
|
🤖 merged-by: apollo · lane: t_01b35d9e · gate: BYPASS: FR judge transient failure on a9dd9ba (ERROR = infra); argus artifact review stands in; guard-coverage P1s carded separately · why: argus r1 APPROVED (artifact lens): pre-yield permit leak REPRODUCED on 2334f26 (starvation to 0/2, healthy caller shed) and closed on a9dd9ba; cap holds both directions under 6-fault storm; 4 mutants RED. FleetReview record is status=ERROR 'judge transient failure' (infra, not a verdict); its 2 P1 member findings target coverage gaps in the NEW guard script, not the fix — split to a follow-up card. |
Kyzcreig
added a commit
that referenced
this pull request
Sep 22, 2026
…tial multi-permit releases FleetReview coverage gaps on PR #860's new class guard scripts/check_preyield_permit_release.py. The landed fix is correct; these are holes in the enforcement mechanism, split out rather than widening a verified branch. Reproduced against the landed guard before changing it (34-case matrix; 7 cases RED pre-fix, 34/34 green after): 1. The window was LINE-based (`acquire.lineno < n.lineno < yield.lineno`), so `await sem.acquire(); _LOG.info(...)` and `_LOG.info(...); yield` both fell outside it and passed clean. Now positional (lineno, col_offset), with a node that CONTAINS the yield excluded -- the yield runs first, so an expression-yield's wrapper is not an offender. NOTE: the finding as filed ("yield expression skipped") does not reproduce; `x = yield`, `await handle((yield))`, tuple/ifexp forms all already fired. Line granularity was the actual gap. 2. A release was matched by METHOD NAME, so at the two-semaphore site gateway/turn_admission.py (total + internal) an except arm releasing only one of them satisfied the check. Release is now matched by RECEIVER and every object acquired before the yield must come back. Independent oracle on real source: deleting only the `self.internal` release from TurnAdmission.slot makes the new guard FIRE (exit 1) while the landed one PASSES (exit 0) -- the escape, reproduced. This also closes the wrong-semaphore false negative disclosed as ISSUE-2 on #860; the docstring boundary section is updated to match measurement rather than restating the old claim. 3. Any single `except BaseException` arm that released satisfied the try, so a sibling `except ValueError:` that re-raised bare leaked whenever it matched. Now every EXITING arm must release; an arm that swallows falls through to the yield still holding the permit and is correctly not asked to. 4. Zero-sites exit 2 fired on per-FILE invocations, making the documented `[paths...]` form unusable from pre-commit (most files hold no sites). Vacuity is now exit 2 only for a directory SWEEP, where it does mean the discovery shape broke; the sweep test still pins that. Gate module (hermes_cli/session_db_heavy_gate.py), same review: 5. `_record_stats` ran FIRST in the pre-yield window, so an abort after it left acquired_count/queued_count/queue_wait_seconds_total counting a slot the caller never received -- overstating served load exactly when the gate was faulting. Moved LAST inside the guarded window. Measured with an injected fault: acquired_count 1 -> 0 on abort, still 1 on a granted admission, permit 2/2 throughout. 6. The per-admission cap read went through `load_config()` (full expansion + copy.deepcopy) on the event loop. Swapped to the repo's existing read-only idiom `read_raw_config_readonly()`, same (mtime_ns, size) freshness key: 445.4 us/call -> 7.5 us/call against the live 22 KB config.yaml (2000-call loop, py3.11.15), value unchanged at 2. Verified: ruff clean on all 3 files; 76 passed (this suite + test_turn_concurrency + test_web_server_sessiondb_eventloop); tree sweep exit 0, 2 sites enumerated. Every new test mutation-proven RED -- guard reverted to a9dd9ba = 9 failures; _record_stats moved back = 1 failure; load_config restored = 1 failure; control 31 passed with both files cmp-verified restored.
Kyzcreig
added a commit
that referenced
this pull request
Sep 22, 2026
…ere" Review found 4 MEASURED permit-leak regressions vs the merged #860 guard. `_handler_may_exit` excused an arm when it found no `ast.Raise`/`ast.Return` node in it -- but an arm also leaves via a call that raises, an `assert`, arithmetic, or a plain logging call given a bad format argument. Each is a real 1-permit loss that #860 caught and this branch let through. "This arm swallows" is not AST-decidable: the SAME arm text leaks 0 with a benign logger and leaks 1 with a bad format arg. So the question is inverted and answered on a whitelist -- an arm is excused only when every statement in it is provably inert (pass, a bare constant, an assignment between names/constants, a nested def). Anything else owes the permit back. The arm check now asks the same question the window check always has. The branch's own fixture at tests/.../:636 was the counterexample (its arm held a logging call), so it is narrowed to statements that cannot leave. MEASURED on the runtime oracle (real asyncio.Semaphore, synchronous fault at the pre-yield log, permit delta; HEALTHY arm clean on all 26 shapes): regressions vs #860 4 -> 0 false negatives 7 -> 2 (X4/X9 only, both pre-existing on #860/#863 and now named in the DOES NOT COVER block) false positives 0 -> 0 every prior win kept I1-I7, F1-F5, X5, X6 still FIRE; H1/H2 still clean X10 closes for free Mutation battery 7/7 RED, control 61 green at both ends: M8 exemption -> "contains ast.Raise" 7 failed / 54 passed M9 _inert_expr always True 6 failed / 55 passed M10 _inert_stmt always True 23 failed / 38 passed M11 _inert_stmt always False 8 failed / 53 passed M1 reachability -> #860 bare-trailing rule 13 failed / 48 passed M2 _released_in -> ast.walk 6 failed / 55 passed M3 BaseException coverage -> exiting only 2 failed / 59 passed Floors held: repo sweep exit 0 over the same 2 sites, per-file exit 0, empty-dir sweep exit 2, ruff clean.
Kyzcreig
added a commit
that referenced
this pull request
Sep 22, 2026
… and credited unreachable releases Pre-existing coverage gaps in scripts/check_preyield_permit_release.py, found by Argus's independent RUNTIME oracle during review of #863 and measured present on BOTH the merged #860 guard and the #863 candidate -- so this is not a regression from either, and is filed separately rather than widening a verified branch. FINDING 1 -- `_handler_exits()` asked "is the LAST statement a bare raise/return?", scanning reversed(handler.body) and breaking on any other statement type. An except arm that exits through a compound statement was therefore classified as swallowing and excused from releasing. It escapes only in the SIBLING shape: a correct `except BaseException:` arm makes the chain look satisfied while the misclassified narrow arm is the one that runs. Replaced by `_handler_may_exit()`, which asks whether ANY path through the arm can leave via raise/return (nested callables excluded -- own lifecycle). Conservative by design: a raise a nested handler swallows still counts as a possible exit, so such an arm is asked to release. False positive at worst. FINDING 2 -- `_released_in()` used ast.walk, so a textually-present release that can never run still satisfied the check. Now only statically-reachable statements are credited: nested def/lambda bodies, `if False:` branches, loops over an empty literal, and everything after an unconditional raise/return are skipped. Anything the compiler cannot settle still counts, so a release under a runtime condition, in a non-empty loop, or in a `with` body is unaffected. FINDING 3 -- `except BaseException: pass` as the only arm was FLAGGED, though it does not leak: the arm swallows and control reaches the yield still legitimately holding the permit, which is the guard's own stated rationale for not demanding a release from a swallowing arm. Cause was asking the BaseException-coverage question of the EXITING arms only, so an all-swallowing chain produced an empty set and read as unprotected. Coverage is now asked of the whole chain; the release demand only of arms that can exit. VERIFIED Runtime oracle (real asyncio.Semaphore, synchronous fault injected at the pre-yield log, measured permit delta; HEALTHY arm clean on all 11 shapes so no degenerate fixtures) reproduces Argus's matrix exactly: I1/I2/I3/I4/I7 and F1/F2/F3/F4 each LEAK 1 permit; controls I6 (releases then conditionally raises) and H1 (BaseException: pass) are clean. The new guard's static verdict matches that measurement 1:1 across all 18 probed shapes, 0 mismatches. 15 new tests (46 total, was 31 on #863's head). Sweep still exit 0 over the same 2 enumerated sites -- no live site has any of these shapes. Per-file invocation exit 0, empty-dir sweep still exit 2 (vacuity floor preserved). ruff 0.15.20 clean on both files.
Kyzcreig
added a commit
that referenced
this pull request
Sep 22, 2026
…ere" Review found 4 MEASURED permit-leak regressions vs the merged #860 guard. `_handler_may_exit` excused an arm when it found no `ast.Raise`/`ast.Return` node in it -- but an arm also leaves via a call that raises, an `assert`, arithmetic, or a plain logging call given a bad format argument. Each is a real 1-permit loss that #860 caught and this branch let through. "This arm swallows" is not AST-decidable: the SAME arm text leaks 0 with a benign logger and leaks 1 with a bad format arg. So the question is inverted and answered on a whitelist -- an arm is excused only when every statement in it is provably inert (pass, a bare constant, an assignment between names/constants, a nested def). Anything else owes the permit back. The arm check now asks the same question the window check always has. The branch's own fixture at tests/.../:636 was the counterexample (its arm held a logging call), so it is narrowed to statements that cannot leave. MEASURED on the runtime oracle (real asyncio.Semaphore, synchronous fault at the pre-yield log, permit delta; HEALTHY arm clean on all 26 shapes): regressions vs #860 4 -> 0 false negatives 7 -> 2 (X4/X9 only, both pre-existing on #860/#863 and now named in the DOES NOT COVER block) false positives 0 -> 0 every prior win kept I1-I7, F1-F5, X5, X6 still FIRE; H1/H2 still clean X10 closes for free Mutation battery 7/7 RED, control 61 green at both ends: M8 exemption -> "contains ast.Raise" 7 failed / 54 passed M9 _inert_expr always True 6 failed / 55 passed M10 _inert_stmt always True 23 failed / 38 passed M11 _inert_stmt always False 8 failed / 53 passed M1 reachability -> #860 bare-trailing rule 13 failed / 48 passed M2 _released_in -> ast.walk 6 failed / 55 passed M3 BaseException coverage -> exiting only 2 failed / 59 passed Floors held: repo sweep exit 0 over the same 2 sites, per-file exit 0, empty-dir sweep exit 2, ruff 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.
Second member of the bug class PR #827 fixed at
gateway/turn_admission.py. Found by Argus during #827 round 4 (cardt_6745cd39); carded ast_01b35d9e.The bug
hermes_cli/session_db_heavy_gate.py::session_db_heavy_read_slotis an@asynccontextmanagerthat acquires its semaphore permit before the firstyield. A generator that raises before its firstyieldnever runs__aexit__, so thetry/finallyaround theyieldnever executes — the pre-yield window is the ONLY release path. Any raise there burns a permit permanently. Repeat itcaptimes and the gate is dead: every dashboard REST (hermes_cli/web_server.py:3283) and TUI WebSocket (tui_gateway/ws.py:336) session-list read shedsSessionDBHeavyReadBusyuntil the process restarts.Latent, not live:
_record_stats/_LOG.infodo not raise today. One refactor away.Why #827's sweep missed it
#827's class sweep cleared this site with an await-only discriminator — "a
CancelledErroris only delivered at anawait, and this window has zero awaits". But #827's own regression test (test_pre_yield_failure_releases_acquired_permits) injects a synchronous raise (it monkeypatcheslogger.infoto raise). So the class the PR itself gates is "the pre-yield region raises for ANY reason", not "a cancel lands at an await".Reproduced before fixing
Live file, cap=2,
_record_statsinjected to raise:After the fix:
The enforcement mechanism, not just the patch
scripts/check_preyield_permit_release.pyreplaces the discriminator that produced the miss. It asks:yieldcontain a raise-capable node (Call/Attribute/Subscript/Await) — not "is there anawait?"trywhoseexcept BaseException/ bareexcept/finallycalls.release()?Zero enumerated sites is exit 2 (vacuously-green floor), not a pass.
DOES NOT COVERstates its boundaries — the acquire-failure path, the release machinery itself, alias releases — rather than hiding them. Inline# noqa: preyield-permitexempts a deliberate site.Gated by
tests/scripts/test_preyield_permit_release.py, which is insidetestpaths— same wiring as the existingscripts/check_subprocess_stdin.py+tests/tools/test_subprocess_stdin_guard.pyprecedent, no bespoke workflow job.Verification
12/12
tests/scripts/test_preyield_permit_release.pypassMutants (each restored byte-identical after):
semaphore.release()deleted from the new guard_RAISE_CAPABLEnarrowed to(ast.Await,)(fix(gateway): bound turn concurrency + boot-resume fan-out (P1 2026-09-21 starvation) #827's discriminator)except Exceptionaccepted as sufficientCross-check on the sibling site (the class proof). Rebased onto
fork/main, which now contains fix(gateway): bound turn concurrency + boot-resume fan-out (P1 2026-09-21 starvation) #827, so the guard enumerates both members of the class and passes:✅ 2 @asynccontextmanager acquire-before-yield site(s). Narrowing fix(gateway): bound turn concurrency + boot-resume fan-out (P1 2026-09-21 starvation) #827's ownexcept BaseExceptiontoexcept asyncio.TimeoutErroringateway/turn_admission.pymakes the guard fire on that file (gateway/turn_admission.py:70 … 11 raise-capable node(s) … not covered, exit 1), then restored byte-identical. The guard therefore gates the class, not just this patch.Guard fires on the real pre-fix file: run against
HEAD~1'ssession_db_heavy_gate.py→ exit 1 at the known leak line; current tree → exit 0.ruff checkclean on all three files.tests/scripts/+tests/test_web_server_sessiondb_eventloop.py: 91 passed, 0 failed.Severity
Major, not Critical. Unlike the turn gate, this one sheds on a 1 s
wait_forbound, so a starved caller gets a retryableSessionDBHeavyReadBusyrather than an unbounded hang.