ENG-9661 fix: cancel stale on_load chains via handler-declared supersession#6713
ENG-9661 fix: cancel stale on_load chains via handler-declared supersession#6713FarhanAliRaza wants to merge 5 commits into
Conversation
Navigating away while a page's on_load chain is still running left the stale chain executing, blocking the new page's events behind it and applying its late deltas (reflex-dev#6593). Add a SUPERSEDES_MARKER for event handlers with latest-wins semantics: enqueuing a new chain-root invocation cancels the previous unfinished event chain rooted at the same handler for the same client token. Mark on_load_internal with it so a newer navigation supersedes the previous page's unfinished load. The EventProcessor tracks the active chain root per (event name, token), drops events chained from an already-cancelled parent before they enter the queue, and defers future cleanup while the handler task is still unwinding so late-chained events can find their cancelled parent.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2db865caa9
ℹ️ 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".
| if future.txid in self._tasks: | ||
| # The handler task is still running or unwinding; keep the future | ||
| # so late-chained events can find their (possibly cancelled) parent. | ||
| return |
There was a problem hiding this comment.
When a state handler raises and BaseStateEventProcessor starts backend_exception_handler, _finish_task stores that exception-handler task in _tasks under the same txid after setting the original future's exception. This new guard keeps the already-done failed future in _futures, so if the exception handler returns a backend EventSpec as the public API allows, ctx.enqueue() finds that done future as the parent and add_child() raises instead of queuing the recovery event. The retention here is only needed for cancelled tasks unwinding, not for failed futures being handled by the backend exception handler.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes a long-standing navigation bug (#6593) where navigating away from a page while its
Confidence Score: 5/5Safe to merge. The supersession path is additive — handlers without the marker are completely unaffected — and on_load_internal is the only handler currently marked, limiting blast radius to navigation chains. The cancellation cascade correctly handles all observable states of a root future: done-with-result (children still live), cancelled (task unwinding), and fully done. The deferred-cleanup guard in _try_clean_future prevents premature removal while a cancelled task is still unwinding, and the explicit _try_clean_future call at the end of _finish_task ensures cleanup always completes. Three targeted unit tests plus a full integration test in test_state.py exercise the main paths (running chain cancelled, queued chain skipped, resurrection prevented). No files require special attention. All changed files are internally consistent and the new _superseded dict is cleared on shutdown. Important Files Changed
Reviews (3): Last reviewed commit: "add reflex-base news fragment" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async def _drain_superseded(ep: EventProcessor) -> None: | ||
| """Give done callbacks a few ticks to clean the supersession tracking. | ||
|
|
||
| Args: | ||
| ep: The event processor to wait on. | ||
| """ | ||
| for _ in range(20): | ||
| if not ep._superseded: | ||
| return | ||
| await asyncio.sleep(0) |
There was a problem hiding this comment.
_drain_superseded polls rather than waiting on the chain
The helper spins up to 20 event-loop ticks and returns early only if _superseded is empty. It offers no signal if the dict is still populated after those ticks — the test just continues and then asserts ep._superseded == {}. If cleanup ever takes more than ~20 scheduling rounds (e.g., under CI load), the assertion can fail non-deterministically. A more reliable approach would be to yield one final tick after current.wait_all() (already awaited just before the call), or use asyncio.wait_for on a poll with an explicit timeout.
Merging this PR will not alter performance
Comparing 
Footnotes
|
| # The chain this event belongs to was cancelled; the event is | ||
| # stillborn and never enters the queue. |
There was a problem hiding this comment.
| # The chain this event belongs to was cancelled; the event is | |
| # stillborn and never enters the queue. | |
| # The chain this event belongs to was cancelled, so cancel the | |
| # tracker since this event will never enter the queue. |
lets get rid of the potentially trama-linked terminology. "stillborn" could be triggering for some
There was a problem hiding this comment.
Applied the suggested wording.
| self._try_clean_future(future) | ||
| if future is None or future.cancelled(): | ||
| if future is not None: | ||
| self._try_clean_future(future) |
There was a problem hiding this comment.
shouldn't this already hit since we add _try_clean_future as a done callback when we create the future?
There was a problem hiding this comment.
You're right — entries examined here never had a task created, so cleanup is never deferred for them and the done callback always removes the future. Dropped the eager _try_clean_future calls in both dispatch paths.
| if future is not None and future.cancelled(): | ||
| self._try_clean_future(future) | ||
| if future is None or future.cancelled(): | ||
| if future is not None: |
There was a problem hiding this comment.
why did this conditional change structure like this? are these two nested conditionals not equivalent to what we had before?
There was a problem hiding this comment.
They're not equivalent: the old code treated a missing future as "proceed and dispatch". Pre-supersession that was unreachable in practice (nothing cancelled a queued root event), but now supersession cancels entries that are still sitting in the queue, and the cancel's _try_clean_future done callback removes the future from _futures before the entry is dequeued. So future is None has to mean "cancelled, skip" — otherwise a superseded entry whose cleanup callback already ran would execute anyway. The nested conditional is gone now (see next comment).
| # A newer navigation supersedes the previous unfinished on_load chain for the | ||
| # same client token, cancelling its stale work (#6593). | ||
| setattr( | ||
| OnLoadInternalState.event_handlers["on_load_internal"].fn, | ||
| SUPERSEDES_MARKER, | ||
| True, | ||
| ) |
There was a problem hiding this comment.
probably would be nice to expose this as a kwarg on rx.event like background
There was a problem hiding this comment.
Done — added supersedes as a kwarg on rx.event (mirroring background), and on_load_internal is now decorated with @event(supersedes=True) instead of the post-class setattr.
- expose supersedes as a kwarg on rx.event and mark on_load_internal with @event(supersedes=True) instead of post-class setattr - only retain cancelled futures in _futures while their task unwinds, so the backend exception handler task can chain recovery events (with regression test) - drop the redundant eager _try_clean_future calls in the dispatch paths; the future's done callback already handles cleanup - reword cancelled-chain comment - make _drain_superseded fail loudly instead of silently giving up
2 participants
Navigating away while a page's on_load chain is still running left the stale chain executing, blocking the new page's events behind it and applying its late deltas (#6593).
Add a SUPERSEDES_MARKER for event handlers with latest-wins semantics: enqueuing a new chain-root invocation cancels the previous unfinished event chain rooted at the same handler for the same client token. Mark on_load_internal with it so a newer navigation supersedes the previous page's unfinished load.
The EventProcessor tracks the active chain root per (event name, token), drops events chained from an already-cancelled parent before they enter the queue, and defers future cleanup while the handler task is still unwinding so late-chained events can find their cancelled parent.
All Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Changes To Core Features: