workflow: close the review-found cancellation and child-lifecycle gaps
Three review findings on the engine's child seam, one mechanism each: - Cancellation now bridges to run.cancel() on every in-flight child, not just the shared request signal — the subagent seam leaves a provider free to honor either channel, so the consumer drives both (listener removed in the child finally). - A child result REJECTION (an infrastructure fault the seam allows) now emits the paired workflow/agent-end before propagating, and propagates as a fatal WorkflowError with the new AGENT_RESULT code — previously it skipped agent-end (permanently open child for seq-matching observers) and dissolved to a per-item null inside parallel()/pipeline(), letting a broken provider read as an ordinary failed child. A rejection landing after cancel stays a cancellation (cancelled outcome + CANCELLED). - Every hook now guards its entry with a shared throwIfCancelled(): phase()/log() no longer emit observer events after a script caught an earlier cancelled rejection, and parallel()/pipeline() refuse entry — cancellation is the next HOOK boundary, not just the next agent().
This commit is contained in:
@@ -242,7 +242,7 @@ Semantics every implementation must honor:
|
|||||||
abstract start(request: WorkflowStartRequest): WorkflowRun
|
abstract start(request: WorkflowStartRequest): WorkflowRun
|
||||||
```
|
```
|
||||||
|
|
||||||
Source: [`packages/workflow/workflow/src/index.ts:194`](../../packages/workflow/workflow/src/index.ts)
|
Source: [`packages/workflow/workflow/src/index.ts:198`](../../packages/workflow/workflow/src/index.ts)
|
||||||
|
|
||||||
## Inherited `ctx` members (cordis core + loader/hmr/timer)
|
## Inherited `ctx` members (cordis core + loader/hmr/timer)
|
||||||
|
|
||||||
|
|||||||
@@ -17,10 +17,11 @@
|
|||||||
* realm-side until they cross through a hook or the final return.
|
* realm-side until they cross through a hook or the final return.
|
||||||
*
|
*
|
||||||
* Failure discipline: fatal {@link WorkflowError}s (bad hook arguments,
|
* Failure discipline: fatal {@link WorkflowError}s (bad hook arguments,
|
||||||
* unsupported options/schemas, tripped caps, seam start failures,
|
* unsupported options/schemas, tripped caps, seam start failures and result
|
||||||
* cancellation) ALWAYS propagate through `parallel`/`pipeline` — recognized
|
* rejections, cancellation) ALWAYS propagate through `parallel`/`pipeline` —
|
||||||
* by host `instanceof`, which a script cannot forge — and the per-item `null`
|
* recognized by host `instanceof`, which a script cannot forge — and the
|
||||||
* is reserved for child-run failures and ordinary in-stage script errors.
|
* per-item `null` is reserved for child-run failures and ordinary in-stage
|
||||||
|
* script errors.
|
||||||
* Every hook-returned promise gets a no-op rejection consumer attached, so a
|
* Every hook-returned promise gets a no-op rejection consumer attached, so a
|
||||||
* script that drops a promise (fires an `agent()` without awaiting it) cannot
|
* script that drops a promise (fires an `agent()` without awaiting it) cannot
|
||||||
* surface an unhandled rejection when cancellation rejects it — the app boot
|
* surface an unhandled rejection when cancellation rejects it — the app boot
|
||||||
@@ -206,6 +207,17 @@ export class WorkflowExecution {
|
|||||||
return this.cancelReason !== undefined
|
return this.cancelReason !== undefined
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Shared hook entry guard: after {@link cancel}, EVERY hook throws
|
||||||
|
* `CANCELLED` at its next call — cancellation is the next HOOK boundary,
|
||||||
|
* not just the next `agent()`, so a script that caught one cancelled
|
||||||
|
* rejection cannot keep emitting progress through `phase`/`log` or enter a
|
||||||
|
* combinator.
|
||||||
|
*/
|
||||||
|
private throwIfCancelled(): void {
|
||||||
|
if (this.isCancelled()) throw this.cancelledError()
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Cancel the run: children abort (the shared signal), waiting `agent()`
|
* Cancel the run: children abort (the shared signal), waiting `agent()`
|
||||||
* slots reject, and every future hook call throws `CANCELLED` — the script
|
* slots reject, and every future hook call throws `CANCELLED` — the script
|
||||||
@@ -358,7 +370,7 @@ export class WorkflowExecution {
|
|||||||
|
|
||||||
/** The `agent(prompt, opts)` hook. */
|
/** The `agent(prompt, opts)` hook. */
|
||||||
private async agent(rawPrompt: unknown, rawOpts: unknown): Promise<unknown> {
|
private async agent(rawPrompt: unknown, rawOpts: unknown): Promise<unknown> {
|
||||||
if (this.isCancelled()) throw this.cancelledError()
|
this.throwIfCancelled()
|
||||||
if (typeof rawPrompt !== 'string' || rawPrompt.length === 0) {
|
if (typeof rawPrompt !== 'string' || rawPrompt.length === 0) {
|
||||||
throw new WorkflowError('agent() requires a non-empty prompt string', 'INVALID_ARGUMENT')
|
throw new WorkflowError('agent() requires a non-empty prompt string', 'INVALID_ARGUMENT')
|
||||||
}
|
}
|
||||||
@@ -381,7 +393,7 @@ export class WorkflowExecution {
|
|||||||
// after its release — a cancel() landing in either window must not
|
// after its release — a cancel() landing in either window must not
|
||||||
// start a child (it would carry an ALREADY-aborted signal, which a
|
// start a child (it would carry an ALREADY-aborted signal, which a
|
||||||
// provider subscribing only to future abort events would never see).
|
// provider subscribing only to future abort events would never see).
|
||||||
if (this.isCancelled()) throw this.cancelledError()
|
this.throwIfCancelled()
|
||||||
let run
|
let run
|
||||||
try {
|
try {
|
||||||
run = this.ctx.subagents.start(this.limits.provider, {
|
run = this.ctx.subagents.start(this.limits.provider, {
|
||||||
@@ -396,8 +408,30 @@ export class WorkflowExecution {
|
|||||||
}
|
}
|
||||||
const info: WorkflowAgentInfo = { seq, label, ...phase !== undefined ? { phase } : {}, childId: run.id }
|
const info: WorkflowAgentInfo = { seq, label, ...phase !== undefined ? { phase } : {}, childId: run.id }
|
||||||
this.observer.agentStart(info)
|
this.observer.agentStart(info)
|
||||||
|
// Cancellation bridges to run.cancel() as well as the request signal:
|
||||||
|
// the seam leaves a provider free to honor either channel, so the
|
||||||
|
// consumer must drive both. The signal cannot be aborted yet (the block
|
||||||
|
// since the post-acquire check is synchronous), so the listener always
|
||||||
|
// arms; `once` plus the finally removal keep it leak-free.
|
||||||
|
const onAbort = (): void => { run.cancel(this.cancelReason) }
|
||||||
|
this.controller.signal.addEventListener('abort', onAbort, { once: true })
|
||||||
try {
|
try {
|
||||||
const result = await run.result
|
let result
|
||||||
|
try {
|
||||||
|
result = await run.result
|
||||||
|
} catch (error: unknown) {
|
||||||
|
// The seam allows `result` to reject for an INFRASTRUCTURE fault —
|
||||||
|
// distinct from a child that failed and resolved. Pair the
|
||||||
|
// lifecycle before propagating, and propagate FATAL: an ordinary
|
||||||
|
// throw would dissolve to a per-item null inside the combinators,
|
||||||
|
// and a broken provider must not read as a failed child.
|
||||||
|
if (this.isCancelled()) {
|
||||||
|
this.observer.agentEnd({ ...info, outcome: 'cancelled' })
|
||||||
|
throw this.cancelledError()
|
||||||
|
}
|
||||||
|
this.observer.agentEnd({ ...info, outcome: 'failed' })
|
||||||
|
throw new WorkflowError(`child agent run failed: ${renderThrown(error)}`, 'AGENT_RESULT', { cause: error })
|
||||||
|
}
|
||||||
if (result.stopReason === 'completed') {
|
if (result.stopReason === 'completed') {
|
||||||
if (opts.schema !== undefined) {
|
if (opts.schema !== undefined) {
|
||||||
// The provider honored outputSchema (capability-gated at start), so
|
// The provider honored outputSchema (capability-gated at start), so
|
||||||
@@ -421,6 +455,7 @@ export class WorkflowExecution {
|
|||||||
this.observer.agentEnd({ ...info, outcome: 'failed' })
|
this.observer.agentEnd({ ...info, outcome: 'failed' })
|
||||||
return null
|
return null
|
||||||
} finally {
|
} finally {
|
||||||
|
this.controller.signal.removeEventListener('abort', onAbort)
|
||||||
await run.dispose()
|
await run.dispose()
|
||||||
}
|
}
|
||||||
} finally {
|
} finally {
|
||||||
@@ -476,6 +511,7 @@ export class WorkflowExecution {
|
|||||||
|
|
||||||
/** The `parallel(thunks)` hook: each thunk caught → `null`; fatal errors propagate. */
|
/** The `parallel(thunks)` hook: each thunk caught → `null`; fatal errors propagate. */
|
||||||
private async parallel(rawThunks: unknown): Promise<unknown[]> {
|
private async parallel(rawThunks: unknown): Promise<unknown[]> {
|
||||||
|
this.throwIfCancelled()
|
||||||
if (!Array.isArray(rawThunks)) {
|
if (!Array.isArray(rawThunks)) {
|
||||||
throw new WorkflowError('parallel() requires an array of zero-argument functions', 'INVALID_ARGUMENT')
|
throw new WorkflowError('parallel() requires an array of zero-argument functions', 'INVALID_ARGUMENT')
|
||||||
}
|
}
|
||||||
@@ -501,6 +537,7 @@ export class WorkflowExecution {
|
|||||||
|
|
||||||
/** The `pipeline(items, ...stages)` hook: per-item stage chains, NO cross-stage barrier. */
|
/** The `pipeline(items, ...stages)` hook: per-item stage chains, NO cross-stage barrier. */
|
||||||
private async pipeline(rawItems: unknown, rawStages: unknown[]): Promise<unknown[]> {
|
private async pipeline(rawItems: unknown, rawStages: unknown[]): Promise<unknown[]> {
|
||||||
|
this.throwIfCancelled()
|
||||||
if (!Array.isArray(rawItems)) {
|
if (!Array.isArray(rawItems)) {
|
||||||
throw new WorkflowError('pipeline() requires an items array', 'INVALID_ARGUMENT')
|
throw new WorkflowError('pipeline() requires an items array', 'INVALID_ARGUMENT')
|
||||||
}
|
}
|
||||||
@@ -542,6 +579,7 @@ export class WorkflowExecution {
|
|||||||
|
|
||||||
/** The `phase(title)` hook: sets the current label for subsequent `agent()` calls and notifies observers. */
|
/** The `phase(title)` hook: sets the current label for subsequent `agent()` calls and notifies observers. */
|
||||||
private phase(title: unknown): void {
|
private phase(title: unknown): void {
|
||||||
|
this.throwIfCancelled()
|
||||||
if (typeof title !== 'string' || title.length === 0) {
|
if (typeof title !== 'string' || title.length === 0) {
|
||||||
throw new WorkflowError('phase() requires a non-empty title string', 'INVALID_ARGUMENT')
|
throw new WorkflowError('phase() requires a non-empty title string', 'INVALID_ARGUMENT')
|
||||||
}
|
}
|
||||||
@@ -551,6 +589,7 @@ export class WorkflowExecution {
|
|||||||
|
|
||||||
/** The `log(message)` hook: narration to observers. */
|
/** The `log(message)` hook: narration to observers. */
|
||||||
private log(message: unknown): void {
|
private log(message: unknown): void {
|
||||||
|
this.throwIfCancelled()
|
||||||
if (typeof message !== 'string') {
|
if (typeof message !== 'string') {
|
||||||
throw new WorkflowError('log() requires a message string', 'INVALID_ARGUMENT')
|
throw new WorkflowError('log() requires a message string', 'INVALID_ARGUMENT')
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -459,7 +459,7 @@ describe('dsh-workflow-vm', () => {
|
|||||||
expect((result.value as { message: string }).message).toContain('"bogus" is not recognized')
|
expect((result.value as { message: string }).message).toContain('"bogus" is not recognized')
|
||||||
})
|
})
|
||||||
|
|
||||||
it('a non-WorkflowError host failure (a rejecting provider result) reaches the script raw', async () => {
|
it('a rejecting provider result is an infrastructure fault: fatal AGENT_RESULT, agent-end paired, no combinator dissolve', async () => {
|
||||||
const ctx = new Context()
|
const ctx = new Context()
|
||||||
await ctx.plugin(SubagentService)
|
await ctx.plugin(SubagentService)
|
||||||
const provider: SubagentProvider = {
|
const provider: SubagentProvider = {
|
||||||
@@ -475,11 +475,22 @@ describe('dsh-workflow-vm', () => {
|
|||||||
}
|
}
|
||||||
ctx.subagents.registerProvider(provider)
|
ctx.subagents.registerProvider(provider)
|
||||||
await ctx.plugin(VmWorkflowEngine, { provider: 'rejecting' })
|
await ctx.plugin(VmWorkflowEngine, { provider: 'rejecting' })
|
||||||
const result = await run(ctx, fakeParent(), script(`
|
const ends: unknown[] = []
|
||||||
try { await agent('p'); return 'unreachable' } catch (e) { return { name: e.name, message: e.message } }
|
ctx.on('workflow/agent-end', (_info, agent) => { ends.push(agent) })
|
||||||
|
// Direct await: the script reads the typed fields (a host object, so
|
||||||
|
// realm instanceof is false — same as every hook failure).
|
||||||
|
const direct = await run(ctx, fakeParent(), script(`
|
||||||
|
try { await agent('p'); return 'unreachable' } catch (e) { return { name: e.name, code: e.code, fatal: e.fatal, message: e.message } }
|
||||||
`))
|
`))
|
||||||
expect(result.value).toMatchObject({ name: 'Error' })
|
expect(direct.value).toMatchObject({ name: 'WorkflowError', code: 'AGENT_RESULT', fatal: true })
|
||||||
expect((result.value as { message: string }).message).toContain('backend exploded')
|
expect((direct.value as { message: string }).message).toContain('backend exploded')
|
||||||
|
// The child's lifecycle stays paired even though result never resolved.
|
||||||
|
expect(ends).toEqual([expect.objectContaining({ seq: 1, outcome: 'failed' })])
|
||||||
|
// Through a combinator the fault PROPAGATES (fatal) — a broken provider
|
||||||
|
// must not dissolve into the per-item null and read as a failed child.
|
||||||
|
const throughParallel = await run(ctx, fakeParent(), script("return await parallel([() => agent('p')])"))
|
||||||
|
expect(throughParallel.stopReason).toBe('error')
|
||||||
|
expect(throughParallel.error).toContain('backend exploded')
|
||||||
})
|
})
|
||||||
|
|
||||||
it('phase()/log() throw host WorkflowErrors synchronously on misuse', async () => {
|
it('phase()/log() throw host WorkflowErrors synchronously on misuse', async () => {
|
||||||
@@ -542,6 +553,87 @@ describe('dsh-workflow-vm', () => {
|
|||||||
await handle.dispose()
|
await handle.dispose()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('cancellation bridges to run.cancel() on every in-flight child, not just the request signal', async () => {
|
||||||
|
const { ctx, parent, provider } = await setup({ manual: true })
|
||||||
|
const handle = ctx.workflows.start({
|
||||||
|
script: script("return await parallel([() => agent('a'), () => agent('b')])"),
|
||||||
|
parent,
|
||||||
|
})
|
||||||
|
await vi.waitFor(() => { expect(provider.runs.length).toBe(2) })
|
||||||
|
handle.cancel('bridged')
|
||||||
|
expect((await handle.result).stopReason).toBe('cancelled')
|
||||||
|
// The seam leaves a provider free to honor run.cancel() rather than the
|
||||||
|
// request signal, so the engine must drive BOTH channels per child.
|
||||||
|
expect(provider.runs.map(r => r.cancelled)).toEqual(['bridged', 'bridged'])
|
||||||
|
await handle.dispose()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('a provider whose result REJECTS on abort still gets a paired cancelled agent-end, and the run reports cancelled', async () => {
|
||||||
|
const ctx = new Context()
|
||||||
|
await ctx.plugin(SubagentService)
|
||||||
|
// The seam allows result to reject for infrastructure faults; a backend
|
||||||
|
// that tears down uncleanly on abort exercises the rejection path WHILE
|
||||||
|
// the run is cancelled — which must stay a cancellation, not AGENT_RESULT.
|
||||||
|
const provider: SubagentProvider = {
|
||||||
|
name: 'reject-on-abort',
|
||||||
|
capabilities: { outputSchema: true, depthLimit: true, toolFilter: true },
|
||||||
|
inheritsParentContext: false,
|
||||||
|
start: request => ({
|
||||||
|
id: AgentId('crashing-child'),
|
||||||
|
result: new Promise((_, reject) => {
|
||||||
|
request.signal?.addEventListener('abort', () => { reject(new Error('backend crashed on abort')) }, { once: true })
|
||||||
|
}),
|
||||||
|
cancel: () => { /* the signal listener above is the teardown */ },
|
||||||
|
dispose: () => Promise.resolve(),
|
||||||
|
}),
|
||||||
|
}
|
||||||
|
ctx.subagents.registerProvider(provider)
|
||||||
|
await ctx.plugin(VmWorkflowEngine, { provider: 'reject-on-abort' })
|
||||||
|
const starts: unknown[] = []
|
||||||
|
const ends: unknown[] = []
|
||||||
|
ctx.on('workflow/agent-start', (_info, agent) => { starts.push(agent) })
|
||||||
|
ctx.on('workflow/agent-end', (_info, agent) => { ends.push(agent) })
|
||||||
|
const handle = ctx.workflows.start({ script: script("return await agent('doomed')"), parent: fakeParent() })
|
||||||
|
await vi.waitFor(() => { expect(starts.length).toBe(1) })
|
||||||
|
handle.cancel('user aborted')
|
||||||
|
const result = await handle.result
|
||||||
|
expect(result.stopReason).toBe('cancelled')
|
||||||
|
expect(result.error).toContain('user aborted')
|
||||||
|
expect(ends).toEqual([expect.objectContaining({ seq: 1, outcome: 'cancelled' })])
|
||||||
|
await handle.dispose()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('after cancellation EVERY hook throws at entry — phase/log/parallel/pipeline, not just agent()', async () => {
|
||||||
|
const { ctx, parent, provider } = await setup({ manual: true })
|
||||||
|
let cancelled = false
|
||||||
|
const postCancel: string[] = []
|
||||||
|
ctx.on('workflow/phase', (_info, title) => { if (cancelled) postCancel.push(`phase:${title}`) })
|
||||||
|
ctx.on('workflow/log', (_info, message) => { if (cancelled) postCancel.push(`log:${message}`) })
|
||||||
|
const handle = ctx.workflows.start({
|
||||||
|
// The script survives each throw by catching, so every guarded hook is
|
||||||
|
// actually ATTEMPTED after the cancel; the run still reports cancelled.
|
||||||
|
script: script(`
|
||||||
|
phase('before')
|
||||||
|
try { await agent('x') } catch (e) {}
|
||||||
|
try { phase('after') } catch (e) {}
|
||||||
|
try { log('after') } catch (e) {}
|
||||||
|
try { await parallel([() => 'ran']) } catch (e) {}
|
||||||
|
try { await pipeline(['item'], p => p) } catch (e) {}
|
||||||
|
return 'survived by catching'
|
||||||
|
`),
|
||||||
|
parent,
|
||||||
|
})
|
||||||
|
await vi.waitFor(() => { expect(provider.runs.length).toBe(1) })
|
||||||
|
cancelled = true
|
||||||
|
handle.cancel('stop everything')
|
||||||
|
const result = await handle.result
|
||||||
|
expect(result.stopReason).toBe('cancelled')
|
||||||
|
// No post-cancel progress ever reached observers, and no child started.
|
||||||
|
expect(postCancel).toEqual([])
|
||||||
|
expect(provider.runs.length).toBe(1)
|
||||||
|
await handle.dispose()
|
||||||
|
})
|
||||||
|
|
||||||
it('an already-aborted request signal cancels before any child starts', async () => {
|
it('an already-aborted request signal cancels before any child starts', async () => {
|
||||||
const { ctx, parent, provider } = await setup({ manual: true })
|
const { ctx, parent, provider } = await setup({ manual: true })
|
||||||
const controller = new AbortController()
|
const controller = new AbortController()
|
||||||
|
|||||||
@@ -127,6 +127,9 @@ export type WorkflowEventName =
|
|||||||
* subset (see dsh-tools).
|
* subset (see dsh-tools).
|
||||||
* - `AGENT_CAP` / `ITEM_CAP` — the run/agent caps tripped.
|
* - `AGENT_CAP` / `ITEM_CAP` — the run/agent caps tripped.
|
||||||
* - `AGENT_START` — the subagent seam refused to start a child.
|
* - `AGENT_START` — the subagent seam refused to start a child.
|
||||||
|
* - `AGENT_RESULT` — a child's `result` REJECTED: an infrastructure fault at
|
||||||
|
* the subagent seam, distinct from a child that failed and resolved (which
|
||||||
|
* is the per-item `null`, never an error).
|
||||||
* - `RESULT_UNSERIALIZABLE` — a value crossing the script/host value boundary
|
* - `RESULT_UNSERIALIZABLE` — a value crossing the script/host value boundary
|
||||||
* is not plain JSON data.
|
* is not plain JSON data.
|
||||||
* - `CANCELLED` — the run was cancelled; pending and future hooks reject
|
* - `CANCELLED` — the run was cancelled; pending and future hooks reject
|
||||||
@@ -141,6 +144,7 @@ export type WorkflowErrorCode =
|
|||||||
| 'AGENT_CAP'
|
| 'AGENT_CAP'
|
||||||
| 'ITEM_CAP'
|
| 'ITEM_CAP'
|
||||||
| 'AGENT_START'
|
| 'AGENT_START'
|
||||||
|
| 'AGENT_RESULT'
|
||||||
| 'RESULT_UNSERIALIZABLE'
|
| 'RESULT_UNSERIALIZABLE'
|
||||||
| 'CANCELLED'
|
| 'CANCELLED'
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user