From fc4df13bbaafe49f1cb79c9f785a2d43a1f5957c Mon Sep 17 00:00:00 2001 From: ciregenz Date: Mon, 17 Aug 2026 23:36:34 -0700 Subject: [PATCH] [eric] browser: orphan-run abort becomes a heartbeat stream; is_disconnected never fires on a half-closed sidecar socket (packaged drill caught the placebo) --- backend/main.py | 43 +++++++++++------ .../tests/test_browser_run_orphan_abort.py | 48 +++++++++---------- 2 files changed, 51 insertions(+), 40 deletions(-) diff --git a/backend/main.py b/backend/main.py index daa06a89..bf613c2d 100644 --- a/backend/main.py +++ b/backend/main.py @@ -530,23 +530,38 @@ async def browser_agent_run(request: Request): pre_selected_browser_ids=pre_selected_browser_ids, parent_session_id=parent_session_id or None, )) + # The caller is the per-session MCP sidecar. If it dies mid-run (the disconnect class of # ENG-327/303), nobody can ever read this result, yet the orphaned run kept driving the card # while the parent's recovery re-dispatched onto it: two drivers, one wedged browser (ENG-338). - while True: - done, p_pending = await asyncio.wait({run_task}, timeout=2.0) - if done: - return JSONResponse({"results": run_task.result()}) - if await request.is_disconnected(): - run_task.cancel() - try: - await run_task - except (asyncio.CancelledError, Exception): - pass - logger.warning( - f"[browser-agent] caller (sidecar) for session {parent_session_id or '?'} disconnected mid-run; " - "aborted the orphaned browser run so the card is free for the retry") - return JSONResponse({"error": "caller disconnected; browser run aborted"}, status_code=499) + # Detection is a HEARTBEAT WRITE, not is_disconnected(): a dead sidecar half-closes (FIN) and + # uvicorn never flags that as a disconnect (packaged drill proved the run kept driving 40s+), + # but a write to the dead socket fails within one RST round-trip and cancels the stream, whose + # finally aborts the run. Leading whitespace is legal JSON, so the sidecar's json.loads is fine. + async def p_stream_with_heartbeat(): + try: + while True: + done, p_pending = await asyncio.wait({run_task}, timeout=2.0) + if done: + try: + yield json.dumps({"results": run_task.result()}).encode() + except Exception as p_run_err: + yield json.dumps({"error": str(p_run_err)}).encode() + return + yield b" " + finally: + if not run_task.done(): + run_task.cancel() + try: + await run_task + except (asyncio.CancelledError, Exception): + pass + logger.warning( + f"[browser-agent] caller (sidecar) for session {parent_session_id or '?'} disconnected mid-run; " + "aborted the orphaned browser run so the card is free for the retry") + + from fastapi.responses import StreamingResponse + return StreamingResponse(p_stream_with_heartbeat(), media_type="application/json") # Allowlisted social platforms whose own-session MCP shims may borrow partition cookies. The allowlist is the real scope: even an authenticated localhost caller can only ever read these sites' cookies, never an arbitrary domain, so this can't become a general cookie-theft oracle. diff --git a/backend/tests/test_browser_run_orphan_abort.py b/backend/tests/test_browser_run_orphan_abort.py index 2a46c618..5b9c4bd6 100644 --- a/backend/tests/test_browser_run_orphan_abort.py +++ b/backend/tests/test_browser_run_orphan_abort.py @@ -1,9 +1,9 @@ """ENG-338: a browser run whose sidecar caller died must be cancelled, not orphaned. -The MCP disconnects themselves are fixed (ENG-303/327), but WHEN one happens the backend used to -keep driving the browser card with a result nobody could ever read, while the parent's stage-3 -recovery re-dispatched onto the same card: two drivers, one wedged browser. The route now watches -its own client and aborts the run the moment the caller is gone.""" +The first fix polled request.is_disconnected(), and the packaged drill proved that NEVER fires on +real uvicorn when the sidecar half-closes (the run kept driving 40s after the sever). The seal is +now a heartbeat STREAM: a write to the dead socket fails, the generator is closed, and its finally +cancels the run. These tests drive the generator directly, both directions.""" import asyncio import pytest @@ -11,22 +11,13 @@ import pytest from backend.main import browser_agent_run -class P_DeadCallerRequest: - """Request stub: valid body, but the client is already gone.""" - - def __init__(self): - self.disconnect_polls = 0 - +class P_Req: async def json(self): return {"tasks": [{"task": "look at example.com"}], "parent_session_id": "s1"} - async def is_disconnected(self): - self.disconnect_polls += 1 - return True - @pytest.mark.asyncio -async def test_a_dead_caller_cancels_the_run_instead_of_orphaning_it(monkeypatch): +async def test_closing_the_stream_mid_run_cancels_the_run(monkeypatch): state = {"cancelled": False, "started": False} async def p_never_ending(**p_kw): @@ -39,25 +30,30 @@ async def test_a_dead_caller_cancels_the_run_instead_of_orphaning_it(monkeypatch import backend.apps.agents.browser.browser_agent as p_ba monkeypatch.setattr(p_ba, "run_browser_agents", p_never_ending) - req = P_DeadCallerRequest() - resp = await browser_agent_run(req) - assert resp.status_code == 499 + resp = await browser_agent_run(P_Req()) + gen = resp.body_iterator + first = await gen.__anext__() + assert first == b" ", "pending run must heartbeat whitespace" + # The dead-socket path: uvicorn closes the generator when a write fails. + await gen.aclose() assert state["started"] is True assert state["cancelled"] is True, "the orphaned run must be cancelled, not left driving the card" - assert req.disconnect_polls >= 1 @pytest.mark.asyncio -async def test_a_live_caller_gets_the_results_untouched(monkeypatch): +async def test_a_live_caller_gets_the_results_as_valid_json(monkeypatch): async def p_quick(**p_kw): + await asyncio.sleep(0) return [{"summary": "done"}] - class P_LiveRequest(P_DeadCallerRequest): - async def is_disconnected(self): - return False + import json import backend.apps.agents.browser.browser_agent as p_ba monkeypatch.setattr(p_ba, "run_browser_agents", p_quick) - resp = await browser_agent_run(P_LiveRequest()) - assert resp.status_code == 200 - assert b"done" in resp.body + resp = await browser_agent_run(P_Req()) + chunks = [] + async for c in resp.body_iterator: + chunks.append(c) + body = b"".join(chunks) + parsed = json.loads(body) + assert parsed["results"][0]["summary"] == "done"