[SDK] Improve timeout stop conditions#1584
Conversation
dosubot
bot
added
the
size:XL
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Caution Review failedPull request was closed or merged during review WalkthroughThis pull request adds stopWhen and timeout options to PromptOptions and implements resolveStopWhen in TestAgent to combine user-provided stop conditions with a default step-count guard. It re-exports hasToolCall, stepCountIs, and the StopCondition type, updates docs and examples across the SDK, and adjusts examples to use TestAgent-focused tooling. New unit and integration tests cover stopWhen and timeout behaviors, and generateText calls are passed timeout values when provided. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdk/tests/TestAgent.timeout.integration.test.ts (1)
116-116: Avoid asserting the exact number of model generations here.
doGenerateCallsis an AI SDK implementation detail, while this test is really about timeout/error propagation. Pinning it to2makes the spec brittle against harmless SDK scheduling changes.Lean alternative
- expect(currentModel.doGenerateCalls).toHaveLength(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tests/TestAgent.timeout.integration.test.ts` at line 116, The test currently asserts an exact SDK implementation detail with expect(currentModel.doGenerateCalls).toHaveLength(2); — replace this brittle assertion with a non-exact check (or remove it entirely). For example, change it to assert a minimal expectation like expect(currentModel.doGenerateCalls.length).toBeGreaterThan(0) or simply delete the line so the test only verifies timeout/error propagation; reference currentModel.doGenerateCalls to locate and update the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/tests/TestAgent.timeout.integration.test.ts`:
- Around line 83-97: The current wait loop checks abortSignal.aborted before
registering the "abort" listener, leaving a race where the signal can fire
between the check and addEventListener; to fix, move the
abortSignal.addEventListener call inside the new Promise constructor used in the
wait path (the Promise that currently awaits rejection), set the listener to
reject with toError(abortSignal.reason) and mark abortObserved = true in the
handler, and then immediately after registering the listener check
abortSignal.aborted and if true call reject/toError (or throw) so the handler
runs whether the signal aborted before or after registration; update the code
around abortSignal, abortObserved, and the Promise used in the await to reflect
this ordering.
---
Nitpick comments:
In `@sdk/tests/TestAgent.timeout.integration.test.ts`:
- Line 116: The test currently asserts an exact SDK implementation detail with
expect(currentModel.doGenerateCalls).toHaveLength(2); — replace this brittle
assertion with a non-exact check (or remove it entirely). For example, change it
to assert a minimal expectation like
expect(currentModel.doGenerateCalls.length).toBeGreaterThan(0) or simply delete
the line so the test only verifies timeout/error propagation; reference
currentModel.doGenerateCalls to locate and update the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32a3576d-f4fa-4bc6-879e-da21148c7006
📒 Files selected for processing (13)
docs/sdk/concepts/connecting-servers.mdxdocs/sdk/concepts/testing-with-llms.mdxdocs/sdk/reference/llm-providers.mdxdocs/sdk/reference/prompt-result.mdxdocs/sdk/reference/test-agent.mdxsdk/README.mdsdk/skills/create-mcp-eval/SKILL.mdsdk/src/EvalAgent.tssdk/src/TestAgent.tssdk/src/index.tssdk/tests/TestAgent.stopWhen.integration.test.tssdk/tests/TestAgent.test.tssdk/tests/TestAgent.timeout.integration.test.ts
| if (abortSignal.aborted) { | ||
| abortObserved = true; | ||
| throw toError(abortSignal.reason); | ||
| } | ||
|
|
||
| await new Promise<never>((_, reject) => { | ||
| abortSignal.addEventListener( | ||
| "abort", | ||
| () => { | ||
| abortObserved = true; | ||
| reject(toError(abortSignal.reason)); | ||
| }, | ||
| { once: true } | ||
| ); | ||
| }); |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
fd 'TestAgent.timeout.integration.test.ts' --type fRepository: MCPJam/inspector
Repository: MCPJam/inspector
Exit code: 0
stdout:
sdk/tests/TestAgent.timeout.integration.test.ts
Script executed:
cat -n sdk/tests/TestAgent.timeout.integration.test.ts | sed -n '70,110p'Repository: MCPJam/inspector
Repository: MCPJam/inspector
Exit code: 0
stdout:
70 wait: dynamicTool({
71 description: "Wait until the abort signal fires",
72 inputSchema: jsonSchema({
73 type: "object",
74 properties: {},
75 }),
76 execute: async (_input, { abortSignal }) => {
77 sawAbortSignal = abortSignal != null;
78
79 if (abortSignal == null) {
80 throw new Error("missing abort signal");
81 }
82
83 if (abortSignal.aborted) {
84 abortObserved = true;
85 throw toError(abortSignal.reason);
86 }
87
88 await new Promise<never>((_, reject) => {
89 abortSignal.addEventListener(
90 "abort",
91 () => {
92 abortObserved = true;
93 reject(toError(abortSignal.reason));
94 },
95 { once: true }
96 );
97 });
98
99 throw new Error("unreachable");
100 },
101 }),
102 },
103 model: "openai/gpt-4o",
104 apiKey: "test-key",
105 });
106
107 const startedAt = Date.now();
108 const result = await agent.prompt("Run the long tool", { timeout: 25 });
109 const elapsedMs = Date.now() - startedAt;
110
AbortSignal addEventListener already aborted listener behavior JavaScript
If an AbortSignal is already aborted, then:
-
Listening for
"abort"on the signal does not “catch up.”
signal.addEventListener("abort", handler)will not invokehandlerif the abort already happened; the"abort"event is fired at the moment the signal is aborted, and adding a listener later won’t retroactively dispatch it. The intended pattern is to checksignal.aborted/signal.reason(or callsignal.throwIfAborted()) immediately when you start work. The DOM standard also says APIs should “reject immediately if the signal is already aborted.” [1], [2] -
Using an already-aborted signal in
addEventListener(..., { signal })prevents registration.
When you dotarget.addEventListener(type, handler, { signal })and thatsignalis already aborted, the DOM standard says the listener-add operation returns early (so the listener is not added at all). [1]
Also note: AbortSignal.abort() creates an already-aborted signal that does not trigger an "abort" event (so attaching an "abort" listener to that signal will never fire). [3]
Typical safe pattern
function onAbort() { /* cleanup */ }
if (signal.aborted) onAbort(); // or: signal.throwIfAborted()
else signal.addEventListener("abort", onAbort, { once: true });Sources: DOM Standard (EventTarget addEventListener + abort signaling) [1]; MDN AbortSignal (immediate rejection guidance) [2]; MDN AbortSignal.abort() (no abort event) [3].
[1] (dom.spec.whatwg.org)
[2] (developer.mozilla.org)
[3] (developer.mozilla.org)
Citations:
- 1: https://dom.spec.whatwg.org/
- 2: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal?utm_source=openai
- 3: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/abort_static?utm_source=openai
Close the abort race in the tool wait path.
The gap between the abortSignal.aborted check (line 83) and listener registration (line 89) allows the signal to abort in that window. Once aborted, the "abort" event won't fire for newly attached listeners, leaving the promise forever pending.
Relocate the listener registration into the promise and check aborted afterward, ensuring the handler runs whether the signal aborts before or after registration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/tests/TestAgent.timeout.integration.test.ts` around lines 83 - 97, The
current wait loop checks abortSignal.aborted before registering the "abort"
listener, leaving a race where the signal can fire between the check and
addEventListener; to fix, move the abortSignal.addEventListener call inside the
new Promise constructor used in the wait path (the Promise that currently awaits
rejection), set the listener to reject with toError(abortSignal.reason) and mark
abortObserved = true in the handler, and then immediately after registering the
listener check abortSignal.aborted and if true call reject/toError (or throw) so
the handler runs whether the signal aborted before or after registration; update
the code around abortSignal, abortObserved, and the Promise used in the await to
reflect this ordering.
1 participant
Note
Medium Risk
Changes the prompt loop termination and runtime bounds passed into the underlying AI SDK, which can alter agent behavior and failure modes; mitigated by added unit/integration coverage for stop conditions and cooperative timeouts.
Overview
Adds new
TestAgent.prompt()options to control prompt loops and runtime bounds:stopWhen(one or many AI SDK stop conditions) is merged with the existingstepCountIs(maxSteps)guard, andtimeoutis forwarded togenerateTextto abort long-running prompts/tools via an abort signal.Re-exports AI SDK helpers (
hasToolCall,stepCountIs,StopCondition) from@mcpjam/sdk, adds unit + integration tests validating stop-condition merging, tool execution behavior, and timeout abort handling, and updates docs/README wording and examples to reflect the newstopWhen/timeoutcapabilities.Written by Cursor Bugbot for commit 8bfc602. This will update automatically on new commits. Configure here.