Repository navigation
fix(dataset): refresh the entity under the conditional-write row lock - #44027
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44027 +/- ##
==========================================
+ Coverage 79.85% 80.04% +0.18%
==========================================
Files 2915 2920 +5
Lines 170702 172008 +1306
Branches 39561 39827 +266
==========================================
+ Hits 136317 137676 +1359
+ Misses 31868 31735 -133
- Partials 2517 2597 +80
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 dataset PUT's conditional-write path locked the row with an id-only SELECT ... FOR UPDATE that discarded the values. On MySQL/InnoDB default REPEATABLE READ every consistent read in a transaction is pinned to the read view established by its first read (the request's auth queries), so the entity the update command loaded after the lock still reflected the pre-lock snapshot — and the ORM, which only emits UPDATEs for attributes differing from the loaded values, could silently lose a concurrent commit that landed between the initial load and the lock. Take the lock as a full-entity ORM read instead: populate_existing() + with_for_update(). A locking read is exempt from the REPEATABLE READ snapshot and returns current committed data, and populate_existing() writes it into the identity-map object that the command's later find_by_id returns. Postgres (READ COMMITTED) and SQLite are not exposed to the staleness; the refresh is harmless there. The staleness is also object-level on every backend (a loaded entity is never refreshed by a plain re-read), which is what the two-transaction integration tests pin. Counterpart of the version-restore fix in apache#44015. sc-120014 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
98847ad to
ac59e41
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
… command Cross-model review blocker: the locking refresh discarded its result, but the session's identity map references clean persistent objects weakly, so with no strong reference the refreshed object is collectible the moment the helper returns. The command's find_by_id would then re-hydrate a new object from a plain consistent read — on MySQL REPEATABLE READ the pre-lock snapshot again — silently reducing the fix to a no-op on the production path (which, unlike the original test, holds no reference of its own between the lock and the lookup). lock_entity_for_update returns the locked entity and the dataset PUT binds it for the rest of the request. New integration test pins both halves: held, DatasetDAO.find_by_id returns the very same refreshed instance; discarded, a weakref proves the object is collected. The unit shape pin asserts the return contract. sc-120014 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
DatasetDAO.find_by_id applies base filters that read g.user; the two tests calling it from the test process (rather than through a request) failed on the CI Postgres/MySQL lanes with AttributeError: user. Wrap the lookups in override_user(admin), matching the suite idiom for direct DAO calls. sc-120014 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
|
@aminghadersohi — requesting review on the conditional-write lock fix (sc-120014), the update-path sibling of #44015's restore fix and squarely in your versioning lane. The dataset PUT's Fix: the lock is now a full-entity ORM read —
Tests: 3 unit statement-shape pins (dropping either flag fails on every backend; mirrors #44015's refresh-flag guard) + 3 two-transaction integration tests (object-level staleness, so the interleave test fails pre-fix on every backend it runs on; SQLite skipped — a second writer connection can't model a concurrent request there). CI green including the full DB matrix. 🤖 Generated with Claude Code |
Code Review Agent Run #afcb23Actionable Suggestions - 0Additional Suggestions - 1
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 |
|
Dependency note for reviewers: this PR closes the entity-read half of the MySQL REPEATABLE READ conditional-write race; #44091 (in review) closes the validator-read half ( 🤖 Generated with Claude Code |
The MySQL CI lane runs InnoDB's default REPEATABLE READ (no isolation_level is configured anywhere, and set_db_default_isolation's READ COMMITTED default is generative-and-discarded, i.e. inert), so the interleave test also flips at the row level there if with_for_update() is dropped. Documented so a future fix making the isolation default effective doesn't silently weaken what this file proves; the unit shape pin stays the every-backend guard. sc-120014 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
…shot Correction round: CI pins the MySQL server to READ COMMITTED (.github/workflows/bashlib.sh:97-98) — the previous docstring note had this backwards. Under CI's RC, a mutant keeping populate_existing() but dropping with_for_update() passes every lane, because an RC re-read sees the concurrent commit without any lock. The new MySQL-lane test forces REPEATABLE READ on the session's connection so that mutant fails behaviorally: the unlocked re-read serves the pre-commit snapshot while the locking read is exempt and returns committed data. (PostgreSQL's RR raises a serialization failure on such a locking read instead — a different, also safe, outcome — so the forced-RR proof is MySQL-scoped.) sc-120014 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks Mike, this is a careful fix and the test story is unusually strong.
I verified it empirically on this branch against a real MySQL 8.0.46 at the server default REPEATABLE READ (stricter than CI, which pins READ COMMITTED in bashlib.sh):
- All 4 integration tests and the 3 unit pins pass at head. At the parent commit (
bc35151) the three behavioral tests fail exactly as the PR body describes, and the quiet path control keeps passing. - Both mutants are killed: dropping
with_for_update()fails the forced RR test and the interleave test, and droppingpopulate_existing()fails both as well, plus the unit shape pin in each case. - I also drove the real PUT route live on that MySQL: a fresh
If-Matchsave returns 200 and bumps the version, a replayed stale token returns 412, and an unconditional PUT is untouched. - The lifetime coupling holds as documented:
DatasetDAOnever setsforce_fetch, soBaseDAO._queryreturns the refreshed identity map instance to the update command, andfind_by_idhanded back the very same object in my runs.
One note for whoever lands second between this and #44091: the #44091 diff rewrites the same hunk in datasets/api.py and currently calls lock_entity_for_update without binding the return, so the rebase must keep the _locked_entity binding or the refresh quietly dies with the discarded reference, exactly as the helper docstring warns. Both PR bodies already carry the caveat, so this is just a reminder at merge time.
Code Review Agent Run #d75c47Actionable 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 |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Confirmed @gabotorresruiz's note is a coordination reminder for #44091, not a blocker here, that one's still open, so there's nothing to rebase against yet. Reproduced the core mechanism myself independently of the MySQL testing already done: a plain SQLAlchemy re-query returns the same stale identity-mapped object after a concurrent commit, populate_existing() correctly overwrites it in place. Binding _locked_entity in datasets/api.py matches the helper's own lifetime contract.
LGTM, merging.
…SHARE The conditional-write guard compared the client's If-Match token against a version-info lookup made with plain consistent reads. On MySQL/InnoDB REPEATABLE READ those are served from the snapshot pinned by the request's first read, so a version row committed by a concurrent writer between that first read and this request's row lock stayed invisible — the stale token matched, raise_for_stale_write passed, and the second writer overwrote the first: the lost update the lock + If-Match pair exists to prevent, surviving on MySQL only. Validator-layer sibling of the entity refresh in apache#44027. Add current_live_transaction_id_for_share — a plain live-row query (not the aggregate: locking clauses and aggregates don't combine reliably across dialects) under with_for_update(read=True), exempt from the RR snapshot without blocking readers — and thread lock_for_stale_check through current_entity_version_info; the dataset PUT passes it only for conditional writes. The displayed version number stays a plain aggregate read: a concurrent commit can leave response metadata one behind, but the guard never consults it. sc-120050 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
SUMMARY
The dataset PUT's conditional-write path (
If-Matchsaves) serialises check-and-write by row-locking the entity first (lock_entity_for_update, called fromsuperset/datasets/api.py). But the lock was an id-onlySELECT id ... FOR UPDATEthat discarded the row's values — it serialised writers without refreshing what the transaction can see:UPDATEs for attributes that differ from the loaded values — could silently lose a concurrent commit that landed between the initial load and the lock. This is the same class as the version-restore partial-write fixed in fix(versioning): row-lock the live entity during version restore #44015, on the ordinary update path.SELECT, so even where the row read is fresh (Postgres READ COMMITTED), the identity-map object the update command operates on stayed stale.Fix (mirroring #44015's locking-refresh): take the lock as a full-entity ORM read —
db.session.query(model).populate_existing().filter(id).with_for_update().one_or_none(). A locking read is exempt from the REPEATABLE READ snapshot and returns current committed data;populate_existing()writes it into the identity-map object, which the update command's laterfind_by_idreturns without re-hydrating from its own (possibly stale) row.Two halves of one protection — merge-order note: this PR closes the entity-read half of the MySQL REPEATABLE READ conditional-write race. The validator-read half (
current_entity_version_infostill being a plain snapshot read, so a staleIf-Matchtoken can pass) is closed by #44091 — the race is fully shut only with both merged, in either order (whichever lands second takes a small rebase indatasets/api.py; #44091's body carries the same note and the_locked_entity-preservation warning).Lifetime contract (from the adversarial review round): the helper returns the locked entity and the caller must hold it — the identity map references clean persistent objects weakly, so a discarded result is collectible the moment the helper returns, after which
find_by_idwould re-hydrate from the stale snapshot and silently undo the fix on the production path (which loads nothing of its own before the lock). The dataset PUT binds the return value for the rest of the request. Missing-row semantics are unchanged:Noneis returned and existence keeps being decided by the command's own lookup (404 as before). Chart/dashboard PUTs have no conditional-write lock path, so the fix is dataset-scoped at the single shared helper.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — concurrency fix with no UI or response-shape change.
TESTING INSTRUCTIONS
pytest tests/unit_tests/versioning/test_lock_entity.py— 3 tests: the dialect-independent regression guard pins the statement shape (populate_existing()andwith_for_update()both chained, result consumed viaone_or_none(); dropping either fails on every backend — the analogue of fix(versioning): row-lock the live entity during version restore #44015'srefresh(..., with_for_update=True)pin), plus the non-numeric/None id early-outs that keep the/<pk>string route from raising a SQL cast error. Reverting the helper flips the shape test.pytest tests/integration_tests/versioning/conditional_write_lock_tests.py— the lifetime pin (held,DatasetDAO.find_by_idreturns the very same refreshed instance; discarded, aweakrefproves the object is collected — the identity map alone does not keep it alive) plus the two-transaction behavioral proof: a second engine-level connection commits a change between this session's entity load and the lock; afterlock_entity_for_updatethe loaded entity must show the concurrently committed value (fails pre-fix on every backend, since the staleness is object-level), plus a quiet-path control asserting the refresh is a no-op (and dirties nothing) when nothing changed underneath. Skipped on SQLite, where a second writer connection deadlocks against the open read transaction instead of modelling a concurrent request — CI's Postgres/MySQL runs execute them (local env here is SQLite-backed, so these two are CI-verified).If-Match. Before the fix the first save's flush could quietly discard the concurrent change on MySQL; after, the command diffs against committed state.ADDITIONAL INFORMATION
🤖 Generated with Claude Code
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW