Repository navigation
Coordinate browser bootstrap across networks before rebuilding indexes - #2898
Conversation
findolor
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe WASM local database startup now coordinates provisioning for targets without persisted watermarks. It downloads eligible seed dumps and imports them as a cohort. The import rebuilds non-unique indexes only when the database is fresh and import ownership is held. The scheduler starts network runners after provisioning completes or fails, unless it stops first. ChangesBrowser database bootstrap
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant LocalDb
participant WebLocks
participant BootstrapRunner
participant ManifestFetcher
participant DumpDownloader
participant NetworkRunner
Scheduler->>LocalDb: check persisted target watermarks
Scheduler->>WebLocks: acquire bootstrap ownership
Scheduler->>BootstrapRunner: provision missing targets
BootstrapRunner->>ManifestFetcher: fetch manifests for missing targets
BootstrapRunner->>DumpDownloader: download seed dumps
BootstrapRunner->>LocalDb: import dump cohort
LocalDb-->>BootstrapRunner: return import result
Scheduler->>NetworkRunner: start runners with acquired leadership guards
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was established. The change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 68.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 8 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 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 |
How to use the Graphite Merge QueueAdd the label Raindex-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@rain-marvin review |
|
🔎 Reviewing |
![rain-marvin[bot]](https://avatars.githubusercontent.com/in/5077017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR changes browser startup so that every network with no local data is seeded together. Under a cross-tab Web Lock it checks watermarks, downloads all missing seeds in parallel, imports them in one sqlite-web session that drops the non-unique indexes once, rebuilds them once and commits, runs one ANALYZE, and only then starts the normal per-network sync loops. If any step fails, it falls back to the existing per-network path.
The core mechanics look correct. The import is atomic, PRIMARY KEY and UNIQUE indexes stay in place, dump envelopes are validated before a session opens, aliases are deduplicated, a cancelled lock request releases ownership, and stop cancels coordination. I found no blocker. The comments below are about cost and fallback behaviour: one slow or broken seed throws away the whole cohort and can bring back the import ordering this PR is meant to remove, warm networks now wait for cold networks, adding one network rebuilds every index over the full DB, and the coordinator can race per-network leaders in another tab. I also noticed that no test drives start() through the new stop/fallback select, so the claim that stop cancels a pending import is only tested at the primitive level.
claude-opus-5-5 · high · 15 min
|
@coderabbitai review |
|
|
@rain-marvin review |
|
🔎 Reviewing |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
This PR makes the browser import every missing network's seed in one shared step on a cold start, so the secondary indexes are rebuilt once after all seeds are in, instead of once per network. Warm networks now start right away, failed downloads only send their own target back to the normal runner, a populated DB keeps its indexes, and networks led by another tab are left out of the cohort.
The four earlier comments are fixed correctly at fe2961b. I found no blocker. Three minor points remain, all about timing and performance on startup:
- The cohort drops each network lock and the runner in the same tab asks for it again with
ifAvailableright away. If the release is still in flight, the first cycle is a falseNotLeaderthat marks the chain ready. import_dumpsdecides on index deferral before it waits for another tab's import. So it can still drop and rebuild indexes on a database that was populated during the wait.- The cohort
ANALYZErepeats theANALYZEthat each cold target's first catch-up cycle runs anyway.
claude-opus-5-5 · high · 23 min
|
@coderabbitai review |
|
|
@rain-marvin review |
|
🔎 Reviewing |
There was a problem hiding this comment.
This PR makes browser startup import the seeds of all cold networks together. It downloads them in parallel, holds the bootstrap lock and each cold network's leadership lock, and on a fresh DB drops the non-unique indexes once, imports every seed in one transaction, and rebuilds the indexes once. Fully warm networks start their loops at once. Each held network lock passes directly to that network's runner. A shared import Web Lock now covers the watermark and index preflight through commit or cancel, for both cohort and single imports.
All six earlier threads are resolved, and the code at 3a85402 matches each resolution: per-target download results, warm networks first, index deferral only on a fresh DB, network lock exclusion, a guard handoff with no release gap, a serialized import preflight, and no extra cohort ANALYZE. The review found no blocking defect. Two minor points remain. On a populated DB the cohort makes small cold networks wait for large ones but gives no shared rebuild. The startup orchestration in start(), which the last round fixed, has no direct test.
claude-opus-5-5 · high · 14 min
|
@rain-marvin review |
|
🔎 Reviewing |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
On a fresh browser database, this PR downloads the seed dumps of all missing networks in parallel and imports them in one atomic sqlite-web session. Non-unique indexes are dropped and rebuilt once, after the last seed. Web Locks keep the cohort, the per-network runners and other tabs from writing the same target. Each network lock goes straight to that network's runner. A populated database skips the cohort, and each network starts on its own with its indexes kept.
My overall read: the head 1f71d21 holds up. The panel checked lock ordering, guard lifetimes over Ok/Err/stop, the cross-tab import lock before the watermark and index preflight, partial download failure, the populated-DB recheck after the bootstrap lock wait, and atomic rollback. No lane found a defect at the blocking bar. All nine earlier threads are resolved, and the code shows the fixes in place. Two small cleanups are left: some warm-target handling in the cohort can no longer run since the last round, and one line in ARCHITECTURE.md overstates the two-minute request bound.
claude-opus-5-5 · high · 23 min
|
@coderabbitai review |
|
|
@rain-marvin approve |
ueco-jb
left a comment
ueco-jb
left a comment
There was a problem hiding this comment.
Lock order is bootstrap, then network ownership, then the tab mutex, then the import lock, with no cycle. Ownership guards survive both outcomes and are handed to each runner, the concatenated seeds export disjoint rows per target, and DROP/CREATE INDEX stay in the same transaction, so a rollback restores them. A cohort or download failure falls back to per-network provisioning without a second import of a committed target.
Merge activity
|
#2898) Browser startup imports the missing networks together on a fresh database, then rebuilds shared secondary indexes once. This layer above #2897 addresses the cross-network startup cost in [RAI-2651](https://linear.app/makeitrain/issue/RAI-2651/integrate-bulk-sql-bootstrap-and-deferred-indexes-in-raindex-browser). **Live effect:** coordinated initial browser imports · **Risk:** medium (stored data, startup scheduling) · **Ships:** next raindex/webapp release after #2897 ## Decisions - On a fresh DB with at least two owned networks, download eligible seeds in parallel and import successful downloads atomically in one bounded sqlite-web session. Rebuild the 41 non-unique secondary indexes once; PRIMARY KEY/UNIQUE constraints remain enforced. Populated DBs start independent runners and retain their indexes. Smaller cohorts hand their guards to individual runners and preserve fresh-DB deferral without cohort downloads/timeouts. - Hold shared bootstrap and per-network ownership through provisioning; transfer each network guard directly into its runner. A shared import Web Lock serializes watermark/index preflight through commit or cancellation, including fallback imports. Recheck DB state after waiting for ownership; unsupported ownership retains indexes. - Failed downloads retry per target; import/manifest failure returns to normal provisioning. Catch-up analyzes applied data without an extra cohort ANALYZE. Producer artifacts, schema, native import and incremental writes retain their behavior. ## Risks - Small cold networks wait for the largest participating seed. Decoded cohort SQL stays in memory, and reads wait behind the atomic import. Coordinated manifest and seed requests are bounded to two minutes each; per-network runner requests keep their existing behavior, and fallback may lose shared deferral. - A seed already at finalized head receives planner statistics after the next applied window. Worker loss during commit can leave its result unknown; subsequent provisioning checks persisted watermarks. ## Proof - Frozen Base + Robinhood comparison on `3a85402`: parent **60.2 s**, cohort **33.9 s** to both ACTIVE; one 30.9 s import, 897 chunks, one rebuild and no extra ANALYZE. All table counts/index definitions/watermarks match; integrity is `ok`. Warm reload: **0.73 s**, zero seed imports. Fixtures: 42.4 MB gzip / 234 MB SQL, about 470 MB stored origin usage. - These are local observations, not a guaranteed speedup. Earlier runs ranged from 31.7–37.0 s, and one took 147.8 s during heavy observed Spotlight activity; parent timing also varied. No production latency or peak-memory claim. - Nix checks and staged independent local reviews cover atomic rollback, uniqueness, cancellation, partial downloads, leadership handoff and cross-tab import preflight. Latest `e240135` [deployed preview](https://rain-orderbook-v6-gaj9om8b9-rain-x-h20.vercel.app/orders) passed a cold import, Orders/Vaults lists, Base order detail with a trade and Robinhood vault detail with balance history; both networks reached ACTIVE and are healthy/ready. The Base seed download took 23.2 s in this functional run, so it is not an import-speed comparison. Warm reload downloads no SQL seed and keeps list data/readiness. Selection was restricted to Base/Robinhood because the existing Ethereum subgraph request remained pending. - Runtime head `1f71d21` also skips coordination on any populated DB, including after waiting for the bootstrap lock, and names synthetic stop/error tests accurately. Fresh Nix checks on `e240135` pass: 1,926 native, 153 Node/Wasm, 151 actual browser/Wasm and 1,168 JS tests; builds, lint, types and Rust static checks pass. Renewed staged local review is clean (two independent Codex reviewers; no OpenCode). - The final single-network bypass was exercised with a frozen Base fixture on `e240135`: one manifest fetch, one seed download, one import, one 41-index rebuild, matching watermark, integrity `ok`, and Base healthy/ready/ACTIVE. No live indexing; this is functional evidence, not a paired speed comparison. - Final `e240135` addresses Marvin’s last minor follow-up: fewer than two owned networks bypass cohort downloads while retaining direct guard handoff and the existing fresh-DB deferred import. All 12 review threads are resolved. Both final-commit bot gates pass: CodeRabbit has no actionable comments and Marvin approved with no open findings. Application CI and the deployed preview pass. Human reviews requested from Jakub, Juan, Alex and Rouz. The known Solidity static failure is accepted separately; human review remains pending. ## Rollout 1. Merge after #2897, release raindex and deploy the webapp. Existing manifests/dumps and sqlite-web 0.0.4 remain supported; this layer needs no producer change. 2. Verify both networks reach ACTIVE, a single rebuild spans the cold cohort, and warm startup downloads no seed. 3. Roll back the raindex/webapp release if startup regresses; schema is unchanged.
8b4438e to
c70fa66
Compare
e240135 to
57e82e0
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved. Approval is disabled; enable |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@rain-marvin approve |
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
This PR changes browser startup on a fresh local DB. When at least two networks are owned, it downloads their SQL seeds in parallel, imports the successful ones in one atomic session, and rebuilds the shared non-unique indexes once. A populated DB, or a cohort of fewer than two owned networks, skips coordination, and each runner uses the parent path. Network leadership guards are kept outside the provisioning future and handed directly to each runner. A shared import Web Lock serializes the watermark and index preflight through commit.
Since my last review (e240135)
The branch was squashed into one commit (57e82e0) and rebased onto the updated parent arda/rai-2651-bulk-sql-bootstrap (c70fa66). All 9 files the PR touches are byte-identical at e240135 and 57e82e0. Everything else in git diff e240135 57e82e0 comes from the base: Solidity contracts and tests, the subgraph, and the subgraph deploy workflow (#2894). None of it touches the local DB code. So there is no new PR code to check, and I found no new defects.
Earlier findings
- My last review had no open findings. The lone-network cohort finding (
scheduler/wasm.rs:263) is still resolved: thecohort_settings.syncs.len() < 2early return is unchanged after the network-lock loop.
Threads resolved by hand (findolor)
- Extra cohort
ANALYZE(bootstrap.rs:142): addressed in code. The cohortANALYZEis gone, and each network's first catch-up apply does the analysis. start()orchestration tests (scheduler/wasm.rs:674): addressed with the narrow option I offered. The tests are named for whatprovision_until_stoppedcovers, and thewith_initial_leadershipdoc says ownership lasts for the runner's lifetime.- Two-minute bound wording (
ARCHITECTURE.md:164): addressed. ARCHITECTURE.md limits the bound to coordinated manifest and seed requests, and the PR risk line says the same.
All other threads were mine and were resolved after I confirmed the fixes. No open findings remain.
claude-opus-5-5 · high · 40 s
|
@coderabbitai approve |
✅ Action performedComments resolved. Approval is disabled; enable |
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Browser startup imports the missing networks together on a fresh database, then rebuilds shared secondary indexes once. This layer above #2897 addresses the cross-network startup cost in RAI-2651.
Live effect: coordinated initial browser imports · Risk: medium (stored data, startup scheduling) · Ships: next raindex/webapp release after #2897
Decisions
Risks
Proof
3a85402: parent 60.2 s, cohort 33.9 s to both ACTIVE; one 30.9 s import, 897 chunks, one rebuild and no extra ANALYZE. All table counts/index definitions/watermarks match; integrity isok. Warm reload: 0.73 s, zero seed imports. Fixtures: 42.4 MB gzip / 234 MB SQL, about 470 MB stored origin usage.e240135deployed preview passed a cold import, Orders/Vaults lists, Base order detail with a trade and Robinhood vault detail with balance history; both networks reached ACTIVE and are healthy/ready. The Base seed download took 23.2 s in this functional run, so it is not an import-speed comparison. Warm reload downloads no SQL seed and keeps list data/readiness. Selection was restricted to Base/Robinhood because the existing Ethereum subgraph request remained pending.1f71d21also skips coordination on any populated DB, including after waiting for the bootstrap lock, and names synthetic stop/error tests accurately. Fresh Nix checks one240135pass: 1,926 native, 153 Node/Wasm, 151 actual browser/Wasm and 1,168 JS tests; builds, lint, types and Rust static checks pass. Renewed staged local review is clean (two independent Codex reviewers; no OpenCode).e240135: one manifest fetch, one seed download, one import, one 41-index rebuild, matching watermark, integrityok, and Base healthy/ready/ACTIVE. No live indexing; this is functional evidence, not a paired speed comparison.e240135addresses Marvin’s last minor follow-up: fewer than two owned networks bypass cohort downloads while retaining direct guard handoff and the existing fresh-DB deferred import. All 12 review threads are resolved. Both final-commit bot gates pass: CodeRabbit has no actionable comments and Marvin approved with no open findings. Application CI and the deployed preview pass. Human reviews requested from Jakub, Juan, Alex and Rouz. The known Solidity static failure is accepted separately; human review remains pending.Rollout
Summary by CodeRabbit