feat(control-plane): instance identity on execution reads and an orphan-reap kill switch for multi-replica agents - #1031
Merged
Conversation
When the re-registration reap fails an in-flight execution, the row's status_reason names the departing instance but nothing in any read API exposed which instance the execution was actually created against. An operator had no way to tell a genuinely orphaned execution from a legacy row that the reap swept because its instance_id was empty. Add agent_node_id and instance_id to ExecutionStatusResponse, populated in renderStatus -- the single builder GET /executions/:id, batch-status and the status callback all funnel through. Both are omitempty: instance_id vanishes for nodes that never report one (only the Python SDK does today), and agent_node_id stays absent on the synthetic not_found/error entries that handleBatchStatus builds inline, so those keep their current shape. The UI details DTO gains instance_id alongside the agent_node_id it already carried, so the DAG step drawer can surface it. No storage or schema change: migrations 033/035 added the columns and every execution SELECT already reads COALESCE(instance_id, ''). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… agents The deferred reap assumes a re-registration with a new instance_id means the previous OS process is gone. With replicas > 1 behind one node id that assumption is wrong: a sibling replica registering is indistinguishable from a replacement, so the reap fails the still-alive sibling's in-flight executions once the drain grace elapses (#987). Add AGENTFIELD_AGENT_ORPHAN_REAP_ENABLED (default true = today's behaviour). When false, the new conjunct on shouldReapOrphans means the deferred goroutine is never armed at all, and the stale-execution sweep stays as the backstop. Startup logs one greppable warning when disabled. Defaulting a bool to true needs presence tracking, since a zero value and an explicit `false` are otherwise identical. This follows the existing ExecutionCleanup.Enabled precedent and covers both loaders: yaml.v3 via an UnmarshalYAML hook on NodeHealthConfig, and viper -- which decodes through mapstructure and never calls that hook -- via IsSet in MarkExecutionCleanupEnabledIfSet. ApplyDefaults runs before ApplyEnvOverrides in all three load paths, so applyBoolEnv gets the last word and its existing warn-and-keep behaviour makes a garbage value fall back to true for free. The grace wiring moves into configureAgentRestartSettings so the startup behaviour is testable without booting a server. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
EXECUTE.md enumerated the polling response's exact field list, so it went stale the moment the two new fields landed. Extend it, and state the semantics that are easy to get wrong: instance_id names the instance the execution was *created against* and is not re-stamped when a dispatch is replayed across an agent restart, so a restart-absorbed execution names the departed process even though the replacement ran the work. Re-stamping stays out of scope because that column is the reap scope key. EXECUTION_RESTART.md notes that the reap also sweeps rows whose instance_id is empty, which is exactly why the new explicit field is what lets an operator tell that legacy case apart from a real orphan. The k8s guide gains the replicas > 1 rationale for the new env var, and a warning not to treat instance_id as guaranteed pod attribution: only the Python SDK reports one, it is a bare uuid4().hex the SDK never logs, and the sole path from that value to a pod is the control plane's own re-registration reap log (old_instance_id / new_instance_id). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
Contributor
📐 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two small, additive pieces for operating agents on Kubernetes. Execution read surfaces now expose
agent_node_idandinstance_id(the instance the execution was created against), so a reaped or orphaned execution can be attributed to the pod that owned it. AndAGENTFIELD_AGENT_ORPHAN_REAP_ENABLED=falsegives deployments that run more than one replica of a node id an escape hatch: with several replicas, a sibling registering is indistinguishable from a replacement today, and the deferred reap would fail the still-alive sibling's in-flight work — exactly the #987 symptom. The stale-execution sweep stays on as the backstop. Default istrue; behaviour is byte-identical to v0.1.137 unless the flag is set tofalse.Why
Refs #987.
Changes
Validation contract
TestGetExecutionStatusHandler_ReturnsAgentAndInstanceIdentifiersTestGetExecutionStatusHandler_OmitsEmptyInstanceIDTestBatchExecutionStatusHandler_IdentifierFieldsOnlyForFoundExecutionsTestUpdateExecutionStatusHandler_SuccessTestGetExecutionDetailsGlobalHandlerReturnsInstanceIDfull control-plane suite passes with every pre-existing assertion unmodified; both additions are omitempty and the three inline batch-status literals were left untoucheddocs/api/EXECUTE.mddocs/api/EXECUTE.mddocs/deploying-on-kubernetes.md (tightened to name Python as the only reporting SDK, verified against sdk/go and sdk/typescript)docs/api/EXECUTION_RESTART.mdTestRegisterNodeHandler_ReapsOrphansOnInstanceChange,TestRegisterNodeHandler_NoReapOnSameInstance,TestRegisterNodeHandler_PersistsInstanceIDTestReRegistrationSkipsOrphanReapWhenDisabledTestOrphanReapEnabledParsing,TestOrphanReapEnabledYAMLFalseIsPreserved,TestOrphanReapEnabledViperFalseIsPreservedHow it was tested
CI-literal gates in the worktree:
go build,gofmt -lon touched files,go vet, full control-plane suite (-tags sqlite_fts5, minusinternal/packages),./scripts/coverage-surface.sh control-plane,./scripts/patch-coverage-gate.sh— ALL-PASS, patch coverage 100% on 44 touched lines. Re-based onto currentmainand the control-plane gate re-run before push.Both loaders needed presence tracking so an explicit
falsesurvives (yaml.v3UnmarshalYAMLhook + viperIsSet— viper decodes via mapstructure and never calls the yaml hook); all three config load paths callApplyDefaultsbeforeApplyEnvOverrides, so the env override always wins. Both paths are tested.Notes / follow-ups
Non-blocking review findings kept as follow-ups:
control-plane/internal/config/config.go— The newAgentOrphanReapEnabled boolbreaks the struct-level contract documented four lines above it at config.go:202 — "NodeHealthConfig ... Zero values are treated as 'use default' — set explicitly to override." Every other field in this struct hocontrol-plane/internal/config/config.go—MarkExecutionCleanupEnabledIfSetnow also records presence foragentfield.node_health.agent_orphan_reap_enabled. The function name says it handles execution cleanup only, so a future loader author who reads the name and skips the call will silentcontrol-plane/internal/handlers/execute_status_update_test.go—TestBatchExecutionStatusHandler_IdentifierFieldsOnlyForFoundExecutionswraps the store inbatchStatusFallbackErrorStore, whoseGetExecutionRecordsBatchalways errors. That forces the handler down the per-ID fallback branch, so the test only exeOnly the Python SDK reports an
instance_idtoday; Go and TypeScript nodes leave it empty — the docs say so explicitly. No web-client change on purpose (the TS interfaces tolerate the additive fields); a UI surfacing pass can follow.🤖 Generated with Claude Code