Skip to content

Conversation

@masenf
Copy link
Collaborator

@masenf masenf commented Nov 5, 2025

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

"""Repro for dirty substate missing from parent state.

Must use StateManagerRedis!

1. Cause common parent state to be touched.
    1a. With oplock enabled, this can be a normal parent var update in a separate `async with self`
    1b. Without oplock, this can be any parent backend var update in the same `async with self`
2. Update a sibling state's backend var to mark it dirty, but NOT result in a delta being sent.
    This causes the parent's dirty_substates to be persisted to redis, because
    `App.modify_state` only calls `_clean()` when a delta is produced.
3. If oplock is enabled, wait for the lock expiration.
4. Enter a new `async with self` and make any change.
    Because the parent state is loaded from redis with dirty_substates already
    set, but the actual substates are not loaded resulting in an error during
    delta resolution.
"""

import asyncio
import reflex as rx


class State(rx.State):
    """The app state."""
    count: int = 0
    _backend_base: int = 0


class BrotherState(State):
    """A sibling state to test oplock missing substate issue."""
    brother_count: int = 0

    @rx.event(background=True)
    async def mark_dirty_no_delta_oplock(self):
        from reflex.istate.manager.redis import StateManagerRedis

        async with self:
            # Must touch any var in parent state in a separate context, so it is marked as touched.
            self.count += 1
        async with self:
            # Must update another state's backend var to mark it dirty, but get no delta.
            sister_state = await self.get_state(SisterState)
            sister_state._backend_sis += 1
            parent_state = self.parent_state
            print("parent dirty_substates 1:", self.parent_state.dirty_substates)
        print(f"parent _was_touched: {parent_state._was_touched} {sister_state._was_touched}")
        # Must wait for the oplock lease to expire.
        state_manager = rx.state.get_state_manager()
        if isinstance(state_manager, StateManagerRedis):
            await asyncio.sleep(state_manager.lock_expiration / 1000)
        async with self:
            # Now when refetching the common parent from redis, it should still have dirty_substates, but the actual substates will not be fetched.
            print("parent dirty_substates 2:", self.parent_state.dirty_substates)
            self.brother_count += 1

    @rx.event(background=True)
    async def mark_dirty_no_delta(self):
        async with self:
            # Must touch backend var in parent state, so it will get persisted.
            self._backend_base += 1
            # Must update another state's backend var to mark it dirty, but get no delta.
            sister_state = await self.get_state(SisterState)
            sister_state._backend_sis += 1
            parent_state = self.parent_state
            print("parent dirty_substates 1:", self.parent_state.dirty_substates)
        print(f"parent _was_touched: {parent_state._was_touched} {sister_state._was_touched}")
        async with self:
            # Now when refetching the common parent from redis, it should still have dirty_substates, but the actual substates will not be fetched.
            print("parent dirty_substates 2:", self.parent_state.dirty_substates)
            self.brother_count += 1
        async with self:
            # No changes here, should not call _clean
            print(self.brother_count)


class SisterState(State):
    """Another sibling state to test oplock missing substate issue."""
    _backend_sis: int = 0


def index() -> rx.Component:
    return rx.container(
        rx.hstack(
            rx.button(
                "Repro w/ Oplock",
                on_click=BrotherState.mark_dirty_no_delta_oplock,
            ),
            rx.button(
                "Repro w/o Oplock",
                on_click=BrotherState.mark_dirty_no_delta,
            ),
        ),
    )


app = rx.App()
app.add_page(index)

(must use redis, may use REFLEX_OPLOCK_ENABLED on or off)

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)
@linear
Copy link

linear bot commented Nov 5, 2025

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 5, 2025

CodSpeed Performance Report

Merging #5949 will not alter performance

Comparing masenf/clean-after-background-update (105ae9f) with main (6663dfd)

Summary

✅ 8 untouched

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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
Loading

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@adhami3310 adhami3310 merged commit 982d2fa into main Nov 5, 2025
47 checks passed
@adhami3310 adhami3310 deleted the masenf/clean-after-background-update branch November 5, 2025 01:56
@larsblumberg
Copy link

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 ❤️

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.

4 participants