fix(ai-client): capture abort signal before await to prevent race condition#377
fix(ai-client): capture abort signal before await to prevent race condition#377FranDias wants to merge 5 commits intoTanStack:mainfrom
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCapture the newly created AbortController's signal into a local variable and use that captured signal when invoking the connection adapter and processing streams to avoid races if Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unit tests for the signal capture fixAdded two tests to 1.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/typescript/ai-client/tests/chat-client-abort.test.ts`:
- Around line 372-398: The test currently fires a nested client.append(...)
inside the onResponse callback without awaiting it and then uses a fixed 50ms
setTimeout to wait, which is non-deterministic and can hide rejections; change
the onResponse callback to capture the returned promise (e.g., assign the result
of client.append(...) to a variable like secondAppendPromise) and then await
that promise after the initial await client.append({...}) instead of using
setTimeout, ensuring any rejection from the second append is surfaced to the
test; reference the onResponse callback and client.append calls to locate and
update the logic accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: baa8584d-a01d-49a3-89fb-9c4840ead742
📒 Files selected for processing (1)
packages/typescript/ai-client/tests/chat-client-abort.test.ts
Capture the nested append() promise and await it directly instead of relying on a fixed 50ms setTimeout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Just a note that this is actively causing a regression for us after upgrading to vite 8 + We're carrying a local patch ( |
Summary
Fixes a race condition in
ChatClient.streamResponse()wherethis.abortController.signalcould reference a stale or null controller by the time it is passed tothis.connection.connect().this.abortController = new AbortController()(line 432) and thethis.connection.connect(..., this.abortController.signal)call (line 459), there is anawait this.callbacksRef.current.onResponse(). During that await, a concurrentstop()call nulls outthis.abortController, or a rapid secondsendMessage()could replace it with a new controller.const signal = this.abortController.signalimmediately after creation, then pass the localsignalvariable toconnect(), ensuring it always receives the signal from the correct AbortController regardless of concurrent mutations.How to reproduce
ChatClientand callsendMessage("hello")onResponsecallback), callstop()or fire anothersendMessage()this.abortControlleris now null or points to a different controllerconnect()receives a wrong/null signal, leading to either a runtime error or the inability to abort the correct streamChanges
packages/typescript/ai-client/src/chat-client.ts: Capturesignallocally right afterAbortControllercreation; pass it toconnect()instead of re-readingthis.abortController.signalTest plan
@tanstack/ai-client)chat-client-abort.test.tsall pass (6 tests)Summary by CodeRabbit
Bug Fixes
Tests