diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index 8b29da3ed80..fc03ad12599 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -1112,14 +1112,30 @@ jobs: # not racing a live process. This also removes the localhost blind # scan surface the bearer-gated proxy defends against. The proxy # started further below runs as root, so this cannot touch it. + # Zombies do not count. A zombie has already exited and released + # everything except its exit status, so it cannot re-plant an + # artifact or touch the agent's inputs — and it cannot be killed + # either, so counting it means this check can never clear. The + # container runs no init to reap orphans, so they are expected. + live_build_processes() { + ps -o pid=,stat=,args= -u node 2>/dev/null | awk '$2 !~ /^Z/' + } pkill -KILL -u node 2>/dev/null || true for _ in 1 2 3; do - pgrep -u node >/dev/null 2>&1 || break + [ -n "$(live_build_processes)" ] || break sleep 1 pkill -KILL -u node 2>/dev/null || true done - if pgrep -u node >/dev/null 2>&1; then - echo "::error::Processes owned by the build user survived; refusing to start the agent." + survivors="$(live_build_processes)" || true + if [ -n "$survivors" ]; then + # Name them. The first version of this guard failed the job with + # nothing but "processes survived", so a real threat and a + # harmless leftover were indistinguishable — including to the + # person who wrote it. + echo "::error::Processes owned by the build user survived SIGKILL; refusing to start the agent." + printf '%s\n' "$survivors" | while IFS= read -r proc; do + echo "::error:: surviving process: ${proc}" + done exit 1 fi @@ -2329,14 +2345,30 @@ jobs: # child can outlive its step, wait for the sweeps below, and then # re-plant artifacts or tamper with the agent's inputs. Without # this, every one-shot cleanup here is racing a live process. + # Zombies do not count. A zombie has already exited and released + # everything except its exit status, so it cannot re-plant an + # artifact or touch the agent's inputs — and it cannot be killed + # either, so counting it means this check can never clear. The + # container runs no init to reap orphans, so they are expected. + live_build_processes() { + ps -o pid=,stat=,args= -u node 2>/dev/null | awk '$2 !~ /^Z/' + } pkill -KILL -u node 2>/dev/null || true for _ in 1 2 3; do - pgrep -u node >/dev/null 2>&1 || break + [ -n "$(live_build_processes)" ] || break sleep 1 pkill -KILL -u node 2>/dev/null || true done - if pgrep -u node >/dev/null 2>&1; then - echo "::error::Processes owned by the build user survived; refusing to start the agent." + survivors="$(live_build_processes)" || true + if [ -n "$survivors" ]; then + # Name them. The first version of this guard failed the job with + # nothing but "processes survived", so a real threat and a + # harmless leftover were indistinguishable — including to the + # person who wrote it. + echo "::error::Processes owned by the build user survived SIGKILL; refusing to start the agent." + printf '%s\n' "$survivors" | while IFS= read -r proc; do + echo "::error:: surviving process: ${proc}" + done exit 1 fi diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index ba7cd32efec..c0130872dcd 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -1256,8 +1256,9 @@ describe('qwen-triage verify hardening round 2', () => { expect(chown).toBeGreaterThan(repin); expect(launch).toBeGreaterThan(home); // Killing is not enough on its own: surviving build processes must - // fail the step rather than race the sweeps that follow. - expect(runStep).toContain('pgrep -u node'); + // fail the step rather than race the sweeps that follow. The check + // disregards zombies — see the build-process-guard suite for why. + expect(runStep).toContain('live_build_processes'); expect(runStep).toContain('refusing to start the agent'); expect(runStep).toContain('"HOME=$AGENT_HOME"'); // The proxy must require this run's bearer, not just a fixed dummy key. @@ -2442,7 +2443,9 @@ describe('qwen-triage verify maintainer-review round', () => { // A mid-body stall closes the response (curl 18), not a hang until the // client's own timeout (curl 28). expect(out).toContain('stall_exit=18'); - }); + // 20 chunks x 200 ms is 4 s before the stall arm even starts, so this + // cannot fit vitest's 5 s default. It was timing out on main. + }, 30000); // GitHub cancels the OLDER pending run in a concurrency group, so the // requester's own /verify proceeds — the earlier "queued behind other @@ -2715,7 +2718,9 @@ describe('qwen-triage tmux lane parity', () => { const out = runProxyWatchdogTest(proxy); expect(out).toContain('chunks=20'); expect(out).toContain('stall_exit=18'); - }); + // Same reason as its verify-lane twin: the stream alone outlasts the + // 5 s default. + }, 30000); // PR lifecycle scripts run before the agent and can plant a // tmp/-tmux-/ directory whose report.md and transcript the @@ -2821,7 +2826,7 @@ describe('qwen-triage tmux lane parity', () => { const runStep = stepIn('tmux-testing', 'Run tmux real-user testing'); expect(runStep).toContain('pkill -KILL -u node'); expect(runStep).toContain( - 'Processes owned by the build user survived; refusing to start the agent.', + 'Processes owned by the build user survived SIGKILL; refusing to start the agent.', ); // Before the sweep and the proxy: the cleanup must not race a live // process, and no leftover child may be alive when the proxy binds. @@ -2926,3 +2931,122 @@ describe('qwen-triage tmux lane parity', () => { expect(reportCap + transcriptCap + envelope).toBeLessThan(65536); }); }); + +describe('qwen-triage build-process guard', () => { + // The guard fired on a real run (job 30267953352) and failed the job with + // nothing but "processes survived" — no pid, no state, no command line. + // Nobody could tell a genuine leftover from a harmless one, including the + // person who wrote it. Both lanes now name what survived. + it('names the surviving processes instead of just refusing', () => { + for (const lane of ['verify', 'tmux-testing']) { + const runStep = stepIn( + lane, + lane === 'verify' + ? 'Run verification agent' + : 'Run tmux real-user testing', + ); + expect(runStep, `${lane} lost the guard`).toContain( + 'live_build_processes', + ); + expect(runStep).toContain('surviving process:'); + expect(runStep).toContain( + 'Processes owned by the build user survived SIGKILL; refusing to start the agent.', + ); + } + }); + + // `ps -u node` exits 1 when the user owns zero processes. Under + // `set -euo pipefail` the bare assignment would die silently on the + // success path — the `|| true` absorbs the no-match status. + it('"survivors" assignment tolerates zero processes under pipefail', () => { + for (const lane of ['verify', 'tmux-testing']) { + const runStep = stepIn( + lane, + lane === 'verify' + ? 'Run verification agent' + : 'Run tmux real-user testing', + ); + expect( + runStep, + `${lane}: survivors assignment must survive ps exit 1`, + ).toContain('survivors="$(live_build_processes)" || true'); + } + }); + + // A zombie cannot be killed and cannot execute anything, so counting one + // means this check can never clear. + // + // PLATFORM NOTE, and the reason this test is split in two: Linux pgrep + // reports defunct processes ("Defunct processes are reported." — pgrep(1), + // procps-ng), which is why the original `pgrep -u node` guard could hang + // on a zombie in CI. macOS pgrep does NOT list them, so the behavioural + // arm below cannot discriminate the old implementation from the new one + // here — verified directly: ps lists our zombie, pgrep does not. The + // structural assertion is therefore the one that holds on every platform. + it('excludes zombies from the surviving-process check', () => { + for (const lane of ['verify', 'tmux-testing']) { + const runStep = stepIn( + lane, + lane === 'verify' + ? 'Run verification agent' + : 'Run tmux real-user testing', + ); + const body = runStep + .match(/live_build_processes\(\) \{\n([\s\S]*?)\n\s*\}/)?.[1] + ?.trim(); + expect(body, `${lane}: no live_build_processes body`).toBeTruthy(); + // It must read process STATE and drop zombies. `pgrep` alone cannot: + // on Linux it reports defunct processes and offers no default filter. + expect(body, `${lane}: the filter must inspect process state`).toMatch( + /stat=/, + ); + expect(body, `${lane}: the filter must exclude zombies`).toMatch( + /\/\^Z\//, + ); + expect(body).not.toMatch(/^pgrep\b/); + } + }); + + // The OS property the exclusion rests on: a zombie survives SIGKILL and + // ps still lists it, so an unfiltered check would never clear. + it('confirms a zombie survives SIGKILL and stays visible to ps', () => { + const dir = mkdtempSync(join(tmpdir(), 'zombie-')); + try { + writeFileSync( + join(dir, 'mkzombie.py'), + [ + 'import os, time', + 'pid = os.fork()', + 'if pid == 0:', + ' os._exit(0)', + 'print(pid, flush=True)', + 'time.sleep(8)', + ].join('\n'), + ); + const driver = [ + 'set -u', + `python3 "$1/mkzombie.py" > "$1/zpid" &`, + 'PP=$!', + 'sleep 1', + 'Z="$(tr -d " \n" < "$1/zpid")"', + '[ -n "$Z" ] || { echo "no-zombie"; kill $PP 2>/dev/null; exit 0; }', + 'kill -9 "$Z" 2>/dev/null', + 'sleep 0.5', + 'echo "state=$(ps -o stat= -p "$Z" 2>/dev/null | tr -d " ")"', + 'echo "unfiltered=$(ps -o pid= -p "$Z" 2>/dev/null | wc -l | tr -d " ")"', + `echo "filtered=$(ps -o pid=,stat=,args= -p "$Z" 2>/dev/null | awk '$2 !~ /^Z/' | wc -l | tr -d ' ')"`, + 'kill $PP 2>/dev/null', + ].join('\n'); + const out = spawnSync('bash', ['-c', driver, '_', dir], { + encoding: 'utf8', + timeout: 30000, + }).stdout; + if (out.includes('no-zombie')) return; + expect(out).toMatch(/state=Z/); + expect(out).toContain('unfiltered=1'); + expect(out).toContain('filtered=0'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }, 30000); +});