fix(db): report rollback outcome correctly in WithTxn

[AmanGIT07]

Summary

WithTxn in pkg/db reported every failed transaction as a rollback failure: the message embedded a nil rollback error (%!s(<nil>)) and carried a duplicate rollback: prefix.

Fixes #1765

Changes

• Check the rollback's own error (rlbErr) instead of the already non-nil callback error when building the returned message.
• Wrap the returned error once instead of twice.
• Remove unused error assignments in the panic path; it rolls back and re-panics as before.
• Add pkg/db/db_test.go covering commit, begin failure, rollback success and failure messages, commit failure, and panic handling, using a fake database/sql/driver (no new dependencies).
• Fix four inverted error assertions in internal/store/postgres/user_repository_test.go: errors.Unwrap(err) == tc.Err failed the test when the error matched, and the old double wrap kept it from firing. Now !errors.Is(err, tc.Err).

Technical Details

The callback error stays wrapped with %w on all paths, so errors.Is/errors.As matching is unchanged. Messages now are:

• rollback succeeds: rollback: <callback error>
• rollback fails: rollback error: <rollback error> while executing: <callback error>

Test Plan

• go test -race -count=2 ./pkg/db/ passes
• go test ./internal/store/postgres/ passes
• golangci-lint run ./pkg/db/... ./internal/store/postgres/... reports no issues
• Build and type checking passes

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
frontier	Ready	Preview	Aug 5, 2026 9:13am

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 366b01f3-da0a-4697-9226-40bc9fdf59e5

📥 Commits

Reviewing files that changed from the base of the PR and between 6251d5c and 7cdc215.

📒 Files selected for processing (2)

• pkg/db/db.go
• pkg/db/db_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• pkg/db/db.go
• pkg/db/db_test.go

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • Improved transaction error handling, including rollback failures and commit errors.
  • Transactions now roll back correctly when unexpected failures occur while preserving the original error or panic.
  • Improved consistency when identifying wrapped database errors.
  • Reduced duplicate error reporting for clearer failure messages.

• Tests

  • Added comprehensive coverage for successful transactions, failures, rollbacks, commits, and panic handling.
  • Expanded validation of database error comparisons and transaction recovery behavior.

Walkthrough

This change fixes WithTxn rollback and panic handling, adds fake-driver coverage for transaction outcomes, and updates Postgres repository tests to use errors.Is and %v error formatting.

Changes

Transaction error handling

Layer / File(s)	Summary
WithTxn rollback and panic handling pkg/db/db.go, pkg/db/db_test.go	WithTxn preserves callback errors, reports rollback failures correctly, rolls back before re-panicking, and avoids duplicate wrapping. Tests cover transaction success, failures, rollback, commit, and panic paths.
Repository wrapped-error assertions internal/store/postgres/user_repository_test.go	Repository tests use errors.Is for expected errors and %v for error output.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• raystack/frontier#1768: Updates transaction error propagation that depends on the WithTxn behavior changed here.

Suggested reviewers: rohilsurana

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PostgreSQL user repository test assertion changes are unrelated to the linked WithTxn issue #1765.	Move the PostgreSQL user repository test corrections to a separate pull request or link an issue that requires them.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The WithTxn changes satisfy issue #1765 by fixing rollback error checks, avoiding double wrapping, and preserving callback error matching.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coveralls]

Coverage Report for CI Build 30987023694

Coverage increased (+0.06%) to 47.585%

Details

• Coverage increased (+0.06%) from the base build.
• Patch coverage: 2 of 2 lines across 1 file are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	39353
Covered Lines:	18726
Line Coverage:	47.58%
Coverage Strength:	15.41 hits per line

---

💛 - Coveralls

[rohilsurana]

Review

Clean, well-scoped bugfix. WithTxn used to report every failed transaction as a rollback failure. It printed the nil rollback error as %!s(<nil>) and added a duplicate rollback: prefix. This PR fixes all three underlying problems: the wrong variable in the condition, the double wrap, and the dead assignments in the panic path. It also adds a focused test and corrects the inverted assertions in user_repository_test.go.

What I checked

• errors.Is / errors.As matching is preserved. The callback error stays wrapped with %w on every path, so downstream matching does not change.
• No caller depends on the old message. The only rollback: match in non-test code is an unrelated comment in core/serviceuser/service.go.
• Panic path behavior is identical. The deferred func re-panics, so the named err return is never observed by the caller. Dropping those assignments changes nothing.
• Test coverage is strong. Commit, begin failure, rollback success and failure with exact messages, commit failure, and panic are all covered. The exact-message asserts guard against the %!s(<nil>) regression directly.

Verdict

Looks good to merge. There is one minor, non-blocking suggestion inline about making the rollback error matchable with errors.Is. The other inline notes are just clarifications for the review. Nice catch on the inverted errors.Unwrap(err) == tc.Err asserts too.