Repository navigation
fix(flows): propagate downstream cancellation to the model stream - #1621
sangkyoonnam wants to merge 1 commit into
Conversation
|
Hi @sangkyoonnam, thank you for your contribution. We appreciate you taking the time to submit this pull request. As per contribution policy, PR should consists of a single commit. Could you please change your commits accordingly? |
26c291a to
a63b9cd
Compare
|
The reporter validated this head (a63b9cd) in production: 311 regression tests green, a mid-stream client disconnect left no further model or tool activity, and their cooperative cancel flag still works alongside. Details in #1618 (comment). |
|
@sangkyoonnam,thank you for the update and I noticed there is nothing in the merge commit. To proceed with the review, could you please squash them into a single commit? |
0bb1da0 to
a63b9cd
Compare
|
Squashed again: the branch is back to the single commit a63b9cd. The extra commit was an empty merge from main made with the Update branch button; I won't use it on this PR again. |
a63b9cd to
d217c29
Compare
|
@sangkyoonnam, thanks for the quick update. Currently this PR is under review by our team, we will keep you posted if any additional information is required. thank you. |
MiloszSobczyk
left a comment
There was a problem hiding this comment.
Hello, thank you for the PR.
I checked the change against FlowableCache, FlowableTakeUntil, and doOnCancel. doOnCancel marks itself cancelled before it runs the callback, so the cancelling subscriber never sees the forced onComplete, and run() does not start the next step. I also ran the new tests: 9/9 pass, and 6/9 fail with the one-line change reverted, as you describe. This also matches Python ADK, which closes the generator chain with Aclosing.
One thing outside this diff, please: Gemini builds the SSE stream as Flowable.fromFuture(streamFuture).flatMapIterable(iterable -> iterable) and never calls ResponseStream.close(). Before this change, the detached cache() read every stream to the end, so the HTTP connection was released. With this change, a cancel stops reading partway and nothing closes the stream, leaving the response open until it is garbage collected. That happens on every client disconnect in /run_sse, which is the case this PR fixes. Could you please close the stream on cancel—for example, using Flowable.using(..., ResponseStream::close)—either here or in a follow-up PR? Note that closing the reader can block while another thread is inside readLine().
Could you please also list the visible behavior changes in the commit message so users can find them in the release notes:
- In-flight tool calls are disposed on cancel. With
ToolExecutionMode.PARALLEL_SUBSCRIBEthe tool runs onSchedulers.io()by default, so its thread is interrupted. - An error that arrives after a cancel goes to
RxJavaPlugins.onError. Without a global error handler this becomes anUndeliverableException, which crashes Android apps. - An escalation in
ParallelAgentorLoopAgentnow stops the sibling sub-agents' model calls and tools, which used to run to the end.
d217c29 to
6461e27
Compare
|
Both are in the amended commit: the stream now closes on cancel, and the commit message lists the visible changes. I did the close here rather than in a follow-up, since this PR is what leaves the stream half-read on a disconnect.
The two cancel tests in Still one commit, rebased onto main. |
6461e27 to
dd914fc
Compare
|
Shortened both Javadocs and replaced the timed wait with |
- Cancelling the `Flowable` from `Runner.runAsync` now cancels the subscription to the current LLM step. `Gemini` schedules a close of the streamed response on cancel, including a response that opens after the cancel. Cancelling an active step also completes its shared cache; later subscribers to that cache replay buffered events and completion. - Active tool subscriptions are disposed on cancel. With two or more calls in `ToolExecutionMode.PARALLEL_SUBSCRIBE`, tool subscriptions use the agent's executor when set, otherwise the invocation's scheduler (`Schedulers.io()` by default). Disposing a running subscription task on the default IO scheduler from another thread interrupts its worker; other schedulers may not. - An error that becomes undeliverable after a cancel can reach `RxJavaPlugins.onError`, usually wrapped in `UndeliverableException`. Without a global handler RxJava passes it to the thread's uncaught exception handler, which can terminate an Android app. - Escalation in `ParallelAgent` now cancels the sibling sub-agents' active LLM steps and tools. Escalation in `LoopAgent`, outside durable resumable mode, now cancels the remaining LLM step and tools of the active sub-agent.
dd914fc to
542ad25
Compare
|
Rebased the single commit onto Since #1565, |
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
This covers the propagation half of #1618; a public cancel API (#1341) stays separate. Rebased onto #1565, which moved the changed line into
runStepAndCheckContinue; the swap is the same there.Problem:
Disposing the
FlowablefromRunner.runAsyncdoesn't stop the model call.BaseLlmFlow.runshares each step asrunOneStep(...).cache(), andFlowableCachenever cancels its upstream:CacheSubscription.cancel()only drops the subscriber from the list. Once the consumer disposes, the step runs on with zero subscribers, including the after-model callbacks and the step's tool calls. I measured the last part: a function call the model emits after the dispose still runs the tool.Solution:
Keep
cache()and put atakeUntilin front of it:step.takeUntil(cancelled.toFlowable()).cache().doOnCancel(cancelled::onComplete), oneCompletableSubjectper step. A cancel completes the signal,takeUntilcancelsrunOneStepand completes the cached stream. Whatrun()relies on fromcache()is unchanged: unbounded upstream demand, terminal delivered as soon as a subscriber drains the buffer, buffer kept for thetoList()subscriber that arrives after the step finished.After the change a disconnect cancels the step's pending reactive work: the in-flight model
Flowable, after-model callbacks not yet run, tool calls not yet started. A tool already executing has itsSingledisposed; whether its work stops is up to the tool. A model error arriving after the cancel can go toRxJavaPlugins.onErrorinstead of sitting in the cache for nobody, which is RxJava's normal post-cancel routing. Whether the HTTP request stops depends on the model client. ForGeministreaming, this PR schedules the response stream's close on cancel, including a stream that opens afterwards; the close can wait behind a blocked read.ParallelAgentescalation now cancels an active sibling's model stream, with a regression test. Outside durable resumable mode,LoopAgentescalation also cancels its active sub-agent's remaining step. #1434's cooperativeCancellationTokencuts the stream insidecallLlmand doesn't touchcache(), so the two compose.Testing Plan
Unit Tests:
BaseLlmFlowCancelTest(new, 9 tests): cancel reaches the model throughBaseLlmFlow.runand throughInMemoryRunner.runAsync, a dispose before a function call leaves the tool unrun, a cancel insideonSubscribedoesn't leave the model streaming (this one already passes on the rebased base, since #1565 checks for the cancel before subscribing to the step), aParallelAgentsibling escalation cancels the model;cache()semantics kept under bounded demand (run(ctx).test(1)gets the final event and completion, and a model error after a buffered event) and a new run that completes whenrun()is subscribed again after a cancel; a control run still runs the tool. Without theBaseLlmFlowchange 5 of the 9 fail../mvnw -pl core test: 2342 tests, 0 failures, 0 errors, 0 skipped.Manual End-to-End (E2E) Tests:
Not run. The dev server's
/run_ssedisposes ononCompletion/onTimeoutand the A2A executor'scancel()disposes itsCompositeDisposable; with this change both reach the model. I read those paths, I haven't exercised them.Checklist
Additional context
runLivedoesn't usecache()and isn't touched. Nothing in core subscribes to onerun()result twice at the same time, so "a subscriber cancels" and "the last subscriber cancels" are the same event here.I wrote this with Claude Code and reviewed and ran it myself.