From eb4e95523ad78461d8c1fbf423353d0c452a45a3 Mon Sep 17 00:00:00 2001 From: wenxind Date: Wed, 10 Jun 2026 16:37:15 +0000 Subject: [PATCH] fix(render-mcp): close LOVR launch context on respawn (no leaked tasks/handles) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit render-mcp parked every LOVR launch in the process-lifetime AsyncExitStack, so on each respawn the previous ManagedProcess context (its two _forward pipe tasks + open log-file handle) was never torn down until whole-process shutdown — N restarts leaked N-1 dead contexts. Each launch now gets its own AsyncExitStack, closed inside _watch as soon as the child exits and before a respawn is allowed. A single _aclose_live_launch callback registered once on the app-lifetime stack covers the shutdown-while-LOVR-running case, so nothing accumulates per launch. The teardown is safe post-exit: ManagedProcess skips terminate when returncode is already set and just cancels the pipe tasks and closes the sink. Adds a regression test (local/gpu-marked) asserting the previous launch context is closed on respawn and the live one is closed at shutdown. Fixes #196. Co-Authored-By: Claude Opus 4.8 --- .../render-mcp/render_mcp/__main__.py | 33 +++++++- docs/changelog.md | 11 +++ tests/test_local_render_mcp.py | 84 ++++++++++++++++++- 3 files changed, 126 insertions(+), 2 deletions(-) diff --git a/agent-mcp-servers/render-mcp/render_mcp/__main__.py b/agent-mcp-servers/render-mcp/render_mcp/__main__.py index 99c2965f..ec252d49 100644 --- a/agent-mcp-servers/render-mcp/render_mcp/__main__.py +++ b/agent-mcp-servers/render-mcp/render_mcp/__main__.py @@ -177,6 +177,15 @@ def __init__(self, cfg: Config, stack: contextlib.AsyncExitStack) -> None: self._render_drops: int = 0 self._spawn_error: str | None = None self._watch_task: asyncio.Task | None = None + # Per-launch context for the current LOVR child. Each launch gets its + # own AsyncExitStack so its ManagedProcess teardown (pipe-task cancel + + # log-sink close) can run on respawn instead of piling up in the + # app-lifetime stack until whole-process shutdown (issue #196). + self._launch_stack: contextlib.AsyncExitStack | None = None + # Safety net: close whatever launch context is live when the server + # shuts down (LOVR still running). Registered once on the app-lifetime + # stack so it never accumulates per launch. + self._stack.push_async_callback(self._aclose_live_launch) # Scene state: { id → { type, position, color, scale } } self._objects: dict[str, dict] = {} @@ -216,9 +225,11 @@ async def start_lovr_once(self) -> dict: logger.info( "render-mcp: starting LOVR bin={} app={}", cfg.lovr_bin, cfg.xr_app_dir, ) - lovr_proc = await self._stack.enter_async_context( + launch_stack = contextlib.AsyncExitStack() + lovr_proc = await launch_stack.enter_async_context( ManagedProcess("lovr", lovr_cmd, cwd=cfg.xr_app_dir) ) + self._launch_stack = launch_stack async def _watch() -> None: rc = await lovr_proc.wait() @@ -226,6 +237,14 @@ async def _watch() -> None: "render-mcp: LOVR child exited (rc={}) — " "resetting lovr_started so next start_xr respawns it", rc, ) + # Tear down this launch's context (cancel the two pipe-forward + # tasks, close the log sink; terminate is a no-op since the + # child already exited) BEFORE allowing a respawn, so dead + # contexts don't accumulate in the app-lifetime stack (#196). + with contextlib.suppress(Exception): + await launch_stack.aclose() + if self._launch_stack is launch_stack: + self._launch_stack = None self._lovr_started = False self._watch_task = asyncio.create_task(_watch(), name="lovr-watch") @@ -326,6 +345,18 @@ async def forward(self, op: str, value: Any) -> dict: except zmq.Again: return {"ok": False, "reason": "backpressure"} + async def _aclose_live_launch(self) -> None: + """Close the current launch context, if any. + + Registered once on the app-lifetime stack so a LOVR child still running + at server shutdown is torn down. On a normal LOVR exit ``_watch`` has + already closed and cleared the launch stack, so this is a no-op. + """ + launch_stack, self._launch_stack = self._launch_stack, None + if launch_stack is not None: + with contextlib.suppress(Exception): + await launch_stack.aclose() + def close(self) -> None: if self._watch_task is not None and not self._watch_task.done(): self._watch_task.cancel() diff --git a/docs/changelog.md b/docs/changelog.md index 538ee04f..020b8182 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -9,6 +9,17 @@ Significant decisions, in reverse-chronological order. Update this whenever a non-trivial architectural or design decision is made so the rationale is preserved and not re-litigated. +### 2026-06-09 — render-mcp: per-launch context for LOVR respawn (no leaked pipe tasks / log handles) + +`render-mcp` parked every LOVR launch's `ManagedProcess` in the +process-lifetime `AsyncExitStack`, so on each respawn the previous context's +teardown (its two `_forward` pipe tasks + open log-file handle) never ran until +whole-process shutdown — N restarts leaked N-1 dead contexts. Each launch now +gets its own `AsyncExitStack`, closed inside `_watch` as soon as the child +exits (before a respawn is allowed); a single `_aclose_live_launch` callback on +the app-lifetime stack covers the shutdown-while-LOVR-running case, so it never +accumulates per launch. Fixes #196. + ### 2026-06-09 — Ctrl-C during startup tears down everything, incl. persist + docker containers Pressing Ctrl-C while `model-servers` was launching (slow image pull / weight diff --git a/tests/test_local_render_mcp.py b/tests/test_local_render_mcp.py index a19b3a38..2d917286 100644 --- a/tests/test_local_render_mcp.py +++ b/tests/test_local_render_mcp.py @@ -312,7 +312,7 @@ async def test_get_health_and_scene_state(self, dispatcher): class _FakeLovrProc: - """Stand-in for ManagedProcess that never exits until cancelled.""" + """Stand-in for ManagedProcess that exits only when told (or never).""" def __init__(self) -> None: self._done = asyncio.Event() @@ -320,15 +320,23 @@ async def wait(self) -> int: await self._done.wait() return 0 + def trigger_exit(self) -> None: + """Simulate the LOVR child exiting, unblocking ``wait()``.""" + self._done.set() + class _FakeManagedProcessCtx: def __init__(self) -> None: self.proc = _FakeLovrProc() + # Records whether this launch's context was torn down — the leak in + # issue #196 is exactly that this never ran until process shutdown. + self.exited = False async def __aenter__(self) -> _FakeLovrProc: return self.proc async def __aexit__(self, *exc) -> None: + self.exited = True return None @@ -372,3 +380,77 @@ async def test_close_cancels_lovr_watch_task(tmp_path: Path, monkeypatch): assert disp._watch_task.done() finally: await stack.__aexit__(None, None, None) + + +@_asyncio +async def test_lovr_respawn_closes_previous_launch_context(tmp_path: Path, monkeypatch): + """Issue #196: each LOVR launch's ManagedProcess context must be torn down + on respawn instead of accumulating in the app-lifetime stack. + + Without the fix the previous context's ``__aexit__`` (pipe-task cancel + + log-sink close) only runs at whole-process shutdown, so N restarts leak + N-1 contexts. With the fix, ``_watch`` closes the per-launch stack as soon + as the child exits, before the next ``start_lovr_once``. + """ + sock_path = _unique_ipc(tmp_path) + lovr_bin = tmp_path / "lovr.sh" + lovr_bin.write_text("#!/bin/sh\nsleep 999\n") + lovr_bin.chmod(0o755) + xr_app_dir = tmp_path / "xr_app" + xr_app_dir.mkdir() + + cfg = Config( + lovr_bin = lovr_bin, + xr_app_dir = xr_app_dir, + scene_socket = sock_path, + cloudxr_env_file = None, + host = "127.0.0.1", + port = 0, + ) + + created: list[_FakeManagedProcessCtx] = [] + + def _make_ctx(*_a, **_kw) -> _FakeManagedProcessCtx: + ctx = _FakeManagedProcessCtx() + created.append(ctx) + return ctx + + monkeypatch.setattr(render_main, "ManagedProcess", _make_ctx) + + stack = contextlib.AsyncExitStack() + await stack.__aenter__() + try: + disp = SceneDispatcher(cfg, stack) + + # First launch. + assert await disp.start_lovr_once() == {"status": "started"} + assert len(created) == 1 + assert disp._launch_stack is not None + assert created[0].exited is False # live + + # Simulate the LOVR child exiting and let _watch run to completion. + created[0].proc.trigger_exit() + await disp._watch_task + + # The previous launch context is torn down on respawn — not leaked. + assert created[0].exited is True + assert disp._launch_stack is None + assert disp._lovr_started is False + + # Second launch reuses a fresh context; the first stays closed (no + # accumulation), the second is live. + assert await disp.start_lovr_once() == {"status": "started"} + assert len(created) == 2 + assert created[0].exited is True + assert created[1].exited is False + assert sum(c.exited for c in created) == 1 # only the dead one closed + + disp.close() + with contextlib.suppress(asyncio.CancelledError): + await disp._watch_task + finally: + await stack.__aexit__(None, None, None) + + # Safety net: the still-live second context is closed when the + # app-lifetime stack unwinds at shutdown. + assert created[1].exited is True