From 7550bd887c11773b67f017427ecd0db908fb3808 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 16 Jul 2026 14:46:40 +0000 Subject: [PATCH] fix(utils): isolated fatal logging teardown Made fatal reporting bypass revoked stderr streams and armed a referenced forced-exit watchdog around bounded cleanup. Separated rotating log and audit namespaces by PID and disabled compression pipelines so concurrent TUI processes cannot race shared rotation state. Fixes #5716 --- packages/coding-agent/CHANGELOG.md | 4 ++ .../coding-agent/src/debug/report-bundle.ts | 2 +- packages/utils/CHANGELOG.md | 4 ++ packages/utils/src/dirs.ts | 6 +-- packages/utils/src/logger.ts | 6 +-- packages/utils/src/postmortem.ts | 37 ++++++++------ .../utils/test/logger-multiprocess.test.ts | 51 +++++++++++++++++++ .../test/postmortem-cleanup-error.test.ts | 18 +++++++ 8 files changed, 105 insertions(+), 23 deletions(-) create mode 100644 packages/utils/test/logger-multiprocess.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 0e753ced7..a136d610d 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed orphaned TUI processes with revoked terminal descriptors remaining alive after a fatal error and amplifying shared log-rotation races into runaway memory, file-descriptor, swap, and disk consumption ([#5716](https://github.com/can1357/oh-my-pi/issues/5716)). + ## [17.0.1] - 2026-07-16 ### Changed diff --git a/packages/coding-agent/src/debug/report-bundle.ts b/packages/coding-agent/src/debug/report-bundle.ts index f633ca64b..9119b70b1 100644 --- a/packages/coding-agent/src/debug/report-bundle.ts +++ b/packages/coding-agent/src/debug/report-bundle.ts @@ -241,7 +241,7 @@ export async function getLogText(): Promise { return readLastLines(getLogPath(), MAX_LOG_LINES); } -const LOG_FILE_PATTERN = new RegExp(`^${APP_NAME}\\.(\\d{4}-\\d{2}-\\d{2})\\.log$`); +const LOG_FILE_PATTERN = new RegExp(`^${APP_NAME}\\.(\\d{4}-\\d{2}-\\d{2})\\.\\d+\\.log$`); export async function createDebugLogSource(): Promise { const logsDir = getLogsDir(); diff --git a/packages/utils/CHANGELOG.md b/packages/utils/CHANGELOG.md index 1bb87b599..4079bc80d 100644 --- a/packages/utils/CHANGELOG.md +++ b/packages/utils/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed fatal cleanup failing to reach `process.exit()` when terminal stderr is revoked, and isolated rotating log files/audit state per process to prevent concurrent OMP instances from racing compression and rotation ([#5716](https://github.com/can1357/oh-my-pi/issues/5716)). + ## [17.0.1] - 2026-07-16 ### Fixed diff --git a/packages/utils/src/dirs.ts b/packages/utils/src/dirs.ts index d0b63af69..cdddbbfdf 100644 --- a/packages/utils/src/dirs.ts +++ b/packages/utils/src/dirs.ts @@ -513,9 +513,9 @@ export function getLogsDir(): string { return dirs.rootSubdir("logs", "state"); } -/** Get the path to a dated log file (~/.omp/logs/omp.YYYY-MM-DD.log). */ -export function getLogPath(date = new Date()): string { - return path.join(getLogsDir(), `${APP_NAME}.${date.toISOString().slice(0, 10)}.log`); +/** Get this process's dated log path (~/.omp/logs/omp.YYYY-MM-DD.PID.log). */ +export function getLogPath(date = new Date(), pid = process.pid): string { + return path.join(getLogsDir(), `${APP_NAME}.${date.toISOString().slice(0, 10)}.${pid}.log`); } /** diff --git a/packages/utils/src/logger.ts b/packages/utils/src/logger.ts index 836756363..5d4f60faf 100644 --- a/packages/utils/src/logger.ts +++ b/packages/utils/src/logger.ts @@ -1,7 +1,7 @@ /** * Centralized logger for omp. * - * Default: rotating `~/.omp/logs/omp..log`, no console output (writing + * Default: rotating `~/.omp/logs/omp...log`, no console output (writing * to stdout/stderr would corrupt the TUI). Long-running headless services * (the auth broker, etc.) call {@link setTransports} to swap in a console * transport so a process supervisor (pm2, journald, k8s) captures the logs. @@ -76,11 +76,11 @@ function getLogFormat(): winston.Logform.Format { function makeFileTransport(dir?: string): winston.transport { return new DailyRotateFile({ dirname: ensureDir(dir ?? getLogsDir()), - filename: "omp.%DATE%.log", + filename: `omp.%DATE%.${process.pid}.log`, datePattern: "YYYY-MM-DD", maxSize: "10m", maxFiles: 5, - zippedArchive: true, + zippedArchive: false, }); } diff --git a/packages/utils/src/postmortem.ts b/packages/utils/src/postmortem.ts index c2784c1e4..9377e9cbd 100644 --- a/packages/utils/src/postmortem.ts +++ b/packages/utils/src/postmortem.ts @@ -5,6 +5,8 @@ * in response to process exit, signals, or fatal exceptions. It is intended to * allow reliably releasing resources or shutting down subprocesses, files, sockets, etc. */ + +import * as fs from "node:fs"; import inspector from "node:inspector"; import { isMainThread } from "node:worker_threads"; import { logger } from "."; @@ -68,7 +70,6 @@ function runCleanup(reason: Reason): Promise { cleanupStage = "complete"; deadline.resolve(); }, CLEANUP_DEADLINE_MS); - deadlineTimer.unref(); cleanupPromise = Promise.race([cleanupSettled, deadline.promise]).finally(() => { clearTimeout(deadlineTimer); }); @@ -175,6 +176,23 @@ function formatFatalError(label: string, err: Error): string { return `\n[${label}] ${name}: ${message}${formattedStack}\n`; } +async function exitAfterFatal(label: string, logMessage: string, err: Error, reason: Reason): Promise { + const forcedExit = setTimeout(() => process.exit(1), CLEANUP_DEADLINE_MS); + try { + restoreTerminalStderr(); + // A revoked terminal can make stream writes raise another fatal error. Use + // the descriptor directly so failure stays synchronous and contained. + try { + fs.writeSync(2, formatFatalError(label, err)); + } catch {} + logger.error(logMessage, { err }); + await runCleanup(reason); + } finally { + clearTimeout(forcedExit); + process.exit(1); + } +} + if (isMainThread) { process .on("SIGINT", async () => { @@ -193,15 +211,7 @@ if (isMainThread) { logger.warn("Ignoring expected cleanup exception", { err }); return; } - // fd 2 may be redirected to the log while a TUI owns the terminal - // (stderr-guard); re-point it at the real terminal so the fatal - // report is visible. Terminal modes are restored moments later by - // the terminal-restore cleanup callback inside runCleanup(). - restoreTerminalStderr(); - process.stderr.write(formatFatalError("Uncaught Exception", err)); - logger.error("Uncaught exception", { err }); - await runCleanup(Reason.UNCAUGHT_EXCEPTION); - process.exit(1); + await exitAfterFatal("Uncaught Exception", "Uncaught exception", err, Reason.UNCAUGHT_EXCEPTION); }) .on("unhandledRejection", async reason => { const err = reason instanceof Error ? reason : new Error(String(reason)); @@ -237,12 +247,7 @@ if (isMainThread) { }); } } - // See uncaughtException above: surface the report on the real stderr. - restoreTerminalStderr(); - process.stderr.write(formatFatalError("Unhandled Rejection", err)); - logger.error("Unhandled rejection", { err }); - await runCleanup(Reason.UNHANDLED_REJECTION); - process.exit(1); + await exitAfterFatal("Unhandled Rejection", "Unhandled rejection", err, Reason.UNHANDLED_REJECTION); }) .on("exit", async () => { void runCleanup(Reason.EXIT); // fire and forget (exit imminent) diff --git a/packages/utils/test/logger-multiprocess.test.ts b/packages/utils/test/logger-multiprocess.test.ts new file mode 100644 index 000000000..c12997a7d --- /dev/null +++ b/packages/utils/test/logger-multiprocess.test.ts @@ -0,0 +1,51 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { pathToFileURL } from "node:url"; + +const loggerModuleUrl = pathToFileURL(path.join(import.meta.dir, "../src/logger.ts")).href; +const roots: string[] = []; + +afterEach(async () => { + await Promise.all(roots.splice(0).map(root => fs.rm(root, { recursive: true, force: true }))); +}); + +async function makeProbe(logsDir: string): Promise { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "omp-logger-probe-")); + roots.push(root); + const probePath = path.join(root, "probe.ts"); + await Bun.write( + probePath, + `import { info, setTransports } from ${JSON.stringify(loggerModuleUrl)};\n` + + `setTransports({ file: ${JSON.stringify(logsDir)} });\n` + + `info("multiprocess probe");\n` + + `setTransports({ file: false });\n`, + ); + return probePath; +} + +async function waitForExit(proc: Bun.Subprocess): Promise { + const code = await proc.exited; + return code; +} + +describe("multiprocess file logging", () => { + it("gives concurrent processes independent rotation files and audit state", async () => { + const logsDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-logger-output-")); + roots.push(logsDir); + const probePath = await makeProbe(logsDir); + const processes = [ + Bun.spawn([process.execPath, probePath], { stdout: "ignore", stderr: "pipe" }), + Bun.spawn([process.execPath, probePath], { stdout: "ignore", stderr: "pipe" }), + ]; + + expect(await Promise.all(processes.map(waitForExit))).toEqual([0, 0]); + const entries = await fs.readdir(logsDir); + const datedPrefix = `omp.${new Date().toISOString().slice(0, 10)}`; + for (const proc of processes) { + expect(entries).toContain(`${datedPrefix}.${proc.pid}.log`); + } + expect(entries.filter(name => name.endsWith("-audit.json"))).toHaveLength(2); + }); +}); diff --git a/packages/utils/test/postmortem-cleanup-error.test.ts b/packages/utils/test/postmortem-cleanup-error.test.ts index 6b3543f39..eac312ac9 100644 --- a/packages/utils/test/postmortem-cleanup-error.test.ts +++ b/packages/utils/test/postmortem-cleanup-error.test.ts @@ -121,6 +121,24 @@ describe("postmortem expected cleanup errors", () => { expect(result.stderr).toContain("[Unhandled Rejection] Error: unexpected cleanup rejection"); }); + it("exits after an uncaught exception when terminal stderr is revoked", async () => { + const result = await runPostmortemProbe(` + import { spyOn } from "bun:test"; + import "${postmortemModuleUrl}"; + + spyOn(process.stderr, "write").mockImplementation(() => { + throw Object.assign(new Error("terminal revoked"), { code: "EIO" }); + }); + queueMicrotask(() => { + throw new Error("fatal after disconnect"); + }); + await Promise.withResolvers().promise; + `); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("[Uncaught Exception] Error: fatal after disconnect"); + }); + it("releases manual cleanup at the deadline even when a callback never settles", async () => { const result = await runPostmortemProbe(` import { vi } from "bun:test";