[fix] Only check the redis session lock once per set_state fan-out - #6853
[fix] Only check the redis session lock once per set_state fan-out#6853qubydev wants to merge 1 commit into
Conversation
|
Would love to know your thoughts on this @harsh21234i |
Greptile SummaryThis PR reduces Redis lock-check round trips by validating ownership once at the root of
Confidence Score: 4/5The PR should not merge until recursive writes remain protected against lock expiry or ownership replacement during the fan-out. The initial lock check is separated from multiple independent asynchronous Redis writes, allowing an expired owner to overwrite state persisted by a newly acquired owner. Files Needing Attention: reflex/istate/manager/redis.py
|
| Filename | Overview |
|---|---|
| reflex/istate/manager/redis.py | Consolidates lock validation across recursive writes, but allows later substate writes to proceed after lease expiry or ownership replacement. |
| tests/units/istate/manager/test_redis.py | Adds precise Redis call-count coverage, but does not exercise lock expiry or replacement between root validation and child persistence. |
| tests/units/test_state.py | Updates the warning-count assertion consistently with the intended once-per-fan-out behavior. |
Reviews (1): Last reviewed commit: "Only check the redis session lock once p..." | Re-trigger Greptile
| lock_id=lock_id, | ||
| lock_checked=lock_checked, | ||
| **context, | ||
| ), |
There was a problem hiding this comment.
Lock ownership expires during fan-out
If the session lock expires and another event acquires it while the asynchronous fan-out is running, recursive calls with lock_checked=True persist their state without validating the current owner, causing stale writes to overwrite the new owner's updates or produce a state tree containing data from different events.
Knowledge Base Used: State Management
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cff19a4ce8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| token, | ||
| substate, | ||
| lock_id=lock_id, | ||
| lock_checked=lock_checked, |
There was a problem hiding this comment.
Preserve lock checks for recursive writes
When a large fan-out takes longer than lock_expiration after the parent check (for example, while building/scheduling many substate tasks before they get to run), this forwards lock_checked=True to every recursive set_state, so dirty substates skip the Redis GET and can still SET after the session lock has expired or been reacquired by another worker. Previously each recursive branch rejected that case with LockExpiredError; keep a validation at the point each branch starts/writes, or otherwise guard against expiry during the fan-out.
Useful? React with 👍 / 👎.
|
Hey @qubydev Thanks for reaching out. The reduction in Redis round-trips makes sense, but the current lock_checked=True propagation appears unsafe because the lock may expire or be reacquired by another worker during the fan-out. Recursive writes should retain an ownership check at the point of persistence, or use an equivalent atomic mechanism. Please add regression coverage for lock expiry and ownership replacement during fan-out. Once that is addressed, I’d be happy to re-review. |
Yeah that's a valid concern, I'll take a deeper look and apply changes. Thanks. |
All Submissions:
Type of change
Changes To Core Features:
closes #6848
Summary
StateManagerRedis.set_statewas rechecking the session lock (GET+PTTL) once per substate while walking the in memory tree. That's the same lock key every time, so for a large state tree it was a lot of useless Redis round-trips, even when nothing was dirty.This validates the lock once at the root of the fan-out and passes
lock_checked=Trueinto the recursive calls. Dirty substates still get written the same way as before.Test plan
test_set_state_validates_lock_once_per_fanouttest_state_manager_lock_warning_threshold_contend(one warning per fan-out, not one per substate)uv run pytest tests/units --cov --no-cov-on-fail --cov-report=uv run ruff check .uv run ruff format .uv run pyright reflex tests