fix(review): make preview-poll-budget increment safe under concurrent triggers

[shin-core]

What & why

Closes #7780.

recordPreviewPollAttempt (src/review/visual/preview-poll-budget.ts) did an unguarded read-modify-write against R2: read the attempt-count marker, compute count + 1, write it back — with no compare-and-swap. The module exists precisely because multiple independent triggers (self-poll job chain, CI-completion webhook, deployment_status webhook, sweep pass) can call into buildCapture for the same head SHA at nearly the same time. When two race, both read the marker at count=N before either writes, both write count=N+1, and one increment is silently lost — letting the real poll count exceed MAX_PREVIEW_POLL_ATTEMPTS.

The fix

Switch the increment to a compare-and-swap against the marker's R2 httpEtag:

• read the marker together with the etag it was stored under;
• write with onlyIf: { etagMatches } when a marker already exists, or onlyIf: { etagDoesNotMatch: "*" } for the first write (create-if-absent);
• on a precondition miss R2 returns null (no write) instead of throwing — re-read the racing writer's newer count and retry, bounded by BUDGET_CAS_MAX_ATTEMPTS = 3.

This is R2's native conditional-write primitive; no CAS precedent existed in the codebase to reuse (the cited actions-fallback.ts markers do plain best-effort writes), so the issue's "conditional/compare-and-swap write against R2" path is taken. The existing fail-open-on-write-failure contract is preserved: exhausting the retries under sustained contention degrades to "this attempt didn't count" — the same safe direction the module already documents for a genuine write failure — and a real write error is still swallowed best-effort.

Tests

• Regression (the race): two concurrent recordPreviewPollAttempt calls for the same SHA, interleaved via a put barrier so both read before either writes; asserts the final count is 2, not 1 — i.e. both increments land. Verified bug-catching: reverting the CAS makes this fail with count=1.
• Retry exhaustion: a store whose conditional put never succeeds; asserts the call still resolves (never throws) and stops after exactly 3 attempts.
• Upgraded the in-memory R2 test stand-in to honor httpEtag + the onlyIf preconditions, matching real R2 semantics.
• All 13 pre-existing budget tests unchanged and green; 100% line + branch coverage on the changed file (22/22 branches).

Validation

• npm run typecheck — clean
• test/unit/preview-poll-budget.test.ts (15) + consumer suites visual-capture/actions-fallback (228 total) — green
• git diff --check — clean; change is scoped to the two files above, no generated-artifact impact

[superagent-security]

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.86%. Comparing base (9d95c96) to head (2ed4b58).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7833      +/-   ##
==========================================
+ Coverage   80.62%   88.86%   +8.23%     
==========================================
  Files         729       89     -640     
  Lines       74671    21240   -53431     
  Branches    22791     3684   -19107     
==========================================
- Hits        60205    18875   -41330     
+ Misses      11654     2187    -9467     
+ Partials     2812      178    -2634     

Flag	Coverage Δ
shard-1	100.00% <100.00%> (+45.50%)	⬆️
shard-2	15.38% <5.88%> (-39.81%)	⬇️
shard-3	84.61% <76.47%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/review/visual/preview-poll-budget.ts	100.00% <100.00%> (+85.00%)	⬆️

... and 640 files with indirect coverage changes

[loopover-orb]

Warning

⏸️ LoopOver review result - manual review recommended

_{Review updated: 2026-07-21 14:38:34 UTC}

2 files · 1 AI reviewer · no blockers · CI green · unstable

⏸️ Suggested Action - Manual Review

Review summary
This correctly replaces the unguarded read-modify-write in recordPreviewPollAttempt with an R2 conditional-write (compare-and-swap) loop against the object's httpEtag, bounded at 3 attempts, with create-if-absent handled via etagDoesNotMatch:"*" and overwrite via etagMatches. The regression test exercises the actual race by stalling the first writer's put until the second has written, then asserting the retry re-reads the newer count and the final tally is 2, and a second test confirms the bounded-retry exhaustion path degrades to a silent no-op rather than throwing or looping forever. The fail-open contract (no REVIEW_AUDIT, malformed marker, stale marker, read/write errors) is preserved exactly as before.

Nits — 5 non-blocking

• preview-poll-budget.ts:87-104: the retry loop re-derives `key` once but re-reads the full marker every iteration including the first, which is fine but could be simplified by returning the read from inside the loop rather than as a separate call outside it — purely stylistic, no functional issue.
• The 'recordPreviewPollAttempt has a TOCTOU race that can lose concurrent poll-count increments #7780' references embedded in comments (flagged by the external brief as a 'magic number') are just issue-tracker citations, not literals needing a named constant — that flag can be disregarded.
• test/unit/preview-poll-budget.test.ts: the pathological-store test hardcodes `expect(putAttempts).toBe(3)` against `BUDGET_CAS_MAX_ATTEMPTS` instead of importing the constant, so a future change to the retry count silently desyncs the test's own docstring claim.
• Consider exporting BUDGET_CAS_MAX_ATTEMPTS (or asserting against it via a re-exported test hook) so the exhaustion test in preview-poll-budget.test.ts can't drift from the production constant.
• The doc comment on recordPreviewPollAttempt could mention the CAS retry bound explicitly for readers who only skim the public API doc block rather than the implementation.

Flagged checks (non-blocking)

• Contributor trust — Contributor flagged for review

Decision drivers

• ✅ Code review — No blockers (1 reviewer)
• ✅ Gate result — Passing (No configured blocker found.)

Context & advisory signals — never blocks the verdict

Signal	Result	Evidence
Linked issue	✅ Linked	#7780
Related work	✅ No active overlap found	No same-issue or scoped active PR overlap found.
Change scope	✅ 20/20	Low review scope from cached public metadata (1 linked issue).
Validation posture	✅ 25/25	PR body includes validation/test evidence.
Contributor workload	✅ 10/10	Author activity: 86 registered-repo PR(s), 46 merged, 0 issue(s).
Contributor context	✅ Confirmed Gittensor contributor	shin-core; Gittensor profile; 86 PR(s), 0 issue(s).
Improvement	✅ Minor	risk: clean · value: minor · LLM: significant

Linked issue satisfaction

Addressed
The diff replaces the unguarded read-modify-write with an R2 compare-and-swap (etagMatches/etagDoesNotMatch) plus bounded retry loop, and preserves the fail-open behavior on exhausted retries or write errors as required. It also adds the requested regression test simulating two concurrent calls for the same SHA asserting the final count reflects both increments (plus a retry-exhaustion test).

Review context

• Author: shin-core
• Role context: outside_contributor
• Public audience mode: oss maintainer
• Lane context: Repository is configured for direct PR review.
• Public profile languages: TypeScript, JavaScript, Solidity, Dart, Python, CSS, PHP, Rust
• Official Gittensor activity: 86 PR(s), 0 issue(s).
• PR-specific overlap: none found.

Contributor next steps

• Keep the PR focused and include validation evidence before maintainer review.

Signal definitions

• Related work = same linked issue, overlapping active PRs, or title/path similarity.
• Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
• Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
• Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
• Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🧪 Chat with LoopOver

Ask 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.

• @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
• @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
• A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

_{🟩 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.

• Re-run LoopOver review

[loopover-orb]

LoopOver approves — the gate is satisfied and CI is green.