fix(r): report a response that fails before it streams anything - #314
fix(r): report a response that fails before it streams anything#314nbenn wants to merge 3 commits into
Conversation
|
Weighing in on the "Calls I made" — this is a well-reasoned change, and
I think keeping it additive is the right call and it's not worth adding a new On the third option — exposing the task. I took a look at this and landed on
So:
Good news! Driving a
At this point, I'm okay with the duplication. If it ends up being annoying, we can chase down a resolution later. |
chat_append_stream_impl() is a coroutine, so the loop consuming the stream runs eagerly, ahead of its first await(). A stream that fails before it yields anything therefore raised its error synchronously rather than rejecting, which skipped the promises::catch() handling in chat_append_stream() entirely -- so nothing was shown in the chat and the browser kept the loading indicator it raised on submit, with the composer left disabled. ellmer::Chat$stream_async() returns a synchronous generator that performs the request on its first advance, so this is the shape of every turn a provider rejects outright: an exhausted quota, an over-long context, a dropped connection. Only a mid-stream failure took the path that reports the error. Catch the synchronous throw and turn it into a rejection, which routes it into the handling already there.
The stream task's result was never read, so a rejection was captured by the ExtendedTask and dropped. on_stream_complete() branches only on "success", and status() reports both a finished and a failed response as "idle", leaving a caller no way to tell them apart. Expose last_error on the chat_server() return, derived from the task rather than stored: it reads the condition when the task is in "error" and NULL otherwise, so it follows the task to "running" on the next turn and to "success" when one lands. A stored copy would need resetting in both places and would read stale in between. This stays observation-only. chat_append_stream() already shows the error in the chat once the failure reaches it, so surfacing it here as well would report the same failure twice.
Both from review. The wait loop had no upper bound, and testthat applies no per-test timeout, so a regression that stopped the rejection reaching the task would have spun until the CI job itself timed out. Bound it with the deadline idiom flush_promises() already uses, keeping the flushReact() interleave this test needs, and add an expect_false() so a missing error reads as a failure rather than a hang. Assert the fail-before-yield rejection is a shiny.silent.error, matching the sibling mid-stream test. That the two paths converge on the same handling is the point of the fix, so the class is what proves it.
904f095 to
14d77d4
Compare
|
Rebased onto Nothing else drifted: The workflow runs are sitting at |
2 participants
Fixes #304.
The error path in
chat_append_stream()never runs for a response that fails before it streams anything. Its stream is consumed inside a coroutine ahead of the firstawait, so the failure is raised synchronously rather than rejecting, skips bothpromises::catch()calls, and ends up in thechat_server()stream task whose result nothing reads. The browser keeps the loading indicator it raised on submit, with the composer disabled. That is the shape of every turn a provider rejects outright, sinceellmer::Chat$stream_async()performs the request on the generator's first advance.Commit 1,
fix(r)— catch the synchronous throw and turn it into a rejection, which routes it into the handling already there. The error then renders throughsanitized_chat_error()and the composer is released.Commit 2,
feat(r)— expose the condition aslast_erroron thechat_server()return, derived from the task rather than stored. The Python package already reads_stream_task.error()in an effect, so R was the outlier. Drop this commit if you would rather take the fix alone.Calls I made
Some judgement calls you might want to weigh in on:
last_errorvalue is additive, rather than makingstatus()three-valued. An"error"state is more honest but breaks callers testingstatus() == "idle"for completion. Exposing the task itself, as Python'slatest_message_streamdoes, is a third option.NotifyExceptionfrom an effect; the R equivalent is anobserve(), where a raised error takes the session down. Control over presentation belongs in R pkg: argument to control how errors are displayed in the chat #276.chat_append_stream()'s. Reading$result()does not mark the task handled. A mid-stream failure already does this, so the two classes now match.last_errortest keeps aMockShinySessionopen rather than usingtestServer(), which leaks an unhandled rejection when anExtendedTasksettles after teardown. Happy to use a house pattern if there is one.