mikebridge commented on PR #44012: URL: https://github.com/apache/superset/pull/44012#issuecomment-5589131708
@aminghadersohi — requesting review on the retention-race pair, in your versioning wheelhouse: two data-correctness fixes on the prune path, now through both my four-lens gate and an independent coordinator review (approve; both races confirmed closed under adversarial re-derivation). **Race 1**: a freshly captured baseline could expire at the very next prune — its transaction was stamped with the entity's historical `changed_on` while retention keys on `issued_at`, and the live-row preservation rule can't save it because the same flush's real edit closes the baseline's shadow row (verified against Continuum's `update_version_validity`). Baselines are now stamped with capture time via a shared `naive_utcnow()` — the *same* helper the prune cutoff derives from, so the one-clock agreement is structural (same UTC reference/derivation; cross-process wall-clock skew is named in the comments and immaterial at day granularity). Attribution stays with the pre-versioning author; ordering is op-type-first so the panel is unaffected. **Race 2**: `get_version` resolved a stable `transaction_id`, discarded it, and re-fetched by OFFSET — a prune between the two silently served a *different version's snapshot* under the requested uuid. Now fetched by the resolved tx id (shadow PK `(id, transaction_id)` makes the un-ordered `limit(1)` deterministic); the third outcome is pinned — the requested version itself pruned mid-request yields an honest 404, never a neighbour. Evidence: 6 tests in `test_retention_races.py` drive the real `_insert_baseline_row`, `get_version`, and `_resolve_prune_window` against a Continuum-shaped in-memory store; the reverted-fix control flips exactly the two race tests (the OFFSET failure surfaces as `assert 102 == 101`), and an end-to-end test locks capture-stamp-vs-cutoff through both real callers. One known residual, deliberately out of scope and now traceable as SC-120012 at both sites: the SqlaTable child-snapshot path (`get_version` reads and `revert(relations=…)` *writes* from closed child shadow rows that retention can legitimately prune). Also flagging for your call: `resolve_version_uuid` is now caller-less (documented as retained façade surface) — happy to delete it and the DAO staticmethod in a follow-up if you'd rather remove the prune-unstable index as an attractive nuisance. -- 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]
