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

@@ -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)

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/<id>`` 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)
 
 

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)