fix(distributed-lock): ownership-checked release (compare-and-delete) - #43462
Conversation
The Redis/KV distributed lock stored a constant value on acquire and deleted the key unconditionally on release. If a holder A's TTL expired and holder B then acquired the same key, A's release deleted B's lock — a classic lost-lock bug on a primitive the Global Task Framework now leans on for task submit/cancel. Acquire now stores a per-acquisition token (uuid4) as the lock's value; the DistributedLock context manager threads that token to release, which only deletes the key when the stored value still matches (compare-and-delete). Both backends: Redis (get-then-compare-then-delete; residual non-atomic window noted, vs a full Lua CAS) and the KeyValue DB fallback (ownership-checked before delete). Release keeps a token=None mode meaning "delete unconditionally", preserving the existing behavior of the one cross-process caller (the Excel export: the API process acquires, the Celery task releases — they cannot share an in-memory token), which is unchanged. Tests: two new ownership regressions (Redis + KV) prove a superseded holder's release leaves the current holder's lock intact; existing happy-path/expiry/ fallback tests updated for the token-shaped value.
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/review |
Code Review Agent Run #3a3f71Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| # The acquisition token to match on release (see AcquireDistributedLock). | ||
| # None means "delete unconditionally" — only for callers that did not | ||
| # acquire via the token-aware path. | ||
| self.token = token |
There was a problem hiding this comment.
Suggestion: The default token=None path still performs an unconditional delete, and the dashboard API directly invokes this release after acquiring the lock without retaining or passing the acquisition token. If the enqueueing request's lock expires and another export acquires the same key before enqueue failure cleanup runs, this call deletes the newer holder's lock. Retain the token for same-process acquisition/cleanup paths; use an explicit separate mechanism for the cross-process Excel task handoff. [api mismatch]
Severity Level: Major ⚠️
- ❌ Failed export enqueue cleanup can remove a newer export lock.
- ⚠️ Concurrent exports can bypass dashboard/user throttling.
- ⚠️ Affected path is the dashboard Excel export API.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/distributed_lock/release.py
**Line:** 58:58
**Comment:**
*Api Mismatch: The default `token=None` path still performs an unconditional delete, and the dashboard API directly invokes this release after acquiring the lock without retaining or passing the acquisition token. If the enqueueing request's lock expires and another export acquires the same key before enqueue failure cleanup runs, this call deletes the newer holder's lock. Retain the token for same-process acquisition/cleanup paths; use an explicit separate mechanism for the cross-process Excel task handoff.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in 075b1c1e8c by threading the acquisition token through the whole Excel-export flow, so no caller on that path relies on the unconditional token=None delete anymore:
- Same-process cleanup: the API now retains
acquire = AcquireDistributedLock(...)and the enqueue-failure release passestoken=acquire.token, so it only deletes the lock it holds — not one a later export acquired if this one's TTL already expired. - Cross-process handoff: the token is passed into the Celery task via
apply_async(kwargs={..., "lock_token": acquire.token});export_dashboard_exceltakes alock_tokenparam and releases with it. Since the token is a plain string it serializes cleanly in the task payload and the ownership check works for both backends (Redis string value / KV{"token": ...}).
The token=None default stays as a documented escape hatch for callers that didn't acquire via the token-aware path, but the throttled Excel path no longer uses it. Extended the existing lock-release tests to assert the token is threaded through (token="lock-tok-1") and the enqueue test to assert apply_async carries lock_token.
|
The flagged issue is correct. The To resolve this, you should ensure that the dashboard API (and any other direct callers) passes the acquisition token to Proposed FixUpdate the dashboard API call to retrieve and pass the token, similar to how the context manager does it: # Example of how to update the direct invocation:
# 1. Acquire the lock and get the token
lock_command = AcquireDistributedLock(namespace, params)
lock_command.run()
token = lock_command.token
# 2. Pass the token to the release command
ReleaseDistributedLock(namespace, params, token=token).run()I have reviewed the PR changes and the implementation of the token-based release appears robust for the context manager. Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset/commands/distributed_lock/release.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gaq-to-gtf #43462 +/- ##
==============================================
- Coverage 78.86% 78.85% -0.01%
==============================================
Files 2883 2883
Lines 164782 164784 +2
Branches 38083 38087 +4
==============================================
- Hits 129949 129943 -6
- Misses 32388 32393 +5
- Partials 2445 2448 +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:
|
1 participant
SUMMARY
Addresses the P2 finding from the #43407 review (targets
gaq-to-gtf): the distributed lock's release is ownership-less.AcquireDistributedLockstored a constant value ("1"/{"value": True}) viaSET NX, andReleaseDistributedLockunconditionally deleted the key. So if holder A's TTL lapsed and holder B acquired the same key, A'sfinallyrelease would delete B's lock. It predates this epic, but the epic makes this primitive central to task submit/cancel (task_lock), so it's worth closing.Fix: acquire now stores a per-acquisition token (
uuid4().hex) as the lock's value; theDistributedLockcontext manager threads that token to release, which only deletes the key when the stored value still matches — compare-and-delete:get→ compare →delete(a get-then-delete; the residual non-atomic window is bounded by one round-trip and vastly smaller than an unconditional delete's exposure — a Lua CAS could close it fully; noted in a comment).Release keeps a
token=Nonemode meaning "delete unconditionally", which preserves the behavior of the one cross-process caller — the Excel export, where the API process acquires and the Celery task releases (they can't share an in-memory token). That path is unchanged.TESTING INSTRUCTIONS
pytest tests/unit_tests/distributed_lock/— two new regressions (Redis + KV) prove a superseded holder's release leaves the current holder's lock intact; existing happy-path / expiry / fallback tests updated for the token-shaped value (57 passing incl. export-excel which uses the unconditional path).ADDITIONAL INFORMATION