Skip to content

fix(miner): open the worktree allocator through local-store's openLocalStoreDb - #6704

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
luciferlive112116:fix/worktree-allocator-local-store-6600
Jul 16, 2026
Merged

fix(miner): open the worktree allocator through local-store's openLocalStoreDb#6704
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
luciferlive112116:fix/worktree-allocator-local-store-6600

Conversation

@luciferlive112116

Copy link
Copy Markdown
Contributor

Summary

worktree-allocator.js's header comment claimed it "Mirrors the package's existing local-store pattern (run-state.js, claim-ledger.js, portfolio-queue.js)". Since #4272 those three open through local-store.js's openLocalStoreDb; this one still hand-rolled the identical mkdirSync/new DatabaseSync(...)/chmodSync/PRAGMA busy_timeout sequence they used to have.

That's not cosmetic. openLocalStoreDb is what calls registerCleanupResource (#4826), so a SIGINT/SIGTERM/crash mid-write is flushed and closed by installCliSignalHandlers. Because the allocator never called it, it was not registered — and it is precisely the store where that matters most: it tracks which worktree paths are leased to which attempts, so a killed process is exactly what strands a slot leased to a PID that no longer exists. The comment asserting it mirrored those three files is what made the gap easy to miss.

The change

  • Opens via openLocalStoreDb(resolvedPath); the hand-rolled mkdir/DatabaseSync/chmod/PRAGMA block is gone.
  • resolveWorktreeAllocatorDbPathresolveLocalStoreDbPath(defaultDbFileName, "LOOPOVER_MINER_WORKTREE_ALLOCATOR_DB", env), and normalizeDbPathnormalizeLocalStoreDbPath(...), matching run-state.js/claim-ledger.js exactly.
  • Header comment rewritten to say what it now does, and why routing through the helper is the point (registration), rather than claiming a resemblance.

Env-var resolution is unchanged: the inline resolver I deleted was line-for-line what resolveLocalStoreDbPath already does, so LOOPOVER_MINER_WORKTREE_ALLOCATOR_DB, LOOPOVER_MINER_CONFIG_DIR and XDG_CONFIG_HOME still resolve identically, in the same precedence order.

close() needed no change: openLocalStoreDb wraps db.close to unregister first, so the existing close() unregisters for free.

Two things worth a reviewer's eye

  • resolveWorktreeBaseDir is untouched, per the issue — it resolves a git-worktree checkout directory, not a store DB path. mkdirSync therefore stays imported and is still used for that tree; only the DB handle's own mkdir/chmod moved into the helper. The import carries a comment saying so, since "why is node:fs still here?" is the obvious next question.
  • normalizeDbPath gets slightly stricter, deliberately. The old code did (dbPath ?? resolve()).trim(), which threw a TypeError on a non-string; the shared helper type-checks and throws the same invalid_worktree_allocator_db_path the empty-string case already threw. That's the sibling stores' behavior and a strictly better error, not a behavior regression on any valid input.

Tests

  • Both new tests fail on the unmigrated source and pass with the migration — verified, not assumed. They'd be worthless otherwise: the whole defect is an absent side effect, so a test that passes either way would pin nothing.
  • One asserts cleanupResourceCount() goes 0 → 1 on open and back to 0 on close(); the other asserts closeAllCleanupResources() — what installCliSignalHandlers invokes on SIGINT/SIGTERM — actually closes an allocator left open mid-write. That second one is the real user-facing guarantee, not just the registration bookkeeping.
  • They mirror miner-local-store.test.ts's existing #4826 block.
  • Both opt out of the suite's shared afterEach close via a small ownClose helper: close() is not idempotent (node:sqlite throws "database is not open"), so a test that closes the handle itself must own its lifetime. The helper documents that so the next person doesn't rediscover it via a confusing failure.

Validation

  • Patch coverage 100%, measured from the v8 JSON report against my 16 changed lines: zero uncovered statements, zero partial branches. Clears the 99% codecov/patch wall on packages/loopover-miner/lib/**.
  • Both new tests pass; npm run build:miner (node --check across every lib file) passes.
  • Ran every consumer suite: miner-worktree-allocator, miner-worktree-allocator-collisions, miner-concurrent-store-races, miner-local-store-readme, miner-local-store.

The remaining local failures are pre-existing and not mine — verified, not assumed. The same 4 tests fail identically on clean main with my work stashed: miner-local-store's and miner-worktree-allocator's DB-path and file-mode cases. They're Windows-only (\ vs / separators, and statSync().mode & 0o077 returning 54 rather than 0, since Windows has no POSIX permission bits). Same count, same tests, before and after.

Scope

  • Two files: the migration and its tests. Wanted paths (packages/, test/).
  • Public API, schema, resolveWorktreeBaseDir, and the honored env vars are all unchanged.
  • No secrets; no changelog, site/, CNAME, or lovable changes.

Safety

  • Strictly additive at runtime: the same file is opened with the same permissions and the same busy-timeout; the only new behavior is that the handle is now registered for cleanup and unregistered on close.
  • Deduplicating onto the shared helper means the allocator can't drift from the three stores its comment points at — which is how this gap opened in the first place.

Closes #6600

…alStoreDb (JSONbored#6600)

The allocator's own header comment claimed to mirror run-state.js/claim-ledger.js/
portfolio-queue.js, but those have opened via openLocalStoreDb since JSONbored#4272 while this
still hand-rolled the mkdirSync/DatabaseSync/chmodSync/PRAGMA sequence they used to
have. Going through the helper is what calls registerCleanupResource (JSONbored#4826), so the
allocator was never registered for crash-safe cleanup -- despite being the store where
a SIGINT/SIGTERM mid-write strands a worktree slot leased to a dead process.

Env-var resolution is unchanged: the deleted inline resolver was line-for-line what
resolveLocalStoreDbPath already does. resolveWorktreeBaseDir is untouched -- it
resolves a git-worktree checkout directory, not a store DB path.

Closes JSONbored#6600
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.64%. Comparing base (3acc512) to head (d3861b6).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6704      +/-   ##
==========================================
- Coverage   93.65%   93.64%   -0.01%     
==========================================
  Files         681      682       +1     
  Lines       68098    68090       -8     
  Branches    18695    18697       +2     
==========================================
- Hits        63775    63766       -9     
  Misses       3347     3347              
- Partials      976      977       +1     
Flag Coverage Δ
shard-1 43.85% <0.00%> (-0.06%) ⬇️
shard-2 36.94% <100.00%> (+0.10%) ⬆️
shard-3 32.27% <100.00%> (-0.13%) ⬇️
shard-4 34.58% <0.00%> (-0.07%) ⬇️
shard-5 31.66% <0.00%> (+0.73%) ⬆️
shard-6 45.74% <100.00%> (+0.08%) ⬆️

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

Files with missing lines Coverage Δ
packages/loopover-miner/lib/worktree-allocator.js 91.30% <100.00%> (-0.86%) ⬇️

... and 1 file with indirect coverage changes

@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 20:03:48 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR routes worktree-allocator.js's DB open through local-store.js's openLocalStoreDb, replacing a hand-rolled mkdirSync/DatabaseSync/chmodSync/PRAGMA sequence, which is the same pattern already used by run-state.js/claim-ledger.js/portfolio-queue.js. The core motivation checks out: openLocalStoreDb registers the handle via registerCleanupResource, so this store is now flushed/closed on SIGINT/SIGTERM like its siblings, closing a real gap where a killed process could strand a leased worktree slot. The env-var resolution and normalizeDbPath changes are pure delegations to existing local-store.js helpers with identical precedence, and the close() path needs no change since openLocalStoreDb wraps db.close to unregister. New tests directly exercise the registration/unregistration and closeAllCleanupResources paths, which is exactly what's needed to cover the fix.

Nits — 4 non-blocking
  • worktree-allocator.js still directly imports mkdirSync from node:fs for the worktree checkout directories in ensureSlots; the comment justifying this is good but consider a brief note at the ensureSlots call site too so the split of concerns doesn't need re-deriving from the header alone.
  • test/unit/miner-worktree-allocator.test.ts's ownClose helper mutates a shared module-level `allocators` array via splice — fine given single-threaded vitest execution, but worth a short comment if reused elsewhere.
  • Consider asserting `db.pragma`/busy_timeout behavior once (e.g. via local-store.js's own test) rather than re-deriving it here, since worktree-allocator.js no longer sets it directly — good that this diff doesn't duplicate that coverage.
  • No changes needed to resolveWorktreeBaseDir per the PR description; confirm the linked issue's scope agrees this file's checkout-dir logic is intentionally out of scope.

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 #6600
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: 166 registered-repo PR(s), 100 merged, 31 issue(s).
Contributor context ✅ Confirmed Gittensor contributor luciferlive112116; Gittensor profile; 166 PR(s), 31 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff replaces the hand-rolled mkdirSync/DatabaseSync/chmodSync/PRAGMA sequence with openLocalStoreDb, migrates resolveWorktreeAllocatorDbPath/normalizeDbPath onto resolveLocalStoreDbPath/normalizeLocalStoreDbPath, leaves resolveWorktreeBaseDir untouched, and updates the header comment to describe the actual openLocalStoreDb usage rather than a mere resemblance. New tests directly assert cleanu

Review context
  • Author: luciferlive112116
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, MDX, Rust, TypeScript
  • Official Gittensor activity: 166 PR(s), 31 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add a concise scope and risk note.
  • Then work through the remaining 2 steps 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 &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; 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 4e9aa2d into JSONbored:main Jul 16, 2026
16 checks passed
thomasalvaedison7777-lgtm pushed a commit to thomasalvaedison7777-lgtm/loopover that referenced this pull request Jul 17, 2026
LOOPOVER_MINER_WORKTREE_ALLOCATOR_DB's default value in the generated
docs went stale after lib/worktree-allocator.js's local-store DB open
path changed (JSONbored#6704) without a regeneration; miner:env-reference:check
has been failing on main since. Re-run npm run miner:env-reference.
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.

worktree-allocator.js's own comment claims to mirror stores that migrated to openLocalStoreDb, but it never did

1 participant