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 1 commit 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 stale
Confidence Score: 3/5The supersession logic is carefully designed and the test suite validates the key paths, but an unchecked edge case in enqueue can raise RuntimeError if a backend exception handler tries to chain events. The new guard in enqueue checks parent_future.cancelled() but EventFuture.add_child raises RuntimeError for any done() state. When backend_exception_handler is set and a handler raises, _finish_task calls set_exception on the parent future then spawns a replacement task reusing the same txid — if that task calls ctx.enqueue, the parent is done-with-exception, cancelled() returns False, the guard is skipped, and add_child raises at runtime. packages/reflex-base/src/reflex_base/event/processor/event_processor.py — specifically the parent_future guard in enqueue and its interaction with the backend_exception_handler task path in _finish_task. Important Files Changed
Reviews (1): Last reviewed commit: "fix: cancel stale on_load chains via han..." | 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.
| setattr( | ||
| OnLoadInternalState.event_handlers["on_load_internal"].fn, | ||
| SUPERSEDES_MARKER, | ||
| True, | ||
| ) |
There was a problem hiding this comment.
setattr with a string key to mark on_load_internal post-class-definition will raise an unguarded KeyError at import time if the method is ever renamed. The existing BACKGROUND_TASK_MARKER is set before class creation via a decorator; it would be safer to give on_load_internal the same treatment, or at least guard with a .get() check.
| setattr( | |
| OnLoadInternalState.event_handlers["on_load_internal"].fn, | |
| SUPERSEDES_MARKER, | |
| True, | |
| ) | |
| _on_load_internal_handler = OnLoadInternalState.event_handlers.get( | |
| "on_load_internal" | |
| ) | |
| if _on_load_internal_handler is not None: | |
| setattr(_on_load_internal_handler.fn, SUPERSEDES_MARKER, True) | |
| else: | |
| raise RuntimeError( | |
| "on_load_internal handler not found; " | |
| "SUPERSEDES_MARKER could not be applied." | |
| ) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| 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.
1 participant
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: