fix(sessions): fire cloud completion notification once per turn#3091
Conversation
Cloud tasks rang duplicate completion notifications — two back-to-back "meep" sounds and two stacked toasts every time a turn finished. The completion notification for cloud runs is fired from `updatePromptStateFromEvents` on every live `turn_complete` entry, with no per-turn idempotency. Cloud SSE streams can re-deliver the `turn_complete` tail (durable-stream re-emit or a reconnect replay), and each re-delivery arrives with a fresh `totalEntryCount`, so it slips past the `processedLineCount` dedup guard and calls `notifyPromptComplete` again — back-to-back, hence the rapid double. The notification bus refactor (#2934) did not introduce this; it only unmasked it. The old path delivered the focused-elsewhere tier as a native OS notification (which the OS coalesces into one banner); the new path stacks in-app toasts and replays a JS completion sound per call, so the pre-existing double became visible and audible. Fix: make the cloud completion notification idempotent per turn. A run is armed for exactly one completion notification when its `session/prompt` is processed (snapshot or live, so opening a task mid-turn still notifies) and the arm is consumed when `turn_complete` fires. A re-delivered `turn_complete` finds nothing armed and rings nothing. This survives the JSON-RPC stopReason response clearing isPromptPending / currentPromptId (so a simple pending-flag guard would not work), and fixes the toast, native, and sound tiers uniformly since they all flow from the single `notifyPromptComplete` call. Local runs were already guarded by the currentPromptId id-match check on their response path. Only the notifications are affected — transcript entries are separately deduped by the frequency-map / processedLineCount machinery. Adds a re-delivery regression test plus realistic per-turn, multi-turn, and opened-mid-turn sequences (verified failing without the guard). Generated-By: PostHog Code Task-Id: da2985fa-c8fc-4e02-b1a0-a47d16e7249b
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
|
Reviews (1): Last reviewed commit: "fix(sessions): fire cloud completion not..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Fold the four per-turn completion-notification cases (live turn, re-delivered turn_complete, multiple turns, prompt-only-in-snapshot) into a single `it.each` keyed by an update sequence and an expected notify count, per the team's parameterised-test convention. Keeps the payload / markActivity assertion as a separate case since it differs in intent, and leaves the snapshot-replay and permission tests untouched. Generated-By: PostHog Code Task-Id: da2985fa-c8fc-4e02-b1a0-a47d16e7249b
adboio
added
the
Stamphog
Graphite App
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean bug fix: arm-on-prompt / consume-on-complete via a Set prevents duplicate completion notifications from durable-stream re-delivery. Teardown is covered in both per-session removal and full clear paths. The resolved bot comment about parameterized tests was addressed in this same PR.
|
Reviews (2): Last reviewed commit: "test(sessions): parameterise cloud compl..." | Re-trigger Greptile |
Replace the per-turn completion-notification guard with source-level deduplication, addressing the root cause instead of the symptom. The durable cloud stream re-sends the tail by id on reconnect/replay. A re-delivered entry was counted as new (advancing totalEntryCount past the renderer's processedLineCount guard) and emitted again — the actual cause of both duplicate transcript entries and the back-to-back completion "meep"/toast. CloudTaskService now tracks the ids it has ingested on the current leg (`seenEventIds`) and drops a log entry whose id it has already seen, in `handleSseEvent`, before it is counted or emitted. Ids are leg-scoped: the set is cleared on a proxy<->django leg switch (their id spaces are unrelated), where the snapshot's content-dedup already covers the gap. Entries without an id (legacy servers) fall through unchanged. This lets the notification path stay simple: the previous `cloudTurnAwaitingCompletionNotify` guard in SessionService is removed and the turn_complete handler reverts to notifying on each live completion — it no longer needs to defend against re-delivery because the stream layer no longer produces it. Adds a CloudTaskService test that a re-sent id is delivered exactly once, and moves the notification re-delivery case out of the renderer suite (dedup now lives upstream). Generated-By: PostHog Code Task-Id: da2985fa-c8fc-4e02-b1a0-a47d16e7249b
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Clean, targeted deduplication fix in core stream handling — seenEventIds Set prevents re-delivered SSE entries from being counted twice. Leg-switch clear is correct, legacy fallthrough is correct, and tests cover the new behavior and align with team's parameterized-test convention.
resetWatcherForRebootstrap nulls lastEventId, so the leg-switch clear in connectSse can never fire after a reset — retained ids from the old leg survived into the rebuilt connection, whose re-resolved leg has an unrelated id space. A false match there silently drops a legitimate new entry before it is counted or emitted (worst case a turn_complete: no notification, prompt stuck pending) — and because the drop happens before counting, totalEntryCount stays self-consistent, so the gap reconciler cannot detect or repair it. Both reset paths are reachable: user-facing retry() and the self-heal re-bootstrap. Clear the set alongside the resume position; the re-fetched snapshot re-delivers history, so no dedup state is lost. Test: retry after ingesting id 1, then verify an id-1 entry on the rebuilt connection is still delivered — fails without the clear. Generated-By: PostHog Code Task-Id: 04a64259-f9c0-4123-b99d-f06dc75dfc08
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Clean, targeted deduplication fix in cloud-task stream handling. The seenEventIds Set is properly scoped to the watcher, cleared on both leg-switch and retry, and legacy (no-id) entries fall through safely. Tests cover dedup within a leg and reset-on-retry. The resolved inline comment about parameterized tests was addressed in this same PR.
adboio
deleted the
posthog-code/fix-duplicate-cloud-completion-notifications
branch
2 participants
What
Cloud tasks fired duplicate completion notifications — two rapid back-to-back "meep" sounds and two stacked in-app toasts every time a turn finished. The same re-delivery could also duplicate transcript entries. This dedups the stream at the source so each entry is delivered exactly once.
Root cause
The durable cloud SSE stream re-sends the tail by id on reconnect/replay. A re-delivered log entry (e.g. a
_posthog/turn_complete) was counted as new — advancingtotalEntryCountpast the renderer'sprocessedLineCountguard — and emitted again, sonotifyPromptCompletefired a second time (and the entry could reappear in the transcript). The count/offset guard can't catch this because the replay legitimately advances the count.The notification-bus refactor (#2934) didn't cause the double; it unmasked it — the old focused-elsewhere tier was a native OS notification (which the OS coalesces), the new one stacks toasts and replays a JS sound per call.
How
Deduplicate re-delivered entries by their SSE
event.idat ingestion, inCloudTaskService.handleSseEvent, before anything is counted or emitted:seenEventIds). A log entry whose id was already seen is dropped.retry()/ stream self-heal), which re-resolves the leg. In both cases the old id space is unrelated to the new one, so a retained id would false-match — and silently drop — a legitimate new entry; the re-fetched snapshot's existing content-dedup covers the gap instead.Because the stream layer no longer produces re-deliveries, the notification path stays simple — the completion notification just fires on each live turn completion, with no special de-dup bookkeeping.
Testing
CloudTaskServicetest: a stream that re-sendsid: 1delivers the entry exactly once (["first","second"], not["first","first","second"]) — verified failing without the dedup.CloudTaskServicetest: afterretry()rebuilds the watcher, an entry on the new connection that reuses a previously-seen id is still delivered — verified failing without the reset-path clear.@posthog/coresuite green (1921 tests), core typecheck clean, biome clean (zeronoRestrictedImports).🤖 Generated with Claude Code