Skip to content

queue(locks): register the actuation and contributor-cap locks in the shutdown held-lock registry #10021

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

src/queue/held-lock-registry.ts exists so a shutdown signal can release every transient lock this
process holds instead of letting each ride out its TTL. Its module doc
(src/queue/held-lock-registry.ts:14-19) states the goal:

 * This is the proactive half: a tiny process-local registry of "locks I currently hold and how to release
 * them", so the shutdown handler can best-effort release every one of them immediately on SIGTERM/SIGINT --
 * before, and independent of, whether the graceful drain itself has time to finish.

registerHeldLock has exactly one production call site: claimAiReviewLock
(src/queue/ai-review-orchestration.ts:135-138). A repo-wide grep for registerHeldLock outside
src/queue/held-lock-registry.ts returns only that line plus test/unit/held-lock-registry.test.ts.

The two other locks built on the same primitive are never registered:

  • claimPrActuationLock (src/queue/transient-locks.ts:210-220), TTL PR_ACTUATION_LOCK_TTL_SECONDS = 600
    (src/queue/transient-locks.ts:203). Claimed at src/queue/processors.ts:4420 and, through
    withPrActuationLock, by all five close-enforcement guards (src/queue/review-evasion.ts:74-89).
  • claimContributorCapLock (src/queue/transient-locks.ts:264-274), TTL 600 seconds
    (src/queue/transient-locks.ts:260).

So releaseAllHeldLocksAtShutdown() at src/server.ts:1590 — including the LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS
cut-short-drain path added by #9468 (src/server.ts:1579-1587) — can only ever release ai-review-lock keys.
A hard kill during a publish-and-maintain pass strands the PR's actuation lock for its full 600 seconds.

The boot-time flush is not a backstop for this. src/server.ts:773-786 runs
flushOrphanedLocksAtBoot only when isSingleInstanceDeployment(process.env) is true
(LOOPOVER_SINGLE_INSTANCE), because on a shared-Redis multi-replica deployment the flush would delete a
sibling's live locks — the explicit #9468 reasoning at src/server.ts:765-771. On any deployment that does
not set that var (the default), the shutdown registry is the only proactive release path, and it covers one
of the three lock namespaces.

The concrete cost is documented in this repo: src/queue/retryable.ts:44-50 records that "the only jobs in
the dead-letter queue over a 7-day window were three actuation-lock contentions, one of which was a
reopen-reclose -- a policy enforcement with a single webhook-gated trigger and no reconciler, so that
enforcement was lost outright." A waiter on an orphaned actuation lock is bounded by
ATTEMPT_FREE_RETRY_DEADLINE_MS = 15 minutes (src/queue/retryable.ts:66), so a 600-second orphaned lock
consumes two-thirds of that budget before the waiter can make any progress.

Requirements

  • claimPrActuationLock (src/queue/transient-locks.ts:210-220) must call registerHeldLock(key, claim.ownerToken, () => releaseTransientLockIfOwner(env, key, claim.ownerToken)) when, and only when, the
    claim both acquired and returned a non-null ownerToken — the identical guard claimAiReviewLock applies
    at src/queue/ai-review-orchestration.ts:135.
  • releasePrActuationLock (src/queue/transient-locks.ts:221-228) must call unregisterHeldLock(key, ownerToken) when ownerToken !== null, matching releaseAiReviewLock
    (src/queue/ai-review-orchestration.ts:191) — token-scoped, so a pass whose lock was stolen (orb(review): forceAiReview does not bypass the AI-review lock — the re-run button is silently inert behind an orphaned lock (root cause of #9000) #9008) cannot
    delete the stealer's registry entry.
  • claimContributorCapLock (src/queue/transient-locks.ts:264-274) and releaseContributorCapLock
    (src/queue/transient-locks.ts:275-282) must do the same.
  • A fail-open claim (ownerToken === null) must register nothing and unregister nothing — there is no key to
    release, and registering one would let shutdown issue a delete against a lock this process does not own.
  • registerHeldLock / unregisterHeldLock / releaseAllHeldLocksAtShutdown themselves must NOT change; the
    token-conditional delete at src/queue/held-lock-registry.ts:48 is already correct.
  • claimAiReviewLock / releaseAiReviewLock must NOT change.
  • The key strings must remain exactly prActuationLockKey(...) and contributorCapLockKey(...) produce
    today, so flushOrphanedLocksAtBoot's prefix matching is unaffected.

⚠️ Required pattern: claimAiReviewLock / releaseAiReviewLock
(src/queue/ai-review-orchestration.ts:127-192) — the claim registers on a real (non-null-token) acquire,
the release unregisters token-scoped. Copy that shape into src/queue/transient-locks.ts's two other
claim/release pairs. What does NOT satisfy this issue: (a) calling registerHeldLock inside
claimTransientLock itself, which would also register the DO-backed and steal paths whose release semantics
the two named pairs do not share, and would register keys no release* sibling unregisters; (b) making the
boot-time flush unconditional (removing the isSingleInstanceDeployment gate), which reintroduces the
sibling-replica lock wipe #9468 fixed; (c) shortening PR_ACTUATION_LOCK_TTL_SECONDS, which reintroduces
the mid-work lapse #9467 fixed; (d) a test-only PR against held-lock-registry.ts, which is already covered.

Deliverables

  • claimPrActuationLock registers the held lock on a real acquire; releasePrActuationLock unregisters
    it token-scoped.
  • claimContributorCapLock registers the held lock on a real acquire; releaseContributorCapLock
    unregisters it token-scoped.
  • A test in test/unit/transient-locks.test.ts asserting that after a successful
    claimPrActuationLock against a cache adapter with a working claim/releaseIfValue,
    heldLockCountForTest() increases by exactly 1, and that releaseAllHeldLocksAtShutdown() then issues
    a releaseIfValue for the pr-actuation-lock: key.
  • The same assertion for claimContributorCapLock / the contributor-cap-lock: key in
    test/unit/transient-locks.test.ts.
  • A test asserting that a FAIL-OPEN claimPrActuationLock (adapter with no claim primitive, so
    ownerToken is null) leaves heldLockCountForTest() unchanged.
  • A regression test at test/unit/transient-locks.test.ts named for this bug asserting that
    releasePrActuationLock with a DIFFERENT ownerToken than the one registered does not remove the
    registry entry (the locks: boot flush, SIGTERM ordering, registry keying and Redis LRU each free a live holder's lock #9468 steal invariant, now applied to this lock too).

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
registering the actuation lock but not unregistering it on release, so the registry grows unboundedly across a
long-lived process and shutdown issues deletes against keys already handed to other passes — does not resolve
this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts, so src/queue/transient-locks.ts is measured and gated. Each added guard is a branch
with two real arms that both need a test: claim.acquired && claim.ownerToken !== null (register) vs the
fail-open claim (do not register), and ownerToken !== null (unregister) vs null (do not). Four new branch
arms across the two claim/release pairs, eight in total.

Expected Outcome

After this ships, a SIGTERM or a cut-short drain releases the per-PR actuation lock and the per-author
contributor-cap lock immediately, the same way it already releases the AI-review lock — so a restart no longer
strands a PR's actuation lock for up to 600 seconds on a multi-replica deployment where the boot-time flush is
deliberately disabled, and a waiting reopen-reclose or draft-dodge enforcement job no longer burns most of
its 15-minute attempt-free budget waiting out a lock whose owner is gone.

Links & Resources

  • src/queue/held-lock-registry.ts:1-19 — the "release every one of them" contract
  • src/queue/ai-review-orchestration.ts:127-192 — the only pair that implements it
  • src/queue/transient-locks.ts:203-282 — the two unregistered claim/release pairs
  • src/server.ts:773-786 — boot-time flush, gated on LOOPOVER_SINGLE_INSTANCE
  • src/server.ts:1579-1594 — the shutdown release path
  • src/queue/retryable.ts:40-66 — the dead-lettered actuation-lock contentions this protects against

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions