Skip to content

Comments

fix (core): consume stream on abort#5492

Merged
iteratetograceness merged 11 commits intovercel:mainfrom
iteratetograceness:consume-stream
Apr 5, 2025
Merged

fix (core): consume stream on abort#5492
iteratetograceness merged 11 commits intovercel:mainfrom
iteratetograceness:consume-stream

Conversation

@iteratetograceness
Copy link
Contributor

@iteratetograceness iteratetograceness commented Apr 1, 2025

Addresses #5459

This PR ensures that the consumeStream method on streamText does not throw unhandled errors.

NOTE: This is an edge case that occurs when an abortSignal is passed to streamText; the aborted signal propagates to consumeStream as a ResponseAborted error but we were not catching this error. This then triggered a network error, caught by the useChat hook (setting the hook status to error).

@lgrammel lgrammel changed the title fix: consume stream on abort fix (core): consume stream on abort Apr 2, 2025
@lgrammel
Copy link
Collaborator

lgrammel commented Apr 2, 2025

Needs a changeset.

@lgrammel
Copy link
Collaborator

lgrammel commented Apr 2, 2025

How does this affect the useChat UI parts?

@iteratetograceness
Copy link
Contributor Author

iteratetograceness commented Apr 2, 2025

How does this affect the useChat UI parts?

Before this change, because result.consumeStream would throw an unhandled response aborted error, and useChat would update status to error (disabling sending of subsequent messages)!

More details here

@lgrammel
Copy link
Collaborator

lgrammel commented Apr 2, 2025

How does this affect the useChat UI parts?

Before this change, because result.consumeStream would throw an unhandled response aborted error, and useChat would update status to error (disabling sending of subsequent messages)!

More details here

So consume stream would break an ongoing response? (asking because at that point consume stream should run async and separately of the function)

@iteratetograceness
Copy link
Contributor Author

So consume stream would break an ongoing response? (asking because at that point consume stream should run async and separately of the function)

Ah aligned, consumeStream = independent from function! I misspoke (corrected above), and the unhandled error does/should not directly impact useChat.

Yesterday, while repro-ing, the unhandled error from consumeStream seemed to put the server in a funky state - verifying now but upon the subsequent user message, useChat's status is stuck on submitted

@lgrammel
Copy link
Collaborator

lgrammel commented Apr 2, 2025

The consumeStream resilience is very good regardless (and maybe it should swallow all errors or have an error callback to not break any server).

@iteratetograceness
Copy link
Contributor Author

@lgrammel heard!

Also, I'm not crazy, I did see status set to error when we do not properly swallow abort errors from consumeStream! See video:

CleanShot.2025-04-02.at.11.49.17.mp4

If stop fires while long response is streaming, consumeStream's unhandled abort error seems to bubble up in triggerRequest's catch as a network error!

@lgrammel
Copy link
Collaborator

lgrammel commented Apr 2, 2025

@iteratetograceness that is very interesting - do you know what happens (on the network level)? is the next js server forwarding the error through the stream somehow?

other server might behave differently

@iteratetograceness
Copy link
Contributor Author

@iteratetograceness that is very interesting - do you know what happens (on the network level)? is the next js server forwarding the error through the stream somehow?

other server might behave differently

I don't think this is Next.js server specific! I believe the unhandled network error is bubbling up to the nearest error handler in the event loop (which prior to these changes would be triggerRequest's catch)

@iteratetograceness
Copy link
Contributor Author

@lgrammel oh but I am seeing a slightly different edge case now! If a subsequent message isn't sent after the stopping, onFinish does not trigger. Any inklings on what causes this? I'm gonna take a closer look at the stream logic in stream-text.ts

@iteratetograceness
Copy link
Contributor Author

@lgrammel tldr, when the abort signal is passed into the streamText, consumeStream does not trigger onFinish (the onFinish callback is only called as a side effect of subsequent messages' flushes)

im stepping away for a bit but will come back to dig deeper and flesh out the appropriate solution (e.g. maybe something along the lines of adding new user-cancellation finishReason and adding logic for this case)

If an error occurs, it is passed to the optional `onError` callback.
*/
consumeStream(): Promise<void>;
consumeStream(onError?: (error: unknown) => void): Promise<void>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer options object

@iteratetograceness iteratetograceness merged commit 665a567 into vercel:main Apr 5, 2025
6 of 7 checks passed
lgrammel pushed a commit that referenced this pull request Apr 6, 2025
@ItayElgazar
Copy link

ItayElgazar commented Apr 19, 2025

is it released already? I am still experiencing the bug, even after upgrading to v4.3.9

@iteratetograceness
Copy link
Contributor Author

@ItayElgazar this fix has already been released. I recommend opening a new discussion/issue with more context or minimal repro steps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants