From 4323796297d8e142d8713dcc586e7cdabb09d8cc Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Wed, 14 Oct 2026 15:44:50 +0800 Subject: [PATCH 1/2] fix: harden workflow lifecycle, concurrency, and retries with structured errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supersedes #247 as a single change. It keeps that PR's lifecycle, concurrency, and error-propagation fixes, and replaces the parts that inferred behavior from text. Structured provider errors (no text matching for known failures) - Add AssistantMessage.errorKind: rate_limit | quota | auth | transient | invalid_request. Providers set it in their catch blocks from the HTTP status, the provider's machine-readable error type/code (insufficient_quota, GoUsageLimitError, rate_limit_exceeded, overloaded_error, ...), and transport error codes (APIConnectionError, ECONNRESET, ...). Mid-stream error events (Anthropic SSE `error`, OpenAI Responses `error`/`response.failed`) now throw ProviderStreamError carrying the code, and ProviderRetryDelayError keeps the original 429 as its cause, so both stay classifiable. - The agent loop classifies errors thrown by a provider stream and marks a stream that closes without a terminal event as transient. - isRetryableAssistantError trusts errorKind when present; the old message patterns remain only for failures that arrive with no status or code. - Session retries pick the separate 429 budget, and workflow congestion control throttles the admission window, only for errorKind === "rate_limit". A 400 that mentions "429" is no longer a rate limit, and a 429 whose code reports exhausted quota is terminal. The regex-based isRateLimitError and isTerminalQuotaError are removed. Subagent children forward the kind in their messages; workflow runners report it as WorkflowAgentRunResult.errorKind. Intent is the model's call, not a regex - Do not add #247's counted fan-out detector (COUNTED_AGENT_LAUNCH, REFUSAL, META_QUESTION); ultraloop-opt-in.ts stays as on main. The detector missed common requests ("开10个子agent分别审查" hit the "别" in 分别, "…看看怎么回事" hit the "怎么" question filter, and verbs like 用/让 were not listed) while trying to rule out negations and questions with word lists. Only the explicit ultraloop/ultracode keyword, the fixed opt-in phrases, and /ultracode session mode are recognized by the harness; the model judges everything else from the existing tool descriptions. No intent guidance is added to the system prompt. One admission window for subagents and workflows - Parallel subagent batches take slots from the same process-wide admission window as workflow nodes, and feed its AIMD control with their model-request events. Running both at once can no longer exceed the host ceiling, and a 429 seen by either lowers the window for both. - Parallel subagent batches also honor STEP_WORKFLOW_CONCURRENCY_LIMIT, so the configured provider limit applies without a workflow running, and an admitted child holds its slot until its process exits (awaitExit), so a child still shutting down or cancelling cannot overlap queued work. Content-addressed workflow resume - resumeFromRunId replays any completed call whose prompt+options hash matches, independent of its sequence number. A failed, inserted, or reordered call (pipeline stages finish in nondeterministic order) now re-runs only itself instead of invalidating every later call. Identical calls are consumed in their original order, one cached result each. Nothing after a malformed journal line is trusted. - A replayed call is journaled under the resumed run's own seq, with the schema-cleaned value the script received, so sequence numbers stay unique and chained resumes see what was consumed. - A cached result must still satisfy the call's JSON schema, otherwise the call runs live. Calls with a runtime (safeParse) schema never replay: such schemas hash to one placeholder, so two different ones are indistinguishable. - The cache closes on the first write, as ZCode's amend-resume does. The default runner forwards each child tool_execution_start (emitted before the tool's permission check and execution) and reports toolActivityObserved only when the child exited normally, since a killed child can lose events. classifyWorkflowToolEffect maps tools to none (clarify_user, find_tools), read (file reads/searches, search_web), or mutate (file writes, shell commands, and any unrecognized tool such as MCP). Completed entries record worldAccess. Once a live call starts a mutating tool, or a live call cannot vouch for its tool activity, only cached entries with worldAccess === false still replay; entries from older journals count as having touched the environment. Calls issued together before any live write, such as a resumed fan-out where a few nodes had failed, still replay in full. - Resume wording in the workflow tool description, its guidance, and the system prompt's workflow section now describes this behavior. Kept from #247 - Workflow runtime: cancel and drain running and queued children before publishing a terminal state, seal terminal states, and abort the run scope on exit. Clamp requested concurrency to a CPU-derived ceiling (optionally lowered by STEP_WORKFLOW_CONCURRENCY_LIMIT) and report clamping as a notice. Share a process-wide admission window with a 25% cut per congestion epoch, recovery after four successes, a Retry-After cooldown capped at 30s, and 60s retention of a throttled window between back-to-back workflows. - Record parallel()/pipeline() callback failures as taskFailures (including nested workflows), accept promise arrays in parallel(), keep node failures out of the callback channel and count them once, and fail runs that return with unawaited operations. Report partial_failed/failed with the failed nodes in the final tool result. - Node timeouts cover execution only (not queue time) and come from our own timer, not a substring match. Tokens a cancelled or timed-out attempt reports while settling are still charged. A runner that ignores cancellation keeps its shared slot until it settles. - Failure recording, progress.json writes, and live-update listeners are best effort and cannot replace a node's real error. A failed progress write no longer poisons the write queue. - Subagents: accept batches above the concurrency window (up to 1000) and queue the excess, wait for workflow children to exit before releasing their slot, mark tool results with failed or aborted children as isError, and report rate-limit retries and request start/success over RPC. - Session retries: an independent 429 budget (retry.rateLimitMaxRetries) that honors Retry-After, falls back to jittered backoff (1s to 30s), and can be cancelled. - Agent loop: honor AgentToolResult.isError. Finalize a partial assistant message when a provider stream throws, ends early, or fails during construction, without reclassifying consumer/persistence errors or emitting a second terminal message. - Providers: preserve Retry-After when the delay cap rejects a wait, and treat an empty or non-numeric retry-after-ms as absent. --- CHANGELOG.md | 6 + packages/agent-core/src/agent-loop.ts | 129 +- packages/agent-core/src/types.ts | 2 + packages/agent-core/test/agent-loop.test.ts | 195 ++- packages/coding-agent/docs/settings.md | 21 + .../coding-agent/docs/workflow-experiments.md | 50 + .../coding-agent/src/core/agent-session.ts | 46 +- packages/coding-agent/src/core/rate-limit.ts | 20 + .../coding-agent/src/core/settings-manager.ts | 8 +- .../src/features/step-subagent.ts | 65 +- .../src/features/subagent/execute.ts | 69 +- .../src/features/subagent/rpc-adapter.ts | 46 +- .../src/features/workflow/admission.ts | 56 + .../src/features/workflow/agent-runner.ts | 26 +- .../src/features/workflow/budget.ts | 111 +- .../src/features/workflow/journal.ts | 47 +- .../src/features/workflow/progress.ts | 6 +- .../src/features/workflow/rendering.ts | 60 +- .../src/features/workflow/runtime.ts | 500 ++++++- .../src/features/workflow/schema.ts | 2 +- .../src/features/workflow/step-workflow.ts | 11 +- .../src/features/workflow/tool-profile.ts | 14 + .../src/features/workflow/types.ts | 40 +- .../coding-agent/src/features/workflow/vm.ts | 55 +- .../coding-agent/src/step/system-prompt.ts | 2 +- packages/coding-agent/src/utils/sleep.ts | 24 +- .../test/agent-session-retry.test.ts | 101 +- packages/coding-agent/test/rate-limit.test.ts | 28 + .../test/step-subagent-events.test.ts | 73 +- .../test/subagent-chain-result.test.ts | 87 +- .../test/subagent-exit-drain.test.ts | 106 ++ .../test/workflow-agent-runner.test.ts | 63 + .../test/workflow-extension.test.ts | 53 + .../test/workflow-runtime.test.ts | 1144 ++++++++++++++++- .../test/workflow-ultraloop-opt-in.test.ts | 13 + .../coding-agent/test/workflow-vm.test.ts | 43 +- .../providers/src/api/anthropic-messages.ts | 8 +- .../providers/src/api/openai-completions.ts | 6 +- .../src/api/openai-responses-shared.ts | 5 +- .../providers/src/api/openai-responses.ts | 6 +- packages/providers/src/index.ts | 1 + packages/providers/src/types.ts | 8 + packages/providers/src/utils/error-kind.ts | 147 +++ packages/providers/src/utils/event-stream.ts | 2 +- .../providers/src/utils/provider-retry.ts | 47 +- packages/providers/src/utils/retry.ts | 10 +- packages/providers/test/error-kind.test.ts | 74 ++ .../test/openai-completions-retry.test.ts | 12 + .../providers/test/provider-retry.test.ts | 34 +- packages/providers/test/retry.test.ts | 14 + 50 files changed, 3484 insertions(+), 212 deletions(-) create mode 100644 packages/coding-agent/docs/workflow-experiments.md create mode 100644 packages/coding-agent/src/core/rate-limit.ts create mode 100644 packages/coding-agent/src/features/workflow/admission.ts create mode 100644 packages/coding-agent/test/rate-limit.test.ts create mode 100644 packages/coding-agent/test/subagent-exit-drain.test.ts create mode 100644 packages/coding-agent/test/workflow-agent-runner.test.ts create mode 100644 packages/providers/src/utils/error-kind.ts create mode 100644 packages/providers/test/error-kind.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index c106e0d1..cd6cfce4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,13 @@ ### Fixed - Stop flooding startup with `[Skill conflicts]` when the same skills are installed in both `~/.agents/skills` and `~/.claude/skills`. Byte-identical copies are now skipped silently, and remaining name collisions collapse to one summary line per directory pair; `--verbose` still lists each one. +- Cancel and drain workflow children before publishing a terminal state, share host admission across concurrent workflows, accept promise arrays in `parallel()`, and distinguish concurrency notices from actual script failures. +- Clamp workflow concurrency to the host ceiling, back off on temporary 429s with an independent retry budget and adaptive admission cap, and report failed nodes in final tool results instead of silently treating partial workflows as successful. - Persist Ultracode on/off state in the current session journal so reopening that session restores its mode without changing global or project settings. - Stop appending Ultracode capability tutorials on every turn. Announce session-mode transitions after user input, reuse unchanged state, and restore necessary reminders when context is compacted or replaced. - Recognize `/exit` as an alias for `/quit`, including while the agent is running or compacting. - Recognize `search_files`, `find_files`, and `list_directory` when selecting default system-prompt file-exploration guidance. +- Report callback errors (including nested workflow errors raised before any agent starts), and queue ordinary subagent batches beyond their concurrency window in the same process-wide admission window as workflow nodes. Subagent batches also honor `STEP_WORKFLOW_CONCURRENCY_LIMIT` and hold each slot until the child process exits. +- Let a tool return `isError: true` while keeping its partial output and details, so failed subagent and workflow results reach the model as errors. Finalize a partial assistant message when a provider stream throws, ends without a terminal event, or fails during construction, instead of leaving the turn unfinished. +- Classify provider failures structurally (HTTP status, provider error code, transport error) as rate limit, quota, auth, transient, or invalid request. Session retries, the separate 429 retry budget, and workflow congestion control use this classification instead of matching error text, so a 400 that mentions "429" is no longer retried and an exhausted quota returned as 429 is not treated as throttling. +- Resume workflows by matching each call's prompt and options instead of the journal's sequence prefix, so a failed or reordered node no longer forces every later call to re-run. Once a live call writes files, runs a shell command, or uses an unrecognized tool, cached calls that read the environment re-run as well; cached results are re-validated against the call's schema, and calls with a runtime (safeParse) schema always run live. diff --git a/packages/agent-core/src/agent-loop.ts b/packages/agent-core/src/agent-loop.ts index c25600d7..ef7ae859 100644 --- a/packages/agent-core/src/agent-loop.ts +++ b/packages/agent-core/src/agent-loop.ts @@ -5,7 +5,9 @@ import { type AssistantMessage, + type AssistantMessageEvent, type Context, + classifyProviderError, EventStream, type ToolResultMessage, validateToolArguments, @@ -47,9 +49,10 @@ export function agentLoop( }, signal, streamFn, - ).then((messages) => { - stream.end(messages); - }); + ).then( + (messages) => stream.end(messages), + (error) => stream.fail(error), + ); return stream; } @@ -86,9 +89,10 @@ export function agentLoopContinue( }, signal, streamFn, - ).then((messages) => { - stream.end(messages); - }); + ).then( + (messages) => stream.end(messages), + (error) => stream.fail(error), + ); return stream; } @@ -143,8 +147,27 @@ export async function runAgentLoopContinue( return newMessages; } -function createAgentStream(): EventStream { - return new EventStream( +class AgentLoopStream extends EventStream { + private failed = false; + private failure: unknown; + fail(error: unknown): void { + this.failed = true; + this.failure = error; + this.end([]); + } + override async *[Symbol.asyncIterator](): AsyncGenerator { + yield* super[Symbol.asyncIterator](); + if (this.failed) throw this.failure; + } + override async result(): Promise { + const result = await super.result(); + if (this.failed) throw this.failure; + return result; + } +} + +function createAgentStream(): AgentLoopStream { + return new AgentLoopStream( (event: AgentEvent) => event.type === "agent_end", (event: AgentEvent) => (event.type === "agent_end" ? event.messages : []), ); @@ -338,19 +361,21 @@ async function streamAssistantResponse( tools: context.tools, }; - // Resolve API key (important for expiring tokens) - const resolvedApiKey = - (config.getApiKey ? await config.getApiKey(config.model.provider) : undefined) || config.apiKey; - - const response = await streamFunction(config.model, llmContext, { - ...config, - apiKey: resolvedApiKey, + const response = normalizeProviderStream( + async () => { + // Auth refresh and stream construction can throw before the first event. + const resolvedApiKey = + (config.getApiKey ? await config.getApiKey(config.model.provider) : undefined) || config.apiKey; + return streamFunction(config.model, llmContext, { ...config, apiKey: resolvedApiKey, signal }); + }, + config.model, signal, - }); + ); let partialMessage: AssistantMessage | null = null; let addedPartial = false; - + // Consumer/persistence errors must escape. Only the producer normalizes + // provider exceptions, so a failed message_end cannot emit a second terminal. for await (const event of response) { switch (event.type) { case "start": @@ -382,7 +407,10 @@ async function streamAssistantResponse( case "done": case "error": { - const finalMessage = await response.result(); + // The terminal event already carries the final message. Do not wait on + // result() here: a provider that closes or rejects immediately after + // emitting the terminal event may leave that promise unresolved. + const finalMessage = event.type === "done" ? event.message : event.error; if (addedPartial) { context.messages[context.messages.length - 1] = finalMessage; } else { @@ -396,16 +424,63 @@ async function streamAssistantResponse( } } } + throw new Error("Provider stream ended without a terminal event"); +} - const finalMessage = await response.result(); - if (addedPartial) { - context.messages[context.messages.length - 1] = finalMessage; - } else { - context.messages.push(finalMessage); - await emit({ type: "message_start", message: { ...finalMessage } }); +async function* normalizeProviderStream( + createStream: () => ReturnType, + model: AgentLoopConfig["model"], + signal: AbortSignal | undefined, +): AsyncGenerator { + let partial: AssistantMessage | null = null; + let terminal = false; + let truncated = false; + try { + const response = await createStream(); + for await (const event of response) { + if ("partial" in event) partial = event.partial; + terminal = event.type === "done" || event.type === "error"; + yield event; + if (terminal) return; + } + truncated = true; + throw new Error("The model response ended before completion"); + } catch (error) { + if (terminal) throw error; + const message = interruptedResponse(partial, model, signal, error); + // A stream that closed without a terminal event is a dropped connection. + const errorKind = signal?.aborted ? undefined : truncated ? "transient" : classifyProviderError(error); + if (errorKind) message.errorKind = errorKind; + yield { type: "error", reason: signal?.aborted ? "aborted" : "error", error: message }; } - await emit({ type: "message_end", message: finalMessage }); - return finalMessage; +} + +function interruptedResponse( + partialMessage: AssistantMessage | null, + model: AgentLoopConfig["model"], + signal: AbortSignal | undefined, + error: unknown, +): AssistantMessage { + return { + ...(partialMessage ?? { + role: "assistant", + content: [], + api: model.api, + provider: model.provider, + model: model.id, + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + timestamp: Date.now(), + }), + stopReason: signal?.aborted ? "aborted" : "error", + errorMessage: error instanceof Error ? error.message : String(error), + }; } /** @@ -736,7 +811,7 @@ async function executePreparedToolCall( ); acceptingUpdates = false; await Promise.all(updateEvents); - return { result, isError: false }; + return { result, isError: result.isError === true }; } catch (error) { acceptingUpdates = false; await Promise.all(updateEvents); diff --git a/packages/agent-core/src/types.ts b/packages/agent-core/src/types.ts index 85947b11..a4acd33d 100644 --- a/packages/agent-core/src/types.ts +++ b/packages/agent-core/src/types.ts @@ -383,6 +383,8 @@ export interface AgentState { /** Final or partial result produced by a tool. */ export interface AgentToolResult { + /** A failed tool result may retain successful partial output and structured details. */ + isError?: boolean; /** Text or image content returned to the model. */ content: (TextContent | ImageContent)[]; /** Arbitrary structured details for logs or UI rendering. */ diff --git a/packages/agent-core/test/agent-loop.test.ts b/packages/agent-core/test/agent-loop.test.ts index 79957ef1..39630f65 100644 --- a/packages/agent-core/test/agent-loop.test.ts +++ b/packages/agent-core/test/agent-loop.test.ts @@ -1,6 +1,7 @@ import { type AssistantMessage, type AssistantMessageEvent, + type AssistantMessageEventStream, EventStream, type Message, type Model, @@ -8,7 +9,7 @@ import { } from "@step-harness/providers"; import { Type } from "typebox"; import { describe, expect, it } from "vitest"; -import { agentLoop, agentLoopContinue } from "../src/agent-loop.ts"; +import { agentLoop, agentLoopContinue, runAgentLoop } from "../src/agent-loop.ts"; import { setDefaultStreamFn } from "../src/index.ts"; import type { AgentContext, AgentEvent, AgentLoopConfig, AgentMessage, AgentTool } from "../src/types.ts"; @@ -163,6 +164,145 @@ describe("agentLoop with AgentMessage", () => { expect(eventTypes).toContain("agent_end"); }); + it("finalizes a partial assistant when the provider stream throws", async () => { + const partial = createAssistantMessage([{ type: "text", text: "partial response" }], "pending"); + const streamFn = () => + ({ + async *[Symbol.asyncIterator]() { + yield { type: "start", partial } satisfies AssistantMessageEvent; + throw new Error("connection terminated"); + }, + result: () => new Promise(() => {}), + }) as unknown as AssistantMessageEventStream; + + const events: AgentEvent[] = []; + const stream = agentLoop( + [createUserMessage("Hello")], + { systemPrompt: "", messages: [], tools: [] }, + { model: createModel(), convertToLlm: identityConverter }, + undefined, + streamFn, + ); + for await (const event of stream) events.push(event); + + const assistantEnd = events.find((event) => event.type === "message_end" && event.message.role === "assistant"); + expect(assistantEnd).toMatchObject({ + type: "message_end", + message: { role: "assistant", stopReason: "error", errorMessage: "connection terminated" }, + }); + expect(events.at(-1)?.type).toBe("agent_end"); + }); + + it.each([ + ["a 429 thrown by the provider", Object.assign(new Error("slow down"), { status: 429 }), "rate_limit"], + ["a stream that closes without a terminal event", undefined, "transient"], + ["an error with no structured signal", new Error("429 in the text only"), undefined], + ])("classifies %s structurally", async (_name, thrown, errorKind) => { + const streamFn = () => + ({ + async *[Symbol.asyncIterator](): AsyncGenerator { + if (thrown) throw thrown; + yield* []; + }, + result: () => new Promise(() => {}), + }) as unknown as AssistantMessageEventStream; + const stream = agentLoop( + [createUserMessage("Hello")], + { systemPrompt: "", messages: [], tools: [] }, + { model: createModel(), convertToLlm: identityConverter }, + undefined, + streamFn, + ); + for await (const _event of stream) { + /* consume */ + } + const last = (await stream.result()).at(-1) as AssistantMessage; + expect(last.stopReason).toBe("error"); + expect(last.errorKind).toBe(errorKind); + }); + + it("finalizes synchronous provider construction failures", async () => { + const stream = agentLoop( + [createUserMessage("Hello")], + { systemPrompt: "", messages: [], tools: [] }, + { model: createModel(), convertToLlm: identityConverter }, + undefined, + () => { + throw new Error("provider construction failed"); + }, + ); + const events: AgentEvent[] = []; + for await (const event of stream) events.push(event); + expect( + events.filter((event) => event.type === "message_end" && event.message.role === "assistant"), + ).toMatchObject([{ message: { stopReason: "error", errorMessage: "provider construction failed" } }]); + expect(events.at(-1)?.type).toBe("agent_end"); + expect((await stream.result()).at(-1)).toMatchObject({ stopReason: "error" }); + }); + + it.each(["new", "continue"])( + "%s loop surfaces conversion failures instead of leaving consumers hanging", + async (mode) => { + const failure = new Error("conversion failed"); + const context = { systemPrompt: "", messages: [createUserMessage("Hello")], tools: [] }; + const config: AgentLoopConfig = { + model: createModel(), + convertToLlm: () => { + throw failure; + }, + }; + const streamFn = () => new MockAssistantStream(); + const stream = + mode === "new" + ? agentLoop([], context, config, undefined, streamFn) + : agentLoopContinue(context, config, undefined, streamFn); + await expect( + (async () => { + for await (const _event of stream) { + /* consume */ + } + })(), + ).rejects.toBe(failure); + await expect(stream.result()).rejects.toBe(failure); + }, + ); + + it.each(["message_update", "message_end"])( + "does not turn %s listener failures into provider errors", + async (failAt) => { + const failure = new Error("listener failed: 503"); + const partial = createAssistantMessage([{ type: "text", text: "partial" }], "pending"); + const final = createAssistantMessage([{ type: "text", text: "complete final content" }]); + const events: AgentEvent[] = []; + await expect( + runAgentLoop( + [createUserMessage("Hello")], + { systemPrompt: "", messages: [], tools: [] }, + { model: createModel(), convertToLlm: identityConverter }, + (event) => { + events.push(event); + if ( + (event.type === "message_update" && failAt === "message_update") || + (event.type === "message_end" && failAt === "message_end" && event.message.role === "assistant") + ) + throw failure; + }, + undefined, + () => { + const response = new MockAssistantStream(); + response.push({ type: "start", partial }); + response.push({ type: "text_delta", contentIndex: 0, delta: "partial", partial }); + response.push({ type: "done", reason: "stop", message: final }); + return response; + }, + ), + ).rejects.toBe(failure); + const ends = events.filter((event) => event.type === "message_end" && event.message.role === "assistant"); + expect(ends).toHaveLength(failAt === "message_end" ? 1 : 0); + if (ends.length) expect(ends[0]).toMatchObject({ message: final }); + }, + ); + it("should handle custom message types via convertToLlm", async () => { // Create a custom message type interface CustomNotification { @@ -368,6 +508,59 @@ describe("agentLoop with AgentMessage", () => { expect(toolResult?.role === "toolResult" ? toolResult.usage : undefined).toEqual(patchedToolUsage); }); + it("preserves explicit tool failure details in events and the next model context", async () => { + const details = { status: "partial_failed", completed: 12, failed: 10 }; + const tool: AgentTool = { + name: "workflow", + label: "Workflow", + description: "Run nodes", + parameters: Type.Object({}), + execute: async () => ({ isError: true, details, content: [{ type: "text", text: "10 nodes failed: 429" }] }), + }; + let calls = 0; + const stream = agentLoop( + [createUserMessage("run")], + { systemPrompt: "", messages: [], tools: [tool] }, + { model: createModel(), convertToLlm: identityConverter }, + undefined, + (_model, context) => { + const response = new MockAssistantStream(); + if (calls++ === 0) { + response.push({ + type: "done", + reason: "toolUse", + message: createAssistantMessage( + [{ type: "toolCall", id: "wf", name: "workflow", arguments: {} }], + "toolUse", + ), + }); + } else { + expect(context.messages.at(-1)).toMatchObject({ + role: "toolResult", + isError: true, + content: [{ text: "10 nodes failed: 429" }], + }); + response.push({ + type: "done", + reason: "stop", + message: createAssistantMessage([{ type: "text", text: "partial failure" }]), + }); + } + return response; + }, + ); + const events: AgentEvent[] = []; + for await (const event of stream) events.push(event); + expect(events.find((event) => event.type === "tool_execution_end")).toMatchObject({ + isError: true, + result: { details }, + }); + expect((await stream.result()).find((message) => message.role === "toolResult")).toMatchObject({ + isError: true, + details, + }); + }); + it("should not execute tool calls from a length-truncated assistant message", async () => { const toolSchema = Type.Object({ value: Type.String() }); const executed: string[] = []; diff --git a/packages/coding-agent/docs/settings.md b/packages/coding-agent/docs/settings.md index 2d10d027..33c40c74 100644 --- a/packages/coding-agent/docs/settings.md +++ b/packages/coding-agent/docs/settings.md @@ -137,6 +137,7 @@ For VS Code, include `--wait` so step resumes after the editor exits: |---------|------|---------|-------------| | `retry.enabled` | boolean | `true` | Enable automatic agent-level retry on transient errors | | `retry.maxRetries` | number | `3` | Maximum agent-level retry attempts | +| `retry.rateLimitMaxRetries` | number | `3` | Independent 429 retry budget per model request (0 disables, maximum 20); does not consume schema or other transient retries | | `retry.baseDelayMs` | number | `2000` | Base delay for agent-level exponential backoff (2s, 4s, 8s) | | `retry.provider.timeoutMs` | number | SDK default | Provider/SDK request timeout in milliseconds | | `retry.provider.maxRetries` | number | `0` | Provider/SDK retry attempts | @@ -146,6 +147,26 @@ When a provider requests a retry delay longer than `retry.provider.maxRetryDelay Keep `retry.provider.maxRetries` at `0` unless provider-level retries are explicitly needed. Setting it above `0` can make SDK/provider retries handle out-of-usage-limit errors before Step sees them, which may block the agent until the provider quota resets in some circumstances. +Temporary 429/concurrency errors retry the current model request, preserving completed tool calls. They honor `Retry-After` when available; otherwise delays grow from 1 second to at most 30 seconds with jitter. Cancellation interrupts the wait. Explicit billing/quota exhaustion is not retried. `retry.enabled: false` disables both session retry budgets. + +### Workflow concurrency and failures + +The harness does not pattern-match requests such as `开10个子agent` / `launch 10 subagents`; the model decides whether they call for the `subagent` tool's parallel mode or a workflow, and whether the message is a request at all. The `subagent` tool accepts batches larger than its concurrent window (up to 1000 by default) and queues the excess. Its children and workflow nodes take slots from the same process-wide admission window, so running both at once cannot exceed the host ceiling, and a 429 seen by either lowers it for both. + +The workflow ceiling is `max(1, min(16, CPU count - 2))`. Model-authored `maxConcurrency` can only lower this ceiling. Set `STEP_WORKFLOW_CONCURRENCY_LIMIT=8` to impose a lower trusted provider limit (for example, a backend allowing eight simultaneous requests). The effective ceiling is the minimum of the machine and configured limits; the final result reports requested, effective, and current concurrency. + +Rate limits are recognized from the provider's structured classification (HTTP 429 or a rate-limit error code), never from text: error messages, agent output, and node labels that mention "429" or "rate limit" are not treated as congestion, and a 429 whose error code reports exhausted quota or billing (`insufficient_quota`, `GoUsageLimitError`, HTTP 402) is terminal. Temporary 429s reduce the admission cap by 25% (minimum one), once per congestion epoch. New nodes wait during backoff; running nodes keep their slots and retry their current request. Four successful responses allow a one-slot recovery when work is queued. Concurrent workflows share a process-wide host window, in addition to each run's lower limit. This conservatively covers all model overrides; it is not a cross-process/account-wide quota lock. + +A node's own retry honors the server's `Retry-After` verbatim, however large, including when the provider's own `maxRetryDelayMs` cap rejected the wait. The admission cooldown that follows a 429 is capped at 30 seconds, because that window is process-wide and its queue time is excluded from node timeouts: an unbounded server delay would otherwise stall unrelated workflows with no error and no timeout. The cap reduction above, not the cooldown length, is what protects the provider. + +`parallel()` accepts both lazy functions (`parallel(items.map(x => () => agent(String(x))))`) and eager promises (`parallel(items.map(x => agent(String(x))))`). Always await or return the operation. Invalid entries reject the batch. A script error or an early return with unfinished calls cancels running and queued nodes and waits for cleanup before publishing the terminal state; production child processes retain their shared slots until they exit. The UI shows concurrency clamping as a Notice, and script failure/cancellation separately. + +Node timeouts cover model requests, tool execution, and retries **after admission**, not time waiting in either queue. Tokens a node reports after it is cancelled or timed out (within the cleanup grace) are still charged to the budget. The deadline is armed per schema attempt, so a node retried for schema mismatches can take up to three times `agentTimeoutMs`. For experiments that report sleep durations or random choices, generate, capture and print all required values in one command, then require structured output with `agent(..., {schema})`. An empty sleep output is not evidence of the chosen duration. See [Workflow experiments](workflow-experiments.md). + +Failed nodes are retained in the journal and final `failures` list even if the script catches errors or filters `parallel()`'s null results. Final status is `partial_failed` for mixed outcomes and `failed` when all nodes fail; both are error tool results with successful output preserved. `summary.completed` counts successful/cached nodes, not attempted calls. Request retry counts (`rateLimitRetries`) are separate from schema `attempt`. + +A callback that catches an agent-node failure and rethrows a **new** error is counted twice, as one failed node and one callback failure, because the node-failure marker travels with the original error object. Rethrow the caught error itself to keep a single count. The shared admission window covers concurrent workflows. When the last one finishes, a window that was never throttled is discarded; a window that was lowered or is cooling down after a 429 is remembered for 60 seconds, so back-to-back workflows do not restart at the full ceiling straight after a 429. After that it starts fresh. + ```json { "retry": { diff --git a/packages/coding-agent/docs/workflow-experiments.md b/packages/coding-agent/docs/workflow-experiments.md new file mode 100644 index 00000000..5ba37e35 --- /dev/null +++ b/packages/coding-agent/docs/workflow-experiments.md @@ -0,0 +1,50 @@ +# Reproducible workflow experiments + +## Preserve shell variables inside JavaScript + +Shell commands are data, not JavaScript expressions. Prefer an ordinary JS string: + +```js +const command = 'd=$((RANDOM % 10 + 1)); sleep $d; echo "slept=${d}s random=$RANDOM"'; +return await parallel(Array.from({length: 10}, (_, i) => () => + agent('Run this command and report only its actual output: ' + command, {label: 'worker-' + (i + 1)}) +)); +``` + +Putting that command unchanged in a backtick JS template evaluates `${d}` in JavaScript, where `d` does not exist. Escape the dollar sign (`\${d}`) if a template is necessary. The runtime does not rewrite interpolation because `${...}` can also be an intentional JS expression. + +Errors in `parallel()` callbacks or `pipeline()` stages are included in `taskFailures`, even when they occur before any `agent()` call, including inside nested workflows. `summary.failedCallbacks` counts those errors separately from failed agent nodes; node failures propagated through the provided helpers are not counted again as callback errors. A run with callback failures cannot report `completed` merely because its agent-call count is zero. Unawaited nested workflow/iterate calls also fail the owning run and are cancelled during cleanup. + +## Random-sleep experiment + +To test a 15-node fan-out, request `maxConcurrency: 15` and leave enough execution time for model requests as well as the tools (for example `agentTimeoutMs: 180000`). A ten-core machine clamps the request to eight slots; excess nodes queue normally. Concurrent workflows share the host window. + +Prefer lazy task functions, though promises are also supported: + +```js +const ids = Array.from({ length: 15 }, (_, i) => i + 1); +return await parallel(ids.map(id => () => { + const command = `node -e 'const {randomInt}=require("node:crypto"); const seconds=randomInt(1,21); const start=Date.now(); setTimeout(()=>console.log(JSON.stringify({id:${id},sleptSeconds:seconds,randomNumber:randomInt(1,1001),elapsedMs:Date.now()-start})),seconds*1000)'`; + return agent( + `Run exactly this one foreground command (timeout_ms: 30000):\n${command}\nReturn only its JSON output. Do not guess or change any values.`, + { + label: `sleep-${id}`, + schema: { + type: "object", + properties: { + id: { type: "integer", const: id }, + sleptSeconds: { type: "integer", minimum: 1, maximum: 20 }, + randomNumber: { type: "integer", minimum: 1, maximum: 1000 }, + elapsedMs: { type: "number", minimum: 1000 }, + }, + required: ["id", "sleptSeconds", "randomNumber", "elapsedMs"], + additionalProperties: false, + }, + }, + ); +})); +``` + +Verification must compare each returned row with its child session's actual tool output. Schema validation proves shape and range, not that a model copied the observed values honestly. Check elapsed time against the selected seconds, and inspect the workflow's `summary`, `failures`, node retry counts, and final status. Aggregate results must come from those verified rows. + +For cancellation tests, start running and queued nodes and then throw a deliberate script error. The terminal snapshot must have no running/queued nodes, the child processes must be gone, and neither progress nor journal may change after the tool returns. A subsequent workflow must not overlap abandoned work. diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index 2b16828d..850d41c6 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -113,6 +113,7 @@ import { type BashExecutionMessage, type CustomMessage, convertToLlm } from "./m import { ModelRegistry } from "./model-registry.ts"; import type { ModelRuntime } from "./model-runtime.ts"; import { expandPromptTemplate, type PromptTemplate } from "./prompt-templates.ts"; +import { isRateLimitError, rateLimitDelay } from "./rate-limit.ts"; import type { ResourceExtensionPaths, ResourceLoader } from "./resource-loader.ts"; import { exportSessionToJsonl } from "./session-export.ts"; import type { BranchSummaryEntry, CompactionEntry, SessionEntry, SessionManager } from "./session-manager.ts"; @@ -158,7 +159,14 @@ export type AgentSessionEvent = willRetry: boolean; errorMessage?: string; } - | { type: "auto_retry_start"; attempt: number; maxAttempts: number; delayMs: number; errorMessage: string } + | { + type: "auto_retry_start"; + attempt: number; + maxAttempts: number; + delayMs: number; + errorMessage: string; + reason?: "rate_limit"; + } | { type: "auto_retry_end"; success: boolean; attempt: number; finalError?: string } | { type: "summarization_retry_scheduled"; @@ -365,6 +373,7 @@ export class AgentSession { // Retry state private _retryAbortController: AbortController | undefined = undefined; private _retryAttempt = 0; + private _rateLimitRetryAttempt = 0; // Bash execution state private readonly _bashAbortControllers = new Set(); @@ -1176,6 +1185,7 @@ export class AgentSession { attempt: this._retryAttempt, }); this._retryAttempt = 0; + this._rateLimitRetryAttempt = 0; } } } @@ -1197,14 +1207,20 @@ export class AgentSession { private _willRetryAfterAgentEnd(event: Extract): boolean { const settings = this.settingsManager.getRetrySettings(); - if (!settings.enabled || this._retryAttempt >= settings.maxRetries) { + if (!settings.enabled) { return false; } for (let i = event.messages.length - 1; i >= 0; i--) { const message = event.messages[i]; if (message.role === "assistant") { - return this._isRetryableError(message as AssistantMessage); + const rateLimited = isRateLimitError(message as AssistantMessage); + return ( + this._isRetryableError(message as AssistantMessage) && + (rateLimited + ? this._rateLimitRetryAttempt < settings.rateLimitMaxRetries + : this._retryAttempt - this._rateLimitRetryAttempt < settings.maxRetries) + ); } } return false; @@ -1681,6 +1697,7 @@ export class AgentSession { // all of them and would leave a stale count for the next prompt to read. // Cleared last so agent_settled listeners still see the final count. this._retryAttempt = 0; + this._rateLimitRetryAttempt = 0; } } @@ -1705,6 +1722,7 @@ export class AgentSession { finalError: msg.errorMessage, }); this._retryAttempt = 0; + this._rateLimitRetryAttempt = 0; } // CLM maintains context for a future request. A successful settled answer @@ -3544,21 +3562,26 @@ export class AgentSession { return false; } - this._retryAttempt++; - - if (this._retryAttempt > settings.maxRetries) { - // Preserve the completed attempt count so post-run handling can emit the final failure. - this._retryAttempt--; + const rateLimited = isRateLimitError(message); + const maxRetries = rateLimited ? settings.rateLimitMaxRetries : settings.maxRetries; + const attempt = + (rateLimited ? this._rateLimitRetryAttempt : this._retryAttempt - this._rateLimitRetryAttempt) + 1; + if (attempt > maxRetries) { return false; } + this._retryAttempt++; + if (rateLimited) this._rateLimitRetryAttempt++; - const delayMs = settings.baseDelayMs * 2 ** (this._retryAttempt - 1); + const delayMs = rateLimited + ? rateLimitDelay(attempt, message.retryAfterMs) + : settings.baseDelayMs * 2 ** (attempt - 1); this._retryAbortController = new AbortController(); this._emit({ type: "auto_retry_start", - attempt: this._retryAttempt, - maxAttempts: settings.maxRetries, + attempt, + maxAttempts: maxRetries, + ...(rateLimited ? { reason: "rate_limit" as const } : {}), delayMs, errorMessage: message.errorMessage || "Unknown error", }); @@ -3576,6 +3599,7 @@ export class AgentSession { // Aborted during sleep - emit end event so UI can clean up const attempt = this._retryAttempt; this._retryAttempt = 0; + this._rateLimitRetryAttempt = 0; this._emit({ type: "auto_retry_end", success: false, diff --git a/packages/coding-agent/src/core/rate-limit.ts b/packages/coding-agent/src/core/rate-limit.ts new file mode 100644 index 00000000..ad4e47c4 --- /dev/null +++ b/packages/coding-agent/src/core/rate-limit.ts @@ -0,0 +1,20 @@ +import type { AssistantMessage } from "@step-harness/providers"; + +/** + * Temporary throttling, as classified by the provider from the HTTP status or + * error code. Exhausted billing or plan quotas are a different kind and never + * count as throttling. + */ +export function isRateLimitError(message: Pick): boolean { + return message.stopReason === "error" && message.errorKind === "rate_limit"; +} + +export function rateLimitDelay(attempt: number, retryAfterMs?: number): number { + if (retryAfterMs !== undefined && Number.isFinite(retryAfterMs) && retryAfterMs >= 0) return retryAfterMs; + return Math.round(Math.min(30_000, 1000 * 2 ** (attempt - 1)) * (0.75 + Math.random() * 0.25)); +} + +export type ModelRequestProgress = + | { type: "started" } + | { type: "succeeded" } + | { type: "rate_limit"; attempt: number; maxRetries: number; delayMs: number; message: string }; diff --git a/packages/coding-agent/src/core/settings-manager.ts b/packages/coding-agent/src/core/settings-manager.ts index a30e2e3b..99e09c0c 100644 --- a/packages/coding-agent/src/core/settings-manager.ts +++ b/packages/coding-agent/src/core/settings-manager.ts @@ -36,6 +36,7 @@ export interface ProviderRetrySettings { export interface RetrySettings { enabled?: boolean; // default: true maxRetries?: number; // default: 3 + rateLimitMaxRetries?: number; // default: 3; independent of transient and schema retries baseDelayMs?: number; // default: 2000 (exponential backoff: 2s, 4s, 8s) provider?: ProviderRetrySettings; } @@ -925,10 +926,15 @@ export class SettingsManager { this.save(); } - getRetrySettings(): { enabled: boolean; maxRetries: number; baseDelayMs: number } { + getRetrySettings(): { enabled: boolean; maxRetries: number; rateLimitMaxRetries: number; baseDelayMs: number } { + const rateLimitMaxRetries = this.settings.retry?.rateLimitMaxRetries; return { enabled: this.getRetryEnabled(), maxRetries: this.settings.retry?.maxRetries ?? 3, + rateLimitMaxRetries: + rateLimitMaxRetries === undefined || !Number.isFinite(rateLimitMaxRetries) + ? 3 + : Math.max(0, Math.min(20, Math.floor(rateLimitMaxRetries))), baseDelayMs: this.settings.retry?.baseDelayMs ?? 2000, }; } diff --git a/packages/coding-agent/src/features/step-subagent.ts b/packages/coding-agent/src/features/step-subagent.ts index 77efd105..0416da58 100644 --- a/packages/coding-agent/src/features/step-subagent.ts +++ b/packages/coding-agent/src/features/step-subagent.ts @@ -11,7 +11,7 @@ import { randomUUID } from "node:crypto"; import path from "node:path"; import type { AgentToolResult, AgentToolUpdateCallback, ThinkingLevel } from "@step-harness/agent-core"; import { Text } from "@step-harness/pi-tui"; -import { type Message, StringEnum, type Usage } from "@step-harness/providers"; +import { type Message, type ProviderErrorKind, StringEnum, type Usage } from "@step-harness/providers"; import { type Static, Type } from "typebox"; import { CONFIG_DIR_NAME } from "../config.ts"; import type { ExtensionAPI, ExtensionContext, ExtensionFactory, InlineExtension } from "../core/extensions/types.ts"; @@ -34,6 +34,7 @@ import { subagentListSignature, } from "./subagent/rendering.ts"; import { createSubagentRpcSession } from "./subagent/rpc-adapter.ts"; +import { clampWorkflowConcurrency } from "./workflow/budget.ts"; // Lane-notification primitives moved to ./subagent/lane-events.ts; re-exported // here to keep this module's public surface stable. @@ -69,8 +70,7 @@ export { WORKFLOW_SESSION_ID_PREFIX, } from "./subagent/helpers.ts"; -const MAX_PARALLEL_TASKS = 8; -const DEFAULT_CONCURRENCY = 4; +const MAX_PARALLEL_TASKS = 1000; export const MAX_JSON_LINE_BYTES = 2 * 1024 * 1024; const MAX_MESSAGES = 256; export const CHILD_MARKER = "STEPCODE_SUBAGENT_CHILD"; @@ -106,6 +106,7 @@ export interface StepSubagentRunResult { } export interface StepSubagentRunInput { + onModelRequest?: (event: import("../core/rate-limit.ts").ModelRequestProgress) => void; agent: StepAgentConfig; task: string; cwd: string; @@ -124,6 +125,12 @@ export interface StepSubagentRunInput { sessionId?: string; /** Keep the rpc child alive after the turn settles so replies reuse it. */ keepAlive?: boolean; + /** + * Resolve only after a non-keep-alive child has exited, so a caller holding a + * shared admission slot keeps it until the process has actually stopped. + * Workflow calls (workflowAcl) always wait. + */ + awaitExit?: boolean; /** * Abandon the turn after this many ms with no output from the child. * Defaults to `resolveSubagentTurnIdleTimeoutMs()`; `0` disables the watchdog. @@ -187,9 +194,9 @@ export interface StepSubagentExtensionOptions { configDirName?: string; /** Include the built-in general/explore/review/planner roles. */ includeBuiltinAgents?: boolean; - /** Maximum number of tasks accepted in one parallel call. */ + /** Maximum batch size accepted in one parallel call; excess tasks queue instead of running. */ maxParallelTasks?: number; - /** Number of child processes allowed to run at once. */ + /** Number of child processes allowed to run at once; tasks beyond this stay queued. */ maxConcurrency?: number; /** Optional isolated git worktree implementation. */ worktreeManager?: StepWorktreeManager; @@ -294,6 +301,25 @@ function usageFromMessage(message: Message, usage: StepSubagentUsage): void { usage.cost += messageUsage.cost?.total || 0; } +/** + * The provider's structured failure class, only while the run's terminal error is + * still the child's own model failure. A local failure (idle timeout, abort, + * process exit) that replaced it has no provider kind. + */ +export function providerErrorKind( + result: Pick, +): ProviderErrorKind | undefined { + if (result.stopReason !== "error") return undefined; + for (let index = result.messages.length - 1; index >= 0; index--) { + const message = result.messages[index]; + if (!isAssistantMessage(message)) continue; + return message.stopReason === "error" && message.errorMessage === result.errorMessage + ? message.errorKind + : undefined; + } + return undefined; +} + export function isFailed(result: Pick): boolean { return result.exitCode !== 0 || result.stopReason === "error" || result.stopReason === "aborted"; } @@ -409,15 +435,34 @@ const spawnedSubagentSessions = new Set(); export async function runStepSubagentProcess(input: StepSubagentRunInput): Promise { const sessionId = input.sessionId?.trim() || `${SUBAGENT_SESSION_ID_PREFIX}${randomUUID()}`; const live = getLiveSubagentSession(sessionId); - if (live) return live.runTurn(input); + if (live) { + const result = await live.runTurn(input); + if ((input.workflowAcl || input.awaitExit) && !input.keepAlive) await live.exited; + return result; + } if (spawnedSubagentSessions.has(sessionId)) input.onChildRespawn?.(); spawnedSubagentSessions.add(sessionId); const session = await createSubagentRpcSession(input, sessionId); - return session.runTurn(input); + const result = await session.runTurn(input); + // A workflow node or admitted subagent owns its child through process exit, not + // just agent_settled. This keeps its shared admission slot until cancellation + // has actually stopped the child, and lets queued work start without orphaned requests. + if ((input.workflowAcl || input.awaitExit) && !input.keepAlive) await session.exited; + return result; } -export function makeToolResult(details: StepSubagentDetails, text: string): AgentToolResult { - return { content: [{ type: "text", text }], details }; +export function makeToolResult( + details: StepSubagentDetails, + text: string, + isError = false, +): AgentToolResult { + return { + content: [{ type: "text", text }], + details, + ...(isError || details.results.some((record) => record.status === "failed" || record.status === "aborted") + ? { isError: true } + : {}), + }; } const AgentSendTargetSchema = Type.Object({ @@ -509,7 +554,7 @@ export function createStepSubagentExtension(options: StepSubagentExtensionOption maxParallelTasks: Math.max(1, Math.min(MAX_PARALLEL_TASKS, options.maxParallelTasks ?? MAX_PARALLEL_TASKS)), maxConcurrency: Math.max( 1, - Math.min(options.maxConcurrency ?? DEFAULT_CONCURRENCY, options.maxParallelTasks ?? MAX_PARALLEL_TASKS), + Math.min(clampWorkflowConcurrency(options.maxConcurrency), options.maxParallelTasks ?? MAX_PARALLEL_TASKS), ), telemetry: options.telemetry, worktreeManager: options.worktreeManager ?? { diff --git a/packages/coding-agent/src/features/subagent/execute.ts b/packages/coding-agent/src/features/subagent/execute.ts index a1b578ea..015633d8 100644 --- a/packages/coding-agent/src/features/subagent/execute.ts +++ b/packages/coding-agent/src/features/subagent/execute.ts @@ -39,6 +39,12 @@ import { type StepAgentDiscoveryResult, type StepAgentScope, } from "../step-subagent-agents.ts"; +import { acquireWorkflowAdmission } from "../workflow/admission.ts"; +import { + clampWorkflowConcurrency, + configuredWorkflowConcurrencyLimit, + type WorkflowSemaphore, +} from "../workflow/budget.ts"; import { truncateText } from "./lane-events.ts"; import type { StepSubagentLaneRuntime } from "./lane-lifecycle.ts"; @@ -279,6 +285,7 @@ export async function executeSubagent( return makeToolResult( details, `Invalid parameters. Provide exactly one mode. Available agents: ${formatStepAgentCatalog(discovery.agents)}`, + true, ); } if (parallel && parallel.length > options.maxParallelTasks) { @@ -286,13 +293,14 @@ export async function executeSubagent( return makeToolResult( details, `Too many parallel tasks (${parallel.length}); maximum is ${options.maxParallelTasks}.`, + true, ); } const tasks = parallel ?? chain ?? [single!]; const mode: StepSubagentDetails["mode"] = chain ? "chain" : parallel ? "parallel" : "single"; if (!(await confirmProjectAgents(ctx, params, discovery, tasks))) { const details = buildDetails(mode, discovery, scope, []); - return makeToolResult(details, "Canceled: project-local agents not approved."); + return makeToolResult(details, "Canceled: project-local agents not approved.", true); } const records = tasks.map((task) => makeEmptyResult(task.agent, task.task)); @@ -337,7 +345,11 @@ export async function executeSubagent( ); }; - const runOne = async (task: StepSubagentTaskInput, index: number): Promise => { + const runOne = async ( + task: StepSubagentTaskInput, + index: number, + admission?: WorkflowSemaphore, + ): Promise => { const agent = resolveStepAgent(discovery.agents, task.agent); if (!agent) { const failed = makeFailureResult( @@ -351,6 +363,33 @@ export async function executeSubagent( return failed; } reportCreated(agent, index); + // A parallel batch shares the process-wide window with workflow nodes, so the + // two together never exceed the host ceiling. The task stays queued until admitted. + let releaseSlot: (() => void) | undefined; + if (admission) { + try { + releaseSlot = await admission.acquire(signal); + } catch (error) { + const aborted = resultRecord( + agent.name, + task.task, + { + messages: [], + stderr: "", + exitCode: 1, + usage: emptyUsage(), + stopReason: "aborted", + errorMessage: error instanceof Error ? error.message : String(error), + }, + { agentSource: agent.source }, + ); + records[index] = aborted; + emit(mode); + reportFinished(aborted, index); + return aborted; + } + } + let requestEpoch = admission?.epoch ?? 0; // Flip the queued placeholder before spawning, so the step reads as running // (with a clock) from the moment it is dispatched, not from its first event. records[index] = { @@ -393,6 +432,18 @@ export async function executeSubagent( : `${laneRuntime.sessionId}-${index}` : `subagent-${randomUUID()}`, keepAlive: laneRuntime?.keepAlive === true, + // An admitted child keeps its shared slot until its process has exited. + ...(admission ? { awaitExit: true } : {}), + ...(admission + ? { + onModelRequest: (event) => { + if (event.type === "started") requestEpoch = admission.epoch; + else if (event.type === "succeeded") admission.succeeded(requestEpoch); + else admission.throttle(requestEpoch, event.delayMs); + if (event.type !== "started") requestEpoch = admission.epoch; + }, + } + : {}), }); const completed = resultRecord(agent.name, task.task, child, { agentSource: agent.source, @@ -412,6 +463,8 @@ export async function executeSubagent( emit(mode); reportFinished(failed, index); return failed; + } finally { + releaseSlot?.(); } }; @@ -422,8 +475,16 @@ export async function executeSubagent( // records are already seeded by makeEmptyResult, so this renders every lane // at 0s, including ones still queued behind maxConcurrency. emit(mode); - if (parallel) await mapWithConcurrency(parallel, options.maxConcurrency, runOne); - else if (chain) { + if (parallel) { + // The configured provider limit caps subagent batches the same way it caps workflows. + const ceiling = Math.min(options.maxConcurrency, clampWorkflowConcurrency(configuredWorkflowConcurrencyLimit())); + const lease = acquireWorkflowAdmission(ceiling); + try { + await mapWithConcurrency(parallel, ceiling, (task, index) => runOne(task, index, lease.semaphore)); + } finally { + lease.release(); + } + } else if (chain) { let previous = ""; for (let index = 0; index < chain.length; index++) { const task = { diff --git a/packages/coding-agent/src/features/subagent/rpc-adapter.ts b/packages/coding-agent/src/features/subagent/rpc-adapter.ts index 9010d79a..72bb9304 100644 --- a/packages/coding-agent/src/features/subagent/rpc-adapter.ts +++ b/packages/coding-agent/src/features/subagent/rpc-adapter.ts @@ -13,6 +13,7 @@ import { randomUUID } from "node:crypto"; import { mkdtemp, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; +import { waitForChildProcess } from "../../utils/child-process.ts"; import { CHILD_MARKER, cloneUsage, @@ -37,6 +38,7 @@ const MAX_STDERR_CHARS = 32_000; * Step-specific needs-input nag are protocol frames, not session events. */ export interface SubagentRpcLineHandlers { + onModelRequest?: StepSubagentRunInput["onModelRequest"]; /** `{"type":"response"}` command acks, correlated by `id`. */ onResponse?: (response: { id?: string; command?: string; success?: boolean; error?: string }) => void; /** `{"type":"extension_ui_request"}` dialogs; blocking ones must be answered or the child hangs. */ @@ -83,6 +85,31 @@ export function routeSubagentRpcLine(line: string, handlers: SubagentRpcLineHand handlers.onNeedsInput?.(typeof parsed.message === "string" ? parsed.message : ""); return; } + if ( + type === "auto_retry_start" && + parsed.reason === "rate_limit" && + typeof parsed.attempt === "number" && + typeof parsed.maxAttempts === "number" && + typeof parsed.delayMs === "number" && + Number.isFinite(parsed.delayMs) && + parsed.delayMs >= 0 + ) { + handlers.onModelRequest?.({ + type: "rate_limit", + attempt: parsed.attempt, + maxRetries: parsed.maxAttempts, + delayMs: parsed.delayMs, + message: String(parsed.errorMessage ?? "429 rate limited"), + }); + } + // turn_start precedes the provider request. message_start may arrive only + // after HTTP headers (or an error), too late to tag a congestion epoch. + if (type === "turn_start") handlers.onModelRequest?.({ type: "started" }); + if (isRecordValue(parsed.message) && parsed.message.role === "assistant") { + if (type === "message_end" && ["stop", "length", "toolUse"].includes(String(parsed.message.stopReason))) { + handlers.onModelRequest?.({ type: "succeeded" }); + } + } handlers.onEvent?.(line, parsed); } @@ -100,6 +127,8 @@ interface SubagentRpcTurn { /** A live `--mode rpc` child bound to one subagent session id. */ export interface StepSubagentRpcSession { + /** Resolves only once the OS child has exited (including forced shutdown). */ + readonly exited: Promise; readonly sessionId: string; /** True while the child can still accept stdin commands. */ isAlive(): boolean; @@ -205,6 +234,13 @@ export async function createSubagentRpcSession( shell: false, stdio: ["pipe", "pipe", "pipe"], }); + // `close` can be held open by inherited descendant pipes after OS exit. + // Reuse the bounded-idle stdio drain used by our other child processes. + const exitStatus = waitForChildProcess(child); + const exited = exitStatus.then( + () => undefined, + () => undefined, + ); let stdoutBuffer = ""; let turn: SubagentRpcTurn | undefined; @@ -341,6 +377,7 @@ export async function createSubagentRpcSession( }, onExtensionError: (message) => appendStderr(`extension_error: ${message}\n`), onNeedsInput: (message) => turn?.input.onNeedsInput?.(message), + onModelRequest: (event) => turn?.input.onModelRequest?.(event), onEvent: (eventLine, event) => { const active = turn; if (!active) return; @@ -388,7 +425,7 @@ export async function createSubagentRpcSession( }); void rm(tempDir, { recursive: true, force: true }); }); - child.once("close", (code) => { + const finishExitedChild = (code: number | null): void => { childExited = true; dropFromRegistry(); clearIdleWatchdog(); @@ -409,7 +446,13 @@ export async function createSubagentRpcSession( } }); void rm(tempDir, { recursive: true, force: true }); + }; + child.once("exit", () => { + childExited = true; + dropFromRegistry(); }); + child.once("close", finishExitedChild); + void exitStatus.then(finishExitedChild, () => undefined); const runTurn = (turnInput: StepSubagentRunInput): Promise => { if (turn) { @@ -484,6 +527,7 @@ export async function createSubagentRpcSession( }; const handle: StepSubagentRpcSession = { + exited, sessionId, isAlive: () => !childExited && !stdinEnded, isTurnActive: () => turn !== undefined, diff --git a/packages/coding-agent/src/features/workflow/admission.ts b/packages/coding-agent/src/features/workflow/admission.ts new file mode 100644 index 00000000..21413e80 --- /dev/null +++ b/packages/coding-agent/src/features/workflow/admission.ts @@ -0,0 +1,56 @@ +import { WorkflowSemaphore } from "./budget.ts"; + +/** A process-wide node window, shared by concurrent workflow tools/sessions. + * Conservatively includes all models: model overrides cannot escape the host + * ceiling. Each run also retains its own lower maxConcurrency window. + */ +let pool: { semaphore: WorkflowSemaphore; members: Set<{ ceiling: number }>; idleSince?: number } | undefined; + +/** + * How long a congested window is remembered after its last workflow finishes. A + * session often runs workflows back to back (one per phase); starting each at the + * full ceiling straight after a 429 would re-trigger the congestion just seen. An + * idle window that never throttled is discarded immediately. + */ +const THROTTLED_POOL_RETENTION_MS = 60_000; + +export interface WorkflowAdmissionLease { + semaphore: WorkflowSemaphore; + release(): void; +} + +export function acquireWorkflowAdmission(ceiling: number): WorkflowAdmissionLease { + if (pool && pool.members.size === 0 && Date.now() - (pool.idleSince ?? 0) >= THROTTLED_POOL_RETENTION_MS) { + pool.semaphore.dispose(); + pool = undefined; + } + pool ??= { semaphore: new WorkflowSemaphore(ceiling), members: new Set() }; + const current = pool; + current.idleSince = undefined; + const member = { ceiling }; + current.members.add(member); + current.semaphore.setCeiling(Math.min(...Array.from(current.members, (item) => item.ceiling))); + return { + semaphore: current.semaphore, + release() { + if (!current.members.delete(member)) return; + if (current.members.size > 0) { + current.semaphore.setCeiling(Math.min(...Array.from(current.members, (item) => item.ceiling))); + } else if (current.semaphore.throttled) { + current.semaphore.park(); + current.idleSince = Date.now(); + } else { + current.semaphore.dispose(); + if (pool === current) pool = undefined; + } + }, + }; +} + +/** Forget retained congestion state. Intended for tests and host shutdown. */ +export function resetWorkflowAdmission(): void { + if (pool && pool.members.size === 0) { + pool.semaphore.dispose(); + pool = undefined; + } +} diff --git a/packages/coding-agent/src/features/workflow/agent-runner.ts b/packages/coding-agent/src/features/workflow/agent-runner.ts index 7bdc602b..2fcfd9c3 100644 --- a/packages/coding-agent/src/features/workflow/agent-runner.ts +++ b/packages/coding-agent/src/features/workflow/agent-runner.ts @@ -1,6 +1,13 @@ import path from "node:path"; import type { ThinkingLevel } from "@step-harness/agent-core"; -import { finalOutput, isFailed, resultText, runStepSubagentProcess } from "../step-subagent.ts"; +import { + finalOutput, + isFailed, + providerErrorKind, + resultText, + runStepSubagentProcess, + type StepSubagentRunResult, +} from "../step-subagent.ts"; import { discoverStepAgents, formatStepAgentCatalog, resolveStepAgent } from "../step-subagent-agents.ts"; import { WORKFLOW_SESSION_ID_PREFIX } from "../subagent/helpers.ts"; import { resolveWorkflowToolProfile } from "./tool-profile.ts"; @@ -43,6 +50,18 @@ export function createDefaultWorkflowAgentRunner(options: DefaultWorkflowAgentRu signal: input.signal, sessionId: `${WORKFLOW_SESSION_ID_PREFIX}${input.runId}-${input.agentId}`, keepAlive: false, + onModelRequest: input.onModelRequest, + // The child reports tool_execution_start before the tool's permission check + // and execution, so the parent learns about a write before it happens. + ...(input.onToolExecutionStart + ? { + onUpdate: (update: StepSubagentRunResult) => { + if (update.lastEvent === "tool_execution_start") { + input.onToolExecutionStart?.(update.activeTool ?? "tool"); + } + }, + } + : {}), workflowAcl: { baseCwd: input.cwd, ...(input.options.readOnly ? { readOnly: input.options.readOnly } : {}), @@ -51,11 +70,15 @@ export function createDefaultWorkflowAgentRunner(options: DefaultWorkflowAgentRu }); const usage = usageFromSubagent(result.usage); if (isFailed(result)) { + const errorKind = providerErrorKind(result); return { + ...(errorKind ? { errorKind } : {}), text: resultText(result), usage, status: result.stopReason === "aborted" ? "aborted" : "failed", errorMessage: result.errorMessage ?? result.stderr, + // A killed or crashed child can lose a tool event in its unread output. + toolActivityObserved: result.exitCode === 0 && result.stopReason !== "aborted", ...(result.model ? { model: result.model } : {}), }; } @@ -64,6 +87,7 @@ export function createDefaultWorkflowAgentRunner(options: DefaultWorkflowAgentRu text, usage, status: "completed", + toolActivityObserved: true, ...(result.model ? { model: result.model } : {}), }; }; diff --git a/packages/coding-agent/src/features/workflow/budget.ts b/packages/coding-agent/src/features/workflow/budget.ts index 0d728c80..cf3f68cd 100644 --- a/packages/coding-agent/src/features/workflow/budget.ts +++ b/packages/coding-agent/src/features/workflow/budget.ts @@ -2,7 +2,6 @@ import os from "node:os"; import { emptyWorkflowUsage, mergeWorkflowUsage, type WorkflowUsage, workflowUsageTokens } from "./types.ts"; const DEFAULT_MAX_CONCURRENCY = 16; -const HARD_MAX_CONCURRENCY = 32; const DEFAULT_MAX_AGENTS = 1000; /** Raised when a workflow cannot afford another completed agent call. */ @@ -75,7 +74,16 @@ export function defaultWorkflowConcurrency(cpuCount = os.cpus().length): number export function clampWorkflowConcurrency(value: number | undefined, cpuCount = os.cpus().length): number { const fallback = defaultWorkflowConcurrency(cpuCount); if (value === undefined || !Number.isFinite(value)) return fallback; - return Math.max(1, Math.min(HARD_MAX_CONCURRENCY, Math.floor(value))); + return Math.max(1, Math.min(fallback, Math.floor(value))); +} + +/** Empty optional environment values mean no configured provider override. */ +export function configuredWorkflowConcurrencyLimit( + raw = process.env.STEP_WORKFLOW_CONCURRENCY_LIMIT, +): number | undefined { + if (raw === undefined || raw.trim() === "") return undefined; + const value = Number(raw); + return Number.isFinite(value) && value > 0 ? value : undefined; } export function clampWorkflowAgentLimit(value: number | undefined): number { @@ -83,9 +91,27 @@ export function clampWorkflowAgentLimit(value: number | undefined): number { return Math.max(1, Math.min(DEFAULT_MAX_AGENTS, Math.floor(value))); } +/** + * Upper bound for an admission cooldown caused by a 429. + * + * A server's `Retry-After` can be hours, or a far-future HTTP date, and the node's + * own retry deliberately honors it verbatim. The admission window is shared by every + * workflow in the process and its queue time is excluded from node timeouts, so an + * unbounded cooldown would stall unrelated workflows indefinitely with no error and + * no timeout. The adaptive cap reduction below is what actually protects the provider; + * this bound only decides how long the window pauses before probing again. + */ +const MAX_THROTTLE_COOLDOWN_MS = 30_000; + /** FIFO semaphore used by agent() so VM scripts cannot fan out unbounded work. */ export class WorkflowSemaphore { - private readonly limit: number; + private limit: number; + private ceiling: number; + private disposed = false; + private generation = 0; + private successes = 0; + private cooldownUntil = 0; + private wakeup?: ReturnType; private activeCount = 0; private peakCount = 0; private readonly waiters: Array<{ @@ -96,7 +122,64 @@ export class WorkflowSemaphore { }> = []; constructor(limit: number) { - this.limit = clampWorkflowConcurrency(limit, limit + 2); + this.limit = Math.max(1, Math.floor(limit)); + this.ceiling = this.limit; + } + + get epoch(): number { + return this.generation; + } + + setCeiling(ceiling: number): void { + if (ceiling === this.ceiling) return; + const wasAtCeiling = this.limit === this.ceiling; + this.ceiling = ceiling; + this.limit = wasAtCeiling ? ceiling : Math.min(this.limit, ceiling); + this.generation++; + this.successes = 0; + this.drain(); + } + + throttle(epoch: number, delayMs: number): void { + if (this.disposed) return; + this.successes = 0; + if (epoch === this.generation) { + this.limit = Math.max(1, Math.floor(this.limit * 0.75)); + this.generation++; + } + const cooldownMs = Number.isFinite(delayMs) ? Math.max(0, Math.min(MAX_THROTTLE_COOLDOWN_MS, delayMs)) : 0; + this.cooldownUntil = Math.max(this.cooldownUntil, Date.now() + cooldownMs); + this.scheduleDrain(); + } + + succeeded(epoch: number): void { + if (this.disposed) return; + if (epoch !== this.generation) return; + if (++this.successes >= 4 && this.waiters.length > 0 && this.limit < this.ceiling) { + this.limit++; + this.successes = 0; + this.drain(); + } + } + + private scheduleDrain(): void { + if (this.wakeup) clearTimeout(this.wakeup); + this.wakeup = setTimeout( + () => { + this.wakeup = undefined; + this.drain(); + }, + Math.min(2_147_483_647, Math.max(0, this.cooldownUntil - Date.now())), + ); + } + + dispose(): void { + this.disposed = true; + if (this.wakeup) clearTimeout(this.wakeup); + for (const waiter of this.waiters.splice(0)) { + if (waiter.signal && waiter.onAbort) waiter.signal.removeEventListener("abort", waiter.onAbort); + waiter.reject(new Error("Workflow admission is closed")); + } } get active(): number { @@ -111,9 +194,21 @@ export class WorkflowSemaphore { return this.limit; } + /** True while congestion has lowered the cap or a cooldown is still pending. */ + get throttled(): boolean { + return this.limit < this.ceiling || Date.now() < this.cooldownUntil; + } + + /** Stop the cooldown timer while no run is attached; cap and cooldown are kept. */ + park(): void { + if (this.wakeup) clearTimeout(this.wakeup); + this.wakeup = undefined; + } + async acquire(signal?: AbortSignal): Promise<() => void> { + if (this.disposed) throw new Error("Workflow admission is closed"); if (signal?.aborted) throw new Error("Workflow operation was aborted"); - if (this.activeCount < this.limit) { + if (this.activeCount < this.limit && Date.now() >= this.cooldownUntil && this.waiters.length === 0) { this.activeCount += 1; this.peakCount = Math.max(this.peakCount, this.activeCount); return this.makeRelease(); @@ -121,6 +216,7 @@ export class WorkflowSemaphore { return new Promise<() => void>((resolve, reject) => { const waiter: (typeof this.waiters)[number] = { resolve, reject, signal }; this.waiters.push(waiter); + if (!this.wakeup && Date.now() < this.cooldownUntil) this.scheduleDrain(); if (signal) { const onAbort = (): void => { const index = this.waiters.indexOf(waiter); @@ -145,6 +241,11 @@ export class WorkflowSemaphore { } private drain(): void { + if (this.disposed) return; + if (Date.now() < this.cooldownUntil) { + this.scheduleDrain(); + return; + } while (this.activeCount < this.limit && this.waiters.length > 0) { const waiter = this.waiters.shift(); if (!waiter) return; diff --git a/packages/coding-agent/src/features/workflow/journal.ts b/packages/coding-agent/src/features/workflow/journal.ts index 3d162e44..e12746f9 100644 --- a/packages/coding-agent/src/features/workflow/journal.ts +++ b/packages/coding-agent/src/features/workflow/journal.ts @@ -81,8 +81,8 @@ export async function readJsonLines(filePath: string): Promise { try { values.push(JSON.parse(line) as T); } catch { - // A partial final line can be left by a killed process. Stop here so a - // later run can only resume the verified contiguous prefix. + // A partial final line can be left by a killed process; nothing after it + // is trusted. break; } } @@ -100,12 +100,18 @@ export async function writeWorkflowFileAtomic(filePath: string, content: string) } } -/** Append-only run journal with conservative prefix-only resume semantics. */ +/** + * Append-only run journal. Resume replays a completed call whose prompt and + * options hash matches, independent of its position: a failed, inserted, or + * reordered call (pipeline stages finish in nondeterministic order) only re-runs + * itself. Identical calls are consumed in their original order, so N identical + * calls replay at most N cached results. The runtime decides which entries stay + * eligible once the resumed run has changed the workspace. + */ export class WorkflowJournal { readonly paths: WorkflowRunPaths; - private readonly entries = new Map(); + private readonly cached = new Map(); private appendQueue: Promise = Promise.resolve(); - private resumeEnabled = false; private initialized = false; constructor(paths: WorkflowRunPaths, resumeFrom?: WorkflowRunPaths) { @@ -129,28 +135,33 @@ export class WorkflowJournal { } const source = resumeFrom?.journalPath ?? this.resumeFromPath; if (source) { - const previous = await readJsonLines(source); + const previous = (await readJsonLines(source)).filter(isJournalEntry); + previous.sort((left, right) => left.seq - right.seq); for (const entry of previous) { - if (isJournalEntry(entry)) this.entries.set(entry.seq, entry); + const queue = this.cached.get(entry.callHash); + if (queue) queue.push(entry); + else this.cached.set(entry.callHash, [entry]); } - this.resumeEnabled = this.entries.size > 0; } this.initialized = true; } - /** Return a cached result only while every prior call has matched. */ - getCached(seq: number, callHash: string): WorkflowJournalEntry | undefined { - if (!this.resumeEnabled) return undefined; - const entry = this.entries.get(seq); - if (!entry || entry.callHash !== callHash || (entry.status !== "completed" && entry.status !== "cached")) { - this.resumeEnabled = false; - return undefined; - } - return entry; + /** + * Take the earliest unused completed result recorded for this exact call that + * `accept` allows. A rejected entry stays available to later calls. + */ + getCached( + callHash: string, + accept: (entry: WorkflowJournalEntry) => boolean = () => true, + ): WorkflowJournalEntry | undefined { + const queue = this.cached.get(callHash); + if (!queue) return undefined; + const index = queue.findIndex(accept); + if (index < 0) return undefined; + return queue.splice(index, 1)[0]; } async append(entry: WorkflowJournalEntry): Promise { - this.entries.set(entry.seq, entry); const line = `${JSON.stringify(entry)}\n`; await this.enqueue(async () => { await mkdir(path.dirname(this.paths.journalPath), { recursive: true }); diff --git a/packages/coding-agent/src/features/workflow/progress.ts b/packages/coding-agent/src/features/workflow/progress.ts index fbf80115..1ef6ce42 100644 --- a/packages/coding-agent/src/features/workflow/progress.ts +++ b/packages/coding-agent/src/features/workflow/progress.ts @@ -40,7 +40,11 @@ export class WorkflowProgressStore { // updates arrive while this snapshot is waiting for its disk write. const snapshot = this.current(); const serialized = `${JSON.stringify(snapshot, null, 2)}\n`; - this.writeQueue = this.writeQueue.then(() => writeWorkflowFileAtomic(this.path, serialized)); + // A failed write must not poison the queue: the next update has to be able to + // persist, or one transient disk error freezes progress.json forever. + this.writeQueue = this.writeQueue + .catch(() => undefined) + .then(() => writeWorkflowFileAtomic(this.path, serialized)); await this.writeQueue; return snapshot; } diff --git a/packages/coding-agent/src/features/workflow/rendering.ts b/packages/coding-agent/src/features/workflow/rendering.ts index 19f91bd9..6e7ef354 100644 --- a/packages/coding-agent/src/features/workflow/rendering.ts +++ b/packages/coding-agent/src/features/workflow/rendering.ts @@ -4,7 +4,7 @@ import type { ToolRenderContext, ToolRenderResultOptions } from "../../core/exte import { keyHint } from "../../render/keybinding-hints.ts"; import type { Theme } from "../../theme/theme.ts"; import type { WorkflowRequest } from "./step-workflow.ts"; -import type { WorkflowProgress, WorkflowProgressAgent, WorkflowRunResult } from "./types.ts"; +import type { WorkflowProgress, WorkflowProgressAgent, WorkflowRunResult, WorkflowTaskFailure } from "./types.ts"; const COLLAPSED_AGENTS = 8; const AGENT_ORDER: Record = { @@ -28,11 +28,17 @@ function singleLine(text: string): string { function progressSummary(progress: WorkflowProgress): string { const counts = { running: 0, queued: 0, completed: 0, failed: 0, aborted: 0, cached: 0 }; for (const agent of progress.agents) counts[agent.status] += 1; + const retrying = progress.agents.filter((agent) => agent.status === "running" && agent.retry).length; return [ progress.status.replace(/_/gu, " "), - `${counts.running} running`, + ...(progress.concurrency + ? [`cap ${progress.concurrency.cap}/${progress.concurrency.limit} (ceiling ${progress.concurrency.ceiling})`] + : []), + `${counts.running - retrying} running`, + ...(retrying ? [`${retrying} retrying`] : []), `${counts.queued} queued`, `${progress.completedAgents}/${progress.totalAgents} completed`, + ...(progress.taskFailures?.length ? [`${progress.taskFailures.length} callback errors`] : []), ...(counts.cached ? [`${counts.cached} cached`] : []), ...(counts.failed ? [`${counts.failed} failed`] : []), ...(counts.aborted ? [`${counts.aborted} aborted`] : []), @@ -40,6 +46,22 @@ function progressSummary(progress: WorkflowProgress): string { ].join(" · "); } +function concurrencyNotice(concurrency: WorkflowProgress["concurrency"]): string | undefined { + return concurrency?.requested !== undefined && concurrency.requested > concurrency.limit + ? `Notice: requested concurrency ${concurrency.requested} was limited to ${concurrency.limit} by the host ceiling (CPU count or configured provider limit); tasks will queue normally.` + : undefined; +} + +/** + * A nested failure's index refers to the child script's array, so the script is the + * only thing that tells a reader which list to inspect. + */ +function taskFailureLine(failure: WorkflowTaskFailure): string { + const stage = failure.stage !== undefined ? `.${failure.stage}` : ""; + const script = failure.script ? `${singleLine(failure.script)} ` : ""; + return `${script}${failure.primitive}[${failure.index}${stage}]: ${singleLine(failure.message)}`; +} + function visibleAgents(progress: WorkflowProgress, expanded: boolean): WorkflowProgressAgent[] { const ordered = [...progress.agents].sort((left, right) => AGENT_ORDER[left.status] - AGENT_ORDER[right.status]); // Show every running task (bounded by the runtime's concurrency limit). @@ -51,19 +73,23 @@ function visibleAgents(progress: WorkflowProgress, expanded: boolean): WorkflowP function agentSummary(agent: WorkflowProgressAgent): string { const label = singleLine(agent.label); const task = agent.task ? singleLine(agent.task) : ""; - return `${agent.status} ${label}${task && task !== label ? `: ${task}` : ""}`; + return `${agent.retry ? `retrying 429 (${agent.retry.attempt}/${agent.retry.maxRetries}, ${agent.retry.delayMs}ms)` : agent.status} ${label}${task && task !== label ? `: ${task}` : ""}${agent.error ? ` — ${singleLine(agent.error.message)}` : ""}`; } /** Text remains useful to RPC/headless consumers; details carry the complete snapshot. */ export function workflowProgressResult(progress: WorkflowProgress): AgentToolResult { const agents = visibleAgents(progress, false); const hidden = progress.agents.length - agents.length; + const notice = concurrencyNotice(progress.concurrency); return { content: [ { type: "text", text: [ progressSummary(progress), + ...(notice ? [notice] : []), + ...(progress.taskFailures ?? []).slice(0, 3).map((failure) => taskFailureLine(failure)), + ...(progress.status !== "running" && progress.message ? [singleLine(progress.message)] : []), ...(progress.currentPhase ? [`Phase: ${singleLine(progress.currentPhase)}`] : []), ...agents.map(agentSummary), ...(hidden ? [`${hidden} more agents`] : []), @@ -98,10 +124,21 @@ export function renderWorkflowResult( const bodyWidth = Math.max(1, width - 4); const rows: string[] = []; let expandable = false; + const notice = concurrencyNotice(progress?.concurrency ?? completed?.concurrency); + if (notice) rows.push(...wrapTextWithAnsi(theme.fg("muted", notice), bodyWidth)); if (progress) { const color = - progress.status === "running" ? "accent" : progress.status === "completed" ? "success" : "error"; + progress.status === "running" + ? "accent" + : progress.status === "stopping" + ? "warning" + : progress.status === "completed" + ? "success" + : "error"; rows.push(...wrapTextWithAnsi(theme.fg(color, progressSummary(progress)), bodyWidth)); + for (const failure of (progress.taskFailures ?? []).slice(0, options.expanded ? undefined : 3)) { + rows.push(...wrapTextWithAnsi(theme.fg("error", taskFailureLine(failure)), bodyWidth)); + } if (progress.currentPhase) { rows.push( ...wrapTextWithAnsi(theme.fg("muted", `Phase: ${singleLine(progress.currentPhase)}`), bodyWidth), @@ -118,14 +155,23 @@ export function renderWorkflowResult( const hidden = progress.agents.length - agents.length; if (hidden) rows.push(theme.fg("muted", `${hidden} more agents`)); expandable = progress.agents.length > 0; - if (progress.status === "running" && progress.message && progress.message !== "Workflow started") { - rows.push(truncateToWidth(theme.fg("dim", singleLine(progress.message)), bodyWidth)); + if ( + progress.message && + progress.message !== "Workflow started" && + progress.message !== "Workflow completed" + ) { + rows.push( + ...wrapTextWithAnsi( + theme.fg(progress.status === "failed" ? "error" : "muted", singleLine(progress.message)), + bodyWidth, + ), + ); } } else if (completed) { rows.push( ...wrapTextWithAnsi( theme.fg( - "success", + completed.status === "completed" ? "success" : "error", `${completed.status} · ${completed.agentCalls} agent calls · ${completed.spentTokens} tokens`, ), bodyWidth, diff --git a/packages/coding-agent/src/features/workflow/runtime.ts b/packages/coding-agent/src/features/workflow/runtime.ts index ac070d58..0537c3c5 100644 --- a/packages/coding-agent/src/features/workflow/runtime.ts +++ b/packages/coding-agent/src/features/workflow/runtime.ts @@ -7,6 +7,7 @@ import { type StepTelemetryReporter, trackStepTelemetry, } from "../../step/telemetry.ts"; +import { acquireWorkflowAdmission, type WorkflowAdmissionLease } from "./admission.ts"; import { createDefaultWorkflowAgentRunner, normalizeWorkflowAgentValue } from "./agent-runner.ts"; import { clampWorkflowAgentLimit, @@ -29,9 +30,10 @@ import { } from "./hoh.ts"; import { type WorkflowJournal, workflowHash, workflowJsonValue } from "./journal.ts"; import { WorkflowProgressStore } from "./progress.ts"; -import { validateWorkflowSchema } from "./schema.ts"; +import { isSafeParseSchema, validateWorkflowSchema } from "./schema.ts"; import { checkWorkflowPathAccess, + classifyWorkflowToolEffect, isWorkflowPathInside, resolveWorkflowToolProfile, type WorkflowAcl, @@ -43,9 +45,11 @@ import type { WorkflowAgentOptions, WorkflowAgentRunner, WorkflowAgentRunResult, + WorkflowJournalEntry, WorkflowProgress, WorkflowProgressAgent, WorkflowRunResult, + WorkflowTaskFailure, WorkflowUsage, } from "./types.ts"; import { emptyWorkflowUsage, mergeWorkflowUsage, workflowUsageTokens } from "./types.ts"; @@ -69,8 +73,12 @@ export interface WorkflowRuntimeOptions { telemetry?: StepTelemetryReporter; budgetTotal?: number | null; maxConcurrency?: number; + /** Trusted host/provider ceiling, never a model-authored override. */ + providerConcurrencyLimit?: number; maxAgents?: number; agentTimeoutMs?: number; + /** Grace for an injected runner to settle after cancellation. Production children exit within 10s. */ + runnerCleanupTimeoutMs?: number; context?: ExtensionContext; nestedWorkflow?: (name: string, args: unknown, parent: WorkflowRuntime, depth: number) => Promise; signal?: AbortSignal; @@ -103,6 +111,7 @@ export class WorkflowRuntime { readonly name: string; readonly budget: WorkflowBudget; readonly semaphore: WorkflowSemaphore; + private readonly concurrency: NonNullable; private readonly journal: WorkflowJournal; private readonly progress: WorkflowProgressStore; private readonly onProgress?: WorkflowRuntimeOptions["onProgress"]; @@ -115,9 +124,23 @@ export class WorkflowRuntime { private readonly signal?: AbortSignal; private readonly maxAgents: number; private readonly agentTimeoutMs: number; + private readonly runnerCleanupTimeoutMs: number; + private readonly controller = new AbortController(); + private readonly pendingOperations = new Set>(); + private readonly nodeErrors = new WeakSet(); + private readonly nestedTaskFailures: WorkflowTaskFailure[] = []; + private readonly recordedFailures = new Set(); + private admission?: WorkflowAdmissionLease; + private lifecycle: "idle" | "running" | "closing" | "closed" = "idle"; private sequence = 0; private agentCount = 0; private cacheHits = 0; + /** + * Set once a live call in this run may have changed the environment. Cached + * results recorded against the previous environment are then stale unless the + * call never touched it. + */ + private environmentChanged = false; private phases: Array<{ title: string; detail?: string }> = []; private startedAt = 0; private activeWriterId: string | undefined; @@ -132,13 +155,20 @@ export class WorkflowRuntime { this.journal = options.journal; this.onProgress = options.onProgress; this.budget = new WorkflowBudget(options.budgetTotal); - this.semaphore = new WorkflowSemaphore(clampWorkflowConcurrency(options.maxConcurrency)); + const ceiling = clampWorkflowConcurrency(options.providerConcurrencyLimit); + const limit = Math.min(ceiling, clampWorkflowConcurrency(options.maxConcurrency)); + this.concurrency = { requested: options.maxConcurrency, ceiling, limit, cap: limit }; + this.semaphore = new WorkflowSemaphore(limit); this.maxAgents = clampWorkflowAgentLimit(options.maxAgents); this.agentTimeoutMs = clampAgentTimeout(options.agentTimeoutMs); + this.runnerCleanupTimeoutMs = options.runnerCleanupTimeoutMs ?? 12_000; this.runner = options.runner ?? createDefaultWorkflowAgentRunner(); this.telemetry = options.telemetry; this.context = options.context; - this.signal = options.signal; + this.signal = AbortSignal.any([ + this.controller.signal, + ...[options.signal, options.context?.signal].filter((signal): signal is AbortSignal => signal !== undefined), + ]); this.nestedWorkflow = options.nestedWorkflow; this.now = options.now ?? Date.now; this.vmExecutor = options.vmExecutor ?? runInQuickJs; @@ -149,6 +179,7 @@ export class WorkflowRuntime { runId: this.runId, name: this.name, status: "running", + concurrency: { ...this.concurrency }, startedAt, updatedAt: startedAt, agents: [], @@ -161,17 +192,25 @@ export class WorkflowRuntime { } async runScript(script: string, args: unknown, vmOptions: WorkflowVmOptions = {}): Promise { - await this.journal.initialize(script); - await this.updateProgress({ status: "running", message: "Workflow started" }); - this.track("workflow_started", { phase_count: 0 }); + if (this.lifecycle !== "idle") throw new Error("Workflow run is already started or closed"); + this.lifecycle = "running"; + this.admission ??= acquireWorkflowAdmission(this.concurrency.ceiling); try { + await this.journal.initialize(script); + await this.updateProgress({ status: "running", message: "Workflow started" }); + this.track("workflow_started", { phase_count: 0 }); if (this.signal?.aborted || this.context?.signal?.aborted) { throw new Error("Workflow operation was aborted"); } const vmResult = await this.vmExecutor(script, args, this.host(0), { ...vmOptions, + signal: this.signal, filename: vmOptions.filename ?? this.journal.paths.scriptPath, }); + if (this.pendingOperations.size > 0) + throw new Error( + "Workflow script returned with unawaited agent calls or workflow/iterate operations; await the complete operation", + ); if (this.signal?.aborted || this.context?.signal?.aborted) { // A swallowing script (parallel()/pipeline() null-on-error) can absorb the abort // thrown by semaphore.acquire()/the runner and return normally; recheck here so a @@ -187,23 +226,58 @@ export class WorkflowRuntime { throw new WorkflowBudgetExceeded(budgetTotal, this.budget.spent(), 0); } const finishedAt = this.readNow(); + const snapshot = this.progress.current(); + const failures = snapshot.agents.filter((agent) => agent.status === "failed" || agent.status === "aborted"); + const taskFailures = [...this.nestedTaskFailures, ...(vmResult.taskFailures ?? [])]; + const status = + failures.length === 0 && taskFailures.length === 0 + ? "completed" + : snapshot.completedAgents > 0 + ? "partial_failed" + : "failed"; await this.updateProgress({ - status: "completed", + status, + taskFailures, spentTokens: this.budget.spent(), - message: "Workflow completed", - }); + message: + status !== "completed" + ? `Workflow finished with ${failures.length} failed nodes and ${taskFailures.length} failed callbacks` + : "Workflow completed", + }).catch(() => undefined); this.track("workflow_finished", { - status: "completed", + status, agent_count: this.agentCount, cache_hits: this.cacheHits, spent_tokens: this.budget.spent(), }); - await Promise.all([this.journal.flush(), this.progress.flush()]); + // The computed result is authoritative; a failed final disk write must not turn a + // completed run into a thrown error. + await Promise.all([this.journal.flush(), this.progress.flush()]).catch(() => undefined); return { schemaVersion: 1, runId: this.runId, name: this.name, - status: "completed", + status, + summary: { + total: snapshot.totalAgents, + completed: snapshot.completedAgents, + failed: failures.length, + ...(taskFailures.length ? { failedCallbacks: taskFailures.length } : {}), + }, + ...(taskFailures.length ? { taskFailures } : {}), + timing: { + runElapsedMs: finishedAt - this.startedAt, + maxNodeExecutionMs: Math.max( + 0, + ...snapshot.agents.map((agent) => + agent.startedAt === undefined || agent.finishedAt === undefined + ? 0 + : agent.finishedAt - agent.startedAt, + ), + ), + }, + failures, + concurrency: this.concurrencySnapshot(), value: vmResult.value, meta: vmResult.meta, scriptPath: this.journal.paths.scriptPath, @@ -221,15 +295,35 @@ export class WorkflowRuntime { : error instanceof WorkflowBudgetExceeded ? "budget_exceeded" : "failed"; - await this.updateProgress({ status, spentTokens: this.budget.spent(), message: errorMessage(error) }); + this.lifecycle = "closing"; + try { + // A progress write failure must never replace the run's terminal error, and a + // run whose disk writes fail must still publish its terminal state. + await this.updateProgress({ + status: "stopping", + message: `${errorMessage(error)}; cancelling unfinished nodes`, + }).catch(() => undefined); + } finally { + this.controller.abort(error); + await Promise.allSettled([...this.pendingOperations]); + } + await this.updateProgress({ status, spentTokens: this.budget.spent(), message: errorMessage(error) }).catch( + () => undefined, + ); this.track("workflow_finished", { status, agent_count: this.agentCount, cache_hits: this.cacheHits, spent_tokens: this.budget.spent(), }); - await Promise.all([this.journal.flush(), this.progress.flush()]); + await Promise.all([this.journal.flush(), this.progress.flush()]).catch(() => undefined); throw error; + } finally { + this.lifecycle = "closed"; + this.controller.abort(); + this.semaphore.dispose(); + this.admission?.release(); + this.admission = undefined; } } @@ -240,11 +334,41 @@ export class WorkflowRuntime { vmOptions: WorkflowVmOptions = {}, ): Promise { if (depth > 1) throw new Error("Nested workflow depth is limited to one level"); - const result = await this.vmExecutor(script, args, this.host(depth), vmOptions); + const result = await this.vmExecutor(script, args, this.host(depth), { ...vmOptions, signal: this.signal }); + this.nestedTaskFailures.push( + ...(result.taskFailures ?? []).map((failure) => ({ + ...failure, + script: vmOptions.filename ?? "nested workflow", + })), + ); return result.value; } - async agent(prompt: string, options: WorkflowAgentOptions = {}): Promise { + agent(prompt: string, options: WorkflowAgentOptions = {}): Promise { + return this.trackOperation(() => this.runAgent(prompt, options)); + } + + private trackOperation(start: () => Promise): Promise { + if (this.lifecycle === "closed" || this.lifecycle === "closing") + return Promise.reject(new Error("Workflow scope is closed")); + this.admission ??= acquireWorkflowAdmission(this.concurrency.ceiling); + const pending = Promise.resolve().then(() => { + if (this.lifecycle === "closed" || this.lifecycle === "closing") throw new Error("Workflow scope is closed"); + return start(); + }); + this.pendingOperations.add(pending); + const settled = (): void => { + this.pendingOperations.delete(pending); + if (this.lifecycle === "idle" && this.pendingOperations.size === 0) { + this.admission?.release(); + this.admission = undefined; + } + }; + void pending.then(settled, settled); + return pending; + } + + private async runAgent(prompt: string, options: WorkflowAgentOptions): Promise { const normalizedPrompt = normalizePrompt(prompt); const normalizedOptions = this.normalizeAgentOptions(options); const seq = this.sequence; @@ -252,7 +376,9 @@ export class WorkflowRuntime { if (this.agentCount >= this.maxAgents) throw new Error(`Workflow exceeded the ${this.maxAgents}-agent limit`); this.agentCount += 1; const callHash = workflowHash({ prompt: normalizedPrompt, options: serializableOptions(normalizedOptions) }); - const cached = this.journal.getCached(seq, callHash); + // Decide replay before the first await: calls a script issues together all + // see the environment as it was before any of their live siblings ran a tool. + const cached = this.takeCached(callHash, normalizedOptions); const task = normalizedPrompt.replace(/\s+/gu, " ").slice(0, 200); const label = normalizedOptions.label?.trim() || task; const agentId = `${this.runId}-${seq + 1}`; @@ -260,7 +386,9 @@ export class WorkflowRuntime { if (cached) { this.cacheHits += 1; await this.journal.append({ - ...cached, + ...cached.entry, + seq, + result: cached.result, callId: agentId, prompt: normalizedPrompt, options: workflowJsonValue(serializableOptions(normalizedOptions)), @@ -272,14 +400,14 @@ export class WorkflowRuntime { ...identity, status: "cached", finishedAt: this.readNow(), - usageTokens: workflowUsageTokens(cached.usage), + usageTokens: workflowUsageTokens(cached.entry.usage), }, spentTokens: this.budget.spent(), }); this.track("workflow_agent_finished", { status: "cached", cached: true, - token_count: workflowUsageTokens(cached.usage), + token_count: workflowUsageTokens(cached.entry.usage), }); this.track("workflow_resumed", { cache_hits: this.cacheHits }); return cached.result; @@ -288,13 +416,29 @@ export class WorkflowRuntime { const retries = clampRetries(normalizedOptions.retries); const parentSignal = this.signal ?? this.context?.signal; let release: (() => void) | undefined; + let releaseShared: (() => void) | undefined; + const shared = this.admission!.semaphore; let startedAt: number | undefined; let lastErrors: string[] = []; let totalUsage: WorkflowUsage = emptyWorkflowUsage(); + let rateLimitRetries = 0; + let cleanupIncomplete = false; + let timedOut = false; + // Only the runner's structured provider classification can indicate congestion. + // Error text, agent output, node labels and ids are never classified. + let providerFailureKind: WorkflowAgentRunResult["errorKind"]; + let schemaAttempt = 0; + let runnerInvoked = false; + let toolActivityObserved = true; + let worldAccess = false; + let requestEpoch = this.semaphore.epoch; + let sharedEpoch = shared.epoch; try { await this.updateProgress({ agent: { ...identity, status: "queued" } }); this.validateMounts(normalizedOptions); release = await this.semaphore.acquire(parentSignal); + releaseShared = await shared.acquire(parentSignal); + if (parentSignal?.aborted) throw new Error("Workflow operation was aborted"); // Check after acquiring a slot so queued waves see completed calls' spend. // Already-running calls can still overshoot by one concurrency wave. this.requireRemainingBudget(); @@ -307,6 +451,8 @@ export class WorkflowRuntime { this.activeWriterId = agentId; } startedAt = this.readNow(); + requestEpoch = this.semaphore.epoch; + sharedEpoch = shared.epoch; await this.updateProgress({ agent: { ...identity, status: "running", startedAt }, totalAgents: this.agentCount, @@ -316,6 +462,7 @@ export class WorkflowRuntime { phase_length: normalizedOptions.phase?.length ?? 0, }); for (let attempt = 1; attempt <= retries; attempt += 1) { + schemaAttempt = attempt; if (attempt > 1) { try { this.requireRemainingBudget(); @@ -328,6 +475,7 @@ export class WorkflowRuntime { totalUsage, attempt - 1, errorMessage(error), + rateLimitRetries, ); throw error; } @@ -341,26 +489,130 @@ export class WorkflowRuntime { const relayAbort = (): void => attemptController.abort(parentSignal?.reason); if (parentSignal?.aborted) relayAbort(); else parentSignal?.addEventListener("abort", relayAbort, { once: true }); + if (attemptController.signal.aborted) + throw attemptController.signal.reason ?? new Error("Workflow operation was aborted"); let timeout: ReturnType | undefined; + let runnerPromise: Promise | undefined; + let cancelAttempt: (() => void) | undefined; + let settling: Promise | undefined; + // After cancellation the runner gets a bounded grace to settle. Returns its + // late result so the usage it reports can still be accounted. + const settleRunner = (): Promise => { + settling ??= (async () => { + if (!runnerPromise || !attemptController.signal.aborted) return undefined; + let cleanupTimer: ReturnType | undefined; + try { + const settled = await Promise.race([ + runnerPromise.then( + (value) => ({ value }), + () => ({ value: undefined }), + ), + new Promise((resolve) => { + cleanupTimer = setTimeout(() => resolve(undefined), this.runnerCleanupTimeoutMs); + }), + ]); + if (settled) return settled.value; + // An injected runner violated the cancellation contract. Keep its + // shared slot reserved until it settles; never admit replacement + // work on top of an operation that may still be using the backend. + cleanupIncomplete = true; + const retainedLease = acquireWorkflowAdmission(this.concurrency.ceiling); + const retainedRelease = releaseShared; + releaseShared = undefined; + const done = (): void => { + retainedRelease?.(); + retainedLease.release(); + }; + void runnerPromise.then(done, done); + return undefined; + } finally { + if (cleanupTimer) clearTimeout(cleanupTimer); + } + })(); + return settling; + }; try { - const timedOut = new Promise((_resolve, reject) => { + const cancelled = new Promise((_resolve, reject) => { + cancelAttempt = () => + reject(attemptController.signal.reason ?? new Error("Workflow operation was aborted")); + attemptController.signal.addEventListener("abort", cancelAttempt, { once: true }); + if (attemptController.signal.aborted) cancelAttempt(); timeout = setTimeout(() => { - attemptController.abort(); - reject(new Error(`Workflow agent ${label} timed out after ${this.agentTimeoutMs}ms`)); + timedOut = true; + attemptController.abort( + new Error( + `Workflow agent ${label} timed out after ${this.agentTimeoutMs}ms (execution only; queue time excluded)`, + ), + ); }, this.agentTimeoutMs); }); - raw = await Promise.race([ - this.runner({ - prompt: attemptPrompt, - options: normalizedOptions, - cwd: this.cwd, - signal: attemptController.signal, - runId: this.runId, - agentId, - }), - timedOut, - ]); + runnerInvoked = true; + runnerPromise = this.runner({ + prompt: attemptPrompt, + options: normalizedOptions, + cwd: this.cwd, + signal: attemptController.signal, + runId: this.runId, + agentId, + onToolExecutionStart: (toolName) => { + const effect = classifyWorkflowToolEffect(toolName); + if (effect !== "none") worldAccess = true; + if (effect === "mutate") this.environmentChanged = true; + }, + onModelRequest: (event) => { + if (attemptController.signal.aborted) return; + if (event.type === "started") { + requestEpoch = this.semaphore.epoch; + sharedEpoch = shared.epoch; + } + if (event.type === "succeeded") { + this.semaphore.succeeded(requestEpoch); + shared.succeeded(sharedEpoch); + // The next request in this turn (a resample after leaked tool-call + // markup) has no "started" event of its own. + requestEpoch = this.semaphore.epoch; + sharedEpoch = shared.epoch; + } + if (event.type === "rate_limit") { + rateLimitRetries++; + this.semaphore.throttle(requestEpoch, event.delayMs); + shared.throttle(sharedEpoch, event.delayMs); + requestEpoch = this.semaphore.epoch; + sharedEpoch = shared.epoch; + } + void this.updateProgress({ + concurrency: this.concurrencySnapshot(), + agent: { + ...identity, + status: "running", + startedAt, + rateLimitRetries, + retry: + event.type === "rate_limit" + ? { attempt: event.attempt, maxRetries: event.maxRetries, delayMs: event.delayMs } + : undefined, + }, + }).catch(() => undefined); + }, + }); + raw = await Promise.race([runnerPromise, cancelled]); + if (raw.toolActivityObserved !== true) toolActivityObserved = false; } catch (error: unknown) { + // A thrown attempt cannot vouch that every tool start was reported. + toolActivityObserved = false; + // A timed-out or aborted attempt still spent tokens. The runner reports + // them when it settles, after the race has already been decided. + const late = await settleRunner(); + if (late) { + const lateUsage = normalizeUsage(late.usage); + totalUsage = mergeWorkflowUsage(totalUsage, lateUsage); + try { + this.budget.consume(lateUsage); + } catch (budgetError: unknown) { + if (budgetError instanceof WorkflowBudgetExceeded) this.budgetError ??= budgetError; + } + } + if (!attemptController.signal.aborted) providerFailureKind = thrownErrorKind(error); await this.appendFailure( seq, callHash, @@ -369,11 +621,14 @@ export class WorkflowRuntime { totalUsage, attempt, errorMessage(error), + rateLimitRetries, ); throw error; } finally { if (timeout) clearTimeout(timeout); + if (cancelAttempt) attemptController.signal.removeEventListener("abort", cancelAttempt); parentSignal?.removeEventListener("abort", relayAbort); + await settleRunner(); } const usage = normalizeUsage(raw.usage); totalUsage = mergeWorkflowUsage(totalUsage, usage); @@ -389,11 +644,13 @@ export class WorkflowRuntime { totalUsage, attempt, errorMessage(error), + rateLimitRetries, ); throw error; } if (raw.status === "failed" || raw.status === "aborted") { const failure = raw.errorMessage ?? raw.text ?? `Workflow agent ${label} ${raw.status}`; + providerFailureKind = raw.errorKind; await this.appendFailure( seq, callHash, @@ -402,6 +659,7 @@ export class WorkflowRuntime { totalUsage, attempt, failure, + rateLimitRetries, ); throw new Error(failure); } @@ -419,6 +677,7 @@ export class WorkflowRuntime { totalUsage, attempt, lastErrors.join("; "), + rateLimitRetries, ); throw new WorkflowSchemaError(lastErrors, attempt); } @@ -435,11 +694,15 @@ export class WorkflowRuntime { usage: totalUsage, attempt, createdAt: this.readNow(), + rateLimitRetries, + ...(toolActivityObserved ? { worldAccess } : {}), }); await this.updateProgress({ agent: { ...identity, status: "completed", + rateLimitRetries, + retry: undefined, startedAt, finishedAt: this.readNow(), usageTokens: workflowUsageTokens(totalUsage), @@ -455,29 +718,103 @@ export class WorkflowRuntime { } throw new WorkflowSchemaError(lastErrors, retries); } catch (error: unknown) { + const message = + errorMessage(error) + + (cleanupIncomplete ? "; runner did not settle after cancellation; its shared slot remains reserved" : ""); + const rateLimited = providerFailureKind === "rate_limit"; + // Classify before recording anything. A failed journal or progress write must + // not replace the node's own error, and the marker has to exist before the + // bridge can ask for it, or parallel()/pipeline() reports this node failure a + // second time as a callback failure. + const propagated = error instanceof Error ? error : new Error(errorMessage(error)); + this.nodeErrors.add(propagated); + // Recording is best effort: the node already failed, and a disk error must not + // turn that into a different terminal error for the run. + if (!this.recordedFailures.has(seq)) { + await this.appendFailure( + seq, + callHash, + normalizedPrompt, + normalizedOptions, + totalUsage, + schemaAttempt, + message, + rateLimitRetries, + ); + } + if (rateLimited) { + this.semaphore.throttle(requestEpoch, 1000); + shared.throttle(sharedEpoch, 1000); + } await this.updateProgress({ + concurrency: this.concurrencySnapshot(), agent: { ...identity, status: parentSignal?.aborted ? "aborted" : "failed", + error: { + code: rateLimited + ? rateLimitRetries > 0 + ? "RATE_LIMIT_RETRIES_EXHAUSTED" + : "RATE_LIMITED" + : error instanceof WorkflowSchemaError + ? "SCHEMA_VALIDATION_FAILED" + : timedOut + ? "NODE_TIMEOUT" + : "NODE_FAILED", + message: message.slice(0, 2000), + retryable: rateLimited, + ...(rateLimited ? { retryExhausted: rateLimitRetries > 0 } : {}), + }, + rateLimitRetries, + retry: undefined, startedAt, finishedAt: this.readNow(), usageTokens: workflowUsageTokens(totalUsage), }, spentTokens: this.budget.spent(), - }); + }).catch(() => undefined); if (error instanceof WorkflowBudgetExceeded) this.track("workflow_budget_exceeded", { spent_tokens: error.spentTokens, requested_tokens: error.requestedTokens, }); - throw error; + throw propagated; } finally { + // Without a complete tool report, a live call may have changed anything. + if (runnerInvoked && !toolActivityObserved) this.environmentChanged = true; + releaseShared?.(); release?.(); if (this.activeWriterId === agentId) this.activeWriterId = undefined; } } + /** + * Find a replayable result for this call. Runtime schemas (safeParse objects) + * cannot be hashed, so their calls always run live. A cached result must still + * satisfy the call's schema, and once this run may have changed the + * environment only results that never touched it remain valid. + */ + private takeCached( + callHash: string, + options: WorkflowAgentOptions, + ): { entry: WorkflowJournalEntry; result: unknown } | undefined { + if (isSafeParseSchema(options.schema)) return undefined; + let result: unknown; + const entry = this.journal.getCached(callHash, (candidate) => { + if (this.environmentChanged && candidate.worldAccess !== false) return false; + if (options.schema === undefined) { + result = candidate.result; + return true; + } + const validation = validateWorkflowSchema(options.schema, candidate.result, true); + result = validation.value; + return validation.valid; + }); + return entry ? { entry, result } : undefined; + } + phase(title: string): void { + if (this.lifecycle === "closed") return; const normalized = title.trim().slice(0, 200); if (!normalized) return; this.phases.push({ title: normalized }); @@ -490,6 +827,7 @@ export class WorkflowRuntime { } log(message: string): void { + if (this.lifecycle === "closed") return; const normalized = message.trim().slice(0, 2_000); if (!normalized) return; void this.updateProgress({ message: normalized }).catch(() => undefined); @@ -597,20 +935,46 @@ export class WorkflowRuntime { } private async updateProgress(patch: Partial & { agent?: WorkflowProgressAgent }): Promise { - const snapshot = await this.progress.update(patch); - this.onProgress?.(snapshot); + if (this.lifecycle === "closed") return; + let snapshot: WorkflowProgress; + try { + snapshot = await this.progress.update({ ...patch, concurrency: this.concurrencySnapshot() }); + } catch { + // progress.json is a projection. The in-memory state is already updated when a + // write fails, and a disk error here must not fail a node whose work finished. + snapshot = this.progress.current(); + } + try { + this.onProgress?.(snapshot); + } catch { + // A live-update listener is a consumer of the projection. Its failure must not + // change run accounting, or one node failure is reported twice. + } + } + + private concurrencySnapshot(): NonNullable { + return { + ...this.concurrency, + cap: Math.min(this.semaphore.max, this.admission?.semaphore.max ?? this.semaphore.max), + sharedActive: this.admission?.semaphore.active ?? 0, + }; } private host(depth: number): WorkflowVmHost { return { + isNodeFailure: (error) => + error !== null && + typeof error === "object" && + (this.nodeErrors.has(error) || + (error as { __workflowNodeFailure?: boolean }).__workflowNodeFailure === true), agent: (prompt, options) => this.agent(prompt, options as WorkflowAgentOptions), phase: (title) => this.phase(title), log: (message) => this.log(message), - iterate: (options) => this.iterate(options), + iterate: (options) => this.trackOperation(() => this.iterate(options)), nestedWorkflow: (name, args) => { if (depth >= 1) return Promise.reject(new Error("Nested workflow depth is limited to one level")); if (!this.nestedWorkflow) return Promise.reject(new Error(`Nested workflow "${name}" is not available`)); - return this.nestedWorkflow(name, args, this, depth); + return this.trackOperation(() => this.nestedWorkflow!(name, args, this, depth)); }, budgetSpent: () => this.budget.spent(), budgetRemaining: () => this.budget.remaining(), @@ -713,23 +1077,32 @@ export class WorkflowRuntime { usage: WorkflowUsage, attempt: number, error: string, + rateLimitRetries = 0, ): Promise { - await this.journal.append({ - schemaVersion: 1, - seq, - callId: `${this.runId}-${seq + 1}`, - callHash, - prompt, - options: workflowJsonValue(serializableOptions(options)), - status: "failed", - usage, - attempt, - createdAt: this.readNow(), - error: error.slice(0, 2_000), - }); + // Recording a failure is best effort. The node already failed, so a journal + // write error must never replace the node's own error, change the run's + // terminal status, or skip failure classification. + await this.journal + .append({ + schemaVersion: 1, + seq, + callId: `${this.runId}-${seq + 1}`, + callHash, + prompt, + options: workflowJsonValue(serializableOptions(options)), + status: this.signal?.aborted ? "aborted" : "failed", + usage, + attempt, + rateLimitRetries, + createdAt: this.readNow(), + error: error.slice(0, 2_000), + }) + .catch(() => undefined); + this.recordedFailures.add(seq); } private track(event: StepTelemetryKnownEventName, properties: Record): void { + if (this.lifecycle === "closed") return; if (this.telemetry) trackStepTelemetry(this.telemetry, event, properties as StepTelemetryProperties); const record = { schemaVersion: 1 as const, @@ -840,8 +1213,25 @@ function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error); } -export function workflowToolResult(value: T): AgentToolResult { - return { content: [{ type: "text", text: JSON.stringify(value, null, 2) }], details: value }; +export function workflowToolResult(value: WorkflowRunResult): AgentToolResult { + return { + content: [{ type: "text", text: JSON.stringify(value, null, 2) }], + details: value, + isError: value.status !== "completed", + }; } export { WorkflowBudgetExceeded }; + +/** A runner may throw an error that carries the provider's structured kind. */ +function thrownErrorKind(error: unknown): WorkflowAgentRunResult["errorKind"] { + if (!error || typeof error !== "object" || !("errorKind" in error)) return undefined; + const kind = (error as { errorKind?: unknown }).errorKind; + return kind === "rate_limit" || + kind === "quota" || + kind === "auth" || + kind === "transient" || + kind === "invalid_request" + ? kind + : undefined; +} diff --git a/packages/coding-agent/src/features/workflow/schema.ts b/packages/coding-agent/src/features/workflow/schema.ts index 6218a0f9..e199cc56 100644 --- a/packages/coding-agent/src/features/workflow/schema.ts +++ b/packages/coding-agent/src/features/workflow/schema.ts @@ -40,7 +40,7 @@ interface SafeParseSchema { safeParse(input: unknown): { success: boolean; data?: unknown; error?: unknown }; } -function isSafeParseSchema(value: unknown): value is SafeParseSchema { +export function isSafeParseSchema(value: unknown): value is SafeParseSchema { return !!value && typeof value === "object" && typeof (value as { safeParse?: unknown }).safeParse === "function"; } diff --git a/packages/coding-agent/src/features/workflow/step-workflow.ts b/packages/coding-agent/src/features/workflow/step-workflow.ts index 3abfd8ab..1acdd884 100644 --- a/packages/coding-agent/src/features/workflow/step-workflow.ts +++ b/packages/coding-agent/src/features/workflow/step-workflow.ts @@ -10,6 +10,7 @@ import type { import { resolveStepStorageRoot } from "../../step/storage-root.ts"; import type { StepTelemetryReporter } from "../../step/telemetry.ts"; import { createDefaultWorkflowAgentRunner } from "./agent-runner.ts"; +import { configuredWorkflowConcurrencyLimit } from "./budget.ts"; import { createWorkflowRunPaths, newWorkflowRunId, resolveWorkflowRoot, WorkflowJournal } from "./journal.ts"; import { formatWorkflowStatus, listSavedWorkflows, listWorkflowRuns } from "./progress.ts"; import { resolveWorkflowRegistration } from "./registration-gate.ts"; @@ -30,7 +31,9 @@ export const WorkflowParams = Type.Object({ scriptPath: Type.Optional(Type.String({ description: "Path to a JavaScript workflow script" })), name: Type.Optional(Type.String({ description: "Saved workflow name" })), args: Type.Optional(Type.Unknown({ description: "JSON arguments exposed as the script's args value" })), - resumeFromRunId: Type.Optional(Type.String({ description: "Resume the matching cached journal prefix" })), + resumeFromRunId: Type.Optional( + Type.String({ description: "Replay completed calls with unchanged prompt and options from this run" }), + ), budget: Type.Optional(Type.Integer({ minimum: 0, description: "Optional input+output token budget" })), maxConcurrency: Type.Optional(Type.Integer({ minimum: 1, maximum: 32 })), agentTimeoutMs: Type.Optional(Type.Integer({ minimum: 1_000, maximum: 3_600_000 })), @@ -174,7 +177,7 @@ Mechanics: - agent(prompt, {schema}) forces structured JSON output and retries schema mismatches up to 3 attempts, feeding validation errors back; failed attempts still consume budget. - Give agent() a short descriptive label; the live tool row shows running/queued counts, assigned tasks, phases, and terminal states while the workflow executes. - Budgets fail closed once spend crosses the limit; budget.total is null when unlimited, so guard adaptive waves with budget.total && budget.remaining() > estimate. A "+500k"-style token target in the user's current message becomes the run's default budget; an explicit budget parameter overrides it. -- Resume: resumeFromRunId with the same script and args is a 100% cache hit on the unchanged prefix; after the first callHash mismatch, everything after it re-runs live. The tool result includes runId and the persisted scriptPath (.stepcode/workflows/runs//script.js) — edit that file and re-invoke with scriptPath to iterate without resending the source. +- Resume: resumeFromRunId replays completed calls whose callHash (prompt and options) is unchanged, in any order; changed, new, and previously failed calls run live. Once a live call in the resumed run writes files, runs a shell command, or uses an unrecognized tool, only cached calls that used no file, shell, or network tools still replay. The tool result includes runId and the persisted scriptPath (.stepcode/workflows/runs//script.js) — edit that file and re-invoke with scriptPath to iterate without resending the source. - Determinism: Date, new Date(), and Math.random() throw inside the VM. Pass timestamps via args; vary prompts by index, not randomness. - iterate({spec, ...}) runs a read-only Planner → single-writer Developer → independent read-only QA loop with schema-guarded evidence and stops on coverage target, stagnation, max iterations, empty objective, or budget. Use it for spec-driven build loops; hand-roll agent() with readOnly/writable mounts when roles or stop conditions differ. - args takes real JSON values, not JSON-encoded strings, and is exposed as the script's args global. @@ -191,7 +194,7 @@ Mechanics: "parallel(tasks) is a barrier: it awaits every task function before returning; a task that throws resolves to null in the result array, so .filter(Boolean) the results.", "Pass schema to agent() whenever a later stage consumes the result; mismatches are retried up to 3 attempts with validation errors appended, then the call fails.", "Budgets fail closed; budget.total is null when unlimited, so guard adaptive extra waves with budget.total && budget.remaining() > estimate. A \"+500k\"-style token target in the user's message sets the run's default budget; an explicit budget parameter overrides it.", - "Resume with resumeFromRunId plus the identical script and args to replay the cached journal prefix; the first changed call and everything after it re-run live. The tool result's scriptPath points at the persisted script copy — edit it and re-invoke with scriptPath to iterate.", + "Resume with resumeFromRunId to replay completed calls whose prompt and options are unchanged, in any order; changed, new, and previously failed calls run live, and after a live call writes or runs a shell command, cached calls that read the environment run live too. The tool result's scriptPath points at the persisted script copy — edit it and re-invoke with scriptPath to iterate.", "The VM throws on Date, new Date(), and Math.random() and exposes no process, require, or network; pass timestamps through args and vary prompts by index.", "Use iterate({spec, ...}) for the Planner → single-writer Developer → independent QA loop with built-in coverage, stagnation, max-iteration, empty-objective, and budget stops; hand-roll agent() with readOnly/writable mounts when roles or stop conditions differ.", ], @@ -228,6 +231,7 @@ Mechanics: telemetry: options.telemetry, budgetTotal: request.budget ?? options.turnState?.budgetTotal ?? options.budgetTotal, maxConcurrency: request.maxConcurrency ?? options.maxConcurrency, + providerConcurrencyLimit: configuredWorkflowConcurrencyLimit(), maxAgents: options.maxAgents, agentTimeoutMs: request.agentTimeoutMs ?? options.agentTimeoutMs, context: { ...ctx, signal: controller.signal }, @@ -247,6 +251,7 @@ Mechanics: }); return workflowToolResult(result); } finally { + controller.abort(); activeRuns.delete(controller); if (signal) signal.removeEventListener("abort", abort); } diff --git a/packages/coding-agent/src/features/workflow/tool-profile.ts b/packages/coding-agent/src/features/workflow/tool-profile.ts index 5264a6d5..7247a63b 100644 --- a/packages/coding-agent/src/features/workflow/tool-profile.ts +++ b/packages/coding-agent/src/features/workflow/tool-profile.ts @@ -132,6 +132,20 @@ const WRITE_TOOL_NAMES = new Set(["write_file", "edit_file", "write", "edit"]); const READ_TOOL_NAMES = new Set(["read_file", "find_files", "search_files", "list_directory", "read"]); const EXECUTE_TOOL_NAMES = new Set(["run_command", "bash", "powershell"]); const NON_FILESYSTEM_TOOL_NAMES = new Set(["clarify_user", "find_tools"]); +const NETWORK_READ_TOOL_NAMES = new Set(["search_web"]); + +export type WorkflowToolEffect = "none" | "read" | "mutate"; + +/** + * Classify a tool's effect on the environment for resume decisions. Shell + * commands and unrecognized tools (MCP, nested agents) count as mutating + * because their effects cannot be established from the tool name. + */ +export function classifyWorkflowToolEffect(toolName: string): WorkflowToolEffect { + if (NON_FILESYSTEM_TOOL_NAMES.has(toolName)) return "none"; + if (READ_TOOL_NAMES.has(toolName) || NETWORK_READ_TOOL_NAMES.has(toolName)) return "read"; + return "mutate"; +} /** * Inspect a child tool call before it crosses the workflow ACL boundary. diff --git a/packages/coding-agent/src/features/workflow/types.ts b/packages/coding-agent/src/features/workflow/types.ts index a826b4b0..c12bdec0 100644 --- a/packages/coding-agent/src/features/workflow/types.ts +++ b/packages/coding-agent/src/features/workflow/types.ts @@ -52,6 +52,7 @@ export interface WorkflowAgentOptions { } export interface WorkflowAgentRunInput { + onModelRequest?: (event: import("../../core/rate-limit.ts").ModelRequestProgress) => void; prompt: string; options: WorkflowAgentOptions; cwd: string; @@ -59,6 +60,8 @@ export interface WorkflowAgentRunInput { /** Stable run/agent identifiers useful to injected runners and ACL hooks. */ runId: string; agentId: string; + /** Reports each tool the agent starts, before the tool runs. */ + onToolExecutionStart?: (toolName: string) => void; } export interface WorkflowAgentRunResult { @@ -69,7 +72,11 @@ export interface WorkflowAgentRunResult { usage?: Partial; status?: "completed" | "failed" | "aborted"; errorMessage?: string; + /** Structured provider failure class for a failed run; never inferred from text. */ + errorKind?: import("@step-harness/providers").ProviderErrorKind; model?: string; + /** True when every tool start was reported through onToolExecutionStart. */ + toolActivityObserved?: boolean; } export type WorkflowAgentRunner = (input: WorkflowAgentRunInput) => Promise; @@ -87,6 +94,9 @@ export interface WorkflowMeta { } export interface WorkflowProgressAgent { + error?: { code: string; message: string; retryable: boolean; retryExhausted?: boolean }; + rateLimitRetries?: number; + retry?: { attempt: number; maxRetries: number; delayMs: number }; id: string; label: string; /** Bounded summary of the assigned prompt, independent of an optional short label. */ @@ -99,10 +109,12 @@ export interface WorkflowProgressAgent { } export interface WorkflowProgress { + taskFailures?: WorkflowTaskFailure[]; schemaVersion: 1; runId: string; name: string; - status: "running" | "completed" | "failed" | "aborted" | "budget_exceeded"; + status: "running" | "stopping" | "completed" | "partial_failed" | "failed" | "aborted" | "budget_exceeded"; + concurrency?: { requested?: number; ceiling: number; limit: number; cap: number; sharedActive?: number }; startedAt: number; updatedAt: number; currentPhase?: string; @@ -118,7 +130,14 @@ export interface WorkflowRunResult { schemaVersion: 1; runId: string; name: string; - status: "completed" | "failed" | "aborted"; + status: "completed" | "partial_failed" | "failed" | "aborted"; + summary?: { total: number; completed: number; failed: number; failedCallbacks?: number }; + /** Callback failures can happen before agent() is reached; they are not extra agent nodes. */ + taskFailures?: WorkflowTaskFailure[]; + /** Node execution includes model/tool/retry/shutdown time; not just tool-reported duration. */ + timing?: { runElapsedMs: number; maxNodeExecutionMs: number }; + failures?: WorkflowProgressAgent[]; + concurrency?: WorkflowProgress["concurrency"]; value: unknown; meta: WorkflowMeta; /** Persisted copy of the executed script; edit and re-invoke with scriptPath to iterate. */ @@ -132,6 +151,14 @@ export interface WorkflowRunResult { stopReason?: string; } +export interface WorkflowTaskFailure { + script?: string; + primitive: "parallel" | "pipeline"; + index: number; + stage?: number; + message: string; +} + export interface IterateOptions { spec: string; maxIterations?: number; @@ -171,12 +198,19 @@ export interface WorkflowJournalEntry { callHash: string; prompt: string; options: WorkflowJsonValue; - status: "completed" | "failed" | "cached"; + status: "completed" | "failed" | "aborted" | "cached"; result?: unknown; usage: WorkflowUsage; attempt: number; + /** Request-level 429 retries across this node's schema attempts. */ + rateLimitRetries?: number; createdAt: number; error?: string; + /** + * Whether the call used any tool that reads or changes the workspace or the + * network. Absent when the runner did not report tool activity. + */ + worldAccess?: boolean; } export interface WorkflowTelemetryRecord { diff --git a/packages/coding-agent/src/features/workflow/vm.ts b/packages/coding-agent/src/features/workflow/vm.ts index a8ed51d2..c5deedce 100644 --- a/packages/coding-agent/src/features/workflow/vm.ts +++ b/packages/coding-agent/src/features/workflow/vm.ts @@ -44,7 +44,7 @@ import type { QuickJSRuntime, QuickJSWASMModule, } from "quickjs-emscripten-core"; -import type { WorkflowJsonSchema, WorkflowMeta } from "./types.ts"; +import type { WorkflowJsonSchema, WorkflowMeta, WorkflowTaskFailure } from "./types.ts"; const MAX_SCRIPT_BYTES = 128 * 1024; const DEFAULT_MEMORY_LIMIT_MB = 64; @@ -52,6 +52,8 @@ const DEFAULT_TIMEOUT_MS = 120_000; /** Host primitives the guest prelude exposes as `agent`, `phase`, `log`, `iterate`, `workflow` and `budget`. */ export interface WorkflowVmHost { + /** True only for failures already recorded as agent-node failures by the owner. */ + isNodeFailure?(error: unknown): boolean; agent(prompt: string, options: Record): Promise; phase(title: string): void; log(message: string): void; @@ -63,6 +65,7 @@ export interface WorkflowVmHost { } export interface WorkflowVmOptions { + signal?: AbortSignal; filename?: string; memoryLimitMb?: number; timeoutMs?: number; @@ -72,6 +75,7 @@ export interface WorkflowVmOptions { export interface WorkflowVmResult { value: unknown; meta: WorkflowMeta; + taskFailures?: WorkflowTaskFailure[]; } /** @@ -118,6 +122,7 @@ export async function runInQuickJs( const sourceBytes = Buffer.byteLength(script, "utf8"); if (sourceBytes > MAX_SCRIPT_BYTES) throw new Error(`Workflow script exceeds ${MAX_SCRIPT_BYTES} bytes`); const quickjs = await loadQuickJsModule(); + if (options.signal?.aborted) throw new Error("Workflow operation was aborted"); const timeoutMs = clampTimeout(options.timeoutMs); const runtime = quickjs.newRuntime(); runtime.setMemoryLimit(clampMemory(options.memoryLimitMb) * 1024 * 1024); @@ -159,6 +164,8 @@ export async function runInQuickJs( const timeoutPromise = new Promise((_resolve, reject) => { rejectTimeout = reject; }); + const onAbort = (): void => rejectTimeout?.(new Error("Workflow operation was aborted")); + options.signal?.addEventListener("abort", onAbort, { once: true }); const timeoutError = (): Error => new Error(`Workflow script timed out after ${timeoutMs}ms`); const checkTimeout = (): void => { if (hostCalls.pending > 0) { @@ -210,8 +217,11 @@ export async function runInQuickJs( } const json = context.getString(settled.value); settled.value.dispose(); + if (hostCalls.pending > 0) + throw new Error("Workflow script returned with unawaited agent calls or workflow/iterate operations"); return readResult(json); } finally { + options.signal?.removeEventListener("abort", onAbort); if (timeout) clearTimeout(timeout); hostCalls.onSettled = undefined; hostCalls.refreshDeadline = undefined; @@ -253,8 +263,12 @@ function readResult(json: string): WorkflowVmResult { if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { return { value: parsed ?? null, meta: {} }; } - const record = parsed as { value?: unknown; meta?: unknown }; - return { value: record.value ?? null, meta: normalizeMeta(record.meta) }; + const record = parsed as { value?: unknown; meta?: unknown; taskFailures?: WorkflowTaskFailure[] }; + return { + value: record.value ?? null, + meta: normalizeMeta(record.meta), + ...(record.taskFailures?.length ? { taskFailures: record.taskFailures } : {}), + }; } /** @@ -267,11 +281,12 @@ function toHostError(context: QuickJSContext, handle: QuickJSHandle, override?: handle.dispose(); if (override) return override; if (dumped && typeof dumped === "object") { - const record = dumped as { name?: unknown; message?: unknown; stack?: unknown }; + const record = dumped as { name?: unknown; message?: unknown; stack?: unknown; __workflowNodeFailure?: boolean }; const message = typeof record.message === "string" ? record.message : JSON.stringify(dumped); const error = new Error(message); if (typeof record.name === "string") error.name = record.name; if (typeof record.stack === "string") error.stack = `${record.name ?? "Error"}: ${message}\n${record.stack}`; + if (record.__workflowNodeFailure === true) Object.assign(error, { __workflowNodeFailure: true }); return error; } return new Error(typeof dumped === "string" ? dumped : String(dumped)); @@ -295,7 +310,10 @@ function installHostBridge( hostCalls.live.add(deferred); hostCalls.pending += 1; void Promise.resolve() - .then(() => operation(values)) + .then(() => { + if (hostCalls.closed) throw new Error("Workflow scope is closed"); + return operation(values); + }) .then( (result) => { // The run may already have returned without awaiting this call; the @@ -309,6 +327,7 @@ function installHostBridge( if (hostCalls.closed) return; const message = error instanceof Error ? error.message : String(error); const encoded = context.newError(message); + if (host.isNodeFailure?.(error)) context.setProp(encoded, "__workflowNodeFailure", context.true); deferred.reject(encoded); encoded.dispose(); }, @@ -427,16 +446,26 @@ for (const name of [ } const __workflow_encode = (value) => JSON.stringify(value === undefined ? null : value); const __workflow_decode = (json) => (typeof json === "string" ? JSON.parse(json) : null); +const __workflow_task_failures = []; globalThis.args = __workflow_decode(__workflow_args_raw); globalThis.agent = async (prompt, options = {}) => __workflow_decode(await __workflow_agent_ref(String(prompt === undefined ? "" : prompt), __workflow_encode(options))); globalThis.iterate = async (options) => __workflow_decode(await __workflow_iterate_ref(__workflow_encode(options || {}))); globalThis.workflow = async (name, options = null) => __workflow_decode(await __workflow_nested_ref(String(name === undefined ? "" : name), __workflow_encode(options))); globalThis.parallel = async (tasks) => { - if (!Array.isArray(tasks)) throw new TypeError("parallel() requires an array of task functions"); + if (!Array.isArray(tasks)) throw new TypeError("parallel() requires an array of functions or promises"); if (tasks.length > 4096) throw new RangeError("parallel() accepts at most 4096 tasks"); - return Promise.all(tasks.map(async (task) => { - if (typeof task !== "function") throw new TypeError("parallel() entries must be functions"); - try { return await task(); } catch { return null; } + // Validate the whole batch before invoking any lazy task. Eager agent() + // promises are already scheduled; their cancellation belongs to the run scope. + for (const task of tasks) { + if (typeof task !== "function" && !(task !== null && typeof task === "object" && typeof task.then === "function")) { + throw new TypeError("parallel() entries must be functions or promises; use parallel(items.map(item => () => agent(String(item))))"); + } + } + return Promise.all(tasks.map(async (task, index) => { + try { return await (typeof task === "function" ? task() : task); } catch (error) { + if (!error || error.__workflowNodeFailure !== true) __workflow_task_failures.push({primitive: "parallel", index, message: String(error).slice(0, 2000)}); + return null; + } })); }; globalThis.pipeline = async (items, ...stages) => { @@ -447,8 +476,11 @@ globalThis.pipeline = async (items, ...stages) => { } return Promise.all(items.map(async (item, index) => { let value = item; - for (const stage of stages) { - try { value = await stage(value, item, index); } catch { return null; } + for (let stage = 0; stage < stages.length; stage++) { + try { value = await stages[stage](value, item, index); } catch (error) { + if (!error || error.__workflowNodeFailure !== true) __workflow_task_failures.push({primitive: "pipeline", index, stage, message: String(error).slice(0, 2000)}); + return null; + } } return value; })); @@ -467,6 +499,7 @@ const __workflow_main = Object.getPrototypeOf(async function() {}).constructor($ return __workflow_main().then((__workflow_value) => JSON.stringify({ value: __workflow_value === undefined ? null : __workflow_value, meta: globalThis.__workflow_meta || {}, + taskFailures: __workflow_task_failures, })); })() `; diff --git a/packages/coding-agent/src/step/system-prompt.ts b/packages/coding-agent/src/step/system-prompt.ts index 91a22789..20c13129 100644 --- a/packages/coding-agent/src/step/system-prompt.ts +++ b/packages/coding-agent/src/step/system-prompt.ts @@ -278,7 +278,7 @@ export function buildStepSystemPromptAppendix( "- iterate({spec, maxIterations, stopWhenSpecCoverage}) runs the HoH loop: a read-only Planner, a single-writer Developer, and an independent read-only QA, with role boundaries enforced by toolProfile and readOnly/writable mounts; it stops on spec coverage, stagnation, max iterations, an empty objective, or exhausted budget.", "- Pass a JSON schema to agent() whenever a later stage consumes the result; output is forced to structured JSON and mismatches are retried up to 3 times before the call fails.", "- Budgets fail closed once exhausted and budget.total is null when no limit is set, so guard scale decisions with budget.total && budget.remaining() > n. Report every drop with log() — top-N truncations, skipped retries, sampling — never cap silently.", - "- Resume with resumeFromRunId: the same script and args replay the unchanged call prefix as a 100% cache hit; the first mismatched call and everything after it re-run live.", + "- Resume with resumeFromRunId: completed calls with unchanged prompt and options replay from the journal in any order; changed, new, and previously failed calls run live. After a live call writes files or runs a shell command, cached calls that read the environment run live too.", "- Scripts must be deterministic: Date, new Date(), and Math.random() throw in the isolated runtime, and process, require, and network access are unavailable. Pass timestamps through args and vary prompts by index.", "- Default to a medium-sized run and stay under roughly 15 agents unless the user asks for scale or ultraloop is on for the session.", ].join("\n"), diff --git a/packages/coding-agent/src/utils/sleep.ts b/packages/coding-agent/src/utils/sleep.ts index 948f93c4..1c254b06 100644 --- a/packages/coding-agent/src/utils/sleep.ts +++ b/packages/coding-agent/src/utils/sleep.ts @@ -1,18 +1,32 @@ /** * Sleep helper that respects abort signal. */ -export function sleep(ms: number, signal?: AbortSignal): Promise { +export async function sleep(ms: number, signal?: AbortSignal): Promise { + // Node turns delays larger than int32 into a 1ms timer. A long Retry-After + // must wait (or be cancelled), rather than accidentally hammering the server. + let remaining = ms; + do { + const delay = Math.min(2_147_483_647, remaining); + await sleepChunk(delay, signal); + remaining -= delay; + } while (remaining > 0); +} + +function sleepChunk(ms: number, signal?: AbortSignal): Promise { return new Promise((resolve, reject) => { if (signal?.aborted) { reject(new Error("Aborted")); return; } - const timeout = setTimeout(resolve, ms); - - signal?.addEventListener("abort", () => { + const onAbort = () => { clearTimeout(timeout); reject(new Error("Aborted")); - }); + }; + const timeout = setTimeout(() => { + signal?.removeEventListener("abort", onAbort); + resolve(); + }, ms); + signal?.addEventListener("abort", onAbort, { once: true }); }); } diff --git a/packages/coding-agent/test/agent-session-retry.test.ts b/packages/coding-agent/test/agent-session-retry.test.ts index b3847df9..f8a45521 100644 --- a/packages/coding-agent/test/agent-session-retry.test.ts +++ b/packages/coding-agent/test/agent-session-retry.test.ts @@ -25,6 +25,8 @@ class MockAssistantStream extends EventStream): AssistantMessage { return { role: "assistant", @@ -71,25 +73,50 @@ describe("AgentSession retry", () => { async function createSession(options?: { failCount?: number; maxRetries?: number; + rateLimitMaxRetries?: number; + errors?: Array>; + toolBeforeFailure?: boolean; delayAssistantMessageEndMs?: number; }) { const failCount = options?.failCount ?? 1; const maxRetries = options?.maxRetries ?? 3; const delayAssistantMessageEndMs = options?.delayAssistantMessageEndMs ?? 0; let callCount = 0; + let toolExecutions = 0; + const tool: AgentTool = { + name: "write_once", + label: "Write", + description: "Record a side effect", + parameters: Type.Object({}), + execute: async () => { + toolExecutions++; + return { content: [{ type: "text", text: "written" }], details: null }; + }, + }; const model = stepModel(); const agent = new Agent({ getApiKey: () => "test-key", - initialState: { model, systemPrompt: "Test", tools: [] }, + initialState: { model, systemPrompt: "Test", tools: options?.toolBeforeFailure ? [tool] : [] }, streamFn: () => { callCount++; const stream = new MockAssistantStream(); queueMicrotask(() => { - if (callCount <= failCount) { + if (options?.toolBeforeFailure && callCount === 1) { + const message = createAssistantMessage("", { + content: [{ type: "toolCall", id: "write-1", name: "write_once", arguments: {} }], + stopReason: "toolUse", + }); + stream.push({ type: "done", reason: "toolUse", message }); + return; + } + const failureIndex = callCount - (options?.toolBeforeFailure ? 1 : 0); + if (options?.errors ? failureIndex <= options.errors.length : failureIndex <= failCount) { + const failure = options?.errors?.[failureIndex - 1] ?? "overloaded_error"; const msg = createAssistantMessage("", { stopReason: "error", - errorMessage: "overloaded_error", + ...(typeof failure === "string" ? { errorMessage: failure } : failure), + retryAfterMs: 0, }); stream.push({ type: "start", partial: msg }); stream.push({ type: "error", reason: "error", error: msg }); @@ -108,7 +135,9 @@ describe("AgentSession retry", () => { const authStorage = AuthStorage.create(join(tempDir, "auth.json")); const modelRegistry = await createModelRegistry(authStorage, tempDir); await authStorage.modify("step", async () => ({ type: "api_key", key: "test-key" })); - settingsManager.applyOverrides({ retry: { enabled: true, maxRetries, baseDelayMs: 1 } }); + settingsManager.applyOverrides({ + retry: { enabled: true, maxRetries, rateLimitMaxRetries: options?.rateLimitMaxRetries, baseDelayMs: 1 }, + }); session = new AgentSession({ agent, @@ -117,6 +146,7 @@ describe("AgentSession retry", () => { cwd: tempDir, modelRuntime: getModelRuntime(modelRegistry), resourceLoader: createTestResourceLoader(), + ...(options?.toolBeforeFailure ? { baseToolsOverride: { write_once: tool } } : {}), }); if (delayAssistantMessageEndMs > 0) { @@ -130,7 +160,7 @@ describe("AgentSession retry", () => { }; } - return { session, getCallCount: () => callCount }; + return { session, getCallCount: () => callCount, getToolExecutions: () => toolExecutions }; } it("retries after a transient error and succeeds", async () => { @@ -148,6 +178,67 @@ describe("AgentSession retry", () => { expect(created.session.isRetrying).toBe(false); }); + it("uses independent rate-limit and transient retry budgets, resetting after success", async () => { + const created = await createSession({ + maxRetries: 1, + rateLimitMaxRetries: 2, + errors: [ + "503 overloaded_error", + { errorMessage: "429 concurrency reached, current: 9, limit: 8", errorKind: "rate_limit" }, + { errorMessage: "opaque gateway text", errorKind: "rate_limit" }, + ], + }); + const retries: unknown[] = []; + created.session.subscribe((event) => { + if (event.type === "auto_retry_start") retries.push(event); + }); + await created.session.prompt("Test"); + expect(created.getCallCount()).toBe(4); + expect(retries).toMatchObject([ + { attempt: 1, maxAttempts: 1 }, + { attempt: 1, maxAttempts: 2, reason: "rate_limit", delayMs: 0 }, + { attempt: 2, maxAttempts: 2, reason: "rate_limit", delayMs: 0 }, + ]); + expect(created.session.retryAttempt).toBe(0); + }); + + it("stops rate-limit retries at their own limit", async () => { + const created = await createSession({ + maxRetries: 10, + rateLimitMaxRetries: 1, + errors: [THROTTLED, THROTTLED, THROTTLED], + }); + await created.session.prompt("Test"); + expect(created.getCallCount()).toBe(2); + }); + + it.each(["429 insufficient_quota", '429: {"type":"FreeUsageLimitError"}', "503 service unavailable"])( + "does not retry a provider-classified quota error even when its text reads %s", + async (errorMessage) => { + const created = await createSession({ errors: [{ errorMessage, errorKind: "quota" }] }); + await created.session.prompt("Test"); + expect(created.getCallCount()).toBe(1); + }, + ); + + it("cancels a scheduled 429 retry without making another request", async () => { + const created = await createSession({ errors: [THROTTLED] }); + created.session.subscribe((event) => { + if (event.type === "auto_retry_start") created.session.abortRetry(); + }); + await created.session.prompt("Test"); + expect(created.getCallCount()).toBe(1); + expect(created.session.retryAttempt).toBe(0); + }); + + it("retries only the failed model request after a completed side effect", async () => { + const created = await createSession({ toolBeforeFailure: true, errors: [THROTTLED] }); + await created.session.prompt("Write then summarize"); + expect(created.getCallCount()).toBe(3); + expect(created.getToolExecutions()).toBe(1); + expect(created.session.messages.filter((message) => message.role === "toolResult")).toHaveLength(1); + }); + it("exhausts max retries and emits failure", async () => { const created = await createSession({ failCount: 99, maxRetries: 2 }); const events: string[] = []; diff --git a/packages/coding-agent/test/rate-limit.test.ts b/packages/coding-agent/test/rate-limit.test.ts new file mode 100644 index 00000000..21dba30d --- /dev/null +++ b/packages/coding-agent/test/rate-limit.test.ts @@ -0,0 +1,28 @@ +import { afterEach, expect, test, vi } from "vitest"; +import { rateLimitDelay } from "../src/core/rate-limit.ts"; +import { sleep } from "../src/utils/sleep.ts"; + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +test("429 delay honors server hints instead of capping them to local backoff", () => { + vi.spyOn(Math, "random").mockReturnValue(0.5); + expect(rateLimitDelay(1)).toBe(875); + expect(rateLimitDelay(2)).toBe(1750); + expect(rateLimitDelay(20)).toBeLessThanOrEqual(30_000); + expect(rateLimitDelay(1, 90_000)).toBe(90_000); +}); + +test("a long Retry-After does not overflow into an immediate retry and remains cancellable", async () => { + vi.useFakeTimers(); + const controller = new AbortController(); + const waiting = sleep(3_000_000_000, controller.signal); + const rejected = expect(waiting).rejects.toThrow("Aborted"); + await vi.advanceTimersByTimeAsync(1000); + expect(vi.getTimerCount()).toBe(1); + controller.abort(); + await rejected; + expect(vi.getTimerCount()).toBe(0); +}); diff --git a/packages/coding-agent/test/step-subagent-events.test.ts b/packages/coding-agent/test/step-subagent-events.test.ts index 3595e726..0587a5e9 100644 --- a/packages/coding-agent/test/step-subagent-events.test.ts +++ b/packages/coding-agent/test/step-subagent-events.test.ts @@ -1,8 +1,8 @@ import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import os, { tmpdir } from "node:os"; import { join } from "node:path"; import type { AgentToolResult } from "@step-harness/agent-core"; -import { expect, test } from "vitest"; +import { expect, test, vi } from "vitest"; import type { ExtensionAPI, ExtensionContext, ToolDefinition } from "../src/core/extensions/types.ts"; import { createStepSubagentExtension, @@ -21,6 +21,31 @@ interface SentMessage { triggerTurn?: boolean; } +test("RPC forwards 429 retry and recovery signals without treating errors as successes", () => { + const progress: unknown[] = []; + const events = [ + { + type: "auto_retry_start", + reason: "rate_limit", + attempt: 2, + maxAttempts: 3, + delayMs: 2000, + errorMessage: "429 concurrency reached", + }, + { type: "message_end", message: { role: "assistant", stopReason: "error" } }, + { type: "turn_start" }, + { type: "message_end", message: { role: "assistant", stopReason: "toolUse" } }, + { type: "message_end", message: { role: "toolResult" } }, + ]; + for (const event of events) + routeSubagentRpcLine(JSON.stringify(event), { onModelRequest: (value) => progress.push(value) }); + expect(progress).toEqual([ + { type: "rate_limit", attempt: 2, maxRetries: 3, delayMs: 2000, message: "429 concurrency reached" }, + { type: "started" }, + { type: "succeeded" }, + ]); +}); + function textResult(value: string): StepSubagentRunResult { return { messages: [ @@ -202,6 +227,49 @@ test("accepts the descriptive general-purpose name as an alias for the built-in expect(result.content[0]).toMatchObject({ type: "text", text: expect.stringContaining("alias output") }); }); +test("a ten-task batch starts eight and queues two on a ten-core host", async () => { + const cpu = os.cpus()[0]!; + const spy = vi.spyOn(os, "cpus").mockReturnValue(Array(10).fill(cpu)); + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + try { + const { api, tools } = createApi(); + let calls = 0; + const counts: Array<{ running: number; queued: number }> = []; + createStepSubagentExtension({ + agentDir: "/tmp/step-agent-test", + runner: async () => { + calls++; + await gate; + return textResult("ok"); + }, + })(api); + const result = tools.get("subagent")!.execute( + "ten", + { tasks: Array.from({ length: 10 }, () => ({ agent: "general", task: "test" })) } as never, + undefined, + (update) => { + const records = (update.details as { results: Array<{ status: string }> }).results; + counts.push({ + running: records.filter((r) => r.status === "running").length, + queued: records.filter((r) => r.status === "queued").length, + }); + }, + createContext("/workspace"), + ); + await waitFor(() => calls === 8); + expect(counts).toContainEqual({ running: 8, queued: 2 }); + release(); + await result; + expect(calls).toBe(10); + } finally { + release(); + spy.mockRestore(); + } +}); + test("an exact custom agent name takes precedence over a descriptive alias", () => { const custom: StepAgentConfig = { name: "general-purpose", @@ -254,6 +322,7 @@ test.each(["single", "parallel"])( ); expect(confirmations).toBe(1); expect(runs).toBe(0); + expect(result.isError).toBe(true); expect(result.content[0]).toMatchObject({ type: "text", text: "Canceled: project-local agents not approved.", diff --git a/packages/coding-agent/test/subagent-chain-result.test.ts b/packages/coding-agent/test/subagent-chain-result.test.ts index 66a9158c..e16533cd 100644 --- a/packages/coding-agent/test/subagent-chain-result.test.ts +++ b/packages/coding-agent/test/subagent-chain-result.test.ts @@ -1,5 +1,5 @@ import type { AgentToolResult } from "@step-harness/agent-core"; -import { expect, test } from "vitest"; +import { afterEach, expect, test, vi } from "vitest"; import type { ExtensionContext } from "../src/core/extensions/types.ts"; import type { StepSubagentDetails, @@ -8,6 +8,12 @@ import type { SubagentParams, } from "../src/features/step-subagent.ts"; import { executeSubagent } from "../src/features/subagent/execute.ts"; +import { acquireWorkflowAdmission, resetWorkflowAdmission } from "../src/features/workflow/admission.ts"; + +afterEach(() => { + vi.unstubAllEnvs(); + resetWorkflowAdmission(); +}); // Deterministic runner outcome keyed by the step's task text. type Outcome = { text: string; exitCode?: number; stopReason?: string; errorMessage?: string }; @@ -56,8 +62,9 @@ const ctx = { async function run( params: SubagentParams, respond: (input: StepSubagentRunInput, index: number) => Outcome, - options: { maxConcurrency?: number } = {}, + options: { maxConcurrency?: number; runnerDelayMs?: number } = {}, ): Promise<{ + peakActive: number; result: AgentToolResult; tasks: string[]; partialModes: StepSubagentDetails["mode"][]; @@ -66,6 +73,8 @@ async function run( const tasks: string[] = []; const partialModes: StepSubagentDetails["mode"][] = []; const partialStatuses: string[][] = []; + let active = 0; + let peakActive = 0; const result = await executeSubagent( params, undefined, @@ -87,11 +96,18 @@ async function run( tasks.push(input.task); const partial = runResult({ text: "" }); input.onUpdate?.({ ...partial, exitCode: -1, messages: [] }); - return runResult(respond(input, index)); + active++; + peakActive = Math.max(peakActive, active); + try { + if (options.runnerDelayMs) await new Promise((resolve) => setTimeout(resolve, options.runnerDelayMs)); + return runResult(respond(input, index)); + } finally { + active--; + } }, }, ); - return { result, tasks, partialModes, partialStatuses }; + return { result, tasks, partialModes, partialStatuses, peakActive }; } function contentText(result: AgentToolResult): string { @@ -218,3 +234,66 @@ test("parallel tasks beyond maxConcurrency wait as queued", async () => { expect(partialStatuses).toContainEqual(["running", "queued"]); expect(partialStatuses.at(-1)).toEqual(["completed", "completed"]); }); + +test("parallel subagents wait for slots in the window shared with workflows", async () => { + const workflow = acquireWorkflowAdmission(2); + const held = [await workflow.semaphore.acquire(), await workflow.semaphore.acquire()]; + let started = 0; + const running = run( + { + tasks: [ + { agent: "explore", task: "a" }, + { agent: "explore", task: "b" }, + ], + } as SubagentParams, + (input) => { + started++; + return { text: `OUT_${input.task}` }; + }, + ); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(started).toBe(0); + for (const release of held) release(); + const { result } = await running; + expect(contentText(result)).toContain("Parallel: 2/2 succeeded"); + expect(started).toBe(2); + workflow.release(); +}); + +test("a 429 inside a parallel subagent lowers the shared window", async () => { + const workflow = acquireWorkflowAdmission(4); + await run({ tasks: [{ agent: "explore", task: "a" }] } as SubagentParams, (input) => { + input.onModelRequest?.({ type: "started" }); + input.onModelRequest?.({ type: "rate_limit", attempt: 1, maxRetries: 3, delayMs: 0, message: "429" }); + return { text: "ok" }; + }); + expect(workflow.semaphore.max).toBe(3); + workflow.release(); +}); + +test("parallel subagents honor the configured provider concurrency limit", async () => { + vi.stubEnv("STEP_WORKFLOW_CONCURRENCY_LIMIT", "1"); + const tasks = ["a", "b", "c"].map((task) => ({ agent: "explore", task })); + const { result, peakActive } = await run({ tasks } as SubagentParams, () => ({ text: "ok" }), { + runnerDelayMs: 10, + }); + expect(contentText(result)).toContain("Parallel: 3/3 succeeded"); + expect(peakActive).toBe(1); + + vi.unstubAllEnvs(); + const unlimited = await run({ tasks } as SubagentParams, () => ({ text: "ok" }), { runnerDelayMs: 10 }); + expect(unlimited.peakActive).toBeGreaterThan(1); +}); + +test("admitted parallel subagents ask the runner to hold their slot until the child exits", async () => { + const seen: Array = []; + await run({ tasks: [{ agent: "explore", task: "a" }] } as SubagentParams, (input) => { + seen.push(input.awaitExit); + return { text: "ok" }; + }); + await run({ agent: "explore", task: "single" } as SubagentParams, (input) => { + seen.push(input.awaitExit); + return { text: "ok" }; + }); + expect(seen).toEqual([true, undefined]); +}); diff --git a/packages/coding-agent/test/subagent-exit-drain.test.ts b/packages/coding-agent/test/subagent-exit-drain.test.ts new file mode 100644 index 00000000..2a3ff540 --- /dev/null +++ b/packages/coding-agent/test/subagent-exit-drain.test.ts @@ -0,0 +1,106 @@ +import type { ChildProcess, SpawnOptions } from "node:child_process"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { expect, test, vi } from "vitest"; +import { runStepSubagentProcess } from "../src/features/step-subagent.ts"; +import { builtinStepAgents } from "../src/features/step-subagent-agents.ts"; + +const state = vi.hoisted(() => ({ + fixture: "", + descendants: [] as number[], + child: undefined as ChildProcess | undefined, +})); +vi.mock("node:child_process", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn: (_command: string, _args: string[], options: SpawnOptions) => { + const child = actual.spawn(process.execPath, [state.fixture], options); + state.child = child; + let buffer = ""; + child.stdout?.on("data", (chunk) => { + buffer += chunk.toString(); + const lines = buffer.split("\n"); + buffer = lines.pop() ?? ""; + for (const line of lines) { + const event = JSON.parse(line); + if (event.type === "descendant_pid") state.descendants.push(event.pid); + } + }); + return child; + }, + }; +}); + +test("workflow child exit settles even when a quiet descendant holds its stdio open", async () => { + const cwd = await mkdtemp(path.join(tmpdir(), "subagent-exit-drain-")); + state.fixture = path.join(cwd, "rpc-child.cjs"); + await writeFile( + state.fixture, + ` +const {spawn} = require('node:child_process'); +process.stdin.once('data', () => { + const descendant = spawn(process.execPath, ['-e', 'setTimeout(()=>{},5000)'], {detached:true,stdio:['ignore',1,2]}); + descendant.unref(); + console.log(JSON.stringify({type:'descendant_pid',pid:descendant.pid})); + console.log(JSON.stringify({type:'message_end',message:{role:'assistant',content:[{type:'text',text:'OK'}],stopReason:'stop',timestamp:Date.now(),usage:{input:0,output:0,cacheRead:0,cacheWrite:0,cost:{total:0}}}})); + console.log(JSON.stringify({type:'agent_settled'})); + process.exit(0); +}); +`, + ); + try { + const started = Date.now(); + const result = await runStepSubagentProcess({ + agent: { ...builtinStepAgents.find((agent) => agent.name === "general")!, systemPrompt: "" }, + task: "test", + cwd, + workflowAcl: { baseCwd: cwd }, + keepAlive: false, + }); + expect(result.stopReason).toBe("stop"); + expect(Date.now() - started).toBeLessThan(2000); + expect(state.descendants).toHaveLength(1); + expect(state.child?.exitCode).toBe(0); + } finally { + for (const pid of state.descendants.splice(0)) { + try { + process.kill(pid); + } catch {} + } + state.child?.kill(); + await rm(cwd, { recursive: true, force: true }); + } +}); + +test("an admitted subagent resolves only after its child exits; others resolve at agent_settled", async () => { + const cwd = await mkdtemp(path.join(tmpdir(), "subagent-await-exit-")); + state.fixture = path.join(cwd, "rpc-child.cjs"); + // The child settles its turn, then takes 300ms to exit after stdin closes. + await writeFile( + state.fixture, + ` +process.stdin.once('data', () => { + console.log(JSON.stringify({type:'message_end',message:{role:'assistant',content:[{type:'text',text:'OK'}],stopReason:'stop',timestamp:Date.now(),usage:{input:0,output:0,cacheRead:0,cacheWrite:0,cost:{total:0}}}})); + console.log(JSON.stringify({type:'agent_settled'})); +}); +process.stdin.on('end', () => setTimeout(() => process.exit(0), 300)); +process.stdin.resume(); +`, + ); + const agent = { ...builtinStepAgents.find((item) => item.name === "general")!, systemPrompt: "" }; + try { + const waited = await runStepSubagentProcess({ agent, task: "test", cwd, keepAlive: false, awaitExit: true }); + expect(waited.stopReason).toBe("stop"); + expect(state.child?.exitCode).toBe(0); + + await runStepSubagentProcess({ agent, task: "test", cwd, keepAlive: false }); + const unadmitted = state.child; + expect(unadmitted?.exitCode).toBeNull(); + await new Promise((resolve) => unadmitted?.once("exit", resolve)); + } finally { + state.child?.kill(); + await rm(cwd, { recursive: true, force: true }); + } +}); diff --git a/packages/coding-agent/test/workflow-agent-runner.test.ts b/packages/coding-agent/test/workflow-agent-runner.test.ts new file mode 100644 index 00000000..a7170c21 --- /dev/null +++ b/packages/coding-agent/test/workflow-agent-runner.test.ts @@ -0,0 +1,63 @@ +import { expect, test, vi } from "vitest"; +import type { StepSubagentRunInput, StepSubagentRunResult } from "../src/features/step-subagent.ts"; + +const runStepSubagentProcess = vi.hoisted(() => vi.fn()); + +vi.mock("../src/features/step-subagent.ts", async (importOriginal) => ({ + ...(await importOriginal()), + runStepSubagentProcess, +})); + +vi.mock("../src/features/step-subagent-agents.ts", async (importOriginal) => ({ + ...(await importOriginal()), + discoverStepAgents: async () => ({ agents: [{ name: "general", systemPrompt: "", source: "builtin" }] }), + resolveStepAgent: (agents: Array<{ name: string }>, name: string) => agents.find((agent) => agent.name === name), +})); + +const { createDefaultWorkflowAgentRunner } = await import("../src/features/workflow/agent-runner.ts"); + +function childResult(overrides: Partial = {}): StepSubagentRunResult { + return { + messages: [{ role: "assistant", content: [{ type: "text", text: "done" }] }], + usage: { input: 1, output: 1, cacheRead: 0, cacheWrite: 0, cost: 0, contextTokens: 1, turns: 1 }, + exitCode: 0, + stopReason: "stop", + ...overrides, + } as StepSubagentRunResult; +} + +test("default runner forwards each child tool start and vouches for complete tool reporting", async () => { + runStepSubagentProcess.mockImplementation(async (input: StepSubagentRunInput) => { + const update = (patch: Partial) => input.onUpdate?.(childResult(patch)); + update({ lastEvent: "message_update", activeTool: "read_file" }); + update({ lastEvent: "tool_execution_start", activeTool: "read_file" }); + update({ lastEvent: "tool_execution_update", activeTool: "read_file" }); + update({ lastEvent: "tool_execution_start", activeTool: "edit_file" }); + update({ lastEvent: "tool_execution_end" }); + return childResult(); + }); + const tools: string[] = []; + const runner = createDefaultWorkflowAgentRunner(); + const result = await runner({ + prompt: "work", + options: {}, + cwd: process.cwd(), + runId: "run", + agentId: "agent", + onToolExecutionStart: (tool) => tools.push(tool), + }); + expect(tools).toEqual(["read_file", "edit_file"]); + expect(result).toMatchObject({ status: "completed", toolActivityObserved: true }); + + runStepSubagentProcess.mockResolvedValueOnce(childResult({ stopReason: "error", errorMessage: "boom" })); + const failed = await runner({ prompt: "work", options: {}, cwd: process.cwd(), runId: "run", agentId: "agent" }); + expect(failed).toMatchObject({ status: "failed", toolActivityObserved: true }); + + // A killed or crashed child may have lost a tool event in its unread output. + runStepSubagentProcess.mockResolvedValueOnce(childResult({ exitCode: 143, stopReason: "aborted" })); + const killed = await runner({ prompt: "work", options: {}, cwd: process.cwd(), runId: "run", agentId: "agent" }); + expect(killed).toMatchObject({ status: "aborted", toolActivityObserved: false }); + runStepSubagentProcess.mockResolvedValueOnce(childResult({ exitCode: 1, stopReason: "error" })); + const crashed = await runner({ prompt: "work", options: {}, cwd: process.cwd(), runId: "run", agentId: "agent" }); + expect(crashed).toMatchObject({ status: "failed", toolActivityObserved: false }); +}); diff --git a/packages/coding-agent/test/workflow-extension.test.ts b/packages/coding-agent/test/workflow-extension.test.ts index 66ee8b72..2cf36205 100644 --- a/packages/coding-agent/test/workflow-extension.test.ts +++ b/packages/coding-agent/test/workflow-extension.test.ts @@ -5,6 +5,7 @@ import type { AgentToolResult } from "@step-harness/agent-core"; import { afterEach, expect, test, vi } from "vitest"; import type { ExtensionAPI, ExtensionCommandContext, ToolDefinition } from "../src/core/extensions/types.ts"; import { registerWorkflowChildAcl, WORKFLOW_ACL_ENV } from "../src/features/workflow/acl-extension.ts"; +import { workflowProgressResult } from "../src/features/workflow/rendering.ts"; import { createStepWorkflowExtension, resolveWorkflowScript, @@ -15,6 +16,58 @@ import type { StepTelemetryReporter } from "../src/step/telemetry.ts"; const cleanups: string[] = []; +test("concurrency clamping is a notice, independent of workflow failure state", () => { + const progress: WorkflowProgress = { + schemaVersion: 1, + runId: "notice", + name: "notice", + status: "running", + startedAt: 0, + updatedAt: 0, + agents: [], + completedAgents: 0, + totalAgents: 15, + spentTokens: 0, + concurrency: { requested: 15, ceiling: 8, limit: 8, cap: 8 }, + }; + const output = workflowProgressResult(progress); + expect(output.details.status).toBe("running"); + expect(output.isError).not.toBe(true); + expect(output.content).toMatchObject([ + { text: expect.stringContaining("Notice: requested concurrency 15 was limited to 8") }, + ]); + const failure = workflowProgressResult({ ...progress, status: "failed", message: "parallel() invalid entry" }); + expect(failure.content).toMatchObject([{ text: expect.stringContaining("parallel() invalid entry") }]); +}); + +test("nested callback failures name their script in the text channels", () => { + const progress: WorkflowProgress = { + schemaVersion: 1, + runId: "nested", + name: "nested", + status: "partial_failed", + startedAt: 0, + updatedAt: 0, + agents: [], + completedAgents: 1, + totalAgents: 1, + spentTokens: 0, + taskFailures: [ + { + script: "child.js", + primitive: "parallel", + index: 1, + message: "ReferenceError: d is not defined", + }, + ], + }; + const output = workflowProgressResult(progress); + expect(output.details?.status).toBe("partial_failed"); + expect(output.content).toMatchObject([ + { text: expect.stringContaining("child.js parallel[1]: ReferenceError: d is not defined") }, + ]); +}); + afterEach(async () => { vi.unstubAllEnvs(); await Promise.all(cleanups.splice(0).map((directory) => rm(directory, { recursive: true, force: true }))); diff --git a/packages/coding-agent/test/workflow-runtime.test.ts b/packages/coding-agent/test/workflow-runtime.test.ts index da55614f..895e8ad4 100644 --- a/packages/coding-agent/test/workflow-runtime.test.ts +++ b/packages/coding-agent/test/workflow-runtime.test.ts @@ -1,29 +1,49 @@ import { mkdir, mkdtemp, readFile, rm, symlink, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import os, { tmpdir } from "node:os"; import path from "node:path"; -import { afterEach, expect, test } from "vitest"; -import { WorkflowBudgetExceeded } from "../src/features/workflow/budget.ts"; +import { afterEach, beforeEach, expect, test, vi } from "vitest"; +import { acquireWorkflowAdmission, resetWorkflowAdmission } from "../src/features/workflow/admission.ts"; +import { + clampWorkflowConcurrency, + configuredWorkflowConcurrencyLimit, + WorkflowBudgetExceeded, + WorkflowSemaphore, +} from "../src/features/workflow/budget.ts"; import { createWorkflowRunPaths, readJsonLines, WorkflowJournal, type WorkflowRunPaths, } from "../src/features/workflow/journal.ts"; -import { WorkflowRuntime } from "../src/features/workflow/runtime.ts"; -import { checkWorkflowPathAccess, checkWorkflowToolCall } from "../src/features/workflow/tool-profile.ts"; +import { WorkflowProgressStore } from "../src/features/workflow/progress.ts"; +import { WorkflowRuntime, workflowToolResult } from "../src/features/workflow/runtime.ts"; +import { + checkWorkflowPathAccess, + checkWorkflowToolCall, + classifyWorkflowToolEffect, +} from "../src/features/workflow/tool-profile.ts"; import type { IterateResult, WorkflowAgentOptions, WorkflowAgentRunInput, WorkflowAgentRunner, WorkflowProgress, + WorkflowProgressAgent, WorkflowTelemetryRecord, } from "../src/features/workflow/types.ts"; const cleanups: string[] = []; let runSequence = 0; +beforeEach(() => { + const cpu = os.cpus()[0]!; + vi.spyOn(os, "cpus").mockReturnValue(Array(10).fill(cpu)); +}); + afterEach(async () => { + vi.useRealTimers(); + vi.restoreAllMocks(); + resetWorkflowAdmission(); await Promise.all(cleanups.splice(0).map((directory) => rm(directory, { recursive: true, force: true }))); }); @@ -40,9 +60,12 @@ function runtimeAt( resumeFrom?: WorkflowRunPaths; budgetTotal?: number; maxConcurrency?: number; + providerConcurrencyLimit?: number; + runnerCleanupTimeoutMs?: number; maxAgents?: number; agentTimeoutMs?: number; vmExecutor?: ConstructorParameters[0]["vmExecutor"]; + nestedWorkflow?: ConstructorParameters[0]["nestedWorkflow"]; } = {}, ): { runtime: WorkflowRuntime; journal: WorkflowJournal; paths: WorkflowRunPaths } { runSequence += 1; @@ -56,9 +79,12 @@ function runtimeAt( runner, budgetTotal: options.budgetTotal, maxConcurrency: options.maxConcurrency, + providerConcurrencyLimit: options.providerConcurrencyLimit, + runnerCleanupTimeoutMs: options.runnerCleanupTimeoutMs, maxAgents: options.maxAgents, agentTimeoutMs: options.agentTimeoutMs, vmExecutor: options.vmExecutor, + nestedWorkflow: options.nestedWorkflow, }); return { runtime, journal, paths }; } @@ -71,6 +97,586 @@ function result(value: unknown, tokens = 1) { }; } +/** A runner result that vouches it reported every tool start (none here). */ +function observed(value: unknown, tokens = 1) { + return { ...result(value, tokens), toolActivityObserved: true }; +} + +function initialProgress(runId: string): WorkflowProgress { + return { + schemaVersion: 1, + runId, + name: "test", + status: "running", + startedAt: 0, + updatedAt: 0, + agents: [], + completedAgents: 0, + totalAgents: 0, + spentTokens: 0, + }; +} + +test("explicit concurrency cannot exceed the machine ceiling", () => { + expect(clampWorkflowConcurrency(22, 10)).toBe(8); + expect(clampWorkflowConcurrency(22, 32)).toBe(16); + expect(clampWorkflowConcurrency(2, 10)).toBe(2); +}); + +test("optional provider concurrency ignores blank and invalid environment values", () => { + for (const raw of ["", " ", "abc", "Infinity", "-1", "0"]) + expect(configuredWorkflowConcurrencyLimit(raw)).toBeUndefined(); + expect(configuredWorkflowConcurrencyLimit(" 8 ")).toBe(8); +}); + +test("a long server Retry-After cannot freeze the shared admission window", async () => { + // A node's own retry honors Retry-After verbatim, but the admission window is + // process-wide, so an hour-long (or far-future-date) cooldown must not stall + // unrelated workflows with no timeout and no error. + vi.useFakeTimers(); + const semaphore = new WorkflowSemaphore(4); + try { + semaphore.throttle(semaphore.epoch, 3_600_000); + let admitted = false; + const acquired = semaphore.acquire().then((release) => { + admitted = true; + return release; + }); + await vi.advanceTimersByTimeAsync(29_000); + expect(admitted).toBe(false); + await vi.advanceTimersByTimeAsync(2_000); + const release = await acquired; + expect(admitted).toBe(true); + release(); + } finally { + semaphore.dispose(); + vi.useRealTimers(); + } +}); + +test("an invalid throttle delay neither blocks admission nor schedules a far-future timer", async () => { + vi.useFakeTimers(); + const semaphore = new WorkflowSemaphore(2); + try { + for (const delay of [Number.NaN, Number.POSITIVE_INFINITY, -1]) semaphore.throttle(semaphore.epoch, delay); + const release = await semaphore.acquire(); + release(); + } finally { + semaphore.dispose(); + vi.useRealTimers(); + } +}); + +test("nested workflow callback failures propagate to the owning run", async () => { + const child = 'return await parallel([() => agent("ok"), () => { throw new ReferenceError("missingRef"); }]);'; + const { runtime } = runtimeAt(await workspace(), async () => result("ok"), { + nestedWorkflow: (_name, args, parent, depth) => + parent.runNestedScript(child, args, depth + 1, { filename: "child.js" }), + }); + const output = await runtime.runScript('return await workflow("child");', null); + expect(output).toMatchObject({ + status: "partial_failed", + summary: { total: 1, completed: 1, failed: 0, failedCallbacks: 1 }, + taskFailures: [{ primitive: "parallel", index: 1, script: "child.js", message: "ReferenceError: missingRef" }], + }); +}); + +test.each([false, true])("agent failures are not double counted as callback failures (nested=%s)", async (nested) => { + const { runtime } = runtimeAt( + await workspace(), + async () => ({ status: "failed", errorMessage: "provider failed" }), + { + nestedWorkflow: (_name, args, parent, depth) => + parent.runNestedScript('return await agent("bad");', args, depth + 1), + }, + ); + const output = await runtime.runScript( + nested ? 'return await parallel([() => workflow("child")]);' : 'return await parallel([() => agent("bad")]);', + null, + ); + expect(output.status).toBe("failed"); + expect(output.summary).toEqual({ total: 1, completed: 0, failed: 1 }); + expect(output.taskFailures).toBeUndefined(); +}); + +test("unawaited nested workflows fail rather than returning a successful zero-call run", async () => { + const { runtime, paths } = runtimeAt(await workspace(), async () => result("ok"), { + nestedWorkflow: (_name, args, parent, depth) => + parent.runNestedScript('return await agent("child");', args, depth + 1), + }); + await expect(runtime.runScript('void workflow("child"); return "done";', null)).rejects.toThrow("unawaited"); + const progress = JSON.parse(await readFile(paths.progressPath, "utf8")); + expect(progress.status).toBe("failed"); + expect(progress.agents.every((agent: { status: string }) => !["running", "queued"].includes(agent.status))).toBe( + true, + ); +}); + +test("a failing terminal progress write never replaces the run's terminal error", async () => { + const cwd = await workspace(); + const paths = createWorkflowRunPaths(cwd, "progress_write_failure"); + const journal = new WorkflowJournal(paths); + // A disk that only fails on terminal writes: the run's real error must survive. + class TerminalFailingProgress extends WorkflowProgressStore { + override async update( + patch: Partial & { agent?: WorkflowProgressAgent }, + ): Promise { + if (patch.status !== undefined && patch.status !== "running") throw new Error("progress disk full"); + return super.update(patch); + } + } + const runtime = new WorkflowRuntime({ + cwd, + runId: "progress_write_failure", + name: "test", + journal, + progress: new TerminalFailingProgress(paths.progressPath, initialProgress("progress_write_failure")), + runner: async () => { + throw new Error("provider exploded"); + }, + }); + await expect(runtime.runScript('return await agent("boom");', null)).rejects.toThrow("provider exploded"); + const entries = await readJsonLines<{ status?: string; error?: string }>(journal.paths.journalPath); + expect(entries.filter((entry) => entry.status === "failed")).toMatchObject([ + { status: "failed", error: expect.stringContaining("provider exploded") }, + ]); +}); + +test("a failed progress write while recording a node keeps the failure classified once", async () => { + const cwd = await workspace(); + const paths = createWorkflowRunPaths(cwd, "node_record_progress_failure"); + // State mutates before the write, so the agent is already failed when the write + // rejects. The run must still report that node once, not also as a callback error. + class LateFailingProgress extends WorkflowProgressStore { + failWrites = false; + override async update( + patch: Partial & { agent?: WorkflowProgressAgent }, + ): Promise { + const snapshot = await super.update(patch); + if (this.failWrites) throw new Error("progress disk full"); + return snapshot; + } + } + const progress = new LateFailingProgress(paths.progressPath, initialProgress("node_record_progress_failure")); + const runtime = new WorkflowRuntime({ + cwd, + runId: "node_record_progress_failure", + name: "test", + journal: new WorkflowJournal(paths), + progress, + runner: async () => { + progress.failWrites = true; + return { status: "failed", errorMessage: "provider failed" }; + }, + }); + const output = await runtime.runScript('return await parallel([() => agent("bad")]);', null); + expect(output).toMatchObject({ status: "failed", summary: { total: 1, completed: 0, failed: 1 } }); + expect(output.taskFailures).toBeUndefined(); + expect(output.failures?.[0]?.error?.message).toContain("provider failed"); +}); + +test("a failed journal append while recording a node keeps the failure classified once", async () => { + const cwd = await workspace(); + const paths = createWorkflowRunPaths(cwd, "node_record_journal_failure"); + await mkdir(paths.runDir, { recursive: true }); + // A directory at the journal path makes every append fail while the script copy + // and progress writes still work. + await mkdir(paths.journalPath, { recursive: true }); + const runtime = new WorkflowRuntime({ + cwd, + runId: "node_record_journal_failure", + name: "test", + journal: new WorkflowJournal(paths), + runner: async () => ({ status: "failed", errorMessage: "provider failed" }), + }); + const output = await runtime.runScript('return await parallel([() => agent("bad")]);', null); + expect(output).toMatchObject({ status: "failed", summary: { total: 1, completed: 0, failed: 1 } }); + expect(output.taskFailures).toBeUndefined(); + expect(output.failures?.[0]?.error?.message).toContain("provider failed"); +}); + +test("a runner that throws while the journal is unwritable still reports the runner error", async () => { + const cwd = await workspace(); + const paths = createWorkflowRunPaths(cwd, "node_throw_journal_failure"); + await mkdir(paths.runDir, { recursive: true }); + await mkdir(paths.journalPath, { recursive: true }); + const runtime = new WorkflowRuntime({ + cwd, + runId: "node_throw_journal_failure", + name: "test", + journal: new WorkflowJournal(paths), + runner: async () => { + throw new Error("provider exploded"); + }, + }); + // The journal error must not replace the runner's error, in either channel. + await expect(runtime.runScript('return await agent("boom");', null)).rejects.toThrow("provider exploded"); + const progress = JSON.parse(await readFile(paths.progressPath, "utf8")); + expect(progress.agents[0].error.message).toContain("provider exploded"); + expect(progress.agents[0].error.message).not.toContain("EISDIR"); +}); + +test("a throwing progress listener cannot double count a node failure", async () => { + const { runtime } = runtimeAt( + await workspace(), + async () => ({ status: "failed", errorMessage: "provider failed" }), + {}, + ); + // Only the update that records the node failure throws, which is the path where a + // listener error used to escape and turn one failure into two. + (runtime as unknown as { onProgress?: (progress: WorkflowProgress) => void }).onProgress = (progress) => { + if (progress.agents.some((agent) => agent.status === "failed")) throw new Error("listener exploded"); + }; + const output = await runtime.runScript('return await parallel([() => agent("bad")]);', null); + expect(output).toMatchObject({ status: "failed", summary: { total: 1, completed: 0, failed: 1 } }); + expect(output.taskFailures).toBeUndefined(); +}); + +test("a node label that looks like a rate limit does not throttle the pool", async () => { + const { runtime } = runtimeAt( + await workspace(), + (input) => + new Promise((resolve) => { + input.signal?.addEventListener("abort", () => resolve({ status: "aborted" }), { once: true }); + }), + { maxConcurrency: 4, agentTimeoutMs: 1_000 }, + ); + const output = await runtime.runScript('return await parallel([() => agent("a", {label: "fix 429"})]);', null); + expect(output.failures?.[0]?.error).toMatchObject({ code: "NODE_TIMEOUT", retryable: false }); + // A synthesized timeout is not congestion, so the shared window keeps its cap. + expect(runtime.semaphore.max).toBe(4); +}); + +test("failed agent output that mentions rate limits is not provider congestion", async () => { + const { runtime } = runtimeAt( + await workspace(), + async () => ({ status: "failed", text: "I could not finish: the 429 rate limit docs were unreachable" }), + { maxConcurrency: 4 }, + ); + const output = await runtime.runScript('return await parallel([() => agent("document the 429 handler")]);', null); + expect(output.failures?.[0]?.error).toMatchObject({ code: "NODE_FAILED", retryable: false }); + expect(runtime.semaphore.max).toBe(4); +}); + +test("a runner failure is congestion only when the provider classified it, never by its text", async () => { + const { runtime } = runtimeAt( + await workspace(), + async (input) => + input.prompt === "quota" + ? { status: "failed", errorMessage: "429 GoUsageLimitError", errorKind: "quota" } + : { status: "failed", errorMessage: "429 Too Many Requests" }, + { maxConcurrency: 4 }, + ); + const output = await runtime.runScript( + 'return await parallel([() => agent("quota"), () => agent("unclassified")]);', + null, + ); + expect(output.failures?.map((failure) => failure.error?.code)).toEqual(["NODE_FAILED", "NODE_FAILED"]); + expect(runtime.semaphore.max).toBe(4); +}); + +test("a tool timeout in agent output is not reported as the node's own timeout", async () => { + const { runtime } = runtimeAt(await workspace(), async () => ({ + status: "failed", + errorMessage: "bash command timed out after 30s", + })); + const output = await runtime.runScript('return await parallel([() => agent("run it")]);', null); + expect(output.failures?.[0]?.error).toMatchObject({ code: "NODE_FAILED" }); +}); + +test("usage reported by a runner after cancellation is still charged to the budget", async () => { + const controller = new AbortController(); + const cwd = await workspace(); + const paths = createWorkflowRunPaths(cwd, "late_usage"); + const runtime = new WorkflowRuntime({ + cwd, + runId: "late_usage", + name: "test", + journal: new WorkflowJournal(paths), + signal: controller.signal, + runner: (input) => + new Promise((resolve) => { + input.signal?.addEventListener( + "abort", + () => setTimeout(() => resolve({ ...result("partial", 7), status: "aborted" }), 5), + { once: true }, + ); + queueMicrotask(() => controller.abort()); + }), + }); + await expect(runtime.runScript('return await agent("spend then cancel");', null)).rejects.toThrow(/aborted/i); + expect(runtime.budget.spent()).toBe(7); + const entries = await readJsonLines<{ status?: string; usage?: { input: number } }>(paths.journalPath); + expect(entries.at(-1)).toMatchObject({ status: "aborted", usage: { input: 7 } }); +}); + +test("a resampled request in the same turn is tagged with the current congestion epoch", async () => { + let finish!: () => void; + const gate = new Promise((resolve) => { + finish = resolve; + }); + let capAfterRecovery = 0; + let runtime!: WorkflowRuntime; + const built = runtimeAt( + await workspace(), + async (input) => { + if (input.prompt !== "d") { + await gate; + return result(input.prompt); + } + input.onModelRequest?.({ type: "started" }); + // Wait until the window is full and "e" is queued behind it. + await vi.waitFor(() => expect(runtime.semaphore.active).toBe(4)); + runtime.semaphore.throttle(runtime.semaphore.epoch, 0); + expect(runtime.semaphore.max).toBe(3); + // The first success belongs to the pre-throttle epoch and is ignored. The + // resample that follows has no "started" event and must count again. + for (let i = 0; i < 5; i++) input.onModelRequest?.({ type: "succeeded" }); + capAfterRecovery = runtime.semaphore.max; + finish(); + return result("d"); + }, + { maxConcurrency: 4 }, + ); + runtime = built.runtime; + // "d" is the probe; "e" is queued behind the full window, so demand exists. + const output = await runtime.runScript( + 'return await parallel([() => agent("a"), () => agent("b"), () => agent("c"), () => agent("d"), () => agent("e")]);', + null, + ); + expect(output.failures).toEqual([]); + expect(output.status).toBe("completed"); + expect(capAfterRecovery).toBe(4); +}); + +test("progress write failures never fail a finished node and recovery persists", async () => { + const cwd = await workspace(); + const paths = createWorkflowRunPaths(cwd, "progress_disk_blip"); + let calls = 0; + const runtime = new WorkflowRuntime({ + cwd, + runId: "progress_disk_blip", + name: "test", + journal: new WorkflowJournal(paths), + runner: async () => { + calls++; + if (calls === 1) { + // Renaming onto a directory fails, so every write while this exists errors. + await rm(paths.progressPath, { force: true }); + await mkdir(paths.progressPath); + } else { + await rm(paths.progressPath, { recursive: true, force: true }); + } + return result(`call ${calls}`); + }, + }); + const output = await runtime.runScript('return [await agent("one"), await agent("two")];', null); + expect(output).toMatchObject({ status: "completed", value: ["call 1", "call 2"], summary: { completed: 2 } }); + const entries = await readJsonLines<{ status?: string }>(paths.journalPath); + expect(entries.map((entry) => entry.status)).toEqual(["completed", "completed"]); + expect(JSON.parse(await readFile(paths.progressPath, "utf8"))).toMatchObject({ + status: "completed", + completedAgents: 2, + }); +}); + +test("a throttled admission window is remembered briefly after the last workflow finishes", async () => { + vi.useFakeTimers(); + const first = acquireWorkflowAdmission(4); + first.semaphore.throttle(first.semaphore.epoch, 20_000); + expect(first.semaphore.max).toBe(3); + first.release(); + // No timer may keep the process alive while nothing is attached. + expect(vi.getTimerCount()).toBe(0); + const second = acquireWorkflowAdmission(4); + expect(second.semaphore.max).toBe(3); + // The retained cooldown still gates admission and re-arms its wakeup for waiters. + let admitted = false; + const acquired = second.semaphore.acquire().then((release) => { + admitted = true; + return release; + }); + await vi.advanceTimersByTimeAsync(19_000); + expect(admitted).toBe(false); + await vi.advanceTimersByTimeAsync(1_500); + (await acquired)(); + expect(admitted).toBe(true); + second.release(); + // After the retention window the window starts fresh. + await vi.advanceTimersByTimeAsync(61_000); + const third = acquireWorkflowAdmission(4); + expect(third.semaphore.max).toBe(4); + third.release(); +}); + +test("an untouched admission window is discarded as soon as it is idle", () => { + const first = acquireWorkflowAdmission(4); + const semaphore = first.semaphore; + first.release(); + const second = acquireWorkflowAdmission(4); + expect(second.semaphore).not.toBe(semaphore); + second.release(); +}); + +test("a transient progress write failure does not poison later writes", async () => { + const cwd = await workspace(); + const paths = createWorkflowRunPaths(cwd, "progress_recovery"); + const store = new WorkflowProgressStore(paths.progressPath, initialProgress("progress_recovery")); + await store.update({ status: "running" }); + // Point the store at a path that cannot be written (a directory sits there), then + // move it back: the second write must still land. + const blocked = path.join(cwd, "blocked"); + await mkdir(path.join(blocked, "progress.json"), { recursive: true }); + const target = store as unknown as { path: string }; + target.path = path.join(blocked, "progress.json"); + await expect(store.update({ message: "first failure" })).rejects.toThrow(/EISDIR|ENOTDIR/); + target.path = paths.progressPath; + await store.update({ status: "completed", message: "recovered" }); + expect(JSON.parse(await readFile(paths.progressPath, "utf8"))).toMatchObject({ + status: "completed", + message: "recovered", + }); +}); + +test("shell interpolation errors before agent() cannot become a successful zero-call workflow", async () => { + let calls = 0; + const { runtime, paths } = runtimeAt(await workspace(), async (input) => { + calls++; + return result(input.prompt); + }); + const output = await runtime.runScript( + // biome-ignore lint/suspicious/noTemplateCurlyInString: Execute the user's broken JS template verbatim in the guest VM. + 'const tasks = Array.from({length: 10}, () => () => agent(`sleep $d; echo "slept=${d}s"`)); return await parallel(tasks);', + null, + ); + expect(calls).toBe(0); + expect(output).toMatchObject({ status: "failed", agentCalls: 0, summary: { failedCallbacks: 10 } }); + expect(output.taskFailures).toHaveLength(10); + expect( + output.taskFailures?.every( + (failure) => failure.message.includes("ReferenceError") && failure.message.includes("d"), + ), + ).toBe(true); + expect(workflowToolResult(output).isError).toBe(true); + expect(workflowToolResult(output).content).toMatchObject([{ text: expect.stringContaining("ReferenceError") }]); + const progress = JSON.parse(await readFile(paths.progressPath, "utf8")); + expect(progress.status).toBe("failed"); + expect(progress.taskFailures).toHaveLength(10); +}); + +test("ordinary JS strings preserve shell substitutions and dispatch all ten agents", async () => { + const prompts: string[] = []; + const { runtime } = runtimeAt(await workspace(), async (input) => { + prompts.push(input.prompt); + return result("ok"); + }); + const output = await runtime.runScript( + // biome-ignore lint/suspicious/noTemplateCurlyInString: Shell substitutions must survive JavaScript string construction. + "const command = 'sleep $d; echo \"slept=${d}s\"'; return await parallel(Array.from({length: 10}, () => () => agent(command)));", + null, + ); + expect(output).toMatchObject({ + status: "completed", + agentCalls: 10, + summary: { total: 10, completed: 10, failed: 0 }, + }); + // biome-ignore lint/suspicious/noTemplateCurlyInString: Assert the exact literal shell command. + expect(prompts).toEqual(Array(10).fill('sleep $d; echo "slept=${d}s"')); +}); + +test("throttling one wave reduces cap once and queued work resumes after releases", async () => { + const semaphore = new WorkflowSemaphore(8); + const releases = await Promise.all(Array.from({ length: 8 }, () => semaphore.acquire())); + let admitted = 0; + const queued = Array.from({ length: 3 }, () => + semaphore.acquire().then((release) => { + admitted++; + return release; + }), + ); + const epoch = semaphore.epoch; + semaphore.throttle(epoch, 0); + semaphore.throttle(epoch, 0); + expect(semaphore.max).toBe(6); + for (const release of releases.slice(0, 2)) release(); + await Promise.resolve(); + expect(admitted).toBe(0); + for (const release of releases.slice(2)) release(); + for (const release of await Promise.all(queued)) release(); + expect(admitted).toBe(3); + semaphore.dispose(); +}); + +test("cooldown blocks new admissions, cancellation removes waiters, and successes recover cap", async () => { + vi.useFakeTimers(); + const semaphore = new WorkflowSemaphore(4); + semaphore.throttle(semaphore.epoch, 1000); + const controller = new AbortController(); + const cancelled = semaphore.acquire(controller.signal); + const cancelledCheck = expect(cancelled).rejects.toThrow("aborted"); + controller.abort(); + await cancelledCheck; + let admitted = 0; + const pending = Array.from({ length: 4 }, () => + semaphore.acquire().then((release) => { + admitted++; + return release; + }), + ); + await vi.advanceTimersByTimeAsync(999); + expect(admitted).toBe(0); + await vi.advanceTimersByTimeAsync(1); + expect(admitted).toBe(3); + for (let i = 0; i < 4; i++) semaphore.succeeded(semaphore.epoch); + expect(semaphore.max).toBe(4); + for (const release of await Promise.all(pending)) release(); + semaphore.dispose(); + expect(vi.getTimerCount()).toBe(0); +}); + +test("parallel failures survive null filtering in final tool output and disk progress", async () => { + const { runtime, paths } = runtimeAt(await workspace(), async (input) => + input.prompt === "bad" + ? { status: "failed", errorMessage: "429 concurrency reached, current: 9, limit: 8", errorKind: "rate_limit" } + : result("ok"), + ); + const output = await runtime.runScript( + 'return (await parallel([() => agent("ok"), () => agent("bad")])).filter(Boolean);', + null, + ); + expect(output).toMatchObject({ + status: "partial_failed", + value: ["ok"], + summary: { total: 2, completed: 1, failed: 1 }, + failures: [{ error: { code: "RATE_LIMITED", retryable: true } }], + }); + expect(workflowToolResult(output).isError).toBe(true); + expect(workflowToolResult(output).content).toMatchObject([ + { text: expect.stringContaining("429 concurrency reached") }, + ]); + const progress = JSON.parse(await readFile(paths.progressPath, "utf8")); + expect(progress.status).toBe("partial_failed"); + expect(progress.agents[1].error.message).toContain("429"); +}); + +test("a recovered 429 adjusts cap without restarting the node or reporting it failed", async () => { + let calls = 0; + const { runtime } = runtimeAt( + await workspace(), + async (input) => { + calls++; + input.onModelRequest?.({ type: "rate_limit", attempt: 1, maxRetries: 3, delayMs: 0, message: "429" }); + input.onModelRequest?.({ type: "started" }); + input.onModelRequest?.({ type: "succeeded" }); + return result("ok"); + }, + { maxConcurrency: 4 }, + ); + const output = await runtime.runScript('return await agent("test");', null); + expect(calls).toBe(1); + expect(output).toMatchObject({ status: "completed", failures: [], concurrency: { limit: 4, cap: 3 } }); +}); + test("agent retries invalid schema output and strips unknown fields", async () => { const cwd = await workspace(); const prompts: string[] = []; @@ -117,9 +723,9 @@ test("agent stops after three schema failures and journals the error", async () expect(entries[0]?.error).toContain("required properties value"); }); -test("resume reuses only the matching journal prefix", async () => { +test("resume replays unchanged calls by content, independent of position", async () => { const cwd = await workspace(); - const original = runtimeAt(cwd, async (input) => result(input.prompt)); + const original = runtimeAt(cwd, async (input) => observed(input.prompt)); await original.journal.initialize("original"); for (const prompt of ["a", "b", "c"]) await original.runtime.agent(prompt); @@ -128,7 +734,7 @@ test("resume reuses only the matching journal prefix", async () => { cwd, async (input) => { exactCalls.push(input.prompt); - return result(input.prompt); + return observed(input.prompt); }, { resumeFrom: original.paths }, ); @@ -144,7 +750,7 @@ test("resume reuses only the matching journal prefix", async () => { cwd, async (input) => { chainedCalls.push(input.prompt); - return result(input.prompt); + return observed(input.prompt); }, { resumeFrom: exact.paths }, ); @@ -159,7 +765,7 @@ test("resume reuses only the matching journal prefix", async () => { cwd, async (input) => { mismatchCalls.push(input.prompt); - return result(input.prompt); + return observed(input.prompt); }, { resumeFrom: original.paths }, ); @@ -167,10 +773,310 @@ test("resume reuses only the matching journal prefix", async () => { expect(await mismatch.runtime.agent("a")).toBe("a"); expect(await mismatch.runtime.agent("changed")).toBe("changed"); expect(await mismatch.runtime.agent("c")).toBe("c"); - expect(mismatchCalls).toEqual(["changed", "c"]); + // Only the changed call runs live; "c" after it still replays. + expect(mismatchCalls).toEqual(["changed"]); + + const reorderedCalls: string[] = []; + const reordered = runtimeAt( + cwd, + async (input) => { + reorderedCalls.push(input.prompt); + return observed(input.prompt); + }, + { resumeFrom: original.paths }, + ); + await reordered.journal.initialize("reordered"); + // Pipeline stages finish in nondeterministic order, so sequence numbers move. + for (const prompt of ["c", "a", "b", "a"]) expect(await reordered.runtime.agent(prompt)).toBe(prompt); + // Each cached result replays once: the second identical "a" runs live. + expect(reorderedCalls).toEqual(["a"]); }); -test("resume stops at a malformed journal line instead of skipping ahead", async () => { +test("resume re-runs a call that failed while replaying the ones after it", async () => { + const cwd = await workspace(); + const original = runtimeAt(cwd, async (input) => + input.prompt === "b" ? { status: "failed", errorMessage: "boom" } : observed(input.prompt), + ); + await original.journal.initialize("failed-source"); + await original.runtime.runScript( + 'return await parallel([() => agent("a"), () => agent("b"), () => agent("c")]);', + null, + ); + await original.journal.flush(); + + const calls: string[] = []; + const resumed = runtimeAt( + cwd, + async (input) => { + calls.push(input.prompt); + return observed(input.prompt); + }, + { resumeFrom: original.paths }, + ); + await resumed.journal.initialize("failed-resume"); + for (const prompt of ["a", "b", "c"]) expect(await resumed.runtime.agent(prompt)).toBe(prompt); + expect(calls).toEqual(["b"]); +}); + +test("resume records replayed calls under the resumed run's own sequence numbers", async () => { + const cwd = await workspace(); + const original = runtimeAt(cwd, async (input) => observed(input.prompt)); + await original.journal.initialize("seq-source"); + for (const prompt of ["a", "b", "c"]) await original.runtime.agent(prompt); + + const resumed = runtimeAt(cwd, async (input) => observed(input.prompt), { resumeFrom: original.paths }); + await resumed.journal.initialize("seq-resume"); + for (const prompt of ["c", "new", "a"]) await resumed.runtime.agent(prompt); + await resumed.journal.flush(); + const entries = await readJsonLines<{ seq: number; prompt: string; status: string }>(resumed.paths.journalPath); + expect(entries.map(({ seq, prompt, status }) => ({ seq, prompt, status }))).toEqual([ + { seq: 0, prompt: "c", status: "cached" }, + { seq: 1, prompt: "new", status: "completed" }, + { seq: 2, prompt: "a", status: "cached" }, + ]); +}); + +test("resume never replays a call whose schema is a runtime object", async () => { + const cwd = await workspace(); + const runtimeSchema = { safeParse: (value: unknown) => ({ success: true, data: value }) }; + const original = runtimeAt(cwd, async () => observed({ kind: "old" })); + await original.journal.initialize("runtime-schema-source"); + await original.runtime.agent("same prompt", { schema: runtimeSchema }); + + let calls = 0; + const resumed = runtimeAt( + cwd, + async () => { + calls += 1; + return observed({ kind: "new" }); + }, + { resumeFrom: original.paths }, + ); + await resumed.journal.initialize("runtime-schema-resume"); + // Runtime schemas hash to one placeholder, so two different schemas would be + // indistinguishable; such calls always run live. + expect(await resumed.runtime.agent("same prompt", { schema: runtimeSchema })).toEqual({ kind: "new" }); + expect(calls).toBe(1); +}); + +test("resume re-runs a cached result that fails the call's schema", async () => { + const cwd = await workspace(); + const schema = { type: "object", required: ["value"], properties: { value: { type: "string" } } }; + const original = runtimeAt(cwd, async () => observed({ value: "ok" })); + await original.journal.initialize("schema-source"); + await original.runtime.agent("structured", { schema }); + await original.journal.flush(); + const journalText = await readFile(original.paths.journalPath, "utf8"); + await writeFile(original.paths.journalPath, journalText.replace('"value":"ok"', '"value":42')); + + let calls = 0; + const resumed = runtimeAt( + cwd, + async () => { + calls += 1; + return observed({ value: "fresh" }); + }, + { resumeFrom: original.paths }, + ); + await resumed.journal.initialize("schema-resume"); + expect(await resumed.runtime.agent("structured", { schema })).toEqual({ value: "fresh" }); + expect(calls).toBe(1); +}); + +/** Original run: "fix" fails; "scan" and "check" read files; "think" uses no tools. */ +async function resumeSourceWithTools(cwd: string): Promise { + const tools: Record = { + scan: ["read_file"], + check: ["search_files", "read_file"], + think: [], + fix: ["edit_file"], + }; + const source = runtimeAt(cwd, async (input) => { + for (const tool of tools[input.prompt] ?? []) input.onToolExecutionStart?.(tool); + return input.prompt === "fix" + ? { status: "failed" as const, errorMessage: "boom", toolActivityObserved: true } + : observed(input.prompt); + }); + await source.journal.initialize("tools-source"); + for (const prompt of ["scan", "fix", "check", "think"]) { + await source.runtime.agent(prompt).catch(() => undefined); + } + await source.journal.flush(); + return source.paths; +} + +test("a live write during resume stops replaying results that touched the environment", async () => { + const cwd = await workspace(); + const sourcePaths = await resumeSourceWithTools(cwd); + const calls: string[] = []; + const resumed = runtimeAt( + cwd, + async (input) => { + calls.push(input.prompt); + if (input.prompt === "fix") input.onToolExecutionStart?.("edit_file"); + return observed(input.prompt); + }, + { resumeFrom: sourcePaths }, + ); + await resumed.journal.initialize("tools-resume"); + for (const prompt of ["scan", "fix", "check", "think"]) expect(await resumed.runtime.agent(prompt)).toBe(prompt); + // "scan" replays before anything changed. After "fix" edits a file, "check" + // read files that may now differ and re-runs; "think" touched nothing and replays. + expect(calls).toEqual(["fix", "check"]); + await resumed.journal.flush(); + const entries = await readJsonLines<{ prompt: string; status: string; worldAccess?: boolean }>( + resumed.paths.journalPath, + ); + expect(entries.map(({ prompt, status, worldAccess }) => ({ prompt, status, worldAccess }))).toEqual([ + { prompt: "scan", status: "cached", worldAccess: true }, + { prompt: "fix", status: "completed", worldAccess: true }, + { prompt: "check", status: "completed", worldAccess: false }, + { prompt: "think", status: "cached", worldAccess: false }, + ]); +}); + +test("shell commands and unknown tools count as writes; reads do not", async () => { + const cases: Array<{ tool: string; replayed: boolean }> = [ + { tool: "read_file", replayed: true }, + { tool: "search_web", replayed: true }, + { tool: "find_tools", replayed: true }, + { tool: "run_command", replayed: false }, + { tool: "write_file", replayed: false }, + { tool: "mcp__github__create_issue", replayed: false }, + { tool: "", replayed: false }, + ]; + for (const { tool, replayed } of cases) { + const cwd = await workspace(); + const sourcePaths = await resumeSourceWithTools(cwd); + const calls: string[] = []; + const resumed = runtimeAt( + cwd, + async (input) => { + calls.push(input.prompt); + if (input.prompt === "fix") input.onToolExecutionStart?.(tool); + return observed(input.prompt); + }, + { resumeFrom: sourcePaths }, + ); + await resumed.journal.initialize(`effect-${tool}`); + await resumed.runtime.agent("fix"); + await resumed.runtime.agent("check"); + expect({ tool, calls }).toEqual({ tool, calls: replayed ? ["fix"] : ["fix", "check"] }); + } + expect(classifyWorkflowToolEffect("clarify_user")).toBe("none"); + expect(classifyWorkflowToolEffect("list_directory")).toBe("read"); + expect(classifyWorkflowToolEffect("bash")).toBe("mutate"); + expect(classifyWorkflowToolEffect("edit")).toBe("mutate"); +}); + +test("a live call without tool reporting is treated as a write", async () => { + const cwd = await workspace(); + const sourcePaths = await resumeSourceWithTools(cwd); + const calls: string[] = []; + const resumed = runtimeAt( + cwd, + async (input) => { + calls.push(input.prompt); + // No toolActivityObserved: the runtime cannot know what this call did. + return result(input.prompt); + }, + { resumeFrom: sourcePaths }, + ); + await resumed.journal.initialize("unobserved-resume"); + for (const prompt of ["fix", "check", "think"]) await resumed.runtime.agent(prompt); + expect(calls).toEqual(["fix", "check"]); + await resumed.journal.flush(); + const entries = await readJsonLines>(resumed.paths.journalPath); + // An unobserved call records no worldAccess, so a later resume treats it as touched. + expect(entries.find((entry) => entry.prompt === "fix")).not.toHaveProperty("worldAccess"); +}); + +test("after a write, entries from journals without tool records are treated as touched", async () => { + const cwd = await workspace(); + const source = runtimeAt(cwd, async (input) => result(input.prompt)); + await source.journal.initialize("legacy-source"); + await source.runtime.agent("think"); + await source.journal.flush(); + const legacy = await readJsonLines>(source.paths.journalPath); + expect(legacy[0]).not.toHaveProperty("worldAccess"); + + const calls: string[] = []; + const resumed = runtimeAt( + cwd, + async (input) => { + calls.push(input.prompt); + if (input.prompt === "fix") input.onToolExecutionStart?.("write_file"); + return observed(input.prompt); + }, + { resumeFrom: source.paths }, + ); + await resumed.journal.initialize("legacy-resume"); + await resumed.runtime.agent("fix"); + await resumed.runtime.agent("think"); + expect(calls).toEqual(["fix", "think"]); +}); + +test("chained resumes keep each replayed call's tool record and schema-cleaned result", async () => { + const cwd = await workspace(); + const sourcePaths = await resumeSourceWithTools(cwd); + const middle = runtimeAt(cwd, async (input) => observed(input.prompt), { resumeFrom: sourcePaths }); + await middle.journal.initialize("chain-middle"); + for (const prompt of ["scan", "think"]) await middle.runtime.agent(prompt); + await middle.journal.flush(); + + const calls: string[] = []; + const last = runtimeAt( + cwd, + async (input) => { + calls.push(input.prompt); + if (input.prompt === "fix") input.onToolExecutionStart?.("run_command"); + return observed(input.prompt); + }, + { resumeFrom: middle.paths }, + ); + await last.journal.initialize("chain-last"); + for (const prompt of ["fix", "scan", "think"]) await last.runtime.agent(prompt); + // "scan" read files in the original run; that record survives the middle replay. + expect(calls).toEqual(["fix", "scan"]); + + const schema = { type: "object", additionalProperties: false, properties: { value: { type: "string" } } }; + const structured = runtimeAt(cwd, async () => observed({ value: "kept" })); + await structured.journal.initialize("strip-source"); + await structured.runtime.agent("structured", { schema }); + await structured.journal.flush(); + const text = await readFile(structured.paths.journalPath, "utf8"); + await writeFile(structured.paths.journalPath, text.replace('"value":"kept"', '"value":"kept","extra":1')); + const replay = runtimeAt(cwd, async () => observed({ value: "live" }), { resumeFrom: structured.paths }); + await replay.journal.initialize("strip-resume"); + expect(await replay.runtime.agent("structured", { schema })).toEqual({ value: "kept" }); + await replay.journal.flush(); + const [entry] = await readJsonLines<{ result: unknown }>(replay.paths.journalPath); + expect(entry?.result).toEqual({ value: "kept" }); +}); + +test("calls issued together still replay when a live sibling writes", async () => { + const cwd = await workspace(); + const sourcePaths = await resumeSourceWithTools(cwd); + const calls: string[] = []; + const resumed = runtimeAt( + cwd, + async (input) => { + calls.push(input.prompt); + if (input.prompt === "fix") input.onToolExecutionStart?.("edit_file"); + return observed(input.prompt); + }, + { resumeFrom: sourcePaths }, + ); + await resumed.journal.initialize("parallel-resume"); + const value = await resumed.runtime.runScript( + 'return await parallel(["fix", "scan", "check", "think"].map((prompt) => () => agent(prompt)));', + null, + ); + expect(value.value).toEqual(["fix", "scan", "check", "think"]); + expect(calls).toEqual(["fix"]); +}); + +test("resume trusts nothing after a malformed journal line", async () => { const cwd = await workspace(); const source = runtimeAt(cwd, async (input) => result(input.prompt)); await source.journal.initialize("malformed-source"); @@ -337,16 +1243,222 @@ test("parallel calls cannot race past the workflow agent limit", async () => { test("agent timeout aborts the attempt and releases its semaphore slot", async () => { const cwd = await workspace(); - const { runtime, journal } = runtimeAt(cwd, async () => new Promise(() => {}), { - agentTimeoutMs: 1_000, - maxConcurrency: 1, - }); + const { runtime, journal } = runtimeAt( + cwd, + async (input) => + new Promise((resolve) => { + input.signal?.addEventListener("abort", () => resolve({ status: "aborted" }), { once: true }); + }), + { + agentTimeoutMs: 1_000, + maxConcurrency: 1, + }, + ); await journal.initialize("agent-timeout"); await expect(runtime.agent("never settles")).rejects.toThrow("timed out after 1000ms"); expect(runtime.semaphore.active).toBe(0); }); +test("script failure cancels running and queued nodes, awaits cleanup, and seals terminal progress", async () => { + let signalStarted!: () => void; + const started = new Promise((resolve) => { + signalStarted = resolve; + }); + const calls: string[] = []; + let cleaned = false; + const { runtime, paths } = runtimeAt( + await workspace(), + async (input) => { + calls.push(input.prompt); + signalStarted(); + await new Promise((resolve) => + input.signal?.addEventListener( + "abort", + () => { + setTimeout(() => { + cleaned = true; + resolve(); + }, 10); + }, + { once: true }, + ), + ); + return { status: "aborted" }; + }, + { + maxConcurrency: 1, + vmExecutor: async (_script, _args, host) => { + void host.agent("running", {}); + void host.agent("queued", {}); + await started; + throw new Error("script exploded"); + }, + }, + ); + await expect(runtime.runScript("bad script", null)).rejects.toThrow("script exploded"); + expect(cleaned).toBe(true); + expect(calls).toEqual(["running"]); + const before = await readFile(paths.progressPath, "utf8"); + const progress = JSON.parse(before); + expect(progress).toMatchObject({ status: "failed", message: "script exploded" }); + expect(progress.agents.every((a: { status: string }) => a.status === "aborted")).toBe(true); + const journal = await readJsonLines<{ status: string }>(paths.journalPath); + expect(journal).toHaveLength(2); + expect(journal.every((entry) => entry.status === "aborted")).toBe(true); + await expect(runtime.agent("late")).rejects.toThrow("scope is closed"); + runtime.log("late update"); + expect(await readFile(paths.progressPath, "utf8")).toBe(before); +}); + +test("returning with detached agent calls fails and cancels them instead of claiming completion", async () => { + let aborted = false; + let signalStarted!: () => void; + const started = new Promise((resolve) => { + signalStarted = resolve; + }); + const { runtime } = runtimeAt( + await workspace(), + async (input) => { + signalStarted(); + await new Promise((resolve) => + input.signal?.addEventListener( + "abort", + () => { + aborted = true; + resolve(); + }, + { once: true }, + ), + ); + return { status: "aborted" }; + }, + { + vmExecutor: async (_script, _args, host) => { + void host.agent("detached", {}); + await started; + return { value: null, meta: {} }; + }, + }, + ); + await expect(runtime.runScript("missing await", null)).rejects.toThrow("unawaited agent calls"); + expect(aborted).toBe(true); +}); + +test("abort during admission progress never calls the runner", async () => { + const cwd = await workspace(); + const controller = new AbortController(); + let calls = 0; + const paths = createWorkflowRunPaths(cwd, "abort-before-runner"); + const runtime = new WorkflowRuntime({ + cwd, + runId: "abort-before-runner", + name: "abort", + journal: new WorkflowJournal(paths), + signal: controller.signal, + onProgress: (progress) => { + if (progress.agents.some((agent) => agent.status === "running")) controller.abort(); + }, + runner: async () => { + calls++; + return result("unexpected"); + }, + }); + await expect(runtime.runScript('return await agent("test");', null)).rejects.toThrow(/aborted/i); + expect(calls).toBe(0); +}); + +test("uncooperative runners retain admission until settlement, then the pool can be recreated", async () => { + let started!: () => void; + const ready = new Promise((resolve) => { + started = resolve; + }); + let finish!: () => void; + const gate = new Promise((resolve) => { + finish = resolve; + }); + const first = runtimeAt( + await workspace(), + async () => { + started(); + await gate; + return result("late"); + }, + { + maxConcurrency: 1, + providerConcurrencyLimit: 1, + runnerCleanupTimeoutMs: 10, + vmExecutor: async (_script, _args, host) => { + void host.agent("uncooperative", {}); + await ready; + throw new Error("stop this run"); + }, + }, + ); + try { + await expect(first.runtime.runScript("stop", null)).rejects.toThrow("stop this run"); + const progress = JSON.parse(await readFile(first.paths.progressPath, "utf8")); + expect(progress.agents[0].error.message).toContain("shared slot remains reserved"); + let nextCalls = 0; + const next = runtimeAt( + await workspace(), + async () => { + nextCalls++; + return result("ok"); + }, + { maxConcurrency: 1, providerConcurrencyLimit: 1 }, + ); + const run = next.runtime.runScript('return await agent("next");', null); + await vi.waitFor(() => expect(next.runtime.semaphore.active).toBe(1)); + expect(nextCalls).toBe(0); + finish(); + expect((await run).status).toBe("completed"); + expect(nextCalls).toBe(1); + const recreated = acquireWorkflowAdmission(2); + expect(recreated.semaphore.active).toBe(0); + expect(recreated.semaphore.max).toBe(2); + recreated.release(); + } finally { + finish(); + } +}); + +test("two workflows share one host window and both drain their queues", async () => { + let active = 0; + let peak = 0; + let startedCount = 0; + let signalSaturated!: () => void; + const saturated = new Promise((resolve) => { + signalSaturated = resolve; + }); + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + const runner: WorkflowAgentRunner = async (input) => { + active++; + startedCount++; + peak = Math.max(peak, active); + if (active === 2) signalSaturated(); + await gate; + active--; + return result(input.prompt); + }; + const first = runtimeAt(await workspace(), runner, { maxConcurrency: 2, providerConcurrencyLimit: 2 }); + const second = runtimeAt(await workspace(), runner, { maxConcurrency: 2, providerConcurrencyLimit: 2 }); + const script = 'return await parallel([agent("a"), agent("b"), agent("c")]);'; + const runs = [first.runtime.runScript(script, null), second.runtime.runScript(script, null)]; + await saturated; + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(startedCount).toBe(2); + release(); + const results = await Promise.all(runs); + expect(results.map((run) => run.status)).toEqual(["completed", "completed"]); + expect(peak).toBe(2); + expect(startedCount).toBe(6); + expect(active).toBe(0); +}); + test("named tool profiles resolve and writable agents remain single-writer", async () => { const cwd = await workspace(); const options: WorkflowAgentOptions[] = []; diff --git a/packages/coding-agent/test/workflow-ultraloop-opt-in.test.ts b/packages/coding-agent/test/workflow-ultraloop-opt-in.test.ts index 2b0374c9..a9d92a71 100644 --- a/packages/coding-agent/test/workflow-ultraloop-opt-in.test.ts +++ b/packages/coding-agent/test/workflow-ultraloop-opt-in.test.ts @@ -13,6 +13,19 @@ afterEach(() => { vi.unstubAllEnvs(); }); +test("natural-language agent counts are left to the model, not pattern-matched as consent", () => { + // Whether "open 10 subagents" means the Agent tool, a workflow, or a question is + // the model's judgment. The harness only recognizes the explicit keyword/phrases. + for (const prompt of [ + "开10个子agent分别审查", + "launch 10 subagents", + "不要开10个子agent", + "开10个子agent要多少钱？", + ]) { + expect(detectUltraloopOptIn(prompt), prompt).toBeUndefined(); + } +}); + type Command = Parameters[1]; interface Recorded { diff --git a/packages/coding-agent/test/workflow-vm.test.ts b/packages/coding-agent/test/workflow-vm.test.ts index 22b1f77a..6c6b5e72 100644 --- a/packages/coding-agent/test/workflow-vm.test.ts +++ b/packages/coding-agent/test/workflow-vm.test.ts @@ -111,6 +111,34 @@ test("parallel() starts every task before its barrier", async () => { expect(probe.peak()).toBe(3); }); +test("parallel accepts eager promises and mixed lazy tasks without losing results", async () => { + const probe = concurrencyProbe(); + const result = await runInQuickJs( + 'return await parallel([agent("a"), () => agent("b"), agent("c")]);', + null, + probe.host, + ); + expect(result.value).toEqual(["a", "b", "c"]); + expect(probe.peak()).toBe(3); +}); + +test("parallel validates the entire batch before invoking lazy tasks", async () => { + const { host, calls } = createHost(); + await expect(runInQuickJs('return parallel([() => agent("must-not-start"), 42]);', null, host)).rejects.toThrow( + "functions or promises", + ); + expect(calls.agents).toEqual([]); +}); + +test("cancellation interrupts a guest waiting forever without host calls", async () => { + const { host } = createHost(); + const controller = new AbortController(); + const run = runInQuickJs("await new Promise(() => {});", null, host, { signal: controller.signal }); + const rejected = expect(run).rejects.toThrow("aborted"); + controller.abort(); + await rejected; +}); + test("pipeline() runs item chains concurrently and passes (prev, item, index) to stages", async () => { const probe = concurrencyProbe(); const result = await runInQuickJs( @@ -144,6 +172,10 @@ test("a throwing stage or task resolves to null instead of rejecting the wave", host, ); expect(result.value).toEqual({ piped: [11, null, 31], waved: ["ok", null, "also-ok"] }); + expect(result.taskFailures).toEqual([ + { primitive: "pipeline", index: 1, stage: 0, message: "Error: boom" }, + { primitive: "parallel", index: 1, message: "Error: boom" }, + ]); }); test("parallel() and pipeline() reject batches above the 4096-entry cap", async () => { @@ -312,26 +344,27 @@ test("keeps a fan-out branch whose host call outlasts the timeout budget", async test.each([ ["a single call", `agent("a"); return "early";`], ["a parallel wave", `parallel([() => agent("a"), () => agent("b")]); return "early";`], -])("discards %s the script never awaited", async (_label, script) => { +])("rejects and releases %s the script never awaited", async (_label, script) => { const { host } = createHost({ agent: async (prompt) => { await new Promise((resolve) => setTimeout(resolve, 50)); return prompt; }, }); - // The run returns while those calls are still in flight. Their deferred + // The run fails while those calls are still in flight. Their deferred // promises have to be released before the context goes away, or QuickJS aborts // the shared WebAssembly instance; the settlements that land afterwards must // leave the disposed context alone rather than raise an unhandled rejection. - const result = await runInQuickJs(script, null, host, { timeoutMs: 5_000 }); - expect(result.value).toBe("early"); + await expect(runInQuickJs(script, null, host, { timeoutMs: 5_000 })).rejects.toThrow("unawaited"); // Give the abandoned calls time to settle: a use-after-free would surface here. await new Promise((resolve) => setTimeout(resolve, 150)); }); test("keeps working after a run that abandoned host calls", async () => { const { host } = createHost(); - await runInQuickJs(`agent("a"); return "early";`, null, host, { timeoutMs: 5_000 }); + await expect(runInQuickJs(`agent("a"); return "early";`, null, host, { timeoutMs: 5_000 })).rejects.toThrow( + "unawaited", + ); await new Promise((resolve) => setTimeout(resolve, 50)); // The WebAssembly module is cached process-wide, so a botched teardown would // poison every later run rather than just the one that caused it. diff --git a/packages/providers/src/api/anthropic-messages.ts b/packages/providers/src/api/anthropic-messages.ts index 20f61fde..4fd36db0 100644 --- a/packages/providers/src/api/anthropic-messages.ts +++ b/packages/providers/src/api/anthropic-messages.ts @@ -29,11 +29,12 @@ import type { ToolResultMessage, } from "../types.ts"; import { splitDeferredTools } from "../utils/deferred-tools.ts"; +import { classifyProviderError, ProviderStreamError } from "../utils/error-kind.ts"; import { AssistantMessageEventStream } from "../utils/event-stream.ts"; import { headersToRecord } from "../utils/headers.ts"; import { parseJsonWithRepair, parseStreamingJson } from "../utils/json-parse.ts"; import { getPiUserAgent } from "../utils/pi-user-agent.ts"; -import { retryProviderRequest } from "../utils/provider-retry.ts"; +import { providerRetryAfterMs, retryProviderRequest } from "../utils/provider-retry.ts"; import { sanitizeSurrogates } from "../utils/sanitize-unicode.ts"; import { getJsonSchemaToolParameters, resolveJsonSchemaStrictSampling } from "./constrained-sampling.ts"; @@ -416,7 +417,7 @@ async function* iterateAnthropicEvents( for await (const sse of iterateSseMessages(response.body, signal)) { if (sse.event === "error") { - throw new Error(sse.data); + throw new ProviderStreamError(sse.data, undefined, sse.data); } if (!ANTHROPIC_MESSAGE_EVENTS.has(sse.event ?? "")) { @@ -733,6 +734,9 @@ export const stream: StreamFunction<"anthropic-messages", AnthropicOptions> = ( } output.stopReason = options?.signal?.aborted ? "aborted" : "error"; output.errorMessage = error instanceof Error ? error.message : JSON.stringify(error); + output.retryAfterMs = providerRetryAfterMs(error); + const errorKind = output.stopReason === "error" ? classifyProviderError(error) : undefined; + if (errorKind) output.errorKind = errorKind; stream.push({ type: "error", reason: output.stopReason, error: output }); stream.end(); } diff --git a/packages/providers/src/api/openai-completions.ts b/packages/providers/src/api/openai-completions.ts index d9b23849..1be20727 100644 --- a/packages/providers/src/api/openai-completions.ts +++ b/packages/providers/src/api/openai-completions.ts @@ -36,12 +36,13 @@ import type { ToolResultMessage, } from "../types.ts"; import { formatProviderError, normalizeProviderError } from "../utils/error-body.ts"; +import { classifyProviderError } from "../utils/error-kind.ts"; import { AssistantMessageEventStream } from "../utils/event-stream.ts"; import { shortHash } from "../utils/hash.ts"; import { headersToRecord } from "../utils/headers.ts"; import { parseStreamingJson } from "../utils/json-parse.ts"; import { getPiUserAgent } from "../utils/pi-user-agent.ts"; -import { retryProviderRequest } from "../utils/provider-retry.ts"; +import { providerRetryAfterMs, retryProviderRequest } from "../utils/provider-retry.ts"; import { sanitizeSurrogates } from "../utils/sanitize-unicode.ts"; import { appendGrammarToolInputJsonDelta, @@ -696,6 +697,9 @@ export const stream: StreamFunction<"openai-completions", OpenAICompletionsOptio } output.stopReason = options?.signal?.aborted ? "aborted" : "error"; output.errorMessage = formatProviderError(normalizeProviderError(error)); + output.retryAfterMs = providerRetryAfterMs(error); + const errorKind = output.stopReason === "error" ? classifyProviderError(error) : undefined; + if (errorKind) output.errorKind = errorKind; // Some providers via OpenRouter give additional information in this field. // normalizeProviderError already stringifies the parsed body (error.error) // into errorMessage, so only append the raw metadata when it is not already diff --git a/packages/providers/src/api/openai-responses-shared.ts b/packages/providers/src/api/openai-responses-shared.ts index dca994a6..ae974536 100644 --- a/packages/providers/src/api/openai-responses-shared.ts +++ b/packages/providers/src/api/openai-responses-shared.ts @@ -28,6 +28,7 @@ import type { ToolCall, Usage, } from "../types.ts"; +import { ProviderStreamError } from "../utils/error-kind.ts"; import type { AssistantMessageEventStream } from "../utils/event-stream.ts"; import { shortHash } from "../utils/hash.ts"; import { parseStreamingJson } from "../utils/json-parse.ts"; @@ -740,7 +741,7 @@ export async function processResponsesStream( } else if (event.type === "response.completed" || event.type === "response.incomplete") { finalizeResponse(event.response); } else if (event.type === "error") { - throw new Error(`Error Code ${event.code}: ${event.message}` || "Unknown error"); + throw new ProviderStreamError(`Error Code ${event.code}: ${event.message}`, event.code ?? undefined); } else if (event.type === "response.failed") { sawTerminalResponseEvent = true; output.rawStopReason = event.response?.status; @@ -751,7 +752,7 @@ export async function processResponsesStream( : details?.reason ? `incomplete: ${details.reason}` : "Unknown error (no error details in response)"; - throw new Error(msg); + throw new ProviderStreamError(msg, error?.code ?? undefined); } } if (!sawTerminalResponseEvent) { diff --git a/packages/providers/src/api/openai-responses.ts b/packages/providers/src/api/openai-responses.ts index b6d05a72..ff8af149 100644 --- a/packages/providers/src/api/openai-responses.ts +++ b/packages/providers/src/api/openai-responses.ts @@ -16,10 +16,11 @@ import type { } from "../types.ts"; import { splitDeferredTools } from "../utils/deferred-tools.ts"; import { formatProviderError, normalizeProviderError } from "../utils/error-body.ts"; +import { classifyProviderError } from "../utils/error-kind.ts"; import { AssistantMessageEventStream } from "../utils/event-stream.ts"; import { headersToRecord } from "../utils/headers.ts"; import { getPiUserAgent } from "../utils/pi-user-agent.ts"; -import { retryProviderRequest } from "../utils/provider-retry.ts"; +import { providerRetryAfterMs, retryProviderRequest } from "../utils/provider-retry.ts"; import { createGrammarToolInputProperties } from "./constrained-sampling.ts"; import { buildCopilotDynamicHeaders, hasCopilotVisionInput } from "./github-copilot-headers.ts"; import { clampOpenAIPromptCacheKey } from "./openai-prompt-cache.ts"; @@ -175,6 +176,9 @@ export const stream: StreamFunction<"openai-responses", OpenAIResponsesOptions> } output.stopReason = options?.signal?.aborted ? "aborted" : "error"; output.errorMessage = formatOpenAIResponsesError(error); + output.retryAfterMs = providerRetryAfterMs(error); + const errorKind = output.stopReason === "error" ? classifyProviderError(error) : undefined; + if (errorKind) output.errorKind = errorKind; stream.push({ type: "error", reason: output.stopReason, error: output }); stream.end(); } diff --git a/packages/providers/src/index.ts b/packages/providers/src/index.ts index ba3d48b2..7c779904 100644 --- a/packages/providers/src/index.ts +++ b/packages/providers/src/index.ts @@ -38,6 +38,7 @@ export * from "./providers/faux.ts"; export * from "./session-resources.ts"; export * from "./types.ts"; export * from "./utils/diagnostics.ts"; +export * from "./utils/error-kind.ts"; export * from "./utils/event-stream.ts"; export * from "./utils/json-parse.ts"; export * from "./utils/overflow.ts"; diff --git a/packages/providers/src/types.ts b/packages/providers/src/types.ts index e0bc30ec..79159e50 100644 --- a/packages/providers/src/types.ts +++ b/packages/providers/src/types.ts @@ -3,6 +3,7 @@ import type { AnthropicOptions } from "./api/anthropic-messages.ts"; import type { OpenAICompletionsOptions } from "./api/openai-completions.ts"; import type { OpenAIResponsesOptions } from "./api/openai-responses.ts"; import type { AssistantMessageDiagnostic } from "./utils/diagnostics.ts"; +import type { ProviderErrorKind } from "./utils/error-kind.ts"; import type { AssistantMessageEventStream } from "./utils/event-stream.ts"; export type { AssistantMessageEventStream } from "./utils/event-stream.ts"; @@ -374,6 +375,13 @@ export interface AssistantMessage { stopReason: StopReason; deferred?: DeferredHandle; errorMessage?: string; + /** + * Structured failure class derived from the HTTP status and provider error code. + * Undefined when the failure carried no machine-readable signal. + */ + errorKind?: ProviderErrorKind; + /** Server-requested backoff retained for session-level retries. */ + retryAfterMs?: number; rawStopReason?: string; /** * Provider indication of whether the model explicitly ended its turn. diff --git a/packages/providers/src/utils/error-kind.ts b/packages/providers/src/utils/error-kind.ts new file mode 100644 index 00000000..f11f57c7 --- /dev/null +++ b/packages/providers/src/utils/error-kind.ts @@ -0,0 +1,147 @@ +/** + * Structured classification of provider failures. + * + * Retry and admission decisions must not depend on how a provider happens to + * word its error text. The kind is derived from the HTTP status, the provider's + * machine-readable error `type`/`code`, and transport error codes, at the point + * where the SDK error object is still available. Errors that carry none of + * these signals stay unclassified (`undefined`) and callers fall back to their + * legacy heuristics. + */ +export type ProviderErrorKind = + /** Temporary throttling (HTTP 429 or a provider rate-limit code). Retry after a delay. */ + | "rate_limit" + /** Billing, plan, or quota exhaustion. Retrying cannot succeed. */ + | "quota" + /** Missing, invalid, or insufficient credentials. */ + | "auth" + /** Server overload, 5xx, timeouts, and dropped connections. Safe to retry. */ + | "transient" + /** The request itself was rejected (4xx other than the above). */ + | "invalid_request"; + +/** Error thrown for an error event inside an otherwise successful stream. */ +export class ProviderStreamError extends Error { + readonly code: string | undefined; + readonly error: unknown; + + constructor(message: string, code?: string, body?: unknown) { + super(message); + this.name = "ProviderStreamError"; + this.code = code; + this.error = body; + } +} + +// Machine-readable error codes/types, compared exactly, never searched in text. +const QUOTA_CODES = new Set([ + "insufficient_quota", + "billing_hard_limit_reached", + "billing_not_active", + "billing_error", + "usage_limit_reached", + "quota_exceeded", + // OpenCode Zen returns these as 429 JSON error types for subscription limits. + "GoUsageLimitError", + "FreeUsageLimitError", +]); +const RATE_LIMIT_CODES = new Set(["rate_limit_exceeded", "rate_limit_error", "rate_limited", "too_many_requests"]); +const TRANSIENT_CODES = new Set([ + "overloaded_error", + "api_error", + "server_error", + "service_unavailable", + "internal_error", + "timeout_error", +]); +const AUTH_CODES = new Set(["authentication_error", "permission_error", "invalid_api_key"]); +const NETWORK_CODES = new Set([ + "ECONNRESET", + "ECONNREFUSED", + "ECONNABORTED", + "ETIMEDOUT", + "ENOTFOUND", + "EAI_AGAIN", + "EPIPE", + "ENETUNREACH", + "EHOSTUNREACH", + "UND_ERR_SOCKET", + "UND_ERR_CONNECT_TIMEOUT", + "UND_ERR_HEADERS_TIMEOUT", + "UND_ERR_BODY_TIMEOUT", +]); +const CONNECTION_ERROR_NAMES = new Set(["APIConnectionError", "APIConnectionTimeoutError"]); + +export function classifyProviderError(error: unknown): ProviderErrorKind | undefined { + return classify(error, 0); +} + +function classify(error: unknown, depth: number): ProviderErrorKind | undefined { + if (!error || typeof error !== "object" || depth > 3) return undefined; + const record = error as Record; + const codes = collectCodes(record); + if (codes.some((code) => QUOTA_CODES.has(code))) return "quota"; + const status = statusOf(record); + if (status === 402) return "quota"; + if (status === 429 || codes.some((code) => RATE_LIMIT_CODES.has(code))) return "rate_limit"; + if (status === 401 || status === 403 || codes.some((code) => AUTH_CODES.has(code))) return "auth"; + if ( + status === 408 || + status === 409 || + (status !== undefined && status >= 500) || + codes.some((code) => TRANSIENT_CODES.has(code)) + ) { + return "transient"; + } + if (status !== undefined && status >= 400) return "invalid_request"; + if (typeof record.name === "string" && CONNECTION_ERROR_NAMES.has(record.name)) return "transient"; + if (typeof record.code === "string" && NETWORK_CODES.has(record.code)) return "transient"; + return classify(record.cause, depth + 1); +} + +function statusOf(record: Record): number | undefined { + for (const value of [ + record.status, + record.statusCode, + (record.$metadata as Record | undefined)?.httpStatusCode, + ]) { + if (typeof value === "number" && Number.isInteger(value)) return value; + } + return undefined; +} + +/** `code`/`type` fields from the SDK error and its parsed body, outermost first. */ +function collectCodes(record: Record): string[] { + const codes: string[] = []; + const visit = (value: unknown, depth: number): void => { + if (depth > 3) return; + if (typeof value === "string") { + const parsed = parseJsonObject(value); + if (parsed) visit(parsed, depth + 1); + return; + } + if (!value || typeof value !== "object" || Array.isArray(value)) return; + const item = value as Record; + for (const key of ["code", "type"]) { + const field = item[key]; + if (typeof field === "string" && field !== "error") codes.push(field); + } + visit(item.error, depth + 1); + }; + visit(record, 0); + visit(record.body, 1); + return codes; +} + +function parseJsonObject(value: string): Record | undefined { + const trimmed = value.trim(); + if (!trimmed.startsWith("{")) return undefined; + try { + const parsed: unknown = JSON.parse(trimmed); + return parsed && typeof parsed === "object" && !Array.isArray(parsed) + ? (parsed as Record) + : undefined; + } catch { + return undefined; + } +} diff --git a/packages/providers/src/utils/event-stream.ts b/packages/providers/src/utils/event-stream.ts index e5eef921..7d33537c 100644 --- a/packages/providers/src/utils/event-stream.ts +++ b/packages/providers/src/utils/event-stream.ts @@ -47,7 +47,7 @@ export class EventStream implements AsyncIterable { } } - async *[Symbol.asyncIterator](): AsyncIterator { + async *[Symbol.asyncIterator](): AsyncGenerator { while (true) { if (this.queue.length > 0) { yield this.queue.shift()!; diff --git a/packages/providers/src/utils/provider-retry.ts b/packages/providers/src/utils/provider-retry.ts index e7b0db57..f916b648 100644 --- a/packages/providers/src/utils/provider-retry.ts +++ b/packages/providers/src/utils/provider-retry.ts @@ -11,6 +11,41 @@ interface ProviderError extends Error { headers: Headers | undefined; } +/** Thrown instead of sleeping when the server asks for a delay above the cap. */ +class ProviderRetryDelayError extends Error { + /** The server's requested delay, kept only when it came from a 429. */ + readonly retryAfterMs: number | undefined; + + constructor(message: string, retryAfterMs: number | undefined, cause: ProviderError) { + // Keep the original error so its status and error code remain classifiable. + super(message, { cause }); + this.name = "ProviderRetryDelayError"; + this.retryAfterMs = retryAfterMs; + } +} + +/** Preserve Retry-After when provider retries are disabled or exhausted. */ +export function providerRetryAfterMs(error: unknown, now = Date.now()): number | undefined { + if (error instanceof ProviderRetryDelayError) { + return error.retryAfterMs !== undefined && Number.isFinite(error.retryAfterMs) ? error.retryAfterMs : undefined; + } + if (!isProviderError(error) || error.status !== 429) return undefined; + const retryAfterMs = error.headers?.get("retry-after-ms"); + const header = error.headers?.get("retry-after"); + // An empty or non-numeric retry-after-ms is not a delay; fall back to retry-after + // rather than reading it as zero. + const milliseconds = + retryAfterMs !== null && retryAfterMs !== undefined && retryAfterMs.trim() !== "" ? Number(retryAfterMs) : NaN; + const value = Number.isFinite(milliseconds) + ? milliseconds + : header + ? /^\d+(\.\d+)?$/.test(header) + ? Number(header) * 1000 + : Date.parse(header) - now + : NaN; + return Number.isFinite(value) ? Math.max(0, value) : undefined; +} + function isProviderError(error: unknown): error is ProviderError { if (!(error instanceof Error) || !("status" in error) || !("headers" in error)) return false; return ( @@ -37,12 +72,14 @@ function isRetryableProviderError(error: ProviderError): boolean { function validateServerRetryDelayMs( delayMs: number, maxRetryDelayMs: number | undefined, - providerErrorMessage: string, + error: ProviderError, ): number { const maxDelayMs = maxRetryDelayMs ?? DEFAULT_MAX_RETRY_DELAY_MS; if (maxDelayMs > 0 && delayMs > maxDelayMs) { - throw new Error( - `Server requested ${Math.ceil(delayMs / 1000)}s retry delay (max: ${Math.ceil(maxDelayMs / 1000)}s). ${providerErrorMessage}`, + throw new ProviderRetryDelayError( + `Server requested ${Math.ceil(delayMs / 1000)}s retry delay (max: ${Math.ceil(maxDelayMs / 1000)}s). ${error.message}`, + error.status === 429 ? delayMs : undefined, + error, ); } return delayMs; @@ -52,14 +89,14 @@ function getRetryDelayMs(error: ProviderError, retryIndex: number, maxRetryDelay const retryAfterMs = error.headers?.get("retry-after-ms"); if (retryAfterMs) { const value = Number.parseFloat(retryAfterMs); - if (!Number.isNaN(value)) return validateServerRetryDelayMs(value, maxRetryDelayMs, error.message); + if (!Number.isNaN(value)) return validateServerRetryDelayMs(value, maxRetryDelayMs, error); } const retryAfter = error.headers?.get("retry-after"); if (retryAfter) { const seconds = Number.parseFloat(retryAfter); const delayMs = Number.isNaN(seconds) ? Date.parse(retryAfter) - Date.now() : seconds * 1000; - return validateServerRetryDelayMs(delayMs, maxRetryDelayMs, error.message); + return validateServerRetryDelayMs(delayMs, maxRetryDelayMs, error); } const exponentialDelay = Math.min(0.5 * 2 ** retryIndex, 8) * 1000; diff --git a/packages/providers/src/utils/retry.ts b/packages/providers/src/utils/retry.ts index b17bf091..a39f93f4 100644 --- a/packages/providers/src/utils/retry.ts +++ b/packages/providers/src/utils/retry.ts @@ -73,6 +73,10 @@ const RETRYABLE_PROVIDER_ERROR_PATTERN = buildProviderErrorPattern([ "ended without", "stream ended before message_stop", "stream ended before a terminal response event", + // The agent loop finalizes a partial assistant message with this text when a + // provider stream closes before its terminal event (agent-core + // normalizeProviderStream). A truncated stream is transient and retryable. + "ended before completion", "http2 request did not get a response", // Provider-requested retry delay cap failures should flow through the outer @@ -221,7 +225,11 @@ export async function retryAssistantCall( * before restarting the assistant turn. */ export function isRetryableAssistantError(message: AssistantMessage): boolean { - if (message.stopReason !== "error" || !message.errorMessage) return false; + if (message.stopReason !== "error") return false; + // A structured classification is authoritative; text matching only covers + // failures that reached us without a status or error code. + if (message.errorKind) return message.errorKind === "rate_limit" || message.errorKind === "transient"; + if (!message.errorMessage) return false; const errorMessage = message.errorMessage; if (NON_RETRYABLE_PROVIDER_LIMIT_ERROR_PATTERN.test(errorMessage)) return false; return RETRYABLE_PROVIDER_ERROR_PATTERN.test(errorMessage); diff --git a/packages/providers/test/error-kind.test.ts b/packages/providers/test/error-kind.test.ts new file mode 100644 index 00000000..0a83fce7 --- /dev/null +++ b/packages/providers/test/error-kind.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from "vitest"; +import { fauxAssistantMessage } from "../src/providers/faux.ts"; +import { classifyProviderError, ProviderStreamError } from "../src/utils/error-kind.ts"; +import { retryProviderRequest } from "../src/utils/provider-retry.ts"; +import { isRetryableAssistantError } from "../src/utils/retry.ts"; + +function httpError(status: number | undefined, error?: unknown, extra: Record = {}): Error { + return Object.assign(new Error(`${status ?? "no"} status`), { status, headers: new Headers(), error, ...extra }); +} + +describe("classifyProviderError", () => { + it("derives the kind from the HTTP status", () => { + expect(classifyProviderError(httpError(429))).toBe("rate_limit"); + expect(classifyProviderError(httpError(402))).toBe("quota"); + expect(classifyProviderError(httpError(401))).toBe("auth"); + expect(classifyProviderError(httpError(403))).toBe("auth"); + expect(classifyProviderError(httpError(503))).toBe("transient"); + expect(classifyProviderError(httpError(408))).toBe("transient"); + expect(classifyProviderError(httpError(400))).toBe("invalid_request"); + }); + + it("separates an exhausted quota from a temporary 429 by its error code", () => { + expect( + classifyProviderError(httpError(429, { code: "insufficient_quota" }, { code: "insufficient_quota" })), + ).toBe("quota"); + expect(classifyProviderError(httpError(429, { type: "GoUsageLimitError", message: "limit" }))).toBe("quota"); + expect(classifyProviderError(httpError(429, { code: "rate_limit_exceeded" }))).toBe("rate_limit"); + }); + + it("does not classify wording: a 400 that mentions 429 or quota stays a rejected request", () => { + expect(classifyProviderError(httpError(400, { message: "429 rate limit quota exceeded" }))).toBe( + "invalid_request", + ); + expect(classifyProviderError(new Error("429 Too Many Requests"))).toBeUndefined(); + }); + + it("reads typed mid-stream errors that carry no status", () => { + const anthropicSse = '{"type":"error","error":{"type":"overloaded_error","message":"Overloaded"}}'; + expect(classifyProviderError(new ProviderStreamError(anthropicSse, undefined, anthropicSse))).toBe("transient"); + expect( + classifyProviderError(new ProviderStreamError("Error Code rate_limit_exceeded", "rate_limit_exceeded")), + ).toBe("rate_limit"); + expect(classifyProviderError(httpError(undefined, { type: "rate_limit_error" }))).toBe("rate_limit"); + }); + + it("treats connection failures as transient through the error cause", () => { + expect(classifyProviderError(Object.assign(new Error("Connection error."), { name: "APIConnectionError" }))).toBe( + "transient", + ); + const reset = Object.assign(new Error("socket hang up"), { code: "ECONNRESET" }); + expect(classifyProviderError(new TypeError("fetch failed", { cause: reset }))).toBe("transient"); + }); + + it("keeps a 429 classifiable after the retry helper refuses an oversized Retry-After", async () => { + const throttled = Object.assign(new Error("slow down"), { + status: 429, + headers: new Headers({ "retry-after": "3600" }), + }); + const failure = await retryProviderRequest(() => Promise.reject(throttled), { maxRetries: 1 }).catch( + (error: unknown) => error, + ); + expect(classifyProviderError(failure)).toBe("rate_limit"); + }); +}); + +describe("isRetryableAssistantError with a structured kind", () => { + it("trusts the kind over the error text", () => { + const quota = fauxAssistantMessage("", { stopReason: "error", errorMessage: "503 service unavailable" }); + expect(isRetryableAssistantError({ ...quota, errorKind: "quota" })).toBe(false); + const throttled = fauxAssistantMessage("", { stopReason: "error", errorMessage: "opaque" }); + expect(isRetryableAssistantError({ ...throttled, errorKind: "rate_limit" })).toBe(true); + expect(isRetryableAssistantError({ ...throttled, errorKind: "invalid_request" })).toBe(false); + }); +}); diff --git a/packages/providers/test/openai-completions-retry.test.ts b/packages/providers/test/openai-completions-retry.test.ts index 0c48b502..4e188609 100644 --- a/packages/providers/test/openai-completions-retry.test.ts +++ b/packages/providers/test/openai-completions-retry.test.ts @@ -89,6 +89,18 @@ describe("openai-completions provider retries", () => { expect(mockState.requestOptions).toEqual([expect.objectContaining({ maxRetries: 0 })]); }); + it("preserves 429 Retry-After for the session retry owner", async () => { + mockState.requestErrors = [ + Object.assign(new Error("429 concurrency reached"), { + status: 429, + headers: new Headers({ "retry-after": "2" }), + }), + ]; + const result = await consume(); + expect(result).toMatchObject({ stopReason: "error", errorKind: "rate_limit", retryAfterMs: 2000 }); + expect(mockState.requestOptions).toHaveLength(1); + }); + it("honors provider retries while keeping SDK retries disabled", async () => { vi.useFakeTimers(); mockState.requestErrors = [ diff --git a/packages/providers/test/provider-retry.test.ts b/packages/providers/test/provider-retry.test.ts index 0c5313ec..ac915ab1 100644 --- a/packages/providers/test/provider-retry.test.ts +++ b/packages/providers/test/provider-retry.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { retryProviderRequest } from "../src/utils/provider-retry.ts"; +import { providerRetryAfterMs, retryProviderRequest } from "../src/utils/provider-retry.ts"; function providerError(status: number | undefined, headers?: Record): Error { return Object.assign(new Error(`Provider error: ${status}`), { @@ -9,6 +9,25 @@ function providerError(status: number | undefined, headers?: Record { + it("parses retry hints without accepting invalid numbers or leaking headers", () => { + expect(providerRetryAfterMs(providerError(429, { "retry-after-ms": "250", "retry-after": "10" }))).toBe(250); + expect(providerRetryAfterMs(providerError(429, { "retry-after": "Thu, 01 Jan 1970 00:00:10 GMT" }), 1000)).toBe( + 9000, + ); + expect(providerRetryAfterMs(providerError(429, { "retry-after": "invalid" }))).toBeUndefined(); + expect(providerRetryAfterMs(providerError(429, { "retry-after-ms": "Infinity" }))).toBeUndefined(); + // Zero and past values are an immediate retry, never a negative or undefined wait. + expect(providerRetryAfterMs(providerError(429, { "retry-after": "0" }))).toBe(0); + expect(providerRetryAfterMs(providerError(429, { "retry-after": "-10" }))).toBe(0); + expect(providerRetryAfterMs(providerError(429, { "retry-after-ms": "-50" }))).toBe(0); + expect(providerRetryAfterMs(providerError(429, { "retry-after": "Thu, 01 Jan 1970 00:00:00 GMT" }), 5000)).toBe( + 0, + ); + // An empty or non-numeric retry-after-ms must fall back to retry-after, not read as zero. + expect(providerRetryAfterMs(providerError(429, { "retry-after-ms": "", "retry-after": "7" }))).toBe(7000); + expect(providerRetryAfterMs(providerError(429, { "retry-after-ms": " ", "retry-after": "7" }))).toBe(7000); + expect(providerRetryAfterMs(providerError(429, { "retry-after-ms": "soon", "retry-after": "7" }))).toBe(7000); + }); afterEach(() => { vi.useRealTimers(); }); @@ -46,6 +65,19 @@ describe("provider request retries", () => { expect(request).toHaveBeenCalledTimes(1); }); + it("keeps the server's 429 delay when the cap rejects it, but not for other statuses", async () => { + const capped = async (status: number): Promise => { + const request = vi + .fn<() => Promise>() + .mockRejectedValue(providerError(status, { "retry-after": "120" })); + return retryProviderRequest(request, { maxRetries: 1, maxRetryDelayMs: 1000 }).catch( + (error: unknown) => error, + ); + }; + expect(providerRetryAfterMs(await capped(429))).toBe(120_000); + expect(providerRetryAfterMs(await capped(503))).toBeUndefined(); + }); + it("allows disabling the provider-requested retry delay cap", async () => { vi.useFakeTimers(); const request = vi diff --git a/packages/providers/test/retry.test.ts b/packages/providers/test/retry.test.ts index 0d8875e0..da21f26f 100644 --- a/packages/providers/test/retry.test.ts +++ b/packages/providers/test/retry.test.ts @@ -68,6 +68,20 @@ describe("provider retry classification", () => { ).toBe(true); }); + it("retries the agent loop's truncated-stream finalization", () => { + // agent-core finalizes a partial assistant message with this text when a + // provider stream closes before its terminal event. A truncated stream is + // transient, so the session retry owner must treat it as retryable. + expect( + isRetryableAssistantError( + fauxAssistantMessage("", { + stopReason: "error", + errorMessage: "The model response ended before completion", + }), + ), + ).toBe(true); + }); + it("keeps provider limit errors non-retryable", () => { expect( isRetryableAssistantError( From 0aefefd6dab62df3077f25c853f83abce1e2308d Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Wed, 14 Oct 2026 17:50:39 +0800 Subject: [PATCH 2/2] test: make subagent concurrency checks independent of host CPU count --- .../coding-agent/test/subagent-chain-result.test.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/test/subagent-chain-result.test.ts b/packages/coding-agent/test/subagent-chain-result.test.ts index e16533cd..8e9a2a4f 100644 --- a/packages/coding-agent/test/subagent-chain-result.test.ts +++ b/packages/coding-agent/test/subagent-chain-result.test.ts @@ -1,5 +1,6 @@ +import os from "node:os"; import type { AgentToolResult } from "@step-harness/agent-core"; -import { afterEach, expect, test, vi } from "vitest"; +import { afterEach, beforeEach, expect, test, vi } from "vitest"; import type { ExtensionContext } from "../src/core/extensions/types.ts"; import type { StepSubagentDetails, @@ -10,7 +11,14 @@ import type { import { executeSubagent } from "../src/features/subagent/execute.ts"; import { acquireWorkflowAdmission, resetWorkflowAdmission } from "../src/features/workflow/admission.ts"; +beforeEach(() => { + // These tests exercise a four-slot window, independent of the CI host's CPU count. + vi.spyOn(os, "cpus").mockReturnValue(Array(6).fill(os.cpus()[0]!)); + vi.stubEnv("STEP_WORKFLOW_CONCURRENCY_LIMIT", ""); +}); + afterEach(() => { + vi.restoreAllMocks(); vi.unstubAllEnvs(); resetWorkflowAdmission(); });