Skip to content

miner(lifecycle): a rejecting captureError skips runCleanup() and the process exit #9688

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

packages/loopover-miner/lib/process-lifecycle.ts is described by its own header as "the single cleanup chokepoint: local stores register themselves when opened ... and installCliSignalHandlers ... flushes/closes every still-open resource before exiting". Its two crash handlers:

proc.on("uncaughtException", async (error: unknown) => {
  log(`loopover-miner: uncaught exception: ${describeError(error)}`);
  await captureError(error, { kind: "uncaughtException" });
  runCleanup();
  exit(1);
});

(identically for unhandledRejection, lines 129-134.)

The "never rejects" requirement on captureError exists only as prose in the option's doc comment (lines 26-27: "Never expected to throw/reject."), with no try/finally enforcing it. If an injected captureError rejects — or throws synchronously — runCleanup() and exit(1) are both skipped: every registered SQLite handle stays open and unflushed, which is exactly the failure mode this module exists to prevent, and the process does not exit. On the unhandledRejection handler the async callback's own rejected promise re-enters the same handler, so it self-triggers.

The production binding does honour the contract today (bin/loopover-miner.ts:70-73 passes a Promise.all([captureMinerErrorAndFlush, captureMinerPostHogErrorAndFlush]), and both swallow internally) — which is precisely why the safety property should live in this chokepoint rather than in two other modules' implementations, especially since captureError is a public injectable option.

Requirements

  • Both handlers must wrap the await captureError(...) call in try/catch so that runCleanup() and exit(1) always run.
  • The catch must call the same injected log with a distinct message (loopover-miner: error capture failed: ${describeError(...)}), so the failure is not silent.
  • runCleanup() and exit(1) must run in their current order and exactly once on each handler invocation.
  • The doc comment on captureError (lines 22-28) must be updated to state that a rejection is caught and logged rather than "Never expected to throw/reject."
  • The two SIGINT/SIGTERM handlers (which do not call captureError) must be unchanged.

⚠️ Required pattern: a local try { await captureError(...) } catch (error) { log(...) } inside each handler, using the module's existing describeError helper. It does NOT satisfy this issue to wrap only one of the two handlers, to swap await for .catch(() => {}) without logging, to move the capture after runCleanup(), or to change the production bin/loopover-miner.ts wiring instead of hardening the seam.

Deliverables

  • Both the uncaughtException and unhandledRejection handlers wrap await captureError(...) in try/catch and log the capture failure.
  • The captureError option's doc comment no longer claims it is "Never expected to throw/reject" and instead documents the catch-and-log behaviour.
  • A new named regression test in test/unit/miner-process-lifecycle.test.ts installs handlers with an injected captureError that returns a rejected promise, fires the uncaughtException listener, and asserts a registered resource's close() was called and the injected exit received 1.
  • A second case does the same for unhandledRejection.
  • A third case asserts an injected captureError that throws synchronously is also caught.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example guarding uncaughtException only, or adding the guards without the rejected-promise regression tests — does not resolve this issue.

Test Coverage Requirements

packages/loopover-miner/lib/**/*.ts IS inside Codecov's coverage.include in vitest.config.ts, so the 99%+ branch-counted codecov/patch gate applies exactly as for src/**. Both arms of each new try/catch need a test (capture resolves, capture rejects) in both handlers, plus the synchronous-throw arm. The tests must be named regression tests that fail against the current code.

Expected Outcome

An uncaught exception or unhandled rejection always closes every registered local store and exits 1, even when the injected error sink itself fails — the guarantee process-lifecycle.ts's own header claims.

Links & Resources

packages/loopover-miner/lib/process-lifecycle.ts:20-30, :100-136, packages/loopover-miner/bin/loopover-miner.ts:70-73, packages/loopover-miner/lib/sentry.ts.

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions