Skip to content

fix(miner): a rejecting captureError must not skip runCleanup()/exit in the crash handlers (#9688) - #9902

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
kai392:fix/9688-capture-error-guard
Jul 29, 2026
Merged

fix(miner): a rejecting captureError must not skip runCleanup()/exit in the crash handlers (#9688)#9902
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
kai392:fix/9688-capture-error-guard

Conversation

@kai392

@kai392 kai392 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What

process-lifecycle.ts calls itself "the single cleanup chokepoint", yet its uncaughtException / unhandledRejection handlers await captureError(...) with no guard. The "never throws/rejects" contract on the public injectable captureError option existed only as prose. If an injected hook rejects — or throws synchronously — runCleanup() and exit(1) are both skipped: every registered SQLite handle stays open and unflushed (the exact failure this module exists to prevent), and on unhandledRejection the handler's own rejected promise re-enters the same handler.

How

  • Wrap the await captureError(...) in both handlers in try/catch, logging a distinct loopover-miner: error capture failed: ... message via the same injected log and the module's existing describeError helper, so runCleanup() + exit(1) always run in their current order and exactly once.
  • Update the captureError doc comment to document the catch-and-log behaviour instead of claiming it is "Never expected to throw/reject".
  • The SIGINT/SIGTERM handlers and the production bin/loopover-miner.ts wiring are untouched.

Tests

Three named regression tests in test/unit/miner-process-lifecycle.test.ts, each failing against the current code:

  1. A rejecting captureError on uncaughtException — the registered store's close() still ran, exit(1) fired, and the failure was logged.
  2. The same on unhandledRejection.
  3. A captureError that throws synchronously is also caught.

These cover the new catch arm of both handlers plus the synchronous-throw path; the resolve arm is already covered by the existing #6011 tests.

Closes #9688

@kai392
kai392 requested a review from JSONbored as a code owner July 29, 2026 20:28
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-29 20:37:03 UTC

2 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR wraps the `await captureError(...)` call in both `uncaughtException` and `unhandledRejection` handlers in try/catch, ensuring a rejecting or synchronously-throwing injected captureError hook can no longer skip runCleanup() and exit(1). The fix is correctly placed at the actual chokepoint (the await site itself, not some downstream symptom), the catch logs via the existing `describeError` helper and injected `log`, and the doc comment is updated to match the new contract. Three new regression tests directly exercise the rejecting and synchronously-throwing paths for both handlers, and they plausibly fail against pre-diff code since no try/catch previously existed around the await.

Nits — 4 non-blocking
  • The two `try { await captureError... } catch { log(...) }` blocks in process-lifecycle.ts:132-150 are identical in shape aside from the `kind` field; a small shared helper (e.g. `safeCaptureError(error, context)`) would remove the duplication, though the current duplication is minor and readable.
  • The new tests assert `log.mock.calls.some(...)` for message-content checks (e.g. 'sink flush failed') alongside `toHaveBeenCalledWith(expect.stringContaining(...))` for the prefix check — consistent with the existing test style in the file, so not a real problem, just worth keeping in mind if the log format ever changes.
  • Consider extracting the repeated try/catch-and-log pattern around `captureError` into a small local helper function to avoid the two near-identical blocks in `installCliSignalHandlers` (process-lifecycle.ts:132-150).
  • The doc comment update accurately reflects the new catch-and-log behavior; no further changes needed there.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9688
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 89 registered-repo PR(s), 43 merged, 7 issue(s).
Contributor context ✅ Confirmed Gittensor contributor kai392; Gittensor profile; 89 PR(s), 7 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff wraps both uncaughtException and unhandledRejection handlers' captureError calls in try/catch that logs a distinct 'error capture failed' message via the injected log/describeError before runCleanup()/exit(1) run, updates the doc comment to reflect catch-and-log behavior instead of the 'never throws/rejects' claim, and adds three named regression tests covering rejecting captureError on b

Review context
  • Author: kai392
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, Cuda, JavaScript, Kotlin, MDX, Perl, Ruby, TypeScript
  • Official Gittensor activity: 89 PR(s), 7 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

…in the crash handlers (JSONbored#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 JSONbored#9688

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 29, 2026
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.31%. Comparing base (133ff83) to head (30fc0a1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9902      +/-   ##
==========================================
+ Coverage   79.29%   79.31%   +0.01%     
==========================================
  Files         281      282       +1     
  Lines       58566    58616      +50     
  Branches     6785     6791       +6     
==========================================
+ Hits        46440    46490      +50     
  Misses      11843    11843              
  Partials      283      283              
Flag Coverage Δ
backend 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-miner/lib/process-lifecycle.ts 100.00% <100.00%> (ø)

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 36dd1c8 into JSONbored:main Jul 29, 2026
8 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 30, 2026
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants