Skip to content

feat(miner): add signal and crash handling to the CLI (#4826) - #5484

Merged
loopover-orb[bot] merged 3 commits into
JSONbored:mainfrom
andriypolanski:feat/miner-cli-signal-handling-4826
Jul 12, 2026
Merged

feat(miner): add signal and crash handling to the CLI (#4826)#5484
loopover-orb[bot] merged 3 commits into
JSONbored:mainfrom
andriypolanski:feat/miner-cli-signal-handling-4826

Conversation

@andriypolanski

Copy link
Copy Markdown
Contributor

Closes #4826.

Summary

The miner CLI had no process-level signal handling anywhere — no SIGINT/SIGTERM/uncaughtException/unhandledRejection handlers. The entrypoint dispatches through a chain of bare process.exit() calls with no cleanup hook, so an interrupted run (Ctrl-C, systemctl stop, or an uncaught error) could die mid-write and leave whatever local SQLite ledger it was touching in an undefined state.

This adds a single cleanup chokepoint and wires it in once at CLI startup, covering every subcommand — without touching any command's business logic (cleanup only, per the issue boundary).

What changed

  • New packages/gittensory-miner/lib/process-lifecycle.js — the crash-safety module:
    • registerCleanupResource(resource) / closeAllCleanupResources() — an ordered registry of closable resources (a { close() } store or a plain function). Returns an idempotent unregister handle; each close is individually try/caught so one failing close can't strand the others.
    • installCliSignalHandlers(options) — installs handlers once (idempotent; force for tests). On SIGINT/SIGTERM: close all resources, then exit with the conventional 128 + signal code (130 / 143). On uncaughtException/unhandledRejection: log the error and exit non-zero (1) instead of crashing silently. Every dependency (process, log, exit) is injectable, so the handlers are fully unit-testable without signalling the test runner.
    • Plus process-lifecycle.d.ts types and test-only helpers (cleanupResourceCount, resetProcessLifecycleForTesting).
  • lib/local-store.jsopenLocalStoreDb now auto-registers every opened store with the cleanup registry and wraps close() to unregister first. This is the DRY chokepoint every local ledger (run-state, claim, portfolio-queue, event) already funnels through, so every ledger is covered automatically. The happy path never double-closes, and a long-running loop never accumulates stale handles.
  • bin/gittensory-miner.js — calls installCliSignalHandlers() once at the top, before any dispatch, so all subcommands (including the local status/doctor/metrics fast paths) are covered.
  • package.json — added lib/process-lifecycle.js to the build node --check list.
  • README.md — documented the crash-safety behavior in the "Local storage" section.

Acceptance criteria

  • Signal handlers registered once at CLI startup, covering every subcommand — installed in bin/gittensory-miner.js before dispatch; idempotent.
  • A defined, tested "safe exit" for each ledger — every store opened via local-store.js is registered and closed on exit; unit-tested against real node:sqlite stores.
  • SIGINT/SIGTERM mid-run leaves every ledger valid/non-corrupt — handler closes all open stores (flushing SQLite) before exiting 130/143.
  • An uncaught exception is logged and exits non-zerouncaughtException/unhandledRejection handlers log the stack/reason and exit(1).

Tests

  • New test/unit/miner-process-lifecycle.test.ts — full-branch coverage of the registry, each signal/error handler (via a fake process whose listeners are invoked directly), default vs. injected log/exit, exit codes 130/143/1, Error-with-stack / Error-without-stack / non-Error reason rendering, idempotency + force, cleanup-error reporting, and the default-real-process branch (listeners cleaned up afterward).
  • Extended test/unit/miner-local-store.test.ts — a store opened via openLocalStoreDb is registered, unregisters on normal close(), and is actually closed by closeAllCleanupResources() at crash time (real :memory: node:sqlite DB).

Scope / boundaries

  • Cleanup only — no command's business logic changed.
  • bin/** is not in the Codecov include globs (only packages/gittensory-miner/lib/**/*.js), so the one-line entrypoint wiring carries no patch-coverage burden; the measured logic lives in process-lifecycle.js + the local-store.js change, both fully unit-tested.

Verification notes

  • node --check passes for all changed JS; no linter errors.
  • Because process-lifecycle.js is pure JS (no node:sqlite), I ran the real compiled module through a standalone script exercising every registry/handler/branch — all green — in addition to the Vitest suite.
  • Run npm run test:ci + npm run test:coverage under Node 22 before pushing (the Vitest gate and node:sqlite-backed local-store tests can't run in this Node 18 sandbox) — the loopover gate is one-shot auto-merge/close.

@andriypolanski
andriypolanski marked this pull request as draft July 12, 2026 22:52
@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb loopover-orb Bot added the gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. label Jul 12, 2026
@loopover-orb

loopover-orb Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-12 23:14:30 UTC

7 files · 2 AI reviewers · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
Adds a single crash-safety chokepoint (`process-lifecycle.js`) with signal/uncaughtException/unhandledRejection handling, wires it once at CLI startup, and threads every `openLocalStoreDb` call through automatic registration/unregistration on close. The implementation is correct: idempotent install, per-resource try/catch during cleanup so one failing close can't strand the others, and the wrapped `close()` unregisters first so the happy path never double-closes. Test coverage is thorough (both signal codes, uncaught exception/rejection with and without stack, injected vs default process/log/exit, cleanup-failure logging), and this closes #4826 cleanly without touching any command's business logic.

Nits — 5 non-blocking
  • packages/gittensory-miner/lib/process-lifecycle.js:12 the `128 + signal` magic number could be a named constant (e.g. `SIGNAL_BASE_EXIT_CODE = 128`) for clarity, though the comment already explains it.
  • packages/gittensory-miner/lib/process-lifecycle.js:59 `closeAllCleanupResources` calls `resource.close()` synchronously without awaiting; if a future resource's `close()` returns a promise (unlike the current synchronous SQLite `DatabaseSync.close`), cleanup could race with `exit()`.
  • test/unit/miner-process-lifecycle.test.ts is comprehensive; consider also asserting the `handlersInstalled` module singleton is truly per-process (tests reset it manually, which is fine, but worth a one-line comment on why `force` exists beyond tests).
  • Consider documenting in process-lifecycle.js that `closeAllCleanupResources` assumes synchronous `close()` implementations, since that's an implicit contract the current single caller (`local-store.js`) relies on.
  • nit: `packages/gittensory-miner/lib/process-lifecycle.js:67` uses `force` to install another full listener set without removing the old one, which is fine for the current fake-process tests but worth documenting as test-only behavior or making it replace listeners if you expect real reuse.
Signal Result Evidence
Code review ✅ No blockers 2 reviewers, synthesized
Linked issue ✅ Linked #4826
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: 154 registered-repo PR(s), 103 merged, 29 issue(s).
Contributor context ✅ Confirmed Gittensor contributor andriypolanski; Gittensor profile; 154 PR(s), 29 issue(s).
Gate result ✅ Passing No configured blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence. LLM value judgment: moderate — The diff directly addresses the linked CLI shutdown gap with a single reusable cleanup chokepoint and meaningful tests across both the lifecycle helper and SQLite store integration.
Review context
  • Author: andriypolanski
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 154 PR(s), 29 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.
[BETA] Chat with Gittensory

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

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

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.74%. Comparing base (f0426b5) to head (c057643).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5484   +/-   ##
=======================================
  Coverage   94.74%   94.74%           
=======================================
  Files         563      564    +1     
  Lines       44823    44869   +46     
  Branches    14669    14669           
=======================================
+ Hits        42467    42513   +46     
  Misses       1621     1621           
  Partials      735      735           
Flag Coverage Δ
shard-1 43.82% <26.08%> (-0.52%) ⬇️
shard-2 35.42% <26.08%> (+0.03%) ⬆️
shard-3 32.12% <45.65%> (+0.16%) ⬆️
shard-4 31.29% <26.08%> (-0.10%) ⬇️
shard-5 33.15% <100.00%> (-0.05%) ⬇️
shard-6 43.82% <26.08%> (+0.25%) ⬆️

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

Files with missing lines Coverage Δ
packages/gittensory-miner/lib/local-store.js 100.00% <100.00%> (ø)
packages/gittensory-miner/lib/process-lifecycle.js 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@andriypolanski
andriypolanski marked this pull request as ready for review July 12, 2026 23:03

@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.

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

@loopover-orb
loopover-orb Bot merged commit 34fc243 into JSONbored:main Jul 12, 2026
16 checks passed
@andriypolanski
andriypolanski deleted the feat/miner-cli-signal-handling-4826 branch July 16, 2026 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add signal/crash handling to the miner CLI

2 participants