Fix Builder collection pagination restarts#2381
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Superseded by #2382, which carries the identical commit from a same-repository branch so Content receives its isolated Netlify and Neon preview environment. |
|
Here's a visual recap of what changed: Open the full interactive recap |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Code Review Summary
PR #2381 adds offset-guarded Builder continuations and an ownership lease around each Builder CMS refresh. The exact-metadata claim is a sound foundation, and the continuation offset checks, orphan recovery, owner-specific renewal/release, and new database regression coverage are strong. The PR is standard risk because it changes shared source-refresh and metadata coordination, with concurrency-sensitive state mutation.
Key Findings
🔴 HIGH: The refresh claim is stored inside metadataJson, but several existing source metadata writers still perform unconditional stale read/modify/write updates. Those writers can erase a live claim after the CAS succeeds, allowing a second refresh to claim the same continuation.
🔴 HIGH: Snapshot row/field mutations are only preceded by one renewal. If a concurrent writer removes or replaces the claim afterward, the refresh continues mutating the snapshot and is rejected only at final metadata completion—too late to prevent overlapping workers from changing rows and fields.
The UI offset plumbing and overlap messaging are appropriately scoped, and the focused tests cover the intended claim mechanics; add race coverage for existing metadata writers and fence each mutation boundary (or transaction) before merging.
🧪 Browser testing: Could not verify — the dev server was healthy, but all browser executors lacked Chrome automation tools; the planned UI flows were therefore couldnt_verify.
| const [claimed] = await getDb() | ||
| .update(schema.contentDatabaseSources) | ||
| .set({ metadataJson: claimedMetadataJson }) | ||
| .where( | ||
| and( | ||
| eq(schema.contentDatabaseSources.id, args.source.id), | ||
| args.source.metadataJson === null | ||
| ? isNull(schema.contentDatabaseSources.metadataJson) | ||
| : eq( | ||
| schema.contentDatabaseSources.metadataJson, | ||
| args.source.metadataJson, | ||
| ), | ||
| ), | ||
| ) | ||
| .returning(); |
There was a problem hiding this comment.
🔴 Protect the refresh claim from stale metadata writers
The claim is stored only inside metadataJson, while existing writers such as write-mode updates, Builder field materialization, and federation updates still read the source and unconditionally write a stale JSON copy. If one commits after this CAS claim, it can erase builderContinuationClaimId, allowing another refresh to claim the same continuation and overlap snapshot mutations. Preserve claims with a metadata CAS/merge (or move the fence to separately guarded state) and add a race regression test.
Additional Info
Found by one reviewer; independently corroborated by another reviewer’s late-mutation fencing analysis as the prerequisite race.
| args.refreshClaimId && | ||
| !(await renewBuilderCmsSourceRefreshClaim({ | ||
| sourceId: args.source.id, | ||
| claimId: args.refreshClaimId, | ||
| })) |
There was a problem hiding this comment.
🔴 Fence snapshot mutations throughout the refresh
The ownership check runs only once after the remote Builder read, but source-field replacement, row deletion/insertion, property materialization, and final metadata updates follow afterward. If any concurrent metadata writer removes/replaces the claim, this worker continues mutating the snapshot and is rejected only at completion, after a second worker may already have claimed and started its own mutations. Re-check/renew at each mutation boundary or guard the whole snapshot mutation phase transactionally with the claim token.
Additional Info
Found independently by one reviewer; related to the claim-erasure race but remains a separate late-mutation fencing gap.
1 participant
Problem
Large Builder collections load through several continuation requests. A delayed or overlapping request could reuse an old offset after a newer request had already finished, restarting the local snapshot at the first page. In the BuilderSync publishing flow this made a completed 580-row source fall back to 100 rows and temporarily disabled the publishing controls.
Approach
Treat each Builder refresh as an owned, leased operation. Automatic continuations include the offset they observed; the server atomically claims that exact source state, renews ownership after the slow Builder read, and refuses stale or overlapping workers before they can mutate the local snapshot.
What changed
Safety and operations
This changes only local Builder source refresh coordination. It does not call Builder write APIs, change publication policy, alter credentials, or target the production
blog-articlemodel. Manual refresh remains available and is serialized through the same claim.Rollback is the single commit in this PR. There is no migration or backfill.
Verification
git diff --checkpassed.Review focus