Repository navigation
fix(versioning): row-lock the live entity during version restore - #44015
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
49d41d6 to
c128f22
Compare
2245308 to
6d3cd27
Compare
|
@aminghadersohi — ready for review when you have a moment. Concurrency fix on version restore: _do_restore() previously never coordinated with a concurrent writer, so an edit committed between the command's read and its revert could be silently lost. It now does a single A cross-model independent pass already went over this and caught the MySQL-RR issue (now fixed) — its consolidated report is in the spec repo. The dialect-independent regression guard is a unit test asserting the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44015 +/- ##
==========================================
+ Coverage 80.16% 80.18% +0.01%
==========================================
Files 2925 2925
Lines 172563 172675 +112
Branches 40058 40079 +21
==========================================
+ Hits 138337 138452 +115
+ Misses 31629 31628 -1
+ Partials 2597 2595 -2
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 — reworked at The earlier
Both are folded into a single locking re-read that re-asserts the active-row predicate: entity = (
db.session.query(self.model_cls)
.populate_existing() # reload current committed state
.filter_by(id=entity.id, deleted_at=None) # exclude soft-deleted rows
.with_for_update() # serialise the revert (MySQL RR)
.one_or_none()
)
if entity is None:
raise self.not_found_exc() # hard- or soft-deleted -> 404One statement now closes all three races (concurrent edit / hard delete / soft delete) with no dependency on which exception a missing-row read raises. Tests: the unit suite asserts the full locking chain ( |
|
Follow-up fix pushed ( |
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
Code Review Agent Run #edc369Actionable Suggestions - 0Additional Suggestions - 2
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 |
github-actions
Bot
added
the
requires:rebase
…restore sc-115423. BaseRestoreVersionCommand.run() wraps _perform() in @transaction(), but _do_restore() never coordinated with a concurrent writer: an overlapping edit (or a second restore) could commit between the command's read (in validate()) and its revert, and the lost update was silent. _do_restore() now does `db.session.refresh(entity, with_for_update=True)` immediately after validate() — a single locking read that (a) takes a FOR UPDATE lock held for the rest of the transaction, serialising the revert against a concurrent write, and (b) reloads the entity from the current committed state. The with_for_update flag is load-bearing, not an optimization: a plain refresh() is a non-locking consistent read that on MySQL/InnoDB REPEATABLE READ returns the transaction's first-read snapshot (from validate(), before the lock), so a concurrent edit would be invisible and silently dropped from the revert UPDATE — a partial restore. A hard delete committed in the read→lock window makes the locking refresh raise ObjectDeletedError, which is mapped to the command's not_found_exc (404), preserving the documented entity-gone-race behaviour rather than a 422. This is the pessimistic (serialise) half; the optimistic If-Match/412 detection the PUT path has is a separate, client-facing follow-up noted in code. Tests: parametrized over the three concrete restore commands asserting the refresh is called with with_for_update=True (dropping the flag fails the test — the guard against the MySQL staleness regression), an ordering test (refresh before resolve before revert), and a hard-delete→404 test. A real two- transaction DB test proving the MySQL full-overwrite is a noted follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
…ng active-row re-read
The coordinator (independent Codex + Path-B) review found the sc-115423
concurrency fix caught the wrong exception. `_do_restore` re-read the live
entity with `Session.refresh(entity, with_for_update=True)`; when the row was
hard-deleted between `validate()`'s unlocked read and the lock, refresh()'s
empty SELECT raises `InvalidRequestError` (a SQLAlchemyError) — NOT
`ObjectDeletedError`. `on_error` then wrapped it into `failed_exc`, so the
documented 404 surfaced as a generic 422. A concurrent *soft* delete was worse:
column loads bypass the global `deleted_at IS NULL` filter, so refresh() would
reload the archived row and the revert would silently resurrect it and report
success.
Replace the refresh with a single locking query that re-asserts the active-row
predicate:
db.session.query(model_cls)
.populate_existing() # reload current committed state
.filter_by(id=..., deleted_at=None) # exclude soft-deleted rows
.with_for_update() # serialise the revert (MySQL RR)
.one_or_none()
A None result (hard- or soft-deleted) maps to the documented `not_found_exc`
(404). One query closes all three races: concurrent content edit (the original
MySQL/InnoDB REPEATABLE-READ staleness), hard delete, and soft delete.
Tests:
- unit: assert the locking chain (populate_existing + filter_by(deleted_at=None)
+ with_for_update + one_or_none), the validate→read→resolve→restore order, and
None→not_found_exc. Dialect-independent guards.
- integration: two real second-connection races — hard delete and soft delete
committed between validate() and the lock — each asserting not_found_exc, not
a silent success or 422.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
…res DISTINCT+FOR UPDATE) The locking active-row re-read added for the restore concurrency fix broke the entire restore path on Postgres: `db.session.query(Slice)...with_for_update()` triggers the `lazy="subquery"` eager loader on `Slice.table`, which wraps the primary query into `SELECT DISTINCT ... FOR UPDATE` to fetch the related rows, and Postgres rejects `FOR UPDATE` with `DISTINCT` (psycopg2 FeatureNotSupported). Every restore returned 422. The mocked unit tests could not catch it — the offending SQL is only generated by the subquery loader at execution against Postgres, not at compile time — so it surfaced on the test-postgres CI leg (the existing happy-path test failed too, not just the new race tests). Fix: `.enable_eagerloads(False)` on the locking query. We only need the locked row's own columns for the revert; relationships load lazily afterward. Verified against real Postgres: the pre-fix query raises "FOR UPDATE is not allowed with DISTINCT clause"; with eager loads disabled it returns the row cleanly. The unit chain assertion now pins `enable_eagerloads(False)` so the step cannot be silently dropped; the test-postgres integration suite remains the behavioural guard for the SQL itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
253d432 to
10b1a23
Compare
Code Review Agent Run #a3813cActionable 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 |
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
Adversarial review of the version-restore row-locking change. The core fix — re-reading the live row under FOR UPDATE with populate_existing() before reverting, and surfacing a missing/soft-deleted row as a clean 404 rather than the transaction wrapper's 422 — is sound (on_error only re-wraps SQLAlchemyError, verified). One production correctness concern about the lock query's pinning, plus a stale docstring and two test-quality nits below. (Automated review; posted by a human reviewer.)
| # Postgres rejects ``FOR UPDATE`` with ``DISTINCT``. We only need the | ||
| # locked row's own columns here; relationships load lazily after. | ||
| .enable_eagerloads(False) | ||
| .filter_by(id=entity.id, deleted_at=None) |
There was a problem hiding this comment.
The lock re-read pins by id alone, not (id, uuid) — inconsistent with restore_version's own id-reuse defense, and yields a 500 instead of a 404. restore.py:138-142 deliberately pins the version lookup to (id, uuid) with the comment: "a hard delete frees the integer id, so matching on it alone can resolve a predecessor's version row and restore its content over the current entity." This locking query matches by id + deleted_at only.
Race: between validate() and this lock, the original entity is hard-deleted and its integer id is reused by a new row (explicit-id insert / seq reuse). This query locks that new row; populate_existing() swaps entity to it (a different uuid). resolve_version still runs against the original self._uuid, and restore_version then hits entity.uuid != entity_uuid and raises ValueError (restore.py:128). ValueError isn't a SQLAlchemyError, so on_error re-raises it as-is → 500, not the documented 404. Adding uuid=entity.uuid to the filter_by closes the window cleanly (row absent → one_or_none() → None → 404), and matches the pinning the codebase already warns is required.
There was a problem hiding this comment.
Good catch — fixed in 0e8c13a: the lock now pins (id, uuid, deleted_at IS NULL), so a hard delete + integer-id reuse reads as absent and one_or_none() → None surfaces the documented 404 instead of restore_version's ValueError → 500. Matches the (id, uuid) pinning restore_version already applies to the version lookup. The unit guard now asserts the uuid is part of the locking filter (dropping it fails 3 of 5 there), and the comment block records this as the fourth closed race.
| by another connection — the concurrent value does not survive. | ||
|
|
||
| The dialect-independent regression guard for the fix is the unit test | ||
| asserting ``refresh(..., with_for_update=True)`` (dropping the flag |
There was a problem hiding this comment.
This docstring describes an implementation that wasn't shipped. It calls the dialect-independent guard "the unit test asserting refresh(..., with_for_update=True)." But the shipped code uses db.session.query(...).populate_existing().enable_eagerloads(False).filter_by(id=…, deleted_at=None).with_for_update().one_or_none() — no refresh() — and the unit test (test_restore_version_concurrency.py) asserts exactly that query chain (populate_existing / with_for_update / filter_by / one_or_none), not any refresh() call. A maintainer reading this would look for a refresh(..., with_for_update=True) that doesn't exist, and might "restore" the refresh() form the code comments explicitly call buggy under REPEATABLE READ. Worth updating the docstring to name the actual guard.
There was a problem hiding this comment.
Fixed in 0e8c13a — the docstring now names the actual guard (the populate_existing().enable_eagerloads(False).filter_by(id=…, uuid=…, deleted_at=None).with_for_update().one_or_none() chain asserted by test_restore_version_concurrency.py) and says explicitly that there is no refresh() and why, so nobody restores the form the code calls buggy under REPEATABLE READ.
| with pytest.raises(cmd.not_found_exc): | ||
| cmd.run() | ||
|
|
||
| # Cleanup — clear the archival flag so a shared/session-scoped row does |
There was a problem hiding this comment.
Soft-delete cleanup runs only if the command raises the expected exception. This UPDATE slices SET deleted_at = NULL sits after the with pytest.raises(cmd.not_found_exc) block. If cmd.run() raises anything other than not_found_exc (e.g. the id-reuse ValueError→500 path, or a lock timeout), pytest.raises propagates it and this cleanup never executes — leaving the shared/session-scoped Boys chart soft-deleted and breaking later tests that query it. A try/finally or a fixture teardown (also for the slice_name reset in the concurrent-edit test) would make the cleanup unconditional.
There was a problem hiding this comment.
Fixed in 0e8c13a — both cleanups (the deleted_at reset here and the slice_name reset in the concurrent-edit test) are now in try/finally, issued over a separate connection after a session.rollback(), so they commit regardless of what run() raises.
| # restore endpoint does not yet also honor an If-Match precondition to | ||
| # *detect* (rather than serialise) a concurrent edit — a follow-up. | ||
| entity = ( | ||
| db.session.query(self.model_cls) |
There was a problem hiding this comment.
Minor (maintainability): this inline FOR UPDATE re-read is a second row-lock formulation alongside versioning/api_helpers.py:lock_entity_for_update (used by the conditional-write PUT path). The two must serialize on the same row, yet lock differently — PUT uses select(model.id).where(id==…).with_for_update(), restore uses query(model).filter_by(id=…, deleted_at=None).…with_for_update(). They do have different needs (restore also refreshes row content via populate_existing, which the helper doesn't), so they're not trivially interchangeable — but a future change to one's locking semantics won't be reflected in the other. Low priority; flagging so the divergence is a deliberate choice rather than an accident.
There was a problem hiding this comment.
Agreed it should be deliberate rather than accidental — recorded in 0e8c13a in the same comment block: restore is its own formulation because it must also reload the locked row's content (populate_existing), assert the active-row predicate, and pin the uuid, whereas lock_entity_for_update locks select(model.id) by id and returns nothing; both lock the same PK row, so the two paths still serialise against each other.
…st cleanup fitzee review on apache#44015: - The FOR UPDATE re-read pinned the live row by id alone. Between validate() and the lock, a concurrent hard delete followed by integer-id REUSE puts a different entity (different uuid) under that id: the lock then caught the stranger, populate_existing swapped `entity` to it, and restore_version's uuid check raised ValueError — a 500 instead of the documented 404. Pin the lock to (id, uuid) — the same defense restore_version already applies to the version lookup — so the reused row reads as absent and one_or_none() -> None surfaces the 404. The unit guard now asserts the uuid pin is part of the locking filter (dropping it fails 3 of 5 there). - Recorded, in the same comment block, why this is deliberately a separate formulation from api_helpers.lock_entity_for_update (PUT locks select(model.id) by id and returns nothing; restore must also reload the locked row's content, assert the active-row predicate, and pin the uuid — both lock the same PK row, so the paths still serialise). - The concurrent-edit test docstring named a `refresh(..., with_for_update=True)` guard that was never shipped; it now names the actual locking-chain guard. - Both integration tests' fixture cleanups (deleted_at reset, slice_name reset) ran only when the command raised the expected exception; a lock timeout or the 500 path would have leaked a soft-deleted / renamed shared chart into later tests. Moved to try/finally, via a separate connection so they commit regardless of the session's state. Verified: unit 5/5 (control 3 fail without the pin), version_restore integration 18/18 on Postgres. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
CI pre-commit (ruff-format) reflowed a hand-wrapped with-statement that fits the line length; my local check ran ruff's linter but not its formatter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
Code Review Agent Run #9f5c5cActionable 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 |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Reviewed aff64263d708; no substantive findings. CI is green. Review was based on source and available test evidence; full local integration/browser validation was not performed.
) Co-authored-by: Mike Bridge <michael.bridge@ext.preset.io> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 7f0ca47)
…ests Since apache#44015, _do_restore reloads the entity through a pessimistic FOR UPDATE read that binds entity.id as a SQL parameter before dispatching to the restore engine. The registry-lookup and pass-through tests hand validate() a MagicMock whose id cannot bind, so they died on a ProgrammingError that the transaction wrapper then mapped to failed_exc, masking the LookupError / RuntimeError the tests exercise. Stub db.session.query so the lock read hands back the mocked entity, letting the tests reach the patched resolve_version/restore_version. The lock query's own SQL is covered end-to-end in test_restore_version_concurrency.py.
SUMMARY
BaseRestoreVersionCommand.run()wraps_perform()in@transaction(), but_do_restore()never locked the live row or otherwise coordinated with a concurrent writer. An overlapping edit (or a second restore) could commit between the command's read and its write, and the lost update was silent._do_restore()now takes aFOR UPDATElock on the live entity immediately aftervalidate()and before resolving/reverting, thendb.session.refresh(entity)under the lock so the revert — and the version-history delta it records — is computed against the current committed state rather than the pre-lock snapshot. The lock is held for the rest of the command's transaction (it runs on the same scoped session inside the@transaction()wrapper).The
FOR UPDATEhelper (lock_entity_for_update) is moved fromversioning/api_helpers.py(REST-endpoint handlers) down toversioning/queries.py(the persistence layer, alongsideresolve_version), so both the conditional-write PUT path and this command depend on it from a shared lower module rather than a command reaching up into the API layer; the command's import is now top-level.This is the pessimistic (serialize) half. The optimistic half — having the restore endpoint honor an
If-Matchprecondition so a client can detect (412) a concurrent edit rather than merely serialize, as the PUT already does viaraise_for_stale_write— is a separate, client-facing follow-up, noted in the code.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — backend concurrency fix, no UI.
TESTING INSTRUCTIONS
tests/unit_tests/commands/test_restore_version_concurrency.py— parametrized over the three concrete restore commands asserting_do_restoreacquires the lock with the entity's id, plus an ordering test asserting the lock is taken beforerestore_version. Each fails when the lock is removed.pytest tests/unit_tests/commands/test_restore_version_concurrency.py(4 passed).FOR UPDATEhelper itself is exercised by the conditional-write PUT path it is shared with.ADDITIONAL INFORMATION
🤖 Generated with Claude Code