[PR #21798] fix: harden distributed lock lifecycle with atomic Lua scripts and resilient retry #49332

New Issue

GiteaMirror · 2026-04-30T01:38:24-05:00

GiteaMirror commented

2026-04-30 01:38:24 -05:00

📋 Pull Request Information

Original PR: https://github.com/open-webui/open-webui/pull/21798
Author: @Classic298
Created: 2/23/2026
Status: 🔄 Open

Base: dev ← Head: redis

📝 Commits (3)

2ca7c3b fix: harden distributed lock lifecycle with atomic Lua scripts and resilient retry
daf107a fix: prevent transient errors from permanently killing cleanup loops
621289d fix: wrap lock acquire/renew in try/except for Redis fault tolerance

📊 Changes

2 files changed (+100 additions, -60 deletions)

View changed files

📝 backend/open_webui/socket/main.py (+72 -55)
📝 backend/open_webui/socket/utils.py (+28 -5)

📄 Description

Problem

The periodic cleanup loops had two categories of bugs in multi-worker Redis deployments:

Race conditions in RedisLock: renew_lock used SET with XX flag, which checks key existence but not ownership. If worker A's lock expired and worker B acquired it, worker A's renewal would silently overwrite B's lock value. Similarly, release_lock performed a non-atomic GET then DELETE, allowing the same ownership-bypass race between calls.
Fragile lifecycle management: periodic_session_pool_cleanup gave up permanently on the first failed acquire attempt. periodic_usage_pool_cleanup retried only twice before exiting forever. In both cases, if lock renewal failed mid-loop the cleanup task would die permanently, leaving stale entries to accumulate indefinitely.

Fix

RedisLock.renew_lock and RedisLock.release_lock now use Lua scripts executed via EVAL to atomically verify lock ownership before modifying the key. This eliminates the TOCTOU race window entirely:

renew_lock: GET + SET in one atomic Lua call, returns nil if not owner
release_lock: GET + DEL in one atomic Lua call, no-ops if not owner

The duplicated lock lifecycle logic is consolidated into a single run_with_lock helper that:

Retries acquisition indefinitely with jittered backoff
Releases and re-acquires automatically when renewal fails
Guarantees release via finally block, even on exceptions

Both cleanup functions now pass their business logic as a callback to run_with_lock, eliminating the ad-hoc retry loops and inconsistent error handling between the two.

Also removes the unused send_usage variable from periodic_usage_pool_cleanup.

Testing Done

All changes were validated with 26 tests across 5 test classes using pytest. Tests used mock-based unit tests for API contract verification, asyncio.run() for async logic, and fakeredis with Lua support for end-to-end integration testing of the actual Lua scripts. No live Redis instance was required.

TestRunWithLock (5 tests) — distributed lock lifecycle helper:

test_acquires_lock_then_runs_work Verifies the happy path: acquire is called once, renew is called before each work invocation, work runs the expected number of times, and release is called exactly once when the loop is cancelled.
test_retries_acquisition_on_failure Simulates two failed acquire attempts followed by a success. Confirms the helper retries with jittered backoff and does not give up. Verifies acquire is called 3 times total, work runs once after the successful acquire, and release is called.
test_reacquires_on_renewal_failure Simulates renewal failing on the first cycle. Confirms the helper releases the lock, re-acquires it, and continues working. Verifies acquire is called twice, renew twice, release twice (once per cycle), and work runs only in the successful renewal cycle.
test_release_called_even_if_work_raises Verifies the finally-block guarantee: when work throws a RuntimeError, release is still called exactly once before the exception propagates.
test_noop_lock_functions_work Verifies compatibility with non-Redis mode where acquire, renew, and release are all lambda: True. Work runs the expected number of times without errors.

TestReapStaleSessions (5 tests) — session reaping business logic:

test_reaps_expired_sessions A session 200 seconds old with a 120-second timeout is removed from the pool. Returns the correct (sid, user_id) tuple.
test_keeps_fresh_sessions A session only 10 seconds old with a 120-second timeout is preserved. Returns an empty list.
test_mixed_stale_and_fresh Pool with three sessions: one very stale (300s), one fresh (10s), one borderline (121s). Only the stale and borderline sessions are reaped; the fresh session survives.
test_empty_pool Empty pool returns an empty list and does not error.
test_missing_last_seen_at_defaults_to_zero A session entry without the last_seen_at field defaults to timestamp 0, making it infinitely old and always eligible for reaping.

TestExpireStaleUsage (5 tests) — usage pool expiry business logic:

test_removes_expired_connections A model with one expired and one fresh connection: only the expired sid is removed, the model entry survives with the fresh connection.
test_removes_model_when_all_connections_expire A model where all connections have expired: the entire model entry is deleted from the pool. Returns the model_id in the cleaned list.
test_keeps_model_with_all_fresh_connections A model with only fresh connections is completely untouched. Returns an empty cleaned list.
test_multiple_models_mixed Three models: one fully dead, one partially expired, one fully alive. Verifies the dead model is removed, the partial model keeps only its fresh connection, and the alive model is untouched.
test_empty_pool Empty pool returns an empty list and does not error.

TestRedisLockAtomicity (6 tests) — mock-based API contract verification:

test_acquire_uses_set_nx Verifies acquire_lock calls redis.set with nx=True and ex=timeout, and returns True on success.
test_acquire_fails_if_already_held Verifies acquire_lock returns False when redis.set returns False.
test_renew_uses_lua_eval Verifies renew_lock calls redis.eval (not redis.set with xx=True), the Lua script contains both GET and SET commands, and the lock_id and timeout are passed as ARGV parameters.
test_renew_returns_false_if_not_owner Verifies renew_lock returns False when the Lua script returns nil, indicating another worker owns the lock.
test_release_uses_lua_eval Verifies release_lock calls redis.eval (not redis.get + redis.delete), the Lua script contains both GET and DEL commands, and the lock_id is passed as an ARGV parameter.
test_release_does_not_delete_if_not_owner Verifies release_lock does not raise an error when the Lua script returns 0, indicating the lock was owned by another worker.

TestRedisLockIntegration (5 tests) — real Lua execution via fakeredis:

test_full_lifecycle End-to-end: acquire sets the key to our lock_id, renew keeps it, release deletes it. Verifies actual Redis state at each step.
test_acquire_is_exclusive Two lock instances on the same key: the first acquires, the second is blocked. After the first releases, the second can acquire. Tests mutual exclusion using real SET NX semantics.
test_renew_rejects_non_owner Worker A acquires, lock expires (simulated via DELETE), worker B acquires. Worker A attempts to renew and gets False. Worker B's lock value is verified to be untouched. This is the exact race condition the Lua scripts are designed to prevent.
test_release_rejects_non_owner Same scenario as above, but worker A attempts release instead of renew. Worker B's lock is verified to remain intact, proving the Lua script correctly prevents deletion of another worker's lock.
test_renew_extends_ttl After acquire and renew, the Redis key's TTL is verified to be positive and within the expected timeout window, confirming the Lua SET with EX correctly resets the expiration.

Contributor License Agreement

By submitting this pull request, I confirm that I have read and fully agree to the Contributor License Agreement (CLA), and I am providing my contributions under its terms.

Note

Deleting the CLA section will lead to immediate closure of your PR and it will not be merged in.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/open-webui/open-webui/pull/21798 **Author:** [@Classic298](https://github.com/Classic298) **Created:** 2/23/2026 **Status:** 🔄 Open **Base:** `dev` ← **Head:** `redis` --- ### 📝 Commits (3) - [`2ca7c3b`](https://github.com/open-webui/open-webui/commit/2ca7c3bfe6dec6f376d6b91b67340e8651e8a950) fix: harden distributed lock lifecycle with atomic Lua scripts and resilient retry - [`daf107a`](https://github.com/open-webui/open-webui/commit/daf107a9934bd1f366300d17809839fde6e15f58) fix: prevent transient errors from permanently killing cleanup loops - [`621289d`](https://github.com/open-webui/open-webui/commit/621289d8d83d7390e8efde39d10cec7a4ff14a42) fix: wrap lock acquire/renew in try/except for Redis fault tolerance ### 📊 Changes **2 files changed** (+100 additions, -60 deletions) <details> <summary>View changed files</summary> 📝 `backend/open_webui/socket/main.py` (+72 -55) 📝 `backend/open_webui/socket/utils.py` (+28 -5) </details> ### 📄 Description # Problem ------- The periodic cleanup loops had two categories of bugs in multi-worker Redis deployments: 1. Race conditions in RedisLock: renew_lock used SET with XX flag, which checks key existence but not ownership. If worker A's lock expired and worker B acquired it, worker A's renewal would silently overwrite B's lock value. Similarly, release_lock performed a non-atomic GET then DELETE, allowing the same ownership-bypass race between calls. 2. Fragile lifecycle management: periodic_session_pool_cleanup gave up permanently on the first failed acquire attempt. periodic_usage_pool_cleanup retried only twice before exiting forever. In both cases, if lock renewal failed mid-loop the cleanup task would die permanently, leaving stale entries to accumulate indefinitely. # Fix --- RedisLock.renew_lock and RedisLock.release_lock now use Lua scripts executed via EVAL to atomically verify lock ownership before modifying the key. This eliminates the TOCTOU race window entirely: - renew_lock: GET + SET in one atomic Lua call, returns nil if not owner - release_lock: GET + DEL in one atomic Lua call, no-ops if not owner The duplicated lock lifecycle logic is consolidated into a single run_with_lock helper that: - Retries acquisition indefinitely with jittered backoff - Releases and re-acquires automatically when renewal fails - Guarantees release via finally block, even on exceptions Both cleanup functions now pass their business logic as a callback to run_with_lock, eliminating the ad-hoc retry loops and inconsistent error handling between the two. Also removes the unused send_usage variable from periodic_usage_pool_cleanup. # Testing Done ------- All changes were validated with 26 tests across 5 test classes using pytest. Tests used mock-based unit tests for API contract verification, asyncio.run() for async logic, and fakeredis with Lua support for end-to-end integration testing of the actual Lua scripts. No live Redis instance was required. TestRunWithLock (5 tests) — distributed lock lifecycle helper: - test_acquires_lock_then_runs_work Verifies the happy path: acquire is called once, renew is called before each work invocation, work runs the expected number of times, and release is called exactly once when the loop is cancelled. - test_retries_acquisition_on_failure Simulates two failed acquire attempts followed by a success. Confirms the helper retries with jittered backoff and does not give up. Verifies acquire is called 3 times total, work runs once after the successful acquire, and release is called. - test_reacquires_on_renewal_failure Simulates renewal failing on the first cycle. Confirms the helper releases the lock, re-acquires it, and continues working. Verifies acquire is called twice, renew twice, release twice (once per cycle), and work runs only in the successful renewal cycle. - test_release_called_even_if_work_raises Verifies the finally-block guarantee: when work throws a RuntimeError, release is still called exactly once before the exception propagates. - test_noop_lock_functions_work Verifies compatibility with non-Redis mode where acquire, renew, and release are all lambda: True. Work runs the expected number of times without errors. TestReapStaleSessions (5 tests) — session reaping business logic: - test_reaps_expired_sessions A session 200 seconds old with a 120-second timeout is removed from the pool. Returns the correct (sid, user_id) tuple. - test_keeps_fresh_sessions A session only 10 seconds old with a 120-second timeout is preserved. Returns an empty list. - test_mixed_stale_and_fresh Pool with three sessions: one very stale (300s), one fresh (10s), one borderline (121s). Only the stale and borderline sessions are reaped; the fresh session survives. - test_empty_pool Empty pool returns an empty list and does not error. - test_missing_last_seen_at_defaults_to_zero A session entry without the last_seen_at field defaults to timestamp 0, making it infinitely old and always eligible for reaping. TestExpireStaleUsage (5 tests) — usage pool expiry business logic: - test_removes_expired_connections A model with one expired and one fresh connection: only the expired sid is removed, the model entry survives with the fresh connection. - test_removes_model_when_all_connections_expire A model where all connections have expired: the entire model entry is deleted from the pool. Returns the model_id in the cleaned list. - test_keeps_model_with_all_fresh_connections A model with only fresh connections is completely untouched. Returns an empty cleaned list. - test_multiple_models_mixed Three models: one fully dead, one partially expired, one fully alive. Verifies the dead model is removed, the partial model keeps only its fresh connection, and the alive model is untouched. - test_empty_pool Empty pool returns an empty list and does not error. TestRedisLockAtomicity (6 tests) — mock-based API contract verification: - test_acquire_uses_set_nx Verifies acquire_lock calls redis.set with nx=True and ex=timeout, and returns True on success. - test_acquire_fails_if_already_held Verifies acquire_lock returns False when redis.set returns False. - test_renew_uses_lua_eval Verifies renew_lock calls redis.eval (not redis.set with xx=True), the Lua script contains both GET and SET commands, and the lock_id and timeout are passed as ARGV parameters. - test_renew_returns_false_if_not_owner Verifies renew_lock returns False when the Lua script returns nil, indicating another worker owns the lock. - test_release_uses_lua_eval Verifies release_lock calls redis.eval (not redis.get + redis.delete), the Lua script contains both GET and DEL commands, and the lock_id is passed as an ARGV parameter. - test_release_does_not_delete_if_not_owner Verifies release_lock does not raise an error when the Lua script returns 0, indicating the lock was owned by another worker. TestRedisLockIntegration (5 tests) — real Lua execution via fakeredis: - test_full_lifecycle End-to-end: acquire sets the key to our lock_id, renew keeps it, release deletes it. Verifies actual Redis state at each step. - test_acquire_is_exclusive Two lock instances on the same key: the first acquires, the second is blocked. After the first releases, the second can acquire. Tests mutual exclusion using real SET NX semantics. - test_renew_rejects_non_owner Worker A acquires, lock expires (simulated via DELETE), worker B acquires. Worker A attempts to renew and gets False. Worker B's lock value is verified to be untouched. This is the exact race condition the Lua scripts are designed to prevent. - test_release_rejects_non_owner Same scenario as above, but worker A attempts release instead of renew. Worker B's lock is verified to remain intact, proving the Lua script correctly prevents deletion of another worker's lock. - test_renew_extends_ttl After acquire and renew, the Redis key's TTL is verified to be positive and within the expected timeout window, confirming the Lua SET with EX correctly resets the expiration. ### Contributor License Agreement  By submitting this pull request, I confirm that I have read and fully agree to the [Contributor License Agreement (CLA)](https://github.com/open-webui/open-webui/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT), and I am providing my contributions under its terms. > [!NOTE] > Deleting the CLA section will lead to immediate closure of your PR and it will not be merged in. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

GiteaMirror added the pull-request label 2026-04-30 01:38:24 -05:00

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/open-webui#49332