refactor(session): use onInterrupt finalizer for cancelled tool output#21751
Closed
kitlangton wants to merge 1 commit intodevfrom
Closed
refactor(session): use onInterrupt finalizer for cancelled tool output#21751kitlangton wants to merge 1 commit intodevfrom
kitlangton wants to merge 1 commit intodevfrom
Conversation
c1f0673 to
f37b4b5
Compare
Wire the AI SDK's abortSignal into the tool fiber via runPromiseExit's signal option so interruption is first-class, and move the "finalize on cancel" path into an Effect.onInterrupt finalizer that re-awaits the still-running native Promise uninterruptibly, builds the output, and posts it through completeToolCall. Replaces the imperative `if (options.abortSignal?.aborted)` tail check with structural interruption handling. When the fiber is interrupted, the finalizer captures the truncated bash output (or MCP tool result) and the .then on runPromiseExit resolves the SDK's Promise with the captured value instead of propagating the interrupt cause as a rejection, so the tool is reported as successfully completed rather than as a tool-error. InstanceRef is provided on the tool fiber so InstanceState.context resolves through ServiceMap rather than falling through to the AsyncLocalStorage, which the onInterrupt finalizer runs outside of.
f37b4b5 to
ea19ee7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #21724. Replaces the tool body's imperative
if (options.abortSignal?.aborted) completeToolCall(...)tail-check with structural interruption handling viaEffect.onInterrupt, then extracts the whole pattern into a smallrunToolExecutehelper so the two call sites (built-in and MCP) read as declarative specs.The core question we were poking at: the AI SDK hands us a detached
Effect.runPromiseorphan fiber — but can Effect's interruption machinery still handle cancel cleanly if we wire it up right? Answer: yes. This PR shows what that looks like.The helper
Four callbacks, four phases:
before— plugin hooks, permission asks, anything pre-executeexecute— the native Promise-returning call. Reference held outside Effect so a finalizer can re-await it.finalize— builds the tool output from the raw result. Runs on both the happy path and the interrupt path.onCancel— the side channel into the processor, only called if interrupted mid-flight.Under the hood:
runPromise(..., { signal: options.abortSignal })wires the SDK abort signal directly into fiber interrupt — the tool fiber is no longer orphan.Effect.onInterruptfinalizer re-awaits the same in-flightpendinguninterruptibly (finalizers always are), runsfinalizeagain, posts viaonCancel, stashes the output in arescuedclosure var.Effect.catchCauseat the tail converts interrupt-with-rescued-output back into success, so the Promise returned to the SDK resolves with the finalized output instead of rejecting. SDK reports the tool as successful, nottool-error.attach()(exported from@/effect/run-service) providesInstanceRefto the fiber soonInterruptfinalizers — which run outside the original ALS chain — can still resolve the instance context through the ServiceMap. EverymakeRuntime-created runner already usesattachinternally; now the tool fibers do too.Built-in tool call site (the whole thing)
No
pendingtracking, nocapturedtracking, norunPromiseExit+Exit.isSuccess/Cause.squashdance, no ALS gotcha at the call site, no imperativeif (aborted)branch. Reads as "here's what to do before; here's the Promise; here's how to build the output; here's the cancel fallback."Gotcha: AsyncLocalStorage loss inside
onInterruptfinalizersThis took a while to track down — documenting it so the next person doesn't repeat it.
The original code worked because the tool body ran synchronously down the Promise chain from inside
Instance.provide'sAsyncLocalStorage.runcall. Node preserves ALS through Promise microtasks, soInstance.currentwas always live all the way down, including inside bus-publish subscribers.With
onInterrupt, the finalizer is invoked from the interrupt propagation path — specifically, thesignal.addEventListener("abort", () => fiber.interruptUnsafe())callback thatrunPromiseregisters. That listener fires outside the original ALS chain. When the finalizer's downstream code (bus.publish→ sync subscriber insync/index.ts:155→result.then(...)→Database.transaction) eventually callsInstance.current, ALS is empty and it throwsNotFound.Fix: the existing
attachhelper from@/effect/run-service(previously private, now exported) capturesInstance.currentsynchronously and provides it asInstanceRef.InstanceState.contextalready prefersInstanceRefover the ALS fallback, so everything resolves through ServiceMap. EverymakeRuntimerunner in the codebase already goes throughattach; the tool fiber was the odd one out.Debug path that led here:
completeToolCallcompletes, part is markedcompleted✓process()fiber's exit isFailure, test assertsExit.isSuccessand failsinstance-state.ts:28viabus/index.ts:172→sync/index.ts:155.then()in the bus publish sync subscriber, running in a microtask whose parent chain originated from the interrupt listener rather than the originalexecute()call — ALS droppedWhat this buys you
executeis a declarative spec. No knowledge ofcompleteToolCallor runPromise plumbing at the call site.attach.What this doesn't buy you
Deferredinprocessor.tsis still required. The processor-cleanup-loop vs tool-finalizer race is a separate concern. Eliminating it needs making tool fibers scoped children of the processor fiber so its cleanup naturally awaits them. Out of scope here.Draft status / open questions
runToolExecuteat the top ofprompt.ts. Could live in a separate file (src/session/tool-execute.ts) if we expect to grow it.anytypes on the callback Effect generics. UsingEffect.Effect<unknown, any, any>forbefore/finalize/onCancelto sidestep E and R unification. Could tighten with proper generics but gets noisy; all call sites haveR = never(services are closure-captured) so it doesn't matter at runtime.Deferredinprocessor.tsas a follow-up by restructuring tool fibers to be scoped children of the processor fiber? Separate PR if so.Testing
bun typecheckcleanbun run test --timeout 60000 test/session/prompt-effect.test.ts -t "cancel finalizes interrupted bash tool output through normal truncation"passesbun run test --timeout 60000 test/session/— 241 pass / 4 skip / 1 todo / 0 fail