Skip to content

streaming: collapse EventStreamClient fan-out#32459

Merged
TirmanSidhu merged 1 commit into
TirmanSidhu/streaming-message-architecturefrom
run-plan/streaming-message-architecture/pr-5
May 28, 2026
Merged

streaming: collapse EventStreamClient fan-out#32459
TirmanSidhu merged 1 commit into
TirmanSidhu/streaming-message-architecturefrom
run-plan/streaming-message-architecture/pr-5

Conversation

@TirmanSidhu
Copy link
Copy Markdown
Contributor

Summary

  • Replaces broadcaster subscribe(filter:) with typed per-domain dispatchers (chat / conversation list / surface / store / etc.).
  • Removes startMessageLoop, messageLoopTask, messageLoopGeneration, ensureMessageLoopStarted from ChatViewModel.
  • Each consumer subscribes to events by type, not by filter. SSE loop runs for the connection lifetime; routing is downstream.
  • Eliminates the double-subscriber window during loop restart (one of the three root failure modes of the duplication bug).

Part of plan: streaming-message-architecture.md (PR 5 of 6)

@TirmanSidhu TirmanSidhu merged commit 5206d3a into TirmanSidhu/streaming-message-architecture May 28, 2026
@TirmanSidhu TirmanSidhu deleted the run-plan/streaming-message-architecture/pr-5 branch May 28, 2026 19:56
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5b81578dc7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +169 to +171
let (stream, continuation) = AsyncStream<ServerMessage>.makeStream(
bufferingPolicy: .bufferingNewest(eventStreamSubscriberBufferLimit)
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Do not drop incremental chat stream events

When a chat subscriber falls more than 256 events behind (for example during a long response while the main actor is busy), .bufferingNewest silently discards the oldest unread ServerMessages. Several routed chat events such as assistantTextDelta, toolInputDelta, and toolOutputChunk are incremental rather than replace-by-latest snapshots, so losing an older chunk permanently truncates the transcript/tool input until a separate history reload happens. The previous unbounded stream did not drop these deltas, so chat streaming needs a lossless path or category-specific buffering for incremental events.

Useful? React with 👍 / 👎.

.messageComplete, .messageQueued, .messageDequeued, .messageRequestComplete,
.messageQueuedDeleted, .messageSteered, .generationCancelled, .generationHandoff,
.userMessageEcho, .userMessagePersisted, .queuedMessageAcked,
.conversationInfo, .conversationError, .confirmationRequest, .confirmationStateChanged,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Route AppDelegate events to appDelegate subscribers

After the fan-out split, AppDelegate+ConnectionSetup.startEventSubscription() only reads subscribeAppDelegateEvents(), but this mapping sends confirmationRequest and conversationError only to .chat. That means the AppDelegate cases that auto-approve CU confirmations / show confirmation notifications and handle managed-assistant auth errors no longer run for those events. Include these AppDelegate-handled event types in the .appDelegate category as well as chat.

Useful? React with 👍 / 👎.

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