Skip to content

Let a redis lock's waiters hear of its release before the releaser can retake it - #7801

Closed
masenf wants to merge 1 commit into
mainfrom
fix/redis-lock-fair-handoff
Closed

masenf wants to merge 1 commit into
mainfrom
fix/redis-lock-fair-handoff

Conversation

@masenf

@masenf masenf commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Found in the reflex 0.11.0a1 pre-release testing (FINDING-030 side note / FINDING-033 investigation; artifacts on branch claude/vibrant-pasteur-3921g3, prerelease-testing/runs/2026-11-11-v0.11.0a1/redis_runtime/ and verify_rds/). Independent of #7795 (reproduces with oplock off); touches the same file in different hunks and merges cleanly with it.

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

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

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Defect

With the Redis state manager, an instance that modifies a token back to back (e.g. a loop of app.modify_state calls) could repeatedly re-take the token's lock ahead of a waiter on another instance. Bisected to #7534 (d36a687): the lock release changed from GETDEL to the compare-and-delete EVAL script (_RELEASE_LOCK_SCRIPT). Redis flushes queued replies in the order each client's first reply was queued: with GETDEL the keyspace notification to the waiters' subscriber is written before the releaser's reply; with EVAL (and plain DEL) the releaser's reply comes first, so in an event loop the releaser wakes first and its next SET NX wins before the remote waiter hears of the release. (The instaque fast path itself is unchanged since 0.10.1; 0.10.1 was fair because of GETDEL's write order.) Measured (bin/order.py, 200 rounds, notification-first): getdel 200/200, eval 0/200, del 0/200, PING pipelined before the EVAL 200/200.

Impact: with two managers in one event loop a remote waiter stalled ~3.8-4.4 s across 1000 back-to-back modifies (0.10.1: 0.01-0.04 s), oplock on or off, redis-py 7 or 8. In a real prod app (2 workers, 3000-iteration modify_state loop on one worker) the worst latency of other-worker requests went from 14-24 ms (0.10.1) to 77-165 ms (0.11.0a1); medians unchanged; final counts always exact. A mild tail-latency regression under a tight loop, not starvation in practice.

Fix

_send_lock_release sends PING and the unchanged release EVAL in one non-transactional pipeline, restoring 0.10.1's reply order — no extra round trip (uncontended modify_state ~1.15 ms/op before and after). The cancellation re-send under shield goes through the same helper. Mutual exclusion, fencing, leases and cancellation cleanup are untouched (same script).

Test plan

  • New test_remote_waiter_not_passed_over_by_back_to_back_modifies[oplock False/True] (real Redis only; skipped on the mock): before the fix 200/200 loop iterations finished before the remote waiter got in (5 runs × both modes); after, 6-7/200 in every run.
  • test_cancel_while_releasing_lock_releases_it now patches _send_lock_release instead of redis.eval; the mock pipeline gained ping / eval.
  • tests/units/istate: 269 (mock) / 270 (real Redis) / 270 (real Redis + oplock) passed. Full unit suite: 15634 passed on the mock, 15650 on real Redis, 81.4-81.5% (Playwright browser-driver module excluded locally for lack of a Chromium binary). ruff, pyright: clean.
  • Before/after (in-process max wait for the remote waiter, 3 trials): 0.11.0a1 3.8-4.2 s → this branch 0.01-0.02 s (oplock on), 0.008-0.009 s (off). Real app worst other-worker latency: 77-165 ms → 11-25 ms.

Notes

  • This relies on Redis's (undocumented) reply write order, as 0.10.1 did by accident; verified on Redis 7.0.15. With io-threads, proxies, Valkey, KeyDB or Dragonfly the order may differ, and behaviour then falls back to today's (no correctness risk). A real fair hand-off (per-waiter queue) is a larger protocol change — follow-up issue.

News fragment: news/+redis-lock-release-wakes-waiters-first.bugfix.md.


Generated by Claude Code

View guided diff Turn on auto-fix

Since #7534 the lock release is a compare-and-delete script instead of
GETDEL. Redis writes the replies it has queued in one pass in the reverse
order of each client's first queued reply: GETDEL queued its reply before
the DEL notified the keyspace subscribers, so they were written first; the
script notifies them before its reply is queued, so the releasing client
hears back first. An instance with no local waiter for the lock then
retakes it at once (the instaque fast path), ahead of the waiters on other
instances that were still being notified.

In one event loop (two managers in-process) that order is deterministic and
a waiter was passed over for a whole 1000-modify loop (~4 s, oplock on and
off; 0.10.1: < 0.25 s). Across prod workers it is a race the releaser now
wins more often: other-worker requests against a 3000-modify server-side
loop waited up to 77-165 ms (0.10.1: 14-24 ms).

Send a PING ahead of the release script in the same pipeline, which queues
this client's reply first again. It costs no round trip, and the script and
its cancellation handling are unchanged; where the write order differs
(proxies, io-threads) the behavior is the same as before.

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 08:28
@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 regression was found.

Summary

Sends PING before the existing lock-release script in one non-transactional Redis pipeline so waiting workers can hear about releases sooner.

  • Redis lock waiters get a turn before the releasing worker retakes the lock.

No actionable issues found. Tests were not run during this review.

Reviews (3) · Last reviewed commit: "fix(state-manager): let a redis lock's w..." · Reviewed by Greptile

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

No issues found across 4 files

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

@codspeed

codspeed Bot commented Nov 11, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 8.79%

2 warnings

⚠️ 18 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

⚠️ 2 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

❌ 2 regressed benchmarks
✅ 149 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ test_modify_state[redis] 60.5 ms 69.5 ms -12.96%
❌ test_process_event_burst_independent_tokens[1] 2.7 ms 2.9 ms -4.42%

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-lock-fair-handoff (2b3401b) with main (2d71015)

Open in CodSpeed

@masenf

masenf commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

I'm moving this to draft and out of the 0.11.0 fix-up set. CodSpeed's −13% on test_modify_state[redis] is real, not runner noise:

  • On real redis-py: pipelining the PING ahead of the release EVAL raises one release from a median of about 92 µs to about 168 µs (345 → 576 Python calls in cProfile). That adds 10–20% client CPU to every uncontended modify_state.
  • On the mock: _Pipeline.execute makes it worse with an asyncio.gather per command.

I looked at cheaper ways to get the same reply order. None fit a fix-up:

  • A single script: it can't reorder the replies, because the notification is queued while the script runs.
  • A reused pipeline: the cost is in execute, and a shared pipeline isn't safe across concurrent releases.
  • Raw connection writes: that means hand-managing redis-py's connection pool and cancellation handling.
  • Pipelining only when contended: without oplock, this instance has no signal that a remote waiter exists.

The gain only shows up in one pattern: an instance that loops modify_state on one token while another worker waits for it. There it cuts worst-case latency from 77–165 ms to 11–25 ms. Correctness is unaffected either way. Making every event pay for that tail isn't worth it, so 0.11.0 ships with today's behaviour. #7805 tracks a fair hand-off that doesn't depend on Redis's reply order and adds no cost to the uncontended path. This branch's regression test is referenced there.

Leaving this open as a draft in case you'd rather take the trade-off anyway.


Generated by Claude Code

@masenf
masenf marked this pull request as draft November 11, 2026 08:46
@masenf masenf removed the on deck PRs lined up to review / merge next label Nov 11, 2026
masenf pushed a commit that referenced this pull request Nov 11, 2026
@masenf

masenf commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

closing, not worth the cost. will fix properly in #7805

@masenf masenf closed this Nov 11, 2026
masenf pushed a commit that referenced this pull request Nov 11, 2026
…last open

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

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.

2 participants