fix(server-core): preserve abortSignal when resumable streams are enabled#1281
fix(server-core): preserve abortSignal when resumable streams are enabled#1281Jefsky wants to merge 1 commit into
Conversation
…bled When resumableStream is enabled, the previous code unconditionally set options.abortSignal = undefined before streamText(), making it impossible for clients to cancel an in-progress generation. The fix: - Saves the client's abortSignal before clearing it - Makes clearActiveStream fire-and-forget (no await) so client abort cannot block handler cleanup - Restores the signal before calling streamText so cancellation works Fixes VoltAgent#1239 Co-authored-by: Jefsky Agent <agent@example.com>
|
📝 WalkthroughWalkthroughThe PR fixes the issue where ChangesAbort Signal Preservation in Resumable Streams
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/server-core/src/handlers/agent.handlers.ts (2)
321-321: 💤 Low valueConsider simplifying the save-and-restore pattern.
Lines 321 and 332 save
options.abortSignaltoclientSignaland then restore it, but no code between these lines modifiesoptions.abortSignal. This pattern appears redundant unless it's intended as defensive programming for future changes.If the intent is clarity, the current implementation is fine. Otherwise, you could simplify by removing both lines since the signal is never cleared in the new implementation.
Also applies to: 332-332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/handlers/agent.handlers.ts` at line 321, The save-and-restore of the abort signal is redundant: remove the temporary variable assignment "const clientSignal = options.abortSignal;" and the later restore "options.abortSignal = clientSignal;" in the surrounding function (where options.abortSignal is referenced) since nothing between them mutates options.abortSignal; ensure no other code in that function expects the restore before deleting these two lines.
319-333: ⚖️ Poor tradeoffThe fire-and-forget cleanup pattern is intentional but does create a potential race condition.
The comments explicitly document that
clearActiveStreamis fire-and-forget to avoid blocking the hot path and prevent client abort signals from delaying cleanup. However, when called without astreamIdparameter (line 326), the operation will delete any active stream for the given context. SincecreateStreamat line 351 is called asynchronously afterward and sets a newactiveStreamId, a race exists where cleanup could delete the newly created stream if it completes aftersetActiveStreamId. This appears to be an intentional performance tradeoff rather than a hidden bug.The save/restore pattern for
abortSignal(lines 321 and 332) appears redundant—nothing modifies the signal between these two lines, and the comment suggests it's precautionary. If the signal was never actually cleared in this block, the save/restore is unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/handlers/agent.handlers.ts` around lines 319 - 333, clearActiveStream is being called fire-and-forget without a streamId which can race with the later createStream/setActiveStreamId and may delete the newly-created stream; update the call to resumableStreamAdapter.clearActiveStream to pass the specific active stream id (e.g., previousActiveStreamId) or otherwise provide a conditional/compare option so cleanup only removes the intended stream, and remove the redundant save/restore of options.abortSignal since nothing mutates it in this block (references: resumableStreamAdapter.clearActiveStream, createStream / setActiveStreamId, options.abortSignal).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/server-core/src/handlers/agent.handlers.ts`:
- Line 321: The save-and-restore of the abort signal is redundant: remove the
temporary variable assignment "const clientSignal = options.abortSignal;" and
the later restore "options.abortSignal = clientSignal;" in the surrounding
function (where options.abortSignal is referenced) since nothing between them
mutates options.abortSignal; ensure no other code in that function expects the
restore before deleting these two lines.
- Around line 319-333: clearActiveStream is being called fire-and-forget without
a streamId which can race with the later createStream/setActiveStreamId and may
delete the newly-created stream; update the call to
resumableStreamAdapter.clearActiveStream to pass the specific active stream id
(e.g., previousActiveStreamId) or otherwise provide a conditional/compare option
so cleanup only removes the intended stream, and remove the redundant
save/restore of options.abortSignal since nothing mutates it in this block
(references: resumableStreamAdapter.clearActiveStream, createStream /
setActiveStreamId, options.abortSignal).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c109407e-7906-4d7c-9b35-0c6e1e207e6e
📒 Files selected for processing (1)
packages/server-core/src/handlers/agent.handlers.ts
Summary
When
resumableStreamis enabled, the handler was unconditionally clearing the client'sabortSignalbefore callingstreamText(). This made it impossible for clients to cancel an in-progress LLM generation — clicking "Stop" would only stop the client-side display while the backend continued running to completion.What changed
packages/server-core/src/handlers/agent.handlers.tsSaves and restores
abortSignal— stores the client's signal in a local variable before clearing it, then restores it beforestreamText(). The client can now cancel the LLM stream as intended.Makes
clearActiveStreamfire-and-forget — instead ofawaiting the cleanup, it calls.catch()so the cleanup runs in the background. This means a client abort mid-cleanup cannot block the handler.Moves
resumableStreamAdapterdeclaration earlier — to make the early fire-and-forget cleanup possible within theresumableStreamEnabledblock.Root cause
Closes #1239
Summary by cubic
Fixes server-side cancellation when
resumableStreamis enabled by preserving the clientabortSignaland making stream cleanup non-blocking. Clients can now reliably stop in-progress LLM generations.abortSignalsoagent.streamText()respects cancellation.resumableStreamAdapter.clearActiveStream(...)fire-and-forget with.catch(...)to avoid blocking on cleanup.Written for commit a8d90f8. Summary will update on new commits.
Summary by CodeRabbit