refactor(db): transaction safety, query consolidation, error diagnostics - #1109
refactor(db): transaction safety, query consolidation, error diagnostics#1109zhaoyuan2024 wants to merge 1 commit into
Conversation
Wrap multi-step DB operations in transactions, consolidate duplicated
queries, and add operation context to error messages for easier
debugging.
Transaction safety:
- run_migrations: wrap all migrations + schema_version update in a
single transaction; failed migrations roll back cleanly
- reset_active_boxes_after_reboot: replace N+1 (list + per-box update)
with a single transaction + batch UPDATE
- remove_all_refs_for_box: wrap SELECT + DELETE in a transaction
Query consolidation:
- load(): merge two separate queries into a single JOIN
- list_all/list_active: extract query_boxes() helper + shared
deserialize_config/deserialize_state functions (~25 lines deduped)
- images.rs: extract row_to_cached_image() helper (named-column
access, shared between get() and list_all())
Error diagnostics:
- db_err! macro: add patterns that accept context string and format
args (db_err!(result, "op(box={})", id))
- Apply context to key write operations: save, update_state,
reset_active_boxes_after_reboot, base_disk insert, image upsert,
remove_all_refs_for_box, run_migrations
SQLite tuning:
- synchronous=FULL -> NORMAL (WAL-recommended; 2-5x write improvement)
- Add mmap_size=256MB for faster reads on large JSON blobs
- is_empty(): use EXISTS instead of COUNT(*)
📦 BoxLite review — couldn't completepowered by BoxLite |
|
|
📝 WalkthroughWalkthroughThe database layer adds contextual errors, shared JSON and image row parsing, atomic box and disk updates, direct existence checks, and transactional migrations. SQLite initialization also changes synchronization and memory-mapped database settings. ChangesDatabase integrity and access
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SQLiteOpen
participant run_migrations
participant v8_to_v9
participant SchemaVersion
SQLiteOpen->>run_migrations: begin migration transaction
run_migrations->>v8_to_v9: execute migration updates
v8_to_v9-->>run_migrations: return migration result
run_migrations->>SchemaVersion: update schema version
run_migrations->>SQLiteOpen: commit transaction
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
🟡 Not ready to approve
Several new db_err! call sites use {var} capture syntax inside plain string literals (so values won’t be interpolated), undermining the PR’s error-diagnostics goal and producing misleading error context.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR refactors Boxlite’s SQLite persistence layer to make multi-step DB operations safer (via transactions), reduce duplicated query/row-mapping code, and improve debugging by attaching operation context to database errors. It also adjusts SQLite PRAGMAs for better WAL-mode performance.
Changes:
- Add transactional safety to migrations and multi-statement DB workflows (e.g., migration batches, base-disk ref cleanup, active-box reset).
- Consolidate/DRY common DB query logic and row deserialization (JOIN-based load, shared helpers for box config/state, shared image row mapping).
- Improve DB error diagnostics by extending
db_err!to accept context strings and formatted context.
File summaries
| File | Description |
|---|---|
| src/boxlite/src/db/mod.rs | Extends db_err! with contextual variants; updates SQLite PRAGMAs; adds tests for rollback + macro behavior. |
| src/boxlite/src/db/migration/mod.rs | Wraps migrations + schema_version update in a single transaction. |
| src/boxlite/src/db/migration/v8_to_v9.rs | Removes inner transaction so migration updates participate in outer transaction. |
| src/boxlite/src/db/images.rs | Deduplicates row mapping with row_to_cached_image; optimizes is_empty() via EXISTS. |
| src/boxlite/src/db/boxes.rs | Adds shared JSON deserializers; consolidates load via JOIN; makes reboot reset atomic with a transaction; adds tests. |
| src/boxlite/src/db/base_disk.rs | Adds transactional wrapper + context to SELECT+DELETE cleanup for base disk refs. |
Review details
Suppressed comments (6)
src/boxlite/src/db/migration/mod.rs:81
- The commit context string uses
{source_version}/{current}capture syntax, but db_err!(result, ctx) won’t format it, so the commit error context will be incorrect.
db_err!(tx.commit(), "run_migrations(v{source_version} -> v{current}): commit")?;
src/boxlite/src/db/migration/mod.rs:79
- This db_err! context string includes
{current}in a string literal, so the version won’t be interpolated and diagnostics will be misleading.
db_err!(tx.execute(
"UPDATE schema_version SET version = ?1, updated_at = ?2 WHERE id = 1",
rusqlite::params![current, now],
), "run_migrations: update schema_version to v{current}")?;
src/boxlite/src/db/boxes.rs:313
- This db_err! context string uses
{id_str}capture syntax, but db_err!(result, ctx) doesn’t format it, so the updated box id won’t appear in the error context.
db_err!(tx.execute(
"UPDATE box_state SET status = ?1, pid = ?2, json = ?3 WHERE id = ?4",
params![state.status.as_str(), state.pid, new_json, id_str],
), "reset_active_boxes_after_reboot: update box={id_str}")?;
src/boxlite/src/db/base_disk.rs:252
- This db_err! context string uses
{box_id}capture syntax, but db_err!(result, ctx) doesn’t format it, so the select error context won’t include the box id.
let mut stmt =
db_err!(tx.prepare("SELECT base_disk_id FROM base_disk_ref WHERE box_id = ?1"),
"remove_all_refs_for_box(box={box_id}): select refs")?;
src/boxlite/src/db/base_disk.rs:271
- This db_err! context string uses
{box_id}capture syntax, but db_err!(result, ctx) doesn’t format it, so delete errors won’t include the box id in context.
db_err!(tx.execute(
"DELETE FROM base_disk_ref WHERE box_id = ?1",
rusqlite::params![box_id],
), "remove_all_refs_for_box(box={box_id}): delete")?;
src/boxlite/src/db/base_disk.rs:273
- This db_err! context string uses
{box_id}capture syntax, but db_err!(result, ctx) doesn’t format it, so commit errors won’t include the box id in context.
db_err!(tx.commit(), "remove_all_refs_for_box(box={box_id}): commit")?;
- Files reviewed: 6/6 changed files
- Comments generated: 5
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| ) -> BoxliteResult<()> { | ||
| let all = all_migrations(); | ||
|
|
||
| let tx = db_err!(conn.unchecked_transaction(), "run_migrations(v{source_version}): begin")?; |
| rusqlite::Error::FromSqlConversionFailure( | ||
| 2, // layers column index | ||
| rusqlite::types::Type::Text, | ||
| Box::new(e), | ||
| ) |
| let rows_affected = db_err!(conn.execute( | ||
| "UPDATE box_state SET status = ?1, pid = ?2, json = ?3 WHERE id = ?4", | ||
| params![state.status.as_str(), state.pid, json, box_id], | ||
| ))?; | ||
| ), "update_state(box={box_id})")?; |
| pub(crate) fn remove_all_refs_for_box(&self, box_id: &str) -> BoxliteResult<Vec<BaseDiskID>> { | ||
| let conn = self.db.conn(); | ||
| let mut conn = self.db.conn(); | ||
| let tx = db_err!(conn.transaction(), "remove_all_refs_for_box(box={box_id}): begin")?; |
| if image.complete { 1 } else { 0 } | ||
| ], | ||
| ))?; | ||
| ), "image_index upsert(ref={reference})")?; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/boxlite/src/db/boxes.rs (1)
111-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winContext string won't interpolate
{box_id}.
db_err!(..., "update_state(box={box_id})")uses the 2-argument form. Because of thedb_err!macro definition,{box_id}is not substituted with the actual box ID; the literal text{box_id}ends up in the error message instead of the real value. See the root-cause comment onsrc/boxlite/src/db/mod.rs(lines 31-46) for the fix.🤖 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 `@src/boxlite/src/db/boxes.rs` around lines 111 - 128, Update the db_err! invocation in update_state to use the macro’s formatting-aware form so box_id is interpolated into the context string. Preserve the existing database error handling and context text while ensuring failures report the actual box identifier instead of the literal "{box_id}".src/boxlite/src/db/base_disk.rs (1)
245-276: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGood atomic select+delete; all four context strings in this function have the same interpolation bug.
Wrapping the SELECT and DELETE for
base_disk_refin one transaction correctly prevents dangling refs from a crash between the two steps. That part is a solid fix.All four
db_err!calls in this function (lines 247, 251-252, 271, 273) use the 2-argument form with{box_id}embedded in the literal ("remove_all_refs_for_box(box={box_id}): begin",...: select refs,...: delete,...: commit). None of these substitute the actualbox_idvalue; the literal text{box_id}appears in each resulting error message. See the root-cause comment onsrc/boxlite/src/db/mod.rs(lines 31-46) for the fix.🤖 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 `@src/boxlite/src/db/base_disk.rs` around lines 245 - 276, Update all four db_err! calls in remove_all_refs_for_box—transaction begin, SELECT preparation, DELETE execution, and commit—to use the formatting form that interpolates the actual box_id value, following the established pattern in the database module. Preserve the existing error context and transaction behavior.
🤖 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 `@src/boxlite/src/db/boxes.rs`:
- Around line 280-321: Fix the error-context formatting in
reset_active_boxes_after_reboot by using the db_err! invocation form that
interpolates id_str in the update error message. Preserve the existing
transaction and update behavior while ensuring failures identify the actual box
ID.
In `@src/boxlite/src/db/images.rs`:
- Line 116: Update the db_err! invocation in the image index upsert path to use
the formatting form that interpolates the reference value, following the
established pattern in the db module. Ensure the resulting error context
contains the actual image reference rather than the literal `{reference}` text.
- Around line 40-66: Update row_to_cached_image so the layers JSON conversion
error uses row.as_ref().column_index("layers").unwrap_or(0) instead of a
hardcoded column index, preserving position-independent error reporting for both
get and list_all callers.
In `@src/boxlite/src/db/migration/mod.rs`:
- Around line 70-81: Update the db_err! calls for the schema-version UPDATE and
transaction commit in run_migrations to use the correct interpolation form,
passing current and source_version as formatting arguments so the actual version
values appear in both context messages.
- Around line 43-60: Update the begin-context argument in run_migrations to use
the db_err! formatting form that interpolates source_version, matching the
established pattern in the database module; preserve the existing transaction
behavior and error context.
In `@src/boxlite/src/db/mod.rs`:
- Around line 31-46: Update the `db_err!` macro and its call sites so context
formatting uses explicit arguments: keep the two-argument form only for direct
literal context strings, and rewrite dynamic or interpolated contexts to use the
three-argument form with the value passed separately. Preserve the existing
database error wrapping and context prefix behavior.
---
Outside diff comments:
In `@src/boxlite/src/db/base_disk.rs`:
- Around line 245-276: Update all four db_err! calls in
remove_all_refs_for_box—transaction begin, SELECT preparation, DELETE execution,
and commit—to use the formatting form that interpolates the actual box_id value,
following the established pattern in the database module. Preserve the existing
error context and transaction behavior.
In `@src/boxlite/src/db/boxes.rs`:
- Around line 111-128: Update the db_err! invocation in update_state to use the
macro’s formatting-aware form so box_id is interpolated into the context string.
Preserve the existing database error handling and context text while ensuring
failures report the actual box identifier instead of the literal "{box_id}".
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44369780-968a-465d-9565-b862dd9e5561
📒 Files selected for processing (6)
src/boxlite/src/db/base_disk.rssrc/boxlite/src/db/boxes.rssrc/boxlite/src/db/images.rssrc/boxlite/src/db/migration/mod.rssrc/boxlite/src/db/migration/v8_to_v9.rssrc/boxlite/src/db/mod.rs
| /// | ||
| /// All resets happen within a single transaction: either every active | ||
| /// box is reset or none are. This avoids the N+1 lock-acquisition + | ||
| /// autocommit pattern of the previous per-box `update_state` loop. | ||
| pub fn reset_active_boxes_after_reboot(&self) -> BoxliteResult<Vec<BoxID>> { | ||
| let active = self.list_active()?; | ||
| let mut reset_ids = Vec::new(); | ||
| let mut conn = self.db.conn(); | ||
| let tx = db_err!(conn.transaction(), "reset_active_boxes_after_reboot: begin")?; | ||
|
|
||
| // Collect active boxes within the transaction. | ||
| let active: Vec<(String, String)> = { | ||
| let mut stmt = db_err!(tx.prepare( | ||
| "SELECT c.id, s.json FROM box_config c | ||
| JOIN box_state s ON c.id = s.id | ||
| WHERE s.status IN ('starting', 'running', 'detached')" | ||
| ), "reset_active_boxes_after_reboot: select active")?; | ||
| let rows = db_err!(stmt.query_map([], |row| { | ||
| Ok((row.get::<_, String>(0)?, row.get::<_, String>(1)?)) | ||
| }))?; | ||
| db_err!(rows.collect::<Result<Vec<_>, _>>(), "reset_active_boxes_after_reboot: collect rows")? | ||
| }; | ||
|
|
||
| for (config, mut state) in active { | ||
| let mut reset_ids = Vec::new(); | ||
| for (id_str, state_json) in active { | ||
| let mut state: BoxState = deserialize_state(&state_json)?; | ||
| state.reset_for_reboot(); | ||
| self.update_state(config.id.as_str(), &state)?; | ||
| reset_ids.push(config.id); | ||
| let new_json = serde_json::to_string(&state).map_err(|e| { | ||
| BoxliteError::Database(format!( | ||
| "Failed to serialize state for box {id_str}: {e}" | ||
| )) | ||
| })?; | ||
| db_err!(tx.execute( | ||
| "UPDATE box_state SET status = ?1, pid = ?2, json = ?3 WHERE id = ?4", | ||
| params![state.status.as_str(), state.pid, new_json, id_str], | ||
| ), "reset_active_boxes_after_reboot: update box={id_str}")?; | ||
| let id = BoxID::parse(&id_str).ok_or_else(|| { | ||
| BoxliteError::Database(format!("Invalid box ID in database: {id_str}")) | ||
| })?; | ||
| reset_ids.push(id); | ||
| } | ||
|
|
||
| db_err!(tx.commit(), "reset_active_boxes_after_reboot: commit")?; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Good atomic reset; one call site has the same context-interpolation bug.
Wrapping selection, reset, and update of active boxes in one transaction is a solid fix: it removes the previous per-box lock/commit pattern and guarantees all-or-nothing resets, matching the docstring.
At line 313, db_err!(..., "reset_active_boxes_after_reboot: update box={id_str}") uses the 2-argument form, so {id_str} is not substituted with the actual box ID string. See the root-cause comment on src/boxlite/src/db/mod.rs (lines 31-46).
🤖 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 `@src/boxlite/src/db/boxes.rs` around lines 280 - 321, Fix the error-context
formatting in reset_active_boxes_after_reboot by using the db_err! invocation
form that interpolates id_str in the update error message. Preserve the existing
transaction and update behavior while ensuring failures identify the actual box
ID.
| /// Map a database row to a `CachedImage`. | ||
| /// | ||
| /// Uses named-column access so the helper works regardless of column | ||
| /// position in the SELECT list (e.g. `get` omits `reference` but | ||
| /// `list_all` includes it). | ||
| fn row_to_cached_image(row: &rusqlite::Row) -> rusqlite::Result<CachedImage> { | ||
| let manifest_digest: String = row.get("manifest_digest")?; | ||
| let config_digest: String = row.get("config_digest")?; | ||
| let layers_json: String = row.get("layers")?; | ||
| let cached_at: String = row.get("cached_at")?; | ||
| let complete: i32 = row.get("complete")?; | ||
| let layers: Vec<String> = serde_json::from_str(&layers_json).map_err(|e| { | ||
| rusqlite::Error::FromSqlConversionFailure( | ||
| 2, // layers column index | ||
| rusqlite::types::Type::Text, | ||
| Box::new(e), | ||
| ) | ||
| })?; | ||
| Ok(CachedImage { | ||
| manifest_digest, | ||
| config_digest, | ||
| layers, | ||
| cached_at, | ||
| complete: complete != 0, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
rusqlite 0.39 Row column_index method by name
💡 Result:
In rusqlite version 0.39, the Row struct provides a method named column_index that allows you to retrieve the index of a column by its name [1][2][3]. The method is defined as: pub fn column_index(&self, name: &str) -> Result This method returns the index of the column as a Result [3][4][5]. If no column with the specified name is found, it returns an Error::InvalidColumnName [6][7]. Note that column names are compared in a case-insensitive manner [5][7]. If a column does not have an explicit AS alias in the SQL statement, the column name is considered implementation-defined and may change between different releases of SQLite [6][7]. While Row provides this method, it is fundamentally a wrapper around the corresponding functionality in the Statement struct, which performs the actual name-to-index lookup [6][5][7].
Citations:
- 1: https://docs.rs/rusqlite-le/latest/rusqlite/struct.Row.html
- 2: https://docs.rs/rusqlite/latest/rusqlite/struct.Row.html
- 3: https://docs.rs/rusqlite/0.25.2/rusqlite/struct.Row.html
- 4: https://docs.rs/rusqlite/0.24.2/rusqlite/struct.Row.html
- 5: https://docs.rs/crate/rusqlite/0.21.0/target-redirect/x86_64-apple-darwin/src/rusqlite/column.rs.html
- 6: https://docs.rs/rusqlite/latest/rusqlite/struct.Statement.html
- 7: https://docs.rs/rusqlite-ic/latest/src/rusqlite/column.rs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files =="
git ls-files | rg 'src/boxlite/src/db/images\.rs|Cargo\.toml|Cargo\.lock' || true
echo "== Rust manifest rusqlite deps =="
for f in Cargo.toml src/boxlite/Cargo.toml $(find . -name Cargo.toml); do
if rg -q 'rusqlite' "$f"; then
echo "--- $f ---"
rg -n 'rusqlite' "$f"
fi
done
echo "== Relevant sqlite.rs section =="
cat -n src/boxlite/src/db/images.rs | sed -n '1,140p'
echo "== Search for row_to_cached_image usages/selectors =="
rg -n "row_to_cached_image|list_all|fn get|SELECT|reference|layers" src/boxlite/src/db/images.rsRepository: boxlite-ai/boxlite
Length of output: 9053
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
queries = {
"get": "SELECT manifest_digest, config_digest, layers, cached_at, complete",
"list_all": "SELECT reference, manifest_digest, config_digest, layers, cached_at, complete",
}
for name, query in queries.items():
cols = [token.strip() for token in " ".join(query.split()).split(",")]
layers_idx = cols.index("layers")
print(f"{name}: layers index is {layers_idx}, columns = {cols}")
if layers_idx == 2:
print(" -> hardcoded layers index 2 matches this query")
else:
print(" -> hardcoded layers index 2 does NOT match this query")
PYRepository: boxlite-ai/boxlite
Length of output: 505
Keep the layers conversion error index position-independent.
The helper is documented as independent of column order, but the SQLite conversion error hardcodes index 2. In list_all(), layers is at index 3, so invalid layer JSON reports a wrong column index. Use row.as_ref().column_index("layers").unwrap_or(0) instead of the hardcoded value.
🤖 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 `@src/boxlite/src/db/images.rs` around lines 40 - 66, Update
row_to_cached_image so the layers JSON conversion error uses
row.as_ref().column_index("layers").unwrap_or(0) instead of a hardcoded column
index, preserving position-independent error reporting for both get and list_all
callers.
| if image.complete { 1 } else { 0 } | ||
| ], | ||
| ))?; | ||
| ), "image_index upsert(ref={reference})")?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Context string won't interpolate {reference}.
db_err!(..., "image_index upsert(ref={reference})") uses the 2-argument form. {reference} is not substituted with the actual image reference; the literal text {reference} ends up in the error message. See the root-cause comment on src/boxlite/src/db/mod.rs (lines 31-46) for the fix.
🤖 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 `@src/boxlite/src/db/images.rs` at line 116, Update the db_err! invocation in
the image index upsert path to use the formatting form that interpolates the
reference value, following the established pattern in the db module. Ensure the
resulting error context contains the actual image reference rather than the
literal `{reference}` text.
| /// | ||
| /// All migrations and the final `schema_version` update are wrapped in a | ||
| /// single transaction. If any migration fails, the entire batch rolls back | ||
| /// so the database is left at `source_version` — the next startup will retry | ||
| /// from the same point. | ||
| /// | ||
| /// **Note**: migrations that perform filesystem operations (e.g. v6→v7 moves | ||
| /// disk files) cannot be rolled back by the transaction. Those migrations are | ||
| /// still responsible for their own filesystem-level idempotency. | ||
| pub(crate) fn run_migrations( | ||
| conn: &Connection, | ||
| source_version: i32, | ||
| home_dir: Option<&Path>, | ||
| ) -> BoxliteResult<()> { | ||
| let all = all_migrations(); | ||
|
|
||
| let tx = db_err!(conn.unchecked_transaction(), "run_migrations(v{source_version}): begin")?; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Good transactional design; the begin-context string won't interpolate.
Wrapping all migrations and the final schema_version update in a single transaction is the right fix: a failed migration now rolls back cleanly and the next startup retries from source_version, as documented.
At line 59, db_err!(..., "run_migrations(v{source_version}): begin") uses the 2-argument form, so {source_version} is not substituted with the actual version number. See the root-cause comment on src/boxlite/src/db/mod.rs (lines 31-46) for the fix.
🤖 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 `@src/boxlite/src/db/migration/mod.rs` around lines 43 - 60, Update the
begin-context argument in run_migrations to use the db_err! formatting form that
interpolates source_version, matching the established pattern in the database
module; preserve the existing transaction behavior and error context.
| m.run(&tx, home_dir)?; | ||
| current = m.target_version(); | ||
| } | ||
| } | ||
|
|
||
| let now = Utc::now().to_rfc3339(); | ||
| db_err!(conn.execute( | ||
| db_err!(tx.execute( | ||
| "UPDATE schema_version SET version = ?1, updated_at = ?2 WHERE id = 1", | ||
| rusqlite::params![current, now], | ||
| ))?; | ||
| ), "run_migrations: update schema_version to v{current}")?; | ||
|
|
||
| db_err!(tx.commit(), "run_migrations(v{source_version} -> v{current}): commit")?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Same context-interpolation bug on the schema-version update and commit.
Lines 76-79 ("run_migrations: update schema_version to v{current}") and line 81 ("run_migrations(v{source_version} -> v{current}): commit") use the 2-argument form with embedded {current}/{source_version}. Neither substitutes the actual values. See the root-cause comment on src/boxlite/src/db/mod.rs (lines 31-46) for the fix.
🤖 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 `@src/boxlite/src/db/migration/mod.rs` around lines 70 - 81, Update the db_err!
calls for the schema-version UPDATE and transaction commit in run_migrations to
use the correct interpolation form, passing current and source_version as
formatting arguments so the actual version values appear in both context
messages.
| /// | ||
| /// Usage: | ||
| /// - `db_err!(result)` — basic, no context | ||
| /// - `db_err!(result, "operation name")` — includes operation context | ||
| /// - `db_err!(result, "op(box={})", id)` — with format arguments | ||
| macro_rules! db_err { | ||
| ($result:expr) => { | ||
| $result.map_err(|e| BoxliteError::Database(e.to_string())) | ||
| }; | ||
| ($result:expr, $ctx:expr) => { | ||
| $result.map_err(|e| BoxliteError::Database(format!("{}: {}", $ctx, e))) | ||
| }; | ||
| ($result:expr, $fmt:expr, $($args:tt)*) => { | ||
| $result.map_err(|e| BoxliteError::Database(format!("{}: {}", format!($fmt, $($args)*), e))) | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Rust format! support implicit named argument capture when the literal is passed through a macro_rules expr fragment to a nested format! call?
💡 Result:
No, Rust's formatting macros (such as format!, println!, etc.) do not support implicit named argument capture when the format string literal is passed through a macro_rules! expr fragment [1][2][3][4]. The Rust compiler explicitly disallows implicit argument capturing (e.g., "{variable}") if the first argument to the formatting macro is not a direct, non-macro-generated string literal [2][3][4]. When a format string is passed through a macro_rules! expansion, it is considered a macro-generated expression rather than a direct literal, which triggers this restriction to prevent issues related to macro hygiene and unpredictable scope resolution [1][2][4]. The underlying implementation in the compiler checks a flag (often called is_direct_literal) to determine if implicit captures are permitted [2][3][4]. If this flag is false—which occurs when the format string originates from a macro expansion—the compiler will emit a diagnostic error if it attempts to resolve an implicit named argument [3][4]. This design was established in RFC 2795 to ensure that implicit captures remain predictable and do not accidentally capture variables from scopes that are not visually apparent at the point of the macro invocation [1][2].
Citations:
- 1: https://github.com/rust-lang/rfcs/blob/master/text/2795-format-args-implicit-identifiers.md
- 2: https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_builtin_macros/format.rs.html
- 3: https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_builtin_macros/format.rs.html
- 4: https://github.com/rust-lang/rust/blob/2972b5e5/compiler/rustc_builtin_macros/src/format.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'mod\.rs|boxes\.rs|base_disk\.rs|images\.rs|migration' . | sed 's#^\./##' | head -200
echo "== db_err macro occurrences =="
rg -n "macro_rules! db_err|db_err!\(|macro_export" -S .
echo "== relevant sections =="
for f in src/boxlite/src/db/mod.rs src/boxlite/src/boxes.rs src/boxlite/src/base_disk.rs src/boxlite/src/images.rs src/boxlite/src/migration/mod.rs; do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
rg -n "db_err!\(|// Usage|macro_rules! db_err" "$f" -C 4 || true
fi
done
echo "== standalone rust macro behavior probe =="
if command -v rustc >/dev/null 2>&1; then
tmp="$(mktemp -d)"
cat > "$tmp/main.rs" <<'RS'
#[macro_export]
macro_rules! db_err_direct {
($result:expr, $ctx:expr) => {
$result.map_err(|e| format!("{}: {}", $ctx, e))
};
}
#[macro_export]
macro_rules! db_err_wrapped {
($result:expr, $ctx:expr) => {
$result.map_err(|e| format!("{}: {}", format!($ctx), e))
};
}
fn main() {
{
let id = "box-xyz";
let result: Result<(), std::io::Error> = Err(std::io::Error::new(std::io::ErrorKind::Other, "locked"));
let err = db_err_direct!(result, "op(box={id})").unwrap_err();
println!("direct={}:contains(box-xyz)={}", err, err.to_string().contains("box-xyz"));
}
{
let id = "box-xyz";
let result: Result<(), std::io::Error> = Err(std::io::Error::new(std::io::ErrorKind::Other, "locked"));
let err = db_err_wrapped!(result, "op(box={id})").unwrap_err();
println!("wrapped={}:contains(box-xyz)={}", err, err.to_string().contains("box-xyz"));
}
}
RS
echo "--- direct should fail at compile time ---"
rustc "$tmp/main.rs" --crate-type bin 2>&1 | sed -n '1,120p' || true
else
echo "rustc not available"
fiRepository: boxlite-ai/boxlite
Length of output: 22348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== focused db_err 2-arg call sites =="
rg -n 'db_err!\([^,]+,\s*"[^"]*\{[[:alnum:]_]+}\.?"' src/boxlite/src/db -S
echo "== focused sections around named captures =="
for f in src/boxlite/src/db/base_disk.rs src/boxlite/src/db/migration/mod.rs; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '220,275p' "$f" | cat -n
sed -n '45,85p' "$f" | cat -n
fi
done
echo "== macro expansion via rustc unavailable fallback: parse placeholder count =="
python3 - <<'PY'
from pathlib import Path
examples = [
'db_err!(tx.execute("..."), "remove_all_refs_for_box(box={box_id}): begin")',
'db_err!(tx.execute("..."), "run_migrations(v{source_version}): begin")',
'db_err!(result, "insert box={}", "box-123")',
]
for e in examples:
s = e.split(', ', 1)[1] if ', ' in e else e
print(f"{e} -> context literal has bracket-style placeholders: {'{' in s and '}' in s}")
PYRepository: boxlite-ai/boxlite
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -u
echo "== exact db_err calls containing {identifier} =="
rg -n 'db_err!\([^,]+,\s*"[^"]*\{[[:alnum:]_]+[^"]*"' src/boxlite/src/db -S || true
echo "== exact db_err calls containing {identifier followed by : in context text =="
rg -n 'db_err!\([^,]+,\s*"[^"]*\{[[:alnum:]_]+[^"]*"' src/boxlite/src/db -S || true
echo "== exact db_err calls where literal contains { and } and maybe identifier =="
python3 - <<'PY'
import re
from pathlib import Path
pat = re.compile(r"""db_err!\(([^;,]+),\s*"(.*?)"(,\s*[^)]+)?\)\?(\?)?\)""")
found = []
for p in Path("src/boxlite/src/db").rglob("*.rs"):
text = p.read_text()
for line_no, line in enumerate(text.splitlines(), 1):
for m in pat.finditer(line):
frag = line[m.start():m.end()]
print(frag)
if "{" in m.group(2):
found.append((str(p), line_no, frag, m.group(2)))
print("TOTAL FOUND", len(found))
for item in found:
print("CAPTURED", item)
PYRepository: boxlite-ai/boxlite
Length of output: 1390
🏁 Script executed:
#!/bin/bash
set -u
echo "== 3-arg db_err calls in db modules =="
rg -n 'db_err!\([^,]+,\s*"[^"]+"[^?]*\?\)?' src/boxlite/src/db -S || true
echo "== macro 2-arg expansion textual shape =="
sed -n '36,52p' src/boxlite/src/db/mod.rs | cat -n
echo "== current tests around named captures =="
sed -n '1,20p' src/boxlite/src/db/mod.rs | cat -n
sed -n '410,452p' src/boxlite/src/db/mod.rs | cat -nRepository: boxlite-ai/boxlite
Length of output: 4221
Fix: db_err!'s 2-argument form needs explicit argument capture semantics
format!("{}: {}", $ctx, e) accepts a plain {:} placeholder string for $ctx; format!($ctx, e) would compile and parse named captures, but it still requires explicit arguments because the inner format! can only infer identifiers from a direct string literal argument, not from an expression captured through the outer macro. Convert the 2-argument form to a 3-argument call only if the context literal is a direct string literal, or rewrite those call sites to pass "context {}", value explicitly.
🤖 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 `@src/boxlite/src/db/mod.rs` around lines 31 - 46, Update the `db_err!` macro
and its call sites so context formatting uses explicit arguments: keep the
two-argument form only for direct literal context strings, and rewrite dynamic
or interpolated contexts to use the three-argument form with the value passed
separately. Preserve the existing database error wrapping and context prefix
behavior.
Wrap multi-step DB operations in transactions, consolidate duplicated queries, and add operation context to error messages for easier debugging.
Transaction safety:
Query consolidation:
Error diagnostics:
SQLite tuning:
Summary
Changes
How to verify
Risks / rollout
Summary by CodeRabbit
Bug Fixes
Improvements