fix(versioning): SAVEPOINT-isolate baseline capture so a failure can't poison the save#41910
fix(versioning): SAVEPOINT-isolate baseline capture so a failure can't poison the save#41910mikebridge wants to merge 2 commits into
Conversation
github-actions
Bot
added
risk:db-migration
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41910 +/- ##
==========================================
- Coverage 64.78% 64.78% -0.01%
==========================================
Files 2739 2739
Lines 152981 152981
Branches 35058 35058
==========================================
- Hits 99114 99107 -7
- Misses 51970 51974 +4
- Partials 1897 1900 +3
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:
|
The parent-baseline direct-SQL inserts in baseline/insertion.py ran under ``try/except`` + ``no_autoflush`` but not a SAVEPOINT. On PostgreSQL a failed statement aborts the whole transaction even when the exception is caught, so a baseline-insert failure would poison the user's save — contradicting the "versioning must never break a save" guarantee. Wrap the inserts in ``session.connection().begin_nested()`` so a failure rolls back to the savepoint and the outer transaction stays healthy, degrading to "no baseline row" instead of a broken save. This mirrors the guard the change-record persist path (changes/listener.py) and the child-baseline path (baseline/collection.py) already use; the parent-baseline path was the one spot missing it. Only reachable with ENABLE_VERSIONING_CAPTURE on (the base infra ships dark/off), so this is a pre-flag-flip correctness fix, not a deploy-time regression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Inject a real failing SQL statement into the parent-baseline path (insert_baseline_shadow_row) during a capture-on chart save, and assert the user's edit still commits. Without the begin_nested() SAVEPOINT this fails on PostgreSQL — the aborted transaction poisons the outer save; with it, the baseline attempt rolls back to the savepoint and the save succeeds. Uses a real SQL error rather than a Python raise because only a failed DB statement poisons a PostgreSQL transaction (so the test is a genuine guard on the Postgres CI job; it passes trivially on SQLite, which doesn't poison). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mikebridge
force-pushed
the
fix-versioning-baseline-savepoint
branch
from
26f82a3 to
44cd2a4
Compare
github-actions
Bot
removed
risk:db-migration
Code Review Agent Run #a82787Actionable 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 |
|
Closing in favor of the broader transaction-correctness follow-up tracked in sc-112938. This PR proves the PostgreSQL transaction-poisoning problem for a failed parent baseline write, but its The replacement work is being developed separately from current |
1 participant
SUMMARY
Parent baseline capture in
superset/versioning/baseline/insertion.pyruns its direct-SQL inserts undertry/except+no_autoflushbut not a SAVEPOINT. On PostgreSQL a failed statement aborts the entire transaction even when the exception is caught, so a baseline-insert failure would poison the user's save — contradicting the "versioning must never break a save" guarantee this subsystem is built around.This wraps the inserts in
session.connection().begin_nested(), so a failure rolls back to the savepoint and the outer transaction stays healthy — degrading to "no baseline row" instead of a broken save. It mirrors the guard the change-record persist path (changes/listener.py) and the child-baseline path (baseline/collection.py:133) already use; the parent-baseline path was the one spot missing it.The affected path only runs with
ENABLE_VERSIONING_CAPTUREon (the base infra ships dark/off), so this is a pre-flag-flip correctness fix, not a deploy-time regression — hence a follow-up rather than a change to #41176 at its merge gate.Surfaced during a multi-model review pass on #41176.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend transaction-safety fix.
TESTING INSTRUCTIONS
Mirrors an existing, tested pattern (
baseline/collection.py). Includes a dedicated fault-injection regression test —test_baseline_failure_does_not_break_the_saveintests/integration_tests/versioning/versions_api_tests.py— which injects a real failing SQL statement into the parent-baseline path during a capture-on chart save and asserts the edit still commits. Without the SAVEPOINT this fails on the PostgreSQL CI job (the aborted transaction poisons the outer save); with it, the baseline rolls back and the save succeeds. It uses real SQL rather than a Python raise because only a failed DB statement poisons a Postgres transaction — so it passes trivially on SQLite, and the Postgres job is the real guard. The existing versioning capture tests cover the happy path.ADDITIONAL INFORMATION
ENABLE_VERSIONING_CAPTURE(the affected code path only runs with capture on)🤖 Generated with Claude Code