Skip to content

fix: harden workflow lifecycle, concurrency, retries, and error propagation - #247

Open
liuedcson wants to merge 7 commits into
mainfrom
fix/workflow-concurrency-lifecycle
Open

liuedcson wants to merge 7 commits into
mainfrom
fix/workflow-concurrency-lifecycle

Conversation

@liuedcson

Copy link
Copy Markdown
Collaborator

Summary

Fixes workflow/subagent concurrency, lifecycle, retry, and error-propagation defects found while reproducing a real "开10个子agent" session that ended in a rejected batch, ten ReferenceError callbacks, and a manual 8+2 rerun.

Workflow lifecycle

  • Cancel and drain running and queued children before publishing a terminal state; terminal states are sealed and the run scope is aborted on exit.
  • Failed/early-returned runs can no longer report completed with agentCalls: 0; a run that returns with unawaited agent()/iterate()/workflow() operations fails closed.
  • The terminal status stays authoritative even when a progress write fails (a full disk can no longer turn a failed run into a hung one or replace the real error).

Concurrency

  • Concurrent workflows share one process-wide admission window; an uncooperative runner keeps its reservation until settlement instead of permanently poisoning the window.
  • Requested concurrency is clamped to a CPU-derived ceiling (max(1, min(16, cpus-2)), optionally lowered by STEP_WORKFLOW_CONCURRENCY_LIMIT, which now ignores blank/invalid values). Clamping is reported as a notice, never as a failure.
  • parallel() accepts functions or promises; ordinary subagent batches larger than the concurrency window queue the excess instead of being rejected (an explicitly configured maxParallelTasks still bounds batch size).
  • Explicit counted fan-out requests ("launch 10 subagents" / "开10个子agent") grant per-turn workflow consent without the Ultraloop keyword.

Error propagation

  • parallel()/pipeline() callback failures are recorded as taskFailures (with script attribution for nested workflows) and surfaced in the live text rows and final result; agent-node failures are no longer double-counted as callback failures.
  • AgentToolResult.isError from child tool results now propagates instead of being reported to the model as success.
  • Temporary 429s get an independent retry budget with Retry-After support and adaptive admission-cap reduction/recovery; terminal quota errors (including OpenCode Zen GoUsageLimitError/FreeUsageLimitError) are never retried.
  • The agent loop finalizes a partial assistant message when a provider stream throws, ends early, or fails during construction — without reclassifying listener/persistence errors as model errors or emitting a second terminal message_end.

Tests

  • New: shell-interpolation regression (broken template → failed with 10 ReferenceErrors; plain string → 10 agents dispatched), nested callback attribution, no-double-counting, unawaited nested workflow, retained-reservation/pool recreation, real child-process exit drain behind a detached descendant, terminal progress-write failure, batch-size vs concurrency semantics, counted opt-in routing, 429 classification.
  • Full suites: coding-agent 4469 passed (3 pre-existing completion-check-cli failures, identical on main), agent-core 536, providers 437; pnpm check clean.
  • Live validation with the user's original prompt: first and only tool call is workflow, observed 8 running + 2 queued with refill before the first wave finished, 10/10 completed in ~23.5s, and each reported value matched the agent's actual command output.

…gation

Workflow runtime:
- Cancel and drain running and queued children before publishing a terminal
  state, seal terminal states, and abort the run scope on exit.
- Share a process-wide admission window across concurrent workflows, clamp
  requested concurrency to a CPU-derived ceiling (optionally lowered by
  STEP_WORKFLOW_CONCURRENCY_LIMIT), and report clamping as a notice instead of
  a failure.
- Record parallel()/pipeline callback failures as taskFailures, including
  nested workflow failures attributed to their script, so a run whose callbacks
  threw can never report completed with zero agent calls.
- Keep agent-node failures out of the callback channel, and fail runs that
  return with unawaited agent/iterate/workflow operations.
- Keep the terminal status authoritative even when a progress write fails.

Subagents and child processes:
- Accept batches larger than the concurrency window and queue the excess; an
  explicitly configured maxParallelTasks still bounds batch size.
- Wait for the OS child to exit through the shared stdio-drain helper so a
  detached descendant holding pipes cannot poison the shared window.
- Propagate AgentToolResult.isError from child tool results.

Retries and streaming:
- Give temporary 429s an independent retry budget with Retry-After support,
  adaptive admission-cap reduction and recovery, and no retries for terminal
  quota errors.
- Finalize a partial assistant message when a provider stream throws, ends
  early, or fails during construction, without reclassifying listener or
  persistence errors as model errors or emitting a second terminal message.
@liuedcson

Copy link
Copy Markdown
Collaborator Author

Heads-up on the first CI run: build-check-test failed once on a 30s test timeout in test/suite/agent-session-tool-output.test.ts > keeps the tail and exit status of a failed command that bash already truncated (a real bash subprocess test on the shared 4-vCPU runner). All 4474 other tests passed in that run. I re-ran the failed job on the identical commit and it passed in 6m22s, so it is a runner flake, not a regression — that test file passes in ~2s locally and is untouched by this change.

A node's own retry honors the server's Retry-After verbatim, but that same
value was passed to the admission window's cooldown. The window is shared by
every workflow in the process and its queue time is excluded from node
timeouts, so a large Retry-After (hours, or a far-future HTTP date) froze
agent() for unrelated workflows indefinitely: nodes sat queued with no error,
no timeout, and no way to fail fast, and the drain timer was re-armed at the
2^31-1 ms ceiling.

Clamp the cooldown to 30 seconds at the semaphore choke point, matching the
cap already used for the jittered backoff fallback. The 25% cap reduction per
congestion epoch, not the cooldown length, is what protects the provider, and
the node's retry still waits the full server-requested delay.
Recording a node failure awaited the journal append and the progress write
before the failure was classified. A disk error (EACCES/ENOSPC/EISDIR) or a
throwing onProgress callback therefore replaced the node's real error, skipped
the nodeErrors marker, and let parallel()/pipeline() report the same failure a
second time as a callback error: one failed node became "1 failed nodes and 1
failed callbacks". The runner-reported failure path had the same problem, where
the journal error surfaced as the node's errorMessage.

Classify before recording, and make every failure-recording write best effort:
the node already failed, so a persistence error must not change the terminal
error or double-count the failure. A completed node's journal append stays
strict, since that is a real persistence failure worth surfacing.

Also tighten the counted fan-out opt-in. Asking about a request is not consent
to perform it, so "开10个子agent要多少钱?", "开10个子agent是不是太多了?" and
"Launch 10 agents? That seems excessive." no longer opt the turn into a
multi-agent workflow, while refusals and deliberation ("还是先用subagent工具吧")
still cancel it. The phrase may now sit anywhere in the message, so "麻烦开3个
子agent查一下", "先看代码,然后开10个子agent", bare "开10个agent", full-width
digits, and mid-sentence English requests are recognized.

Each behavior is pinned by a regression test; removing either fix fails them.
The provider retry-hint parser clamps negative and past HTTP-date values to
zero, but only the positive, invalid, and unbounded cases were pinned. Zero
and past values must never surface as a negative wait or as undefined.
…nt paths

Adversarial review of the finished change set found several real gaps:

Consent detection (ultraloop-opt-in.ts):
- English refusals and hedging were read as consent: "Please don't launch 10
  subagents", "never run 3 agents here", "Rather than launch 10 agents, just
  read the file", and "I might launch 10 subagents later" all opted the turn
  into a multi-agent workflow. Refusal and hedging now cancel a counted
  request in both languages; the asymmetry is deliberate, because launching
  agents the user declined costs more than missing an opt-in.
- The bare "agent" noun over-matched: "create 3 agent definition files",
  "创建3个agent配置文件", "start 4 agentic tests", "run 3 agents.md files" and
  "start 3 agent-based tests" all counted as fan-out requests. The noun now
  has to be a fan-out target, so a following word character, hyphen, dot, or
  file/role noun rejects the match.
- "别" matched inside "特别", so "开10个子agent,特别是review" was silently
  dropped. A lookbehind keeps real negations working.

Workflow runtime:
- appendFailure is now best effort inside the function instead of at four of
  six call sites. A journal write error (EACCES/ENOSPC/EISDIR) replaced the
  node's real error at the attempt, timeout, abort, and budget sites, so a
  provider failure surfaced as a filesystem error and lost its rate-limit
  classification.
- A node label is no longer read as a rate limit: "fix 429" used to match
  isRateLimitError on the synthesized timeout message and throttle the shared
  window for every workflow in the process.

Progress store:
- One failed write poisoned the queue permanently, because the rejection was
  chained into writeQueue. progress.json then stayed at its last successful
  state, usually "running", forever. The queue now recovers like the
  journal's does.

Providers:
- An empty or non-numeric retry-after-ms read as zero and skipped a valid
  retry-after; it now falls back to the seconds header.
- The agent loop's truncated-stream finalization ("The model response ended
  before completion") matched no retryable pattern, so a stream that closed
  early was a hard failure instead of a retry.

Docs: agentTimeoutMs is per schema attempt (up to 3x), and the ordinary
subagent tool shares the CPU-derived ceiling formula but not the configured
provider limit or the workflow admission window.
…fetime

Both are observable behaviors a reader could otherwise mistake for bugs: a
callback that rethrows a new error around an agent-node failure counts as one
failed node plus one callback failure, and the process-wide admission window
is released when the last concurrent workflow finishes.
…te usage

Congestion is now read only from text the model request reported as the
failure. Previously the whole message was classified, so agent output ("the
429 rate limit docs were unreachable"), a node label ("fix 429"), or a tool's
own "timed out after 30s" text throttled the process-wide window for every
workflow and mislabelled the node. NODE_TIMEOUT now comes from a flag set by
our own timer instead of a substring match.

A cancelled or timed-out attempt still spends tokens, and the runner reports
them after the race has already been decided. Cleanup moved into a shared
settleRunner() that returns the late result, so its usage is merged and charged
before the failure is journalled. The non-settling-runner slot retention is
unchanged.

An in-turn resample emits no "started" event, so the request epoch tag went
stale after a throttle and every later success was ignored, which stopped the
cap from recovering. The tag is refreshed after each success.

progress.json is a projection: updateProgress now survives both a failed write
and a throwing live-update listener. A listener error used to escape and turn a
single node failure into a failed node plus a callback failure, and could kill
a run at its first progress write.

The provider retry-delay cap threw a plain Error, so the server's requested
delay was lost and the session fell back to its own backoff. It now throws a
ProviderRetryDelayError that carries the delay for 429s, and providerRetryAfterMs
reads it. The message text is unchanged.

A throttled admission window is remembered for 60 seconds after the last
workflow finishes instead of being discarded, because sessions run workflows
back to back and restarting at the full ceiling straight after a 429 re-triggers
the congestion. An untouched window is still discarded at once, and a retained
one is parked so it holds no timer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant