Fix thread detail subscription race#3174
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Approved This is a targeted bug fix for a race condition in thread detail subscriptions. The fix acquires the live event subscription and buffers events before loading snapshots, then deduplicates by sequence number. The change is well-tested with two scenarios covering the race conditions, and follows existing patterns in the codebase. You can customize Macroscope's approvability policy. Learn more. |
|
Comment audit complete: I refreshed the tracked PR comments and found no actionable review feedback. Current comments are informational only: CodeRabbit skipped automated review because reviews are disabled, and Macroscope marked the PR approved. There are no inline review threads to resolve. |
|
Comment audit refreshed: I rechecked the PR issue comments, review comments, and review threads. There is still no actionable review feedback to address. Current comments remain informational only: CodeRabbit skipped automated review because repository reviews are disabled, Macroscope marked the PR approved, and there are no inline review threads to resolve. |
Dismissing prior approval to re-evaluate 7a1c76d
There was a problem hiding this comment.
Effect service review: one convention violation found in the new desktop backend advertisement error. See inline comment.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Effect service convention review found 3 issues. See inline comments.
Posted via Macroscope — Effect Service Conventions
9e8cc6b to
58463a9
Compare
- Limit subscription buffering to a scoped bounded queue - Backpressure slow thread-detail consumers instead of growing memory
Dismissing prior approval to re-evaluate 2f3b463
- Buffer live shell events before the initial snapshot resolves - Drop buffered events already covered by the snapshot sequence - Add a regression test for the shell subscription race
- Replace replaying test queues with callback streams - Publish events only after live stream consumption starts
- Acquire domain event subscriptions before loading snapshots - Cover shell and thread races with PubSub-backed tests
- Derive stream events from scoped subscriptions - Share orchestration event mocks in server tests - Cover shell snapshot deduplication
- Add a sliding buffer before websocket stream filtering - Keep shell and thread subscribers from growing unbounded backlogs
Dismissing prior approval to re-evaluate ecffa75
- Replace hot domain-event streams with scoped subscriptions - Update tests and mocks to publish through PubSub
- Add shared listener helper with failure logging - Reuse helper across reactors and relay - Simplify domain event subscription test setup
Dismissing prior approval to re-evaluate 8568992
…ubscription-race # Conflicts: # apps/server/src/ws.ts
- Drain the live subscription into a scoped thread-filtered queue - Resume the post-snapshot tail from the buffered event stream
- Track the highest persisted sequence before draining live events - Share scoped live buffering and document its non-dropping lifetime - Cover snapshot filtering and replay overlap in server tests
Dismissing prior approval to re-evaluate 414eafb
Summary
Fixes a race in the thread-detail WebSocket subscription path where live events could be emitted while the server was still loading the initial snapshot and snapshot sequence.
subscribeThreadnow attaches its live domain-event subscription before snapshot loading, still emits the loaded snapshot first, then emits only matching thread-detail events newer than the loaded snapshot sequence.What Changed
OrchestrationEngine.subscribeDomainEventsso callers that need gap-free live observation can acquire a scopedPubSubsubscription before starting dependent work, while preservingstreamDomainEventsas the existing live stream surface.streamDomainEventsimplementation for older or test engines.subscribeThreadto attach the live stream before loading the thread snapshot and snapshot sequence.Why
This addresses an intermittent first-prompt rendering bug: when sending the first prompt, the first user message could briefly appear in the conversation and then disappear, leaving only the rest of the conversation. In that failure mode, the first visible conversation item could become the
Working for...label instead of the user's submitted message.Validation
pnpm exec vp test run apps/server/src/server.test.ts -t "buffers thread detail events emitted while loading the initial thread snapshot" --reporter=dotpassed on head8568992f.pnpm exec vp checkpassed on head8568992f, with existing lint warnings outside the PR diff.pnpm exec vp run typecheckpassed on head8568992f, with existing desktop Effect suggestions outside the PR diff.8568992f, GitHub reports the PR is open, mergeable, and approved; CodeRabbit, Vercel, PR Vouch, PR Size, Cursor Bugbot, Macroscope Approvability, Macroscope Correctness, and Macroscope Effect Service Conventions are successful or skipped where appropriate.Proof
Note
Medium Risk
Changes real-time delivery semantics on a core conversation WebSocket path; mistakes could duplicate or omit messages, though new tests target the race.
Overview
Fixes a race where
subscribeThreadcould drop live thread-detail events that arrived while the server was still loading the initial snapshot or finishing persisted catch-up replay.OrchestrationEngine now exposes
subscribeDomainEvents, a scopedPubSubsubscription so callers can attach a dedicated live listener before slower work runs.streamDomainEventsis unchanged for existing consumers.The WebSocket path starts that subscription first, filters to thread-detail events, and drains them into a scoped unbounded queue. After the snapshot (or replay through
afterSequence), it emits only events with sequence greater than the loaded/replayed cursor so overlaps are not duplicated.Regression tests cover delayed snapshot load and replay overlapping with live publishes.
Reviewed by Cursor Bugbot for commit b516f9c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix race condition in
subscribeThreadWebSocket handler by buffering events during snapshot loadsubscribeThreadhandler in ws.ts previously used a direct stream fromorchestrationEngine.streamDomainEvents, which could drop events that arrived while the initial snapshot or catch-up replay was loading.subscribeDomainEventsmethod is added toOrchestrationEngineShapethat returns a scoped, independentPubSubsubscription per caller.afterSequencereplay flows.subscribeThreadstream.Macroscope summarized b516f9c.