diff --git a/crates/didbot-serve/src/bin/didbot-pds.rs b/crates/didbot-serve/src/bin/didbot-pds.rs index 65486a4f..40251187 100644 --- a/crates/didbot-serve/src/bin/didbot-pds.rs +++ b/crates/didbot-serve/src/bin/didbot-pds.rs @@ -413,8 +413,10 @@ usage: didbot-pds [options] says which. Unset means no file latch, only the socket. --estop-socket bind the e-stop admin socket here (default: under - $XDG_RUNTIME_DIR, or a temp directory without one). - `none` turns the socket off entirely. + $XDG_RUNTIME_DIR, or a temp directory without one). A path + that cannot be bound refuses startup, because the socket is + this run's only wired stop; `none` turns the socket off + entirely and is how a deployment says it wants none. --close-disclosure close one or more disclosure routes: a comma-separated list drawn from `list-agents`, `list-agent-ledgers`, @@ -1659,6 +1661,14 @@ async fn run(mut args: Args) -> Result<(), String> { } }); + // A socket that was asked for and could not be bound refuses the run. + // The alternative is a server that comes up with the only wired stop + // absent, and says so once in a log line that reads as a warning: the + // operator finds out when `PAUSE` answers "connection refused" during the + // incident it was needed for. `--estop-socket none` is how a deployment + // says it wants no socket, so a path that was named and did not bind is a + // configuration or ownership problem, not a reason to serve without a + // stop. let estop_admin_handle = match &args.estop_socket { Some(path) => { estop_admin::remove_if_stale(path); @@ -1670,8 +1680,12 @@ async fn run(mut args: Args) -> Result<(), String> { ) { Ok(handle) => Some(handle), Err(err) => { - warn!(path = %path.display(), %err, "could not bind the e-stop admin socket"); - None + return Err(format!( + "--estop-socket {}: {err}\n\nthis is the only e-stop this run has \ + wired, so it refuses to serve without it. Fix the path, or pass \ + `--estop-socket none` to run with no socket.", + path.display() + )) } } } diff --git a/crates/didbot-serve/src/estop_admin.rs b/crates/didbot-serve/src/estop_admin.rs index f46536d3..01269ae9 100644 --- a/crates/didbot-serve/src/estop_admin.rs +++ b/crates/didbot-serve/src/estop_admin.rs @@ -134,6 +134,10 @@ impl EstopAdmin { /// finds this deployment, and silently unlinking one under a process that /// still holds it would be its own outage. /// +/// `didbot-pds` turns that failure into a refusal to start, because this +/// socket is the only e-stop a deployment reliably has wired; see +/// `plan/e-stop.md`. +/// /// # Where this binds, and who else could reach it /// /// No command here takes a credential — see this module's own doc — which diff --git a/crates/didbot-serve/tests/startup_refusal.rs b/crates/didbot-serve/tests/startup_refusal.rs index b68f1188..c657a3c1 100644 --- a/crates/didbot-serve/tests/startup_refusal.rs +++ b/crates/didbot-serve/tests/startup_refusal.rs @@ -16,6 +16,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; +use std::time::{Duration, Instant}; /// A directory this test owns, removed on drop. /// @@ -134,3 +135,88 @@ fn a_writable_hosted_zone_with_no_record_target_is_refused() { data.join(didbot_pds::lock::LOCK_FILE).display() ); } + +/// A named e-stop socket that cannot be bound refuses the run rather than +/// serving without it. +/// +/// The failure this reproduces is an instance rebuilt from a file-level copy +/// that materialised `estop.sock` as an ordinary file. `remove_if_stale` +/// deliberately leaves anything that is not a socket in place, so `bind` +/// fails with `EADDRINUSE`. Before this refusal the server logged one warning +/// and served normally, and the operator found out when `PAUSE` answered +/// "connection refused" during the incident it was needed for. +/// +/// A regression here does not fail fast: the server would come up and serve, +/// so the child is waited on with a deadline and killed rather than joined. +#[test] +fn an_e_stop_socket_that_cannot_be_bound_refuses_the_run() { + let dir = TempDir::new("estop-bind"); + let data = dir.path().join("data"); + // Not a socket, so `remove_if_stale` leaves it and the bind fails. + let socket = dir.path().join("estop.sock"); + std::fs::write(&socket, b"what a file-level restore left here").expect("plant the file"); + + // A real port, not `--port 0`: the run has to reach the socket bind, and + // the steps before it build this server's own account from the zone and + // the port it was handed. + let reserved = + std::net::TcpListener::bind(("127.0.0.1", 0)).expect("an ephemeral port to spare"); + let port = reserved.local_addr().expect("the reserved address").port(); + drop(reserved); + + let said = refused_within( + Command::new(env!("CARGO_BIN_EXE_didbot-pds")) + .args(["--port", &port.to_string()]) + .args(["--zone", "agents.localhost"]) + .arg("--estop-socket") + .arg(&socket) + .arg("--data") + .arg(&data) + .env("NO_COLOR", "1"), + Duration::from_secs(30), + ); + + assert!( + said.contains("--estop-socket"), + "the refusal did not name the socket as the problem:\n{said}" + ); + assert!( + said.contains("--estop-socket none"), + "the refusal did not say how to run without a socket:\n{said}" + ); + assert!( + socket.exists(), + "the refused run unlinked the path it could not bind" + ); +} + +/// Runs `command`, expecting it to exit non-zero inside `within`, and returns +/// everything it said. Kills it and fails if it is still running, so a +/// regression that goes back to serving is a failure rather than a hang. +fn refused_within(command: &mut Command, within: Duration) -> String { + use std::process::Stdio; + + let mut child = command + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("didbot-pds runs"); + let deadline = Instant::now() + within; + let status = loop { + match child.try_wait().expect("wait on didbot-pds") { + Some(status) => break status, + None if Instant::now() < deadline => std::thread::sleep(Duration::from_millis(50)), + None => { + let _ = child.kill(); + let _ = child.wait(); + panic!("didbot-pds was still running after {within:?}, so it served anyway"); + } + } + }; + let output = child.wait_with_output().expect("collect what it said"); + assert!( + !status.success(), + "didbot-pds exited cleanly rather than refusing: {status:?}" + ); + String::from_utf8_lossy(&output.stderr).into_owned() + &String::from_utf8_lossy(&output.stdout) +} diff --git a/plan/e-stop.md b/plan/e-stop.md index 23628854..81902723 100644 --- a/plan/e-stop.md +++ b/plan/e-stop.md @@ -79,6 +79,15 @@ control that is.** socket (`didbot-serve::estop_admin`) and a file whose presence is the latch (`Estop`'s own file check), so it works from a shell with nothing else running. +- [x] **A socket that was asked for and did not bind refuses the run.** The + availability trade was settled the other way than a server usually + settles it, because `--estop-socket none` already exists: a deployment + that wants no socket says so, and a path that was named and failed to + bind is a configuration or ownership problem. The alternative is a + server that serves with the only wired stop absent, having said so once + in a line that reads as a warning, and an operator who finds out when + `PAUSE` answers "connection refused" during the incident. See + `startup_refusal.rs`. - [x] **A latch that cannot be read is engaged.** An unreadable file (bad permissions) and a readable-but-corrupt one (garbage contents) both read as `Mode::Revoke` — the strongest setting, not merely "engaged" —