diff --git a/CHANGELOG.md b/CHANGELOG.md index 58a4934..8d3aeeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to Lobster will be documented in this file. ## Unreleased +- Fix `timeout_ms` + `retry` so per-attempt timeouts retry as documented while external workflow cancellation still stops immediately. Thanks to [@KrasimirKralev](https://github.com/KrasimirKralev) (PR [#106](https://github.com/openclaw/lobster/pull/106)). + ## 2026.5.22 - Memoize Ajv schema compilation for repeated validation paths to avoid retained SchemaEnv/closure growth in long-running processes. Thanks to [@KrasimirKralev](https://github.com/KrasimirKralev) (PR [#98](https://github.com/openclaw/lobster/pull/98)) and [@cmi525](https://github.com/cmi525) (Issue [#96](https://github.com/openclaw/lobster/issues/96)). diff --git a/src/core/retry.ts b/src/core/retry.ts index aa7ee5b..dbbd4db 100644 --- a/src/core/retry.ts +++ b/src/core/retry.ts @@ -63,7 +63,9 @@ function abortableSleep(ms: number, signal?: AbortSignal): Promise { /** * Execute `fn` with retries according to the given config. - * Abort errors always propagate immediately (no retry). + * External cancellation (options.signal aborted) always propagates immediately. + * Per-attempt timeout AbortErrors flow through shouldRetry like any other error, + * so timeout_ms + retry.max combinations work as documented. * Returns the result of the first successful call, or throws * the last error after all retries are exhausted. */ @@ -81,8 +83,10 @@ export async function withRetry( try { return await fn(); } catch (err: any) { - // Never retry abort/cancellation errors - if (err?.name === "AbortError" || err?.code === "ABORT_ERR") { + // Only propagate AbortError immediately for external workflow cancellation. + // Per-attempt timeout AbortErrors (options.signal not aborted) flow through + // shouldRetry so timeout_ms + retry.max combinations work as documented. + if ((err?.name === "AbortError" || err?.code === "ABORT_ERR") && options?.signal?.aborted) { throw err; } lastError = err; diff --git a/test/step_retry.test.ts b/test/step_retry.test.ts index 0bc019d..1da07b4 100644 --- a/test/step_retry.test.ts +++ b/test/step_retry.test.ts @@ -69,8 +69,10 @@ test("withRetry throws after max exhausted", async () => { assert.equal(calls, 2); }); -test("withRetry never retries abort errors", async () => { +test("withRetry never retries external abort errors", async () => { let calls = 0; + const controller = new AbortController(); + controller.abort(); const abortErr = new DOMException("aborted", "AbortError"); await assert.rejects( withRetry( @@ -79,12 +81,29 @@ test("withRetry never retries abort errors", async () => { throw abortErr; }, resolveRetryConfig({ max: 3, delay_ms: 10 }), + { signal: controller.signal }, ), (err: any) => err.name === "AbortError", ); assert.equal(calls, 1); }); +test("withRetry retries per-attempt timeout AbortErrors when external signal is not aborted", async () => { + let calls = 0; + const timeoutAbortErr = new DOMException("step timed out", "AbortError"); + // No external signal — per-attempt timeout abort should be retriable + const result = await withRetry( + async () => { + calls++; + if (calls < 3) throw timeoutAbortErr; + return "recovered"; + }, + resolveRetryConfig({ max: 3, delay_ms: 10 }), + ); + assert.equal(result, "recovered"); + assert.equal(calls, 3); +}); + test("withRetry calls onRetry callback", async () => { const retries: number[] = []; let calls = 0; @@ -216,6 +235,37 @@ test("step retries and succeeds after transient failure", async () => { assert.ok(stderrOutput.includes("[RETRY]"), "should log retry attempts"); }); +test("step with timeout_ms + retry retries on per-attempt timeout (issue #105)", async () => { + // Behavior proof: a step that hangs past timeout_ms on its first two attempts + // must be retried (per-attempt timeout produces an AbortError that should NOT + // bypass retry) and succeed on the third. Pre-fix, withRetry short-circuited on + // any AbortError, so retry.max was inert for timed-out steps and this ran once. + const tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), "lobster-retry-")); + const counterFile = path.join(tmpDir, "counter"); + await fsp.writeFile(counterFile, "0", "utf8"); + + // The counter is incremented synchronously before the hang, so each timed-out + // (SIGKILLed) attempt is still recorded. Attempts 1-2 hang 8s (killed by the + // 3000ms timeout); attempt 3 returns immediately. The timeout leaves enough + // room for slow CI machines to start Node and write the counter before kill. + const workflow = { + name: "retry-timeout", + steps: [ + { + id: "slow", + command: `node -e "const fs=require('fs');const c=Number(fs.readFileSync('${counterFile}','utf8'))+1;fs.writeFileSync('${counterFile}',String(c));if(c<3){setTimeout(()=>{},6000);}else{process.stdout.write(JSON.stringify({attempt:c}));}"`, + timeout_ms: 3000, + retry: { max: 3, delay_ms: 50 }, + }, + ], + }; + const { result, stderrOutput } = await runWorkflow(workflow); + assert.equal(result.status, "ok"); + const output = result.output as any[]; + assert.equal(output[0].attempt, 3); + assert.ok(stderrOutput.includes("[RETRY]"), "should log retry attempts on timeout"); +}); + test("step exhausts retries and throws", async () => { const workflow = { name: "retry-exhaust",