mirror of
https://github.com/openai/codex.git
synced 2026-10-28 18:58:46 +00:00
Preserve child signal failures in the npm wrapper (#57352)
## Why The npm wrapper can exit successfully after its native child is terminated by a signal if Node ignores the re-emitted signal or a forwarding handler intercepts it. ## What changed Set a fallback exit status of `128 + signal number` and remove listeners for the terminating signal before re-emitting it, preserving signal termination where supported. ## Testing Add a non-Windows integration test using a built Codex binary to cover numeric argument-error exits, forwarded `SIGHUP` termination, and the fallback exit status for `SIGXFSZ`, which Node ignores by default. GitOrigin-RevId: cad18a47aa50fd78517c367f8d8aae83bbbf7843
This commit is contained in:
committed by
copyberry
parent
b7f7e7f7e8
commit
9510f36df2
@@ -4,6 +4,7 @@
|
||||
import { spawn } from "node:child_process";
|
||||
import { existsSync, readFileSync, realpathSync } from "fs";
|
||||
import { createRequire } from "node:module";
|
||||
import { constants } from "node:os";
|
||||
import path from "path";
|
||||
import { fileURLToPath } from "url";
|
||||
|
||||
@@ -287,8 +288,9 @@ const childResult = await new Promise((resolve) => {
|
||||
});
|
||||
|
||||
if (childResult.type === "signal") {
|
||||
// Re-emit the same signal so that the parent terminates with the expected
|
||||
// semantics (this also sets the correct exit code of 128 + n).
|
||||
// Preserve a failure exit code even if Node ignores the re-emitted signal.
|
||||
process.exitCode = 128 + constants.signals[childResult.signal];
|
||||
process.removeAllListeners(childResult.signal);
|
||||
process.kill(process.pid, childResult.signal);
|
||||
} else {
|
||||
process.exit(childResult.exitCode);
|
||||
|
||||
@@ -0,0 +1,120 @@
|
||||
// Run with CODEX_TEST_BINARY=/absolute/path/to/codex node --test codex-cli/tests/exit-status.test.mjs.
|
||||
import assert from "node:assert/strict";
|
||||
import { execFileSync, spawn, spawnSync } from "node:child_process";
|
||||
import { once } from "node:events";
|
||||
import {
|
||||
copyFile,
|
||||
mkdir,
|
||||
mkdtemp,
|
||||
rm,
|
||||
symlink,
|
||||
writeFile,
|
||||
} from "node:fs/promises";
|
||||
import { constants, tmpdir } from "node:os";
|
||||
import path from "node:path";
|
||||
import { createInterface } from "node:readline";
|
||||
import test from "node:test";
|
||||
|
||||
test(
|
||||
"the npm wrapper preserves numeric exits and child signals",
|
||||
{
|
||||
skip: process.platform === "win32",
|
||||
},
|
||||
async () => {
|
||||
assert.ok(
|
||||
process.env.CODEX_TEST_BINARY,
|
||||
"Set CODEX_TEST_BINARY to a built Codex binary",
|
||||
);
|
||||
const directory = await mkdtemp(path.join(tmpdir(), "codex-npm-exit-"));
|
||||
let child;
|
||||
let killTimer;
|
||||
try {
|
||||
const arch = process.arch === "arm64" ? "aarch64" : "x86_64";
|
||||
const target = `${arch}-${process.platform === "darwin" ? "apple-darwin" : "unknown-linux-musl"}`;
|
||||
const bin = path.join(directory, "vendor", target, "bin");
|
||||
const home = path.join(directory, "home");
|
||||
await mkdir(bin, { recursive: true });
|
||||
await mkdir(home);
|
||||
await mkdir(path.join(directory, "bin"));
|
||||
await writeFile(
|
||||
path.join(directory, "package.json"),
|
||||
'{"type":"module"}',
|
||||
);
|
||||
const wrapper = path.join(directory, "bin", "codex.js");
|
||||
await copyFile(new URL("../bin/codex.js", import.meta.url), wrapper);
|
||||
await symlink(
|
||||
path.resolve(process.env.CODEX_TEST_BINARY),
|
||||
path.join(bin, "codex"),
|
||||
);
|
||||
const env = { ...process.env, CODEX_HOME: home, CODEX_SQLITE_HOME: home };
|
||||
|
||||
// A normal CLI argument error must retain its numeric exit status.
|
||||
const invalid = spawnSync(
|
||||
process.execPath,
|
||||
[wrapper, "--invalid-codex-option"],
|
||||
{ env, timeout: 30_000, killSignal: "SIGKILL" },
|
||||
);
|
||||
assert.equal(invalid.error, undefined);
|
||||
assert.deepEqual([invalid.status, invalid.signal], [2, null]);
|
||||
|
||||
for (const signal of ["SIGHUP", "SIGXFSZ"]) {
|
||||
// Wait for the real migration prompt before terminating a process.
|
||||
child = spawn(process.execPath, [wrapper, "migrate-nfs-db"], {
|
||||
env,
|
||||
detached: true,
|
||||
});
|
||||
const processGroup = child.pid;
|
||||
killTimer = setTimeout(() => {
|
||||
try {
|
||||
process.kill(-processGroup, "SIGKILL");
|
||||
} catch (error) {
|
||||
if (error.code !== "ESRCH") throw error;
|
||||
}
|
||||
}, 30_000);
|
||||
const exited = once(child, "exit");
|
||||
const lines = createInterface({ input: child.stderr });
|
||||
let prompted = false;
|
||||
for await (const line of lines) {
|
||||
if (line.endsWith("[y/N]:")) {
|
||||
prompted = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
assert.ok(prompted, "migration must reach its confirmation prompt");
|
||||
if (signal === "SIGHUP") {
|
||||
// The wrapper must forward the signal and retain signal termination.
|
||||
child.kill(signal);
|
||||
assert.deepEqual(await exited, [null, signal]);
|
||||
} else {
|
||||
// A native child can die from a signal that Node ignores by default.
|
||||
const processes = execFileSync("ps", ["-axo", "pid=,ppid="], {
|
||||
encoding: "utf8",
|
||||
});
|
||||
const children = processes
|
||||
.trim()
|
||||
.split("\n")
|
||||
.map((line) => line.trim().split(/\s+/).map(Number))
|
||||
.filter(([, parent]) => parent === child.pid);
|
||||
assert.equal(
|
||||
children.length,
|
||||
1,
|
||||
"wrapper must have one native child",
|
||||
);
|
||||
process.kill(children[0][0], signal);
|
||||
assert.deepEqual(await exited, [
|
||||
128 + constants.signals[signal],
|
||||
null,
|
||||
]);
|
||||
}
|
||||
clearTimeout(killTimer);
|
||||
}
|
||||
} finally {
|
||||
clearTimeout(killTimer);
|
||||
if (child && child.exitCode === null && child.signalCode === null) {
|
||||
process.kill(-child.pid, "SIGKILL");
|
||||
await once(child, "exit");
|
||||
}
|
||||
await rm(directory, { recursive: true, force: true });
|
||||
}
|
||||
},
|
||||
);
|
||||
Reference in New Issue
Block a user