feat(runnerd): idle auto-stop for sessions whose agent has finished - #87
feat(runnerd): idle auto-stop for sessions whose agent has finished#87jiashuoz wants to merge 7 commits into
Conversation
The near-term half of #85: a session whose child has exited and that has had no attachment for --idle-stop is cold-suspended by the runner, exactly as `rainier stop` does — files kept, slot released, resumable by attach. Records what a stop actually is end to end (suspended_cold, not warm), the rule, the claim-under-lock that makes the sweep race-free, the two additive capacity counts, and the cold-resume placement-generation limitation this change does not introduce and does not fix. Co-Authored-By: Claude <noreply@anthropic.com>
A sandbox held one of --slots from creation until somebody stopped or deleted its session, whether or not its agent had finished hours earlier: fourteen finished sessions filled a hosted runner while three agents worked. runnerd now stops a session whose child has exited and that has had no attachment for --idle-stop (default 30m, 0 disables), by the same cold suspend `rainier stop` performs — container stopped, files kept, slot released, state suspended_cold, resumable by attach. A session whose child is still running is never stopped however long nobody has watched it, and nothing is ever deleted. The decision and the claim are one locked registry call, so two sweeps, or a sweep and a stop arriving from controld, cannot both stop one session, and a session that stops being idle in between is simply not claimed. Both stops go through one coldSuspend, so the operator's and the runner's can never drift. The idle clock starts at the later of the child's exit and the last detach. Reports it three ways: one structured log line per stop, an unsolicited suspended_cold event (which controlapp's transition table already accepts, so the session row follows the sandbox), and two additive counts — active and idle_exited — beside the used/total that already ride every runner message. Design: docs/design/runner-idle-stop.md. Near-term half of #85. Co-Authored-By: Claude <noreply@anthropic.com>
Drives the feature end to end with no docker: a websocket standing in for the container's sessiond reports child_exited over the real control channel, a real client on /attach holds the session open past the timeout, and only once it leaves and the timeout passes does the runner stop it — slot released, workspace kept, registry and announce on suspended_cold, and the event with its two counts arriving at a fake controld. The sandbox conn is then killed, as `docker stop` kills it in production, to pin that the entry survives it. The dial_attach front gets the same coverage, because a viewer who came through controld's attach plane must hold a session open exactly as a local one does. Fixes a bug the end-to-end test found in the commit before it: the detach was recorded as `defer s.reg.attachEnded(id, s.now())`, and a deferred call's arguments are evaluated where the defer is written — so every detach was stamped with the time of its own ATTACH, and a session being watched right now looked like one nobody had touched since the viewer arrived. Both fronts now take the clock inside a closure. Also walks the new suspended_cold event through runnerplane's translation table into control.StateSuspendedCold. Co-Authored-By: Claude <noreply@anthropic.com>
Seven findings, each with the regression test that catches it (every one verified by mutating the fix back out and watching the test fail): A control frame can outlive the sandbox boot that sent it — a hub read loop stalled writing to a wedged viewer drains its buffered frames whenever it comes back, which can be after the sandbox has been stopped, resumed and re-registered. That buffered child_exited landed on the NEW boot, and half an hour later the runner stopped a session whose agent was working: the one thing this must never do. Every control frame now carries the boot token its /register minted, and the registry drops a child exit naming any other — the same guard, for the same reason, as hubDied's deadHub. The auto-stop event no longer carries a placement generation. A cold resume opens a new generation on the session row but sends the runner none, so the runner's is stale from a session's second life onwards and the event would be fenced — leaving the row reading "running" over a stopped container, which `rainier attach` will not resume and cannot reach. Zero fences nothing, and a re-placement to a different runner is still fenced by runner identity. A warm suspend, a resume and a snapshot now take the session out of the sweep for their duration: each leaves the entry reading "running" while it runs, so a sweep could turn an operator's pause — which deliberately keeps the slot — into a stop, or stop a container mid-`docker commit`. A session whose boot chain failed is never auto-stopped. Its child exits with the failing stage, so it looks exactly like a finished agent; but attaching to read the log that says why is the only thing left to do with it, and neither a stopped sandbox nor a failed row can serve that. The stop's rollback and its landing state are compare-and-swap on "suspending", so a Delete that overtook the stop keeps its "destroying" marker instead of having it wiped — the marker that stops the register goroutine destroying the container twice and reporting the session dead. The attach front counts a viewer from the moment the session is known to be attaching, not after the first client frame: that frame crosses the client's network, and a sweep in that window stopped the session under them. Both driver calls on the stop path are bounded (30s, 5s, the bounds this package already uses off a background goroutine), so a wedged daemon cannot park the sweep goroutine for good, and a failed capacity reading logs "unknown" rather than a plausible-looking number. Also pins the new wire tags in protocol/runner, and corrects comments and the design doc where they over-claimed: the monotonic clock does not survive host suspend, reconciliation runs per connection rather than "on the next announce", counts are not an arithmetic partition of used, and cmd/runnerd's wiring has no test. Co-Authored-By: Claude <noreply@anthropic.com>
Both reviewers re-read the fixes and landed on the same worst one: the boot token was minted per REGISTRATION, so a plain sessiond redial — where the container never restarted and the child never changed — opened a new epoch and dropped a child_exited that was already in flight across it. sessiond re-sends only events it never delivered, so that was the only copy: the session would have held its slot for the life of the runner, silently, which is the incident this whole change exists to end. register now READS the epoch and only a cold resume moves it — the only thing that can change the child, since sandboxes run with --rm and nothing sets a restart policy. Bounding `docker stop` turned out to manufacture a case that destroyed sessions: the CLI is killed at the deadline while the daemon goes on stopping the container, and believing that error rolled the entry back to "running" — the container then died seconds later, the register goroutine read a "running" entry as a crash, destroyed the container and reported a merely idle session dead. A failed stop now asks the driver what the container is actually doing and lands the entry on the answer, leaving it parked when there is no answer. Also: a failed attach handshake no longer stamps the detach time (the local /attach surface has no auth, so a client looping on a failed dial could have pushed every deadline on the runner out indefinitely); markBootFailed is boot-guarded like the child exit and cleared by a cold resume, so a late stage failure cannot pin a healthy session out of auto-stop for ever; the dial_attach front counts its viewer from before the dial, not after it and the hub wait; the in-flight bracket is taken before the guard it protects rather than after; beginColdSuspend joins its two siblings in respecting a Delete's marker; and Recover says in the log that the sessions it rebuilt are exempt. Nine mutations, nine caught — including one gap the reviewers found in my own first round: the resume-window epoch bump was not pinned by any test, and the redial case needed a test that goes through the real /register handler rather than the registry underneath it. Co-Authored-By: Claude <noreply@anthropic.com>
The third review pass found the last door into the one invariant. Whether a resumed session's agent is a NEW process was inferred from a flag this runner set when it ASKED for a cold suspend — the runner's intent, not what the container did — and the two come apart in both directions: A warm-paused container that the docker daemon restarts underneath a surviving runnerd (a daemon upgrade, an OOM kill; the runner is a host process, not a container) is resumed with `docker start` while the entry still reads paused. The new agent inherited the old one's exit and was stopped half an hour into its work — exactly what idle auto-stop must never do. And an entry left marked cold by a stop whose outcome could not be read is resumed by an unpause, or by nothing at all, while the boot epoch moved anyway — under a connection that is still alive, whose captured token then went stale. That sandbox's child exit was dropped, sessiond re-sends only what it never delivered, and the session would have held its slot for the life of the runner in silence. Nothing above the driver can tell the two apart: Inspect folds paused, exited and created into one StateSuspended. So Driver.Resume now reports `restarted bool` — true only where it ran a start — the contract suite pins that every driver agrees on it (unpause and already-running report false, a cold resume reports true), the fake models it, and resumed() clears the idle bookkeeping and closes the boot epoch on exactly that answer. The intent flag is gone. Also converts Delete's own rollback to the same compare-and-swap as its three siblings, so a cold suspend that landed while drv.Destroy was in flight is not stamped back to "running" over a container that really is stopped. Four more mutations, four caught. Co-Authored-By: Claude <noreply@anthropic.com>
The last review round's residual. `docker start` on an already-started container exits 0, so a second resume reports a restart as honestly as the first — and two are a real shape, since the control plane dispatches the command to the runner before it transitions the row, so two racing clients both send one. The second bump moved the boot epoch out from under the connection the restarted sandbox had already opened, whose child exit was then dropped: a slot that never comes back, silently. resumed() now clears the bookkeeping only when the resume is also the one that brought a parked entry back, which consumes the transition the way the deleted intent flag used to. The design doc records why the hub pointer is not the guard here (it would be per-connection again, and would drop an exit in flight across a plain redial), and names the one corner this leaves: an entry the runner keeps as "running" because Inspect failed, resumed through the local dev surface, restarts without clearing. Also drops the stale comment about the flag that is gone. Co-Authored-By: Claude <noreply@anthropic.com>
|
Independent Opus review of head 6db15bc (semantics, races, wire additivity and scope verified as stated). Verbatim: Review complete. Head Findings, most severe first1. Medium — mixed-version deploy reproduces exactly the "no way back into the session" outcome the design doc treats as unacceptable. 2. Medium — 3. Low/Medium — the capacity counts are actively misleading after a runnerd restart, which is the one case the feature cannot handle. 4. Low — two comments still describe the mechanism that round 3 deleted. 5. Low — a resume that overtakes an in-flight stop can leave the entry 6. Low — an operator cannot tell an auto-stop from their own stop on the session. 7. Low — no operator knob on the hosted launcher. 8. Nit — the PR body's follow-up 2 contradicts the code it describes. It says "An auto-stop event about a cold-resumed session is fenced the same way", but Verified clean
Not exercised here (no Docker): Merge recommendation: merge after fixes 1 and 2 — everything else is comment/doc or genuinely narrow. 1 is a deploy-sequencing hazard that costs a user access to a live session and needs only a documented order or a flag default; 2 is a one-line A fix session has been dispatched for findings 1–8. |
1 participant
What this does
runnerd now stops a session whose child process has exited and that has had no attachment — viewer or controller, on either attach front — for
--idle-stop(a duration flag, default30m,0disables), by exactly the pathrainier stoptakes: a cold suspend, so the container is stopped, the workspace volume is kept, the slot is released, the registry entry lands on"suspended", the announce and the unsolicited event both saysuspended_cold, the session row readssuspended_cold(whichrainier lsprints asstopped), andrainier attachresumes it; the idle clock starts at the later of the child's exit and the last detach, so reading a finished agent's scrollback for an hour resets nothing while attached and gives the full timeout after detaching; a session whose child is still running is never stopped however long nobody has watched it, a session with any attachment open is never idle, a session the operator already stopped is never stopped again, a resumed session becomes eligible again only through its new child's exit, and nothing is ever deleted.Design:
docs/design/runner-idle-stop.md. This is the near-term half of #85, which supersedes it: resource-aware admission, queueing instead of refusal, warm/cold suspension policy, one shared activity signal, and the status report are that issue's, and #85 names this half as the agreed small one.How it is safe
The decision and the claim are one locked registry call (
claimIdle): it re-checks the whole rule and marks the entry"suspending"in the same critical section, so two sweeps — or a sweep and a stop arriving from controld — cannot both stop one session, and a session that stops being idle in between is simply not claimed. Both stops go through onecoldSuspend, so the operator's and the runner's can never drift apart; a driver refusal rolls the entry back to"running"and the next sweep tries again."suspending"is the marker that keeps the register goroutine from reading the resulting sessiond conn death as a crash and destroying the container. Every duration isnow.Sub(stored)with both times from one clock (time.Nowin production, a fake in tests), so a wall-clock step cannot make a session look idle early.Saying it
runnerd: idle auto-stop session=… idle=… slots_free=… slots_total=… active=… idle_exited=….suspended_coldevent to controld.controlapp's transition table already accepts exactly that ({running, suspended_cold} → suspended_cold), so onlyrunnerplane's translation arm was needed and no control-plane change was.used/totalthat already ride every runner message:active(sandbox up, child running) andidle_exited(sandbox up, child gone).active + idle_exited ≤ used— a warm-suspended or still-creating sandbox is in neither count.What the reviews changed
Two Opus reviews (independent + adversarial) and three verification rounds, each waited for before the next change. Fourteen findings, fourteen regression tests, and every fix verified by mutating it back out and watching its test fail (14/14 caught). Two of the rounds found defects in the previous round's fixes, which is why there were three.
Round 1 (
1698ce0)child_exitedlanded on the new boot, and half an hour later the runner stopped a session whose agent was working. Every control frame now carries the boot token its/registerminted, and the registry drops a child exit naming any other — the same guard, for the same reason, ashubDied'sdeadHub.runningover a stopped container, whichrainier attachwill not resume and cannot reach. Zero fences nothing, and a re-placement onto a different runner is still fenced by runner identity. (See follow-up 2 for the real fix.)"running"while it runs, so a sweep could have turned an operator'sdocker pause— which deliberately keeps the slot — into a stop, or stopped a container mid-docker commit.failedrow (not resumable) can serve that. It holds its slot until someone removes it — deliberate, and reclaiming those is Runner capacity: replace the fixed slot count with resource-aware admission and idle auto-suspend #85's."suspending", so aDeletethat overtook the stop keeps its"destroying"marker rather than having it wiped.unknownrather than a plausible-looking number.Plus: the new wire tags are pinned, and comments/design doc corrected where they over-claimed (the monotonic clock does not survive host suspend; reconciliation runs per connection, not "on the next announce"; the counts are not an arithmetic partition of
used;cmd/runnerd's wiring has no test).Round 2 (
cfc3214) — including a defect a round-1 fix introducedchild_exitedalready in flight across it. sessiond re-sends only what it never delivered, so that was the only copy: the session would have held its slot for the life of the runner, silently, which is the incident this change exists to end.registernow reads the epoch; only a cold resume moves it (sandboxes run with no restart policy, so nothing else can change the child).docker stopmanufactured a session-destroying case. The CLI is killed at the deadline while the daemon goes on stopping the container; believing that error rolled the entry back to"running", the container died seconds later, and the register goroutine read that as a crash — destroying the container and reporting a merely idle session dead. A failed stop now asks the driver what the container is actually doing, and leaves the entry parked when there is no answer./attachsurface has no auth, so a client looping on a failed dial could have pushed every deadline on the runner out indefinitely).markBootFailedis boot-guarded and cleared by a cold resume. Thedial_attachfront counts its viewer from before the dial. The in-flight bracket is taken before the guard it protects.beginColdSuspendjoins its siblings in respecting aDelete's marker.Recoversays in the log that the sessions it rebuilt are exempt.Round 3 (
b246779,6db15bc) — the last door into the invariantWhether a resumed session's agent is a new process was inferred from a flag the runner set when it asked for a cold suspend. Intent and outcome come apart both ways: a warm-paused container that the docker daemon restarts underneath a surviving runnerd (an upgrade, an OOM kill — the runner is a host process) is resumed with
docker startwhile the entry still reads paused, so the new agent inherited the old one's exit and was stopped half an hour into its work. And an entry left marked cold by a stop whose outcome could not be read moved the epoch under a connection that was still live, deafening it.Nothing above the driver can tell the two apart —
Inspectfolds paused, exited and created into one state — soDriver.Resumenow reportsrestarted bool, the driver contract suite pins that every driver agrees on it, and the reset follows that answer rather than the runner's intent. It clears only on the resume that actually unparks the entry, sincedocker starton an already-started container also exits 0 and two racing resumes are a real shape.Follow-ups, deliberately not in this PR
active/idle_exitedyet — this PR carries them on the wire and wires nothing further. Makingrainier statussay "16 slots, 3 active, 13 idle" instead of "no free capacity" needscontrol.Runner, the fleet repository and the CLI, which is Runner capacity: replace the fixed slot count with resource-aware admission and idle auto-suspend #85's status report.ResumeSessionopens a new placement generation for a cold resume, but theresumecommand carries none, so the runner keeps echoing its create-time generation andApplyRunnerEventfences every later event about that session —deadandchild_exitedincluded, today, before this change. An auto-stop event about a cold-resumed session is fenced the same way; the runner still frees the slot and the row heals on the next announce. Fixing it means carrying the generation onresume(protocol + controlapp).Recoverafter a runnerd restart are not auto-stop candidates until they report a new child exit — the exit fact lived only in memory. Safe direction; a durable activity record is Runner capacity: replace the fixed slot count with resource-aware admission and idle auto-suspend #85's.--slots' default is unchanged at 16; the hosted runner passes its own. Both flags are now documented incmd/runnerd's help and the README's new "Runner capacity" section.Tests
No test needs Docker (
internal/driver.Fakeplus this package's in-process/registersandbox is the whole stack below the runner):internal/runnerd/idlestop_test.go— table-driven on a fake clock: 29m59s vs 30m, stopped once and not once per sweep, a child still running for hours, a viewer attached, the timer running from the detach, one of two viewers leaving,--idle-stop 0, a session the operator already stopped, a resumed session whose child exits again, warm pause/unpause keeping the fact, eight concurrent sweeps stopping it exactly once, a driver refusal rolling back, a re-deliveredchild_exitednot moving the deadline, the sweep-interval clamp, and the loop itself.internal/runnerd/idlestop_e2e_test.go— the real path end to end: a websocket sessiond reportingchild_exitedover the real control channel, a real/attachclient holding the session open past the timeout, the stop after it leaves, the workspace still there, the event and its counts arriving at a fake controld, and the sandbox conn then dying asdocker stopkills it. Same for thedial_attachfront.runnerplane/plane_test.go— the new event state translating tocontrol.StateSuspendedCold.One interface change comes with this:
driver.Driver.Resumereturns(restarted bool, err error). Both implementations, the contract suite and every call site are updated; the bool exists because idle auto-stop's correctness depends on it and only a driver can answer it.make verifyis green with PostgreSQL 17 up (RAINIER_TEST_PG_DSNpointed at the maintenance database, sointernal/controld/pgstoreandinternal/e2erun rather than skip), andgo test ./internal/runnerd/ -racepasses.🤖 Generated with Claude Code