fix(selfhost): use ownership tokens for transient PR actuation locks - #3050
Conversation
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-04 18:07:17 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 5 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3050 +/- ##
=======================================
Coverage 96.06% 96.06%
=======================================
Files 260 260
Lines 28685 28696 +11
Branches 10437 10440 +3
=======================================
+ Hits 27556 27567 +11
Misses 493 493
Partials 636 636
🚀 New features to boost your workflow:
|
Summary
claimTransientLock(src/queue/processors.ts) — the shared mutex behind the per-PR actuation lock and the per-(repo, PR, head, mode) AI-review lock — used a constant lock value ("1") for every holder, and release was a blinddel(). The code's own comment called this out as aKNOWN LIMITATION: if a holder ran past its TTL, a NEW claimant's live lock could be deleted by the first holder's stalefinallyrelease, reopening the exact race the mutex exists to close (a maintenance pass and a draft-dodge close, or two AI-review passes, both proceeding for the same PR/head).Fix
claimTransientLocknow writes a fresh randomownerToken(node:crypto'srandomUUID()) per claim instead of the shared constant, returning{ acquired, ownerToken }.releaseTransientLockIfOwnerreleases viareleaseIfValue(atomic compare-and-delete) — it only deletes the key when the caller's own token still matches what's stored, so a stale holder's late release can never delete a different, live holder's claim. A cache withoutreleaseIfValueskips release entirely and relies on the TTL, rather than falling back to a blinddel()that would reopen the same race.SELFHOST_TRANSIENT_CACHE.releaseIfValueis a new optional cache-adapter method (src/env.d.ts), implemented for the Redis adapter (src/selfhost/redis-cache.ts) via a single Luaeval(get+del must be one atomic server-side step, or the check and the delete could themselves race a new claimant's write).claimPrActuationLock/claimAiReviewLocknow returnTransientLockClaim({ acquired, ownerToken }) instead of a bareboolean;releasePrActuationLock/releaseAiReviewLocktake the caller'sownerTokenand thread it throughreleaseTransientLockIfOwner. All four call sites (maybeRunAgentMaintenance,runAiReviewForAdvisory,maybeCloseDraftDodgeAttempt,maybeRecloseDisallowedReopen) updated to hold onto the claim result through theirtry/finally.New regression tests cover: a stale holder's release not deleting a successor's live lock, release being a no-op when
ownerTokenis null (nothing was actually claimed), and release skipping the cache entirely whenreleaseIfValueisn't implemented.Prior art / why this is safe
This exact fix was already attempted and reviewed in #2991 (closed): the AI reviewer found no blockers and confirmed the design — "directly closes the stale-holder race described in the PR... queue call sites correctly retain the returned token through their finally blocks... Redis implementation uses a single Lua eval for atomicity" — it was closed solely because
codecov/patchlanded at 96.15% (1 partial branch short of the 99% target) insrc/queue/processors.ts. This PR reimplements the same design with full branch coverage: I split the combined null-token / cache-presence check from the closed PR'sreleaseTransientLockIfOwnerinto two separate, independently-testableifguards, and verified every branch across all eight touched functions/call sites is hit on both sides (via--coverage.includescoped to the two changed source files, checked directly against the v8 coverage JSON, not just the summary %).Why no linked issue
Directly reproduces and fixes the exact gap #2991 (closed, coverage-only failure) already diagnosed and the AI reviewer already approved the design for; this repo's
linkedIssuePolicyispreferred, not required.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
ui:openapi:check,ui:lint,ui:typecheck, andui:buildare not applicable.npm run test:coveragewas run scoped totest/unit/queue.test.ts+test/unit/ai-review-advisory.test.ts+test/unit/selfhost-redis-cache.test.ts(--coverage.includeonsrc/queue/processors.ts+src/selfhost/redis-cache.ts): every branch across all touched functions and call sites is hit on both sides (verified directly against the v8 coverage JSON). The full unsharded suite could not be run clean in my local Windows dev environment for unrelated reasons: several test files depend on tooling not present there (Docker daemon, Python,sentry-cli), and generated-file "stale" checks (openapi.json, cf-typegen, selfhost-env-reference) false-positive on this checkout's CRLF line endings vs. the repo's LF convention — confirmed unrelated to this diff by reproducing them identically on a clean, unmodifiedmain. Also ran the fullqueue.test.ts+ai-review-advisory.test.ts+selfhost-redis-cache.test.tssuites (519 tests) clean.npm run test:mcp-packhits a Windows-onlyspawnSync("npm", ...)resolution failure locally (noshell: true,npmresolves tonpm.cmdon Windows) — unrelated to this diff, expected to run on the Linux CI runner.npm run actionlint,npm run typecheck,npm run test:workers,npm run build:mcp, andnpm auditall ran clean.Safety
UI Evidencesection below. — N/A, no UI changes.UI Evidence
N/A — no UI/frontend/docs changes.
Notes
TransientLockClaiminsrc/queue/processors.ts.SELFHOST_TRANSIENT_CACHE.releaseIfValue(src/env.d.ts), implemented insrc/selfhost/redis-cache.tsand the test D1 helper (test/helpers/d1.ts).