Skip to content

Give each forked worker its own redis lock contender identity - #7795

Merged
FarhanAliRaza merged 4 commits into
mainfrom
fix/redis-instance-id-per-worker
Nov 11, 2026
Merged

FarhanAliRaza merged 4 commits into
mainfrom
fix/redis-instance-id-per-worker

Conversation

@masenf

@masenf masenf commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

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/ and verify_rds/).

Defect

With REFLEX_OPLOCK_ENABLED=true and several production backend workers (fork, the Linux default), an event waiting for a token's lock stalled about a tenth of lock_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:

  1. StateManagerRedis._instance_id is chosen once when the manager is built, and the manager is built in the supervisor and inherited by every forked worker (_setup_event_processor re-ids only the token manager). All workers registered in <lock>_waiters under the same member: a second worker's SADD emitted no keyspace sadd event, 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 its lock_expiration / 10 re-check.
  2. Even with distinct ids, one waiter leaving SREM'd its instance while other waiters of the same instance were still queued.

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_processor calls state_manager._reset_instance_id() next to the token manager's reset; the hook is a no-op on StateManager and picks a new uuid in StateManagerRedis, so the memory and disk managers are untouched.
  • _request_lock_release removes the instance from the waiter set only when its last local waiter for that lock leaves (reusing the existing _lock_waiters list). 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 the try, so a waiter cancelled while registering also runs the cleanup.
  • _n_lock_contenders counts only other instances (one atomic script: SCARD minus SISMEMBER(self), still one round trip), so a holder whose only waiters are its own keeps sharing its lease with them.

Test plan

  • New / extended regression tests, failing on unfixed main:
    • tests/units/test_app.py::test_forked_workers_publish_deltas_to_the_socket_owner (extended to a StateManagerRedis): 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).
  • Race regression tests for the cleanup loop:
    • tests/units/istate/manager/test_redis.py::test_oplock_waiter_reregistering_while_the_other_waiter_leaves_deregisters
    • tests/units/istate/manager/test_redis.py::test_oplock_waiter_cancelled_while_registering_deregisters
  • test_redis.py 3× on the mock and on a real redis-server; test_state.py, test_app.py, tests/units/istate, test_token_manager.py on the mock, real Redis, and real Redis with REFLEX_OPLOCK_ENABLED=true: pass.
  • Full unit suite: 15636 passed, 66 skipped, coverage 81.43% (the Playwright browser-driver module was excluded locally because no Chromium binary was available). ruff check, ruff format, pyright reflex tests: clean.
  • End to end (prod, Redis, oplock on, fork; 1 client, 300 pipelined events + 100 /api/bump via app.modify_state, 6 runs per cell):
conc 1 conc 2 conc 4
0.11.0a1, 2 workers 2/6 stalled, p99 1058 ms 2/6, 1029 ms 6/6, 1039 ms
0.11.0a1, 4 workers 0/6, 46 ms 5/6, 2036 ms 6/6, 3035 ms
this branch, 2 workers 0/6, 72 ms 0/6, 81 ms 0/6, 92 ms
this branch, 4 workers 0/6, 70 ms 0/6, 110 ms 0/6, 66 ms

(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

  • Bug fix (non-breaking change which fixes an issue)

Notes for reviewers

  • Visible behaviour change: a holder whose only waiters are on its own instance now always leases (before it sometimes did single updates); that is the intended oplock behaviour.
  • The module-level debug prefix SMR = f"[SMR:{os.getpid()}]" is still inherited across fork (cosmetic, unchanged).
  • Not addressed here: the instance "instaque" fast path in _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

View guided diff Turn on auto-fix

…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
@masenf
masenf requested a review from a team as a code owner November 11, 2026 07:26
@masenf masenf added this to the v0.11.0 milestone Nov 11, 2026
@masenf masenf added the on deck PRs lined up to review / merge next label Nov 11, 2026
@greptile-apps

greptile-apps Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues remain from this review.

Summary

Gives each forked worker its own Redis lock-waiter identity and keeps a worker registered until its last waiter leaves.

  • Forked Redis workers use separate lock-waiter identities.
  • Redis keeps each worker visible until its last lock waiter leaves.

Reviews (4) · Last reviewed commit: "refactor(state-manager): fix the instanc..." · Reviewed by Greptile

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

@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 7 files

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

View guided diff | Turn on auto-fix | Re-trigger cubic

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

codspeed Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 1.6%

2 warnings

⚠️ 21 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ 3 benchmarks with a significant change ran on different hardware

Their base and head results were measured on machines with a different CPU, core count or memory, so the changes might not come from the code. Run base and head on the same runner type.

Learn more about Macro Runners

See affected benchmarks

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 148 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ test_route_matching[100] 47.8 µs 50.6 µs -5.59%
❌ test_process_event_burst_independent_tokens[1] 2.7 ms 2.8 ms -4.38%
⚡ test_state_collection_assignment[100000] 1.1 s 1 s +5.54%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing fix/redis-instance-id-per-worker (bb766d0) with main (b4d4cc8)

Open in CodSpeed

@masenf

masenf commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

The failing CodSpeed Performance Analysis check is not caused by this PR. The four flagged benchmarks are test_route_matching[10] / [1000], test_state_collection_assignment[100000] and test_process_event_burst_independent_tokens[1] (−4 to −5 %). This diff only touches the Redis state manager's waiter bookkeeping (reflex/istate/manager/redis.py, app.py's per-worker reset), which the route-matching and state-delta benchmarks never reach, and the benchmarks run on the in-memory manager. CodSpeed's own warning says "4 benchmarks with a significant change ran on different hardware". The same test_route_matching pair regresses by the same amount on the unrelated #7798. The required performance-tests-gate passes, and the CodSpeed check is not in the required set. This needs an acknowledgement on CodSpeed (or a re-run on matching hardware); there is no code fix to make here.


Generated by Claude Code

@masenf masenf left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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
masenf pushed a commit that referenced this pull request Nov 11, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi

@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 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
masenf pushed a commit that referenced this pull request Nov 11, 2026
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.
masenf pushed a commit that referenced this pull request Nov 11, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi
@FarhanAliRaza
FarhanAliRaza merged commit f0088e0 into main Nov 11, 2026
167 of 168 checks passed
@FarhanAliRaza
FarhanAliRaza deleted the fix/redis-instance-id-per-worker branch November 11, 2026 15:03
masenf added a commit that referenced this pull request Nov 11, 2026
* 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)
masenf pushed a commit that referenced this pull request Nov 11, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dxjadq3qkhKnqZR2b9WBFi
@masenf masenf added the release-train Issues found during agentic /prerelease-testing fanout, not necessarily validated bugs. label Nov 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on deck PRs lined up to review / merge next release-train Issues found during agentic /prerelease-testing fanout, not necessarily validated bugs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants