feat: per-step AbortController timeout & signal propagation - #46
Open
Dwifax wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR adds a configurable per-step timeout and threads cancellation through step health checks and payment requests. Timed-out steps now fail with timeout-specific results, and abort signals reach the x402 and MPP clients. ChangesPer-step timeout and cancellation flow
Sequence Diagram(s)sequenceDiagram
participant PlanExecutor.execute
participant executeStep
participant checkHealth
participant makeX402Payment
participant makeMPPPayment
PlanExecutor.execute->>executeStep: start step with controller.signal
executeStep->>checkHealth: poll with signal
executeStep->>makeX402Payment: request payment with signal
executeStep->>makeMPPPayment: request payment with signal
Note over PlanExecutor.execute,executeStep: timer aborts the step controller on timeout
checkHealth-->>executeStep: stop polling on abort
makeX402Payment-->>executeStep: AbortError on cancelled request
makeMPPPayment-->>executeStep: AbortError on cancelled request
executeStep-->>PlanExecutor.execute: failed StepResult on timeout or abort
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/orchestrator/src/x402-client.ts (1)
52-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not retry or back off after abort.
An abort raised by
payingFetchis caught by the retry handler, logged, and delayed; cancellation should propagate immediately, and the retry sleep should also stop when the signal aborts.Proposed fix
+async function abortableDelay(ms: number, signal?: AbortSignal): Promise<void> { + if (!signal) { + await new Promise((r) => setTimeout(r, ms)); + return; + } + if (signal.aborted) throw new DOMException('The operation was aborted', 'AbortError'); + await new Promise<void>((resolve, reject) => { + const onAbort = () => { + clearTimeout(timer); + reject(new DOMException('The operation was aborted', 'AbortError')); + }; + const timer = setTimeout(() => { + signal.removeEventListener('abort', onAbort); + resolve(); + }, ms); + signal.addEventListener('abort', onAbort, { once: true }); + }); +} + @@ } catch (err: any) { + if ((err as Error).name === 'AbortError' || signal?.aborted) { + throw (err as Error).name === 'AbortError' + ? err + : new DOMException('The operation was aborted', 'AbortError'); + } lastError = err; if (attempt < MAX_ATTEMPTS) { @@ - await new Promise((r) => setTimeout(r, backoffMs)); + await abortableDelay(backoffMs, signal); } }Also applies to: 93-100
🤖 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/orchestrator/src/x402-client.ts` around lines 52 - 64, The retry loop in x402-client should stop treating aborts like transient failures: in the main request path around buildPayingFetch/payingFetch, detect AbortError from either the pre-check or the caught exception and rethrow immediately instead of logging, backing off, or retrying. Also update the retry delay helper used by the MAX_ATTEMPTS loop so it watches the same AbortSignal and exits as soon as the signal is aborted, preventing any sleep after cancellation.packages/orchestrator/src/executor.ts (2)
352-363: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLet payment aborts escape
executeStep.Now that payment calls receive
signal, theirAbortErrors occur inside the broad catch and become generic failed results, so the timeout-specific branch inexecuteis bypassed.Proposed fix
} catch (err: any) { + if ((err as Error).name === 'AbortError' || signal?.aborted) { + throw (err as Error).name === 'AbortError' + ? err + : new DOMException('The operation was aborted', 'AbortError'); + } const latency_ms = Date.now() - stepStart; const result = this.makeFailedResult(step, err.message ?? String(err), latency_ms);Also applies to: 400-409
🤖 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/orchestrator/src/executor.ts` around lines 352 - 363, The payment abort handling inside executeStep is being swallowed by the broad catch, which turns AbortError into a generic failure and prevents execute from reaching its timeout branch. Update executeStep so payment-related AbortError from makeX402Payment/makeMPPPayment (and other signal-aware payment calls) is rethrown or explicitly preserved instead of being converted into a failed result, while keeping non-abort errors wrapped as before. Use executeStep and the payment-call block guarded by signal to locate the change, and ensure execute can still detect the abort path correctly.
289-306: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winRe-check cancellation inside the serialized vault release.
Line 291 checks before entering
releaseSequential, but a step can time out while waiting behind another release; the callback will still callreleasePayment, releasing escrow for a cancelled step.Proposed fix
if (VAULT_ACTIVE && this.orchestratorKeypair && this.vaultTaskId !== null) { const released = await this.releaseSequential(async () => { + if (signal?.aborted) { + throw new DOMException('The operation was aborted', 'AbortError'); + } return releasePayment(this.orchestratorKeypair!, this.vaultTaskId!, amountUsdc); });🤖 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/orchestrator/src/executor.ts` around lines 289 - 306, The abort check in the vault release path only runs before entering `releaseSequential`, so a step can still proceed to `releasePayment` after timing out while queued. Update `executor.ts` in the `releaseSequential` callback to re-check `signal?.aborted` immediately before calling `releasePayment`, and short-circuit with a failed result (or skip release) if the step was cancelled. Keep the fix localized around `releaseSequential`, `releasePayment`, and `makeFailedResult` so cancelled steps never release escrow.
🤖 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.
Inline comments:
In `@packages/orchestrator/src/executor.ts`:
- Around line 97-101: The health-check polling in the executor still waits
through the full delay and treats an aborted fetch as a generic failure instead
of honoring the step timeout. Update the polling flow in executor.ts around the
health-check loop so the sleep is abort-aware using the existing step signal,
and in the fetch try/catch rethrow when the abort comes from the outer step
signal so the outer timeout path handles it; use the existing signal, delays,
and fetch health-check logic to locate the fix.
- Line 56: The STEP_TIMEOUT_MS constant in executor.ts is parsed directly from
the environment and can become NaN, 0, or negative, which can cause immediate
timer behavior and misleading step timeouts. Update the initialization around
STEP_TIMEOUT_MS to validate the parsed value as a positive finite integer and
fall back to the default 30000 whenever the env value is invalid. Keep the
change localized to the timer configuration used by executor.ts so setTimeout
always receives a safe timeout value.
- Around line 167-176: The timeout branch in executor.ts’s step execution catch
path returns a failed StepResult but does not emit the same step_failed event
used by other failures. Update the AbortError handling in the step runner around
makeFailedResult so timed-out steps also publish step_failed with the step
context and timeout message before returning the failure result, matching the
existing non-timeout failure flow.
---
Outside diff comments:
In `@packages/orchestrator/src/executor.ts`:
- Around line 352-363: The payment abort handling inside executeStep is being
swallowed by the broad catch, which turns AbortError into a generic failure and
prevents execute from reaching its timeout branch. Update executeStep so
payment-related AbortError from makeX402Payment/makeMPPPayment (and other
signal-aware payment calls) is rethrown or explicitly preserved instead of being
converted into a failed result, while keeping non-abort errors wrapped as
before. Use executeStep and the payment-call block guarded by signal to locate
the change, and ensure execute can still detect the abort path correctly.
- Around line 289-306: The abort check in the vault release path only runs
before entering `releaseSequential`, so a step can still proceed to
`releasePayment` after timing out while queued. Update `executor.ts` in the
`releaseSequential` callback to re-check `signal?.aborted` immediately before
calling `releasePayment`, and short-circuit with a failed result (or skip
release) if the step was cancelled. Keep the fix localized around
`releaseSequential`, `releasePayment`, and `makeFailedResult` so cancelled steps
never release escrow.
In `@packages/orchestrator/src/x402-client.ts`:
- Around line 52-64: The retry loop in x402-client should stop treating aborts
like transient failures: in the main request path around
buildPayingFetch/payingFetch, detect AbortError from either the pre-check or the
caught exception and rethrow immediately instead of logging, backing off, or
retrying. Also update the retry delay helper used by the MAX_ATTEMPTS loop so it
watches the same AbortSignal and exits as soon as the signal is aborted,
preventing any sleep after cancellation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7fa5a8a-2fcd-4e37-aa77-4cbd8d9b5091
📒 Files selected for processing (4)
.env.examplepackages/orchestrator/src/executor.tspackages/orchestrator/src/mpp-client.tspackages/orchestrator/src/x402-client.ts
Comment on lines
+97
to
+101
| // Bail early if the outer step timeout fired — do not waste time on further polls | ||
| if (signal?.aborted) return false; | ||
| if (delays[attempt] > 0) await new Promise((r) => setTimeout(r, delays[attempt])); | ||
| try { | ||
| const response = await fetch(agent.health_check, { signal: AbortSignal.timeout(15000) }); | ||
| const response = await fetch(agent.health_check, { signal: signal ?? AbortSignal.timeout(15000) }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make health-check sleeps abort-aware.
If the step times out during the 10s polling delay, the promise still waits for that delay; if fetch aborts, the catch converts the timeout into a generic health-check failure. Throw on the step signal so the outer timeout path handles it.
Proposed fix
+async function abortableDelay(ms: number, signal?: AbortSignal): Promise<void> {
+ if (!signal) {
+ await new Promise((r) => setTimeout(r, ms));
+ return;
+ }
+ if (signal.aborted) throw new DOMException('The operation was aborted', 'AbortError');
+ await new Promise<void>((resolve, reject) => {
+ const onAbort = () => {
+ clearTimeout(timer);
+ reject(new DOMException('The operation was aborted', 'AbortError'));
+ };
+ const timer = setTimeout(() => {
+ signal.removeEventListener('abort', onAbort);
+ resolve();
+ }, ms);
+ signal.addEventListener('abort', onAbort, { once: true });
+ });
+}
+
async function checkHealth(agent: AgentRecord, signal?: AbortSignal): Promise<boolean> {
@@
- if (signal?.aborted) return false;
- if (delays[attempt] > 0) await new Promise((r) => setTimeout(r, delays[attempt]));
+ if (signal?.aborted) throw new DOMException('The operation was aborted', 'AbortError');
+ if (delays[attempt] > 0) await abortableDelay(delays[attempt], signal);
try {
const response = await fetch(agent.health_check, { signal: signal ?? AbortSignal.timeout(15000) });
@@
- } catch {
+ } catch (err) {
+ if (signal?.aborted) throw err;
// Network error / timeout — keep retrying
}Also applies to: 105-107
🤖 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/orchestrator/src/executor.ts` around lines 97 - 101, The
health-check polling in the executor still waits through the full delay and
treats an aborted fetch as a generic failure instead of honoring the step
timeout. Update the polling flow in executor.ts around the health-check loop so
the sleep is abort-aware using the existing step signal, and in the fetch
try/catch rethrow when the abort comes from the outer step signal so the outer
timeout path handles it; use the existing signal, delays, and fetch health-check
logic to locate the fix.
Comment on lines
+167
to
+176
| .catch((err) => { | ||
| if ((err as Error).name === 'AbortError') { | ||
| // The signal was aborted either by the outer timer or by a | ||
| // propagated AbortError from an internal fetch — in both | ||
| // cases the step timed out. | ||
| return this.makeFailedResult( | ||
| step, | ||
| `Step ${step.step_id} timed out after ${STEP_TIMEOUT_MS}ms`, | ||
| Date.now() - stepStart, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Emit step_failed for timed-out steps.
The new timeout branch returns a failed StepResult but skips the step_failed event used by other failure paths, so event consumers miss timeout failures.
Proposed fix
- return this.makeFailedResult(
+ const result = this.makeFailedResult(
step,
`Step ${step.step_id} timed out after ${STEP_TIMEOUT_MS}ms`,
Date.now() - stepStart,
);
+ this.emit('step_failed', {
+ task_id,
+ step_id: step.step_id,
+ agent_name: step.agent_name,
+ error: result.error!,
+ });
+ return result;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .catch((err) => { | |
| if ((err as Error).name === 'AbortError') { | |
| // The signal was aborted either by the outer timer or by a | |
| // propagated AbortError from an internal fetch — in both | |
| // cases the step timed out. | |
| return this.makeFailedResult( | |
| step, | |
| `Step ${step.step_id} timed out after ${STEP_TIMEOUT_MS}ms`, | |
| Date.now() - stepStart, | |
| ); | |
| .catch((err) => { | |
| if ((err as Error).name === 'AbortError') { | |
| // The signal was aborted either by the outer timer or by a | |
| // propagated AbortError from an internal fetch — in both | |
| // cases the step timed out. | |
| const result = this.makeFailedResult( | |
| step, | |
| `Step ${step.step_id} timed out after ${STEP_TIMEOUT_MS}ms`, | |
| Date.now() - stepStart, | |
| ); | |
| this.emit('step_failed', { | |
| task_id, | |
| step_id: step.step_id, | |
| agent_name: step.agent_name, | |
| error: result.error!, | |
| }); | |
| return result; |
🤖 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/orchestrator/src/executor.ts` around lines 167 - 176, The timeout
branch in executor.ts’s step execution catch path returns a failed StepResult
but does not emit the same step_failed event used by other failures. Update the
AbortError handling in the step runner around makeFailedResult so timed-out
steps also publish step_failed with the step context and timeout message before
returning the failure result, matching the existing non-timeout failure flow.
Collaborator
|
lgtm. Fix failing CI and address major code rabbit comments @Dwifax |
Collaborator
|
@Dwifax check CI |
Collaborator
|
any updates? @Dwifax . Also mention associated issue in your PR |
added 3 commits
June 28, 2026 23:13
Add STEP_TIMEOUT_MS env var (default 30s). Create AbortController per step with setTimeout→abort(). Catch AbortError as timed-out step. Guard vault releasePayment with signal?.aborted to avoid wasting on-chain escrow. Propagate AbortSignal through x402-client, mpp-client, and checkHealth. Document STEP_TIMEOUT_MS in .env.example.
…e lock abort check
Dwifax
force-pushed
the
feat/step-timeout-abort
branch
from
June 28, 2026 15:21
2d79203 to
c822de3
Compare
Author
|
Pushed an update that should clear the remaining blockers:
CI is currently showing |
Collaborator
|
CI still fails. Pls address that @Dwifax |
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
2 participants
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.
Closes #27
Summary
Adds per-step execution timeout via AbortController so hung agent calls don't block the orchestrator indefinitely.
Changes
packages/orchestrator/src/executor.tsSTEP_TIMEOUT_MSenv var (default 30s)AbortControllerper step withsetTimeout→abort()executeStepin.catchto handleAbortErroras timed-out stepsignal?.abortedguard before vaultreleasePaymentsignaltocheckHealth,makeX402Payment,makeMPPPaymentpackages/orchestrator/src/x402-client.tsAbortSignalparametersignal?.abortedbetween retry attemptssignalthrough tofetchpackages/orchestrator/src/mpp-client.tsAbortSignalparametersignalthrough tomppFetch.env.exampleSTEP_TIMEOUT_MS=30000Verification
npm run typecheck— cleannpm run lint— 0 errors (pre-existing warnings only)Summary by CodeRabbit
New Features
STEP_TIMEOUT_MSto configure a per-step specialist execution timeout.AbortSignal.Bug Fixes
Documentation
STEP_TIMEOUT_MSwith guidance on timeout behavior.