fix(code-runtime-python): guard teardown, log prefix, and settlement flush

Four independent corrections in the run lifecycle.

`killGroup` signalled `-child.pid` with a raw `process.kill`. Node keeps the
numeric `child.pid` after the leader is reaped and only clears its internal
handle, so `child.kill()` refuses while the raw call does not; `close` can
trail `exit` by seconds when a pipe-holding descendant keeps the streams open.
A recycled pgid could therefore receive this run's SIGTERM and armed SIGKILL.
`groupEmpty()` does not cover it: it reports whether the group has members, not
whether they are ours, and it first runs after the signal. The leader's start
time is now read at spawn and re-checked before each signal, matching the
position packages/subprocess/subprocess-local already states
("ProcessIdentity ... preventing teardown escalation after PID reuse"). Kept
local rather than depending on that package, which would add an architectural
edge. Linux reads /proc; Darwin has no /proc, so the reader reports undefined
and the guard degrades to the previous behavior instead of forking `ps` on a
teardown path.

`_push_bounded_prefix` built `(*self._pending, extra)`, copying every pending
reference into a same-size tuple before the bounded loop. For a
single-character drip that is a second pointer array as large as the list:
measured +80 MiB of tuple over a 40 MiB list for 5.2M chunks, the allocation
the bounded prefix exists to avoid. It now iterates the list in place and
handles `extra` in the loop's `else`; 4000 randomized inputs produce byte-identical
prefixes.

The settlement `flush_out()`/`flush_err()` ran outside any guard while `done`
was already decided, so a flush raising under memory pressure skipped
`send_done` and downgraded a child-classified `exception` into a host-side
`worker-exit`. Both are now wrapped, swallowing only the log tail.

The boot re-check's `if effective_soft != RLIM_INFINITY` was dead: `_clamped`
is asked for a finite `addr_bytes` on both sides and each branch returns that
value or a `min` with an inherited bound, so RLIM_INFINITY is unreachable. The
guard could only ever have skipped the re-check it claimed to protect.
This commit is contained in:
Chinesezjc
2026-08-31 14:26:23 +08:00
committed by Tianyi Cui
parent 8f7d9121d1
commit e6b547bef4
3 changed files with 122 additions and 17 deletions
@@ -319,14 +319,25 @@ class _LogStream(io.TextIOBase):
# prefix that still fails LogBuffer's ``len(text) + 3 > remaining``
# check; the accumulation stops there, so the copy is bounded by the log
# budget however large the pending chunks are.
#
# `self._pending` is iterated IN PLACE and `extra` handled after it:
# `(*self._pending, extra)` would first copy every pending reference into
# a same-size tuple, which for a single-character drip (millions of tiny
# chunks) is a second pointer array as large as the list itself --
# measured at +80 MiB of tuple on top of a 40 MiB list for 5.2M chunks,
# the allocation this bounded prefix exists to avoid.
limit = self._logs.remaining + 4
parts: list[str] = []
total = 0
for chunk in (*self._pending, extra):
for chunk in self._pending:
parts.append(chunk[: limit - total])
total += len(parts[-1])
if total >= limit:
break
else:
# Only reached when the pending chunks did not fill the prefix, so
# `extra` is the one remaining source of text.
parts.append(extra[: limit - total])
self._pending = []
self._pending_chars = 0
self._logs.push("".join(parts))
@@ -669,17 +680,22 @@ async def _run(channel: ProtocolChannel) -> None:
# boot rather than letting a near-budget output OOM mid-run. The
# constants match src/index.ts's OUTPUT_BUDGET_WORST_CASE_ADDRESS_
# SPACE_MULTIPLE and INTERPRETER_BASELINE_BYTES.
# `effective_soft` is always finite, so the re-check is
# unconditional: `_clamped` was asked for the finite `addr_bytes` on
# both sides, and each of its branches returns either that value or a
# `min` with an inherited bound -- RLIM_INFINITY is not reachable. A
# guard here would have silently skipped the whole re-check on the
# branch it claimed to protect.
effective_soft = effective_as[0]
if effective_soft != resource.RLIM_INFINITY:
budgetable = effective_soft - _INTERPRETER_BASELINE_BYTES
for _budget_key in ("maxLogBytes", "maxValueBytes"):
if int(boot[_budget_key]) * _OUTPUT_BUDGET_WORST_CASE_MULTIPLE >= budgetable:
raise ValueError(
"config.%s is too large for the inherited RLIMIT_AS of %d bytes "
"(a near-budget output would breach it during encode); "
"lower the budget or raise the inherited address-space limit"
% (_budget_key, effective_soft)
)
budgetable = effective_soft - _INTERPRETER_BASELINE_BYTES
for _budget_key in ("maxLogBytes", "maxValueBytes"):
if int(boot[_budget_key]) * _OUTPUT_BUDGET_WORST_CASE_MULTIPLE >= budgetable:
raise ValueError(
"config.%s is too large for the inherited RLIMIT_AS of %d bytes "
"(a near-budget output would breach it during encode); "
"lower the budget or raise the inherited address-space limit"
% (_budget_key, effective_soft)
)
except BaseException as exc: # noqa: BLE001 -- report every failure to host
channel.send_sync(
{
@@ -924,8 +940,21 @@ async def _run(channel: ProtocolChannel) -> None:
# final partial line is not silently dropped. The success path already
# flushed before framing the value; this is an idempotent no-op there and
# the flush the exception path needs.
flush_out()
flush_err()
#
# Guarded because `done` is ALREADY DECIDED here: on the exception path the
# handler above built it, and a flush that raises (its join/encode under
# memory pressure, after the program left a near-maxLogBytes pending and then
# allocated toward RLIMIT_AS) would skip `send_done` and downgrade a run the
# child already classified as `exception` into a host-side `worker-exit`.
# Losing the log tail is the lesser outcome, and the marker the ledger
# already pushed still reports the truncation. Same rule as
# `_make_failure_reporter`: a settled verdict must not be swallowed by the
# reporting that follows it.
for _flush in (flush_out, flush_err):
try:
_flush()
except BaseException: # noqa: BLE001 -- swallow ONLY the log tail; `done` must reach the host
pass
reply_task.cancel()
send_done(done)
@@ -13,7 +13,7 @@
*/
import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'
import { accessSync, copyFileSync, constants as fsConstants, mkdtempSync, rmSync } from 'node:fs'
import { accessSync, copyFileSync, constants as fsConstants, mkdtempSync, readFileSync, rmSync } from 'node:fs'
import { tmpdir } from 'node:os'
import { delimiter, dirname, isAbsolute, join } from 'node:path'
import { fileURLToPath } from 'node:url'
@@ -313,6 +313,36 @@ const GROUP_REAP_POLL_MS = 50
* @returns The value's message or string form; a fixed placeholder when its own
* conversion throws.
*/
/**
* A process's start time, as the identity half of (pid, started).
*
* A pid is reusable the moment the kernel reaps it, so signalling one that a
* later process inherited would terminate an unrelated process group. Start
* time is what distinguishes the original from its replacement: `kill(pid, 0)`
* answers "does this number exist", which is true for both.
*
* Linux reads field 22 of `/proc/<pid>/stat` (starttime in clock ticks); the
* field is positional after the comm field's closing parenthesis, which is
* parsed from the LAST such character because a process name may contain one.
* Darwin has no `/proc`, so the caller gets `undefined` there and the guard
* degrades to the pre-existing behavior rather than paying a `ps` fork on a
* teardown path. Any read failure is `undefined` for the same reason: this
* hardens a narrow race and must never be the thing that breaks teardown.
* @param pid - the process to read.
* @returns its start time, or undefined when unavailable.
*/
export function readProcessStart(pid: number): string | undefined {
if (process.platform !== 'linux') return undefined
try {
const stat = readFileSync(`/proc/${String(pid)}/stat`, 'utf8')
const fields = stat.slice(stat.lastIndexOf(')') + 2).split(' ')
// Field 22 overall; the slice above dropped pid and comm, so it is index 19.
return fields[19]
} catch {
return undefined
}
}
function messageOf(error: unknown): string {
try {
return String(error instanceof Error ? error.message : error)
@@ -1355,10 +1385,30 @@ export class PythonCodeRuntime extends CodeRuntime {
// finish() arms this deadline; when it fires we detach our stream handles
// and settle on the already-decided result regardless of the orphan.
let closeDeadline: NodeJS.Timeout | undefined
// The leader's start time, read once while it is certainly alive. `child.pid`
// keeps its numeric value after the leader is reaped (Node clears the
// internal handle, not the field), and `close` can trail `exit` by seconds
// while a pipe-holding descendant keeps the streams open. Signalling
// `-child.pid` in that window is a RAW syscall -- `child.kill()` would
// refuse, having dropped its handle, but `process.kill` has no such guard --
// so a recycled pgid would receive this run's SIGTERM and armed SIGKILL.
// `groupEmpty()` cannot cover it: it reports whether the group has members,
// not whether they are OURS, and it runs only after the first signal.
// The repository already takes this position in
// packages/subprocess/subprocess-local (`ProcessIdentity`, "preventing
// teardown escalation after PID reuse"); this is the same guard, kept local
// because a dependency on that package would be a new architectural edge.
const leaderStarted = child.pid === undefined ? undefined : readProcessStart(child.pid)
const killGroup = (sig: NodeJS.Signals): void => {
try {
/* v8 ignore next -- undefined pid means spawn never produced a process; finish() short-circuits before reaching kill(). */
if (child.pid !== undefined) process.kill(-child.pid, sig)
if (child.pid === undefined) return
// A pid alone cannot answer this: `process.kill(pid, 0)` succeeds just
// as well for a REPLACEMENT process holding the recycled number. Only
// the start time distinguishes the two, so a reading that no longer
// matches means the group is not this run's and must not be signalled.
if (leaderStarted !== undefined && readProcessStart(child.pid) !== leaderStarted) return
process.kill(-child.pid, sig)
} catch {
// ESRCH — the process already died. Nothing to do.
}
@@ -3,8 +3,8 @@ import { mkdtemp, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { basename, dirname, join } from 'node:path'
import { describe, expect, it, vi } from 'vitest'
import { Context } from 'cordis'
import { PythonCodeRuntime } from '../src/index.ts'
import { Context } from '@deepseek-ai/cordis'
import { PythonCodeRuntime, readProcessStart } from '../src/index.ts'
import { logTruncationMarker } from '../src/protocol.ts'
import type { Config } from '../src/index.ts'
import type { CodeBindingFunction, CodeJsonValue, CodeRunResult } from '@deepseek-ai/dsh-code-runtime'
@@ -383,6 +383,32 @@ describe('PythonCodeRuntime — seam descriptors and misuse', () => {
}, 15_000)
})
describe('PythonCodeRuntime — process identity', () => {
it('reads a live process start time and distinguishes it from an absent pid', () => {
// The teardown guard signals `-child.pid` with a RAW `process.kill`, which
// (unlike `child.kill()`) has no handle check, so it would reach a recycled
// pgid during the window between the leader being reaped and `close` firing.
// A pid alone cannot separate the original from its replacement -- both
// answer `kill(pid, 0)` -- so the guard compares START TIME, and this pins
// that the reading is stable for one process and absent for a pid that
// cannot be read.
const own = readProcessStart(process.pid)
if (process.platform === 'linux') {
// Same process, two reads: the identity must be stable, or the guard would
// refuse to signal its own live group.
expect(own).toBeDefined()
expect(readProcessStart(process.pid)).toBe(own)
// Pid 0 is never a readable /proc entry, so the guard degrades to
// undefined rather than throwing on a teardown path.
expect(readProcessStart(0)).toBeUndefined()
} else {
// Darwin has no /proc: the reader reports undefined, and `killGroup` then
// keeps its pre-existing behavior instead of paying a `ps` fork per signal.
expect(own).toBeUndefined()
}
})
})
describe('PythonCodeRuntime — inherited resource limits', () => {
it('runs under an inherited hard limit tighter than addressSpaceMb', async () => {
// An unprivileged process may lower a hard rlimit but never raise it. Under