Skip to content

Make savepoint destruction noexcept - #76

Merged
vinzenz merged 1 commit into
masterfrom
t3code/resolve-issue-55
Sep 13, 2026
Merged

vinzenz merged 1 commit into
masterfrom
t3code/resolve-issue-55

Conversation

@vinzenz

@vinzenz vinzenz commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Fixes #55

Summary

  • Make savepoint destruction non-throwing.
  • Ignore cleanup failures when a savepoint was invalidated externally.
  • Preserve error reporting for explicit release() calls.
  • Add transaction and savepoint lifecycle regression tests.

Testing

  • 4 new regression tests covering the issue's checklist: savepoint destruction after outer rollback and after outer commit, nested savepoint unwinding via a user exception, and explicit release() failure reporting. Full suite: 45/45 passing (Linux, GCC 15.2.1, C++20, Debug).
  • Confirmed the new tests terminate with the issue's exact error (no such savepoint: s [SQL: RELEASE SAVEPOINT s]) when the fix is reverted.

Summary by CodeRabbit

  • Bug Fixes
    • Improved savepoint cleanup reliability during transaction commits, rollbacks, and exception handling.
    • Savepoint destruction no longer propagates cleanup errors or unexpectedly terminates the application.
    • Explicit savepoint release continues to report errors when needed.

- Ignore release failures during destructor cleanup
- Preserve explicit release() error reporting
- Add transaction and nested savepoint regression tests
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 9f920d6d-c9ac-46ef-a8b2-cf7d7059bf56

📥 Commits

Reviewing files that changed from the base of the PR and between d83b6a5 and cbb0fab.

📒 Files selected for processing (3)
  • include/sqlite/savepoint.hpp
  • src/sqlite/savepoint.cpp
  • tests/test_transaction.cpp

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


📝 Walkthrough

Walkthrough

The savepoint destructor is now explicitly non-throwing. It suppresses cleanup failures, while explicit release() calls still report errors. Transaction tests cover rollback, commit, exception unwinding, and explicit release behavior.

Changes

Savepoint cleanup behavior

Layer / File(s) Summary
Non-throwing destructor contract
include/sqlite/savepoint.hpp, src/sqlite/savepoint.cpp
The destructor is declared noexcept and catches exceptions from release(). Explicit release() remains error-reporting.
Savepoint lifecycle tests
tests/test_transaction.cpp
Tests verify safe destruction after rollback or commit, safe destruction during exception unwinding, and failure reporting from explicit release().

Priority: ⬇️ Low

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

Merge Risk: ⚪ Minimal · up to cbb0f

Savepoint cleanup no longer throws when an outer transaction has invalidated it, while callers can still receive release errors through explicit release(). No concrete merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: savepoint destruction is now non-throwing.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/resolve-issue-55

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.

@vinzenz

vinzenz commented Sep 12, 2026

Copy link
Copy Markdown
Owner Author

@deslopify review this

@vinzenz

vinzenz commented Sep 13, 2026

Copy link
Copy Markdown
Owner Author

@deslopify rerun

1 similar comment
@vinzenz

vinzenz commented Sep 13, 2026

Copy link
Copy Markdown
Owner Author

@deslopify rerun

@vinzenz
vinzenz merged commit 2f833c6 into master Sep 13, 2026
20 checks passed
@deslopify

deslopify Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review request received. The standard review is completed. Use @deslopify rerun to request a new attempt once this one finishes.

@vinzenz
vinzenz deleted the t3code/resolve-issue-55 branch September 19, 2026 22:04
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.

[P1] Savepoint destruction terminates after an enclosing transaction ends

2 participants