Skip to content

[fix] Only check the redis session lock once per set_state fan-out - #6853

Open
qubydev wants to merge 1 commit into
reflex-dev:mainfrom
qubydev:fix/dedupe-redis-state-lock-checks
Open

[fix] Only check the redis session lock once per set_state fan-out#6853
qubydev wants to merge 1 commit into
reflex-dev:mainfrom
qubydev:fix/dedupe-redis-state-lock-checks

Conversation

@qubydev

@qubydev qubydev commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • 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?

closes #6848

Summary

StateManagerRedis.set_state was 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=True into the recursive calls. Dirty substates still get written the same way as before.

Test plan

  • Added test_set_state_validates_lock_once_per_fanout
  • Updated test_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

Review in cubic

@qubydev
qubydev requested a review from a team as a code owner August 6, 2026 20:48
@qubydev

qubydev commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Would love to know your thoughts on this @harsh21234i

@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR reduces Redis lock-check round trips by validating ownership once at the root of StateManagerRedis.set_state and propagating that result through recursive substate writes.

  • Adds a lock_checked recursion flag to the Redis state manager.
  • Adds coverage asserting one lock GET/PTTL per state-tree fan-out.
  • Updates lock-warning expectations from one warning per substate to one per fan-out.

Confidence Score: 4/5

The 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

Important Files Changed

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,
),

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.

P1 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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge 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 👍 / 👎.

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

Re-trigger cubic

@harsh21234i

harsh21234i commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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.

@qubydev

qubydev commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

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.

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.

Redundant per-substate lock checks in set_state

2 participants