Skip to content

feat(gateway): PendingCompletionRegistry — explicit state machine for completion delivery - #73427

Closed
handnewb wants to merge 6 commits into
NousResearch:mainfrom
handnewb:registry/pending-completion-state-machine
Closed

feat(gateway): PendingCompletionRegistry — explicit state machine for completion delivery#73427
handnewb wants to merge 6 commits into
NousResearch:mainfrom
handnewb:registry/pending-completion-state-machine

Conversation

@handnewb

@handnewb handnewb commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Two approaches are offered for #70300 — maintainers pick one.

Both close the reported flood. They are independent branches, not a stack — reviewing either
one alone is enough. I am not advocating for one over the other; #73427 is the better code
and #72675 is the smaller risk, and that trade is yours to make.
Root-cause writeup: #70300.


Summary

Multiple background process completions arriving in the same tick flooded the session with
one synthetic turn each (#70300). This PR makes them arrive as a single coalesced turn.

The delivery path is reworked around an explicit pending-completion registry rather than
patching the individual failure modes, because two review rounds on #72675 established that
they share one cause. Root-cause writeup and maintainer discussion: #70300.

Root cause

Pending-completion state was split across three structures that could not observe each other:

  • _completion_notification_batches — a dict of lists with no per-entry state
  • one Future per waiter, carrying Optional[bool]
  • one asyncio.Task per route, whose death was indistinguishable from success

Every finding from both review rounds follows from that split: cancelling destroyed the only
record that anything was pending; claiming and delivering were the same call, so siblings
could not be claimed ahead of the adapter await; failed and unknown shared one value; and
there was nowhere to hold an attempt counter.

Design

PendingCompletionRegistry, owned by the gateway lifecycle, one entry per completion
identity.

PENDING --claim(batch_id)--> CLAIMED --delivered--> DELIVERED   (terminal)
   ^                            |
   |                            +-- transient failure --> PENDING     (attempts+1, backoff)
   |                            +-- attempts exhausted --> FAILED      (terminal, logged)
   |                            +-- already delivered --> SUPERSEDED  (terminal)
   |                            +-- no route -----------> UNROUTABLE  (terminal)
   +---- cancel -----------------+-- gateway stopping --> STOPPING    (terminal, counted)

Entry fields: identity, route_key, state, attempts, next_attempt_at, payload,
batch_id.

Invariants, each covered by a test:

  • No entry leaves a non-terminal state except through a declared transition. There is no
    path on which a pending completion disappears.
  • claim(batch_id) transitions every fresh sibling of the batch atomically, before the
    adapter await. Failure releases all of them, never a subset.
  • Dedupe reads the registry, so "claimed by this flush" and "claimed by another path" are
    distinct states rather than the same boolean.
  • Cancellation is never terminal by omission: in-window cancellation returns the entry to
    PENDING; cancellation at stop moves it to STOPPING.

Shutdown contract

On stop, pending completions transition to STOPPING. They are not delivered, not
retried
, and the count is logged. Shutdown stays bounded.

Delivering a synthetic turn into a session that is being torn down surfaces it to nobody, so
there is no "last delivery chance" worth buying with an unbounded shutdown. STOPPING is
terminal by declared contract, counted, and tested — not a silent drop.

An in-memory registry does not survive SIGKILL. That is a pre-existing property of the
delivery path rather than a behaviour change here, and out of scope for #70300; see the
discussion in that issue.

Status — work in progress

The registry and state machine exist (34f19f7), but migration of the delivery path onto them is
not finished. _flush_process_completion_batch and
_enqueue_process_completion_notification still run on the previous batch-dict mechanism, so
both mechanisms currently coexist. That is worse than either alone, and it is what I am fixing
next.

Still open:

  • _completion_notification_batches and _completion_notification_batch_tasks are still live
    alongside the registry
  • the CompletionDisposition enum from 9a6f2ef sits on the old path rather than on registry
    state, and will be replaced by the state field
  • commits de9acba through 9a6f2ef are the pointwise series carried over from fix(gateway): coalesce concurrent process completions and watch events #72675; they
    belong to that PR and are being removed from this branch, not kept
  • no property tests over the state machine, and no deterministic concurrency harness, so the
    cancellation and shutdown paths are currently argued rather than proven

Please do not review the delivery path yet. I will convert this out of draft when the old
structures are gone and the invariants in Design are covered by tests.

Verification still owed

The issue body diagnosed the flood as the post-turn drain loop injecting one notification per
event. A review comment on #72675 observed that _drain_gateway_watch_events() forwards only
watch_match/watch_disabled, which would mean standard completions never reach that loop.
Both cannot be true.

Both PRs coalesce at both seams, so the reported behaviour is fixed either way — but I have not
proven which seam produced the six injections in the log. I am instrumenting that against
main before finishing the migration, and I will correct the issue body with the result.

Separately, the log in #70300 shows a second defect that coalescing masks rather than fixes:
Persisted transcript lagged live cached history (disk=154, memory=157). Reducing six
injections to one removes the trigger, not the cause. Filing that separately.

Relationship to #72675

#72675 contains pointwise patches for the same five findings — useful if maintainers prefer
a minimal change. This PR replaces the patches with a root-cause fix. Both
are offered; maintainers choose one.

Relationship to #71898 and #70319

The original batching core and its delivery tests carry forward from #71898. That groundwork
is preserved in git history via co-author trailers on the relevant commits, and I am happy to
land this in whatever shape the maintainers prefer — including as a PR stacked on #71898's
branch so those commits stay first. See #70300 for that discussion.

Supersedes #71898 and #72675

root and others added 6 commits July 28, 2026 11:01
Coalesce multiple background process completions and watch events that
share the same gateway route so the agent receives one synthetic turn
instead of one turn per process (NousResearch#70300).

Two-pronged approach, each at the correct ownership seam:

1. Standard completions: batched at _run_process_watcher() via
   _enqueue_process_completion_notification() with a short (100ms)
   window and threshold-based early flush (5+ entries = immediate).
   Single completions pass through with zero extra latency.

2. watch_match / watch_disabled events: coalesced at the post-turn
   drain via _coalesce_and_inject_watch_events(), grouped by type
   and session_key.

Output format includes:
- Per-process status with visual indicators (✅/❌)
- Exit codes, elapsed time, reason
- Aggregate summary (N succeeded, M failed)
- Bounded output (10 detailed results, 800-char tails)

Edge cases covered:
- Flush during delivery schedules next batch
- Duplicate primary tries next batch identity
- Formatter failure resolves all waiters with False
- Lazy init for tests using object.__new__
- None-safe batch key and type handling

Closes NousResearch#70300
Supersedes NousResearch#70319 and NousResearch#71898

Co-authored-by: yuzilongleif-collab <235949691+yuzilongleif-collab@users.noreply.github.com>
The LLM processes exit_code=0 text the same as a checkmark emoji.
Removed redundant visual indicators from coalesced batch output to
save tokens without losing signal. Summary line and exit_code values
already communicate success/failure unambiguously.

Co-authored-by: yuzilongleif-collab <235949691+yuzilongleif-collab@users.noreply.github.com>
Co-authored-by: yuzilongleif-collab <235949691+yuzilongleif-collab@users.noreply.github.com>
…and test gaps

yuzilongleif-collab second review (NousResearch#72675) — P1 lifecycle hole, shutdown
ordering, missing summary regression test, and stale docstrings.

P1 — Cancellation/lifecycle hole:
- _flush_process_completion_batch now catches CancelledError,
  detaches the batch from _completion_notification_batches, cleans up
  the task key, resolves all pending Futures as retryable (False),
  and re-raises — so neither cancel-during-window nor
  cancel-during-delivery leaks an unresolved waiter or stale batch.
- Two regression tests: cancel during the window, cancel during
  adapter-blocked delivery — both assert no unresolved Future,
  no stale batch entry, no stale task key, and retryable disposition.

P1 — Shutdown ordering:
- _stop_impl_body now drains background flush tasks (cancel + await
  with 3-second best-effort deadline) BEFORE adapter teardown, so
  completions get a last delivery chance or fail retryable before
  adapters are gone.

Point 2 — Omitted-tail summary regression:
- test_batch_summary_counts_failures_in_omitted_tail: 12-entry batch
  with exit_code=1 only on entries[10:11]; asserts the summary still
  reports `2 failed` even though those entries are not shown.

Docstrings:
- Removed unimplemented None→False adapter-set escalation comment
  from _flush_process_completion_batch.
- Removed stale `or until the threshold is reached` claim from
  _enqueue_process_completion_notification docstring.

34 tests passed, 0 failed. Ruff/git diff --check clean.

Co-authored-by: yuzilongleif-collab <235949691+yuzilongleif-collab@users.noreply.github.com>
…um, and safety gaps

T2 — Shutdown drain comment: aligned with actual behaviour (cancel
first, then await — no false promise of "last delivery chance").

T3 — CompletionDisposition enum: replaces implicit Optional[bool]
resolution in _flush_process_completion_batch with explicit enum
variants (DELIVERED / RETRY / DROP_DUPLICATE / DROP_UNROUTABLE /
SHUTTING_DOWN).  Every waiter now resolves to an explicit disposition;
the CancelledError handler resolves as RETRY instead of bare False.
_enqueue_process_completion_notification returns CompletionDisposition;
the process watcher checks is RETRY instead of is False.

T6 — Secret redaction in batched watch_match snippets: the P4
per-process snippet path now routes command and output through
_redact_gateway_user_facing_secrets — preventing secrets from
leaking in batched messages that were previously raw.

T7 — None-sentinel batch key: _completion_notification_batch_key
now uses \x00<none> for None fields so None and "" produce
distinct keys, preventing session-boundary crossing.

T8 — Batch entry cap (50): entries beyond the cap resolve
immediately as DROP_UNROUTABLE instead of growing unbounded.

Co-authored-by: yuzilongleif-collab <235949691+yuzilongleif-collab@users.noreply.github.com>
Introduces an explicit state machine (PENDING->CLAIMED->DELIVERED/FAILED/
SUPERSEDED/UNROUTABLE/STOPPING) to replace the split-state architecture
that caused all five review findings on PR NousResearch#72675.

The registry provides:
- Atomic claim_batch() for all sibling identities before adapter await
- Per-entry attempt counter with exponential backoff ceiling
- Explicit STOPPING state for shutdown (terminal, no silent drops)
- resolve_futures() that guarantees every waiter sees a disposition

GatewayRunner.__init__ now creates the registry instead of the old
_completion_notification_batches dict.  Full migration of
_flush_process_completion_batch and _enqueue_process_completion_notification
to use the registry's state transitions is in progress.

Root-cause writeup: NousResearch#70300
Alternative (pointwise patches): NousResearch#72675

Co-authored-by: yuzilongleif-collab <235949691+yuzilongleif-collab@users.noreply.github.com>
@handnewb

Copy link
Copy Markdown
Contributor Author

@yuzilongleif-collab — this is the PendingCompletionRegistry approach discussed in #70300. It replaces the pointwise patches in #72675 with an explicit state machine that eliminates the shared root cause of all five review findings. Both PRs are offered; maintainers choose one. HEAD: 34f19f7ac10ed050cfab6915bb22add40890b112

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 28, 2026
@handnewb handnewb closed this Jul 28, 2026
@handnewb

Copy link
Copy Markdown
Contributor Author

Closed in favour of #73469.

This branch carried the four pointwise commits from #72675 on top of the registry work, which
meant the two approaches were not independent — reviewing this one implied reviewing that one
plus more. #73469 is rebuilt from main and contains only the registry replacement, so the two
offers are now genuinely disjoint: #72675 is the pointwise patch set, #73469 is the root-cause
fix. GitHub does not allow repointing a PR's head branch, hence the new number rather than an
update here.

handnewb added a commit to handnewb/hermes-agent that referenced this pull request Aug 6, 2026
…and CompletionStore

Core infrastructure for NousResearch#70300 root-cause fix (NousResearch#73427).

- CompletionDisposition enum: DELIVERED, RETRY, DROP_DUPLICATE,
  DROP_UNROUTABLE, DROPPED_OVERFLOW, SHUTTING_DOWN
- PendingCompletionRegistry: explicit state machine with 8 states,
  atomic claim_batch(), attempt counter with exponential backoff
  ceiling (5 attempts), signal_stop() for shutdown, observability
  counters per terminal state
- CompletionStore protocol + NullCompletionStore default: seam for
  optional durability without changing behaviour

Co-authored-by: yuzilongleif-collab <235949691+yuzilongleif-collab@users.noreply.github.com>
handnewb added a commit to handnewb/hermes-agent that referenced this pull request Aug 6, 2026
… coalescing

Implements the full delivery-path refactor on top of
PendingCompletionRegistry (NousResearch#70300 / NousResearch#73427).

- _enqueue_process_completion_notification: inserts PENDING entries
  into the registry, schedules per-route flush tasks
- _flush_process_completion_batch: atomic claim_batch() over all
  siblings before adapter await; CancelledError transitions to PENDING
  with attempt counter and re-raises
- _format_coalesced_process_completions: bounded rendering (10 entries,
  800-char tails), aggregate summary over ALL entries including
  overflow counted as DROPPED_OVERFLOW
- _coalesce_and_inject_watch_events: groups watch_match/watch_disabled
  by type+session_key at post-turn drain; batched snippets routed
  through _redact_gateway_user_facing_secrets
- _completion_notification_batch_key: None sentinel prevents
  session-boundary crossing
- _run_process_watcher: routes through _enqueue_process_completion_notification;
  retry check uses CompletionDisposition.RETRY instead of 'is False'
- post-turn drain: replaces per-event loop with
  _coalesce_and_inject_watch_events
- _stop_impl_body: cancels flush tasks with bounded wait, drains
  registry to STOPPING with structured log, then tears down adapters
- GatewayRunner.__init__: creates PendingCompletionRegistry +
  batching state (route_keys, entry_data, batch_tasks)

Co-authored-by: yuzilongleif-collab <235949691+yuzilongleif-collab@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants