fix(control-plane): stop counting agent restarts as failed executions - #941
Conversation
A long-running agent node restarts constantly during development — the user saves a file, hits Ctrl-C, or the process dies on a syntax error. The control plane kept believing the node was healthy for as long as it took the health checker to notice (~30s) or the heartbeat to go stale (60s). Every call that landed inside that window created an execution record, failed to dial the dead process, and was recorded as a failed execution. The user was charged a failure for a five-second restart, and it is the largest single source of failed executions a new user produces. Three changes, in the order a call meets them: 1. Fail fast on a node we already know is down. prepareExecutionForTarget checked pending-approval but never health, so a dispatch into a dead node persisted a row and then failed it. It now returns 503 node_unavailable BEFORE the execution record exists — a request we never dispatched is a rejected request, not a failed execution. This mirrors the check reasoners.go has always had on the legacy proxy route. Only definitively down states are rejected: "unknown" (no heartbeat yet) still goes through, or the first call of every session would fail. 2. Absorb the restart when we do NOT know the node is down. A dial failure is the one transport error that is unambiguously safe to retry — no bytes reached the agent, so nothing can run twice — so the dispatch now waits for the node to come back and replays against its current address, which may be a different port. Recovery is read from the node record (a fresh instance_id, or a heartbeat that advanced), not guessed by blind retrying. Bounded by agent_restart_grace, default 15s, well inside the 90s agent call timeout; serverless targets are excluded because they have no resident process to come back. 3. Close the detection gap. When the wait expires we demote the node to inactive, conditional on the heartbeat we last observed, so the next caller takes path 1 and fails fast instead of repeating the wait. Also exempts held executions from the orphan reaper. The re-registration that ENDS a restart is exactly what triggers MarkAgentExecutionsOrphaned, so the reaper was failing the very execution the retry was about to complete, and the sync caller received that failure even though the work went on to succeed. Executions marked awaiting_agent_restart are now skipped, on both the executions table and the workflow_executions row the DAG UI reads. Finally, "agent 'x' not found" now classifies as target_not_found rather than internal_error. That is the quickstart curl run before `python main.py`: a normal mistake that was being reported as a broken control plane. The category already existed in canonicalFailureCategory and nothing ever assigned it. Verified against a real Python SDK agent: a call fired while the agent was down, with the process returning 4s later, now returns HTTP 200 succeeded and emits execution_completed. Before this change the same sequence returned 502 with an agent_restart_orphaned message. The documented quickstart runs clean end to end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dings Four behavioral fixes to the agent-restart grace mechanism, each found by adversarial review of the original commit: 1. Serverless nodes are exempt from the fail-fast health gate. They have no heartbeat loop and the health monitor never polls them, so the presence sweep marks every serverless node inactive shortly after registration — the gate would have rejected every serverless invocation with 503 node_unavailable, permanently. The gate now runs after serverless normalization and skips serverless targets entirely. 2. Replay requests bypass the gate. A replay hit is served from the recorded run without contacting the agent, so the node being down must not reject it. A replay miss dials and fails exactly as before the gate existed. 3. A cancel that lands during the restart wait now wins: the loop re-reads the execution record before replaying (mirroring callAgent's pre-dispatch check) and aborts without handing the agent work the caller disowned. A pause similarly waits for its resume. 4. The wait aborts as soon as the node record reports the node definitively down (demoted by the health checker or by another dispatch whose grace expired first). Queued dispatches aimed at one dead node no longer each burn their full grace serially on the async worker pool. Also: the awaiting_agent_restart hold is now released on every exit path (including caller-context cancellation, via a detached write), the comment overstating what completeExecution can repair after an orphan-reaper race is corrected (workflow_executions has no exit from failed), and the test fake's UpdateAgentHealthAtomic now enforces the same conditional-heartbeat semantics as LocalStorage instead of discarding the argument. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ece441d to
8409f9e
Compare
|
Rebased onto latest main — the branch was carrying #937's already-merged desktop commits, which is what the conflict was. The diff is now just the 10 control-plane files. Verified the fix locally against a real Python SDK agent (isolated control plane, fresh home): a call fired while the agent was down with the process returning 4s later gives HTTP 200 succeeded with no failed row (baseline main: instant 504 + failed execution); a known-down node gets an instant 503 with no execution row; grace expiry demotes the node; the quickstart curl before Also pushed a hardening commit for issues that came out of adversarial review of the retry path:
Each fix has a test. Full handlers/storage/config/types suites green locally. |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
AbirAbbas
left a comment
There was a problem hiding this comment.
Rebased, adversarially reviewed, hardened (serverless gate exemption, cancel-during-wait, early demotion abort, replay bypass), and verified live against a real Python SDK agent — restart absorb returns 200 succeeded where main returns 504 + a failed row. Full verification details in the comment above.
Why
New users produce failed executions they did not cause. The largest single source is the edit-restart loop.
A long-running agent node restarts constantly during development — save a file, Ctrl-C, or a syntax error kills the process. The control plane keeps believing the node is healthy for as long as it takes the health checker to notice (
check_interval×consecutive_failures, ~30s) or the heartbeat to go stale (60s). Every call inside that window created an execution record, failed to dial the dead process, and was recorded as failed.Reproduced against a real Python SDK agent on a clean install, telemetry captured locally:
HTTP 502,agent_restart_orphaned,execution_failedHTTP 200 succeeded,execution_completed503 node_unavailable, no execution rowpython main.pyerror_category: internal_errorerror_category: target_not_foundWhat changed
Three fixes, in the order a call meets them.
1. Fail fast on a node we already know is down.
prepareExecutionForTargetchecked pending-approval but never health — the versioned routing path filtered healthy nodes viaselectVersionedAgent, the unversioned path (what every new user has) did not. It now returns503 node_unavailablebefore the execution record exists. A request we never dispatched is a rejected request, not a failed execution — which is already how an unknown node behaves, so this only makes the two consistent. Mirrors the checkreasoners.gohas always had on the legacy proxy route.Only definitively-down states are rejected.
unknown(no heartbeat yet) anddegradedstill go through, or the first call of every session would fail.2. Absorb the restart when we do not know the node is down. A dial failure is the one transport error that is unambiguously safe to retry — no bytes reached the agent, so nothing can execute twice.
net.OpError.Op == "dial"is the portable signal; a read/write failure mid-request is deliberately not retried.The dispatch waits for the node to come back and replays against its current address, which may be a different port. Recovery is read from the node record — a fresh
instance_id, or a heartbeat that advanced — rather than guessed by blind retrying. Bounded byagent_restart_grace(default 15s, well inside the 90s agent call timeout,AGENTFIELD_AGENT_RESTART_GRACE/node_health.agent_restart_graceto tune, negative to disable). Serverless targets are excluded: they have no resident process to come back, so a dial failure there is a real outage and must surface immediately.3. Close the detection gap. When the wait expires the node is demoted to
inactiveviaUpdateAgentHealthAtomic, conditional on the heartbeat we last observed — so if it heartbeated while we waited, the health checker keeps ownership. The next caller then takes path 1 and fails fast instead of repeating the wait.Two things the end-to-end run turned up
The orphan reaper was failing the execution the retry was saving. Re-registration is exactly what ends a restart and what triggers
MarkAgentExecutionsOrphaned. The first end-to-end run returnedHTTP 502with anagent_restart_orphanedmessage even though the retry had succeeded and telemetry recordedexecution_completed— the sync caller was reading the reaped row. Executions markedawaiting_agent_restartare now exempt, on both theexecutionstable and theworkflow_executionsrow the DAG UI and dashboard counts read. Those are not orphans: the control plane still owns them and is about to re-dispatch.agent 'x' not foundreported asinternal_error. That is the quickstart curl run beforepython main.py— a normal mistake presented as a broken control plane. It now classifies astarget_not_found, a category that already existed incanonicalFailureCategoryand that nothing ever assigned.Scope
Not addressed here, and worth separate PRs:
MarkAgentExecutionsOrphaned,MarkStaleExecutions,MarkStaleWorkflowExecutions) write terminal failures with no event published, soexecution_started≠completed+failedin telemetry.validationandpermission_deniedare still dead categories; an input-schema rejection reports asagent_error.usage_context, so the local dev loop cannot be separated from real traffic in product metrics.Verification
internal/handlers, 1 ininternal/storage, 1 ininternal/config. New code is at 89–100% per function.go build ./...,go vet,gofmtclean.golangci-lintreports nothing in the changed files.go test ./...shows the same pre-existing failures as the base commit (internal/cli,internal/packages,internal/skillkit— they need afurrowbinary and interactive secret prompts). Confirmed identical by re-running against a stash of these changes.af init→af server→python main.py→ the printed curl) runs clean end to end on the final build.🤖 Generated with Claude Code