fix(ai-client): capture abort signal before await to prevent race condition#377
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:
📝 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 |
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
|
Just a note that this is actively causing a regression for us after upgrading to vite 8 + We're carrying a local patch ( |
|
View your CI Pipeline Execution ↗ for commit 2aac545
☁️ Nx Cloud last updated this comment at |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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>
cd935b0 to
1d420c2
Compare
Rewrites the second race-condition test to use reload() instead of a queued append() — append() early-returns when isLoading and queues via queuePostStreamAction, so it never reassigns this.abortController mid-flight. reload() calls cancelInFlightStream() synchronously then starts a new streamResponse(), which is the actual code path that triggers the race the fix protects against. Adds asChunk() casts so the new yields satisfy the strict AGUIEvent typing introduced by AG-UI core interop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tombeckenham
left a comment
There was a problem hiding this comment.
Thank you for finding this @FranDias and my sincere apologies for the delay in approving it. We had a bit of a backlog, and we're now getting through it.
@tanstack/ai
@tanstack/ai-anthropic
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-skills
@tanstack/ai-devtools-core
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-openrouter
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
…wait Capturing the AbortController signal locally avoided the original null deref but exposed a latent deadlock: when stop() runs during the onResponse await, cancelInFlightStream() calls resolveProcessing() before waitForProcessing() has set processingResolve, so the call is a no-op. Post-fix, streamResponse no longer crashed on the now-null controller, reached waitForProcessing() (creating a fresh resolver nothing would resolve), and hung on `await processingComplete` — breaking the ai-react useChat unmount test. Add a `signal.aborted` check after the onResponse await to short-circuit cancelled or superseded streams cleanly, restoring main's pre-fix flow control without relying on a thrown TypeError. Update the two race tests to reflect the correct semantics: cancelled streams must not invoke the connection layer or surface errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
As someone who just got some popularity on an OS project I get it, no worries here :) Thanks for taking a look, I'm excited to get rid of that patch file. Thank you for doing what you're doing. |
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