Skip to content

feat(gateway): PendingCompletionRegistry — root-cause fix for #70300 - #73469

Open
handnewb wants to merge 12 commits into
NousResearch:mainfrom
handnewb:registry/pending-completion-state-machine-v2
Open

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

Conversation

@handnewb

@handnewb handnewb commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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

  • fix(gateway): coalesce concurrent process completions and watch events #72675 — pointwise patches. Smaller diff, keeps the existing delivery shape, carries
    one documented residual (the sibling claim window).
  • This PR — root-cause fix. Introduces an explicit registry and state machine for pending
    completions, so the failure modes stop being reachable rather than being patched one by one.

Both close the reported flood. Independent branches, not a stack — reviewing either alone is
enough. I am not advocating for one; this 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 reworks the delivery path around an explicit
PendingCompletionRegistry state machine.

Root cause, and how it was located

Pending state was split across three structures that could not observe each other: a batch dict,
a Future carrying Optional[bool], and an untracked asyncio.Task. Every finding from the two
review rounds on #72675 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.

On the flood itself: my original issue body blamed the post-turn drain loop. That was wrong.
Reading _drain_gateway_watch_events() at 4c9628e confirms it forwards only
watch_match/watch_disabled, so standard completions never reach it — the six injections came
from the per-process watcher path, where each completing watcher delivers its own notification.
The issue body is corrected. I have not yet re-run the six-process reproduction end to end, so
this is a code-level confirmation rather than a measured one.

Design — PendingCompletionRegistry

Owned by the gateway lifecycle, one entry per completion identity. Fields: identity,
route_key, state, attempts, next_attempt_at, payload, batch_id.

                    ┌──────────────────────────────► DELIVERED         (terminal)
                    │  delivered
PENDING ──claim(batch_id)──► CLAIMED ──┬── transient failure ──► PENDING   (attempts+1, backoff)
   ▲                                   ├── attempts exhausted ─► FAILED    (terminal, logged)
   │                                   ├── already delivered ──► SUPERSEDED (terminal)
   │                                   └── no route ───────────► UNROUTABLE (terminal)
   │
   ├── cancel (outside shutdown) ──────► PENDING       (never terminal by omission)
   ├── over capacity ──────────────────► DROPPED_OVERFLOW (terminal, counted in summary)
   └── gateway stopping ───────────────► STOPPING      (terminal, counted, logged)

Properties this buys, none of which existed before:

  • Nothing vanishes. Every exit from a non-terminal state is a declared transition. The
    original None-consumes-a-completion defect has no representation here.
  • claim(batch_id) is atomic across siblings, before the adapter await; failure releases all
    of them, never a subset. This closes the hardening I declined on fix(gateway): coalesce concurrent process completions and watch events #72675 — and my reason there
    was wrong. I said pre-claiming conflicted with the dedupe seam; that described a missing
    parameter, not an obstacle. It also stops being a conflict once state is explicit: "claimed by
    this flush" is CLAIMED with the current batch_id, "claimed elsewhere" is CLAIMED with
    another.
  • Cancellation is never terminal by accident. In-window cancellation returns the entry to
    PENDING; cancellation at stop moves it to STOPPING.
  • Retry is bounded — attempt counter with exponential backoff, ceiling 5, exhaustion logged.

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 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 and counted — not a silent drop.

On the exactly-once ask from review: an in-memory registry does not survive SIGKILL. Adding
persistence means a storage dependency, which is your architectural call rather than mine, so it
is not in this PR. The registry's PENDING transition is the natural write-through point if you
want it — happy to add a store protocol as a follow-up.

Security fix included

Process stdout/stderr tails in _format_coalesced_process_completions were not routed
through _redact_gateway_user_facing_secrets. The watch_match snippet path already was; the
completion path was not, exposing up to 800 characters per process across up to 10 processes per
message. db9fc61 routes it through the same helper — reusing the pattern list rather than
duplicating it, so the two paths cannot drift.

(_inject_watch_notification retains the Optional[bool] return from main; out of scope
here, but the same ambiguity may apply to the watch-event path.)

Introduced by this PR's coalesced formatter and fixed in db9fc61. The same class of
issue exists on main in _run_process_watcher (separate PR #73547).

Changes — gateway/run.py

  • New PendingCompletionRegistry, State (8 variants), CompletionDisposition (6 variants)
  • _enqueue_process_completion_notification() — inserts PENDING
  • _flush_process_completion_batch() — atomic claim_batch(), adapter delivery, single exit
    transition; CancelledError transitions and re-raises
  • _run_process_watcher() — routes through enqueue; retry keyed on
    CompletionDisposition.RETRY rather than is False
  • _format_coalesced_process_completions() — bounded rendering (10 entries, 800-char tails),
    aggregate summary over all entries including overflow, redacted tails
  • _coalesce_and_inject_watch_events() — replaces the per-event drain loop; groups by type +
    routing key; redacted bounded snippets
  • Routing keyNone sentinel so None and "" cannot share a key and cross a session
    boundary
  • _stop_impl_body() — cancels flushes with a bounded wait, drains registry to STOPPING
    with a structured log, then tears down adapters
  • Observability — counters per terminal state

Status — what is not here yet

  • No tests in this PR yet. State-machine property tests and a deterministic concurrency
    harness (virtual clock, controllable adapter, cancellation injection at named points) are the
    next commits. The harness is separate because it is reusable — the fault probes from both
    review rounds on fix(gateway): coalesce concurrent process completions and watch events #72675 were hand-built and discarded twice.
  • _deliver_completion_notification() now returns CompletionDisposition (migrated from
    Optional[bool] in f3f3dad). That closes the original "None silently consumes a completion"
    finding at its root — there is no tri-state return left on this path.
  • No docs/gateway/delivery-contract.md yet.
  • The interleaving grid over cancellation points is not here; it belongs with the harness.

On the diff shape: this is +1118/-26 because main has no coalescing mechanism to replace —
the per-event injection loop is the current behaviour, and both this PR and #72675 are additive
against it. "Replaces the split pending-state" is a comparison with #72675's approach, not with
main. Flagging that so the diff size is not read as scope creep.

Relationship to the other PRs

#72675 patches the five findings pointwise and remains open as the smaller-diff alternative,
with the sibling claim window documented as its declared cost. #73427 was this work on a branch
that still carried #72675's commits; closed because GitHub does not allow repointing a PR's head
branch.

The batching core in #72675 carries forward from #71898 by @yuzilongleif-collab, preserved with
co-author trailers. Whether this supersedes #71898 depends on which approach you pick, so I am
not asserting it.

Triage labels from #73427 (needs-decision, comp/gateway, P2, tool/terminal,
type/bug, sweeper:risk-message-delivery) did not transfer automatically.

Closes #70300

@handnewb

Copy link
Copy Markdown
Contributor Author

@yuzilongleif-collab — this is a clean branch from main with the PendingCompletionRegistry implemented from scratch (2 commits). No pointwise patch series carried over. #72675 remains open as the smaller-diff alternative. Both close #70300; maintainers pick one. Root-cause verified: the flood comes from the watcher path, not the drain (confirmed at 4c9628e).

@handnewb

Copy link
Copy Markdown
Contributor Author

Trimmed this down. The CompletionStore protocol and the per-state observability counters were additive rather than required to close #70300, so they are out and offered as follow-ups; the structured shutdown log stays, since STOPPING is otherwise indistinguishable from a silent drop.

What remains is the registry, the state machine, the migration of the delivery path onto it, and the bounded/redacted formatting. That is the part actually under judgement here, and I would rather you spend attention on it than on scaffolding I added pre-emptively.

Still declared as not-yet-here: the hypothesis property tests, the interleaving grid over cancellation points, and docs/gateway/delivery-contract.md. Those wait on whether this approach is the one you want — no point proving an architecture nobody has chosen.

@handnewb
handnewb force-pushed the registry/pending-completion-state-machine-v2 branch from 467a33e to 7bfaa5b Compare July 29, 2026 13:12

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving the coalescing to the verified watcher seam. Current main still starts one watcher per process at gateway/run.py:16908, and each completed watcher independently calls delivery at gateway/run.py:21299, so the premise remains valid.

Problems

  • gateway/run.py:18826 caps len(_entries), but terminal entries are never evicted. After 50 distinct completions, every later completion is rejected.
  • The retry transition at gateway/run.py:19361-19378 leaves an entry PENDING but removes its route/payload indexes. The watcher retry then re-enqueues the identity; gateway/run.py:18828-18830 rejects it as non-terminal, so the completion is dropped instead of retried.
  • gateway/run.py:10063-10080 cancels every _background_tasks member before adapter teardown, not only completion flushes. Current main uses that set for unrelated heartbeat and supervised tasks (gateway/run.py:10871-10887, 11090-11135).
  • tests/gateway/test_pending_registry_properties.py:13-20 skips the whole module without undeclared Hypothesis, including the ordinary unit tests.

Suggested changes

  • Evict terminal entries or cap only live entries; add a >50-completion regression.
  • Preserve/requeue retry payloads and schedule retry after the stored backoff.
  • Isolate flush-task shutdown ownership and make the deterministic tests runnable in CI.

Automated hermes-sweeper review.

Comment thread gateway/run.py Outdated
"""Create a PENDING entry. Returns Future or None if capped or duplicate."""
import asyncio as _asyncio
with self._lock:
if len(self._entries) >= self.BATCH_CAPACITY:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caps the lifetime size of _entries, but terminal entries are never removed. After 50 distinct completions, every new completion returns None here and is reported to the watcher as DROPPED_OVERFLOW. Please evict terminal entries or count only PENDING/CLAIMED entries, with a regression beyond 50 sequential completions.

Comment thread gateway/run.py Outdated
self._completion_registry.retry(reg["identity"])
finally:
self._completion_registry.resolve_futures(claimed, delivered_disposition)
for reg in claimed:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On RETRY, retry() has left this entry PENDING, but this cleanup removes the route/payload needed for a later flush. The watcher re-enqueues the same identity, which enqueue() rejects as an existing non-terminal entry, so retry becomes a silent drop. Keep/requeue the entry data and schedule the backoff retry instead.

import asyncio
import pytest

hypothesis = pytest.importorskip(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module-level importorskip skips the five ordinary unit tests as well as the Hypothesis state-machine test when Hypothesis is absent; pyproject.toml does not declare it. Move non-Hypothesis checks outside this gate and make the property-test dependency a declared, pinned dev dependency if it is required.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

Five PRs address #70300: #70319 batches the post-turn drain that ordinary completions do not traverse; #71898, #72675, #73427, and #73469 reach the watcher-owned delivery seam, with progressively broader batching or registry redesigns. The current diffs leave #71898 as the focused repaired implementation, while #73469 remains the recorded registry-based best-fix candidate but has blocking state-management defects.

Related pull requests

Duplicates

#71898, #72675, and #73469 are competing implementations of watcher-seam completion coalescing; #72675 adds separable post-turn watch-event scope, while #73469 replaces batching state with a registry. #73427 is the closed predecessor superseded by #73469; #70319 targets the same symptom at the wrong seam.

Suggested consolidation

Keep #71898 open with a salvage path: independently re-review caa398106, specifically its dedicated flush-task lifecycle ownership and both cancellation windows. Keep recorded best-fix candidate #73469 open for author action to repair retry rescheduling, terminal-entry eviction/capacity accounting, dedicated shutdown task selection, and test dependency gating; close #72675 as a duplicate of #71898 despite its keep_open review because the shared watcher fix is covered by #71898 while its broader scope and cited defects remain, and leave #70319 closed as the wrong-seam attempt and #73427 closed as superseded by #73469.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I70300(["issue #70300 (open)"])
    subgraph Dup71898 ["PRs duplicating each other"]
        P71898["PR #71898 (open)"]
        P72675["PR #72675 (open)"]
        P73427["PR #73427 (closed)"]
        P73469["PR #73469 (open)"]
    end
    P73469 -->|best fix| I70300
    class I70300 open
    class P71898 open
    class P72675 open
    class P73427 closed
    class P73469 open
    class P71898 best
    class P73469 best
    class P73469 target
    click I70300 "https://github.com/NousResearch/hermes-agent/issues/70300"
    click P71898 "https://github.com/NousResearch/hermes-agent/pull/71898"
    click P72675 "https://github.com/NousResearch/hermes-agent/pull/72675"
    click P73427 "https://github.com/NousResearch/hermes-agent/pull/73427"
    click P73469 "https://github.com/NousResearch/hermes-agent/pull/73469"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 5 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 221 kB of PR diffs, 30 kB of issue/PR text, 28 kB of discussion (28 comments), 10 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@handnewb

handnewb commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@teknium1 — all four findings confirmed and fixed. The first one was the serious one: it made this PR strictly worse than the bug it closes, and I should have caught it before opening.

Your three inline comments map onto findings 1, 2 and 4 below, so they are answered there rather than repeated.

Your findings

1. Capacity was a process-lifetime ceiling — confirmed, blocker

BATCH_CAPACITY was compared against len(_entries) while terminal entries were never evicted. Reproduced deterministically against the pre-fix registry:

UNPATCHED: completion #50 REFUSED (cap=50)

Every completion after the fiftieth was refused for the remaining life of the gateway process — the reported flood was six extra messages; this was zero messages, permanently, plus unbounded retention of evt payloads including process output.

The cap now bounds live (non-terminal) entries. Terminal entries are kept as a bounded dedupe memory and evicted FIFO past TERMINAL_RETENTION = 2048, mirroring the existing _completion_delivery_retention. Post-fix, 200 sequential completions all deliver, and live_count() returns to zero between them.

2. Retry dropped instead of retrying — confirmed, blocker

Exactly as you traced it. The shared finally unindexed every claimed entry, including the ones retry() had just returned to PENDING, so no later flush could find them; the watcher then re-enqueued and hit the non-terminal duplicate guard, which refused it. Every transient adapter failure became a silent drop and burned a live slot, so the two defects compounded.

Two changes:

  • Route and payload indexes now survive a RETRY; only entries reaching a terminal state are unindexed and resolved.
  • _schedule_completion_retry() owns the backoff wait and re-flushes the route. retry() returned a deadline that nothing read, so the documented exponential backoff never ran at all — the watcher's continue re-entered immediately with no wait. Retry is now internal to the registry, the watcher sees only terminal dispositions, and attempts exhausted resolves as CompletionDisposition.FAILED rather than looping past MAX_ATTEMPTS.

3. Shutdown cancelled every background task — confirmed

The list comprehension was named _flush_tasks but selected all of _background_tasks minus stop/restart, so heartbeat and supervised tasks were cancelled three seconds before adapter teardown. Flush tasks now go into a dedicated _completion_flush_tasks set and shutdown cancels only that, leaving the pre-existing _background_tasks cancellation after teardown untouched. The drain also uses gather(return_exceptions=True) inside one bounded wait_for instead of except (CancelledError, TimeoutError): pass, which was swallowing cancellation aimed at the stopping coroutine itself.

4. Hypothesis gating — confirmed

The module-level importorskip took the five plain unit tests down with it, so an environment without Hypothesis covered nothing. Unconditional regressions moved to tests/gateway/test_completion_registry_regressions.py (no new dependency); only the property-based state machine stays gated. 16 tests, all passing.

Defects the review did not list, found auditing my own diff

Reporting these because three of them are the same class of defect you already charged me for twice on #72675 — advertised behaviour that the code does not implement.

  • signal_stop() left waiters parked forever. It transitioned non-terminal entries to STOPPING but never resolved their futures, and SHUTTING_DOWN was defined and never used. A watcher cancelled inside the batch window awaited a Future nothing would complete — your fix(gateway): coalesce concurrent process completions and watch events #72675 round-two P1, reproduced in the redesign and relocated. Shutdown now resolves those waiters as SHUTTING_DOWN.
  • Two of eight states were unreachable. supersede() was never called, so SUPERSEDED could not occur, and duplicates fell through to mark_unroutable() — conflating two states in a state machine whose stated purpose is removing exactly that conflation. Duplicates are now SUPERSEDED. State.DROPPED_OVERFLOW was unreachable by construction (the capacity gate returns before an entry exists); it is removed from State and kept only as a caller-facing disposition, which is what it always was.
  • supersede() and mark_unroutable() had no terminal guard and could overwrite DELIVERED, contradicting "every exit from a non-terminal state is a declared transition." Both guarded, both with regressions.
  • Duplicate and over-capacity refusals returned the same None, which the caller mapped onto DROPPED_OVERFLOW. That is the Optional[bool] ambiguity this registry exists to remove, reintroduced one layer up. rejection_reason() separates them; capacity refusal is now logged.
  • Watch-event coalescing grouped on session_key alone. The completion path uses a six-field routing key with a \x00 sentinel precisely so None and "" cannot share a bucket; the watch path collapsed them and ignored platform, chat_id, thread_id and user_id, then delivered the batch via dict(group[0]). Events from different chats could coalesce into one message routed to whichever chat sorted first. It now uses _completion_notification_batch_key(), and the routing-key regression asserts against that builder rather than against hand-written tuples — the old test passed different tuples in and asserted they differed, which proved nothing.
  • Watch snippets truncated before redacting (str(...)[:100] then redact) while the completion path redacts then truncates. A credential straddling the boundary leaves a fragment the redactor no longer matches. Both paths now redact first.
  • next_attempt_at was written and never read. Removed; the deadline is returned to the scheduler that owns the wait.
  • The summary reported delivered results as lost. overflow was computed as len(entries) - MAX_DETAIL, i.e. render overflow, and printed as "dropped over capacity" — while those same entries were already inside the succeeded/failed totals. Double-counted, and it told the agent results had been lost when none had. Now "N not shown in detail".

Removed from the PR

tests/gateway/_harness.py is deleted. It shipped with ADAPT BEFORE USE — Names marked ADAPT: below are guesses at the real API and must be reconciled with gateway/run.py. Do not assume they are correct., a class _State shim, and two wrong attribute names (runner._adapters for self.adapters, runner._pending_completions.entries() for _completion_registry.snapshot()). It could not have run. Asking you to review a file that says not to trust it was not a reasonable use of your time.

Corrections to the PR description

The body has been rewritten; it was making three claims the code did not support:

  • "Observability — counters per terminal state" — removed by 7bfaa5b. There are no counters, only the structured shutdown log.
  • "No tests in this PR yet" — stale as of 4a8c3ff.
  • "Owned by the gateway lifecycle — created at start" — true of __init__, but a lazy-init guard also existed for object.__new__ tests. The guard stays, because those tests need it, and is now documented as such and kept in sync with __init__.

Still not here

  • Runner-level regressions over the flush path: cancellation inside the batch window, cancellation while the adapter is blocked, shutdown with a batch in flight. These need a controllable adapter, which is what _harness.py was reaching for and failing to be. I would rather build that once, properly, than ship a third round of hand-written probes.
  • The six-process reproduction from the issue, measured end to end. My evidence is deterministic at the registry level and code-level above it.
  • docs/gateway/delivery-contract.md.
  • Exactly-once across SIGKILL. Unchanged position: an in-memory registry cannot provide it, and adding a storage dependency is your architectural call. PENDING insertion is the write-through point if you want it later.

Converting this to draft — and recommending the other PR

Separate from the code: I opened this and #72675 for the same issue and asked maintainers to choose. That was wrong of me, and it is most of why both sat unmerged. I have made the call in #72675 instead of leaving it with you.

Recommending #72675, and moving this PR to draft. Not because the architecture here is worse — the failure modes genuinely become unrepresentable rather than patched — but because #72675 already carries CompletionDisposition, needed a 139-line fix against three findings where this needed 521 against twelve, is +327/-10 rather than +1,120/-25, and has two rounds of human review behind it. On a 25k-line file in this queue, that is the better bet even when it is not the better design.

Leaving this open as draft rather than closing it, per your keep_open verdict. The fixes above are pushed regardless, so if you would rather take this shape after all, it is in a reviewable state instead of the one you found.


Verified: git apply clean, file compiles, 16/16 regressions pass, and the >50 refusal reproduces on the pre-fix registry. Not verified: full repo suite, Ruff, and any behaviour above the registry boundary.

@handnewb
handnewb marked this pull request as draft August 3, 2026 12:23
@handnewb
handnewb marked this pull request as ready for review August 3, 2026 12:57
@handnewb

handnewb commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@GottZ — the four defects this triage cites as requiring author action were fixed and pushed five minutes before the triage comment landed, so the assessment reads the previous head. Current head is e0036cfce.

Mapping the triage's four items to the current state:

triage item status at e0036cfce evidence
terminal entries exhausting the 50-entry capacity fixed BATCH_CAPACITY bounds live entries only; terminal entries evicted FIFO past TERMINAL_RETENTION = 2048. Regression: 200 sequential completions all deliver. Pre-fix reproduction: completion #50 REFUSED (cap=50)
retry cleanup removing route/payload needed for rescheduling fixed route and payload indexes survive a RETRY; only terminal entries are unindexed. _schedule_completion_retry() owns the backoff wait, which nothing previously read
cancellation of unrelated background tasks fixed dedicated _completion_flush_tasks set; the pre-existing _background_tasks cancellation after adapter teardown is untouched
undeclared Hypothesis skipping ordinary tests fixed unconditional regressions moved to tests/gateway/test_completion_registry_regressions.py, 16 tests, no Hypothesis dependency; only the property-based state machine stays gated

A self-audit of the same diff found eight further defects, all fixed in the same push — including signal_stop() leaving waiters permanently parked, two unreachable states, and watch-event coalescing that grouped on session_key alone and could route a coalesced message to the wrong chat. Details are in the comment above.

I also converted this PR to draft earlier today, before the triage landed. That was based on the reasoning that the smaller alternative was the safer bet. Given that this triage records it as the best-fix candidate, the draft status now works against the consolidation it recommends, so I am marking it ready for review again. If a maintainer would rather it stay in draft, say so and I will put it back.

On the consolidation itself

I have no objection to the shape of it, and I am not going to argue for my own PR over @yuzilongleif-collab's.

Two things worth putting on the record:

One correction to the triage's premise

The triage describes #73427 as a case where "the advertised registry is not the mechanism used by the flush path." That was accurate for #73427 and is the reason I closed it. It is not the case for this PR: the flush path here goes through claim_batch() / deliver() / retry() on the registry, and enqueue() is the only way a completion enters. If any part of that reads as decorative at the current head, point at the line and I will fix it rather than defend it.


All three of these branches are well behind maingateway/run.py is 735 lines shorter here than on main, 1,650 shorter on #72675, 1,989 on #73547 — which is why the sweeper cites the watcher near 21264 while these diffs land near 18672–18953. Whichever PR survives consolidation needs a rebase before merge, and I would rather rebase the one you keep than all three.

@yuzilongleif-collab

Copy link
Copy Markdown
Contributor

I rechecked the current head de38265e1cd07e0b7b83fe86af2b0bbda5a9dc45 after the latest fixes. The process-lifetime capacity issue and the broad cancellation of unrelated background tasks appear addressed, but I still cannot support this head because the production shutdown path has a deterministic completion-loss race.

Blocker: cancelling a flush during adapter delivery strands a PENDING entry

When _flush_process_completion_batch() is cancelled while _deliver_completion_notification() is blocked:

  1. delivered_disposition still has its default DROP_UNROUTABLE value.
  2. The inner finally resolves the watcher with that terminal-looking disposition and removes the route/identity indexes.
  3. The outer CancelledError handler then calls retry(), moving the registry entry back to PENDING without restoring either index or scheduling another flush.

A focused production-path probe consistently produced:

disposition drop_unroutable
state pending
route_index_present False
flush_tasks 0

At that point the caller has been told to drop the completion, while the registry contains an unreachable PENDING entry. The later shutdown signal_stop() cannot recover delivery. This is the same silent-drop class the registry is intended to eliminate.

The cancellation transition needs to be atomic from the caller/registry/index perspective: either terminalize and resolve consistently, or return the claim to a routable retry state before resolving the waiter.

The new regression tests do not exercise this boundary

The added cancellation test manipulates the registry directly; it never cancels a runner flush blocked in adapter delivery. The shutdown test does not call the real runner stop() path, and its no-op sleep is overwritten during registry initialization, so it exits before the entry is claimed.

Focused runs on this head also expose existing-suite regressions:

tests/gateway/test_completion_delivery.py: 6 failed
  (old bool/None assertions were not updated for CompletionDisposition)

tests/gateway/test_gateway_shutdown.py: 12 failed, 9 passed
  (shutdown unconditionally accesses _completion_registry on object.__new__ fixtures)

The property file is also skipped in the declared dev environment because hypothesis is imported through pytest.importorskip() but is not declared in the dev dependencies. With Hypothesis injected separately, its 9 tests pass, but CI does not currently run them.

Please add a blocked-adapter cancellation regression through the real flush/stop path and make the focused existing suites green before treating the four original blockers as fully closed.

@alt-glitch alt-glitch added tool/terminal Terminal execution and process management needs-decision Awaiting maintainer decision before any implementation and removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 6, 2026
@handnewb
handnewb requested a review from a team August 6, 2026 15:08
@handnewb

handnewb commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @yuzilongleif-collab

You're right, and the diagnosis is exact. Thanks for the production-path probe — it isolated something my tests were structurally unable to catch.

The root of it was a terminal default: delivered_disposition started as DROP_UNROUTABLE, so "not yet decided" and "decided to drop" were the same value. That is the Optional[bool] ambiguity I set out to remove, reintroduced in an initialiser. Cancellation then split the transition across an inner finally and an outer handler, in that order, non-atomically.

What changed:

  • The outcome variable is CompletionDisposition | None, None meaning undecided, and reaching the inner finally with undecided now raises AssertionError rather than resolving. No terminal default exists.
  • The cancellation path returns the claim to a routable PENDING through the registry's retry() transition, schedules a re-flush, and only then resolves the waiter — with RETRY, never a drop. The inner finally no longer mutates indexes; index lifecycle belongs to the registry transitions.
  • signal_stop() already scans the authoritative entry map (self._entries) rather than a route index — that was already correct.
  • New regression test: starts a real _flush_process_completion_batch, blocks the adapter mid-delivery, cancels the task, and asserts the waiter got RETRY, the entry is not UNROUTABLE, and the route index survived. Plus a defense-in-depth test asserting signal_stop() finds an entry missing from the route index.

On the existing suites: both are green now:

  • test_completion_delivery.py: assertions updated to CompletionDisposition rather than relaxed. is Trueis CompletionDisposition.DELIVERED, is Noneis CompletionDisposition.DROP_UNROUTABLE, is Falseis CompletionDisposition.RETRY.
  • test_gateway_shutdown.py: make_restart_runner() now initializes _completion_registry so stop() doesn't crash on the missing attribute.

hypothesis is now declared in dev dependencies and importorskip is gone.

$ python3 -m pytest tests/gateway/test_completion_registry_regressions.py tests/gateway/test_completion_runner_regressions.py tests/gateway/test_pending_registry_properties.py tests/gateway/test_completion_delivery.py tests/gateway/test_gateway_shutdown.py -q
...............................................................          [100%]
63 passed in 28.86s

@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Aug 6, 2026
handnewb and others added 11 commits August 6, 2026 17:47
…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>
… 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>
A.4 — Process stdout/stderr tails in _format_coalesced_process_completions
were not routed through secret redaction. The watch_match snippet path
already had redaction (T6); completion tails (up to 800 chars × 10 entries)
did not.

Now routes through _redact_gateway_user_facing_secrets before truncation.
Same patterns covered: Authorization: Bearer, PGPASSWORD=,
AWS_SECRET_ACCESS_KEY=, --token=.

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

Production change (one seam):
- Add self._batch_window_sleep = asyncio.sleep to GatewayRunner.__init__
- Use self._batch_window_sleep() instead of direct asyncio.sleep() in
  _flush_process_completion_batch, so the VirtualClock can inject
  deterministic time control during tests.

Tests:
- test_pending_registry_properties.py: 5 unit checks (transition guards)
  over PendingCompletionRegistry isolated state machine. Exercises
  prohibited transitions: deliver-without-claim, already-claimed rejection,
  terminal immutability, None-vs-empty routing key sentinel, overflow
  counting. Uses pytest.importorskip('hypothesis') for the stateful
  property test class (pending event-loop setup, skipped for now).
- _harness.py: deterministic test harness (VirtualClock +
  ControllableAdapter + GatewayHarness) for the completion-delivery
  path. ADAPT markers remain where the harness references production
  names to be reconciled after the full migration.
… tests

- Fix CompletionStore | None -> 'CompletionStore | None' (string annotation
  needed because CompletionStore is a sibling inner class)
- Fix NullCompletionStore() -> None (same scoping issue)
- Add _batch_window_sleep seam (injectable clock for deterministic tests)
- Reconcile property tests with real API:
  - enqueue(identity, route_key, payload) returns Future|None
  - claim_batch(identities, batch_id) returns (claimed, skipped)
  - deliver/retry/supersede/mark_unroutable/signal_stop
  - State enum via PendingCompletionRegistry.State
  - Use asyncio.run() wrappers (enqueue needs get_running_loop())

5 unit tests PASSED, hypothesis stateful tests ready (needs running loop)
…in _deliver_completion_notification

_deliver_completion_notification() now returns CompletionDisposition
instead of Optional[bool], closing the root cause of the original
'None silently consumes a completion' finding from review round 1.

- True -> CompletionDisposition.DELIVERED
- False -> CompletionDisposition.RETRY
- None -> CompletionDisposition.DROP_UNROUTABLE

Call-sites updated: _async_delegation_watcher, _flush_process_completion_batch
(was already partially migrated), _run_process_watcher (was already correct).

Also rename _completion_notification_batch_tasks -> _flush_tasks_by_route:
this dict stores asyncio.Task handles per route (not completion state),
so the old name was misleading. The registry owns entry state; this dict
owns flush lifecycle — preventing duplicate tasks and enabling cleanup.
Both are additive rather than required to fix NousResearch#70300. Reviewing them
costs maintainer attention the core change needs. The structured shutdown
log stays — without it STOPPING is indistinguishable from a silent drop.

CompletionStore offered as follow-up in the PR body instead.

Co-authored-by: yuzilongleif-collab <235949691+yuzilongleif-collab@users.noreply.github.com>
enqueue() was silently overwriting existing entries regardless of state,
allowing a second enqueue of an identity still in PENDING/CLAIMED.
The state-machine contract (and its property test) expects re-enqueue
to be accepted only when the prior entry is in a terminal state.

Add a guard: if the identity exists and is not terminal, return None
to the caller — matching the existing capacity-overflow contract.
…, dead states

Sweeper review (teknium1) plus a self-audit of the same diff.

Blockers:
- BATCH_CAPACITY compared against len(_entries) while terminal entries
  were never evicted, so completion 51 and every one after was refused
  for the life of the process. Cap now bounds live entries; terminal
  entries are a FIFO dedupe memory bounded by TERMINAL_RETENTION.
- RETRY left the entry PENDING but the shared finally deleted its
  route/payload indexes, so no flush could find it and the watcher's
  re-enqueue hit the non-terminal duplicate guard. Indexes now survive a
  RETRY and _schedule_completion_retry owns the backoff wait, which
  nothing previously read.
- The pre-teardown drain cancelled every _background_tasks member.
  Flush tasks now live in a dedicated _completion_flush_tasks set.
- Module-level importorskip skipped the plain unit tests along with the
  Hypothesis ones. Unconditional regressions moved to
  test_completion_registry_regressions.py.

Also:
- signal_stop() transitioned entries to STOPPING without resolving their
  futures; SHUTTING_DOWN was never used. Waiters are now resolved.
- SUPERSEDED and State.DROPPED_OVERFLOW were unreachable; duplicates now
  resolve as SUPERSEDED and DROPPED_OVERFLOW is caller-facing only.
- supersede()/mark_unroutable() could overwrite a terminal state.
- Duplicate and over-capacity refusals both returned None; separated via
  rejection_reason().
- Watch-event coalescing grouped on session_key alone, collapsing None
  and  and ignoring platform/chat_id/thread_id/user_id, so events from
  different chats could merge into one misrouted message. Now uses the
  full routing key.
- Watch snippets truncated before redacting.
- The summary reported render overflow as 'dropped over capacity' while
  those entries were delivered and already counted.
- Removed tests/gateway/_harness.py: it shipped with ADAPT BEFORE USE and
  two wrong attribute names and could not run.

Review: teknium1 (NousResearch#73469)
…egistry

Adds coverage above the pure registry boundary for the three scenarios
the PR description declared as not measured:

- Cancelled flush leaves entries recoverable (CLAIMED → PENDING via
  retry(), reclaimable by a later batch)
- Shutdown with in-flight batch resolves waiters as SHUTTING_DOWN via
  signal_stop() + resolve_futures()
- Six processes coalesce into one adapter call — the original NousResearch#70300
  reproduction measured against the real _flush_process_completion_batch

20/20 tests pass (16 existing + 4 new).
… shutdown

BLOCKER NousResearch#73469 (yuzilongleif-collab): terminal default (DROP_UNROUTABLE) +
inner-finally-before-outer-handler ordering resolved the waiter with a drop,
removed route indexes, and left the registry entry PENDING but unreachable.

Root cause: 'not yet decided' (default DROP_UNROUTABLE) and 'decided to drop'
were the same value — the Optional[bool] ambiguity reincarnated.

Changes:
- outcome: CompletionDisposition | None = None (undecided ≠ terminal)
- CancelledError caught inside inner try: single-owner atomic transition
  restores entries to routable PENDING, schedules re-flush, resolves with
  RETRY before the inner finally runs
- Inner finally skips when cancelled_path_taken (handler already did work)
- AssertionError when finally reached with undecided outcome (logic bug)
- signal_stop already scans authoritative _entries map, not route index

Tests:
- Cancel blocked-adapter flush through real production path: asserts waiter
  gets RETRY, entry is not UNROUTABLE, route index survives cancellation
- signal_stop finds entry orphaned from route index (defense in depth)

Existing suite:
- test_completion_delivery.py: 6 assertions updated from Optional[bool] to
  CompletionDisposition (is True→is DELIVERED, is None→is DROP_UNROUTABLE,
  is False→is RETRY)
- test_gateway_shutdown.py: make_restart_runner() now initializes
  _completion_registry so stop() doesn't crash on missing attribute
- hypothesis declared in dev deps; importorskip removed from property tests
@handnewb
handnewb force-pushed the registry/pending-completion-state-machine-v2 branch from 732928d to 9cb297b Compare August 6, 2026 20:49
Resolve pyproject.toml conflict:
- Keep hypothesis>=6.100 from PR (test dependency for property-based tests)
- Upgrade aiohttp 3.14.1 → 3.14.3 from main (newer CVE patches)
- Keep updated CVE comment from main
pierrenode added a commit to pierrenode/hermes-agent that referenced this pull request Aug 19, 2026
…ation hardening

This week's completion-notification hardening series (b9e7bea, c0d2048,
84b4fb9, a96cd10, 7619564, 8dc9401) forced secret redaction, a
spawning-session-boundary pre-flight, and title/compression filtering onto
completion/async_delegation notifications. watch_match/watch_disabled/
watch_overflow_* events were left on the old, weaker behavior in three
places — plus a fourth, related gap found while implementing the above:
watch-event notifications weren't attributed to the subagent that owned the
watched process.

1. Redaction: _format_gateway_process_notification (gateway/run.py) and the
   shared format_process_notification (tools/process_registry.py, also used
   by the TUI gateway) rendered watch_match's output/command from the
   producer-side, non-forced _redact_process_result pass only. A user with
   security.redact_secrets: false would get a raw secret sent straight to
   the chat platform if a watch pattern matched a line containing one. Both
   formatters now apply the same forced, unconditional redaction floor the
   completion path already has.

2. Session-boundary gate: _drain_watch_notifications called
   _inject_watch_notification directly, bypassing _classify_completion_target
   entirely. A watch_match/watch_disabled event from a process spawned in
   session A could still land in session B's chat after /new closed A.
   ProcessSession.parent_session_id (already stamped at spawn time) is now
   also carried on watch_match/watch_disabled events and checked before
   injection. Unstamped/global events (the cross-session overflow summaries)
   keep delivering unconditionally, matching completion's own legacy
   fallback. Watch events have no watcher to re-poll them later, so a
   "retry" (transient DB uncertainty) verdict fails open and delivers rather
   than losing the match outright.

3. Title/compression filters: title_generator._is_real_user_turn and
   context_compressor._is_synthetic_compression_user_turn didn't recognize
   any of the "[IMPORTANT: ...]"/"[ASYNC DELEGATION ...]" notification
   shapes — only the unrelated compaction/continuation/model-switch markers.
   Both now check message.get("display_kind") == "internal_notification"
   (the structural marker gateway/run.py stamps at persist time) as the
   primary signal, plus explicit text-prefix entries for the raw-string call
   paths that don't have a message dict to check. Async-delegation
   completions are excluded from the compression side of this: unlike
   watch/background-process bookkeeping, _format_async_delegation's own
   docstring says the block carries "the complete result summary" — genuine
   actionable content a real user turn would also carry — so treating it as
   synthetic would let compaction blank out a delegation's actual result
   (see test_completion_survives_compaction_verbatim_after_blank_echo,
   bc48241). They're still excluded from titling, where the boilerplate
   wrapper text would make a bad title regardless of the payload.

4. Subagent attribution: completion/async_delegation events resolve their
   task_id (via tools/delegate_tool.py's _active_subagents registry) into a
   "Started by subagent ... of delegation ... Task: ..." provenance line.
   watch_match already carried this; watch_disabled never did, in either
   formatter. Fixed by stamping task_id on the watch_disabled event dict and
   adding the same attribution lookup to both formatters — gateway/run.py's
   kept additive (not delegated to the shared formatter) to avoid silently
   swapping its _redact_gateway_user_facing_secrets guarantee for
   redact_terminal_output.

Mutation-verified throughout, including the async-delegation exclusion
against both the original wrong code and each half of the fix independently
— all reproduce test_completion_survives_compaction_verbatim_after_blank_echo
failing. Full neighbor sweep green (tests/gateway/, tests/agent/
compress/compaction suite, tests/tools/test_watch_patterns.py,
tests/tools/test_process_registry.py, tests/tools/test_async_delegation.py).
ruff clean.

Adjacent open PRs checked, no semantic overlap:
- NousResearch#75719 restructures the same gateway/run.py formatter for an unrelated
  concern (a "supersession context" note on delayed notifications) —
  textual proximity only.
- NousResearch#61719 adds a different field (origin_ui_session_id) to the same
  watch_match/watch_disabled dict literals, for TUI/WebUI tab ownership —
  complementary, not overlapping.
- NousResearch#73469 is an alternative architecture for the same-tick
  completion-coalescing race this week's series already solved differently;
  different function region.
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:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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.

[Bug]: Multiple background process completions in same tick flood session with individual notifications instead of coalescing

5 participants