Skip to content

fix(selfhost): require releaseIfValue when transient cache exposes claim() - #3164

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/transient-lock-claim-release-pairing
Jul 4, 2026
Merged

fix(selfhost): require releaseIfValue when transient cache exposes claim()#3164
JSONbored merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/transient-lock-claim-release-pairing

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

Follow-up to merged #3050: fixes the critical defect flagged when #3153 was closed — adapters with claim() but no releaseIfValue() pinned actuation locks for 600s and AI-review locks for 1800s after normal successful work.

Problem

#3050 added ownership tokens + compare-and-delete release, but when releaseIfValue was absent the runtime skipped release and relied on TTL. Any cache adapter implementing claim() without ownership-aware release blocked follow-up PR work for minutes after every successful pass.

Root cause

claimTransientLock() acquired locks via claim() even when release could not safely run, leaving unreleasable keys until TTL expiry.

Implementation

  • 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 no unreleasable lock either.
  • Docs: env.d.ts documents that claim and releaseIfValue must be implemented together.
  • Tests: fix(selfhost): use ownership tokens for transient PR actuation locks #3153 misconfigured-adapter path, stale AI-review-holder regression, boot guard; removed upstream test that encoded the buggy TTL-only release behavior.

Testing performed

  • npm run typecheck
  • 18 lock-related unit tests in test/unit/queue.test.ts
  • test/unit/selfhost-redis-cache.test.ts (9 tests)

Compatibility

  • Production Redis (createRedisCache) already implements both methods — no behavior change for deployed self-host.
  • Custom adapters must implement both claim and releaseIfValue, or omit claim() entirely.

Why this approach

Implements the exact remediation Gittensory suggested on #3153: reject misconfigured adapters at boot, and never acquire unreleasable locks at runtime.

Notes

Builds on #3050 (already merged). Supersedes closed #3153/#3161 which duplicated the merged base fix.

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:28
@superagent-security

Copy link
Copy Markdown
Contributor

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

@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

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-04 18:32:42 UTC

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

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
This change fixes the self-host transient-lock ownership gap at the right runtime layer: `claimTransientLock()` no longer calls `claim()` unless ownership-aware release is available, so a misconfigured adapter cannot create an unreleasable lock. The Redis adapter still implements both methods, and the added regression tests exercise both stale-holder compare-and-delete behavior and the claim-without-release fail-open path. I do not see a reachable correctness blocker in the visible diff.

Nits — 5 non-blocking
  • nit: src/selfhost/redis-cache.ts:41 duplicates the transient-cache method shape inline; consider reusing the existing Env cache shape or a local exported type so future signature changes do not drift between the guard and env.d.ts.
  • nit: test/unit/selfhost-redis-cache.test.ts:90 only covers the rejecting and Redis-success cases; add a tiny positive case for an adapter with neither claim nor releaseIfValue so the documented fallback shape is locked down too.
  • src/selfhost/redis-cache.ts:41 can be made less drift-prone with a named type, e.g. `type TransientOwnershipCache = Pick<NonNullable<Env["SELFHOST_TRANSIENT_CACHE"]>, "claim" | "releaseIfValue">;`, or an equivalent local interface if importing the global Env type is awkward.
  • test/unit/selfhost-redis-cache.test.ts:90 should include `expect(() => assertSelfhostTransientCacheOwnershipRelease({})).not.toThrow();` to document that omitting `claim()` remains a valid fail-open adapter shape.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
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

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.15%. Comparing base (9a5d217) to head (74c5e23).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3164      +/-   ##
==========================================
- Coverage   95.15%   95.15%   -0.01%     
==========================================
  Files         268      268              
  Lines       29402    29406       +4     
  Branches    10715    10717       +2     
==========================================
+ Hits        27978    27980       +2     
- Misses        780      781       +1     
- Partials      644      645       +1     
Files with missing lines Coverage Δ
src/queue/processors.ts 92.88% <100.00%> (-0.08%) ⬇️
src/selfhost/redis-cache.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit 2b484ec into JSONbored:main Jul 4, 2026
9 checks passed
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.

Development

Successfully merging this pull request may close these issues.

2 participants