Skip to content

fix(selfhost): ownership tokens for transient locks with claim/releaseIfValue pairing - #3161

Closed
RealDiligent wants to merge 4 commits into
JSONbored:mainfrom
RealDiligent:fix/transient-lock-ownership-release
Closed

fix(selfhost): ownership tokens for transient locks with claim/releaseIfValue pairing#3161
RealDiligent wants to merge 4 commits into
JSONbored:mainfrom
RealDiligent:fix/transient-lock-ownership-release

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

Fixes the transient PR actuation / AI-review lock stale-holder race (#2129/#2135) using ownership tokens and compare-and-delete release, and addresses the critical defect that caused #3153 to be closed: adapters with claim() but no releaseIfValue() would pin locks for 600s/1800s after normal successful work.

Supersedes closed #3153 and #2991.

Problem

Two related failure modes in the shared transient-cache mutex:

  1. Stale-holder blind del(): Lock values were constant "1" and release used unconditional del(key). A holder that ran past TTL could delete a successor's live lock in finally, reopening merge/close actuation races.

  2. Misconfigured adapter throughput collapse (fix(selfhost): use ownership tokens for transient PR actuation locks #3153): The first fix skipped release when releaseIfValue was absent (TTL backstop). Any adapter implementing claim() without ownership-aware release would hold actuation locks for 10 minutes and AI-review locks for 30 minutes after every successful pass — blocking follow-up work on the same PR.

Root cause

Lock release was not ownership-aware, and the initial ownership-token fix traded one bug for another on partially implemented cache adapters.

Implementation

  • releaseIfValue(key, value) on the Redis transient cache (Lua compare-and-delete).
  • claimTransientLock() generates UUID owner tokens; release uses compare-and-delete only.
  • Boot guard: assertSelfhostTransientCacheOwnershipRelease() in server.ts rejects claim() without releaseIfValue at startup.
  • Runtime guard: claimTransientLock() fails open without calling claim() when releaseIfValue is missing — no exclusivity, but also no unreleasable lock.
  • All actuation / AI-review lock call sites pass owner tokens through acquire/release.

Testing performed

  • npm run typecheck
  • Lock unit tests: stale-holder regressions for both PR actuation and AI-review namespaces
  • Misconfigured adapter (fix(selfhost): use ownership tokens for transient PR actuation locks #3153): claim not invoked, no lock stored, ownerToken: null
  • Boot guard: assertSelfhostTransientCacheOwnershipRelease throws for claim-only adapters; passes for createRedisCache
  • releaseIfValue error path (best-effort, TTL backstop)
  • test/unit/selfhost-redis-cache.test.ts

Compatibility

  • Production self-host Redis adapters already implement both claim and releaseIfValue (createRedisCache).
  • Custom adapters must implement both methods together or omit claim() entirely (fail-open, no exclusivity).
  • Exported lock helpers return TransientLockClaim instead of boolean; all in-repo call sites updated.

Why this approach

Compare-and-delete is the minimal correct fix for Redis-style transient locks. Pairing claim + releaseIfValue at boot prevents silent misconfiguration; runtime fail-open without acquiring unreleasable locks preserves the defense-in-depth philosophy when tests inject partial mocks.

Scope

Validation

  • npm run typecheck
  • Targeted lock/coverage unit tests locally
  • Full CI pending on this PR

Safety

  • No secrets in diff
  • Negative-path tests for stale holder, misconfigured adapter, and release errors

Notes

#3153 was auto-closed by Gittensory with: "releaseTransientLockIfOwner skip release whenever cache has claim but no releaseIfValue … keeps PR actuation locks for 600 seconds". This revision implements the suggested boot rejection plus runtime fail-open-without-claim behavior.

RealDiligent and others added 4 commits July 5, 2026 01:50
Per-PR actuation and AI-review mutexes claimed Redis keys with a constant
value and released via blind del(). A holder running past the TTL could
delete a successor's live lock in finally, reopening merge/close races the
mutex exists to prevent (JSONbored#2129/JSONbored#2135).

Store a per-holder UUID at claim time and release with compare-and-delete
(releaseIfValue) on the Redis cache adapter. Skip release when fail-open
(no cache) or when the adapter lacks compare-and-delete (TTL backstop).

Co-authored-by: Cursor <cursoragent@cursor.com>
Add regression tests for caches without releaseIfValue and for
releaseIfValue failures so stale-holder protection branches are fully
exercised.

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

Co-authored-by: Cursor <cursoragent@cursor.com>
Ownership-token release fixed stale-holder blind del() (JSONbored#2129), but skipping
release when releaseIfValue was absent pinned locks for 600s/1800s after normal
work on misconfigured adapters (JSONbored#3153).

- Boot: assertSelfhostTransientCacheOwnershipRelease() in server.ts
- Runtime: fail open without calling claim() when releaseIfValue is missing
- Tests: stale-holder regressions for both lock namespaces, boot guard, JSONbored#3153 path

Co-authored-by: Cursor <cursoragent@cursor.com>
@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 4, 2026 18:13
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.15%. Comparing base (7455342) to head (9840e52).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/queue/processors.ts 92.00% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (93.10%) is below the target coverage (99.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3161      +/-   ##
==========================================
- Coverage   95.15%   95.15%   -0.01%     
==========================================
  Files         268      268              
  Lines       29391    29406      +15     
  Branches    10712    10717       +5     
==========================================
+ Hits        27967    27980      +13     
- Misses        780      781       +1     
- Partials      644      645       +1     
Files with missing lines Coverage Δ
src/selfhost/redis-cache.ts 100.00% <100.00%> (ø)
src/queue/processors.ts 92.88% <92.00%> (-0.06%) ⬇️
🚀 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:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 4, 2026
@loopover-orb

loopover-orb Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - fixes required

Review updated: 2026-07-04 18:19:07 UTC

8 files · 1 AI reviewer · no blockers · readiness 80/100 · CI failing · dirty

🛑 Suggested Action - Fix Blockers

  • Touches a guarded path — held for manual review

Review summary
The change converts the transient mutexes from blind delete to token-owned compare-and-delete, and the changed call sites now carry the owner token through the actuation and AI-review finally blocks. The Redis adapter implements the needed atomic release primitive, and the runtime fail-open path avoids acquiring unreleasable claim-only locks, which addresses the stale-holder and #3153 throughput-collapse failure modes in the visible diff. I do not see a reachable correctness blocker in the provided changes.

Nits — 6 non-blocking
  • nit: src/selfhost/redis-cache.ts:38 keeps the boot-guard parameter type inline, which duplicates the Env cache contract and will drift if the transient cache shape changes again.
  • nit: test/unit/queue.test.ts:5587 only exercises the claim-without-releaseIfValue fail-open behavior through claimPrActuationLock; because claimAiReviewLock has a separate exported surface and longer TTL blast radius, add the same assertion there or table-drive both wrappers through the shared helper behavior.
  • src/selfhost/redis-cache.ts:38: reuse the transient cache type from Env or a local exported alias instead of spelling out the structural type inline.
  • test/unit/queue.test.ts:5587: extend the claim-only-adapter regression to cover claimAiReviewLock as well, confirming it returns { acquired: true, ownerToken: null } and never calls claim().
  • test/unit/queue.test.ts:5549: add an assertion that a successful claim passes a non-constant UUID-like token into the adapter so the test proves the production path no longer stores the old constant "1" value.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.

CI checks failing

  • codecov/patch — 93.10% of diff hit (target 99.00%)
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #3153
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 ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 124 registered-repo PR(s), 14 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 124 PR(s), 0 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 124 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Await review-lane availability.
  • Refresh registry data or choose a registered active repo.
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

@loopover-orb

loopover-orb Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Gittensory is closing this pull request on the maintainer's behalf (CI is failing (codecov/patch); conflicts with the base branch — resolve and open a fresh PR). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

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.

1 participant