fix(locks): stop contention killing jobs, and stop three paths freeing a live holder's lock (#9465, #9468) - #9506
Merged
Merged
Conversation
…g a live holder's lock (#9465, #9468) Lock contention was charged an attempt like any other error, so with a flat 5s retry and maxRetries 5 a waiter died after roughly 25 seconds -- against a lock designed to be held for minutes (PR_ACTUATION_LOCK_TTL_SECONDS is 600s, spanning a whole publish -> AI review -> maintain pass). Production confirmed it: the only dead-lettered jobs in a 7-day window were three actuation-lock contentions, and one was a reopen-reclose, whose single webhook-gated trigger and absent reconciler meant that one-shot enforcement was lost outright (PR #9450). Both backends now re-pend without consuming the budget -- the same treatment the sibling GitHub rate-limit path already gets -- bounded by job age so a genuinely wedged lock still converges to a dead job an operator can see, under a distinct job_lock_contended audit event so "waiting its turn" is never read as a failure. Three paths freed locks whose owners were still running. The boot flush deletes every key matching the orphaned-lock patterns, which is only sound on a single instance -- but nothing enforced that, while the same Redis is explicitly shared across replicas and the pg backend exists to support more than one; a restarting sibling therefore freed a live replica's in-flight ai-review and pr-actuation locks, letting the next pass duplicate that review and its actuation with no TTL expiry involved. It is now opt-in via LOOPOVER_SINGLE_INSTANCE, defaulting to skip because the cost of skipping is a lock riding out its TTL (which #9008's steal path already recovers) while the cost of flushing wrongly is a double close or merge. Shutdown released every held lock BEFORE the drain, but the drain deliberately lets in-flight work finish -- so a completed drain left jobs running with their locks already freed, and a sibling or the new container in an overlapped deploy could claim one and duplicate the actuation. The drain now runs first and each job releases its own lock through its own finally; the proactive bulk release remains for the short-grace case it was written for, but only fires when the drain has not finished within LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS. The held-lock registry was keyed by lock key alone, so an earlier holder's cleanup could evict a later one's live entry: a forced re-run steals a lock in-process, and the original pass's unregister then deleted the stealer's entry, so a SIGTERM skipped that key and it stranded for the full TTL after a hard kill -- exactly the shape the registry exists to prevent. Register and unregister are now token-scoped, mirroring the store-side compare-and-delete.
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 01:46:10 UTC
Review summary Nits — 6 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.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 3eacc8c | Commit Preview URL Branch Preview URL |
Jul 28 2026, 01:00 AM |
Bundle ReportChanges will increase total bundle size by 127 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9506 +/- ##
==========================================
- Coverage 89.55% 88.65% -0.90%
==========================================
Files 843 843
Lines 110073 110083 +10
Branches 26194 26199 +5
==========================================
- Hits 98573 97592 -981
- 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
gittensor:bug
… lifecycle arms The attempt-free deferral block -- the core of #9465 -- had no coverage in either backend. Adds a regression per backend (re-pends without consuming an attempt, tagged deferred_by='lock_contended') plus two invariants: a genuine failure still consumes its budget and converges to dead, and past the age deadline a wedged lock does too rather than deferring forever. Also covers the single-instance boot-flush gate (defaulting to false is the fail-safe direction) and the null-token release arm, which must never evict a live holder. makePool's enqueueJob gained an optional createdAt so the age-based deadline is testable; it defaults to now, so every existing caller is unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The per-PR lock is mis-handled on both sides at once: waiters are killed for waiting, and three separate paths free a lock whose owner is still running. This PR fixes the contender side and the lifecycle edges.
Closes #9465
Closes #9468
The holder side (#9467 — the 600s TTL underrunning the AI review it wraps, with no renewal) is deliberately not here. It changes the primitive every actuation path depends on, and getting it wrong causes double-merges rather than preventing them, so it gets its own PR with invariant + regression coverage.
#9465 — contention burned the retry budget and killed the job
PrActuationLockContendedErrorretries on a flat 5s delay, but every retry incrementedattemptsand the job died atmaxRetries(default 5). So a waiter died after ~25 seconds of contention — against a lock whose TTL is 600s and which spans an entire publish → AI-review → maintain pass.Production evidence (7 days to 2026-07-27): the only dead-lettered jobs on the deployment were three actuation-lock contentions:
The
public-surface-publishdeaths are recovered later by the stale-surface sweep. Thereopen-reclosedeath is not:maybeRecloseDisallowedReopenhas exactly one call site, gated onpayload.action === "reopened", and no sweep, watchdog or reconciler re-derives it. That one-shot close enforcement was lost outright on #9450.The queue already models this correctly for the other "not our turn yet" condition — a GitHub rate-limit failure re-pends without incrementing attempts (
deferred_by='rate_limit'). Lock contention now gets the same treatment in both backends, bounded by job age (ATTEMPT_FREE_RETRY_DEADLINE_MS, 15 min — comfortably past the 600s TTL) so a genuinely wedged lock still converges to a dead job rather than deferring forever. It lands under a distinctjob_lock_contendedaudit event and its own counter, so "waiting its turn" is never read as a failure in the audit trail or its dashboards.#9468 — three paths freed a live holder's lock
Boot flush.
flushOrphanedLocksAtBootdeletes every key matching the orphaned-lock patterns. Its justification — "on this single-instance deployment any lock present at boot is provably orphaned" — was never enforced: the same Redis is explicitly shared across replicas (the token cache says so), and the pg queue backend exists precisely to support more than one instance. A restarting replica therefore freed a live sibling's in-flightai-review-lockandpr-actuation-lock, and the next pass claimed them fresh and duplicated the review and the actuation — with no TTL expiry needed at all. A crash-looping replica becomes a periodic fleet-wide lock wipe.Now opt-in via
LOOPOVER_SINGLE_INSTANCE, defaulting to skip: the cost of skipping is a genuinely orphaned lock riding out its TTL (which #9008's steal path already recovers on demand), while the cost of flushing wrongly is a double close or merge.Shutdown ordering. #8998 released every held lock first, before the drain, so a SIGKILL mid-drain could not strand an
ai-review-lockfor its full 1800s TTL. Butbackend.shutdown()deliberately lets in-flight work finish (#9007) — so in the case where the drain completes, jobs kept running with their locks already freed, and a sibling (or the new container in an overlapped deploy) could claim one and duplicate the actuation. The token-checked release protects the new claim from corruption; it does not stop the double-run.The drain now runs first and each job releases its own lock through its own
finally— correct and precise. The proactive bulk release stays for the short-grace case #8998 was written for, but only fires when the drain has not finished withinLOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS(unset = wait for the drain, which is right wherever the grace period comfortably exceeds a review — this deployment'sstop_grace_periodis 300s).Registry keying.
held-lock-registrywas keyed by lock key alone, so an earlier holder's cleanup could evict a later one's live entry: a maintainer's forced re-run steals a lock in-process (#9008), overwriting the entry with the stealer's release; when the original pass finished, its store-side release correctly no-oped (wrong token) but itsunregisterHeldLockdeleted the stealer's entry. A SIGTERM during the stealer's still-running review then skipped that key, stranding it for the full TTL after a hard kill — exactly the #8998 shape the registry exists to prevent. Register and unregister are now token-scoped, mirroring the store-side compare-and-delete.Validation
npx tsc --noEmit -p tsconfig.json— cleannpm run selfhost:env-reference— regenerated and committed (two newenv.*reads)held-lock-registry,retryable,selfhost-pg-queue,selfhost-sqlite-queue,selfhost-redis-cache,selfhost-metricsNew tests:
The existing registry tests were updated to thread a token rather than being deleted, and two of my new assertions were made relative to a baseline count after the shared registry leaked state between tests.
Note on the new env vars
Both default to the safe behaviour, so this deploys as a pure improvement with no configuration change: no boot flush (locks recover via TTL/steal), and no premature lock release (wait for the drain). An operator on a short SIGKILL grace period sets
LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS; a genuinely single-instance operator setsLOOPOVER_SINGLE_INSTANCE=trueto keep the faster orphan recovery.