Skip to content

[sandbox audit] Address background processes by their server-assigned id - #4836

Draft
Wauplin wants to merge 1 commit into
security/pool-lifecycle-ownershipfrom
security/opaque-process-ids
Draft

[sandbox audit] Address background processes by their server-assigned id#4836
Wauplin wants to merge 1 commit into
security/pool-lifecycle-ownershipfrom
security/opaque-process-ids

Conversation

@Wauplin

@Wauplin Wauplin commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

[sandbox audit] — PR 7 of 9 in this repo's stack; merge in order.
Previous: #4840 · Next: #4837
Review only the commits this PR adds on top of its base; bases collapse to main as the stack lands.

Server PR: huggingface/sandbox-server#22deploy the server first.

Why

SandboxProcess.kill() sent the OS pid. The server matches on the opaque id it assigned
(p-3) and answered 200 regardless. So the call stopped nothing, reported success, and left
the process running — which also kept the sandbox non-idle, so idle eviction never fired and
the job kept billing.

What changed

  • SandboxProcess carries id as its identifier; kill() uses it.
  • pid stays, documented as observational — useful for correlating with ps inside the
    sandbox, but the OS may reuse it.
  • kill() returns whether this call stopped the process, so a caller can distinguish
    "stopped it" from "it had already exited". Both are successes; they're different answers.
  • A process from a host running an older server has id=None; kill() raises with an
    explanation rather than sending a pid the new server will reject.
  • processes() docstring corrected: the server keeps a bounded number of finished
    processes, not all of them until the sandbox is deleted.

The fake server was the root cause

This survived 41 green tests because the fake deleted by pid — it implemented a protocol
nobody serves. It now deletes by opaque id only and 400s on a pid, exactly like the real
server. And there's a test that asserts the pid path fails, so if the fake goes lax again
that gets caught rather than silently re-enabling the bug.

This is the second instance of the same failure mode in this stack (the other was the fake
accepting the host token on every route), which is why it's worth fixing the fake rather than
just the client.

Validation

60 tests pass, up from 55. New coverage: kill() sends the opaque id; it's idempotent and
reports which case; listed processes carry their id; sending a pid is refused; a process
without an id refuses to be killed with a clear message.

Behaviour changes

  • kill() returns bool instead of None. Existing code ignoring the return value is
    unaffected.
  • Against an old server, kill() now raises instead of silently doing nothing. That is the
    point, but it means pool hosts booted before the server upgrade need recycling for
    kill() to work at all — previously it "worked" by doing nothing.

Note

Medium Risk
Behavior change: kill() now raises on old servers without opaque ids (pool hosts need recycling after server deploy); return type changed from None to bool.

Overview
Fixes SandboxProcess.kill() so it actually terminates background work: it now sends DELETE /processes/{id} with the server’s opaque process id (e.g. p-0), not the OS pid the server ignores while still returning success.

SandboxProcess gains an id field (from spawn/list responses); pid stays for correlating with ps only. kill() returns bool (True if this call stopped it, False if already gone) and raises SandboxError when id is missing (pre-upgrade pool hosts)—instead of silently doing nothing.

Docs drop the “kill doesn’t work” caveat and clarify that finished processes are listed only up to a bounded server history. Tests align the fake server with production (kill by id, 400 on pid) and add regression coverage for idempotent kill and legacy hosts.

Reviewed by Cursor Bugbot for commit 4b27a62. Bugbot is set up for automated code reviews on this repo. Configure here.

@bot-ci-comment

bot-ci-comment Bot commented Sep 8, 2026

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

`SandboxProcess.kill()` sent the OS pid. The server matches on the opaque id
it assigned (`p-3`) and answered 200 regardless, so the call stopped
nothing, reported success, and left the process running -- which also kept
the sandbox non-idle, so idle eviction never fired and the job kept billing.

`SandboxProcess` now carries `id` as its identifier and `kill()` uses it.
`pid` stays, documented as observational: it is useful for correlating with
`ps` inside the sandbox, but the OS may reuse it. `kill()` returns whether
this call is what stopped the process, so a caller can tell "stopped it"
from "it had already exited" -- both are successes, but they are different
answers.

A process from a host running a server that predates opaque ids has
`id=None`; `kill()` raises with an explanation rather than sending a pid the
new server will reject.

The test fake is the reason this survived a green suite: it deleted by pid,
so it implemented a protocol nobody serves. It now deletes by opaque id only
and 400s on a pid, exactly like the real server -- and there is a test that
asserts the pid path *fails*, so the fake going lax again would be caught
rather than silently re-enabling the bug.

Also corrects the `processes()` docstring: the server keeps a bounded number
of finished processes, not all of them until the sandbox is deleted.

Requires the server change to be deployed first. Validation: 60 tests pass,
up from 55.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Wauplin
Wauplin force-pushed the security/opaque-process-ids branch from 87ba141 to 4b27a62 Compare September 8, 2026 15:11
@Wauplin
Wauplin changed the base branch from security/host-adoption-admission-control to security/pool-lifecycle-ownership September 8, 2026 15:11
@Wauplin Wauplin changed the title Address background processes by their server-assigned id [sandbox audit] Address background processes by their server-assigned id Sep 9, 2026
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.

1 participant