From 3802d29a6074bcc911bf18e2374188e88bee20b2 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Thu, 10 Sep 2026 15:15:59 +0200 Subject: [PATCH 1/4] fix(core): retry transient failures when provisioning the run workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A run died in production today with: {"status":"failed","error":"Service Unavailable","steps":0} Zero steps. Workspace provisioning runs before step one, so a single 503 from Relaycast killed the whole run and reported an error that tells the user nothing about their flow. Relaycast sheds load with 503 under database pressure, which is exactly the shape worth retrying — the flow itself was fine, and the same flow passed on the next attempt with no changes. `ensureRelaycastApiKey` now retries `POST /v1/workspaces` on 5xx and on network errors, up to 4 attempts with linear backoff. A 4xx is the caller's fault and is never retried. `Retry-After` is honoured when present, in both the delay-seconds and HTTP-date forms, clamped to 15s so a large or malformed value cannot stall a run. When the retries are exhausted the message now says the service is shedding load rather than implying a problem with the flow, so the next person to hit this does not start debugging their YAML. Five regression tests, four of them red against the parent: retry-then-succeed, dropped connection, no-retry on 4xx, Retry-After honoured and capped, and bounded give-up with an actionable message. Test-hygiene note: the new tests restore ONLY their own `fetch` spy. This file has no global mock cleanup, and `vi.restoreAllMocks()` here tears down the module-level mocks later describes depend on — it took 28 unrelated failures to notice. 97 tests pass; the file's own baseline is 92. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFC87QQoACaBWwonRBWTEb --- .../src/__tests__/workflow-runner.test.ts | 134 +++++++++++++++++- packages/core/src/runner.ts | 84 ++++++++++- 2 files changed, 211 insertions(+), 7 deletions(-) diff --git a/packages/core/src/__tests__/workflow-runner.test.ts b/packages/core/src/__tests__/workflow-runner.test.ts index 76054cc..b7b5f6a 100644 --- a/packages/core/src/__tests__/workflow-runner.test.ts +++ b/packages/core/src/__tests__/workflow-runner.test.ts @@ -5,7 +5,7 @@ * with a mocked DB adapter and mocked AgentRelay. */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { existsSync, mkdirSync, @@ -498,6 +498,138 @@ agents: // ── Execution ────────────────────────────────────────────────────────── + describe('Relaycast workspace provisioning resilience', () => { + // Regression for a real production failure: on 2026-09-10 a run died with + // `"status":"failed","error":"Service Unavailable","steps":0`. Workspace + // provisioning runs before step one, so a single 503 from Relaycast — which + // sheds load that way under database pressure — killed the whole run with an + // error that says nothing about the user's flow. + const okWorkspace = () => + ({ + ok: true, + status: 200, + headers: { get: () => null }, + json: async () => ({ data: { api_key: 'rk_live_test' } }), + text: async () => '', + }) as unknown as Response; + const unavailable = (retryAfter?: string) => + ({ + ok: false, + status: 503, + headers: { get: (h: string) => (h.toLowerCase() === 'retry-after' ? (retryAfter ?? null) : null) }, + text: async () => 'database_overloaded', + json: async () => ({}), + }) as unknown as Response; + + function runnerIn(prefix: string) { + const tmpDir = mkdtempSync(path.join(os.tmpdir(), prefix)); + return new WorkflowRunner({ db, cwd: tmpDir }); + } + + // This file has no global mock cleanup — the other fetch-spying tests each + // restore inside their own try/finally. Restore ONLY this block's fetch spy: + // `vi.restoreAllMocks()` is too broad here and tears down the module-level + // mocks (HarnessDriverClient.spawn and friends) that later describes rely on. + let fetchSpy: ReturnType | undefined; + const spyFetch = (impl: unknown) => { + fetchSpy = vi.spyOn(globalThis, 'fetch').mockImplementation(impl as never); + return fetchSpy; + }; + afterEach(() => { + fetchSpy?.mockRestore(); + fetchSpy = undefined; + }); + + it('retries a 503 and succeeds without failing the run', async () => { + let calls = 0; + spyFetch((async (url: string) => { + // Only /v1/workspaces is a provisioning attempt; the method also fires a + // best-effort dashboard key push that must not count. + if (!String(url).includes('/v1/workspaces')) return okWorkspace(); + calls += 1; + return calls < 3 ? unavailable() : okWorkspace(); + })); + const r = runnerIn('relayflows-provision-retry-'); + vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + await (r as any).ensureRelaycastApiKey('wf-retry'); + + expect(calls).toBe(3); + expect((r as any).relayApiKey).toBe('rk_live_test'); + }); + + it('retries a dropped connection, not just an HTTP status', async () => { + let calls = 0; + spyFetch((async (url: string) => { + if (!String(url).includes('/v1/workspaces')) return okWorkspace(); + calls += 1; + if (calls === 1) throw new Error('fetch failed'); + return okWorkspace(); + })); + const r = runnerIn('relayflows-provision-network-'); + vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + await (r as any).ensureRelaycastApiKey('wf-network'); + + expect(calls).toBe(2); + expect((r as any).relayApiKey).toBe('rk_live_test'); + }); + + it('does NOT retry a 4xx — that is the caller at fault', async () => { + let calls = 0; + spyFetch((async (url: string) => { + if (!String(url).includes('/v1/workspaces')) return okWorkspace(); + calls += 1; + return { + ok: false, + status: 401, + headers: { get: () => null }, + text: async () => 'unauthorized', + } as unknown as Response; + })); + const r = runnerIn('relayflows-provision-4xx-'); + vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + await expect((r as any).ensureRelaycastApiKey('wf-4xx')).rejects.toThrow(/401/); + expect(calls).toBe(1); + }); + + it('honours Retry-After, capped so a large value cannot stall the run', async () => { + let calls = 0; + spyFetch((async (url: string) => { + if (!String(url).includes('/v1/workspaces')) return okWorkspace(); + calls += 1; + return calls === 1 ? unavailable('3600') : okWorkspace(); + })); + const r = runnerIn('relayflows-provision-retry-after-'); + const delaySpy = vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + await (r as any).ensureRelaycastApiKey('wf-retry-after'); + + // 3600s would stall the run; the ceiling clamps it. + const waited = delaySpy.mock.calls.map(([ms]) => ms as number); + expect(Math.max(...waited)).toBeLessThanOrEqual(15_000); + expect(Math.max(...waited)).toBeGreaterThan(1_000); + }); + + it('gives up after a bounded number of attempts with an actionable message', async () => { + let calls = 0; + spyFetch((async (url: string) => { + if (!String(url).includes('/v1/workspaces')) return okWorkspace(); + calls += 1; + return unavailable(); + })); + const r = runnerIn('relayflows-provision-exhaust-'); + vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + // The message must tell the user this is upstream, not their flow. + await expect((r as any).ensureRelaycastApiKey('wf-exhaust')).rejects.toThrow( + /shedding load, not a problem with this flow/ + ); + expect(calls).toBe(4); + }); + }); + describe('Relaycast base URL consistency', () => { it('uses the default origin for workspace creation, observer minting, broker, and child env', async () => { const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relayflows-base-url-')); diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index 781a273..78e9c70 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -673,6 +673,15 @@ const BROKER_OPERATION_MAX_ATTEMPTS = 3; const BROKER_OPERATION_RETRY_DELAY_MS = 1_000; const AGENT_TRANSIENT_NETWORK_MAX_ATTEMPTS = 3; const AGENT_TRANSIENT_NETWORK_RETRY_DELAY_MS = 1_000; +/** + * Workspace provisioning runs BEFORE step one, so a single transient failure + * there kills the whole run with zero steps executed and an error that says + * nothing about the user's flow ("Service Unavailable"). Relaycast sheds load + * with 503 under database pressure, which is exactly the shape worth retrying. + */ +const WORKSPACE_PROVISION_MAX_ATTEMPTS = 4; +const WORKSPACE_PROVISION_RETRY_DELAY_MS = 1_000; +const WORKSPACE_PROVISION_MAX_RETRY_AFTER_MS = 15_000; /** * The one Relayfile base URL default. @@ -2392,6 +2401,23 @@ export class WorkflowRunner { this.wireRelayClient(context.runId); } + /** + * Parse a Retry-After header. Supports both forms in RFC 9110: delay-seconds + * and an HTTP-date. Returns undefined when absent or unparseable, and clamps + * to a ceiling so a hostile or mistaken value cannot stall a run. + */ + private parseRetryAfterMs(header: string | null | undefined): number | undefined { + if (!header) return undefined; + const trimmed = header.trim(); + if (!trimmed) return undefined; + const seconds = Number(trimmed); + const ms = Number.isFinite(seconds) + ? seconds * 1000 + : Date.parse(trimmed) - Date.now(); + if (!Number.isFinite(ms) || ms <= 0) return undefined; + return Math.min(ms, WORKSPACE_PROVISION_MAX_RETRY_AFTER_MS); + } + private isRetryableProtocolError(error: unknown): boolean { const candidate = error as { retryable?: unknown; status?: unknown; message?: unknown } | undefined; if (candidate?.retryable === true) return true; @@ -2491,14 +2517,60 @@ export class WorkflowRunner { // Always create a fresh workspace — each run gets full isolation. const workspaceName = `relay-${channel}-${randomBytes(4).toString('hex')}`; const baseUrl = this.getRelaycastBaseUrl(); - const res = await fetch(`${baseUrl}/v1/workspaces`, { - method: 'POST', - headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ name: workspaceName }), - }); + // Retry transient upstream failures. This call happens before any step + // runs, so giving up here fails the run with zero steps and an opaque + // message. A 5xx or a dropped connection is the service shedding load, not + // a problem with the flow; a 4xx is the caller's fault and must not retry. + let res: Response | undefined; + let lastNetworkError: unknown; + for (let attempt = 1; attempt <= WORKSPACE_PROVISION_MAX_ATTEMPTS; attempt++) { + lastNetworkError = undefined; + try { + res = await fetch(`${baseUrl}/v1/workspaces`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ name: workspaceName }), + }); + } catch (error) { + lastNetworkError = error; + } + + const retryable = + lastNetworkError !== undefined || (res !== undefined && res.status >= 500); + if (!retryable) break; + if (attempt >= WORKSPACE_PROVISION_MAX_ATTEMPTS) break; + + // Honour Retry-After when the service tells us how long to wait, capped so + // a large or malformed value cannot stall the run indefinitely. + const retryAfterMs = this.parseRetryAfterMs(res?.headers?.get('retry-after')); + const backoffMs = WORKSPACE_PROVISION_RETRY_DELAY_MS * attempt; + const reason = lastNetworkError !== undefined ? 'network error' : `HTTP ${res?.status}`; + this.log( + `Relaycast workspace provisioning failed (${reason}); retrying ${attempt}/${WORKSPACE_PROVISION_MAX_ATTEMPTS - 1}...` + ); + await this.delay(Math.max(retryAfterMs ?? 0, backoffMs)); + } + + if (lastNetworkError !== undefined) { + const message = + lastNetworkError instanceof Error ? lastNetworkError.message : String(lastNetworkError); + throw new Error( + `Failed to reach Relaycast to create a workspace after ${WORKSPACE_PROVISION_MAX_ATTEMPTS} attempts: ${message}` + ); + } + if (!res) { + throw new Error('Failed to auto-create Relaycast workspace: no response'); + } if (!res.ok) { - throw new Error(`Failed to auto-create Relaycast workspace: ${res.status} ${await res.text()}`); + const detail = await res.text(); + const suffix = + res.status >= 500 + ? ` (still failing after ${WORKSPACE_PROVISION_MAX_ATTEMPTS} attempts; Relaycast is shedding load, not a problem with this flow)` + : ''; + throw new Error( + `Failed to auto-create Relaycast workspace: ${res.status} ${detail}${suffix}` + ); } const body = (await res.json()) as Record; From 50e9ff6a9cd8823ae653be73eefc45b51242e450 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Thu, 10 Sep 2026 15:22:01 +0200 Subject: [PATCH 2/4] fix(core): attribute pre-step setup failures and retry broker startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the workspace-provisioning retry in this branch, addressing the reason that fix had to be chosen by inference rather than evidence. ATTRIBUTION. The 2026-09-10 failure recorded exactly this: {"status":"failed","error":"Service Unavailable","steps":0} Three calls run before step one — workspace provisioning, observer minting and broker startup — and any of them can produce that message. Nothing in the run record said which. The failing call had to be GUESSED, and the guess drove which code got fixed; if the guess was wrong, the fix would have missed entirely and the same failure would have recurred looking identical. `describeSetupFailure` now tags these errors with the stage that produced them, so a failed run says `[setup:workspace-provisioning]` or `[setup:broker-startup]` instead of leaving the next person to infer it. BROKER STARTUP RETRY. `HarnessDriverClient.spawn` was the other pre-step-one call with no retry, and it registers with Relaycast, so it sheds load exactly the way workspace provisioning does. It now retries transient protocol and network failures (3 attempts, linear backoff) using the existing `isRetryableProtocolError` classification. A misconfiguration — a missing binary, a bad path — fails identically every time, so it is not retried; retrying it only delays a real error behind a longer wait. Four regression tests, three red against the parent: a tagged provisioning failure, a retried transient broker failure, a tagged exhausted broker failure, and a misconfiguration that is NOT retried. 101 tests pass; the file's baseline before this branch was 92. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFC87QQoACaBWwonRBWTEb --- .../src/__tests__/workflow-runner.test.ts | 94 +++++++++++++++++++ packages/core/src/runner.ts | 71 ++++++++++++-- 2 files changed, 159 insertions(+), 6 deletions(-) diff --git a/packages/core/src/__tests__/workflow-runner.test.ts b/packages/core/src/__tests__/workflow-runner.test.ts index b7b5f6a..5bb7d5d 100644 --- a/packages/core/src/__tests__/workflow-runner.test.ts +++ b/packages/core/src/__tests__/workflow-runner.test.ts @@ -630,6 +630,100 @@ agents: }); }); + describe('setup-stage attribution and broker startup retry', () => { + // The 2026-09-10 failure recorded a bare `Service Unavailable` with zero + // steps. Three calls run before step one — workspace provisioning, observer + // minting, broker startup — and any can produce that message, so the failing + // call had to be guessed. These pin that it no longer has to be. + let fetchSpy2: ReturnType | undefined; + afterEach(() => { + fetchSpy2?.mockRestore(); + fetchSpy2 = undefined; + }); + + const okWorkspace2 = () => + ({ + ok: true, + status: 200, + headers: { get: () => null }, + json: async () => ({ data: { api_key: 'rk_live_test' } }), + text: async () => '', + }) as unknown as Response; + + it('tags a workspace provisioning failure with its stage', async () => { + fetchSpy2 = vi.spyOn(globalThis, 'fetch').mockImplementation((async (url: string) => { + if (!String(url).includes('/v1/workspaces')) return okWorkspace2(); + return { + ok: false, + status: 503, + headers: { get: () => null }, + text: async () => 'database_overloaded', + } as unknown as Response; + }) as never); + const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relayflows-stage-ws-')); + const r = new WorkflowRunner({ db, cwd: tmpDir }); + vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + await expect((r as any).ensureRelaycastApiKey('wf-stage')).rejects.toThrow( + /\[setup:workspace-provisioning\]/ + ); + }); + + it('retries a transient broker startup failure instead of failing the run', async () => { + fetchSpy2 = vi.spyOn(globalThis, 'fetch').mockImplementation((async () => okWorkspace2()) as never); + const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relayflows-broker-retry-')); + const r = new WorkflowRunner({ db, cwd: tmpDir }); + vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + let attempts = 0; + mockHarnessDriverSpawn.mockImplementation(async () => { + attempts += 1; + if (attempts === 1) throw new Error('Service Unavailable'); + return mockRelayInstance; + }); + + await (r as any).startOrReuseSharedBroker('run-broker-retry', 'wf-broker-retry', false); + + expect(attempts).toBe(2); + }); + + it('tags an exhausted broker startup failure with its stage', async () => { + fetchSpy2 = vi.spyOn(globalThis, 'fetch').mockImplementation((async () => okWorkspace2()) as never); + const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relayflows-broker-exhaust-')); + const r = new WorkflowRunner({ db, cwd: tmpDir }); + vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + mockHarnessDriverSpawn.mockImplementation(async () => { + throw new Error('Service Unavailable'); + }); + + // The stage tag is the whole point: this is what tells the next person + // WHICH pre-step call failed instead of leaving them to infer it. + await expect( + (r as any).startOrReuseSharedBroker('run-broker-exhaust', 'wf-broker-exhaust', false) + ).rejects.toThrow(/\[setup:broker-startup\]/); + }); + + it('does not retry a broker misconfiguration', async () => { + fetchSpy2 = vi.spyOn(globalThis, 'fetch').mockImplementation((async () => okWorkspace2()) as never); + const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relayflows-broker-config-')); + const r = new WorkflowRunner({ db, cwd: tmpDir }); + vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + let attempts = 0; + mockHarnessDriverSpawn.mockImplementation(async () => { + attempts += 1; + throw new Error('binary not found: agent-relay-broker'); + }); + + await expect( + (r as any).startOrReuseSharedBroker('run-broker-config', 'wf-broker-config', false) + ).rejects.toThrow(/binary not found/); + // A misconfiguration fails identically every time; retrying only delays it. + expect(attempts).toBe(1); + }); + }); + describe('Relaycast base URL consistency', () => { it('uses the default origin for workspace creation, observer minting, broker, and child env', async () => { const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relayflows-base-url-')); diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index 78e9c70..1a2b982 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -682,6 +682,32 @@ const AGENT_TRANSIENT_NETWORK_RETRY_DELAY_MS = 1_000; const WORKSPACE_PROVISION_MAX_ATTEMPTS = 4; const WORKSPACE_PROVISION_RETRY_DELAY_MS = 1_000; const WORKSPACE_PROVISION_MAX_RETRY_AFTER_MS = 15_000; +/** + * Broker startup also registers with Relaycast, so it fails the same way and + * for the same reason as workspace provisioning — before step one, taking the + * whole run with it. + */ +const BROKER_SPAWN_MAX_ATTEMPTS = 3; +const BROKER_SPAWN_RETRY_DELAY_MS = 2_000; + +/** + * Names the setup call that failed, so a pre-step-one failure is diagnosable + * from the run record alone. + * + * A bare `Service Unavailable` in a failed run is unattributable: three + * separate calls happen before the first step (workspace provisioning, observer + * minting, broker startup) and any of them can produce it. On 2026-09-10 that + * ambiguity meant the failing call had to be GUESSED from a run record, which + * is a poor basis for choosing what to fix. + */ +export function describeSetupFailure(stage: string, error: unknown): Error { + const message = error instanceof Error ? error.message : String(error); + const tagged = new Error(`[setup:${stage}] ${message}`); + if (error instanceof Error && error.stack) tagged.stack = error.stack; + (tagged as { cause?: unknown }).cause = error; + (tagged as { setupStage?: string }).setupStage = stage; + return tagged; +} /** * The one Relayfile base URL default. @@ -2555,12 +2581,18 @@ export class WorkflowRunner { if (lastNetworkError !== undefined) { const message = lastNetworkError instanceof Error ? lastNetworkError.message : String(lastNetworkError); - throw new Error( - `Failed to reach Relaycast to create a workspace after ${WORKSPACE_PROVISION_MAX_ATTEMPTS} attempts: ${message}` + throw describeSetupFailure( + 'workspace-provisioning', + new Error( + `Failed to reach Relaycast to create a workspace after ${WORKSPACE_PROVISION_MAX_ATTEMPTS} attempts: ${message}` + ) ); } if (!res) { - throw new Error('Failed to auto-create Relaycast workspace: no response'); + throw describeSetupFailure( + 'workspace-provisioning', + new Error('Failed to auto-create Relaycast workspace: no response') + ); } if (!res.ok) { const detail = await res.text(); @@ -2568,8 +2600,9 @@ export class WorkflowRunner { res.status >= 500 ? ` (still failing after ${WORKSPACE_PROVISION_MAX_ATTEMPTS} attempts; Relaycast is shedding load, not a problem with this flow)` : ''; - throw new Error( - `Failed to auto-create Relaycast workspace: ${res.status} ${detail}${suffix}` + throw describeSetupFailure( + 'workspace-provisioning', + new Error(`Failed to auto-create Relaycast workspace: ${res.status} ${detail}${suffix}`) ); } @@ -3171,7 +3204,10 @@ export class WorkflowRunner { ? { RELAYCAST_BASE_URL: expectedBaseUrl, RELAY_BASE_URL: expectedBaseUrl } : {}), }; - this.relay = await HarnessDriverClient.spawn({ + // Broker startup registers with Relaycast, so it sheds load the same way + // workspace provisioning does — and it runs before step one, so an + // unretried blip fails the whole run with zero steps executed. + const spawnBroker = () => HarnessDriverClient.spawn({ ...this.relayOptions, cwd: brokerCwd, brokerName, @@ -3195,6 +3231,29 @@ export class WorkflowRunner { console.log(`${chalk.dim.yellow('[broker]')} ${line}`); }, }); + + let spawnError: unknown; + for (let attempt = 1; attempt <= BROKER_SPAWN_MAX_ATTEMPTS; attempt++) { + try { + this.relay = await spawnBroker(); + spawnError = undefined; + break; + } catch (error) { + spawnError = error; + // Only transient protocol/network failures are worth another go; a + // misconfiguration fails identically every time and retrying it just + // delays a real error behind a longer wait. + if (!this.isRetryableProtocolError(error) || attempt >= BROKER_SPAWN_MAX_ATTEMPTS) break; + this.log( + `Broker startup failed (${error instanceof Error ? error.message : String(error)}); retrying ${attempt}/${BROKER_SPAWN_MAX_ATTEMPTS - 1}...` + ); + await this.delay(BROKER_SPAWN_RETRY_DELAY_MS * attempt); + } + } + if (spawnError !== undefined) { + throw describeSetupFailure('broker-startup', spawnError); + } + lease.startedBroker = true; this.writeSharedBrokerOwner(lease); } finally { From 829ccefb33a57dd2e5a2e9439963822a35d84c3a Mon Sep 17 00:00:00 2001 From: Khaliq Date: Thu, 10 Sep 2026 15:26:46 +0200 Subject: [PATCH 3/4] fix(core): read the response body inside the retry attempt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from Codex review. Each is a transient failure that would still have killed a run before step one despite the retry loop. 1. `fetch` resolves as soon as headers arrive, so a connection that drops during body transfer rejects at `res.json()` — which sat AFTER the loop. That transient failure escaped the retry entirely and failed the run exactly like an unretried 503. The body read now happens inside the attempt, so a mid-body drop is classified and retried like any other network error. 2. `res` was declared outside the loop and only `lastNetworkError` was cleared per attempt. After a 503 carrying `Retry-After`, a subsequent attempt whose `fetch` REJECTED still saw the previous response, read its stale header, and waited the 15s ceiling on every following connection failure instead of the intended linear backoff. Response and failure state are now both scoped to the attempt, and `Retry-After` is only read from a response this attempt actually received. Failure state is now a single discriminated value rather than two loosely coupled variables, which is what allowed the two to disagree. Two regression tests, both red against the parent: a body-transfer drop is retried, and a network rejection after a 503 falls back to linear backoff instead of reusing the stale header. 103 tests pass; the file's baseline before this branch was 92. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFC87QQoACaBWwonRBWTEb --- .../src/__tests__/workflow-runner.test.ts | 81 +++++++++++++++++++ packages/core/src/runner.ts | 62 ++++++++------ 2 files changed, 119 insertions(+), 24 deletions(-) diff --git a/packages/core/src/__tests__/workflow-runner.test.ts b/packages/core/src/__tests__/workflow-runner.test.ts index 5bb7d5d..86e96c4 100644 --- a/packages/core/src/__tests__/workflow-runner.test.ts +++ b/packages/core/src/__tests__/workflow-runner.test.ts @@ -630,6 +630,87 @@ agents: }); }); + describe('workspace provisioning transport edge cases', () => { + // Both from Codex review on #60. Each is a transient failure that would + // still have killed a run before step one despite the retry loop. + let fetchSpy3: ReturnType | undefined; + afterEach(() => { + fetchSpy3?.mockRestore(); + fetchSpy3 = undefined; + }); + + const okWorkspace3 = () => + ({ + ok: true, + status: 200, + headers: { get: () => null }, + json: async () => ({ data: { api_key: 'rk_live_test' } }), + text: async () => '', + }) as unknown as Response; + + it('retries when the connection drops while reading the response body', async () => { + let calls = 0; + fetchSpy3 = vi.spyOn(globalThis, 'fetch').mockImplementation((async (url: string) => { + if (!String(url).includes('/v1/workspaces')) return okWorkspace3(); + calls += 1; + if (calls === 1) { + // `fetch` resolves on headers; the body transfer fails afterwards. + return { + ok: true, + status: 200, + headers: { get: () => null }, + json: async () => { + throw new Error('terminated: aborted'); + }, + text: async () => '', + } as unknown as Response; + } + return okWorkspace3(); + }) as never); + const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relayflows-body-drop-')); + const r = new WorkflowRunner({ db, cwd: tmpDir }); + vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + await (r as any).ensureRelaycastApiKey('wf-body-drop'); + + expect(calls).toBe(2); + expect((r as any).relayApiKey).toBe('rk_live_test'); + }); + + it('does not reuse a previous response Retry-After after a later connection failure', async () => { + let calls = 0; + fetchSpy3 = vi.spyOn(globalThis, 'fetch').mockImplementation((async (url: string) => { + if (!String(url).includes('/v1/workspaces')) return okWorkspace3(); + calls += 1; + if (calls === 1) { + // 503 carrying a large Retry-After, clamped to the 15s ceiling. + return { + ok: false, + status: 503, + headers: { get: (h: string) => (h.toLowerCase() === 'retry-after' ? '3600' : null) }, + text: async () => 'database_overloaded', + } as unknown as Response; + } + if (calls === 2) throw new Error('fetch failed'); + return okWorkspace3(); + }) as never); + const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relayflows-stale-retry-after-')); + const r = new WorkflowRunner({ db, cwd: tmpDir }); + const delaySpy = vi.spyOn(r as any, 'delay').mockResolvedValue(undefined); + + await (r as any).ensureRelaycastApiKey('wf-stale-retry-after'); + + const waited = delaySpy.mock.calls.map(([ms]) => ms as number); + expect(waited).toHaveLength(2); + // First wait honours the (clamped) header from that attempt's own response. + expect(waited[0]).toBe(15_000); + // The second attempt is a network rejection with NO response, so it must + // fall back to linear backoff rather than reusing the stale 503 header. + expect(waited[1]).toBe(2_000); + expect((r as any).relayApiKey).toBe('rk_live_test'); + }); + }); + describe('setup-stage attribution and broker startup retry', () => { // The 2026-09-10 failure recorded a bare `Service Unavailable` with zero // steps. Three calls run before step one — workspace provisioning, observer diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index 1a2b982..6bb0d83 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -2548,39 +2548,54 @@ export class WorkflowRunner { // runs, so giving up here fails the run with zero steps and an opaque // message. A 5xx or a dropped connection is the service shedding load, not // a problem with the flow; a 4xx is the caller's fault and must not retry. - let res: Response | undefined; - let lastNetworkError: unknown; + // + // The BODY READ is inside the attempt on purpose. `fetch` resolves as soon + // as headers arrive, so a connection that drops mid-body rejects at + // `res.json()` — after the loop, if parsing sits outside it — and that + // transient failure would kill the run exactly like an unretried 503. + let parsed: Record | undefined; + let failure: { kind: 'network'; error: unknown } | { kind: 'http'; status: number; detail: string } | undefined; for (let attempt = 1; attempt <= WORKSPACE_PROVISION_MAX_ATTEMPTS; attempt++) { - lastNetworkError = undefined; + // Both must be cleared per attempt. Leaving a previous response in scope + // makes a later network rejection read the OLD response's `Retry-After`, + // so a stale 3600s header would stall every subsequent retry instead of + // applying the intended linear backoff. + let res: Response | undefined; + failure = undefined; try { res = await fetch(`${baseUrl}/v1/workspaces`, { method: 'POST', headers: { 'content-type': 'application/json' }, body: JSON.stringify({ name: workspaceName }), }); + if (res.ok) { + parsed = (await res.json()) as Record; + } else { + failure = { kind: 'http', status: res.status, detail: await res.text() }; + } } catch (error) { - lastNetworkError = error; + failure = { kind: 'network', error }; } - const retryable = - lastNetworkError !== undefined || (res !== undefined && res.status >= 500); - if (!retryable) break; - if (attempt >= WORKSPACE_PROVISION_MAX_ATTEMPTS) break; + if (!failure) break; + const retryable = failure.kind === 'network' || failure.status >= 500; + if (!retryable || attempt >= WORKSPACE_PROVISION_MAX_ATTEMPTS) break; - // Honour Retry-After when the service tells us how long to wait, capped so - // a large or malformed value cannot stall the run indefinitely. - const retryAfterMs = this.parseRetryAfterMs(res?.headers?.get('retry-after')); + // Honour Retry-After only from THIS attempt's response, capped so a large + // or malformed value cannot stall the run indefinitely. + const retryAfterMs = + failure.kind === 'http' ? this.parseRetryAfterMs(res?.headers?.get('retry-after')) : undefined; const backoffMs = WORKSPACE_PROVISION_RETRY_DELAY_MS * attempt; - const reason = lastNetworkError !== undefined ? 'network error' : `HTTP ${res?.status}`; + const reason = failure.kind === 'network' ? 'network error' : `HTTP ${failure.status}`; this.log( `Relaycast workspace provisioning failed (${reason}); retrying ${attempt}/${WORKSPACE_PROVISION_MAX_ATTEMPTS - 1}...` ); await this.delay(Math.max(retryAfterMs ?? 0, backoffMs)); } - if (lastNetworkError !== undefined) { + if (failure?.kind === 'network') { const message = - lastNetworkError instanceof Error ? lastNetworkError.message : String(lastNetworkError); + failure.error instanceof Error ? failure.error.message : String(failure.error); throw describeSetupFailure( 'workspace-provisioning', new Error( @@ -2588,25 +2603,24 @@ export class WorkflowRunner { ) ); } - if (!res) { + if (failure?.kind === 'http') { + const suffix = + failure.status >= 500 + ? ` (still failing after ${WORKSPACE_PROVISION_MAX_ATTEMPTS} attempts; Relaycast is shedding load, not a problem with this flow)` + : ''; throw describeSetupFailure( 'workspace-provisioning', - new Error('Failed to auto-create Relaycast workspace: no response') + new Error(`Failed to auto-create Relaycast workspace: ${failure.status} ${failure.detail}${suffix}`) ); } - if (!res.ok) { - const detail = await res.text(); - const suffix = - res.status >= 500 - ? ` (still failing after ${WORKSPACE_PROVISION_MAX_ATTEMPTS} attempts; Relaycast is shedding load, not a problem with this flow)` - : ''; + if (!parsed) { throw describeSetupFailure( 'workspace-provisioning', - new Error(`Failed to auto-create Relaycast workspace: ${res.status} ${detail}${suffix}`) + new Error('Failed to auto-create Relaycast workspace: no response') ); } - const body = (await res.json()) as Record; + const body = parsed; const data = (body.data ?? body) as Record; const apiKey = data.api_key as string; From e4e090c89356fac48f54c69203de047ebbd65c78 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Thu, 10 Sep 2026 15:29:03 +0200 Subject: [PATCH 4/4] fix(core): make provisioning and broker retries cancellation-aware CodeRabbit review. Retry backoff must not outlive a cancellation: with a clamped 15s Retry-After plus linear backoff, an aborted run would keep sleeping for tens of seconds and then issue another provisioning request. Adds `abortableDelay`, which resolves immediately when the run's abort signal fires, and re-checks `checkAborted()` after each backoff so no further attempt starts. The provisioning fetch also carries the abort signal. Broker startup retries use the same delay. Regression test aborts during the first backoff and asserts exactly one request was made and the call rejects. 104 tests pass; the file's baseline before this branch was 92. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFC87QQoACaBWwonRBWTEb --- .../src/__tests__/workflow-runner.test.ts | 36 +++++++++++++++++++ packages/core/src/runner.ts | 30 ++++++++++++++-- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/packages/core/src/__tests__/workflow-runner.test.ts b/packages/core/src/__tests__/workflow-runner.test.ts index 86e96c4..d6c74ef 100644 --- a/packages/core/src/__tests__/workflow-runner.test.ts +++ b/packages/core/src/__tests__/workflow-runner.test.ts @@ -711,6 +711,42 @@ agents: }); }); + describe('provisioning retries are cancellation-aware', () => { + let fetchSpy4: ReturnType | undefined; + afterEach(() => { + fetchSpy4?.mockRestore(); + fetchSpy4 = undefined; + }); + + it('does not start another provisioning request after an abort during the delay', async () => { + let calls = 0; + fetchSpy4 = vi.spyOn(globalThis, 'fetch').mockImplementation((async (url: string) => { + if (!String(url).includes('/v1/workspaces')) { + return { ok: true, status: 200, headers: { get: () => null }, json: async () => ({}), text: async () => '' } as unknown as Response; + } + calls += 1; + return { + ok: false, + status: 503, + headers: { get: (h: string) => (h.toLowerCase() === 'retry-after' ? '3600' : null) }, + text: async () => 'database_overloaded', + } as unknown as Response; + }) as never); + const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relayflows-abort-')); + const r = new WorkflowRunner({ db, cwd: tmpDir }); + (r as any).abortController = new AbortController(); + // Abort while the first backoff is in flight. + vi.spyOn(r as any, 'abortableDelay').mockImplementation(async () => { + (r as any).abortController.abort(); + }); + + await expect((r as any).ensureRelaycastApiKey('wf-abort')).rejects.toThrow(/aborted/i); + // One attempt made, none after the abort — without the check this would + // keep retrying for tens of seconds against a cancelled run. + expect(calls).toBe(1); + }); + }); + describe('setup-stage attribution and broker startup retry', () => { // The 2026-09-10 failure recorded a bare `Service Unavailable` with zero // steps. Three calls run before step one — workspace provisioning, observer diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index 6bb0d83..5acd942 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -2567,6 +2567,7 @@ export class WorkflowRunner { method: 'POST', headers: { 'content-type': 'application/json' }, body: JSON.stringify({ name: workspaceName }), + ...(this.abortController ? { signal: this.abortController.signal } : {}), }); if (res.ok) { parsed = (await res.json()) as Record; @@ -2590,7 +2591,9 @@ export class WorkflowRunner { this.log( `Relaycast workspace provisioning failed (${reason}); retrying ${attempt}/${WORKSPACE_PROVISION_MAX_ATTEMPTS - 1}...` ); - await this.delay(Math.max(retryAfterMs ?? 0, backoffMs)); + await this.abortableDelay(Math.max(retryAfterMs ?? 0, backoffMs)); + // An abort during the delay must not start another provisioning request. + this.checkAborted(); } if (failure?.kind === 'network') { @@ -3261,7 +3264,8 @@ export class WorkflowRunner { this.log( `Broker startup failed (${error instanceof Error ? error.message : String(error)}); retrying ${attempt}/${BROKER_SPAWN_MAX_ATTEMPTS - 1}...` ); - await this.delay(BROKER_SPAWN_RETRY_DELAY_MS * attempt); + await this.abortableDelay(BROKER_SPAWN_RETRY_DELAY_MS * attempt); + this.checkAborted(); } } if (spawnError !== undefined) { @@ -12145,6 +12149,28 @@ export class WorkflowRunner { return new Promise((resolve) => setTimeout(resolve, ms)); } + /** + * A delay that gives up as soon as the run is aborted. + * + * Retry backoff must not outlive a cancellation: with a clamped 15s + * `Retry-After` plus linear backoff, a plain `delay` would keep an aborted run + * sleeping for tens of seconds and then issue another request. + */ + private abortableDelay(ms: number): Promise { + const signal = this.abortController?.signal; + if (!signal) return this.delay(ms); + if (signal.aborted) return Promise.resolve(); + return new Promise((resolve) => { + const done = () => { + clearTimeout(timer); + signal.removeEventListener('abort', done); + resolve(); + }; + const timer = setTimeout(done, ms); + signal.addEventListener('abort', done, { once: true }); + }); + } + // ── Channel messaging ────────────────────────────────────────────────── /**