Fix database corruption by adding wipe_and_recreate support#2418
Merged
Fix database corruption by adding wipe_and_recreate support#2418
Conversation
Adds a default implementation that returns an error, allowing executors to optionally support wiping and recreating the database.
Extends JsCallbackExecutor to accept an optional wipe callback that can be invoked to wipe and recreate the database. Includes tests for success, missing callback, and error propagation cases.
Updates LocalDb to forward wipe_and_recreate calls to the underlying executor and updates from_js_callback to accept both query and wipe callbacks.
Changes set_local_db_callback to accept an optional wipe callback parameter. Updates all call sites in tests to use the new signature.
When integrity check fails, the bootstrap pipeline now calls wipe_and_recreate before resetting the database schema. This ensures corrupted database files are properly cleaned up. Adds tests for wipe behavior on corruption and error propagation.
Passes localDb.wipeAndRecreate to setDbCallback so the database can be wiped when corruption is detected.
Contributor
WalkthroughAdds an async wipe_and_recreate method to LocalDbQueryExecutor, wires an optional JS wipe callback through JsCallbackExecutor → LocalDb → RaindexClient, and invokes wipe_and_recreate from the bootstrap reset path for unhealthy DBs; many tests and call sites updated and multiple executors have not-implemented stubs. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/common/src/raindex_client/local_db/executor.rs`:
- Around line 132-159: The wipe_and_recreate method currently runs without
acquiring the operation serialization lock and can race with queries/batches;
modify wipe_and_recreate to acquire the same serialize mutex used by other DB
operations (e.g., call self.serialize.lock().await and hold the guard for the
duration of the function) before invoking wipe_callback, promise resolution, and
deserialization so the entire method is executed under the same lock and
prevents concurrent DB access.
In `@packages/webapp/src/routes/`+layout.ts:
- Around line 87-89: The current call to raindexClient.setDbCallback uses
localDb.wipeAndRecreate.bind(localDb) which will throw if wipeAndRecreate is
undefined; update the invocation to guard the binding by checking the method
exists (e.g., pass localDb.query.bind(localDb) and for the wipe callback pass
localDb.wipeAndRecreate ? localDb.wipeAndRecreate.bind(localDb) : undefined) or
otherwise adapt to raindexClient.setDbCallback’s expected signature, referencing
the symbols raindexClient.setDbCallback, localDb.query, and
localDb.wipeAndRecreate so the init path won’t throw when sqlite-web lacks
wipeAndRecreate.
Comment on lines
87
to
89
| if (localDb && raindexClient) { | ||
| raindexClient.setDbCallback(localDb.query.bind(localDb)); | ||
| raindexClient.setDbCallback(localDb.query.bind(localDb), localDb.wipeAndRecreate.bind(localDb)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Guard wipeAndRecreate binding or verify sqlite-web version.
If a sqlite-web build (or test double) doesn’t expose wipeAndRecreate, the .bind call will throw during app init. Consider a safe optional binding or confirm the dependency/typings are updated.
🔧 Suggested defensive binding
- raindexClient.setDbCallback(localDb.query.bind(localDb), localDb.wipeAndRecreate.bind(localDb));
+ const wipeCallback = localDb.wipeAndRecreate?.bind(localDb);
+ raindexClient.setDbCallback(localDb.query.bind(localDb), wipeCallback);🤖 Prompt for AI Agents
In `@packages/webapp/src/routes/`+layout.ts around lines 87 - 89, The current call
to raindexClient.setDbCallback uses localDb.wipeAndRecreate.bind(localDb) which
will throw if wipeAndRecreate is undefined; update the invocation to guard the
binding by checking the method exists (e.g., pass localDb.query.bind(localDb)
and for the wipe callback pass localDb.wipeAndRecreate ?
localDb.wipeAndRecreate.bind(localDb) : undefined) or otherwise adapt to
raindexClient.setDbCallback’s expected signature, referencing the symbols
raindexClient.setDbCallback, localDb.query, and localDb.wipeAndRecreate so the
init path won’t throw when sqlite-web lacks wipeAndRecreate.
Collaborator
Author
There was a problem hiding this comment.
The @rainlanguage/sqlite-web package will be updated to include wipeAndRecreate before this PR is merged, so the method will always exist. No defensive binding needed.
Contributor
There was a problem hiding this comment.
@findolor, thank you for the clarification! With the @rainlanguage/sqlite-web dependency being updated to include wipeAndRecreate as part of the coordinated merge, the binding is indeed safe and no defensive handling is needed. Appreciate the context about the dependency update timing.
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2111
File: packages/webapp/src/routes/db-playground/+page.svelte:149-153
Timestamp: 2025-10-06T10:23:57.479Z
Learning: The db-playground page at packages/webapp/src/routes/db-playground/+page.svelte is used for testing purposes, and production-level concerns (such as invalid RPC tokens) are acceptable in this context.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1917
File: tauri-app/src/lib/stores/order.ts:10-15
Timestamp: 2025-06-11T11:27:14.391Z
Learning: In this codebase, Svelte writable/derived stores (e.g., `subgraph` in `tauri-app/src/lib/stores/settings.ts`) expose a custom asynchronous `.load()` helper that retrieves the current value, so calls like `await subgraph.load()` are valid.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2145
File: crates/common/src/raindex_client/local_db/query/create_tables/query.sql:71-72
Timestamp: 2025-10-06T11:44:07.888Z
Learning: The local DB feature in the rain.orderbook codebase is not live yet (as of PR `#2145`), so schema migrations for existing databases are not required when modifying table structures in `crates/common/src/raindex_client/local_db/query/create_tables/query.sql`.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2278
File: packages/webapp/src/lib/services/startLocalDbSync.ts:23-29
Timestamp: 2025-10-29T13:29:29.578Z
Learning: In packages/webapp/src/lib/services/startLocalDbSync.ts, the hard-coded default orderbook address '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' is a temporary placeholder that will be removed when new sync logic is introduced in the Raindex client, as documented in the NOTE comment.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 2246
File: packages/webapp/src/hooks.client.ts:2-2
Timestamp: 2025-10-19T20:24:08.629Z
Learning: In the rain.orderbook repository, the import `rainlanguage/orderbook/esm` in packages/webapp/src/hooks.client.ts is intentional and should not be changed to the root package path, as it's specific to the webapp build configuration.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2376
File: crates/common/src/raindex_client/orders.rs:2-2
Timestamp: 2026-01-07T14:21:44.132Z
Learning: When local DB is enabled in the rain.orderbook codebase, the intended design is to use it exclusively without falling back to the subgraph, even when results are empty. Methods like get_trades_list() in trades.rs and get_vaults_volume() in orders.rs follow this pattern—they return local DB results directly when is_chain_supported_local_db() and local_db is available. Mixing data sources could cause inconsistencies. The get_balance_changes() method in vaults.rs that checks !local_changes.is_empty() before returning is an outlier that needs to be fixed to align with the exclusive local DB pattern.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2155
File: crates/common/src/raindex_client/trades.rs:133-152
Timestamp: 2025-10-06T14:13:18.531Z
Learning: In the rain.orderbook codebase, the `page` parameter in `RaindexOrder::get_trades_list` method (in crates/common/src/raindex_client/trades.rs) is kept for backwards compatibility with subgraph logic, but the LocalDb fast-path intentionally returns all trades without implementing pagination.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2000
File: crates/common/src/raindex_client/vaults.rs:183-183
Timestamp: 2025-07-16T10:40:05.717Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor considers breaking changes from Option<U256> to U256 for required fields like decimals in RaindexVaultToken to be acceptable and safe, even when they affect multiple usage sites across the codebase.
0xgleb
requested changes
Jan 28, 2026
Comment on lines
+20
to
+24
| async fn wipe_and_recreate(&self) -> Result<(), LocalDbQueryError> { | ||
| Err(LocalDbQueryError::database( | ||
| "wipe_and_recreate not supported", | ||
| )) | ||
| } |
Collaborator
There was a problem hiding this comment.
i don't think it's a default method implementation that just errors is a good idea. if an impl needs this, i think it would be better to have that explicitly in the impl
Remove default implementation that returned an error and require all LocalDbQueryExecutor implementations to explicitly implement wipe_and_recreate. Add NotImplemented error variant to LocalDbQueryError for implementations that don't support this operation.
Add healthy_db_callback that returns ok for integrity check queries so bootstrap doesn't trigger wipe_and_recreate.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/cli/src/commands/local_db/executor.rs`:
- Around line 168-170: Implement the wipe_and_recreate method so CLI recovery
can succeed: replace the NotImplemented error in wipe_and_recreate with logic
that deletes the local DB storage (e.g., remove DB file(s)/directory used by the
CLI executor), recreates an empty database/storage in the same location, and
returns Ok(()) on success; map/convert any IO or DB errors into
LocalDbQueryError (use its existing constructors) and ensure resources are
properly closed before deleting; locate the method named wipe_and_recreate in
the CLI executor implementation and update it to perform the
remove-then-recreate flow rather than returning
LocalDbQueryError::not_implemented("wipe_and_recreate").
In `@package.json`:
- Line 78: The dependency "@rainlanguage/sqlite-web" at version "0.0.2"
introduces many vulnerabilities (including critical/high)—do not merge; run a
full npm/yarn audit and identify whether the issues are transitive from
sub-dependencies or come from the package itself, attempt to upgrade to a
non-vulnerable version or replace the package, check the package repo/issue
tracker and contact the maintainers about fixes, consider applying temporary
mitigations (shrinkwrap/patch-package, restrict usage, or remove the dependency)
and get explicit security approval if you must keep it despite unresolved
critical vulnerabilities.
Comment on lines
+168
to
+170
| async fn wipe_and_recreate(&self) -> Result<(), LocalDbQueryError> { | ||
| Err(LocalDbQueryError::not_implemented("wipe_and_recreate")) | ||
| } |
Contributor
There was a problem hiding this comment.
Implement wipe_and_recreate for the CLI executor.
Bootstrap now calls this on corruption; returning NotImplemented makes the recovery path fail for CLI/local runs.
🐛 Proposed fix
- async fn wipe_and_recreate(&self) -> Result<(), LocalDbQueryError> {
- Err(LocalDbQueryError::not_implemented("wipe_and_recreate"))
- }
+ async fn wipe_and_recreate(&self) -> Result<(), LocalDbQueryError> {
+ let db_path = self.db_path.clone();
+ spawn_blocking(move || {
+ let path_str = db_path.to_string_lossy();
+ let candidates = [
+ db_path.clone(),
+ PathBuf::from(format!("{path_str}-wal")),
+ PathBuf::from(format!("{path_str}-shm")),
+ ];
+ for path in candidates {
+ match std::fs::remove_file(&path) {
+ Ok(_) => {}
+ Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
+ Err(e) => {
+ return Err(LocalDbQueryError::database(format!(
+ "Failed to remove db file {}: {e}",
+ path.display()
+ )))
+ }
+ }
+ }
+ Ok(())
+ })
+ .await
+ .map_err(join_err)?
+ }🤖 Prompt for AI Agents
In `@crates/cli/src/commands/local_db/executor.rs` around lines 168 - 170,
Implement the wipe_and_recreate method so CLI recovery can succeed: replace the
NotImplemented error in wipe_and_recreate with logic that deletes the local DB
storage (e.g., remove DB file(s)/directory used by the CLI executor), recreates
an empty database/storage in the same location, and returns Ok(()) on success;
map/convert any IO or DB errors into LocalDbQueryError (use its existing
constructors) and ensure resources are properly closed before deleting; locate
the method named wipe_and_recreate in the CLI executor implementation and update
it to perform the remove-then-recreate flow rather than returning
LocalDbQueryError::not_implemented("wipe_and_recreate").
Collaborator
Author
There was a problem hiding this comment.
The CLI executor doesn't need wipe_and_recreate implemented. The database corruption issue this PR addresses is specific to the browser environment using sqlite-web. CLI users have direct file system access and can delete a corrupted database file manually if needed. The NotImplemented stub is sufficient here.
Contributor
There was a problem hiding this comment.
@findolor, thank you for the clarification! That makes perfect sense—the browser environment requires automated recovery through wipe_and_recreate since users can't directly access the file system, while CLI users can simply remove the corrupted database files manually. I appreciate you explaining the different execution contexts.
✏️ Learnings added
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2418
File: crates/cli/src/commands/local_db/executor.rs:168-170
Timestamp: 2026-01-28T14:11:12.683Z
Learning: In the rain.orderbook codebase, the `wipe_and_recreate` method on `LocalDbQueryExecutor` is primarily needed for the browser/WASM environment (JsCallbackExecutor with sqlite-web) to handle database corruption automatically. The CLI executor (RusqliteExecutor in `crates/cli/src/commands/local_db/executor.rs`) can return `NotImplemented` for `wipe_and_recreate` because CLI users have direct file system access and can manually delete corrupted database files if needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2159
File: crates/cli/src/commands/local_db/sync/runner/mod.rs:52-113
Timestamp: 2025-10-06T14:41:41.909Z
Learning: The local DB sync CLI command (crates/cli/src/commands/local_db/sync/) is designed for CI-only usage, and simple println! statements are preferred over structured logging for status messages.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2123
File: crates/common/src/raindex_client/local_db/mod.rs:23-29
Timestamp: 2025-10-06T11:13:29.956Z
Learning: In `crates/common/src/raindex_client/local_db/mod.rs`, the `Default` implementation for `LocalDb` that creates an RPC client pointing to `http://localhost:4444` is acceptable because the RPC client must be explicitly configured before actual usage in production scenarios.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2145
File: crates/common/src/raindex_client/local_db/query/create_tables/query.sql:71-72
Timestamp: 2025-10-06T11:44:07.888Z
Learning: The local DB feature in the rain.orderbook codebase is not live yet (as of PR `#2145`), so schema migrations for existing databases are not required when modifying table structures in `crates/common/src/raindex_client/local_db/query/create_tables/query.sql`.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2376
File: crates/common/src/raindex_client/orders.rs:2-2
Timestamp: 2026-01-07T14:21:44.132Z
Learning: When local DB is enabled in the rain.orderbook codebase, the intended design is to use it exclusively without falling back to the subgraph, even when results are empty. Methods like get_trades_list() in trades.rs and get_vaults_volume() in orders.rs follow this pattern—they return local DB results directly when is_chain_supported_local_db() and local_db is available. Mixing data sources could cause inconsistencies. The get_balance_changes() method in vaults.rs that checks !local_changes.is_empty() before returning is an outlier that needs to be fixed to align with the exclusive local DB pattern.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/raindex_client/remove_orders.rs (1)
245-278: Consider consolidating duplicateCountingJsonExectest helper.
CountingJsonExecis duplicated with identical implementation in bothadd_orders.rsandremove_orders.rs. If more test modules need this pattern, consider extracting it to a shared test utilities module (e.g.,#[cfg(test)] mod test_utilsor similar) to reduce duplication.This is a minor DRY concern and keeping test helpers local is also acceptable for test isolation.
0xgleb
approved these changes
Jan 29, 2026
hardyjosh
approved these changes
Jan 29, 2026
Contributor
|
@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:
|
Contributor
|
The author of this PR is on the CodeRabbit Free Plan. In order to use the Chat feature, please upgrade the PR author to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dependent PRs
Motivation
See issues:
When the local database becomes corrupted (malformed database schema), the previous fix in #2387 did not fully resolve the issue. The database file needs to be wiped and recreated to recover from corruption, rather than just clearing tables.
Solution
Add
wipe_and_recreatefunctionality that allows the bootstrap pipeline to fully reset a corrupted database:wipe_and_recreatemethod toLocalDbQueryExecutortrait with a default error implementationJsCallbackExecutorto accept an optional wipe callback alongside the query callbackLocalDbabstractionsetDbCallbackto accept both query and wipe callbackswipe_and_recreatebefore resetting schema when integrity check failslocalDb.wipeAndRecreateto the clientThis works in conjunction with the sqlite-web library update (PR #27) which implements the actual database file deletion and recreation logic.
Checks
By submitting this for review, I'm confirming I've done the following:
fix #2385
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.