Skip to content

fix: abortTransaction does not call provided execute function when parent span is null#3062

Merged
buenaflor merged 8 commits into
getsentry:mainfrom
ichiko:fix-3058-abortTransaction-without-parent-span
Jul 14, 2025
Merged

fix: abortTransaction does not call provided execute function when parent span is null#3062
buenaflor merged 8 commits into
getsentry:mainfrom
ichiko:fix-3058-abortTransaction-without-parent-span

Conversation

@ichiko

@ichiko ichiko commented Jul 10, 2025

Copy link
Copy Markdown
Contributor

📜 Description

This PR fixes a bug in the abortTransaction method where database operations were blocked when no parent span exists. The issue was reported in #3058.

💡 Motivation and Context

When parentSpan is null in the abortTransaction method (lines 147-153), the method was logging a warning and returning Future<T>.value() without executing the provided execute function. This meant the actual database transaction abort operation was never performed, leaving the database in an inconsistent state where:

  • Database transactions remained open indefinitely
  • Subsequent database operations could be blocked
  • Application could hang or fail on subsequent DB access attempts
  • Data integrity issues could occur

💚 How did you test it?

Added comprehensive tests to verify the fix.

  • abortTransaction executes function even when parentSpan is null - Verifies that the execute function is called even when no parent span exists
  • abortTransaction with parentSpan executes function and marks span as aborted - Verifies normal behavior when parent span exists

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@codecov

codecov Bot commented Jul 10, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (79f6b41) to head (e62cea5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##             main     #3062       +/-   ##
============================================
+ Coverage   78.66%   100.00%   +21.33%     
============================================
  Files          34         1       -33     
  Lines        1364        51     -1313     
============================================
- Hits         1073        51     -1022     
+ Misses        291         0      -291     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@buenaflor

buenaflor commented Jul 10, 2025

Copy link
Copy Markdown
Contributor

Thank you for the investigation and bug fix! I'll have a look later today (sry, accidentally clicked the close button)

@buenaflor buenaflor closed this Jul 10, 2025
@buenaflor buenaflor reopened this Jul 10, 2025

@buenaflor buenaflor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, I would only change one test case

Comment thread drift/test/sentry_drift_test.dart Outdated
@buenaflor

Copy link
Copy Markdown
Contributor

@ichiko last task is to update the changelog:

### Fixes

- Drift transaction rollback not executed when parent span is null ([#3062](https://github.com/getsentry/sentry-dart/pull/3062))

@buenaflor
buenaflor merged commit 9b706c1 into getsentry:main Jul 14, 2025
18 checks passed
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.

2 participants