fix(workflow-executor): skip activity-log update when server returns no id (PRD-428)#1615
Conversation
…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>
|
Coverage Impact Unable to calculate total coverage change because base branch coverage was not found. Modified Files with Diff Coverage (1)
🛟 Help
|
…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>
|
|
||
| async markSucceeded(handle: ActivityLogHandle): Promise<void> { | ||
| // Not persisted (see createPending) → no log to update; skip the 404-retrying status call. | ||
| if (!('index' in handle)) return undefined; |
There was a problem hiding this comment.
[claude-opus-4-8] Preferential — Narrowing via !('index' in handle) is correct and compiles, but the union's natural discriminant is id ({ id: null } vs { id: string; index: string }). Keying on handle.id === null would align the check with the documented discriminant and the "not persisted" comments, making intent more obvious. Same pattern at line 95 (markFailed).
2 participants
Problem
When an MCP step completes, the executor logs showed a burst of retry warnings ending in an error:
Root cause (confirmed in
forestadmin-server)The Forest server's
ActivityLogCreator.create()returnsnullwhen the request carries no collection (authorization check), and the create route then responds HTTP 200 withdata: { id: null }by design.createPendingreturned that handle verbatim, so the subsequent mark-as-completed/failedPATCH /{index}/null/statushit a permanent 404 — retried 3× (404 is inextraRetryStatuses) before a swallowed error, polluting logs and burning background latency on every affected step.Fix (client robustness)
createPendingwarns once when the server returns no id.markSucceeded/markFailedearly-return when the handle has no id → kills the 404 storm at the source.This is defense-in-depth; the functional fix that makes the log actually persist (passing
collectionId) already landed in #1608.Tests
createPendingreturns a non-persisted handle and warns on 200 +id: null.markSucceeded/markFaileddo not callupdateActivityLogStatuswhen the handle has no id.fixes PRD-428
🤖 Generated with Claude Code
Note
Skip activity-log status updates in
ForestadminClientActivityLogPortwhen server returns no idcreatePendingnow returns{ id: null }and logs a warning when the server responds with a null id, indicating the log was not persisted.markSucceededandmarkFailedboth short-circuit when the handle has noindexproperty (i.e.{ id: null }), skippingdrainer.trackandupdateActivityLogStatuscalls.ActivityLogHandletype in activity-log-port.ts is updated to a discriminated union covering both persisted and non-persisted cases.Macroscope summarized 1ae6cc3.