(MOT-4216) fix(database): resolve omitted db to the sole/primary pool - #585
Conversation
Every sub-agent in the rctest orchestration runs burned its first database call on `serialization error: missing field `db`` — a wasted error round-trip per session, four per run. `db` is now optional on query/execute/executeBatch/transaction/ prepareStatement/beginTransaction. Explicit names behave exactly as before (unknown ones still get UNKNOWN_DB); an omitted `db` resolves to the sole configured pool, then to `primary` when several exist, and only errors — the new MISSING_DB, which enumerates the available names so a caller can self-correct from one failure — when neither rule disambiguates.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHandlers now accept omitted database names, resolve configured defaults, reject transaction-control SQL on pooled operations, and roll back leaked SQLite transactions before connection reuse. Error serialization and tests cover missing-database reporting and the updated behavior. ChangesDatabase routing and transaction safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant AppState
participant SQLitePool
Client->>Handler: Submit request with optional db and SQL
Handler->>AppState: resolve_db(optional db)
AppState-->>Handler: Resolved database or MISSING_DB
Handler->>Handler: reject_tx_control_sql(SQL)
Handler->>SQLitePool: Acquire resolved database connection
SQLitePool-->>Handler: Clean connection
Handler-->>Client: Execute result or validation error
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
skill-check — worker0 verified, 48 skipped (no docs/).
Four for four. Nicely done. |
…elf-heal leaked transactions
rctest5 postmortem, attempt 3: an agent ran execute("BEGIN") expecting a
session. Each call draws a fresh pooled connection, so the BEGIN/COMMIT
pair never met — worse, the BEGIN's connection went back to the pool
still inside an open transaction, and every later caller unlucky enough
to draw it failed `cannot start a transaction within a transaction`.
Three writer agents starved on one poisoned connection; 7 of 15 rows
landed.
Two layers:
* execute/query/prepareStatement now reject transaction-control SQL
(BEGIN/COMMIT/ROLLBACK/SAVEPOINT, comment/whitespace-prefixed forms
included, via the existing tx_sql_guard) with INVALID_PARAM naming the
real transactional surfaces (beginTransaction/transactionExecute/
commitTransaction, executeBatch). query is included because sqlite
happily starts a transaction from query("BEGIN") and returns no rows.
* SqlitePool::acquire rolls back any leaked open transaction before
handing the connection out — a regression anywhere upstream degrades
to one warning line instead of a permanently poisoned pool.
|
Second commit ( Live-run postmortem (rctest5, attempt 3): an agent ran
225 tests pass; clippy clean. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
database/src/handlers/query.rs (1)
225-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplicate
db-not-required schema assertion across sibling request types.Only
QueryReqgets a schema regression test confirmingdbisn't required.ExecuteReq,PrepareReq,TxReq,BeginTxReq, andExecuteBatchReqall made the same field change but have no equivalent test, so a future accidental regression (e.g., dropping#[serde(default)]) on any of them wouldn't be caught.🤖 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 `@database/src/handlers/query.rs` around lines 225 - 234, Add equivalent schema regression tests for ExecuteReq, PrepareReq, TxReq, BeginTxReq, and ExecuteBatchReq, alongside request_schema_marks_db_optional. Each test should serialize its request schema, assert db is absent from required, and retain the existing expectation that sql is required where applicable, matching the QueryReq test pattern.
🤖 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 `@database/src/pool/sqlite.rs`:
- Around line 91-96: The connection-acquisition path must not return a
connection when the ROLLBACK issued after detecting an open transaction fails.
Update the handling around conn.execute_batch("ROLLBACK") to propagate the
rollback error and discard or invalidate the affected pooled connection before
returning, while preserving normal reuse when rollback succeeds.
---
Nitpick comments:
In `@database/src/handlers/query.rs`:
- Around line 225-234: Add equivalent schema regression tests for ExecuteReq,
PrepareReq, TxReq, BeginTxReq, and ExecuteBatchReq, alongside
request_schema_marks_db_optional. Each test should serialize its request schema,
assert db is absent from required, and retain the existing expectation that sql
is required where applicable, matching the QueryReq test pattern.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1257c698-1524-46c6-9dcb-cb1598c83976
📒 Files selected for processing (9)
database/src/error.rsdatabase/src/handlers/begin_transaction.rsdatabase/src/handlers/execute.rsdatabase/src/handlers/execute_batch.rsdatabase/src/handlers/mod.rsdatabase/src/handlers/prepare.rsdatabase/src/handlers/query.rsdatabase/src/handlers/transaction.rsdatabase/src/pool/sqlite.rs
…aked transaction won't roll back CodeRabbit: discarding the ROLLBACK error still handed out a connection possibly inside a transaction — recreating the pool poisoning the check exists to prevent. A failed or ineffective rollback now fails the acquire with a distinct stuck-transaction DriverError instead; the connection returns to the pool and the rollback is retried on its next checkout, so a transient failure self-heals rather than poisoning forever, and callers see the real cause instead of `cannot start a transaction within a transaction`.
1 participant
Why
Every sub-agent session in the rctest orchestration runs (reactor, finalizer, repair, orchestrator — rctest5-K7mQ most recently) burned its first
database::*call onserialization error: missing field 'db'. LLM callers routinely omitdb; a hard serde failure there is a wasted round-trip in every live session.What
dbis now optional onquery,execute,executeBatch,transaction,prepareStatement,beginTransaction(documented in the function schema, so callers see it's optional).AppState::resolve_db): explicit name → unchanged (unknown names still getUNKNOWN_DB); omitted → the sole configured pool, elseprimarywhen several exist, else the newMISSING_DBerror which enumerates the available names — same self-correction pattern as MOT-4208'sUNKNOWN_DB.Tests
dbon a sole pool executes against itdbamong many pools prefersprimarydbwith no unambiguous default →MISSING_DBwith sortedavailablelistdbrequiredMISSING_DBwire-envelope serialization222 database tests pass; clippy clean.
Closes MOT-4216.
Summary by CodeRabbit
New Features
primarydatabase when available.Bug Fixes