Skip to content

Refuse a warm pool no Bot can be handed a computer from - #273

Open
zopeVaibhav wants to merge 1 commit into
CopilotKit:mainfrom
zopeVaibhav:fix/warm-pool-never-claimed
Open

Refuse a warm pool no Bot can be handed a computer from#273
zopeVaibhav wants to merge 1 commit into
CopilotKit:mainfrom
zopeVaibhav:fix/warm-pool-never-claimed

Conversation

@zopeVaibhav

Copy link
Copy Markdown
Contributor

Closes #272

What this changes

computers.sandbox.warmPool.enabled=true renders a SandboxWarmPool, grants the service account create and delete on sandboxclaims, and holds replicas browsers ready. A Bot reaches a warm one by claiming it, and nothing in the server creates a SandboxClaimlocate posts a new Sandbox and 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: sandbox on 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.yaml and the sandboxclaims grant 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.yaml comment 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 SandboxClaim and 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 SandboxClaim schema. 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

  • New state that outlives a request? None. Chart templates and a CI step; no server code is touched.
  • What happens on the second replica? Nothing differs. This changes what helm template will render, not what any process does.
  • Anything serialised? Nothing new.
  • Anything fanned out to a browser? No.
  • New listener, port, or schedule? No. It removes the ability to ask for pods that do nothing.

Boundary and audit

  • Every acting call still goes through the gateway: no acting call is touched.
  • New refusals and new failures each write a row: this refusal is at helm template time, before anything runs, so there is no trail to write to and none is skipped.
  • Nothing new is trusted from the client: no request handling is touched.

Changelog

No CHANGELOG.md entry. Every deployment on main has warmPool.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-versions for the CRDs, as the chart's own CI does.

Pool on:

$ helm template ob charts/openbot --set computers.mode=sandbox \
    --set computers.sandbox.warmPool.enabled=true ...
exit: 1
Error: execution error at (openbot/templates/validation.yaml:133:4): computers.sandbox.warmPool.enabled
is on, but nothing claims from the pool yet: the server creates a Sandbox per Bot and never a
SandboxClaim, so the pool would run and bill without ever shortening a first action. Set
computers.sandbox.warmPool.enabled=false until claiming ships.

Pool off, which is the default and every shipped values file:

exit: 0

The same command against main, with the pool on:

exit: 0

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.yaml passes.

The CI step

The chart job already has a Refusals fire block whose purpose is that no fail in this chart is decorative. This adds one line to it, armed only where the target asks for per-Bot computers:

if grep -qE '^ *mode: sandbox' charts/openbot/ci/${{ matrix.target }}-values.yaml; then
  refuses "a warm pool no Bot can be handed a computer from" \
    --set computers.sandbox.warmPool.enabled=true
else
  echo "skipped: the warm-pool refusal is only armed for computers.mode: sandbox"
fi

Of the five values files, only eks-sandbox-values.yaml is mode: sandbox, and it does not set warmPool, so it inherits false and the existing render stays green. On main this step fails, because helm template exits 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 way server/tests/computer-sandbox.test.ts does:

provider.warm implemented?  undefined
methods: name, isolation, locate, status, stop, reset, list, sessionOf

GET  /apis/agents.x-k8s.io/v1beta1/namespaces/openbot/sandboxes/bot-knowledge-19lp3wl   404
POST /apis/agents.x-k8s.io/v1beta1/namespaces/openbot/sandboxes
GET  /apis/agents.x-k8s.io/v1beta1/namespaces/openbot/sandboxes/bot-knowledge-19lp3wl

Three calls, none of them a claim, while two warm sandboxes sit idle. typeof provider.warm is undefined for all three provider shapes — docker, shared and sandbox — and grep -rn sandboxclaim server/ returns nothing.

Not done: no live cluster. Everything above is helm template and 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The warm pool is created and never claimed from, so a Bot's first action still waits for a cold start

1 participant