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 Straightforward race condition fix that buffers events during snapshot loading to prevent them from being dropped. Limited scope with a clear test demonstrating the fix. 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
ID: 3174
URL: #3174
Title: Fix thread detail subscription race
Summary
Fixes a race in the thread-detail WebSocket subscription path where live thread events could be emitted while the server was still loading the initial thread snapshot. The subscription now starts observing live thread-detail events before snapshot loading, buffers them, then emits only events newer than the snapshot sequence after the snapshot has been sent.
What Changed
streamThreadDetailEventsAfterSnapshothelper that filters live orchestration domain events to the requested thread, thread-detail event types, and events newer than a given snapshot sequence.subscribeThreadto start a scoped live listener beforegetThreadDetailByIdandgetSnapshotSequenceresolve, queueing matching live events that arrive during snapshot loading.thread.message-sentevent.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 head1e7fd5d.pnpm exec vp checkpassed on head1e7fd5d, with existing lint warnings outside the PR diff.pnpm exec vp run typecheckpassed on head1e7fd5d.1e7fd5d.Proof
Note
Medium Risk
Changes real-time WebSocket subscription ordering for thread detail; behavior is intentional but affects all thread subscribers and relies on event sequence vs snapshot sequence correctness.
Overview
Fixes a race where `subscribeThread` could drop live thread-detail events that arrived while the server was still loading the initial snapshot—clients could briefly show the first user message and then lose it.
`subscribeThread` now forks a scoped listener on the domain event stream before `getThreadDetailById` / `getSnapshotSequence` finish, pushing matching events into an unbounded queue. A new `streamThreadDetailEventsAfterSnapshot` helper filters by thread, thread-detail event types, and `sequence > snapshotSequence`. The stream still sends the snapshot first, then queued/live events whose sequence is greater than the loaded snapshot sequence (no duplicates from events already in the snapshot).
Adds a server regression test that delays the snapshot, emits a `thread.message-sent` during the delay, and asserts the client receives snapshot then the buffered event.
Reviewed by Cursor Bugbot for commit 1e7fd5d. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix race condition in `subscribeThread` that drops events emitted during snapshot loading
Macroscope summarized 1e7fd5d.