Skip to content

Commit 1cf396d

Browse files
AdamMaguedclaude
andcommitted
docs(web): document the browser-liveness lock-scope invariant (#5)
_lock only guards _ensure_browser() inside _page(); it is released before the page reference is returned, so a caller's subsequent use of the page happens outside the lock. This is a latent race ONLY if the documented single-run-per-process invariant (MCP serializes tool calls) is ever violated — not currently reachable. Extending the lock to cover every caller's page-use span would mean turning every `await browser._page()` call site across navigation.py / actions.py / observation.py / resolution.py into a context-manager pattern, for a scenario that cannot happen today. Left the mechanism unchanged and documented the invariant clearly at both the lock definition and _page() itself, with a pointer to the right fix (_page() returning a lock-holding context manager) if concurrent dispatch is ever introduced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 767ab80 commit 1cf396d

1 file changed

Lines changed: 26 additions & 0 deletions

File tree

‎mcp_server/tools/web/browser.py‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,26 @@ async def _guard(route, request) -> None:
201201

202202
# Module-level singletons. The MCP server is a single subprocess, so these
203203
# survive across tool calls within a session.
204+
#
205+
# `_lock` (LOW #5): only guards `_ensure_browser()` inside `_page()` — it is
206+
# released BEFORE `_page()` returns the page object to its caller. Every web
207+
# tool handler then uses that page reference for the rest of its own async
208+
# function body with the lock no longer held. This is safe ONLY because the
209+
# MCP server serializes tool calls (one call's handler runs to completion —
210+
# including every `await` inside it — before the next one starts), which is
211+
# the documented single-run-per-process invariant for this whole package. If
212+
# that invariant is ever violated (e.g. a future change dispatches multiple
213+
# tool calls concurrently within one MCP server process), two overlapping
214+
# handlers could interleave like: A calls `_page()` and gets page P; B calls
215+
# `_page()` concurrently, finds P dead, resets `_browser_ctx`/`_active_page`
216+
# and swaps in a new page Q; A continues operating on its now-stale local
217+
# reference to P instead of Q. This is NOT reachable today — do not "fix" it
218+
# by holding `_lock` across each handler's full page-use span (that would
219+
# require turning every `await browser._page()` call site in navigation.py /
220+
# actions.py / observation.py / resolution.py into a context-manager pattern
221+
# for a currently-impossible scenario). If concurrent dispatch is ever added,
222+
# revisit this by having `_page()` return a lock-holding context manager
223+
# instead of a bare page reference.
204224
_lock = asyncio.Lock()
205225
_playwright: Any = None
206226
_browser_ctx: Any = None
@@ -527,6 +547,12 @@ async def _ensure_browser() -> None:
527547

528548

529549
async def _page():
550+
"""Return the current live page, lazily starting the browser if needed.
551+
552+
`_lock` is held only for `_ensure_browser()`, not for the caller's
553+
subsequent use of the returned page — see the invariant documented on
554+
`_lock`'s definition above (#5).
555+
"""
530556
async with _lock:
531557
await _ensure_browser()
532558
return _active_page

0 commit comments

Comments
 (0)