Repository navigation
fix(gateway): honor external drain at durable input admission - #140135
GregoryMoravAgente wants to merge 1 commit into
Conversation
Refuse new input inside its SQLite admission transaction while allowing accepted FIFO work to drain. Preserve restart queue-only behavior and revalidate a head committed after an empty claim preflight. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
GitHub has placed the fork-PR workflows in Could an upstream maintainer review the two-file commit and approve the workflows if appropriate? Local validation is recorded in the PR: 85 related tests passed, 5 platform skips, and all 12 official checks passed. No upstream CI pass is claimed. |
alt-glitch
added
comp/gateway
|
Thanks for separating new-input admission from claims of already-accepted FIFO work. We found this PR while preparing a locally used maintenance fix, and our operator contract is different from the one this PR intentionally selects, so this is a semantic comparison, not a competing duplicate PR or a claim that your tests are wrong. The two contracts
We leave the current public admission gate unchanged. The patch adds claim-only checks before preflight, just before claim and in the SQLite claim guard. A private pause signal releases claim/mutation locks before waiting; it retains the waiter and checks the in-memory maintenance flag once per second. Clearing it resumes the same pump without resubmitting input. A rescan also breaks the wait so cancelling the last queued row reaches the normal empty exit. The exact Reproduction and validation boundaryAt local baseline Original author checks used real admission/claim/cancel/settlement and temporary SQLite with a finite synthetic executor, including cancel-last-empty, replace-head/rescan, revalidation, storage retry and ordinary-stop behavior. Separately recorded non-author checks covered repeated toggles, FIFO/epoch reopen and no remaining async task. These records are not new execution on this PR head, not a maintainer review, and not a full CLI/model-worker end-to-end run. The original author kept earlier failing candidates and harness failures rather than counting them as passes. A claim that passed the transaction guard just before maintenance becomes active remains in flight. The marker watcher also has its own response interval. This does not provide a cross-thread atomic promise that writing a marker prevents every later Request for maintainer directionWould a distinct pause-new-claims / queue-new-input maintenance mode be appropriate, alongside this PR's drain-accepted-work behavior? We are sharing the exact locally used patch to make that design choice concrete, not asking that it silently replace your chosen semantics. The two patches are not drop-in cumulative: combining them requires an explicit mode and fresh admission/claim race regressions. The patch below passed application checking against fixed upstream Environment and evidence boundary
Exact locally used source patchThis is an uncommitted source diff, not an upstream PR or merged fix. It is shared for review; no local installation or restart is performed by this report. Source patchdiff --git a/gateway/session_authority.py b/gateway/session_authority.py
index 0fb0b5b349cfe0ca9902a973c3f0e768b2025586..178ab8757841dca2f00e34dbf2963f60556a658b 100644
--- a/gateway/session_authority.py
+++ b/gateway/session_authority.py
@@ -28,6 +28,8 @@ from hermes_state_runtime import (
# Capped backoff for a claim refused by transient storage (writer lock, full disk).
_CLAIM_RETRY_MIN_S, _CLAIM_RETRY_MAX_S = 0.25, 5.0
+# External maintenance parks only a queued claim; this memory-only wait must remain cancellable.
+_EXTERNAL_CLAIM_POLL_S = 1.0
# Longest a settled turn's adapter may hold its adopted crash marker before the follower is claimed.
_MARKER_RELEASE_WAIT_S = 60.0
# True in the admitting task once ``admit_native``'s shielded ledger write began: a caller cancelled
@@ -40,6 +42,10 @@ native_write_started = ContextVar('native_write_started', default=False)
messaging_admission = ContextVar('messaging_admission', default=None)
+class _ExternalClaimPaused(Exception):
+ """Internal claim-only pause: accepted input and its observers remain pending."""
+
+
@dataclass
class LiveSession:
source: object
@@ -235,6 +241,21 @@ class SessionAuthority:
authorize(conn)
return guard
+ def _require_claim_open(self, head):
+ """Refuse a new queued claim during external maintenance, leaving in-flight work alone.
+
+ ``head=None`` is an empty read, bound to that same head in the store transaction:
+ a later admission can only cause a rescan, never become a claim through this gate.
+ """
+ self._require_admission_open()
+ if head is not None and getattr(self.runner, '_external_drain_active', False):
+ raise _ExternalClaimPaused()
+
+ def _claim_gate(self, head):
+ def guard(conn):
+ self._require_claim_open(head)
+ return guard
+
def logical_owner(self, session_id):
"""The FIFO/admission identity of a route: the root of its compression lineage.
Compression advances the physical transcript, never the admission identity. A session this
@@ -1004,6 +1025,8 @@ class SessionAuthority:
if any(row['status'] == 'unknown' for row in pending):
raise RuntimeStoreError('unknown_execution')
first = next((row for row in pending if row['status'] == 'queued'), None)
+ head = first['admission_id'] if first is not None else None
+ self._require_claim_open(head) # park before connector/MCP preflight, outside its awaits
from gateway.config import Platform
if live.source.platform == Platform.LOCAL:
# A reasoning command or a model pick made while the previous turn ran lands here, at the
@@ -1062,14 +1085,14 @@ class SessionAuthority:
# a claim now would hand this row to its busy path and settle it unrun. The head
# waits queued, as main's pending slot did; /stop and /new still cancel it.
return 'classic_busy', first
- self._require_admission_open()
+ self._require_claim_open(head)
from gateway.session_runtime_workers import tracked_write
live.rescan = False # a _schedule from here on may not be visible to this claim's read
# The row these checks validated, never whichever is first when the write runs: a
# cancellation commits from a worker thread and can advance the FIFO at any point.
row = await tracked_write(self, partial(claim_session_input, self.db, epoch=self.epoch,
- session_id=ref.session_id, _guard=self._admission_gate(),
- head=first['admission_id'] if first is not None else None),
+ session_id=ref.session_id, _guard=self._claim_gate(head),
+ head=head),
then=partial(self._stamp_claim, ref, live))
# Input scheduled while this claim's transaction ran may postdate its read: re-read
# instead of letting the drain exit idle over a committed admission.
@@ -1105,6 +1128,19 @@ class SessionAuthority:
while True:
try:
row, first = await self._claim_next(ref, live)
+ except _ExternalClaimPaused:
+ # _claim_next has released both locks. Keep the accepted row and its waiters;
+ # exiting here would strand it until another submit re-scheduled this pump.
+ while True:
+ if self._drain_stopping(ref, live):
+ return None
+ if not getattr(self.runner, '_external_drain_active', False) or live.rescan:
+ # Re-read a changed FIFO; cancelling its last row must reach the idle exit.
+ live.rescan = False
+ break
+ await asyncio.sleep(_EXTERNAL_CLAIM_POLL_S)
+ backoff = 0.0
+ continue
except RuntimeStoreError as exc:
if exc.reason == 'unknown_execution':
# An automation turn nobody is waiting on settles interrupted (GW-10); only aThanks to the Hermes maintainers and contributors for the foundation and follow-up fixes. If this carefully scoped repro/patch helps, a little 👍 would make our human-and-agent team smile. No obligation — useful review comes first. 🐣 |
What does this PR do?
Reversible external drain rejects new in-process turns, but routed session authority can commit their durable input before that later refusal. This closes new-input admission at both authority preflight and the SQLite transaction guard.
Already accepted FIFO work continues draining. Claim operations keep the exact head pin and fresh per-head checks: input committed after an empty preflight causes a reread rather than pausing accepted work. Ordinary shutdown still refuses claims. The restart-wait queued-input exception remains available unless external drain is also active.
Related Issue
No upstream issue. Tracked as AGC-1663; owner
codex-hermes-export-proposal-01a120af-20261016.Type of Change
Changes Made
gateway/session_authority.py: distinguish new-input admission from claiming accepted durable input at the shared gate; preserve all existing per-head authorization and ordinary shutdown checks.tests/gateway/test_external_drain_control.py: deterministic real SQLite regressions for admission/claim races, accepted FIFO settlement, combined external-drain/restart-wait priority, and ordinary shutdown/restart compatibility.How to Test
Run the canonical
scripts/run_tests.sh -j 2against the external-drain, session-authority, admission-results, ingress-admission, restart-after-turn, restart-drain, gateway-shutdown, clean-shutdown-marker, executor-quiesce, managed-turn-shutdown and API-queued-admission modules. Result: 85 passed, 0 failed, 5 platform skips on macOS arm64/Python 3.14, using the frozen dev/test/messaging environment.python scripts/check --commit HEAD: 12 checks passed, zero blocking or advisory code-health findings. Causal failing controls were retained for new input committing during drain and accepted work being stranded, including the empty-preflight interleaving.Checklist
🤖 Generated with Claude Code