From 65c97a07606986f3565a175afad63af12d53ec92 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 20:32:18 +0000 Subject: [PATCH] test(spec): order the sdui cleanup fd count on the child's own post-exec report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sdui_spawn_detached` returns as soon as the pidfile is non-empty, and its runner writes that pid on its FIRST line — before the loop that closes the descriptors inherited from the caller. Measured on this tree: the pid is published 151–297us before the close completes, and the only thing keeping a pidfile-ordered count green is the caller's unrelated 50ms polling sleep (margin 10–54ms against a 0.2ms critical section). So the count was ordered on a barrier that means "the leader exists", not "the leader let go of my descriptors", and it could legitimately observe the leader still holding the caller's lock fd — which is what `Test Core (1/6)` saw once as LOCKFDS=1 with LEADER_ALIVE_AT_COUNT=yes. The stub now takes the reading of itself after `exec` has replaced that runner and publishes it, and the harness orders the session-wide count on that report and on the helper having joined the session. Both barriers wait for the reading to become takeable, never for a value; a timed-out barrier is its own named red. The stub is released by a file instead of a 1200ms timer, so LEADER_ALIVE_AT_COUNT stops racing node's boot time, and the orphan precondition waits for PPID 1 instead of sleeping a guessed two seconds. No assertion was skipped, loosened, retried or given a longer timeout. Claude-Session: https://claude.ai/code/session_01KB5PFtxuy1x3dcR5gxudx6 Co-authored-by: Claude --- .../scripts/gen-sdui-manifest-cleanup.test.ts | 116 +++++++++++++++--- 1 file changed, 98 insertions(+), 18 deletions(-) diff --git a/packages/spec/scripts/gen-sdui-manifest-cleanup.test.ts b/packages/spec/scripts/gen-sdui-manifest-cleanup.test.ts index 1b56e35e990..42500933aac 100644 --- a/packages/spec/scripts/gen-sdui-manifest-cleanup.test.ts +++ b/packages/spec/scripts/gen-sdui-manifest-cleanup.test.ts @@ -30,6 +30,23 @@ // into "this container is closed for business". This half matters even // when the kill works, because it is what makes a MISSED kill survivable. // +// WHAT THIS COUNT MUST BE ORDERED AFTER, AND WHY THE PIDFILE IS NOT IT. +// `sdui_spawn_detached` returns as soon as the pidfile is non-empty, and +// its runner writes that pid on its FIRST line -- before the loop that +// closes the inherited descriptors. Measured on this tree: the pid is +// published 151-297us before the close completes, and the ONLY thing that +// kept a pidfile-ordered reading green was the caller's unrelated 50ms +// polling sleep (margin 10-54ms, from a 0.2ms critical section). So the +// pidfile is a barrier for "the leader exists", never for "the leader has +// let go of my descriptors", and a count ordered on it reads a state the +// spawn legitimately passes through -- observed once in CI as LOCKFDS=1 +// with LEADER_ALIVE_AT_COUNT=yes and the leader still `bash`, reproduced +// here by lagging the runner between those two steps. The barrier below is +// the spawned command's OWN post-exec report instead: `exec` replaces the +// runner that does the closing, so that line cannot exist until the close +// has run. We wait for the REPORT, never for a value -- the stub prints +// whatever it counts, so a real leak still arrives as a red. +// // 3. After the trap, nothing from that session survives — specifically // including a process that has been reparented to init. That is the case // `kill "$!"` provably cannot reach, and the test asserts the orphan @@ -68,15 +85,33 @@ describe.skipIf(!RUNNABLE)('gen-sdui-manifest.sh cleanup contract', () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'sdui-cleanup-')); const lock = path.join(dir, 'caller.lock'); const harness = path.join(dir, 'harness.sh'); + const devlog = path.join(dir, 'dev.log'); + const release = path.join(dir, 'release'); fs.writeFileSync(lock, ''); // A stub server that reproduces the shape that defeats `kill "$!"`: it // spawns a helper and then exits, leaving the helper reparented to init // while still inside the session the script created. + // + // It also takes the in-child reading of property (2) and publishes the two + // facts the harness needs to order itself: this code runs only after `exec` + // replaced the shell that closes the inherited descriptors, so every line + // it prints is causally after that close. It exits on a release file rather + // than a wall-clock timer, so "was the leader alive when we counted?" stops + // being a race against node's boot time; the 60s cap is a self-destruct for + // the case where the harness dies without reaping, never a wait anyone + // relies on. const stub = [ + 'const fs = require("node:fs");', 'const { spawn } = require("node:child_process");', - 'spawn(process.execPath, ["-e", "setInterval(() => {}, 1e9)"], { stdio: "ignore" });', - 'setTimeout(() => process.exit(0), 1200);', + 'const LOCK = fs.realpathSync(process.argv[1]); const RELEASE = process.argv[2];', + 'const mine = fs.readdirSync("/proc/self/fd").filter((n) => {', + ' try { return fs.readlinkSync("/proc/self/fd/" + n) === LOCK; } catch { return false; }', + '}).length;', + 'const helper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1e9)"], { stdio: "ignore" });', + 'fs.writeSync(1, "CHILD_LOCKFDS=" + mine + "\\nHELPER=" + helper.pid + "\\nEXECED=1\\n");', + 'setInterval(() => { if (fs.existsSync(RELEASE)) process.exit(0); }, 25);', + 'setTimeout(() => process.exit(0), 60000);', ].join(''); fs.writeFileSync( @@ -87,32 +122,70 @@ describe.skipIf(!RUNNABLE)('gen-sdui-manifest.sh cleanup contract', () => { // Sourcing runs no generation — the script returns right after defining // its lifecycle helpers, so this exercises the REAL functions. `source ${JSON.stringify(SCRIPT)}`, + `DEVLOG=${JSON.stringify(devlog)}`, + `RELEASE=${JSON.stringify(release)}`, + `LOCK=${JSON.stringify(lock)}`, + `LOCKBASE=${JSON.stringify(path.basename(lock))}`, 'DUMP_PID_FILE="$(mktemp "${TMPDIR:-/tmp}/sdui-dump-pid.XXXXXX")"', `trap 'sdui_stop_detached "$DUMP_PID_FILE"' EXIT`, - `sdui_spawn_detached "$DUMP_PID_FILE" ${JSON.stringify(path.join(dir, 'dev.log'))} \\`, - ` "$(command -v node)" -e ${JSON.stringify(stub)}`, + 'sdui_spawn_detached "$DUMP_PID_FILE" "$DEVLOG" \\', + ` "$(command -v node)" -e ${JSON.stringify(stub)} "$LOCK" "$RELEASE"`, 'LEADER="$(cat "$DUMP_PID_FILE")"', 'echo "LEADER=$LEADER"', 'echo "LEADER_SID=$(ps -o sid= -p "$LEADER" | tr -d " ")"', 'echo "SCRIPT_PGID=$(ps -o pgid= -p $$ | tr -d " ")"', - // Count inherited descriptors NOW, while the session LEADER is still - // alive. Counting later would inspect only the reparented helper, which - // never inherits the fd anyway (node closes it across its own spawn) — - // an assertion that stays green with the fd hygiene deleted. That green - // was observed before this line moved, so the ordering is the test. + // Barrier 1 — the spawned command's own post-exec report. NOT the + // pidfile: see the note on property (2) in this file's header. This + // waits for the reading to become TAKEABLE, never for it to become + // zero. + 'CHILD_REPORTED=no', + 'for _i in $(seq 1 600); do', + ' if grep -q "^EXECED=1$" "$DEVLOG" 2>/dev/null; then CHILD_REPORTED=yes; break; fi', + ' sleep 0.05', + 'done', + 'echo "CHILD_REPORTED=$CHILD_REPORTED"', + 'CHILD_LOCKFDS="$(sed -n "s/^CHILD_LOCKFDS=//p" "$DEVLOG" 2>/dev/null || true)"', + 'HELPER="$(sed -n "s/^HELPER=//p" "$DEVLOG" 2>/dev/null || true)"', + 'echo "CHILD_LOCKFDS=$CHILD_LOCKFDS"', + 'echo "HELPER=$HELPER"', + // Barrier 2 — the helper has joined the session, so the session-wide + // count below covers both members instead of whoever happened to exist. + 'HELPER_IN_SESSION=no', + 'for _i in $(seq 1 200); do', + ' MEMB="$(pgrep -s "$LEADER" 2>/dev/null | tr "\\n" " " || true)"', + ' case " $MEMB " in *" $HELPER "*) HELPER_IN_SESSION=yes ;; esac', + ' if [ "$HELPER_IN_SESSION" = yes ]; then break; fi', + ' sleep 0.05', + 'done', + 'echo "HELPER_IN_SESSION=$HELPER_IN_SESSION"', + // The session-wide count, now ordered after both barriers. Naming the + // holders turns a future red from "1, expected 0" into a process. 'FDS=0', + 'HOLDERS=""', 'for p in $(pgrep -s "$LEADER"); do', - ` n=$(ls -l "/proc/$p/fd" 2>/dev/null | grep -c ${JSON.stringify(path.basename(lock))} || true)`, + ' n=$(ls -l "/proc/$p/fd" 2>/dev/null | grep -c "$LOCKBASE" || true)', + ' if [ "$n" != 0 ]; then HOLDERS="$HOLDERS $p($(tr -d "\\0\\n" < "/proc/$p/comm" 2>/dev/null || echo unknown))"; fi', ' FDS=$((FDS + n))', 'done', 'echo "LOCKFDS=$FDS"', + 'echo "LOCKFD_HOLDERS=[$HOLDERS]"', `echo "LEADER_ALIVE_AT_COUNT=$(kill -0 "$LEADER" 2>/dev/null && echo yes || echo no)"`, - // Now let the stub exit, so its helper is reparented to init before cleanup. - 'sleep 2', - 'MEMBERS="$(pgrep -s "$LEADER" | tr "\\n" " ")"', - 'echo "MEMBERS=$MEMBERS"', + // Now release the stub, so its helper is reparented to init before + // cleanup, and wait for that to have HAPPENED rather than sleeping a + // guessed interval. A timeout leaves ORPHAN_BEFORE empty, which the + // precondition assertion reads as a red. + ': > "$RELEASE"', 'ORPHAN=""', - 'for p in $MEMBERS; do [ "$(ps -o ppid= -p "$p" | tr -d " ")" = "1" ] && ORPHAN="$p"; done', + 'MEMBERS=""', + 'for _i in $(seq 1 400); do', + ' MEMBERS="$(pgrep -s "$LEADER" 2>/dev/null | tr "\\n" " " || true)"', + ' for p in $MEMBERS; do', + ' if [ "$(ps -o ppid= -p "$p" 2>/dev/null | tr -d " ")" = "1" ]; then ORPHAN="$p"; fi', + ' done', + ' if [ -n "$ORPHAN" ]; then break; fi', + ' sleep 0.05', + 'done', + 'echo "MEMBERS=$MEMBERS"', 'echo "ORPHAN_BEFORE=$ORPHAN"', // Falling off the end fires the EXIT trap, which is the path under test. ].join('\n'), @@ -133,13 +206,20 @@ describe.skipIf(!RUNNABLE)('gen-sdui-manifest.sh cleanup contract', () => { expect(field('LEADER_SID'), out).toBe(leader); expect(field('SCRIPT_PGID'), out).not.toBe(leader); + // The barriers themselves: a timed-out barrier must arrive as a named red, + // not as a count taken at some other moment. + expect(field('CHILD_REPORTED'), out).toBe('yes'); + expect(field('HELPER_IN_SESSION'), out).toBe('yes'); + // (3, precondition) the orphan the naive `kill "$!"` cannot reach really // existed. Without this, the survivor assertion below could pass vacuously. expect(field('ORPHAN_BEFORE'), out).toMatch(/^\d+$/); - // (2) nothing in the session inherited the caller's lock descriptor — - // counted while the leader was still alive, which is the only moment the - // count can be non-zero and therefore the only moment it means anything. + // (2) nothing in the session inherited the caller's lock descriptor. Two + // distinct facts: the surviving-capable process's reading of ITSELF, taken + // after `exec` and therefore after the close; and the session-wide count, + // taken while the leader was still alive and after both barriers. + expect(field('CHILD_LOCKFDS'), out).toBe('0'); expect(field('LEADER_ALIVE_AT_COUNT'), out).toBe('yes'); expect(field('LOCKFDS'), out).toBe('0');