mirror of
https://github.com/openclaw/lobster.git
synced 2026-08-14 00:48:09 +00:00
fix: retry timed-out workflow steps
* fix(retry): only propagate AbortError on external cancellation, not per-attempt timeout Fixes #105. withRetry unconditionally re-threw AbortErrors before calling shouldRetry, causing timeout_ms + retry.max combinations to always result in a single attempt regardless of retry configuration. Fix: check options?.signal?.aborted before short-circuiting — external workflow cancellation still propagates immediately, but per-attempt timeout AbortErrors now flow through shouldRetry. * test(retry): update abort-error test to use aborted external signal; add timeout-retry unit test Update withRetry test to properly simulate external cancellation (aborted signal) rather than a bare AbortError without signal context. Add unit test proving per-attempt timeout AbortErrors (no external signal) are now retried as documented when timeout_ms + retry are combined. * fix(retry): revert quote style to single-quote (match fork base) * fix(retry): revert test quote style to single-quote (match fork base) * test: add workflow-level proof that timeout_ms + retry retries on timeout Integration test that runs a real step with timeout_ms=1500 + retry.max=3 where the command hangs past the timeout on attempts 1-2 (SIGKILLed) then succeeds on attempt 3. Asserts status ok + attempt 3 + [RETRY] logs. Verified the test fails against the pre-fix withRetry (short-circuit on any AbortError) and passes with the fix. Addresses the review request for real behavior proof at the workflow level, complementing the existing withRetry unit tests. * docs: credit timeout retry fix * test: harden timeout retry workflow proof * style: format timeout retry patch --------- Co-authored-by: KrasimirKralev <krasi@idrobots.com> Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
co-authored by
KrasimirKralev
Peter Steinberger
parent
c1649457ce
commit
930930a02c
@@ -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)).
|
||||
|
||||
+7
-3
@@ -63,7 +63,9 @@ function abortableSleep(ms: number, signal?: AbortSignal): Promise<void> {
|
||||
|
||||
/**
|
||||
* 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<T>(
|
||||
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;
|
||||
|
||||
+51
-1
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user