feat(control-plane,k8s): shutdown min delay, drain-aware readiness, chart defaults and a coherent tuning recipe - #1030
Merged
Conversation
…ware readiness On Kubernetes the control plane closes its listener the instant it is signalled, while kube-proxy is still routing traffic to the pod: in-flight and newly arriving requests get connection refusals for however long endpoint removal takes to propagate. There was also no way to tell a draining control plane from a healthy one -- /health and /api/v1/health answer 200 right up to the moment the listener goes away, so a readiness probe pointed at them can never fail early enough to help. Add two pieces that fix that together: - AGENTFIELD_SHUTDOWN_MIN_DELAY: a control-plane-only wait between the shutdown signal and the start of Stop(). It defaults to 0, which reproduces the previous timing exactly, and accepts bare seconds or a Go duration like AGENTFIELD_SHUTDOWN_TIMEOUT does. It gets its own non-negative parser rather than relaxing parseShutdownTimeout, because that parser rejecting 0 is what keeps AGENTFIELD_SHUTDOWN_TIMEOUT=0 from silently changing meaning. The wait is placed after cmd/af's stopSignals() and after cmd/agentfield-server's waitForShutdown helper has returned, so a second SIGTERM during the window still kills the process immediately. - GET /readyz and GET /api/v1/health/ready: readiness routes that run the same dependency checks as /health but answer 503 as soon as BeginDrain has run. Liveness is deliberately untouched, so the kubelet does not kill a pod that is draining on purpose, and the process keeps accepting and completing requests for the whole window. /readyz is added to the API-key skip list (the middleware only exempts the /api/v1/health prefix, /health and /metrics), and both paths are listed in the DID auth skip paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d the shipped probe path Each test maps to one observable behaviour rather than to the code shape: - the env table covers 5, 5s, 500ms, 0, unset, abc and -1s, with 0 accepted as a real value and the invalid cases leaving the configured value alone; - a separate case asserts AGENTFIELD_SHUTDOWN_TIMEOUT=0 still keeps its current value, which is the guard against someone "simplifying" the two parsers back into one; - the routing test walks /readyz, /api/v1/health/ready, /health and /api/v1/health before and after BeginDrain, with an API key configured so it also proves the skip-path entry, and then asks for /api/v1/version to show ordinary traffic is still served while draining; - both entry points assert beginDrain runs before stop, that stop is not reached before a 50ms delay elapses, and that a zero delay does not wait. The manifest test reads the chart values and the kustomize base and asserts the shipped readinessProbe path is still /api/v1/health. The chart defaults to image tag latest with IfNotPresent and one replica, so flipping that path to one an older cached image does not serve would leave the Service with zero endpoints -- that regression should fail a test, not a cluster. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… readiness path The chart and the kustomize base now set AGENTFIELD_SHUTDOWN_MIN_DELAY to 5s and raise the control-plane pod grace from 45 to 60 seconds, which is what the shutdown actually needs: 5s minimum delay + the 30s AGENTFIELD_SHUTDOWN_TIMEOUT drain + roughly 20s of tail (a fresh >=5s async-pool budget plus 5s each for package maintenance, the observability forwarder and the tracer). The agent templates are left alone. The readinessProbe path is deliberately NOT flipped to the new shutdown-aware route. controlPlane.image.tag defaults to latest with pullPolicy IfNotPresent and replicaCount 1, so a chart upgrade can land on a node holding an older cached image; pointing the probe at a path that image 404s would leave a single-replica Service with zero endpoints. The path moves behind controlPlane.readinessProbe.path, defaulting to today's /api/v1/health, with a comment saying when it is safe to switch. The min-delay env entry is skipped when controlPlane.env already defines AGENTFIELD_SHUTDOWN_MIN_DELAY, so an explicit operator value never renders a duplicate env name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng code The Kubernetes guide told operators to raise terminationGracePeriodSeconds when they raised AGENTFIELD_SHUTDOWN_TIMEOUT, and stopped there. That is the exact configuration that breaks: the deferred reap fires AGENTFIELD_AGENT_DRAIN_GRACE after the REPLACEMENT registers, regardless of how much drain budget the departing pod still has, and under a default rolling update the replacement is Ready and registered before the old pod is even signalled. A reader following the old text ends up with a long reasoner reaped mid-flight and a 409 on its own success callback. State the invariant instead, and the things a reader cannot guess: - the drain-grace inequality, with maxSurge: 0 as the way to zero the registration-to-SIGTERM lag term, and a worked 10-minute-reasoner example written with unit suffixes, because AGENTFIELD_AGENT_DRAIN_GRACE takes Go durations only -- a bare 660 is dropped silently and the 60s default survives; - that 0s does not disable the reap (zero keeps the default) and a negative duration makes it immediate, so it is not an opt-out; - that drain grace is one global setting that also feeds agentIsDraining, so a 12m value holds every dead node's dispatches and rows for 12m; - that AGENTFIELD_EXECUTION_STALE_TIMEOUT (30m) is a second ceiling no drain tuning can raise, and the exact status_reason to grep for; - the 409 / idempotent-200 / 500 callback outcomes after a reap; - why replicas must stay 1 today, in terms of instance_id and the single callback URL field, not as a roadmap promise. Also document the new AGENTFIELD_SHUTDOWN_MIN_DELAY and readiness routes, the real control-plane pod-grace arithmetic (min delay + shutdown timeout + ~20s of tail, not the optimistic +5s), and reconcile the agent pod-grace floor to one number (+15s) across both files. Drops the stale claim that Agent.setup_signal_handlers() is retained for compatibility -- that delegate was removed and this line was its last mention in the repo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review caught 11m15s being written as 690s. Derive the number from the stated invariant (budget + settlement + headroom) instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… keys The values.yaml parse test would keep passing if the deployment template stopped referencing controlPlane.readinessProbe.path, shutdownMinDelay or terminationGracePeriodSeconds. Table-driven template-reference assertions close that hole without depending on a helm binary. 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 was referenced Aug 31, 2026
[Control Plane] Kubernetes new deployments causes every in-flight run to fail and never recover
#987
Open
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
The Kubernetes "min delay" half of graceful shutdown, plus one coherent tuning recipe.
AGENTFIELD_SHUTDOWN_MIN_DELAY(default0= exactly today's timing; accepts5,5s,1m) keeps the control plane serving for that long after SIGTERM while a shutdown-aware readiness surface flips to 503 immediately and liveness stays green — the standard fix for endpoint-propagation lag, so kube-proxy stops routing before the listener closes. The Helm chart opts the control plane intoshutdownMinDelay: 5sand raises itsterminationGracePeriodSecondsto 60; agent templates are untouched. The shipped readiness probe path is unchanged by default — the new path is behind a values key defaulting to the current one, so a cached older image can never brick readiness.docs/deploying-on-kubernetes.mdis rewritten into a derivable recipe (drain budget, settlement, grace windows, preStop, worked examples), linked from the docs index, and every path/env/default in it resolves to code in this branch.Why
Refs #989 (the
minSigtermDelayask; the max side shipped in v0.1.137) and #987 (the drain-grace sizing invariant for long orchestrators, and the replicas=1 caveat, are now stated plainly).Deliberately not done, with reasons in the linked issues: no SDK-side min delay (equivalent to
preStop.sleep, and the control plane already holds dispatch once an agent announces shutdown) and no GET handler on/shutdown(a process-killing side effect behind an unauthenticated GET;preStop: {sleep: ...}covers the k8s-native ask).Changes
Validation contract
AGENTFIELD_SHUTDOWN_MIN_DELAYunset/0reproduces v0.1.137 shutdown timing exactly →TestDrainOnShutdownZeroDelayStopsImmediately,TestFinishShutdownZeroDelayStopsImmediately,TestShutdownMinDelayDefaultsToZeroTestReadinessTurnsUnavailableDuringDrainWhileLivenessStaysHealthy,TestDrainOnShutdownBeginsDrainBeforeMinimumDelayAndStop,TestFinishShutdownBeginsDrainBeforeMinimumDelayAndStopTestReadinessTurnsUnavailableDuringDrainWhileLivenessStaysHealthyTestShutdownMinDelayEnvParsing;AGENTFIELD_SHUTDOWN_TIMEOUTparsing unchanged →TestShutdownTimeoutZeroStillKeepsCurrentValue,TestShutdownTimeoutEnvOverrideIgnoresInvalidValue,TestShutdownTimeoutEnvAcceptsBareSecondsLikeTheSDKsTestReadinessTurnsUnavailableDuringDrainWhileLivenessStaysHealthyTestManifestReadinessDefaultsRemainBackwardCompatible(values defaults + template-reference assertions)helm lint+helm templatein the gate runHow it was tested
CI-literal gates in the worktree:
go build,gofmt -l,go vet, full control-plane suite (-tags sqlite_fts5, minusinternal/packages),./scripts/coverage-surface.sh control-plane,./scripts/patch-coverage-gate.sh(≥80 % on touched lines),helm lint+helm template, YAML parse of all manifests — ALL-PASS. Rebased onto currentmain; the full control-plane suite was re-run post-rebase.Notes / follow-ups
An adversarial review ran between the first four commits and the last two; both findings it raised (a wrong worked-example sum; helm values wiring untested) are fixed in the review-round commits. The k8s doc states the two operational caveats explicitly: the deferred reap fires
AGENTFIELD_AGENT_DRAIN_GRACEafter the replacement registers regardless of the departing pod's budget, and agent Deployments should runreplicas: 1today (a sibling replica is indistinguishable from a replacement; the escape hatch ships separately).🤖 Generated with Claude Code