Skip to content

[sandbox audit] Make the process id protocol unambiguous - #22

Draft
Wauplin wants to merge 1 commit into
security/robust-process-supervisionfrom
security/opaque-process-ids
Draft

[sandbox audit] Make the process id protocol unambiguous#22
Wauplin wants to merge 1 commit into
security/robust-process-supervisionfrom
security/opaque-process-ids

Conversation

@Wauplin

@Wauplin Wauplin commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

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

Companion client PR: huggingface/huggingface_hub#4836 — deploy this server first.

Why

DELETE /v1/processes/{id} matches on the server-assigned opaque id (p-3) and answered
{"id": ..., "ok": true} whatever was passed. The client sends the OS pid. So:

  • the delete matched nothing,
  • the server reported success,
  • the process kept running,
  • and because it kept running, the sandbox never looked idle — so idle eviction never fired
    and the job kept billing.

Both halves of that were silent, which is why it survived: the client's fake test server
deletes by pid, so the whole suite was green against a protocol nobody implements.

What changed

  • A malformed id is a 400 that names what to use instead, rather than a no-op. This is the
    part that makes the mistake loud, and it's worth keeping permanently — the same class of
    client bug then cannot recur quietly.
  • killed: true vs killed: false, so a caller can tell "stopped it" from "it was
    already gone". Still idempotent: a second delete is a 200 with killed: false.
  • POST /v1/exec {background: true} now returns id. It allocated one, registered it,
    and returned only pid and tag — so the single identifier DELETE accepts was never
    disclosed, and a process started that way was unstoppable through the documented protocol
    even after the fix above. The Python client happens to use POST /v1/processes instead, so
    this was latent, but the route is public and documented.

Validation

41 unit tests, including:

  • every id the registry allocates has a shape the route accepts — the two can't drift
    apart, which is the actual root cause here;
  • a bare pid and a dozen near-misses (p-, p, P-1, p-1a, " p-1", …) are rejected;
  • a scoped lookup does not reach a sibling sandbox's process, and removal is not idempotent
    at the registry level (so the route's idempotency comes from the route, not from a
    double-remove).

Behaviour changes

  • DELETE /v1/processes/{id} with a numeric id now returns 400 instead of 200. Anything
    relying on that was relying on the bug — but it does mean the current client's kill()
    starts erroring instead of silently doing nothing
    , which is why the client PR must ship
    with this and why the server must be deployed first.
  • The response field is killed rather than ok.

`DELETE /v1/processes/{id}` matches on the server-assigned opaque id
(`p-3`), and answered `{"id": ..., "ok": true}` whatever was passed. The
client sends the OS pid. So a `kill()` matched nothing, the server reported
success, and the process kept running and kept the sandbox non-idle --
which also defeats idle eviction, so the job kept billing.

Both sides of that were silent, and both are fixed here:

- A malformed id -- a bare number, most likely a pid -- is now a 400 that
  says what to use instead, rather than a cheerful no-op. This is the change
  that turns the mistake loud; keeping it permanently means the same class of
  client bug cannot recur quietly.
- The reply distinguishes `killed: true` from `killed: false`, so a caller
  can tell "stopped it" from "it was already gone". Still idempotent: a
  second delete of the same id is a 200 with `killed: false`.

Also fixes a gap in the same protocol that the client happens not to hit:
`POST /v1/exec {background: true}` allocated an opaque id, registered it,
and then returned only `pid` and `tag`. The one identifier `DELETE` accepts
was never disclosed, so a process started through that route was
unstoppable through the documented protocol -- and would have stayed so even
after the fix above. It now returns `id` as well.

Validation: 41 unit tests, including that every id the registry allocates
has a shape the route accepts (so the two cannot drift apart), that a bare
pid and a dozen other near-misses are rejected, and that a scoped lookup
does not reach a sibling sandbox's process.

The companion client change is in huggingface_hub.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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