Skip to content

fix(sse): bound workspace SSE connection lifetime - #7058

Merged
waleedlatif1 merged 5 commits into
stagingfrom
fix/sse-connection-teardown
Aug 25, 2026
Merged

fix(sse): bound workspace SSE connection lifetime#7058
waleedlatif1 merged 5 commits into
stagingfrom
fix/sse-connection-teardown

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • createWorkspaceSSE tore down a connection only from the request.signal abort listener and the stream's cancel(). Both fire only when the runtime reports the client disconnect, and nothing else bounded the connection — so a missed report left the pub/sub handler in a process-lifetime Set, the heartbeat interval running, and the stream's undrained queue held until the process restarted. Retention scaled with uptime instead of with concurrent clients.
  • Unread-consumer check — the heartbeat reads controller.desiredSize (the default strategy reports 1 - queued) and closes once nothing has been pulled for several minutes. This is what actually reclaims a vanished consumer, within minutes. A healthy client keeps its stream drained and is never unread, so this never touches a working connection.
  • Lifetime ceiling (4h, jittered) — the backstop for whatever the abort, cancel, and unread paths all miss, so retention is bounded by the ceiling rather than by process uptime. Checked on the existing heartbeat tick, so it adds no second timer. Matches lib/realtime/event-stream-route.ts, which uses the same deadline pattern for the same purpose. Deliberately far longer than the unread window: a short ceiling would only force reconnects on the connections that are working, and every reconnect is a window in which a transient event can be missed.
  • The abort listener is now removed on every path. { once: true } only self-removes if abort fires; the ceiling and unread paths close while the signal is still live, so the listener would have stayed installed retaining close → the controller, the teardown list, and the workspace id — most of the scope this change exists to release. It registers with an owned AbortController as its removal token.
  • A failed heartbeat enqueue now runs full teardown instead of clearing only its own interval and leaving the subscriptions registered.
  • cleanup takes a reason, so the close log says which path ran (aborted, cancelled, expired, unread, errored). Opens minus closes, grouped by reason, is what makes this class of leak visible rather than inferred.
  • Reconnect resync (useMothershipChatEvents)task_status events are transient and never replayed, and the hook previously reconciled nothing on reconnect. It now re-syncs the workspace chat lists on reconnect, on the first open of a re-subscription, and on a first open that only succeeded after an error, matching the pattern useMcpToolsEvents already uses. Only the lists: they carry the create/rename/delete state a gap can drop and are always safe to refetch.

Chat detail reconciliation is deliberately out of scope and unchanged from staging. Refetching a mounted detail can replace an in-flight optimistic transcript with a server copy that lacks the streaming message, and cached state cannot reliably say whether a turn is still running — the optimistic markers outlive it. Doing that properly needs the streaming state that can answer it, and belongs in its own change rather than being half-solved here.

createWorkspaceSSE is the only push-driven, indefinite-lifetime SSE surface in the app. The two other long-lived SSE routes are poll loops that already carry their own duration ceilings and never depended on abort. A shared bounded-SSE primitive would be the honest end state for all three, but that is a three-site refactor and doesn't belong in a leak fix.

Type of Change

  • Bug fix

Testing

sse-endpoint.test.ts covers the ceiling, the unread path, a drained connection surviving past the unread threshold, abort, consumer cancel, and teardown idempotency when two paths overlap. use-mothership-chat-events.test.ts covers the resync invalidating the lists and leaving details untouched.

Verified the tests are load-bearing rather than assuming it: disabling each backstop reds exactly its own test and leaves the rest green. The abort-listener removal isn't unit-observable (AbortSignal exposes no listener count), so it's covered by review — I dropped an earlier test that looked like it covered it but only re-asserted an idempotency guard another test already owns.

bun run lint, bun run check:audits (33 audits), and tsc clean. 163 test files / 1816 tests green across hooks, lib/events, and lib/copilot/chat.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

Teardown ran only from the request abort listener and the stream cancel callback, both of which fire only when the runtime reports a client disconnect. Nothing else bounded the connection, so a missed report left the pub/sub handler, the heartbeat timer, and the stream's undrained queue held for the life of the process.

Add a jittered lifetime ceiling checked on the existing heartbeat tick, tighten reclaim for a vanished consumer via desiredSize, remove the abort listener on every teardown path, and run full teardown when a heartbeat enqueue fails. Log the close reason so opens minus closes is observable.
@vercel

vercel Bot commented Aug 25, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 25, 2026 3:35am

Request Review

@cursor

cursor Bot commented Aug 25, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes long-lived SSE teardown and reconnect cache behavior for mothership chat; bounded reconnects are intentional but can briefly miss events until list resync runs.

Overview
Workspace SSE (createWorkspaceSSE) no longer relies only on client abort/cancel for teardown. Heartbeats now close connections when the stream stays undrained for several minutes, when a 4h (+ jitter) lifetime ceiling is reached, or when enqueue fails — with idempotent cleanup, close-reason logging, and abort listener removal so expiry/unread paths do not leak scope.

Because forced reconnects can miss transient task_status events, useMothershipChatEvents adds resyncMothershipChatCaches, which invalidates workspace chat lists only on SSE onopen after reconnect, workspace re-subscribe, or a first open that followed an error — not on a clean first subscription. Chat detail queries are intentionally not invalidated to avoid refetching over an in-flight stream.

New sse-endpoint.test.ts exercises ceiling, unread, abort/cancel, and overlapping teardown; use-mothership-chat-events.test.ts covers list resync behavior.

Reviewed by Cursor Bugbot for commit cbe8898. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR bounds workspace SSE connection retention and reconciles workspace chat lists after reconnect gaps.

  • Adds unread-consumer detection, a jittered four-hour lifetime ceiling, and comprehensive teardown handling.
  • Re-syncs workspace chat lists when a connection reopens after a possible event gap.
  • Adds tests for SSE lifetime behavior, cleanup paths, and reconnect cache invalidation.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/lib/events/sse-endpoint.ts Adds bounded connection lifetime, unread-consumer detection, and centralized idempotent resource cleanup.
apps/sim/lib/events/sse-endpoint.test.ts Covers expiry, unread detection, healthy draining, abort, cancellation, and overlapping teardown paths.
apps/sim/hooks/use-mothership-chat-events.ts Adds reconnect-aware workspace-list invalidation while deliberately leaving chat-detail reconciliation unchanged.
apps/sim/hooks/use-mothership-chat-events.test.ts Verifies that reconnect resynchronization invalidates workspace lists without touching chat details.

Sequence Diagram

sequenceDiagram
  participant Client as EventSource client
  participant SSE as Workspace SSE
  participant PubSub as Pub/Sub
  participant Cache as Query cache
  Client->>SSE: Open connection
  PubSub-->>SSE: Transient workspace event
  SSE-->>Client: SSE event
  alt Client stops draining
    SSE-->>SSE: Queue exceeds unread threshold
    SSE-->>Client: Close stream
  else Connection reaches ceiling
    SSE-->>SSE: Jittered deadline expires
    SSE-->>Client: Close stream
  end
  SSE->>PubSub: Unsubscribe and clear resources
  Client->>SSE: Automatically reconnect
  SSE-->>Client: Connection opens
  Client->>Cache: Invalidate workspace chat lists
Loading

Reviews (5): Last reviewed commit: "fix(sse): raise the ceiling and narrow r..." | Re-trigger Greptile

Comment thread apps/sim/lib/events/sse-endpoint.ts
Comment thread apps/sim/lib/events/sse-endpoint.ts
task_status events are transient and never replayed, so any window with no open connection can drop a create, rename, delete, or completion. The chat hook reconnected silently and reconciled nothing, leaving list and detail caches stale until an unrelated action refreshed them.

Resync on reconnect, on the first open of a re-subscription, and on a first open that only succeeded after an error, matching the pattern useMcpToolsEvents already uses for the same gap.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/hooks/use-mothership-chat-events.ts
The resync invalidated every chat detail, including one whose stream this client is rendering optimistically. Refetching there replaces the local transcript with a server copy that does not yet hold the in-flight message, which is exactly what status events avoid via shouldSkipDetailInvalidationForStreamEvent.

Filter the detail invalidation with the same isLocalOptimisticActiveStream check. Those chats reconcile when their own stream finishes.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 70541f4. Configure here.

Comment thread apps/sim/hooks/use-mothership-chat-events.ts Outdated
Optimistic markers alone were the skip condition, but a finished turn can leave activeStreamId and its live-assistant message cached when finalization skips detail invalidation for a queued follow-up. That chat would then be excluded from every future resync — permanently, since only a refetch clears the markers, and the resync was the refetch.

Gate the skip on a non-terminal streamSnapshot status so it covers turns that are genuinely still streaming. Exports isTerminalStreamStatus, which was already the private check for this in effective-transcript.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/hooks/use-mothership-chat-events.ts Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit f76765f. Configure here.

Deciding from cache whether a chat is still streaming is not reliable — the optimistic markers outlive the turn, and each refinement of that predicate exposed another state where it answers wrongly. Drop it: the resync now invalidates only the workspace lists, which is always safe, and chat detail reconciliation stays as it is today rather than being half-solved here.

Raise the ceiling to 4h, matching lib/realtime/event-stream-route.ts. A healthy client is drained and so is never unread; the unread check is what reclaims a vanished consumer, and it does so within minutes. A short ceiling would therefore only force reconnects on the connections that are working, and every reconnect is a window where a transient event can be missed. Retention stays bounded by the ceiling instead of by process uptime.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit cbe8898. Configure here.

@waleedlatif1
waleedlatif1 merged commit ef42424 into staging Aug 25, 2026
31 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/sse-connection-teardown branch August 25, 2026 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant