fix(control-plane): gate every dispatch lane before persistence and complete the rejection contract - #1033
Merged
Merged
Conversation
…atch lane The per-agent concurrency limit and the LLM circuit breaker were checked before persistence only on the async lane. On the sync, restart and MCP lanes the check ran *after* prepare had already written an executions row, a workflow_executions row and an input payload blob, so a gate-rejected request was charged a failed execution for work that was never attempted. There is now a single admission point, ahead of persistence, on all four lanes: prepareExecutionForTargetWithAdmission takes acquireSlot=true from the sync handler, the restart handler and the MCP start_run path, and the duplicate post-prepare gate blocks are gone. findReplayHit moves ahead of the gate so a replay hit — which never dials the agent — is never rejected by it and consumes no slot; the async lane no longer acquires and releases a slot for one. preparedExecution.slotHeld records whether a plan actually owns a slot, so every release site releases exactly what it took. Two other holes in the rejection contract close with it: - handleAsync abandoned the already-persisted row in "running" when submitReserved found a stopped pool. It now terminates it through failForControlPlaneShutdown (failed + status_reason control_plane_shutdown on both tables), on a detached context because the request context is very likely being cancelled by the same drain. - The restart and MCP queue-full paths reserved pool capacity only after prepare, so a queue-full burst wrote rows and then failed them, and the restart lane persisted status_reason internal_error while answering concurrency_limit. reserve() is hoisted ahead of prepare on both, and a single typed executionPreconditionError now feeds both failExecution and the response. Retry-After is completed at the same time: writeExecutionError takes the value stamped on the error, else a per-category default, so llm_unavailable advertises the circuit breaker's remaining recovery window (default 30s, floor 1s) via the new LLMHealthMonitor.RetryAfterSeconds — circuitOpenedAt is unexported, so the window has to be computed inside services — while concurrency_limit and node_unavailable stay at 1 and non-retryable rejections (413, agent_pending_approval) still carry nothing. The stale reservation comment above pool.reserve() is corrected: the worker releases the reservation when its job returns, not on dequeue, so a reservation covers preparation, queue wait and the whole dispatch. Both gate halves are opt-in and off by default (AGENTFIELD_MAX_CONCURRENT_PER_AGENT=0, llm_health.enabled=false), so a stock deployment sees no behaviour change. Refs #986 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ontract One test per observable behaviour of the new admission point, written from the caller's side (HTTP status, headers, body, and what the store holds afterwards) rather than from the implementation: - sync concurrency and llm_unavailable rejections persist no executions row, no workflow_executions row and no payload blob; - a replay hit against an agent already at its cap still returns 200/202 with X-AgentField-Replay-Hit, never dials the agent, and consumes no slot; - the per-agent running count is 1 during a successful sync call and back to 0 after success, an agent 5xx and a pre-gate precondition rejection; - restart rejections (gate and queue-full) persist nothing and carry Retry-After plus retry_after; - an async request whose pool stops between reserve() and submitReserved ends as failed/control_plane_shutdown on both tables with the slot released exactly once; - writeExecutionError's Retry-After table, including that 413 and agent_pending_approval carry neither header nor field; - LLMHealthMonitor.RetryAfterSeconds counts the window down, floors at 1, and falls back to the configured recovery timeout (30s) when the circuit is closed, the endpoint is unknown, or the receiver is nil; - an MCP start_run rejected by the gate persists nothing. Two existing fixtures build a preparedExecution by hand after acquiring a slot themselves; they now set slotHeld so the job still releases what they took. TestPrepareExecution_AdditionalCoverage pins the process-global limiter to nil, because prepareExecution now acquires a slot and its four direct calls would otherwise leak counts into unrelated tests. Refs #986 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
README advertised "a durable PostgreSQL queue with lease-based processing, so a crash or a restart resumes where it left off". No such thing exists: the lease columns in migrations 011 and 013 are inert plumbing, and there is no acquisition, renewal or expiry-reclaim code anywhere in the tree. What the control plane actually does is admit work into a bounded in-process queue with backpressure (429/503 plus Retry-After) and, on graceful shutdown, terminate in-flight executions with status_reason control_plane_shutdown instead of silently dropping them. Both README claims now say that. Alongside it: - docs/api/EXECUTE.md said llm_unavailable carried no Retry-After. It now does, advertising the circuit breaker's remaining recovery window, and a new line under the table states that these pre-dispatch rejections persist no rows — with the one exception of a request rejected after preparation because the pool has already stopped. - docs/api/EXECUTION_RESTART.md records that the restart lane runs the same admission checks before persistence and returns Retry-After on queue-full. - AGENTFIELD_EXEC_ASYNC_QUEUE_CAPACITY was documented as the number of executions "waiting for a worker". The admission bound is really workers + queue_capacity and a reservation is held across preparation, queue wait and the worker's dispatch (up to 24h for a paused execution). Refs #986 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… answers with The restart handler can still lose the race between reserve() and submitReserved when the pool stops in between. That branch is the one that used to answer concurrency_limit while writing status_reason internal_error, because the queue error was an untyped errors.New. Drive it through the same CreateExecutionRecord seam the async pool-stopped test uses and assert the persisted status_reason equals the error_category in the body, that Retry-After and retry_after are both present, and that the per-agent slot is released. Refs #986 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hrough a detached context The restart and MCP submit-failure paths persisted the terminal state with the request context — during shutdown that context is likely already cancelled, stranding the freshly created rows in running (the same bug class #1001 fixed on the async lane). MCP also discarded the persistence error entirely. All three lanes now share one helper: detached bounded persistence context, failed/control_plane_shutdown on both tables, and a warn log carrying node_id and execution_id when persistence itself fails. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Finishes the execute-admission contract that #1001 started. The async lane already gated before persisting; the sync, restart and MCP lanes still persisted the payload blob and both execution rows before the per-agent concurrency / LLM-circuit gate, so a rejected burst left orphan
failedrows and blobs behind. Now every dispatch lane admits first and persists after; a request admitted right as the async pool stops is terminalized (failed/control_plane_shutdown) through a detached bounded persistence context instead of being abandoned inrunning; every time-retryable rejection (429concurrency_limit, 503 queue-full / pool-stopped /node_unavailable/llm_unavailable) carries both aRetry-Afterheader and aretry_afterbody field, withllm_unavailableadvertising the circuit-breaker's real recovery window; and the README/docs no longer claim a "durable PostgreSQL queue with lease-based processing" that does not exist — the copy now describes the actual bounded in-process admission model.Why
Refs #986. Rejections were supposed to be cheap; on three of the four lanes they wrote to disk first. The stale README claim is very likely what set the expectation that a burst would queue rather than 503.
Changes
Validation contract
Retry-After: 1+error_category: concurrency_limitand creates no rows and no payload blob →TestExecuteHandler_ConcurrencyRejectionHasNoPersistencellm_unavailableand persists nothing →TestExecuteHandler_LLMUnavailableRejectionHasNoPersistenceTestExecuteHandler_ReplayHitNotGatedBySaturatedAgentTestRestartHandler_ConcurrencyRejectionHasNoPersistence,TestMCP_ExecuteReasonerConcurrencyRejectionHasNoPersistenceRetry-After+retry_after→TestRestartHandler_QueueFullCarriesRetryAfterTestExecuteHandler_SlotBalancedAcrossOutcomes+ the rejection tests abovefailed/control_plane_shutdownon both tables, even when the request context is already cancelled, on the async, restart and MCP lanes →TestExecuteAsyncHandler_PoolStoppedTerminatesPersistedRow,TestRestartHandler_PoolStoppedPersistsMatchingStatusReason,TestMCP_ExecuteReasonerPoolStoppedTerminatesPersistedRowsWithCancelledRequest,TestMCP_ExecuteReasonerPoolStoppedExercisesPersistenceFailureRetry-Afterper category, with 413 andagent_pending_approvalcarrying none →TestWriteExecutionError_RetryAfterPerCategoryllm_unavailableadvertises the circuit recovery window (default 30s, floor 1s), computed in services →TestLLMHealthMonitor_RetryAfterSecondsstatus_reasonit answers with →TestRestartHandler_PoolStoppedPersistsMatchingStatusReasongrep -rn "lease-based" README.md docs/returns nothingHow 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) — ALL-PASS. Rebased onto currentmainbefore push. An adversarial review ran between the first four commits and the fix commit; its one blocking finding (restart/MCP pool-stopped persistence using the request context — the same bug class #1001 fixed on the async lane) is what the fix commit addresses.Notes / follow-ups
The gate halves are opt-in and off by default (
AGENTFIELD_MAX_CONCURRENT_PER_AGENT=0= unlimited;llm_health.enabled=false), so a stock deployment sees no behavioural change. The bigger asks on #986 — admit-and-queue at capacity, aqueuedpersisted status, holding the per-agent slot until the terminal callback — are design decisions tracked on the issue, not smuggled in here.🤖 Generated with Claude Code