feat(browser): reap idle Chromium after configurable timeout (#485) - #509
Closed
angri450 wants to merge 2 commits into
Closed
feat(browser): reap idle Chromium after configurable timeout (#485)#509angri450 wants to merge 2 commits into
angri450 wants to merge 2 commits into
Conversation
…Cloud#485) harness-browser keeps its headless Chromium resident forever after a session closes (profile reuse, sticky CDP targets), a permanent ~500MB RSS tax on headless servers (issue TencentCloud#485). Add an opt-in idle reaper: - config: browser_idle_timeout_minutes (0 = disabled, env OCTOP_BROWSER_IDLE_TIMEOUT_MINUTES), default off. - BrowserIdleMonitor (infra/browser/idle.py): background task checks every 60s. A browser is reaped only when ALL hold: no live browser-stream WS connection, no non-blank page target, and no notify_activity within the timeout window, and the CDP port answers. Activity is recorded from resolve_harness_session (create=True only - dashboard status polling must not extend the timer) and from stream WS open/close. - Reap closes registered harness sessions, then pkills Chrome by the actual user-data-dir scanned from running processes (Octop relocates profiles to /tmp/harness-browser-profiles-*), then clears stale locks. The next browser_tool / dashboard open relaunches automatically. - Unit tests cover the decision logic (stream conn, timeout window, active pages, missing browser, reap side effects). Verified live: checks run every 60s, reaping triggers at the timeout, and an open dashboard panel (WS conn) correctly keeps the browser alive.
There was a problem hiding this comment.
🟡 Changes recommended
新增的单元测试里 patch/断言方式会导致测试直接失败，且 WS listen-only 连接被计入活跃连接会让 idle 回收在常见场景下永远不触发。
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
该 PR 为 Octop 引入可配置的 Chromium 空闲回收机制（issue #485），在浏览器无 WS 交互连接、无活动页面且超过空闲窗口时，自动关闭 resident Chromium 进程以降低长期内存占用；同时在服务启动/关闭与浏览器 WS/会话解析路径上接入活动打点，并补充单元测试覆盖决策逻辑。
Changes:
- 新增
BrowserIdleMonitor（后台 60s 周期检查）与回收逻辑（关闭 harness session registry + 按 user-data-dir 终止 Chrome）。 - 新增配置项
browser_idle_timeout_minutes（含 envOCTOP_BROWSER_IDLE_TIMEOUT_MINUTES）并在OctopServer启动时按需启用、停止时优雅关闭。 - 在浏览器 WS 流与 harness session resolve 路径上增加活动/连接打点，并新增单元测试。
File summaries
| File | Description |
|---|---|
src/octop/infra/browser/idle.py |
新增空闲监控与回收实现（CDP 探活、页面活跃判断、进程终止与锁清理）。 |
src/octop/infra/server.py |
启动时按配置创建/启动 idle monitor，停止时 shutdown。 |
src/octop/config.py |
增加配置字段与 env 覆盖读取。 |
src/octop/api/routers/browser/stream.py |
WS open/close 打点接入 idle monitor。 |
src/octop/api/routers/browser/harness.py |
resolve_harness_session 增加（仅 create=True）活动打点入口。 |
tests/unit/browser/test_browser_idle.py |
新增 idle monitor 决策逻辑与 reap 行为的单测。 |
Review details
Suppressed comments (4)
tests/unit/browser/test_browser_idle.py:51
- 同上：
m._reap.assert_not_awaited()在with patch.object(..., new=AsyncMock())之后会因为 patch 已恢复而报错。请对_reap的 AsyncMock 使用as mock_reap并在块外断言。
with patch.object(m, "_cdp_alive", new=AsyncMock(return_value=True)), patch.object(
m, "_reap", new=AsyncMock()
):
await m._check_once()
m._reap.assert_not_awaited() # type: ignore[attr-defined]
tests/unit/browser/test_browser_idle.py:61
- 同上：断言需要针对 patch 返回/绑定的 AsyncMock，而不是
m._reap（离开with后已还原）。
with patch.object(m, "_cdp_alive", new=AsyncMock(return_value=True)), patch.object(
m, "_has_active_pages", new=AsyncMock(return_value=True)
), patch.object(m, "_reap", new=AsyncMock()):
await m._check_once()
m._reap.assert_not_awaited() # type: ignore[attr-defined]
tests/unit/browser/test_browser_idle.py:71
- 同上：这里的
m._reap.assert_awaited_once()在离开 patch 后会失效并导致测试失败，需要改为对mock_reap断言。
with patch.object(m, "_cdp_alive", new=AsyncMock(return_value=True)), patch.object(
m, "_has_active_pages", new=AsyncMock(return_value=False)
), patch.object(m, "_reap", new=AsyncMock()):
await m._check_once()
m._reap.assert_awaited_once() # type: ignore[attr-defined]
tests/unit/browser/test_browser_idle.py:81
- 同上：这里的断言需要针对 patch 生成的 AsyncMock；当前写法会在
with结束后对原方法调用assert_not_awaited()并报错。
with patch.object(m, "_cdp_alive", new=AsyncMock(return_value=False)), patch.object(
m, "_reap", new=AsyncMock()
):
await m._check_once()
m._reap.assert_not_awaited() # type: ignore[attr-defined]
- Files reviewed: 6/6 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+36
to
+40
| with patch.object(m, "_cdp_alive", new=AsyncMock(return_value=True)), patch.object( | ||
| m, "_reap", new=AsyncMock() | ||
| ): | ||
| await m._check_once() | ||
| m._reap.assert_not_awaited() # type: ignore[attr-defined] |
Comment on lines
+196
to
+210
| out = subprocess.run( | ||
| ["pgrep", "-af", "chrome"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=5, | ||
| check=False, | ||
| ).stdout | ||
| dirs = {Path(d) for d in re.findall(r"user-data-dir=([^\s]+)", out)} | ||
| for d in dirs: | ||
| await asyncio.to_thread(pkill_chrome_profile, d) | ||
| await asyncio.to_thread(clear_profile_locks, d) | ||
| except Exception as exc: # noqa: BLE001 | ||
| logger.warning("browser reap failed: %s", exc) | ||
| self._last_activity = time.monotonic() # 防止紧接的重启风暴 | ||
| logger.info("browser reaped; next use will relaunch automatically") |
Comment on lines
+104
to
+105
| patch("octop.infra.browser.setup.pkill_chrome_profile", AsyncMock()), | ||
| patch("octop.infra.browser.setup.clear_profile_locks", AsyncMock()), |
Comment on lines
374
to
376
| await websocket.accept() | ||
| _notify_stream_open(websocket) | ||
| sess: Any | None = None |
Comment on lines
374
to
378
| await websocket.accept() | ||
| _notify_stream_open(websocket) | ||
| sess: Any | None = None | ||
| profile = "default" | ||
| stream_task: asyncio.Task[None] | None = None |
| await _send_json(websocket, {"type": "error", "message": str(exc)}) | ||
| await _send_json(websocket, {"type": "status", "status": "error"}) | ||
| finally: | ||
| _notify_stream_close(websocket) |
The reap step scanned every running Chrome's user-data-dir and killed all of them. That would also terminate user-launched browsers (e.g. a dedicated workstation on a different CDP port with its own profile). Filter to harness-managed profile directories (harness-browser-profiles, .octop/browser-profiles, .harness-browser/profiles).
Collaborator
|
Thanks for the detailed follow-up on #485 — the problem is real, and the measurements (idle RSS/PSS, listen-only vs activity, extra CDP clients on :9222) are useful. We are closing this PR because an idle reaper in Octop is the wrong layer, and the current heuristics are not reliable enough to ship. Why not Octop-onlyChrome lifetime is owned by
An Octop monitor therefore has to infer idleness (WS conn count,
Where the fix should liveharness-browser should own process lifetime:
Octop then only:
Please continue this on the harness-browser side (and/or reopen a much smaller Octop PR for the explicit close button once a kill API exists). Sorry to bounce a substantial patch — the contrib is appreciated, the layer is the blocker. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #485.
harness-browserkeeps its headless Chromium resident forever after a session closes (profile reuse, sticky CDP targets), so on a headless server a single browser use leaves a permanent ~500MB RSS / ~260MB PSS process. This PR adds an opt-in idle reaper to Octop:browser_idle_timeout_minutes(0 = disabled, default), envOCTOP_BROWSER_IDLE_TIMEOUT_MINUTES.BrowserIdleMonitor(infra/browser/idle.py): background task, 60s interval. A browser is reaped only when ALL of these hold:/api/browser-stream/wsconnection (open dashboard panel keeps it alive),notify_activitywithin the timeout window,resolve_harness_session(create=True)and stream WS open/close.create=False(dashboard status polling, every 2s) deliberately does NOT extend the timer — otherwise the browser would never be reaped.user-data-dirscanned from running processes (Octop relocates profiles under/tmp/harness-browser-profiles-*), clears stale locks. Nextbrowser_tool/ dashboard open relaunches automatically (launch_or_attach+setup.pylock cleanup already handle this).tests/unit/browser/test_browser_idle.py) cover the decision logic.Test plan
Verified live on Octop 0.9.28 + Chromium 151 headless:
OCTOP_BROWSER_IDLE_TIMEOUT_MINUTES=2: checks run every 60s (browser idle checklogs), reaping triggers at the 120s timeout (browser idle for 121s, reaping Chrome).browser_toolafter a reap relaunches Chrome automatically (existinglaunch_or_attachpath).The reaper is opt-in (default disabled) so existing deployments are unaffected.
Related