fix: hold completion notifications until the parent run settles - #186
Open
vincelwt wants to merge 1 commit into
Open
fix: hold completion notifications until the parent run settles#186vincelwt wants to merge 1 commit into
vincelwt wants to merge 1 commit into
Conversation
A background agent's completion notification is sent with `deliverAs: "followUp"`, which pi delivers only once the agent has no more tool calls. Emitting one mid-run parks it in pi's follow-up queue until the run ends, and a parked message can no longer be withdrawn — so an agent the orchestrator joined with `get_subagent_result` in the meantime still notifies after the final answer. The `resultConsumed` guard cannot prevent this: it runs when the notification is enqueued, 200ms after completion, while the join happens minutes later. With pi's default `followUpMode: "one-at-a-time"` those stale notifications then drain one turn each. Notifications now wait in-process instead of in pi's queue. A due notification is delivered immediately when the parent is idle, and otherwise parked until `agent_settled` — the first point where pi will not continue on its own. Since each send closure already re-checks `resultConsumed`, deferring the call defers the check, so a joined agent simply never notifies. Busy state is read from `ctx.isIdle()` at delivery time rather than tracked locally, so an unbalanced lifecycle event cannot strand notifications.
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #185.
The problem
emitIndividualNudgere-checksrecord.resultConsumedbefore sending, but that check runs when the message is handed topi.sendMessage— 200ms after completion (NUDGE_HOLD_MS). Delivery happens much later:deliverAs: "followUp"is only delivered once the agent has no more tool calls, so a notification emitted mid-run sits in pi's follow-up queue until the run ends, where it can no longer be withdrawn. The orchestrator usually joins the agent withget_subagent_resultsomewhere in that gap, so the notification is already stale by the time it lands — after the final answer, costing a turn to answer "already incorporated". With pi's defaultfollowUpMode: "one-at-a-time", a batch drains one turn each.The change
src/nudge-queue.ts(new, ~100 lines) takes over the timing that was inline inindex.ts:agent_settled.agent_settledrather thanagent_endorturn_end: those fire with a retry, auto-compaction, or another tool-calling turn still ahead, so a notification emitted there is parked by pi exactly as before.agent_settledis the first point where pi won't continue on its own.The suppression logic itself is untouched. Every send closure already re-checks
resultConsumed(and the group callback re-filters unconsumed records), so deferring the call defers the check — an agent joined in the meantime simply never notifies. No new suppression path to keep in sync.Busy state is read through
ctx.isIdle()at delivery time rather than tracked as a local flag, so an unbalanced lifecycle event can't strand notifications; an absent ctx falls through to immediate delivery, i.e. today's behaviour.dispose()drops everything undelivered, matching the existingsession_shutdownsemantics (results are undeliverable once the session is gone, which is why shutdown already callsabortAll()).Notes
scheduleNudge/cancelNudgekeep their signatures and call sites; they're now thin wrappers, which keeps the diff inindex.tssmall.group:key. Their callback already re-filters onresultConsumedand returns early when nothing is left unconsumed, so a fully-joined group now correctly produces no notification at all.group-join.tsandstatus-note.tsare structured.Tests
10 new tests in
test/nudge-queue.test.ts(fake timers, in the style ofgroup-join.test.ts): idle delivery, mid-run parking, flush, cancellation while parked (the actual regression), cancellation inside the hold window, busy state re-read at delivery rather than schedule time, same-key replacement, flush-once, one throwing send not blocking the rest, and dispose dropping both parked and in-window notifications.npm run lint,npm run typecheck,npm test(765 passed, 5 skipped),npm run buildall pass.I left
CHANGELOG.mdalone per CONTRIBUTING. Nothing in the README describes the timing being corrected here, so I didn't touch it either — happy to add a line if you'd like one.