fix(versioning): make baseline capture atomic#41941
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41941 +/- ##
==========================================
+ Coverage 64.79% 64.80% +0.01%
==========================================
Files 2740 2740
Lines 153125 153122 -3
Branches 35124 35124
==========================================
+ Hits 99219 99234 +15
+ Misses 52009 51987 -22
- Partials 1897 1901 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mikebridge
force-pushed
the
sc-112938-baseline-fail-open
branch
from
5ab3c09 to
69fbac8
Compare
mikebridge
left a comment
mikebridge
left a comment
There was a problem hiding this comment.
Parallel local review synthesis
Review lenses: SQLAlchemy, Python, Superset committer, Tidy First, and Domain-Driven Design.
Update: all findings applied
- HIGH — the connection-level SAVEPOINT inside
before_flushneeded proof that canonical Continuum history survives. Applied by asserting both parent and child failure paths still create exactly one canonicaloperation_type=1shadow containing the committed value, linked to the legitimateversion_transaction, and exposed through the versions API. - MEDIUM — atomic rollback checks omitted the synthetic transaction row. Applied by asserting exactly one transaction remains after failure: the canonical update transaction. No optional baseline transaction leaks.
- MEDIUM — the unusual connection-level SAVEPOINT needed lifecycle rationale. Applied in the function documentation:
Session.begin_nested()would recursively flush insidebefore_flush, while the connection-level boundary isolates only direct SQL; the canonical-history regressions protect its Continuum compatibility. - LOW — tests imported a private
conftestcleanup helper. Applied by exposing and using the publicclear_version_tablestest utility. - LOW — the function docstring lacked a complete one-line summary. Applied with a concise first-line summary.
- CI status. A fresh CI run was triggered by commit
69fbac8e47; required jobs are being monitored before the PR leaves draft.
The Domain-Driven Design review returned no findings and considers the synthetic transaction, parent shadow, and child shadows one appropriate consistency boundary.
Post-fix evidence:
- 2 PostgreSQL fault-injection tests passed.
- 18 complete baseline/capture regression tests passed on fresh PostgreSQL.
- Focused mypy and Ruff checks passed.
Resolution: all actionable findings from this review round were applied; none remain open.
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Nice, @mikebridge, LGTM. The connection-level SAVEPOINT is the right way to keep this atomic without recursively flushing inside before_flush, and the fault-injection tests are solid. Approving!
Code Review Agent Run #5eb4abActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Makes optional entity-version baseline capture genuinely fail-open and all-or-nothing.
A single SAVEPOINT now contains the synthetic baseline transaction row, parent shadow row, and every child shadow row. Child-handler failures propagate to that boundary so the complete optional baseline unit rolls back before the error is logged, while the outer user save remains committable.
This supersedes the parent-only approach in #41910, which was closed in favor of the broader parent/child atomicity work tracked by sc-112938.
Parent/child atomicity
The parent is the versioned entity baseline:
tables_versiondashboards_versionChildren are the related shadows required to reconstruct that entity:
table_columns_versionandsql_metrics_versiondashboard_slices_versionandslices_versionThe atomic baseline unit is:
If every optional write succeeds, the SAVEPOINT is released and the complete baseline remains. If any parent or child write fails, the SAVEPOINT removes the synthetic transaction, parent baseline, and every child baseline written so far. The actual user edit still commits with its normal canonical Continuum update history.
This prevents partial history such as a dataset baseline that exists without its column or metric baselines, which could make a later restore incomplete or incorrect.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; backend transaction-safety fix.
TESTING INSTRUCTIONS
Run the focused PostgreSQL failure cases:
Run the complete baseline/capture regression set:
Observed locally: 2 fault-injection cases and 18 complete regression cases passed on PostgreSQL. The failure cases assert that canonical
operation_type=1history and the versions API result survive rollback of the optional baseline.GitHub CI passes on PostgreSQL, MySQL, and SQLite, including pre-commit.
ADDITIONAL INFORMATION