fix(P02): destroy kills the inner namespace init, not just the unshare shim
Verifier-found P0: UnshareBackend.destroy reaped agent → inner → helper as
asyncio subprocesses, but the inner entry is the `unshare --fork` PARENT —
its forked child (the `sleep` that is PID 1 of the sandbox pid/mnt/net ns)
reparents to host init and survives, holding the tmpfs + workspace bind for
the sleep duration (3600s). Every tracked-sandbox destroy leaked one
namespace process: ~30 orphaned `sleep 3600` observed after one suite run.
`--kill-child` does not reach the child under this flag combo (verified
empirically: the child still survives parent SIGTERM).
Fix: SIGKILL the ns-init's host pid (already tracked as `inner_pid` for
nsenter) in destroy(), after reaping the agent so it cannot flush into a
dead sandbox. Regression test creates a REAL task sandbox, asserts agent +
ns-init alive, destroys, and asserts both host pids are gone — fails on the
old code, passes with the fix. Full suite 218 green; ruff clean.
---ci---
phase: 2
milestone: v0.3
status: verify
requirements: {covered: [REQ-3-003], partial: []}
---/ci---
This commit is contained in:
@@ -59,6 +59,7 @@ import asyncio
|
||||
import os
|
||||
import shlex
|
||||
import shutil
|
||||
import signal
|
||||
import time
|
||||
from datetime import UTC, datetime
|
||||
from pathlib import Path
|
||||
@@ -488,11 +489,35 @@ class UnshareBackend: # satisfies SandboxBackend structurally (Protocol)
|
||||
"""
|
||||
tracked = self._tracked.pop(handle.id, None)
|
||||
if tracked is not None:
|
||||
# Agent first (it must not flush a "stopped" event into a dead
|
||||
# sandbox), then the namespace tree. The inner `unshare --fork`
|
||||
# shim is NOT the namespace init: killing it orphans its child
|
||||
# (the `sleep` that is PID 1 of the sandbox pid+mnt+net ns),
|
||||
# which reparents to host init and holds the tmpfs + bind for
|
||||
# a full hour (observed: ~30 leaked `sleep 3600` after a test
|
||||
# run). `--kill-child` does not reach it either (util-linux
|
||||
# 2.38 leaks the same child under this flag combo — the child
|
||||
# is reparented before unshare's signal handler runs). The
|
||||
# deterministic kill is SIGKILL on the ns-init's HOST pid,
|
||||
# which we already track as `tracked.inner_pid` (nsenter uses
|
||||
# it for exec); the kernel then tears down the namespace with
|
||||
# its init (no processes remain).
|
||||
for proc in (tracked.agent, tracked.inner, tracked.helper):
|
||||
if proc is not None:
|
||||
await self._reap(proc)
|
||||
self._kill_pid(tracked.inner_pid)
|
||||
handle.pid = None
|
||||
|
||||
@staticmethod
|
||||
def _kill_pid(pid: int | None, sig: int = signal.SIGKILL) -> None:
|
||||
"""Best-effort host-side signal; pid recycled or gone is not an error."""
|
||||
if pid is None:
|
||||
return
|
||||
try:
|
||||
os.kill(pid, sig)
|
||||
except (ProcessLookupError, PermissionError):
|
||||
pass # already dead, or not ours — nothing to do
|
||||
|
||||
@staticmethod
|
||||
async def _reap(proc: asyncio.subprocess.Process) -> None:
|
||||
"""SIGTERM then SIGKILL, tolerant of an already-dead process."""
|
||||
|
||||
@@ -135,6 +135,57 @@ async def test_no_task_id_means_no_capture(tmp_path: Path) -> None:
|
||||
store.close()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_destroy_kills_inner_namespace_not_just_the_shim() -> None:
|
||||
"""Destroy must reap the ns-init, not only the `unshare --fork` shim.
|
||||
|
||||
Regression: `_reap(inner)` kills the unshare PARENT, but its forked child
|
||||
(PID 1 of the sandbox pid/mnt/net namespace, the `sleep`) reparents to
|
||||
host init and holds the tmpfs + workspace bind for the full sleep
|
||||
duration — a leaked sandbox per destroy (observed ~30 orphaned
|
||||
`sleep 3600` processes after one suite run; `--kill-child` did not reach
|
||||
it under this flag combo). The ns-init's host pid is `tracked.inner_pid`.
|
||||
"""
|
||||
_userns_probe()
|
||||
|
||||
backend = UnshareBackend()
|
||||
manager = SandboxManager(backend=backend, settings=Settings())
|
||||
|
||||
handle = await manager.create("lifecycle-learner", task_id="lifecycle-task")
|
||||
try:
|
||||
tracked = backend._tracked[handle.id] # noqa: SLF001
|
||||
assert tracked.agent is not None
|
||||
assert _proc_alive(tracked.agent.pid), "agent should live with the sandbox"
|
||||
assert _proc_alive(tracked.inner_pid), "ns-init should live with the sandbox"
|
||||
finally:
|
||||
await manager.destroy(handle.id)
|
||||
|
||||
# Reaping the unshare shim is asynchronous up to _reap's timeouts; poll
|
||||
# until both the agent AND the ns-init host pid are gone.
|
||||
deadline = time.monotonic() + 20.0
|
||||
while time.monotonic() < deadline and (
|
||||
_proc_alive(tracked.agent.pid) or _proc_alive(tracked.inner_pid)
|
||||
):
|
||||
await asyncio.sleep(0.25)
|
||||
assert not _proc_alive(tracked.agent.pid), "agent pid survived destroy (lifecycle)"
|
||||
assert not _proc_alive(tracked.inner_pid), (
|
||||
"inner namespace init survived destroy — leaked sandbox (tmpfs + bind "
|
||||
"held for a full hour; every destroy leaked one namespace process)"
|
||||
)
|
||||
|
||||
|
||||
def _proc_alive(pid: int | None) -> bool:
|
||||
if pid is None:
|
||||
return False
|
||||
try:
|
||||
with open(f"/proc/{pid}/stat") as fh:
|
||||
# state is the first field after comm: Z (zombie) counts as dead
|
||||
# for our purpose — it holds no namespace and is reaped next.
|
||||
return fh.read().rsplit(") ", 1)[1].split()[0] != "Z"
|
||||
except OSError:
|
||||
return False
|
||||
|
||||
|
||||
async def _await_events(
|
||||
store: SQLiteTraceStore, learner: str, task: str, *, minimum: int, timeout_s: float = 20.0
|
||||
) -> list:
|
||||
|
||||
Reference in New Issue
Block a user