fix(adapter-libsql): clean up failed commits - #30071
Conversation
|
|
|
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)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesLibSQL commit failure recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change cleans up libsql transactions after failed commits while preserving the original error, with focused tests and build coverage; no actionable merge-blocking risk remains. Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Nice find — this is a real leak and the fix is correct. I reproduced it end to end before writing this, so here is the evidence in case it is useful for the maintainer review. The premise checks out, and there is a strong argument for it in-treeThe comment in the diff says the transaction manager never sends a real COMMIT/ROLLBACK for phantom-query adapters. That is exactly what // packages/client-engine-runtime/src/transaction-manager/transaction-manager.ts:534
if (tx.transaction.options.usePhantomQuery) {
await this.#withQuerySpanAndEvent(PHANTOM_COMMIT_QUERY(), tx.transaction, () => tx.transaction!.commit())
} else {
const query = COMMIT_QUERY()
await this.#withQuerySpanAndEvent(query, tx.transaction, () => tx.transaction!.executeRaw(query)).then(
() => tx.transaction!.commit(),
(err) => {
const fail = () => Promise.reject(err)
return tx.transaction!.rollback().then(fail, fail) // <-- exactly this PR's cleanup
},
)
}The non-phantom branch already does rollback-on-failed-commit-and-rethrow-the-original-error. The phantom branch does not. Reproduced with a real commit failure on a real libsql databaseI forced a genuine
Without the patch the second connection is locked out permanently, not transiently. The commit error surfaces as Confirming the mechanism at the driver level, Failure-mode sweepI walked every way the commit path can fail rather than only the one the PR names:
The On the remote transports this is a safe no-op rather than a behaviour change, which is worth knowing: Tests: the two new cases fail on the base commit (4 failures, once per Two optional notes, neither blocking1. 2. Where should this live? // packages/adapter-mssql/src/mssql.ts:101
async commit(): Promise<void> {
const release = await this.#mutex.acquire()
try {
await this.transaction.commit()
} finally {
release() // no rollback cleanup if commit() threw
}
}For completeness on the other phantom-query adapters: Also worth noting for whoever merges: the diff's inline comment is unusually good at explaining why this is the only place that can clean up. Please keep it. Nothing here needs to change for this to ship, in my read. Not a maintainer — just wanted to hand over the reproduction so the review is cheaper. |
Related to #30028
When the libsql adapter uses phantom transactions, the transaction manager delegates the actual COMMIT/ROLLBACK calls to the adapter. If COMMIT fails, the underlying libsql transaction can remain open unless the adapter explicitly cleans it up.
This updates
LibSqlTransaction.commit()to attempt a rollback after a failed commit. If that cleanup rollback also fails, it closes the transaction handle as a fallback while preserving the original commit error.This PR is limited to cleaning up the transaction after a failed COMMIT. SQLITE_BUSY error classification is outside the scope of this change.
Tests:
pnpm --filter @prisma/adapter-libsql testpnpm --filter @prisma/adapter-libsql build