fix(sandbox): stop counting pod-less sandboxes against the running limit - #1555
Open
0xAlcibiades wants to merge 1 commit into
Open
fix(sandbox): stop counting pod-less sandboxes against the running limit#15550xAlcibiades wants to merge 1 commit into
0xAlcibiades wants to merge 1 commit into
Conversation
A Sandbox CR with `spec.replicas: 1` and no backing Pod mapped to `SandboxStatus::Created`, which consumes a running slot. Nothing could ever return that slot: - The admission controller's stale-retirement arm only fires on `Stopped`/`Gone`/`NotFound`, so it never sees a CR reporting `Created`. - Idle pausing needs a DB-idle-eligible session. A session whose execution rows are stuck non-terminal is never eligible. - The reaper only stops past-max-lifetime sandboxes, and the cleanup worker only reaps sandboxes no session references. So the observed running count sits permanently one above reality. Once it reaches the cap, every `cold_create` and `resume` is refused with "sandbox running capacity exceeded", the execution is marked failed with no retry, and the originating thread receives nothing. The fleet stays saturated until an operator deletes the CR by hand. The fix is a portable `SandboxStatus::Vacant`: the record asks for a process and the backend has none for it. That is a different thing from `Created`, where a process exists and is still coming up, and from `Suspended`, where nothing is asking for one. Because `status_consumes_running_slot` lists the statuses that count rather than the ones that do not, adding the variant is what fixes the accounting, in both the admission path and the warm pool (replenishment is gated on the same count, so a phantom blocked refills too). The cleanup worker gets a second arm that pauses vacant sandboxes, so the CR stops asking for a replica that never arrives instead of reporting drift on every reconcile. It pauses and never stops: `replicas: 0` keeps the CR, its state volume and its proxy, so the owning session resumes with its workspace intact, while stopping would delete the volume and every uncommitted change on it. Retirement needs two consecutive sweeps, mirroring the existing orphan arm, because a fresh create is briefly vacant between the CR landing and its pod being scheduled. Both sandbox arms now share one `list_observed` call. They do different things to a sandbox, so they should not disagree about what they saw. Elsewhere `Vacant` is handled where the compiler required a decision: reconciliation reports drift against a Running desired state and pauses against a Suspended one; admission and the idle backstop fall through to their pause, which is the repair; and `existing_sandbox_action` resumes rather than replaces, because the state volume outlived the pod.
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
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.
Closes #1554.
A
SandboxCR withspec.replicas: 1and no backing Pod maps toSandboxStatus::Created, which consumes a running slot. Nothing can return that slot: the admission controller's stale-retirement arm fires only onStopped/Gone/NotFound, idle pausing needs a DB-idle-eligible session, the reaper only stops past-max-lifetime sandboxes, and the cleanup worker only reaps sandboxes no session references. So the count drifts permanently above reality, and once it hitsmax_runningeverycold_createandresumeis refused, the execution is marked failed with no retry, and nothing surfaces on the thread that triggered it.The fix
A portable
SandboxStatus::Vacant— the record asks for a process and the backend has none for it. That is a different state fromCreated, where a process exists and is still coming up, and fromSuspended, where nothing is asking for one. TodayCreatedmeans both, which is exactly why the count and the reclamation paths cannot tell them apart.Because
status_consumes_running_slotlists the statuses that do count rather than the ones that do not, adding the variant is what fixes the accounting — in the admission path and in the warm pool, whose replenishment is gated on the same count and so was blocked from refilling by the same phantom.A second cleanup-worker arm that pauses vacant sandboxes. Not counting them is the capacity fix; this is the other half. Left alone the CR keeps requesting a replica that never arrives, so reconciliation reports drift on every pass and it never settles.
It pauses and never stops.
replicas: 0keeps the CR, its state volume and its proxy, so the owning session resumes with its workspace intact; stopping would delete the volume and every uncommitted change on it. That holds whether or not a session still references the sandbox — an unreferenced one readsSuspendedafter this arm runs, and the existing orphan arm can then reap it on its usual two passes.Retirement needs two consecutive sweeps, mirroring
pending_orphans, because a fresh create is briefly vacant between the CR landing and its Pod being scheduled.Both sandbox arms now share one
list_observedcall. They do different things to a sandbox, so they should not disagree about what they saw.The other
VacantarmsThe compiler required a decision at each of these; recording the reasoning since none is forced:
ReconcilePlan, desiredRunningReportDrift(MissingWhileRunning)ReconcilePlan, desiredSuspendedPausereplicasis still 1, so pausing is the repair.ReconcilePlan, desiredStoppedStoppause_capacity_candidaterecord_idle_pauseexisting_sandbox_actionResumeOrReplaceTests
maps_agent_sandbox_replicas_and_pod_readiness_to_status—(1, None)is nowVacant.vacant_sandboxes_do_not_consume_running_slotsin bothcentaur-session-runtimeandcentaur-sandbox-manager::warm_pool— the bug in one assertion each.vacant_retire_requires_two_consecutive_passes,a_sandbox_that_gets_its_pod_back_is_not_retired,only_vacant_sandboxes_are_retired— the new arm.vacant_sandboxes_are_left_to_the_retire_arm— the orphan arm does not stop one, so the state volume survives.vacant_sandbox_resumes_rather_than_replaces.cargo test --workspace,cargo clippy --workspace --all-targetsandcargo fmt --all --checkare clean.Scope
Capacity accounting and pod-less retirement only. Evicting paused sandboxes under pressure, and queueing instead of dropping at the cap, are separate problems — a turn refused at admission still fails silently with nothing on the originating surface, which this does not change.
SandboxStatusis public, so the new variant is a breaking change for anything matching on it exhaustively outside this workspace. Happy to gate it differently if you would rather not take that.