Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/frontend_visualqa/mcp_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@
_server_browser_config: BrowserConfig | None = None
_config_frozen = False

# asyncio only holds a *weak* reference to a scheduled Task; with no other
# strong reference, a fire-and-forget create_task() can be garbage-collected
# before it finishes closing the runner's browser session. Holding a strong
# ref here — cleared via a completion callback — is the same pattern used by
# navigator_client.py's _schedule_close for the identical hazard.
_pending_close_tasks: set[asyncio.Task[None]] = set()


def get_mcp_server() -> FastMCP:
"""Return the configured FastMCP server instance."""
Expand Down Expand Up @@ -140,7 +147,9 @@ def close_runners_sync() -> None:
return

if runners:
loop.create_task(_close_detached_runners(runners))
task = loop.create_task(_close_detached_runners(runners))
_pending_close_tasks.add(task)
task.add_done_callback(_pending_close_tasks.discard)


@mcp.tool(
Expand Down
37 changes: 37 additions & 0 deletions tests/test_mcp_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,43 @@ async def test_close_runners_sync_resets_state_immediately_inside_running_loop(
assert fake_runner.close_calls == 1


@pytest.mark.asyncio
async def test_close_runners_sync_holds_strong_reference_until_task_completes(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""Regression guard: a bare ``loop.create_task(...)`` with no retained reference is
only weakly held by the loop and can be garbage-collected before the runner
finishes closing, silently dropping browser cleanup. ``close_runners_sync`` must
keep the task alive in ``_pending_close_tasks`` until it finishes, then release it
(mirroring ``navigator_client._schedule_close``'s fix for the identical hazard).
"""
module = _import_mcp_server_module()
release = asyncio.Event()
closed: list[bool] = []

class SlowClosingRunner:
async def close(self) -> None:
await release.wait()
closed.append(True)

_reset_server_module_state(module)
monkeypatch.setitem(module._runners_by_loop, module._loop_key(), SlowClosingRunner())
module._server_browser_config = _persistent_browser_config()
module._config_frozen = True

module.close_runners_sync()
await asyncio.sleep(0) # let the task start and reach `await release.wait()`

assert len(module._pending_close_tasks) == 1
pending_task = next(iter(module._pending_close_tasks))

release.set()
await pending_task # wait for the retained task itself, not a GC-prone proxy

assert closed == [True]
assert module._pending_close_tasks == set()


def test_run_stdio_server_runs_then_closes_runners() -> None:
module = _import_mcp_server_module()
calls: list[str] = []
Expand Down