feat(database): add application errors and safe schema transactions - #739
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (69)
✨ 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9defb44208
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sqlx::query("PRAGMA busy_timeout = 5000") | ||
| .execute(transaction.connection()) | ||
| .await?; |
There was a problem hiding this comment.
Executing a schema transaction permanently overwrites the acquired connection's busy_timeout with 5000 ms because cleanup restores only foreign_keys. This is directly observable with a single-connection in-memory database, and it also affects whichever pooled file connection is reused: a program that configured a shorter or longer timeout before the migration silently gets different lock-wait behavior afterward, including after failed begins or rollbacks. Capture the previous pragma value and restore it before returning the connection to the pool.
Useful? React with 👍 / 👎.
| )), | ||
| }; | ||
| } | ||
| restore_connection(self.connection.take().expect("owned connection")).await |
There was a problem hiding this comment.
If COMMIT succeeds but restoring foreign_keys fails, this returns an ordinary error whose cleanup message tells the caller to retry even though the schema and migration-ledger writes are already durable. A caller that retries the migration rather than re-reading its ledger can repeat non-idempotent work or report a false rollback. Preserve the committed outcome explicitly in this error path and direct callers to inspect the committed state instead of treating it as an aborted transaction.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| sqlx::query("PRAGMA busy_timeout = 5000") | ||
| .execute(transaction.connection()) | ||
| .await?; |
There was a problem hiding this comment.
🟡 Schema transactions overwrite busy timeout
After a schema transaction, busy_timeout remains 5000 instead of retaining the connection's prior setting. In-memory handles reuse that connection, so later ordinary statements can unexpectedly wait five seconds.
Learn more
PRAGMA busy_timeout is connection-local and remains active until changed. Schema mode changes it only to bound BEGIN IMMEDIATE, but successful commit, rollback, failed setup, and cancellation all return or close the connection without restoring the previous value. This is deterministic for sqlite::memory:, whose pool contains exactly one connection.
Example: A program sets PRAGMA busy_timeout = 100, runs one schema transaction, then reads PRAGMA busy_timeout. It gets 5000, and a later ordinary write can wait five seconds instead of 100 milliseconds.
Recommended fix: Read and retain the connection's existing busy_timeout before changing it. Restore that value alongside foreign_keys on every success, rollback, setup-failure, and cancellation cleanup path; discard the connection if restoration fails.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Value::Text(message) if !message.trim().is_empty() => { | ||
| Err(RuntimeError::new(message.to_string(), 0, 0)) | ||
| } |
There was a problem hiding this comment.
🟡 Expression-form errors expose runtime wrapper
With raise_error of message, error_message includes an internal native-function and line-zero wrapper. The FunctionCall path stringifies native errors, unlike ActionCall, so the documented forms produce different diagnostics.
Learn more
The parser represents raise_error of message as Expression::FunctionCall, while call raise_error with message becomes Expression::ActionCall. The native returns a RuntimeError containing only the application message. The expression evaluator converts that error to text and wraps it in a new RuntimeError, adding internal location and function text; the action-call evaluator only updates the original location.
Example: raise_error of "Save failed" is caught with an error_message resembling Error in native function: Runtime error at line 0, column 0: Save failed. call raise_error with "Save failed" yields Save failed.
Recommended fix: Make native invocation through Expression::FunctionCall preserve the original RuntimeError and replace only its call-site line and column, matching the Expression::ActionCall path. Check existing native-call diagnostics before applying this behavior globally.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let begin = async { | ||
| let connection = pool.acquire().await?; | ||
| // Install the cancellation guard before touching connection state. | ||
| let mut transaction = Self { | ||
| connection: Some(connection), | ||
| }; | ||
| sqlx::query("PRAGMA busy_timeout = 5000") | ||
| .execute(transaction.connection()) | ||
| .await?; | ||
| sqlx::query("PRAGMA foreign_keys = OFF") | ||
| .execute(transaction.connection()) | ||
| .await?; | ||
| sqlx::query("BEGIN IMMEDIATE") | ||
| .execute(transaction.connection()) | ||
| .await?; | ||
| Ok::<_, sqlx::Error>(transaction) | ||
| }; | ||
| match tokio::time::timeout(LOCK_WAIT, begin).await { |
There was a problem hiding this comment.
🔍 Lock diagnostic conflates pool exhaustion
The five-second bound includes pool.acquire(), so pool exhaustion reports a competing database writer. Review whether acquisition needs a distinct diagnostic or timeout.
Was this helpful? React with 👍 or 👎 to provide feedback.
…-prerequisites # Conflicts: # Docs/reference/keyword-reference.md # Docs/reference/reserved-keywords.md # src/fixer/source.rs
# Conflicts: # Docs/05-standard-library/core-module.md # src/builtins.rs # src/stdlib/core.rs
1 participant
Summary
Add catchable application errors and SQLite schema transactions so WFL migration libraries can validate data, fail atomically, and rebuild tables without deleting referencing extension rows.
The reproducer demonstrated that ordinary table rebuilds with foreign keys enabled could delete referencing rows. Libraries also lacked a deliberate error primitive for validation failures that must unwind a transaction.
Changes
raise_errorwith safe text validation and normal catchable error propagation.in transaction on db for schema changes:: pin the owned SQLite connection, disable foreign keys beforeBEGIN IMMEDIATE, check foreign keys before commit, and restore the connection on success, failure, and cancellation.Compatibility and risk
Risk class R3: database durability, cancellation, resource ownership, and backward compatibility. Ordinary transactions retain their existing behavior, including normal returns committing. Schema mode is explicit and SQLite-specific. Rebuild code remains responsible for preserving intended schema objects and data; the runtime provides atomicity and foreign-key validation.
Independent review found a canceled-handler ownership leak. The final implementation adds scope cleanup and verifies restoration of the same in-memory connection. A connection whose safe state cannot be restored is discarded; losing the only connection of an in-memory database requires reopening that handle. No production database was changed.
Validation
8d82d785ea59300834de1c48b04a7ba0e187a1cd; GitHub readback confirmsmerged: true. Post-merge CI35506031173 passed on this merge, including every required job and the automatic version bump. The bump pusheda1249033b0f1cff87ecea8282619808f727d22c3and tagv26.9.15; version synchronization, both lockfiles, the locked fuzz check, and pre-push hygiene passed. Post-merge program totals remained Linux 181 / Windows 180 with zero failures or timeouts; both integration gates passed 157 programs, documentation 36/36 and web 3/3. Docker35506031179, config lint35506031178, Auto Format35506031176 and CodeQL35506030920 also passed.5d87d9b7records the unsafe rebuild and missing application-error capability. Green source:d450dee9; initial evidence:9defb442.9defb44208fd3fdc10c3758b3dc1f3cc56706251, including actual new WFL suites on Linux and Windows.59709aed97d045dbd9a04093fff5c32ae60a95e3. A fresh release build passes all 48/48 new WFL cases (17 schema/error, eight HTTP, 23 process), 147/147 existing Rust compatibility tests, strict Clippy, formatting, locked fuzz compilation, documentation 36/36, and static hygiene. Runtime source and test trees match the earlier combined consumer candidatedf6ad9a2.Checklist