diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 0c4b7980c..3fa07c72b 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -366,7 +366,11 @@ jobs: # docs/deployment/serve-decisions.md). What it does assert is everything # between the browser and that boundary. serve: - timeout-minutes: 30 + # Was 30 against a measured 9.5m. The lifecycle step below builds + # biorouter-cli's tests, which unify its dev-dependency features and so + # recompile part of the graph a plain `cargo build` already built (4m56s on + # a loaded M4 Max); the budget keeps the same margin over that. + timeout-minutes: 45 if: github.event_name != 'schedule' runs-on: ubuntu-latest steps: @@ -437,7 +441,9 @@ jobs: [ "$(curl -s -o /dev/null -w '%{http_code}' 'http://127.0.0.1:18777/?t=wrong')" = 401 ] \ || fail 'a wrong token was accepted' - # The token is spent once, for a cookie, and leaves the address bar. + # The token is exchanged for a cookie and leaves the address bar. It is + # not consumed (SD-9 in docs/deployment/serve-decisions.md): the + # readiness loop above has already redeemed it. curl -s -o /dev/null -D "$RUNNER_TEMP/h" "http://127.0.0.1:18777/?t=citoken" grep -qi '^HTTP/1.1 303' "$RUNNER_TEMP/h" || fail 'no redirect after the token exchange' grep -qi 'set-cookie: biorouter_session=' "$RUNNER_TEMP/h" || fail 'no session cookie' @@ -474,5 +480,21 @@ jobs: 'http://127.0.0.1:18777/headless/fs/read?path=/etc/passwd')" = 403 ] \ || fail 'the filesystem endpoint read outside its allowed roots' - kill "$serve_pid" || true + # Stopping serve by pid stops its daemon. It used to leave the daemon + # running, still holding this port and still honouring the token. + # `serve` reaps the daemon before it exits, so the port is closed the + # moment `wait` returns. + kill "$serve_pid" || fail 'serve was no longer running' + wait "$serve_pid" || true + ! curl -s -o /dev/null --max-time 5 http://127.0.0.1:18777/status \ + || fail 'the daemon outlived serve' echo 'the browser contract holds' + + # SIGTERM and SIGINT to `serve`, and SIGKILL, which leaves only the + # daemon's own parent watch to stop it. Here rather than in the workspace + # test job, which runs `--lib --bins` only: this needs a `biorouterd` from + # the same tree beside `biorouter`, which the build step above provides. + - name: Stopping serve stops its daemon + env: + BIOROUTER_DISABLE_KEYRING: "true" + run: cargo test -p biorouter-cli --test serve_lifecycle diff --git a/CLAUDE.md b/CLAUDE.md index 17e8c1799..3692a60ac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1164,7 +1164,7 @@ Test the gate where it is: the unit tests in `agents/agent.rs` prints a URL. The daemon serves the SPA **on its own origin**, so nothing is proxied. This replaced a standalone `biorouter-headless` binary and its Linux tarball, both deleted 2026-08-23; release assets went 11 → 10. Design and reasoning: -[`docs/deployment/serve-decisions.md`](docs/deployment/serve-decisions.md) (SD-1..SD-8), +[`docs/deployment/serve-decisions.md`](docs/deployment/serve-decisions.md) (SD-1..SD-9), [`serve-architecture.md`](docs/deployment/serve-architecture.md), [`browser-access.md`](docs/deployment/browser-access.md). @@ -1203,16 +1203,31 @@ replaced a standalone `biorouter-headless` binary and its Linux tarball, both de - **The serving path.** `Settings.serve_ui` (`BIOROUTER_SERVE_UI`) → `routes::web_ui::attach`, called **after** `check_token` in `commands/agent.rs` so the shell and bundle sit *structurally outside* that middleware rather than being exempted by path. - The document is gated by a browser token exchanged once for an `HttpOnly; SameSite=Strict` + The document is gated by a browser token exchanged for an `HttpOnly; SameSite=Strict` cookie; the cookie authenticates **the document only** — API routes still take - `X-Secret-Key`, so there is no CSRF surface and `check_token` needed no change. -- **`routes::shell`** holds the 16 `/headless/*` endpoints (path kept deliberately; the - renderer builds `origin + '/headless'`). They had **no authentication at all** on the old + `X-Secret-Key`, so there is no CSRF surface and `check_token` needed no change. ⚠ The + exchange does **not** consume the token — it is honoured as often as it is presented until the + daemon stops, and the cookie's value *is* the token. Deliberate, not an oversight: SD-9 in + `serve-decisions.md` records why single use was rejected. It used to be called "spent". +- **`routes::shell`** holds the 16 `/headless/*` paths — 17 handlers, since `/headless/settings` + answers both GET and POST (path kept deliberately; the renderer builds + `origin + '/headless'`). They had **no authentication at all** on the old binary and `fs_read` had no path validation; the port confines every filesystem handler to an allowlist and refuses credential stores by name. - **WebSocket origins**: `routes::origin_matches_host` compares `Origin` to the request's own `Host`. That is a same-origin test, not a wildcard, and it is what lets a browser reach the daemon at a LAN address `is_local_origin` has never heard of. +- **`serve` owns its daemon's lifetime**, because the daemon honours the token and serves the + shell carrying its secret for as long as it runs: stopping `serve` is the only revocation. + Two layers. Every exit path after the spawn goes through `stop_daemon` (SIGTERM, a 10 s grace, + then SIGKILL and reap), with SIGINT/SIGTERM listeners installed *before* the spawn; and on + Unix the daemon is started with `biorouterd agent --exit-with-parent `, which + covers a SIGKILLed or crashed `serve`. ⚠ Until 2026-09 neither held — the `Child` was moved + into a `spawn_blocking` wait, so only a terminal's process-group Ctrl-C ever reached the + daemon and `kill ` orphaned it on the port. ⚠ The flag is opt-in and compares + `getppid()` with the pid `serve` named, **not with 1**: an orphan is re-parented to the + nearest subreaper (`systemd --user`, a container init), so `== 1` never fires there. Never + pass it from the desktop. - ⚠ **Three traps.** The app uses a **HashRouter**, so its routes live in the fragment and never reach the daemon — that is the only reason `/sessions/{id}` (a real API route) does not collide with the app's own; a history router would break pages silently. The bundle must @@ -1220,10 +1235,16 @@ replaced a standalone `biorouter-headless` binary and its Linux tarball, both de *relative* base and a relative bundle served at `/` breaks deep links while the landing page looks fine. And `/../web` resolves for the packaged app and Windows zip but **not** for deb/rpm (`/usr/bin/../web` = `/usr/web`), hence `/usr/share/biorouter/web`. -- **Tests:** `cargo test -p biorouter-server --lib routes::web_ui routes::shell`, +- **Tests:** `cargo test -p biorouter-server --lib -- routes::web_ui routes::shell` (the `--` + is required: cargo takes one filter before it and rejects a second with a usage error), `cargo test -p biorouter-cli --lib commands::serve`, the `serve` job in `.github/workflows/rust.yml`, and `smoke_serve` in - `scripts/smoke-test-release-artifacts.sh`. + `scripts/smoke-test-release-artifacts.sh`. The lifecycle has its own binary, + `cargo test -p biorouter-cli --test serve_lifecycle`, which runs the real `serve` and + signals it by pid; ⚠ it needs a `biorouterd` from the same tree beside `biorouter`, which + `cargo test -p biorouter-cli` does not build — run `cargo build -p biorouter-cli -p + biorouter-server` first. CI runs it in the `serve` job, because the workspace test job is + `--lib --bins` and never runs an integration binary. ### Communication Flow diff --git a/Cargo.lock b/Cargo.lock index af6e84c3c..e7e890ea9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1152,6 +1152,7 @@ dependencies = [ "futures", "http 1.4.0", "indicatif", + "libc", "open", "pulldown-cmark", "rand 0.8.5", diff --git a/crates/biorouter-cli/Cargo.toml b/crates/biorouter-cli/Cargo.toml index 9feb236fc..bf20acac4 100644 --- a/crates/biorouter-cli/Cargo.toml +++ b/crates/biorouter-cli/Cargo.toml @@ -81,6 +81,10 @@ tikv-jemalloc-ctl = { workspace = true, optional = true } [target.'cfg(target_os = "windows")'.dependencies] winapi = { version = "0.3", features = ["wincred"] } +# `serve` asks the daemon it started to stop with SIGTERM before it kills it. +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [features] default = ["jemalloc"] # Use tuned jemalloc as the global allocator (returns freed pages to the OS). diff --git a/crates/biorouter-cli/src/cli.rs b/crates/biorouter-cli/src/cli.rs index c4beb87ab..f05c3725e 100644 --- a/crates/biorouter-cli/src/cli.rs +++ b/crates/biorouter-cli/src/cli.rs @@ -1584,7 +1584,11 @@ enum Command { no_token: bool, /// Directory holding the built interface - #[arg(long, help = "Directory holding the built web interface")] + #[arg( + long, + help = "Directory holding the built web interface. Takes precedence over \ + BIOROUTER_SERVE_UI; either must contain an index.html" + )] web_dir: Option, /// Open a browser once it is ready diff --git a/crates/biorouter-cli/src/commands/serve.rs b/crates/biorouter-cli/src/commands/serve.rs index 87182283e..9056b7ca8 100644 --- a/crates/biorouter-cli/src/commands/serve.rs +++ b/crates/biorouter-cli/src/commands/serve.rs @@ -25,13 +25,33 @@ //! configuration, not a missing feature — but it does mean the daemon a `serve` //! session talks to is less capable than the one the desktop application //! starts, and anything assuming otherwise is wrong. +//! +//! # Why the daemon cannot outlive this command +//! +//! The daemon holds the port, and it answers the browser token and serves the +//! shell carrying its secret for as long as it runs — so stopping `serve` is +//! the only way an operator has to revoke the URL it printed. Two layers make +//! that hold however `serve` ends: +//! +//! 1. Every way out of [`handle_serve`] after the spawn — SIGINT, SIGTERM, the +//! daemon dying, a startup that never became ready — goes through +//! [`stop_daemon`], which asks the daemon to stop, waits, and then kills +//! and reaps it. +//! 2. On Unix the daemon is started with `--exit-with-parent ` and +//! stops itself once this process is gone, which covers the endings that run +//! no code at all: SIGKILL, a crash. +//! +//! Before this, the only thing that ever stopped the daemon was a terminal's +//! Ctrl-C, which reaches the whole foreground process group and so the daemon +//! directly. `kill ` from anywhere else left it running. use crate::commands::exe_path::{biorouterd_for, current_exe_resolved, daemon_file_name}; use anyhow::{bail, Context, Result}; -use std::net::{TcpListener, TcpStream, ToSocketAddrs}; +use std::net::{TcpListener, ToSocketAddrs}; use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; +use std::process::Stdio; use std::time::{Duration, Instant}; +use tokio::process::{Child, Command}; /// Not 3000. That is `biorouterd`'s own default, and the old `biorouter web` /// used it too — a default that collides with the daemon this command starts is @@ -41,6 +61,14 @@ pub const DEFAULT_PORT: u16 = 8765; /// How long to wait for the daemon to answer before giving up. const READY_TIMEOUT: Duration = Duration::from_secs(60); +/// How long the daemon gets to shut down on its own before it is killed. +/// +/// Its graceful shutdown waits for open connections to finish, and a browser +/// tab that is still open holds some that never do — so without a limit, +/// stopping `serve` with a tab open would wait forever. The daemon applies the +/// same figure to itself when it finds itself orphaned. +const STOP_GRACE: Duration = Duration::from_secs(10); + /// Run the browser-served interface. #[allow(clippy::too_many_arguments)] pub async fn handle_serve( @@ -64,18 +92,7 @@ pub async fn handle_serve( ); } - let web_dir = match web_dir { - Some(dir) => { - if !dir.join("index.html").is_file() { - bail!( - "no web interface at {} (expected an index.html there)", - dir.display() - ); - } - dir - } - None => resolve_web_dir()?, - }; + let web_dir = resolve_web_dir(web_dir)?; let browser_token = if no_token { None @@ -90,9 +107,21 @@ pub async fn handle_serve( // child, so the pair covers the race this pre-flight cannot. preflight_port(&host, port)?; + // Listen for a stop request BEFORE the daemon exists. A handler replaces + // the default action — which for SIGTERM is to end this process on the spot + // and leave the daemon behind — so from here on a signal waits to be read, + // including one that lands during the readiness wait below. + let mut stop = StopSignals::install()?; + let daemon = resolve_biorouterd()?; - let mut child = Command::new(&daemon) - .arg("agent") + let mut command = Command::new(&daemon); + command.arg("agent"); + // The second layer; see the module documentation. + #[cfg(unix)] + command + .arg("--exit-with-parent") + .arg(std::process::id().to_string()); + let mut child = command .env("BIOROUTER_HOST", &host) .env("BIOROUTER_PORT", port.to_string()) .env("BIOROUTER_SERVER__SECRET_KEY", &secret_key) @@ -104,22 +133,65 @@ pub async fn handle_serve( ) // See the module documentation: no proof-of-user digest, on purpose. .stdin(Stdio::null()) + // A backstop for a panic unwinding through here. Every ordinary path + // goes through `stop_daemon`, which asks before it insists. + .kill_on_drop(true) .spawn() .with_context(|| format!("could not start {}", daemon.display()))?; - if let Err(e) = wait_until_ready(&host, port, &mut child) { - let _ = child.kill(); - let _ = child.wait(); - return Err(e); + let outcome: Result<()> = async { + tokio::select! { + ready = wait_until_ready(&host, port, &mut child) => ready?, + _ = stop.recv() => { + println!("\nStopping."); + return Ok(()); + } + } + + let url = browser_url(&host, port, browser_token.as_deref()); + print_banner( + &url, + &host, + port, + browser_token.as_deref(), + bind_is_loopback, + ); + if open_browser { + let _ = webbrowser::open(&url); + } + + tokio::select! { + _ = stop.recv() => { + println!("\nStopping."); + Ok(()) + } + status = child.wait() => match status { + Ok(s) if s.success() => Ok(()), + Ok(s) => bail!("biorouterd exited with {s}"), + Err(e) => bail!("could not wait on biorouterd: {e}"), + }, + } } + .await; - let url = browser_url(&host, port, browser_token.as_deref()); + stop_daemon(&mut child, &mut stop).await; + outcome +} + +/// What the operator reads once the daemon is answering. +fn print_banner( + url: &str, + host: &str, + port: u16, + browser_token: Option<&str>, + bind_is_loopback: bool, +) { println!("\n Biorouter is serving at\n\n {url}\n"); if !bind_is_loopback { - match reachable_address(&host) { + match reachable_address(host) { Some(addr) => println!( " From another machine on this network:\n\n {}\n", - browser_url(&addr, port, browser_token.as_deref()) + browser_url(&addr, port, browser_token) ), // The old implementation fell back to 127.0.0.1 here, which printed // a URL that could not possibly work from the other machine the user @@ -138,36 +210,110 @@ pub async fn handle_serve( } println!(" The model is whichever `biorouter configure` chose; a browser cannot change it."); println!(" Press Ctrl-C to stop.\n"); +} - if open_browser { - let _ = webbrowser::open(&url); +/// The requests to stop that `serve` honours: SIGINT and SIGTERM on Unix, +/// Ctrl-C elsewhere. +/// +/// SIGHUP is deliberately left alone. A terminal hanging up signals the whole +/// foreground process group, daemon included; `nohup` exists to make both +/// ignore it, and installing a handler here would override that. Any other +/// SIGHUP ends `serve` the default way and the daemon's parent watch follows. +struct StopSignals { + #[cfg(unix)] + interrupt: tokio::signal::unix::Signal, + #[cfg(unix)] + terminate: tokio::signal::unix::Signal, +} + +impl StopSignals { + fn install() -> Result { + #[cfg(unix)] + use tokio::signal::unix::{signal, SignalKind}; + Ok(Self { + #[cfg(unix)] + interrupt: signal(SignalKind::interrupt()).context("could not listen for SIGINT")?, + #[cfg(unix)] + terminate: signal(SignalKind::terminate()).context("could not listen for SIGTERM")?, + }) } - // Hand the terminal back to the daemon and stop when it does, or when the - // user interrupts. Killing the child on the way out is what stops a stray - // daemon holding the port after Ctrl-C. - let result = tokio::select! { - _ = tokio::signal::ctrl_c() => { - println!("\nStopping."); - Ok(()) + /// Resolve on the next request to stop. + async fn recv(&mut self) { + #[cfg(unix)] + tokio::select! { + _ = self.interrupt.recv() => {} + _ = self.terminate.recv() => {} } - status = tokio::task::spawn_blocking(move || child.wait()) => { - match status { - Ok(Ok(s)) if s.success() => Ok(()), - Ok(Ok(s)) => bail!("biorouterd exited with {s}"), - Ok(Err(e)) => bail!("could not wait on biorouterd: {e}"), - Err(e) => bail!("could not wait on biorouterd: {e}"), + // A console Ctrl-C reaches every process attached to the console, so on + // Windows the daemon has been told as well and is already stopping. + #[cfg(not(unix))] + { + let _ = tokio::signal::ctrl_c().await; + } + } +} + +/// Stop the daemon and reap it: ask, give it [`STOP_GRACE`], then insist. +/// +/// A second request to stop while it is shutting down skips the rest of the +/// wait. A daemon that has already exited is only reaped, so every path can end +/// here without first asking whether it needs to. +async fn stop_daemon(child: &mut Child, stop: &mut StopSignals) { + if matches!(child.try_wait(), Ok(Some(_))) { + return; + } + ask_to_stop(child); + tokio::select! { + waited = tokio::time::timeout(STOP_GRACE, child.wait()) => { + if matches!(waited, Ok(Ok(_))) { + return; } + // The usual reason, measured: an open browser tab keeps the + // renderer's 25 s catalog long poll parked on the daemon, and its + // graceful shutdown waits for that request to finish. + eprintln!( + "biorouterd did not finish within {}s (an open browser tab keeps a request \ + waiting); killing it.", + STOP_GRACE.as_secs() + ); } + _ = stop.recv() => eprintln!("Killing biorouterd."), + } + let _ = child.start_kill(); + let _ = child.wait().await; +} + +/// Ask the daemon to shut down gracefully: SIGTERM, which it handles exactly as +/// it handles Ctrl-C — draining connections and taking a llama-server sidecar +/// down with it. +#[cfg(unix)] +fn ask_to_stop(child: &Child) { + let Some(pid) = child.id().and_then(|p| libc::pid_t::try_from(p).ok()) else { + return; }; - result + // SAFETY: `kill(2)` has no memory-safety preconditions. `id()` is `None` + // once the child has been reaped, so this pid is still our own child and + // cannot have been recycled for an unrelated process. + unsafe { + libc::kill(pid, libc::SIGTERM); + } } +/// Windows has no SIGTERM to send. A console Ctrl-C has usually reached the +/// daemon already; if nothing has, [`stop_daemon`] kills it once the grace has +/// passed. +#[cfg(not(unix))] +fn ask_to_stop(_child: &Child) {} + /// The URL to open, with the browser token in it. /// -/// The token is spent on the first request: the daemon exchanges it for a -/// session cookie and redirects, so it does not linger in the address bar or in -/// the `Referer` of anything the page later loads. +/// Opening it exchanges the token for a session cookie and redirects, so the +/// token does not linger in the address bar or in the `Referer` of anything the +/// page later loads. It is not *spent*, which is how this comment used to put +/// it: the exchange works as often as the token is presented, for anyone who +/// has it, until the daemon stops. Decision SD-9 in +/// `docs/deployment/serve-decisions.md` records why that is deliberate. fn browser_url(host: &str, port: u16, token: Option<&str>) -> String { // A bare IPv6 address needs brackets in a URL; a hostname must not have them. let host = if host.contains(':') && !host.starts_with('[') { @@ -238,7 +384,10 @@ fn preflight_port(host: &str, port: u16) -> Result<()> { /// reports success against *any* listener on that port — so a daemon that died /// on startup, next to some unrelated process holding the port, looks exactly /// like a healthy one. -fn wait_until_ready(host: &str, port: u16, child: &mut std::process::Child) -> Result<()> { +/// +/// Asynchronous so that a request to stop can interrupt it: the wait can run +/// for a minute, and the daemon is already running for all of it. +async fn wait_until_ready(host: &str, port: u16, child: &mut Child) -> Result<()> { let connect_host = match host { "0.0.0.0" => "127.0.0.1", "::" | "[::]" => "::1", @@ -249,9 +398,13 @@ fn wait_until_ready(host: &str, port: u16, child: &mut std::process::Child) -> R if let Some(status) = child.try_wait().context("could not poll biorouterd")? { bail!("biorouterd exited during startup with {status}"); } - if let Ok(addrs) = (connect_host, port).to_socket_addrs() { + if let Ok(addrs) = tokio::net::lookup_host((connect_host, port)).await { for addr in addrs { - if TcpStream::connect_timeout(&addr, Duration::from_millis(250)).is_ok() { + let attempt = tokio::time::timeout( + Duration::from_millis(250), + tokio::net::TcpStream::connect(addr), + ); + if matches!(attempt.await, Ok(Ok(_))) { return Ok(()); } } @@ -262,7 +415,7 @@ fn wait_until_ready(host: &str, port: u16, child: &mut std::process::Child) -> R READY_TIMEOUT.as_secs() ); } - std::thread::sleep(Duration::from_millis(100)); + tokio::time::sleep(Duration::from_millis(100)).await; } } @@ -281,7 +434,70 @@ fn resolve_biorouterd() -> Result { Ok(PathBuf::from(daemon_file_name())) } -/// Candidate locations for the built interface, in order. +/// Where the interface comes from: `--web-dir`, else `BIOROUTER_SERVE_UI`, else +/// the first of [`web_dir_candidates`] that holds one. +fn resolve_web_dir(flag: Option) -> Result { + choose_web_dir( + flag, + std::env::var_os("BIOROUTER_SERVE_UI"), + web_dir_candidates, + ) +} + +/// [`resolve_web_dir`], with what it reads passed in. +/// +/// A directory the operator names — with the flag or with the variable — is +/// used as named or refused, never skipped. The variable used to be only the +/// first *candidate* of the search, so one naming a directory with no +/// `index.html` was passed over in silence and `serve` went on to serve +/// whatever the search found next: a bundle the operator had not chosen, with +/// nothing to say so, while the same path given as `--web-dir` was refused. +/// The flag wins when both are set, as a command line does over the +/// environment it runs in. +fn choose_web_dir( + flag: Option, + variable: Option, + candidates: impl FnOnce() -> Vec, +) -> Result { + let named = match (flag, variable) { + (Some(dir), _) => Some((dir, "--web-dir")), + // Blank reads as unset, as it does for `BIOROUTER_PATH_ROOT`: taken + // literally, an empty path is the working directory. + (None, Some(dir)) if !dir.to_string_lossy().trim().is_empty() => { + Some((PathBuf::from(dir), "BIOROUTER_SERVE_UI")) + } + (None, _) => None, + }; + if let Some((dir, source)) = named { + if !dir.join("index.html").is_file() { + bail!( + "no web interface at {} (expected an index.html there; the path came from \ + {source})", + dir.display() + ); + } + return Ok(dir); + } + + let candidates = candidates(); + for candidate in &candidates { + if candidate.join("index.html").is_file() { + return Ok(normalise(candidate)); + } + } + let tried = candidates + .iter() + .map(|p| format!(" {}", normalise(p).display())) + .collect::>() + .join("\n"); + bail!( + "could not find the Biorouter web interface. Tried:\n{tried}\n\nPoint at it with \ + --web-dir , or set BIOROUTER_SERVE_UI. In a development tree, build it with \ + `cd ui/desktop && npm run build:web`." + ) +} + +/// Where to look for the built interface when none was named, in order. /// /// Returned as a list so the failure can name every one of them. An error that /// says only "not found" leaves the reader guessing which of four layouts the @@ -307,9 +523,6 @@ fn web_dir_candidates() -> Vec { /// location, which is a fixed path that has nothing to do with this install. fn web_dir_candidates_for(exe: Option<&Path>) -> Vec { let mut out = Vec::new(); - if let Ok(dir) = std::env::var("BIOROUTER_SERVE_UI") { - out.push(PathBuf::from(dir)); - } if let Some(dir) = exe.and_then(Path::parent) { // Packaged: the binaries sit in `Resources/bin`, the bundle beside // them in `Resources/web`. @@ -338,25 +551,6 @@ fn web_dir_candidates_for(exe: Option<&Path>) -> Vec { out } -fn resolve_web_dir() -> Result { - let candidates = web_dir_candidates(); - for candidate in &candidates { - if candidate.join("index.html").is_file() { - return Ok(normalise(candidate)); - } - } - let tried = candidates - .iter() - .map(|p| format!(" {}", normalise(p).display())) - .collect::>() - .join("\n"); - bail!( - "could not find the Biorouter web interface. Tried:\n{tried}\n\nPoint at it with \ - --web-dir , or set BIOROUTER_SERVE_UI. In a development tree, build it with \ - `cd ui/desktop && npm run build:web`." - ) -} - /// Tidy `a/b/../c` for display without touching the filesystem. /// /// `canonicalize` is not usable here: it fails on a path that does not exist, @@ -445,9 +639,6 @@ mod tests { /// installation layouts they are in. #[test] fn a_missing_interface_names_every_path_it_tried() { - // Held because the sibling tests below set this variable, and - // `web_dir_candidates` reads it. - let _env = env_lock::lock_env([("BIOROUTER_SERVE_UI", None::)]); let candidates = web_dir_candidates(); assert!( candidates.len() >= 2, @@ -459,23 +650,129 @@ mod tests { ); } - /// Restated against `web_dir_candidates_for`, which takes the executable as - /// an argument: the previous version scanned this file's own source for the - /// order two string literals appear in, and would have passed against a - /// build that never read the variable at all. + /// A search that would find a bundle, so the tests below can tell a + /// refusal from a quiet fall-back to something else — which is exactly + /// what finding F8 was. + fn a_bundle_found_elsewhere(root: &Path) -> impl FnOnce() -> Vec { + let web = root.join("found-elsewhere"); + std::fs::create_dir_all(&web).unwrap(); + std::fs::write(web.join("index.html"), b"").unwrap(); + move || vec![web] + } + + fn an_interface_at(dir: &Path) -> PathBuf { + std::fs::create_dir_all(dir).unwrap(); + std::fs::write(dir.join("index.html"), b"").unwrap(); + dir.to_path_buf() + } + + /// Finding F8 of the 2026-09-10 QA run: `--web-dir` naming a directory + /// with no interface was fatal, while `BIOROUTER_SERVE_UI` naming the same + /// directory was skipped and `serve` served the next bundle it found. + #[test] + fn a_named_directory_without_an_interface_is_refused_however_it_was_named() { + let tmp = tempfile::tempdir().unwrap(); + let typo = tmp.path().join("wbe"); + for (flag, variable, source) in [ + (Some(typo.clone()), None, "--web-dir"), + ( + None, + Some(typo.clone().into_os_string()), + "BIOROUTER_SERVE_UI", + ), + ] { + let err = choose_web_dir(flag, variable, a_bundle_found_elsewhere(tmp.path())) + .expect_err("a named directory with no index.html must be refused, not skipped") + .to_string(); + assert!( + err.starts_with(&format!( + "no web interface at {} (expected an index.html there", + typo.display() + )), + "both spellings must fail with the same message: {err}" + ); + assert!( + err.contains(source), + "the refusal must say where the path came from: {err}" + ); + } + } + + #[test] + fn a_named_directory_is_used_in_preference_to_anything_the_search_finds() { + let tmp = tempfile::tempdir().unwrap(); + let named = an_interface_at(&tmp.path().join("named")); + assert_eq!( + choose_web_dir( + None, + Some(named.clone().into_os_string()), + a_bundle_found_elsewhere(tmp.path()) + ) + .unwrap(), + named + ); + assert_eq!( + choose_web_dir( + Some(named.clone()), + None, + a_bundle_found_elsewhere(tmp.path()) + ) + .unwrap(), + named + ); + } + + /// The flag wins, and the variable is not even checked when it does: a + /// stale export in a shell profile must not fail a command line that says + /// exactly what to serve. #[test] - fn an_explicit_setting_is_looked_at_before_anything_else() { - let _env = env_lock::lock_env([( - "BIOROUTER_SERVE_UI", - Some("/somewhere/explicit/web".to_string()), - )]); - let candidates = - web_dir_candidates_for(Some(Path::new("/opt/Biorouter/resources/bin/biorouter"))); + fn the_flag_takes_precedence_over_the_variable() { + let tmp = tempfile::tempdir().unwrap(); + let flag = an_interface_at(&tmp.path().join("flag")); + let stale = tmp.path().join("stale").into_os_string(); assert_eq!( - candidates.first(), - Some(&PathBuf::from("/somewhere/explicit/web")), - "the explicit setting must be consulted before the packaged locations: \ - {candidates:?}" + choose_web_dir( + Some(flag.clone()), + Some(stale), + a_bundle_found_elsewhere(tmp.path()) + ) + .unwrap(), + flag + ); + } + + #[test] + fn a_blank_variable_is_not_a_choice() { + let tmp = tempfile::tempdir().unwrap(); + for blank in ["", " "] { + let found = choose_web_dir( + None, + Some(blank.into()), + a_bundle_found_elsewhere(tmp.path()), + ) + .unwrap(); + assert!( + found.ends_with("found-elsewhere"), + "a blank value must fall through to the search, got {found:?}" + ); + } + } + + /// The tests above pass the variable in, so on their own they would pass + /// against a build that never read it. This one goes through the real + /// environment. + #[test] + fn serve_reads_the_variable_it_documents() { + let tmp = tempfile::tempdir().unwrap(); + let missing = tmp.path().join("no-interface-here"); + let _env = + env_lock::lock_env([("BIOROUTER_SERVE_UI", Some(missing.display().to_string()))]); + let err = resolve_web_dir(None) + .expect_err("a variable naming a directory with no interface must be refused") + .to_string(); + assert!( + err.contains("the path came from BIOROUTER_SERVE_UI"), + "{err}" ); } @@ -489,7 +786,6 @@ mod tests { #[cfg(unix)] #[test] fn a_symlinked_executable_resolves_to_the_real_installation() { - let _env = env_lock::lock_env([("BIOROUTER_SERVE_UI", None::)]); let tmp = tempfile::tempdir().unwrap(); let bin = tmp.path().join("Resources").join("bin"); @@ -573,7 +869,6 @@ mod tests { /// rule. #[test] fn the_resolved_candidates_are_never_windows_verbatim_paths() { - let _env = env_lock::lock_env([("BIOROUTER_SERVE_UI", None::)]); let tmp = tempfile::tempdir().unwrap(); let exe = tmp .path() @@ -648,7 +943,6 @@ mod tests { /// `%LOCALAPPDATA%\ui\desktop\src\web`, neither of which can ever exist. #[test] fn a_windows_style_install_finds_the_bundle_through_its_breadcrumb() { - let _env = env_lock::lock_env([("BIOROUTER_SERVE_UI", None::)]); let tmp = tempfile::tempdir().unwrap(); let source_bin = application(&tmp.path().join("Application")); let exe = windows_style_install(tmp.path()); @@ -683,7 +977,6 @@ mod tests { /// the binary is this installation's own and must win. #[test] fn the_breadcrumb_is_consulted_after_the_locations_beside_the_binary() { - let _env = env_lock::lock_env([("BIOROUTER_SERVE_UI", None::)]); let tmp = tempfile::tempdir().unwrap(); let source_bin = application(&tmp.path().join("Application")); let exe = windows_style_install(tmp.path()); @@ -721,7 +1014,6 @@ mod tests { /// install that used to work no longer does. #[test] fn a_stale_breadcrumb_is_named_among_the_paths_that_were_tried() { - let _env = env_lock::lock_env([("BIOROUTER_SERVE_UI", None::)]); let tmp = tempfile::tempdir().unwrap(); let application_root = tmp.path().join("Application"); let source_bin = application(&application_root); @@ -757,7 +1049,6 @@ mod tests { /// at all, and a file that is empty or is not a path. #[test] fn a_missing_or_unusable_breadcrumb_falls_back_without_panicking() { - let _env = env_lock::lock_env([("BIOROUTER_SERVE_UI", None::)]); let tmp = tempfile::tempdir().unwrap(); let exe = windows_style_install(tmp.path()); let install = exe.parent().unwrap().to_path_buf(); diff --git a/crates/biorouter-cli/tests/serve_lifecycle.rs b/crates/biorouter-cli/tests/serve_lifecycle.rs new file mode 100644 index 000000000..ab85e801f --- /dev/null +++ b/crates/biorouter-cli/tests/serve_lifecycle.rs @@ -0,0 +1,352 @@ +//! Stopping `biorouter serve` stops the daemon it started. +//! +//! It did not. `serve` never killed its child — the `Child` had been moved into +//! the task waiting on it, so the Ctrl-C arm held no handle — and the only +//! thing that ever stopped the daemon was a terminal's Ctrl-C, which reaches +//! the whole foreground process group. `kill ` left the daemon +//! running, still holding the port, still accepting the launch token, and still +//! serving the shell that carries its secret. These tests run the real binaries +//! and stop `serve` the way a process manager or an operator does: by pid. +//! +//! ⚠ They need a `biorouterd` built from this tree beside the `biorouter` under +//! test. `cargo test -p biorouter-cli` does not build another package's binary, +//! so build it first: +//! +//! ```text +//! cargo build -p biorouter-cli -p biorouter-server +//! cargo test -p biorouter-cli --test serve_lifecycle +//! ``` +//! +//! Unix only: the second layer (`--exit-with-parent`) is Unix only, and there +//! is no SIGTERM to send on Windows. +#![cfg(unix)] + +use std::io::{Read, Write}; +use std::net::{Ipv4Addr, SocketAddr, TcpListener, TcpStream}; +use std::path::PathBuf; +use std::process::{Child, Command, ExitStatus, Stdio}; +use std::time::{Duration, Instant}; + +/// How long `serve` may take to stop. Its grace for the daemon is ten seconds; +/// this leaves room for a loaded machine beyond that, so a pass means "the +/// daemon did not survive", not "it survived for less than N seconds". +const STOP_BUDGET: Duration = Duration::from_secs(30); + +/// A debug daemon's cold start on a busy CI runner. +const READY_BUDGET: Duration = Duration::from_secs(120); + +const TOKEN: &str = "lifecycle-test-token"; + +fn biorouter() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_biorouter")) +} + +/// `serve` starts the daemon that sits beside it, so this is the one it runs. +fn biorouterd() -> PathBuf { + biorouter().with_file_name("biorouterd") +} + +/// Refuse to run against a daemon these tests cannot be about. +/// +/// A stale one would fail the second-layer test for a reason that has nothing +/// to do with `serve`, and a missing one fails every test with a spawn error. +/// Either should say what to do rather than leave the reader to work it out. +fn require_a_daemon_from_this_tree() { + let daemon = biorouterd(); + assert!( + daemon.is_file(), + "{} does not exist. `cargo test -p biorouter-cli` does not build another \ + package's binary; run `cargo build -p biorouter-server --bin biorouterd` first.", + daemon.display() + ); + let help = Command::new(&daemon) + .args(["agent", "--help"]) + .output() + .expect("run biorouterd agent --help"); + assert!( + String::from_utf8_lossy(&help.stdout).contains("--exit-with-parent"), + "{} predates --exit-with-parent, so it is older than this test. Rebuild it with \ + `cargo build -p biorouter-server --bin biorouterd`.", + daemon.display() + ); +} + +/// A port nothing is listening on. Released before `serve` binds it, which +/// leaves a window another process could take it in; `serve` then refuses to +/// start and the readiness wait reports that, rather than a wrong result. +fn free_port() -> u16 { + TcpListener::bind((Ipv4Addr::LOCALHOST, 0)) + .and_then(|l| l.local_addr()) + .map(|a| a.port()) + .expect("find a free port") +} + +fn port_is_open(port: u16) -> bool { + TcpStream::connect_timeout( + &SocketAddr::from((Ipv4Addr::LOCALHOST, port)), + Duration::from_millis(500), + ) + .is_ok() +} + +/// The status code of `GET path`, or `None` when nothing answered. +fn http_status(port: u16, path: &str) -> Option { + let mut stream = TcpStream::connect_timeout( + &SocketAddr::from((Ipv4Addr::LOCALHOST, port)), + Duration::from_millis(500), + ) + .ok()?; + stream.set_read_timeout(Some(Duration::from_secs(5))).ok()?; + write!( + stream, + "GET {path} HTTP/1.1\r\nHost: 127.0.0.1:{port}\r\nConnection: close\r\n\r\n" + ) + .ok()?; + let mut head = [0u8; 64]; + let read = stream.read(&mut head).ok()?; + String::from_utf8_lossy(&head[..read]) + .split_whitespace() + .nth(1)? + .parse() + .ok() +} + +/// Whether `pid` is a process that has not yet exited. A zombie has exited: it +/// is waiting to be reaped by a parent, and holds no port and no memory. +fn is_running(pid: u32) -> bool { + let out = Command::new("ps") + .args(["-o", "stat=", "-p", &pid.to_string()]) + .output() + .expect("run ps"); + let stat = String::from_utf8_lossy(&out.stdout); + let stat = stat.trim(); + !stat.is_empty() && !stat.starts_with('Z') +} + +/// Start time and command line: together they name one process, where a pid +/// alone can be recycled for an unrelated one once the original has exited. +/// Empty once the process is gone. +fn identity(pid: u32) -> String { + let out = Command::new("ps") + .args(["-o", "lstart=,command=", "-p", &pid.to_string()]) + .output() + .expect("run ps"); + String::from_utf8_lossy(&out.stdout).trim().to_string() +} + +fn signal(pid: u32, name: &str) { + let status = Command::new("kill") + .args([&format!("-{name}"), &pid.to_string()]) + .status() + .expect("run kill"); + assert!(status.success(), "kill -{name} {pid} failed"); +} + +fn wait_for(budget: Duration, mut done: impl FnMut() -> bool) -> Option { + let start = Instant::now(); + while start.elapsed() < budget { + if done() { + return Some(start.elapsed()); + } + std::thread::sleep(Duration::from_millis(100)); + } + None +} + +/// A running `serve` and the daemon it started. +struct Served { + serve: Child, + daemon: u32, + /// The daemon's [`identity`] when it was found, so cleanup can tell it + /// from a later process that happens to reuse its pid. + daemon_identity: String, + port: u16, + root: tempfile::TempDir, +} + +impl Served { + fn start() -> Self { + require_a_daemon_from_this_tree(); + + let root = tempfile::tempdir().expect("temp dir"); + let web = root.path().join("web"); + std::fs::create_dir_all(&web).unwrap(); + std::fs::write( + web.join("index.html"), + "lifecycle", + ) + .unwrap(); + let home = root.path().join("home"); + std::fs::create_dir_all(&home).unwrap(); + let log = std::fs::File::create(root.path().join("serve.log")).unwrap(); + + let port = free_port(); + let serve = Command::new(biorouter()) + .args(["serve", "--port", &port.to_string(), "--token", TOKEN]) + .arg("--web-dir") + .arg(&web) + // Nothing here may touch the developer's own configuration, + // sessions or keychain. + .env("HOME", &home) + .env("BIOROUTER_PATH_ROOT", root.path().join("biorouter")) + .env("BIOROUTER_DISABLE_KEYRING", "true") + // `serve` sets both for the daemon; an inherited value must not + // leak into what is under test. + .env_remove("BIOROUTER_SERVE_UI") + .env_remove("BIOROUTER_BROWSER_TOKEN") + .stdin(Stdio::null()) + .stdout(log.try_clone().unwrap()) + .stderr(log) + .spawn() + .expect("spawn biorouter serve"); + + let mut served = Self { + serve, + daemon: 0, + daemon_identity: String::new(), + port, + root, + }; + let ready = wait_for(READY_BUDGET, || { + http_status(port, "/status") == Some(200) + || matches!(served.serve.try_wait(), Ok(Some(_))) + }); + assert!( + ready.is_some() && matches!(served.serve.try_wait(), Ok(None)), + "serve did not come up on port {port}:\n{}", + served.log() + ); + served.daemon = served.only_child(); + served.daemon_identity = identity(served.daemon); + assert!( + served.daemon_identity.contains("biorouterd") + && served.daemon_identity.contains("agent"), + "serve's child is not the daemon: {:?}", + served.daemon_identity + ); + served + } + + /// The daemon: `serve`'s one child. + fn only_child(&self) -> u32 { + let out = Command::new("pgrep") + .args(["-P", &self.serve.id().to_string()]) + .output() + .expect("run pgrep"); + let pids: Vec = String::from_utf8_lossy(&out.stdout) + .split_whitespace() + .filter_map(|p| p.parse().ok()) + .collect(); + assert_eq!( + pids.len(), + 1, + "expected serve to have exactly one child, the daemon: {pids:?}" + ); + pids[0] + } + + fn log(&self) -> String { + std::fs::read_to_string(self.root.path().join("serve.log")).unwrap_or_default() + } + + /// Stop `serve` with `name` and assert that it exits and takes the daemon + /// with it. + fn stop_with(mut self, name: &str) -> ExitStatus { + signal(self.serve.id(), name); + let mut status = None; + let took = wait_for(STOP_BUDGET, || { + status = self.serve.try_wait().ok().flatten(); + status.is_some() + }); + let status = match (took, status) { + (Some(took), Some(status)) => { + eprintln!("serve exited {took:?} after SIG{name}"); + status + } + _ => panic!( + "serve was still running {STOP_BUDGET:?} after SIG{name} (its daemon is pid \ + {}):\n{}", + self.daemon, + self.log() + ), + }; + + // `serve` reaps the daemon before it exits, so both of these hold the + // moment it is gone. They are what an operator stopping the service + // needs to be true: nothing left running, and nothing on the port. + assert!( + !is_running(self.daemon), + "the daemon (pid {}) outlived serve after SIG{name}:\n{}", + self.daemon, + self.log() + ); + assert!( + !port_is_open(self.port), + "port {} is still accepting connections after serve exited on SIG{name}", + self.port + ); + status + } +} + +impl Drop for Served { + /// A failed assertion must not leave a daemon running on the machine. The + /// daemon is killed only while its pid still names the process that was + /// found at startup, so a pid recycled for something unrelated is left + /// alone. + fn drop(&mut self) { + let _ = self.serve.kill(); + let _ = self.serve.wait(); + if self.daemon != 0 && identity(self.daemon) == self.daemon_identity { + let _ = Command::new("kill") + .args(["-KILL", &self.daemon.to_string()]) + .status(); + } + } +} + +#[test] +fn sigterm_to_serve_stops_its_daemon() { + let status = Served::start().stop_with("TERM"); + assert!( + status.success(), + "a requested stop is a clean exit: {status}" + ); +} + +/// The report measured this one hanging as well as orphaning: the Ctrl-C arm +/// returned, and then the runtime waited forever on the task that was still +/// blocked waiting for the daemon to exit. +#[test] +fn sigint_to_serve_stops_its_daemon() { + let status = Served::start().stop_with("INT"); + assert!( + status.success(), + "a requested stop is a clean exit: {status}" + ); +} + +/// SIGKILL runs no code in `serve` at all, so this is the second layer alone: +/// the daemon sees that its parent is gone and stops itself. +#[test] +fn a_daemon_whose_serve_was_killed_outright_stops_itself() { + let mut served = Served::start(); + let daemon = served.daemon; + served.serve.kill().expect("SIGKILL serve"); + served.serve.wait().expect("reap serve"); + + let took = wait_for(STOP_BUDGET, || !is_running(daemon)); + match took { + Some(took) => eprintln!("the orphaned daemon stopped {took:?} after serve was killed"), + None => panic!( + "the daemon (pid {daemon}) was still running {STOP_BUDGET:?} after serve was \ + killed:\n{}", + served.log() + ), + } + assert!( + !port_is_open(served.port), + "port {} is still accepting connections after the orphaned daemon stopped", + served.port + ); +} diff --git a/crates/biorouter-server/src/commands/agent.rs b/crates/biorouter-server/src/commands/agent.rs index e9e784051..8862757c8 100644 --- a/crates/biorouter-server/src/commands/agent.rs +++ b/crates/biorouter-server/src/commands/agent.rs @@ -4,13 +4,30 @@ use anyhow::Result; use axum::middleware; use biorouter_server::auth::check_token; use http::HeaderValue; +use tokio_util::sync::CancellationToken; use tower_http::compression::CompressionLayer; use tower_http::cors::{AllowOrigin, Any, CorsLayer}; use tracing::info; +/// How often a daemon started with `--exit-with-parent` checks that the process +/// that launched it is still there. +#[cfg(unix)] +const PARENT_POLL: std::time::Duration = std::time::Duration::from_millis(500); + +/// How long an orphaned daemon gives its own graceful shutdown before it exits +/// regardless. +/// +/// Only the orphan has a deadline. Everywhere else, whoever sent the signal is +/// still there to escalate if the daemon does not finish — `biorouter serve` +/// does, after a grace of the same length. An orphan has nobody left to, and +/// a graceful shutdown waits for open connections to finish: a browser tab that +/// outlived `serve` holds some that never do. +#[cfg(unix)] +const ORPHAN_EXIT_DEADLINE: std::time::Duration = std::time::Duration::from_secs(10); + // Graceful shutdown signal #[cfg(unix)] -async fn shutdown_signal() { +async fn shutdown_signal(orphaned: CancellationToken) { use tokio::signal::unix::{signal, SignalKind}; let mut sigint = signal(SignalKind::interrupt()).expect("failed to install SIGINT handler"); @@ -19,12 +36,77 @@ async fn shutdown_signal() { tokio::select! { _ = sigint.recv() => {}, _ = sigterm.recv() => {}, + _ = orphaned.cancelled() => {}, } } #[cfg(not(unix))] -async fn shutdown_signal() { - let _ = tokio::signal::ctrl_c().await; +async fn shutdown_signal(orphaned: CancellationToken) { + tokio::select! { + _ = tokio::signal::ctrl_c() => {}, + _ = orphaned.cancelled() => {}, + } +} + +/// Resolve once process `expected` is no longer this process's parent. +/// +/// Compared with the pid the launcher named, not with 1. An orphan is +/// re-parented to the nearest *subreaper*, and that is init only when there is +/// no other: under `systemd --user`, beneath a container's init shim, or below +/// anything that set `PR_SET_CHILD_SUBREAPER`, `getppid() == 1` is never true +/// and the daemon would outlive its launcher indefinitely. Nor can the daemon +/// read its parent's pid for itself at startup, because a launcher that died +/// before that read would be recorded as the subreaper that inherited it. +#[cfg(unix)] +async fn until_orphaned(expected: u32) { + let mut tick = tokio::time::interval(PARENT_POLL); + loop { + tick.tick().await; + if std::os::unix::process::parent_id() != expected { + return; + } + } +} + +/// Cancel the returned token once the launcher named by `--exit-with-parent` +/// has gone, and make sure this process follows it. +/// +/// This is the second layer of `biorouter serve`'s supervision. The first is +/// `serve` itself stopping its daemon on every way it can exit; this covers +/// the ways it cannot run any code at all — SIGKILL, a crash. Started before +/// anything slow, so a daemon whose launcher is already gone does not finish +/// a startup nobody will use. +#[cfg(unix)] +fn watch_parent(expected: u32) -> CancellationToken { + let orphaned = CancellationToken::new(); + let token = orphaned.clone(); + tokio::spawn(async move { + until_orphaned(expected).await; + // Armed before the graceful shutdown begins, and on a plain OS thread + // rather than a task: what it guards against includes the runtime + // itself never finishing — a drain parked on a connection that will not + // close, or a runtime drop waiting on a blocking task. + std::thread::spawn(|| { + std::thread::sleep(ORPHAN_EXIT_DEADLINE); + std::process::exit(1); + }); + token.cancel(); + tracing::warn!( + "the process that started this daemon (pid {expected}) is gone; shutting down, \ + and exiting regardless in {}s", + ORPHAN_EXIT_DEADLINE.as_secs() + ); + }); + orphaned +} + +#[cfg(not(unix))] +fn watch_parent(expected: u32) -> CancellationToken { + tracing::warn!( + "--exit-with-parent {expected} is not supported on this platform; this daemon will \ + not watch its parent" + ); + CancellationToken::new() } /// Read the launcher's SHA-256 user-action digest off stdin, as one hex line @@ -70,9 +152,15 @@ async fn read_user_action_digest() -> Option<[u8; 32]> { <[u8; 32]>::try_from(bytes.as_slice()).ok() } -pub async fn run() -> Result<()> { +pub async fn run(exit_with_parent: Option) -> Result<()> { crate::logging::setup_logging(Some("biorouterd"))?; + // Opt-in: only `biorouter serve` passes the flag. See `watch_parent`. + let orphaned = match exit_with_parent { + Some(pid) => watch_parent(pid), + None => CancellationToken::new(), + }; + let settings = configuration::Settings::new()?; // Issue #56 Task 30, hardening measure (3): the master privacy switch is @@ -224,7 +312,7 @@ pub async fn run() -> Result<()> { listener, app.into_make_service_with_connect_info::(), ) - .with_graceful_shutdown(shutdown_signal()) + .with_graceful_shutdown(shutdown_signal(orphaned)) .await?; // Take the llama-server sidecar down with us. @@ -247,3 +335,34 @@ pub async fn run() -> Result<()> { info!("server shutdown complete"); Ok(()) } + +/// Only [`until_orphaned`], never [`watch_parent`]: the latter arms a +/// `process::exit`, which would take the test binary down with it. +#[cfg(all(test, unix))] +mod tests { + use super::*; + use std::time::Duration; + + #[tokio::test] + async fn a_daemon_notices_that_its_named_parent_is_not_its_parent() { + // No process has this pid, so it cannot be the parent: this is exactly + // what a daemon sees once `serve` has been killed and it was re-parented. + tokio::time::timeout(Duration::from_secs(5), until_orphaned(u32::MAX)) + .await + .expect("an orphan must notice within a few polls"); + } + + /// Paired with the test above, so a watch that fired unconditionally — + /// which would stop every `serve` daemon half a second after it started — + /// fails here rather than passing both. + #[tokio::test] + async fn a_daemon_whose_parent_is_still_there_keeps_running() { + let parent = std::os::unix::process::parent_id(); + assert!( + tokio::time::timeout(PARENT_POLL * 4, until_orphaned(parent)) + .await + .is_err(), + "a daemon whose launcher is alive must not shut itself down" + ); + } +} diff --git a/crates/biorouter-server/src/main.rs b/crates/biorouter-server/src/main.rs index 5d8581581..8f4a6f863 100644 --- a/crates/biorouter-server/src/main.rs +++ b/crates/biorouter-server/src/main.rs @@ -51,7 +51,15 @@ struct Cli { #[derive(Subcommand)] enum Commands { /// Run the agent server - Agent, + Agent { + /// Stop when process PID is no longer this daemon's parent. + /// + /// `biorouter serve` passes its own pid, so a daemon it started cannot + /// outlive it even when it is killed outright. Opt-in: the desktop app + /// and a hand-started daemon do not pass it. Unix only. + #[arg(long, value_name = "PID")] + exit_with_parent: Option, + }, /// Run the MCP server Mcp { #[arg(value_parser = clap::value_parser!(McpCommand))] @@ -82,8 +90,8 @@ async fn async_main() -> anyhow::Result<()> { let cli = Cli::parse(); match cli.command { - Commands::Agent => { - commands::agent::run().await?; + Commands::Agent { exit_with_parent } => { + commands::agent::run(exit_with_parent).await?; } Commands::Mcp { server } => { logging::setup_logging(Some(&format!("mcp-{}", server.name())))?; diff --git a/crates/biorouter-server/src/routes/shell.rs b/crates/biorouter-server/src/routes/shell.rs index 67dcdecb9..355e824a6 100644 --- a/crates/biorouter-server/src/routes/shell.rs +++ b/crates/biorouter-server/src/routes/shell.rs @@ -2223,7 +2223,8 @@ fn resolve_program(name: &str, search_path: &OsStr) -> PathBuf { // Router. // --------------------------------------------------------------------------- -/// The seventeen `/headless/*` routes. +/// The sixteen `/headless/*` paths — seventeen handlers, since +/// `/headless/settings` answers both `GET` and `POST`. /// /// No handler reads [`AppState`]: this surface is about the machine the daemon /// runs on, not about its sessions. The parameter is kept so the module is @@ -3029,7 +3030,7 @@ mod tests { /// Every route the retired binary served is still served, at the same path. #[test] - fn all_seventeen_routes_are_registered() { + fn all_sixteen_paths_are_registered() { let source = include_str!("shell.rs"); for path in [ "/headless/health", diff --git a/crates/biorouter-server/src/routes/web_ui.rs b/crates/biorouter-server/src/routes/web_ui.rs index f452c422b..110fa984c 100644 --- a/crates/biorouter-server/src/routes/web_ui.rs +++ b/crates/biorouter-server/src/routes/web_ui.rs @@ -34,6 +34,12 @@ //! 4. From then on the application presents `X-Secret-Key` exactly as the //! desktop renderer does, and every API route is guarded exactly as before. //! +//! Step 2 does not consume the token. It is honoured as often as it is +//! presented until the daemon stops, and the cookie's value is the token +//! itself, so single use would need a daemon-minted session and would break a +//! second browser, a bookmark, and a browser that has dropped its cookie. +//! Decision SD-9 in `docs/deployment/serve-decisions.md` has the reasoning. +//! //! **The cookie gates the document and nothing else.** It is not accepted as //! authentication on any API route. Accepting it there would make every API //! route reachable by a credential the browser attaches automatically, which is @@ -408,6 +414,31 @@ mod tests { assert_eq!(res.status(), StatusCode::UNAUTHORIZED); } + /// The exchange does not spend the token (SD-9 in + /// `docs/deployment/serve-decisions.md`): a second browser, a bookmark of a + /// `--token` address, and a browser that has dropped its cookie all present + /// it again. Making it single-use is a decision to revisit there, not a fix + /// to make here. + #[tokio::test] + async fn the_token_is_not_consumed_by_the_exchange() { + let ui = ui_with(Some("tok")); + for attempt in 1..=3 { + let res = index( + State(ui.clone()), + HeaderMap::new(), + Query(IndexQuery { + t: Some("tok".into()), + }), + ) + .await; + assert_eq!( + res.status(), + StatusCode::SEE_OTHER, + "redemption {attempt} must succeed like the first" + ); + } + } + #[tokio::test] async fn a_valid_cookie_is_enough_on_a_later_request() { let ui = ui_with(Some("tok")); diff --git a/docs/cli/command-reference.md b/docs/cli/command-reference.md index baf34bc0e..4ee754135 100644 --- a/docs/cli/command-reference.md +++ b/docs/cli/command-reference.md @@ -738,7 +738,7 @@ Run Biorouter and reach it from a browser. `serve` starts the `biorouterd` daemo - **`-p, --port `**: Port to listen on. Default is `8765` — deliberately not `3000`, which is `biorouterd`'s own default - **`--token `**: Use this access token instead of generating a fresh one - **`--no-token`**: Serve without an access token. Refused for a non-loopback bind, and cannot be combined with `--token` -- **`--web-dir `**: Directory holding the built interface. Located automatically when unset +- **`--web-dir `**: Directory holding the built interface. Takes precedence over `BIOROUTER_SERVE_UI`; whichever of the two is used must contain an `index.html`, or `serve` refuses to start. Located automatically when neither is set - **`--open`**: Open a browser once the server is ready **Usage:** @@ -757,7 +757,7 @@ biorouter serve --host 0.0.0.0 biorouter serve --host 0.0.0.0 --token "$(openssl rand -hex 32)" ``` -The printed URL carries a one-off access token as `?t=`, minted per launch and shown once. Opening it exchanges the token for a session cookie and redirects, so the token leaves the address bar. Use `Ctrl+C` to stop the server. +The printed URL carries an access token as `?t=`, minted per launch and shown once. Opening it exchanges the token for a session cookie and redirects, so the token leaves the address bar; it is not used up, and opens the interface again for anyone who has it until the daemon stops. Use `Ctrl+C` to stop the server, or send `serve` `SIGTERM` (`kill `); either way it stops the daemon it started and frees the port. > **Note.** A browser session cannot change its model or provider, deliberately — run `biorouter configure` to choose them **before** starting `serve`. [Reaching Biorouter from a browser](../deployment/browser-access.md) explains why, and covers the access token, remote access and troubleshooting. diff --git a/docs/configuration/environment-variables.md b/docs/configuration/environment-variables.md index 52da1dcad..a56a4ebf8 100644 --- a/docs/configuration/environment-variables.md +++ b/docs/configuration/environment-variables.md @@ -200,8 +200,8 @@ restart. | Variable | Purpose | Values | Default | |----------|---------|---------|---------| -| `BIOROUTER_SERVE_UI` | Directory holding the built browser interface for the daemon to serve. When it is unset the daemon serves no interface and answers only its API, which is what the desktop app wants — Electron loads the interface itself. `biorouter serve --web-dir ` sets it. | Path to a directory containing an `index.html` | Unset (no interface served) | -| `BIOROUTER_BROWSER_TOKEN` | The access token a browser presents to be handed the interface. It is exchanged on the first request for a session cookie and authenticates nothing else. When it is unset no token is required, which is only correct for a loopback bind — `biorouter serve` refuses to expose an untokened port. `biorouter serve --token ` sets it; otherwise the command generates a new one per launch. | Token string (`biorouter serve` uses 64 hexadecimal characters) | Unset (no token required) | +| `BIOROUTER_SERVE_UI` | Directory holding the built browser interface for the daemon to serve. When it is unset the daemon serves no interface and answers only its API, which is what the desktop app wants — Electron loads the interface itself. `biorouter serve` sets it on the daemon it starts, and reads it as well: without `--web-dir`, which takes precedence, a non-blank value is the directory `serve` uses, and one with no `index.html` stops `serve` with an error rather than being skipped. | Path to a directory containing an `index.html` | Unset (no interface served) | +| `BIOROUTER_BROWSER_TOKEN` | The access token a browser presents to be handed the interface. Opening the address exchanges it for a session cookie — every time it is presented, until the daemon stops; it is not single-use — and it authenticates nothing else. When it is unset no token is required, which is only correct for a loopback bind — `biorouter serve` refuses to expose an untokened port. `biorouter serve --token ` sets it; otherwise the command generates a new one per launch. | Token string (`biorouter serve` uses 64 hexadecimal characters) | Unset (no token required) | > **Warning.** `BIOROUTER_BROWSER_TOKEN` is a credential. Put it in a file readable only by the > service user rather than on a command line, where `ps` shows it to every user on the host. diff --git a/docs/deployment/README.md b/docs/deployment/README.md index 200c019f5..9a6160499 100644 --- a/docs/deployment/README.md +++ b/docs/deployment/README.md @@ -26,7 +26,7 @@ any deployment live in [configuration](../configuration/environment-variables.md | [Headless Linux deployment](headless-linux.md) | Running `biorouter serve` as a long-lived service on a Linux host with no graphical desktop: the CLI-only packages, the systemd unit, migrating secrets onto the host, and network exposure. | | [Reaching a private chat from a script](programmatic-session-access.md) | The `X-Caller-Provider` header: how a monitoring dashboard, a CI job or a shell script reads and follows a **private** conversation over the HTTP API, what the header is not (it is not authentication), and which routes honour it. | | [How browser-served Biorouter is built](serve-architecture.md) | Developer-facing architecture: what the daemon does with a web directory, how a browser is authenticated, and what the retired front door was replaced by. | -| [Decisions behind `biorouter serve`](serve-decisions.md) | The seven decision records governing the serving path — why a browser session cannot change its model, why the bind defaults to loopback, and why the standalone binary was retired. | +| [Decisions behind `biorouter serve`](serve-decisions.md) | The nine decision records governing the serving path — why a browser session cannot change its model, why the bind defaults to loopback, why the standalone binary was retired, and why the launch token is reusable until the daemon stops. | ## Related documentation diff --git a/docs/deployment/browser-access.md b/docs/deployment/browser-access.md index 299acf6b2..44526d471 100644 --- a/docs/deployment/browser-access.md +++ b/docs/deployment/browser-access.md @@ -43,7 +43,8 @@ biorouter serve --open ``` `--open` launches your browser at that address. Without it, copy the URL — including the `?t=` -part, which is what authenticates you. `Ctrl-C` stops the daemon and frees the port. +part, which is what authenticates you. `Ctrl-C` stops the daemon and frees the port; so does +stopping `serve` any other way (see [Stopping it](#stopping-it)). `biorouter headless` is an accepted alias for `biorouter serve` and behaves identically. It is the name the retired standalone binary was known by, kept so older instructions still land in the right @@ -61,7 +62,7 @@ biorouter serve [--host ] [--port ] [--token ] [--no-token] [--web-d | `-p, --port ` | Port to listen on. | `8765` | | `--token ` | Use this access token instead of generating a fresh one. | A new random token each launch | | `--no-token` | Serve with no access token. Refused for a non-loopback bind, and cannot be combined with `--token`. | Off | -| `--web-dir ` | Directory holding the built interface. The directory must contain an `index.html`. | Found automatically — see [When the interface cannot be found](#when-the-interface-cannot-be-found) | +| `--web-dir ` | Directory holding the built interface. It must contain an `index.html`, or `serve` refuses to start. Takes precedence over `BIOROUTER_SERVE_UI`. | `BIOROUTER_SERVE_UI` if set, otherwise found automatically — see [When the interface cannot be found](#when-the-interface-cannot-be-found) | | `--open` | Open a browser once the server is ready. | Off | The default port is `8765` rather than `3000` deliberately: `3000` is `biorouterd`'s own default, so @@ -72,18 +73,45 @@ a `serve` default of `3000` would collide with the daemon the command starts. > visible from both, but a turn running in one is not visible to the other. Having the desktop > application open does not mean `serve` is talking to it. +## Stopping it + +Stop `serve` with `Ctrl-C` in its terminal, or by sending it `SIGTERM` — `kill `, which is +also what `systemctl stop` and most process managers send. Either way `serve` stops the daemon it +started and frees the port: it asks the daemon to shut down, gives it ten seconds to finish, then +kills it. A second `Ctrl-C` skips the wait. + +With no browser tab open the daemon is gone in a fraction of a second. With one open, expect the +full ten seconds and the line `biorouterd did not finish within 10s … killing it.` — the interface +always keeps a request waiting on the daemon, and a graceful shutdown waits for it. That is +expected, not a fault. The one thing a daemon stopped that way skips is shutting down a +llama-server it started for a local model; the next Biorouter launch cleans that up. + +This is more than tidiness. The daemon honours the access token for as long as it runs, so +stopping `serve` is how you revoke the address it printed. + +If `serve` itself is killed outright (`kill -9`) it cannot stop anything, so on macOS and Linux the +daemon watches for that: it notices within a second that its parent has gone and shuts itself +down, taking at most ten seconds more. On Windows there is no such watch — `Ctrl-C` reaches both +processes, but if `biorouter.exe` is ended some other way, from Task Manager for example, end +`biorouterd.exe` as well. + ## The access token A browser cannot send an authentication header on its first request, so the address `serve` prints -carries a one-off credential instead. +carries a credential instead. - **It is minted per launch** — 32 random bytes, printed as 64 hexadecimal characters in the URL's `?t=` parameter, and different every time. It is shown once, in the terminal; there is nowhere else to read it back from. -- **It is spent on the first request.** Opening the URL validates the token, sets an `HttpOnly`, - `SameSite=Strict` session cookie named `biorouter_session`, and redirects to `/`. The token then - disappears from the address bar, so it is not left in browser history or in the `Referer` of - anything the page later loads. +- **Opening the address exchanges it for a cookie.** The daemon validates the token, sets an + `HttpOnly`, `SameSite=Strict` session cookie named `biorouter_session`, and redirects to `/`. The + token then disappears from the address bar, so it is not left in browser history or in the + `Referer` of anything the page later loads. +- **It is not used up.** The exchange works every time the token is presented, from any browser, + until the daemon stops — so a second browser, a colleague, or the same browser after it has + discarded the cookie can all open the same address. Anyone holding the address can do the same, + and stopping `serve` is how you revoke it ([decision SD-9](serve-decisions.md#sd-9--the-launch-token-works-until-the-daemon-stops-it-is-not-single-use) + records why it is not single-use). - **The cookie gates the document and nothing else.** It is not accepted as authentication on any API route. From the moment the page loads, the interface presents the daemon's secret key as a header, exactly as the desktop application does. @@ -196,8 +224,11 @@ differs: ## Troubleshooting -**`port 8765 on 127.0.0.1 is already in use.`** Something else holds the port — often an earlier -`serve` that did not exit. Choose another with `--port `, or stop the other process. +**`port 8765 on 127.0.0.1 is already in use.`** Something else holds the port. Choose another with +`--port `, or stop the other process — on macOS and Linux, `lsof -nP -iTCP:8765 -sTCP:LISTEN` +names it. A `biorouterd agent` holding it is usually left over from a `serve` of version 1.90.3 or +earlier, which left its daemon running when it was stopped by anything other than `Ctrl-C` in its +own terminal. **`biorouterd exited during startup`, or it never starts listening.** The daemon is started as a child process and watched while it comes up; if it dies, `serve` reports that rather than pretending @@ -225,24 +256,36 @@ serving machine first. ### When the interface cannot be found -`serve` looks for the built interface in a fixed order, and names every location it tried when it +A directory you name is used as named, or not at all. `--web-dir ` wins when it is given; +otherwise `BIOROUTER_SERVE_UI` does, if it is set to anything but blank. Whichever it is must +contain an `index.html`. If it does not, `serve` stops with the same error for both, naming where +the path came from: + +```text +no web interface at /srv/biorouter/wbe (expected an index.html there; the path came from BIOROUTER_SERVE_UI) +``` + +It does not move on to a bundle it found somewhere else. Through version 1.90.3 a +`BIOROUTER_SERVE_UI` with no `index.html` was skipped without a word, and `serve` served whichever +bundle the search below turned up next — one you had not chosen. + +With neither set, `serve` searches in a fixed order, and names every location it tried when it finds none: -1. `BIOROUTER_SERVE_UI`, or `--web-dir`, if either is set. -2. `web/` beside the installed binaries (a packaged application). -3. `ui/desktop/src/web/` in a development tree. -4. `web/` beside the application a Windows install was made from. -5. `/usr/share/biorouter/web` (where the Linux packages put it). +1. `web/` beside the installed binaries (a packaged application). +2. `ui/desktop/src/web/` in a development tree. +3. `web/` beside the application a Windows install was made from. +4. `/usr/share/biorouter/web` (where the Linux packages put it). "Beside" means beside the **real** binary. On macOS and Linux the CLI is installed on `PATH` as a -symlink (`~/.local/bin/biorouter` → the application bundle), and steps 2 and 3 follow that link +symlink (`~/.local/bin/biorouter` → the application bundle), and steps 1 and 2 follow that link before deriving anything from it — otherwise they would name directories in your home folder, which is what they did in v1.89.5 through v1.90.2. Windows has no symlink to follow. `biorouter setup-path` — and the in-app "Biorouter CLI Update" card, which runs it — *copies* `biorouter.exe` into `%LOCALAPPDATA%\Biorouter\bin`, leaving `biorouterd.exe` and the interface behind inside the application. So the copy also records the -folder it came from, in a small file named `.biorouter-origin` beside itself, and step 4 reads that +folder it came from, in a small file named `.biorouter-origin` beside itself, and step 3 reads that back. The same record is how `serve` and `biorouter apps` find `biorouterd.exe`. It is rewritten every time you install, so updating the application and running `biorouter setup-path` again is what points the CLI at the new one. diff --git a/docs/deployment/headless-linux.md b/docs/deployment/headless-linux.md index 6dab3d2ef..f98df7a97 100644 --- a/docs/deployment/headless-linux.md +++ b/docs/deployment/headless-linux.md @@ -170,8 +170,10 @@ systemctl status biorouter.service The URL to hand to users is `http://:8765/?t=`. It stays valid until you change the token. -> **Note.** `biorouter serve` starts `biorouterd` as a child process and stops it on the way out, so -> systemd supervises one unit and not two. There is no separate daemon unit to enable. +> **Note.** `biorouter serve` starts `biorouterd` as a child process and stops it whenever it stops +> itself — `systemctl stop` sends `serve` SIGTERM, and `serve` gives the daemon ten seconds to +> finish before killing it — so systemd supervises one unit and not two. There is no separate +> daemon unit to enable. See [Stopping it](browser-access.md#stopping-it). ## Decide who can reach the port diff --git a/docs/deployment/serve-architecture.md b/docs/deployment/serve-architecture.md index 28c0cde6e..a8be9f870 100644 --- a/docs/deployment/serve-architecture.md +++ b/docs/deployment/serve-architecture.md @@ -50,7 +50,7 @@ retry-with-backoff meant they failed silently rather than reporting it. whatever arrived. Now the browser presents the same credential every other client presents, and `check_token` is the only thing that inspects it. -**The interface's own endpoints are authenticated.** The sixteen `/headless/*` endpoints — the +**The interface's own endpoints are authenticated.** The sixteen `/headless/*` paths — the filesystem browser, settings, extension installation, skill extraction — were previously served by a router carrying exactly one layer, `TraceLayer`. Moved into the daemon they sit behind the same middleware as everything else. @@ -161,6 +161,53 @@ no wildcard, and it holds for every address the interface is reached at, includi daemon could not have enumerated because it bound `0.0.0.0`. Both are compared whole, so a `Host` of `evil.com.attacker.net` does not admit an `Origin` of `http://evil.com`. +## How `serve` starts and stops the daemon + +`biorouter serve` spawns `biorouterd agent` rather than running the server itself (SD-7), so it +is a supervisor, and the half of supervision that matters is stopping. The daemon, not `serve`, +holds the port, answers the browser token and serves the shell that carries its secret — so for +as long as it runs, the URL `serve` printed works. The daemon has to stop whenever `serve` does, +and two layers see to that: + +| How `serve` ends | What stops the daemon | +|---|---| +| `Ctrl-C`, or `SIGTERM` (`kill `, `systemctl stop`) | `serve` sends the daemon `SIGTERM`, waits up to ten seconds, then kills it, and reaps it before exiting. A second request skips the wait. | +| The daemon exits, or never becomes ready | The same path, with nothing or less to stop. | +| `serve` is killed outright (`SIGKILL`), or crashes | On Unix the daemon was started with `--exit-with-parent `. It sees within half a second that its parent has changed and shuts itself down, exiting regardless ten seconds later. | + +The listeners for the first row are installed **before** the daemon is spawned. Installing one +replaces the default action, which for `SIGTERM` was to end `serve` on the spot — so a signal +that arrives during the readiness wait is held until it is read, not lost with the daemon still +running. + +Three details are deliberate: + +- **The ten seconds exist because a graceful shutdown waits for open connections to finish**, + and a browser tab left open holds some that never do. The daemon applies the same figure to + itself when it is orphaned, because then nobody is left to escalate. Measured with one + request in flight — the renderer's catalog long poll, which an open tab always has parked, for + up to 25 s — `serve` exited at 10.05 s by killing the daemon; with none, in under 0.1 s. A + daemon killed that way skips its own cleanup, so a llama-server sidecar it started is left for + the next launch's pidfile reaper (`llamacpp_sidecar::reap_orphans`). +- **The parent check compares `getppid()` with the pid `serve` named, not with 1.** An orphan is + re-parented to the nearest *subreaper* — `systemd --user` on most Linux desktops, a + container's init shim — and to pid 1 only when there is none, so `getppid() == 1` would + never fire there. And the pid is passed in rather than read by the daemon at startup, because + a `serve` that died before that read would be recorded as the subreaper that inherited it. +- **The flag is opt-in.** The desktop application starts `biorouterd agent` without it, and so + does anyone running the daemon by hand. + +Windows has neither `SIGTERM` nor the parent check. A console `Ctrl-C` reaches every process +attached to the console, so both stop; `biorouter.exe` ended any other way leaves the daemon +running. + +Until 2026-09 neither layer existed, although a comment in `serve` said the first did. The +daemon's `Child` had been moved into the task waiting on it, so the `Ctrl-C` handler held no +handle to kill it with; only a terminal's `Ctrl-C`, which signals the whole foreground process +group, ever reached the daemon. `kill ` from anywhere else left it running with the +port, the token and the secret. `crates/biorouter-cli/tests/serve_lifecycle.rs` stops `serve` by +pid with each signal and asserts the daemon is gone and the port is closed. + ## What is deleted The `biorouter-headless` crate goes entirely (SD-6). Of its two thousand lines, the parts with no @@ -172,8 +219,9 @@ successor are: stylesheet rewrites, and the routes registered to serve the rewritten copies (SD-4); - the cloud-metadata probes performed on every start. -What moves rather than dies is the sixteen `/headless/*` handlers, which become a route module in -the daemon, and the resolution of where the web directory lives. +What moves rather than dies is the `/headless/*` surface — sixteen paths, seventeen handlers, +since `/headless/settings` answers both `GET` and `POST` — which becomes a route module in the +daemon, and the resolution of where the web directory lives. ## Where the bundle comes from @@ -187,9 +235,12 @@ package script so every platform's packaging can call it. > root base, from `vite.renderer.config.mts` directly. Reusing the packaged Electron bundle is not > a shortcut; it is a different artifact. -The resolver looks for the directory in a fixed order — an explicit flag or environment variable, -then a location relative to the executable, then a system-wide path for the Linux packages. When -it finds none, the error names every path it tried. +A directory named explicitly — `--web-dir`, or else `BIOROUTER_SERVE_UI` — is used as given or +refused with the same error, never skipped. The variable used to be only the first candidate of +the search, so one naming an empty directory was passed over and `serve` served whatever the +search found next, while the same path given as `--web-dir` was fatal. With neither set, the +resolver looks in a fixed order — locations relative to the executable, then a system-wide path +for the Linux packages — and when it finds none, the error names every path it tried. ## Related documentation diff --git a/docs/deployment/serve-decisions.md b/docs/deployment/serve-decisions.md index f413d2bef..24b74e3a5 100644 --- a/docs/deployment/serve-decisions.md +++ b/docs/deployment/serve-decisions.md @@ -1,9 +1,10 @@ # Decisions behind `biorouter serve` > **What this is.** The decision records governing browser-served Biorouter — why the daemon -> serves the interface itself, why a browser session cannot change its model, and why the -> standalone `biorouter-headless` binary was retired. Each record states the ruling, the -> alternatives it displaced, and the consequence a future change would have to accept. +> serves the interface itself, why a browser session cannot change its model, why the +> standalone `biorouter-headless` binary was retired, and how long the launch token stays good +> for. Each record states the ruling, the alternatives it displaced, and the consequence a +> future change would have to accept. > **Status:** Current. > **Audience:** developers working on the daemon, the CLI, or release packaging; agents making > changes anywhere near the serving path. @@ -27,10 +28,15 @@ keeps its number and says what replaced it. ## SD-1 — A browser session cannot change its model or provider, and that is the point -**Ruling.** `POST /config/provider` continues to refuse a request that carries no proof a human -made it. Browser-served Biorouter installs no such proof. A browser session therefore runs -whatever provider and model the machine was already configured with, and the model picker is -inert. +**Ruling.** `POST /config/set_provider` (`set_config_provider` in +`crates/biorouter-server/src/routes/config_management.rs`) continues to refuse a request that +carries no proof a human made it. Browser-served Biorouter installs no such proof. A browser +session therefore runs whatever provider and model the machine was already configured with, and +the model picker is inert. + +> **Note.** Until 2026-09 this record named the route `POST /config/provider`. No such route +> exists, so an audit of SD-1 that followed the old text measured a 404 and could read it as +> "no gate". The gate is on `/config/set_provider`, which answers a browser session with 409. **Why.** This looks like a missing feature and is actually the privacy boundary holding. The privacy tier system (issue #56) classifies a conversation by the sensitivity of what it has @@ -193,6 +199,14 @@ proof-of-user digest. Under SD-1 that is the intended configuration, not a limit means the daemon a `serve` session talks to is deliberately less capable than the one the desktop application starts, and anything that assumes otherwise is wrong. +**And the child must never outlive the parent.** The daemon, not `serve`, holds the port, +answers the browser token and serves the shell carrying its secret, so a `serve` that exits +without stopping it has revoked nothing. `serve` therefore stops the daemon on every path it can +run code on, and on Unix starts it with `--exit-with-parent` so that it stops itself on the paths +`serve` cannot — see [how `serve` starts and stops the daemon](serve-architecture.md#how-serve-starts-and-stops-the-daemon). +A comment in `serve` claimed the first half from the start; until 2026-09 neither half was true, +and only a terminal's `Ctrl-C`, which signals the whole process group, ever reached the daemon. + --- ## SD-8 — A control that can never work here says so, rather than failing on click @@ -227,6 +241,49 @@ can never half-believe a person is reachable. --- +## SD-9 — The launch token works until the daemon stops; it is not single-use + +**Ruling.** `GET /?t=` exchanges the token for the session cookie every time it is +presented, not only the first time. The exchange takes the token out of the address bar; it does +not consume it. The token stops working when the daemon stops — which SD-7 ties to `serve` +stopping — or, for one passed with `--token`, when a different one is passed. + +**Why.** The token was first described as "spent on the first request", and that was never true: +the 2026-09-10 QA run redeemed one token four more times after the first and got a 303 each time. +The choice was then whether to make the description true or correct it, and single use cannot be +had without breaking what the product promises: + +- **It would be a different mechanism, not an added check.** The session cookie's value *is* the + token — the daemon compares both against one string — so a "spent" token would still open the + shell for anyone who set the cookie by hand. Real single use needs a cookie the daemon mints + and remembers: a session table, emptied by every restart. +- **The supported uses need a second redemption.** A second browser, or a colleague on a shared + host, where everyone who opens the address is the same user; the same browser after it has + dropped its session cookie, which carries no expiry and may be discarded when the browser + closes; and a bookmark of an address fixed with `--token`, which + [browser access](browser-access.md) offers precisely so that the address survives restarts. +- **Things other than people fetch links.** A browser prefetching a pasted address, or a chat + client unfurling it, would spend a single-use link before anyone clicked it. + +What the exchange is for is keeping the token out of browser history and out of the `Referer` of +everything the page loads afterwards, and the redirect does that whether or not the token is +consumed. + +**Displaced alternatives.** + +- *Single use, with a session cookie minted by the daemon.* Rejected for the reasons above. +- *Keep the word "spent".* Rejected. In a section about security it reads as single use, and an + operator who believes a leaked address stopped working after its first use has the wrong + picture of their exposure. + +**Consequence to accept.** The address `serve` prints is a bearer credential for as long as the +daemon runs. Revoking it means stopping `serve` — which is why SD-7 requires that the daemon never +outlive it — and, for an address fixed with `--token`, choosing a new token. Treat it like the +password it is. `the_token_is_not_consumed_by_the_exchange` in `routes::web_ui` pins the +behaviour, so changing it means revisiting this record, not making a quiet fix. + +--- + ## Related documentation - [Architecture of the serving path](serve-architecture.md) — how the decisions above are built. diff --git a/scripts/smoke-test-release-artifacts.sh b/scripts/smoke-test-release-artifacts.sh index e10119401..08eee2e9d 100755 --- a/scripts/smoke-test-release-artifacts.sh +++ b/scripts/smoke-test-release-artifacts.sh @@ -169,7 +169,9 @@ smoke_serve() { code=$(curl -s -o /dev/null -w "%{http_code}" http://127.0.0.1:18080/) test "$code" = "401" - # The token is exchanged for a session cookie, once. + # The token is exchanged for a session cookie. It is not consumed + # (SD-9 in docs/deployment/serve-decisions.md): the readiness loop above + # has already redeemed it. curl -s -o /dev/null -D /tmp/h "http://127.0.0.1:18080/?t=smoketoken" grep -qi "^HTTP/1.1 303" /tmp/h grep -qi "set-cookie: biorouter_session=" /tmp/h @@ -201,10 +203,17 @@ smoke_serve() { curl -fsS -H "X-Secret-Key: $secret" http://127.0.0.1:18080/headless/health >/tmp/health.json grep -q "\"status\":\"ok\"" /tmp/health.json + # Stopping serve by pid stops its daemon: serve reaps it before exiting, + # so the port is closed once wait returns. It used to stay bound by an + # orphaned daemon that still honoured the token. kill "$pid" wait "$pid" || true + if curl -s -o /dev/null --max-time 5 http://127.0.0.1:18080/status; then + echo "the daemon outlived serve" >&2 + exit 1 + fi ' - log "biorouter serve: token exchange, gated shell, root-base bundle, and authenticated endpoints passed" + log "biorouter serve: token exchange, gated shell, root-base bundle, authenticated endpoints, and stopping with its daemon passed" } case "$TARGET" in