Refuse a warm pool no Bot can be handed a computer from - #273
Open
zopeVaibhav wants to merge 1 commit into
Open
Conversation
zopeVaibhav
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
August 27, 2026 06:08
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 #272
What this changes
computers.sandbox.warmPool.enabled=truerenders aSandboxWarmPool, grants the service accountcreateanddeleteonsandboxclaims, and holdsreplicasbrowsers ready. A Bot reaches a warm one by claiming it, and nothing in the server creates aSandboxClaim—locateposts a newSandboxand waits for it, pool or no pool.warm?()is declared on the provider interface and called at startup, but no provider implements it, so the call is skipped and TypeScript is content because the method is optional.The pool therefore runs, reports healthy, and bills for browsers nobody is handed, while every first action waits exactly as long as it did before. Nothing fails, so there is no moment at which an operator finds out.
This makes the chart refuse to render with the pool on, and says why. It is the same treatment the chart already gives
computers.mode: sandboxon a cluster with no Sandbox CRD — an install that would look finished and is not — and the argument is stronger here, because the CRD case at least fails at the first browser action while this one never fails at all.Nothing is deleted.
warmpool.yaml,sandbox-template.yamland thesandboxclaimsgrant stay exactly as they are, and the refusal block says in its own comment to remove it when the provider claims from the pool. The missing half is the claim, not any of the chart.The
values.yamlcomment stops promising a shorter first action and says what the state actually is.The alternative, and why I did not send it
The other way to close this is to implement claiming: have the sandbox provider create a
SandboxClaimand adopt the sandbox the controller hands back, instead of posting a fresh one. That is clearly the better end state and it is what the chart was written for.I did not send it because I would be guessing at the
SandboxClaimschema. Nothing in this repository describes one — no field names, no example, no type — so the shape would come from reading an upstream controller rather than from anything here, and I could not verify it against a cluster. A refusal I can prove beats a claim path I cannot.If claiming is close, this is the wrong patch and I will drop it. If it is not close, this stops the pool costing money quietly in the meantime. Happy either way.
Where it runs
helm templatewill render, not what any process does.Boundary and audit
helm templatetime, before anything runs, so there is no trail to write to and none is skipped.Changelog
No
CHANGELOG.mdentry. Every deployment onmainhaswarmPool.enabled: false, which is the default and the only value that rendered a working deployment before this. An operator who had it on was already getting nothing from it; they now get told so at install instead. Say the word if you would rather that were written down.Proof
The refusal fires, and only where it should. All three runs use
--api-versionsfor the CRDs, as the chart's own CI does.Pool on:
Pool off, which is the default and every shipped values file:
The same command against
main, with the pool on:That is the bug: it renders happily, and two browsers start that nothing will ever reach.
helm lint charts/openbot --values charts/openbot/ci/eks-sandbox-values.yamlpasses.The CI step
The chart job already has a
Refusals fireblock whose purpose is that nofailin this chart is decorative. This adds one line to it, armed only where the target asks for per-Bot computers:Of the five values files, only
eks-sandbox-values.yamlismode: sandbox, and it does not setwarmPool, so it inheritsfalseand the existing render stays green. Onmainthis step fails, becausehelm templateexits 0 on the state it is asserting is refused.How the finding was made
Every request the sandbox provider makes on a cold start, captured through the provider's own injectable
fetchImpl, the wayserver/tests/computer-sandbox.test.tsdoes:Three calls, none of them a claim, while two warm sandboxes sit idle.
typeof provider.warmisundefinedfor all three provider shapes — docker, shared and sandbox — andgrep -rn sandboxclaim server/returns nothing.Not done: no live cluster. Everything above is
helm templateand the provider's own test harness, so this proves what the chart renders and what the provider sends, not how the agent-sandbox controller behaves.