Skip to content

queue(locks): wire the lock-heartbeat onLost callback at both production call sites #10019

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

startLockHeartbeat documents a contract that no production code implements. src/queue/transient-locks.ts:151-155:

 * The queue proved this shape for its own processing lease in #9023; this is the same idea for transient locks.
 * Renewal is compare-and-extend, so a holder that has ALREADY lost the key (TTL lapsed and someone re-claimed,
 * or a maintainer stole it via #9008) cannot extend the new owner's lock -- it learns it lost instead, via
 * `onLost`, and its caller aborts before mutating anything.

The mechanism exists — src/queue/transient-locks.ts:181-186 calls options?.onLost?.() when
renewIfValue reports the key is no longer ours — but no caller ever supplies onLost. A repo-wide grep
for onLost returns only the option's declarations (src/queue/transient-locks.ts:167,
src/queue/ai-review-orchestration.ts:163), the invocation, and test/unit/transient-locks.test.ts. Both
production heartbeat call sites omit it:

src/queue/processors.ts:4436-4441 (the per-PR actuation lock, spanning the whole publish → review → maintain
unit):

  const actuationHeartbeat = startLockHeartbeat(
    env,
    prActuationLockKeyForHeartbeat(repoFullName, pr.number),
    actuationLock.ownerToken,
    PR_ACTUATION_LOCK_TTL_SECONDS,
  );

src/queue/processors.ts:11655-11661 (the AI-review lock) passes the same five arguments and no options
object either.

So when a renewal comes back "not ours" — the maintainer forced-re-run steal path (#9008), or a genuine TTL
lapse followed by a re-claim — the heartbeat silently stops itself and the losing pass keeps running to
completion
: it still publishes the surface, still executes the merge/close/comment actuation under
actuationHeartbeat, and still upserts ai_review_cache under the AI-review heartbeat. That is precisely the
outcome src/queue/transient-locks.ts:145-149 and src/queue/ai-review-orchestration.ts:141-149 describe as
the bug the heartbeat was introduced to eliminate ("both actuated it, producing a duplicate close plus a
duplicate explanation comment", "two contradictory verdicts could alternate across passes at an unchanged
head"). The heartbeat currently only narrows the window; it never closes it, because the "caller aborts" half
of the documented contract was never written.

Requirements

  • startAiReviewLockHeartbeat at src/queue/processors.ts:11655 must pass an onLost handler that records
    a structured console.error line (event: "ai_review_lock_lost", carrying repoFullName, pullNumber,
    headSha, aiReviewMode) and sets a pass-local flag.
  • When that flag is set, the pass must NOT write the AI-review cache entry for this head: the existing
    aiReviewCacheReadDecideAndRun result must be discarded in favour of the lock-contended placeholder shape
    already produced by aiReviewLockContendedResult (src/queue/ai-review-orchestration.ts:197-208), which is
    the exact shape a pass that never acquired the lock already returns.
  • startLockHeartbeat at src/queue/processors.ts:4436 must pass an onLost handler that records a
    structured console.error line (event: "pr_actuation_lock_lost", carrying repoFullName, pullNumber,
    deliveryId) and sets a pass-local flag.
  • When that flag is set, the publish-and-maintain unit must abort before any further GitHub mutation by
    throwing PrActuationLockContendedError(repoFullName, pr.number, "actuation-lock-lost") — the same error
    class and uncaught-propagate shape the initial contention path already uses at
    src/queue/processors.ts:4430, so the job takes the existing attempt-free retry route
    (ATTEMPT_FREE_RETRY_KINDS, src/queue/retryable.ts:40) rather than a new one.
  • startLockHeartbeat's own behaviour must NOT change: it already stops the interval and calls onLost
    exactly once, and its fail-open posture (no renewIfValue, null token, or throwing renewal ⇒ no onLost)
    is correct and load-bearing.
  • The onLost handler must not itself release the lock — the key is already owned by someone else, and
    releaseTransientLockIfOwner's compare-and-delete is what protects the new owner.

⚠️ Required pattern: mirror src/queue/processors.ts:4420-4431's existing lock-contention handling — audit
event plus throw new PrActuationLockContendedError(...), uncaught, so the queue's attempt-free retry
handles it. What does NOT satisfy this issue: (a) adding the onLost handlers but having them only log,
so the losing pass still actuates; (b) removing the onLost option and the doc sentence instead of wiring
it, which deletes the safety property rather than delivering it; (c) changing startLockHeartbeat to throw
from inside the interval callback, which would surface as an unhandled rejection rather than aborting the
caller; (d) a test-only PR against startLockHeartbeat, which is already covered.

Deliverables

  • src/queue/processors.ts:11655 passes { onLost: ... } to startAiReviewLockHeartbeat, and the
    resulting flag causes the pass to return aiReviewLockContendedResult(advisory) instead of writing
    ai_review_cache.
  • src/queue/processors.ts:4436 passes { onLost: ... } to startLockHeartbeat, and the resulting flag
    causes the publish-and-maintain unit to throw PrActuationLockContendedError before any further GitHub
    mutation.
  • A test asserting that when SELFHOST_TRANSIENT_CACHE.renewIfValue returns false mid-pass, the
    AI-review pass produces the lock-contended placeholder and performs zero putCachedAiReview write.
    New file test/unit/lock-heartbeat-onlost.test.ts (create it) unless an existing
    test/unit/transient-locks.test.ts describe block is extended.
  • A test in the same file asserting that when renewIfValue returns false during the actuation-lock
    section, the pass throws PrActuationLockContendedError and performs zero merge/close GitHub call.
  • A regression test at test/unit/lock-heartbeat-onlost.test.ts named for this bug asserting that with
    an adapter that has NO renewIfValue, neither onLost handler fires and both passes complete exactly
    as they do today (the fail-open posture is preserved).

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
wiring onLost on the AI-review lock only, and leaving the actuation lock (the one that performs
irreversible merge/close mutations) unguarded — 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 both src/queue/processors.ts and src/queue/transient-locks.ts are measured and
gated. The change introduces one new boolean branch per call site (lock-lost vs not) — both arms of each need
a test: the lost arm (renewal returns false) and the retained arm (renewal returns true, pass completes
normally). The fail-open arm (adapter without renewIfValue, so onLost is unreachable) is a third distinct
path and is covered by the required regression test.

Expected Outcome

After this ships, a pass whose transient lock is stolen or lapses mid-flight learns it lost and stops before
mutating anything: the AI-review pass no longer overwrites the winner's cached verdict, and the
publish-and-maintain pass no longer produces a duplicate close plus a duplicate explanation comment. The
onLost contract documented at src/queue/transient-locks.ts:154 is actually enforced by the two callers
that hold real locks, instead of being an unreachable option.

Links & Resources

  • src/queue/transient-locks.ts:139-201startLockHeartbeat and the onLost contract
  • src/queue/processors.ts:4436-4441 — actuation-lock heartbeat, no onLost
  • src/queue/processors.ts:11655-11661 — AI-review-lock heartbeat, no onLost
  • src/queue/ai-review-orchestration.ts:197-208aiReviewLockContendedResult, the shape a losing pass returns
  • src/queue/transient-locks.ts:235-243PrActuationLockContendedError
  • src/queue/retryable.ts:40-63 — the attempt-free retry treatment this error already receives

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