Repository navigation
fix(versioning): close the two retention-interaction races - #44012
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44012 +/- ##
=======================================
Coverage 80.16% 80.16%
=======================================
Files 2925 2925
Lines 172563 172567 +4
Branches 40058 40058
=======================================
+ Hits 138337 138344 +7
Misses 31629 31629
+ Partials 2597 2594 -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:
|
|
@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 Race 2: Evidence: 6 tests in |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed at head eff38b0e3b031b3776f513e98a7b569a089cd48c. I verified both races independently rather than reading the rationale — including actually running the revert-the-fix control. One finding, and it lands on this PR's own thesis.
Race 2 — verified correct, and I can show the bug
I reverted only the five production files to master, kept your tests, and ran them (RAN, pytest tests/unit_tests/versioning/test_retention_races.py): 4 of 6 fail, and the failure is the bug itself, not a shape mismatch:
test_get_version_returns_none_when_resolved_version_is_pruned
E AssertionError: assert {'value': 'v2', '_version': {'transaction_id': 102, ...}} is None
On master, get_version hands back v2's snapshot under v1's version uuid. That is exactly the silent wrong-snapshot failure, reproduced. At head all 6 pass. The two that also pass on master are the control/invariant tests, which is the correct design.
Two supporting details I checked rather than assumed:
- Dropping
.order_by()and keeping a bare.limit(1)is deterministic:identity_filter(id, uuid)+.where(ver_tbl.c.transaction_id == transaction_id)(queries.py:535-536) is an exact match onsa.PrimaryKeyConstraint("id", "transaction_id")(migration56cd24c07170, lines 167/217/278). At most one row, so no ordering is needed. It is also now a PK lookup instead of an ordered scan-and-skip — strictly cheaper than theOFFSETit replaced. - Your tests genuinely open the race window rather than asserting the new query shape:
resolve_versionis pinned to the pre-prune result, then the delete commits, then the fetch runs. That interleaving is what makes them regression tests.
Race 1 — it closes; the clock helper isn't what closes it
Worth stating plainly, because the PR description leads with the shared clock: the fix here is baseline_issued_at = naive_utcnow() (insertion.py:131) replacing the entity's audit timestamp. Retention keys on issued_at < cutoff, and config.py is explicit that "Baseline rows (operation_type=0) ... age out alongside the rest" — so a baseline captured today for an entity last edited two years ago was immediately prunable. Capture-time stamping closes that outright.
The cross-process skew you name in the docstring does not reopen it: _prune_old_versions_impl returns early when retention_days <= 0 (version_history_retention.py:479), so the smallest live window is one day, against NTP-scale skew. Bounded and immaterial, as you say.
I also checked the ordering claim rather than taking it: _baseline_first_ordering and resolve_version both sort (operation_type != 0).asc(), transaction_id.asc() — never issued_at — so the panel is unaffected. The activity feed does sort issued_at DESC (activity/queries.py:503), but it reads version_change_record, and baseline insertion writes only a transaction plus shadow rows and emits no change records. So that surface is unaffected too. Your stated trade-off — the baseline's displayed timestamp — is the whole of the visible change, and scoping it that way is accurate.
Finding: naive_utcnow already exists, so the "single clock" isn't single
superset/tasks/utils.py:185 already defines naive_utcnow() on master, with a byte-identical body (return datetime.now(timezone.utc).replace(tzinfo=None)). This PR adds a second one at superset/utils/dates.py:37 — same name, same signature, same semantics, different module.
That undercuts the new docstring's central claim, which is the load-bearing argument for the helper:
every writer and comparator of such columns ... must derive its value from this one helper so their agreement is structural, not comment-enforced.
It is not one helper. The existing one already has established consumers on naive-UTC columns, including a SQLAlchemy column default:
superset/models/task_subscribers.py:70—subscribed_at = Column(DateTime, nullable=False, default=naive_utcnow)superset/daos/tasks.py(started_at/ended_at),superset/models/tasks.py,superset/tasks/heartbeat.py:28
The sharpest version: inside superset/tasks/, two sibling modules now import the same symbol name from two different sources — heartbeat.py:28 from superset.tasks.utils, and your version_history_retention.py:60 from superset.utils.dates. That is the collision that bites a later reader or patch, and it makes the agreement comment-enforced again at the repo level, which is the exact failure mode the helper was introduced to remove.
I think your placement is the better one — utils/dates.py is the lower layer, and versioning/ importing from tasks/ would be poor layering. So the fix is to finish the move rather than revert it: keep the canonical definition in utils/dates.py and have tasks/utils.py re-export it. utils/dates.py imports only datetime and pytz, so there is no cycle. Behaviour is unchanged either way since the bodies are identical, which makes it safe to do here. Non-blocking, but I would rather it not land as two.
Things I checked and am not filing
- SC-120012 residual. "Pre-existing" is accurate: the only non-comment changes in
queries.pyare theresolve_versionswap and theOFFSET→transaction_idswap. The child-fetch block is untouched, the number of round-trips is unchanged, so the window is structurally identical tomaster. If anything the parent side narrows — a pruned target now yields an honestNoneinstead of a mismatched parent paired with current children. Documenting it at both the read site and the durable write site inrestore.pyis the right call. (SC-tracker IDs are established convention here — 18 files onmaster— so no objection to the reference style.) - The two polish commits. I diffed them excluding comments:
ba3711a2cdandeff38b0e3bchange no behaviour. Every replacement is the identical expression routed through the helper, includingnow_as_float(). Comments, naming, and tests only. - No dead code or stale imports.
_baseline_first_orderingis still used atqueries.py:379;datetimeis still used for annotations inversion_history_retention.py;sais still used atinsertion.py:88.
On resolve_version_uuid — since you asked
Confirmed caller-less: the only references at head are the definition (queries.py:462) and the VersionDAO binding (daos/version.py:74). I would keep it. Every other query function is bound on that façade, whose own docstring frames it as the namespace for API handlers and command classes; removing one member makes the surface arbitrary rather than safer. Your docstring already does the real mitigation by naming it prune-unstable and pointing at resolve_version. I would not spend a follow-up on it.
Gates
mergeable=MERGEABLE, mergeStateStatus=BLOCKED (review required only). CI re-derived from statusCheckRollup taking the latest leg per name (101 raw → 61 deduped): 49 success, 9 skipped, 2 neutral, 1 netlify context success — zero failing, zero pending. Codecov reports all modified lines covered.
Comment rather than approval — I am not an ASF committer on this repo, so this carries no merge authority. The only thing I would want changed before merge is the duplicate helper.
Code Review Agent Run #efa25cActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks Mike, verified and approving. I re-ran everything at head eff38b0 rather than trusting the description:
tests/unit_tests/versioning: 151 passed at head.- Reverted-fix control (the five production files checked out from base
7d0cf1a, your tests kept): 4 of 6 fail, and the failures are the bugs themselves. TheOFFSETfetch hands backv2's snapshot underv1's version uuid, and the freshly captured baseline lands straight in the prune candidate window. The attribution and no-prune controls stay green in both states, matching Amin's run exactly. - Race 1 mechanism confirmed in source: the same flush's real edit closes the baseline's shadow row, so the live-row preservation rule in
_resolve_prune_windowcannot protect the baseline transaction andissued_atis its only defense. Capture-time stamping vianaive_utcnow()keeps it at least one full retention window from any cutoff, since_prune_old_versions_implreturns early forretention_days <= 0. - What I like most about the Race 2 fix is that it does not lean on isolation level at all: the fetch addresses
(id, uuid, transaction_id), an exact match on the shadow PK. On Postgres READ COMMITTED a prune committing between resolution and fetch yields an honestNone; on MySQL REPEATABLE READ the transaction's snapshot serves the correct row; a wrong neighbour is impossible on any backend. It also makes the read path symmetric withrestore_version, which already addressed bytransaction_id. - No coupling with the other open versioning PRs: no shared files, and this change introduces no locks, so there is no lock-ordering or merge-order constraint with the row lock in #44027.
One ask before merge, seconding Amin's finding rather than re-litigating it: naive_utcnow now exists twice (superset/tasks/utils.py and the new superset/utils/dates.py), with two sibling modules under superset/tasks/ importing the same name from different sources. His suggested shape is right: keep the canonical definition in utils/dates.py and re-export from tasks/utils.py. Small, behavior-preserving, and it makes the docstring's "one helper" claim true. Approving now since it changes no behavior, but please fold it in before merging.
Just a small NIT: the description's test counts predate the last two commits (the file has 6 tests, the suite is 151, and the control flips 4, not 2). Worth refreshing so future readers reproduce the same numbers.
The baseline transaction's issued_at was sourced from the entity's historical audit fields, but retention keys on issued_at — so capturing a baseline for an entity last edited outside the retention window let the very next prune delete it, permanently emptying the entity's pre-edit history (its shadow row is closed by the same flush's real edit, so the live-row preservation rule does not protect it). The baseline is now stamped with capture time on the same naive-UTC clock the prune's cutoff uses (the old sa.func.now() fallback mixed in the database server's local clock). Panel ordering is unaffected — the baseline sorts first by operation_type, not issued_at — and attribution stays with the pre-versioning author via user_id. Tests drive the real _insert_baseline_row against a Continuum-shaped in-memory store: a fresh baseline for a 100-day-idle entity survives a 90-day window (reverted fix: expires immediately), the stamp is naive, and attribution + shadow shape are pinned unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
get_version resolved the target version to a stable transaction_id and then threw that resolution away, re-fetching the snapshot by positional OFFSET over the ordered row set. A retention prune committing between resolution and fetch shifts the offset and silently surfaces a DIFFERENT version's snapshot under the requested version uuid — on the preview/restore read path. The fetch now addresses the row by the resolved transaction_id (with the identity filter kept, so a transaction id recycled on another entity can never match); the restore command already addressed by transaction_id, so this closes the read side. Tests drive the real get_version against the shared Continuum-shaped in-memory store: with the resolution pinned pre-prune and the baseline row deleted mid-request, the tx-addressed fetch still returns the resolved version (the reverted OFFSET fetch returns the successor's snapshot); a no-prune control pins the unchanged happy path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
Four-lens round (clean-code, tidy-first, python, sqlalchemy): - the one-clock invariant is now structural: naive_utcnow() in utils/dates.py is the single derivation used by BOTH the baseline capture stamp and the retention prune's cutoff, instead of two inlined expressions agreeing by comment - get_version's docstring no longer names the helper the fix replaced (all four lenses); resolve_version_uuid documents that it has no in-repo callers and that prune-surviving callers must address by transaction id - the insertion comment owns the accepted trade-off in one sentence (pre-versioning author shown beside a capture-time stamp) and is phrased timelessly; the queries comment notes the display index may lag a concurrent prune while the snapshot cannot; the SqlaTable child-snapshot fetches carry a residual-window note (pre-existing policy behavior, per the sqlalchemy lens) - third race outcome pinned: the requested version itself pruned between resolution and fetch returns None, never a neighbour's snapshot - test idioms aligned (collections.abc.Iterator, MockerFixture) and the tzinfo assertion no longer overclaims what SQLite can verify Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
Independent (coordinator) review approved both fixes; this folds its required items and two accepted suggestions: - the deferred child-snapshot gap is now traceable: SC-120012 referenced at BOTH sites — get_version's read-only note and, per the committer lens's trace, the restore command's revert(relations=...), where a pruned closed child row means a durable incomplete column/metric write, not just a stale read - the one-clock comments no longer overstate: same UTC reference and derivation via the shared helper, with cross-process wall-clock skew named (web/worker vs Celery beat) and dismissed at day granularity - now_as_float() derives from naive_utcnow(), making the helper the single home of the naive-UTC expression in this module - end-to-end agreement test: a baseline captured via the real _insert_baseline_row, then closed as production's same-flush edit closes it, is not even a CANDIDATE for the real _resolve_prune_window at a 90-day cutoff — locking the stamp and the cutoff to one clock through both real callers resolve_version_uuid stays (documented caller-less faade surface); removal offered as a follow-up in review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
…-exported from tasks.utils gabotorresruiz review on apache#44012 (seconding an earlier finding): `naive_utcnow` existed twice — the original in `superset/tasks/utils.py` and a new copy in `superset/utils/dates.py` — with sibling modules importing it from different sources, so the "single clock" claim in its docstring was not structurally true. Keep the canonical definition in `superset/utils/dates.py` (the leaf module every naive-UTC writer can import) and turn `superset/tasks/utils.py`'s definition into a re-export, so the task modules that import it from there keep working unchanged. Behaviour-preserving: both bodies were identical (`datetime.now(timezone.utc).replace(tzinfo=None)`). The canonical docstring now carries both rationales (Continuum's tz-naive `issued_at` and the naive task timestamp columns, where a tz-aware write lets some drivers convert to session-local time). `timezone` is dropped from `tasks/utils.py`'s imports as it was only used by the removed copy. Verified after the change: retention-races file 6/6, versioning unit suite 158, `naive_utcnow` importers (tasks) 41/41; reverted-fix control still flips 4 of 6 (the dedupe cannot mask the fix — same value from either import path). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
eff38b0 to
80d3c66
Compare
|
Folded in the |
Code Review Agent Run #3c4f3aActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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 |
|
@gabotorresruiz can your review bot not approve things if you're asking for fixes before merging? Just don't approve, or request changes. Approving something that's not ready can lead to issues if it gets merged before it's actually ready by your review's standards. |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM! Thanks @aminghadersohi and @gabotorresruiz for the thorough verification here, actually reverting the fix and running the races rather than just reading the rationale. Approving and merging.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Mike Bridge <michael.bridge@ext.preset.io> (cherry picked from commit 96ef982)
SUMMARY
Fixes SC-115615 — two retention-interaction races in the versioning backend, both confirmed by two independent review panels (10-lens capstone + Codex capstone Highs #4/#5), sharing test scaffolding. Both scale with
SUPERSET_VERSION_HISTORY_RETENTION_DAYSset and capture on — the fleet-enablement configuration.1. Freshly captured baselines could immediately expire (
superset/versioning/baseline/insertion.py). The baseline transaction'sissued_atwas sourced from the entity's historicalchanged_on, but the retention prune keys onissued_at— and the baseline's shadow row is closed by the same flush's real edit, so the prune's live-row preservation rule doesn't protect it. Capture a baseline for an entity last edited outside the retention window and the very next prune deleted it: the entity's pre-edit history gone permanently. The baseline is now stamped with capture time on one clock (naive-UTC, matching Continuum's storage and the prune's cutoff derivation; the oldsa.func.now()fallback mixed in the DB server's local clock). Panel chronology is unaffected — baseline-first ordering is byoperation_type, notissued_at— and attribution stays with the pre-versioning author viauser_id.2. Snapshot fetch by OFFSET shifted under a concurrent prune (
superset/versioning/queries.py).get_versionresolved the target to a stabletransaction_id, threw it away, and re-fetched by positional OFFSET — a prune committing in between shifts the offset and silently surfaces a different version's snapshot under the requested version uuid, on the preview/restore read path. The fetch now addresses the row by the resolvedtransaction_id(identity filter kept, so a transaction id recycled on another entity can never match). The restore command already addressed bytransaction_id; this closes the read side.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend correctness.
TESTING INSTRUCTIONS
python -m pytest tests/unit_tests/versioning -q # 158 passed (6 in test_retention_races.py)The new tests drive the real production functions against a shared Continuum-shaped in-memory store. Reverted-fix control (the four fixed source files checked out from the base commit): four of the six tests flip — the two Race 1 tests,
test_baseline_survives_a_full_retention_window_from_capture(baseline expires immediately) andtest_fresh_baseline_is_not_in_the_prune_candidate_window, and the two Race 2 tests,test_get_version_returns_resolved_tx_after_concurrent_prune(OFFSET returns the successor's snapshot) andtest_get_version_returns_none_when_resolved_version_is_pruned— while the attribution/shadow-shape and no-prune controls stay green in both states (4 failed, 2 passed).ADDITIONAL INFORMATION
ENABLE_VERSIONING_CAPTURE(+ retention configured, for the code paths involved)🤖 Generated with Claude Code
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW