Skip to content

fix(miner): governor-chokepoint-persisted's load-evaluate-save cycle isn't atomic - #8993

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
phamngocquy:miner/issue-8856
Jul 26, 2026
Merged

fix(miner): governor-chokepoint-persisted's load-evaluate-save cycle isn't atomic#8993
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
phamngocquy:miner/issue-8856

Conversation

@phamngocquy

Copy link
Copy Markdown
Contributor

Summary

packages/loopover-miner/lib/governor-chokepoint-persisted.ts:42-53's loadRateLimitState()/loadCapUsage() are read outside any transaction, then fed through evaluateGovernorChokepointGate, then saved via saveRateLimitState's own BEGIN IMMEDIATE -- which only protects the write, not the preceding read. Two fleet containers evaluating concurrently can both load the same bucket state and independently compute "advance by one," and the second save clobbers the first -- a lost update in the rate-limit safety core.

Deliverables

  • The load-evaluate-save cycle in governor-chokepoint-persisted.ts runs inside a single atomic transaction
  • A concurrency test simulating two overlapping chokepoint evaluations against the same rate-limit bucket, asserting both advances are correctly reflected (no lost update)

All of the above Deliverables are required in the same PR unless the deliverable text itself states otherwise.

Test plan

packages/loopover-miner/** -- 99%+ patch coverage including the new concurrency test path.

Fixes #8856

@phamngocquy
phamngocquy requested a review from JSONbored as a code owner July 26, 2026 15:26
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.58%. Comparing base (f32e140) to head (781a3bb).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8993      +/-   ##
==========================================
+ Coverage   90.56%   90.58%   +0.02%     
==========================================
  Files          96       98       +2     
  Lines       22490    22653     +163     
  Branches     3884     3934      +50     
==========================================
+ Hits        20367    20521     +154     
- Misses       1945     1950       +5     
- Partials      178      182       +4     
Flag Coverage Δ
backend 94.47% <100.00%> (?)

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

Files with missing lines Coverage Δ
...oopover-miner/lib/governor-chokepoint-persisted.ts 100.00% <100.00%> (ø)
packages/loopover-miner/lib/governor-state.ts 93.87% <100.00%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-26 15:35:41 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This wraps governor-chokepoint-persisted's load→evaluate→save cycle in a single BEGIN IMMEDIATE (governor-chokepoint-persisted.ts:44, governor-state.ts's new withScalarStateTransaction) so the write lock is taken before the read instead of only around the trailing save, correctly closing the lost-update race described in #8856. The reentrancy guard (transactionDepth) lets saveRateLimitState's own withTransaction call skip a nested BEGIN/COMMIT without breaking rollback semantics, since only the outer frame owns try/catch/rollback. The concurrency regression test spawns two real Node child processes against dist/lib output and asserts the final bucket count is 2, which is a real fleet-container-shaped race (not a fabricated in-process scenario), and a second unit test verifies the nested-transaction commit behavior directly.

Nits — 4 non-blocking
  • governor-chokepoint-persisted.ts:44-56 now runs options.append (the ledger write) inside the governor-state BEGIN IMMEDIATE critical section, lengthening the write-lock hold time under concurrent load — consider moving the ledger append after the transaction closes if ledger latency ever becomes non-trivial.
  • The new concurrency test (test/unit/miner-governor-chokepoint-persisted.test.ts) depends on packages/loopover-miner/dist/lib being pre-built and current; worth a comment noting the build-before-test dependency for future contributors unfamiliar with that convention.
  • Consider a short comment at governor-state.ts's transactionDepth declaration clarifying it is per-connection (in-process) reentrancy only, not a cross-process guard, to avoid future confusion with the BEGIN IMMEDIATE cross-process locking it sits alongside.
  • If ledger-append latency is a concern under contention, benchmark holding the scalar-state lock across the append call versus moving it outside the transaction.

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 #8856
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: 1 registered-repo PR(s), 0 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor phamngocquy; Gittensor profile; 1 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Addressed
The load+evaluate+save cycle is now wrapped in a single BEGIN IMMEDIATE via the new re-entrant withScalarStateTransaction, directly fixing the lost-update race, and the PR adds a real cross-process concurrency test (two Node child processes racing the same bucket) asserting the final count reflects both advances.

Review context
  • Author: phamngocquy
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Java, Python, Lua, Jupyter Notebook, C, Dockerfile, JavaScript, Shell
  • Official Gittensor activity: 1 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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.

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.

fix(miner): governor-chokepoint-persisted's load-evaluate-save cycle isn't atomic

1 participant