feat(python): streaming-stdio popen — Sandbox.popen + Process (RFC #67)

[dzerik]

Python ergonomic layer over the C ABI sandlock_popen / sandlock_handle_kill (merged in #120). Third in the RFC #67 popen series: core (#118) → FFI (#120) → Python (this PR).

Sandbox.popen(cmd, stdin/stdout/stderr=StdioMode) returns a live Process whose PIPED streams are unbuffered binary file objects the caller reads/writes while the child runs; INHERIT/NULL streams are None. Streams default to INHERIT (parity with subprocess.Popen).

• Process is a context manager: __exit__ kills+reaps the child (kill before wait, so an undrained pipe cannot hang teardown) and closes the streams.
• wait(timeout=None) bounds the wait via sandlock_handle_wait_timeout; on timeout the child is killed and a non-success Result returned (parity with Sandbox.run), so the wait cannot hang forever.
• wait() closes a still-open piped stdin first (so a reader child like cat sees EOF), caches its Result (idempotent, no double-free), and frees the handle.
• __del__ safety net: a Process dropped without wait()/context reaps the child and emits a ResourceWarning (best-effort) instead of leaking the handle.
• stdio modes are normalized/validated with StdioMode(x) — a bad mode raises a clear ValueError in Python rather than an opaque null handle across the FFI.
• On a failed fd handoff, Process.__init__ closes any fds it opened and popen() frees the handle — nothing leaks on the error path.

Tests mirror the FFI suite and add destructively-verified coverage: stdout streaming, stdin/stdout roundtrip through cat, wait()-delivers-EOF-to-unclosed-stdin (watchdog), wait(timeout) kills-and-returns (watchdog), both stdout+stderr piped, Null yields no stream, kill lifecycle, context-manager reap + sandbox reuse, del reaps an abandoned process, idempotent cached wait, unknown-mode rejection, second-process guard.

Refs #67

[congwang-mk]

Review focused on high-level API design. The API shape is right: popen returning a Process, StdioMode as the ABI-stable IntEnum, INHERIT defaults matching subprocess.Popen, validation before the FFI boundary, and unusually honest deadlock documentation. Tests are genuinely destructive (watchdogs, GC reaping). Four inline comments below; the first one (handle ownership) is the load-bearing design decision and the only one I would hold the merge on.

[congwang-mk]

Design: Process borrows Sandbox._handle instead of owning its handle. Every Process method resolves the handle through self._sandbox._handle at call time (here, plus wait at 1334, __exit__ at 1375, __del__ at 1399), so a Process means "whatever child the sandbox is running right now", not "the child I spawned". Once the sandbox is reused (which this PR supports and tests), a stale Process aliases the new child:

p1 = sb.popen(["a"]); p1.wait()   # frees handle, sb._handle = None
p2 = sb.popen(["b"])              # sb._handle = b's handle
p1.kill()                          # SIGKILLs b's process group

A second p1.wait() is worse: the cached-result shortcut only fires when sb._handle is None, so with p2 live it waits on and frees p2's handle and overwrites p1._result with b's result; p2.wait() then raises "process is not running". And the plain rebind loop p = sb.popen(a); p.wait(); p = sb.popen(b) installs b's handle before the old p is decref'd, so old p.__del__ (its guard checks only sb._handle is None, not self._result) warns, SIGKILLs, and reaps the brand-new child.

Suggested fix: move the handle onto Process (owned for its lifetime), and keep on Sandbox only a busy marker for the one-process-per-sandbox rule plus pid/is_running. That makes stale-Process aliasing structurally impossible instead of guarded. The minimal patch (early return in kill/wait/__del__ when self._result is not None) closes these scenarios but leaves the aliasing latent for the next method someone adds.

[congwang-mk]

API overlap: Sandbox.wait()/kill()/pause()/resume() also drive a popen handle, with conflicting semantics. Because the popen handle lives in the same _handle slot the create/start/spawn lifecycle uses, sb.wait() (line 820) "works" on a popen'd sandbox, but: it does not close a piped stdin (a cat child hangs it forever), it returns empty captured stdout/stderr, and it frees the handle behind the Process's back, so a later proc.wait() raises RuntimeError for a child that exited normally. Nothing steers users away from this trap. This falls out of the handle-ownership comment at line 1308: if Process owns the handle, sb.wait() on a popen'd sandbox fails loudly (or delegates) instead of half-working.

[congwang-mk]

timeout=0 and sub-millisecond timeouts wait indefinitely. int(timeout * 1000) if timeout else 0 maps timeout=0 and e.g. timeout=0.0005 to 0, which the FFI treats as wait-forever, the opposite of the caller's intent. Should be if timeout is not None, clamping sub-ms values up to 1. Copied from run() (line 561), so it is parity with an existing bug; worth fixing the idiom here rather than duplicating it, with a follow-up for run().

[congwang-mk]

kill() discards the FFI error return. sandlock_handle_kill distinguishes already-exited (0, fine per its contract) from genuine failure (-1), but the return value is dropped here, so a real kill failure is indistinguishable from success and the caller proceeds to a wait() that may block forever. Raise (or at least warn) on -1.

[dzerik]

Thanks — the handle-ownership comment was the load-bearing one, and you're right that the minimal guard would have left the aliasing latent. Made the structural change instead:

• Process owns its handle for its lifetime; every method acts on self._handle and never re-resolves through the sandbox. Sandbox keeps only a busy marker — and it's a weakref, so the one-process-per-sandbox rule and pid/is_running/ports still see the live child, while abandoning a Process still triggers its __del__ reap instead of the sandbox pinning it alive. Your three aliasing scenarios (stale kill() hitting the next child, stale wait() reaping it, the rebind-loop __del__) are now structurally impossible; added a regression test for the reuse case.
• sb.wait()/kill()/pause()/resume() on a popen'd sandbox now fail loudly (they raise, directing you to the Process) instead of freeing/signalling behind the Process's back.
• timeout=0 / sub-ms: clamp a finite timeout up to 1 ms so it no longer collapses to 0 (= wait forever); None still waits forever. Fixed the same idiom in run() while there.
• kill() return: rather than checking sandlock_handle_kill's return, I switched Process.kill to signal the process group by a cached pid (like Sandbox.kill and the Go binding). That was needed anyway — driving the kill through the handle races wait() freeing it from another thread (the documented watchdog pattern). Process now guards the handle with a lock + waiting flag; the blocking wait runs without the lock so a kill from another thread interrupts it safely. A genuine kill failure raises (an already-exited group is a no-op). Added a concurrent kill-during-wait test.

Also moved the handle assignment to after the fd wrapping in Process.__init__, so an fdopen failure leaves ownership with popen()'s caller and can't double-free via __del__.

Force-pushed; CI green.

[congwang-mk]

Thanks for the quick followup. Some additional reviews below:

[congwang-mk]

Is it safe to use self.handle without _lock?

[congwang-mk]

Defined but not used?

[congwang-mk]

One more question: does checkpoint() need to be updated too? I don't see it in the diff.

[dzerik]

Thanks — got all three:

• checkpoint() — right, it needed the same guard. It now _reject_if_popen()s (SIGSTOP + ptrace freeze would fight the streaming Process for the handle), consistent with wait/pause/resume/kill.
• self._handle without _lock (pid:563) — good catch. Sandbox.pid/ports were doing a raw FFI read of the Process's handle via _live_handle(), racing a concurrent wait() free. Both now delegate to the Process — Process.pid is lock-guarded + cached, and a new lock-guarded Process.ports() reports {} while a wait() holds the handle (mirroring the Go waiting guard).
• _sdk.py:338 — right, sandlock_handle_kill is dead after Process.kill switched to killpg; removed the binding.