Skip to content

test(miner): add real crash-recovery test for portfolio-queue stuck items - #5469

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:test/miner-portfolio-queue-crash-recovery
Jul 12, 2026
Merged

test(miner): add real crash-recovery test for portfolio-queue stuck items#5469
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:test/miner-portfolio-queue-crash-recovery

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Summary

  • There was no test for the stuck-in_progress-item crash-recovery scenario against a real crashed process — the existing suite (test/unit/miner-portfolio-queue-expiry.test.ts) only exercises sweepStuckItems/findStuckItems in-process against fake timers, never an actual killed OS process.
  • Adds test/unit/miner-portfolio-queue-crash-recovery.test.ts, mirroring the fixture-child-process pattern from test/unit/miner-worktree-allocator-collisions.test.ts and test/unit/miner-concurrent-store-races.test.ts (Add a real concurrent-process test for ledger races #4867, merged): spawn a real Node child process that claims the only queued item via dequeueNext() (stamping a real on-disk lease), report the claim, then idle forever with no cleanup handler — simulating a process that claims work and crashes mid-attempt. The test SIGKILLs the child and asserts the full recovery cycle:
    1. The crash alone does not un-stick the row — it stays genuinely in_progress on disk.
    2. Sweeping before the lease bound elapses does not reclaim it (no premature reclaim).
    3. Sweeping after the lease bound elapses reclaims it back to queued.
    4. The reclaimed item is claimable again via a fresh dequeueNext() — full crash → detect → reclaim → re-claim cycle.
  • New fixture: test/fixtures/miner-concurrent-stores/claim-and-hold-child.mjs (added to the existing miner-concurrent-stores fixtures directory from Add a real concurrent-process test for ledger races #4867 rather than a new directory, since it's the same claim-and-race fixture family).
  • Per the issue's explicit scope note, this only tests portfolio-queue's own single-miner crash/reclaim behavior (sweepStuckItems/reclaimStuckItem, already shipped under Add lease/timeout/reclaim to portfolio-queue's stuck items #4827) — it does not touch, and is not extended into, cross-miner claim-conflict resolution, which stays maintainer-only.

Fixes #4868

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — this change lives entirely under test/unit/**/test/fixtures/** exercising packages/gittensory-miner/**, which sits outside vitest's coverage.include glob today, so codecov/patch cannot measure it directly — consistent with the existing worktree-allocator/claim-ledger cross-process collision tests.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run build:miner
  • npm run test:miner-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:test
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

This PR is the test addition: 2 new tests (the real crash → sweep → reclaim → re-claim cycle, including both the too-soon-to-sweep and past-the-bound-to-sweep sides of the expiry check; plus a fixture-argument-validation guard), driving the real store via a real spawned-and-killed node child process against a real on-disk SQLite file. Ran the new file 4 times in a row locally to check for timing/kill-signal flakiness — stable every time (2/2 passing, ~0.4–0.7s total).

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. — N/A, no auth/session/CORS surface touched.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A, no API/OpenAPI/MCP surface change; test-only addition.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, no UI surface touched.
  • Visible UI changes include a UI Evidence section below — N/A, no visible/UI change.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

Not applicable — this PR only adds a crash-recovery integration test for the portfolio-queue store; there is no visible UI change.

Notes

  • dequeueNext()'s own return shape carries no leasedAt field (only listInProgress()'s lease-annotated projection does) — the fixture script calls listInProgress() right after claiming to capture the real stamped lease time the test needs to compute expiry windows against. This is a pre-existing, intentional asymmetry in portfolio-queue.js (not something this PR changes), just something the fixture had to account for.
  • No production code in packages/gittensory-miner/lib/portfolio-queue.js or portfolio-queue-expiry.js is touched — this is purely a test/fixture addition on top of the already-shipped Add lease/timeout/reclaim to portfolio-queue's stuck items #4827 lease/sweep mechanism.

…tems (JSONbored#4868)

The existing suite only exercised sweepStuckItems/findStuckItems
in-process against fake timers. Spawns a real Node child process
that claims the only queued item, then idles forever with no cleanup
handler; the test SIGKILLs it and verifies the item stays genuinely
stuck until its lease expires, then is swept back to queued and
re-claimable.
@galuis116
galuis116 requested a review from JSONbored as a code owner July 12, 2026 21:46
@superagent-security

Copy link
Copy Markdown
Contributor

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

@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.67%. Comparing base (6a6426e) to head (4c7cf68).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5469      +/-   ##
==========================================
- Coverage   94.71%   94.67%   -0.04%     
==========================================
  Files         556      556              
  Lines       44546    44546              
  Branches    14664    14664              
==========================================
- Hits        42192    42175      -17     
- Misses       1619     1636      +17     
  Partials      735      735              
Flag Coverage Δ
shard-1 43.88% <ø> (-0.14%) ⬇️
shard-2 35.37% <ø> (+0.13%) ⬆️
shard-3 31.97% <ø> (-0.03%) ⬇️
shard-4 31.43% <ø> (-0.69%) ⬇️
shard-5 33.14% <ø> (+0.39%) ⬆️
shard-6 43.76% <ø> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@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 21:52:40 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR adds a real cross-process crash-recovery test for portfolio-queue's stuck-item lease/reclaim mechanism, spawning a child process that claims an item via dequeueNext() then idles forever, SIGKILLs it, and asserts the crash-then-lease-expiry-then-resweep-then-reclaim cycle end to end. It follows the established fixture-child-process pattern from PR #4867's tests and correctly reuses the miner-concurrent-stores fixtures directory. The test logic is sound: it checks the un-swept state, the too-soon sweep boundary, the post-expiry sweep, and re-claimability, which together genuinely exercise the real on-disk lease path rather than fake timers.

Nits — 5 non-blocking
  • test/unit/miner-portfolio-queue-crash-recovery.test.ts: the child-claim wait logic only looks for a line starting with 'CLAIMED ' but doesn't guard against JSON.parse throwing on a malformed/partial line if stdout is chunked mid-write, though in practice this is unlikely given the single process.stdout.write call.
  • test/fixtures/miner-concurrent-stores/claim-and-hold-child.mjs: relies on listInProgress() to backfill leasedAt after dequeueNext(); if dequeueNext() itself returned leasedAt directly this indirection could be avoided, worth confirming dequeueNext()'s return shape can't be extended instead.
  • No test asserts what happens if enqueue's single item is dequeued by the child but the child never emits 'CLAIMED' at all (e.g., dequeueNext() returns null) — the spawnAndWaitForClaim promise would hang until the 'exit' handler fires, which is fine but relies on the exit code being non-zero to reject.
  • Consider adding a short timeout/race in spawnAndWaitForClaim so a genuinely hung child (that neither claims nor exits) doesn't stall the whole test suite indefinitely.
  • The second test ('rejects the claim-and-hold-child helper when required args are missing') is a nice, cheap addition for the fixture's own arg-validation path — keep this pattern for future fixtures.
Signal Result Evidence
Code review ✅ No blockers 2 reviewers, synthesized
Linked issue ✅ Linked #4868
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: 1869 registered-repo PR(s), 1232 merged, 49 issue(s).
Contributor context ✅ Confirmed Gittensor contributor galuis116; Gittensor profile; 1869 PR(s), 49 issue(s).
Gate result ✅ Passing No configured blocker found.
Improvement ⚠️ ℹ️ Insufficient signal risk: clean · value: insufficient-signal — Nothing measurable for the structural-improvement analyzers on this PR (e.g. no code files changed). LLM value judgment: moderate — The PR closes a real coverage gap (issue #4868) by adding a genuine cross-process crash test rather than a fake-timer unit test, directly extending an established and proven fixture pattern from #4867 without scope creep into cross-miner conflict resolution.
Linked issue satisfaction

Addressed
The PR adds a real cross-process crash-recovery test that spawns and SIGKILLs a child process holding a lease, then verifies the item stays in_progress, is not prematurely reclaimed, is reclaimed once the lease expires, and becomes claimable again — matching the issue's deliverable and staying within the stated single-miner scope boundary.

Review context
  • Author: galuis116
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: JavaScript, Python, Dart, TypeScript, HTML, MDX, Rust, C++
  • Official Gittensor activity: 1869 PR(s), 49 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 &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @gittensory chat &lt;question&gt; 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

@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 6b9bb4a into JSONbored:main Jul 12, 2026
15 checks passed
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 a crash-recovery test for the stuck-queue scenario

1 participant