Skip to content

fix(state-manager): stop redis lock handoffs from stalling events until the lock expires - #7534

Open
benedikt-bartscher wants to merge 17 commits into
reflex-dev:mainfrom
benedikt-bartscher:fix/redis-lock-handoff
Open

benedikt-bartscher wants to merge 17 commits into
reflex-dev:mainfrom
benedikt-bartscher:fix/redis-lock-handoff

Conversation

@benedikt-bartscher

@benedikt-bartscher benedikt-bartscher commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Under CPU load, our app's events stalled for 5 or 10 seconds (the oplock hold time and the lock expiration), and some updates made under a lease were lost. StateManagerRedis had several ways to lose a lock or a wakeup:

  • A modify cancelled while taking the lock left the lock behind until it expired. supersedes and a new on_load_internal cancel events at any await. This covers a SET NX whose reply never arrived, the awaits in _try_modify_state before a lease or the single update owns the lock, and a lease task cancelled before its first step, which never enters the context that releases the lock.
  • The release was a GETDEL, so a holder releasing after its lock expired deleted the next holder's lock.
  • A lease never extended its lock while events kept using the cached state. When they outlasted the lock expiration, the flush failed with LockExpiredError and dropped every change made under the lease.
  • A queued waiter only learns about a lease from the holder ahead of it, which skipped the notification when it raised or its lock had expired, and a waiter cancelled after its wakeup took the wakeup with it. The other waiters then sat out the whole lock expiration.

This PR:

  • releases the lock with a compare-and-delete script, also when the acquisition is cancelled, and runs a release a cancellation interrupted again
  • closes the lock context on every path that does not hand it to a lease, and hands it over with AsyncExitStack.pop_all() once the state is fetched
  • flushes and releases a lease that was cancelled before it started
  • gives a lease a full expiration when it starts, and while the flush waits for and writes the cached state, renews the lock as long as the lease still owns it, giving each use of the cached state the lock expiration from when it got it, so a use that runs over it still ends in LockExpiredError
  • wakes the next waiter in all of those cases, and lets waiters re-check the lock every tenth of its expiration, so a missed notification costs at most that

Each case has a test in tests/units/istate/manager/test_redis.py that fails without the fix. test_oplock_immediate_cancel now checks the outcome, because its lease can be cancelled before it starts.

With a script that cancels a waiter around the handoff (2 s lock expiration, 50 ms redis latency), 19 of 30 waiters waited out the expiration before and none of 60 after. A fuzzer with up to 2 s holders lost up to 23 of 26 increments per round before and none after.

The unit tests pass (12512 without Redis, 12525 against Redis with and without oplock).

View guided diff

@greptile-apps

greptile-apps Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[High impact] The PR appears safe to merge; no blocking findings remain.

Summary

This PR makes Redis lock handoffs safer when events are cancelled or locks expire.

  • Lock release and renewal check who owns the lock.
  • Lease cleanup waits for cached changes to be written before releasing the lock.
  • Cached-state handoffs wake renewal, and waiters periodically re-check the lock.
  • The latest changes add the missing helper docstrings without changing behavior. No new actionable issues were found.

Reviews (7) · Last reviewed commit: "Merge branch 'redis-lock-handoff-fixline..." · Reviewed by Greptile

Comment thread reflex/istate/manager/redis.py Outdated
Comment thread reflex/istate/manager/redis.py
@codspeed

codspeed Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ 17 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

✅ 148 untouched benchmarks


Comparing benedikt-bartscher:fix/redis-lock-handoff (1c17f40) with main (2eb1b44)

Open in CodSpeed

Comment thread reflex/istate/manager/redis.py Outdated
@benedikt-bartscher
benedikt-bartscher marked this pull request as ready for review October 11, 2026 11:12
@benedikt-bartscher
benedikt-bartscher requested a review from a team as a code owner October 11, 2026 11:12

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 4 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread reflex/istate/manager/redis.py Outdated
Comment thread reflex/istate/manager/redis.py

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant