Skip to content

fix(miner): route the governor/prediction/plan local stores through openLocalStoreDb#6667

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
lourincedaging0-commits:fix/miner-ledgers-crash-safe-open
Jul 16, 2026
Merged

fix(miner): route the governor/prediction/plan local stores through openLocalStoreDb#6667
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
lourincedaging0-commits:fix/miner-ledgers-crash-safe-open

Conversation

@lourincedaging0-commits

Copy link
Copy Markdown
Contributor

Summary

packages/loopover-miner/lib/local-store.js's openLocalStoreDb (#4272) is the package's shared crash-safe SQLite open: it does the mkdirSync(0o700) + DatabaseSync + chmodSync(0o600) + PRAGMA busy_timeout, and — critically — registers the handle via registerCleanupResource (#4826) so a SIGINT/SIGTERM/uncaught-exception closes it cleanly mid-run.

governor-ledger.js, prediction-ledger.js, and plan-store.js each hand-rolled that same open inline, bypassing the helper — so their handles were never registered for cleanup, and a crash/signal mid-run left them half-written instead of closed. This routes all three through openLocalStoreDb (which also handles the ':memory:' no-file case plan-store relied on), so they get the identical open behavior plus the crash-safe cleanup registration the package's other stores already have.

Pure boilerplate consolidation — no schema, path-resolution, or query behavior changes; the now-unused node:fs/node:sqlite/dirname imports are dropped.

Tests

All existing store + CLI tests pass unchanged (miner-governor-ledger, miner-prediction-ledger, miner-plan-store + their -cli variants — 43 tests), confirming identical open/read/write behavior through the shared helper.

Closes #6595.

…penLocalStoreDb

governor-ledger.js, prediction-ledger.js, and plan-store.js each hand-rolled
their own mkdir(0o700)/DatabaseSync/chmod(0o600)/busy_timeout open, bypassing
local-store.js's openLocalStoreDb — so unlike the package's other local stores
their handles were never registered with registerCleanupResource, and a
SIGINT/SIGTERM/uncaught-exception mid-run left them half-written instead of
closed cleanly (JSONbored#4826). Route all three through openLocalStoreDb, which
centralizes that boilerplate (including the ':memory:' no-file case) and the
crash-safe cleanup registration.

Closes JSONbored#6595.
@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:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 16, 2026
@loopover-orb

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-16 18:18:22 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This is a clean boilerplate consolidation: all three stores (governor-ledger, prediction-ledger, plan-store) now route their DatabaseSync open through the shared openLocalStoreDb helper instead of hand-rolling mkdirSync/chmodSync/PRAGMA busy_timeout, gaining crash-safe cleanup registration in the process. The now-unused node:fs/node:sqlite/dirname imports are correctly dropped in all three files, and plan-store's ':memory:' special-casing is preserved by delegating it into the shared helper. The diff is narrow, behavior-preserving, and directly addresses the linked issue #6595.

Nits — 5 non-blocking
  • No new or updated tests accompany this change — the PR description claims the existing 43 tests cover it, but none specifically assert that the returned db handle is registered via registerCleanupResource, so the actual crash-safe behavior this PR adds is unverified by the test suite.
  • Worth double-checking that openLocalStoreDb's behavior for a resolvedPath that doesn't yet exist as a directory matches the previous per-file mkdirSync(dirname(...)) call for all three consumers, including governor-ledger and prediction-ledger which never special-cased ':memory:' before.
  • Add a small test (or point to one if it already exists in local-store.js's own suite) verifying that opening any of these three stores registers a cleanup resource, since that's the actual behavioral gain this PR claims.
  • Consider a one-line changelog/comment cross-reference to Add signal/crash handling to the miner CLI #4826 in each file (already partially done in plan-store.js) so future readers landing on governor-ledger.js/prediction-ledger.js immediately see why the open changed.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.

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 #6595
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High 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: 103 registered-repo PR(s), 48 merged, 5 issue(s).
Contributor context ✅ Confirmed Gittensor contributor lourincedaging0-commits; Gittensor profile; 103 PR(s), 5 issue(s).
Improvement ℹ️ None detected risk: low · value: none · LLM: moderate
Linked issue satisfaction

Partially addressed
All three stores are correctly migrated to openLocalStoreDb for the actual DB-open sequence, but the diff still uses local normalizeDbPath/resolve*DbPath helpers rather than switching to resolveLocalStoreDbPath/normalizeLocalStoreDbPath from local-store.js as explicitly required, and no new cleanupResourceCount() assertions were added to the three unit test files as mandated by the deliverables.

Review context
  • Author: lourincedaging0-commits
  • 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: 103 PR(s), 5 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add a concise scope and risk note.
  • Then work through the remaining 1 step in the Signals table above.
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

@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 0591ffc into JSONbored:main Jul 16, 2026
9 checks passed
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.

governor-ledger.js, prediction-ledger.js, and plan-store.js bypass local-store.js's crash-safe openLocalStoreDb

1 participant