fix: clean up stale and orphaned Redis locks

[carlosthe19916]

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]

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]

I needed to refactor some parts of the code

1. SCAN cost → per-owner registry. Added pulp:owner_locks:{owner} SET, maintained in the acquire/release Lua scripts. cleanup_locks_for_owner() now uses SMEMBERS +
one Lua call (O(locks held), no SCAN). Legacy SCAN kept only as a rolling-upgrade fallback, throttled fleet-wide.

Also: the registry alone didn't fix enumeration — collect_lock_owners() still did scan_iter(match="pulp:owner_locks:*"), and SCAN+MATCH still walks the whole keyspace
(100 keys → 1 round-trip, 50k → 100). Added a global pulp:active_owners SET and switched collect_lock_owners() to SMEMBERS (O(#owners), scan-free). None of the three
paths (startup, reconcile, missing-worker) SCAN the keyspace now.

2. Per-task exception handling. Each task in the cleanup_redis_locks_for_worker loop is now wrapped in try/except; one Redis failure no longer aborts the rest, and the
AppStatus is retained for retry.

3. Non-atomic delete. Replaced GET-then-DEL with delete-if-owner Lua (registry cleanup is one script; legacy fallback uses atomic per-key scripts).

Tests. Added coverage for all three paths plus keys-touched tests asserting each path's Redis cost is independent of keyspace size (100 vs 50k keys, zero SCANs), with a
legacy-SCAN control.

[dkliban]

All concerns from the previous review have been addressed:

1. Per-owner lock registry — acquire/release Lua scripts now maintain pulp:owner_locks:{owner} SETs and a global pulp:active_owners SET. Cleanup is O(locks_per_owner) via SMEMBERS instead of O(all_locks) via SCAN. Owner enumeration uses SMEMBERS on active_owners — no keyspace scan.

2. Per-task exception handling — cleanup_redis_locks_for_worker catches per-task exceptions inside the for loop and continues cleaning other tasks. Returns success = False if any step failed.

3. Atomic delete-if-owner — REDIS_DELETE_STRING_IF_OWNER_SCRIPT and REDIS_SREM_OWNER_SCRIPT Lua scripts for the legacy SCAN fallback.

Additional improvements:

• Legacy SCAN throttled fleet-wide via LEGACY_OWNER_SCAN_KEY (15 min interval)
• Brand-new workers skip cleanup entirely (no registry, no prior AppStatus → no SCAN at mass scale-up)
• WAITING tasks left reclaimable (app_lock detached, not failed)
• Immediate-task owner grace (checks if task is still running before cleanup)
• 20 comprehensive unit tests

LGTM.

[dkliban]

Follow-up from adversarial architect review

1. Remove active_owners cleanup from release Lua script

The release script removes the owner from active_owners when SCARD == 0:

if redis.call("scard", owner_registry_key) == 0 then
    redis.call("srem", "pulp:active_owners", lock_owner)
end

This creates a race: between the release script (removes from active_owners) and the next acquire_locks script (adds back), the owner is briefly invisible. If reconcile_orphan_redis_locks runs in that gap, it won't see the owner.

Recommendation: Remove these lines from the release Lua script. Only remove from active_owners during cleanup of dead owners (the CLEANUP_OWNER_LOCKS_SCRIPT already does this). The trade-off is that active_owners may contain live workers that currently hold zero locks, but that's harmless — reconciliation checks for an AppStatus record and skips live workers.

2. Owner identity collision across incarnations

When worker-1 crashes and another worker starts cleanup, the cleanup's final registry sweep runs CLEANUP_OWNER_LOCKS_SCRIPT. If new worker-1 starts and acquires fresh locks between the successor check (Python) and the Lua script execution, the cleanup reads the new registry (same key pulp:owner_locks:worker-1), sees GET key == "worker-1" (matches), and deletes the new worker's live locks.

The successor_online check in cleanup_redis_locks_for_worker guards against this, but it's a Python check followed by a separate Lua execution — there's a TOCTOU window.

Recommendation: Consider adding a generation token (e.g., the AppStatus PK or a UUID) to the registry key: pulp:owner_locks:{worker_name}:{generation}. The cleanup script operates on the dead incarnation's registry key, which is distinct from the new incarnation's key. This eliminates the race entirely.

3. Cleanup script silently skips unknown key types

CLEANUP_OWNER_LOCKS_SCRIPT handles string and set types but silently skips anything else, then unconditionally deletes the registry. A lock with an unexpected type becomes permanently orphaned.

Recommendation: Log or count skipped keys, and consider not deleting the registry if any keys were skipped — or remove only successfully-cleaned keys from the registry individually rather than deleting it wholesale.

[dkliban]

Follow-up: the owner identity collision race (point 2 in my previous comment) has a very narrow TOCTOU window — the successor must start, create AppStatus, AND acquire locks in the milliseconds between the Python successor check and the Lua cleanup execution. The existing successor_online guard is sufficient for now. Not worth the complexity of a generation token.

Points 1 (remove active_owners from release script) and 3 (handle unknown key types) still stand.