From c52b8d6191ca60c3b255c7941f26413fd265a55d Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 1 Jun 2026 18:36:42 +0200 Subject: [PATCH 1/2] fix(workflow-executor): skip activity-log update when server returns no id (PRD-428) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Forest server returns HTTP 200 with `id: null` when it silently declines to persist an activity log (e.g. a request without a collection fails the authorization check). createPending used to return that handle verbatim, so the subsequent mark-as-completed/failed PATCH targeted `/{index}/null/status` and got a permanent 404 — retried 3x (404 is in extraRetryStatuses) before a swallowed error, polluting logs and burning background latency on every affected step. createPending now warns once when the server returns no id. markSucceeded and markFailed early-return when the handle has no id, killing the 404 storm at the source. 404 stays retriable for valid ids (transient read-path propagation). fixes PRD-428 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../forestadmin-client-activity-log-port.ts | 16 ++++++- ...restadmin-client-activity-log-port.test.ts | 42 +++++++++++++++++++ 2 files changed, 57 insertions(+), 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 07e0dcf97c..719ee90c7e 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 @@ -40,7 +40,15 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort { logger: this.logger }, ); - return { id: response.id, index: response.attributes.index }; + // Server returns 200 with id:null when it silently declines to persist (e.g. no collection). + if (!response.id) { + this.logger.warn('activity log not persisted by server — skipping status update', { + action: args.action, + collectionId: args.collectionId, + }); + } + + return { id: response.id, index: response.attributes?.index }; } catch (cause) { this.logger.error('activity log create failed', { action: args.action, @@ -53,6 +61,9 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort } async markSucceeded(handle: ActivityLogHandle): Promise { + // No id → log not persisted (see createPending); skip to avoid a permanent-404 retry storm. + if (!handle.id) return undefined; + return this.drainer.track(async () => { try { await withRetry( @@ -75,6 +86,9 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort } async markFailed(handle: ActivityLogHandle, errorMessage: string): Promise { + // No id → log not persisted (see createPending); skip to avoid a permanent-404 retry storm. + if (!handle.id) return undefined; + return this.drainer.track(async () => { try { await withRetry( 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 1614b330ab..32a650bdc9 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 @@ -166,9 +166,39 @@ describe('ForestadminClientActivityLogPort', () => { expect.objectContaining({ collectionName: '11' }), ); }); + + it('warns and returns a non-persisted handle when the server responds 200 with id:null', async () => { + const service = makeService(); + service.createActivityLog.mockResolvedValue({ + id: null, + attributes: {}, + } as unknown as { id: string; attributes: { index: string } }); + const logger = makeLogger(); + const port = makePort(service, { logger }); + + const handle = await port.createPending({ renderingId: 5, action: 'action', type: 'write' }); + + expect(handle.id).toBeNull(); + expect(logger.warn).toHaveBeenCalledWith( + 'activity log not persisted by server — skipping status update', + expect.objectContaining({ action: 'action' }), + ); + }); }); describe('markSucceeded', () => { + it('skips the status update (no 404 storm) when the handle was not persisted (id absent)', async () => { + const service = makeService(); + const port = makePort(service); + + await port.markSucceeded({ id: null, index: undefined } as unknown as { + id: string; + index: string; + }); + + expect(service.updateActivityLogStatus).not.toHaveBeenCalled(); + }); + it('retries on 503 and eventually resolves without rethrowing', async () => { const service = makeService(); service.updateActivityLogStatus @@ -214,6 +244,18 @@ describe('ForestadminClientActivityLogPort', () => { }); describe('markFailed', () => { + it('skips the status update (no 404 storm) when the handle was not persisted (id absent)', async () => { + const service = makeService(); + const port = makePort(service); + + await port.markFailed( + { id: null, index: undefined } as unknown as { id: string; index: string }, + 'boom', + ); + + expect(service.updateActivityLogStatus).not.toHaveBeenCalled(); + }); + it('sends status: failed (no errorMessage — server schema rejects unknown fields) and retries on 503', async () => { const service = makeService(); service.updateActivityLogStatus From 1ae6cc3c7863267021ee35fb9bcf24447a2b9efd Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 1 Jun 2026 19:04:29 +0200 Subject: [PATCH 2/2] refactor(workflow-executor): model not-persisted activity-log handle as a union (PRD-428) Addresses PR review feedback: - ActivityLogHandle becomes `{ id: string; index: string } | { id: null }` so the not-persisted case is type-honest; guards narrow via `'index' in handle` and the optional chaining + `as unknown as` test casts are gone. - Soften the createPending comment (observed behavior, cite PRD-428); markFailed back-references markSucceeded instead of duplicating the rationale. - Strengthen tests: assert the full `{ id: null }` handle, that drainer.track is not called on the skip path, and that collectionId propagates into the warn. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../forestadmin-client-activity-log-port.ts | 29 +++++++++----- .../src/ports/activity-log-port.ts | 7 ++-- ...restadmin-client-activity-log-port.test.ts | 40 +++++++++++-------- 3 files changed, 44 insertions(+), 32 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 719ee90c7e..783cd63a19 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 @@ -40,15 +40,18 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort { logger: this.logger }, ); - // Server returns 200 with id:null when it silently declines to persist (e.g. no collection). + // Observed: server replies 200 with a null id when it declines to persist (PRD-428). + // Surface it once here; later mark-as-* calls then no-op instead of 404-looping. if (!response.id) { - this.logger.warn('activity log not persisted by server — skipping status update', { + this.logger.warn('activity log not persisted by server — status updates will be skipped', { action: args.action, collectionId: args.collectionId, }); + + return { id: null }; } - return { id: response.id, index: response.attributes?.index }; + return { id: response.id, index: response.attributes.index }; } catch (cause) { this.logger.error('activity log create failed', { action: args.action, @@ -61,8 +64,10 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort } async markSucceeded(handle: ActivityLogHandle): Promise { - // No id → log not persisted (see createPending); skip to avoid a permanent-404 retry storm. - if (!handle.id) return undefined; + // Not persisted (see createPending) → no log to update; skip the 404-retrying status call. + if (!('index' in handle)) return undefined; + + const { id, index } = handle; return this.drainer.track(async () => { try { @@ -71,14 +76,14 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort () => this.service.updateActivityLogStatus({ forestServerToken: this.forestServerToken, - activityLog: { id: handle.id, attributes: { index: handle.index } }, + activityLog: { id, attributes: { index } }, status: 'completed', }), { logger: this.logger, extraRetryStatuses: [404] }, ); } catch (err) { this.logger.error('activity log mark-as-completed failed', { - handleId: handle.id, + handleId: id, error: extractErrorMessage(err), }); } @@ -86,8 +91,10 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort } async markFailed(handle: ActivityLogHandle, errorMessage: string): Promise { - // No id → log not persisted (see createPending); skip to avoid a permanent-404 retry storm. - if (!handle.id) return undefined; + // Not persisted (see markSucceeded) → skip. + if (!('index' in handle)) return undefined; + + const { id, index } = handle; return this.drainer.track(async () => { try { @@ -96,14 +103,14 @@ export default class ForestadminClientActivityLogPort implements ActivityLogPort () => this.service.updateActivityLogStatus({ forestServerToken: this.forestServerToken, - activityLog: { id: handle.id, attributes: { index: handle.index } }, + activityLog: { id, attributes: { index } }, status: 'failed', }), { logger: this.logger, extraRetryStatuses: [404] }, ); } catch (err) { this.logger.error('activity log mark-as-failed failed', { - handleId: handle.id, + handleId: id, stepErrorMessage: errorMessage, error: extractErrorMessage(err), }); diff --git a/packages/workflow-executor/src/ports/activity-log-port.ts b/packages/workflow-executor/src/ports/activity-log-port.ts index 15ce3f4801..c1843ff8ca 100644 --- a/packages/workflow-executor/src/ports/activity-log-port.ts +++ b/packages/workflow-executor/src/ports/activity-log-port.ts @@ -7,10 +7,9 @@ export interface CreateActivityLogArgs { label?: string; } -export interface ActivityLogHandle { - id: string; - index: string; -} +// `{ id: null }` means the server declined to persist the log (returns 200 with a null id). +// markSucceeded/markFailed short-circuit on it — there is nothing to update. +export type ActivityLogHandle = { id: string; index: string } | { id: null }; // Per-run scoped port: token baked into the adapter's constructor. markSucceeded/markFailed // retry transient failures internally and are invoked with `void` from base-step-executor. 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 32a650bdc9..680f5d7948 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 @@ -167,7 +167,7 @@ describe('ForestadminClientActivityLogPort', () => { ); }); - it('warns and returns a non-persisted handle when the server responds 200 with id:null', async () => { + it('warns (with collectionId) and returns a not-persisted handle when the server responds 200 with id:null', async () => { const service = makeService(); service.createActivityLog.mockResolvedValue({ id: null, @@ -176,27 +176,33 @@ describe('ForestadminClientActivityLogPort', () => { const logger = makeLogger(); const port = makePort(service, { logger }); - const handle = await port.createPending({ renderingId: 5, action: 'action', type: 'write' }); + const handle = await port.createPending({ + renderingId: 5, + action: 'action', + type: 'write', + collectionId: '11', + }); - expect(handle.id).toBeNull(); + expect(handle).toEqual({ id: null }); + expect(service.createActivityLog).toHaveBeenCalledTimes(1); expect(logger.warn).toHaveBeenCalledWith( - 'activity log not persisted by server — skipping status update', - expect.objectContaining({ action: 'action' }), + 'activity log not persisted by server — status updates will be skipped', + { action: 'action', collectionId: '11' }, ); }); }); describe('markSucceeded', () => { - it('skips the status update (no 404 storm) when the handle was not persisted (id absent)', async () => { + it('skips the status update and drainer tracking when the handle was not persisted', async () => { const service = makeService(); - const port = makePort(service); + const drainer = new ActivityLogDrainer(); + const trackSpy = jest.spyOn(drainer, 'track'); + const port = makePort(service, { drainer }); - await port.markSucceeded({ id: null, index: undefined } as unknown as { - id: string; - index: string; - }); + await port.markSucceeded({ id: null }); expect(service.updateActivityLogStatus).not.toHaveBeenCalled(); + expect(trackSpy).not.toHaveBeenCalled(); }); it('retries on 503 and eventually resolves without rethrowing', async () => { @@ -244,16 +250,16 @@ describe('ForestadminClientActivityLogPort', () => { }); describe('markFailed', () => { - it('skips the status update (no 404 storm) when the handle was not persisted (id absent)', async () => { + it('skips the status update and drainer tracking when the handle was not persisted', async () => { const service = makeService(); - const port = makePort(service); + const drainer = new ActivityLogDrainer(); + const trackSpy = jest.spyOn(drainer, 'track'); + const port = makePort(service, { drainer }); - await port.markFailed( - { id: null, index: undefined } as unknown as { id: string; index: string }, - 'boom', - ); + await port.markFailed({ id: null }, 'boom'); expect(service.updateActivityLogStatus).not.toHaveBeenCalled(); + expect(trackSpy).not.toHaveBeenCalled(); }); it('sends status: failed (no errorMessage — server schema rejects unknown fields) and retries on 503', async () => {