Puppeteer/Playwright: drop cache_dir entirely, inline install_root/cache

claude · claude · commit 836123bbfce1 · 2026-04-21T23:00:40.000Z
``cache_dir`` was a one-line computed helper (``install_root/cache``
when managed, ``None`` otherwise) that only existed as a convenience
alias — every callsite already branched on ``is not None``. Remove
the computed field and inline ``install_root / "cache"`` at each use:

- ``ENV`` now returns ``{}`` when ``install_root`` is None and
  ``{"PUPPETEER_CACHE_DIR": "&lt;install_root&gt;/cache"}`` /
  ``{"PLAYWRIGHT_BROWSERS_PATH": "&lt;install_root&gt;/cache"}`` otherwise.
- ``setup`` creates ``&lt;install_root&gt;/cache`` next to the existing
  ``install_root.mkdir``.
- ``_normalize_install_args`` / ``_list_installed_browsers`` pass
  ``--path=&lt;install_root&gt;/cache`` only when managed.
- ``_cleanup_partial_browser_cache`` takes an early-return when
  ``install_root`` is None and uses a local ``cache_dir`` var for the
  glob/rmtree loop.
- Playwright ``build_exec_env`` sudo wrapper and
  ``default_abspath_handler`` scope check both use a local
  ``cache_dir`` var under ``install_root is not None``.
- Uninstall handlers already resolved the browser dir via ``load()``
  in the previous commit, so they don't touch cache_dir at all.

Net result: both providers' public field surface is just
``install_root`` + ``bin_dir``, matching the "only
install_root/bin_dir" direction.
diff --git a/README.md b/README.md
@@ -1074,11 +1074,10 @@ INSTALLER_BIN = "puppeteer-browsers"
 PATH = ""
 install_root = $ABXPKG_PUPPETEER_ROOT or $ABXPKG_LIB_DIR/puppeteer
 bin_dir = <install_root>/bin
-cache_dir = <install_root>/cache  # computed; None when install_root is unset
 ```
 
 - Install root: set `install_root` for the root dir and `bin_dir` for symlinked executables. Leave it unset for ambient/global mode, where cache ownership stays with the host and `INSTALLER_BINARY` must already be resolvable from the ambient provider set.
-- Cache dir: `cache_dir` is a computed helper property. When `install_root` is pinned it's `<install_root>/cache` and abxpkg owns it — exported as `PUPPETEER_CACHE_DIR` to every subprocess, used for `--path=` on `puppeteer-browsers install` / `list`, and rmtree'd per-browser on `uninstall()`. When `install_root` is unset, `cache_dir` is `None`: the provider is in pure passthrough mode. The caller's ambient `$PUPPETEER_CACHE_DIR` (or the CLI's `~/.cache/puppeteer` default) flows through to subprocesses unchanged, `load()` trusts whatever path `puppeteer-browsers list` reports, and `uninstall()` is a no-op — we don't touch the user's own cache.
+- Browser cache: when `install_root` is pinned, abxpkg manages `<install_root>/cache` end-to-end — it's exported as `PUPPETEER_CACHE_DIR` to every subprocess, used for `--path=` on `puppeteer-browsers install` / `list`, and `uninstall()` resolves the real browser directory via `load()` then rmtrees it. When `install_root` is unset the provider is in pure passthrough mode: the caller's ambient `$PUPPETEER_CACHE_DIR` (or the CLI's `~/.cache/puppeteer` default) flows through to subprocesses unchanged, `load()` trusts whatever path `puppeteer-browsers list` reports, and `uninstall()` still rmtrees the real browser directory returned by `load()` — leaving any unrelated browsers in the shared cache alone.
 - Auto-switching: bootstraps `@puppeteer/browsers` through `NpmProvider` and then uses that CLI for browser installs.
 - `dry_run`: shared behavior.
 - Security: `min_release_age` is unsupported for browser installs and is ignored with a warning if explicitly requested. `postinstall_scripts=False` is supported for the underlying npm bootstrap path, and `ABXPKG_POSTINSTALL_SCRIPTS` hydrates the provider default here.
@@ -1097,12 +1096,11 @@ INSTALLER_BIN = "playwright"
 PATH = ""
 install_root = None              # abxpkg-managed root dir for bin_dir / nested npm prefix
 bin_dir = <install_root>/bin     # symlink dir for resolved browsers
-cache_dir = <install_root>/cache # computed; None when install_root is unset
 euid = 0                         # routes exec() through sudo-first-then-fallback
 ```
 
 - Install root: set `install_root` to pin the abxpkg-managed root dir (where `bin_dir` symlinks and the nested npm prefix live). Leave it unset to let playwright use its own OS-default browsers path (`~/.cache/ms-playwright` on Linux etc.) — in that case abxpkg maintains no symlink dir or npm prefix at all, the `playwright` npm CLI bootstraps against the host's npm default, and `load()` returns the resolved `executablePath()` directly. `bin_dir` overrides the symlink directory when `install_root` is pinned.
-- Cache dir: `cache_dir` is a computed helper property. When `install_root` is pinned it's `<install_root>/cache` and abxpkg owns it — exported as `PLAYWRIGHT_BROWSERS_PATH` to every subprocess (including the `env KEY=VAL -- ...` wrapper used when we go through sudo), used to scope `executablePath()` hits on `load()`, and walked for per-browser rmtree on `uninstall()`. When `install_root` is unset, `cache_dir` is `None`: the provider is in pure passthrough mode. The caller's ambient `$PLAYWRIGHT_BROWSERS_PATH` (or playwright's `~/.cache/ms-playwright` default on Linux) flows through to subprocesses unchanged, `load()` trusts whatever path `executablePath()` reports, and `uninstall()` is a no-op — we don't touch the user's own cache.
+- Browser cache: when `install_root` is pinned, abxpkg manages `<install_root>/cache` end-to-end — exported as `PLAYWRIGHT_BROWSERS_PATH` to every subprocess (including the `env KEY=VAL -- ...` wrapper used when we go through sudo), used to scope `executablePath()` hits on `load()`, and `uninstall()` resolves the real browser directory via `load()` then rmtrees it. When `install_root` is unset the provider is in pure passthrough mode: the caller's ambient `$PLAYWRIGHT_BROWSERS_PATH` (or playwright's `~/.cache/ms-playwright` default on Linux) flows through to subprocesses unchanged, `load()` trusts whatever path `executablePath()` reports, and `uninstall()` still rmtrees the real browser directory returned by `load()`.
 - Auto-switching: bootstraps the `playwright` npm package through `NpmProvider`, then runs `playwright install --with-deps <install_args>` against it. Resolves each installed browser's real executable via the `playwright-core` Node.js API (`chromium.executablePath()` etc.) and writes a symlink into `bin_dir` when one is configured.
 - `dry_run`: shared behavior — the install handler short-circuits to a placeholder without touching the host.
 - Privilege handling: `--with-deps` installs system packages and requires root on Linux. ``euid`` defaults to ``0``, which routes every ``exec()`` call through the base ``BinProvider.exec`` sudo-first-then-fallback path — it tries ``sudo -n -- playwright install --with-deps ...`` first on non-root hosts, falls back to running the command directly if sudo fails or isn't available, and merges both stderr outputs into the final error if both attempts fail.
diff --git a/abxpkg/binprovider_playwright.py b/abxpkg/binprovider_playwright.py
@@ -77,29 +77,16 @@ class PlaywrightProvider(BinProvider):
     # run as the normal user by default.
     euid: int | None = 0 if platform.system().lower() == "linux" else None
 
-    @computed_field
-    @property
-    def cache_dir(self) -> Path | None:
-        """``<install_root>/cache`` when managed, else ``None``.
-
-        Internal helper for the install/uninstall/load sites. When
-        ``install_root`` is unset we stay out of the way: the ambient
-        ``PLAYWRIGHT_BROWSERS_PATH`` (or playwright's
-        ``~/.cache/ms-playwright`` default) passes through to
-        subprocesses untouched, ``default_abspath_handler`` trusts
-        whatever path ``executablePath()`` reports, and
-        ``default_uninstall_handler`` skips rmtree of the user's cache.
-        """
-        if self.install_root is None:
-            return None
-        return self.install_root / "cache"
-
     @computed_field
     @property
     def ENV(self) -> "dict[str, str]":
-        if not self.cache_dir:
+        # In managed mode we pin ``PLAYWRIGHT_BROWSERS_PATH`` to
+        # ``<install_root>/cache``. In unmanaged mode we export nothing
+        # and the ambient env (or playwright's own
+        # ``~/.cache/ms-playwright`` default) passes through untouched.
+        if self.install_root is None:
             return {}
-        return {"PLAYWRIGHT_BROWSERS_PATH": str(self.cache_dir)}
+        return {"PLAYWRIGHT_BROWSERS_PATH": str(self.install_root / "cache")}
 
     def supports_min_release_age(self, action, no_cache: bool = False) -> bool:
         return False
@@ -308,8 +295,8 @@ def exec(
         # contain our bin_dir.
         env = self.build_exec_env(base_env=(kwargs.pop("env", None) or os.environ))
         env_assignments: list[str] = []
-        cache_dir = self.cache_dir
-        if cache_dir is not None:
+        if self.install_root is not None:
+            cache_dir = self.install_root / "cache"
             env["PLAYWRIGHT_BROWSERS_PATH"] = str(cache_dir)
             env_assignments.append(
                 f"PLAYWRIGHT_BROWSERS_PATH={cache_dir}",
@@ -587,14 +574,13 @@ def default_abspath_handler(
         )
         if not resolved:
             return None
-        # When ``cache_dir`` is pinned (either explicitly, via
-        # ``PLAYWRIGHT_BROWSERS_PATH``, or derived from ``install_root``),
-        # an ``executablePath()`` hit that points outside that tree
-        # (e.g. an ambient system install) should not satisfy ``load()``
-        # — otherwise an unrelated host-wide playwright install would
-        # silently hijack resolution.
-        if self.cache_dir is not None:
-            cache_real = self.cache_dir.resolve(strict=False)
+        # When ``install_root`` is pinned, an ``executablePath()`` hit
+        # that points outside our managed cache tree (e.g. an ambient
+        # system install) should not satisfy ``load()`` — otherwise an
+        # unrelated host-wide playwright install would silently hijack
+        # resolution.
+        if self.install_root is not None:
+            cache_real = (self.install_root / "cache").resolve(strict=False)
             if cache_real not in resolved.resolve(strict=False).parents:
                 return None
         if self.bin_dir is None:
diff --git a/abxpkg/binprovider_puppeteer.py b/abxpkg/binprovider_puppeteer.py
@@ -67,27 +67,16 @@ class PuppeteerProvider(BinProvider):
     # browser launch shims under ``<install_root>/bin``; global mode leaves it unset.
     bin_dir: Path | None = None
 
-    @computed_field
-    @property
-    def cache_dir(self) -> Path | None:
-        """``<install_root>/cache`` when managed, else ``None``.
-
-        Internal helper for the install/uninstall/load sites. When
-        ``install_root`` is unset we stay out of the way: the ambient
-        ``PUPPETEER_CACHE_DIR`` (or the CLI's ``~/.cache/puppeteer``
-        default) passes through to subprocesses untouched, and we
-        refuse to rmtree anything in the user's cache on ``uninstall``.
-        """
-        if self.install_root is None:
-            return None
-        return self.install_root / "cache"
-
     @computed_field
     @property
     def ENV(self) -> "dict[str, str]":
-        if self.cache_dir is None:
+        # In managed mode we pin ``PUPPETEER_CACHE_DIR`` to
+        # ``<install_root>/cache``. In unmanaged mode we export nothing
+        # and the ambient env (or puppeteer-browsers' own
+        # ``~/.cache/puppeteer`` default) passes through untouched.
+        if self.install_root is None:
             return {}
-        return {"PUPPETEER_CACHE_DIR": str(self.cache_dir)}
+        return {"PUPPETEER_CACHE_DIR": str(self.install_root / "cache")}
 
     def supports_postinstall_disable(self, action, no_cache: bool = False) -> bool:
         return action in ("install", "update")
@@ -210,7 +199,9 @@ def setup(
                 owner_paths=(
                     self.install_root,
                     self.bin_dir,
-                    self.cache_dir,
+                    self.install_root / "cache"
+                    if self.install_root is not None
+                    else None,
                     self.install_root / "npm"
                     if self.install_root is not None
                     else None,
@@ -246,10 +237,9 @@ def setup(
 
         if self.install_root is not None:
             self.install_root.mkdir(parents=True, exist_ok=True)
+            (self.install_root / "cache").mkdir(parents=True, exist_ok=True)
         if self.bin_dir is not None:
             self.bin_dir.mkdir(parents=True, exist_ok=True)
-        if self.cache_dir is not None:
-            self.cache_dir.mkdir(parents=True, exist_ok=True)
 
         cli_binary = self._cli_binary(
             postinstall_scripts=postinstall_scripts,
@@ -300,8 +290,8 @@ def _normalize_install_args(self, install_args: Iterable[str]) -> list[str]:
             if arg_str.startswith("--path="):
                 continue
             normalized.append(arg_str)
-        if self.cache_dir is not None:
-            normalized.append(f"--path={self.cache_dir}")
+        if self.install_root is not None:
+            normalized.append(f"--path={self.install_root / 'cache'}")
         return normalized
 
     def _list_installed_browsers(
@@ -315,8 +305,8 @@ def _list_installed_browsers(
         if not installer_bin:
             return []
         cmd = ["list"]
-        if self.cache_dir is not None:
-            cmd.append(f"--path={self.cache_dir}")
+        if self.install_root is not None:
+            cmd.append(f"--path={self.install_root / 'cache'}")
         proc = self.exec(
             bin_name=installer_bin,
             cmd=cmd,
@@ -448,10 +438,11 @@ def _cleanup_partial_browser_cache(
         install_output: str,
         browser_name: str,
     ) -> bool:
-        if self.cache_dir is None:
+        if self.install_root is None:
             return False
+        cache_dir = self.install_root / "cache"
         targets: set[Path] = set()
-        browser_cache_dir = self.cache_dir / browser_name
+        browser_cache_dir = cache_dir / browser_name
 
         missing_dir_match = re.search(
             r"browser folder \(([^)]+)\) exists but the executable",
@@ -473,7 +464,7 @@ def _cleanup_partial_browser_cache(
             targets.update(browser_cache_dir.glob(f"*{build_id}*"))
 
         removed_any = False
-        resolved_cache = self.cache_dir.resolve(strict=False)
+        resolved_cache = cache_dir.resolve(strict=False)
         for target in targets:
             resolved_target = target.resolve(strict=False)
             if not (
@@ -551,27 +542,25 @@ def _run_install_with_sudo(
             cwd=self.install_root or ".",
             timeout=self.install_timeout,
         )
-        if (
-            proc.returncode == 0
-            and self.cache_dir is not None
-            and self.cache_dir.exists()
-        ):
-            uid = os.getuid()
-            gid = os.getgid()
-            chown_proc = self.exec(
-                bin_name=sudo_binary.loaded_abspath,
-                cmd=["chown", "-R", f"{uid}:{gid}", str(self.cache_dir)],
-                cwd=self.install_root or ".",
-                timeout=30,
-                quiet=True,
-            )
-            if chown_proc.returncode != 0:
-                log_subprocess_output(
-                    logger,
-                    f"{self.__class__.__name__} sudo chown",
-                    chown_proc.stdout,
-                    chown_proc.stderr,
+        if proc.returncode == 0 and self.install_root is not None:
+            cache_dir = self.install_root / "cache"
+            if cache_dir.exists():
+                uid = os.getuid()
+                gid = os.getgid()
+                chown_proc = self.exec(
+                    bin_name=sudo_binary.loaded_abspath,
+                    cmd=["chown", "-R", f"{uid}:{gid}", str(cache_dir)],
+                    cwd=self.install_root or ".",
+                    timeout=30,
+                    quiet=True,
                 )
+                if chown_proc.returncode != 0:
+                    log_subprocess_output(
+                        logger,
+                        f"{self.__class__.__name__} sudo chown",
+                        chown_proc.stdout,
+                        chown_proc.stderr,
+                    )
         return proc
 
     @remap_kwargs({"packages": "install_args"})
