test: fix three pid/signal races behind CI flakes (batch 3) (#109877)

A. check-extension-package-tsc-boundary "force-kills aborted async node
step process groups" (run 29568640449): two mechanisms behind the
expected-false-to-be-true at the child-alive assertion.
- Pid-file existence polls can observe writeFileSync's open-truncate
  0-byte window, parse NaN, and process.kill(NaN, 0) throws, so
  isProcessAlive reads false (the #109140 class, same as tsdown-build's
  b1e6c6883b). Added waitForPidFile (poll until content parses to a
  positive pid) and applied it to all three pid-read sites in the file.
- The fail-fast step exited on a 150ms fuse after the sibling's pid file
  appeared, racing the test's aliveness assertion: a worker descheduled
  >150ms observes the abort chain's SIGKILL first. The step now exits
  only after the test writes abort.ack, which happens strictly after the
  assertion (event-driven; the step's 5s timeout bounds a wedged run).
  Forced proof: injecting a 300ms stall between pid-file wait and the
  assertion reproduced the exact CI failure, and passed post-fix with
  the same stall in place.

B. telegram-user-crabbox-proof "stops Crabbox recording when the desktop
probe fails" (waitFor timeout on recorder.term): the fake recorder
published its pid file before installing its SIGTERM handler. The probe
throws as soon as the pid file exists and recordProbeVideo SIGTERMs the
recorder in its finally, so a recorder preempted between publish and
process.on dies to the default disposition and never writes
recorder.term. Handler now arms before the pid publish. Forced proof: a
100ms spin between publish and process.on reproduced the exact CI
failure deterministically; moving the spin after the reordered handler
passes. Applied the same handler-before-readiness ordering to the three
sibling fakes in the file, and made the gateway-grandchild pid read
parse-validated inside its poll (a NaN pid skipped both the dead-check
and the finally SIGKILL cleanup, leaking the grandchild).

C. openclaw-live-updater "treats an owner file creation race as a normal
overlap" (run 29569279237): prod bug in acquireMaintenanceLock. Only
ENOENT got the bounded creation-window retry; a reader hitting the
racing writer's open-truncate window read an empty owner.json, JSON.parse
threw SyntaxError, and the script escalated to invalid_lock instead of
overlap. Any owner read/parse failure now re-reads within the same
bounded budget before declaring the lock invalid. New regression test
freezes the window (pre-created empty owner.json, delayed writer):
fails with the exact CI error pre-fix, passes post-fix.

Proof: focused runs green; 10x per-file stress at
OPENCLAW_VITEST_MAX_WORKERS=6 green for all three files (30/30).
This commit is contained in:
Peter Steinberger
2026-07-17 11:01:38 +01:00
committed by GitHub
parent 44f20956e3
commit 51fa35c94d
4 changed files with 89 additions and 27 deletions
@@ -490,8 +490,13 @@ export function acquireMaintenanceLock(checkout, requestedPath) {
let owner;
try {
owner = JSON.parse(readFileSync(path.join(lockPath, "owner.json"), "utf8"));
} catch (ownerError) {
if (ownerError?.code === "ENOENT" && incompleteLockRetries < 20) {
} catch {
// The mkdir winner publishes owner.json right after creating the lock
// dir, so readers can see ENOENT before the write and an empty/partial
// file during writeFileSync's open-truncate window. Both are
// creation-in-progress, not corruption; re-read within the bounded
// budget and only then declare the lock invalid.
if (incompleteLockRetries < 20) {
incompleteLockRetries += 1;
spawnSync("sleep", ["0.01"]);
continue;
@@ -74,6 +74,23 @@ async function waitForFile(filePath: string, timeoutMs: number): Promise<void> {
throw new Error(`timeout waiting for ${filePath}`);
}
// Pid files are written with plain writeFileSync, so an existence poll can
// observe the open-truncate 0-byte window and parse NaN (the #109140 flake
// class). Wait until the content parses to a real pid, not just for the file.
async function waitForPidFile(filePath: string, timeoutMs: number): Promise<number> {
const deadlineAt = Date.now() + timeoutMs;
while (Date.now() < deadlineAt) {
if (fs.existsSync(filePath)) {
const pid = Number.parseInt(fs.readFileSync(filePath, "utf8"), 10);
if (Number.isInteger(pid) && pid > 0) {
return pid;
}
}
await sleep(5);
}
throw new Error(`timed out waiting for pid in ${filePath}`);
}
async function waitForDead(pid: number, timeoutMs: number): Promise<void> {
const deadlineAt = Date.now() + timeoutMs;
while (Date.now() < deadlineAt) {
@@ -574,8 +591,7 @@ describe("check-extension-package-tsc-boundary", () => {
(error: unknown) => error,
);
await waitForFile(childPidPath, 2_000);
childPid = Number.parseInt(fs.readFileSync(childPidPath, "utf8"), 10);
childPid = await waitForPidFile(childPidPath, 2_000);
expect(isProcessAlive(childPid)).toBe(true);
const failure = await failurePromise;
@@ -620,6 +636,7 @@ describe("check-extension-package-tsc-boundary", () => {
async () => {
const { rootDir: root } = createTempExtensionRoot("abort-group");
const childPidPath = path.join(root, "child.pid");
const abortAckPath = path.join(root, "abort.ack");
let childPid = 0;
const childScript = ["process.on('SIGTERM', () => {});", "setInterval(() => {}, 1000);"].join(
"",
@@ -632,16 +649,15 @@ describe("check-extension-package-tsc-boundary", () => {
"process.on('SIGTERM', () => process.exit(0));",
"setInterval(() => {}, 1000);",
].join("");
const failAfterSiblingStartsScript = [
// fail-fast exits only after the test writes abort.ack, which happens
// strictly after the child-alive assertion below. A time-based fuse here
// races that assertion: a descheduled worker can observe the abort chain
// already SIGKILLing the group. The step's 5s timeout bounds a wedged run.
const failAfterTestAckScript = [
"const fs = require('node:fs');",
`const childPidPath = ${JSON.stringify(childPidPath)};`,
"const deadlineAt = Date.now() + 2_000;",
`const ackPath = ${JSON.stringify(abortAckPath)};`,
"const wait = () => {",
" if (fs.existsSync(childPidPath)) {",
" setTimeout(() => process.exit(2), 150);",
" return;",
" }",
" if (Date.now() >= deadlineAt) {",
" if (fs.existsSync(ackPath)) {",
" process.exit(2);",
" return;",
" }",
@@ -655,7 +671,7 @@ describe("check-extension-package-tsc-boundary", () => {
[
{
label: "fail-fast",
args: ["--eval", failAfterSiblingStartsScript],
args: ["--eval", failAfterTestAckScript],
timeoutMs: 5_000,
},
{
@@ -667,9 +683,9 @@ describe("check-extension-package-tsc-boundary", () => {
2,
);
await waitForFile(childPidPath, 2_000);
childPid = Number.parseInt(fs.readFileSync(childPidPath, "utf8"), 10);
childPid = await waitForPidFile(childPidPath, 2_000);
expect(isProcessAlive(childPid)).toBe(true);
fs.writeFileSync(abortAckPath, "go");
await expect(command).rejects.toThrow("fail-fast");
await waitForDead(childPid, 2_000);
@@ -725,8 +741,7 @@ describe("check-extension-package-tsc-boundary", () => {
});
await waitForFile(readyPath, 2_000);
await waitForFile(childPidPath, 2_000);
childPid = Number.parseInt(fs.readFileSync(childPidPath, "utf8"), 10);
childPid = await waitForPidFile(childPidPath, 2_000);
expect(isProcessAlive(childPid)).toBe(true);
runner.kill("SIGTERM");
@@ -1941,6 +1941,34 @@ describe("openclaw live updater", () => {
}
});
test("re-reads an empty owner file left by a racing writer's creation window", () => {
const { root, mirror } = makeFixture();
const lockPath = path.join(root, "maintenance.lock");
mkdirSync(lockPath);
// Freeze writeFileSync's open-truncate window (the #109140 flake class):
// owner.json exists but is still empty when the reader first sees it, and
// the racing writer publishes the owner content shortly after.
writeFileSync(path.join(lockPath, "owner.json"), "");
const owner = { pid: process.pid, checkout: mirror, startedAt: "racing" };
const writer = spawn(
"sh",
["-c", 'sleep 0.03; printf "%s\\n" "$OWNER_JSON" > "$LOCK_PATH/owner.json"'],
{
env: { ...process.env, LOCK_PATH: lockPath, OWNER_JSON: JSON.stringify(owner) },
stdio: "ignore",
},
);
try {
expect(acquireMaintenanceLock(mirror, lockPath)).toMatchObject({
acquired: false,
owner,
});
} finally {
writer.kill();
}
});
test("refuses dirty work without moving HEAD", () => {
const { mirror } = makeFixture();
const before = git(mirror, "rev-parse", "HEAD");
@@ -523,13 +523,13 @@ const descendant = spawn(process.execPath, [
"--eval",
${JSON.stringify(
`import { writeFileSync } from "node:fs";
writeFileSync(${JSON.stringify(readyPath)}, "ready");
process.on("SIGTERM", () => {
setTimeout(() => {
writeFileSync(${JSON.stringify(donePath)}, "done");
process.exit(0);
}, 75);
});
writeFileSync(${JSON.stringify(readyPath)}, "ready");
setInterval(() => {}, 1000);`,
)},
], { stdio: "ignore" });
@@ -575,11 +575,11 @@ const descendant = spawn(process.execPath, [
"-e",
${JSON.stringify(
`const fs = require("node:fs");
fs.writeFileSync(${JSON.stringify(descendantPidPath)}, String(process.pid));
process.on("SIGTERM", () => {
fs.writeFileSync(${JSON.stringify(descendantTermPath)}, "terminated");
process.exit(0);
});
fs.writeFileSync(${JSON.stringify(descendantPidPath)}, String(process.pid));
setInterval(() => {}, 1000);`,
)},
], { stdio: "ignore" });
@@ -654,12 +654,14 @@ setInterval(() => {}, 1000);
`
import fs from "node:fs";
fs.writeFileSync(${JSON.stringify(mockPidPath)}, String(process.pid));
process.stdout.write("mock-openai listening\\n");
// Handler before the readiness line: SIGTERM arrives once the gateway spawn
// fails, and a late-registered handler can lose it to the default disposition.
process.on("SIGTERM", () => {
fs.writeFileSync(${JSON.stringify(mockTermPath)}, "terminated");
process.exit(0);
});
fs.writeFileSync(${JSON.stringify(mockPidPath)}, String(process.pid));
process.stdout.write("mock-openai listening\\n");
setInterval(() => {}, 1000);
`,
);
@@ -765,11 +767,19 @@ process.exit(2);
await waitFor(() => output().includes("mock-openai listening"));
return;
}
await waitFor(() => fs.existsSync(gatewayGrandchildPidPath));
gatewayGrandchildPid = Number.parseInt(
fs.readFileSync(gatewayGrandchildPidPath, "utf8"),
10,
);
// Parse inside the poll: existsSync can observe writeFileSync's
// 0-byte open-truncate window, and a NaN pid would skip both the
// dead-check and the finally-block SIGKILL cleanup.
await waitFor(() => {
if (!fs.existsSync(gatewayGrandchildPidPath)) {
return false;
}
gatewayGrandchildPid = Number.parseInt(
fs.readFileSync(gatewayGrandchildPidPath, "utf8"),
10,
);
return Number.isInteger(gatewayGrandchildPid) && gatewayGrandchildPid > 1;
});
if (child.exitCode === null && child.signalCode === null) {
await new Promise<void>((resolve) => {
child.once("exit", () => resolve());
@@ -799,11 +809,15 @@ process.exit(2);
`#!/usr/bin/env node
import fs from "node:fs";
fs.writeFileSync(${JSON.stringify(recorderPidPath)}, String(process.pid));
// Arm the SIGTERM handler before publishing the pid file: the probe throws as
// soon as the pid file exists and recordProbeVideo SIGTERMs the recorder in
// its finally, so a handler installed after publish can lose that signal to
// the default disposition and recorder.term is never written.
process.on("SIGTERM", () => {
fs.writeFileSync(${JSON.stringify(recorderTermPath)}, "terminated");
process.exit(0);
});
fs.writeFileSync(${JSON.stringify(recorderPidPath)}, String(process.pid));
setInterval(() => {}, 1000);
`,
);