Repository navigation
Fix awaiting an event handler from a background task: caught exceptions and finished superseding runs - #7907
FarhanAliRaza wants to merge 2 commits into
Conversation
An exception raised by a handler awaited from a background task (no state lock held, so the event is enqueued) was delivered to the awaiting task and also reported to the backend exception handler, showing the error toast even when the task caught it. The enqueued event's future is now marked as awaited, and its exception is left to the caller: if the task does not catch it, the task's own failure reports it once. A supersedes=True handler run that had returned, but whose task done callback had not yet resolved its future, was cancelled by a newer invocation enqueued in that window, so the awaiting task got CancelledError for a run that completed. The in-memory state manager hits the window because a task waiting for the state lock resumes without suspending before it enqueues. Superseding such a run now only cancels the events it chained. Closes #7839 Claude-Session: https://claude.ai/code/session_01LwJUCU8RtEco15mQvkxdQb
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/units/reflex_base/event/processor/test_base_state_processor.py">
<violation number="1" location="tests/units/reflex_base/event/processor/test_base_state_processor.py:5704">
P2: This fixed sleep does not guarantee that the second background run has reached the superseding enqueue, so the test can pass without exercising the race it claims to cover. Synchronize on an event set by the second run immediately before it awaits `search`, or use another deterministic barrier instead of a timed sleep.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| token, Event.from_event_type(BgSearchState.run("second"))[0] | ||
| ) | ||
| # Let the second task start and queue for the lock the search holds. | ||
| await asyncio.sleep(0.1) |
There was a problem hiding this comment.
P2: This fixed sleep does not guarantee that the second background run has reached the superseding enqueue, so the test can pass without exercising the race it claims to cover. Synchronize on an event set by the second run immediately before it awaits search, or use another deterministic barrier instead of a timed sleep.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/units/reflex_base/event/processor/test_base_state_processor.py, line 5704:
<comment>This fixed sleep does not guarantee that the second background run has reached the superseding enqueue, so the test can pass without exercising the race it claims to cover. Synchronize on an event set by the second run immediately before it awaits `search`, or use another deterministic barrier instead of a timed sleep.</comment>
<file context>
@@ -5582,3 +5582,131 @@ def ping(self):
+ token, Event.from_event_type(BgSearchState.run("second"))[0]
+ )
+ # Let the second task start and queue for the lock the search holds.
+ await asyncio.sleep(0.1)
+ release.set()
+ await asyncio.wait_for(
</file context>
[Medium impact] The PR appears safe to merge; the previous result-loss issue is fixed. SummaryAwaited handlers now leave caught errors to their callers and keep results from runs that finished before being replaced.
Reviews (2) · Last reviewed commit: "Resolve a finished event's future before..." · Reviewed by Greptile |
The next queued handler for a token starts eagerly on Python 3.12+, and could supersede the run that just finished while its future was still unresolved and its task already forgotten, cancelling a completed run. Claude-Session: https://claude.ai/code/session_01LwJUCU8RtEco15mQvkxdQb
Merging this PR will not alter performance1 warning
Comparing Footnotes
|
All Submissions:
Type of change
Changes To Core Features:
Problem
Awaiting an event handler from a background task with no state lock held enqueues the event and awaits its future. Two things differed from the inline await (#7839), both reproduced on
main:_resolve_futureset the exception on the future (so the awaiting task received it) and then also ran the backend exception handler, so the user got the error toast even though the task caught the exception. If the task did not catch it, it was reported twice: once for the awaited event and once for the background task.supersedes=Truerun was reported as cancelled._cancel_older_chainscancelled the previous invocation's future whenever it was not done. A handler task that has returned resolves its future in a done callback, one loop iteration later. A newer invocation enqueued in that window cancelled the future of a run that had completed and committed its writes. The in-memory state manager hits the window every time: the second background task is waiting for the state lock the first search holds, and once it gets the lock it reachesenqueuewithout suspending, ahead of the first task's done callback. Redis suspends on the way, so the future is already resolved.Fix
EventFuturegets anawaitedflag, set byexecute_event_inlineon the future it is about to await. When_resolve_futuredelivers an exception to an awaited future, it stops there: no telemetry, no backend exception handler. An exception the caller does not catch fails the background task, whose own failure path reports it once._cancel_older_chainsleaves alone a future whose task is already done and only cancels the events that run chained; the done callback then resolves the future with the run's result (or exception). A run that is still in progress is cancelled as before.Not changed
Tests
tests/units/reflex_base/event/processor/test_base_state_processor.pytest_exception_of_handler_awaited_from_background_is_the_callers: caught → backend handler not called; uncaught → called exactly once.test_superseding_handler_awaited_from_background_returns_finished_result(memory and Redis managers): both tasks get their results.tests/units/reflex_base/event/processor/test_event_processor.pytest_superseding_event_keeps_the_result_of_a_finished_run: deterministic version of the window (enqueue between the handler returning and its done callback); the finished run resolves with its result and its chained event is cancelled.tests/integration/tests_playwright/test_background_task.pytest_await_failing_handler_from_background_taskin the existing app: no toast when the background task catches the exception.All four fail on
main(except the Redis parametrization, which is the reference behaviour) and pass with the fix.Run locally:
uv run pytest tests/units/reflex_base/event tests/units/test_state.py tests/units/istate -q— passesPLAYWRIGHT_BROWSERS_PATH=... uv run pytest tests/integration/tests_playwright/test_background_task.py -q— 8 passeduv run ruff check .,uv run ruff format .— cleanuv run pyrighton the changed files — 0 errorscloses #7839
https://claude.ai/code/session_01LwJUCU8RtEco15mQvkxdQb
Generated by Claude Code