Repository navigation
Give each forked worker its own redis lock contender identity - #7795
Conversation
…der identity Forked production workers inherited the supervisor's StateManagerRedis, so they all registered as one member of a lock's `<lock>_waiters` set. A second worker's SADD was a no-op that emitted no keyspace event, so it did not break the holder's lease, and any waiter's SREM deregistered every worker. The next lease then passed over the remaining waiters until their re-check at a tenth of the lock expiration (~1 s by default, up to 3 s observed at 4 workers). - Reset the state manager's instance id per worker in App._setup_event_processor, next to the token manager's, through a no-op StateManager._reset_instance_id hook that StateManagerRedis overrides. - Keep an instance registered until its last local waiter for the lock leaves, re-registering when a waiter queued during the SREM. - Count only other instances as contenders (one atomic script), since a lease serves the instance's own waiters; this keeps the same-instance lease sharing that the refcount would otherwise turn into single updates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi
[Medium impact] The PR appears safe to merge; no actionable issues remain from this review. SummaryGives each forked worker its own Redis lock-waiter identity and keeps a worker registered until its last waiter leaves.
Reviews (4) · Last reviewed commit: "refactor(state-manager): fix the instanc..." · Reviewed by Greptile |
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Merging this PR will degrade performance by 1.6%2 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
|
The failing Generated by Claude Code |
masenf
left a comment
There was a problem hiding this comment.
looks good aside from reviewbot feedback
…til it is stable A waiter that left while the last other waiter re-registered counted that one as staying and skipped its SREM, so the instance stayed in `<lock>_waiters` with no local waiter until the set's pexpire. The same happened to a waiter cancelled during its registration, which ran before the cleanup's `try` while the others already counted it. - Register inside the `try`, so a cancellation during registration still runs the cleanup. - Repeat the SREM while this is the last local waiter, re-registering only while a waiter that queued during it is still there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
A regression that never reached a rendezvous hung the suite instead of failing. Each wait now times out, and the paused or hanging waiter is released or cancelled in `finally`, so a failure leaves no pending task. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi
…ike the other redis calls The instance id is only this instance's member in the lock waiter sets; _local_leases is keyed by token. The async client always returns an awaitable, so cast it as the eval calls do instead of checking at runtime.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi
* fix(state-manager): give each forked worker its own redis lock contender identity Forked production workers inherited the supervisor's StateManagerRedis, so they all registered as one member of a lock's `<lock>_waiters` set. A second worker's SADD was a no-op that emitted no keyspace event, so it did not break the holder's lease, and any waiter's SREM deregistered every worker. The next lease then passed over the remaining waiters until their re-check at a tenth of the lock expiration (~1 s by default, up to 3 s observed at 4 workers). - Reset the state manager's instance id per worker in App._setup_event_processor, next to the token manager's, through a no-op StateManager._reset_instance_id hook that StateManagerRedis overrides. - Keep an instance registered until its last local waiter for the lock leaves, re-registering when a waiter queued during the SREM. - Count only other instances as contenders (one atomic script), since a lease serves the instance's own waiters; this keeps the same-instance lease sharing that the refcount would otherwise turn into single updates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi * fix(state-manager): reconcile an instance's contender registration until it is stable A waiter that left while the last other waiter re-registered counted that one as staying and skipped its SREM, so the instance stayed in `<lock>_waiters` with no local waiter until the set's pexpire. The same happened to a waiter cancelled during its registration, which ran before the cleanup's `try` while the others already counted it. - Register inside the `try`, so a cancellation during registration still runs the cleanup. - Repeat the SREM while this is the last local waiter, re-registering only while a waiter that queued during it is still there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi * test(state-manager): bound the contender race tests' rendezvous waits A regression that never reached a rendezvous hung the suite instead of failing. Each wait now times out, and the paused or hanging waiter is released or cancelled in `finally`, so a failure leaves no pending task. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi * refactor(state-manager): fix the instance id comment and await srem like the other redis calls The instance id is only this instance's member in the lock waiter sets; _local_leases is keyed by token. The async client always returns an awaitable, so cast it as the eval calls do instead of checking at runtime. --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Farhan <www.mfarvirus@gmail.com> (cherry picked from commit f0088e0)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi
Fixes FINDING-030 from the reflex 0.11.0a1 pre-release testing (artifacts on branch
claude/vibrant-pasteur-3921g3,prerelease-testing/runs/2026-11-11-v0.11.0a1/redis_runtime/andverify_rds/).Defect
With
REFLEX_OPLOCK_ENABLED=trueand several production backend workers (fork, the Linux default), an event waiting for a token's lock stalled about a tenth oflock_expiration(~1 s by default) under contention, contrary to the #7534 changelog line "events waiting on other instances are no longer passed over by the next lease". Two causes:StateManagerRedis._instance_idis chosen once when the manager is built, and the manager is built in the supervisor and inherited by every forked worker (_setup_event_processorre-ids only the token manager). All workers registered in<lock>_waitersunder the same member: a second worker's SADD emitted no keyspacesaddevent, so the holder's lease was not broken, and any waiter's SREM on exit deregistered every worker. The passed-over waiter only re-registered at itslock_expiration / 10re-check.The instance id is used only as the member of the waiter sets (lock values, leases, pubsub channels and socket records use other ids), so resetting it per worker leaves nothing pointing at the supervisor's id.
Fix
App._setup_event_processorcallsstate_manager._reset_instance_id()next to the token manager's reset; the hook is a no-op onStateManagerand picks a new uuid inStateManagerRedis, so the memory and disk managers are untouched._request_lock_releaseremoves the instance from the waiter set only when its last local waiter for that lock leaves (reusing the existing_lock_waiterslist). Its cleanup loops until the registration is stable: it re-registers while a waiter that queued during the SREM is still there, and deregisters again once that waiter has left. The registration runs inside thetry, so a waiter cancelled while registering also runs the cleanup._n_lock_contenderscounts only other instances (one atomic script:SCARDminusSISMEMBER(self), still one round trip), so a holder whose only waiters are its own keeps sharing its lease with them.Test plan
main:tests/units/test_app.py::test_forked_workers_publish_deltas_to_the_socket_owner(extended to aStateManagerRedis): before,AssertionError: assert 'c56438be-…' not in {'c56438be-…'}(worker id equals the supervisor's).tests/units/istate/manager/test_redis.py::test_oplock_contenders_are_the_other_instances_with_a_waiter: before,assert 1 == 0.tests/units/istate/manager/test_redis.py::test_oplock_waiter_leaving_keeps_the_next_lease_from_passing_over_its_instance(lock_expiration 20 s → 2 s re-check; asserts the remaining remote waiter finishes within 1 s): before,TimeoutError(2.003 s on the mock, 2.006 s on real Redis — exactly one re-check interval).tests/units/istate/manager/test_redis.py::test_oplock_waiter_reregistering_while_the_other_waiter_leaves_deregisterstests/units/istate/manager/test_redis.py::test_oplock_waiter_cancelled_while_registering_deregisterstest_redis.py3× on the mock and on a realredis-server;test_state.py,test_app.py,tests/units/istate,test_token_manager.pyon the mock, real Redis, and real Redis withREFLEX_OPLOCK_ENABLED=true: pass.ruff check,ruff format,pyright reflex tests: clean./api/bumpviaapp.modify_state, 6 runs per cell):(stalled = an api wait over 500 ms). Final counts exact in 72/72 runs; worker-id probe: 4 pids / 1 id before, 4 / 4 after; spawn with 4 workers also 0/6.
Type of change
Notes for reviewers
SMR = f"[SMR:{os.getpid()}]"is still inherited across fork (cosmetic, unchanged)._wait_lock(byte-identical to 0.10.1) can let a tight local loop starve a remote waiter; separate follow-up.News fragment:
news/+redis-oplock-worker-contenders.bugfix.md.Generated by Claude Code