-
Notifications
You must be signed in to change notification settings - Fork 23
fix(eventbus): only drain events in the waiting chain, not all buses (#5509) #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Ethanz11-creat
wants to merge
3
commits into
browser-use:main
Choose a base branch
from
Ethanz11-creat:fix/5509-cross-loop-contamination
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
50bc193
fix(eventbus): only drain events in the waiting chain, not all buses …
Ethanz11-creat 6aabb68
fix(eventbus): refine drain scoping per review feedback
Ethanz11-creat 6ef3c3e
refactor(eventbus): dedupe event lookup and snapshot buses once per d…
Ethanz11-creat File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| """ | ||
| Reproduction for issue #5509: cross-loop contamination in the EventBus drain loop. | ||
|
|
||
| Scenario: two EventBus instances running in parallel (like two concurrent | ||
| agent sessions in browser-use). A handler chain on bus A (parent -> child -> | ||
| grandchild) enters the inner `__await__` drain loop, which iterates over | ||
| `EventBus.all_instances` and processes queued events from ALL buses. | ||
|
|
||
| Before the fix, a bus B event queued during that window is processed by the | ||
| drain loop — i.e. INSIDE bus A's handler context (while bus A holds the global | ||
| lock and bus B's own run loop is starved). This cross-loop execution steals | ||
| bus B's scheduling, and with many parallel sessions it is what drives the | ||
| EventBus capacity errors reported in the issue. | ||
|
|
||
| After the fix, the drain loop only processes events belonging to its own | ||
| waiting chain; bus B's independent event stays queued and is handled by bus | ||
| B's own run loop once the lock is released. | ||
| """ | ||
|
|
||
| import asyncio | ||
|
|
||
| import pytest | ||
|
|
||
| from bubus import BaseEvent, EventBus | ||
|
|
||
|
|
||
| class ParentAEvent(BaseEvent[str]): | ||
| message: str | ||
|
|
||
|
|
||
| class ChildAEvent(BaseEvent[str]): | ||
| data: str | ||
|
|
||
|
|
||
| class GrandchildAEvent(BaseEvent[str]): | ||
| value: int | ||
|
|
||
|
|
||
| class BusBEvent(BaseEvent[str]): | ||
| """Independent event that should only be handled on bus B.""" | ||
|
|
||
| payload: str | ||
|
|
||
|
|
||
| class StartEvent(BaseEvent[str]): | ||
| """Handler-less event used only to auto-start a bus's run loop.""" | ||
|
|
||
| data: str | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| async def buses(): | ||
| """Two isolated buses simulating parallel sessions.""" | ||
| # Create bus_b first so it appears before bus_a in the | ||
| # `EventBus.all_instances` iteration order (WeakSet keeps insertion | ||
| # order); the drain loop scans buses in that order, so bus B's queued | ||
| # event is hit before the drain completes its own chain and breaks. | ||
| bus_b = EventBus(name="bus_b") | ||
| bus_a = EventBus(name="bus_a") | ||
| # First dispatch auto-starts each bus's run loop (and creates event_queue). | ||
| bus_a.dispatch(StartEvent(data="__start__")) | ||
| bus_b.dispatch(StartEvent(data="__start__")) | ||
| await asyncio.gather( | ||
| bus_a.wait_until_idle(timeout=5), | ||
| bus_b.wait_until_idle(timeout=5), | ||
| ) | ||
| yield bus_a, bus_b | ||
| await bus_a.stop(clear=True) | ||
| await bus_b.stop(clear=True) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_bus_b_event_not_processed_inside_bus_a_drain(buses): | ||
| bus_a, bus_b = buses | ||
|
|
||
| ready = asyncio.Event() | ||
| proceed = asyncio.Event() | ||
| order: list[str] = [] | ||
|
|
||
| async def grandchild_handler(event: GrandchildAEvent) -> int: | ||
| await asyncio.sleep(0.5) | ||
| return event.value | ||
|
|
||
| async def child_handler(event: ChildAEvent) -> str: | ||
| # Tell the test the chain is armed; wait until bus B's independent | ||
| # event is queued BEFORE forwarding the grandchild, so bus B's queue | ||
| # holds [independent event, grandchild] when the inner drain loop | ||
| # scans it — the drain must skip the unrelated event and still | ||
| # process the grandchild. | ||
| ready.set() | ||
| await proceed.wait() | ||
| order.append("a_drain_enter") | ||
| grandchild = GrandchildAEvent(value=1) | ||
| bus_b.dispatch(grandchild) | ||
| await grandchild | ||
| order.append("a_drain_exit") | ||
| return "child_handled" | ||
|
|
||
| async def handler_a(event: ParentAEvent) -> str: | ||
| child = ChildAEvent(data=f"child_of_{event.message}") | ||
| bus_a.dispatch(child) | ||
| await child | ||
| return "a_handled" | ||
|
|
||
| async def handler_b(event: BusBEvent) -> str: | ||
| order.append("b_handled") | ||
| return "b_handled" | ||
|
|
||
| bus_a.on("GrandchildAEvent", grandchild_handler) | ||
| bus_a.on("ChildAEvent", child_handler) | ||
| bus_a.on("ParentAEvent", handler_a) | ||
| bus_b.on("BusBEvent", handler_b) | ||
|
|
||
| # Start bus A's event chain; wait until its handler holds the global lock | ||
| # (child_handler is parked on `proceed` inside bus A's run loop). | ||
| bus_a.dispatch(ParentAEvent(message="trigger")) | ||
| await asyncio.wait_for(ready.wait(), timeout=5) | ||
|
|
||
| # Dispatch an event on bus B: its run loop `get()`s it (outside the lock) | ||
| # and then blocks acquiring the global lock held by bus A — so bus B's | ||
| # consumer is parked and will not `get()` again until the lock is free. | ||
| bus_b.dispatch(BusBEvent(payload="first")) | ||
| await asyncio.sleep(0.05) | ||
|
|
||
| # Queue a SECOND independent event on bus B (directly into its queue): | ||
| # with bus B's run loop parked on the lock, this event is only reachable | ||
| # by bus A's drain loop — the cross-loop window (issue #5509). | ||
| bus_b.event_queue.put_nowait(BusBEvent(payload="second")) | ||
| proceed.set() | ||
|
|
||
| await asyncio.gather( | ||
| bus_a.wait_until_idle(timeout=5), | ||
| bus_b.wait_until_idle(timeout=5), | ||
| ) | ||
|
|
||
| # Bus B's events must NOT be processed while bus A's drain loop is running | ||
| # (inside bus A's handler context, holding the global lock): that is the | ||
| # cross-loop contamination. Both events must be handled afterwards by bus | ||
| # B's own run loop, once the lock is released. | ||
| assert order[:2] == ["a_drain_enter", "a_drain_exit"], f"drain markers missing: {order!r}" | ||
| assert order[2:] == ["b_handled", "b_handled"], ( | ||
| f"cross-loop contamination: bus B events were processed inside bus A's " | ||
| f"drain loop (order was {order!r}, expected both bus B events after the drain)" | ||
| ) |
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: The lookup snapshot is frozen once, but the drain loop below still re-enumerates
list(EventBus.all_instances)on every iteration (for bus in list(EventBus.all_instances)). A bus created mid-drain is iterated by the drain but is invisible to_find_event_by_id, which only scansbuses_snapshot. Because the snapshot was taken before those buses existed, any of their queued events that belong to this waiting chain getparent_candidate = Noneduring the descendant walk and are never drained, while unrelated events on them are scanned in the loop — so the drain's behavior silently depends on when the snapshot happened and contradicts the stated "snapshot once per drain" intent.Prompt for AI agents