Skip to content

fix(sqlite): respect transaction context during commit - #106

Closed
Erfouni wants to merge 2 commits into
gopherium:mainfrom
Erfouni:fix/sqlite-commit-context
Closed

Erfouni wants to merge 2 commits into
gopherium:mainfrom
Erfouni:fix/sqlite-commit-context

Conversation

@Erfouni

@Erfouni Erfouni commented Oct 21, 2026 •

Copy link
Copy Markdown

Commit now stops waiting for SQLite locks when the transaction context ends, while rollback still completes safely.

Added regression tests for cancellation, deadlines, and connection reuse. The full SQLite suite passes with the race detector and 100% statement coverage. Lint and vet pass too.

Fixes #104

Summary by CodeRabbit

  • Bug Fixes
    • Transaction commits now stop waiting when the transaction context is canceled or reaches its deadline.
    • Commit errors now clearly reflect transaction termination caused by context cancellation.
    • Rollbacks continue completing after context cancellation, helping prevent partial writes.
    • Improved handling of busy SQLite locks, including bounded wait behavior and connection reuse.

@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 35 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 52b170c2-1b85-4ecb-aa06-e64648b43f24
📥 Commits

Reviewing files that changed from the base of the PR and between c373a0b and a25c5fe.

📒 Files selected for processing: 1
  • dbkit/sqlite/busy_tx_test.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: bd0b70f2-852f-4fbd-b11d-0827cf6d5394
📥 Commits

Reviewing files that changed from the base of the PR and between 2f0e7db and c373a0b.

📒 Files selected for processing: 3
  • dbkit/sqlite/CHANGELOG.md
  • dbkit/sqlite/busy.go
  • dbkit/sqlite/busy_tx_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

BeginTx now wraps successful SQLite transactions so Commit uses the transaction’s original context. Rollback remains provided by the embedded driver transaction. New tests cover busy locks, cancellation, rollback, and connection reuse.

Changes: SQLite transaction context handling

Layer / File(s) Summary
Context-bound commit behavior
dbkit/sqlite/busy.go, dbkit/sqlite/busy_tx_test.go, dbkit/sqlite/CHANGELOG.md
BeginTx wraps successful transactions in busyTx, which retains the original context and connection wait state. Commit runs under that context and handles context termination on commit errors. Rollback remains provided by the embedded driver transaction. Tests cover busy locks, cancellation, rollback, bounded waits, and connection reuse. The changelog records the commit and rollback behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: sirlouen

Merge Risk: ⚪ Minimal · up to c373a

Commit lock waits now stop when the transaction context ends, and the investigated failure path does not leave the connection in an active transaction. No merge-blocking issue remains from this review.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: SQLite transaction commits now respect the transaction context.
Linked Issues check Passed Issue #104 requires context-aware commit waiting, unwrapped rollback, and regression coverage for rollback safety. busyConn.BeginTx returns busyTx; busyTx.Commit runs the driver commit under the…
Out of Scope Changes check Passed The changes stay within issue #104. The implementation changes SQLite transaction commit handling. The tests cover the required lock, cancellation, deadline, rollback, and connection-reuse behavior. T…
Full details: Linked Issues check

Explanation

Issue #104 requires context-aware commit waiting, unwrapped rollback, and regression coverage for rollback safety. busyConn.BeginTx returns busyTx; busyTx.Commit runs the driver commit under the original BeginTx context. The embedded driver.Tx.Rollback remains unwrapped. Tests cover busy commit rollback, rollback after cancellation, deadline termination, cancellation termination, bounded duration, and connection reuse.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #104. The implementation changes SQLite transaction commit handling. The tests cover the required lock, cancellation, deadline, rollback, and connection-reuse behavior. The changelog documents the same fix. No unrelated change is shown in the supplied scope.

Full details: Docstring Coverage

Explanation

Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

codescene-access[bot]

This comment was marked as outdated.

@greptile-apps

greptile-apps Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; this update only adds test descriptions.

Summary

Commit lock waits now use the transaction's context. Rollback remains independent of cancellation.

  • SQLite commits stop waiting for locks when their transaction context ends.

Reviews (2) · Last reviewed commit: "docs(sqlite): explain transaction regres..." · Reviewed by Greptile

@Erfouni

Erfouni commented Oct 21, 2026

Copy link
Copy Markdown
Author

I've added short doc comments to the three regression tests, clarifying the cancellation and rollback behavior they cover. The full SQLite suite passes locally with the race detector, and lint and vet are clean.

@codescene-access codescene-access Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

@Erfouni

Erfouni commented Oct 21, 2026

Copy link
Copy Markdown
Author

Hi, the three failing security checks are flagging a Go 1.27.2 standard library vulnerability in unchanged modules. The scan lists Go 1.27.3 as the fix. All tests and lint checks pass. Could you take a look at the project’s Go version when you have a moment?

@SirLouen

Copy link
Copy Markdown
Member

Thanks for working on this, and sorry to close it.

modernc.org/sqlite v1.61.0 now ends a lock wait with the call's context on its own, so #108 removed dbkit's busy handler, and busy.go with it.

dbkit no longer wraps the driver's connection either, so the fix for Commit now belongs in the driver.

I asked for it upstream

#104 stays open until dbkit moves to a driver release that has it.

@SirLouen SirLouen closed this Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop a SQLite commit that waits for a lock once its transaction's context ends

2 participants