Skip to content

visual(capture): bound the render-failure retry chain with a durable per-head budget #10061

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/review/visual/preview-poll-budget.ts:6 documents, in its own header, why a job-payload attempt counter
cannot bound a retry chain in this system:

// The pre-existing MAX_PREVIEW_POLLS cap in processors.ts only bounded the self-poll job chain's OWN
// `attempt` payload field. Every OTHER re-review trigger calls reReviewStoredPullRequest without threading
// that counter through at all, so it silently reads back as 0 -- each one independently re-arms a fresh
// 5-attempt budget.

That is why #6323 made the preview-poll budget durable and keyed by headSha. It is grep-confirmed still
true today: reReviewStoredPullRequest's optional previewPollAttempt parameter
(src/queue/processors.ts:4240) is supplied by exactly one caller in the repo —
src/queue/job-dispatch.ts:507, the recapture-preview job. The CI-completion trigger
(src/queue/processors.ts:5642), the deployment_status trigger (src/queue/processors.ts:5850), the
maintenance sweep (src/queue/processors.ts:2065), and every other caller pass nothing.

#9464 later added a SECOND retry trigger — a swallowed renderer failure — and bounded it with the counter
preview-poll-budget.ts had already documented as unreliable. src/queue/processors.ts:12999:

          const previewPollAttempt = webhook.previewPollAttempt ?? 0;
          if (capture.previewPending || capture.renderFailed) {
            await scheduleVisualCaptureRetry(env, {
              webhook, repoFullName, pr, installationId, previewPollAttempt,
            });

and the only bound inside that scheduler — src/queue/processors.ts:9941:

  if (args.previewPollAttempt >= MAX_PREVIEW_POLL_ATTEMPTS) {

The previewPending half of that condition is separately protected: buildCapture consults and increments
the durable head-keyed budget itself (src/review/visual/capture.ts:741 and :760), so once the budget is
spent previewPending stops being returned at all. The renderFailed half touches the durable budget
nowhere
— grep for previewPollAttemptCount / recordPreviewPollAttempt returns only
src/review/visual/capture.ts:741 and :760. Neither does the thrown-capture catch path at
src/queue/processors.ts:13057, which schedules with webhook.previewPollAttempt ?? 0 too.

The consequence is not just extra retries. markPullRequestVisualCaptureRetryPending
(src/db/repositories.ts:4701) re-stamps the latch clock on every mark, and justifies that with a claim that
is false for this path — src/db/repositories.ts:4707:

    // #9876: stamp WHEN, so the latch's age can expire it even if every code path that should release it turns
    // out to be unreachable [...] Re-stamped on every mark rather than preserved from the first: each mark
    // means a fresh retry was just scheduled, so a fresh deadline is the accurate one. [...] here the chain is
    // already bounded by that very count, so re-stamping cannot extend it indefinitely.

"that very count" is the durable budget, which the render-failure path never consults. So during a sustained
browserless outage on a busy repo, every CI-completion, deployment_status, and sweep trigger arrives with
previewPollAttempt = 0, schedules a fresh chain, and re-stamps visual_capture_retry_pending_at to now —
resetting the VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS deadline
(src/review/visual/visual-capture-retry-latch.ts:41, 60 minutes) before it can ever elapse. The age bound
#9876 added precisely so a latch "is stale by arithmetic, whatever code did or did not run" is defeated by
arithmetic on a clock that keeps moving. The PR can be neither closed nor merged for as long as the renderer
stays down, which is exactly the freeze both #9462 and #9876 were filed for.

No test covers this: test/unit/queue-3.test.ts:2097 asserts a single renderFailed capture defers the
close; nothing asserts how many retries a repeated renderFailed across independent triggers can schedule.

Requirements

  • scheduleVisualCaptureRetry (src/queue/processors.ts:9931) must bound its retry chain on a durable,
    headSha-keyed
    attempt count that it increments on every retry it actually enqueues — not on
    args.previewPollAttempt.
  • That durable count must live in a namespace SEPARATE from preview-poll-budget.ts's
    loopover/preview-poll-budget/ marker, so a previewPending retry (already counted inside buildCapture)
    is never double-charged and the two budgets stay independently reasonable.
  • The exhausted-budget behaviour must be exactly what src/queue/processors.ts:99419963 does today: clear
    the latch via clearPullRequestVisualCaptureRetryPending and return without enqueuing.
  • args.previewPollAttempt must still be threaded into the enqueued job's attempt payload field
    (src/queue/processors.ts:9975) so the recapture-preview chain keeps its existing shape.
  • Behaviour that must NOT change: the enqueue-then-mark ordering at src/queue/processors.ts:99689999
    (The visual-capture retry latch can never be released once the durable poll budget ends the chain #9876 — a failed enqueue must still leave no latch); the PREVIEW_POLL_SECONDS delay; the
    previewPending path's own budget accounting inside buildCapture; and
    markPullRequestVisualCaptureRetryPending's re-stamping, which becomes correct once the count it depends on
    is real.
  • The new budget's read path must fail OPEN (an unreadable/absent marker counts as 0 attempts), matching
    previewPollAttemptCount's documented contract at src/review/visual/preview-poll-budget.ts:80, and its
    write path must be best-effort and compare-and-swap, matching recordPreviewPollAttempt.

⚠️ Required pattern: mirror src/review/visual/preview-poll-budget.ts exactly — same R2 REVIEW_AUDIT
marker shape, same sha256Hex(headSha + ':' + <namespace>) key derivation, same BUDGET_MARKER_MAX_AGE_MS
fail-safe expiry, same BUDGET_CAS_MAX_ATTEMPTS conditional-write loop. What does NOT satisfy this issue:
(a) threading previewPollAttempt through the remaining reReviewStoredPullRequest call sites — the
counter is per-trigger and there is no trigger-independent value to thread, which is the whole reason
#6323 went durable; (b) reusing previewPollAttemptCount/recordPreviewPollAttempt directly, which
double-charges the preview-poll budget and shortens the previewPending chain; (c) removing the
re-stamp in markPullRequestVisualCaptureRetryPending instead of fixing the count its correctness rests
on; (d) shortening VISUAL_CAPTURE_RETRY_LATCH_MAX_AGE_MS — that re-arms the false-positive close the
latch exists to prevent; (e) a test-only PR.

Deliverables

  • A durable, head-keyed capture-retry budget (read + best-effort compare-and-swap increment) in
    src/review/visual/preview-poll-budget.ts, under its own R2 namespace, exported alongside the existing
    previewPollAttemptCount/recordPreviewPollAttempt.
  • scheduleVisualCaptureRetry (src/queue/processors.ts:9931) consults that budget for args.pr.headSha
    instead of args.previewPollAttempt when deciding whether the chain is exhausted, and increments it on
    every retry it enqueues.
  • Test in test/unit/preview-poll-budget.test.ts: the new counter is independent of
    previewPollAttemptCount — recording a preview-poll attempt for a head does not advance the capture-retry
    count for that head, and vice versa.
  • Test in test/unit/preview-poll-budget.test.ts: the new read path returns 0 when env.REVIEW_AUDIT is
    unbound and when the stored object is malformed.
  • Test in test/unit/queue-3.test.ts: with buildCapture mocked to return renderFailed: true on every
    call, driving MAX_PREVIEW_POLL_ATTEMPTS + 1 independent re-reviews that each pass NO
    previewPollAttempt enqueues at most MAX_PREVIEW_POLL_ATTEMPTS recapture-preview jobs in total, and
    the last one clears visual_capture_retry_pending_sha instead of re-stamping it.
  • Test in test/unit/queue-3.test.ts named for this bug (regression test): repeated renderFailed
    captures from independent triggers cannot keep visual_capture_retry_pending_at moving forever — after
    the budget is spent the row's latch columns are null.
  • Test in test/unit/queue-3.test.ts: a single previewPending: true capture still schedules exactly one
    recapture-preview and still marks the latch (today's behaviour, pinned).

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that adds the budget module functions but leaves scheduleVisualCaptureRetry reading
args.previewPollAttempt, or one that changes the scheduler without the multi-trigger regression test that
actually proves the chain is bounded — 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 and packages/loopover-engine/src/**/*.ts — both touched files
(src/review/visual/preview-poll-budget.ts, src/queue/processors.ts) are under src/**, so both are
measured and gated
. Every branch needs both arms: the new budget's env.REVIEW_AUDIT bound/unbound arms,
its object-present/absent arms, its malformed/valid parse arms, its marker-expired/live arm, its
etag-present/absent conditional-write arms and the CAS retry-exhausted arm; and in
scheduleVisualCaptureRetry the exhausted/not-exhausted arms, the args.pr.headSha truthy/falsy arms on
both the clear and the mark, and the enqueued true/false arms. No change lands in
packages/loopover-engine/src/**, so no engine-suite upload is involved.

Expected Outcome

A sustained renderer outage produces a bounded number of recapture attempts per head, exactly like a
still-building preview already does, and the visual-capture retry latch's age bound can actually elapse
because nothing keeps resetting its clock. A PR caught in a browserless outage stops being un-closeable and
un-mergeable indefinitely.

Links & Resources

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