Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent nested transactions #32401

Merged
merged 2 commits into from
Feb 28, 2025
Merged

Conversation

betodealmeida
Copy link
Member

SUMMARY

The @transaction decorator is used at the command level to ensure a single point of commit/rollback in the application stack. Using the decorator instead of individual calls to db.session.commit() in the stack ensure that requests are atomic.

Unfortunately we have a pattern where commands may call other commands. For example, the UpdateDatabaseCommand.run() method (which is decorated) calls the UpdateSSHTunnelCommand.run() method, which is also decorated with @transaction. This can lead to a situation where the SSH tunnel is updated and committed, but the database update fails and tries to rollback, resulting in an inconsistent state.

To ensure atomicity, this PR modifies the decorator to check if it's inside another decorated method. If so, the decorator returns the function unmodified, behaving as a no-op. With this change, for the example above the SSH tunnel update would only be committed together with the database changes.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Added basic unit tests.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot bot added the change:backend Requires changing the backend label Feb 26, 2025
@betodealmeida betodealmeida force-pushed the prevent-nested-transactions branch from a6fb190 to 8625151 Compare February 26, 2025 20:27
Copy link

korbit-ai bot commented Feb 26, 2025

I was unable to post the issues I found. This could be because a force push or squash has changed the commit history since I scanned this pull request. You can get another review by commenting /korbit-review.

Copy link
Contributor

@Vitor-Avila Vitor-Avila left a comment

Choose a reason for hiding this comment

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

This is super cool, and I think it's a more elegant solution than calling a run() (or similar method) with or without the @transaction decorator. I've tested this locally (as it will simplify #32231) and confirmed it works 🙌

@betodealmeida betodealmeida merged commit 128c45e into master Feb 28, 2025
40 checks passed
@betodealmeida betodealmeida deleted the prevent-nested-transactions branch February 28, 2025 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:backend Requires changing the backend size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants