Skip to content

recordPreviewPollAttempt has a TOCTOU race that can lose concurrent poll-count increments #7780

Description

@JSONbored

Context

recordPreviewPollAttempt (src/review/visual/preview-poll-budget.ts:70-79) does an unguarded read-modify-write against R2: read the current attempt-count marker via readBudgetMarker (:43-55), compute count + 1, write it back. There's no compare-and-swap or conditional write. The module's own header explains this budget exists precisely because multiple independent triggers (self-poll job chain, CI-completion webhook, deployment_status webhook, sweep pass) can all call into buildCapture for the same head SHA around the same time. If two triggers race — both read the marker at count=N before either writes — both write count=N+1, silently losing one increment. This lets the actual number of "still building, keep trying" poll attempts modestly exceed MAX_PREVIEW_POLL_ATTEMPTS (5). Consequence is soft (a few extra polls, not a correctness/security issue) — the existing doc comment already reasons about a single write failing ("a failed write just means this specific attempt doesn't count toward the budget... the safer failure direction"), just not about two concurrent successful writes racing.

Requirements

Make recordPreviewPollAttempt's increment safe under concurrent callers for the same head SHA — e.g. a conditional/compare-and-swap write against R2 (only write if the marker's current value still matches what was just read; retry on mismatch), or an equivalent mechanism already used elsewhere in this codebase for the same class of problem if one exists (check for a precedent before inventing a new one). Preserve the existing fail-open behavior for a genuine write failure (per the module's own stated rationale) — this issue is only about the concurrent-success race, not changing the failure-direction policy.

Deliverables

  • recordPreviewPollAttempt in src/review/visual/preview-poll-budget.ts no longer loses increments when two triggers race for the same head SHA.
  • A regression test simulating two concurrent calls for the same SHA, asserting the final count reflects both increments (not just one).

Test Coverage Requirements

src/review/** is under the top-level 99% patch coverage gate — the fix and its concurrency test must both be covered.

Expected Outcome

Two triggers racing to record a preview-poll attempt for the same head SHA both have their increment counted, keeping the actual poll count from silently exceeding MAX_PREVIEW_POLL_ATTEMPTS by more than the documented fail-open margin.

Links & Resources

  • src/review/visual/preview-poll-budget.ts:43-55 (readBudgetMarker), :70-79 (recordPreviewPollAttempt)

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