Skip to content

test(miner-concurrency): parallel-attempt collision regression suite - #4455

Closed
kiannidev wants to merge 2 commits into
JSONbored:mainfrom
kiannidev:feat/4298-worktree-allocator-collisions
Closed

test(miner-concurrency): parallel-attempt collision regression suite#4455
kiannidev wants to merge 2 commits into
JSONbored:mainfrom
kiannidev:feat/4298-worktree-allocator-collisions

Conversation

@kiannidev

Copy link
Copy Markdown
Contributor

Summary

  • Add packages/gittensory-miner/lib/worktree-allocator.js — SQLite-backed acquire/release pool with configurable concurrency cap and orphan reclamation on startup (feat(miner-concurrency): add git-worktree-per-attempt allocator #4297 prerequisite).
  • Add test/unit/miner-worktree-allocator-collisions.test.ts proving:
    • concurrent acquire() returns distinct worktree paths
    • excess parallel acquires are rejected at the cap (worktree_capacity_exceeded)
    • release() frees a path for reuse
    • crash recovery reclaims orphaned active allocations on reopen
  • Add test/unit/miner-worktree-allocator.test.ts for path resolution and basic allocator behavior.

Closes #4298

Also delivers the allocator foundation required by #4297 (hard dependency noted in the issue).

Test plan

  • npm run build:miner
  • npx vitest run test/unit/miner-worktree-allocator.test.ts test/unit/miner-worktree-allocator-collisions.test.ts
  • CI: validate, codecov/patch, security

UI Evidence

N/A — test-only / miner library PR (no visible UI).

Made with Cursor

…suite

Introduce the git-worktree-per-attempt allocator (JSONbored#4297) and prove its
concurrency guarantees with parallel acquire, cap enforcement, release
reuse, and crash-recovery regression tests.

Closes JSONbored#4298

Co-authored-by: Cursor <cursoragent@cursor.com>
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.01%. Comparing base (861e8b7) to head (9108348).
⚠️ Report is 6 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4455   +/-   ##
=======================================
  Coverage   94.01%   94.01%           
=======================================
  Files         418      418           
  Lines       37417    37417           
  Branches    13677    13677           
=======================================
  Hits        35178    35178           
  Misses       1583     1583           
  Partials      656      656           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kiannidev
kiannidev marked this pull request as draft July 9, 2026 19:29
…ests

Co-authored-by: Cursor <cursoragent@cursor.com>
@kiannidev
kiannidev marked this pull request as ready for review July 9, 2026 19:46
@loopover-orb loopover-orb Bot added gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. labels Jul 9, 2026
@loopover-orb

loopover-orb Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-09 19:58:37 UTC

5 files · 1 AI reviewer · no blockers · readiness 100/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

Review summary
Adds a new SQLite-backed worktree-slot allocator (worktree-allocator.js) plus two test files. The allocator's transaction logic (BEGIN IMMEDIATE + re-check) is designed for cross-process contention, but node:sqlite's DatabaseSync API is fully synchronous and JS itself is single-threaded, so the 'parallel-attempt collision' tests — which only interleave calls via Promise.resolve().then() microtasks inside one process — cannot actually exercise concurrent/racing acquisitions; they'd pass identically even with the BEGIN IMMEDIATE transaction removed, so they don't validate what the PR claims to regression-test. The PR also delivers the full #4297 allocator implementation (not just tests) under a PR titled/scoped as a 'test' PR closing #4298, which is scope creep beyond what the linked issue covers.

Blockers

  • test/unit/miner-worktree-allocator-collisions.test.ts's 'concurrent'/'simultaneous' acquire tests only chain Promise.resolve().then() calls within a single Node process; since DatabaseSync is synchronous and JS is single-threaded, these calls never actually race, so the BEGIN IMMEDIATE transaction/raced-recheck branch in worktree-allocator.js acquire() (lines ~180-200) is never genuinely exercised — the suite proves the allocator's plain (non-concurrent) logic works, not that it survives real multi-process collisions, which is the actual failure mode the allocator exists to prevent.
  • The PR description states this closes test(miner-concurrency): add parallel-attempt collision regression suite #4298 (test-only) but also ships the entire feat(miner-concurrency): add git-worktree-per-attempt allocator #4297 production allocator (261 new lines of library code) as a 'prerequisite' bundled into the same PR — that's feature scope-creep beyond the linked issue and should be split or the issue link corrected to feat(miner-concurrency): add git-worktree-per-attempt allocator #4297 as well.
Nits — 5 non-blocking
  • worktree-allocator.js isProcessAlive() (top of file) has a ternary in its catch block — `error.code === 'ESRCH' ? false : false` — both branches return the same value, so the ESRCH check is dead code; likely meant `... ? false : true` so an EPERM (process exists, different owner) correctly reports alive instead of being silently reclaimed.
  • ensureSlots() only ever INSERT OR IGNOREs new slots up to maxConcurrency and never removes rows if a store is reopened with a smaller maxConcurrency than a prior run — worth a comment or guard since a shrinking config leaves orphaned extra slot rows that count toward listSlots() but not the intended cap.
  • openWorktreeAllocator() is a 90+ line function per the size-smell scan; consider extracting statement-preparation into a small helper to keep the constructor scannable.
  • 0o700/0o600 literals are repeated across mkdirSync/chmodSync calls; a couple of named constants (e.g. `DIR_MODE`, `DB_FILE_MODE`) would self-document intent and match the pattern likely used in claim-ledger.js's sibling module.
  • Replace or supplement the in-process 'concurrent' tests with a real cross-process test (spawn 2+ child processes via node:child_process each calling acquire() against the same dbPath) to actually exercise the BEGIN IMMEDIATE contention path this allocator is built for.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #4298
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: 1447 registered-repo PR(s), 745 merged, 33 issue(s).
Contributor context ✅ Confirmed Gittensor contributor kiannidev; Gittensor profile; 1447 PR(s), 33 issue(s).
Gate result ✅ Passing No configured blocker found.
Linked issue satisfaction

Addressed
The PR adds the exact deliverable file `test/unit/miner-worktree-allocator-collisions.test.ts` and covers all four required scenarios: concurrent acquire returning distinct paths, cap enforcement rejecting excess simultaneous acquires, release/reuse, and crash recovery via a manually seeded orphaned active row reclaimed on reopen, using the temp-dir/db-per-test pattern consistent with sibling mine

Review context
  • Author: kiannidev
  • 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: 1447 PR(s), 33 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.

🟩 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

@kiannidev
kiannidev marked this pull request as draft July 9, 2026 19:56

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

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Fix + resubmit:

test/unit/miner-worktree-allocator-collisions.test.ts's 'concurrent'/'simultaneous' acquire tests only chain Promise.resolve().then() calls within a single Node process; since DatabaseSync is synchronous and JS is single-threaded, these calls never actually race, so the BEGIN IMMEDIATE transaction/raced-recheck branch in worktree-allocator.js acquire() (lines ~180-200) is never genuinely exercised — the suite proves the allocator's plain (non-concurrent) logic works, not that it survives real multi-process collisions, which is the actual failure mode the allocator exists to prevent.

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. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier.

Development

Successfully merging this pull request may close these issues.

test(miner-concurrency): add parallel-attempt collision regression suite

2 participants