RFC(events): selective clean via a resolution-dirt ledger - #6922
Draft
adhami3310 wants to merge 7 commits into
Draft
RFC(events): selective clean via a resolution-dirt ledger#6922adhami3310 wants to merge 7 commits into
adhami3310 wants to merge 7 commits into
Conversation
This was referenced Aug 20, 2026
adhami3310
changed the base branch from
khaleel/background-unlocked-trailing-clean
to
main
August 20, 2026 21:31
Contributor
Greptile SummaryThis RFC introduces task-local resolution-dirt tracking so asynchronous delta flushes selectively clean the names they snapshotted or dirtied during resolution.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the scope of this follow-up review. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/event/processor/base_state_processor.py | Splits synchronous and asynchronous delta resolution, using snapshot-and-selective-clean behavior for coroutine-bearing deltas. |
| reflex/state.py | Centralizes dirty-var recording and adds the resolution ledger, dirty snapshot, and selective-clean primitives. |
| reflex/istate/shared.py | Extends selective cleaning to preserve SharedState fan-out capture semantics. |
| reflex/istate/proxy.py | Routes mutable-proxy mutations through the centralized dirty-var recorder. |
| tests/units/reflex_base/event/processor/test_base_state_processor.py | Covers preservation and later publication of a different variable written during asynchronous delta resolution. |
| tests/units/test_state.py | Covers delta traversal when a dirty substate name has no attached instance. |
Reviews (8): Last reviewed commit: "RFC(events): selective clean only for fl..." | Re-trigger Greptile
Merging this PR will not alter performance
Comparing Footnotes
|
adhami3310
force-pushed
the
khaleel/rfc-resolution-ledger
branch
from
August 21, 2026 18:08
653c455 to
72333ed
Compare
adhami3310
changed the base branch from
main
to
khaleel/background-unlocked-trailing-clean
August 21, 2026 18:08
The atomic snapshot-clean reverted on this branch failed because _clean() cannot tell two producers of post-snapshot dirt apart: a concurrent writer's update, which must survive, and resolution's own side effects (the SharedState patch machinery reached through async computed vars), which the fan-out capture must publish and then discard. One dirty set made those indistinguishable. Give them separate ledgers. _record_dirty_var is the single choke point for a var becoming dirty; during _resolve_delta a task-local contextvar routes the resolving task tree's marks into a ledger, which concurrent events' tasks do not inherit. chain_updates then cleans exactly what it flushed: the dirty snapshot plus the ledger. The clean still runs after resolution, the SharedState capture is computed from those same sets so the fan-out seed stays complete, and a foreign write landing mid-resolution is in neither set, so it survives. This un-reverts the atomicity regression test and passes tests/integration/test_linked_state.py, the test that forced the revert.
…undary A contextvar lookup per dirty mark showed up as a 4.8% regression on the process_event benchmark. Gate it behind a plain global depth counter: while no flush is resolving, marking dirty costs one int check. Also document the design boundary the review surfaced: dirt is name-only, so a concurrent rewrite of a var already in a flush's snapshot, landing during that flush's resolution window, is cleaned with the snapshot and the newer value waits for its next dirtying. Closing that needs per-var write versions. Unreachable today: every current flush holds the token lock, so nothing can rewrite mid-window.
Redis CI caught the interaction: selective cleaning preserves another writer's dirt across events, set_state pickles the parent's dirty_substates with it, and a later event fetching only the handler's slice restores a dirty name whose substate is not attached. get_delta indexed it unguarded and raised KeyError; _clean has always skipped missing substates for the same reason. The unfetched substate cannot contribute to this delta, and its dirt stays in its own record until an event fetches it.
adhami3310
force-pushed
the
khaleel/rfc-resolution-ledger
branch
from
August 21, 2026 18:30
72333ed to
f871122
Compare
CodSpeed still measured -4.35% after the depth-counter gate: the cost was the unconditional helper call per dirty mark and the per-flush ledger context, not the contextvar lookup the gate removed. Split the mark into a hot half (set-add plus one falsy global check) and a cold recording half reached only while a flush resolves, and open the ledger context only for deltas that contain coroutines, since a coroutine-free resolution never yields the loop and nothing can mark dirt mid-flush.
…olution CodSpeed still measured -3.99%: the per-flush snapshot walk and selective clean ran for every event, including the sync-only ones that dominate real traffic. A coroutine-free resolution never yields the loop, so nothing can interleave before the emit and the machinery records and subtracts nothing; those flushes now take the plain resolve-emit-clean path the pipeline has always had, and the selective path engages exactly where the lost-update window exists: flushes whose resolution suspends. The boundary statement tightens accordingly: for a hypothetical unlocked caller, a sync-delta flush's emit await reverts to full-clean semantics, identical to the shipped pipeline.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RFC / draft for discussion — the second of two competing shapes for hardening the delta-flush path after #6920. Not intended to merge as-is.
#6920 gates the one caller that flushed a shared root without the token lock. An atomic snapshot-and-clean in
chain_updates— protection at the primitive — was attempted there and reverted, because_clean()faced two indistinguishable producers of post-snapshot dirt: a concurrent writer's update (must survive) and resolution's own side effects via the SharedState patch machinery (must be captured for fan-out, then discarded). One dirty set cannot serve both.This RFC separates the ledgers:
_record_dirty_varbecomes the single choke point for "a var became dirty" (__setattr__, computed-var marking,MutableProxy)._resolve_delta, a task-local contextvar routes the resolving task tree's marks into a ledger. Contextvars are inherited by tasks created during resolution but not by concurrent events' tasks, so task-locality is what distinguishes the writers.chain_updatescleans selectively: dirty snapshot ∪ resolution ledger. The clean still runs after resolution;SharedState's fan-out capture is computed from those same sets (including backend vars like_who, which never reach a delta but must fan out); a foreign write mid-resolution is in neither set and survives.The proof obligations from the #6920 revert are both met: the atomicity regression test is un-reverted and passes, and
tests/integration/test_linked_state.py— the test that forced the revert — passes on every leg I can run locally. Suite baseline unchanged; 3.10 and 3.14 green.Known costs and open questions: after three CodSpeed rounds (-4.8% naive, -4.35% with a depth-counter gate, -3.99% with a hot/cold mark split), the machinery now engages only for flushes whose delta contains coroutines — a coroutine-free resolution never yields the loop, so the selective clean protects nothing there and those flushes take the pipeline's original path (for a hypothetical unlocked caller, a sync-delta flush's emit await reverts to full-clean semantics, identical to the shipped pipeline); a name-only boundary the review surfaced: a concurrent rewrite of a var already in a flush's snapshot, landing mid-resolution, is still cleaned with it (unreachable while every flush holds the lock, which all current callers do; closing it for hypothetical unlocked callers needs per-var write versions); the other three flush sites (
app.modify_state,StateProxy.__aexit__, hydrate) still full-clean under their locks, which is safe but could adopt the same shape via a sharedDeltaFlushhelper; and_snapshot_dirty_varswalks the dirty chain once more per flush.Competing RFC: #6921 — instead of making the flush safe, it makes lock ownership a required capability (
LockedRoot), so an unlocked flush is aTypeErrorrather than a race. The two compose: #6921 is the guard rail, this is the reason the rail could eventually come down.