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 2 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 stale
Confidence Score: 4/5Safe to merge for the targeted bug; the supersession logic is sound and well-tested, but event_processor.py carries pre-existing edge cases around exception-handler task re-use that this PR partially exposes and that remain unresolved. The core fix is correctly implemented and the tests exercise the key paths well. The _try_clean_future deferred-cleanup guard and the _superseded ownership invariant are subtle but correct on careful trace. The remaining concern is the enqueue parent-future guard, which only checks one terminal state and could raise RuntimeError via add_child when a parent future reaches a different terminal state through the exception-handler task path. event_processor.py — the _try_clean_future deferred-cleanup guard and the enqueue parent-future terminal-state check warrant a second look before landing. Important Files Changed
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if parent_future is not None: | ||
| if parent_future.cancelled(): | ||
| # The chain this event belongs to was cancelled; the event is | ||
| # stillborn and never enters the queue. | ||
| tracked.cancel() | ||
| return tracked | ||
| parent_future.add_child(tracked) |
There was a problem hiding this comment.
cancelled(), not all terminal states
The new early-return guards against parent_future.cancelled(), but add_child raises RuntimeError if the parent is done() in any state. A parent future can be done-with-exception (not cancelled) when the backend_exception_handler creates a replacement task reusing the same ev_ctx.txid: by that point set_exception has already been called on the parent future, so parent_future.done() is True and parent_future.cancelled() is False. If that exception-handler task then calls ctx.enqueue(...), add_child raises RuntimeError. Using parent_future.done() instead of parent_future.cancelled() would make the guard consistent with what add_child actually checks.
There was a problem hiding this comment.
i think this is fine actually. if the parent_future reached a done state by some other means including exception, that doesn't affect the scheduling of chained events.
There was a problem hiding this comment.
You're right — I was conflating two different scenarios. The cancelled() check is intentionally specific to the supersession path: only an explicitly cancelled chain should suppress its children as stillborn. A parent that's done() with an exception represents a different error path, and any RuntimeError from add_child in that case would be a pre-existing invariant violation unrelated to this PR's logic. The narrow cancelled() guard is correct. Sorry for the noise.
| 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
| if parent_future is not None: | ||
| if parent_future.cancelled(): | ||
| # The chain this event belongs to was cancelled; the event is | ||
| # stillborn and never enters the queue. | ||
| tracked.cancel() | ||
| return tracked | ||
| parent_future.add_child(tracked) |
There was a problem hiding this comment.
i think this is fine actually. if the parent_future reached a done state by some other means including exception, that doesn't affect the scheduling of chained events.
| 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?
| 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?
| # 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
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: