fix(queue): owner-verified release for per-PR actuation lock - #3057
fix(queue): owner-verified release for per-PR actuation lock#3057andriypolanski wants to merge 1 commit into
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3057 +/- ##
=======================================
Coverage 96.11% 96.11%
=======================================
Files 263 263
Lines 28960 28975 +15
Branches 10537 10541 +4
=======================================
+ Hits 27834 27849 +15
Misses 492 492
Partials 634 634
🚀 New features to boost your workflow:
|
|
Caution 🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥 🛑 Gittensory review result - reject/close recommendedReview updated: 2026-07-04 18:07:30 UTC
🛑 Suggested Action - Reject/Close
Review summary Blockers
Nits — 6 non-blocking
Why this is blocked
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.
|
|
Gittensory is closing this pull request on the maintainer's behalf (AI reviewers agree on a likely critical defect: src/queue/processors.ts:3488 makes claimTransientLockWithOwnerToken take the lock whenever claim() exists, but src/queue/processors.ts:3502 then refuses to release it when releaseIfValue() is absent, so any existing adapter with claim() but not releaseIfValue() will leave the PR actuation mutex held for PR_ACTUATION_LOCK_TTL_SECONDS after a successful pass and cause later maybeRunAgentMaintenance/maybeCloseDraftDodgeAttempt/maybeRecloseDisallowedReopen work for the same PR to defer or throw until the TTL expires; change the claim helper to fail open unless both primitives are present, for example `if (!env.SELFHOST_TRANSIENT_CACHE?.claim || !env.SELFHOST_TRANSIENT_CACHE.releaseIfValue) return "";`, or make releaseIfValue mandatory everywhere this cache contract is implemented.). 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. |
Summary
The shared per-PR actuation mutex (
claimPrActuationLock/releasePrActuationLock) used a constant lock value and unconditionaldelon release. If a holder ran past the 600s TTL, a later worker could claim the lock; when the first worker’sfinallyran, it deleted the second worker’s live lock — reopening the exact race the mutex exists to prevent (concurrent merge/close/reopen-reclose on the same PR).This PR stores a per-holder ownership token on claim and releases via atomic compare-and-delete (
releaseIfValue). A stale holder’sfinallyno longer deletes a later claimer’s lock.Changed files
src/env.d.tsreleaseIfValue(key, value)toSELFHOST_TRANSIENT_CACHE— atomic delete only when the stored owner matches.src/selfhost/redis-cache.tsreleaseIfValuewith a Lua compare-and-delete script.src/queue/processors.tsclaimPrActuationLockreturnsPrActuationLockClaim(token / fail-open""/ contendednull);releasePrActuationLocktakes the token and callsreleaseIfValue. AddclaimTransientLockWithOwnerTokenandreleaseTransientLockIfOwnerhelpers. Update three call sites (maybeRunAgentMaintenance,maybeCloseDraftDodgeAttempt,maybeRecloseDisallowedReopen). Remove the documented KNOWN LIMITATION.test/helpers/d1.tsreleaseIfValueon the in-memory transient cache used by unit tests.test/unit/queue.test.tstest/unit/selfhost-redis-cache.test.tsreleaseIfValuematch / mismatch paths.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:
npm run test:cigreen locally (~7 min, 8,234+ tests). Requirednpm run selfhost:env-referencefirst on this tree (stale line refs inselfhost-env-reference.tsunrelated to this PR — do not commit unless your branch also needs it).codecov/patchnot verified locally — depends on GitHub CI upload.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.No UI, API, OpenAPI, or MCP surface changes — internal queue concurrency fix only.
UI Evidence
N/A — no visible UI changes.
Test plan
releaseIfValue— deletes when owner matches, refuses when owner differs (selfhost-redis-cache.test.ts)queue.test.ts)claimPrActuationLocksuite updated for token return type (claim / contended / fail-open / concurrent)queue.test.ts)npm run test:cigreen (8,234+ tests)Notes
PrActuationLockClaimcontract:releasePrActuationLockinfinally""→ fail-open (noclaim()primitive or claim threw); skip releasenull→ contended; skip work (or throwPrActuationLockContendedErroron retryable paths)When
releaseIfValueis unavailable on a cache adapter, release is a no-op and the key expires via TTL rather than risking an unconditional delete.Analogue:
claimRegateFanoutSlot()uses the same atomic conditional-update pattern for burst dedup.