Răsfoiți Sursa

Honor an already-aborted signal in the subagent tool bridge (review feedback)

addEventListener('abort') does not fire for a signal already aborted before the
listener is added, so a parent step cancelled before the subagent tool ran
would never reach the child — the tool leaned on each provider re-checking
request.signal itself, leaving the bridge's own claim incomplete for any
provider that relies on run.cancel(). Re-check exec.signal.aborted right after
registering and cancel explicitly. Regression test uses a spy provider that
only reacts to cancel() (never inspects the signal); proven to hang without the
fix (result never settles) and settle aborted with it.
Tianyi Cui 3 luni în urmă
părinte
comite
8723186398

+ 5 - 0
packages/subagent/tool-subagent/src/index.ts

@@ -136,6 +136,11 @@ export function apply(ctx: Context, config: Config): void {
       // aborted while the child is in flight, cancel the child too.
       const onAbort = (): void => { run.cancel('parent step aborted') }
       exec.signal?.addEventListener('abort', onAbort, { once: true })
+      // `addEventListener` does NOT fire for a signal already aborted before this
+      // line, so a step cancelled before the tool ran would never reach the
+      // child. Cancel explicitly in that case — the bridge must honor an
+      // already-aborted signal, not lean on each provider re-checking it.
+      if (exec.signal?.aborted) run.cancel('parent step aborted')
 
       try {
         const result = await run.result

+ 37 - 0
packages/subagent/tool-subagent/tests/tool-subagent.spec.ts

@@ -282,6 +282,43 @@ describe('dsh-tool-subagent', () => {
     expect(result.isError).toBe(true)
   })
 
+  it('cancels the run when the tool signal is ALREADY aborted before execute (no missed abort)', async () => {
+    // `addEventListener('abort')` does not fire for a signal already aborted
+    // before the listener is added, so a step cancelled before the tool ran
+    // would never reach the child unless the bridge re-checks `signal.aborted`.
+    // A provider that leans only on the abort EVENT (this spy never inspects
+    // request.signal) proves the bridge itself must cancel.
+    const cancelled = vi.fn()
+    const ctx = new Context()
+    await ctx.plugin(SystemPrompt)
+    await ctx.plugin(ToolRegistry)
+    await ctx.plugin(SubagentService)
+    ctx.subagents.registerProvider({
+      name: 'spy',
+      capabilities: { outputSchema: false, depthLimit: false, toolFilter: false },
+      start: () => {
+        let resolveResult: (r: { output: never[]; stopReason: 'aborted' }) => void
+        const result = new Promise<{ output: never[]; stopReason: 'aborted' }>((res) => { resolveResult = res })
+        return {
+          id: AgentId('spy-child'),
+          result,
+          cancel: () => {
+            cancelled()
+            resolveResult({ output: [], stopReason: 'aborted' })
+          },
+          dispose: async () => {},
+        }
+      },
+    })
+    await ctx.plugin(tool, { provider: 'spy' })
+
+    const controller = new AbortController()
+    controller.abort() // already aborted BEFORE the tool runs
+    const result = await callSubagent(ctx, { description: 'd', prompt: 'p' }, { signal: controller.signal })
+    expect(cancelled).toHaveBeenCalledTimes(1)
+    expect(result.isError).toBe(true)
+  })
+
   it('tools depend on the service: no `subagent` tool without ctx.subagents', async () => {
     const ctx = new Context()
     await ctx.plugin(SystemPrompt)