From f87f958e926ec997546cade7acf10807af6df120 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 22 Oct 2026 19:21:45 -0700 Subject: [PATCH] fix(browser): launch records, leases and the key registry match one browser launch, never a pid number A pid recurs across PID namespaces and boots, and records of earlier containers are kept (their pids cannot be judged from here). Every match now compares the full launch identity: profile realpath, pid, start time and the record (whose name carries namespace + boot); a browser of another environment is bound by the DevTools browser id its endpoint serves, which every launch record now carries once the browser is up. - attach leases the record describing THIS launch, not the first record on the pid number (an earlier container's record no longer hides the attacher from the live browser's owner) - _heir_key: two same-pid browsers of two containers are no longer one browser, so stopping one key drops its own lease - _tracked_browser, _adopt_record, _last_holder_out, _foreign_record_names compare the same identity - other_live_holders drops a lease naming another start time from any namespace that knew it --- tests/tools/test_browser_launcher_identity.py | 165 ++++++++++++++++++ tools/browser_launcher.py | 93 +++++++--- tools/browser_launcher_holders.py | 9 +- 3 files changed, 240 insertions(+), 27 deletions(-) create mode 100644 tests/tools/test_browser_launcher_identity.py diff --git a/tests/tools/test_browser_launcher_identity.py b/tests/tools/test_browser_launcher_identity.py new file mode 100644 index 0000000000..7ecf0b88da --- /dev/null +++ b/tests/tools/test_browser_launcher_identity.py @@ -0,0 +1,165 @@ +"""A browser in launcher state is ONE launch: its PID namespace and boot, pid, process start time and profile +(``--user-data-dir`` realpath), never its pid number alone. + +Records from earlier containers or boots are kept (their pids cannot be judged from here), so a pid number can +recur across them, and two containers can give two different browsers the same number. An attacher leases the +launch it actually reached (for a browser of another environment: the record whose DevTools browser id its +endpoint serves), and stopping one key lets go only of that key's own browser (``tools/browser_launcher.py``). +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import textwrap +from pathlib import Path + +import pytest + +from tests.tools.browser_launcher_fakes import kill_fake, write_fake_chromium +from tests.waiting import wait_until +from tools import browser_launcher as launcher + +pytestmark = pytest.mark.platforms("linux") + +_REPO = Path(__file__).resolve().parents[2] +_OTHER_HOST = "container-3f9c2a" +_OLD_NS = "4026531836:00000000-0000-0000-0000-000000000000" # a container (or boot) that is gone +_LIVE_NS = "4026532999:11111111-1111-1111-1111-111111111111" # the container running the browser now + + +def _dead_pid() -> int: + proc = subprocess.Popen([sys.executable, "-c", "pass"]) + proc.wait() + return proc.pid + + +def _start_fake(script, udd) -> subprocess.Popen: + proc = subprocess.Popen([str(script), "--remote-debugging-port=0", f"--user-data-dir={udd}"], + stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + assert wait_until(lambda: (udd / "DevToolsActivePort").exists(), timeout=15, what="the fake to come up") + return proc + + +def _as_container_browser(udd, pid: int) -> None: + """Make the profile's SingletonLock name ``pid`` under a container's hostname: its number in THAT namespace.""" + lock = udd / "SingletonLock" + lock.unlink(missing_ok=True) + os.symlink(f"{_OTHER_HOST}-{pid}", lock) + + +def _record(udd, pid: int, tag: str, ns: str, start_time: int, devtools) -> Path: + """The launch record a Hermes in namespace ``ns`` wrote for its browser ``pid`` on ``udd``.""" + path = launcher._shared_state_dir(str(udd)) / f"{launcher._profile_digest(str(udd))[:16]}-{pid}{tag}.json" + data = {"owner_pid": pid + 1, "owner_start_time": 1, "pid": pid, "start_time": start_time, + "user_data_dir": str(udd), "home_key": "elsewhere", "ephemeral": False, "ns": ns} + path.write_text(json.dumps({**data, **({"devtools": devtools} if devtools else {})})) + return path + + +# The owner's own view: ``other_live_holders`` run as a process of the container that launched the browser. +_OWNER_VIEW = textwrap.dedent("""\ + import sys + from pathlib import Path + from tools import browser_launcher_holders as holders + holders.ns_identity = lambda: sys.argv[2] + print(len(holders.other_live_holders(Path(sys.argv[1]), int(sys.argv[3]))), flush=True) +""") + + +def _owner_sees(record: Path, ns: str, start_time: int) -> int: + out = subprocess.run([sys.executable, "-c", _OWNER_VIEW, str(record), ns, str(start_time)], cwd=_REPO, + env=dict(os.environ), capture_output=True, text=True, timeout=60, check=True) + return int(out.stdout.strip()) + + +@pytest.mark.live_system_guard_bypass # the fake browser is ours to stop if a leg failed +@pytest.mark.parametrize("old_devtools", ["/devtools/browser/an-earlier-launch", None], + ids=["old-launch-was-ready", "old-launch-never-ready"]) +def test_an_older_record_on_the_same_pid_does_not_take_the_live_browsers_holders(tmp_path, old_devtools): + script = write_fake_chromium(tmp_path / "chromium") + udd, key = tmp_path / "pinned-profile", f"profile:{tmp_path / 'pinned-profile'}" + theirs = _start_fake(script, udd) + try: + pid = _dead_pid() # the container's number for its browser: names nobody here + _as_container_browser(udd, pid) + live_path = (udd / "DevToolsActivePort").read_text().split()[1] + # Sorted first: a record an earlier container (or boot) left for ITS browser on the same pid number. + old = _record(udd, pid, "-n000000000000", _OLD_NS, 111, old_devtools) + live = _record(udd, pid, "-nffffffffffff", _LIVE_NS, 222, live_path) + old_bytes = old.read_bytes() + + browser, err = launcher.launch_chromium(key, executable=str(script), user_data_dir=str(udd), + ephemeral=False, headed=False, env=dict(os.environ), timeout=30) + + assert err is None and browser is not None and browser.foreign + assert _owner_sees(live, _LIVE_NS, 222) == 1, "the live browser's owner does not see the attacher" + assert not list(old.parent.glob(f"{old.stem}@*.lease")), "the attacher leased an earlier launch" + assert old.read_bytes() == old_bytes, "the earlier launch's record was rewritten" + + launcher.stop_browser(key) + assert not list(live.parent.glob("*.lease")) + assert theirs.poll() is None, "stopped a browser another environment holds" + finally: + launcher.stop_browser(key) + kill_fake(theirs.pid, script) + theirs.wait(timeout=10) + + +@pytest.mark.live_system_guard_bypass # the fake browsers are ours to stop if a leg failed +def test_stopping_one_of_two_same_pid_browsers_drops_only_its_own_lease(tmp_path): + """Two containers' browsers on two pinned profiles, each pid N in its own namespace: one session ending must + let go of its browser only, not leave its lease behind because the other key looked like the same browser.""" + script = write_fake_chromium(tmp_path / "chromium") + pid = _dead_pid() + profiles = [tmp_path / "profile-a", tmp_path / "profile-b"] + fakes = [_start_fake(script, udd) for udd in profiles] + keys = [f"profile:{udd}" for udd in profiles] + try: + for udd in profiles: + _as_container_browser(udd, pid) + for key, udd in zip(keys, profiles): + browser, err = launcher.launch_chromium(key, executable=str(script), user_data_dir=str(udd), + ephemeral=False, headed=False, env=dict(os.environ), timeout=30) + assert err is None and browser is not None and browser.foreign and browser.pid == pid + states = [launcher._shared_state_dir(str(udd)) for udd in profiles] + assert all(list(state.glob("*.lease")) for state in states) + + launcher.stop_browser(keys[0]) + + assert not list(states[0].glob("*.lease")), "the stopped session left its lease: its browser is held forever" + assert list(states[1].glob("*.lease")), "stopping one browser dropped the other one's lease" + assert launcher.get_browser(keys[1]) is not None + assert all(fake.poll() is None for fake in fakes) + finally: + for key in keys: + launcher.stop_browser(key) + for fake in fakes: + kill_fake(fake.pid, script) + fake.wait(timeout=10) + + +@pytest.mark.live_system_guard_bypass # the fake browser is ours to stop if a leg failed +def test_a_lease_on_an_earlier_launch_does_not_keep_the_current_browser_up(tmp_path): + """A holder in another namespace leased an earlier browser filed under the same record name (that namespace's + pid recycled): its lease names that launch's start time, not this one's, so the last real holder still stops + the browser it launched.""" + script = write_fake_chromium(tmp_path / "chromium") + udd, key = tmp_path / "pinned-profile", f"profile:{tmp_path / 'pinned-profile'}" + mine, err = launcher.launch_chromium(key, executable=str(script), user_data_dir=str(udd), ephemeral=False, + headed=False, env=dict(os.environ), timeout=30) + assert err is None and mine.owned + try: + record, gone_here = launcher._record_path(mine), _dead_pid() + stale = record.with_name(f"{record.stem}@{gone_here}-nffffffffffff.lease") + stale.write_text(json.dumps({"holder_pid": gone_here, "holder_start_time": None, "pid": mine.pid, + "start_time": mine.start_time - 1, "user_data_dir": str(udd), "ns": _LIVE_NS})) + + launcher.stop_browser(key) + + assert wait_until(lambda: not launcher._pid_alive(mine.pid), timeout=15, what="the browser to stop") + assert not record.exists() and not stale.exists() + finally: + kill_fake(mine.pid, script) diff --git a/tools/browser_launcher.py b/tools/browser_launcher.py index b34b7f18fb..f1254779fa 100644 --- a/tools/browser_launcher.py +++ b/tools/browser_launcher.py @@ -329,14 +329,19 @@ def _foreign_holder(user_data_dir: str) -> Optional[int]: def _foreign_record_names(user_data_dir: str, pid: int) -> bool: - """A launch record from another PID namespace names ``pid`` as shared profile ``user_data_dir``'s browser.""" + """A launch record from another PID namespace names ``pid`` as shared profile ``user_data_dir``'s browser: the one + serving the profile's DevTools endpoint now, or one not yet up there (its record has no endpoint). An earlier + launch there that reached another endpoint is not that browser, whatever pid number it had.""" from tools.browser_launcher_holders import same_namespace state = _shared_state_dir(user_data_dir) if state is None: return False + found = _read_devtools_file(user_data_dir) for name in _record_names(state, user_data_dir, pid): record = _load_record(state / name) - if record is not None and record.get("pid") == pid and not same_namespace(record): + if (record is not None and record.get("pid") == pid and not same_namespace(record) + and _same_profile(record.get("user_data_dir"), user_data_dir) + and record.get("devtools") in (None, found and found[1])): return True return False @@ -439,23 +444,27 @@ def _attach_running(key: str, user_data_dir: str, home_key: str) -> Optional[Lau records = _state_dir(browser) if records is not None: from tools import browser_launcher_holders as holders - browser.record_name = _launch_record_name(records, browser) + _bind_launch_record(records, browser) _adopt_record(browser) holders.write_lease(_record_path(browser), browser) # its launcher's exit must not stop it under us return browser -def _launch_record_name(records: Path, browser: LaunchedBrowser) -> Optional[str]: - """The launch record already naming ``browser`` in ``records``, written in ANY PID namespace: an attacher - leases the browser under its launcher's record, or a launcher in another namespace (a container gateway a - host CLI attached to) would miss the lease and stop the browser under it. This namespace's record wins - over another's on the same pid number; None when there is no record (a browser a human started).""" - from tools.browser_launcher_holders import ns_tag - names = _record_names(records, browser.user_data_dir, browser.pid) - own = f"{_profile_digest(browser.user_data_dir)[:16]}-{browser.pid}{ns_tag()}.json" - if browser.foreign: # its pid number is another namespace's: a record of ours on that number is another browser - return next((name for name in names if name != own), None) - return own if own in names else next(iter(names), None) +def _bind_launch_record(records: Path, browser: LaunchedBrowser) -> None: + """Lease ``browser`` under the launch record that describes THIS launch of it (:func:`_record_is`), written in + ANY PID namespace: its launcher (a container gateway a host CLI attached to) finds its holders only there. + Records of earlier containers and boots are kept, so the pid number alone can name another browser's record. + This namespace's record wins; with none (a browser a human started) the lease goes under this namespace's + name. A browser of another environment takes its record's start time: its full identity from then on.""" + own = _record_name(browser) + names = sorted(_record_names(records, browser.user_data_dir, browser.pid), key=lambda name: name != own) + for name in names: + record = _load_record(records / name) + if record is not None and _record_is(record, browser): + browser.record_name = name + if browser.foreign: + browser.start_time = record.get("start_time") + return def _attach_foreign(key: str, user_data_dir: str, home_key: str, pid: int) -> Optional[LaunchedBrowser]: @@ -472,7 +481,7 @@ def _attach_foreign(key: str, user_data_dir: str, home_key: str, pid: int) -> Op records = _state_dir(browser) if records is not None: from tools import browser_launcher_holders as holders - browser.record_name = _launch_record_name(records, browser) + _bind_launch_record(records, browser) holders.write_lease(_record_path(browser), browser) # its launcher's exit must not stop it under us return browser @@ -593,6 +602,7 @@ def _launch_owned(key: str, executable: str, udd: str, *, ephemeral: bool, packa return None, err browser = _make_browser(key, found[0], found[1], pid=proc.pid, start_time=start_time, user_data_dir=udd, owned=True, ephemeral=ephemeral, home_key=home_key) + _write_record(browser) # now with its DevTools browser id: how another environment tells this launch apart with _lock: _launching.pop(proc.pid, None) _browsers[key] = browser @@ -660,8 +670,16 @@ def _heir_key(browser: LaunchedBrowser) -> Optional[str]: return None with _lock: return next((k for k, b in _browsers.items() if b is not browser and not b.ephemeral - and b.foreign == browser.foreign and b.pid == browser.pid - and b.start_time == browser.start_time), None) + and _same_browser(b, browser)), None) + + +def _same_browser(a: LaunchedBrowser, b: LaunchedBrowser) -> bool: + """One launch: same profile (realpath), pid, start time and launch record (whose name carries the PID namespace + and boot its pid belongs to); a browser of another environment also by the DevTools browser id it serves (two + containers can give two browsers one pid number, neither with a start time readable here).""" + return (a.foreign == b.foreign and a.pid == b.pid and a.start_time == b.start_time + and _record_name(a) == _record_name(b) and _same_profile(a.user_data_dir, b.user_data_dir) + and (not a.foreign or a.cdp_url == b.cdp_url)) def _forget(key: str, browser: LaunchedBrowser, *, drop_record: bool = True) -> None: @@ -746,7 +764,7 @@ def _last_holder_out(record: Path, browser: LaunchedBrowser) -> bool: return False others = holders.other_live_holders(record, browser.start_time) data = _load_record(record) if record.exists() else None - if data is not None and data.get("start_time") != browser.start_time: + if data is not None and not _record_is(data, browser): data = None # an earlier browser on a recycled pid held_elsewhere = data is not None and not holders.held_here(data, "owner_pid") and _owner_alive(data) heir = holders.successor(others) @@ -816,6 +834,29 @@ def _profile_digest(user_data_dir: str) -> str: return hashlib.sha256(os.path.realpath(user_data_dir).encode()).hexdigest() +def _same_profile(recorded, user_data_dir: str) -> bool: + return isinstance(recorded, str) and os.path.realpath(recorded) == os.path.realpath(user_data_dir) + + +def _devtools_id(browser: LaunchedBrowser) -> Optional[str]: + """The ``/devtools/browser/`` path of ``browser``'s endpoint (new per launch); None before it is up.""" + at = browser.cdp_url.find("/devtools/browser/") + return browser.cdp_url[at:] if at >= 0 else None + + +def _record_is(record: dict, browser: LaunchedBrowser) -> bool: + """``record`` describes this launch of ``browser``: same pid and profile, and its start time; for a browser of + another environment (its pid is another namespace's number and its start time unreadable here), a record from + another namespace naming the DevTools browser id its endpoint serves.""" + from tools.browser_launcher_holders import same_namespace + if record.get("pid") != browser.pid or not _same_profile(record.get("user_data_dir"), browser.user_data_dir): + return False + if browser.foreign: + return not same_namespace(record) and record.get("devtools") is not None and ( + record.get("devtools") == _devtools_id(browser)) + return record.get("start_time") == browser.start_time + + def _shared_state_dir(user_data_dir: str) -> Optional[Path]: """Shared profile ``user_data_dir``'s own private dir (its launch record, leases and lock).""" root = _private_dir(_shared_profiles_root()) @@ -834,10 +875,13 @@ def _record_path(browser: LaunchedBrowser) -> Optional[Path]: records = _state_dir(browser) if records is None: return None - if browser.record_name: - return records / browser.record_name + return records / _record_name(browser) + + +def _record_name(browser: LaunchedBrowser) -> str: + """The launch record ``browser``'s leases are keyed by: the one it was bound to, else this namespace's name.""" from tools.browser_launcher_holders import ns_tag - return records / f"{_profile_digest(browser.user_data_dir)[:16]}-{browser.pid}{ns_tag()}.json" + return browser.record_name or f"{_profile_digest(browser.user_data_dir)[:16]}-{browser.pid}{ns_tag()}.json" def _write_record(browser: LaunchedBrowser) -> None: @@ -845,7 +889,8 @@ def _write_record(browser: LaunchedBrowser) -> None: from utils import atomic_json_write record = {"owner_pid": os.getpid(), "owner_start_time": _start_time(os.getpid()), "pid": browser.pid, "start_time": browser.start_time, "user_data_dir": browser.user_data_dir, - "home_key": browser.home_key, "ephemeral": browser.ephemeral, "ns": holders.ns_identity()} + "home_key": browser.home_key, "ephemeral": browser.ephemeral, "ns": holders.ns_identity(), + "devtools": _devtools_id(browser)} try: path = _record_path(browser) if path is not None: @@ -916,7 +961,7 @@ def _is_recorded_browser(record: dict, home: Optional[str]) -> bool: def _tracked_browser(record: dict) -> Optional[LaunchedBrowser]: with _lock: # a browser another environment holds has a pid of THAT namespace: it tracks no record here return next((b for b in (*_browsers.values(), *_launching.values()) - if not b.foreign and b.pid == record.get("pid")), None) + if not b.foreign and _record_is(record, b)), None) def _adopt_record(browser: LaunchedBrowser) -> None: @@ -926,7 +971,7 @@ def _adopt_record(browser: LaunchedBrowser) -> None: if path is None or not path.exists(): return record = _load_record(path) - if record is not None and record.get("start_time") == browser.start_time and not _owner_alive(record): + if record is not None and _record_is(record, browser) and not _owner_alive(record): _write_record(browser) diff --git a/tools/browser_launcher_holders.py b/tools/browser_launcher_holders.py index 358084c1cd..5d7021de0f 100644 --- a/tools/browser_launcher_holders.py +++ b/tools/browser_launcher_holders.py @@ -169,14 +169,17 @@ def _holder_gone(lease: dict) -> bool: def other_live_holders(record: Path, browser_start_time) -> list[dict]: """Leases of OTHER live processes on the browser ``record`` names (caller holds :func:`records_lock`). - A lease of a dead holder, or one naming an earlier browser on the same pid, is removed; one from another - namespace counts as live (its holder cannot be checked from here).""" + A lease of a dead holder, or one naming an earlier browser on the same pid (another start time, from whichever + namespace that knew it), is removed; one from another namespace counts as live (its holder cannot be checked + from here).""" holders = [] for path in sorted(record.parent.glob(f"{record.stem}@*.lease")): lease = _load(path) if lease is None or held_here(lease, "holder_pid"): continue - if same_namespace(lease) and (lease.get("start_time") != browser_start_time or _holder_gone(lease)): + other_launch = lease.get("start_time") != browser_start_time and ( + same_namespace(lease) or lease.get("start_time") is not None) + if other_launch or _holder_gone(lease): path.unlink(missing_ok=True) continue holders.append(lease)