fix(code-runtime-python): reap same-group survivors and fix cross-loop bindings

Two review findings on the CPython backend:
- Disposal could return while a same-group descendant that ignores SIGTERM
  but releases the inherited pipes was still alive: the leader's close fired
  and the previous fix relied on an unref'd SIGKILL timer that a short-lived
  host never fires, reparenting the survivor to init. settle() now withholds
  the run's finished promise on a ref'd process-group poll until the SIGKILL
  has emptied the group (bounded by graceMs + margin, zero-cost when already
  empty), so teardown's "await each child's exit" holds.
- A binding called from a model worker thread via asyncio.run created its
  reply Future on that thread's loop, but _pump_replies completed it directly
  from the main loop; asyncio.Future is not thread-safe across loops, so the
  call hung to the wall clock. Replies now complete via the owning loop's
  call_soon_threadsafe, and a lock serializes the id claim/write/advance.

Tests: the same-group reap case now asserts a heartbeat file stops (robust
whether the killed descendant is reaped or a zombie, so it holds where PID 1
does not wait() orphans); a cross-loop case runs a binding from a worker
thread and asserts the reply round-trips instead of timing out. Agent Note
expanded to all six fixes with rejected alternatives; zh pair re-recorded.
This commit is contained in:
Chinesezjc
2026-08-31 14:21:19 +08:00
committed by Tianyi Cui
parent 46db9e2ad4
commit 6cb70e6e69
6 changed files with 271 additions and 97 deletions
@@ -616,8 +616,25 @@ async def _run(channel: ProtocolChannel) -> None:
)
# 2. Wire the tools proxies and the ack.
pending: dict[int, asyncio.Future[Any]] = {}
#
# Each entry records the reply Future AND the loop it was created on. Model
# code may call a binding from a THREAD it started, spelled
# ``asyncio.run(tools.x(...))`` or its own new loop in that thread, so a
# Future here can belong to a loop other than the one ``_pump_replies`` runs
# on. ``asyncio.Future`` is not thread-safe: completing it from another
# thread does not wake its own loop, so the pump schedules the completion on
# the owning loop via ``call_soon_threadsafe`` (see ``_pump_replies``) rather
# than calling ``set_result`` directly.
pending: dict[int, tuple[asyncio.AbstractEventLoop, asyncio.Future[Any]]] = {}
next_id = 0
# Serializes the id claim + write + counter advance in ``dispatch`` against
# both other binding-calling threads and the pump's ``pop``. ``dispatch`` may
# run concurrently on several loops/threads, and the host answers a ``call``
# only when its id is the exact successor of the last one — so ids must reach
# the wire in the order they are claimed. Holding this lock across the write
# (not just the counter arithmetic) is what keeps two threads' frames from
# interleaving on fd 3 out of id order, which the host would reject.
pending_lock = threading.Lock()
error_classes: dict[str, type] = {}
@@ -646,25 +663,35 @@ async def _run(channel: ProtocolChannel) -> None:
# state it retains to a single number. A frame that never reaches the
# host must therefore not consume an id, so the counter advances only
# once the write has succeeded.
call_id = next_id
fut: asyncio.Future[Any] = asyncio.get_event_loop().create_future()
pending[call_id] = fut
try:
channel.send_sync(
{
"type": "call",
"id": call_id,
"global": global_name,
"name": name,
"args": args,
}
)
except (TypeError, ValueError) as exc:
pending.pop(call_id, None)
raise call_failure(
f"binding arguments must be lossless JSON: {exc}"
) from exc
next_id += 1
#
# The whole claim-write-advance runs under ``pending_lock`` because a
# binding may be called from more than one thread/loop at once (the model
# can start a thread that runs ``asyncio.run(tools.x(...))``). Without
# the lock two callers could claim the same id, or write their frames to
# fd 3 in an order that does not match their ids — either of which the
# host rejects as an out-of-sequence call. The Future's own loop is
# captured here so ``_pump_replies`` can complete it thread-safely.
loop = asyncio.get_event_loop()
with pending_lock:
call_id = next_id
fut: asyncio.Future[Any] = loop.create_future()
pending[call_id] = (loop, fut)
try:
channel.send_sync(
{
"type": "call",
"id": call_id,
"global": global_name,
"name": name,
"args": args,
}
)
except (TypeError, ValueError) as exc:
pending.pop(call_id, None)
raise call_failure(
f"binding arguments must be lossless JSON: {exc}"
) from exc
next_id += 1
try:
return await fut
except _BindingRejection as exc:
@@ -689,7 +716,9 @@ async def _run(channel: ProtocolChannel) -> None:
# 3. Start a reply-pump task before the run message: replies can arrive
# interleaved with the run's own binding traffic.
reply_task = asyncio.get_event_loop().create_task(_pump_replies(channel, pending))
reply_task = asyncio.get_event_loop().create_task(
_pump_replies(channel, pending, pending_lock)
)
# 4. Read the run message.
run = channel.read_frame()
@@ -793,28 +822,51 @@ async def _run(channel: ProtocolChannel) -> None:
async def _pump_replies(
channel: ProtocolChannel, pending: dict[int, asyncio.Future[Any]]
channel: ProtocolChannel,
pending: dict[int, tuple[asyncio.AbstractEventLoop, asyncio.Future[Any]]],
pending_lock: "threading.Lock",
) -> None:
"""Background task: read reply frames and settle pending futures.
Cancelled after ``done`` is posted. Unknown ids and post-settlement replies
are ignored (mirrors the worker backend's hostile-peer stance, though here
the host is the trusted side; the guards defend against races).
A pending Future may belong to a loop other than this pump's — the model can
call a binding from a thread running its own loop (``asyncio.run(tools.x())``).
``asyncio.Future`` is not thread-safe, so the completion is scheduled on the
Future's OWN loop via ``call_soon_threadsafe`` rather than mutated here; a
direct ``set_result`` would never wake the waiting loop and the call would
hang to the wall clock. The ``pop`` shares ``pending_lock`` with ``dispatch``
so a reply cannot race the claim that registers its id.
"""
def complete(fut: asyncio.Future[Any], ok: bool, value: Any, message: Any) -> None:
# Runs on the Future's own loop. `done()` re-checked here because
# cancellation or a duplicate reply may have settled it between the pop
# and this callback.
if fut.done():
return
if ok:
fut.set_result(value)
else:
fut.set_exception(_BindingRejection(str(message)))
while True:
frame = await channel.read_frame_async()
if frame is None:
return
if frame.get("type") != "reply":
continue
fut = pending.pop(frame.get("id"), None)
if fut is None or fut.done():
with pending_lock:
entry = pending.pop(frame.get("id"), None)
if entry is None:
continue
if frame.get("ok"):
fut.set_result(frame.get("value"))
else:
fut.set_exception(_BindingRejection(str(frame.get("message"))))
loop, fut = entry
ok = bool(frame.get("ok"))
value = frame.get("value")
message = frame.get("message")
loop.call_soon_threadsafe(complete, fut, ok, value, message)
_SCALAR_RE = re.compile(
@@ -217,6 +217,17 @@ const FRAME_ENVELOPE_BYTES = 64
*/
const CLOSE_REAP_MARGIN_MS = 2_000
/**
* Interval between process-group liveness probes while settlement waits for an
* escalated SIGKILL to empty the group (see the `killing` branch in
* {@link PythonCodeRuntime.execute}'s settle). A poll rather than an event
* because the group members are the model's own descendants, which the host does
* not `wait()` for and gets no exit signal from; the probe is a signal-0
* `process.kill(-pid, 0)`, so the interval only bounds how promptly a now-empty
* group is noticed, capped by `graceMs + CLOSE_REAP_MARGIN_MS`.
*/
const GROUP_REAP_POLL_MS = 50
/**
* Extract a human message from an unknown thrown value.
*
@@ -961,6 +972,7 @@ export class PythonCodeRuntime extends CodeRuntime {
// Escalate SIGTERM → grace → SIGKILL on the entire process group. Idempotent
// via `killing`.
let killing = false
let graceTimer: NodeJS.Timeout | undefined
// A backstop for the one case `close` cannot cover: model code that starts
// a descendant with `os.setsid()`/`start_new_session=True` moves it into a
// fresh process group, so the SIGTERM/SIGKILL aimed at the child's group
@@ -983,22 +995,29 @@ export class PythonCodeRuntime extends CodeRuntime {
if (killing) return
killing = true
killGroup('SIGTERM')
// The SIGKILL is left to fire on its own timer and is deliberately NOT
// cancelled at settlement. A model program can leave a descendant in the
// SAME process group `kill(-pid)` targets — no setsid, so it stays in the
// group — that ignores SIGTERM but releases the inherited stdout/stderr/
// fd-3 pipes: the leader then exits, its `close` fires (the pipes drained),
// and settle() runs while that descendant is still alive. Cancelling the
// timer there would strand it, breaking "no subprocess outlives the fiber".
// Letting the timer elapse SIGKILLs the whole group, reaching the survivor;
// `killGroup` swallows ESRCH, so firing against an already-dead group (the
// normal case, where the leader was the only member) is harmless. `unref`
// so a pending SIGKILL never keeps the host process alive after run()
// resolves. (A setsid-escaped orphan in a FRESH group is the different case
// `closeDeadline` in finish() covers, since `close` never fires there.)
const graceTimer = setTimeout(() => { killGroup('SIGKILL') }, this.config.graceMs)
// Escalate to SIGKILL after the grace window. The timer is `unref`'d so a
// pending SIGKILL never keeps the host process alive on its own; the
// guarantee that a same-group survivor is actually reaped before the fiber
// goes quiescent is enforced by settle() awaiting the group's death (see
// there), NOT by this timer firing during host lifetime. A setsid-escaped
// orphan in a FRESH group is the different case `closeDeadline` in finish()
// covers, since `close` never fires there.
graceTimer = setTimeout(() => { killGroup('SIGKILL') }, this.config.graceMs)
graceTimer.unref()
}
// True once the group has no members left: a signal-0 probe to the whole
// group (`kill(-pid, 0)`) throws ESRCH when empty (EPERM would still mean a
// member exists). Only meaningful once a spawn produced a pid.
const groupEmpty = (): boolean => {
/* v8 ignore next -- pid is always defined once escalation runs; the guard narrows the type. */
if (child.pid === undefined) return true
try {
process.kill(-child.pid, 0)
return false
} catch (error: unknown) {
return (error as NodeJS.ErrnoException).code === 'ESRCH'
}
}
let finishResolve!: () => void
const finished = new Promise<void>((done) => { finishResolve = done })
@@ -1017,7 +1036,8 @@ export class PythonCodeRuntime extends CodeRuntime {
// same-group descendant that ignored SIGTERM but released the pipes lets
// `close` fire (and settle() run) while it is still alive, so the pending
// SIGKILL must remain armed to reap it (see kill()). The timer is
// `unref`'d, so leaving it pending cannot keep the host process alive.
// `unref`'d; quiescence does not depend on it firing during host lifetime
// — `finished` (below) is withheld until the group is confirmed empty.
if (closeDeadline !== undefined) clearTimeout(closeDeadline)
// Drop from `live` only at settlement (close / pid-less spawn failure),
// NOT at finish(): between finish() and the child's `close` the child
@@ -1042,8 +1062,32 @@ export class PythonCodeRuntime extends CodeRuntime {
// tracked; the directory holds no secret, only a copy of two
// checked-in scripts.
}
finishResolve()
resolve({ ...result, logs })
// `finished` is what teardown awaits to honor "no subprocess outlives the
// fiber". When no escalation ran (normal completion, no kill) or the group
// is already empty, resolve it now. Otherwise a same-group descendant that
// ignored SIGTERM but released the pipes is still alive here (its `close`
// is what got us to settle); withhold `finished` until the grace-window
// SIGKILL has emptied the group. The poll timers are REF'd on purpose: a
// short-lived host (a one-shot headless run, a config subprocess) would
// otherwise exit before the unref'd SIGKILL timer fired, reparenting the
// survivor to init — the leak this await exists to prevent. The wait is
// bounded by the same graceMs + margin the SIGKILL escalation uses, so a
// truly unreapable process (it cannot be, since it is in the group
// `kill(-pid)` reaches) could not hang disposal.
if (!killing || groupEmpty()) {
finishResolve()
return
}
const deadline = Date.now() + this.config.graceMs + CLOSE_REAP_MARGIN_MS
const pollGroup = (): void => {
if (groupEmpty() || Date.now() >= deadline) {
finishResolve()
return
}
setTimeout(pollGroup, GROUP_REAP_POLL_MS)
}
pollGroup()
}
const finish = (result: Omit<CodeRunResult, 'logs'>): void => {
@@ -1,4 +1,4 @@
import { existsSync, readdirSync, realpathSync } from 'node:fs'
import { existsSync, readdirSync, realpathSync, statSync } from 'node:fs'
import { mkdtemp, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { basename, dirname, join } from 'node:path'
@@ -1932,72 +1932,73 @@ describe('PythonCodeRuntime — budgets, termination, disposal', () => {
// it does not hold — here by giving the Popen child DEVNULL streams and
// letting close_fds drop fd 3. The leader then writes `done` and exits, its
// `close` fires because the pipes drained, and settle() runs while that
// descendant is still alive. If settle() cancelled the grace-window SIGKILL
// the descendant would outlive the fiber; leaving the unref'd timer to fire
// SIGKILLs the whole group and reaps it.
// descendant is still alive. settle() then keeps a REF'd poll alive until the
// grace-window SIGKILL has emptied the whole process group, so the host cannot
// exit and reparent the survivor to init: no subprocess outlives the fiber.
//
// The descendant must have SIG_IGN installed BEFORE the host sends SIGTERM,
// or it dies from the default SIGTERM whether the fix is present or not — so
// it writes a readiness marker after trapping and the leader waits for that
// marker before returning. The descendant sleeps 30 s as a safety net so a
// broken fix cannot leak it forever; the assertion window is far shorter, so
// it genuinely tests the SIGKILL reaping rather than the self-timeout.
// marker before returning. While alive it bumps a heartbeat file every 50 ms;
// the test asserts the heartbeat STOPS, which is what "no longer executing"
// means whether the killed descendant is reaped or lingers as a zombie (a
// SIGKILL'd process runs no more code either way). It sleeps 30 s as a safety
// net so a broken fix cannot leak it forever.
const handoff = await mkdtemp(join(tmpdir(), 'dsh-samegroup-'))
const readyMarker = join(handoff, 'ready')
const heartbeat = join(handoff, 'heartbeat')
const { runtime } = await setup({ maxWallMs: 10_000, graceMs: 300 })
let reportedPid!: (pid: number) => void
const childPid = new Promise<number>((resolve) => { reportedPid = resolve })
const result = await runtime.run({
program: [
'import subprocess, sys, os, time',
`marker = ${JSON.stringify(readyMarker)}`,
`heartbeat = ${JSON.stringify(heartbeat)}`,
// Same group (no start_new_session); ignores SIGTERM; holds none of the
// leader's pipes (DEVNULL std streams, close_fds drops fd 3). It writes
// the marker (its argv[1]) only AFTER the trap is installed, so the
// leader cannot return and the host cannot send SIGTERM before the
// descendant ignores it.
'code = "import signal, sys, time; signal.signal(signal.SIGTERM, signal.SIG_IGN); open(sys.argv[1], \'w\').close(); time.sleep(30)"',
'child = subprocess.Popen([sys.executable, "-c", code, marker],',
// the marker (argv[1]) only AFTER the trap is installed so the leader
// cannot return, and the host cannot send SIGTERM, before it is ignored —
// then rewrites the heartbeat (argv[2]) every 50 ms for up to 30 s.
'code = ("import signal, sys, time\\n"',
' "signal.signal(signal.SIGTERM, signal.SIG_IGN)\\n"',
' "open(sys.argv[1], \'w\').close()\\n"',
' "end = time.time() + 30\\n"',
' "while time.time() < end:\\n"',
' " open(sys.argv[2], \'w\').close()\\n"',
' " time.sleep(0.05)\\n")',
'child = subprocess.Popen([sys.executable, "-c", code, marker, heartbeat],',
' stdin=subprocess.DEVNULL,',
' stdout=subprocess.DEVNULL,',
' stderr=subprocess.DEVNULL)',
'deadline = time.time() + 5',
'while not os.path.exists(marker) and time.time() < deadline:',
' time.sleep(0.02)',
'await tools.report({"pid": child.pid})',
'return "spawned"',
].join('\n'),
bindings: tools({
report: async (args) => {
reportedPid((args as { pid: number }).pid)
return 'ok'
},
}),
bindings: [],
})
expect(result.error).toBeUndefined()
expect(result.value).toBe('spawned')
const pid = await childPid
expect(Number.isInteger(pid) && pid > 0).toBe(true)
// The trap really installed before the leader returned, so this is the
// SIGTERM-ignoring descendant, not one that would have died to the default.
expect(existsSync(readyMarker)).toBe(true)
// run() resolved inside the grace window, so the descendant is still alive
// here; the pending SIGKILL reaps it shortly after graceMs. Poll until it is
// gone, well within the descendant's own 30 s self-timeout.
const deadline = Date.now() + 5_000
const alive = (): boolean => {
try {
process.kill(pid, 0)
return true
} catch {
return false
}
// The grace-window SIGKILL (graceMs 300 + reap margin) empties the group. Once
// it has, the descendant stops bumping the heartbeat. Poll the heartbeat's
// mtime: two consecutive reads far enough apart with no change means it is no
// longer executing — true whether it was reaped or lingers as a zombie, so
// the assertion holds in a container whose init does not wait() orphans. The
// window (well under the 30 s self-timeout) proves the SIGKILL did the work.
const mtime = (): number => { try { return statSync(heartbeat).mtimeMs } catch { return 0 } }
const stopDeadline = Date.now() + 8_000
let last = mtime()
let still = false
while (Date.now() < stopDeadline) {
await new Promise(resolve => setTimeout(resolve, 400))
const now = mtime()
if (now === last && now !== 0) { still = true; break }
last = now
}
while (alive() && Date.now() < deadline) {
await new Promise(resolve => setTimeout(resolve, 50))
}
expect(() => process.kill(pid, 0)).toThrow(/ESRCH/)
}, 15_000)
expect(still).toBe(true)
}, 20_000)
})
describe('PythonCodeRuntime — hostile peer', () => {
@@ -2305,6 +2306,47 @@ describe('PythonCodeRuntime — hostile peer', () => {
}
}, 30_000)
it('completes a binding called from a worker thread on its own event loop', async () => {
// A binding reply Future is created on the loop that ran `dispatch`. When the
// model calls a binding from a worker THREAD via `asyncio.run(tools.x(...))`,
// that Future belongs to the thread's loop, not the main loop where
// `_pump_replies` reads the reply. `asyncio.Future` is not thread-safe:
// completing it from another thread does not wake its own loop, so a direct
// `set_result` would strand the awaiting thread and the run would degrade to a
// wall-clock timeout. The pump must schedule completion on the Future's own
// loop via `call_soon_threadsafe`. The tight maxWallMs makes the pre-fix
// failure a fast timeout rather than a hang.
//
// The main coroutine yields with `await asyncio.sleep` while the worker runs,
// rather than a synchronous `t.join()`: joining would block the main thread,
// so the main loop could not run `_pump_replies` and the call would deadlock
// regardless of the fix — that blocks the pump, not the cross-loop delivery
// this test pins.
const { runtime } = await setup({ maxWallMs: 8_000 })
const seen: unknown[] = []
const result = await runtime.run({
program: [
'import asyncio, threading',
'result = {}',
'def worker():',
// A fresh loop in this thread; the binding Future is created here.
' result["value"] = asyncio.run(tools.echo({"from": "thread"}))',
't = threading.Thread(target=worker)',
't.start()',
'while t.is_alive():',
' await asyncio.sleep(0.02)',
'return result["value"]',
].join('\n'),
bindings: tools({
echo: async (args) => { seen.push(args); return args as CodeJsonValue },
}),
})
expect(result.error).toBeUndefined()
expect(result.value).toEqual({ from: 'thread' })
// The host binding actually ran (the reply round-tripped), not a timeout.
expect(seen).toEqual([{ from: 'thread' }])
}, 15_000)
it('round-trips an exactly representable large integer through a binding echo', async () => {
// The reply serializer must print BigInt digits for a beyond-safe
// integral double: String(2**60) emits a rounded form, and the child