Просмотр исходного кода

workflow: re-check cancellation after the slot acquire

Codex convergence round 1 on the review-response commits: agent()'s
post-acquire window was real and unguarded. `await acquireSlot()` yields
at least one microtask tick even when a slot is free (and a queued
waiter resumes a tick after its release), so a cancel() landing in that
tick let the continuation start a child carrying an ALREADY-aborted
signal — the in-code comment claimed the window could not exist. A
provider that subscribes only to future abort events (the test stub;
the seam does not promise pre-aborted-signal handling) would never
settle such a child, leaking it until the dispose grace abandoned the
run, and a backend that misses the pre-aborted signal would burn a full
model turn after the user cancelled.

agent() now re-checks isCancelled() immediately after the acquire
(inside the slot-owning try, so the finally still releases), making
every post-cancel path reject before subagents.start. New deterministic
regression: cancel() in the same synchronous frame as start() lands in
the free-slot await tick — the run settles cancelled with ZERO children
started (previously: one leaked child and a grace-delayed settle). The
raced-release test's comment now states what it actually pins (the
queued-waiter rejection path). Also aligns the RFC's auto-concurrency
formula with the code (min(16, max(1, availableParallelism() - 2))).
Tianyi Cui 2 месяцев назад
Родитель
Сommit
706691a7df

+ 1 - 1
docs/rfc/implemented/feature/2026-07-05-dynamic-workflows.md

@@ -30,7 +30,7 @@ One deliberate strictness DIVERGENCE from CC: hook misuse — unknown or deferre
 
 **Meta extraction**: a string/comment-aware brace scanner (template interpolation rejected) finds the literal; it is evaluated ALONE in an empty, timed vm context; the result must materialize to plain JSON data and pass shape validation (unknown fields rejected loud); the statement is blanked line-preservingly so stacks keep script line numbers.
 
-**Value boundary**: values entering the host (meta, hook options, schemas, the return value) go through `materializeFromRealm` — a plain recursive walk that rejects loud everything JSON cannot carry (exotic prototypes, functions, symbols, cycles, sparse arrays, non-finite numbers, nested `undefined`), copying via `Object.defineProperty` so a `"__proto__"` key becomes a data property, never a prototype mutation; getters are read ordinarily and their RESULT crosses (a throwing read fails loud). Values entering the realm (`args`, `agent()` results, hook promises and failures, combinator arrays) are handed over directly as host values — the script is trusted, so host prototypes are not a leak; `args` is host-`structuredClone`d once so a script cannot mutate the caller's object. Hook failures are host `WorkflowError`s: the combinators recognize fatality by host `instanceof` (unforgeable from the realm), and the script-visible consequence — in-script `instanceof Error` is `false` for hook errors; branch on `e.name`/`e.code` — is documented in the engine README. Realm functions (stages, thunks) are called, never materialized. Thrown script values are rendered by a total host-side renderer (stack → message → `String()`, fixed label if rendering throws), so `result` cannot reject. Caps (`maxConcurrentAgents` auto = `min(16, cores - 2)`, `maxTotalAgents` 1000, `maxItemsPerCall` 4096) and timeouts are validated Config, not literals.
+**Value boundary**: values entering the host (meta, hook options, schemas, the return value) go through `materializeFromRealm` — a plain recursive walk that rejects loud everything JSON cannot carry (exotic prototypes, functions, symbols, cycles, sparse arrays, non-finite numbers, nested `undefined`), copying via `Object.defineProperty` so a `"__proto__"` key becomes a data property, never a prototype mutation; getters are read ordinarily and their RESULT crosses (a throwing read fails loud). Values entering the realm (`args`, `agent()` results, hook promises and failures, combinator arrays) are handed over directly as host values — the script is trusted, so host prototypes are not a leak; `args` is host-`structuredClone`d once so a script cannot mutate the caller's object. Hook failures are host `WorkflowError`s: the combinators recognize fatality by host `instanceof` (unforgeable from the realm), and the script-visible consequence — in-script `instanceof Error` is `false` for hook errors; branch on `e.name`/`e.code` — is documented in the engine README. Realm functions (stages, thunks) are called, never materialized. Thrown script values are rendered by a total host-side renderer (stack → message → `String()`, fixed label if rendering throws), so `result` cannot reject. Caps (`maxConcurrentAgents` auto = `min(16, max(1, availableParallelism() - 2))`, `maxTotalAgents` 1000, `maxItemsPerCall` 4096) and timeouts are validated Config, not literals.
 
 ### The consumer (dsh-tool-workflow)
 

+ 6 - 4
packages/workflow/workflow-vm/src/runtime.ts

@@ -375,10 +375,12 @@ export class WorkflowExecution {
 
     await this.acquireSlot()
     try {
-      // No cancelled re-check here: a cancel cannot interleave between a
-      // waiter's resolution and this continuation (single-threaded, no await
-      // between them), and a child started moments after a cancel still dies
-      // via the shared abort signal — the CANCELLED mapping below covers it.
+      // Re-check after the acquire: the await yields at least one microtask
+      // tick even when a slot is free, and a queued waiter resumes a tick
+      // after its release — a cancel() landing in either window must not
+      // start a child (it would carry an ALREADY-aborted signal, which a
+      // provider subscribing only to future abort events would never see).
+      if (this.isCancelled()) throw this.cancelledError()
       let run
       try {
         run = this.ctx.subagents.start(this.limits.provider, {

+ 20 - 2
packages/workflow/workflow-vm/tests/workflow-vm.spec.ts

@@ -636,6 +636,22 @@ describe('dsh-workflow-vm', () => {
       expect(result.error).toBe('stackless failure')
     })
 
+    it('cancel() in the same frame as start(): the awaited slot tick cannot start a child', async () => {
+      const { ctx, parent, provider } = await setup({ manual: true })
+      // agent() enters during start()'s synchronous slice and suspends on the
+      // acquireSlot await (one microtask tick even with a free slot); the
+      // synchronous cancel below lands in that tick. Without the post-acquire
+      // re-check the continuation would start a child carrying an ALREADY-
+      // aborted signal — which the stub provider (subscribing only to future
+      // abort events, like a real backend) would never settle, leaking it.
+      const handle = ctx.workflows.start({ script: script("return await agent('never')"), parent })
+      handle.cancel('immediately after start')
+      const result = await handle.result
+      expect(result.stopReason).toBe('cancelled')
+      expect(provider.runs.length).toBe(0)
+      await handle.dispose()
+    })
+
     it('a waiter resumed by a release RACING a cancel still dies at the post-acquire check', async () => {
       const { ctx, parent, provider } = await setup({ manual: true, config: { provider: 'stub', maxConcurrentAgents: 1 } })
       const handle = ctx.workflows.start({
@@ -643,8 +659,10 @@ describe('dsh-workflow-vm', () => {
         parent,
       })
       await vi.waitFor(() => { expect(provider.runs.length).toBe(1) })
-      // Same synchronous block: the release resolves b's waiter, then the
-      // cancel lands BEFORE b's continuation runs — b must not start a child.
+      // Same synchronous block: b is still a QUEUED waiter when the cancel
+      // lands, so cancel() rejects it outright; together with the immediate-
+      // cancel test above (the resumed-waiter tick), no post-cancel path can
+      // reach subagents.start.
       provider.runs[0]!.settle(text('a-done'))
       handle.cancel('raced')
       const result = await handle.result