fix(locks): renew a held lock while its work runs, so a slow pass cannot be claimed out from under (#9467) - #9507
Conversation
…not be claimed out from under (#9467) The transient-lock TTLs were sized when the actuation lock covered a short plan-and-execute section. #9013 moved the claim to BEFORE maybePublishPrPublicSurface, so the 600s actuation lock now spans the whole publish -> AI review -> maintain unit -- and the AI review alone can exceed it (3 attempts x up to 600s per attempt per model, with an operator override clamped at 30 minutes PER ATTEMPT). When the TTL lapsed mid-work the holder was never told: a second worker claimed the same PR and both actuated it, producing a duplicate close plus a duplicate explanation comment and duplicate decision records, or a losing pass's placeholder republishing over a real verdict -- the exact thrash #9013 existed to eliminate. The AI-review lock has the same shape: its 1800s TTL is exactly one model's max-effort retry budget, so a second reviewer or a per-repo timeout override runs past it and the two passes' ai_review_cache upserts race. Renewal is compare-and-extend, mirroring the compare-and-delete release and for the same reason: a holder whose key already lapsed and was re-claimed must NOT extend the new owner's lock. It learns it lost instead, so a caller can abort before mutating anything. Fails open throughout, matching every other operation in this module: an adapter without renewIfValue, a fail-open claim with no token, or a throwing renewal all leave the lock on its original fixed TTL -- never worse than before, and never a reason to block real work. A transient renewal error is deliberately NOT treated as losing the lock, since that would abort work that is still legitimately holding it. The end-to-end tests drive claim -> heartbeat -> competing claim against a cache that genuinely expires keys, so the invariant asserted is the one that matters -- a competing pass stays refused while the holder works -- and the paired regression shows the same lock lapsing without the heartbeat.
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 01:59:10 UTC
Review summary Nits — 5 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk 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.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 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.
|
|
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 #9507 +/- ##
==========================================
- Coverage 89.55% 88.65% -0.90%
==========================================
Files 843 843
Lines 110073 110097 +24
Branches 26194 26196 +2
==========================================
- Hits 98573 97606 -967
- Misses 10238 11520 +1282
+ Partials 1262 971 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
the
gittensor:bug
…eartbeat (#9467) stop() clears the interval, so a callback can never START after it -- the guard before the first await was dead. The check that matters is the one after the renewal await, where stop() can genuinely have landed mid-call, and that stays.
1 participant
Summary
Keeps a held lock alive for as long as its owner is actually working, instead of betting the work finishes inside a fixed TTL.
Closes #9467
This is the holder side of the lock epic. The contender side and the lifecycle edges are #9506. It is deliberately its own PR: it changes the primitive every actuation path depends on, and getting it wrong causes double-merges rather than preventing them — so it carries invariant + regression coverage rather than being bundled to hit an issue count.
The defect
The TTLs were sized when the actuation lock covered only a short plan-and-execute section. #9013 moved the claim to before
maybePublishPrPublicSurface, so the 600s actuation lock now wraps the entire publish → AI-review → maintain unit — and the AI review alone can exceed it: 3 attempts × up to 600s per attempt per model, with an operator override clamped at 1,800,000 ms per attempt (#8458). At default medium effort, one timeout-retry on each of two reviewers is already 720s of LLM time before grounding, RAG, enrichment, publish and the entire maintenance half.When the TTL lapsed mid-work, the holder was never told. A second worker claimed the same PR and both actuated it:
The executor's freshness check is read-then-act, so both passes read "open" within the same seconds and neither can detect the other.
The AI-review lock has the same shape at a different scale: its 1800s TTL is exactly one model's max-effort retry budget (3 × 600s), so any second reviewer, fallback chain, or per-repo
claudeTimeoutMsoverride runs past it. A second pass then fires a duplicate LLM call and the two passes'ai_review_cacheupserts race — last writer wins, so two contradictory verdicts can alternate across passes at an unchanged head.The fix
A
startLockHeartbeatprimitive that re-asserts the TTL everyTTL/3(two chances to recover from a transient failure before a lapse), wired into both long-hold call sites and stopped in the samefinallythat releases the lock.Renewal is compare-and-extend (
renewIfValue), mirroring the existing compare-and-delete release and for exactly the same reason: a holder whose key already lapsed and was re-claimed by someone else must not extend the new owner's lock. It learns it lost instead, viaonLost.The queue proved this shape for its own processing lease in #9023; this is the same idea for transient locks.
Fail-open posture, unchanged from the rest of the module
renewIfValueon the adapter → no renewal; the lock behaves exactly as before (fixed TTL).The timer is
unref'd so a heartbeat can never hold the process open.Validation
npx tsc --noEmit -p tsconfig.json— cleantransient-locks,selfhost-redis-cache,held-lock-registry,queue-3Invariants (the properties that must hold, not just "the function was called"):
Regressions:
onLost.renewIfValueextends only on a token match, returns false for a missing key so a stale holder learns it lost, and does not resurrect a released lock.The
fakeRedistest double was upgraded to model TTLs and to distinguish the two compare-and-act Lua evals by the command they call, so ownership is genuinely exercised rather than assumed.Merge-order note
This touches lines adjacent to #9506 in
ai-review-orchestration.ts(both are around theregisterHeldLockcall). Both are branched frommain, so whichever merges second will need a trivial conflict resolution there — flagging so it is expected rather than surprising. No logical conflict: #9506 makes the registry token-scoped, this adds the heartbeat beside it.