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

docs/source/en/concepts/sandbox.md

@@ -249,7 +249,6 @@ This section is deliberately exhaustive rather than reassuring: if you are decid
 ### Lifecycle and operational gaps
 
 - **A detached descendant can outlive `kill()`.** `SandboxProcess.kill()` signals the command's process group; a descendant that calls `setsid()` leaves it. Deleting the sandbox (pool) or the job (dedicated) does terminate everything. Use `timeout=` if you need a hard bound.
-- **`SandboxProcess.kill()` does not currently stop the process** — the client sends the OS pid where the server expects its own opaque process id, and the server answers `200` either way. Until this is fixed, stop background work by deleting the sandbox.
 - **A long foreground command can trip the idle watchdog.** `idle_timeout` counts API requests, and a running foreground command is not counted as activity, so a command that runs longer than `idle_timeout` without other API traffic can have its sandbox shut down under it. Raise `idle_timeout` (or pass `None`) for long single commands.
 - **`max_hosts` is best-effort, not a hard cap.** It is now checked against every host running for the pool (found via labels), not just the ones the current process tracks — but two processes can still count simultaneously and both decide there is room. A hard cap has to be enforced where Jobs are created, not by clients racing to count them. Per-host `sandboxes_per_host` *is* enforced server-side.
 - **The server binary is not pinned or verified.** Each job downloads `sbx-server` from a mutable public bucket path and executes it as root without checking a digest or signature.

src/huggingface_hub/_sandbox.py

@@ -167,31 +167,44 @@ class SandboxProcess:
     """A background process started in a sandbox with [`Sandbox.run`]`(..., background=True)`.
 
     List a sandbox's processes with [`Sandbox.processes`] and stop one with [`SandboxProcess.kill`].
-    Completed processes stay in the listing until the sandbox is deleted, so `running` and
-    `exit_code` tell whether a process is still alive or already exited (as of when it was listed).
+    Recently completed processes stay in the listing (the server keeps a bounded number of
+    them), so `running` and `exit_code` tell whether a process is still alive or already
+    exited (as of when it was listed).
     """
 
+    # Server-assigned handle, and the only identifier that addresses this process.
+    # `pid` is the OS pid: useful for correlating with `ps` inside the sandbox, but
+    # the OS may reuse it, so it is observational only. `None` for a process on a
+    # host running a server that predates opaque ids.
+    id: str | None
     pid: int
     cmd: str | List[str]
     # Back-reference to the sandbox, used by `kill()`. Excluded from repr/eq so a process
-    # stays a plain data object (and two with the same pid compare equal).
+    # stays a plain data object.
     _sandbox: "Sandbox" = field(repr=False, compare=False)
     tag: str | None = None
     started_at_ms: int | None = None
     running: bool = True
     exit_code: int | None = None
 
-    def kill(self) -> None:
-        """Terminate the background process (idempotent server-side).
+    def kill(self) -> bool:
+        """Terminate the background process. Idempotent.
 
-        > [!WARNING]
-        > This currently does not stop the process: it addresses it by OS pid, while the
-        > server expects the opaque process id it assigned, and answers `200` either way.
-        > Until that is fixed, stop background work by deleting the sandbox. Note also that a
-        > descendant which detaches with `setsid()` leaves the signalled process group and
-        > outlives this call.
+        Returns whether this call is what stopped it: `False` means it had already
+        exited or been terminated, which is not an error.
+
+        Note that a descendant which detaches with `setsid()` leaves the signalled
+        process group and outlives this call. Delete the sandbox to be certain
+        everything it started is gone.
         """
-        self._sandbox._request("DELETE", f"/processes/{self.pid}")
+        if self.id is None:
+            raise SandboxError(
+                "This process cannot be stopped individually: its sandbox runs a sandbox server "
+                "that predates opaque process ids, so the server never issued one. Recycle the "
+                "pool's hosts, or delete the sandbox to stop everything it started."
+            )
+        response = self._sandbox._request("DELETE", f"/processes/{self.id}")
+        return bool(response.json().get("killed", False))
 
 
 @dataclass
@@ -1031,7 +1044,15 @@ def run(
             payload["cwd"] = cwd
         if background:
             data = self._request("POST", "/processes", json=payload).json()
-            return SandboxProcess(pid=data["pid"], cmd=cmd, tag=data.get("tag"), _sandbox=self)
+            return SandboxProcess(
+                # Absent on a server that predates opaque ids; `kill()` then says so
+                # rather than sending a pid the server will reject.
+                id=data.get("id"),
+                pid=data["pid"],
+                cmd=cmd,
+                tag=data.get("tag"),
+                _sandbox=self,
+            )
         if timeout is not None:
             payload["timeout"] = timeout
         if stdin is not None:
@@ -1087,12 +1108,14 @@ def processes(self) -> List[SandboxProcess]:
         """List the background processes of this sandbox.
 
         Returns the processes started with [`Sandbox.run`]`(..., background=True)`; stop one
-        with [`SandboxProcess.kill`]. Completed processes stay listed (with `running=False` and
-        their `exit_code`) until the sandbox is deleted.
+        with [`SandboxProcess.kill`]. Recently completed processes stay listed (with
+        `running=False` and their `exit_code`); the server keeps a bounded number of them, so a
+        sandbox that has run thousands of short commands will not list them all.
         """
         data = self._request("GET", "/processes").json()
         return [
             SandboxProcess(
+                id=p.get("id"),
                 pid=p["pid"],
                 cmd=p["cmd"],
                 tag=p.get("tag"),

tests/test_sandbox.py

@@ -127,9 +127,9 @@ def _ndjson(self, events) -> None:
             self.wfile.write((json.dumps(event) + "\n").encode())
             self.wfile.flush()
 
-    def _json(self, obj) -> None:
+    def _json(self, obj, status: int = 200) -> None:
         body = json.dumps(obj).encode()
-        self.send_response(200)
+        self.send_response(status)
         self.send_header("Content-Type", "application/json")
         self.send_header("Content-Length", str(len(body)))
         self.end_headers()
@@ -172,6 +172,7 @@ def do_POST(self) -> None:
         elif self.path.endswith("/processes"):  # spawn a background process
             type(self).last_exec = body
             proc = {
+                "id": f"p-{cls.proc_seq}",
                 "pid": 9000 + cls.proc_seq,
                 "tag": body.get("tag"),
                 "cmd": body["cmd"],
@@ -181,7 +182,7 @@ def do_POST(self) -> None:
             }
             cls.proc_seq += 1
             cls.processes.append(proc)
-            self._json({"pid": proc["pid"], "tag": proc["tag"]})
+            self._json({"id": proc["id"], "pid": proc["pid"], "tag": proc["tag"]})
         elif self.path == "/v1/sandboxes":  # batch-create sandboxes (server-authoritative capacity)
             count = int(body.get("count", 1))
             created = []
@@ -208,8 +209,15 @@ def do_DELETE(self) -> None:
         self._expect_token()
         last = self.path.rsplit("/", 1)[-1]
         if "/processes/" in self.path:  # kill a background process
-            type(self).processes = [p for p in type(self).processes if str(p["pid"]) != last]
-            self._json({"pid": last, "killed": True})
+            # Deletion is by opaque id only, and a pid is a 400 -- exactly like the
+            # real server. The fake used to delete by pid, which is what let the
+            # client's `kill()` send a pid and still pass every test.
+            if not (last.startswith("p-") and last[2:].isdigit()):
+                self._json({"error": f"{last!r} is not a process id"}, status=400)
+                return
+            before = len(type(self).processes)
+            type(self).processes = [p for p in type(self).processes if p["id"] != last]
+            self._json({"id": last, "killed": len(type(self).processes) != before})
             return
         type(self).sandboxes.discard(last)
         self._json({"id": last, "deleted": True})
@@ -449,6 +457,55 @@ def test_list_follows_the_servers_pagination(self, fake_server: str) -> None:
             _FakeServer.list_pages = None
 
 
+class TestBackgroundProcesses:
+    """`kill()` has to address a process the way the server identifies it. It used
+    to send the OS pid, which matches nothing server-side, and the server answered
+    200 anyway -- so a process the user asked to stop kept running, kept the
+    sandbox non-idle, and kept the job billing."""
+
+    def test_kill_uses_the_server_assigned_id(self, fake_server: str) -> None:
+        sandbox = _make_sandbox(fake_server)
+        process = sandbox.run("sleep 60", background=True)
+
+        assert process.id == "p-0"
+        assert process.pid == 9000  # still exposed, for correlating with `ps`
+        # The fake rejects a pid with a 400, exactly like the real server, so this
+        # passing is evidence the opaque id was sent.
+        assert process.kill() is True
+        assert sandbox.processes() == []
+
+    def test_kill_is_idempotent_and_says_whether_it_did_anything(self, fake_server: str) -> None:
+        sandbox = _make_sandbox(fake_server)
+        process = sandbox.run("sleep 60", background=True)
+
+        assert process.kill() is True
+        assert process.kill() is False  # already gone: not an error
+
+    def test_listed_processes_carry_their_id(self, fake_server: str) -> None:
+        sandbox = _make_sandbox(fake_server)
+        sandbox.run("sleep 60", background=True)
+        listed = sandbox.processes()
+
+        assert [p.id for p in listed] == ["p-0"]
+        assert listed[0].kill() is True
+
+    def test_sending_a_pid_is_refused_by_the_server(self, fake_server: str) -> None:
+        # Guards the fake as much as the client: if this ever passes, the fake has
+        # gone lax again and the original bug could return unnoticed.
+        sandbox = _make_sandbox(fake_server)
+        process = sandbox.run("sleep 60", background=True)
+        with pytest.raises(SandboxError):
+            sandbox._request("DELETE", f"/processes/{process.pid}")
+
+    def test_a_process_without_an_id_refuses_to_be_killed(self, fake_server: str) -> None:
+        # A host running a server that predates opaque ids issues no id. Better a
+        # clear error than a pid the server will reject.
+        sandbox = _make_sandbox(fake_server)
+        process = sandbox_mod.SandboxProcess(id=None, pid=9001, cmd="sleep 60", _sandbox=sandbox)
+        with pytest.raises(SandboxError, match="predates opaque process ids"):
+            process.kill()
+
+
 class TestSharedSandbox:
     """A shared sandbox routes operations under /v1/sandboxes/<local_id>/ and is
     terminated with a DELETE on the host (the host job keeps running)."""