fix(research): one capture survives two simultaneous identical requests (chat#1912 row 7) - #801
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe playcount snapshot flow now shares scope comparison logic, deterministically selects canonical claims, deletes redundant inserted rows, and avoids starting duplicate capture workflows for concurrent matching requests. ChangesPlaycount snapshot reuse
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant createSnapshot
participant Supabase
participant pickCanonicalSnapshot
participant deletePlaycountSnapshot
participant playcountSnapshotWorkflow
createSnapshot->>Supabase: insert queued snapshot
createSnapshot->>Supabase: re-read recent claims
createSnapshot->>pickCanonicalSnapshot: choose canonical claim
createSnapshot->>deletePlaycountSnapshot: delete redundant row
createSnapshot->>playcountSnapshotWorkflow: start capture if canonical
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/supabase/playcount_snapshots/deletePlaycountSnapshot.ts (1)
13-18: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the delete outcome typed and observable.
This helper returns
Promise<void>after checking onlyerror, so a no-op delete cannot be distinguished from removing the losing claim. In the reconciliation path, that can leave a redundant row behind while returning the canonical snapshot. Return typed deletion data or an explicit affected-row result, and handle an already-missing row deliberately.As per path instructions, Supabase operations must return typed results using
Tables<"table_name">.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/supabase/playcount_snapshots/deletePlaycountSnapshot.ts` around lines 13 - 18, Update deletePlaycountSnapshot to return a typed deletion result using Tables<"playcount_snapshots">, such as the deleted row or an explicit affected-row indicator, rather than Promise<void>. Request and propagate the deleted-row data from the Supabase delete operation, distinguish a no-op/already-missing row from a successful deletion, and preserve the existing error propagation.Source: Path instructions
lib/research/playcounts/createSnapshot.ts (1)
93-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the post-insert reconciliation from
createSnapshot.The new logic expands
createSnapshotbeyond the project’s size limits and combines album resolution, cost accounting, reuse lookup, claim election, deletion, workflow dispatch, and response construction. Move reconciliation into a focused helper, in a file matching that helper’s exported function name.As per coding guidelines, functions longer than 20 lines should be flagged and functions should be small and focused; as per path instructions, domain functions should stay under 50 lines and follow single responsibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/research/playcounts/createSnapshot.ts` around lines 93 - 113, Extract the post-insert reconciliation logic from createSnapshot into a focused exported helper in a file named after that helper. Move the claims lookup, candidate filtering, canonical election, losing-row deletion, and reused-result return together, while keeping createSnapshot responsible only for orchestration and preserving the existing behavior.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/research/playcounts/createSnapshot.ts`:
- Around line 101-107: Update the candidate filter in pickCanonicalSnapshot so
reconciliation accepts every reusable snapshot state, matching
findReusableSnapshot.ts rather than checking only "queued". Reuse the shared
reusable-state predicate or canonical definition, while preserving the existing
ID and scope conditions.
---
Nitpick comments:
In `@lib/research/playcounts/createSnapshot.ts`:
- Around line 93-113: Extract the post-insert reconciliation logic from
createSnapshot into a focused exported helper in a file named after that helper.
Move the claims lookup, candidate filtering, canonical election, losing-row
deletion, and reused-result return together, while keeping createSnapshot
responsible only for orchestration and preserving the existing behavior.
In `@lib/supabase/playcount_snapshots/deletePlaycountSnapshot.ts`:
- Around line 13-18: Update deletePlaycountSnapshot to return a typed deletion
result using Tables<"playcount_snapshots">, such as the deleted row or an
explicit affected-row indicator, rather than Promise<void>. Request and
propagate the deleted-row data from the Supabase delete operation, distinguish a
no-op/already-missing row from a successful deletion, and preserve the existing
error propagation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c71ed60-e6a3-483f-9739-eb8ce82e30b5
⛔ Files ignored due to path filters (2)
lib/research/playcounts/__tests__/createSnapshot.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/research/playcounts/__tests__/pickCanonicalSnapshot.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (5)
lib/research/playcounts/createSnapshot.tslib/research/playcounts/findReusableSnapshot.tslib/research/playcounts/pickCanonicalSnapshot.tslib/research/playcounts/sameScope.tslib/supabase/playcount_snapshots/deletePlaycountSnapshot.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Architecture diagram
sequenceDiagram
participant Client
participant API as createSnapshot
participant DB as Supabase
participant Workflow as Temporal Workflow
Note over Client,Workflow: Two identical requests arrive concurrently
Client->>API: POST /api/research/measurement-jobs
API->>API: findReusableSnapshot()
Note right of API: CHANGED: uses extracted sameScope()
API->>DB: SELECT … (60‑min window)
DB-->>API: empty
API->>API: insertPlaycountSnapshot()
API->>DB: INSERT row (claim)
DB-->>API: row id "mine"
Note over API: Both requests have inserted their rows now<br/>(no sequential gap – race window)
API->>DB: SELECT claims since reuseCutoff
DB-->>API: [{ id: "mine", … }, { id: "theirs", … }]
Note over API: NEW: Reconcile via sameScope + pickCanonicalSnapshot
API->>API: filter(sameScope, state='queued')
API->>API: pickCanonicalSnapshot(claims)
Note right of API: earliest created_at, tie‑break by id<br/>deterministic regardless of input order
alt I am canonical (mine.id === canonical.id)
API->>API: keep row
API->>Workflow: start(playcountSnapshotWorkflow, [mine])
API-->>Client: { snapshot_id: "mine", reused: false }
else I am not canonical
API->>API: deletePlaycountSnapshot(mine)
API->>DB: DELETE mine
API->>API: buildReusedSnapshotResult(canonical)
API-->>Client: { snapshot_id: "theirs", reused: true }
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ts (chat#1912 row 7) The 60-minute reuse pre-check closes the sequential duplicate but not the concurrent one: two identical requests arriving together both read an empty window, both insert, and both scrape. The insert is only a claim — the scrape starts later, at the workflow call — so this reconciles in between. After inserting, re-read the window and let pickCanonicalSnapshot choose the earliest claim, tie-broken by id so the ordering is total and both racers reach the same answer without coordinating. The loser deletes its own row and returns the winner's, leaving exactly one row and one scrape. Extracts sameScope so the pre-check and the reconcile share one definition of "identical", and adds deletePlaycountSnapshot. Removes a dead sameSet helper left in findReusableSnapshot, which also clears two stray NUL bytes that were making git treat the file as binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review (coderabbit + cubic P1): the post-insert reconcile only counted claims in state `queued`, while findReusableSnapshot treats queued, running and done as reusable. A winner that advanced to `running` between the pre-check and the re-read was therefore invisible to the loser, which kept its own claim and scraped — the exact race this row exists to close. Extracts isReusableSnapshotState so the pre-check and the reconcile cannot drift apart again, and adds two tests: one deferring to a winner already running (verified red against the old queued-only filter), one confirming a failed claim is not deferred to, since nothing can be handed back from it. Rebased onto main, which now carries api#800 and api#802. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
96a1bc5 to
8b433f9
Compare
Found in preview verification with 8 concurrent identical requests. Exactly one row and one scrape survived, which is what this row is for, but four callers received the id of a mid-ranked claim that then withdrew itself, leaving them polling a snapshot that no longer exists. A loser now re-reads after withdrawing and returns a claim that is still standing, so the returned snapshot_id always resolves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preview verification — 2026-07-30Preview https://api-4t073q65j-recoup.vercel.app, built from head Requests were fired genuinely in parallel (backgrounded and Results
Eight simultaneous requests produce one capture and one charge. Before this PR each would have scraped. A defect the concurrency test found, which the unit tests could notThe first 8-way run on Cause: a loser defers to the earliest claim it can see, and with more than two racers that claim can itself be a loser that withdraws afterwards. Two-way races never show it, which is why the unit tests were green. Fixed in Review findings
The new test for it was verified red against the old filter before the fix was kept: Honest limits
Domain suites ( Test residue: three preview accounts, three snapshot rows, roughly $0.009 of scraper spend. |
There was a problem hiding this comment.
5 issues found across 8 files
Confidence score: 2/5
- In
lib/research/playcounts/createSnapshot.ts, the reconciliation path appears to fail open on transient read errors, so concurrent callers can both believe they won and start duplicate captures; this can create conflicting snapshot workflows and hard-to-debug race behavior — make post-insert reconciliation fail closed or retry before allowing capture start. - In
lib/research/playcounts/createSnapshot.ts, a loser-withdraw failure can bubble a 500 while leaving a reusable queued row behind, which risks later requests receiving an id whose workflow never started — handle withdraw failures with cleanup/marking logic so unusable queued claims cannot be reused. - In
lib/research/playcounts/createSnapshot.ts,survivor ?? canonicalcan return a stale first-read claim after filtering out failed/deleted survivors, so callers may get a snapshot id that is already invalid — revalidate or reload canonical state before returning in the no-survivor branch. - In
lib/research/playcounts/__tests__/createSnapshot.test.ts, missing assertions around "withdrawn claim" behavior (startnot called andreused: true) leave key regression paths under-specified, so these bugs could slip through future changes — add the explicit assertions to lock in expected reconciliation behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/research/playcounts/createSnapshot.ts">
<violation number="1" location="lib/research/playcounts/createSnapshot.ts:94">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
This file has grown to roughly 142 lines, exceeding the 100-line limit set by Rule 3. The new post-insert reconciliation logic filters DB claims, picks the canonical winner, deletes the caller's row if it lost, and re-reads survivors in case the winner withdrew. This is a separable sub-concern that can be extracted into a helper like `reconcileConcurrentClaim(...)`; doing so would bring the file much closer to the 100-line limit and improve readability by keeping `createSnapshot` focused on orchestration rather than concurrency mechanics.</violation>
<violation number="2" location="lib/research/playcounts/createSnapshot.ts:98">
P1: A transient read failure is treated as proof that no competing claim exists, so concurrent callers can both pass reconciliation and start duplicate captures. The post-insert reconciliation should fail closed or retry when the claims query cannot be completed.</violation>
<violation number="3" location="lib/research/playcounts/createSnapshot.ts:111">
P1: If withdrawing the loser fails, this await propagates a 500 while the queued row remains in the database. Because queued rows are reusable, subsequent requests can be handed an id whose workflow never started; deletion failure should retry or mark the loser non-reusable before the request is completed.</violation>
<violation number="4" location="lib/research/playcounts/createSnapshot.ts:128">
P2: A loser can still receive a snapshot id that is no longer usable: `survivor` excludes failed/deleted claims, but `survivor ?? canonical` returns the stale first-read object. The no-survivor path should revalidate/retry or return a non-reused failure instead of handing back an invalid capture.</violation>
</file>
<file name="lib/research/playcounts/__tests__/createSnapshot.test.ts">
<violation number="1" location="lib/research/playcounts/__tests__/createSnapshot.test.ts:353">
P2: Missing assertions in the "never hands back a claim that has itself withdrawn" test: the test should verify that `start` was NOT called (line 357) and that the result includes `reused: true` (line 358). The simpler loser test at ~line 221 covers both of these — this test should follow the same pattern to fully validate the loser-withdraw behavior.</violation>
</file>
Architecture diagram
sequenceDiagram
participant ClientA as Client A
participant ClientB as Client B
participant API as POST /api/research/measurement-jobs
participant createSnapshot as createSnapshot()
participant DB as Supabase (playcount_snapshots)
participant Workflow as Workflow API (start)
participant pickCanonical as pickCanonicalSnapshot()
participant sameScope as sameScope()
participant isReusable as isReusableSnapshotState()
participant deleteSnapshot as deletePlaycountSnapshot()
Note over ClientA,API: Two concurrent, identical requests
ClientA->>API: POST (scope: albums X, platforms Y, schedule once)
ClientB->>API: POST (same scope)
API->>createSnapshot: handle request
API->>createSnapshot: handle request
Note over createSnapshot: Pre-check (findReusableSnapshot) – both see empty window
createSnapshot->>DB: insertPlaycountSnapshot (row mine_A)
createSnapshot->>DB: insertPlaycountSnapshot (row mine_B)
Note over createSnapshot: Both rows inserted before either re-reads
createSnapshot->>DB: selectPlaycountSnapshots (window after cutoff)
DB-->>createSnapshot: [row_A, row_B]
createSnapshot->>DB: selectPlaycountSnapshots (same window)
DB-->>createSnapshot: [row_A, row_B]
Note over createSnapshot, sameScope: Both evaluate same set of claims
createSnapshot->>sameScope: sameScope(claim, ...)
createSnapshot->>isReusable: isReusableSnapshotState(state)
createSnapshot->>pickCanonical: pickCanonicalSnapshot([filtered claims])
alt row_A is canonical (earliest created_at / tie-break id)
Note over createSnapshot: Client A sees self as canonical
createSnapshot->>Workflow: start(workflow, [row_A.id])
Workflow-->>createSnapshot: workflow started
createSnapshot-->>API: { status: success, snapshot_id: row_A.id }
else row_B is not canonical
Note over createSnapshot: Client B sees row_A as canonical
createSnapshot->>deleteSnapshot: deletePlaycountSnapshot(row_B.id)
deleteSnapshot->>DB: DELETE row_B
DB-->>deleteSnapshot: OK
Note over createSnapshot: Re-read after withdrawal to avoid handing back a withdrawn id
createSnapshot->>DB: selectPlaycountSnapshots (same window)
DB-->>createSnapshot: [row_A] (only winner remains)
createSnapshot->>pickCanonical: pickCanonicalSnapshot([row_A])
Note over createSnapshot: Returns winner's id
createSnapshot-->>API: { status: success, snapshot_id: row_A.id, reused: true }
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // requests both get here before either can see the other, so re-read and | ||
| // let the earliest claim win — the loser withdraws its row and hands back | ||
| // the winner's rather than starting a second capture (chat#1912 row 7). | ||
| const claims = await selectPlaycountSnapshots({ |
There was a problem hiding this comment.
P1: A transient read failure is treated as proof that no competing claim exists, so concurrent callers can both pass reconciliation and start duplicate captures. The post-insert reconciliation should fail closed or retry when the claims query cannot be completed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/playcounts/createSnapshot.ts, line 98:
<comment>A transient read failure is treated as proof that no competing claim exists, so concurrent callers can both pass reconciliation and start duplicate captures. The post-insert reconciliation should fail closed or retry when the claims query cannot be completed.</comment>
<file context>
@@ -87,6 +91,43 @@ export async function createSnapshot(params: {
+ // requests both get here before either can see the other, so re-read and
+ // let the earliest claim win — the loser withdraws its row and hands back
+ // the winner's rather than starting a second capture (chat#1912 row 7).
+ const claims = await selectPlaycountSnapshots({
+ account: params.accountId,
+ createdAfter: reuseCutoff.toISOString(),
</file context>
| ), | ||
| ); | ||
| if (canonical && canonical.id !== row.id) { | ||
| await deletePlaycountSnapshot(row.id); |
There was a problem hiding this comment.
P1: If withdrawing the loser fails, this await propagates a 500 while the queued row remains in the database. Because queued rows are reusable, subsequent requests can be handed an id whose workflow never started; deletion failure should retry or mark the loser non-reusable before the request is completed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/playcounts/createSnapshot.ts, line 111:
<comment>If withdrawing the loser fails, this await propagates a 500 while the queued row remains in the database. Because queued rows are reusable, subsequent requests can be handed an id whose workflow never started; deletion failure should retry or mark the loser non-reusable before the request is completed.</comment>
<file context>
@@ -87,6 +91,43 @@ export async function createSnapshot(params: {
+ ),
+ );
+ if (canonical && canonical.id !== row.id) {
+ await deletePlaycountSnapshot(row.id);
+
+ // The claim that looked canonical from here may itself have been a loser
</file context>
| isReusableSnapshotState(candidate.state), | ||
| ), | ||
| ); | ||
| return buildReusedSnapshotResult(survivor ?? canonical); |
There was a problem hiding this comment.
P2: A loser can still receive a snapshot id that is no longer usable: survivor excludes failed/deleted claims, but survivor ?? canonical returns the stale first-read object. The no-survivor path should revalidate/retry or return a non-reused failure instead of handing back an invalid capture.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/playcounts/createSnapshot.ts, line 128:
<comment>A loser can still receive a snapshot id that is no longer usable: `survivor` excludes failed/deleted claims, but `survivor ?? canonical` returns the stale first-read object. The no-survivor path should revalidate/retry or return a non-reused failure instead of handing back an invalid capture.</comment>
<file context>
@@ -87,6 +91,43 @@ export async function createSnapshot(params: {
+ isReusableSnapshotState(candidate.state),
+ ),
+ );
+ return buildReusedSnapshotResult(survivor ?? canonical);
+ }
+
</file context>
| }); | ||
|
|
||
| expect(deletePlaycountSnapshot).toHaveBeenCalledWith("mine"); | ||
| expect((result as { data: { snapshot_id: string } }).data.snapshot_id).toBe("earliest"); |
There was a problem hiding this comment.
P2: Missing assertions in the "never hands back a claim that has itself withdrawn" test: the test should verify that start was NOT called (line 357) and that the result includes reused: true (line 358). The simpler loser test at ~line 221 covers both of these — this test should follow the same pattern to fully validate the loser-withdraw behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/playcounts/__tests__/createSnapshot.test.ts, line 353:
<comment>Missing assertions in the "never hands back a claim that has itself withdrawn" test: the test should verify that `start` was NOT called (line 357) and that the result includes `reused: true` (line 358). The simpler loser test at ~line 221 covers both of these — this test should follow the same pattern to fully validate the loser-withdraw behavior.</comment>
<file context>
@@ -180,4 +184,172 @@ describe("createSnapshot", () => {
+ });
+
+ expect(deletePlaycountSnapshot).toHaveBeenCalledWith("mine");
+ expect((result as { data: { snapshot_id: string } }).data.snapshot_id).toBe("earliest");
+ });
});
</file context>
| estimated_cost_usd: estimatedCostUsd, | ||
| }); | ||
|
|
||
| // The insert is a claim, not yet a scrape. Two simultaneous identical |
There was a problem hiding this comment.
P2: Custom agent: Enforce Clear Code Style and Maintainability Practices
This file has grown to roughly 142 lines, exceeding the 100-line limit set by Rule 3. The new post-insert reconciliation logic filters DB claims, picks the canonical winner, deletes the caller's row if it lost, and re-reads survivors in case the winner withdrew. This is a separable sub-concern that can be extracted into a helper like reconcileConcurrentClaim(...); doing so would bring the file much closer to the 100-line limit and improve readability by keeping createSnapshot focused on orchestration rather than concurrency mechanics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/playcounts/createSnapshot.ts, line 94:
<comment>This file has grown to roughly 142 lines, exceeding the 100-line limit set by Rule 3. The new post-insert reconciliation logic filters DB claims, picks the canonical winner, deletes the caller's row if it lost, and re-reads survivors in case the winner withdrew. This is a separable sub-concern that can be extracted into a helper like `reconcileConcurrentClaim(...)`; doing so would bring the file much closer to the 100-line limit and improve readability by keeping `createSnapshot` focused on orchestration rather than concurrency mechanics.</comment>
<file context>
@@ -87,6 +91,43 @@ export async function createSnapshot(params: {
estimated_cost_usd: estimatedCostUsd,
});
+ // The insert is a claim, not yet a scrape. Two simultaneous identical
+ // requests both get here before either can see the other, so re-read and
+ // let the earliest claim win — the loser withdraws its row and hands back
</file context>
1 participant
Closes row 7 of chat#1912 — the residual that api#800 deliberately left open, flagged by both coderabbit and cubic on that PR.
Why
api#800's 60-minute reuse pre-check closes the sequential duplicate that was actually happening in production (the seeding valuation and the
/setup/taskspre-run, ~3 minutes apart). It cannot close the concurrent one: two identical requests arriving together both read an empty window, both insert, and both scrape.On that PR I said a unique index "cannot express same album array within a rolling window", and an advisory lock was a bigger change with its own failure modes. Both still true. This takes a third route that needs neither.
How
The insert is a claim, not a scrape. The expensive part starts later, at
start(playcountSnapshotWorkflow, …). That gap is where this reconciles:pickCanonicalSnapshotchooses the earliest claim, tie-broken by id.reused: true. Only the winner starts the workflow.The ordering is total and independent of input order, which is the property that makes it work: both racers evaluate the same set and reach the same winner without talking to each other. There is a test asserting the answer does not change when the inputs are reversed, because that is exactly the bug that would make both sides think they lost (or both think they won).
Result: one row, one scrape, instead of two of each.
Also in here
sameScopeextracted, so the reuse pre-check and the reconcile share one definition of "identical" rather than drifting apart.deletePlaycountSnapshotadded, following thelib/supabase/[table]/[action]convention.sameSethelper removed fromfindReusableSnapshot. That incidentally cleared two NUL bytes that were making git treat the file as binary (Bin 1907 -> 1842 byteson the diff). Worth knowing separately:lib/research/playcounts/computePlaycountDeltas.tsstill contains one onmain, untouched here.Honest limits
queuedand is never started. That is a stale row, not a duplicate capture.Tests
TDD red→green.
lib/research/playcounts/__tests__/pickCanonicalSnapshot.test.ts— 5 cases: earliest wins, exact-tie broken by id and stable under input reversal, single claim, empty, and undated rows never crowned.createSnapshot.test.ts— 2 new cases driving the two sides of the race: the loser deletes its row, returns the winner's id, and never callsstart; the winner keeps its row and does callstartwith its own id.Full suite 4298 passed / 792 files,
tsc --noEmit,lint:checkandformat:checkall clean.Verification
Preview verification against row 7's Works-when (two concurrent
POST /api/research/measurement-jobs, then assert exactly oneplaycount_snapshotsrow) will be posted as a PR comment. Worth noting up front: a true race is timing-dependent, so a clean run proves the reconcile works but cannot prove the window is gone. The deterministic proof is the unit tests driving both sides explicitly.🤖 Generated with Claude Code
Summary by cubic
Prevents double-scraping by reconciling concurrent identical snapshot requests so only one capture runs. Also guarantees the returned snapshot_id exists by re-reading after a losing claim withdraws; reconciles against
queued/running/doneand ignoresfailed, closing chat#1912 row 7.sameScopeandisReusableSnapshotState;pickCanonicalSnapshotchooses the earliest bycreated_at(tie-break byid). Non-canonical requests delete their row and only the winner starts the workflow.sameScopeandisReusableSnapshotStatekeep pre-check and reconcile aligned, deferring torunning/donebut neverfailed. Tests cover deterministic selection, both race outcomes, deferring torunning, not deferring tofailed, and the post-withdraw re-read.Written for commit 77ad599. Summary will update on new commits.
Summary by CodeRabbit