fix(libsql): recover cancelled transactions and history migration - #6935
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🚅 Deployed to the ironclaw-pr-6935 environment in ironclaw-ci-preview
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change synchronously discards connections when transactions drop, separates writer-holder cleanup from connection disposal, and re-reads transcript rows inside migration transactions to avoid stale CAS updates. Regression tests cover rollback, writer reuse, and concurrent message updates. ChangesTransaction and migration correctness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MigrationRaceBackend
participant ThreadIndexMigration
participant LibSqlStorageTxn
MigrationRaceBackend->>ThreadIndexMigration: return listed transcript row
ThreadIndexMigration->>LibSqlStorageTxn: begin transaction
MigrationRaceBackend->>MigrationRaceBackend: apply concurrent message update
ThreadIndexMigration->>MigrationRaceBackend: re-fetch transcript row
MigrationRaceBackend-->>ThreadIndexMigration: return current row version
ThreadIndexMigration->>LibSqlStorageTxn: apply CAS migration update
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
🔎 Review · PR #6935
Submitted review →Reviewed the complete trusted base-to-head comparison. The transaction discard, writer-holder lifecycle, checkout telemetry, migration re-read, and regression tests are coherent; no concrete actionable defects were found. Automatic · PR opened · attempt 1 of 3 · completed in 1m 52s Run details
|
There was a problem hiding this comment.
🔍 Review complete · PR #6935
✅ No actionable findings
Reviewed the complete trusted base-to-head comparison. The transaction discard, writer-holder lifecycle, checkout telemetry, migration re-read, and regression tests are coherent; no concrete actionable defects were found.
Validation and technical details
- Verified trusted refs resolve to base ae0989c and head 55033b0.
- Inspected all four changed files and surrounding transaction, pool recycling, pagination, migration, and test-double code.
- Confirmed the existing pool recycle hook rejects connections returned while a transaction is active, complementing the new explicit discard path.
- Ran git diff --check successfully.
- Focused cargo tests could not be rerun because cargo is unavailable in the review environment.
- Base:
main - Head:
codex/fix-libsql-qa-503at55033b0 - Run:
1a7d586a-3245-4ec9-adaa-ca1020b05445
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.45% — 317678 / 367485 lines Per-crate breakdown (60 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Resolves conflicts from main's attachments feature (#6364), libsql transaction/history-migration fix (#6935), and the architecture baselines/ratchet commit (#6936) landing alongside this branch's hosted MCP registration work. Conflicts: - crates/ironclaw_extension_host/src/lib.rs: union of module declarations and re-exports (hosted_mcp_manifest/hosted_mcp_preparation from this branch, inbound_batches from main). - crates/ironclaw_product/src/lib.rs: union of the reborn_services re-export list (EXTENSION_REGISTER_HOSTED_MCP_CAPABILITY[_ID] from this branch, AttachmentCleanupReport from main). All other conflicts (.github/workflows/reborn-e2e.yml, Cargo.toml, Cargo.lock, ironclaw_host_api/src/lib.rs) auto-merged cleanly via git's default resolution and were verified afterward.
Summary
DEBUGtoTRACE, and emit accurateDEBUGdiagnostics only for failed checkouts.047e9307-25b8-41fb-b943-795bfce08d2bfirst showedTimelineUnavailablealongsideexpected version 1, found version 2, then entered repeated 10-second writer checkout timeouts withqueued=2. PR Collapse lifecycle state into the row-native process journal #6696 introduced both failing paths after PR fix(libsql): serialize writers and recover transient contention #6863 established the shared one-writer runtime.Change Type
Linked Issue
Related #6871. Regression follow-up to #6696 and #6863.
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings— Not run workspace-wide; affected crates passedcargo clippy -p ironclaw_libsql_runtime -p ironclaw_filesystem -p ironclaw_threads --all-targets --all-features -- -D warnings.cargo build— Not run separately; affected crates were compiled by tests and clippy.ironclaw_libsql_runtime,ironclaw_filesystem, andironclaw_threadssuites; focused storage architecture gate.cargo test --features integrationif database-backed or integration behavior changed — Not applicable: the changed libSQL contract is covered against a real temporary libSQL database in the owning crate; no PostgreSQL or composition behavior changed.review-prorpr-shepherd --fixwas run before requesting reviewTest Strategy
User behavior: Loading conversation history and timelines remains available while the one-time transcript migration overlaps an active message update; a cancelled storage transaction no longer leaves QA's writer lane unavailable until restart.
Risk areas:
Tests added or updated:
filesystem_history_survives_transcript_migration_racing_a_message_updatedeterministically injects the observed CAS race through the real filesystem thread service.dropping_storage_transaction_releases_writer_lane_synchronouslyuses a real local libSQL database and the production size-one writer runtime.What the tests prove:
BEGIN IMMEDIATEno longer escapes as a history/timeline backend error.Commands run:
cargo test -p ironclaw_libsql_runtimecargo test -p ironclaw_filesystemcargo test -p ironclaw_threadscargo fmt --all -- --checkcargo clippy -p ironclaw_libsql_runtime -p ironclaw_filesystem -p ironclaw_threads --all-targets --all-features -- -D warningscargo test -p ironclaw_architecture --test reborn_process_storage_scan_gatecargo test -p ironclaw_architecture— partially passed, then hit the pre-existing generated-WebUI ratchet failure forbuiltin.profile_setin committedcrates/ironclaw_webui/frontend/distassets; none of those files are changed here.Security Impact
None. No permissions, network calls, secrets, file-access authority, tool execution, or sandbox policy changed. Runtime error displays remain redacted.
Reborn Trust-Boundary Checklist
serde(default)fields fail closed or have migration tests. Not applicable; no serialized fields changed.Transient,Permanent,Misconfigured,PolicyDeniedor equivalent). Existing checkout and filesystem error classification is unchanged.Database Impact
No schema or data migration. libSQL cancellation cleanup now discards an open transaction's connection so SQLite rolls it back on close and the pool creates a clean replacement. Transcript index migration re-reads each listed row inside its existing transaction; PostgreSQL behavior is otherwise unchanged.
Blast Radius
The changes touch the shared libSQL writer lease, libSQL filesystem transaction cancellation, and one-time transcript index migration. Main risks are connection-pool capacity recovery after discard and migration atomicity under concurrent writes; both have regression coverage plus the full owning-crate suites.
Rollback Plan
Revert commit
55033b0deand redeploy. There is no schema or persisted-format rollback. Reverting restores the known risk that a cancelled transaction can retain the sole writer lease and that a first-read migration race can surface a 503.Review Follow-Through
Please focus review on deadpool's permanent object removal during cancellation and the transaction-local re-read in transcript migration. The broad architecture suite's unrelated generated-asset failure is documented above; the storage-specific architecture gate passes.
Review track: C