fix(foreman): free the agent's in-process slot while a Job-mode task runs

[joryirving]

What

Stop a Job-mode AgenticTask from holding the agent's single in-process execution
slot for the lifetime of its Job.

Why

Fixes #1559

#1558 stopped the controller reserving a FleetNode for Job-mode tasks. That was
necessary but not sufficient: the controller can now place a second task on the
node, but the agent will not pick it up, because AgenticTaskWatcher holds one
process-wide inflight slot for every task it claims regardless of execution
mode.

Observed on an 8-node fleet: one node ran a Job-mode coder task with two further
tasks stuck in Scheduled behind it while the other seven FleetNodes sat Ready
with nothing assigned. The supervising agent process was idle throughout — the
coder loop, workspace and toolchain all run inside the Job pod.

How

In-process runs keep the strict one-at-a-time slot: they own the agent process's
CPU, memory and workspace directory, which is what the slot was for.

Job-mode runs get a separate budget, MaxSupervisedTasks, default 4, settable
with --max-supervised-tasks. Bounded rather than unbounded because each
supervision holds a goroutine and a liveness probe, and more importantly each is
an outstanding coder Job competing for pods, cache volumes and inference
capacity. Nothing else caps that per node.

The bound is per node and composes with Agent.spec.maxConcurrentTasks (#1497),
which is per Agent and enforced by the controller before a task is ever
Scheduled. Different axes; whichever is tighter wins. A task the controller
declines never reaches the watcher.

The watcher learns a task's mode by asking the Executor, through a new optional
SupervisingExecutor interface implemented over the same useCoderJobPath
predicate Execute already dispatches on, so slot accounting cannot drift from
the path actually taken. This is deliberately not a watcher-side
spec.execution.mode check: a Job-mode Agent whose executor has no wired
CoderJobSubmitter runs in-process, and a mode check would hand out a slot it
isn't holding. Executors that don't implement the interface (StubExecutor)
are treated as in-process, so stub mode stays strictly serialised.

The reservation happens before the goroutine starts and the release is a defer
registered first inside it, so the slot is returned on every exit path —
executor error, context cancellation, or panic.

Out of scope

reserveFirstFitNode returns the alphabetically first eligible node for
Job-mode tasks and, correctly per #1496, does not reserve it — so Job-mode tasks
concentrate on a single node (measured: 10 of 11 in one day, with seven nodes
idle). This change raises that node's concurrency from 1 to
MaxSupervisedTasks, but does not spread work across the fleet, which means the
per-node bound currently behaves like a fleet-wide cap. Filed separately as
#1634. Happy to fold the two together if you'd rather they land as one change.

Also worth knowing: FleetNode.status.currentTask still reports only the
in-process task, so a node supervising Jobs looks idle in kubectl get fleetnode. That follows from #1558's design rather than this change, and is now
documented.

Checklist

• Tests added/updated
• make test passes locally
• make lint passes locally
• Commit messages follow conventional commits
• All commits are signed off (git commit -s) per DCO
• AI assistance (if any) is disclosed above, per CONTRIBUTING.md
• Documentation updated (if user-facing change)

make lint-all also passes (GOOS=darwin and GOOS=linux, 0 issues).

Tests: pkg/foreman/agent covers in-process serialisation, Job-mode
concurrency, the supervision bound holding a third task at Scheduled, slot
release on the executor-error path for both modes, and a 5-case table on
SupervisesExternally (submitter wired, no submitter, in-process Agent, missing
Agent, no agentRef). Verified the new tests fail with the fix reverted:
stubbing the mode check to always return in-process gives
want 2 got 1 on both concurrency tests.

Not manually exercised in a cluster yet — the fleet behaviour above is the
observation that motivated the fix, not a verification of it.

Assisted-by: Claude Opus (wrote the implementation, tests and docs; I reviewed
the diff, confirmed the tests fail without the fix, and ran make test / lint /
lint-all)

[codecov]

Codecov Report

❌ Patch coverage is 82.97872% with 16 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/foreman-agent/main.go	0.00%	13 Missing ⚠️
pkg/foreman/agent/runtask.go	40.00%	2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[Defilan]

Thanks Jory, this is a good catch and a well-shaped fix. #1559 has been quietly costing us a slot per Job-mode run and I appreciate you going after the accounting rather than papering over it with a bigger pool. The branch compiles clean, go vet is quiet, and the new tests pass under -race -count=3 on my machine, so the mechanics are solid.

I went through it carefully and have a handful of things, one of which I think has to change before merge. Everything else is negotiable or a follow-up, and I have marked which is which.

Blocking: the error path reintroduces the bug it fixes

pkg/foreman/agent/executor_coderjob.go:154

SupervisesExternally swallows every Get error and answers "in-process". The agent's client is uncached (cmd/foreman-agent/main.go:332), so that is a live call to the apiserver on every check. On a transient failure, a timeout, a 429, a reset connection, supervises returns false and we claim the Job-mode task into the single in-process slot. A moment later Execute does its own separate Get, that one succeeds, useCoderJobPath returns true, and executeCoderJob runs. The node now holds inflight for the whole coder-Job lifetime while doing nothing, which is exactly #1559, and because the error was discarded there is nothing in the logs to explain it.

The two independent resolutions break the invariant the other way too. Edit Agent.spec.execution.mode between the two calls and the run gets counted against supervised while it is actually burning host CPU. So the doc comment's "slot accounting cannot drift from the path actually taken" is not quite true as written, and "Execute fails it on the same error a moment later" only holds when the failure is persistent.

I would rather we resolve the Agent once and pass the decision through to Execute than resolve it twice and hope the two agree. That also removes the duplicated prologue: SupervisesExternally currently repeats executor_native.go:284-310 step for step, so we pay a second GET per claimed task and have two copies of the same logic to keep in sync.

Blocking: the headline behaviour is not covered

pkg/foreman/agent/watcher_concurrency_test.go:115

modeExecutor.SupervisesExternally ignores its task argument and returns one fixed mode, so both concurrency tests run a uniform fleet: two in-process tasks, or two Job-mode tasks. What is not exercised is the mixed case, which is the whole point of the PR. Specifically:

• the [BUG] Job-mode AgenticTask holds the agent's single in-flight slot for the Job's whole lifetime #1559 state itself, an in-process run holding inflight while a Job-mode task is claimed anyway
• the claim in your README section that a node supervising Job-mode tasks can still pick up an in-process review or gate task, which needs supervised > 0 and an in-process claim at the same time

Keying modeExecutor off the task name, a map[string]bool would do, gets you both directions cheaply. AGENTS.md asks for a test per behaviour change and these are the two behaviours being changed.

Please fix before merge, but small

The new tunable does not reach the agents that need it. charts/foreman/templates/agent-deployment.yaml has no --max-supervised-tasks argument and values.yaml has no key for it, so chart-managed in-cluster agents, which are the ones actually running Job-mode coders, are pinned at the hardcoded 4. The docs you added in this same PR tell operators to "use that one to protect" their nodes, so right now we are documenting a knob they cannot turn. A {{- if $agent.maxSupervisedTasks }} arg plus the values key, per pool alongside mode and roles, should do it.

--max-supervised-tasks=0 does the opposite of what it looks like. maxSupervised() maps anything <= 0 to the default of 4. Every other int flag in that block treats 0 as unset or off (--max-context-tokens, --tokens-per-second, --total-ram-gb all document it that way), so someone who wants to disable fan-out and get the old serialization back types =0 and gets four concurrent coder Jobs, silently. Either reject negatives and treat 0 as 1, or document that 0 means the default.

Poll loop got more expensive than intended

Two related things, and I do not think either was deliberate:

watcher.go:303. The "we are busy, skip the List" short-circuit is effectively dead now, because hasCapacityFor(true) is true whenever supervised < 4. A node running --agent-mode=stub, or a native node with no Job-mode Agent, used to return immediately with zero API calls. It now issues a full uncached namespace List every poll interval for the entire run and then skips every candidate. On the 8 node fleet from your description that is roughly 96 pointless Lists a minute, sustained. Folding the executor's capability into the guard fixes it:

_, canSupervise := w.Executor.(SupervisingExecutor)
if !w.hasCapacityFor(false) && (!canSupervise || !w.hasCapacityFor(true)) {
    return nil
}

watcher.go:332. w.supervises fires one uncached GET per Scheduled candidate per poll, including for candidates we cannot claim. Busy node with three tasks queued behind it is 36 Agent GETs a minute, per node, forever, and in practice those candidates almost always share a single Agent name. Memoizing per pollOnce pass, or only resolving the mode for the candidate about to be claimed, takes care of it.

Accuracy nits worth correcting while we are here

api/foreman/v1alpha1/fleetnode_types.go:159. The CurrentTask godoc still says "empty if idle" and "the scheduler skips nodes with a non-empty CurrentTask (v0.1 concurrency is one task per node)". After this PR a node can be running one in-process task plus four supervised ones, and the second sentence has been false for Job mode since #1496 and #1558 anyway. You documented the caveat in the markdown README, which is great, but kubectl explain fleetnode.status.currentTask and the shipped CRD still tell operators an empty CurrentTask means idle and available. Worth fixing at the source, then make manifests and make foreman-chart-crds since the chart copy is generated.

docs/site/foreman/README.md:80. The reason given for the one in-process task limit is not the real one. The doc says the task "owns the host's CPU, memory and workspace directory; running two would have them fight over the same workspace", but executor_native.go:345 builds filepath.Join(workspaceRoot, task.Namespace, task.Name), so workspaces are already per task and torn down per run. The actual constraint is host CPU and RAM plus the shared toolchain. I care about this one because publishing a mechanism that does not exist invites the next person to lift the limit on the grounds that they have "fixed" the workspace collision, which was never there.

charts/foreman/templates/agent-deployment.yaml:29. The comment justifying strategy: Recreate says each agent claims one task at a time, which is now wrong, and the blast radius it reasons about grew. On restart recoverOrphanedTasks resets every Running task on the node to Pending, so a chart upgrade can now yank back up to five tasks at once instead of one, each re-submitting a coder Job. Not asking you to solve the drain problem in this PR, but the comment should stop claiming the old bound.

Follow-ups, happy for these to be separate

• watcher.go:346. pollOnce still claims at most one task per tick and returns. That was right when there was one slot. With independent slots, four Job-mode tasks landing at once now fill over about 20 seconds at the default cadence, and because sortTasksDepthFirst puts review and verify ahead of issue-fix, a Job-mode claim also defers in-process work behind it by a full interval. The loop probably wants to keep scanning after a successful launch until no slot has capacity.
• watcher.go:128, the bigger one. The supervision budget lives only in the agent's memory while the cluster's model of node capacity is still the single FleetNode.status.currentTask string. reserveFirstFitNode returns the first eligible node for Job mode without reserving it, so the controller keeps stamping tasks onto the same node, and once its agent hits the bound those tasks sit at Scheduled with assignedNode set and never migrate, because checkClaimExpiry only releases on a stale heartbeat and the node is heartbeating fine. You filed the spread half as [BUG] Job-mode AgenticTasks all land on the alphabetically first FleetNode #1634, which I appreciate. The part that worries me more is that there is no metric, status field, or log line for "held back by the supervision bound", so that state is invisible from the cluster. Worth thinking about whether the budget belongs in FleetNode.status rather than a process-local int.

Tests, minor

• watcher_concurrency_test.go:82. The new assertions reach into w.supervised and w.inflight. AGENTS.md asks for observable behaviour rather than internals, and the observable equivalent is already in this file: after the error path, a follow-up pollOnce should be able to claim another task. If we later move the budget to a semaphore or a channel, which finding above makes likely, these break without the behaviour breaking.
• watcher_concurrency_test.go:48. t.Cleanup(close(release)) unblocks Execute goroutines that nothing joins, so they call patchTerminal and log after the test body returns. It passes today, I checked under -race -count=3, but it is an unjoined goroutine per subtest and it is the standard setup for a "log in goroutine after test has completed" panic the first time something in this package points logf at a testing sink. A sync.WaitGroup waited on after the close would close it off.
• watcher_concurrency_test.go:275. stubCoderJobSubmitter duplicates fakeCoderJobSubmitter from executor_native_test.go:1823. Three interchangeable doubles for one two method seam is more than we need; reusing the existing one is fine here.

Thanks again for this. The diagnosis is right and the shape of the fix is right; it is the error path and the test matrix that need another pass. Happy to talk through the resolve-once approach if you want to sketch it before writing it.

[joryirving]

Thanks Chris, you're right on all of it.

Taking the error path first, because your framing corrected mine. I had SupervisesExternally returning in-process on a failed GET and called that failing closed, which was backwards: in-process is the slot-holding answer, so a transient 429 buys exactly the bug the PR claims to fix, with the error swallowed so nothing says why. And you're right that resolving twice is what permits drift in the first place, so the doc comment claiming accounting can't drift from the path taken is false as written. Resolving once and threading the decision through is the correct shape.

Sketch of what I intend, tell me if you'd cut it differently. The watcher resolves the Agent once for the candidate it is about to claim, and derives the mode from that single read. It then hands the resolved Agent to Execute rather than letting Execute do its own GET, which deletes the duplicated prologue at executor_native.go:284-310 and makes one read authoritative for both the accounting and the dispatch. That means changing the Executor.Execute signature to carry the resolved Agent, or a small value holding it plus the derived mode, and SupervisingExecutor disappears rather than growing.

On the failure case: if that single GET fails, the right answer is to not claim the task at all. Skip the candidate, log the error, let the next poll retry. Unknown mode should mean no claim, not a guessed claim, which I think is the actual fail-closed behaviour I thought I'd written.

One consequence worth flagging early since it touches your dead-short-circuit fix: if SupervisingExecutor goes away, the canSupervise type assertion in the guard needs another way to ask whether this executor can ever supervise. Simplest is probably a static property of the executor rather than a per-task question, but I'd rather agree the shape with you than guess.

On the tests, agreed and it's the more embarrassing of the two. modeExecutor returning a fixed mode means both cases are uniform fleets, so the mixed case, which is the entire point, is untested, along with the in-process-claimed-while-supervising claim I made in the README. Keying the double off task name is the fix. I'll write those once the resolve-once shape is settled, since the double's shape follows from it.

The chart gap is mine and it's a bad one: I documented a knob in the same PR that adds it and never checked an operator could reach it. Same for =0 silently meaning 4 when every sibling int flag treats 0 as off.

The dead short-circuit I straightforwardly did not think about. Roughly 96 pointless Lists a minute on my fleet is a regression I introduced and your guard is the fix.

On the README workspace claim, thank you for pushing on this rather than letting it go as a wording nit. It was fabricated. executor_native.go:345 builds a per-task path and I asserted a collision that has never existed, and your reason for caring is the right one, since the next person lifts the limit on the grounds they fixed something that was never broken. Replacing it with the actual constraint, host CPU and RAM plus the shared toolchain, and nothing I haven't checked.

Taking the CurrentTask godoc and the CRD too. Documenting the caveat in markdown while kubectl explain still says empty means idle is the worse half of the problem, and you're right that the second sentence has been wrong since #1496 regardless of this PR.

Both follow-ups make sense as separate issues and I'll file them. The second one is the one I keep coming back to as well: a process-local int is a strange place for capacity to live when the cluster's model is still a single currentTask string, and the invisible-backpressure part is worse than the placement half I filed as #1634. Tasks land on the concentrated node, hit the bound, and sit at Scheduled with nothing anywhere saying that's why. Keeping it out of this PR though.

I've started on everything except the two blocking items, which I'll hold until we've agreed the resolve-once shape.

[Defilan]

Yes to the resolve-once shape. Resolve the Agent once for the candidate about to
be claimed, derive the mode from that read, and hand the same Agent to Execute.
That is the right cut and it deletes the duplicated prologue at
executor_native.go:284-310 as a side effect.

The four commits you pushed all look right to me. The chart knob reaches agents
now and values-multi.yaml covers the per-pool path; the README replacement
names the real constraint and the actual per-task path, which is exactly what
stops the next person re-deriving the collision that never existed; the
CurrentTask godoc now says plainly that empty does not mean idle, and both
generated CRD copies match; and the poll-loop guard and the per-pass memo are
what I had in mind. go test ./pkg/foreman/agent/ -race is green here and
helm unittest charts/foreman is 42/42.

On canSupervise: the interface does not need to go away

I think the worry dissolves once the method stops doing I/O. Keep
SupervisingExecutor, change what it asks:

type SupervisingExecutor interface {
    SupervisesAgent(agent *foremanv1alpha1.Agent) bool
}

NativeAgentLoopExecutor implements it as return e.useCoderJobPath(agent), a
one-liner over the resolved Agent you already have in hand. No ctx, no GET, no
second read. That gets you three things at once:

• the "accounting cannot drift from the path actually taken" claim becomes true,
  because it is now the same function applied to the same value rather than two
  independent resolutions that can disagree
• the type assertion in the poll guard is unchanged, so canSupervise keeps
  working and the dead-short-circuit fix stays exactly as you wrote it
• the stub still does not implement it, so the guard still fires for stub nodes

Why a bare static property would only get you half

Worth being precise about, because it is what makes the interface the right home
for this. useCoderJobPath is a conjunction of two different kinds of fact:

if e.CoderJobSubmitter == nil { return false }              // static: instance wiring
return agent.Spec.Execution.Mode == ExecutionModeJob        // dynamic: per-Agent

A static executor property answers the first half only. If the watcher computed
supervise itself it would need both halves, which means it would have to know
about CoderJobSubmitter. That is executor wiring leaking into the dispatch
loop. Passing the resolved Agent back to the executor keeps the predicate where
it lives and the read where it belongs.

On whether the type assertion is a safe proxy for "can this executor ever
supervise": today yes, but by wiring rather than by construction. There really is
a nil-submitter NativeAgentLoopExecutor in production, the one RunTask builds
inside the coder Job pod so a Job cannot recurse into another Job
(cmd/foreman-agent/main.go:454 spells that out). It never meets a watcher:
RunTask constructs none, and the only watcher at main.go:486 always gets the
submitter-wired executor. So type-has-method and instance-can-supervise agree.

If they ever diverge the failure is silent but cheap: canSupervise reads true,
the guard stops firing, and the pointless Lists come back. API load, not
correctness. I would record that assumption in a comment on the guard and not
build anything further for it.

For scope: there are only three Executor.Execute implementations to touch,
StubExecutor, NativeAgentLoopExecutor, and the blockingExecutor double that
modeExecutor embeds.

One refinement on the failure case

"Unknown mode means no claim" is right for transient errors, and it is the
fail-closed behaviour you were reaching for. But a blanket skip-on-error changes
behaviour for a deleted Agent. Today Execute resolves it, hits IsNotFound,
and returns FailureAgentNotFound as a terminal result
(executor_native.go:292-295). Under blanket skip that task sits at Scheduled
indefinitely with nothing anywhere saying why, which is the same invisible
backpressure you and I both flagged as the worrying half of the follow-up.

I would split it:

• apierrors.IsNotFound on the Agent: claim the task and let Execute fail it
  with FailureAgentNotFound, preserving today's terminal outcome. The scheduler
  should not have placed it, and a task whose Agent is genuinely gone deserves a
  verdict, not a silent hold.
• any other error: skip the candidate, log it, retry next poll.

That keeps the ambiguous case fail-closed without stranding the unambiguous one.

Two small leftovers

watcher_concurrency_test.go still carries stubCoderJobSubmitter alongside
fakeCoderJobSubmitter in executor_native_test.go. Minor, and easy to lose in
a bigger pass, so flagging it again rather than assuming.

Heads up on a collision rather than a request: your rewritten Recreate comment
keeps the claim that the strategy stops two agents racing for the same Scheduled
tasks. I have a change in flight against #1438 editing that same comment in the
other direction, because that race turns out to be unreachable today:
FLEET_NODE_NAME is set from spec.nodeName but read by nothing, so identity
falls back to os.Hostname(), which is the pod name, and pollOnce filters
claims with if t.Status.AssignedNode != w.NodeName { continue }
(watcher.go:336 on your branch, :265 on main).
Filed as #1640. Two pods can only share a FleetNode identity under the intended
design, which the dead wiring has been defeating. Not asking you to absorb that
here, but let us not land two contradictory explanations of the same comment. The
#1438 change is smaller, so I will sequence around whichever of ours lands first.

Nothing else from me. The blocking pair is the last of it.

[Defilan]

One small follow-up from the collision I flagged earlier, now that the identity
question is settled: #1640 is decided (comment there has the full reasoning),
and the outcome affects one line of your Recreate comment rewrite.

Your current wording keeps "stops a rolling update from briefly running two
agents that race for the same Scheduled tasks" as a present-tense fact. It
turns out that race is unreachable today: FLEET_NODE_NAME is dead wiring, so
each replica registers its own FleetNode from its pod name, and pollOnce
claims only tasks assigned to its own node. Two pods are never offered the same
task. And with #1640 resolved to process-scoped identity (the env var now feeds
a new status.kubernetesNode property instead, #1649), that stays true by
design rather than by accident.

The blast-radius half of your comment — one in-process task plus up to
--max-supervised-tasks supervisions, all reset by recoverOrphanedTasks on
restart — is the genuinely load-bearing part and worth keeping exactly as you
wrote it. Suggested replacement for just the first sentence:

# Recreate: never runs two agents for this pool at once. Note the blast
# radius: an agent holds one in-process task plus up to
# --max-supervised-tasks Job-mode supervisions (#1559), and on restart
# recoverOrphanedTasks resets EVERY Running task on the node back to
# Pending, so an upgrade can yank back more than one run at a time and
# each is re-dispatched from the start.

Sequencing note: #1647 touches the same region (it makes the strategy
configurable and the surrounding comment mode-conditional), so whichever of the
two lands second rebases over the other. If #1647 goes first, this line
resolves itself in the rebase and you only need to keep your blast-radius
sentences. Nothing else in your plan changes; the resolve-once shape we agreed
is unaffected.

[joryirving]

Both of your refinements are better than what I sketched, and one of them catches a regression I would have shipped.

Keeping SupervisingExecutor and changing it to SupervisesAgent(agent) is the right cut. I had the interface disappearing, which would have forced the poll guard to be rebuilt around some other capability signal for no gain. Taking the I/O out instead gets the same "one read, one predicate" property, keeps canSupervise and the short-circuit exactly as they are, and leaves the stub still not implementing it. And your point about useCoderJobPath being a conjunction is the part I had not thought through: a static property answers the wiring half only, so the watcher would end up knowing about CoderJobSubmitter, which is precisely the leak I was trying to avoid by putting the question on the executor in the first place.

The IsNotFound split I had straightforwardly wrong. Blanket skip-on-error would have taken a deleted Agent from a terminal FailureAgentNotFound to a task parked at Scheduled forever with nothing recording why — the same invisible hold we both called the worrying half of the follow-up, introduced by the fix meant to avoid it. Claiming the not-found case so Execute can fail it terminally, and skipping only on ambiguous errors, is right.

Noted on the guard assumption. I will record that type-has-method and instance-can-supervise agree today by wiring rather than construction, with the nil-submitter RunTask executor as the reason it holds, and leave it at that rather than building for a divergence whose cost is API load.

Taking your Recreate wording verbatim and keeping the blast-radius sentences. Thanks for catching that before it landed — I had rewritten one wrong explanation into another, and #1640 settling the identity question is what makes the replacement true by design rather than by accident. Happy to rebase over #1647 if it goes first.

Also taking another look at stubCoderJobSubmitter. It was declined last pass on a package boundary, executor_native_test.go being package agent_test and the fake unexported, but SupervisesAgent not needing a submitter to answer may dissolve the need for a double there at all. If it does not, I will say why rather than leave it flagged a third time.

Working the blocking pair now: resolve-once in the shape above, and the mixed-mode matrix keyed off the task, covering the in-process-holding-inflight-while-a-Job-mode-task-is-claimed case and the supervising-plus-in-process-claim case, plus the not-found and transient-error paths.

[Defilan]

Approving. All twenty items are addressed, and I checked each against the branch rather than reading the summary.

The two blockers are genuinely closed. resolveTaskAgent is now the single read and Execute/SupervisesAgent consume the value it returns, so the two independent resolutions that reintroduced #1559 are gone; the mixed-fleet gap is covered in both directions at watcher_concurrency_test.go:475. Both perf regressions are fixed the way we discussed: the poll guard asserts zero Lists over three polls, and the per-pass memo asserts one GET for three candidates.

I spent most of the time on the concurrency, since that is where this PR can hurt. It holds up:

• Release is exactly once, on every path. Acquire is synchronous in launchExecutor before the goroutine starts, and the release defer is registered first inside it, so LIFO puts it after patchTerminal. That also means there is no race between freeing the slot and recording the terminal status, which was the thing I was most worried about. Error return, context cancellation and panic unwinding all pass through it, and the PR adds no defer inside a loop.
• No early release. Submit blocks until the Job reaches a terminal phase, so the supervision slot is held for the Job's whole lifetime, bounded by PollTimeout.
• No second writer to FleetNode.status. CurrentTask is still written only by the controller's reserveNode/clearNodeCurrentTask, the heartbeat only by the registrar's own loop, and the budget is a process-local counter under inflightMu. The part that makes this actually safe rather than accidentally safe is that reserveFirstFitNode deliberately does not stamp CurrentTask for Job-mode, so a second claimed task never contends for a single-valued field.
• Check-then-act is sound because only pollOnce reserves and it is single-threaded, while executor goroutines only release. Capacity can only grow between the check and the reserve.

I also confirmed the restart story: recoverOrphanedTasks resets Running to Pending, and reapPreviousJobs deletes the previous Job, so there is no double-run. The blast radius growing from 1 to 1+N is real and the chart comment says so plainly.

Gates on your head commit: build, vet, full go test (35 packages, zero failures), -race over the new tests five times, golangci-lint clean on darwin and GOOS=linux, helm unittest 42/42.

On test quality, I mutated the ten load-bearing behaviours and every one was caught. The two that matter most: making supervises() always false, which is the #1559 bug itself, fails three tests; and flipping IsNotFound to a blanket skip, which is the regression I flagged in the first round, fails TestPollOnce_DeletedAgentIsStillClaimed. No test in the new suite reads w.supervised or w.inflight directly, so T1 is honoured, and stubCoderJobSubmitter is gone as you predicted on its third flag.

Thanks for filing #1638 and #1639 rather than widening this PR into them, and for taking SupervisesAgent over the alternative.

Five minor things, none blocking, take or leave:

1. cmd/foreman-agent/main.go:504 logs the raw flag rather than the resolved value, so an operator who passes --max-supervised-tasks=0 sees maxSupervisedTasks: 0 at startup while the agent actually runs 4. That is exactly the confusion the =0 doc change was about, surviving in the one place an operator looks to confirm it.
2. executor_coderjob.go:127 dereferences agent.Spec with no nil check. Safe today via the two call-site guards, but the thesis of this PR is one predicate and one value, so the guard belongs inside the predicate.
3. watcher_concurrency_test.go:108 calls e.wg.Add(1) inside the launched goroutine, so t.Cleanup's wg.Wait() is only sound because every test's waitStarted(n) matches its claim count exactly. They all do today, and I checked all eight. A future test that counts low reintroduces the unjoined-goroutine hazard. Pre-adding outside the goroutine makes it structural instead of hand-maintained.
4. Worth a note on #1638: the per-pass memo hands one shared *Agent pointer to callers. Nothing writes to it today and pollOnce returns after a single claim, so it is fine now, but multi-claim off one memo entry would make it shared across goroutines.
5. Sequencing only: the Recreate comment collides with #1647 and #1438. Whoever lands second rebases, and #1647's comment needs to keep your blast-radius text, since an agent holding one in-process task plus N supervisions is exactly what makes a rolling update yank back more than one run.