Skip to content

fix(workflow-executor): skip activity-log update when server returns no id (PRD-428)#1615

Open
christophebrun-forest wants to merge 2 commits into
feat/prd-214-server-step-mapperfrom
fix/prd-428-workflow-executor-skip-activity-log-completion-when-the
Open

fix(workflow-executor): skip activity-log update when server returns no id (PRD-428)#1615
christophebrun-forest wants to merge 2 commits into
feat/prd-214-server-step-mapperfrom
fix/prd-428-workflow-executor-skip-activity-log-completion-when-the

Conversation

@christophebrun-forest
Copy link
Copy Markdown
Member

@christophebrun-forest christophebrun-forest commented Jun 1, 2026

Problem

When an MCP step completes, the executor logs showed a burst of retry warnings ending in an error:

warn  "activity log mark-as-completed" failed, retrying attempt=1 status=404 error="Activity log not found"
warn  "activity log mark-as-completed" failed, retrying attempt=2 status=404 error="Activity log not found"
warn  "activity log mark-as-completed" failed, retrying attempt=3 status=404 error="Activity log not found"
error activity log mark-as-completed failed handleId=null error="Activity log not found"

Root cause (confirmed in forestadmin-server)

The Forest server's ActivityLogCreator.create() returns null when the request carries no collection (authorization check), and the create route then responds HTTP 200 with data: { id: null } by design. createPending returned that handle verbatim, so the subsequent mark-as-completed/failed PATCH /{index}/null/status hit a permanent 404 — retried 3× (404 is in extraRetryStatuses) before a swallowed error, polluting logs and burning background latency on every affected step.

Fix (client robustness)

  • createPending warns once when the server returns no id.
  • markSucceeded / markFailed early-return when the handle has no id → kills the 404 storm at the source.
  • 404 stays retriable for valid ids (transient read-path propagation) — unchanged.

This is defense-in-depth; the functional fix that makes the log actually persist (passing collectionId) already landed in #1608.

Tests

  • createPending returns a non-persisted handle and warns on 200 + id: null.
  • markSucceeded / markFailed do not call updateActivityLogStatus when the handle has no id.
  • Existing valid-id behavior (incl. 404 retry) unchanged. Full suite: 836/836 green, lint clean, build OK.

fixes PRD-428

🤖 Generated with Claude Code

Note

Skip activity-log status updates in ForestadminClientActivityLogPort when server returns no id

  • createPending now returns { id: null } and logs a warning when the server responds with a null id, indicating the log was not persisted.
  • markSucceeded and markFailed both short-circuit when the handle has no index property (i.e. { id: null }), skipping drainer.track and updateActivityLogStatus calls.
  • The ActivityLogHandle type in activity-log-port.ts is updated to a discriminated union covering both persisted and non-persisted cases.

Macroscope summarized 1ae6cc3.

…no id (PRD-428)

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) <noreply@anthropic.com>
@linear
Copy link
Copy Markdown

linear Bot commented Jun 1, 2026

PRD-428

@qltysh
Copy link
Copy Markdown

qltysh Bot commented Jun 1, 2026

Qlty


Coverage Impact

Unable to calculate total coverage change because base branch coverage was not found.

Modified Files with Diff Coverage (1)

RatingFile% DiffUncovered Line #s
New Coverage rating: A
...-executor/src/adapters/forestadmin-client-activity-log-port.ts100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

…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) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant