From 6f8b156e67e21c9a0ffc084c22c2f509823d9f9d Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 26 May 2026 15:43:40 +0200 Subject: [PATCH 1/8] fix(workflow-executor): retry 404 in markSucceeded/markFailed to handle eventual consistency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server may return 404 on updateActivityLogStatus called too quickly after createActivityLog — the record is not yet visible on the read path. Add retry404 option to withRetry (default false, only active for mark methods). Also removes misleading "after retries" suffix from error log labels. fixes PRD-392 Co-Authored-By: Claude Sonnet 4.6 --- .../forestadmin-client-activity-log-port.ts | 8 ++-- .../src/adapters/with-retry.ts | 10 +++-- ...restadmin-client-activity-log-port.test.ts | 30 +++++++++++++- .../test/adapters/with-retry.test.ts | 41 +++++++++++++++++++ 4 files changed, 79 insertions(+), 10 deletions(-) diff --git a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts index c3232cd770..42e398a08e 100644 --- a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts +++ b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts @@ -63,10 +63,10 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort activityLog: { id: handle.id, attributes: { index: handle.index } }, status: 'completed', }), - { logger: this.logger }, + { logger: this.logger, retry404: true }, ); } catch (err) { - this.logger.error('Activity log markSucceeded failed after retries', { + this.logger.error('Activity log markSucceeded failed', { handleId: handle.id, error: extractErrorMessage(err), }); @@ -85,10 +85,10 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort activityLog: { id: handle.id, attributes: { index: handle.index } }, status: 'failed', }), - { logger: this.logger }, + { logger: this.logger, retry404: true }, ); } catch (err) { - this.logger.error('Activity log markFailed failed after retries', { + this.logger.error('Activity log markFailed failed', { handleId: handle.id, stepErrorMessage: errorMessage, error: extractErrorMessage(err), diff --git a/packages/workflow-executor/src/adapters/with-retry.ts b/packages/workflow-executor/src/adapters/with-retry.ts index a5558df329..21225d6061 100644 --- a/packages/workflow-executor/src/adapters/with-retry.ts +++ b/packages/workflow-executor/src/adapters/with-retry.ts @@ -11,16 +11,18 @@ function sleep(ms: number): Promise { }); } -function isRetryable(err: unknown): boolean { +function isRetryable(err: unknown, retry404: boolean): boolean { const { status } = err as { status?: number }; + if (typeof status !== 'number') return false; + if (retry404 && status === 404) return true; - return typeof status === 'number' && RETRYABLE_STATUS.has(status); + return RETRYABLE_STATUS.has(status); } export default async function withRetry( label: string, fn: () => Promise, - { logger }: { logger: Logger }, + { logger, retry404 = false }: { logger: Logger; retry404?: boolean }, ): Promise { let lastError: unknown; @@ -30,7 +32,7 @@ export default async function withRetry( return await fn(); } catch (err) { lastError = err; - if (!isRetryable(err) || attempt === RETRY_DELAYS_MS.length) throw err; + if (!isRetryable(err, retry404) || attempt === RETRY_DELAYS_MS.length) throw err; logger.info(`"${label}" failed, retrying`, { attempt: attempt + 1, error: extractErrorMessage(err), diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts index 33c807dbb4..7a0b4ca72b 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts @@ -183,10 +183,23 @@ describe('ForestadminClientActivityLogPort', () => { await jest.advanceTimersByTimeAsync(2_600); await expect(promise).resolves.toBeUndefined(); expect(logger.error).toHaveBeenCalledWith( - expect.stringContaining('markSucceeded failed'), + 'Activity log markSucceeded failed', expect.objectContaining({ handleId: 'log-1' }), ); }); + + it('retries on 404 — eventual consistency: record may not be visible yet on the read path', async () => { + const service = makeService(); + service.updateActivityLogStatus + .mockRejectedValueOnce(makeHttpError(404)) + .mockResolvedValueOnce(undefined); + const port = makePort(service); + + const promise = port.markSucceeded({ id: 'log-1', index: '0' }); + await jest.advanceTimersByTimeAsync(100); + await expect(promise).resolves.toBeUndefined(); + expect(service.updateActivityLogStatus).toHaveBeenCalledTimes(2); + }); }); describe('markFailed', () => { @@ -222,13 +235,26 @@ describe('ForestadminClientActivityLogPort', () => { await jest.advanceTimersByTimeAsync(2_600); await expect(promise).resolves.toBeUndefined(); expect(logger.error).toHaveBeenCalledWith( - expect.stringContaining('markFailed failed'), + 'Activity log markFailed failed', expect.objectContaining({ handleId: 'log-1', stepErrorMessage: 'step-error-msg', }), ); }); + + it('retries on 404 — eventual consistency: record may not be visible yet on the read path', async () => { + const service = makeService(); + service.updateActivityLogStatus + .mockRejectedValueOnce(makeHttpError(404)) + .mockResolvedValueOnce(undefined); + const port = makePort(service); + + const promise = port.markFailed({ id: 'log-1', index: '0' }, 'boom'); + await jest.advanceTimersByTimeAsync(100); + await expect(promise).resolves.toBeUndefined(); + expect(service.updateActivityLogStatus).toHaveBeenCalledTimes(2); + }); }); describe('drainer integration', () => { diff --git a/packages/workflow-executor/test/adapters/with-retry.test.ts b/packages/workflow-executor/test/adapters/with-retry.test.ts index 3713b58876..c1ac291fe9 100644 --- a/packages/workflow-executor/test/adapters/with-retry.test.ts +++ b/packages/workflow-executor/test/adapters/with-retry.test.ts @@ -130,4 +130,45 @@ describe('withRetry', () => { await expect(withRetry('test', fn, { logger })).rejects.toThrow('plain error'); expect(fn).toHaveBeenCalledTimes(1); }); + + it('throws immediately on 404 without retry404 flag', async () => { + const logger = makeLogger(); + const fn = jest.fn().mockRejectedValue(makeHttpError(404)); + + await expect(withRetry('test', fn, { logger })).rejects.toMatchObject({ status: 404 }); + expect(fn).toHaveBeenCalledTimes(1); + expect(logger.info).not.toHaveBeenCalled(); + }); + + it('retries on 404 when retry404: true', async () => { + const logger = makeLogger(); + const fn = jest + .fn() + .mockRejectedValueOnce(makeHttpError(404)) + .mockRejectedValueOnce(makeHttpError(404)) + .mockRejectedValueOnce(makeHttpError(404)) + .mockRejectedValueOnce(makeHttpError(404)); + + let caught: unknown; + const promise = withRetry('test', fn, { logger, retry404: true }).catch(err => { + caught = err; + }); + await jest.advanceTimersByTimeAsync(100 + 500 + 2000); + await promise; + + expect(fn).toHaveBeenCalledTimes(4); + expect((caught as { status: number }).status).toBe(404); + expect(logger.info).toHaveBeenCalledTimes(3); + }); + + it('succeeds on 2nd attempt when retry404: true', async () => { + const logger = makeLogger(); + const fn = jest.fn().mockRejectedValueOnce(makeHttpError(404)).mockResolvedValueOnce('ok'); + + const promise = withRetry('test', fn, { logger, retry404: true }); + await jest.advanceTimersByTimeAsync(100); + + await expect(promise).resolves.toBe('ok'); + expect(fn).toHaveBeenCalledTimes(2); + }); }); From 0f2b294bcd1210c6ad376855b53b9587a069a607 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 26 May 2026 15:48:24 +0200 Subject: [PATCH 2/8] fix(workflow-executor): add 404 to RETRYABLE_STATUS for eventual consistency Simpler than a per-caller flag: 404 is now retried everywhere. Callers that hit a genuine 404 still get the error after exhausting retries. fixes PRD-392 Co-Authored-By: Claude Sonnet 4.6 --- .../forestadmin-client-activity-log-port.ts | 4 +- .../src/adapters/with-retry.ts | 12 ++--- .../test/adapters/with-retry.test.ts | 52 ++++--------------- 3 files changed, 18 insertions(+), 50 deletions(-) diff --git a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts index 42e398a08e..bfd6215b04 100644 --- a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts +++ b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts @@ -63,7 +63,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort activityLog: { id: handle.id, attributes: { index: handle.index } }, status: 'completed', }), - { logger: this.logger, retry404: true }, + { logger: this.logger }, ); } catch (err) { this.logger.error('Activity log markSucceeded failed', { @@ -85,7 +85,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort activityLog: { id: handle.id, attributes: { index: handle.index } }, status: 'failed', }), - { logger: this.logger, retry404: true }, + { logger: this.logger }, ); } catch (err) { this.logger.error('Activity log markFailed failed', { diff --git a/packages/workflow-executor/src/adapters/with-retry.ts b/packages/workflow-executor/src/adapters/with-retry.ts index 21225d6061..ccb2452bbe 100644 --- a/packages/workflow-executor/src/adapters/with-retry.ts +++ b/packages/workflow-executor/src/adapters/with-retry.ts @@ -3,7 +3,7 @@ import type { Logger } from '../ports/logger-port'; import { extractErrorMessage } from '../errors'; const RETRY_DELAYS_MS = [100, 500, 2_000]; -const RETRYABLE_STATUS = new Set([408, 429, 500, 502, 503, 504]); +const RETRYABLE_STATUS = new Set([404, 408, 429, 500, 502, 503, 504]); function sleep(ms: number): Promise { return new Promise(resolve => { @@ -11,18 +11,16 @@ function sleep(ms: number): Promise { }); } -function isRetryable(err: unknown, retry404: boolean): boolean { +function isRetryable(err: unknown): boolean { const { status } = err as { status?: number }; - if (typeof status !== 'number') return false; - if (retry404 && status === 404) return true; - return RETRYABLE_STATUS.has(status); + return typeof status === 'number' && RETRYABLE_STATUS.has(status); } export default async function withRetry( label: string, fn: () => Promise, - { logger, retry404 = false }: { logger: Logger; retry404?: boolean }, + { logger }: { logger: Logger }, ): Promise { let lastError: unknown; @@ -32,7 +30,7 @@ export default async function withRetry( return await fn(); } catch (err) { lastError = err; - if (!isRetryable(err, retry404) || attempt === RETRY_DELAYS_MS.length) throw err; + if (!isRetryable(err) || attempt === RETRY_DELAYS_MS.length) throw err; logger.info(`"${label}" failed, retrying`, { attempt: attempt + 1, error: extractErrorMessage(err), diff --git a/packages/workflow-executor/test/adapters/with-retry.test.ts b/packages/workflow-executor/test/adapters/with-retry.test.ts index c1ac291fe9..8b807a9248 100644 --- a/packages/workflow-executor/test/adapters/with-retry.test.ts +++ b/packages/workflow-executor/test/adapters/with-retry.test.ts @@ -114,6 +114,17 @@ describe('withRetry', () => { expect(fn).toHaveBeenCalledTimes(4); }); + it('retries on status 404 (eventual consistency)', async () => { + const logger = makeLogger(); + const fn = jest.fn().mockRejectedValueOnce(makeHttpError(404)).mockResolvedValueOnce('ok'); + + const promise = withRetry('test', fn, { logger }); + await jest.advanceTimersByTimeAsync(100); + + await expect(promise).resolves.toBe('ok'); + expect(fn).toHaveBeenCalledTimes(2); + }); + it('throws immediately on non-retryable errors (4xx)', async () => { const logger = makeLogger(); const fn = jest.fn().mockRejectedValue(makeHttpError(400)); @@ -130,45 +141,4 @@ describe('withRetry', () => { await expect(withRetry('test', fn, { logger })).rejects.toThrow('plain error'); expect(fn).toHaveBeenCalledTimes(1); }); - - it('throws immediately on 404 without retry404 flag', async () => { - const logger = makeLogger(); - const fn = jest.fn().mockRejectedValue(makeHttpError(404)); - - await expect(withRetry('test', fn, { logger })).rejects.toMatchObject({ status: 404 }); - expect(fn).toHaveBeenCalledTimes(1); - expect(logger.info).not.toHaveBeenCalled(); - }); - - it('retries on 404 when retry404: true', async () => { - const logger = makeLogger(); - const fn = jest - .fn() - .mockRejectedValueOnce(makeHttpError(404)) - .mockRejectedValueOnce(makeHttpError(404)) - .mockRejectedValueOnce(makeHttpError(404)) - .mockRejectedValueOnce(makeHttpError(404)); - - let caught: unknown; - const promise = withRetry('test', fn, { logger, retry404: true }).catch(err => { - caught = err; - }); - await jest.advanceTimersByTimeAsync(100 + 500 + 2000); - await promise; - - expect(fn).toHaveBeenCalledTimes(4); - expect((caught as { status: number }).status).toBe(404); - expect(logger.info).toHaveBeenCalledTimes(3); - }); - - it('succeeds on 2nd attempt when retry404: true', async () => { - const logger = makeLogger(); - const fn = jest.fn().mockRejectedValueOnce(makeHttpError(404)).mockResolvedValueOnce('ok'); - - const promise = withRetry('test', fn, { logger, retry404: true }); - await jest.advanceTimersByTimeAsync(100); - - await expect(promise).resolves.toBe('ok'); - expect(fn).toHaveBeenCalledTimes(2); - }); }); From c07ed6f86c60b1ff798482d974bf121a8e684d5c Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 26 May 2026 15:54:27 +0200 Subject: [PATCH 3/8] fix(workflow-executor): improve activity log error message wording Co-Authored-By: Claude Sonnet 4.6 --- .../src/adapters/forestadmin-client-activity-log-port.ts | 4 ++-- .../adapters/forestadmin-client-activity-log-port.test.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts index bfd6215b04..1091e00a3e 100644 --- a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts +++ b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts @@ -66,7 +66,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort { logger: this.logger }, ); } catch (err) { - this.logger.error('Activity log markSucceeded failed', { + this.logger.error('Failed to mark activity log as succeeded', { handleId: handle.id, error: extractErrorMessage(err), }); @@ -88,7 +88,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort { logger: this.logger }, ); } catch (err) { - this.logger.error('Activity log markFailed failed', { + this.logger.error('Failed to mark activity log as failed', { handleId: handle.id, stepErrorMessage: errorMessage, error: extractErrorMessage(err), diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts index 7a0b4ca72b..160b836d56 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts @@ -183,7 +183,7 @@ describe('ForestadminClientActivityLogPort', () => { await jest.advanceTimersByTimeAsync(2_600); await expect(promise).resolves.toBeUndefined(); expect(logger.error).toHaveBeenCalledWith( - 'Activity log markSucceeded failed', + 'Failed to mark activity log as succeeded', expect.objectContaining({ handleId: 'log-1' }), ); }); @@ -235,7 +235,7 @@ describe('ForestadminClientActivityLogPort', () => { await jest.advanceTimersByTimeAsync(2_600); await expect(promise).resolves.toBeUndefined(); expect(logger.error).toHaveBeenCalledWith( - 'Activity log markFailed failed', + 'Failed to mark activity log as failed', expect.objectContaining({ handleId: 'log-1', stepErrorMessage: 'step-error-msg', From 21f8ad323b95566ca8eebc78e08afe215b19bb0b Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 26 May 2026 15:55:03 +0200 Subject: [PATCH 4/8] fix(workflow-executor): improve activity log error message wording (createPending) Co-Authored-By: Claude Sonnet 4.6 --- .../src/adapters/forestadmin-client-activity-log-port.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts index 1091e00a3e..8271dbee60 100644 --- a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts +++ b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts @@ -42,7 +42,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort return { id: response.id, index: response.attributes.index }; } catch (cause) { - this.logger.error('Activity log creation failed', { + this.logger.error('Failed to create activity log', { action: args.action, collectionId: args.collectionId, status: (cause as { status?: number }).status, From 2d0c033eada7c13de74a52f5f866ff428b175d46 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 26 May 2026 16:07:14 +0200 Subject: [PATCH 5/8] =?UTF-8?q?fix(workflow-executor):=20address=20review?= =?UTF-8?q?=20findings=20=E2=80=94=20status=20in=20retry=20log,=20stronger?= =?UTF-8?q?=20assertions,=20createPending=20404=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.6 --- .../workflow-executor/src/adapters/with-retry.ts | 1 + .../forestadmin-client-activity-log-port.test.ts | 13 +++++++++++++ .../test/adapters/with-retry.test.ts | 4 ++++ 3 files changed, 18 insertions(+) diff --git a/packages/workflow-executor/src/adapters/with-retry.ts b/packages/workflow-executor/src/adapters/with-retry.ts index ccb2452bbe..3027e93006 100644 --- a/packages/workflow-executor/src/adapters/with-retry.ts +++ b/packages/workflow-executor/src/adapters/with-retry.ts @@ -33,6 +33,7 @@ export default async function withRetry( if (!isRetryable(err) || attempt === RETRY_DELAYS_MS.length) throw err; logger.info(`"${label}" failed, retrying`, { attempt: attempt + 1, + status: (err as { status?: number }).status, error: extractErrorMessage(err), }); // eslint-disable-next-line no-await-in-loop diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts index 160b836d56..846217e0db 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts @@ -97,6 +97,19 @@ describe('ForestadminClientActivityLogPort', () => { expect(service.createActivityLog).toHaveBeenCalledTimes(4); }); + it('retries on 404 — same global policy as markSucceeded/markFailed, accepted tradeoff for simplicity', async () => { + const service = makeService(); + service.createActivityLog + .mockRejectedValueOnce(makeHttpError(404)) + .mockResolvedValueOnce({ id: 'log-404', attributes: { index: '0' } }); + const port = makePort(service); + + const promise = port.createPending({ renderingId: 5, action: 'update', type: 'write' }); + await jest.advanceTimersByTimeAsync(100); + await expect(promise).resolves.toEqual({ id: 'log-404', index: '0' }); + expect(service.createActivityLog).toHaveBeenCalledTimes(2); + }); + it('does not retry on 401 (not a transient error)', async () => { const service = makeService(); service.createActivityLog.mockRejectedValue(makeHttpError(401)); diff --git a/packages/workflow-executor/test/adapters/with-retry.test.ts b/packages/workflow-executor/test/adapters/with-retry.test.ts index 8b807a9248..97c470bc84 100644 --- a/packages/workflow-executor/test/adapters/with-retry.test.ts +++ b/packages/workflow-executor/test/adapters/with-retry.test.ts @@ -123,6 +123,10 @@ describe('withRetry', () => { await expect(promise).resolves.toBe('ok'); expect(fn).toHaveBeenCalledTimes(2); + expect(logger.info).toHaveBeenCalledWith( + '"test" failed, retrying', + expect.objectContaining({ attempt: 1, status: 404 }), + ); }); it('throws immediately on non-retryable errors (4xx)', async () => { From 256935a05febf3f32a3964a8dbb1dd755223038a Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 26 May 2026 16:30:13 +0200 Subject: [PATCH 6/8] =?UTF-8?q?fix(workflow-executor):=20log=20retry=20att?= =?UTF-8?q?empts=20as=20error=20=E2=80=94=20activity=20logs=20are=20critic?= =?UTF-8?q?al?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.6 --- packages/workflow-executor/src/adapters/with-retry.ts | 2 +- .../adapters/forestadmin-client-activity-log-port.test.ts | 2 +- .../workflow-executor/test/adapters/with-retry.test.ts | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/workflow-executor/src/adapters/with-retry.ts b/packages/workflow-executor/src/adapters/with-retry.ts index 3027e93006..d16812bb5d 100644 --- a/packages/workflow-executor/src/adapters/with-retry.ts +++ b/packages/workflow-executor/src/adapters/with-retry.ts @@ -31,7 +31,7 @@ export default async function withRetry( } catch (err) { lastError = err; if (!isRetryable(err) || attempt === RETRY_DELAYS_MS.length) throw err; - logger.info(`"${label}" failed, retrying`, { + logger.error(`"${label}" failed, retrying`, { attempt: attempt + 1, status: (err as { status?: number }).status, error: extractErrorMessage(err), diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts index 846217e0db..585b095716 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts @@ -77,7 +77,7 @@ describe('ForestadminClientActivityLogPort', () => { expect(handle).toEqual({ id: 'log-2', index: '1' }); expect(service.createActivityLog).toHaveBeenCalledTimes(2); - expect(logger.info).toHaveBeenCalledWith( + expect(logger.error).toHaveBeenCalledWith( expect.stringContaining('createPending'), expect.objectContaining({ attempt: 1 }), ); diff --git a/packages/workflow-executor/test/adapters/with-retry.test.ts b/packages/workflow-executor/test/adapters/with-retry.test.ts index 97c470bc84..6bd6005131 100644 --- a/packages/workflow-executor/test/adapters/with-retry.test.ts +++ b/packages/workflow-executor/test/adapters/with-retry.test.ts @@ -32,7 +32,7 @@ describe('withRetry', () => { expect(result).toBe('ok'); expect(fn).toHaveBeenCalledTimes(1); - expect(logger.info).not.toHaveBeenCalled(); + expect(logger.error).not.toHaveBeenCalled(); }); it('retries on retryable HTTP status codes (503)', async () => { @@ -44,7 +44,7 @@ describe('withRetry', () => { await expect(promise).resolves.toBe('ok'); expect(fn).toHaveBeenCalledTimes(2); - expect(logger.info).toHaveBeenCalledWith( + expect(logger.error).toHaveBeenCalledWith( '"test" failed, retrying', expect.objectContaining({ attempt: 1 }), ); @@ -123,7 +123,7 @@ describe('withRetry', () => { await expect(promise).resolves.toBe('ok'); expect(fn).toHaveBeenCalledTimes(2); - expect(logger.info).toHaveBeenCalledWith( + expect(logger.error).toHaveBeenCalledWith( '"test" failed, retrying', expect.objectContaining({ attempt: 1, status: 404 }), ); @@ -135,7 +135,7 @@ describe('withRetry', () => { await expect(withRetry('test', fn, { logger })).rejects.toMatchObject({ status: 400 }); expect(fn).toHaveBeenCalledTimes(1); - expect(logger.info).not.toHaveBeenCalled(); + expect(logger.error).not.toHaveBeenCalled(); }); it('throws immediately on errors with no status', async () => { From f643a2c288f075cd815c9f75f4373934a7ab46e1 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 26 May 2026 17:14:57 +0200 Subject: [PATCH 7/8] fix(workflow-executor): use warn for retry attempts, error for final failure Co-Authored-By: Claude Sonnet 4.6 --- packages/workflow-executor/src/adapters/with-retry.ts | 2 +- .../adapters/forestadmin-client-activity-log-port.test.ts | 2 +- .../workflow-executor/test/adapters/with-retry.test.ts | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/workflow-executor/src/adapters/with-retry.ts b/packages/workflow-executor/src/adapters/with-retry.ts index d16812bb5d..23f0ae285e 100644 --- a/packages/workflow-executor/src/adapters/with-retry.ts +++ b/packages/workflow-executor/src/adapters/with-retry.ts @@ -31,7 +31,7 @@ export default async function withRetry( } catch (err) { lastError = err; if (!isRetryable(err) || attempt === RETRY_DELAYS_MS.length) throw err; - logger.error(`"${label}" failed, retrying`, { + logger.warn(`"${label}" failed, retrying`, { attempt: attempt + 1, status: (err as { status?: number }).status, error: extractErrorMessage(err), diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts index 585b095716..8164efd893 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts @@ -77,7 +77,7 @@ describe('ForestadminClientActivityLogPort', () => { expect(handle).toEqual({ id: 'log-2', index: '1' }); expect(service.createActivityLog).toHaveBeenCalledTimes(2); - expect(logger.error).toHaveBeenCalledWith( + expect(logger.warn).toHaveBeenCalledWith( expect.stringContaining('createPending'), expect.objectContaining({ attempt: 1 }), ); diff --git a/packages/workflow-executor/test/adapters/with-retry.test.ts b/packages/workflow-executor/test/adapters/with-retry.test.ts index 6bd6005131..0c7f66b6ea 100644 --- a/packages/workflow-executor/test/adapters/with-retry.test.ts +++ b/packages/workflow-executor/test/adapters/with-retry.test.ts @@ -32,7 +32,7 @@ describe('withRetry', () => { expect(result).toBe('ok'); expect(fn).toHaveBeenCalledTimes(1); - expect(logger.error).not.toHaveBeenCalled(); + expect(logger.warn).not.toHaveBeenCalled(); }); it('retries on retryable HTTP status codes (503)', async () => { @@ -44,7 +44,7 @@ describe('withRetry', () => { await expect(promise).resolves.toBe('ok'); expect(fn).toHaveBeenCalledTimes(2); - expect(logger.error).toHaveBeenCalledWith( + expect(logger.warn).toHaveBeenCalledWith( '"test" failed, retrying', expect.objectContaining({ attempt: 1 }), ); @@ -123,7 +123,7 @@ describe('withRetry', () => { await expect(promise).resolves.toBe('ok'); expect(fn).toHaveBeenCalledTimes(2); - expect(logger.error).toHaveBeenCalledWith( + expect(logger.warn).toHaveBeenCalledWith( '"test" failed, retrying', expect.objectContaining({ attempt: 1, status: 404 }), ); @@ -135,7 +135,7 @@ describe('withRetry', () => { await expect(withRetry('test', fn, { logger })).rejects.toMatchObject({ status: 400 }); expect(fn).toHaveBeenCalledTimes(1); - expect(logger.error).not.toHaveBeenCalled(); + expect(logger.warn).not.toHaveBeenCalled(); }); it('throws immediately on errors with no status', async () => { From a9faf2deb2cebe31c8239f3ee8ecdb4901b42ddb Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 27 May 2026 15:18:25 +0200 Subject: [PATCH 8/8] fix(workflow-executor): retry 404 on markSucceeded/markFailed via extraRetryStatuses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eventual consistency: updateActivityLogStatus can 404 when called immediately after createActivityLog (record not yet visible on read path). Pass extraRetryStatuses: [404] to withRetry only for mark calls — createPending 404 stays a hard failure. Co-Authored-By: Claude Sonnet 4.6 --- .../forestadmin-client-activity-log-port.ts | 4 ++-- .../workflow-executor/src/adapters/with-retry.ts | 11 ++++++----- .../forestadmin-client-activity-log-port.test.ts | 14 ++++++-------- .../test/adapters/with-retry.test.ts | 13 +++++++++++-- 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts index 8271dbee60..bb646c579e 100644 --- a/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts +++ b/packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts @@ -63,7 +63,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort activityLog: { id: handle.id, attributes: { index: handle.index } }, status: 'completed', }), - { logger: this.logger }, + { logger: this.logger, extraRetryStatuses: [404] }, ); } catch (err) { this.logger.error('Failed to mark activity log as succeeded', { @@ -85,7 +85,7 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort activityLog: { id: handle.id, attributes: { index: handle.index } }, status: 'failed', }), - { logger: this.logger }, + { logger: this.logger, extraRetryStatuses: [404] }, ); } catch (err) { this.logger.error('Failed to mark activity log as failed', { diff --git a/packages/workflow-executor/src/adapters/with-retry.ts b/packages/workflow-executor/src/adapters/with-retry.ts index 23f0ae285e..0c8eeab6d2 100644 --- a/packages/workflow-executor/src/adapters/with-retry.ts +++ b/packages/workflow-executor/src/adapters/with-retry.ts @@ -3,7 +3,7 @@ import type { Logger } from '../ports/logger-port'; import { extractErrorMessage } from '../errors'; const RETRY_DELAYS_MS = [100, 500, 2_000]; -const RETRYABLE_STATUS = new Set([404, 408, 429, 500, 502, 503, 504]); +const RETRYABLE_STATUS = new Set([408, 429, 500, 502, 503, 504]); function sleep(ms: number): Promise { return new Promise(resolve => { @@ -11,16 +11,17 @@ function sleep(ms: number): Promise { }); } -function isRetryable(err: unknown): boolean { +function isRetryable(err: unknown, extra: number[]): boolean { const { status } = err as { status?: number }; + if (typeof status !== 'number') return false; - return typeof status === 'number' && RETRYABLE_STATUS.has(status); + return RETRYABLE_STATUS.has(status) || extra.includes(status); } export default async function withRetry( label: string, fn: () => Promise, - { logger }: { logger: Logger }, + { logger, extraRetryStatuses = [] }: { logger: Logger; extraRetryStatuses?: number[] }, ): Promise { let lastError: unknown; @@ -30,7 +31,7 @@ export default async function withRetry( return await fn(); } catch (err) { lastError = err; - if (!isRetryable(err) || attempt === RETRY_DELAYS_MS.length) throw err; + if (!isRetryable(err, extraRetryStatuses) || attempt === RETRY_DELAYS_MS.length) throw err; logger.warn(`"${label}" failed, retrying`, { attempt: attempt + 1, status: (err as { status?: number }).status, diff --git a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts index 8164efd893..bd84336434 100644 --- a/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts +++ b/packages/workflow-executor/test/adapters/forestadmin-client-activity-log-port.test.ts @@ -97,17 +97,15 @@ describe('ForestadminClientActivityLogPort', () => { expect(service.createActivityLog).toHaveBeenCalledTimes(4); }); - it('retries on 404 — same global policy as markSucceeded/markFailed, accepted tradeoff for simplicity', async () => { + it('throws immediately on 404 — not retriable for createPending (unlike markSucceeded/markFailed)', async () => { const service = makeService(); - service.createActivityLog - .mockRejectedValueOnce(makeHttpError(404)) - .mockResolvedValueOnce({ id: 'log-404', attributes: { index: '0' } }); + service.createActivityLog.mockRejectedValueOnce(makeHttpError(404)); const port = makePort(service); - const promise = port.createPending({ renderingId: 5, action: 'update', type: 'write' }); - await jest.advanceTimersByTimeAsync(100); - await expect(promise).resolves.toEqual({ id: 'log-404', index: '0' }); - expect(service.createActivityLog).toHaveBeenCalledTimes(2); + await expect( + port.createPending({ renderingId: 5, action: 'update', type: 'write' }), + ).rejects.toBeInstanceOf(ActivityLogCreationError); + expect(service.createActivityLog).toHaveBeenCalledTimes(1); }); it('does not retry on 401 (not a transient error)', async () => { diff --git a/packages/workflow-executor/test/adapters/with-retry.test.ts b/packages/workflow-executor/test/adapters/with-retry.test.ts index 0c7f66b6ea..098c525b90 100644 --- a/packages/workflow-executor/test/adapters/with-retry.test.ts +++ b/packages/workflow-executor/test/adapters/with-retry.test.ts @@ -114,11 +114,11 @@ describe('withRetry', () => { expect(fn).toHaveBeenCalledTimes(4); }); - it('retries on status 404 (eventual consistency)', async () => { + it('retries on status 404 when extraRetryStatuses includes 404', async () => { const logger = makeLogger(); const fn = jest.fn().mockRejectedValueOnce(makeHttpError(404)).mockResolvedValueOnce('ok'); - const promise = withRetry('test', fn, { logger }); + const promise = withRetry('test', fn, { logger, extraRetryStatuses: [404] }); await jest.advanceTimersByTimeAsync(100); await expect(promise).resolves.toBe('ok'); @@ -129,6 +129,15 @@ describe('withRetry', () => { ); }); + it('throws immediately on 404 without extraRetryStatuses', async () => { + const logger = makeLogger(); + const fn = jest.fn().mockRejectedValue(makeHttpError(404)); + + await expect(withRetry('test', fn, { logger })).rejects.toMatchObject({ status: 404 }); + expect(fn).toHaveBeenCalledTimes(1); + expect(logger.warn).not.toHaveBeenCalled(); + }); + it('throws immediately on non-retryable errors (4xx)', async () => { const logger = makeLogger(); const fn = jest.fn().mockRejectedValue(makeHttpError(400));