[BugFix] Locker: rollback partial intensive-lock acquisition#72423
Merged
alvin-phoenix-ai merged 1 commit intoMay 2, 2026
Merged
Conversation
lockTableWithIntensiveDbLock and lockTablesWithIntensiveDbLock acquire
a DB intention lock first and then one or more table locks. If any of
the table-lock acquisitions threw (deadlock victim selection, interrupt,
timeout, etc.), the helpers caught LockException and rethrew it as
ErrorReportException without releasing the DB intention lock that was
already held -- and, in the multi-table variant, without releasing any
table locks acquired earlier in the loop. The result was a stranded
IS / IX lock on the database that blocked subsequent DB-WRITE
operations (DROP DATABASE / DROP TABLE / quota change / rename)
indefinitely until the FE process restarted.
Callers could not work around this from the call site:
- Lock-acquisition outside try/finally: catch is never reached, the
finally never runs, so the partial DB lock leaks.
- Lock-acquisition inside try/finally: the finally calls the matching
unLock... helper, which first releases the DB intention lock (good)
and then iterates the table-id list calling release on each -- but
table locks past the failure point were never acquired, so their
release throws and masks the original LockException with a less
informative "lock not held" error. The DB lock is released in this
case, but the diagnostics are wrong.
Fix the helpers themselves. On any LockException during acquisition,
call a new private rollbackPartialIntensiveLock that releases each
table lock that did get acquired (in acquisition order, like the
existing tryLockTablesWithIntensiveDbLock pattern at lines 338-348),
then releases the DB intention lock if it was acquired. Each release
is wrapped in a defensive try/catch that logs and swallows any
secondary error, so the original LockException surfaces to the caller
instead of being masked by a release-side failure.
After this change, the lock-acquisition-outside-try/finally idiom at
the call sites is correct: a partial-acquire failure leaves no held
locks behind, so there is nothing for a finally block to release.
Signed-off-by: Kevin Cai <kevin.cai@celerdata.com>
|
@codex review |
Contributor
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Contributor
[FE Incremental Coverage Report]✅ pass : 24 / 28 (85.71%) file detail
|
Contributor
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
alvin-phoenix-ai
approved these changes
May 1, 2026
Contributor
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
24 tasks
Contributor
|
@Mergifyio backport branch-3.4 |
Contributor
|
@Mergifyio backport branch-4.0 |
Contributor
|
@Mergifyio backport branch-3.5 |
Contributor
|
@Mergifyio backport branch-4.1 |
Contributor
✅ Backports have been createdDetails
Cherry-pick of f32f70b has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Contributor
✅ Backports have been createdDetails
|
Contributor
✅ Backports have been createdDetails
|
Contributor
✅ Backports have been createdDetails
|
24 tasks
This was referenced May 2, 2026
Contributor
Author
|
give up backport to branch-3.4 |
robd003
pushed a commit
to robd003/starrocks
that referenced
this pull request
May 6, 2026
…ks#72423) Signed-off-by: Kevin Cai <kevin.cai@celerdata.com>
24 tasks
xiangguangyxg
pushed a commit
to xiangguangyxg/starrocks
that referenced
this pull request
May 7, 2026
…ks#72423) Signed-off-by: Kevin Cai <kevin.cai@celerdata.com> Signed-off-by: xiangguangyxg <xiangguangyxg@gmail.com>
meegoo
pushed a commit
to meegoo/starrocks
that referenced
this pull request
May 29, 2026
…POST-1561) Companion to the Locker-level rollback that already landed upstream (StarRocks#72423, StarRocks#72789). Those PRs ensure that `Locker.lock(Table|Tables)WithIntensiveDbLock` releases anything it partially acquired before re-throwing. This commit closes the remaining symptom at the PlannerMetaLocker layer. Pre-fix behavior (the SmartNews 4.0.9 incident): caller: try { plannerMetaLocker.lock(); ... } finally { unlock(); } 1. lock() iterates (db, tables) entries calling inner Locker.lockTablesWithIntensiveDbLock on each. 2. The inner call throws on some entry (e.g. ERR_NO_FILES_FOUND in the FILES() planning path forcing the planner to bail, or a real LockException). Inner Locker rolls itself back — good. 3. PlannerMetaLocker.lock() does NOT roll back any earlier (db, tables) entries that already succeeded; it just lets the exception propagate. 4. The caller's finally calls unlock(), which blindly iterates the full `tables` map and releases entries that were never acquired. 5. LockManager.release sees a Locker with no matching LockHolder and throws IllegalMonitorStateException: "Attempt to unlock lock, not locked by current locker" — surfaced to the client as ERROR 5600, masquerading as the original ERR_NO_FILES_FOUND. Fix: * `PlannerMetaLocker` now records `heldEntries` on successful lock()/tryLock(). unlock() only releases entries in `heldEntries`, and is a no-op when null. * lock() wraps its multi-db loop in try/catch: on failure it releases the earlier (db, tables) entries it had successfully acquired (in reverse order, best-effort with logging), clears `heldEntries`, and re-throws. Inner-layer rollback (already in upstream Locker) handles the failing entry itself; this outer rollback only addresses entries that fully succeeded before the failing one. * AutoCloseable.close() → unlock() is now safe to call when lock() was never invoked or threw. Tests: * `PlannerMetaLockerRollbackTest` (new): unlock-without-lock, close-without-lock, normal lock/unlock round-trip, single-db rollback (inner fails), multi-db rollback (first db succeeds, second throws — asserts the first db's release happened). * `LockerIntensiveLockRollbackTest` (new): regression test pinning the contract that the Locker-layer fix from StarRocks#72423/StarRocks#72789 provides — six cases (mid-loop failure, first-rid failure, db-intent failure, single-table variant, WRITE mode, success path), each verified by trying to acquire an exclusive WRITE on the rid afterward from a fresh Locker (any leaked IS/READ would time out the attempt). Related: POST-1561.
23 tasks
meegoo
pushed a commit
to meegoo/starrocks
that referenced
this pull request
Jun 1, 2026
…POST-1561) Companion to the Locker-level rollback that already landed upstream (StarRocks#72423, StarRocks#72789). Those PRs ensure that `Locker.lock(Table|Tables)WithIntensiveDbLock` releases anything it partially acquired before re-throwing. This commit closes the remaining symptom at the PlannerMetaLocker layer. Pre-fix behavior (the SmartNews 4.0.9 incident): caller: try { plannerMetaLocker.lock(); ... } finally { unlock(); } 1. lock() iterates (db, tables) entries calling inner Locker.lockTablesWithIntensiveDbLock on each. 2. The inner call throws on some entry (e.g. ERR_NO_FILES_FOUND in the FILES() planning path forcing the planner to bail, or a real LockException). Inner Locker rolls itself back — good. 3. PlannerMetaLocker.lock() does NOT roll back any earlier (db, tables) entries that already succeeded; it just lets the exception propagate. 4. The caller's finally calls unlock(), which blindly iterates the full `tables` map and releases entries that were never acquired. 5. LockManager.release sees a Locker with no matching LockHolder and throws IllegalMonitorStateException: "Attempt to unlock lock, not locked by current locker" — surfaced to the client as ERROR 5600, masquerading as the original ERR_NO_FILES_FOUND. Fix: * `PlannerMetaLocker` now records `heldEntries` on successful lock()/tryLock(). unlock() only releases entries in `heldEntries`, and is a no-op when null. * lock() wraps its multi-db loop in try/catch: on failure it releases the earlier (db, tables) entries it had successfully acquired (in reverse order, best-effort with logging), clears `heldEntries`, and re-throws. Inner-layer rollback (already in upstream Locker) handles the failing entry itself; this outer rollback only addresses entries that fully succeeded before the failing one. * AutoCloseable.close() → unlock() is now safe to call when lock() was never invoked or threw. Tests: * `PlannerMetaLockerRollbackTest` (new): unlock-without-lock, close-without-lock, normal lock/unlock round-trip, single-db rollback (inner fails), multi-db rollback (first db succeeds, second throws — asserts the first db's release happened). * `LockerIntensiveLockRollbackTest` (new): regression test pinning the contract that the Locker-layer fix from StarRocks#72423/StarRocks#72789 provides — six cases (mid-loop failure, first-rid failure, db-intent failure, single-table variant, WRITE mode, success path), each verified by trying to acquire an exclusive WRITE on the rid afterward from a fresh Locker (any leaked IS/READ would time out the attempt). Related: POST-1561.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
lockTableWithIntensiveDbLock and lockTablesWithIntensiveDbLock acquire a DB intention lock first and then one or more table locks. If any of the table-lock acquisitions threw (deadlock victim selection, interrupt, timeout, etc.), the helpers caught LockException and rethrew it as ErrorReportException without releasing the DB intention lock that was already held -- and, in the multi-table variant, without releasing any table locks acquired earlier in the loop. The result was a stranded IS / IX lock on the database that blocked subsequent DB-WRITE operations (DROP DATABASE / DROP TABLE / quota change / rename) indefinitely until the FE process restarted.
Callers could not work around this from the call site:
Fix the helpers themselves. On any LockException during acquisition, call a new private rollbackPartialIntensiveLock that releases each table lock that did get acquired (in acquisition order, like the existing tryLockTablesWithIntensiveDbLock pattern at lines 338-348), then releases the DB intention lock if it was acquired. Each release is wrapped in a defensive try/catch that logs and swallows any secondary error, so the original LockException surfaces to the caller instead of being masked by a release-side failure.
After this change, the lock-acquisition-outside-try/finally idiom at the call sites is correct: a partial-acquire failure leaves no held locks behind, so there is nothing for a finally block to release.
Why I'm doing:
What I'm doing:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: