Skip to content

fix(control-plane): instance-scoped, deferred orphan reap on agent re-registration; hold dispatch while a node drains - #1004

Merged
AbirAbbas merged 11 commits into
mainfrom
fix/cp-orphan-reap
Aug 28, 2026
Merged

fix(control-plane): instance-scoped, deferred orphan reap on agent re-registration; hold dispatch while a node drains#1004
AbirAbbas merged 11 commits into
mainfrom
fix/cp-orphan-reap

Conversation

@AbirAbbas

@AbirAbbas AbirAbbas commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Control-plane half of #987 (Kubernetes rollout kills every in-flight run with in-flight reasoner cannot be revived) and the hold-vs-503 half of #989. Unchanged from 0.1.127 to main before this PR — nodes_register.go had no commits in that range; #941's restart-absorb covers only a failed dial, deliberately not already-dispatched work.

What was wrong. When a node re-registered with a new instance_id, RegisterNodeHandler synchronously failed every non-terminal execution whose agent_node_id matched (MarkAgentExecutionsOrphaned) — no grace, and no way to tell which pod owned a row because instance_id existed only on agent_nodes. Under a default rolling update (maxSurge 1) the new pod registers before the old one is signalled, so the CP declared work dead that was still running and about to succeed; with replicas > 1 behind one node id, one replica's restart killed every other replica's work.

What changes.

  • Migration 035_execution_instance_id: nullable instance_id on executions and workflow_executions (indexed with agent_node_id), stamped at dispatch from the serving agent record; SQLite auto-migrate gets the same columns. Legacy rows stay empty.
  • The reap is scoped to instance_id = <departing> OR instance_id IS empty (legacy) — work served by another live instance of the same node id survives.
  • The reap is deferred off the registration request by AGENTFIELD_AGENT_DRAIN_GRACE (default 60 s; negative disables deferral). A completion arriving inside the window makes the row terminal, so the reap skips it. The in-memory timer can be lost on a control-plane restart; the stale-execution sweep remains the backstop (documented).
  • The status_reason keeps the agent_restart_orphaned token operators grep for, but no longer claims "cannot be revived".
  • Hold while draining: a dispatch to a node that has marked itself offline (POST /nodes/{id}/shutdown, or an offline heartbeat) is held for up to the restart-grace window and dispatched to the replacement when it registers — reusing fix(control-plane): stop counting agent restarts as failed executions #941's agentMayRestart/agentCameBack machinery — instead of an immediate 503. No execution row exists while holding; if nothing registers in time, today's 503 node_unavailable is returned. The hold applies only to a node that went quiet recently — lifecycle offline with a last heartbeat inside AGENTFIELD_AGENT_DRAIN_GRACE (the same window that defers the reap). Health is deliberately not consulted: every node-announced offline transition (/shutdown, lifecycle/status, status PATCH) and the health monitor's own demotion all record health inactive, so it cannot tell a draining pod from a dead one — recency can. A node silent for longer than the drain grace, or explicitly deregistered, fails fast with 503 node_unavailable exactly as before and is never held.
  • Both grace windows are logged at startup and documented, with the Kubernetes guidance (terminationGracePeriodSeconds above the SDK drain; keep the agent version stable across rollouts — a version bump creates a separate registration and is recovered only by the stale sweep).

The (id, version) registration lookup is intentionally untouched: a version-bumping rollout is currently the one style that survives, and reaping on version bumps would extend the bug to it.

Validation contract → tests

  1. Both records persist the serving instance; legacy rows stay empty — TestExecutionRecordsPersistServingInstance
  2. Only the departing instance and legacy rows are reaped; another live replica's execution survives — TestMarkAgentInstanceExecutionsOrphaned_ReapsDepartingAndLegacyOnly (instance-a survives instance-b's restart)
  3. Registration returns before the deferred reap fires — TestRegisterNodeHandler_ReapsOrphansOnInstanceChange (asserts empty, then Eventually), TestRegisterNodeHandler_InstanceChangeWithNothingToReap
  4. Legacy empty-instance rows are still reaped after the window — same storage test (exec-legacy)
  5. Reason token stable, message factual — TestRegisterNodeHandler_ReapsOrphansOnInstanceChange
  6. Hold while draining: dispatched to the replacement; timeout → 503 with no row — TestWaitForDrainingAgentDispatchesToReplacement, TestWaitForDrainingAgentTimesOutWithoutExecution; gating predicate covered by existing TestAgentMayRestart / TestC17InactiveNodeWithoutUpdateIsRejectedAsUnavailable; a pod that just announced its shutdown (offline, health inactive, fresh heartbeat) is held for the restart grace and creates no row — TestExecuteHoldsRecentlyOfflineNodeRegardlessOfHealth; a node silent for longer than the drain grace is rejected in < 250 ms with a 2 s grace configured and no execution row — TestExecuteRejectsKnownDownNodeWithoutRecordingAnExecution; predicate table — TestAgentIsDrainingUsesRecencyNotHealth
  7. Env/YAML config + startup log — TestAgentDrainGraceFromEnvironment
  8. Insert column contract — TestWorkflowExecutionInsertQueriesCoverLifecycleColumns

Test plan

  • cd control-plane && go build ./... && go test ./... -count=1 (full suite green on the branch)
  • gofmt -l on touched files: clean
  • Note: TestDispatchAgentRequestStopsWaitingOnceTheNodeIsDemoted (internal/handlers) fails on a main-equivalent tree on this loaded machine as well — pre-existing timing flake, not from this diff.

Expected to conflict trivially with #1001 in execute_prepare.go (adjacent hunks); whichever lands second gets rebased.

Refs #987 #989 #941

🤖 Generated with Claude Code

Live smoke (real agentfield-server binary from this head, fake long-running node, AGENTFIELD_AGENT_RESTART_GRACE=6s, AGENTFIELD_AGENT_DRAIN_GRACE=3s)

Scenario Observed
POST /nodes/{id}/shutdown → sync execute immediately → replacement instance registers 2 s later held 2.25 s, then 200; the execution row is stamped with the replacement's instance_id
/shutdown → execute, no replacement 503 node_unavailable after 6.00 s (the restart grace), no execution row
/shutdown → wait 4 s (> drain grace) → execute 503 in 1 ms, no execution row
offline + health inactive, last heartbeat far in the past (agent smoke, earlier head) 503 in < 4 ms, no row

Known limitation (pre-existing, surfaced by manual testing of this branch): if the departed instance eventually answers after the drain grace, UpdateExecutionRecord (no state-transition guard) flips the executions row back to succeeded while workflow_executions correctly refuses failed → succeeded; the two stores then disagree and the execution API serves succeeded with the stale agent_restart_orphaned reason attached. Same behaviour on main with the synchronous reap. Follow-up: either reject late writes to a reaped row or clear the reason and reconcile the workflow row.

@AbirAbbas
AbirAbbas requested a review from a team as a code owner August 27, 2026 23:00
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

📊 Coverage gate

Thresholds from .coverage-gate.toml: per-surface ≥ 84%, aggregate ≥ 85%, max per-surface regression ≤ 1.0 pp, max aggregate regression ≤ 0.50 pp.

Surface Current Baseline Δ
control-plane 87.50% 87.40% ↑ +0.10 pp 🟡
sdk-go 93.00% 92.00% ↑ +1.00 pp 🟢
sdk-python 94.38% 93.73% ↑ +0.65 pp 🟢
sdk-typescript 91.66% 90.42% ↑ +1.24 pp 🟢
web-ui 84.76% 84.79% ↓ -0.03 pp 🟡
aggregate 85.77% 85.75% ↑ +0.02 pp 🟡

✅ Gate passed

No surface regressed past the allowed threshold and the aggregate stayed above the floor.

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

📐 Patch coverage gate

Threshold: 80% on lines this PR touches vs origin/main (from .coverage-gate.toml:thresholds.min_patch).

Surface Touched lines Patch coverage Status
control-plane 133 82.00%
sdk-go 0 ➖ no changes
sdk-python 0 ➖ no changes
sdk-typescript 0 ➖ no changes
web-ui 0 ➖ no changes

✅ Patch gate passed

Every surface whose lines were touched by this PR has patch coverage at or above the threshold.

AbirAbbas and others added 11 commits August 28, 2026 11:02
Every node-announced offline transition (POST /nodes/{id}/shutdown, the
lifecycle/status route, a status PATCH) records health as inactive, and so
does the health monitor's own demotion, so gating the hold on health kept
the one case it exists for — a pod that just announced its shutdown — on
the fail-fast path and held only monitor-demoted nodes.

Gate on recency instead: a node whose last heartbeat is within
AGENTFIELD_AGENT_DRAIN_GRACE is treated as draining and held for the
restart grace; one silent for longer is dead and fails fast with 503 and no
execution row. The same window already defers the orphan reap, so both
sides of the drain agree on what "recently" means.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keeps this PR's example variables next to the section they belong to
instead of appending at end-of-file, so sibling PRs that also extend
.env.example merge in any order.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The workflow-execution SELECT now reads COALESCE(instance_id, ''), so the
scripted driver row and the shared lifecycle column list must carry the
column too; CI's coverage run caught the 43-vs-44 Scan mismatch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AbirAbbas
AbirAbbas merged commit c477f6d into main Aug 28, 2026
26 checks passed
@AbirAbbas
AbirAbbas deleted the fix/cp-orphan-reap branch August 28, 2026 15:30
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