From 30fc0a1c606ea90a375fa25aafcefc77685689e5 Mon Sep 17 00:00:00 2001 From: kai392 Date: Thu, 30 Jul 2026 04:33:18 +0800 Subject: [PATCH] fix(miner): a rejecting captureError must not skip runCleanup()/exit in the crash handlers (#9688) process-lifecycle.ts calls itself "the single cleanup chokepoint", but its uncaughtException/unhandledRejection handlers `await captureError(...)` with no guard. The "never throws/rejects" contract on the injectable captureError option lived only in prose -- if an injected hook rejects (or throws synchronously), runCleanup() and exit(1) are both skipped: every registered SQLite handle stays open and unflushed, and on unhandledRejection the handler's own rejected promise re-enters the same handler. - Wrap the `await captureError(...)` in each handler in try/catch, logging a distinct `error capture failed: ...` message via the same injected log and existing describeError helper, so runCleanup() + exit(1) always run in their current order. - Update the captureError doc comment to document the catch-and-log behaviour instead of claiming it is "Never expected to throw/reject". - SIGINT/SIGTERM handlers and the production bin wiring are untouched. Three named regression tests: a rejecting captureError on each handler, and a synchronously-throwing one, each asserting the registered store's close() ran and exit received 1. All three fail against the current code. Closes #9688 Co-Authored-By: Claude Opus 4.8 --- .../loopover-miner/lib/process-lifecycle.ts | 21 ++++++-- test/unit/miner-process-lifecycle.test.ts | 52 +++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/packages/loopover-miner/lib/process-lifecycle.ts b/packages/loopover-miner/lib/process-lifecycle.ts index a89178ae0c..96eab63bea 100644 --- a/packages/loopover-miner/lib/process-lifecycle.ts +++ b/packages/loopover-miner/lib/process-lifecycle.ts @@ -23,8 +23,9 @@ export type InstallCliSignalHandlersOptions = { /** Called (in addition to `log`) for uncaughtException/unhandledRejection specifically -- not the clean * SIGINT/SIGTERM exits, which are not errors. AWAITED before the process exits, so it should both capture * AND flush (see captureMinerErrorAndFlush in bin/loopover-miner.js) -- a synchronous capture alone only - * queues the event, which process.exit() would then likely never deliver. No-op default. Never expected to - * throw/reject. */ + * queues the event, which process.exit() would then likely never deliver. No-op default. A synchronous throw + * or a rejected promise from this hook is caught and logged (`error capture failed: ...`) so that the cleanup + * sweep and the process exit always run -- a failing error sink can never leave the miner's stores open. */ captureError?: (error: unknown, context?: Record) => void | Promise; /** Reinstall even if handlers were already installed (mainly for tests). */ force?: boolean; @@ -119,16 +120,28 @@ export function installCliSignalHandlers(options: InstallCliSignalHandlersOption // not require these handlers to be synchronous: nothing exits the process until this handler itself calls // `exit()`, so awaiting first is safe. captureError's own default is a synchronous no-op, so `await`-ing it // is a harmless no-op for every caller that doesn't pass one. + // captureError is a public injectable option whose "never throws/rejects" contract lived only in prose. Enforce + // it HERE, at the chokepoint: a synchronous throw or a rejected promise from the hook is caught and logged so + // runCleanup() + exit() always run -- otherwise a failing error sink would leave every registered store open and + // (on unhandledRejection) the async handler's own rejection would re-enter this same handler. proc.on("uncaughtException", async (error: unknown) => { log(`loopover-miner: uncaught exception: ${describeError(error)}`); - await captureError(error, { kind: "uncaughtException" }); + try { + await captureError(error, { kind: "uncaughtException" }); + } catch (captureFailure) { + log(`loopover-miner: error capture failed: ${describeError(captureFailure)}`); + } runCleanup(); exit(1); }); proc.on("unhandledRejection", async (reason: unknown) => { log(`loopover-miner: unhandled promise rejection: ${describeError(reason)}`); - await captureError(reason, { kind: "unhandledRejection" }); + try { + await captureError(reason, { kind: "unhandledRejection" }); + } catch (captureFailure) { + log(`loopover-miner: error capture failed: ${describeError(captureFailure)}`); + } runCleanup(); exit(1); }); diff --git a/test/unit/miner-process-lifecycle.test.ts b/test/unit/miner-process-lifecycle.test.ts index ff9e4d5f2a..327a61a60f 100644 --- a/test/unit/miner-process-lifecycle.test.ts +++ b/test/unit/miner-process-lifecycle.test.ts @@ -240,6 +240,58 @@ describe("loopover-miner process lifecycle / crash-safety (#4826)", () => { expect(log.mock.calls.some((call) => String(call[0]).includes("cleanup boom"))).toBe(true); }); + it("REGRESSION (#9688): a REJECTING captureError on uncaughtException is caught -- cleanup still runs and exit(1) still fires", async () => { + const { proc, handlers, exit } = makeFakeProcess(); + const log = vi.fn(); + const store = { close: vi.fn() }; + registerCleanupResource(store); + const captureError = vi.fn(() => Promise.reject(new Error("sink flush failed"))); + installCliSignalHandlers({ process: proc, log, exit, captureError }); + + await handlers.get("uncaughtException")?.(new Error("kaboom")); + + expect(captureError).toHaveBeenCalledTimes(1); + expect(store.close).toHaveBeenCalledTimes(1); // the registered store was NOT left open + expect(cleanupResourceCount()).toBe(0); + expect(exit).toHaveBeenCalledWith(1); + expect(log).toHaveBeenCalledWith(expect.stringContaining("error capture failed: ")); + expect(log.mock.calls.some((call) => String(call[0]).includes("sink flush failed"))).toBe(true); + }); + + it("REGRESSION (#9688): a REJECTING captureError on unhandledRejection is caught -- cleanup still runs and exit(1) still fires", async () => { + const { proc, handlers, exit } = makeFakeProcess(); + const log = vi.fn(); + const store = { close: vi.fn() }; + registerCleanupResource(store); + const captureError = vi.fn(() => Promise.reject(new Error("sink flush failed"))); + installCliSignalHandlers({ process: proc, log, exit, captureError }); + + await handlers.get("unhandledRejection")?.("plain reason"); + + expect(captureError).toHaveBeenCalledTimes(1); + expect(store.close).toHaveBeenCalledTimes(1); + expect(cleanupResourceCount()).toBe(0); + expect(exit).toHaveBeenCalledWith(1); + expect(log).toHaveBeenCalledWith(expect.stringContaining("error capture failed: ")); + }); + + it("REGRESSION (#9688): a captureError that throws SYNCHRONOUSLY is also caught -- cleanup still runs and exit(1) still fires", async () => { + const { proc, handlers, exit } = makeFakeProcess(); + const log = vi.fn(); + const store = { close: vi.fn() }; + registerCleanupResource(store); + const captureError = vi.fn(() => { + throw new Error("sync sink boom"); + }); + installCliSignalHandlers({ process: proc, log, exit, captureError }); + + await handlers.get("uncaughtException")?.(new Error("kaboom")); + + expect(store.close).toHaveBeenCalledTimes(1); + expect(exit).toHaveBeenCalledWith(1); + expect(log.mock.calls.some((call) => String(call[0]).includes("sync sink boom"))).toBe(true); + }); + it("defaults to the real process when none is injected", () => { withRealProcessCleanup(() => { expect(installCliSignalHandlers({ log: vi.fn(), exit: vi.fn(), force: true })).toBe(true);