-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ENG-8227: always _clean() after app.modify_state #5949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
app.modify_state might not always generate a delta, but it _does_ always need to be cleaned so that dirty backend vars and dirty substates (with backend vars) are not persisted to redis (and subsequently reloaded when the referenced states may not be cached)
CodSpeed Performance ReportMerging #5949 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Fixed a critical bug where app.modify_state would leave dirty state metadata (dirty_vars and dirty_substates) in memory when backend variables were updated without generating frontend deltas. This caused dirty substates to be incorrectly persisted to Redis and subsequently reloaded, leading to errors when the referenced substates were not cached.
Key changes:
- Moved
state._clean()call to always execute after_get_resolved_delta(), regardless of whether a delta was generated - Updated comment to clarify that cleanup happens for all state modifications, not just frontend var changes
- Added comprehensive test coverage for all four scenarios: root/substate × frontend/backend variable updates
Why this matters:
When a backend variable is modified in a substate, it marks the parent state's dirty_substates but produces no delta (backend vars don't sync to frontend). Previously, _clean() only ran when a delta existed, so the parent state was persisted to Redis with stale dirty_substates metadata. On subsequent state reloads, the parent would reference substates that weren't actually loaded, causing runtime errors.
Confidence Score: 5/5
- This PR is safe to merge - it fixes a critical state management bug with minimal code changes and comprehensive test coverage
- The change is a simple one-line reordering that makes the cleanup unconditional, which is the correct behavior. The fix is well-understood, addresses a real bug with a detailed repro case, and includes thorough test coverage for all scenarios (4 parameterized test cases). The logic is sound:
_clean()should always run after state modifications to prevent dirty state metadata from being persisted. - No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| reflex/app.py | 5/5 | Moved state._clean() call outside the if delta: block to ensure dirty state is always cleaned up, preventing Redis persistence of dirty state metadata |
| tests/units/test_app.py | 5/5 | Added comprehensive test covering all four scenarios (root/substate × frontend/backend) to verify _clean() is always called and dirty state is properly reset |
Sequence Diagram
sequenceDiagram
participant Client
participant App
participant StateManager
participant State
participant Redis
Note over Client,Redis: Before: Bug Scenario (Backend var update)
Client->>App: modify_state(token)
App->>StateManager: modify_state(token)
StateManager->>Redis: Load state
Redis-->>StateManager: state instance
StateManager-->>State: yield state
State-->>App: state context
Note over State: Update backend var<br/>dirty_vars={_backend}<br/>dirty_substates={SubState}
App->>State: _get_resolved_delta()
State-->>App: {} (empty - no frontend changes)
Note over App,State: ❌ OLD: _clean() NOT called<br/>dirty_substates persists!
StateManager->>Redis: Save state with dirty_substates
Note over Client,Redis: Later: Reload triggers error
App->>StateManager: modify_state(token)
StateManager->>Redis: Load state
Redis-->>StateManager: state with dirty_substates
Note over State: ❌ Substates not loaded<br/>but dirty_substates set<br/>→ Runtime Error!
Note over Client,Redis: After: Fixed Behavior
Client->>App: modify_state(token)
App->>StateManager: modify_state(token)
StateManager->>State: yield state
Note over State: Update backend var<br/>dirty_vars={_backend}<br/>dirty_substates={SubState}
App->>State: _get_resolved_delta()
State-->>App: {} (empty - no frontend changes)
App->>State: ✅ _clean() ALWAYS called
Note over State: dirty_vars cleared<br/>dirty_substates cleared<br/>_was_touched updated
StateManager->>Redis: Save clean state
Note over Redis: ✅ No stale metadata persisted
2 files reviewed, no comments
|
Hi @adhami3310, thanks for making these technical issues visible for everyone who follows the development of Reflex! The above flow diagram helps understanding the architecture and issues such as the persisting of dirty state more insightful ❤️ |
app.modify_state might not always generate a delta, but it does always need to be cleaned so that dirty backend vars and dirty substates (with backend vars) are not persisted to redis (and subsequently reloaded when the referenced states may not be cached).
Real world repro
(must use redis, may use REFLEX_OPLOCK_ENABLED on or off)