-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: resolve message queue race condition during LLM processing (#8536) #8538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Move queue check before status mutation logic to prevent race condition - Add continuous queue monitoring during pWaitFor to catch messages that arrive during processing - Process queued messages immediately when detected during wait period - Add comprehensive tests for queue race condition scenarios Fixes #8536
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code like a mirror debugging itself: reflections guaranteed, bias not included.
| // The condition should now detect the message and process it | ||
| task.setMessageResponse("delayed message") | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 — Test doesn't exercise the intended queue-monitoring path. This line directly sets askResponse via setMessageResponse(), so pWaitFor() returns true without the ask() loop detecting and processing the newly queued message. Remove this direct call and let ask() consume the queued message, then assert queue empties and response/text come from the queue.
| const originalPWaitFor = (await import("p-wait-for")).default | ||
| let conditionCheckCount = 0 | ||
| vi.mocked(originalPWaitFor).mockImplementation(async (condition, options) => { | ||
| // Simulate checking the condition multiple times |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3 — Test isolation: mockImplementation overrides the globally mocked p-wait-for for subsequent tests. Prefer mockImplementationOnce for this specific case or restore the mock in afterEach to avoid bleed-over.
| // Mock pWaitFor to simulate adding a message during the wait | ||
| const originalPWaitFor = (await import("p-wait-for")).default | ||
| let conditionCheckCount = 0 | ||
| vi.mocked(originalPWaitFor).mockImplementation(async (condition, options) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the test 'should check for new messages during wait period', the p-wait-for implementation is overridden but not restored. Consider restoring the original implementation after the test to avoid side‐effects on subsequent tests.
Review SummaryReviewed the message queue race condition fix. Found 2 issues that need to be addressed: Issues to Fix
Implementation ReviewThe core fix looks solid:
Once the test issues are addressed, this should be ready to merge. |
Description
This PR fixes a critical race condition in the message queue system where messages sent during LLM processing could silently disappear. The issue occurred when users sent messages at a specific timing - after a queued message was dequeued but before LLM processing completed.
Problem
As reported in #8536, messages were being lost when:
Solution
The fix implements a two-phase approach:
pWaitForKey Changes
pWaitForloopTesting
Review Confidence
Internal review showed 95% confidence with no security concerns and good code quality adherence.
Fixes #8536
Important
Fixes race condition in
Task.tsby updatingask()to handle message queue processing before and during wait states, with new tests added inTask.spec.ts.Task.tswhere messages sent during LLM processing could disappear.ask()to process queued messages before waiting and monitor for new messages during wait.Task.spec.tsto cover race condition scenarios.This description was created by
for a1f583a. You can customize this summary. It will automatically update as commits are pushed.