diff --git a/packages/code-runtime/code-runtime-python/py/bootstrap.py b/packages/code-runtime/code-runtime-python/py/bootstrap.py index e685b74816..2a19a39c54 100644 --- a/packages/code-runtime/code-runtime-python/py/bootstrap.py +++ b/packages/code-runtime/code-runtime-python/py/bootstrap.py @@ -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) diff --git a/packages/code-runtime/code-runtime-python/src/index.ts b/packages/code-runtime/code-runtime-python/src/index.ts index 723fda695b..a1dfae947c 100644 --- a/packages/code-runtime/code-runtime-python/src/index.ts +++ b/packages/code-runtime/code-runtime-python/src/index.ts @@ -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//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. } diff --git a/packages/code-runtime/code-runtime-python/tests/runtime.spec.ts b/packages/code-runtime/code-runtime-python/tests/runtime.spec.ts index d6335fbdb8..febd598512 100644 --- a/packages/code-runtime/code-runtime-python/tests/runtime.spec.ts +++ b/packages/code-runtime/code-runtime-python/tests/runtime.spec.ts @@ -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