fix(adapter-better-sqlite3): recover from failed transactions - #29955
fix(adapter-better-sqlite3): recover from failed transactions#29955wehamed wants to merge 1 commit into
Conversation
Signed-off-by: Hamed Elaraby <qef0@hotmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe better-sqlite3 adapter now performs SQLite rollback cleanup after failed commits and releases its mutex when transaction startup fails. Tests cover commit, rollback, ChangesSQLite transaction recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 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 |
SevInf
left a comment
There was a problem hiding this comment.
Thank you for submitting PR, we appreciate it.
I don't think it is a reliable fix for the issue though. If either COMMIT or ROLLBACK failed we are not sure if connection is stuck in transactional state or not and best effort attempts to repair it might not be enough. I feel like in those 2 cases our only option to ensure next query succeeds is killing and re-creating the connection. Could you update the PR to do that instead?
SevInf
left a comment
There was a problem hiding this comment.
(See above for requested changes)
Linked issue
Fixes #29933
Summary
A failed SQLite
COMMITcould leave the shared better-sqlite3 connectioninside an open transaction, while a later
BEGINfailure could leak theadapter mutex permanently.
This makes
rollback()perform best-effort connection cleanup when SQLitestill reports an active transaction, and releases the mutex when
BEGINfails. Successful transaction behavior and the existing
usePhantomQuery: falsecontract are unchanged.Testing performed
rollback, failed-COMMIT recovery, failed-BEGIN mutex release, and cleanup
failure behavior.
SQLITE_BUSY-mapped errorrelease or failed-COMMIT cleanup is removed.
git diff --checkpassed.The full adapter package suite has one pre-existing
errors.test.ts > UniqueConstraintViolationfailure caused by the localbetter-sqlite3 12.6.0 error-message shape. The same failure was reproduced on
a pristine v7 checkout; the new transaction tests remain green.
Skill update
n/a — bug fix for existing adapter transaction behavior; no skill changes required.
Checklist
the conventional
fix(scope):form.Notes for the reviewer
With
usePhantomQuery: false, the transaction manager owns normalCOMMIT/ROLLBACK SQL and the adapter's
commit()/rollback()methods arelifecycle callbacks.
The cleanup added here therefore runs only when
rollback()observes thatbetter-sqlite3 is still inside a transaction, which is the failed-COMMIT case.
Cleanup is best-effort so a secondary ROLLBACK failure cannot replace the
original transaction error.
The misleading
SQLITE_BUSY→SocketTimeoutmapping is intentionally outof scope.
Summary by CodeRabbit
Bug Fixes
Tests