Skip to content

fix: clean up stale and orphaned Redis locks - #7951

Draft
carlosthe19916 wants to merge 1 commit into
pulp:mainfrom
carlosthe19916:fix/7919-7920-orphan-redis-locks
Draft

fix: clean up stale and orphaned Redis locks#7951
carlosthe19916 wants to merge 1 commit into
pulp:mainfrom
carlosthe19916:fix/7919-7920-orphan-redis-locks

Conversation

@carlosthe19916

@carlosthe19916 carlosthe19916 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Problem

When a K8s pod restarts, the new worker gets the same name but Redis still holds locks from the dead predecessor. These orphan locks block resources indefinitely because the periodic cleanup either sees the old AppStatus as still online or it was already deleted without releasing its locks.

Solution

  • Introduce a per-owner lock registry (pulp:owner_locks:{owner} Redis SET), maintained atomically inside the acquire/release Lua scripts, so an owner's locks can be cleaned in O(locks held) without scanning the whole keyspace.
  • Add release_stale_locks_for_self() at worker startup to release Redis locks left under this worker's name by a dead same-name predecessor. (Stale AppStatus rows are cleaned separately by the periodic missing-worker pass — startup only handles the locks.)
  • Add reconcile_orphan_redis_locks() to the periodic cleanup to release locks whose owner has no AppStatus row at all.
  • Make cleanup_redis_locks_for_worker() sweep any remaining locks via the registry, with per-task exception isolation so one Redis failure doesn't abort cleanup for the worker's other tasks. A throttled keyspace SCAN is kept only as a rolling-upgrade fallback for locks acquired before the registry existed.

Fixes #7919, #7920.

Test plan

  • oci-env test -p pulpcore unit -k "orphan_redis_locks"
  • oci-env test -p pulpcore functional -k "test_tasking"

@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 design is solid — startup cleanup, successor detection, retry-safe cleanup, WAITING task preservation, and the test coverage are all well done. The main concern is the SCAN-based lock discovery which creates scaling issues.

SCAN cost at scale

cleanup_locks_for_owner() does a full SCAN of all task:* keys and all pulp:resource_lock:* keys, checking each key's value against the owner. collect_lock_owners() does the same to enumerate all owners. These are O(all_locks_in_redis) operations.

Three code paths trigger these scans:

  1. release_stale_locks_for_self() — runs at every worker startup unconditionally (even with no prior incarnation). At scale-up from 10 to 150 workers, that's 150 concurrent full SCANs.

  2. reconcile_orphan_redis_locks() — runs every ~1000 seconds. Does collect_lock_owners() (full SCAN) plus cleanup_locks_for_owner() for each orphan (another full SCAN each).

  3. cleanup_redis_locks_for_worker() — runs per missing worker during periodic cleanup. One full SCAN per missing worker.

With 200+ locks in production (observed on 2026-07-24), each SCAN touches every key. During the incident with 150 workers crash-looping, this would compound the Redis pressure.

Recommendation: per-owner lock registry

Instead of scanning all keys to find locks for a specific owner, maintain a per-owner set in Redis:

Key: pulp:owner_locks:{worker_name}
Type: Redis SET
Members: all lock keys held by this owner (task locks + resource locks)

Add SADD pulp:owner_locks:{owner} <key> to the acquire_locks Lua script and SREM pulp:owner_locks:{owner} <key> to the release_resource_locks Lua script. This is atomic with the lock operations — no sync issues.

Then cleanup_locks_for_owner() becomes:

keys = redis_conn.smembers(f"pulp:owner_locks:{owner}")
for key in keys:
    # delete-if-owner-matches (Lua for atomicity)
redis_conn.delete(f"pulp:owner_locks:{owner}")

This is O(locks_held_by_owner) instead of O(all_locks_in_redis). For a worker that held 5 locks, it reads 5 keys instead of scanning 200+.

For backward compatibility during rolling upgrades: old workers don't write to the registry, so their locks won't appear in it. Keep the SCAN as a fallback only when the registry set doesn't exist for an owner. After a full rollout, all owners will have registries and SCAN is never used.

Per-task exception handling in cleanup_redis_locks_for_worker

The DB-linked cleanup loop (the for task in tasks block) has a single try/except around the entire loop. After #7945 merged, release_resource_locks raises RedisError. A single Redis failure on one task aborts cleanup for all remaining tasks of that worker.

Recommendation: catch per-task exceptions inside the for loop:

for task in tasks:
    try:
        safe_release_task_locks(task, lock_owner=self.name)
        self._fail_incomplete_task(task, worker_name, "Worker has gone missing.")
    except Exception as e:
        _logger.error("Error cleaning up task %s for worker %s: %s", task.pk, worker_name, e)

Non-atomic delete in cleanup_locks_for_owner

The function uses GET key then DEL key as separate commands. Between them, another worker could acquire the same lock key. Use a Lua script for atomic delete-if-owner-matches:

if redis.call("get", KEYS[1]) == ARGV[1] then
    return redis.call("del", KEYS[1])
end
return 0

What's good

  • Startup cleanup before accepting work — correct placement
  • Successor detection avoids releasing locks belonging to new incarnation
  • Retry-safe design with bool return from cleanup
  • WAITING tasks left reclaimable
  • Comprehensive test coverage (12 unit tests)

@carlosthe19916
carlosthe19916 force-pushed the fix/7919-7920-orphan-redis-locks branch from 43d0ecc to 374bbcf Compare August 11, 2026 16:34
@carlosthe19916
carlosthe19916 force-pushed the fix/7919-7920-orphan-redis-locks branch 2 times, most recently from ad7bb06 to 8c1ec9f Compare August 12, 2026 10:40
@carlosthe19916
carlosthe19916 marked this pull request as draft August 12, 2026 10:40
  RedisWorker could leave distributed locks stranded in Redis, causing tasks
  to stay WAITING and resources to stay blocked until a manual
  release-task-locks:

  - pulp#7919: a same-name pod restart reused a worker name while Redis still
    held locks from the dead incarnation, so the new worker was blocked by
    its own name's stale locks.
  - pulp#7920: periodic cleanup only handled owners found via
    AppStatus.objects.missing(); locks whose AppStatus row was already
    deleted (graceful shutdown, prior cleanup, kill -9) persisted with no
    DB record pointing at them.

  Introduce a per-owner lock registry (Redis SET pulp:owner_locks:{owner})
  maintained atomically inside the acquire/release Lua scripts, making
  cleanup O(locks-held-by-owner) instead of scanning the whole keyspace. A
  throttled legacy SCAN fallback (pulp:last_legacy_owner_scan) covers locks
  predating the registry during rolling upgrades.

  Add release_stale_locks_for_self() at worker startup (with successor
  detection and a brand-new-worker fast path) and
  reconcile_orphan_redis_locks() to the periodic cleanup (releasing locks
  only for owners with zero AppStatus row, never for stale heartbeats).
  Refactor cleanup_redis_locks_for_worker() to release locks without
  failing WAITING tasks, isolate per-task failures, and retain the
  AppStatus row for retry on failure. Immediate-task locks are given a
  grace period while their task is still incomplete.

  Add unit tests covering the S1-S12 scenario matrix.

Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
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 should clean up stale Redis locks from previous incarnation at startup

2 participants