fix: review feedback — signal handling, cancel race, stderr consistency
Address issues found by code review: 1. Bug: timeout/signal-killed child reported as 'completed' with exit code 0 because close handler ignored the signal parameter. Now treats any signal termination as timed_out. 2. Bug: cancelled job gets overwritten by watcher on child exit. The watcher now re-reads the job file before writing and skips if the status has been changed to 'cancelled'. 3. Inconsistency: watcher path skipped stderr noise filtering. Added filterStderrNoise to the watcher (duplicated from execute.ts to keep the watcher self-contained). 4. getJobResult now guards against missing result field instead of using non-null assertion.
This commit is contained in:
@@ -10,7 +10,29 @@
|
|||||||
*/
|
*/
|
||||||
import { spawn } from "node:child_process";
|
import { spawn } from "node:child_process";
|
||||||
import { readFileSync, writeFileSync } from "node:fs";
|
import { readFileSync, writeFileSync } from "node:fs";
|
||||||
import type { JobRecord, ExecResult, JobStatus } from "./types.js";
|
import type { JobRecord, ExecResult, JobStatus, ClientName } from "./types.js";
|
||||||
|
|
||||||
|
// Must import CLIENT_ARGS to know the client command mapping
|
||||||
|
// And the noise filter for consistent stderr handling
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Known stderr noise patterns per client (duplicated from execute.ts to keep
|
||||||
|
* the watcher self-contained with no runtime dependency on execute.ts).
|
||||||
|
*/
|
||||||
|
const STDERR_NOISE_PATTERNS: Partial<Record<ClientName, RegExp[]>> = {
|
||||||
|
codex: [
|
||||||
|
/^\d{4}-\d{2}-\d{2}T[\d:.]+Z\s+ERROR\s+codex_core::util:\s+ReasoningSummary\w*\s+/,
|
||||||
|
],
|
||||||
|
};
|
||||||
|
|
||||||
|
function filterStderrNoise(client: ClientName, stderr: string, exitCode: number): string {
|
||||||
|
if (exitCode !== 0) return stderr;
|
||||||
|
const patterns = STDERR_NOISE_PATTERNS[client];
|
||||||
|
if (!patterns) return stderr;
|
||||||
|
const lines = stderr.split("\n");
|
||||||
|
const filtered = lines.filter((line) => !patterns.some((p) => p.test(line)));
|
||||||
|
return filtered.join("\n").replace(/\n+$/, "");
|
||||||
|
}
|
||||||
|
|
||||||
const jobFile = process.argv[2];
|
const jobFile = process.argv[2];
|
||||||
const command = process.argv[3];
|
const command = process.argv[3];
|
||||||
@@ -69,12 +91,17 @@ function finalize(status: JobStatus, result?: ExecResult, error?: string) {
|
|||||||
...record,
|
...record,
|
||||||
status,
|
status,
|
||||||
stdout,
|
stdout,
|
||||||
stderr,
|
stderr: result ? filterStderrNoise(record.client, stderr, result.exitCode) : stderr,
|
||||||
result: result ? { ...result, durationMs } : undefined,
|
result: result ? { ...result, durationMs } : undefined,
|
||||||
error,
|
error,
|
||||||
completedAt,
|
completedAt,
|
||||||
};
|
};
|
||||||
try {
|
try {
|
||||||
|
// Check if the job was cancelled while we were running
|
||||||
|
const current = JSON.parse(readFileSync(jobFile, "utf-8")) as JobRecord;
|
||||||
|
if (current.status === "cancelled") {
|
||||||
|
return; // Don't overwrite a cancelled job
|
||||||
|
}
|
||||||
writeFileSync(jobFile, JSON.stringify(finalRecord, null, 2));
|
writeFileSync(jobFile, JSON.stringify(finalRecord, null, 2));
|
||||||
} catch { /* best effort */ }
|
} catch { /* best effort */ }
|
||||||
process.exit(0);
|
process.exit(0);
|
||||||
@@ -84,8 +111,8 @@ child.on("error", (err: NodeJS.ErrnoException) => {
|
|||||||
finalize("failed", undefined, err.message);
|
finalize("failed", undefined, err.message);
|
||||||
});
|
});
|
||||||
|
|
||||||
child.on("close", (code: number | null) => {
|
child.on("close", (code: number | null, signal: NodeJS.Signals | null) => {
|
||||||
if (timedOut) {
|
if (timedOut || signal) {
|
||||||
finalize("timed_out", {
|
finalize("timed_out", {
|
||||||
stdout, stderr, exitCode: -1, client: record.client, durationMs: 0,
|
stdout, stderr, exitCode: -1, client: record.client, durationMs: 0,
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -176,7 +176,10 @@ export function getJobResult(jobId: string, options: JobOperationsOptions = {}):
|
|||||||
if (job.status !== "completed") {
|
if (job.status !== "completed") {
|
||||||
throw new JobResultUnavailableError(jobId, job.status);
|
throw new JobResultUnavailableError(jobId, job.status);
|
||||||
}
|
}
|
||||||
return job.result!;
|
if (!job.result) {
|
||||||
|
throw new JobResultUnavailableError(jobId, "completed");
|
||||||
|
}
|
||||||
|
return job.result;
|
||||||
}
|
}
|
||||||
|
|
||||||
export function cancelJob(jobId: string, options: JobOperationsOptions = {}): void {
|
export function cancelJob(jobId: string, options: JobOperationsOptions = {}): void {
|
||||||
|
|||||||
Reference in New Issue
Block a user