fix: redisWorker lock leaks across acquire, abort, cancel, and dispatch - #7945
Conversation
824731f to
db061f5
Compare
dkliban
left a comment
There was a problem hiding this comment.
Review
The PR addresses real lock leak paths — the fetch_task exception handler, _release_task_locks_any_owner for cancel, and retry wrappers for dispatch are all good fixes. A few issues need addressing:
1. handle_tasks safety net should stay if task and task.immediate
The change from if task and task.immediate to if task double-releases locks for deferred tasks. Deferred tasks run in a subprocess (perform_task) which releases its own locks. The subprocess has its own copy of the Task instance, so _all_locks_released on the parent's Task object is still False. This means:
- Every successful deferred task generates spurious "lock not owned" warning logs
- If the subprocess hasn't released locks yet (e.g. shutdown signal hits during execution), the parent releases them prematurely while the subprocess is still running
Recommendation: revert to if task and task.immediate.
2. supervise_task — don't move _maybe_release_locks to finally
The finally block runs on every cancel path, including when task.state in TASK_FINAL_STATES (subprocess already handled everything and released its own locks). This is another double-release scenario.
Also, the original code released locks BEFORE set_canceling(). The PR moves it to AFTER the cancel flow, changing the ordering semantics.
Recommendation: keep _maybe_release_locks where it is (inside the if task.state not in TASK_FINAL_STATES block). Add a separate except block to handle the case where refresh_from_db() or set_canceling() raises — release locks there.
3. Use asyncio.sleep in _aretry_safe_release_task_locks
# Current:
await sync_to_async(time.sleep)(0.1 * (attempt + 1))
# Should be:
import asyncio
await asyncio.sleep(0.1 * (attempt + 1))sync_to_async(time.sleep) wastes a thread pool thread for a simple delay.
4. release_resource_locks re-raise is fine
Given that PR #7951 adds startup lock cleanup (release_stale_locks_for_self), a raised RedisError from lock release is safe — if the worker crashes, the restart cleans up orphaned locks. The two PRs together make the raise safe.
What's good
fetch_taskexception handler releasing locks (line 551-555) — correct fix for a real leak path observed in the 2026-07-24 incident_release_task_locks_any_ownercorrectly reads lock owner from Redis before releasing — necessary for cancel_task where the canceler isn't the lock holder- Retry wrappers for dispatch with backoff
- Changelog entry
- fetch_task: release locks in exception handler after acquire_locks succeeds
- supervise_task: wrap cancel path in try/except so refresh_from_db or
set_canceling failures still release locks
- cancel_task: read lock owner from Redis to release locks for WAITING
tasks (handles race between acquire and cancel)
- dispatch/adispatch: retry lock release with backoff on transient Redis errors
- release_resource_locks: re-raise RedisError so callers can detect and retry
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
db061f5 to
babb5cc
Compare
| except Exception: | ||
| _logger.exception("Error in cancel path for task %s", task.pk) | ||
| try: | ||
| self._maybe_release_locks(task) | ||
| except Exception: | ||
| _logger.exception("Failed to release locks for task %s", task.pk) |
There was a problem hiding this comment.
@dkliban the _maybe_release_locks is now within if task.state not in TASK_FINAL_STATES: as well as within the Exception section.
I added an extra try/except block here just in case the "release lock" generates an exception, this way the exception is not propagated
dkliban
left a comment
There was a problem hiding this comment.
All issues from the previous review have been addressed:
handle_taskssafety net — correctly unchanged, staysif task and task.immediatesupervise_task—_maybe_release_locksstays in the correct position (before cancel state), with anexceptblock added to release locks if the cancel path throws. The_all_locks_releasedflag prevents double-release in all cases.asyncio.sleep— fixed, no longer wastes a thread pool threadfetch_taskexception handler,_release_task_locks_any_owner, retry wrappers — all correct
The release_resource_locks re-raise is safe given PR #7951 adds startup lock cleanup as a safety net.
LGTM.
closes #7902
Summary
Redis task/resource locks have no TTL. Several RedisWorker/dispatch paths could acquire them and never release, permanently blocking work.
Solution
Release locks on every leak path: fetch_task failures after acquire, supervise_task abort (finally), all claimed tasks in handle_tasks, WAITING cancel_task (owner from the task lock key), and retried release on transient Redis errors in immediate dispatch/adispatch.
📜 Checklist
See: Pull Request Walkthrough