deepseek-harness/.agents/notes/implemented/bug-fix/2026-07-31-code-runtime-python-settlement-fixes.md

92 lines
24 KiB
Markdown
Raw Normal View History

# Agent Note: Settlement, framing, and lifecycle fixes in the CPython backend
Status: implemented
English | [中文](2026-07-31-code-runtime-python-settlement-fixes.zh.md)
## Problem
The CPython subprocess backend for Code Mode, built on the [fd-3 frame protocol](../architecture/2026-07-31-code-runtime-python-fd3-protocol.md), resolves every program outcome as a `CodeRunResult`, rejects `run()` only for seam misuse, and disposes to quiescence so no subprocess that stays in the child's process group outlives the fiber (a descendant that escapes the group with `setsid()` is the documented exception — see the package README's Known Limitations). A sequence of review passes surfaced defects that broke those contracts in ways unit coverage did not catch — each hid behind a `/* v8 ignore */`, a captured-callable that read as a fix but was not, a memory effect invisible through the seam, a load-time bound that double-counted, a process-group escalation that a survivor could outlast, a cross-event-loop completion that silently deadlocked, a synchronous throw outside the settlement path, or a transport boundary rendered as a log boundary. Most behavioral fixes ship with a test that fails without them; two do not, and are called out as such — the chunked frame read (a syscall-count improvement with no cross-platform-deterministic failure) and the confirmed-empty finalize (its only seam-observable effect, a frozen heartbeat, freezes the instant SIGKILL is delivered, which the pre-fix finalize-on-delivery code also produced, and the discriminating probe is the signal-0 check the Alternatives reject as cross-environment-unreliable).
## Decision
Independent corrections, each in the package that owns the defect.
### Boot-write failure no longer rejects run()
In [`src/index.ts`](../../../../packages/code-runtime/code-runtime-python/src/index.ts) the fd-3 boot-frame write is the last statement of `run()`'s synchronous setup. Its `catch` calls `finish()`, and `finish()` reads `wallTimer` and `onAbort` and — through `settle()` — `live`. Those bindings are `const` and were declared AFTER the boot-write, so on a synchronous write failure `finish()` touched them in their temporal dead zone and threw a `ReferenceError`. That escaped the Promise executor and REJECTED `run()`, violating the seam's "outcomes resolve" contract: the caller saw a thrown error instead of the `worker-exit` the catch constructs. The boot-write block is now emitted after `wallTimer`, `onAbort`, and `live` are initialized, and the `/* v8 ignore */` that had hidden the branch from coverage is removed so the catch is measured.
### Log capture is serialized against settlement
In [`py/bootstrap.py`](../../../../packages/code-runtime/code-runtime-python/py/bootstrap.py) the settlement `flush_out()`/`flush_err()` on the main coroutine read and clear each stream's `_pending` list and mutate the shared `LogBuffer` ledger. Model code may start daemon threads whose `print`/`write` mutate the same state concurrently. Capturing the bound method (`out_stream.flush_line`) fixed only WHICH callable settlement invokes, not what it reads mid-flight: an interleaved flush could join a `_pending` list being mutated under it, corrupting the ledger and costing the `done` frame — stranding the run to the wall clock. `LogBuffer` now owns one re-entrant lock shared by both streams; `_LogStream.write` and `flush_line`, and `LogBuffer.push`, take it, so the whole read-modify-write is atomic across threads.
### Fd-3 residual is copied, not viewed
Also in `src/index.ts`, after the newline loop over a `Buffer.concat` of the pending fd-3 chunks, the leftover partial line was carried forward as the `subarray` VIEW it was sliced to. A view keeps the entire concat backing allocation alive, so a large frame followed by a tiny trailing fragment pinned a whole frame's worth of memory while `pendingBytes` — set to the fragment's length — reported far less than was retained. The residual is now detached into a fresh right-sized `Buffer` via the exported `detachResidual` helper, letting the concat allocation be collected and keeping `pendingBytes` an honest measure.
### Output-cap load bound is ceiling minus envelope, not divided by six
The load-time check that rejects a `maxLogBytes`/`maxValueBytes` larger than one fd-3 frame can carry divided the frame ceiling by six for worst-case escape expansion. But both budgets are metered in ALREADY-ESCAPED serialized bytes — the host log ledger charges `Buffer.byteLength(JSON.stringify(text))`, `checkDoneValue` measures the escaped form, and the producing-side `_cap_message` also caps by serialized cost — so a payload admitted under the cap occupies at most `cap + envelope` on the wire; escaping is inside the charge and must not be multiplied in again. The bound is now `FRAME_CEILING_BYTES - FRAME_ENVELOPE_BYTES`, and the unused `MAX_JSON_ESCAPE_EXPANSION` constant is gone. The old bound was not unsafe — it under-admitted — but it silently forbade legitimate large caps. The same load check also rejects a NON-INTEGER `maxLogBytes`/`maxValueBytes`: the child reads each budget through `int(...)`, which floors a float, so `maxLogBytes: 3.5` would truncate at 3 bytes child-side while the host meters the fraction — the two sides enforcing different public config. Rejecting the float at load keeps them in step, matching the worker backend.
### Same-group survivors are reaped before the fiber goes quiescent
A model program can leave a descendant in the child's OWN process group (no `setsid`, so `kill(-pid)` reaches it) that ignores SIGTERM but releases the inherited stdout/stderr/fd-3 pipes. The leader then exits, its `close` fires because the pipes drained, and settlement runs while that descendant is still alive. `kill()` arms an `unref`'d SIGKILL timer after SIGTERM; the fix is that `settle()` no longer resolves the run's `finished` promise — nor drops the run from `live` — immediately when an escalation is in flight. Instead, when `killing` is set and the process group is not yet empty (`process.kill(-pid, 0)` does not throw ESRCH), it polls the group on a REF'd timer, bounded by `graceMs + CLOSE_REAP_MARGIN_MS`, and both drops the run from `live` and resolves `finished` only once the group has emptied. The ref'd poll is the load-bearing part: it keeps the host event loop alive until the SIGKILL has actually reaped the group, so even a short-lived host — a one-shot headless run, a config subprocess — cannot exit and reparent the survivor to init. Deferring the `live` removal is what makes a `dispose()` racing a just-resolved `run()` still await the survivor: dropping the run from `live` at settlement (before the reap) would let teardown snapshot an empty set and return while the descendant lived. In the normal case (the leader was the only member) the first probe returns ESRCH and settlement finalizes with zero added latency. `teardown()` awaits each run's `finished`, so disposal is genuinely quiescent, matching its JSDoc — including for a run that already resolved.
Settlement also CANCELS the SIGKILL timer the moment the group is confirmed empty (the normal path, and when the poll sees the survivor gone). Leaving it armed would expose a PID-reuse hazard: a `kill(-pid)` left pending for up to `graceMs` after the leader was reaped could hit a RECYCLED pgid once the kernel reused the leader's pid, SIGKILLing an unrelated group (`killGroup` swallowing ESRCH does not help — the danger is precisely the kill that SUCCEEDS against a reused group). Clearing it on the empty probe bounds the reuse window to only the genuine-survivor case, where the group cannot be empty to reuse.
The reap poll also handles a host event loop BLOCKED past both timers. If a synchronous computation holds the loop from before the poll was scheduled until after its deadline, both the poll timer and the grace-window SIGKILL timer are overdue when the loop resumes, and Node runs the earlier-scheduled poll first — so the grace SIGKILL may never have fired. The deadline branch therefore sends SIGKILL ITSELF (idempotent if the timer already ran) rather than cancelling the unfired escalation, then grants ONE more `CLOSE_REAP_MARGIN_MS` and keeps polling until the group is confirmed empty, because finalizing on mere signal delivery would declare quiescence while the group is still dying. The outer bound on the wait is therefore `graceMs + 2 * CLOSE_REAP_MARGIN_MS`. A final hard bound finalizes if that extra margin elapses with the group still non-empty; that branch carries a `/* v8 ignore */` because it is reachable only where a SIGKILL'd survivor lingers as a zombie and is never `wait()`'d — a container whose PID 1 does not reap orphans — which cannot be built deterministically across CI platforms. The ignore's reason states that environment dependence rather than claiming the branch cannot run, cross-referencing the Alternatives entry that rejected the signal-0 reap assertion for the same reason.
### RLIMIT clamps against the inherited soft limit, not only the hard
In [`py/bootstrap.py`](../../../../packages/code-runtime/code-runtime-python/py/bootstrap.py) `_clamped` bounded a requested `(soft, hard)` rlimit pair by the inherited HARD limit alone. A deployment that inherited a soft limit below the requested one — say inherited `(100, 200)`, requested `(150, 160)` — got back `(150, 160)`, RAISING the effective soft from 100 to 150: for `RLIMIT_AS` that loosens the memory ceiling, for `RLIMIT_CPU` it defers SIGXCPU, both violating "strictest of configured and inherited". `_clamped` now clamps each side against its own inherited counterpart (`RLIM_INFINITY` imposing no ceiling), then pins soft under hard so `setrlimit` never sees an inverted pair. The settlement-time CPU recheck (`die_if_cpu_exhausted`) follows the same rule: it compares spent CPU against the EFFECTIVE clamped `cpu_soft`, not the configured `cpuSeconds`, so a program that traps SIGXCPU and burns past a stricter inherited soft before returning is reported as a timeout rather than a false success. The SIGXCPU diagnostic no longer names the configured `cpuSeconds` as the effective budget — under a stricter inherited soft that number is wrong — and instead reports that CPU time was exhausted at "at most the configured N seconds", which holds whichever limit fired.
### Binding replies complete on the calling loop's thread
Also in `py/bootstrap.py`, 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 the direct `set_result`/`set_exception` left the awaiting thread stranded and the run degraded to a wall-clock timeout. Each pending entry now records its Future's loop alongside the Future, and `_pump_replies` completes it via that loop's `call_soon_threadsafe`. The shared `pending`/`next_id` state is guarded by a `threading.Lock` held across the id claim, the fd-3 write, and the counter advance, so concurrent callers cannot interleave frames out of the id order the host requires. `call_soon_threadsafe` onto a loop that has already CLOSED (the worker thread finished and abandoned its call before the reply arrived) raises `RuntimeError`; that schedule is wrapped so the moot reply is dropped rather than letting the exception end the pump task and strand every later reply.
### The blocking frame reader reads in chunks, not byte by byte
`ProtocolChannel.read_frame` — used for the `boot` and `run` handshake frames — read through `FileIO.readline()` on the unbuffered (`buffering=0`) fd, which issues one `os.read(1)` per byte. The `run` frame arrives AFTER `RLIMIT_CPU` is in force, so a legitimate multi-megabyte program burned seconds of CPU in millions of single-byte syscalls before `ast.parse` ran — potentially exhausting the budget on the read alone. It now reads in `_READ_CHUNK_BYTES` chunks into the same `_pending` residual buffer the async reader already uses (the wrapping `os.fdopen` object is gone; both readers call `os.read(self._fd, ...)` directly), so the read cost is trivial and read-ahead past a newline is preserved for the next frame. Both readers track a running scan offset (`find(b"\n", scanned)`) so a large frame accumulated across many chunks is scanned once, not re-scanned from index 0 per chunk — a chunked rescan would have replaced the byte-at-a-time cost with an O(N²) memchr cost on the same large-frame path.
### Synchronous spawn failure resolves worker-exit, not reject
Also in `src/index.ts`, `spawn` is called before the settlement Promise executor exists. Node defers only a fixed set of spawn errnos (EACCES, EAGAIN, EMFILE, ENFILE, ENOENT) to an asynchronous `error` event, which the settlement path already turns into a `worker-exit`; every other errno throws SYNCHRONOUSLY from `spawn`. A `pythonBin` longer than the platform PATH_MAX passes the load-time validation (non-empty, no NUL) but makes `spawn` throw `ENAMETOOLONG` here — outside the executor — so `run()` REJECTED instead of resolving, violating resolve-don't-reject, and left this run's just-materialized staging directory on disk since only `settle()` removes it. The `spawn` call and the fd-3 narrowing are now wrapped: a synchronous throw removes the staging directory and resolves the same `worker-exit` class (`python spawn error: …`) the async `error` event produces.
### Stray pipe output is aggregated by line, not by transport chunk
Also in `src/index.ts`, native stdout/stderr bytes (C-extension writes, `os.write` past the pipe buffer) were pushed to `logs` one entry per Node `data` chunk. `logs` entries are joined with `\n` downstream (Code Mode), so a single newline-free write larger than one pipe read — arriving as several `data` chunks — read back with model-visible newlines inserted at arbitrary transport boundaries. Capture now holds a per-stream residual, admits an entry only on a real `\n`, and flushes the trailing partial once on the pipe's `end`, matching the child's own line-granular `log` frames. The residual stays bounded by the ledger: when it would cross the budget with no newline in sight it is admitted (and truncated) immediately, and once the ledger has truncated, buffering stops so a newline-free flood cannot retain host memory for output that can never be admitted.
## Testing
- `tests/boot-write-failure.spec.ts` mocks `spawn` so the fd-3 pipe throws on the boot write — the one path a real subprocess cannot be coerced into — and asserts `run()` resolves a `worker-exit` rather than rejecting. A sibling case makes the mocked `spawn` throw SYNCHRONOUSLY and asserts `run()` still resolves a `worker-exit` and leaves no `dsh-code-runtime-python-*` directory behind in `tmpdir` (before/after diff). Both are isolated in this spec so the real-subprocess suite is untouched.
- `tests/residual-detach.spec.ts` unit-tests `detachResidual`: the carried copy equals the residual, owns a backing store sized to its own length (fixture kept above Node's Buffer pool threshold), and does not share the source frame's `ArrayBuffer`.
- `tests/runtime.spec.ts` — the output-cap case asserts the `ceiling - envelope` bound (268435392) and its message. A daemon-thread case drives four threads emitting unterminated writes through settlement's flush. A native-write case writes 200 KiB with no newline via `os.write` under a raised `maxLogBytes` and asserts it reads back as EXACTLY one log entry (proving stray output is aggregated by line, not split at pipe-chunk boundaries); a companion writes `b"one\ntwo\nthree"` and asserts three entries (proving real newlines still delimit). The same-group reap case spawns a SIGTERM-ignoring same-group descendant that releases the pipes and bumps a heartbeat file; the test asserts the heartbeat STOPS after the grace-window SIGKILL — an assertion robust whether the killed descendant is reaped or lingers as a zombie, so it holds where PID 1 does not wait() orphans. A dispose-after-resolve case asserts `dispose()` of a completed run with a same-group survivor returns only after the survivor stops executing (proving the run stays in `live` until its group is reaped), with an `expect(afterDispose).toBeGreaterThan(0)` guard so the frozen-heartbeat assertion cannot pass vacuously when the file was never written. A deadline case busy-blocks the event loop past both timers and asserts the survivor's heartbeat freezes (proving the poll's deadline arm sends SIGKILL itself rather than cancelling the unfired escalation). The cross-loop case runs a binding from a worker thread's own `asyncio.run` loop while the main coroutine yields with `await asyncio.sleep`, asserting the reply round-trips instead of timing out; a companion case abandons a thread's call so its loop closes, then answers it before a later binding — asserting the pump survives the closed-loop `call_soon_threadsafe` (host-gated ordering makes it deterministic, fail-before hangs the later binding to the wall clock). The inherited-soft-limit case runs the interpreter through a `ulimit -S -t` wrapper that sets a CPU soft limit below `cpuSeconds` and asserts the applied `RLIMIT_CPU` soft is the inherited value, not the configured one (CPU rather than address space, since macOS ignores `ulimit -v`); a companion inherits a 1 s CPU soft, has the program trap SIGXCPU and busy-loop past it, and asserts the settlement recheck reports a timeout — proving the recheck uses the effective soft, not the configured `cpuSeconds`. A control-heavy-diagnostic case raises a NUL-flood exception under a small `maxValueBytes` and asserts the serialized frame fits (proving the diagnostic is metered by serialized cost). A non-integer-budget case asserts a fractional `maxLogBytes`/`maxValueBytes` rejects at load.
## Alternatives considered
**Leave the boot-write `/* v8 ignore */` and fix only the ordering.** Rejected: the ignore is what let the TDZ regression ship uncaught. Removing it makes the catch a measured branch, so per-file 100% coverage now proves the failure path is exercised.
**Fix the flush race by capturing more bound methods.** Rejected: this is the approach that already failed. Binding a callable fixes reference resolution, not concurrent access to the mutable state the callable reads. Only mutual exclusion over the shared ledger closes the race.
**Guard the residual with a size threshold (copy only large frames).** Rejected: the branch runs once per newline-bearing read, the copy is bounded by the residual's own length (always a partial line), and a threshold adds a tunable and a second code path for no measurable saving. An unconditional right-sized copy is simpler and always correct.
**Assert the residual memory effect through the seam.** Rejected: the retained allocation is not observable through `CodeRunResult`, so a black-box test could not distinguish fixed from unfixed. Extracting `detachResidual` makes the backing-store invariant a deterministic unit test instead.
**Reap the same-group survivor with a fire-and-forget `unref`'d SIGKILL timer alone.** Rejected: an `unref`'d timer does not keep the host alive, so a host that exits within the grace window (a one-shot run, a config subprocess) never fires the SIGKILL and the survivor is reparented to init — the same "no subprocess outlives the fiber" violation in a different shape, and `teardown`'s "await each child's exit" JSDoc would be false. Awaiting the group's death on a ref'd poll keeps the host alive exactly long enough to reap, at zero cost in the common empty-group case.
**Assert the reap with `process.kill(pid, 0)` throwing ESRCH.** Rejected: a SIGKILL'd process lingers as a zombie until its parent `wait()`s it, and in a container whose PID 1 does not reap orphans the signal-0 probe keeps succeeding, so the assertion would false-fail cross-environment. A heartbeat file that stops advancing detects "no longer executing," which a reaped process and a zombie both satisfy.
**Complete the cross-loop Future with a plain `set_result` and rely on the GIL.** Rejected: the GIL serializes bytecode but does not make `asyncio.Future` cross-loop-safe — completing a Future from a thread other than its loop's does not schedule its callbacks or wake the loop. `call_soon_threadsafe` on the owning loop is the documented mechanism.
**Leave the SIGKILL timer armed after settlement (the earlier same-group fix).** Rejected: an unref'd timer left to fire up to `graceMs` after the leader was reaped can `kill(-pid)` a RECYCLED pgid, striking an unrelated group; the danger is the kill that succeeds, which `killGroup`'s ESRCH swallow cannot prevent. Clearing the timer once the group is confirmed empty bounds the reuse window to the genuine-survivor case, where the group is not empty to reuse.
**Clamp rlimits by the inherited hard limit only.** Rejected: that silently RAISES an inherited soft limit stricter than the request, loosening the very containment the clamp exists to preserve. Clamping each side against its own inherited bound (then pinning soft under hard) keeps the strictest of configured and inherited on both.
**Bill the host-side `capMessage` backstop by serialized cost, matching the child's `_cap_message`.** Rejected: the two caps guard different things. `_cap_message`'s output re-crosses fd 3 as a JSON string, so its escaped width is what the frame ceiling bounds — serialized billing is required there. `capMessage`'s output goes straight into `CodeRunResult.error.message` and never re-crosses a frame-bounded channel, so the honest measure of what it retains is the raw byte length of the model-visible string. An honest child has already capped by serialized cost and raw length ≤ serialized cost, so a well-formed message passes unchanged; a forged control-heavy message could serialize to ~6× its raw length, but since it travels no capped channel, billing it by that inflated wire width would truncate a legitimately-sized diagnostic for no containment gain. Each side's JSDoc documents the split and points at the other.
**Push stray pipe output one entry per `data` chunk.** Rejected: `logs` entries are joined with `\n` downstream, so a transport chunk boundary would become a model-visible newline — a single native write split across pipe reads would read back with spurious line breaks. Aggregating by real newline (residual + flush on `end`) matches the child's line-granular `log` frames; the ledger still bounds a newline-free flood by admitting-and-truncating the residual when it would cross the budget.
## Consequences
The seam's resolve-don't-reject contract holds on the boot-write path and the synchronous-spawn-failure path, both with measured coverage, and neither strands a staging directory. Log capture is thread-safe at the cost of one re-entrant lock acquisition per write and flush, and stray native output is delimited by its own newlines rather than by transport chunks. Fd-3 residual memory is bounded by the actual retained bytes, and both frame readers scan an accumulating frame once rather than quadratically. The output caps admit every value a frame can carry and reject a non-integer budget at load. Disposal is genuinely quiescent against a same-group survivor — bounded by `graceMs + 2 * CLOSE_REAP_MARGIN_MS`, zero-cost when the group is already empty, with the SIGKILL timer cleared once the group empties so a stale kill cannot strike a recycled pgid — RLIMIT enforcement keeps the strictest of configured and inherited on both soft and hard (and the SIGXCPU diagnostic no longer names a budget the host cannot guarantee), bindings called from model-created threads complete instead of timing out, and the handshake frame reader no longer burns the CPU budget on a large program. Every behavioral fix carries a test that fails without it, except the two called out in the Problem section — the chunked frame read (a syscall-count improvement) and the confirmed-empty finalize (whose only seam-observable effect freezes at signal delivery, which the pre-fix code also produced) — so a future regression on the rest goes red.