fix(gateway): coalesce concurrent process completions

[yuzilongleif-collab]

Summary

• coalesce concurrent notify_on_complete process completions for the same gateway route into one synthetic turn
• preserve the original rich notification for a single completion
• retain per-process lifecycle dedupe and adapter retry semantics for every member of a batch
• keep different conversations, watch_match, and async-delegation ownership isolated

Fixes #70300.

Root cause

Each _run_process_watcher() directly called _deliver_completion_notification(). When several terminal sessions completed together, each watcher independently injected an internal message through adapter.handle_message(), producing one agent turn per process.

Standard process completions are owned by the per-process watcher path; the post-turn gateway drain only returns watch_match events. Coalescing after that drain therefore cannot affect the production completion path.

Implementation

• add a short (100 ms), route-scoped completion batch at the actual _run_process_watcher() delivery seam
• make every watcher await the shared batch result, so adapter failures continue through the existing retry loop
• record every coalesced completion identity after successful delivery
• detach a flushing batch before adapter I/O so a completion arriving during delivery schedules the next flush instead of being stranded
• skip a lifecycle-duplicate primary and try the next fresh batch identity
• bound aggregate output to 10 detailed results with 800-character tails, plus an omitted-count summary

A single completion still uses its original pre-redacted rich notification; only concurrent same-route completions take the aggregate format.

Tests

uv run pytest -q \
  tests/gateway/test_completion_delivery.py \
  tests/gateway/test_background_process_notifications.py \
  tests/gateway/test_internal_event_bypass_pairing.py \
  tests/gateway/test_cancel_background_drain.py \
  tests/gateway/test_multiplex_background_task_scope.py

76 passed

New regression coverage proves:

• three real concurrent _run_process_watcher() tasks produce one synthetic turn
• different route keys never coalesce
• batch delivery failure retries every member
• every successful member is present in the lifecycle ledger
• a duplicate primary cannot discard a fresh sibling
• formatter failure resolves waiters for retry
• a completion arriving during adapter delivery creates a second flush

Concurrency/race-focused tests were also repeated 20 times without failure.

Static checks:

ruff check gateway/run.py tests/gateway/test_completion_delivery.py
python -m py_compile gateway/run.py tests/gateway/test_completion_delivery.py
git diff --check origin/main...HEAD

All passed on Ubuntu Linux with Python 3.11. Type diagnostics introduced by this patch: 0 (the touched-file baseline had 145 existing diagnostics; patched tree had 144).

Compatibility notes

• single completions gain up to 100 ms of notification latency
• completion notifications remain lifecycle-scoped/non-durable, as before
• async delegation and watch_match delivery paths are intentionally unchanged

Related prior attempt: #70319. This PR places batching on the per-process watcher ownership path and adds end-to-end watcher coverage.

[alt-glitch]

This was generated by AI during triage.

Related: #70319 also targets completion flood for #70300, but its current diff batches the post-turn watch-queue drain. This PR batches standard completions at the watcher delivery seam, with retry and lifecycle-dedupe coverage; the approaches are complementary rather than duplicates.

[handnewb]

Really solid implementation — the batching at the watcher delivery seam with Future-based retry/dedupe is exactly the right pattern. The 100ms route-scoped batch, flush-during-delivery handling, and duplicate-primary fallback are all well thought out.

I've opened #72675 which builds directly on your approach and adds:

• watch_match/watch_disabled coalescing at the post-turn drain — the other half of [Bug]: Multiple background process completions in same tick flood session with individual notifications instead of coalescing #70300 that was still flooding the agent
• Threshold-based early flush: 5+ entries → deliver immediately, no need to wait out the 100ms window
• Visual status indicators (✅/❌), aggregate summary ("3 succeeded ✅, 1 failed ❌"), elapsed time per process in the batch format
• Zero-latency single completions: single completions pass through without waiting for the batch window
• Extended _format_gateway_process_notification() to handle standard completion events

The watcher-path batching follows the same Future-based pattern you established — just with the additions above. Would be great to get your review on the combined approach!

[handnewb]

@yuzilongleif-collab — wanted to drop a quick thank-you.

Your review on #70319 was the key insight: standard completions don't flow through the post-turn drain, they're owned by _run_process_watcher(). Without that catch, we'd have shipped a fix that never executes on the real code path.

Your #71898 also established the right pattern — Future-based batching at the watcher seam, route-scoped keys, dedup/retry preservation, flush-during-delivery handling. #72675 follows that same architecture, just extended to also cover watch_match/watch_disabled events at the post-turn drain (the other half of the flood problem) and added threshold-based early flush + aggregate summaries.

Solid collaboration — your debugging saved us from a silent failure. Thanks!

[yuzilongleif-collab]

Thanks — I really appreciate the thoughtful follow-up and the clear attribution. I’m glad tracing the production watcher path caught that disconnect before it shipped.

The way #72675 extends the same batching model across watcher-owned completions and post-turn watch events makes sense, while preserving the important delivery, retry, and dedupe invariants. The threshold-based early flush and aggregate summaries also look like a strong continuation of the approach.

Great collaboration — and thanks again for taking the time to connect the work so clearly.

[teknium1]

Thanks for tracing the standard-completion path to the watcher seam; that premise is correct on current main. gateway/run.py:21299-21305 still directly delivers each exited watcher's completion, while the post-turn drain explicitly excludes ordinary completions at gateway/run.py:3090-3114.

Problems

• The new flush task is created without lifecycle ownership in the PR's _enqueue_process_completion_notification(). Current gateway tasks are retained in _background_tasks at gateway/run.py:5866-5867 and shutdown cancels that set at gateway/run.py:12230-12240. The PR also has no CancelledError path: cancellation during the batching sleep leaves queued futures unresolved; cancellation during adapter delivery can resolve them as None, which the watcher does not retry.
• The added tests cover normal fan-in and ordinary failures, but not either cancellation window or shutdown with a pending batch.

Suggested changes

• Make flush tasks lifecycle-owned and settle every waiter retryably on cancellation before adapter teardown.
• Add deterministic tests for cancellation during the window and during blocked adapter delivery.

This is an automated hermes-sweeper review.

[yuzilongleif-collab]

Addressed the requested flush-task lifecycle/cancellation gaps in caa398106 and rebuilt the branch on current main after the upstream history rewrite.

What changed:

• retain every delayed flush in both GatewayRunner._background_tasks and a dedicated flush-task set;
• cancel and await all flushes before adapter teardown;
• settle every batch waiter with retryable False when cancellation lands during the fan-in sleep or blocked adapter delivery;
• stop accepting new completion batches once shutdown begins;
• preserve same-route overlap: an older in-flight delivery and a newer fan-in task are both lifecycle-owned and cancelled;
• keep duck-typed / object.__new__ test runners compatible.

Verification:

• focused + adjacent lifecycle tests: 44 passed;
• cancellation race stress: 100 passed (5 lifecycle/shutdown tests × 20);
• Ruff 0.15.10 and git diff --check: passed;
• full tests/gateway comparison: branch 4444 passed, 11 failed; clean origin/main 4433 passed, 11 failed, with the identical 11 environment/baseline failures (missing Discord extra, local DNS/IPv6 environment, and existing state/status assertions).

The new deterministic tests cover cancellation inside the real fan-in wait, cancellation while adapter delivery is blocked, shutdown ordering before disconnect, overlapping flushes for one route, successful task-reference cleanup, and the documented 10-entry summary bound.

[GottZ]

This was generated by AI during triage.

Summary

Five PRs address #70300. #70319 batches an unreachable post-turn path; #71898 batches at the verified watcher-owned completion seam and now owns cancellation/shutdown lifecycle; #72675 broadens that approach but retains reviewed correctness defects; and #73427/#73469 pursue a larger registry redesign, with #73427 superseded and #73469 still carrying blocking state-management defects.

Related pull requests

• fix: coalesce multiple background process completion notifications by session_key #70319 [closed] related — (+317/-8) — does not fix the reported path: the diff coalesces events after _drain_gateway_watch_events(), while ordinary completions are delivered directly by _run_process_watcher() and never reach that drain. It remains relevant as the closed wrong-seam predecessor that led to the watcher-seam implementations.
• fix(gateway): coalesce concurrent process completions #71898 best fix — (+615/-2) — best-fix candidate with a concrete salvage path: the diff batches route-scoped completions at _run_process_watcher() and tests fan-in, retry, dedupe, route isolation, overlap, cancellation, and shutdown. Consistent with the automated keep_open review, commit caa398106 addresses its cited lifecycle gaps by owning flush tasks, cancelling them before adapter teardown, and resolving both cancellation windows as retryable; an independent re-review of that head remains the appropriate gate.
• fix(gateway): coalesce concurrent process completions and watch events #72675 duplicate — (+1255/-9) — overlapping but broader partial fix: it reaches the watcher seam and additionally coalesces post-turn watch events, but the contributor keep_open review identifies adapter cancellation mapped to DROP_UNROUTABLE, stale bool assertions after the CompletionDisposition change, and shutdown handling that selects unrelated background tasks. Despite that keep_open review, consolidation under fix(gateway): coalesce concurrent process completions #71898 is warranted because fix(gateway): coalesce concurrent process completions #71898's current diff implements the shared watcher fix with dedicated flush-task ownership and deterministic lifecycle tests, while no supplied delta demonstrates equivalent repairs here.
• feat(gateway): PendingCompletionRegistry — explicit state machine for completion delivery #73427 [closed] fixes — (+1399/-9) — superseded registry attempt: it reaches the watcher seam through stacked pointwise batching, but the proposed registry is not the mechanism actually used by the flush path. It remains relevant because it was closed in favor of the clean-branch replacement feat(gateway): PendingCompletionRegistry — root-cause fix for #70300 #73469.
• feat(gateway): PendingCompletionRegistry — root-cause fix for #70300 #73469 best fix — (+1120/-25) — registry-based partial fix: it coalesces at the correct watcher seam, but the contributor keep_open review shows that terminal entries exhaust the 50-entry capacity, retry cleanup removes the route/payload needed for rescheduling, shutdown cancels unrelated background tasks, and undeclared Hypothesis skips ordinary tests. Despite its recorded best-fix verdict and keep_open review, the diff-specific silent-drop and lifetime-cap defects make it less salvageable for this issue than fix(gateway): coalesce concurrent process completions #71898's repaired focused path.

Duplicates

#71898, #72675, and #73469 are competing implementations of the same watcher-seam completion coalescing; #72675 adds post-turn watch-event scope, while #73469 replaces batching state with a registry. #73427 is the closed, superseded predecessor of #73469; #70319 seeks the same outcome but is not functionally equivalent because it modifies the wrong path.

Suggested consolidation

Keep #71898 open with a salvage path: obtain an independent re-review of caa398106, specifically exercising the newly lifecycle-owned flush tasks and both cancellation windows. Then close #72675 as a duplicate of #71898 because its additional watch-event scope is separable and its contributor keep_open defects remain unresolved in the supplied evidence; close #73469 as a duplicate of #71898 despite its recorded best-fix and keep_open verdicts because its diff still contains concrete post-50-completion rejection, retry-time silent drops, unrelated-task cancellation, and skipped-test defects. Keep #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
    P71898 -->|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 P71898 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]

Independent re-review of caa398106, which the 2026-08-03 triage names as the remaining salvage gate on this PR. I am the author of #72675 and #73469, so I have an obvious interest here — I have tried to write this as the review I would want on my own branch, and the two defects below are ones I would want raised against mine.

Short version: the lifecycle work is correct, and better than what I had in #72675 on three counts. Two real defects, one of them a message-delivery boundary.

What is right

Cancellation is retryable in both windows. The except asyncio.CancelledError sets delivered = False, recovers entries if the batch had not yet detached, and re-raises; the finally then resolves every waiter with False, which is the path _run_process_watcher retries. This is the defect the sweeper found in #72675, where CancelledError bypassed except Exception but still reached a finally that mapped delivered=None through _bool_to_disposition to DROP_UNROUTABLE. This branch does not have that hole.

Flush-task ownership is properly scoped. _cancel_process_completion_batch_tasks() drains _completion_notification_batch_flush_tasks and nothing else, with one gather(..., return_exceptions=True). Both #72675 and #73469 originally selected from _background_tasks and cancelled startup-resume and supervised watcher tasks before adapter teardown. This branch never had that bug.

The shutdown gate is something neither of mine had. _completion_notification_batches_stopping is checked in _enqueue_process_completion_notification, so a completion arriving mid-teardown cannot schedule a new flush behind the drain. The orphan sweep afterwards — resolving any queue that has no live flush task, then clearing — closes the window I left open.

Per-attribute hasattr guards. Worth pointing out explicitly, because #72675 put five attributes behind one guard on _completion_notification_batches, so a runner that pre-initialised some of them skipped the rest and raised AttributeError: _completion_notification_batch_max. This branch guards each independently and does not have that trap.

Detach-before-delivery is right, and the duplicate-primary loop that tries the next batch identity so a fresh sibling is not discarded with a duplicate is the correct behaviour.

Defect 1 — the routing key cannot distinguish None from ""

return tuple(str(evt.get(field) or "") for field in (
    "session_key", "platform", "chat_type", "chat_id", "thread_id", "user_id",
))

str(None or "") and str("" or "") are both "". Two events whose routes differ only in that one field is absent on one and empty on the other therefore produce an identical key and coalesce.

The consequence is not a wrong-looking message, it is a lost one. Delivery iterates candidates and breaks on the first non-None result:

for _text, candidate_evt, _future in entries:
    delivered = await self._deliver_completion_notification(synth_text, candidate_evt)
    if delivered is not None:
        break

So one route receives the consolidated text and the other route's waiter is resolved True while its chat receives nothing. Silent, and it looks like success in the ledger.

#72675 handles this with a sentinel rather than or "":

_SENTINEL = "\x00"
return tuple(
    _SENTINEL if evt.get(field) is None else str(evt.get(field))
    for field in (...)
)

Any distinct marker works; the requirement is only that absent and empty do not converge. It is a two-line change and I would rather see it here than argue for my PR over it.

Defect 2 — the coalesced formatter emits unredacted output

output = str(evt.get("output") or "").strip()
if len(output) > 800:
    output = f"[… truncated …]\n{output[-800:]}"

evt["output"] comes from _run_process_watcher, where it is passed through redact_terminal_output() without force=True. agent/redact.py returns the text unmodified when security.redact_secrets is disabled, so with that setting off this formatter appends up to 800 raw characters per process into a synthetic turn that goes to the chat platform.

The docstring says "several redacted completions", which is true only when the setting is on.

Two ways to close it, not mutually exclusive:

1. #73547 adds the unconditional gateway floor to _out before completion_evt is built, which makes evt["output"] force-redacted at the source and this formatter safe. That PR is salvageability: high and independent of the consolidation — it is effectively a safety dependency of this branch, which is an argument for landing it regardless of which coalescing PR wins.
2. Defence in depth in the formatter itself: output = _redact_gateway_user_facing_secrets(output) before truncating. Note the order — redacting after slicing can leave a credential fragment the pattern no longer matches.

Two smaller notes

No cap on batch size. _completion_notification_batches[key] grows without bound inside the window. #72675 caps it at 50 with the overflow counted in the summary. Probably an edge case rather than a real risk at the observed rates, but the formatter already bounds rendering at 10 entries, so bounding the queue is consistent with that.

Watch events are untouched. watch_match / watch_disabled are forwarded individually by _drain_gateway_watch_events() and have the same flood shape for a different reason. That is out of scope here and I am not suggesting it be added — I am noting it so the issue is not treated as fully closed when this merges. If #72675 closes as a duplicate, I will resubmit that scope separately, with the same sentinel fix, since the version in #72675 groups on session_key alone and is worse than what is described above.

On the consolidation

For the record, since I am the competing author: I have no objection to this branch being the one that lands. The lifecycle handling here is cleaner than mine, and the two defects above are contained. If it helps, I can open a PR against this branch with the sentinel and the formatter redaction rather than leaving them as review comments.

---

Method: read gateway/run.py at caa398106. _completion_notification_batch_key, _flush_process_completion_batch, _cancel_process_completion_batch_tasks and _enqueue_process_completion_notification read in full. Defect 1 is a property of the expression, not an observed failure; Defect 2 is traced from the redact_terminal_output call signature in _run_process_watcher and agent/redact.py's disabled-path early return, not measured on a live gateway. I have not run this branch's tests.

[yuzilongleif-collab]

Thanks for the careful review. I pushed 2af3536 to address the forced-redaction finding.

Addressed: defence-in-depth redaction at the coalesced formatter

_format_coalesced_process_completions() now applies _redact_gateway_user_facing_secrets() to every rendered output before the 800-character tail bound. Redaction deliberately happens before truncation so slicing cannot remove the prefix/key needed by the authoritative secret patterns.

Two regressions were added with agent.redact._REDACT_ENABLED = False:

• a process environment-style token must still be removed while ordinary output remains visible;
• a long secret whose recognizable key would be cut off by tail truncation must not leave its value suffix behind.

Verification:

pytest -q tests/gateway/test_completion_delivery.py
22 passed

Challenged: missing vs empty optional route fields

I did not add a missing-value sentinel to _completion_notification_batch_key(), because the current delivery semantics explicitly normalize those values to the same route:

thread_id=str(evt.get("thread_id") or "").strip() or None
user_id=str(evt.get("user_id") or "").strip() or None

That is the normalization used by _build_process_event_source(). The batch key is also a fixed six-element tuple, so optional fields cannot shift across positions. Consequently, missing/None/empty optional values coalesce only where the actual SessionSource also treats them as the same destination. Adding a sentinel would split semantically identical routes; it would not prevent a real cross-route merge.

The inverse mismatch is fail-safe: values such as "Telegram" vs "telegram" may currently form separate batches even though source construction lowercases them, which only reduces coalescing and does not misroute a completion.

Safety boundary with #73547

This commit adds defence in depth specifically at the multi-completion formatter. #73547 remains the broader source/direct-egress safety dependency for single completions and direct background-process notifications, so it should still land before or alongside this PR.

I have left the suggested batch-size cap as a non-blocking follow-up rather than expanding this correctness fix.

[teknium1]

Thanks @yuzilongleif-collab — your per-route completion fan-in landed in PR #85938 (rebase-merged) with all three of your commits and authorship preserved via cherry-pick, extended with async-delegation batch grouping on top. Closing this original; #70300 is fixed largely by your work.