From b602201cccfe5312321089f1f38d08f730de07c7 Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 26 Aug 2026 23:07:22 +0800 Subject: [PATCH 1/6] feat: support Microsoft Edge, and key the daemon per browser endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Edge is Chromium and already spoke the same protocol, so the only Chrome-specific code was the default-profile lookup. Add a --browser flag (chrome|edge, env CHROME_BROWSER) that selects the vendor path table, and make the DevToolsActivePort error name the right browser and inspect URL instead of always saying Chrome. Channel aliases stay vendor-scoped (--channel chrome only for Chrome, edge only for Edge), and Edge Canary on Linux is rejected outright rather than resolved to a directory that cannot exist. Supporting a second browser exposed a pre-existing correctness bug: the daemon was keyed only to the uid and bound to whichever endpoint the first command resolved, so every later --browser/--user-data-dir flag was silently ignored. A Chrome-bound daemon would happily answer --browser edge commands and return Chrome's results. Key each daemon by a hash of its resolved ws URL instead. Chrome, Edge, each channel and each headless instance now get their own daemon and can be driven concurrently. The URL's browser GUID changes per launch, so a restarted browser gets a fresh daemon rather than inheriting a dead connection; the orphan exits on its existing idle timeout. Two notes on the implementation: - The lock file is deliberately NOT keyed. It only serializes the brief write-pid-then-bind window, and lock files are never removed, so a per-instance lock would accumulate a file per browser session forever. - The new info sidecar is another predictable name in shared /tmp, so write_pid_file_checked is generalized to write_private_file_checked (O_NOFOLLOW, O_NONBLOCK, mode 0600, deferred truncate) rather than using fs::write, which follows symlinks. kill-daemon is now target-scoped, so `--browser edge kill-daemon` cannot stop the Chrome daemon. --all stops every daemon for the user and is the only way to clear one whose browser has already exited, since a scoped kill has no endpoint left to resolve. --all also sweeps the pre-key chrome-devtools-daemon-.pid name so an upgrade does not strand an unreachable daemon. Add list-daemons (PID, browser, endpoint, uptime, state; --json supported). It reads only on-disk state, so it works when every browser has exited, and flags rows whose PID is dead as stale. Docs: the skill's "one daemon per user, bound to one Chrome" warning was backwards after this change, and the headless recipe's bare kill-daemon calls would have killed the user's daemon rather than the throwaway one — both corrected, and the recipe now warns that a temp profile is not automatically isolated in Edge, whose first-run import pulls open tabs and extensions from the default browser. Also documented that remote debugging is enabled by the persistent chrome://inspect/#remote-debugging toggle (edge://inspect for Edge), stored as devtools.remote_debugging.user-enabled, not only by a launch flag. Verified against a live Chrome and Edge at once: each command reached its own browser, scoped kill left the other daemon serving, --all cleared everything including stale and legacy files, and one lock file remained. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 47 +++- skill/chrome-devtools/SKILL.md | 97 ++++++-- src/browser.rs | 286 +++++++++++++++++---- src/client.rs | 30 ++- src/commands/executor.rs | 11 +- src/daemon.rs | 104 ++++++-- src/lib.rs | 442 +++++++++++++++++++++++++++------ src/main.rs | 5 +- src/protocol.rs | 179 ++++++++++++- 9 files changed, 1010 insertions(+), 191 deletions(-) diff --git a/README.md b/README.md index eec27cd..7697b32 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,14 @@ Chrome must have remote debugging enabled: ## Auto-connect +Enable remote debugging once per browser at `chrome://inspect/#remote-debugging` +(Edge: `edge://inspect/#remote-debugging`). It applies to the running process +immediately and persists across restarts — it is stored in the profile's +`Local State` as `devtools.remote_debugging.user-enabled`, so no launch flag is +needed. The toggle is per-browser: enabling it in Chrome does not enable it in +Edge. The `--remote-debugging-port` flag is the alternative, and is meant for a +throwaway instance rather than your everyday browser. + By default, the CLI reads `DevToolsActivePort` from Chrome's user data directory: | OS | Default path | @@ -80,14 +88,29 @@ By default, the CLI reads `DevToolsActivePort` from Chrome's user data directory | Linux | `~/.config/google-chrome/` | | Windows | `%LOCALAPPDATA%\Google\Chrome\User Data\` | -Override with `--user-data-dir`, `--channel` (beta/canary/dev), or `--ws-endpoint`. All three also read from environment variables: +Override with `--user-data-dir`, `--browser` (chrome/edge), `--channel` (beta/canary/dev), or `--ws-endpoint`. All four also read from environment variables: | Environment Variable | Corresponding Flag | |----------------------|--------------------| | `CHROME_WS_ENDPOINT` | `--ws-endpoint` | | `CHROME_USER_DATA_DIR` | `--user-data-dir` | +| `CHROME_BROWSER` | `--browser` | | `CHROME_CHANNEL` | `--channel` | +### Microsoft Edge + +Edge is Chromium and speaks the same DevTools Protocol, so every command works against it. Pass `--browser edge` to auto-connect to Edge's profile instead of Chrome's: + +| OS | Default path | +|----|-------------| +| macOS | `~/Library/Application Support/Microsoft Edge/` | +| Linux | `~/.config/microsoft-edge/` | +| Windows | `%LOCALAPPDATA%\Microsoft\Edge\User Data\` | + +`--channel` composes with it (`--browser edge --channel beta`). Edge Canary is not distributed for Linux, and that combination is rejected rather than pointed at a directory that cannot exist. An explicit `--ws-endpoint` or `--user-data-dir` needs no `--browser` — it only selects the default profile location. + +Enterprise-managed Edge can have remote debugging disabled by policy; `DevToolsActivePort` then never appears and auto-connect fails with the message above. That is the same failure mode as Chrome under the equivalent policy, just more common on managed fleets. + ## Page targeting Every page-level command outputs a friendly target name like `[target:red-snake]`. This is a deterministic word-pair derived from Chrome's internal target ID — same page always gets the same name. @@ -204,7 +227,7 @@ A drain without a `--duration` returns instantly. Adding `--duration N` switches |---------|-------------| | `kill-daemon` | Stop the background daemon cleanly | -`kill-daemon` signals the running daemon with `SIGTERM`, removes the socket and PID file, and exits. It's a no-op if no daemon is running. Prefer this over `pkill -f __daemon__` — the process name is shared by legitimate Chrome children processes. +`kill-daemon` signals the targeted daemon with `SIGTERM`, removes its socket, info and PID files, and exits. It's a no-op if no daemon is running. Prefer this over `pkill -f __daemon__` — the process name is shared by legitimate Chrome children processes. ## Global options @@ -217,22 +240,28 @@ A drain without a `--duration` returns instantly. Adding `--duration N` switches | `--block-url ` | Add a URL pattern to the active tab's block list (repeatable; persists until un-blocked or cleared) | | `--unblock-url ` | Remove a URL pattern from the active tab's block list (repeatable) | | `--ws-endpoint ` | Explicit WebSocket URL | -| `--user-data-dir ` | Custom Chrome profile directory | -| `--channel ` | Chrome channel (stable/beta/canary/dev) | +| `--user-data-dir ` | Custom browser profile directory | +| `--browser ` | Browser to auto-connect to (chrome/edge) | +| `--channel ` | Browser release channel (stable/beta/canary/dev) | + +Commands: `list-daemons` shows every running daemon; `kill-daemon [--all]` stops one or all. Global `--block-url` and `--unblock-url` update the **active tab's** block list and apply via `Network.setBlockedURLs`; the daemon re-applies each tab's list when that tab is in use, so blocking is isolated per tab. **Note:** Chrome only blocks *subresources* (images, scripts, fetch/XHR, stylesheets, CDN, trackers, fonts). The top-level navigation document itself is never blocked — e.g. `--block-url "*example.com*"` then `navigate https://example.com` still loads the page, but any `*.png`, `*.woff2`, etc. subresources on it are blocked. ## Daemon details -- **Endpoint (Unix)**: socket at `$TMPDIR/chrome-devtools-daemon-.sock` (uid-suffixed so users on a shared machine don't collide) +- **Instance identity**: one daemon per browser endpoint. The socket/PID/info filenames carry a 16-hex-digit key derived from the resolved `ws://` URL, so Chrome, Edge, every channel and every headless instance get a daemon of their own, and a command aimed at one browser can never be served by a daemon attached to another. The URL's browser GUID changes on every browser launch, so a restarted browser gets a fresh daemon instead of inheriting a dead connection; the orphan exits on its idle timeout. +- **Endpoint (Unix)**: socket at `$TMPDIR/chrome-devtools-daemon--.sock` (uid-suffixed so users on a shared machine don't collide) - **Endpoint (Windows)**: loopback TCP listener; its address is written to `%TEMP%\chrome-devtools-daemon.addr` (`%TEMP%` is already per-user, so no suffix) -- **PID file**: `$TMPDIR/chrome-devtools-daemon-.pid` (Windows: `%TEMP%\chrome-devtools-daemon.pid`) -- **Lock file**: `$TMPDIR/chrome-devtools-daemon-.lock` (Windows: `%TEMP%\chrome-devtools-daemon.lock`) — serializes daemon startup/cleanup; intentionally never removed automatically. Locks bind to the inode, not the name: deleting the file while any daemon process is still starting, running, or shutting down lets a new process lock a fresh replacement inode and bypass the serialization entirely. Only delete it once no daemon process exists at all — and there's rarely a reason to, since a leftover lock file is harmless. +- **PID file**: `$TMPDIR/chrome-devtools-daemon--.pid` (Windows: `%TEMP%\chrome-devtools-daemon-.pid`) +- **Info file**: `$TMPDIR/chrome-devtools-daemon--.info` — JSON naming the browser, endpoint, PID and start time, so `list-daemons` can label rows. Best-effort: a daemon with no info file still lists, with `?` columns. +- **Lock file**: `$TMPDIR/chrome-devtools-daemon-.lock` (Windows: `%TEMP%\chrome-devtools-daemon.lock`) — **not** keyed per instance: one lock covers all of them, because it only serializes the brief write-pid-then-bind window, while a per-instance lock would accumulate a never-removed file per browser session. Serializes daemon startup/cleanup; intentionally never removed automatically. Locks bind to the inode, not the name: deleting the file while any daemon process is still starting, running, or shutting down lets a new process lock a fresh replacement inode and bypass the serialization entirely. Only delete it once no daemon process exists at all — and there's rarely a reason to, since a leftover lock file is harmless. - **Idle timeout**: 5 minutes (auto-exits, cleans up its files) - **Cleanup**: endpoint + PID files are also removed on panics, and on Unix on SIGTERM/SIGINT; Windows Ctrl-C cleanup is best-effort only (a background daemon has no console to receive it). SIGQUIT, SIGHUP and SIGKILL skip cleanup by design — the leftover files are harmless and are reclaimed by the next daemon start. - **Protocol**: Length-prefixed JSON over the Unix socket / loopback TCP -- **Spawned by**: First CLI invocation (transparent to user) -- **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file; leave the lock file — see above). It sends SIGTERM and returns once the signal is delivered, not once the process is gone: the daemon exits *between* requests, so one that is mid-command finishes it and answers that client first. Expect up to one command's latency, and note that a daemon wedged inside a CDP call outlives the command that stopped it. +- **Spawned by**: First CLI invocation for a given endpoint (transparent to user) +- **List**: `chrome-devtools list-daemons` — PID, browser, endpoint, uptime and state for every daemon this user owns. `--json` for machine-readable output. Reads only on-disk state, so it works when every browser has exited; rows whose PID no longer exists are marked `stale`. +- **Kill**: `chrome-devtools kill-daemon` stops only the daemon for the endpoint its flags resolve to, so `--browser edge kill-daemon` cannot stop your Chrome daemon. Add `--all` to stop every daemon for this user — which is also the only way to clear one whose browser has already exited, since a scoped kill has no endpoint left to resolve (it fails and says so). `--all` also sweeps the pre-key `chrome-devtools-daemon-.pid` name left by older versions. (Or delete the socket + PID file by hand; leave the lock file — see above.) It sends SIGTERM and returns once the signal is delivered, not once the process is gone: the daemon exits *between* requests, so one that is mid-command finishes it and answers that client first. Expect up to one command's latency, and note that a daemon wedged inside a CDP call outlives the command that stopped it. - **Kill (Windows)**: not supported — `kill-daemon` says so and exits, and a backgrounded daemon has no console for Ctrl-C. Use `taskkill /PID ` with the PID from `%TEMP%\chrome-devtools-daemon.pid`, or wait out the idle timeout. The daemon keeps a persistent CDP session on the current page to: diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index a6d52bb..c4ca976 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -10,12 +10,43 @@ A CLI that talks directly to your running Chrome via the DevTools Protocol. ## Prerequisites -Chrome must have remote debugging enabled: -1. Open Chrome -2. Go to `chrome://inspect/#remote-debugging` +The browser must have remote debugging enabled: +1. Open Chrome (or Edge) +2. Go to `chrome://inspect/#remote-debugging` (Edge: `edge://inspect/#remote-debugging`) 3. Enable the remote debugging server -The CLI auto-connects — no URL needed. A daemon is spawned on first invocation and reused across commands (5-minute idle timeout). +This is a **persistent, in-browser toggle** — not a launch flag. Three things +follow from that, and they are what make it the right route for attaching to an +everyday browser: + +- **It takes effect immediately.** The server starts on the already-running + process; no restart, no relaunch, nothing to quit. +- **It survives restarts.** The choice is stored in the profile's `Local State` + as `devtools.remote_debugging.user-enabled`, so every later launch serves a + port with no flags. To check whether a profile has it on, read that key. +- **It is per-browser and per-profile.** Enabling it in Chrome does nothing for + Edge, and vice versa — each has its own toggle and its own `Local State`. + +The port is chosen by the browser and recorded in `/DevToolsActivePort`. +Read that file rather than assuming a number: it differs per browser and can +change between launches. Auto-connect does this for you, which is why no URL is +needed. A daemon is spawned on first invocation and reused across commands +(5-minute idle timeout). + +**Microsoft Edge** works the same — it is Chromium and speaks the same protocol. +Add `--browser edge` so auto-connect reads Edge's profile instead of Chrome's +(`--channel` still selects stable/beta/dev/canary). `--ws-endpoint` and +`--user-data-dir` need no `--browser`; they already say where to connect. +Everything below applies unchanged — only the profile location differs. + +The `--remote-debugging-port` launch flag is the *other* route, and it is for a +throwaway instance rather than your everyday browser — see the headless recipe +below. Two traps if you reach for it: it is ignored when that profile is already +running (the launch hands off to the existing process and no port file appears, +so quit first or use a different `--user-data-dir`), and a fresh Edge profile is +not the clean room it looks like — Edge's first-run import pulls open tabs and +extensions from your default browser, so a `mktemp -d` profile can come up +holding your real session. ## ⚠️ Critical: How Page Targeting Works @@ -377,9 +408,10 @@ prompt ever appears** — the whole flow runs unattended. ```bash PROFILE=$(mktemp -d) -# 1. If a daemon is already attached to the user's real Chrome, stop it first -# (the daemon is per-user and sticks to whichever Chrome it first connected to) -chrome-devtools kill-daemon --force +# 1. Clear any daemon left over from a previous run of THIS profile. Not needed +# for isolation — daemons are per-endpoint, so the user's browser is +# unaffected either way — but a stale one here would hold a dead connection. +chrome-devtools --user-data-dir "$PROFILE" kill-daemon --force 2>/dev/null # 2. Spawn headless Chrome with an isolated profile; port 0 = pick a free port "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" \ @@ -389,11 +421,13 @@ chrome-devtools kill-daemon --force about:blank & CHROME_PID=$! -# 3. Cleanup — REQUIRED even if a later step fails: the daemon is bound to the -# headless instance and would otherwise hijack later commands aimed at the -# user's real Chrome. A trap runs it on every exit path, not just success. +# 3. Cleanup — a trap runs it on every exit path, not just success. The daemon +# can no longer hijack commands aimed at another browser, but it does hold a +# CDP connection and an idle timer, so stop it rather than leaking it. cleanup() { - chrome-devtools kill-daemon --force + # Scoped to this profile: a bare kill-daemon would resolve the user's default + # Chrome profile and stop their daemon instead of this one. + chrome-devtools --user-data-dir "$PROFILE" kill-daemon --force kill "$CHROME_PID" 2>/dev/null # Chrome shuts down asynchronously, so deleting the profile right after # SIGTERM races its teardown and can leave it running against a directory @@ -431,13 +465,40 @@ chrome-devtools --user-data-dir "$PROFILE" screenshot --output /tmp/shot.png ``` Linux path: `google-chrome` or `chromium` on `$PATH` replaces the macOS -`.app` binary path. - -**⚠️ One daemon per user, bound to one Chrome.** The daemon connects to -whichever Chrome the first command resolved, and later commands reuse it even -if their flags point elsewhere. Always `kill-daemon --force` when switching -between the user's Chrome and a headless instance — in both directions -(step 1 and the EXIT trap above). +`.app` binary path. For a headless Edge, swap the binary for +`/Applications/Microsoft Edge.app/Contents/MacOS/Microsoft Edge` (Linux: +`microsoft-edge`) — the flags are identical, and `--user-data-dir` already +points the CLI at the right profile, so `--browser edge` is optional here. + +**⚠️ A temp profile is not automatically a clean room in Edge.** Edge's +first-run import pulls open tabs *and* extensions from the default browser, and +`--no-first-run` does not reliably suppress it, so a `mktemp -d` profile can come +up holding a copy of the real browsing session — and the debug port then fronts a +signed-in profile. Verify what you actually got with `list-pages` before +assuming isolation, and treat the port as sensitive until you have. + +**One daemon per browser endpoint.** A daemon is identified by the endpoint it +is attached to, so the user's Chrome, the user's Edge, and each headless +instance each get their own — you can drive them concurrently, and a command +can never be answered by a daemon attached to a different browser. Two +consequences for the recipe above: + +- Step 1 is not needed for isolation. It only clears a daemon left over from a + *previous* run of this same profile. +- The EXIT trap must scope its kill to this profile: + `chrome-devtools --user-data-dir "$PROFILE" kill-daemon --force`. A bare + `kill-daemon` resolves the default Chrome profile and would stop the user's + daemon instead of the headless one. + +`list-daemons` shows what is running (PID, browser, endpoint, uptime), and +`kill-daemon --all` clears every daemon regardless of endpoint — including ones +whose browser has already exited, which a scoped kill cannot reach because it +has no endpoint left to resolve. + +Older versions ran a single daemon per user, bound to whichever browser the +first command resolved, and silently ignored later `--browser`/`--user-data-dir` +flags. If you are reading advice that says to `kill-daemon` when switching +browsers, it predates this. ## Complete Command Reference diff --git a/src/browser.rs b/src/browser.rs index 3482dfb..33f5a51 100644 --- a/src/browser.rs +++ b/src/browser.rs @@ -1,7 +1,7 @@ use anyhow::{anyhow, bail, Result}; use std::path::{Path, PathBuf}; -/// Resolve the WebSocket URL for connecting to Chrome. +/// Resolve the WebSocket URL for connecting to the browser. /// /// Priority: /// 1. Explicit --ws-endpoint @@ -9,33 +9,38 @@ use std::path::{Path, PathBuf}; pub fn resolve_ws_url( ws_endpoint: Option<&str>, user_data_dir: Option<&str>, + browser: &str, channel: &str, ) -> Result { if let Some(ws) = ws_endpoint { return Ok(ws.to_string()); } - // Auto-connect: read DevToolsActivePort from Chrome's user data directory + let browser = Browser::parse(browser)?; + + // Auto-connect: read DevToolsActivePort from the browser's user data directory let data_dir = match user_data_dir { Some(dir) => PathBuf::from(dir), - None => default_chrome_user_data_dir(channel)?, + None => browser.default_user_data_dir(channel)?, }; - read_devtools_active_port(&data_dir) + read_devtools_active_port(&data_dir, browser) } /// Read DevToolsActivePort file and construct the WebSocket URL. -fn read_devtools_active_port(user_data_dir: &Path) -> Result { +fn read_devtools_active_port(user_data_dir: &Path, browser: Browser) -> Result { let port_path = user_data_dir.join("DevToolsActivePort"); + let label = browser.label(); let content = std::fs::read_to_string(&port_path).map_err(|_| { anyhow!( "Could not read DevToolsActivePort at {}\n\n\ - Make sure Chrome is running with remote debugging enabled:\n\ - 1. Open Chrome\n\ - 2. Go to chrome://inspect/#remote-debugging\n\ + Make sure {label} is running with remote debugging enabled:\n\ + 1. Open {label}\n\ + 2. Go to {}://inspect/#remote-debugging\n\ 3. Enable the remote debugging server", - port_path.display() + port_path.display(), + browser.scheme() ) })?; @@ -57,54 +62,247 @@ fn read_devtools_active_port(user_data_dir: &Path) -> Result { .map_err(|_| anyhow!("Invalid port '{}' in DevToolsActivePort", lines[0]))?; if port == 0 { - bail!("Port 0 in DevToolsActivePort — Chrome may not be running"); + bail!("Port 0 in DevToolsActivePort — {label} may not be running"); } let path = lines[1]; Ok(format!("ws://127.0.0.1:{port}{path}")) } -/// Get the default Chrome user data directory for the given channel. -fn default_chrome_user_data_dir(channel: &str) -> Result { +/// A Chromium-based browser the CLI knows how to auto-connect to. +/// +/// Both speak the same DevTools Protocol; they differ only in where the +/// profile (and therefore `DevToolsActivePort`) lives. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum Browser { + Chrome, + Edge, +} + +impl Browser { + fn parse(name: &str) -> Result { + match name { + "chrome" => Ok(Self::Chrome), + "edge" | "msedge" => Ok(Self::Edge), + _ => bail!("Unknown browser: {name} (expected 'chrome' or 'edge')"), + } + } + + /// Human-readable name, for error messages. + fn label(self) -> &'static str { + match self { + Self::Chrome => "Chrome", + Self::Edge => "Microsoft Edge", + } + } + + /// URL scheme for the `://inspect` hint. + fn scheme(self) -> &'static str { + match self { + Self::Chrome => "chrome", + Self::Edge => "edge", + } + } + + /// Default user data directory for the given release channel. + fn default_user_data_dir(self, channel: &str) -> Result { + #[cfg(target_os = "macos")] + { + let home = + dirs::home_dir().ok_or_else(|| anyhow!("Cannot determine home directory"))?; + let base = home.join("Library/Application Support"); + let dir = match (self, channel) { + (Self::Chrome, "stable" | "chrome") => base.join("Google/Chrome"), + (Self::Chrome, "beta") => base.join("Google/Chrome Beta"), + (Self::Chrome, "canary") => base.join("Google/Chrome Canary"), + (Self::Chrome, "dev") => base.join("Google/Chrome Dev"), + (Self::Edge, "stable" | "edge") => base.join("Microsoft Edge"), + (Self::Edge, "beta") => base.join("Microsoft Edge Beta"), + (Self::Edge, "canary") => base.join("Microsoft Edge Canary"), + (Self::Edge, "dev") => base.join("Microsoft Edge Dev"), + _ => bail!("Unknown {} channel: {channel}", self.label()), + }; + Ok(dir) + } + + #[cfg(target_os = "linux")] + { + let home = + dirs::home_dir().ok_or_else(|| anyhow!("Cannot determine home directory"))?; + let dir = match (self, channel) { + (Self::Chrome, "stable" | "chrome") => home.join(".config/google-chrome"), + (Self::Chrome, "beta") => home.join(".config/google-chrome-beta"), + // Chrome ships no Canary for Linux; unstable is the dev channel. + (Self::Chrome, "canary" | "dev") => home.join(".config/google-chrome-unstable"), + (Self::Edge, "stable" | "edge") => home.join(".config/microsoft-edge"), + (Self::Edge, "beta") => home.join(".config/microsoft-edge-beta"), + (Self::Edge, "dev") => home.join(".config/microsoft-edge-dev"), + (Self::Edge, "canary") => { + bail!("Microsoft Edge Canary is not distributed for Linux") + } + _ => bail!("Unknown {} channel: {channel}", self.label()), + }; + Ok(dir) + } + + #[cfg(target_os = "windows")] + { + let local_app_data = + std::env::var("LOCALAPPDATA").map_err(|_| anyhow!("LOCALAPPDATA not set"))?; + let base = PathBuf::from(local_app_data); + let dir = match (self, channel) { + (Self::Chrome, "stable" | "chrome") => base.join("Google/Chrome/User Data"), + (Self::Chrome, "beta") => base.join("Google/Chrome Beta/User Data"), + (Self::Chrome, "canary") => base.join("Google/Chrome SxS/User Data"), + (Self::Chrome, "dev") => base.join("Google/Chrome Dev/User Data"), + (Self::Edge, "stable" | "edge") => base.join("Microsoft/Edge/User Data"), + (Self::Edge, "beta") => base.join("Microsoft/Edge Beta/User Data"), + (Self::Edge, "canary") => base.join("Microsoft/Edge SxS/User Data"), + (Self::Edge, "dev") => base.join("Microsoft/Edge Dev/User Data"), + _ => bail!("Unknown {} channel: {channel}", self.label()), + }; + Ok(dir) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_known_browsers() { + assert_eq!(Browser::parse("chrome").unwrap(), Browser::Chrome); + assert_eq!(Browser::parse("edge").unwrap(), Browser::Edge); + assert_eq!(Browser::parse("msedge").unwrap(), Browser::Edge); + } + + #[test] + fn rejects_unknown_browser() { + let err = Browser::parse("firefox").unwrap_err().to_string(); + assert!(err.contains("Unknown browser: firefox"), "{err}"); + } + + #[test] + fn rejects_cross_browser_channel_alias() { + // "chrome" is a stable alias for Chrome only, "edge" for Edge only. + assert!(Browser::Edge.default_user_data_dir("chrome").is_err()); + assert!(Browser::Chrome.default_user_data_dir("edge").is_err()); + } + + #[test] + fn rejects_unknown_channel() { + let err = Browser::Edge + .default_user_data_dir("nightly") + .unwrap_err() + .to_string(); + assert!(err.contains("Microsoft Edge channel: nightly"), "{err}"); + } + + #[test] + fn stable_and_self_named_channels_agree() { + for browser in [Browser::Chrome, Browser::Edge] { + let stable = browser.default_user_data_dir("stable").unwrap(); + let alias = browser.default_user_data_dir(browser.scheme()).unwrap(); + assert_eq!(stable, alias); + } + } + + #[test] + fn browsers_resolve_to_distinct_dirs() { + let chrome = Browser::Chrome.default_user_data_dir("stable").unwrap(); + let edge = Browser::Edge.default_user_data_dir("stable").unwrap(); + assert_ne!(chrome, edge); + } + + #[cfg(target_os = "macos")] + #[test] + fn macos_edge_paths() { + let home = dirs::home_dir().unwrap(); + let base = home.join("Library/Application Support"); + for (channel, expected) in [ + ("stable", "Microsoft Edge"), + ("beta", "Microsoft Edge Beta"), + ("dev", "Microsoft Edge Dev"), + ("canary", "Microsoft Edge Canary"), + ] { + assert_eq!( + Browser::Edge.default_user_data_dir(channel).unwrap(), + base.join(expected), + "channel {channel}" + ); + } + } + #[cfg(target_os = "macos")] - { - let home = dirs::home_dir().ok_or_else(|| anyhow!("Cannot determine home directory"))?; + #[test] + fn macos_chrome_paths_unchanged() { + let home = dirs::home_dir().unwrap(); let base = home.join("Library/Application Support/Google"); - let dir = match channel { - "stable" | "chrome" => base.join("Chrome"), - "beta" => base.join("Chrome Beta"), - "canary" => base.join("Chrome Canary"), - "dev" => base.join("Chrome Dev"), - _ => bail!("Unknown Chrome channel: {channel}"), - }; - Ok(dir) + for (channel, expected) in [ + ("stable", "Chrome"), + ("beta", "Chrome Beta"), + ("dev", "Chrome Dev"), + ("canary", "Chrome Canary"), + ] { + assert_eq!( + Browser::Chrome.default_user_data_dir(channel).unwrap(), + base.join(expected), + "channel {channel}" + ); + } } #[cfg(target_os = "linux")] - { - let home = dirs::home_dir().ok_or_else(|| anyhow!("Cannot determine home directory"))?; - let dir = match channel { - "stable" | "chrome" => home.join(".config/google-chrome"), - "beta" => home.join(".config/google-chrome-beta"), - "canary" => home.join(".config/google-chrome-unstable"), - "dev" => home.join(".config/google-chrome-unstable"), - _ => bail!("Unknown Chrome channel: {channel}"), - }; - Ok(dir) + #[test] + fn linux_edge_paths() { + let home = dirs::home_dir().unwrap(); + for (channel, expected) in [ + ("stable", ".config/microsoft-edge"), + ("beta", ".config/microsoft-edge-beta"), + ("dev", ".config/microsoft-edge-dev"), + ] { + assert_eq!( + Browser::Edge.default_user_data_dir(channel).unwrap(), + home.join(expected), + "channel {channel}" + ); + } + assert!(Browser::Edge.default_user_data_dir("canary").is_err()); } #[cfg(target_os = "windows")] - { - let local_app_data = - std::env::var("LOCALAPPDATA").map_err(|_| anyhow!("LOCALAPPDATA not set"))?; - let base = PathBuf::from(local_app_data).join("Google"); - let dir = match channel { - "stable" | "chrome" => base.join("Chrome/User Data"), - "beta" => base.join("Chrome Beta/User Data"), - "canary" => base.join("Chrome SxS/User Data"), - "dev" => base.join("Chrome Dev/User Data"), - _ => bail!("Unknown Chrome channel: {channel}"), - }; - Ok(dir) + #[test] + fn windows_edge_paths() { + let base = PathBuf::from(std::env::var("LOCALAPPDATA").unwrap()); + for (channel, expected) in [ + ("stable", "Microsoft/Edge/User Data"), + ("beta", "Microsoft/Edge Beta/User Data"), + ("dev", "Microsoft/Edge Dev/User Data"), + ("canary", "Microsoft/Edge SxS/User Data"), + ] { + assert_eq!( + Browser::Edge.default_user_data_dir(channel).unwrap(), + base.join(expected), + "channel {channel}" + ); + } + } + + #[test] + fn ws_endpoint_short_circuits_browser_validation() { + // An explicit endpoint needs no profile, so the browser is irrelevant. + let ws = resolve_ws_url(Some("ws://127.0.0.1:9222/x"), None, "firefox", "stable").unwrap(); + assert_eq!(ws, "ws://127.0.0.1:9222/x"); + } + + #[test] + fn active_port_error_names_the_browser() { + let dir = std::env::temp_dir().join("chrome-devtools-cli-nonexistent-profile"); + let err = resolve_ws_url(None, Some(dir.to_str().unwrap()), "edge", "stable") + .unwrap_err() + .to_string(); + assert!(err.contains("Microsoft Edge is running"), "{err}"); + assert!(err.contains("edge://inspect"), "{err}"); } } diff --git a/src/client.rs b/src/client.rs index aa812b3..f87ea14 100644 --- a/src/client.rs +++ b/src/client.rs @@ -10,13 +10,13 @@ use tokio::net::UnixStream; use crate::protocol::*; #[cfg(unix)] -async fn connect_daemon() -> Result { - Ok(UnixStream::connect(socket_path()).await?) +async fn connect_daemon(key: &str) -> Result { + Ok(UnixStream::connect(socket_path(key)).await?) } #[cfg(windows)] -async fn connect_daemon() -> Result { - let addr = std::fs::read_to_string(addr_path())?; +async fn connect_daemon(key: &str) -> Result { + let addr = std::fs::read_to_string(addr_path(key))?; Ok(TcpStream::connect(addr.trim()).await?) } @@ -32,9 +32,10 @@ pub(crate) fn daemon_wait_timeout() -> Duration { .unwrap_or(Duration::from_secs(5)) } -/// Try to send a request to the daemon. Returns error if daemon is not running. -pub async fn send_to_daemon(request: &DaemonRequest) -> Result { - let mut stream = connect_daemon().await?; +/// Try to send a request to the daemon for `key`. Returns an error if no +/// daemon is attached to that endpoint. +pub async fn send_to_daemon(key: &str, request: &DaemonRequest) -> Result { + let mut stream = connect_daemon(key).await?; let req_bytes = serde_json::to_vec(request)?; write_msg(&mut stream, &req_bytes).await?; @@ -45,10 +46,14 @@ pub async fn send_to_daemon(request: &DaemonRequest) -> Result { } /// Spawn the daemon process in the background. -pub fn spawn_daemon(ws_url: &str) -> Result<()> { +/// +/// `browser` is descriptive only — it is recorded in the daemon's info file so +/// `list-daemons` can name the browser. The daemon's identity comes from +/// `ws_url` alone. +pub fn spawn_daemon(ws_url: &str, browser: &str) -> Result<()> { let exe = std::env::current_exe()?; let mut cmd = std::process::Command::new(&exe); - cmd.args(["__daemon__", ws_url]) + cmd.args(["__daemon__", ws_url, browser]) .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()); @@ -60,8 +65,9 @@ pub fn spawn_daemon(ws_url: &str) -> Result<()> { Ok(()) } -/// Wait for the daemon socket to become available, with exponential backoff. -pub async fn wait_for_daemon() -> Result<()> { +/// Wait for the daemon socket for `key` to become available, with exponential +/// backoff. +pub async fn wait_for_daemon(key: &str) -> Result<()> { let deadline = tokio::time::Instant::now() + daemon_wait_timeout(); let mut delay = Duration::from_millis(50); loop { @@ -71,7 +77,7 @@ pub async fn wait_for_daemon() -> Result<()> { daemon_wait_timeout().as_secs() ); } - if connect_daemon().await.is_ok() { + if connect_daemon(key).await.is_ok() { return Ok(()); } // Simple jitter based on current time subseconds diff --git a/src/commands/executor.rs b/src/commands/executor.rs index 75f4bcf..e936f78 100644 --- a/src/commands/executor.rs +++ b/src/commands/executor.rs @@ -88,7 +88,8 @@ pub fn known_args(cmd: &str) -> &'static [&'static str] { "output", "track_navigation", ], - "kill-daemon" => &["force"], + "kill-daemon" => &["force", "all"], + "list-daemons" => &[], _ => &[], } } @@ -130,7 +131,10 @@ fn validate_args(cmd: &str, args: &serde_json::Value) -> Result<()> { /// rather than hitting the `_ => unreachable!()` arm in the browser-level /// dispatch and panicking. fn is_browser_level(cmd: &str) -> bool { - matches!(cmd, "list-pages" | "new-page" | "sw-logs" | "kill-daemon") + matches!( + cmd, + "list-pages" | "new-page" | "sw-logs" | "kill-daemon" | "list-daemons" + ) } /// Execute a single command from a [`DaemonRequest`]. @@ -207,6 +211,9 @@ pub async fn execute_command(client: &mut CdpClient, req: &DaemonRequest) -> Res commands::sw_logs::collect_sw_logs(client, duration, extension_id, req.format()) .await } + "list-daemons" => Ok(CommandResult::output( + "list-daemons is handled directly by the CLI, not the daemon.", + )), "kill-daemon" => Ok(CommandResult::output( "kill-daemon is handled directly by the CLI, not the daemon.", )), diff --git a/src/daemon.rs b/src/daemon.rs index 2d0b1f8..8b13bf4 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -95,7 +95,7 @@ fn open_lock_file_at(path: &std::path::Path) -> Result { /// pass the uid check, but planting one requires owning the target under /// Linux's default `protected_hardlinks`. #[cfg(unix)] -fn write_pid_file_checked(path: &std::path::Path) -> Result<()> { +fn write_private_file_checked(path: &std::path::Path, contents: &[u8], what: &str) -> Result<()> { use std::io::Write; use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; let mut f = std::fs::OpenOptions::new() @@ -106,10 +106,10 @@ fn write_pid_file_checked(path: &std::path::Path) -> Result<()> { .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) .mode(0o600) .open(path) - .with_context(|| format!("Failed to open daemon PID file {}", path.display()))?; + .with_context(|| format!("Failed to open daemon {what} file {}", path.display()))?; let md = f.metadata().with_context(|| { format!( - "Failed to read metadata of daemon PID file {}", + "Failed to read metadata of daemon {what} file {}", path.display() ) })?; @@ -117,22 +117,53 @@ fn write_pid_file_checked(path: &std::path::Path) -> Result<()> { // thread-safe and cannot fail. if !md.is_file() || md.uid() != unsafe { libc::geteuid() } { anyhow::bail!( - "Daemon PID path {} is not a regular file owned by the current user; refusing to write it", + "Daemon {what} path {} is not a regular file owned by the current user; refusing to write it", path.display() ); } f.set_len(0) - .with_context(|| format!("Failed to truncate daemon PID file {}", path.display()))?; - f.write_all(std::process::id().to_string().as_bytes()) - .with_context(|| format!("Failed to write daemon PID file {}", path.display())) + .with_context(|| format!("Failed to truncate daemon {what} file {}", path.display()))?; + f.write_all(contents) + .with_context(|| format!("Failed to write daemon {what} file {}", path.display())) } /// Windows has no O_NOFOLLOW, and `%TEMP%` is already per-user, so the /// shared-/tmp squatting the Unix version defends against doesn't apply. #[cfg(not(unix))] +fn write_private_file_checked(path: &std::path::Path, contents: &[u8], what: &str) -> Result<()> { + std::fs::write(path, contents) + .with_context(|| format!("Failed to write daemon {what} file {}", path.display())) +} + +/// Write this process's PID to `path` (see [`write_private_file_checked`]). fn write_pid_file_checked(path: &std::path::Path) -> Result<()> { - std::fs::write(path, std::process::id().to_string()) - .with_context(|| format!("Failed to write daemon PID file {}", path.display())) + write_private_file_checked(path, std::process::id().to_string().as_bytes(), "PID") +} + +/// Publish the daemon's metadata sidecar, so `list-daemons` can name the +/// browser without inspecting process arguments. +/// +/// Best-effort by design: readers tolerate a missing info file, so a failure +/// here is reported to the daemon's (usually null) stderr rather than failing +/// startup — losing a display column must not cost a working daemon. +fn write_info_file(key: &str, browser: &str, ws_url: &str) { + let info = crate::protocol::DaemonInfo { + browser: browser.to_string(), + ws_url: ws_url.to_string(), + pid: std::process::id(), + started_unix: std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |d| d.as_secs()), + }; + let path = crate::protocol::info_path(key); + match serde_json::to_vec(&info) { + Ok(bytes) => { + if let Err(e) = write_private_file_checked(&path, &bytes, "info") { + eprintln!("daemon: could not write info file: {e:#}"); + } + } + Err(e) => eprintln!("daemon: could not serialize info file: {e}"), + } } /// Ceiling on the lock wait. Legitimate holders (a predecessor's cleanup, @@ -213,17 +244,32 @@ async fn lock_daemon_files() -> Result { /// place and self-heal on the next daemon start), so the messages exist for /// interactive debugging, not operational monitoring. fn cleanup() { + // Published by run_daemon before the guard is armed. Unset means nothing + // has been written yet, so there is nothing to remove. + let Some(key) = INSTANCE_KEY.get() else { + return; + }; #[cfg(unix)] - let endpoint = socket_path(); + let endpoint = socket_path(key); #[cfg(windows)] - let endpoint = addr_path(); - cleanup_at(&lock_path(), &pid_path(), &endpoint); + let endpoint = addr_path(key); + cleanup_at(&lock_path(), &pid_path(key), &endpoint, &info_path(key)); } +/// This daemon's instance key, so [`cleanup`] can derive its paths when it +/// runs from a `Drop` guard, a signal handler or the panic hook — none of +/// which can be passed an argument. +static INSTANCE_KEY: std::sync::OnceLock = std::sync::OnceLock::new(); + /// Path-parameterized body of [`cleanup`] (see its doc for the locking and /// ownership rules), so tests can drive it against a scratch directory /// instead of the real `temp_dir()` files. -fn cleanup_at(lock: &std::path::Path, pid: &std::path::Path, endpoint: &std::path::Path) { +fn cleanup_at( + lock: &std::path::Path, + pid: &std::path::Path, + endpoint: &std::path::Path, + info: &std::path::Path, +) { // `lock_file` holds the OS lock until it drops at the end of this scope, // covering the ownership check and removals below. let lock_file = match open_lock_file_at(lock) { @@ -266,6 +312,7 @@ fn cleanup_at(lock: &std::path::Path, pid: &std::path::Path, endpoint: &std::pat return; } let _ = std::fs::remove_file(endpoint); + let _ = std::fs::remove_file(info); let _ = std::fs::remove_file(pid); } @@ -381,7 +428,12 @@ fn shutdown_signal() -> impl std::future::Future { } } -pub async fn run_daemon(ws_url: &str) -> Result<()> { +pub async fn run_daemon(ws_url: &str, browser: &str) -> Result<()> { + // Published before the guard is armed: this only sets an in-memory cell — + // nothing on disk yet — and cleanup() needs it to find the files at all. + let key = crate::protocol::instance_key(ws_url); + let key = INSTANCE_KEY.get_or_init(|| key).clone(); + // Armed before anything is written: cleanup() verifies pid-file ownership // first, so firing "too early" is a no-op, and this declaration order // means the startup lock below is released (locals drop in reverse order) @@ -401,12 +453,13 @@ pub async fn run_daemon(ws_url: &str) -> Result<()> { // predecessor can delete files this daemon just claimed. let startup_lock = lock_daemon_files().await?; - write_pid_file_checked(&pid_path())?; + write_pid_file_checked(&pid_path(&key))?; + write_info_file(&key, browser, ws_url); #[cfg(unix)] let listener = { // Clean up stale socket - let sock = socket_path(); + let sock = socket_path(&key); let _ = std::fs::remove_file(&sock); // Bind socket FIRST so the CLI knows the daemon is alive and can connect. @@ -418,13 +471,13 @@ pub async fn run_daemon(ws_url: &str) -> Result<()> { #[cfg(windows)] let listener = { // Clean up stale address file - let _ = std::fs::remove_file(addr_path()); + let _ = std::fs::remove_file(addr_path(&key)); // Bind listener FIRST so the CLI knows the daemon is alive and can connect. // If we wait for CdpClient::connect first, a Chrome/network permission prompt // can block the daemon and cause the CLI's 5-second wait_for_daemon timeout to expire. let listener = TcpListener::bind("127.0.0.1:0").await?; - std::fs::write(addr_path(), listener.local_addr()?.to_string())?; + std::fs::write(addr_path(&key), listener.local_addr()?.to_string())?; listener }; @@ -671,14 +724,17 @@ mod tests { let lock = dir.path().join("daemon.lock"); let pid = dir.path().join("daemon.pid"); let endpoint = dir.path().join("daemon.sock"); + let info = dir.path().join("daemon.info"); std::fs::write(&pid, std::process::id().wrapping_add(1).to_string()).unwrap(); std::fs::write(&endpoint, "").unwrap(); - cleanup_at(&lock, &pid, &endpoint); + std::fs::write(&info, "{}").unwrap(); + cleanup_at(&lock, &pid, &endpoint, &info); assert!(pid.exists(), "foreign PID file must survive cleanup"); assert!( endpoint.exists(), "foreign endpoint file must survive cleanup" ); + assert!(info.exists(), "foreign info file must survive cleanup"); } #[test] @@ -687,11 +743,14 @@ mod tests { let lock = dir.path().join("daemon.lock"); let pid = dir.path().join("daemon.pid"); let endpoint = dir.path().join("daemon.sock"); + let info = dir.path().join("daemon.info"); std::fs::write(&pid, std::process::id().to_string()).unwrap(); std::fs::write(&endpoint, "").unwrap(); - cleanup_at(&lock, &pid, &endpoint); + std::fs::write(&info, "{}").unwrap(); + cleanup_at(&lock, &pid, &endpoint, &info); assert!(!pid.exists()); assert!(!endpoint.exists()); + assert!(!info.exists(), "own info file must be removed"); assert!( lock.exists(), "the lock file is intentionally never removed" @@ -706,14 +765,17 @@ mod tests { let lock = dir.path().join("daemon.lock"); let pid = dir.path().join("daemon.pid"); let endpoint = dir.path().join("daemon.sock"); + let info = dir.path().join("daemon.info"); std::fs::write(&pid, std::process::id().to_string()).unwrap(); std::fs::write(&endpoint, "").unwrap(); + std::fs::write(&info, "{}").unwrap(); let holder = open_lock_file_at(&lock).unwrap(); holder.try_lock().unwrap(); // A replacement holds the lock (mid-startup): even our own files must // be left alone, since the replacement may be about to rebind them. - cleanup_at(&lock, &pid, &endpoint); + cleanup_at(&lock, &pid, &endpoint, &info); assert!(pid.exists()); assert!(endpoint.exists()); + assert!(info.exists()); } } diff --git a/src/lib.rs b/src/lib.rs index 10d63cd..720d441 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -28,11 +28,15 @@ pub struct Cli { #[arg(long, global = true, env = "CHROME_WS_ENDPOINT")] pub ws_endpoint: Option, - /// Chrome user data directory (for auto-connect) + /// Browser user data directory (for auto-connect) #[arg(long, global = true, env = "CHROME_USER_DATA_DIR")] pub user_data_dir: Option, - /// Chrome channel: stable, beta, canary, dev + /// Browser to auto-connect to: chrome, edge + #[arg(long, global = true, default_value = "chrome", env = "CHROME_BROWSER")] + pub browser: String, + + /// Browser release channel: stable, beta, canary, dev #[arg(long, global = true, default_value = "stable", env = "CHROME_CHANNEL")] pub channel: String, @@ -408,6 +412,10 @@ pub enum Commands { track_navigation: bool, }, + /// List the running daemons and the browser each is attached to + #[command(name = "list-daemons")] + ListDaemons, + /// Stop the background daemon process #[command(name = "kill-daemon")] KillDaemon { @@ -419,6 +427,14 @@ pub enum Commands { /// errors — do not use it as a retry step. #[arg(long)] force: bool, + + /// Stop every daemon for this user, whatever browser each is attached + /// to, instead of only the one for the resolved target. + /// + /// Needs no reachable browser, so this is the way to clear daemons + /// orphaned by a browser that has already exited. + #[arg(long)] + all: bool, }, } @@ -466,10 +482,261 @@ impl Cli { Commands::RunScript { .. } => "run-script", Commands::Adapter { .. } => "adapter", Commands::KillDaemon { .. } => "kill-daemon", + Commands::ListDaemons => "list-daemons", } } } +/// Whether a PID names a live process. +/// +/// `kill(pid, 0)` delivers no signal but performs the same existence and +/// permission checks: `EPERM` means the process exists and is someone else's, +/// which for a daemon list is still "running". +#[cfg(unix)] +fn daemon_pid_alive(pid: i32) -> bool { + // SAFETY: kill() has no memory-safety preconditions, and signal 0 is the + // no-op existence probe. The pid is validated positive by the caller, so + // it cannot address a process group. + let ret = unsafe { libc::kill(pid as libc::pid_t, 0) }; + ret == 0 || std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) +} + +/// Human-readable uptime, coarsened to the largest two units that matter. +fn format_uptime(secs: u64) -> String { + match secs { + s if s < 60 => format!("{s}s"), + s if s < 3600 => format!("{}m{}s", s / 60, s % 60), + s => format!("{}h{}m", s / 3600, (s % 3600) / 60), + } +} + +/// Endpoint as `host:port`, dropping the browser-GUID path that makes the full +/// URL too wide to tabulate. +fn short_endpoint(ws_url: &str) -> String { + ws_url + .strip_prefix("ws://") + .unwrap_or(ws_url) + .split('/') + .next() + .unwrap_or(ws_url) + .to_string() +} + +/// Print one row per daemon: which browser it is attached to, where, and for +/// how long. +/// +/// Reads only on-disk daemon state, so it works when every browser is gone. +/// A daemon whose info sidecar is missing still lists — with `?` columns — +/// because knowing a daemon holds a connection matters more than labeling it. +fn print_daemon_list(format: format::OutputFormat) { + #[derive(serde::Serialize)] + struct Row { + pid: Option, + browser: String, + endpoint: String, + uptime_secs: Option, + running: Option, + key: String, + } + + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |d| d.as_secs()); + + let mut rows = Vec::new(); + for key in protocol::enumerate_instance_keys() { + let info: Option = std::fs::read(protocol::info_path(&key)) + .ok() + .and_then(|b| serde_json::from_slice(&b).ok()); + + #[cfg(unix)] + let pid_str = read_pid_file_checked(&protocol::pid_path(&key)).ok(); + #[cfg(not(unix))] + let pid_str = std::fs::read_to_string(protocol::pid_path(&key)).ok(); + let pid = pid_str.and_then(|s| s.trim().parse::().ok()); + + #[cfg(unix)] + let running = pid + .and_then(|p| i32::try_from(p).ok()) + .map(daemon_pid_alive); + #[cfg(not(unix))] + let running: Option = None; + + rows.push(Row { + pid, + browser: info + .as_ref() + .map_or_else(|| "?".to_string(), |i| i.browser.clone()), + endpoint: info + .as_ref() + .map_or_else(|| "?".to_string(), |i| short_endpoint(&i.ws_url)), + uptime_secs: info + .as_ref() + .filter(|i| i.started_unix > 0 && now >= i.started_unix) + .map(|i| now - i.started_unix), + running, + key, + }); + } + + if matches!(format, format::OutputFormat::Json) { + println!( + "{}", + serde_json::to_string_pretty(&rows).unwrap_or_else(|_| "[]".to_string()) + ); + return; + } + + if rows.is_empty() { + println!("No daemons running."); + return; + } + + println!("PID BROWSER ENDPOINT UPTIME STATE"); + println!("{}", "-".repeat(58)); + for r in &rows { + let pid = r.pid.map_or_else(|| "?".to_string(), |p| p.to_string()); + let uptime = r.uptime_secs.map_or_else(|| "?".to_string(), format_uptime); + let state = match r.running { + Some(true) => "running", + Some(false) => "stale", + None => "?", + }; + println!( + "{:<7} {:<8} {:<21} {:<9} {}", + pid, r.browser, r.endpoint, uptime, state + ); + } + if rows.iter().any(|r| r.running == Some(false)) { + println!("\nstale = PID file with no live process; `kill-daemon --all` clears them."); + } +} + +/// Stop the daemon instance identified by `key`, removing its socket, info and +/// PID files. +/// +/// Split out of `kill-daemon` so the same ownership-checked, signal-once logic +/// serves both the target-scoped kill and the `--all` sweep. Prints what it did +/// (or that there was nothing to do) and returns an error only when the daemon +/// may still be running — the caller decides whether one failure aborts a +/// whole sweep. +fn stop_daemon_instance(key: &str) -> Result<()> { + stop_daemon_at( + &protocol::pid_path(key), + &protocol::info_path(key), + #[cfg(unix)] + &protocol::socket_path(key), + ) +} + +/// Stop the pre-instance-key daemon (`chrome-devtools-daemon-.pid`) left +/// by a version that ran one daemon per user. +/// +/// Swept by `--all` only: it is attached to some browser we can no longer +/// identify, so no scoped target can claim it, and after an upgrade nothing +/// else will ever reach it. Silent when the files don't exist, which is the +/// common case. +fn stop_legacy_unkeyed_daemon() -> Result<()> { + let pid_path = protocol::legacy_unkeyed_pid_path(); + if !pid_path.exists() { + return Ok(()); + } + // No info sidecar existed in that layout; point at a path that is + // guaranteed absent so the removal is a no-op. + let info_path = pid_path.with_extension("info"); + stop_daemon_at( + &pid_path, + &info_path, + #[cfg(unix)] + &protocol::legacy_unkeyed_socket_path(), + ) +} + +/// Signal one daemon and remove its files, given their paths. +/// +/// The ownership-checked read and single SIGTERM live here so the keyed and +/// legacy layouts cannot drift apart in how carefully they treat a predictable +/// path in shared `/tmp`. +fn stop_daemon_at( + pid_path: &std::path::Path, + info_path: &std::path::Path, + #[cfg(unix)] sock_path: &std::path::Path, +) -> Result<()> { + // Ownership-checked read: the path is predictable in shared /tmp, so + // never act on a PID file that was planted there by another user. + #[cfg(unix)] + let read_result = read_pid_file_checked(pid_path); + #[cfg(not(unix))] + let read_result = { + use anyhow::Context as _; + std::fs::read_to_string(pid_path) + .with_context(|| format!("Failed to read PID file {}", pid_path.display())) + }; + match read_result { + Ok(pid_str) => { + #[cfg(unix)] + { + // parse_pid_file_contents enforces the safety rules for + // what may be passed to kill() (no pid 0, no pid_t + // overflow — see its doc comment). + let pid = parse_pid_file_contents(&pid_str).ok_or_else(|| { + anyhow::anyhow!( + "PID {:?} in {} is not a positive integer that fits libc::pid_t; refusing to signal", + pid_str.trim(), + pid_path.display() + ) + })?; + // Signal the process directly via libc to avoid shelling out + // to /usr/bin/kill. A return of 0 means the signal was + // delivered; -1 with errno ESRCH means the process is gone + // (and the PID file was stale). + // SAFETY: kill() has no memory-safety preconditions; the + // pid was validated positive and in pid_t range above, so + // it cannot alias a process group. + let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; + if ret == 0 { + // Signal delivered — daemon is shutting down; clean up. + let _ = std::fs::remove_file(sock_path); + let _ = std::fs::remove_file(info_path); + let _ = std::fs::remove_file(pid_path); + println!("Daemon (PID {pid}) stopped."); + } else { + let err = std::io::Error::last_os_error(); + if err.raw_os_error() == Some(libc::ESRCH) { + // Process is gone — the PID file was stale; clean up. + let _ = std::fs::remove_file(sock_path); + let _ = std::fs::remove_file(info_path); + let _ = std::fs::remove_file(pid_path); + println!("Daemon (PID {pid}) was not running. Cleaned up stale files."); + } else { + // e.g. EPERM: the daemon may still be running. Leave the + // socket/PID files so it stays reachable. + return Err(anyhow::anyhow!( + "Failed to signal daemon (PID {pid}): {err}. Left socket/PID files in place." + )); + } + } + } + #[cfg(not(unix))] + { + let _ = pid_str; + println!("kill-daemon is only supported on Unix systems."); + } + } + Err(e) if pid_file_missing(&e) => { + println!("No daemon running (PID file not found)."); + } + Err(e) => { + // Surface via the standard error path (uniform formatting, + // telemetry flush, typed exit code) — matching the EPERM + // signal-failure case above rather than exiting directly. The + // error already names the failing operation and the path. + return Err(e); + } + } + Ok(()) +} + /// What to do when `kill-daemon` is invoked, given `--force` and whether /// stdin is a TTY. Kept pure so the policy is unit-testable without /// mocking stdin I/O. @@ -939,6 +1206,9 @@ fn build_request(cli: &Cli) -> Result { }), ), Commands::KillDaemon { .. } => unreachable!("KillDaemon is handled before build_request"), + Commands::ListDaemons => { + unreachable!("ListDaemons is handled before build_request") + } Commands::InspectHeapSnapshotNode { .. } => { unreachable!("InspectHeapSnapshotNode is handled before build_request") } @@ -1031,12 +1301,21 @@ pub async fn run() -> Result<()> { } }; + // Pure local state: reads daemon files and never connects to a browser, so + // it must run before endpoint resolution — which would fail in exactly the + // situation where you most want the list (a browser that has exited, + // leaving a daemon behind). + if matches!(cli.command, Commands::ListDaemons) { + print_daemon_list(cli.output_format()); + return Ok(()); + } + // Handle kill-daemon without connecting to Chrome. Match by reference // like the other `cli.command` intercepts below: binding only the Copy // `force` field happens to avoid a partial move today, but a by-ref match // keeps that from silently breaking if the variant ever gains a non-Copy // field. - if let Commands::KillDaemon { force } = &cli.command { + if let Commands::KillDaemon { force, all } = &cli.command { use std::io::IsTerminal; // A prompt is only useful if the user can both type (stdin) and see it // (stderr). If stderr is not a TTY (e.g. `2>file`) while stdin still is, @@ -1070,78 +1349,51 @@ pub async fn run() -> Result<()> { KillDaemonDecision::Proceed => {} } - let pid_path = protocol::pid_path(); - #[cfg(unix)] - let sock_path = protocol::socket_path(); - // Ownership-checked read: the path is predictable in shared /tmp, so - // never act on a PID file that was planted there by another user. - #[cfg(unix)] - let read_result = read_pid_file_checked(&pid_path); - #[cfg(not(unix))] - let read_result = { - use anyhow::Context as _; - std::fs::read_to_string(&pid_path) - .with_context(|| format!("Failed to read PID file {}", pid_path.display())) - }; - match read_result { - Ok(pid_str) => { - #[cfg(unix)] - { - // parse_pid_file_contents enforces the safety rules for - // what may be passed to kill() (no pid 0, no pid_t - // overflow — see its doc comment). - let pid = parse_pid_file_contents(&pid_str).ok_or_else(|| { - anyhow::anyhow!( - "PID {:?} in {} is not a positive integer that fits libc::pid_t; refusing to signal", - pid_str.trim(), - pid_path.display() - ) - })?; - // Signal the process directly via libc to avoid shelling out - // to /usr/bin/kill. A return of 0 means the signal was - // delivered; -1 with errno ESRCH means the process is gone - // (and the PID file was stale). - // SAFETY: kill() has no memory-safety preconditions; the - // pid was validated positive and in pid_t range above, so - // it cannot alias a process group. - let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; - if ret == 0 { - // Signal delivered — daemon is shutting down; clean up. - let _ = std::fs::remove_file(&sock_path); - let _ = std::fs::remove_file(&pid_path); - println!("Daemon (PID {pid}) stopped."); - } else { - let err = std::io::Error::last_os_error(); - if err.raw_os_error() == Some(libc::ESRCH) { - // Process is gone — the PID file was stale; clean up. - let _ = std::fs::remove_file(&sock_path); - let _ = std::fs::remove_file(&pid_path); - println!("Daemon (PID {pid}) was not running. Cleaned up stale files."); - } else { - // e.g. EPERM: the daemon may still be running. Leave the - // socket/PID files so it stays reachable. - return Err(anyhow::anyhow!( - "Failed to signal daemon (PID {pid}): {err}. Left socket/PID files in place." - )); - } - } - } - #[cfg(not(unix))] - { - let _ = pid_str; - println!("kill-daemon is only supported on Unix systems."); + if *all { + // Clean slate: every daemon this user owns, whatever browser it is + // attached to. Keep going after a failure so one unkillable daemon + // cannot strand the rest. + let keys = protocol::enumerate_instance_keys(); + if keys.is_empty() { + println!("No daemons running."); + } + let mut failures = 0usize; + for key in &keys { + if let Err(e) = stop_daemon_instance(key) { + failures += 1; + eprintln!("{e:#}"); } } - Err(e) if pid_file_missing(&e) => { - println!("No daemon running (PID file not found)."); + if let Err(e) = stop_legacy_unkeyed_daemon() { + failures += 1; + eprintln!("{e:#}"); } - Err(e) => { - // Surface via the standard error path (uniform formatting, - // telemetry flush, typed exit code) — matching the EPERM - // signal-failure case above rather than exiting directly. The - // error already names the failing operation and the path. - return Err(e); + if failures > 0 { + return Err(anyhow::anyhow!( + "{failures} daemon(s) could not be stopped (of {} found)", + keys.len() + )); } + } else { + // Target-scoped: resolve the same endpoint a real command would, + // so `--browser edge kill-daemon` can only ever stop the Edge + // daemon. Resolution needs the browser to be reachable, which a + // dead browser with an orphaned daemon is not — hence the pointer + // to --all, which needs no endpoint. + let ws_url = browser::resolve_ws_url( + cli.ws_endpoint.as_deref(), + cli.user_data_dir.as_deref(), + &cli.browser, + &cli.channel, + ) + .map_err(|e| { + anyhow::anyhow!( + "{e:#}\n\nkill-daemon targets the daemon for a specific browser, so it \ + needs to resolve that browser's endpoint. If the browser is already gone \ + and you just want to stop leftover daemons, use --all." + ) + })?; + stop_daemon_instance(&protocol::instance_key(&ws_url))?; } // Best-effort sweep of the pre-uid-suffix file names: a daemon @@ -1278,25 +1530,31 @@ pub async fn run() -> Result<()> { let ws_url = browser::resolve_ws_url( cli.ws_endpoint.as_deref(), cli.user_data_dir.as_deref(), + &cli.browser, &cli.channel, )?; let request = build_request(&cli)?; + // One daemon per endpoint: a daemon serves only the browser session it was + // spawned for, so this command can never be answered by a daemon attached + // to a different browser. + let key = protocol::instance_key(&ws_url); + // Try daemon first - if let Ok(resp) = client::send_to_daemon(&request).await { + if let Ok(resp) = client::send_to_daemon(&key, &request).await { print_response(&resp); return Ok(()); } // Daemon not running — spawn it - client::spawn_daemon(&ws_url)?; - if let Err(e) = client::wait_for_daemon().await { + client::spawn_daemon(&ws_url, &cli.browser)?; + if let Err(e) = client::wait_for_daemon(&key).await { return run_direct_fallback(&cli, &ws_url, &e).await; } // Retry via daemon - match client::send_to_daemon(&request).await { + match client::send_to_daemon(&key, &request).await { Ok(resp) => { print_response(&resp); Ok(()) @@ -1809,6 +2067,40 @@ mod tests { assert!(!obj.get("bool_false").unwrap().as_bool().unwrap()); } + #[test] + fn test_format_uptime_units() { + assert_eq!(format_uptime(0), "0s"); + assert_eq!(format_uptime(8), "8s"); + assert_eq!(format_uptime(72), "1m12s"); + assert_eq!(format_uptime(3600), "1h0m"); + assert_eq!(format_uptime(7500), "2h5m"); + } + + #[test] + fn test_short_endpoint_drops_guid_path() { + assert_eq!( + short_endpoint("ws://127.0.0.1:56912/devtools/browser/1fff9d85"), + "127.0.0.1:56912" + ); + // Degrades to the input rather than empty when the shape is unexpected. + assert_eq!(short_endpoint("127.0.0.1:9222"), "127.0.0.1:9222"); + assert_eq!(short_endpoint(""), ""); + } + + /// A live process must read as running; PID 0 is never probed because + /// `kill(0, …)` addresses the caller's own process group. + #[cfg(unix)] + #[test] + fn test_daemon_pid_alive_detects_self_and_absence() { + let me = i32::try_from(std::process::id()).unwrap(); + assert!(daemon_pid_alive(me), "this process is alive"); + // PID 1 exists on every Unix; a very high PID almost certainly does not. + assert!( + !daemon_pid_alive(0x7FFF_FFFE), + "implausible PID is not alive" + ); + } + #[test] fn test_kill_daemon_decision_force_always_proceeds() { assert_eq!( diff --git a/src/main.rs b/src/main.rs index 386ec4b..697674a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -13,7 +13,10 @@ async fn main() { let args: Vec = std::env::args().collect(); if args.get(1).map(|s| s.as_str()) == Some("__daemon__") { let ws_url = args.get(2).expect("daemon requires ws_url argument"); - let result = daemon::run_daemon(ws_url).await; + // Descriptive only, for `list-daemons`. Older spawners passed no + // browser argument, so default rather than panic. + let browser = args.get(3).map(String::as_str).unwrap_or("chrome"); + let result = daemon::run_daemon(ws_url, browser).await; telemetry::shutdown_logger(); if let Err(e) = result { eprintln!("daemon error: {e:#}"); diff --git a/src/protocol.rs b/src/protocol.rs index 9bf4a6e..89806a8 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -70,29 +70,109 @@ fn user_suffix() -> std::borrow::Cow<'static, str> { std::borrow::Cow::Borrowed("") } +/// Identity of a single daemon instance: a short hash of the resolved +/// WebSocket endpoint. +/// +/// A daemon owns exactly one live CDP connection, so its identity *is* that +/// connection. Keying the socket/PID/info filenames by endpoint gives Chrome, +/// Edge, every release channel and every headless instance a daemon of its +/// own, which is what stops a command aimed at one browser from being served +/// by a daemon attached to another — the failure mode when a single per-user +/// daemon bound to whichever browser happened to be resolved first, and every +/// later `--browser`/`--user-data-dir` flag was silently ignored. +/// +/// The endpoint's browser GUID changes on every browser launch, so a restarted +/// browser deliberately gets a fresh daemon instead of inheriting a dead +/// connection. The orphan holds no lock and exits on its own idle timeout. +pub fn instance_key(ws_url: &str) -> String { + // FNV-1a, 64-bit. Not cryptographic, and does not need to be: the input is + // a loopback URL this process just derived, not attacker-chosen, and a + // collision would only mean two browser sessions sharing one daemon — + // exactly the behavior that predates keying. + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + for byte in ws_url.as_bytes() { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x0000_0100_0000_01b3); + } + format!("{hash:016x}") +} + +/// Shared leading part of every per-user daemon filename. +fn daemon_file_prefix() -> String { + format!("chrome-devtools-daemon{}", user_suffix()) +} + /// Path to the Unix domain socket for daemon communication. #[cfg(unix)] -pub fn socket_path() -> PathBuf { - std::env::temp_dir().join(format!("chrome-devtools-daemon{}.sock", user_suffix())) +pub fn socket_path(key: &str) -> PathBuf { + std::env::temp_dir().join(format!("{}-{key}.sock", daemon_file_prefix())) } /// Path to the named-pipe address file for daemon communication (Windows). #[cfg(windows)] -pub fn addr_path() -> PathBuf { - std::env::temp_dir().join(format!("chrome-devtools-daemon{}.addr", user_suffix())) +pub fn addr_path(key: &str) -> PathBuf { + std::env::temp_dir().join(format!("{}-{key}.addr", daemon_file_prefix())) } /// Path to the daemon PID file. -pub fn pid_path() -> PathBuf { - std::env::temp_dir().join(format!("chrome-devtools-daemon{}.pid", user_suffix())) +pub fn pid_path(key: &str) -> PathBuf { + std::env::temp_dir().join(format!("{}-{key}.pid", daemon_file_prefix())) +} + +/// Path to the daemon's metadata sidecar (see [`DaemonInfo`]). +pub fn info_path(key: &str) -> PathBuf { + std::env::temp_dir().join(format!("{}-{key}.info", daemon_file_prefix())) } /// Path to the lock file serializing daemon startup and cleanup. /// -/// The lock file is never removed once created: deleting it while another -/// process may be about to lock it would reintroduce the race it prevents. +/// Deliberately *not* keyed per instance. It only serializes the brief +/// write-pid-then-bind critical section, so one lock for all instances costs +/// nothing measurable — while a per-instance lock would accumulate a file per +/// browser session forever, since the lock file is never removed once created: +/// deleting it while another process may be about to lock it would reintroduce +/// the race it prevents. pub fn lock_path() -> PathBuf { - std::env::temp_dir().join(format!("chrome-devtools-daemon{}.lock", user_suffix())) + std::env::temp_dir().join(format!("{}.lock", daemon_file_prefix())) +} + +/// Every daemon instance key with a PID file belonging to this user. +/// +/// Used by `list-daemons` and `kill-daemon --all`. Unreadable temp dirs and +/// non-UTF-8 names are skipped rather than reported: a name we cannot parse is +/// not a daemon of ours. +pub fn enumerate_instance_keys() -> Vec { + let prefix = format!("{}-", daemon_file_prefix()); + let Ok(entries) = std::fs::read_dir(std::env::temp_dir()) else { + return Vec::new(); + }; + let mut keys: Vec = entries + .flatten() + .filter_map(|e| { + let name = e.file_name(); + let name = name.to_str()?; + let key = name.strip_prefix(&prefix)?.strip_suffix(".pid")?; + (!key.is_empty()).then(|| key.to_string()) + }) + .collect(); + keys.sort(); + keys +} + +/// Metadata a daemon publishes about itself, so `list-daemons` can name the +/// browser it is attached to without inspecting process arguments. +/// +/// Best-effort and purely descriptive: the daemon works if this file is +/// missing, and readers must tolerate its absence. +#[derive(Serialize, Deserialize, Debug, Clone)] +pub struct DaemonInfo { + /// Browser the daemon was spawned for, as passed to `--browser`. + pub browser: String, + /// The resolved endpoint this daemon is attached to. + pub ws_url: String, + pub pid: u32, + /// Unix seconds at daemon start, for the uptime column. + pub started_unix: u64, } /// Pre-uid-suffix PID file name, so `kill-daemon` can still stop a daemon @@ -102,6 +182,19 @@ pub fn legacy_pid_path() -> PathBuf { std::env::temp_dir().join("chrome-devtools-daemon.pid") } +/// Pre-instance-key PID file name: `chrome-devtools-daemon-.pid`, written +/// by versions that ran one daemon per user. Swept by `kill-daemon --all` so an +/// upgrade doesn't leave an unreachable daemon holding a CDP connection. +pub fn legacy_unkeyed_pid_path() -> PathBuf { + std::env::temp_dir().join(format!("{}.pid", daemon_file_prefix())) +} + +/// Pre-instance-key socket name (see [`legacy_unkeyed_pid_path`]). +#[cfg(unix)] +pub fn legacy_unkeyed_socket_path() -> PathBuf { + std::env::temp_dir().join(format!("{}.sock", daemon_file_prefix())) +} + /// Pre-uid-suffix socket name (see [`legacy_pid_path`]). #[cfg(unix)] pub fn legacy_socket_path() -> PathBuf { @@ -130,3 +223,71 @@ pub async fn read_msg(r: &mut R) -> anyhow::Result Date: Wed, 26 Aug 2026 23:48:41 +0800 Subject: [PATCH 2/6] fix: honor --toon in list-daemons, correct Windows and Edge docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the per-endpoint daemon change. list-daemons special-cased Json and let --toon fall through to the text table. Route both structured formats through the existing format_structured helper instead, which already encodes TOON. The empty list stays structural in both (`[]` / `[0]:`); the "No daemons running." sentence remains text-only. kill-daemon's --all summary counted failures across keyed instances plus the legacy daemon but sized the total from keys.len(), so a legacy-only failure could report "1 of 0". Count the legacy stop as an attempt — and only when a legacy PID file was actually there, so the total does not inflate on the common path. stop_legacy_unkeyed_daemon now reports whether it found anything. Accept browser and channel names case-insensitively, trimmed, so --browser Edge and CHROME_BROWSER=EDGE work; error messages still quote the name as typed. Unknown values behave as before. That made the raw spelling visible in list-daemons, since the info file records what was passed, so the daemon label is canonicalized (--browser EDGE and --browser msedge both list as "edge"). Docs: - The kill-daemon description implied it signals and cleans up on every platform. The signal and file removal are Unix-only; Windows prints that it is unsupported and touches nothing. - Windows daemon filenames are keyed like every other platform's, so document chrome-devtools-daemon-.addr rather than the pre-key name, and point the taskkill instructions at list-daemons, since there can now be several PIDs to choose between. - The skill said --channel selects stable/beta/dev/canary for Edge without noting that Edge ships no Canary for Linux, which browser.rs rejects outright. - Both scoped kill-daemon calls in the headless recipe could abort the script: a scoped kill must resolve its profile's endpoint, and there is none on a fresh profile (step 1) or after a browser that died before writing its port file (the EXIT trap). Both are now best-effort with `|| true`, still profile-scoped rather than reaching for --all, which would stop the user's own daemons. Verified: --toon and --json render structurally including when empty, alias and mixed-case spellings resolve and scope kills correctly, and the daemon table labels canonically. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 6 +-- skill/chrome-devtools/SKILL.md | 16 +++++-- src/browser.rs | 78 ++++++++++++++++++++++++++++++++-- src/lib.rs | 48 +++++++++++++-------- 4 files changed, 120 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index 7697b32..970c5a6 100644 --- a/README.md +++ b/README.md @@ -227,7 +227,7 @@ A drain without a `--duration` returns instantly. Adding `--duration N` switches |---------|-------------| | `kill-daemon` | Stop the background daemon cleanly | -`kill-daemon` signals the targeted daemon with `SIGTERM`, removes its socket, info and PID files, and exits. It's a no-op if no daemon is running. Prefer this over `pkill -f __daemon__` — the process name is shared by legitimate Chrome children processes. +On Unix, `kill-daemon` signals the targeted daemon with `SIGTERM`, removes its socket, info and PID files, and exits. It's a no-op if no daemon is running. Prefer this over `pkill -f __daemon__` — the process name is shared by legitimate Chrome children processes. On Windows it is not supported: it prints that and exits without signalling anything or removing any files (see **Kill (Windows)** below). ## Global options @@ -252,7 +252,7 @@ Global `--block-url` and `--unblock-url` update the **active tab's** block list - **Instance identity**: one daemon per browser endpoint. The socket/PID/info filenames carry a 16-hex-digit key derived from the resolved `ws://` URL, so Chrome, Edge, every channel and every headless instance get a daemon of their own, and a command aimed at one browser can never be served by a daemon attached to another. The URL's browser GUID changes on every browser launch, so a restarted browser gets a fresh daemon instead of inheriting a dead connection; the orphan exits on its idle timeout. - **Endpoint (Unix)**: socket at `$TMPDIR/chrome-devtools-daemon--.sock` (uid-suffixed so users on a shared machine don't collide) -- **Endpoint (Windows)**: loopback TCP listener; its address is written to `%TEMP%\chrome-devtools-daemon.addr` (`%TEMP%` is already per-user, so no suffix) +- **Endpoint (Windows)**: loopback TCP listener; its address is written to `%TEMP%\chrome-devtools-daemon-.addr` (`%TEMP%` is already per-user, so there is no uid suffix — but the instance key is still present, since one daemon per endpoint applies on every platform) - **PID file**: `$TMPDIR/chrome-devtools-daemon--.pid` (Windows: `%TEMP%\chrome-devtools-daemon-.pid`) - **Info file**: `$TMPDIR/chrome-devtools-daemon--.info` — JSON naming the browser, endpoint, PID and start time, so `list-daemons` can label rows. Best-effort: a daemon with no info file still lists, with `?` columns. - **Lock file**: `$TMPDIR/chrome-devtools-daemon-.lock` (Windows: `%TEMP%\chrome-devtools-daemon.lock`) — **not** keyed per instance: one lock covers all of them, because it only serializes the brief write-pid-then-bind window, while a per-instance lock would accumulate a never-removed file per browser session. Serializes daemon startup/cleanup; intentionally never removed automatically. Locks bind to the inode, not the name: deleting the file while any daemon process is still starting, running, or shutting down lets a new process lock a fresh replacement inode and bypass the serialization entirely. Only delete it once no daemon process exists at all — and there's rarely a reason to, since a leftover lock file is harmless. @@ -262,7 +262,7 @@ Global `--block-url` and `--unblock-url` update the **active tab's** block list - **Spawned by**: First CLI invocation for a given endpoint (transparent to user) - **List**: `chrome-devtools list-daemons` — PID, browser, endpoint, uptime and state for every daemon this user owns. `--json` for machine-readable output. Reads only on-disk state, so it works when every browser has exited; rows whose PID no longer exists are marked `stale`. - **Kill**: `chrome-devtools kill-daemon` stops only the daemon for the endpoint its flags resolve to, so `--browser edge kill-daemon` cannot stop your Chrome daemon. Add `--all` to stop every daemon for this user — which is also the only way to clear one whose browser has already exited, since a scoped kill has no endpoint left to resolve (it fails and says so). `--all` also sweeps the pre-key `chrome-devtools-daemon-.pid` name left by older versions. (Or delete the socket + PID file by hand; leave the lock file — see above.) It sends SIGTERM and returns once the signal is delivered, not once the process is gone: the daemon exits *between* requests, so one that is mid-command finishes it and answers that client first. Expect up to one command's latency, and note that a daemon wedged inside a CDP call outlives the command that stopped it. -- **Kill (Windows)**: not supported — `kill-daemon` says so and exits, and a backgrounded daemon has no console for Ctrl-C. Use `taskkill /PID ` with the PID from `%TEMP%\chrome-devtools-daemon.pid`, or wait out the idle timeout. +- **Kill (Windows)**: not supported — `kill-daemon` says so and exits, and a backgrounded daemon has no console for Ctrl-C. Run `chrome-devtools list-daemons` to get the PID of the daemon you want (there may be several, one per endpoint) and pass it to `taskkill /PID `, or wait out the idle timeout. Reading `%TEMP%\chrome-devtools-daemon-.pid` directly works too, but only if you already know which key you want. The daemon keeps a persistent CDP session on the current page to: - Continuously collect `Network.*` and `Runtime.consoleAPICalled`/`exceptionThrown` events for `console` and `network` drains. diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index c4ca976..7f23574 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -35,7 +35,9 @@ needed. A daemon is spawned on first invocation and reused across commands **Microsoft Edge** works the same — it is Chromium and speaks the same protocol. Add `--browser edge` so auto-connect reads Edge's profile instead of Chrome's -(`--channel` still selects stable/beta/dev/canary). `--ws-endpoint` and +(`--channel` still selects stable/beta/dev/canary, except that Edge ships no +Canary for Linux — `--browser edge --channel canary` is rejected there rather +than pointed at a directory that cannot exist). `--ws-endpoint` and `--user-data-dir` need no `--browser`; they already say where to connect. Everything below applies unchanged — only the profile location differs. @@ -411,7 +413,11 @@ PROFILE=$(mktemp -d) # 1. Clear any daemon left over from a previous run of THIS profile. Not needed # for isolation — daemons are per-endpoint, so the user's browser is # unaffected either way — but a stale one here would hold a dead connection. -chrome-devtools --user-data-dir "$PROFILE" kill-daemon --force 2>/dev/null +# Best-effort: a scoped kill has to resolve this profile's endpoint, and on a +# fresh $PROFILE there is none yet, so it exits non-zero and removes nothing. +# `|| true` keeps that expected failure from aborting the script under `set -e`. +# Do NOT reach for --all here: it would also stop the user's own daemons. +chrome-devtools --user-data-dir "$PROFILE" kill-daemon --force 2>/dev/null || true # 2. Spawn headless Chrome with an isolated profile; port 0 = pick a free port "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" \ @@ -426,8 +432,10 @@ CHROME_PID=$! # CDP connection and an idle timer, so stop it rather than leaking it. cleanup() { # Scoped to this profile: a bare kill-daemon would resolve the user's default - # Chrome profile and stop their daemon instead of this one. - chrome-devtools --user-data-dir "$PROFILE" kill-daemon --force + # Chrome profile and stop their daemon instead of this one. Best-effort for + # the same reason as step 1 — if Chrome died before writing its port file + # there is no endpoint to resolve, and a trap must not fail on that. + chrome-devtools --user-data-dir "$PROFILE" kill-daemon --force 2>/dev/null || true kill "$CHROME_PID" 2>/dev/null # Chrome shuts down asynchronously, so deleting the profile right after # SIGTERM races its teardown and can leave it running against a directory diff --git a/src/browser.rs b/src/browser.rs index 33f5a51..acdd3d2 100644 --- a/src/browser.rs +++ b/src/browser.rs @@ -69,6 +69,16 @@ fn read_devtools_active_port(user_data_dir: &Path, browser: Browser) -> Result String { + Browser::parse(name).map_or_else(|_| name.to_string(), |b| b.flag_name().to_string()) +} + /// A Chromium-based browser the CLI knows how to auto-connect to. /// /// Both speak the same DevTools Protocol; they differ only in where the @@ -80,8 +90,10 @@ enum Browser { } impl Browser { + /// Case- and whitespace-insensitive, so `--browser Edge` and + /// `CHROME_BROWSER=EDGE` work. The error quotes the name as typed. fn parse(name: &str) -> Result { - match name { + match name.trim().to_ascii_lowercase().as_str() { "chrome" => Ok(Self::Chrome), "edge" | "msedge" => Ok(Self::Edge), _ => bail!("Unknown browser: {name} (expected 'chrome' or 'edge')"), @@ -96,6 +108,16 @@ impl Browser { } } + /// Canonical `--browser` spelling, for anything that records or displays + /// the choice. Kept separate from [`Browser::scheme`] so the two can + /// diverge if a browser ever needs different values. + fn flag_name(self) -> &'static str { + match self { + Self::Chrome => "chrome", + Self::Edge => "edge", + } + } + /// URL scheme for the `://inspect` hint. fn scheme(self) -> &'static str { match self { @@ -106,12 +128,17 @@ impl Browser { /// Default user data directory for the given release channel. fn default_user_data_dir(self, channel: &str) -> Result { + // Matched case-insensitively for the same reason as `parse`; the error + // arms still report the channel as the user typed it. + let normalized = channel.trim().to_ascii_lowercase(); + let channel_key = normalized.as_str(); + #[cfg(target_os = "macos")] { let home = dirs::home_dir().ok_or_else(|| anyhow!("Cannot determine home directory"))?; let base = home.join("Library/Application Support"); - let dir = match (self, channel) { + let dir = match (self, channel_key) { (Self::Chrome, "stable" | "chrome") => base.join("Google/Chrome"), (Self::Chrome, "beta") => base.join("Google/Chrome Beta"), (Self::Chrome, "canary") => base.join("Google/Chrome Canary"), @@ -129,7 +156,7 @@ impl Browser { { let home = dirs::home_dir().ok_or_else(|| anyhow!("Cannot determine home directory"))?; - let dir = match (self, channel) { + let dir = match (self, channel_key) { (Self::Chrome, "stable" | "chrome") => home.join(".config/google-chrome"), (Self::Chrome, "beta") => home.join(".config/google-chrome-beta"), // Chrome ships no Canary for Linux; unstable is the dev channel. @@ -150,7 +177,7 @@ impl Browser { let local_app_data = std::env::var("LOCALAPPDATA").map_err(|_| anyhow!("LOCALAPPDATA not set"))?; let base = PathBuf::from(local_app_data); - let dir = match (self, channel) { + let dir = match (self, channel_key) { (Self::Chrome, "stable" | "chrome") => base.join("Google/Chrome/User Data"), (Self::Chrome, "beta") => base.join("Google/Chrome Beta/User Data"), (Self::Chrome, "canary") => base.join("Google/Chrome SxS/User Data"), @@ -177,6 +204,49 @@ mod tests { assert_eq!(Browser::parse("msedge").unwrap(), Browser::Edge); } + #[test] + fn parses_browsers_case_insensitively() { + for name in ["Chrome", "CHROME", " chrome "] { + assert_eq!(Browser::parse(name).unwrap(), Browser::Chrome, "{name}"); + } + for name in ["Edge", "EDGE", "MSEdge", " edge "] { + assert_eq!(Browser::parse(name).unwrap(), Browser::Edge, "{name}"); + } + } + + #[test] + fn channels_are_matched_case_insensitively() { + for browser in [Browser::Chrome, Browser::Edge] { + assert_eq!( + browser.default_user_data_dir("stable").unwrap(), + browser.default_user_data_dir("STABLE").unwrap() + ); + assert_eq!( + browser.default_user_data_dir("beta").unwrap(), + browser.default_user_data_dir(" Beta ").unwrap() + ); + } + } + + /// The error must quote what the user typed, not the normalized form. + #[test] + fn unknown_browser_error_quotes_the_original_spelling() { + let err = Browser::parse("FireFox").unwrap_err().to_string(); + assert!(err.contains("FireFox"), "{err}"); + } + + #[test] + fn canonical_name_normalizes_spelling_and_aliases() { + for name in ["edge", "Edge", "EDGE", "msedge", "MSEdge"] { + assert_eq!(canonical_name(name), "edge", "{name}"); + } + for name in ["chrome", "Chrome", "CHROME"] { + assert_eq!(canonical_name(name), "chrome", "{name}"); + } + // Unknown names pass through rather than being silently relabelled. + assert_eq!(canonical_name("firefox"), "firefox"); + } + #[test] fn rejects_unknown_browser() { let err = Browser::parse("firefox").unwrap_err().to_string(); diff --git a/src/lib.rs b/src/lib.rs index 720d441..1385734 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -528,7 +528,7 @@ fn short_endpoint(ws_url: &str) -> String { /// Reads only on-disk daemon state, so it works when every browser is gone. /// A daemon whose info sidecar is missing still lists — with `?` columns — /// because knowing a daemon holds a connection matters more than labeling it. -fn print_daemon_list(format: format::OutputFormat) { +fn print_daemon_list(format: format::OutputFormat) -> Result<()> { #[derive(serde::Serialize)] struct Row { pid: Option, @@ -579,17 +579,19 @@ fn print_daemon_list(format: format::OutputFormat) { }); } - if matches!(format, format::OutputFormat::Json) { - println!( - "{}", - serde_json::to_string_pretty(&rows).unwrap_or_else(|_| "[]".to_string()) - ); - return; + // Both structured formats go through the shared encoder, so --toon gets + // TOON instead of silently falling through to the text table. The empty + // list is emitted structurally too: `[]` is the answer, not the + // "No daemons running." sentence, which belongs to text output only. + if !format.is_text() { + let value = serde_json::to_value(&rows)?; + println!("{}", format::format_structured(&value, format)?); + return Ok(()); } if rows.is_empty() { println!("No daemons running."); - return; + return Ok(()); } println!("PID BROWSER ENDPOINT UPTIME STATE"); @@ -610,6 +612,7 @@ fn print_daemon_list(format: format::OutputFormat) { if rows.iter().any(|r| r.running == Some(false)) { println!("\nstale = PID file with no live process; `kill-daemon --all` clears them."); } + Ok(()) } /// Stop the daemon instance identified by `key`, removing its socket, info and @@ -636,10 +639,10 @@ fn stop_daemon_instance(key: &str) -> Result<()> { /// identify, so no scoped target can claim it, and after an upgrade nothing /// else will ever reach it. Silent when the files don't exist, which is the /// common case. -fn stop_legacy_unkeyed_daemon() -> Result<()> { +fn stop_legacy_unkeyed_daemon() -> Result { let pid_path = protocol::legacy_unkeyed_pid_path(); if !pid_path.exists() { - return Ok(()); + return Ok(false); } // No info sidecar existed in that layout; point at a path that is // guaranteed absent so the removal is a no-op. @@ -650,6 +653,7 @@ fn stop_legacy_unkeyed_daemon() -> Result<()> { #[cfg(unix)] &protocol::legacy_unkeyed_socket_path(), ) + .map(|()| true) } /// Signal one daemon and remove its files, given their paths. @@ -1306,7 +1310,7 @@ pub async fn run() -> Result<()> { // situation where you most want the list (a browser that has exited, // leaving a daemon behind). if matches!(cli.command, Commands::ListDaemons) { - print_daemon_list(cli.output_format()); + print_daemon_list(cli.output_format())?; return Ok(()); } @@ -1358,20 +1362,28 @@ pub async fn run() -> Result<()> { println!("No daemons running."); } let mut failures = 0usize; + let mut attempted = keys.len(); for key in &keys { if let Err(e) = stop_daemon_instance(key) { failures += 1; eprintln!("{e:#}"); } } - if let Err(e) = stop_legacy_unkeyed_daemon() { - failures += 1; - eprintln!("{e:#}"); + // The legacy daemon is another stop attempt, so it counts toward + // the total — otherwise a legacy-only failure reports "1 of 0". + // It counts only when one was actually there, so the total does + // not inflate on the common no-legacy-files path. + match stop_legacy_unkeyed_daemon() { + Ok(found) => attempted += usize::from(found), + Err(e) => { + attempted += 1; + failures += 1; + eprintln!("{e:#}"); + } } if failures > 0 { return Err(anyhow::anyhow!( - "{failures} daemon(s) could not be stopped (of {} found)", - keys.len() + "{failures} of {attempted} daemon(s) could not be stopped" )); } } else { @@ -1548,7 +1560,9 @@ pub async fn run() -> Result<()> { } // Daemon not running — spawn it - client::spawn_daemon(&ws_url, &cli.browser)?; + // Canonical spelling, so `--browser EDGE` and `--browser msedge` both label + // the daemon `edge` in list-daemons. + client::spawn_daemon(&ws_url, &browser::canonical_name(&cli.browser))?; if let Err(e) = client::wait_for_daemon(&key).await { return run_direct_fallback(&cli, &ws_url, &e).await; } From a9f7112be0f267bb50e84b7a1bd88e3c0b938afe Mon Sep 17 00:00:00 2001 From: Aero Date: Thu, 27 Aug 2026 11:10:23 +0800 Subject: [PATCH 3/6] fix: validate PIDs before probing liveness in list-daemons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second round of review follow-ups. print_daemon_list parsed the PID file with a bare trim().parse::(), which accepts 0. daemon_pid_alive(0) then calls kill(0, 0), and signal 0 to PID 0 probes the caller's own process group rather than a daemon — it succeeds, so a PID file containing 0 listed as "running". Route the read through parse_pid_file_contents, the same validation kill-daemon applies before signalling, so 0 and values outside pid_t are rejected and the row reports an unknown PID instead. This also closes a gap between code and comment: a test already asserted that PID 0 is never probed, but nothing on this path enforced it. Docs, all three cases where a Unix-only behavior was stated as general: - The architecture diagram still showed the pre-key chrome-devtools-daemon-.sock, missed when the other daemon filenames were keyed. - list-daemons can only mark a row stale on Unix; Windows never probes liveness, so every row's state is "?" and stale never appears. - kill-daemon --all "clears every daemon" is false on Windows, where kill-daemon prints that it is unsupported and stops nothing. Scoped to Unix, with taskkill guidance for Windows. Also expand resolve_ws_url's doc comment to say why an explicit --ws-endpoint takes precedence: it names the browser directly, so it must not be second-guessed by local profile discovery, and it short-circuits --browser/--channel, which exist only to locate a profile directory. Verified: PID files containing 0, 4294967295 and non-numeric text all render as unknown rather than running, and a live daemon still lists normally. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 4 ++-- skill/chrome-devtools/SKILL.md | 13 +++++++++---- src/browser.rs | 13 +++++++++++-- src/lib.rs | 31 +++++++++++++++++++++---------- 4 files changed, 43 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 970c5a6..ff2ec56 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ This is a lightweight Rust binary that talks directly to Chrome's DevTools Proto ``` chrome-devtools navigate https://example.com │ - ├─ Try daemon (Unix socket $TMPDIR/chrome-devtools-daemon-.sock; + ├─ Try daemon (Unix socket $TMPDIR/chrome-devtools-daemon--.sock; │ loopback TCP on Windows) │ └─ If running → send command → get result │ @@ -260,7 +260,7 @@ Global `--block-url` and `--unblock-url` update the **active tab's** block list - **Cleanup**: endpoint + PID files are also removed on panics, and on Unix on SIGTERM/SIGINT; Windows Ctrl-C cleanup is best-effort only (a background daemon has no console to receive it). SIGQUIT, SIGHUP and SIGKILL skip cleanup by design — the leftover files are harmless and are reclaimed by the next daemon start. - **Protocol**: Length-prefixed JSON over the Unix socket / loopback TCP - **Spawned by**: First CLI invocation for a given endpoint (transparent to user) -- **List**: `chrome-devtools list-daemons` — PID, browser, endpoint, uptime and state for every daemon this user owns. `--json` for machine-readable output. Reads only on-disk state, so it works when every browser has exited; rows whose PID no longer exists are marked `stale`. +- **List**: `chrome-devtools list-daemons` — PID, browser, endpoint, uptime and state for every daemon this user owns. `--json` (or `--toon`) for machine-readable output. Reads only on-disk state, so it works when every browser has exited. On Unix, rows whose PID no longer exists are marked `stale`; on Windows liveness is not probed, so every row's state is `?` and `stale` never appears. - **Kill**: `chrome-devtools kill-daemon` stops only the daemon for the endpoint its flags resolve to, so `--browser edge kill-daemon` cannot stop your Chrome daemon. Add `--all` to stop every daemon for this user — which is also the only way to clear one whose browser has already exited, since a scoped kill has no endpoint left to resolve (it fails and says so). `--all` also sweeps the pre-key `chrome-devtools-daemon-.pid` name left by older versions. (Or delete the socket + PID file by hand; leave the lock file — see above.) It sends SIGTERM and returns once the signal is delivered, not once the process is gone: the daemon exits *between* requests, so one that is mid-command finishes it and answers that client first. Expect up to one command's latency, and note that a daemon wedged inside a CDP call outlives the command that stopped it. - **Kill (Windows)**: not supported — `kill-daemon` says so and exits, and a backgrounded daemon has no console for Ctrl-C. Run `chrome-devtools list-daemons` to get the PID of the daemon you want (there may be several, one per endpoint) and pass it to `taskkill /PID `, or wait out the idle timeout. Reading `%TEMP%\chrome-devtools-daemon-.pid` directly works too, but only if you already know which key you want. diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index 7f23574..0c8f6e5 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -498,10 +498,15 @@ consequences for the recipe above: `kill-daemon` resolves the default Chrome profile and would stop the user's daemon instead of the headless one. -`list-daemons` shows what is running (PID, browser, endpoint, uptime), and -`kill-daemon --all` clears every daemon regardless of endpoint — including ones -whose browser has already exited, which a scoped kill cannot reach because it -has no endpoint left to resolve. +`list-daemons` shows what is running (PID, browser, endpoint, uptime), and on +Unix `kill-daemon --all` clears every daemon regardless of endpoint — including +ones whose browser has already exited, which a scoped kill cannot reach because +it has no endpoint left to resolve. + +On Windows neither form of `kill-daemon` stops anything: it prints that it is +unsupported and exits. Take the PID from `list-daemons` (its state column reads +`?` there, since liveness is not probed) and pass it to `taskkill /PID `, +or wait out the 5-minute idle timeout. Older versions ran a single daemon per user, bound to whichever browser the first command resolved, and silently ignored later `--browser`/`--user-data-dir` diff --git a/src/browser.rs b/src/browser.rs index acdd3d2..dbf6e2f 100644 --- a/src/browser.rs +++ b/src/browser.rs @@ -4,8 +4,17 @@ use std::path::{Path, PathBuf}; /// Resolve the WebSocket URL for connecting to the browser. /// /// Priority: -/// 1. Explicit --ws-endpoint -/// 2. Auto-connect via DevToolsActivePort (default) +/// 1. Explicit `--ws-endpoint` +/// 2. Auto-connect via `DevToolsActivePort` (default) +/// +/// An explicit endpoint wins because it names the browser directly, so it must +/// not be second-guessed by local profile discovery: it is how you reach a +/// browser this machine cannot find on disk — another host, a container, a +/// port-forwarded device, or an instance whose profile lives somewhere the +/// channel tables don't describe. It also short-circuits `--browser` and +/// `--channel` entirely, since those exist only to locate a profile directory. +/// `DevToolsActivePort` is the automatic fallback for the ordinary case where +/// the browser is local and its profile is where the vendor puts it. pub fn resolve_ws_url( ws_endpoint: Option<&str>, user_data_dir: Option<&str>, diff --git a/src/lib.rs b/src/lib.rs index 1385734..b3a2871 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -549,18 +549,29 @@ fn print_daemon_list(format: format::OutputFormat) -> Result<()> { .ok() .and_then(|b| serde_json::from_slice(&b).ok()); + // Same validation kill-daemon applies before signalling: a PID file + // holding 0, or a value outside pid_t, is not a PID we will act on. + // It matters for a read-only listing too — `kill(0, 0)` probes the + // caller's own process group and succeeds, so a stray 0 would other- + // wise be reported as a running daemon. #[cfg(unix)] - let pid_str = read_pid_file_checked(&protocol::pid_path(&key)).ok(); - #[cfg(not(unix))] - let pid_str = std::fs::read_to_string(protocol::pid_path(&key)).ok(); - let pid = pid_str.and_then(|s| s.trim().parse::().ok()); - - #[cfg(unix)] - let running = pid - .and_then(|p| i32::try_from(p).ok()) - .map(daemon_pid_alive); + let (pid, running) = { + let contents = read_pid_file_checked(&protocol::pid_path(&key)).ok(); + let pid = contents.as_deref().and_then(parse_pid_file_contents); + ( + pid.and_then(|p| u32::try_from(p).ok()), + pid.map(daemon_pid_alive), + ) + }; + // No cheap liveness probe without libc::kill, so liveness is unknown + // and the state column stays "?" rather than claiming stale/running. #[cfg(not(unix))] - let running: Option = None; + let (pid, running) = ( + std::fs::read_to_string(protocol::pid_path(&key)) + .ok() + .and_then(|s| s.trim().parse::().ok()), + None::, + ); rows.push(Row { pid, From 564e7460d29978e43096e1aa6410eb7a18a307e3 Mon Sep 17 00:00:00 2001 From: Aero Date: Thu, 27 Aug 2026 21:14:22 +0800 Subject: [PATCH 4/6] fix: verify a daemon owns a PID before signalling it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round of review follow-ups. kill-daemon signalled whatever PID it read from the PID file. A daemon killed with SIGKILL leaves that file behind — cleanup is skipped by design — the OS is then free to reuse the PID for another process of the same user, and SIGTERM would land on that process instead. The --all sweep widened the exposure by walking every PID file rather than one. Verify against the live process instead of the recorded metadata. Cross- checking the PID file against the info sidecar would prove nothing: both are written by the same daemon at the same moment and go stale together. Only the daemon binds its keyed socket, and it does so under the same startup lock that wrote the PID file, so a live listener at that path is proof the recorded PID is still ours, and ECONNREFUSED is proof it is not. stop_daemon_at now probes the socket and, when nothing answers, removes the files without signalling. The probe sends nothing and drops the connection; the daemon treats that as a read error and keeps serving. A daemon wedged inside a CDP call still passes, since the kernel completes the connect from the listen backlog without the daemon accepting. Deliberate trade-off: if the socket file is deleted while the daemon is alive, kill-daemon now declines to signal and the orphan exits on its 5-minute idle timeout. Leaving an orphan that self-heals is the better failure of the two. Also: - short_endpoint stripped only ws://, so a wss:// endpoint — which --ws-endpoint accepts verbatim — rendered in list-daemons as the bare scheme "wss:". Strip either scheme. - The README described list-daemons as showing running daemons; it also shows stale entries. - Add anyhow context to the daemon connect paths (Unix socket, Windows address-file read, Windows TCP connect) and to the Windows address-file write. These surface in the user-visible "daemon unavailable" warning from run_direct_fallback. - The four path helpers in protocol.rs restated their signatures; their comments now explain what the instance key is for. Verified: a live non-daemon process recorded in a daemon PID file is no longer signalled (it was, before this), and a real listening daemon is still stopped normally. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- src/client.rs | 20 +++++++++--- src/daemon.rs | 8 ++++- src/lib.rs | 84 ++++++++++++++++++++++++++++++++++++++++++++++++- src/protocol.rs | 15 ++++++--- 5 files changed, 118 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ff2ec56..28b616c 100644 --- a/README.md +++ b/README.md @@ -244,7 +244,7 @@ On Unix, `kill-daemon` signals the targeted daemon with `SIGTERM`, removes its s | `--browser ` | Browser to auto-connect to (chrome/edge) | | `--channel ` | Browser release channel (stable/beta/canary/dev) | -Commands: `list-daemons` shows every running daemon; `kill-daemon [--all]` stops one or all. +Commands: `list-daemons` shows every daemon this user has on-disk state for — running ones and, on Unix, stale entries left by a daemon that died without cleaning up; `kill-daemon [--all]` stops one or all. Global `--block-url` and `--unblock-url` update the **active tab's** block list and apply via `Network.setBlockedURLs`; the daemon re-applies each tab's list when that tab is in use, so blocking is isolated per tab. **Note:** Chrome only blocks *subresources* (images, scripts, fetch/XHR, stylesheets, CDN, trackers, fonts). The top-level navigation document itself is never blocked — e.g. `--block-url "*example.com*"` then `navigate https://example.com` still loads the page, but any `*.png`, `*.woff2`, etc. subresources on it are blocked. diff --git a/src/client.rs b/src/client.rs index f87ea14..b4757ed 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1,4 +1,4 @@ -use anyhow::{bail, Result}; +use anyhow::{bail, Context, Result}; #[cfg(windows)] use std::os::windows::process::CommandExt; use std::time::{Duration, SystemTime}; @@ -11,13 +11,25 @@ use crate::protocol::*; #[cfg(unix)] async fn connect_daemon(key: &str) -> Result { - Ok(UnixStream::connect(socket_path(key)).await?) + let path = socket_path(key); + UnixStream::connect(&path) + .await + .with_context(|| format!("Failed to connect to daemon {key} at {}", path.display())) } #[cfg(windows)] async fn connect_daemon(key: &str) -> Result { - let addr = std::fs::read_to_string(addr_path(key))?; - Ok(TcpStream::connect(addr.trim()).await?) + let path = addr_path(key); + let addr = std::fs::read_to_string(&path).with_context(|| { + format!( + "Failed to read daemon {key} address file {}", + path.display() + ) + })?; + let addr = addr.trim(); + TcpStream::connect(addr) + .await + .with_context(|| format!("Failed to connect to daemon {key} at {addr}")) } /// Read the daemon wait timeout from `DAEMON_WAIT_TIMEOUT_SECS`, defaulting to diff --git a/src/daemon.rs b/src/daemon.rs index 8b13bf4..6cd04af 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -477,7 +477,13 @@ pub async fn run_daemon(ws_url: &str, browser: &str) -> Result<()> { // If we wait for CdpClient::connect first, a Chrome/network permission prompt // can block the daemon and cause the CLI's 5-second wait_for_daemon timeout to expire. let listener = TcpListener::bind("127.0.0.1:0").await?; - std::fs::write(addr_path(&key), listener.local_addr()?.to_string())?; + let addr_file = addr_path(&key); + std::fs::write(&addr_file, listener.local_addr()?.to_string()).with_context(|| { + format!( + "Failed to write daemon address file {}", + addr_file.display() + ) + })?; listener }; diff --git a/src/lib.rs b/src/lib.rs index b3a2871..cd21b52 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -501,6 +501,26 @@ fn daemon_pid_alive(pid: i32) -> bool { ret == 0 || std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) } +/// Whether a daemon is actually listening on `sock_path`. +/// +/// Used to confirm a PID file still describes a live daemon before signalling +/// it. Connecting is the check: only the daemon binds this path, so a successful +/// connect proves one is alive, while `ECONNREFUSED` proves the opposite — a +/// socket file left behind by a daemon that died without cleaning up. +/// +/// The probe sends nothing and drops the connection immediately; the daemon +/// treats that as a read error and continues serving. A daemon wedged inside a +/// CDP call still passes, because the kernel completes the connect from the +/// listen backlog without the daemon having to accept it. +/// +/// Any other error (a missing socket file, a permission problem) counts as "not +/// verified" and therefore as not alive: leaving an orphan that exits on its own +/// idle timeout is a better failure than signalling an unrelated process. +#[cfg(unix)] +fn daemon_listening_at(sock_path: &std::path::Path) -> bool { + std::os::unix::net::UnixStream::connect(sock_path).is_ok() +} + /// Human-readable uptime, coarsened to the largest two units that matter. fn format_uptime(secs: u64) -> String { match secs { @@ -513,8 +533,13 @@ fn format_uptime(secs: u64) -> String { /// Endpoint as `host:port`, dropping the browser-GUID path that makes the full /// URL too wide to tabulate. fn short_endpoint(ws_url: &str) -> String { + // Auto-connect only ever builds ws://, but --ws-endpoint is taken verbatim + // and may be wss://. Strip the longer scheme first: "ws://" is not a prefix + // of "wss://", but checking in the other order would still be a trap for + // anyone adding schemes later. ws_url - .strip_prefix("ws://") + .strip_prefix("wss://") + .or_else(|| ws_url.strip_prefix("ws://")) .unwrap_or(ws_url) .split('/') .next() @@ -701,6 +726,25 @@ fn stop_daemon_at( pid_path.display() ) })?; + // Do not signal a PID we cannot tie back to a live daemon. + // A daemon killed with SIGKILL leaves its PID file behind + // (cleanup is skipped by design), the OS is free to reuse that + // PID for an unrelated process of this same user, and SIGTERM + // would then land on that process instead. Only the daemon + // binds this socket, and it does so under the same startup lock + // that wrote the PID file, so a live listener there is proof + // the recorded PID is still ours. + if !daemon_listening_at(sock_path) { + let _ = std::fs::remove_file(sock_path); + let _ = std::fs::remove_file(info_path); + let _ = std::fs::remove_file(pid_path); + println!( + "Daemon (PID {pid}) is not listening on {}; cleaned up its files \ + without signalling, since that PID may now belong to another process.", + sock_path.display() + ); + return Ok(()); + } // Signal the process directly via libc to avoid shelling out // to /usr/bin/kill. A return of 0 means the signal was // delivered; -1 with errno ESRCH means the process is gone @@ -2112,6 +2156,44 @@ mod tests { assert_eq!(short_endpoint(""), ""); } + /// --ws-endpoint is taken verbatim, so a wss:// URL reaches this function + /// and must not render as the bare scheme "wss:". + #[test] + fn test_short_endpoint_handles_secure_scheme() { + assert_eq!( + short_endpoint("wss://example.test:9222/devtools/browser/abc"), + "example.test:9222" + ); + assert_eq!( + short_endpoint("wss://example.test:9222"), + "example.test:9222" + ); + } + + /// The guard that keeps SIGTERM off a PID the daemon no longer owns. + #[cfg(unix)] + #[test] + fn test_daemon_listening_at_requires_a_live_listener() { + let dir = tempfile::tempdir().unwrap(); + + // Nothing bound: a bare path, and a plain file where a socket should be. + assert!(!daemon_listening_at(&dir.path().join("absent.sock"))); + let regular = dir.path().join("regular.sock"); + std::fs::write(®ular, "").unwrap(); + assert!(!daemon_listening_at(®ular)); + + // A real listener at the path is what proves a daemon is alive. + let live = dir.path().join("live.sock"); + let listener = std::os::unix::net::UnixListener::bind(&live).unwrap(); + assert!(daemon_listening_at(&live)); + + // Once it goes away the socket file remains but connects are refused — + // exactly the SIGKILLed-daemon case that must not be signalled. + drop(listener); + assert!(live.exists(), "socket file outlives the listener"); + assert!(!daemon_listening_at(&live)); + } + /// A live process must read as running; PID 0 is never probed because /// `kill(0, …)` addresses the caller's own process group. #[cfg(unix)] diff --git a/src/protocol.rs b/src/protocol.rs index 89806a8..ab8eab7 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -102,24 +102,31 @@ fn daemon_file_prefix() -> String { format!("chrome-devtools-daemon{}", user_suffix()) } -/// Path to the Unix domain socket for daemon communication. +/// Unix domain socket a daemon listens on. The key keeps concurrent daemons +/// from binding the same path, which is what lets one browser's daemon exist +/// alongside another's. #[cfg(unix)] pub fn socket_path(key: &str) -> PathBuf { std::env::temp_dir().join(format!("{}-{key}.sock", daemon_file_prefix())) } -/// Path to the named-pipe address file for daemon communication (Windows). +/// File recording the loopback TCP address a daemon listens on (Windows has no +/// Unix sockets, so the address is published rather than the endpoint itself). +/// Keyed for the same reason as [`socket_path`]. #[cfg(windows)] pub fn addr_path(key: &str) -> PathBuf { std::env::temp_dir().join(format!("{}-{key}.addr", daemon_file_prefix())) } -/// Path to the daemon PID file. +/// PID file for one daemon. Keyed so a sweep can tell the instances apart and +/// signal only the one it means to — an unkeyed name would make every daemon +/// look like the same process to `kill-daemon`. pub fn pid_path(key: &str) -> PathBuf { std::env::temp_dir().join(format!("{}-{key}.pid", daemon_file_prefix())) } -/// Path to the daemon's metadata sidecar (see [`DaemonInfo`]). +/// Metadata sidecar for one daemon (see [`DaemonInfo`]), keyed alongside its +/// PID and socket so the three always describe the same instance. pub fn info_path(key: &str) -> PathBuf { std::env::temp_dir().join(format!("{}-{key}.info", daemon_file_prefix())) } From 8a04c9a794f9e9fef6610aef2150663a2215c6eb Mon Sep 17 00:00:00 2001 From: Aero Date: Thu, 27 Aug 2026 22:37:45 +0800 Subject: [PATCH 5/6] fix: hold the startup lock across kill-daemon's read, probe and signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The socket probe added in 564e746 closed the stale-PID case but not a startup race, correctly spotted in review. stop_daemon_at read the PID, probed the socket and signalled without holding any lock. A daemon can start for the same key between the read and the probe, so the read could return a stale PID the OS has since recycled while the probe sees the newcomer's listener. The listener then proves only that some daemon holds the key — not that it owns the PID already in hand — and SIGTERM lands on an unrelated process. Take daemon::lock_daemon_files() for the whole sequence, the same lock that covers PID-file creation and socket binding. The two observations become one atomic step: no daemon can have appeared since the read, so a live listener proves the PID read is that listener's. On timeout the stop fails closed, signalling nothing, and says to retry. This cannot be blocked by the case kill-daemon exists for: the daemon drops the startup lock before entering its accept loop, so a daemon wedged inside a CDP call does not hold it, and only a concurrent startup or cleanup contends. It also composes with the daemon's own exit path, where cleanup_at try_locks and backs off when contended — the dying daemon skips its file removal and the caller does it instead. The guarantee depends on run_daemon writing the PID file before binding the endpoint, both under that lock, which was an implicit coupling. Noted at that site so a reorder does not silently break kill-daemon. The race is a cross-process TOCTOU and is not reproducible from a single process, so the new test covers the enforceable half: with the lock held via a second file descriptor, stop_daemon_at must refuse, signal nothing, and leave the PID and info files in place for the retry. Verified both directions after the change: a process holding the keyed socket with its PID recorded is signalled and stopped, while a live process recorded without a listener is spared and only its files removed. Co-Authored-By: Claude Opus 5 (1M context) --- src/daemon.rs | 10 +++++-- src/lib.rs | 83 ++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 80 insertions(+), 13 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index 6cd04af..8fcf475 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -36,7 +36,7 @@ enum ConnectionOutcome { /// open() itself (it has no effect on regular files); the `is_file` check /// then rejects it. macOS $TMPDIR and Windows %TEMP% are per-user, so there /// the checks are inert. -fn open_lock_file() -> Result { +pub(crate) fn open_lock_file() -> Result { open_lock_file_at(&lock_path()) } @@ -204,7 +204,7 @@ fn lock_wait_timeout() -> Duration { /// pre-acquired the predictable lock path park daemon startup forever. /// Async (poll + `tokio::time::sleep`) so a contended lock never blocks the /// runtime's worker thread. -async fn lock_daemon_files() -> Result { +pub(crate) async fn lock_daemon_files() -> Result { let f = open_lock_file()?; let timeout = lock_wait_timeout(); let deadline = tokio::time::Instant::now() + timeout; @@ -453,6 +453,12 @@ pub async fn run_daemon(ws_url: &str, browser: &str) -> Result<()> { // predecessor can delete files this daemon just claimed. let startup_lock = lock_daemon_files().await?; + // Order matters beyond this function: the PID file is written *before* the + // endpoint is bound, and both happen under `startup_lock`. `kill-daemon` + // relies on that pairing — holding the same lock, it can treat a live + // listener as proof that the PID it just read is this daemon's, because no + // daemon can have bound the socket without its PID already being on disk. + // Reordering these, or moving either outside the lock, breaks that. write_pid_file_checked(&pid_path(&key))?; write_info_file(&key, browser, ws_url); diff --git a/src/lib.rs b/src/lib.rs index cd21b52..907239e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -659,13 +659,14 @@ fn print_daemon_list(format: format::OutputFormat) -> Result<()> { /// (or that there was nothing to do) and returns an error only when the daemon /// may still be running — the caller decides whether one failure aborts a /// whole sweep. -fn stop_daemon_instance(key: &str) -> Result<()> { +async fn stop_daemon_instance(key: &str) -> Result<()> { stop_daemon_at( &protocol::pid_path(key), &protocol::info_path(key), #[cfg(unix)] &protocol::socket_path(key), ) + .await } /// Stop the pre-instance-key daemon (`chrome-devtools-daemon-.pid`) left @@ -675,7 +676,7 @@ fn stop_daemon_instance(key: &str) -> Result<()> { /// identify, so no scoped target can claim it, and after an upgrade nothing /// else will ever reach it. Silent when the files don't exist, which is the /// common case. -fn stop_legacy_unkeyed_daemon() -> Result { +async fn stop_legacy_unkeyed_daemon() -> Result { let pid_path = protocol::legacy_unkeyed_pid_path(); if !pid_path.exists() { return Ok(false); @@ -689,6 +690,7 @@ fn stop_legacy_unkeyed_daemon() -> Result { #[cfg(unix)] &protocol::legacy_unkeyed_socket_path(), ) + .await .map(|()| true) } @@ -697,11 +699,34 @@ fn stop_legacy_unkeyed_daemon() -> Result { /// The ownership-checked read and single SIGTERM live here so the keyed and /// legacy layouts cannot drift apart in how carefully they treat a predictable /// path in shared `/tmp`. -fn stop_daemon_at( +async fn stop_daemon_at( pid_path: &std::path::Path, info_path: &std::path::Path, #[cfg(unix)] sock_path: &std::path::Path, ) -> Result<()> { + // Held across the whole read -> probe -> signal sequence, and it must be + // the same lock the daemon takes to write its PID file and bind its socket. + // + // Without it the liveness probe below proves only that *some* daemon holds + // the key, not that the PID already in hand is that daemon's: a daemon can + // start between the read and the probe, so the read could return a stale + // PID the OS has since recycled while the probe sees the newcomer's + // listener — and the signal would land on an unrelated process. Holding the + // lock makes the two observations one atomic step. + // + // This cannot be blocked by a daemon wedged inside a CDP call: the daemon + // drops the startup lock before entering its accept loop, so only a + // concurrent startup or cleanup contends, and both are brief. + #[cfg(unix)] + let _startup_lock = { + use anyhow::Context as _; + daemon::lock_daemon_files().await.context( + "Could not take the daemon startup lock, so a PID could not be safely matched to \ + its daemon; nothing was signalled. Retry — a concurrent daemon start or shutdown \ + holds this lock only briefly.", + )? + }; + // Ownership-checked read: the path is predictable in shared /tmp, so // never act on a PID file that was planted there by another user. #[cfg(unix)] @@ -730,10 +755,13 @@ fn stop_daemon_at( // A daemon killed with SIGKILL leaves its PID file behind // (cleanup is skipped by design), the OS is free to reuse that // PID for an unrelated process of this same user, and SIGTERM - // would then land on that process instead. Only the daemon - // binds this socket, and it does so under the same startup lock - // that wrote the PID file, so a live listener there is proof - // the recorded PID is still ours. + // would then land on that process instead. + // + // Only the daemon binds this socket, and it binds it *after* + // writing its PID file, both under the startup lock this + // function holds. So no daemon can have appeared since the read + // above, and a live listener is proof that the PID just read is + // that listener's. if !daemon_listening_at(sock_path) { let _ = std::fs::remove_file(sock_path); let _ = std::fs::remove_file(info_path); @@ -1419,7 +1447,7 @@ pub async fn run() -> Result<()> { let mut failures = 0usize; let mut attempted = keys.len(); for key in &keys { - if let Err(e) = stop_daemon_instance(key) { + if let Err(e) = stop_daemon_instance(key).await { failures += 1; eprintln!("{e:#}"); } @@ -1428,7 +1456,7 @@ pub async fn run() -> Result<()> { // the total — otherwise a legacy-only failure reports "1 of 0". // It counts only when one was actually there, so the total does // not inflate on the common no-legacy-files path. - match stop_legacy_unkeyed_daemon() { + match stop_legacy_unkeyed_daemon().await { Ok(found) => attempted += usize::from(found), Err(e) => { attempted += 1; @@ -1460,7 +1488,7 @@ pub async fn run() -> Result<()> { and you just want to stop leftover daemons, use --all." ) })?; - stop_daemon_instance(&protocol::instance_key(&ws_url))?; + stop_daemon_instance(&protocol::instance_key(&ws_url)).await?; } // Best-effort sweep of the pre-uid-suffix file names: a daemon @@ -2170,7 +2198,40 @@ mod tests { ); } - /// The guard that keeps SIGTERM off a PID the daemon no longer owns. + /// `kill-daemon` must not signal while it cannot hold the startup lock: + /// without it, the PID it read and the listener it probed can belong to + /// different daemons. `flock` is per open-file-description, so a second + /// handle in this process contends exactly as a starting daemon would. + /// + /// Takes the real lock file, briefly, because the path is not injectable; + /// no other test contends for it, and nothing here is ever signalled — the + /// lock is unavailable, so the function returns before reaching `kill`. + #[cfg(unix)] + #[tokio::test] + async fn test_stop_daemon_at_will_not_signal_without_the_startup_lock() { + let dir = tempfile::tempdir().unwrap(); + let pid_path = dir.path().join("d.pid"); + let info_path = dir.path().join("d.info"); + let sock_path = dir.path().join("d.sock"); + std::fs::write(&pid_path, std::process::id().to_string()).unwrap(); + std::fs::write(&info_path, "{}").unwrap(); + + let holder = daemon::open_lock_file().unwrap(); + holder.try_lock().unwrap(); + + let err = stop_daemon_at(&pid_path, &info_path, &sock_path) + .await + .expect_err("must refuse while the startup lock is held elsewhere"); + let msg = err.to_string(); + assert!(msg.contains("startup lock"), "{msg}"); + assert!(msg.contains("nothing was signalled"), "{msg}"); + + // Failing closed also means leaving state alone for the retry. + assert!(pid_path.exists(), "PID file must survive a refused stop"); + assert!(info_path.exists(), "info file must survive a refused stop"); + } + + /// The guard that keeps SIGTERM off a PID the daemon no longer owns. /// The guard that keeps SIGTERM off a PID the daemon no longer owns. #[cfg(unix)] #[test] fn test_daemon_listening_at_requires_a_live_listener() { From dd48c04180daa85f74005d56549a3c402c727c16 Mon Sep 17 00:00:00 2001 From: Aero Date: Thu, 27 Aug 2026 23:44:24 +0800 Subject: [PATCH 6/6] fix: name the actual browser in CDP connection failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth round of review follow-ups. The daemon reported every CDP connection failure as "Failed to connect to Chrome" regardless of --browser. Fixing only that prefix would have made the message contradict itself, because the inner error from CdpClient::connect also hardcoded Chrome: Failed to connect to Microsoft Edge: Failed to connect to Chrome at ws://… So the label is threaded one level deeper, into CdpClient::connect — two call sites, the daemon and the direct fallback — and the now-redundant outer prefix is dropped. Adds browser::display_name for the human-readable form, which falls back to the input so a diagnostic never silently claims the wrong browser. That also corrects the message users actually read, which the review did not mention: the connect timeout said "Chrome may be waiting for a human to approve the remote-debugging connection dialog … ask the human to check Chrome". Under --browser edge that sent people to the wrong window. Five doc and help sites match "Failed to connect to Chrome" as a literal lookup key. Chrome's text still contains that exact substring, verified, so they keep working; the skill's failure-handling heading and the --force help text now say the browser name varies, so an Edge user recognises their own error. Also: - The skill described list-daemons as showing what is running; it also shows stale entries on Unix. - Remove a doc sentence duplicated onto one line in lib.rs, introduced when the startup-lock test was inserted ahead of that anchor in 8a04c9a. Verified at runtime on the daemon path, the direct path and via the msedge alias: Edge failures name Microsoft Edge, Chrome failures still contain the documented substring, and neither message repeats itself. Co-Authored-By: Claude Opus 5 (1M context) --- skill/chrome-devtools/SKILL.md | 9 +++++++-- src/browser.rs | 9 +++++++++ src/cdp.rs | 22 +++++++++++++--------- src/daemon.rs | 20 ++++++++++++++------ src/lib.rs | 12 ++++++------ 5 files changed, 49 insertions(+), 23 deletions(-) diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index 0c8f6e5..85bf01c 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -498,7 +498,9 @@ consequences for the recipe above: `kill-daemon` resolves the default Chrome profile and would stop the user's daemon instead of the headless one. -`list-daemons` shows what is running (PID, browser, endpoint, uptime), and on +`list-daemons` shows every daemon with on-disk state — running ones and, on +Unix, stale entries whose process is gone — with PID, browser, endpoint and +uptime for each, and on Unix `kill-daemon --all` clears every daemon regardless of endpoint — including ones whose browser has already exited, which a scoped kill cannot reach because it has no endpoint left to resolve. @@ -587,13 +589,16 @@ chrome-devtools kill-daemon # refuses when run non-interactively (age chrome-devtools kill-daemon --force # kills unconditionally ``` -## Failure Handling: "Failed to connect to Chrome" / a command hangs +## Failure Handling: "Failed to connect to \" / a command hangs Chrome's remote-debugging connection requires a one-time **human approval dialog** in Chrome. If a command hangs or fails with a connection/timeout error, the most likely cause is that this dialog is open and waiting for the human — not a bug you can fix by retrying. +The error names whichever browser you targeted — `Failed to connect to Chrome`, +or `Failed to connect to Microsoft Edge` under `--browser edge`. + **If a command hangs for a long time or errors with "Failed to connect to Chrome" or "Timed out ... connecting to Chrome":** 1. Retry **at most once** (the human may have already approved it just now). diff --git a/src/browser.rs b/src/browser.rs index dbf6e2f..ce587c0 100644 --- a/src/browser.rs +++ b/src/browser.rs @@ -88,6 +88,15 @@ pub fn canonical_name(name: &str) -> String { Browser::parse(name).map_or_else(|_| name.to_string(), |b| b.flag_name().to_string()) } +/// Human-readable browser name for messages the user reads — "Chrome", +/// "Microsoft Edge". +/// +/// Falls back to the input for names we don't know, so a diagnostic never +/// silently claims the wrong browser. +pub fn display_name(name: &str) -> String { + Browser::parse(name).map_or_else(|_| name.to_string(), |b| b.label().to_string()) +} + /// A Chromium-based browser the CLI knows how to auto-connect to. /// /// Both speak the same DevTools Protocol; they differ only in where the diff --git a/src/cdp.rs b/src/cdp.rs index 4a104d1..d8063de 100644 --- a/src/cdp.rs +++ b/src/cdp.rs @@ -192,27 +192,31 @@ fn connect_timeout() -> std::time::Duration { } impl CdpClient { - /// Connect to Chrome via WebSocket and return a CDP client. + /// Connect to the browser via WebSocket and return a CDP client. /// - /// Bounded by a timeout: without it, a pending Chrome remote-debugging - /// consent dialog leaves the WebSocket handshake hanging indefinitely, - /// which (via the daemon) makes every command appear to hang forever - /// with no diagnostic. - pub async fn connect(ws_url: &str) -> Result { + /// Bounded by a timeout: without it, a pending remote-debugging consent + /// dialog leaves the WebSocket handshake hanging indefinitely, which (via + /// the daemon) makes every command appear to hang forever with no + /// diagnostic. + /// + /// `browser` is the human-readable name for the diagnostics only — these + /// messages are the ones a user actually reads, and telling an Edge user to + /// go and check Chrome sends them to the wrong window. + pub async fn connect(ws_url: &str, browser: &str) -> Result { let timeout_dur = connect_timeout(); let (ws, _) = tokio::time::timeout(timeout_dur, connect_async(ws_url)) .await .map_err(|_| { anyhow!( - "Timed out after {}s connecting to Chrome at {ws_url}. Chrome may be \ + "Timed out after {}s connecting to {browser} at {ws_url}. {browser} may be \ waiting for a human to approve the remote-debugging connection dialog. \ If you are an automated agent: do not retry in a loop and do not run \ kill-daemon (it will not fix this and will require a fresh approval) — \ - stop and ask the human to check Chrome, then retry once.", + stop and ask the human to check {browser}, then retry once.", timeout_dur.as_secs() ) })? - .map_err(|e| anyhow!("Failed to connect to Chrome at {ws_url}: {e}"))?; + .map_err(|e| anyhow!("Failed to connect to {browser} at {ws_url}: {e}"))?; let (write, read) = ws.split(); Ok(Self { write, diff --git a/src/daemon.rs b/src/daemon.rs index 8fcf475..4900d25 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -335,7 +335,7 @@ impl Drop for CleanupGuard { /// - `$shutdown` is polled by `&mut` reference, so the caller must pin it /// first (`tokio::pin!`); passing an unpinned future fails to compile. macro_rules! run_accept_loop_body { - ($accept:expr, $client:expr, $ws_url:expr, $shutdown:expr) => { + ($accept:expr, $client:expr, $ws_url:expr, $browser:expr, $shutdown:expr) => { loop { tokio::select! { // Shutdown first, and `biased` so a ready signal always wins @@ -351,7 +351,7 @@ macro_rules! run_accept_loop_body { break; } accept = tokio::time::timeout(idle_timeout(), $accept) => match accept { - Ok(Ok((stream, _))) => match handle_connection(stream, $client, $ws_url).await { + Ok(Ok((stream, _))) => match handle_connection(stream, $client, $ws_url, $browser).await { ConnectionOutcome::Continue => {} ConnectionOutcome::Fatal => break, }, @@ -501,7 +501,7 @@ pub async fn run_daemon(ws_url: &str, browser: &str) -> Result<()> { let mut client: Option = None; // Signal readiness by socket/address existence (it's already bound) - run_accept_loop_body!(listener.accept(), &mut client, ws_url, shutdown); + run_accept_loop_body!(listener.accept(), &mut client, ws_url, browser, shutdown); // File cleanup is handled by `_guard` (also covers signal/panic exits). @@ -516,6 +516,7 @@ async fn handle_connection( mut stream: S, client: &mut Option, ws_url: &str, + browser: &str, ) -> ConnectionOutcome where S: AsyncReadExt + AsyncWriteExt + Unpin, @@ -547,13 +548,17 @@ where // Connect lazily if client.is_none() { - match CdpClient::connect(ws_url).await { + let label = crate::browser::display_name(browser); + match CdpClient::connect(ws_url, &label).await { Ok(c) => *client = Some(c), Err(e) => { let resp = DaemonResponse { success: false, output: String::new(), - error: format!("Failed to connect to Chrome: {e:#}"), + // No prefix: CdpClient::connect's error already names the + // browser and the endpoint, so wrapping it here produced + // "Failed to connect to X: Failed to connect to X at ...". + error: format!("{e:#}"), navigated_to: None, error_code: Some(ErrorCode::ChromeConnection as u32), }; @@ -571,7 +576,10 @@ where None => DaemonResponse { success: false, output: String::new(), - error: String::from("Failed to connect to Chrome: client initialization failed"), + error: format!( + "Failed to connect to {}: client initialization failed", + crate::browser::display_name(browser) + ), navigated_to: None, error_code: Some(ErrorCode::ChromeConnection as u32), }, diff --git a/src/lib.rs b/src/lib.rs index 907239e..3dc3078 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -421,10 +421,10 @@ pub enum Commands { KillDaemon { /// Skip the confirmation/refusal guard and kill unconditionally. /// - /// Killing the daemon drops any already-approved Chrome remote-debugging - /// connection; reconnecting requires the human to re-approve Chrome's - /// consent dialog. This does NOT fix "Failed to connect to Chrome" - /// errors — do not use it as a retry step. + /// Killing the daemon drops any already-approved remote-debugging + /// connection; reconnecting requires the human to re-approve the + /// browser's consent dialog. This does NOT fix "Failed to connect to + /// " errors — do not use it as a retry step. #[arg(long)] force: bool, @@ -1687,7 +1687,7 @@ async fn run_direct_fallback(cli: &Cli, ws_url: &str, error: &anyhow::Error) -> /// Direct execution without daemon (fallback). async fn run_direct(cli: &Cli, ws_url: &str) -> Result { - let mut client = cdp::CdpClient::connect(ws_url).await?; + let mut client = cdp::CdpClient::connect(ws_url, &browser::display_name(&cli.browser)).await?; let is_browser = cli.is_browser_level(); @@ -2231,7 +2231,7 @@ mod tests { assert!(info_path.exists(), "info file must survive a refused stop"); } - /// The guard that keeps SIGTERM off a PID the daemon no longer owns. /// The guard that keeps SIGTERM off a PID the daemon no longer owns. + /// The guard that keeps SIGTERM off a PID the daemon no longer owns. #[cfg(unix)] #[test] fn test_daemon_listening_at_requires_a_live_listener() {