refactor: extract shared retain_background_task helper for fire-and-forget tasks

[dhruvbatra]

What

navigator_client.py's _schedule_close (#300) and mcp_server.py's close_runners_sync (#302) each independently fixed the identical unretained-fire-and-forget-task GC hazard, but each one hand-rolled the same "add task to a module-level set, remove it via a done_callback" pattern — their own comments already cross-reference each other as guarding "the identical hazard." overlay.py's _on_navigation guards the same hazard a third time with an instance-level set plus an extra logging callback.

This PR extracts retain_background_task(pending, task) into utils.py — home to this repo's other small shared helpers (resolve_optional_method, elapsed_ms, now_ms) — and rewires all three call sites onto it instead of duplicating the add+discard logic. overlay.py still chains its own extra completion callback after the shared call.

Why it's safe

• Pure extraction: the helper does exactly pending.add(task); task.add_done_callback(pending.discard); return task — byte-identical to what each site did inline.
• Each call site keeps its own _pending_close_tasks / _navigation_tasks set object; only the add+discard mechanics moved, so existing regression tests that assert directly on module._pending_close_tasks (added by fix(navigator_client): hold a strong reference to scheduled close() background tasks #300/fix(mcp_server): hold a strong reference to close_runners_sync's background close task #302) pass unchanged.
• No behavior change: same set, same task, same discard timing.
• Added direct unit tests for the new helper (test_retain_background_task_holds_reference_until_completion, test_retain_background_task_returns_the_same_task).

Verification

• uv run pytest tests/ → 326 passed / 21 failed (the documented pre-existing environment-only Chromium-sandbox failures — sandbox has no Chromium binary; same 21 test names as a git stash-backed baseline run). 326 vs. the baseline's 324 is exactly this PR's 2 new tests.
• uv run ruff check src/ tests/ → clean.
• uv run ruff format --check on all 5 touched files → all already formatted.
• Directly re-ran tests/test_navigator_client.py, tests/test_mcp_server.py, tests/test_overlay.py, tests/test_utils.py → 91 passed.

Only open PR (#148) is unrelated pre-existing human feature work — untouched by this change.

---

Generated by Claude Code

---

Note

Low Risk
Behavior-preserving refactor with no API or shutdown-path logic changes beyond centralizing identical task-retention code.

Overview
Introduces retain_background_task in utils.py to keep a strong reference on fire-and-forget asyncio.Task instances until they finish, avoiding premature GC when only the event loop holds a weak reference.

mcp_server.close_runners_sync, navigator_client._schedule_close, and overlay._on_navigation now call this helper instead of duplicating pending.add(task) plus task.add_done_callback(pending.discard). Each site still uses its own pending set; overlay continues to attach its navigation-restore logging callback on the returned task. Inline comments were shortened to point at the helper docstring.

Adds two unit tests in tests/test_utils.py that assert the task stays in the set until completion and that the same task object is returned.

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

Summary by CodeRabbit

• Reliability Improvements

  • Improved handling of background tasks during asynchronous cleanup and navigation restoration.
  • Helps ensure scheduled tasks remain active until completion and are cleaned up afterward.

• Tests

  • Added coverage validating background-task retention, completion cleanup, and task reuse.

[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: d1bc96a9-639b-450f-aeed-e4d8f9b7b9bc

📥 Commits

Reviewing files that changed from the base of the PR and between 7bc78a2 and e3261d6.

📒 Files selected for processing (5)

• src/frontend_visualqa/mcp_server.py
• src/frontend_visualqa/navigator_client.py
• src/frontend_visualqa/overlay.py
• src/frontend_visualqa/utils.py
• tests/test_utils.py

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

---

📝 Walkthrough

Walkthrough

The change adds retain_background_task to centralize asynchronous task retention and cleanup. The MCP server, navigator client, and overlay use the utility. Tests verify task retention, cleanup, and identity.

Changes

Background task retention

Layer / File(s)	Summary
Retention utility and tests src/frontend_visualqa/utils.py, tests/test_utils.py	Adds retain_background_task, which retains tasks in a pending set, removes them after completion, and returns the original task. Tests cover these behaviors.
Task scheduling integrations src/frontend_visualqa/mcp_server.py, src/frontend_visualqa/navigator_client.py, src/frontend_visualqa/overlay.py	Replaces local task registration and completion callbacks with retain_background_task for shutdown, HTTP-client close, and navigation restore tasks.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to e3261

This PR centralizes existing background-task retention logic without changing call-site behavior or APIs; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 5 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: extracting a shared retain_background_task helper for fire-and-forget tasks.
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.

• Fix all pre-merge checks with AI

✨ 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-eiz9f9

---

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.}