Repository navigation
fix(dataset): lock the If-Match validator's transaction-id read - #44091
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44091 +/- ##
=======================================
Coverage 80.40% 80.40%
=======================================
Files 2932 2932
Lines 174525 174571 +46
Branches 40548 40557 +9
=======================================
+ Hits 140319 140363 +44
- Misses 31537 31539 +2
Partials 2669 2669
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:
|
mikebridge
changed the title
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #df79b1Actionable Suggestions - 0Additional Suggestions - 4
Filtered 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 |
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
Adversarial review of the If-Match transaction-id locking change. The core idea — take the transaction-id read under an exclusive row lock so a stale token can't slip past REPEATABLE READ, and map genuine lock contention to a retryable 409 — is sound and well-documented. Five findings below; the first is a correctness gap in the 409-mapping goal, the rest are a doc/code contradiction, a misleading comment, a classifier gap, and a cleanup. (Automated review; posted by a human reviewer.)
|
Needs a rebase, then I can review when needed. |
…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
Three-lens findings on the FOR SHARE design, all addressed: - The lock is now EXCLUSIVE (with_for_update()): this transaction later closes the very row it reads (Continuum sets end_transaction_id at commit), so holding a shared lock first invited InnoDB's shared-to-exclusive upgrade deadlock whenever anything queued for the row in between (e.g. retention pruning). Plain MVCC readers are not blocked by either strength, and same-entity writers are already serialised by the entity lock, so exclusivity costs nothing. Renamed to current_live_transaction_id_locked accordingly. - The plain-row read is ordered-and-limited so a defensively tolerated multi-open-row state (which the aggregate's max absorbs) degrades the same way instead of raising MultipleResultsFound on every PUT. - MySQL's gap lock on the zero-live-row range leaves a rare cross-entity deadlock at Continuum's insert; a lock race lost there IS a concurrent interleave, so the PUT path maps lock-contention errors (is_lock_contention_error) to the same 412 retry semantics as a stale token instead of a 500. Residuals documented on the query helper. - Test holes closed: the endpoint wiring is pinned (the 412 unit test asserts lock_for_stale_check=True reaches the helper — previously deleting the kwarg at the call site survived every test), the guard's real input is pinned (derive_version_uuid must consume the LOCKED id), and the zero-live-row branch has an integration test. lock flag is keyword-only; is_conditional_write() hoisted; open-PR citations reworded intent-shaped; cleanup restores the captured original via session.get. sc-120050 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
Cross-model round (author-verified): the documented cross-entity gap-lock deadlock surfaces at Continuum's version-row INSERT inside the update command — where the @transaction error mapping already yields the pre-existing 422 — while the OperationalError handler at the call site covers only a deadlock at the locking read itself. The residual doc implied the handler covered the insert case, and its 412 told the loser to refetch a token that was never proven stale. Re-map the read-point handler to 409 with retry-the-same-request guidance, rewrite the residual paragraph to name both surfacing points and their true outcomes, align is_lock_contention_error's docstring, and pin the 409 mapping (status + message) so it cannot drift back to 412. sc-120050 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
…t setup) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
… handler Review round (fitzee): - The entity-row lock acquisition sat OUTSIDE the contention mapping: writer B blocking on writer A's row lock and timing out (1205) — the most common contention outcome of all three lock points — propagated as an uncaught 500 instead of the retryable 409 this PR's contract advertises. Now wrapped with the same classification. - The three lock points share _lock_contention_response(): one rollback + one i18n string instead of two hand-synced copies. Its docstring states the rollback is LOAD-BEARING for lock-wait-timeout (with innodb_rollback_on_timeout OFF a 1205 rolls back only the failing statement; the transaction still holds the entity row lock and the rollback is what releases it) — replacing the misleading "transaction is already dead" cleanup framing. - Stale comments fixed: queries.py's residual note and the read-point parenthetical claimed the insert-point deadlock surfaces as the pre-existing 422; the handler added in this same PR classifies it to the same 409. - is_lock_contention_error's text fallback now recognizes SQLite's "database (table) is locked" (no error code exists to match), so contention on the SQLite-backed integration suite classifies correctly; parametrized tests added. - put() keeps # noqa: C901 (15 > 10 even after the extraction). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
9aa4d73 to
0970543
Compare
Code Review Agent Run #a9b289Actionable 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 |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Reviewed 0970543ced89.
Following up on the existing error-classification concern: P2 — unrelated failures can become retryable 409s. db_errors.py:65–76 searches str(exc), including SQL and bound parameters. An isolated probe with a non-locking DataError and deadlock only in its parameters matched as contention. Could we inspect the underlying driver error instead and add negative tests for lock-related words in SQL/parameters? Retrying the same invalid request cannot fix this failure.
…ot the wrapper's rendered SQL (sc-120050) Richard Fogaça's review: is_lock_contention_error's fallback searched str(exc), and str(DBAPIError) appends the failing statement and its bound parameters — so an unrelated failure whose SQL or values merely contain a lock word (a chart named "deadlock analysis", a column called lock_wait_timeout) was answered 409 "Retry the same request", advice that can never succeed because nothing is contending. Reachable at the response boundary: the command wraps SQLAlchemy errors as DatasetUpdateFailedError and the endpoint classifies its __cause__. Classification now reads the DRIVER DIAGNOSTIC only. A structured code is authoritative: a MySQL errno or PostgreSQL SQLSTATE outside the contention sets ends classification at False instead of falling through to a keyword match. Only code-less drivers (SQLite) reach the phrase check, and then against their own message. A wrapper whose driver error was not preserved is unclassifiable, not retryable; a raw driver exception is still its own diagnostic. Tests: the fallback positives now put their message in the driver error rather than the statement argument; new negatives cover each phrase appearing only in SQL or parameters on an unrelated DataError and OperationalError, a non-contention code whose diagnostic mentions a deadlock (pinning code precedence), and a wrapper with no orig. At the response boundary, a contaminated non-lock DataError keeps its 422 while genuine contention stays 409. 11 of these fail without the fix. Also, the interleaved MySQL regression the lens pass found missing: two sessions, deterministic, no sleeps. Writer A commits a competing version between this request's read view opening and its validator read; the plain read is served from the pre-commit snapshot and still names v1 — so the client's stale If-Match token MATCHES and the update would clobber A — while the locking read derives v2's token and the same token is rejected. Zero-history-to-first-version covered too (the MySQL gap-lock branch). Both force REPEATABLE READ per-session, since CI pins the MySQL server to READ COMMITTED and the staleness cannot flip under it. Verified against a real MySQL 8: 4/4 pass, and with the locking clause removed both new tests fail while both pre-existing ones still pass — the verification gap, closed. Plus: the PUT contract documents the retryable 409 and contrasts it with 412's refetch guidance, and the new-code annotations the lens pass listed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
Code Review Agent Run #fcbd3eActionable 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 |
…ponses check-openapi-spec-drift went red: the new retryable-409 response and the reworded 412 were added to the docstrings but the published spec was never regenerated, so the two disagreed. Generated in an isolated clone with the CLI's resolved module path asserted to lie inside it first -- the entry point builds its output path from the imported module's __file__, so a bare invocation can write into whichever editable install answers the import. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4MGEkUcDpyLReqjQBhPy
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
No production defect found — the locking read behaves as documented on both MySQL and Postgres. Two inline notes, both test-coverage gaps on paths this PR adds.
| # refresh raises its own exception type and cannot reach this | ||
| # branch.) Same retryable classification as the read-point | ||
| # handler above: the token is not proven stale. | ||
| if conditional and is_lock_contention_error(ex.__cause__): |
There was a problem hiding this comment.
Only negative coverage here: the contaminated-DataError test asserts this does NOT fire. Replacing it with if False: leaves all 52 unit tests green. A positive twin — args=(1213, ...) as __cause__ — passes as written and fails once removed. Credits fitzee's 422-vs-409 thread.
There was a problem hiding this comment.
Added in c6767b2: a positive command failure whose cause is OperationalError with driver args (1213, ...), plus an unconditional control that must remain 422 without retry text. Replacing the conditional cause handler with if False in a process-local mutation made the positive fail (422 vs 409) while the unconditional control passed. Final candidate: 59 focused tests and 221 versioning unit tests pass, complete changed-file pre-commit passes, and independent final-snapshot review approves. This is unit verification; fresh live MySQL REPEATABLE READ integration remains a CI gate.
| already rolled the transaction back and this clears the aborted | ||
| session before responding. | ||
| """ | ||
| db.session.rollback() # pylint: disable=consider-using-transaction |
There was a problem hiding this comment.
db.session.rollback() → pass also survives all 52 tests. fitzee's thread warned a future cleanup trusting the comment would leak the entity lock on a 1205; the docstring now says LOAD-BEARING, but nothing pins it. Asserting it on the 1205 path closes that.
There was a problem hiding this comment.
Added in c6767b2: the 1205 test covers both direct lock points and asserts rollback exactly once before request teardown. Replacing the helper rollback with pass in-process made both cases fail with zero rollback calls. The docstring also distinguishes the required direct-catch rollback from the decorated command path where rollback already occurred. Final focused suite (59) and full versioning unit suite (221) pass; changed-file hooks and independent final-snapshot review pass. No claim of a fresh live-server lock-release test.
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Follow-up on the classifier's branch ORDER, after a sibling PR shipped the inverse idiom. db_errors.py is correct here — but the ordering that makes it correct is unpinned. One inline.
| _FakeLockDriverError(args=(1213, "Deadlock found when trying to get lock")), | ||
| _FakeLockDriverError(args=(1205, "Lock wait timeout exceeded")), |
There was a problem hiding this comment.
These fakes set args XOR sqlstate; real PyMySQL (dist 1.2.0) sets both — 1213 carries 40001, 1205 carries HY000. So nothing pins errno-before-sqlstate: swapping that order keeps all 52 green while real 1205 flips to False. 1213 survives the swap by coincidence, so the case has to be 1205.
| _FakeLockDriverError(args=(1213, "Deadlock found when trying to get lock")), | |
| _FakeLockDriverError(args=(1205, "Lock wait timeout exceeded")), | |
| _FakeLockDriverError( | |
| args=(1213, "Deadlock found when trying to get lock"), sqlstate="40001" | |
| ), | |
| _FakeLockDriverError( | |
| args=(1205, "Lock wait timeout exceeded"), sqlstate="HY000" | |
| ), |
There was a problem hiding this comment.
Added both combined driver shapes in c6767b2: 1213/40001 and 1205/HY000. A process-local SQLSTATE-first mutant fails specifically on 1205/HY000 (1 failed, 7 passed in the positive cases), so this pins the decisive errno-first order rather than relying on 1213 agreeing by coincidence. The production classification order is unchanged. Final 59 focused tests, 221 versioning unit tests, complete changed-file hooks and independent final-snapshot review pass; current-head CI is separate.
Code Review Agent Run #398987Actionable 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 |
|
CI refresh: merged Apache master containing the already-merged migration repair #44364 into this branch, published at 181b79e. No duplicate migration was added. The fresh PR merge ref includes repair commit 4c77b0b; its migration directory matches that repaired baseline. Local verification: 60 focused tests passed (including the single-migration-head regression), all applicable pre-commit hooks passed against the complete branch-changed file list, and the final source snapshot received independent Python review. The only additional changes correct test driver-exception fixtures and their annotations; production behavior is unchanged. Fresh CI has been triggered; this is not an all-green claim. Live database integration remains a remote-CI gate. The older migration failures tested pre-repair merge snapshots and are not evidence about this new head. |
|
Readiness follow-through at 181b79e: the fresh checks have completed with no failing or pending checks (64 success, 10 skipped, 2 neutral). The PostgreSQL job tested a084632, which includes merged migration repair #44364, and passed 2,792 tests. The MySQL job passed 2,780 tests and ran all four The positive chained-cause, rollback and combined errno/SQLSTATE regressions described in the three inline replies remain present. Author remediation is complete; those open threads still need reviewer confirmation. This is not a new approval or automatic thread resolution. |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Verified at 181b79e: the rollback, chained-__cause__ and errno-before-sqlstate mutants are all killed now — the order mutant fails on 1205/HY000 specifically. No new findings; the production delta is annotations and comment fixes only.
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here: re-reviewed at 181b79e. My earlier concern about the classifier matching on rendered SQL/parameters is resolved. It now reads only the driver diagnostic and treats a wrapper without orig as unclassifiable. I also confirmed the locking read is index-backed on each dialect and that dataset PUT is the only If-Match call site. No blocking findings.
Small suggestion, totally optional and fine as a follow-up: the dataset editor special-cases 412 but lets the new 409 fall through to the generic error dialog, so users see "Retry the same request." Could we give 409 a friendlier message along the lines of "another save is in progress, try saving again"? Happy to keep as-is if this is intentional.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Mike Bridge <michael.bridge@ext.preset.io> (cherry picked from commit fc71d0f)
SUMMARY
The dataset PUT's conditional-write (
If-Match) guard compared the client's token against a version-info lookup made with plain consistent reads. On MySQL/InnoDB default REPEATABLE READ, those reads are served from the snapshot pinned by the request's first read (the auth queries), 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_writepassed, and the second writer overwrote the first — exactly the lost update the lock +If-Matchpair exists to prevent, surviving on MySQL only (R1 and R2 both open with token v1; R1 locks and commits v2; R2 acquires the lock but its validator read still sees v1). This is the validator-layer sibling of #44027, which fixed the entity read one layer down. Postgres (READ COMMITTED, fresh snapshot per statement) and SQLite are unaffected.Fix:
current_live_transaction_id_locked(versioning/queries.py) — the live version row'stransaction_id(the token input) read under an exclusive row lock (with_for_update()): a locking read is exempt from the REPEATABLE READ snapshot and returns committed data. Exclusive rather thanFOR SHAREbecause this transaction later closes the very row it reads (Continuum setsend_transaction_idat commit), and a shared lock first invites InnoDB's shared→exclusive upgrade deadlock; plain MVCC readers are blocked by neither strength. Deliberately a plain row query, not the existing aggregate — locking clauses and aggregates don't combine (Postgres rejects the combination) — and ordered-and-limited so a defensively tolerated multi-open-row state degrades like the aggregate'smaxinstead of raisingMultipleResultsFound.If-Matchtoken is never proven stale, so 412's refetch-the-token guidance would be wrong): the two directOperationalErrorhandlers (entity-row acquisition and validator read), and the insert-point (Continuum's version-row INSERT inside the command) classified narrowly in theDatasetUpdateFailedErrorbranch viaex.__cause__— the post-commitoverride_columnsrefresh raises its own exception type and structurally cannot reach that branch.is_lock_contention_errorlives inversioning/db_errors.pybesideis_missing_table_error, hardened (guarded empty driver args,pgcode or sqlstate, PG55P03, acceptsNone) with a parameterized test set. The brief retention-prune blocking residual is documented on the query helper.current_entity_version_infogrowslock_for_stale_check; when set, the locked transaction id replaces the aggregate's. The displayed version number stays a plain aggregate read: a concurrent commit can leave response metadata one behind, but the guard never consults it (documented in the docstring).lock_for_stale_check=is_conditional_write()— only conditional saves pay for the lock; every GET path and unconditional PUT keeps lock-free reads (pinned by test).Integration note: #44027 merged on September 14 and is included in this branch. Its entity-read refresh complements this PR’s validator-read fix. The
_locked_entity = lock_entity_for_update(...)strong reference is preserved because the identity map is weak. Residual lazy child-collection staleness remains separate.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — concurrency fix. Before (MySQL only): a stale
If-Matchtoken could pass the 412 guard after a concurrent commit. After: the guard's input reflects committed state.TESTING INSTRUCTIONS
pytest tests/unit_tests/versioning/test_db_errors.py— parameterized classifier coverage: MySQL 1213/1205, PGpgcode/sqlstate40001/40P01/55P03, text fallback, unrelated codes, empty driver args, andNone(no IndexError-to-500).tests/unit_tests/datasets/api_tests.py— the 409 mapping pinned (status + message, so it cannot drift back to 412), the 412 stale-token path unchanged, the endpoint wiring pinned, and an unconditional-PUT test pinning that no lock and no locking read happen withoutIf-Match(the perf contract both ways — hard-coding the flag fails it).pytest tests/unit_tests/versioning/test_version_info_locking.py— 3 dialect-independent pins: the conditional-path read chains an exclusivewith_for_update()withorder_by desc + limit 1(weakening or dropping the clause fails on every backend),current_entity_version_infothreads the flag AND derives the token from the locked id (derive_version_uuidpinned — the guard consumesversion_uuid, nottransaction_id), and the default path never invokes the locking read (GETs stay lock-free). The endpoint wiring itself is pinned intests/unit_tests/datasets/api_tests.py::test_put_dataset_rejects_stale_if_match(lock_for_stale_check=Truemust reach the helper).pytest tests/integration_tests/versioning/conditional_token_lock_tests.py— real-backend proof: the locking read executes on the actual dialect, agrees with the plain read on the quiet path, and the zero-live-row branch (lazy baselines — the gap-lock branch) resolves toNone/unversioned-token exactly like the plain path. The MySQL RR staleness itself cannot flip on Postgres and needs an interleaved read view on MySQL, so — per the honest-scope precedent of fix(versioning): row-lock the live entity during version restore #44015/fix(dataset): refresh the entity under the conditional-write row lock #44027 — the statement-shape pin is the load-bearing regression guard.If-Match; before the fix the second could pass the guard and clobber the first; after, it gets 412.ADDITIONAL INFORMATION
🤖 Generated with Claude Code
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW