fix(mcp_server): hold a strong reference to close_runners_sync's background close task

[dhruvbatra]

What

close_runners_sync()'s in-running-loop branch fires
loop.create_task(_close_detached_runners(runners)) and discards the
returned Task, with no other strong reference kept anywhere.

Why

asyncio only holds a weak reference to a scheduled Task; with no other
strong reference, the task can be garbage-collected before it finishes —
silently dropping the browser-session cleanup this shutdown path exists to
guarantee. This is the exact fire-and-forget hazard PR #300 fixed for
navigator_client.py's _schedule_close (same underlying asyncio pitfall,
same docstring warning). This code path is exercised whenever
close_runners_sync() runs while a loop is already active — covered by the
existing test_close_runners_sync_resets_state_immediately_inside_running_loop
test, which only asserts on state reset, not on the task surviving to
completion.

Fix

Apply the identical pattern PR #300 established: retain the task in a
module-level _pending_close_tasks set and drop it via a completion
callback (task.add_done_callback(_pending_close_tasks.discard)).

Why safe

• Single file touched for the behavior change (mcp_server.py), plus a
  regression test in tests/test_mcp_server.py mirroring the one added for
  _schedule_close in PR fix(navigator_client): hold a strong reference to scheduled close() background tasks #300 (asserts the task lands in
  _pending_close_tasks, then completes and clears out of it once its
  awaited close() finishes).
• No functional/API change — same runners get closed, same control flow;
  only the task's GC-eligibility window changes (now waits for completion
  instead of being collectible immediately).
• uv run ruff check src/ tests/ and uv run pytest tests/ pass with the
  same known baseline (324 passed = 323 pre-existing + 1 new test, same 21
  pre-existing Chromium-sandbox environment failures, unchanged from
  unmodified main). uv run ruff format --check flags the same 7
  pre-existing unrelated files on main before this change; no newly
  touched file is among them.

---

Generated by Claude Code

---

Note

Low Risk
Shutdown-only lifecycle fix with no API or behavior change beyond ensuring async close tasks complete; low risk aside from edge cases during MCP server exit.

Overview
Fixes a shutdown race where close_runners_sync() could schedule _close_detached_runners with loop.create_task(...) and drop the Task, allowing asyncio to garbage-collect it before Playwright/browser cleanup finishes.

The change adds a module-level _pending_close_tasks set (same approach as navigator_client._schedule_close) and registers each close task with add_done_callback so the reference is held until completion, then removed.

A new async regression test uses a slow close() to assert the task stays in _pending_close_tasks until it finishes and that runner cleanup actually runs.

^{Reviewed by Cursor Bugbot for commit 5fb362c. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• Bug Fixes

  • Improved shutdown handling to ensure asynchronous cleanup tasks complete reliably.
  • Prevented runner resources from being closed prematurely during shutdown.

• Tests

  • Added regression coverage verifying cleanup tasks remain tracked until completion.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 85ed7295-d513-4e44-b5d6-8967031dc451

📥 Commits

Reviewing files that changed from the base of the PR and between 943e0b9 and 5fb362c.

📒 Files selected for processing (2)

• src/frontend_visualqa/mcp_server.py
• tests/test_mcp_server.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The runner shutdown path now retains asynchronous close tasks until they finish. A regression test verifies task retention during blocked cleanup and removal after completion.

Changes

Runner shutdown lifecycle

Layer / File(s)	Summary
Track and validate asynchronous close tasks src/frontend_visualqa/mcp_server.py, tests/test_mcp_server.py	close_runners_sync stores pending close tasks and removes them through a completion callback. The regression test verifies retention and cleanup.
Estimated code review effort: 2 (Simple)	~10 minutes

Merge Risk: ⚪ Minimal · up to 5fb36

This localized shutdown fix keeps browser-session cleanup running to completion without changing the public API or normal control flow; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: retaining a strong reference to the background close task in close_runners_sync.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/admiring-hawking-7ztmew

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}