fix(code-runtime-python): restore SIGXCPU disposition before unblocking and floor the budgets

Addresses the review's two code warnings and one suggestion: - die_if_cpu_exhausted now restores SIG_DFL BEFORE unblocking SIGXCPU: a program that installed a custom handler AND masked the signal would otherwise have that pending handler run at the unblock (in model code, re-masking or raising) and escape the re-raise; with SIG_DFL first the pending signal kills inside the kernel with no bytecode window. A trap+mask combined regression test pins it (the mask-only case was already covered). - The constructor rejects budgets too small to honor: maxLogBytes must fit the truncation marker plus the serialized outer-array envelope (floor 64), and maxValueBytes must at least represent the smallest JSON completion (floor 4, matching the worker backend). The exact-limit test moves to the 64 floor and a rejection test pins the floors. - The pthread_sigmask None-guard comment cites the real rationale (defensive against stripped CPython builds; win32 is refused at construction), not the unreachable Windows path.
2026-09-11 04:00:38 +00:00 · 2026-08-31 14:41:49 +08:00
parent a2eda792e3
commit 4a8c49f78c
3 changed files with 90 additions and 29 deletions
@@ -1957,7 +1957,9 @@ def _make_cpu_enforcer() -> Any:
    # SIGXCPU unmasking primitives for the re-raise below: a program can mask
    # the signal and return past the soft limit, so the re-delivered signal
    # must be unblocked first. Captured here (import time) so a rebind cannot
-    # defeat them; ``None`` on platforms without ``pthread_sigmask`` (Windows).
+    # defeat them. The ``getattr``/``None`` guard is defensive against a
+    # stripped CPython build (the host refuses win32 at construction, so every
+    # platform this backend actually starts on has ``pthread_sigmask``).
    pthread_sigmask = getattr(signal, "pthread_sigmask", None)
    sig_unblock = getattr(signal, "SIG_UNBLOCK", None)

@@ -2014,14 +2016,20 @@ def _make_cpu_enforcer() -> Any:
            # A program can mask SIGXCPU (``pthread_sigmask(SIG_BLOCK, ...)``),
            # burn past the soft limit, and return during the soft-to-hard gap;
            # the re-delivered SIGXCPU below would then stay PENDING and the
-            # child would exit normally with a success result. Unblock it on the
-            # current thread before re-raising, so the signal is delivered and
-            # the host sees the kernel-authoritative timeout classification. On
-            # platforms without ``pthread_sigmask`` (Windows) the signal is not
-            # maskable this way, so the call is guarded.
+            # child would exit normally with a success result. Restore the
+            # default disposition BEFORE unblocking: a program that installed a
+            # custom handler AND masked the signal has that pending handler run
+            # the moment the signal is unblocked (CPython delivers it at the next
+            # eval-breaker checkpoint in model code), and it could re-mask or
+            # raise — so the disposition must already be SIG_DFL when the signal
+            # is released. With SIG_DFL restored first, the pending signal kills
+            # the process inside the kernel with no bytecode window; the ``kill``
+            # below is the fallback for the never-pending case. ``pthread_sigmask``
+            # is ``None``-guarded defensively (every platform this backend starts
+            # on has it; the host refuses win32 at construction).
+            set_signal(sigxcpu, sig_dfl)
            if pthread_sigmask is not None:
                pthread_sigmask(sig_unblock, (sigxcpu,))
-            set_signal(sigxcpu, sig_dfl)
            kill(getpid(), sigxcpu)

    return die_if_cpu_exhausted
@@ -226,6 +226,20 @@ const MAX_PENDING_CHUNKS = 1024
 */
 const FRAME_ENVELOPE_BYTES = 64

+/**
+ * Smallest `maxLogBytes` the backend can honor. The log ledger's truncation
+ * marker (`logTruncationMarker`) plus the serialized outer-array envelope must
+ * fit the budget, or a truncated run returns more than the configured cap: the
+ * marker text is `[dsh-code-runtime-python] log capture truncated at <N>
+ * bytes` — 49 fixed characters plus the digits of N plus 6 — serialized with
+ * quotes and brackets adds 4, so the smallest N that admits its own marker is
+ * 61 (49 + 2 + 6 + 4); 62 is the floor with one byte of room. `maxValueBytes`
+ * has no floor beyond the positive-integer requirement: a completion can be as
+ * small as a single byte (`1`), and the done-frame envelope is seam protocol
+ * cost, not the advertised completion budget.
+ */
+const MIN_LOG_BYTES = 62
+
 /**
 * Extra time added to `graceMs` before the post-kill close-deadline force-settles
 * a run whose `close` never fires (a setsid-escaped orphan holds our inherited
@@ -767,6 +781,12 @@ export class PythonCodeRuntime extends CodeRuntime {
      if (this.config[key] > limit) {
        throw new Error(`dsh-code-runtime-python: config.${key} must not exceed ${limit} (a payload that large cannot cross the ${FRAME_CEILING_BYTES}-byte fd-3 frame ceiling, so the run would fail as worker-exit rather than output-limit), got ${String(this.config[key])}`)
      }
+      // Reject a log budget too small to honor: the ledger must fit its
+      // truncation marker plus the serialized outer-array envelope, or a
+      // truncated run returns more than the configured cap.
+      if (key === 'maxLogBytes' && this.config[key] < MIN_LOG_BYTES) {
+        throw new Error(`dsh-code-runtime-python: config.maxLogBytes must be at least ${MIN_LOG_BYTES} (a smaller budget cannot serialize the truncation marker plus the outer-array envelope, so the run would return more than the configured cap), got ${String(this.config[key])}`)
+      }
    }
    // The child builds, charges, and frames a `maxLogBytes` log entry or a
    // `maxValueBytes` completion value under `RLIMIT_AS`, and both paths trigger
@@ -566,6 +566,36 @@ describe('PythonCodeRuntime — inherited resource limits', () => {
    expect(result.value).toBeUndefined()
  }, 20_000)

+  it('reports a timeout when a program traps AND masks SIGXCPU and returns past the soft limit', async () => {
+    // The mask-only case exercises the unblock; the trap+mask combination is
+    // the harder one: a program that installed a custom handler AND masked the
+    // signal has that PENDING handler run the moment the signal is unblocked
+    // (CPython delivers it at the next eval-breaker checkpoint in model code),
+    // and the handler re-masks — so the settlement recheck must restore the
+    // default disposition BEFORE unblocking. With SIG_DFL restored first, the
+    // pending signal kills the process inside the kernel with no bytecode
+    // window; without it, the handler re-blocks and the child exits normally
+    // with a success value. Fail-before: the run reports `value: "escaped"`.
+    const { runtime } = await setup({ cpuSeconds: 1, maxWallMs: 12_000 })
+    const result = await runtime.run({
+      program: [
+        'import signal, time',
+        'if hasattr(signal, "pthread_sigmask"):',
+        '    def h(signum, frame):',
+        '        signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGXCPU})',
+        '    signal.signal(signal.SIGXCPU, h)',
+        '    signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGXCPU})',
+        '    end = time.perf_counter() + 1.05',
+        '    while time.perf_counter() < end:',
+        '        pass',
+        'return "escaped"',
+      ].join('\n'),
+      bindings: [],
+    })
+    expect(result.error?.kind).toBe('timeout')
+    expect(result.value).toBeUndefined()
+  }, 20_000)
+
  it('rechecks CPU at settlement against the effective inherited soft limit', async () => {
    // The settlement-time CPU recheck must compare against the EFFECTIVE soft
    // limit (`_clamped` may have lowered it to a stricter inherited value), not
@@ -2884,7 +2914,7 @@ describe('PythonCodeRuntime — hostile peer', () => {
  it('truncates host-side logs once the budget is exhausted and emits the marker', async () => {
    // Set a tiny host-side budget; the Python side has a much larger one, so
    // its LogBuffer will not truncate — the host ledger fires first.
-    const { runtime } = await setup({ maxLogBytes: 32 })
+    const { runtime } = await setup({ maxLogBytes: 128 })
    const result = await runtime.run({
      program: [
        'for _ in range(50):',
@@ -2894,7 +2924,7 @@ describe('PythonCodeRuntime — hostile peer', () => {
      bindings: [],
    })
    expect(result.error).toBeUndefined()
-    const markers = result.logs.filter(line => line.includes('log capture truncated at 32 bytes'))
+    const markers = result.logs.filter(line => line.includes('log capture truncated at 128 bytes'))
    expect(markers.length).toBeGreaterThanOrEqual(1)
  })

@@ -3891,28 +3921,31 @@ describe('PythonCodeRuntime — hostile peer', () => {
    // Each entry is charged its JSON-string cost plus one separator byte, and
    // the serialized outer logs array adds one more byte of envelope (two
    // brackets and n-1 commas). The ledgers reserve that byte, so a result that
-    // exactly exhausts the ledger still serializes within the configured cap:
-    // `["a"]` is 5 bytes, and `maxLogBytes: 5` admits it (ledger 4 = the 3-byte
-    // quoted entry + 1 separator), while `maxLogBytes: 4` (ledger 3) truncates
-    // to the marker alone.
-    const { runtime } = await setup({ maxLogBytes: 5, maxWallMs: 10_000 })
+    // exactly exhausts the ledger still serializes within the configured cap.
+    // At the 64-byte floor: ledger 63, a 60-character line serializes as
+    // `"aaa...a"` (62 bytes) + 1 separator = 63, exactly exhausting the ledger
+    // and serializing as `["aaa...a"]` = 64 = the cap; a 61-character line
+    // costs 64 > 63 and truncates to the marker alone.
+    const { runtime } = await setup({ maxLogBytes: 64, maxWallMs: 10_000 })
    const result = await runtime.run({
-      program: ['print("a")', 'return "done"'].join('\n'),
+      program: ['print("a" * 60 + "\\n" + "b" * 61, end="")', 'return "done"'].join('\n'),
      bindings: [],
    })
    expect(result.error).toBeUndefined()
    expect(result.value).toBe('done')
-    expect(result.logs).toContain('a')
-    expect(result.logs.some(line => line.includes('log capture truncated'))).toBe(false)
+    // The 60-character line was admitted; the 61-character line was not (a
+    // single 'b' would also match the marker's "bytes", so check for the line).
+    expect(result.logs).toContain('a'.repeat(60))
+    expect(result.logs.some(line => line.includes('b'.repeat(61)))).toBe(false)
+    expect(result.logs.some(line => line.includes('log capture truncated'))).toBe(true)
+  }, 15_000)

-    const tight = await setup({ maxLogBytes: 4, maxWallMs: 10_000 })
-    const result2 = await tight.runtime.run({
-      program: ['print("a")', 'return "done"'].join('\n'),
-      bindings: [],
-    })
-    expect(result2.error).toBeUndefined()
-    expect(result2.logs).not.toContain('a')
-    expect(result2.logs.some(line => line.includes('log capture truncated'))).toBe(true)
+  it('rejects a log budget too small to serialize the truncation marker', async () => {
+    // A maxLogBytes below 62 cannot serialize the truncation marker plus the
+    // outer-array envelope; it is rejected at construction so a tiny config
+    // cannot report more than the public cap. maxValueBytes keeps no floor
+    // beyond the positive-integer requirement (a completion can be 1 byte).
+    await expect(setup({ maxLogBytes: 61, maxWallMs: 10_000 })).rejects.toThrow(/must be at least 62/)
  }, 15_000)

  it('charges the JSON-escaped cost of control characters against the log ledger', async () => {
@@ -4344,7 +4377,7 @@ describe('PythonCodeRuntime — hostile peer', () => {
    // hundreds-of-megabytes host allocation. The cheap `length + 3` lower bound
    // truncates it instead. The host's own heap is what is under test, so keep
    // the child's address space generous enough to BUILD the frame.
-    const { runtime } = await setup({ maxLogBytes: 32, addressSpaceMb: 1024, maxWallMs: 60_000 })
+    const { runtime } = await setup({ maxLogBytes: 128, addressSpaceMb: 1024, maxWallMs: 60_000 })
    const before = process.memoryUsage().heapUsed
    const result = await runtime.run({
      program: [
@@ -4358,7 +4391,7 @@ describe('PythonCodeRuntime — hostile peer', () => {
    expect(result.error).toBeUndefined()
    expect(result.value).toBe('settled')
    // The frame was dropped as one truncation marker, not retained.
-    expect(result.logs).toEqual([logTruncationMarker(32)])
+    expect(result.logs).toEqual([logTruncationMarker(128)])
    // The escaped copy (~144 MiB) was never materialized.
    expect(process.memoryUsage().heapUsed - before).toBeLessThan(256 * 1024 * 1024)
  }, 90_000)
@@ -4368,7 +4401,7 @@ describe('PythonCodeRuntime — hostile peer', () => {
    // control-heavy frame clears it and must still be charged what it costs on
    // the wire. Ten NULs are 13 against the 32-byte lower bound but 63 escaped
    // (six bytes each, two quotes, one separator), so the full charge truncates.
-    const { runtime } = await setup({ maxLogBytes: 32 })
+    const { runtime } = await setup({ maxLogBytes: 63 })
    const result = await runtime.run({
      program: [
        'import os',
@@ -4379,7 +4412,7 @@ describe('PythonCodeRuntime — hostile peer', () => {
    })
    expect(result.error).toBeUndefined()
    expect(result.value).toBe('settled')
-    expect(result.logs).toEqual([logTruncationMarker(32)])
+    expect(result.logs).toEqual([logTruncationMarker(63)])
  }, 8000)

  it('caps a forged done error.message from its code-unit prefix, never encoding the whole message', async () => {