-
-
Notifications
You must be signed in to change notification settings - Fork 178
fix(ai-client): prevent drainPostStreamActions re-entrancy race condition #429
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
base: main
Are you sure you want to change the base?
Changes from all commits
7f3e8b6
09585fa
f915ab2
6d27f70
3d06f8b
67c96ad
29377a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@tanstack/ai-client': patch | ||
| --- | ||
|
|
||
| fix(ai-client): prevent drainPostStreamActions re-entrancy stealing queued actions | ||
|
|
||
| When multiple client tools complete in the same round, nested `drainPostStreamActions()` calls from `streamResponse()`'s `finally` block could steal queued actions, permanently stalling the conversation. Added a re-entrancy guard and a `shouldAutoSend()` check requiring tool-call parts before triggering continuation. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| import { test, expect } from '../fixtures' | ||
| import { | ||
| selectScenario, | ||
| runTest, | ||
| waitForTestComplete, | ||
| getMetadata, | ||
| getEventLog, | ||
| getToolCalls, | ||
| } from './helpers' | ||
|
|
||
| /** | ||
| * Drain Re-Entrancy Guard E2E Tests | ||
| * | ||
| * Regression test for GitHub issue #302 / PR #429. | ||
| * | ||
| * When multiple client tools complete in the same round, each addToolResult() | ||
| * queues a checkForContinuation action. Without a re-entrancy guard on | ||
| * drainPostStreamActions(), the first action's streamResponse() → finally → | ||
| * drainPostStreamActions() (nested) steals the remaining actions from the | ||
| * queue, permanently stalling the conversation. The user sees tool results | ||
| * but the model never produces its follow-up text response. | ||
| * | ||
| * The fix adds a re-entrancy guard to drainPostStreamActions() and a | ||
| * shouldAutoSend() check requiring tool-call parts before triggering | ||
| * continuation. | ||
| * | ||
| * This test uses the parallel-client-tools scenario (2 client tools in the | ||
| * same turn) and verifies not just that both tools execute, but critically | ||
| * that the **continuation fires and the follow-up text response arrives**. | ||
| * Without the fix, the test would time out waiting for the follow-up text. | ||
| */ | ||
|
|
||
| test.describe('Drain Re-Entrancy Guard (Regression #302)', () => { | ||
| test('parallel client tools complete and continuation fires with follow-up text', async ({ | ||
| page, | ||
| testId, | ||
| aimockPort, | ||
| }) => { | ||
| await selectScenario(page, 'parallel-client-tools', testId, aimockPort) | ||
| await runTest(page) | ||
|
|
||
| // Wait for the test to fully complete — this includes the continuation | ||
| // round producing the follow-up text. Without the fix, this would | ||
| // time out because the continuation never fires. | ||
| await waitForTestComplete(page, 20000, 2) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an explicit wait for continuation text to reduce E2E flakiness. Line 45 waits on generic completion state; Line 91 then immediately asserts the follow-up text. If UI/message rendering lags slightly, this can fail intermittently even when continuation did fire. Suggested patch@@
await waitForTestComplete(page, 20000, 2)
+
+ // Ensure continuation text is actually rendered before asserting content.
+ await page.waitForFunction(() => {
+ const el = document.getElementById('messages-json-content')
+ if (!el?.textContent) return false
+ try {
+ const messages = JSON.parse(el.textContent)
+ const text = messages
+ .filter((m: any) => m.role === 'assistant')
+ .flatMap((m: any) =>
+ (m.parts || [])
+ .filter((p: any) => p.type === 'text')
+ .map((p: any) => p.content),
+ )
+ .join(' ')
+ return text.includes('All displayed')
+ } catch {
+ return false
+ }
+ }, { timeout: 5000 })Also applies to: 67-91 🤖 Prompt for AI Agents |
||
|
|
||
| // Verify both client tools executed | ||
| const metadata = await getMetadata(page) | ||
| expect(metadata.testComplete).toBe('true') | ||
| expect(metadata.isLoading).toBe('false') | ||
|
|
||
| const events = await getEventLog(page) | ||
| const toolNames = [...new Set(events.map((e) => e.toolName))] | ||
| expect(toolNames).toContain('show_notification') | ||
| expect(toolNames).toContain('display_chart') | ||
|
|
||
| // Verify both tools reached execution-complete state | ||
| const completionEvents = events.filter( | ||
| (e) => e.type === 'execution-complete', | ||
| ) | ||
| expect(completionEvents.length).toBe(2) | ||
|
|
||
| // CRITICAL ASSERTION: Verify the follow-up text from round 2 was received. | ||
| // Without the re-entrancy fix, the conversation stalls after both tools | ||
| // complete — the continuation request is never sent, so this text never | ||
| // arrives. | ||
| const messages = await page.evaluate(() => { | ||
| const el = document.getElementById('messages-json-content') | ||
| if (!el) return [] | ||
| try { | ||
| return JSON.parse(el.textContent || '[]') | ||
| } catch { | ||
| return [] | ||
| } | ||
| }) | ||
|
|
||
| const assistantMessages = messages.filter( | ||
| (m: any) => m.role === 'assistant', | ||
| ) | ||
|
|
||
| // There should be at least 2 assistant messages: | ||
| // 1. The tool-call round (with both tool calls + results) | ||
| // 2. The continuation round (with the follow-up text) | ||
| expect(assistantMessages.length).toBeGreaterThanOrEqual(2) | ||
|
|
||
| // The follow-up text from the continuation round should be present | ||
| const allTextParts = assistantMessages.flatMap((m: any) => | ||
| m.parts.filter((p: any) => p.type === 'text').map((p: any) => p.content), | ||
| ) | ||
| const allText = allTextParts.join(' ') | ||
| expect(allText).toContain('All displayed') | ||
| }) | ||
|
|
||
| test.afterEach(async ({ page }, testInfo) => { | ||
| if (testInfo.status !== testInfo.expectedStatus) { | ||
| await page.screenshot({ | ||
| path: `test-results/drain-reentrance-failure-${testInfo.title.replace(/\s+/g, '-')}.png`, | ||
| fullPage: true, | ||
| }) | ||
|
|
||
| const toolCalls = await getToolCalls(page) | ||
| const metadata = await getMetadata(page) | ||
| const events = await getEventLog(page) | ||
|
|
||
| console.log('Test failed. Debug info:') | ||
| console.log('Metadata:', metadata) | ||
| console.log('Tool calls:', toolCalls) | ||
| console.log('Events:', events) | ||
| } | ||
| }) | ||
| }) | ||
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.
Fix import member order to satisfy lint.
The import specifiers are not alphabetically sorted, which matches the reported
sort-importserror.🔧 Suggested fix
import type { - ConnectionAdapter, ConnectConnectionAdapter, + ConnectionAdapter, } from '../src/connection-adapters'📝 Committable suggestion
🧰 Tools
🪛 ESLint
[error] 13-13: Member 'ConnectConnectionAdapter' of the import declaration should be sorted alphabetically.
(sort-imports)
🤖 Prompt for AI Agents