mikebridge opened a new pull request, #44251: URL: https://github.com/apache/superset/pull/44251
### SUMMARY Restoring a `SqlaTable` version reconstructs its columns/metrics from the child shadow rows valid at the target transaction. Version-history retention can have legitimately pruned exactly those rows while the parent's row survives — and SQLAlchemy-Continuum's reverter then finds no child version at the target and **deletes the live child outright**: a durable write of an incomplete column/metric set, silently reported as success (sc-120012, surfaced by the independent review of #44012). **Restore now fails closed.** Before any write, the engine verifies the target's child history and refuses with a user-facing 422 (`PrunedChildHistoryError`: "…can no longer be fully restored: … pruned by version-history retention. The entity was left unchanged.") when a needed row was pruned. Detection is validity-**interval** analysis over the surviving rows — the pruner also deletes the `version_transaction` rows and cascades the change records, so the chain is the only evidence: - a surviving row's `[transaction_id, end_transaction_id)` covering the target proves the state either way (non-DELETE: present, restored; DELETE: provably absent — a closed end is Continuum's evidence of a re-insert); - with no covering interval, only born-after passes (every surviving row beyond the target, earliest is the birth INSERT); - anything else is a gap that may have covered the target: refuse. The verification read is taken `FOR UPDATE` in the transaction that owns the restore — every row the verdict depends on, covering DELETE witnesses included (the reverter selects the latest surviving row at/before the target *then* excludes DELETEs, so a pruned absence witness would resurrect an older incarnation) — closing the check-to-write window under READ COMMITTED: the pruner's SERIALIZABLE pass blocks on the locks until the restore commits. On SQLite, where `FOR UPDATE` is a no-op and pysqlite's legacy mode holds no transaction for SELECTs, the verifier takes a `BEGIN IMMEDIATE` write reservation up front (issued through the SQLAlchemy connection so contention translates and maps to 422). Also per the ticket: `get_version`'s parent + child reads now run in ONE snapshot (`REPEATABLE READ` on MySQL/Postgres; an explicit BEGIN on SQLite via the documented pysqlite recipe) so a concurrent prune cannot age out child rows between the two reads, and the deferred notes in `get_version` / `restore.py` carry the sc-120012 disposition. **Known limitation** (ratified on sc-120012): the guard refuses every *detectable* pruning of needed child history and protects all restores targeting versions within the retention window. One residual fails open: a column/metric deleted **after** the target whose entire shadow chain — including its closing DELETE row — has aged out of retention leaves no surviving evidence anywhere (shadow, transaction, and change-record rows are pruned together; change records are per-tx diffs, never a full set), so no read-side guard can detect it. It is only reachable for targets older than the retention window, is pinned by a documented-limitation test (to be inverted into a requirement when the follow-up lands), and closing it requires the deferred retention-policy change: the pruner preserving the dependency closure of restorable parents. Review: three adversarial cross-model rounds (committer + sqlalchemy lenses) folded before opening — including an interval-semantics rewrite of the verifier after a confirmed counterexample, the DELETE-witness lock scope, and the SQLite reservation path. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF API behavior. Before: `POST /api/v1/dataset/<uuid>/versions/<version>/restore` against a version whose child history was pruned → 200 OK, dataset silently loses columns. After: 422 with the message above; the dataset is untouched. ### TESTING INSTRUCTIONS - `pytest tests/unit_tests/versioning/test_restore.py` — 13-case interval-algebra parametrization (covered / DELETE-covered / born-after pass; every detectable hole refuses, including the review round's confirmed counterexample), engine guard branches, endpoint 422 mapping pin. - `pytest tests/integration_tests/versioning/restore_fails_closed_tests.py` — DB-level: command-level refusal with a COMMITTED prune and no test-owned rollback (the command's transactional cleanup owns the unchanged state); refusal-precedes-any-write ordering pin (write phase never entered, session pending-state clean before rollback); the guard-removed CONTROL proving the incomplete write reappears; intact-history restore unchanged; FOR UPDATE lock test (PG/MySQL: concurrent DELETE hits the dialect's lock-wait error, succeeds after release); SQLite write-reservation pins (real transaction held; concurrent writer blocked; contention surfaces as the command failure type); the documented-limitation pin. ### ADDITIONAL INFORMATION - [x] Has associated issue: sc-120012 - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
