Skip to content

fix: redisWorker lock leaks across acquire, abort, cancel, and dispatch - #7945

Merged
dkliban merged 1 commit into
pulp:mainfrom
carlosthe19916:fix/7902-redis-lock-leaks
Aug 11, 2026
Merged

fix: redisWorker lock leaks across acquire, abort, cancel, and dispatch#7945
dkliban merged 1 commit into
pulp:mainfrom
carlosthe19916:fix/7902-redis-lock-leaks

Conversation

@carlosthe19916

Copy link
Copy Markdown
Contributor

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

  • Commits are cleanly separated with meaningful messages (simple features and bug fixes should be squashed to one commit)
  • A changelog entry or entries has been added for any significant changes
  • Follows the Pulp policy on AI Usage
  • (For new features) - User documentation and test coverage has been added

See: Pull Request Walkthrough

@carlosthe19916 carlosthe19916 changed the title Fix RedisWorker lock leaks across acquire, abort, cancel, and dispatch fix: redisWorker lock leaks across acquire, abort, cancel, and dispatch Aug 5, 2026
@carlosthe19916
carlosthe19916 force-pushed the fix/7902-redis-lock-leaks branch from 824731f to db061f5 Compare August 5, 2026 13:59

@dkliban dkliban left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_task exception handler releasing locks (line 551-555) — correct fix for a real leak path observed in the 2026-07-24 incident
  • _release_task_locks_any_owner correctly 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>
@carlosthe19916
carlosthe19916 force-pushed the fix/7902-redis-lock-leaks branch from db061f5 to babb5cc Compare August 11, 2026 08:08
Comment on lines +692 to +697
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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@carlosthe19916
carlosthe19916 requested a review from dkliban August 11, 2026 08:14

@dkliban dkliban left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All issues from the previous review have been addressed:

  1. handle_tasks safety net — correctly unchanged, stays if task and task.immediate
  2. supervise_task_maybe_release_locks stays in the correct position (before cancel state), with an except block added to release locks if the cancel path throws. The _all_locks_released flag prevents double-release in all cases.
  3. asyncio.sleep — fixed, no longer wastes a thread pool thread
  4. fetch_task exception 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.

@dkliban
dkliban merged commit bd09646 into pulp:main Aug 11, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RedisWorker leaks Redis locks in multiple code paths

2 participants