brokerd: runbook pointers for startup failures, no thread panics, args_os

M3a review findings 3, 5 (the cast), 6, 7 (brokerd), 11. A config, directory
or socket failure at start now ends with docs/runbook.md#brokerd-start-failed,
and losing a listener with #brokerd-listener-lost; both entries are new.
Threads start through thread::Builder, so a refused thread is reported
instead of silently killing a listener; an aborted connection no longer stops
the daemon. brokerd reads args_os and keeps the config path as a path. The
"requester went away" result is recorded at the time it happens.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

crates/brokerd/src/args.rs

@@ -6,7 +6,7 @@
 
 use serde::{Deserialize, Serialize};
 use std::fmt;
-
+/// Maximum length, in bytes, of a path.
 /// Maximum length, in bytes, of a path or of a URL.
 pub const MAX_PATH: usize = 4096;
 /// Maximum length, in bytes, of a URL.

crates/brokerd/src/broker.rs

@@ -301,7 +301,7 @@ fn pending(
                         ToolResponse::Failed {
                             message: GONE.to_string(),
                         },
-                        now,
+                        Timestamp::now(), // now, not when it was decided: records keep time order
                     );
                     return None; // 5. the peer left at the last look; nothing ran
                 }

crates/brokerd/src/grants.rs

@@ -91,8 +91,7 @@ pub fn load(dir: &Path) -> Result<GrantSet, Vec<GrantProblem>> {
         match entry {
             Ok(entry) => names.push(entry.file_name().to_string_lossy().to_string()),
             Err(error) => {
-                // An entry that cannot be read is the same problem as the directory; keep going.
-                names.push(String::new());
+                // An entry that cannot be read is the same problem as the directory.
                 return Err(vec![GrantProblem {
                     file: dir.to_string_lossy().to_string(),
                     line: None,
@@ -198,7 +197,8 @@ fn span_line(text: &str, error: &toml::de::Error) -> u64 {
                 Some(before) => before,
                 None => text,
             };
-            before.bytes().filter(|&b| b == b'\n').count() as u64 + 1
+            let newlines = before.bytes().filter(|&b| b == b'\n').count();
+            u64::try_from(newlines).map_or(u64::MAX, |n| n.saturating_add(1))
         }
         None => 1,
     }

crates/brokerd/src/main.rs

@@ -1,5 +1,6 @@
 //! `brokerd`: the broker role. `serve` takes the audit lock, binds both sockets, and serves.
 
+use std::ffi::OsString;
 use std::path::PathBuf;
 use std::process::ExitCode;
 use std::sync::Arc;
@@ -10,10 +11,15 @@ use brokerd::runner::Refusing;
 use brokerd::serve::{self, ServeError};
 
 const USAGE: &str = "usage: brokerd serve --config <path> [--accept-break]";
+/// Where the owner looks when brokerd cannot start: its config, a directory or a socket.
+const START_FAILED: &str = "see docs/runbook.md#brokerd-start-failed";
+/// Where the owner looks when brokerd stops serving after it started.
+const LISTENER_LOST: &str = "see docs/runbook.md#brokerd-listener-lost";
 
 fn main() -> ExitCode {
     // The first argument must be `serve`.
-    let mut args = std::env::args().skip(1);
+    // `args_os`: a path need not be UTF-8, and `args` would panic on one that is not.
+    let mut args = std::env::args_os().skip(1);
     match args.next() {
         Some(command) if command == "serve" => {}
         _ => return usage(),
@@ -29,7 +35,7 @@ fn main() -> ExitCode {
     let cfg = match Config::load(&config_path) {
         Ok(cfg) => cfg,
         Err(e) => {
-            eprintln!("brokerd: {e}");
+            eprintln!("brokerd: {e}\n{START_FAILED}");
             return ExitCode::from(1);
         }
     };
@@ -49,10 +55,15 @@ fn main() -> ExitCode {
             eprintln!("brokerd: {e}");
             return ExitCode::from(2);
         }
-        Err(e) => {
+        // The audit errors carry their own entry; the rest are this one.
+        Err(ServeError::Audit(e)) => {
             eprintln!("brokerd: {e}");
             return ExitCode::from(1);
         }
+        Err(e) => {
+            eprintln!("brokerd: {e}\n{START_FAILED}");
+            return ExitCode::from(1);
+        }
     };
 
     if started.recovered {
@@ -73,7 +84,7 @@ fn main() -> ExitCode {
     match started.run() {
         Ok(()) => ExitCode::SUCCESS,
         Err(e) => {
-            eprintln!("brokerd: {e}");
+            eprintln!("brokerd: stopped serving: {e}\n{LISTENER_LOST}");
             ExitCode::from(1)
         }
     }
@@ -86,8 +97,10 @@ fn usage() -> ExitCode {
 }
 
 /// The `--config` and `--accept-break` flags after `serve`, or `Err(())` for a bad list.
-fn parse_serve<I: Iterator<Item = String>>(mut args: I) -> Result<(PathBuf, bool), ()> {
-    let mut config: Option<String> = None;
+/// Flags are compared as `OsStr`, so one that is not UTF-8 is simply unknown; the path is kept as
+/// given.
+fn parse_serve<I: Iterator<Item = OsString>>(mut args: I) -> Result<(PathBuf, bool), ()> {
+    let mut config: Option<OsString> = None;
     let mut accept_break = false;
     loop {
         match args.next() {
@@ -97,24 +110,22 @@ fn parse_serve<I: Iterator<Item = String>>(mut args: I) -> Result<(PathBuf, bool
                     None => Err(()),
                 };
             }
-            Some(arg) => match arg.as_str() {
-                "--config" => match args.next() {
-                    Some(path) => {
-                        if config.is_some() {
-                            return Err(());
-                        }
-                        config = Some(path);
-                    }
-                    None => return Err(()),
-                },
-                "--accept-break" => {
-                    if accept_break {
+            Some(arg) if arg == "--config" => match args.next() {
+                Some(path) => {
+                    if config.is_some() {
                         return Err(());
                     }
-                    accept_break = true;
+                    config = Some(path);
                 }
-                _ => return Err(()),
+                None => return Err(()),
             },
+            Some(arg) if arg == "--accept-break" => {
+                if accept_break {
+                    return Err(());
+                }
+                accept_break = true;
+            }
+            Some(_) => return Err(()),
         }
     }
 }

crates/brokerd/src/serve.rs

@@ -3,7 +3,7 @@
 use std::fs::{DirBuilder, Permissions};
 use std::io;
 use std::os::unix::fs::{DirBuilderExt, PermissionsExt};
-use std::os::unix::net::UnixListener;
+use std::os::unix::net::{UnixListener, UnixStream};
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 use std::sync::mpsc;
@@ -148,21 +148,24 @@ impl Started {
         } = self;
 
         // The expiry thread: once a second, expire the approvals whose time has run out.
+        // `Builder` rather than `spawn`, which panics when the system refuses a thread.
         {
             let broker = Arc::clone(&broker);
-            std::thread::spawn(move || {
-                loop {
-                    std::thread::sleep(Duration::from_secs(1));
-                    admin::expire_due(&broker, Timestamp::now());
-                }
-            });
+            std::thread::Builder::new()
+                .name("expiry".to_string())
+                .spawn(move || {
+                    loop {
+                        std::thread::sleep(Duration::from_secs(1));
+                        admin::expire_due(&broker, Timestamp::now());
+                    }
+                })?;
         }
 
         // One accept thread per socket, each with its own clone of the broker.
         let (tx, rx) = mpsc::channel::<io::Error>();
 
-        accept_loop(tools, Arc::clone(&broker), tx.clone(), Which::Tools);
-        accept_loop(admin, Arc::clone(&broker), tx, Which::Admin);
+        accept_loop(tools, Arc::clone(&broker), tx.clone(), Which::Tools)?;
+        accept_loop(admin, Arc::clone(&broker), tx, Which::Admin)?;
 
         // The first listener error stops the daemon.
         match rx.recv() {
@@ -172,28 +175,56 @@ impl Started {
     }
 }
 
-/// Accept streams from one listener forever, spawning a handler thread per stream.
+/// Accept streams from one listener forever, starting a handler thread per stream. An error that
+/// only concerns one connection (the peer aborted it, a signal interrupted `accept`) is skipped;
+/// any other stops the daemon through `tx`.
 fn accept_loop(
     listener: UnixListener,
     broker: Arc<Broker>,
     tx: mpsc::Sender<io::Error>,
     which: Which,
-) {
-    std::thread::spawn(move || {
-        for stream in listener.incoming() {
-            match stream {
-                Ok(stream) => {
-                    let broker = Arc::clone(&broker);
-                    std::thread::spawn(move || match which {
-                        Which::Tools => broker::handle(stream, &broker),
-                        Which::Admin => admin::handle(stream, &broker),
-                    });
-                }
-                Err(e) => {
-                    // Once `run` has returned, nobody receives; dropping this is correct.
-                    let _ = tx.send(e);
+) -> io::Result<()> {
+    let name = match which {
+        Which::Tools => "accept-broker",
+        Which::Admin => "accept-admin",
+    };
+    std::thread::Builder::new()
+        .name(name.to_string())
+        .spawn(move || {
+            for stream in listener.incoming() {
+                match stream {
+                    Ok(stream) => serve_one(stream, &broker, which),
+                    Err(e) if one_connection_only(&e) => {}
+                    Err(e) => {
+                        // Once `run` has returned, nobody receives; dropping this is correct.
+                        let _ = tx.send(e);
+                        return;
+                    }
                 }
             }
-        }
+        })?;
+    Ok(())
+}
+
+/// Hand one stream to its own thread. If the system refuses the thread, the stream is closed, which
+/// the peer sees as the broker being unavailable, and the refusal is printed.
+fn serve_one(stream: UnixStream, broker: &Arc<Broker>, which: Which) {
+    let for_thread = Arc::clone(broker);
+    let started = std::thread::Builder::new().spawn(move || match which {
+        Which::Tools => broker::handle(stream, &for_thread),
+        Which::Admin => admin::handle(stream, &for_thread),
     });
+    if let Err(e) = started {
+        broker.log(&format!(
+            "brokerd: cannot start a thread for a connection, so it was closed: {e}\nsee docs/runbook.md#brokerd-listener-lost"
+        ));
+    }
+}
+
+/// Errors from `accept` that end one connection, not the listener.
+fn one_connection_only(e: &io::Error) -> bool {
+    matches!(
+        e.kind(),
+        io::ErrorKind::ConnectionAborted | io::ErrorKind::Interrupted | io::ErrorKind::WouldBlock
+    )
 }