fix(events): stop background tasks from cleaning the root state unlocked - #6920
fix(events): stop background tasks from cleaning the root state unlocked#6920adhami3310 wants to merge 12 commits into
Conversation
After dropping the state lock, the background branch of _execute_event passed root_state into process_event, whose trailing chain_updates snapshotted dirty vars, suspended for delta resolution and the socket emit, and then ran _clean() on the root, all without holding the lock. On a shared state tree (opportunistic locking, or the in-memory state manager) that window races concurrent events: a foreground handler's write landing between the snapshot and the _clean() is discarded before any delta harvests it, so the value never reaches the frontend. With a 1 Hz background poller this silently eats roughly 1 in 8 foreground updates that land near a poll tick. Pass root_state=None instead: the trailing chain_updates still routes yielded and returned events, but does no delta work. A background task's own state changes are emitted and cleaned by its async-with-self context exits, which re-acquire the lock, so nothing is lost.
bfb2150 to
517c538
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
Greptile SummaryThis PR prevents background event processing from resolving and cleaning shared root state after releasing the state lock. It adds per-yield lock-aware flushing, preserves locked delta refreshes for handlers that never enter a mutable context, tracks successful proxy-context entry, and adds deterministic regression coverage for concurrent writes and yield ordering. Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/event/processor/base_state_processor.py | Restricts background delta flushing to lock-held paths and adds a locked compatibility flush for handlers that never enter a proxy context. |
| reflex/istate/proxy.py | Tracks whether a background proxy successfully entered a mutable context so the processor can select the appropriate final-flush behavior. |
| tests/units/reflex_base/event/processor/test_base_state_processor.py | Adds deterministic coverage for concurrent foreground writes, delta-before-event ordering, no-context compatibility flushing, lock gating, and failed context entry. |
| news/6920.bugfix.md | Documents the user-visible stale-UI race and its lock-safe resolution. |
| packages/reflex-base/news/6920.bugfix.md | Documents the underlying unlocked snapshot-and-clean race for the reflex-base package. |
Reviews (8): Last reviewed commit: "docs: state the user-facing symptom in t..." | Re-trigger Greptile
Two follow-ups from review and CI: Background generators yielding from inside async-with-self are suspended while their proxy still holds the lock, so flushing the delta there is safe and keeps the documented ordering (deltas reach the frontend before the yielded event). Gate the per-yield flush on the lock being held instead of dropping it for the whole background handler. The regression test's wait_for suppress missed on py3.10, where asyncio.TimeoutError is not yet the builtin; use asyncio.wait, which does not raise on timeout.
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The foreground handler now proceeds on FIRST_COMPLETED of the trailing resolution's signal (pre-fix code) or the background event's own future (fixed code), and afterwards awaits that future, which completes strictly after the trailing clean. No wall-clock waits remain in either world, and the pre-fix failure is forced by construction rather than by a tick budget.
… locked Review follow-ups on the unlocked-clean fix: chain_updates now snapshots and cleans in one step before awaiting delta resolution, so a concurrent write landing mid-resolution stays dirty for the next harvest even if a caller ever passes an unlocked root -- the protection lives in the primitive, not only in the caller's gate. A background handler that never enters async-with-self used to get its only delta from the removed trailing flush (uncached computed vars refreshed on every background tick). Restore that flush, under the state lock, detected via a proxy subclass that records context entry. Also move a function-level asyncio import to the module top.
…locked-trailing-clean # Conflicts: # tests/units/reflex_base/event/processor/test_base_state_processor.py
The reorder broke SharedState fan-out (test_linked_state, all memory legs): resolving the delta is itself what re-marks linked vars dirty, through the patch machinery that async computed vars reach via get_state, and SharedState._clean captures its dirty vars for the cross-client fan-out at clean time. Cleaning before resolution captured the pre-resolution set, so the seed lost the linked writes and other clients never saw them. Post-snapshot dirt from that machinery is indistinguishable from a concurrent foreground write at clean time, so there is no cheap selective clean either; hardening the primitive needs the fan-out capture decoupled from _clean, left as follow-up. The background-branch gate and the locked trailing flush, which fix the observed lost-update bug, are unchanged. The chain_updates ordering dependency is now documented at the clean site.
|
Two competing RFC drafts now sit on top of this branch, sketching how the delta-flush path could be restructured so the invariant this PR guards by convention becomes structural:
They compose, but each stands alone. Both are drafts for discussion, based on this branch so the diffs read as deltas. 🤖 Addressed by Claude Code |
Review feedback: the closure was redefined on every process_event call just to capture two locals. A module-level function saves the redefinition, is directly testable, and is reusable wherever a caller needs to know whether a StateProxy currently holds the lock. Still evaluated per yield, since a generator can move between inside and outside its proxy context.
Review feedback: the enter-tracking subclass existed only to record whether the handler ever entered async-with-self, which is a fact about the proxy. Track it natively on StateProxy and drop the subclass. The flag is set at the top of __aenter__ so the parent-delegation branch records entry too.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="reflex/istate/proxy.py">
<violation number="1" location="reflex/istate/proxy.py:101">
P2: A state field named `_self_entered_context` is silently shadowed by this new proxy attribute, so background handlers read or update the bookkeeping flag instead of the user state. Reserve this name explicitly with a clear error, or store the marker outside the wrapped state-attribute namespace.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…nter Review feedback: setting the flag at the top of __aenter__ meant a failed enter, swallowed by the handler, would skip the locked fallback flush and strand preamble dirty vars like router_data. Entered now means a context actually opened, whose exit will flush; both branches set it after their enter succeeds, and the failure path is pinned by a test.
The bug
After dropping the state lock, the background branch of
_execute_eventpassesroot_stateintoprocess_event, whose trailingchain_updatesdoes three things on that root with no lock held:get_delta()snapshots the dirty vars (sync)_get_resolved_delta()/emit_delta()suspend (async computed vars, socket write)finally: root_state._clean()wipes all dirty varsOn a shared state tree (opportunistic locking, or
StateManagerMemory) that window races whatever event holds the lock now. A foreground handler's write landing between the snapshot and the clean is discarded before any delta harvests it. The value reaches no delta, ever: the handler completes normally, its delta arrives carrying only always-dirty computed vars, and the frontend keeps the stale value.Writes made after the window survive, so a single handler can lose its early assignments and deliver its late ones, which makes the failure look impossible from the app side.
How it was found
A merge-queue flake in a downstream app: clicking a card 5-7 ms after a 1 Hz background poll silently did nothing, roughly 1 in 8 such clicks. The click's handler ran, its
selected_integrationassignment was wiped mid-handler by the poll's trailing clean, and the panel never opened. The same mechanism can wipe any state var, including auth state.The fix
Pass
root_state=Nonein the background branch.chain_updatesalready treatsNoneas "route events only, no delta work", so the unlocked snapshot/clean simply stops existing. A background task's own state changes are emitted and cleaned by itsasync with selfcontext exits, which re-acquire the lock, so nothing legitimate is lost. Yielded and returned events still route exactly as before.Validation
REFLEX_OPLOCK_ENABLEDgoes from 13/100 lost foreground deltas to 0/100 with this fix. The oplock-off control loses 0/100 either way.tests/units/test_state.py,tests/units/reflex_base/,tests/units/test_app.pyproduce a failure set identical to the pre-fix baseline on this checkout.Standalone repro (redis on localhost:6399)
Notes for reviewers
async with selfstill flush ahead of the yielded event (the proxy holds the lock there); proxy context exits emit as documented; and a handler that never entersasync with selfgets one trailing flush under the state lock, preserving the old per-tick refresh of uncached computed vars. The only removed behavior is the unlocked flush itself.chain_updateswas attempted and reverted (it broketest_linked_stateon every in-memory leg): delta resolution itself re-marks linked vars dirty through the SharedState patch machinery, andSharedState._cleancaptures its dirty vars for the cross-client fan-out at clean time, so the clean must stay after resolution. That ordering dependency is now documented at the clean site; decoupling the fan-out capture from_cleanso the primitive can be hardened is left as follow-up.cache=Falsevar permanently mutates the sharedState._always_dirty_substates(the test discards its entry in afinally, mirroringreload_state_module), andtest_router_var_depleaks arouteredge into class-levelState._var_dependenciesthat breaks any later test whose events assign router state (the new test pre-seedsrouter_datainstead of carrying it on events). Happy to file these separately.reflex_enterprise.utils.call_event_from_computed_varhas the same defect class (a detachedstate._clean()outside any lock); that is a separate repo and a separate fix.