Skip to content

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

Merged
JSONbored merged 3 commits into
JSONbored:mainfrom
kiannidev:feat/4298-worktree-allocator-collisions
Jul 9, 2026
Merged

test(miner-concurrency): parallel-attempt collision regression suite#4469
JSONbored merged 3 commits into
JSONbored:mainfrom
kiannidev:feat/4298-worktree-allocator-collisions

Conversation

@kiannidev

Copy link
Copy Markdown
Contributor

Summary

Collision regression suite for the worktree allocator (#4298). Includes minimal worktree-allocator.js scaffolding required to exercise acquire/release (hard dependency noted in the issue; full allocator tracked separately in #4297).

  • test/unit/miner-worktree-allocator-collisions.test.ts — cross-process collision guarantees via barriered child_process workers against one shared dbPath:
    • distinct worktree paths under simultaneous acquire
    • cap enforcement (worktree_capacity_exceeded) when excess processes contend
    • same allocation returned when two processes race on one attempt id (BEGIN IMMEDIATE raced-recheck path)
    • release() reuse
    • crash recovery / orphan reclamation on reopen
  • test/fixtures/miner-worktree-allocator/acquire-child.mjs — child worker harness (stdin barrier → acquire())
  • test/unit/miner-worktree-allocator.test.ts — basic scaffolding coverage (path resolution, validation, isProcessAlive EPERM)

Addresses maintainer feedback on #4455: replaces in-process Promise.resolve().then() microtasks (which never raced on synchronous DatabaseSync) with real multi-process contention.

Closes #4298

Test plan

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

UI Evidence

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

Made with Cursor

…ion suite

Add miner-worktree-allocator-collisions.test.ts with barriered child_process
workers that contend on a shared SQLite store, exercising BEGIN IMMEDIATE
acquire races for distinct paths, cap enforcement, release/reuse, and crash
recovery. Includes minimal worktree-allocator scaffolding required by the
JSONbored#4298 hard dependency on acquire/release existing.

Closes JSONbored#4298

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

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

Superagent found 1 security concern(s).

Comment thread packages/gittensory-miner/lib/worktree-allocator.js Outdated
@superagent-security superagent-security Bot added the pr:flagged PR flagged for review by security analysis. label Jul 9, 2026
@kiannidev kiannidev closed this Jul 9, 2026
@kiannidev kiannidev reopened this Jul 9, 2026
reclaimOrphanedAllocations previously skipped reclaim only when the slot
owner matched the reopening process, so slots held by other live processes
were incorrectly freed. Check isProcessAlive(row.owner_pid) per slot and
add a regression test for live cross-process ownership.

Co-authored-by: Cursor <cursoragent@cursor.com>
@superagent-security superagent-security Bot removed the pr:flagged PR flagged for review by security analysis. label Jul 9, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

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

@superagent-security superagent-security Bot added pr:flagged PR flagged for review by security analysis. and removed pr:flagged PR flagged for review by security analysis. labels Jul 9, 2026
@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

Tip

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

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-09 20:59:12 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
Test-focused PR that adds a minimal worktree-allocator.js (mirroring the existing claim-ledger.js local-sqlite pattern) purely to give the new collision regression suite something real to exercise, plus a genuine cross-process collision test suite that replaces the previously-criticized in-process microtask fake-concurrency test from #4455. The acquire() locking is correct: a non-transactional fast-path read is followed by BEGIN IMMEDIATE plus a re-check inside the transaction, so two processes racing on one attempt id serialize on SQLite's write lock and the loser observes the winner's committed row. Capacity enforcement and orphan reclamation via isProcessAlive/owner_pid are both correctly scoped, and this is a new local-only sqlite file (no D1/drizzle schema or migration concern).

Nits — 5 non-blocking
  • worktree-allocator.js:121,129,157 repeat 0o700/0o600 literals inline; a named DIR_MODE/FILE_MODE constant would read better, though claim-ledger.js does the same inline pattern.
  • openWorktreeAllocator (worktree-allocator.js:151) is a ~93-line function bundling path resolution, table creation, slot seeding, orphan reclaim, and statement prep — could extract the prepared-statement block, but it matches the existing openClaimLedger shape.
  • isProcessAlive (worktree-allocator.js:74) nests to depth 5 in its ternary/catch; an early return for the ESRCH case would flatten it.
  • This is test/scaffolding only — no production call site wires acquireWorktree/releaseWorktree yet (expected per feat(miner-concurrency): add git-worktree-per-attempt allocator #4297 tracked separately), worth a one-line confirmation in the PR that nothing else in the repo expects it live.
  • Confirm in the PR description whether test(miner-concurrency): add parallel-attempt collision regression suite #4298 is fully closed by this test coverage or only partially, since the linked-issue coverage looked partial from the external brief.
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: 1455 registered-repo PR(s), 750 merged, 33 issue(s).
Contributor context ✅ Confirmed Gittensor contributor kiannidev; Gittensor profile; 1455 PR(s), 33 issue(s).
Gate result ✅ Passing No configured blocker found.
Linked issue satisfaction

Addressed
The PR adds test/unit/miner-worktree-allocator-collisions.test.ts (plus a scaffolding test file) covering all four required cases: distinct paths under concurrent acquire, cap enforcement via 'worktree_capacity_exceeded', release/reuse, and crash-recovery/orphan reclamation on reopen, using cross-process workers per prior review feedback rather than in-process microtasks. It also ships minimal all

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: 1455 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

@JSONbored
JSONbored merged commit 9ed89c4 into JSONbored:main Jul 9, 2026
8 checks passed
@loopover-orb loopover-orb Bot added gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. and removed 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
@JSONbored JSONbored added gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. and removed gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 9, 2026
@JSONbored JSONbored added the gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. label Jul 9, 2026
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. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

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

2 participants