From a92e584dc6ee1af948a3accc018933be8e81bbea Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Mon, 20 Jul 2026 21:01:12 -0700 Subject: [PATCH 01/10] chore: add 'tests' directory to excluded files in Cargo.toml --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 959183d..56d6f7b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ authors = ["Christopher Sardegna "] categories = ["command-line-interface", "command-line-utilities"] description = "A fleet-view supervisor for arbitrary shell commands." edition = "2024" -exclude = [".github", "docs", ".gitignore", ".gitattributes"] +exclude = [".github", "docs", "tests", ".gitignore", ".gitattributes"] keywords = ["cli", "tui", "pty", "supervisor", "process"] license = "GPL-3.0-or-later" name = "fleetcom" From 3f388ae858e46c7b9f053207f845a248554d6d0d Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Mon, 20 Jul 2026 21:22:51 -0700 Subject: [PATCH 02/10] docs: correct picker Backspace behavior; add Tab section navigation --- docs/commands.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/commands.md b/docs/commands.md index f6649ae..98a0b39 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -21,6 +21,7 @@ | Key | Command | | -- | -- | | `↑` `↓` / `k` `j` | Move the selection | +| `Tab` / `Shift-Tab` | Jump the selection to the next / previous section | | `Enter` | Attach to the selected task | | `Space` | Peek at the selected task | | `n` | New command in the invocation directory | @@ -113,7 +114,7 @@ The daemon removes control characters, trims surrounding whitespace, and limits - Recent directories: ones you've launched in before; `Enter` runs there, `Tab`/`→` browses into them. - Subdirectories of the current path: `Enter` or `Tab`/`→` descends into one. -Typing filters the rows; `Backspace` climbs the typed path; `↑`/`↓` move the highlight; `Esc` cancels. Completion updates on each input, permitting navigation and launch without leaving the dashboard. `←`/`→` move the caret within the typed path (`→` descends only when the caret is at the end), and `Ctrl-A`/`Ctrl-E` (or `Home`/`End`) jump to either end; the same caret keys work in every `fleetcom` text field. +Typing filters the rows; `Backspace` deletes one character and the matches re-filter; `↑`/`↓` move the highlight; `Esc` cancels. Completion updates on each input, permitting navigation and launch without leaving the dashboard. `←`/`→` move the caret within the typed path (`→` descends only when the caret is at the end), and `Ctrl-A`/`Ctrl-E` (or `Home`/`End`) jump to either end; the same caret keys work in every `fleetcom` text field. ## The `g` group picker From fb3b2e5db50ea1f0ef3c82eabfbb0c838f849654 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Mon, 20 Jul 2026 21:23:55 -0700 Subject: [PATCH 03/10] fix: prefix terminal errors and refuse non-tty stdout at the process boundary --- src/main.rs | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/src/main.rs b/src/main.rs index 055fd5b..cf32b5d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -36,7 +36,7 @@ mod testutil; pub(crate) use terminal::{ansi, emulator, format, frame, input}; use std::{ - io, + io::{self, IsTerminal}, sync::{ Arc, atomic::{AtomicBool, Ordering}, @@ -153,7 +153,30 @@ fn parse_args(args: &[String]) -> Result { } } -fn main() -> io::Result<()> { +/// The single stderr shape for a fatal error: the `fleetcom: ` prefix plus +/// the error's `Display` form. Every terminal error goes through this or +/// prints the same prefix by hand with added context (the daemon-connect +/// failure); nothing reaches the runtime's `Debug` handler. +fn error_line(e: impl std::fmt::Display) -> String { + format!("fleetcom: {e}") +} + +fn main() { + // Matching here instead of returning `io::Result` from `main` is the whole + // point: the runtime's default handler `Debug`-prints an `Err` — raw + // struct noise, no prefix. Every error that propagates this far becomes + // one prefixed, human-readable stderr line with a deliberate exit code. + if let Err(e) = run() { + eprintln!("{}", error_line(&e)); + std::process::exit(1); + } +} + +/// The whole program, minus error presentation: `main` owns how an `Err` +/// prints and exits. Paths with a more specific report or exit code +/// (usage errors, the daemon-connect failure, the tty guard) print and +/// `exit` directly rather than flattening into the generic exit-1 line. +fn run() -> io::Result<()> { let args: Vec = std::env::args().skip(1).collect(); let (foreground, session, scrollback) = match parse_args(&args) { Ok(Invocation::Help) => { @@ -174,12 +197,23 @@ fn main() -> io::Result<()> { scrollback, }) => (foreground, session, scrollback), Err(e) => { - eprintln!("fleetcom: {e}"); + eprintln!("{}", error_line(e)); eprintln!("try 'fleetcom --help'"); std::process::exit(2); } }; + // Refuse a redirected stdout before any terminal setup: crossterm falls + // back to /dev/tty for the control sequences, so `fleetcom > file` would + // leave the user's real terminal raw and blank while frames stream into + // the file. Client path only — `--daemon` is deliberately headless, and + // help/version/`--kill` never touch the terminal. No degraded mode: + // refusing loudly beats rendering into a pipe. + if !io::stdout().is_terminal() { + eprintln!("{}", error_line("stdout is not a terminal")); + std::process::exit(1); + } + // Install the value before constructing or autostarting a supervisor. if let Some(lines) = scrollback { supervisor::set_scrollback_flag(lines); @@ -419,6 +453,21 @@ mod tests { assert!(parse(&["one", "two"]).is_err()); } + /// The boundary contract: fatal errors print the `Display` form behind + /// the `fleetcom: ` prefix — never `Debug`'s struct noise. + #[test] + fn error_line_prefixes_display_form() { + let e = io::Error::new(io::ErrorKind::TimedOut, "daemon did not exit after SIGTERM"); + assert_eq!( + error_line(&e), + "fleetcom: daemon did not exit after SIGTERM" + ); + assert_eq!( + error_line("stdout is not a terminal"), + "fleetcom: stdout is not a terminal" + ); + } + fn contains(haystack: &[u8], needle: &[u8]) -> bool { haystack.windows(needle.len()).any(|w| w == needle) } From 5d979a2392ece33f40c96de6308e7c27bb0bb45a Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Mon, 20 Jul 2026 21:25:41 -0700 Subject: [PATCH 04/10] fix: cap session filenames by bytes, not chars; document path conventions --- docs/sessions.md | 8 ++--- src/session.rs | 83 ++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 74 insertions(+), 17 deletions(-) diff --git a/docs/sessions.md b/docs/sessions.md index 8b84f3c..7221031 100644 --- a/docs/sessions.md +++ b/docs/sessions.md @@ -14,7 +14,7 @@ A session is a launch recipe, not a process snapshot. It records commands, worki The first save creates the directory. This matches the [configuration path resolution](README.md#config-directory-sessions) used by save, list, and load. -The filename derives from the session name. `fleetcom` trims leading and trailing whitespace, replaces control characters and any of `* " / \ < > : | ? .` with `_`, and caps the result at 255 characters: `my/session` becomes `my_session.json`, and `a.b` becomes `a_b.json`. Replacing `.` prevents the session name from supplying another extension. +The filename derives from the session name. `fleetcom` trims leading and trailing whitespace, replaces control characters and any of `* " / \ < > : | ? .` with `_`, and caps the result at 250 bytes: `NAME_MAX` is 255 bytes on common Unix filesystems, and the `.json` extension takes the other 5. The cap falls on a character boundary — a multibyte character that would cross it is dropped whole, never split. `my/session` becomes `my_session.json`, and `a.b` becomes `a_b.json`. Replacing `.` prevents the session name from supplying another extension. ## Format @@ -24,7 +24,7 @@ A session file is a JSON object with two fields. `name` holds the session name a { "name": "work/api", "dirs": { - "/home/you/work/api": [ + "~/work/api": [ "cargo watch -x test", { "cmd": "cargo run", "group": "api", "name": "api server" } ], @@ -36,7 +36,7 @@ A session file is a JSON object with two fields. `name` holds the session name a ``` - `name` exists because sanitization collapses distinct session names onto one filename: `a/b` and `a.b` both save to `a_b.json`. Saving compares the stored name against the incoming one and refuses a mismatch with an error naming both sessions. The load picker also displays it, so the list shows `a/b`, not `a_b`. -- Keys under `dirs` are directory paths: each task's working directory. +- Keys under `dirs` are directory paths: each task's working directory. Saves write directories under `$HOME` as `~/…`; other paths stay absolute. On load, `~` expands to `$HOME`, and a relative key resolves against the invocation directory of the client loading the session. - Values are ordered lists. A string member is a bare shell command; the object form adds the optional group and display name assigned on load. Order is preserved, and each command runs in its own PTY under that directory. - Directories serialize alphabetically. Command order remains stable within each directory. @@ -54,7 +54,7 @@ A bare agent command does not identify its conversation, so saving it verbatim w { "name": "agents", "dirs": { - "/home/you/work/api": [ + "~/work/api": [ "claude --resume 'c8c4a5cc-0b32-4ba0-a6b4-6ed08c218e0d'", { "cmd": "codex resume '019f5453-de22-7240-b2e5-0d32692aa6d9'", "name": "reviewer" } ] diff --git a/src/session.rs b/src/session.rs index 636cd11..15d4479 100644 --- a/src/session.rs +++ b/src/session.rs @@ -28,19 +28,38 @@ pub const FLEETCOM_CONFIG_DIR: &str = "FLEETCOM_CONFIG_DIR"; /// Characters replaced with `_` in session filenames. const DISALLOWED: &[char] = &['*', '"', '/', '\\', '<', '>', ':', '|', '?', '.']; -/// Make `name` safe as a bare filename. +/// Byte cap for sanitized names: `NAME_MAX` (255 bytes) common to Unix +/// filesystems minus the 5-byte `.json` extension `save_in` appends. +const MAX_STEM_BYTES: usize = 250; + +/// Make `name` safe as a bare filename. The cap counts encoded bytes, not +/// chars: a char that would cross [`MAX_STEM_BYTES`] drops whole, never split. fn sanitize(name: &str) -> String { - name.trim() - .chars() - .map(|c| { - if c.is_control() || DISALLOWED.contains(&c) { - '_' - } else { - c - } - }) - .take(255) - .collect() + let mut out = String::new(); + for c in name.trim().chars() { + let c = if c.is_control() || DISALLOWED.contains(&c) { + '_' + } else { + c + }; + if out.len() + c.len_utf8() > MAX_STEM_BYTES { + break; + } + out.push(c); + } + out +} + +/// Longest prefix of `s` at most `max` bytes long, on a char boundary. +fn prefix_bytes(s: &str, max: usize) -> &str { + if s.len() <= max { + return s; + } + let mut end = max; + while !s.is_char_boundary(end) { + end -= 1; + } + &s[..end] } /// Session-recipe directory: `/sessions`. A caller-supplied @@ -172,7 +191,12 @@ pub fn save_in(dir: &Path, name: &str, cfg: &SessionConfig) -> io::Result Date: Mon, 20 Jul 2026 21:27:22 -0700 Subject: [PATCH 05/10] fix: reap dead-owner capture namespaces at install --- src/harness/assets.rs | 155 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 147 insertions(+), 8 deletions(-) diff --git a/src/harness/assets.rs b/src/harness/assets.rs index c482e4e..1be9f62 100644 --- a/src/harness/assets.rs +++ b/src/harness/assets.rs @@ -4,6 +4,11 @@ //! `task--.json` path per task run. The nonce isolates concurrent //! processes and prevents PID reuse from selecting an existing namespace. //! +//! Namespace lifecycle: `Drop` removes this process's own namespace; `install` +//! reaps dead-owner siblings. Drop alone cannot cover a SIGKILLed or OOM-killed +//! supervisor, and the cache-directory fallback root persists across boots, so +//! without the reap those namespaces would accumulate forever. +//! //! Asset contracts: //! - `claude`: `--settings ` layers a `SessionStart` hook //! (`cat > "$FLEETCOM_CAPTURE_FILE"`) over the user's settings. The hook @@ -79,6 +84,67 @@ pub fn runtime_root(override_dir: Option<&Path>) -> Option { dirs::cache_dir().map(|c| c.join("fleetcom").join("run")) } +/// Remove sibling namespaces whose owning supervisor is dead. Drop-only +/// cleanup leaks the namespace of any supervisor that dies without unwinding +/// (SIGKILL, OOM kill), so each install sweeps the root's immediate entries +/// and removes every directory named `-` whose pid probes dead. +/// Removal is best-effort per entry: a failed reap never fails `install`. +/// +/// One reachable corner: a TERM-refusing reparented child of a dead supervisor +/// may still hold `FLEETCOM_CAPTURE_FILE` pointing into a reaped namespace. +/// Its eventual write fails on the missing path, which is harmless — the codex +/// notify script continues to its chain, and the claude hook errors +/// cosmetically. +fn reap_dead_namespaces(root: &Path) { + let Ok(entries) = fs::read_dir(root) else { + return; + }; + for entry in entries.flatten() { + // `DirEntry::file_type` does not follow symlinks, so a symlink named + // like a namespace is not a directory here and stays untouched. + if !entry.file_type().is_ok_and(|t| t.is_dir()) { + continue; + } + let name = entry.file_name(); + let Some(pid) = namespace_owner(name.to_str().unwrap_or("")) else { + continue; + }; + if owner_is_dead(pid) { + let _ = fs::remove_dir_all(entry.path()); + } + } +} + +/// Parse a namespace name into its owner pid. The match is strict — split on +/// the first `-`, the pid part is all decimal digits parsing as `i32 > 0` +/// (parse failure covers overflow past `i32::MAX`), and the nonce part is +/// exactly 12 lowercase hex chars. Anything else is not a namespace and must +/// be kept: root-level legacy files and foreign layouts pass through here. +fn namespace_owner(name: &str) -> Option { + let (pid, nonce) = name.split_once('-')?; + if pid.is_empty() || !pid.bytes().all(|b| b.is_ascii_digit()) { + return None; + } + if nonce.len() != 12 + || !nonce + .bytes() + .all(|b| b.is_ascii_hexdigit() && !b.is_ascii_uppercase()) + { + return None; + } + pid.parse::().ok().filter(|p| *p > 0) +} + +/// Whether `pid` provably names no live process. The probe's errors are +/// one-sided by design: only `ESRCH` reads as dead. `Ok` and `EPERM` read as +/// alive (or not ours to judge), and any other errno keeps the entry. A +/// recycled pid therefore misreads only as "alive" — the reaper can keep +/// garbage but can never delete a live supervisor's namespace. +fn owner_is_dead(pid: i32) -> bool { + use nix::{errno::Errno, sys::signal::kill, unistd::Pid}; + matches!(kill(Pid::from_raw(pid), None), Err(Errno::ESRCH)) +} + /// Capture assets owned by one supervisor process. #[derive(Debug)] pub struct CaptureAssets { @@ -92,7 +158,8 @@ impl CaptureAssets { /// Create `root` and a private `/-` namespace. The /// namespace uses mode `0700`; its Claude settings use `0600`, and its /// executable Codex notifier uses `0700`. Existing root entries remain - /// unchanged. + /// unchanged except sibling namespaces whose owner is provably dead: those + /// are reaped best-effort before the new namespace is minted. pub fn install(root: &Path, pid: u32) -> io::Result { fs::DirBuilder::new() .recursive(true) @@ -101,6 +168,8 @@ impl CaptureAssets { // Recursive creation retains a pre-existing directory's permissions. fs::set_permissions(root, fs::Permissions::from_mode(0o700))?; + reap_dead_namespaces(root); + // The first 12 dash-free UUID characters contain 48 random bits; the // UUID version and variant occur later in the string. let nonce: String = super::uuid_v4() @@ -244,11 +313,13 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// Installation leaves every pre-existing root entry unchanged. + /// Installation leaves live-owner namespaces and every non-namespace root + /// entry unchanged. Pid 1 is always alive and never ours to signal, so the + /// probe reads it as alive on both its `Ok` and `EPERM` outcomes. #[test] - fn install_never_deletes_foreign_or_legacy_files() { + fn install_never_deletes_live_owner_namespaces_or_legacy_files() { let root = temp("assets_retain"); - let foreign = root.join("99999-0123456789ab"); + let foreign = root.join("1-0123456789ab"); fs::create_dir_all(&foreign).unwrap(); fs::write(foreign.join("task-1-0.json"), "{}").unwrap(); fs::write(root.join("task-1-0.json"), "{}").unwrap(); @@ -258,7 +329,7 @@ mod tests { let assets = CaptureAssets::install(&root, std::process::id()).unwrap(); assert!( foreign.join("task-1-0.json").exists(), - "another process's capture file must survive" + "a live process's capture file must survive" ); assert!( root.join("task-1-0.json").exists(), @@ -278,7 +349,9 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// A matching PID prefix does not cause an existing namespace to be reused. + /// A matching PID prefix does not cause an existing namespace to be reused + /// or reaped: the planted namespace carries this test's own live pid, so + /// the liveness probe keeps it and the nonce forces a distinct directory. #[test] fn install_after_pid_reuse_leaves_the_predecessor_namespace_alone() { let root = temp("assets_reuse"); @@ -317,6 +390,71 @@ mod tests { let _ = fs::remove_dir_all(&root); } + /// Spawn and wait a trivial child, returning its now-dead pid. Waiting + /// reaps the zombie — a zombie still accepts signal 0 and would probe as + /// alive. The tiny window for pid reuse before the probe is accepted. + fn dead_pid() -> u32 { + let mut child = Command::new("sh").arg("-c").arg("exit 0").spawn().unwrap(); + let pid = child.id(); + child.wait().unwrap(); + pid + } + + /// A namespace directory whose owner pid probes dead is reaped at + /// install, contents included. + #[test] + fn install_reaps_a_dead_owner_namespace() { + let root = temp("assets_reap"); + let dead = root.join(format!("{}-0123456789ab", dead_pid())); + fs::create_dir_all(&dead).unwrap(); + fs::write(dead.join("task-1-0.json"), "{}").unwrap(); + + let assets = CaptureAssets::install(&root, std::process::id()).unwrap(); + assert!(!dead.exists(), "a dead owner's namespace must be reaped"); + assert!(assets.claude_settings.exists()); + let _ = fs::remove_dir_all(&root); + } + + /// Only directories are namespaces: a root-level file named like a dead + /// owner's namespace is kept. + #[test] + fn install_keeps_a_file_named_like_a_dead_namespace() { + let root = temp("assets_reap_file"); + fs::create_dir_all(&root).unwrap(); + let decoy = root.join(format!("{}-0123456789ab", dead_pid())); + fs::write(&decoy, "not a namespace").unwrap(); + + let _assets = CaptureAssets::install(&root, std::process::id()).unwrap(); + assert!(decoy.exists(), "a file is never a reap candidate"); + let _ = fs::remove_dir_all(&root); + } + + /// Names outside the strict `-<12 lowercase hex>` shape are + /// never probed or reaped, whatever their pid part would mean. + #[test] + fn install_keeps_directories_with_malformed_namespace_names() { + let root = temp("assets_reap_malformed"); + let names = [ + "abc-0123456789ab", // non-numeric pid + "-1-0123456789ab", // negative pid: empty first field + "+42-0123456789ab", // sign prefix is not a decimal digit + "99999999999-0123456789ab", // past i32::MAX + "42-0123456789AB", // uppercase nonce + "42-0123456789a", // 11-char nonce + "42-0123456789abc", // 13-char nonce + "42", // no dash at all + ]; + for name in names { + fs::create_dir_all(root.join(name)).unwrap(); + } + + let _assets = CaptureAssets::install(&root, std::process::id()).unwrap(); + for name in names { + assert!(root.join(name).exists(), "{name:?} must be kept"); + } + let _ = fs::remove_dir_all(&root); + } + /// The notifier overwrites the capture file with its first argument. With /// no capture path or chain, it exits without producing output. #[test] @@ -474,11 +612,12 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// Drop removes only the owned namespace and its contents. + /// Drop removes only the owned namespace and its contents. The sibling + /// carries pid 1 so `install`'s reap keeps it: the test isolates Drop. #[test] fn drop_removes_only_the_incarnation_namespace() { let root = temp("assets_drop"); - let sibling = root.join("99999-0123456789ab"); + let sibling = root.join("1-0123456789ab"); fs::create_dir_all(&sibling).unwrap(); fs::write(sibling.join("task-1-0.json"), "{}").unwrap(); From 5224c3c6b70eca363bbff8e85914e97a6e1acfb0 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Mon, 20 Jul 2026 21:28:26 -0700 Subject: [PATCH 06/10] fix: bound the --kill socket fallback; surface an ignored --scrollback --- src/app.rs | 5 +- src/daemon.rs | 222 ++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 201 insertions(+), 26 deletions(-) diff --git a/src/app.rs b/src/app.rs index 115af38..0b948ab 100644 --- a/src/app.rs +++ b/src/app.rs @@ -257,7 +257,7 @@ impl App { /// *this* client's env. The core lives in `fleetcom --daemon`, reached over /// the socket. pub fn connect(rows: u16, cols: u16) -> io::Result { - let stream = crate::daemon::connect_ready()?; + let (stream, origin) = crate::daemon::connect_ready()?; // Split the stream here (the fallible part) so the transport factory in // `assemble` (which owns the wake sender) stays infallible. let read = stream.try_clone()?; @@ -265,6 +265,9 @@ impl App { Box::new(SocketTransport::from_halves(stream, read, wait_tx)) }); app.daemon_backed = true; + // A `--scrollback` that an already-running daemon never saw gets one + // visible status-line notice instead of a silently kept old value. + app.status = crate::daemon::ignored_scrollback_notice(origin); Ok(app) } diff --git a/src/daemon.rs b/src/daemon.rs index 77d438f..04f684e 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -33,7 +33,7 @@ use std::{ mpsc::channel, }, thread, - time::Duration, + time::{Duration, Instant}, }; use nix::{ @@ -200,11 +200,45 @@ fn check_hello_ack(kind: u8, payload: &[u8]) -> io::Result<()> { } } +/// How this client's daemon connection came to exist. The distinction matters +/// because startup-only configuration (`--scrollback`) reaches the daemon +/// solely through `spawn_daemon`'s environment: an `AlreadyRunning` daemon +/// never saw this invocation's flags. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum DaemonOrigin { + /// The pre-spawn connect succeeded: a daemon predates this invocation. + AlreadyRunning, + /// This invocation spawned the daemon, which inherited its environment. + Autostarted, +} + +/// The dashboard notice for a `--scrollback` the daemon never saw, or `None` +/// when the flag took effect (autostart) or was not passed. Kept beside +/// `DaemonOrigin` because the *why* is daemon lifecycle: the flag rides +/// `spawn_daemon`'s env, so an already-running daemon keeps its resolved +/// depth until `--kill` restarts it. +fn scrollback_notice(origin: DaemonOrigin, flag: Option) -> Option { + match (origin, flag) { + (DaemonOrigin::AlreadyRunning, Some(lines)) => Some(format!( + "--scrollback {lines} ignored: the daemon was already running and \ + keeps its scrollback until 'fleetcom --kill'" + )), + _ => None, + } +} + +/// `scrollback_notice` over this process's actual `--scrollback` flag. +pub fn ignored_scrollback_notice(origin: DaemonOrigin) -> Option { + scrollback_notice(origin, supervisor::scrollback_flag()) +} + /// Connect (autostarting if needed) and complete the hello handshake: send /// this process's protocol version and launch context, require the daemon's /// ack. Every launch this connection makes then runs under *this* client's /// env, and a version mismatch surfaces as one actionable error here instead -/// of a silently wrong environment later. +/// of a silently wrong environment later. Also reports whether this +/// invocation autostarted the daemon, so the caller can flag startup-only +/// options an already-running daemon ignored. /// /// The daemon serves one client at a time, so a slow handshake means "queued /// behind another client", not failure: announce it and wait without a @@ -214,8 +248,8 @@ fn check_hello_ack(kind: u8, payload: &[u8]) -> io::Result<()> { /// timed-out partial `write_all` would corrupt the framing. Callers run this /// *before* touching terminal state (raw mode, alternate screen), so the /// notice prints normally and Ctrl-C aborts cleanly while waiting. -pub fn connect_ready() -> io::Result { - let mut stream = connect_or_autostart()?; +pub fn connect_ready() -> io::Result<(UnixStream, DaemonOrigin)> { + let (mut stream, origin) = connect_or_autostart()?; let done = Arc::new(AtomicBool::new(false)); { @@ -237,7 +271,7 @@ pub fn connect_ready() -> io::Result { done.store(true, Ordering::Relaxed); let (kind, payload) = reply.map_err(hello_read_error)?; check_hello_ack(kind, &payload)?; - Ok(stream) + Ok((stream, origin)) } /// Convert handshake timeouts to a busy-daemon error; preserve other errors. @@ -260,7 +294,9 @@ fn busy_daemon_error(e: io::Error) -> io::Error { /// connection) must not wedge the UI either. A timed-out write drops the /// connection, so a partial frame is never read. pub fn connect_ready_bounded() -> io::Result { - let mut stream = connect_or_autostart()?; + // The origin is dropped: the ignored-`--scrollback` notice is a startup + // concern, shown once by `App::connect`, not repeated on every reconnect. + let (mut stream, _) = connect_or_autostart()?; stream.set_write_timeout(Some(HANDSHAKE_TIMEOUT))?; let (kind, payload) = encode_hello(&LaunchContext::here()); write_frame(&mut stream, kind, &payload).map_err(busy_daemon_error)?; @@ -273,18 +309,20 @@ pub fn connect_ready_bounded() -> io::Result { } /// Connect to the daemon or start one, then wait up to one second for its socket. -fn connect_or_autostart() -> io::Result { +fn connect_or_autostart() -> io::Result<(UnixStream, DaemonOrigin)> { connect_or_autostart_in(&runtime_dir()) } -/// Validate `dir` before the first daemon connection attempt. -fn connect_or_autostart_in(dir: &Path) -> io::Result { +/// Validate `dir` before the first daemon connection attempt. The returned +/// origin records which branch produced the stream: the pre-spawn connect +/// succeeding is the one signal that a daemon predates this invocation. +fn connect_or_autostart_in(dir: &Path) -> io::Result<(UnixStream, DaemonOrigin)> { // Validate before connecting because the hello sends the client's // environment and a successful connection skips daemon-side validation. ensure_runtime_dir(dir)?; let path = socket_in(dir); if let Ok(s) = UnixStream::connect(&path) { - return Ok(s); + return Ok((s, DaemonOrigin::AlreadyRunning)); } // Never unlink the socket here: `ECONNREFUSED` on AF_UNIX can also mean a // live daemon's accept backlog is momentarily full. @@ -293,7 +331,7 @@ fn connect_or_autostart_in(dir: &Path) -> io::Result { spawn_daemon()?; for _ in 0..100 { if let Ok(s) = UnixStream::connect(&path) { - return Ok(s); + return Ok((s, DaemonOrigin::Autostarted)); } thread::sleep(Duration::from_millis(10)); } @@ -364,7 +402,8 @@ pub fn run_kill() -> io::Result<()> { file.read_to_string(&mut pid_str)?; let Some(pid) = pid_str.trim().parse::().ok().filter(|p| *p > 0) else { // Without a usable pid, fall back to a Shutdown frame over the socket. - // This path waits until any attached client disconnects. + // Bounded: an attached client keeps the daemon from ever accepting + // this connection, and `--kill` must terminate regardless. return kill_via_socket(); }; @@ -391,22 +430,46 @@ pub fn run_kill() -> io::Result<()> { )) } +/// Overall budget for the socket-fallback kill exchange, matching the pid +/// path's 10 s flock poll. Without it the fallback inherits the daemon's +/// one-client-at-a-time serving: with another client attached, the hello +/// reply never comes and `--kill` would block forever, silently. +const KILL_SOCKET_TIMEOUT: Duration = Duration::from_secs(10); + +/// The one error the socket-fallback deadline produces. The daemon is +/// demonstrably up (the connect succeeded) but never finished the exchange, +/// which with a healthy daemon means it is serving an attached client. +fn kill_handshake_timeout() -> io::Error { + io::Error::new( + ErrorKind::TimedOut, + "the daemon is running but did not complete the kill handshake in \ + time (another client may be attached); retry after it detaches, or \ + send SIGTERM to the daemon process directly", + ) +} + +/// Arm the stream's read timeout with what is left of `deadline`. Errors out +/// when the budget is already spent, because `set_read_timeout` rejects a +/// zero duration and a stale timeout would silently extend the deadline. +fn arm_read_deadline(s: &UnixStream, deadline: Instant) -> io::Result<()> { + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + return Err(kill_handshake_timeout()); + } + s.set_read_timeout(Some(left)) +} + /// Send `Shutdown` when the lock file has no usable pid. Complete the handshake /// first, then wait for the daemon to close the socket after stopping its tasks. fn kill_via_socket() -> io::Result<()> { - let path = socket_path(); - match UnixStream::connect(&path) { - Ok(mut s) => { - let (kind, payload) = encode_hello(&LaunchContext::here()); - write_frame(&mut s, kind, &payload)?; - let (kind, payload) = read_frame(&mut s)?; - check_hello_ack(kind, &payload)?; - let (kind, payload) = encode_command(&Command::Shutdown); - write_frame(&mut s, kind, &payload)?; - let mut buf = [0u8; 256]; - while s.read(&mut buf).map(|n| n > 0).unwrap_or(false) {} - Ok(()) - } + kill_via_socket_at(&socket_path(), KILL_SOCKET_TIMEOUT) +} + +/// `kill_via_socket` against an explicit socket under an injectable budget +/// (tests use a short one against a mute listener). +fn kill_via_socket_at(path: &Path, budget: Duration) -> io::Result<()> { + match UnixStream::connect(path) { + Ok(mut s) => kill_over_stream(&mut s, budget), Err(_) => { eprintln!("fleetcom: no daemon running"); Ok(()) @@ -414,6 +477,44 @@ fn kill_via_socket() -> io::Result<()> { } } +/// Drive the Shutdown exchange over a connected stream, every blocking step +/// bounded by one overall deadline. Writes share the budget too: an attached +/// client means our connection sits unaccepted, and while the small frames +/// almost surely fit the send buffer, "almost surely" is not a deadline. +fn kill_over_stream(s: &mut UnixStream, budget: Duration) -> io::Result<()> { + let deadline = Instant::now() + budget; + s.set_write_timeout(Some(budget))?; + + let (kind, payload) = encode_hello(&LaunchContext::here()); + write_frame(s, kind, &payload)?; + arm_read_deadline(s, deadline)?; + let (kind, payload) = read_frame(s).map_err(|e| { + if is_timeout(&e) { + kill_handshake_timeout() + } else { + e + } + })?; + check_hello_ack(kind, &payload)?; + + let (kind, payload) = encode_command(&Command::Shutdown); + write_frame(s, kind, &payload)?; + // Drain until the daemon closes the socket: its exit is the completion + // signal, mirroring the pid path's flock poll. Each pass re-arms the + // remaining budget so the loop cannot outlive the deadline. Non-timeout + // read errors keep the old meaning — the daemon is gone, kill complete. + let mut buf = [0u8; 256]; + loop { + arm_read_deadline(s, deadline)?; + match s.read(&mut buf) { + Ok(0) => return Ok(()), + Ok(_) => {} + Err(e) if is_timeout(&e) => return Err(kill_handshake_timeout()), + Err(_) => return Ok(()), + } + } +} + /// The daemon entry point (`fleetcom --daemon`). Binds the socket and serves clients /// until an explicit shutdown. The supervisor is created once and persists across /// reconnects: tasks outlive any single client. @@ -823,6 +924,77 @@ mod tests { let _ = fs::remove_dir_all(&base); } + /// The ignored-`--scrollback` decision table: only "flag passed" plus + /// "daemon predates this invocation" produces the notice. + #[test] + fn scrollback_notice_requires_flag_and_preexisting_daemon() { + assert_eq!( + scrollback_notice(DaemonOrigin::Autostarted, Some(50_000)), + None + ); + assert_eq!(scrollback_notice(DaemonOrigin::Autostarted, None), None); + assert_eq!(scrollback_notice(DaemonOrigin::AlreadyRunning, None), None); + let notice = scrollback_notice(DaemonOrigin::AlreadyRunning, Some(50_000)).unwrap(); + assert!(notice.contains("--scrollback 50000"), "{notice}"); + assert!(notice.contains("--kill"), "{notice}"); + } + + /// A daemon that accepts the connection but never answers the hello (the + /// attached-client shape: our connection sits in the backlog) must error + /// out on the deadline, not hang `--kill` forever. + #[test] + fn kill_via_socket_bounds_the_handshake_wait() { + let base = temp("kill_socket_mute"); + fs::create_dir_all(&base).unwrap(); + let sock = base.join("mute.sock"); + // Never accepted: connect still succeeds via the backlog, exactly like + // a live daemon busy serving another client. + let _listener = UnixListener::bind(&sock).unwrap(); + let start = Instant::now(); + let err = kill_via_socket_at(&sock, Duration::from_millis(200)).unwrap_err(); + assert_eq!(err.kind(), ErrorKind::TimedOut); + assert!(err.to_string().contains("kill handshake"), "{err}"); + assert!( + start.elapsed() < Duration::from_secs(5), + "the deadline must fire, not the test's timeout" + ); + let _ = fs::remove_dir_all(&base); + } + + /// The post-Shutdown drain is bounded too: a daemon that acks the hello + /// and swallows `Shutdown` without ever closing the socket must not turn + /// the completion wait into a hang. + #[test] + fn kill_via_socket_bounds_the_drain_wait() { + let base = temp("kill_socket_drain"); + fs::create_dir_all(&base).unwrap(); + let sock = base.join("stuck.sock"); + let listener = UnixListener::bind(&sock).unwrap(); + let server = thread::spawn(move || { + let (mut s, _) = listener.accept().unwrap(); + let _ = read_frame(&mut s); // hello + let (kind, payload) = encode_event(&Event::HelloOk); + let _ = write_frame(&mut s, kind, &payload); + let _ = read_frame(&mut s); // Shutdown, swallowed + // Hold the socket open until the client gives up and drops its + // end; this read's EOF is the thread's exit signal. + let _ = s.read(&mut [0u8; 16]); + }); + let err = kill_via_socket_at(&sock, Duration::from_millis(300)).unwrap_err(); + assert_eq!(err.kind(), ErrorKind::TimedOut); + server.join().unwrap(); + let _ = fs::remove_dir_all(&base); + } + + /// No socket means no daemon: the fallback stays a friendly no-op. + #[test] + fn kill_via_socket_without_a_socket_is_a_noop() { + let base = temp("kill_socket_absent"); + fs::create_dir_all(&base).unwrap(); + assert!(kill_via_socket_at(&base.join("absent.sock"), Duration::from_millis(100)).is_ok()); + let _ = fs::remove_dir_all(&base); + } + /// Remove group and other read/execute permissions from a valid directory. #[test] fn ensure_runtime_dir_tightens_harmless_bits() { From 3789416eb77f2c4ced28175bfa97285f619a6bfa Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Mon, 20 Jul 2026 21:30:33 -0700 Subject: [PATCH 07/10] fix(ui): pad task-derived text by display columns, not char counts --- src/ui.rs | 121 +++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 96 insertions(+), 25 deletions(-) diff --git a/src/ui.rs b/src/ui.rs index 72c15b5..77ceb25 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -324,7 +324,9 @@ fn row_age(v: &TaskView) -> Duration { } /// Split a task row into its leading, preview, and time cells so the preview -/// can be styled independently. +/// can be styled independently. Cells are column-exact: `pad` measures +/// terminal columns, so a wide-glyph title or preview (CJK, emoji) fills its +/// budget instead of overflowing it, and the three widths always sum to `cols`. fn task_row_parts(v: &TaskView, cols: usize) -> (String, String, String) { let glyph = match v.lifecycle { Lifecycle::Active => "✻", @@ -335,15 +337,13 @@ fn task_row_parts(v: &TaskView, cols: usize) -> (String, String, String) { let tag = if v.tagged { "◆" } else { " " }; let time = rel_time(row_age(v)); let title_w = 26.min(cols / 3); - let title = truncate(display_label(v), title_w); - // prefix(2) glyph+sp(2) tag+sp(2) title(title_w) sp(1) preview(prev_w) sp(1) time - let used = 2 + 2 + 2 + title_w + 1 + 1 + time.chars().count(); + // indent(2) glyph(1) sp(1) tag(1) title(title_w) sp(1) preview(prev_w) sp(1) time + let used = 2 + 1 + 1 + 1 + title_w + 1 + 1 + time.width(); let prev_w = cols.saturating_sub(used); - let preview = truncate(&v.preview.text, prev_w); ( - format!(" {glyph} {tag}{title: String { /// Paint a task row with only its padded preview cell dimmed. fn dim_preview_row(out: &mut impl Write, y: u16, v: &TaskView, cols: usize) -> io::Result<()> { + // Queue the three column-exact cells directly rather than re-splitting a + // composed string: a char-count split desynchronizes from cell boundaries + // as soon as a cell holds wide glyphs. Clamping each cell to the columns + // still open reproduces `put`'s pad-to-`cols` behavior on narrow terminals. let (lead, preview, time) = task_row_parts(v, cols); - let display = pad(&format!("{lead}{preview}{time}"), cols); - let mut chars = display.chars(); - let lead: String = chars.by_ref().take(lead.chars().count()).collect(); - let preview: String = chars.by_ref().take(preview.chars().count()).collect(); - let rest: String = chars.collect(); + let lead = truncate(&lead, cols); + let mut rem = cols - lead.width(); + let preview = truncate(&preview, rem); + rem -= preview.width(); queue!( out, MoveTo(0, y), @@ -368,10 +371,22 @@ fn dim_preview_row(out: &mut impl Write, y: u16, v: &TaskView, cols: usize) -> i SetAttribute(Attribute::Dim), Print(preview), SetAttribute(Attribute::Reset), - Print(rest) + Print(pad(&time, rem)) ) } +/// Peek-box top border: `─ label ` extended with `─` fill to exactly +/// `inner_w` columns. The label is measured in display columns — wide glyphs +/// consume two — so the fill always meets the corner. +fn peek_top_border(label: &str, inner_w: usize) -> String { + let mut border = format!("─ {} ", truncate(label, inner_w.saturating_sub(4))); + let w = border.width(); + if w < inner_w { + border.extend(std::iter::repeat_n('─', inner_w - w)); + } + border +} + fn render_peek(out: &mut impl Write, app: &App) -> io::Result<()> { let Some(i) = app.selected_task() else { return Ok(()); @@ -394,19 +409,10 @@ fn render_peek(out: &mut impl Write, app: &App) -> io::Result<()> { let start = lines.len().saturating_sub(inner_h); let tail = &lines[start..]; - // Top border with the task's display label inlined. - let mut top_mid = format!( - "─ {} ", - truncate(display_label(v), inner_w.saturating_sub(4)) - ); - let tl = top_mid.chars().count(); - if tl < inner_w { - top_mid.extend(std::iter::repeat_n('─', inner_w - tl)); - } queue!( out, MoveTo(x0 as u16, y0 as u16), - Print(format!("┌{top_mid}┐")) + Print(format!("┌{}┐", peek_top_border(display_label(v), inner_w))) )?; for k in 0..inner_h { @@ -616,13 +622,13 @@ fn render_session_picker(out: &mut impl Write, app: &App) -> io::Result<()> { /// Center `s` in `width` columns (a full-width string, so it overwrites the row). fn center(s: &str, width: usize) -> String { - let len = s.chars().count(); + let len = s.width(); if len >= width { return truncate(s, width); } let mut out = " ".repeat((width - len) / 2); out.push_str(s); - let cur = out.chars().count(); + let cur = out.width(); out.push_str(&" ".repeat(width - cur)); out } @@ -798,6 +804,71 @@ mod tests { ); } + /// Every cell is padded in display columns, so the composed row is + /// exactly `cols` wide and the time cell survives at the right edge for + /// any title/preview content: CJK, emoji, combining marks. + #[test] + fn task_row_is_column_exact_for_wide_glyphs() { + let titles = [ + "plain ascii title", + "日本語のタスクタイトルです", // 13 wide chars: fills title_w exactly at 80 cols + "🚀 emoji 🚀 title", + "e\u{0301}e\u{0301} combining", // combining marks are zero-width + ]; + let previews = [ + "build ok", + "ビルド中の😀プレビュー出力がここに続いています", + "", + ]; + for cols in [40usize, 80] { + for title in titles { + for preview in previews { + let mut v = timed_view(Lifecycle::Active, false, None, None); + v.name = Some(title.to_string()); + v.preview = Preview::floor(preview.to_string()); + let row = task_row(&v, cols); + assert_eq!( + row.width(), + cols, + "cols {cols} title {title:?} preview {preview:?}: {row:?}" + ); + assert!(row.ends_with(" 2h"), "time cell lost: {row:?}"); + } + } + } + } + + /// Cell budgets are fixed by `cols`, not by content: `dim_preview_row` + /// queues the cells separately, so the dim run must cover exactly the + /// preview cell whatever glyphs the cells hold. + #[test] + fn task_row_cells_hold_their_column_budgets() { + let cols = 72; + let ascii = timed_view(Lifecycle::Active, false, None, None); + let mut wide = timed_view(Lifecycle::Active, false, None, None); + wide.name = Some("日本語のテスト".into()); + wide.preview = Preview::floor("進捗 50% 😀".into()); + let (al, ap, at) = task_row_parts(&ascii, cols); + let (wl, wp, wt) = task_row_parts(&wide, cols); + assert_eq!(al.width(), wl.width(), "lead width varies with content"); + assert_eq!(ap.width(), wp.width(), "preview width varies with content"); + assert_eq!(at.width(), wt.width(), "time width varies with content"); + assert_eq!(wl.width() + wp.width() + wt.width(), cols); + } + + /// The peek top border fills to exactly the inner width for any label, + /// including wide glyphs and labels longer than the border. + #[test] + fn peek_top_border_fills_to_inner_width() { + for label in ["cargo test", "日本語のテスト", "🚀 build", "e\u{0301}", ""] { + let b = peek_top_border(label, 40); + assert_eq!(b.width(), 40, "label {label:?}: {b:?}"); + } + // Overlong labels truncate inside the border rather than widening it. + let b = peek_top_border(&"長".repeat(40), 40); + assert_eq!(b.width(), 40, "{b:?}"); + } + /// The peek footer's provenance label composes source, rule, and frozen. #[test] fn preview_provenance_label_shapes() { From 511e94c34578b54239f0226dd6d38942555a6227 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Mon, 20 Jul 2026 21:38:41 -0700 Subject: [PATCH 08/10] feat: gate session loads on an on-disk format version --- docs/sessions.md | 8 ++- src/session.rs | 149 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 151 insertions(+), 6 deletions(-) diff --git a/docs/sessions.md b/docs/sessions.md index 7221031..efb0cee 100644 --- a/docs/sessions.md +++ b/docs/sessions.md @@ -18,10 +18,11 @@ The filename derives from the session name. `fleetcom` trims leading and trailin ## Format -A session file is a JSON object with two fields. `name` holds the session name as typed, trimmed but not sanitized. `dirs` maps each working directory to an ordered list of entries. An entry with neither a group nor a name is a command string. An entry carrying either is an object with `cmd` plus the optional `group` and `name` fields: +A session file is a JSON object with three fields. `version` is the format version, currently 1. `name` holds the session name as typed, trimmed but not sanitized. `dirs` maps each working directory to an ordered list of entries. An entry with neither a group nor a name is a command string. An entry carrying either is an object with `cmd` plus the optional `group` and `name` fields: ```json { + "version": 1, "name": "work/api", "dirs": { "~/work/api": [ @@ -40,7 +41,9 @@ A session file is a JSON object with two fields. `name` holds the session name a - Values are ordered lists. A string member is a bare shell command; the object form adds the optional group and display name assigned on load. Order is preserved, and each command runs in its own PTY under that directory. - Directories serialize alphabetically. Command order remains stable within each directory. -There is no version field; the shape discriminates the schema. An object-valued `dirs` marks the wrapped form shown above. The loader also accepts a flat map whose top-level keys are directories and whose values are entry arrays. In that form, an array-valued key named `dirs` remains a directory entry. Flat-map files list by filename stem because they have no stored name. Saving one writes the wrapped form and permits overwriting it without a stored-name collision check. +Files carry a format version. A missing `version` means 1: every file written before the key existed (releases 0.6.0–0.8.0) is structurally version 1, and that absence rule is permanent. A version newer than the running `fleetcom` supports refuses to load — the error names both versions — instead of dropping the members this build does not recognize and rewriting the file on the next save; a newer build loads it. + +The shape, not the version, discriminates the schema. An object-valued `dirs` marks the wrapped form shown above. The loader also accepts a flat map whose top-level keys are directories and whose values are entry arrays. In that form, an array-valued key named `dirs` remains a directory entry, but a top-level `version` member is always the format version, never a directory. Flat-map files list by filename stem because they have no stored name. Saving one writes the wrapped form and permits overwriting it without a stored-name collision check. Saves are atomic: `fleetcom` writes and syncs a private temporary file in the session directory, then renames it over the recipe. Recipes persist full command lines, which can embed secrets. New session directories use mode 0700, saves remove group and other permissions from existing session directories, and recipe files use mode 0600. @@ -52,6 +55,7 @@ A bare agent command does not identify its conversation, so saving it verbatim w ```json { + "version": 1, "name": "agents", "dirs": { "~/work/api": [ diff --git a/src/session.rs b/src/session.rs index 15d4479..b3fcb92 100644 --- a/src/session.rs +++ b/src/session.rs @@ -72,6 +72,15 @@ pub fn sessions_dir(root: Option) -> Option { .map(|base| base.join("sessions")) } +/// On-disk session format version: written by `to_json`, the newest +/// `from_json` accepts. The contract: any change an older reader would decode +/// lossily bumps this number, and a reader refuses versions above its own +/// rather than dropping what it does not recognize and rewriting the file on +/// the next save. A missing `version` key means 1 — every file written by +/// released fleetcom (0.6.0–0.8.0) predates the key and is structurally v1 — +/// and that absence rule is permanent. +const FORMAT_VERSION: u64 = 1; + /// Serialize `{"name": , "dirs": {...}}`. The stored name lets /// `save_in` distinguish names that sanitize to the same filename. fn to_json(name: &str, cfg: &SessionConfig) -> String { @@ -98,6 +107,7 @@ fn to_json(name: &str, cfg: &SessionConfig) -> String { let _ = dirs.insert(dir, arr); } let mut obj = jzon::JsonValue::new_object(); + let _ = obj.insert("version", FORMAT_VERSION); let _ = obj.insert("name", name); let _ = obj.insert("dirs", dirs); obj.pretty(2) @@ -106,15 +116,49 @@ fn to_json(name: &str, cfg: &SessionConfig) -> String { /// Parse wrapped and flat schemas, returning the stored name when present. /// A wrapped file has an object-valued `dirs`; flat files have entry arrays at /// the top level, including when a directory is literally named `dirs`. +/// A top-level `version` above [`FORMAT_VERSION`] refuses to load: this +/// build would drop the members it does not recognize, and the next save +/// would rewrite the file without them. fn from_json(text: &str) -> io::Result<(Option, SessionConfig)> { let parsed = jzon::parse(text).map_err(|e| io::Error::other(e.to_string()))?; - let (name, dirs) = if parsed["dirs"].is_object() { - (parsed["name"].as_str().map(str::to_string), &parsed["dirs"]) + // Gate before schema detection. Absence means version 1: every file + // written by released fleetcom (0.6.0–0.8.0) predates the key. + let version = &parsed["version"]; + if !version.is_null() { + match version.as_u64() { + Some(n) if (1..=FORMAT_VERSION).contains(&n) => {} + Some(n) if n > FORMAT_VERSION => { + return Err(io::Error::other(format!( + "session format version {n} is newer than this fleetcom \ + (supports {FORMAT_VERSION}); load it with a newer build" + ))); + } + // Zero, fractional, negative, or non-numeric: an encoding this + // build cannot interpret — refuse over guess. Zero lands here, + // not in the arm above: "0 is newer" would be a false claim. + _ => { + return Err(io::Error::other(format!( + "session format version {} is not one this fleetcom reads \ + (supports {FORMAT_VERSION}); load it with a newer build", + version.dump() + ))); + } + } + } + let (name, dirs, flat) = if parsed["dirs"].is_object() { + let name = parsed["name"].as_str().map(str::to_string); + (name, &parsed["dirs"], false) } else { - (None, &parsed) + (None, &parsed, true) }; let mut cfg = SessionConfig::new(); for (dir, val) in dirs.entries() { + // In a flat file the accepted `version` member sits beside directory + // keys; it is metadata, not a directory. Wrapped iteration reads only + // `dirs`, which never contains it. + if flat && dir == "version" { + continue; + } // Ignore members that match neither supported entry form. let entries = val .members() @@ -361,10 +405,107 @@ mod tests { cfg.insert("~/proj".into(), vec![e("cargo test"), e("vim")]); cfg.insert("/tmp".into(), vec![e("top")]); - let expected = "{\n \"name\": \"work\",\n \"dirs\": {\n \"/tmp\": [\n \"top\"\n ],\n \"~/proj\": [\n \"cargo test\",\n \"vim\"\n ]\n }\n}"; + let expected = "{\n \"version\": 1,\n \"name\": \"work\",\n \"dirs\": {\n \"/tmp\": [\n \"top\"\n ],\n \"~/proj\": [\n \"cargo test\",\n \"vim\"\n ]\n }\n}"; assert_eq!(to_json("work", &cfg), expected); } + /// Saved files carry the format version and load back under the gate. + #[test] + fn save_writes_version_1_and_load_accepts_it() { + let dir = temp("session_version_roundtrip"); + let mut cfg = SessionConfig::new(); + cfg.insert("~/p".into(), vec![e("vim")]); + + let file = save_in(&dir, "versioned", &cfg).unwrap(); + assert!( + fs::read_to_string(&file) + .unwrap() + .contains("\"version\": 1") + ); + assert_eq!(load_in(&dir, "versioned").unwrap(), cfg); + let _ = fs::remove_dir_all(&dir); + } + + /// A wrapped file without the key is version 1: every file written by + /// released fleetcom (0.6.0–0.8.0) predates it. The rule is permanent. + #[test] + fn missing_version_means_version_1() { + let (name, cfg) = from_json(r#"{"name": "old", "dirs": {"~/proj": ["vim"]}}"#).unwrap(); + assert_eq!(name, Some("old".to_string())); + assert_eq!(cfg["~/proj"], vec![e("vim")]); + } + + /// An explicit `"version": 1` passes the gate. + #[test] + fn explicit_version_1_loads() { + let (_, cfg) = + from_json(r#"{"version": 1, "name": "v", "dirs": {"~/proj": ["vim"]}}"#).unwrap(); + assert_eq!(cfg["~/proj"], vec![e("vim")]); + } + + /// A newer format refuses with an error naming both versions rather than + /// dropping unrecognized members and rewriting the file on the next save. + #[test] + fn newer_version_refuses_naming_both_versions() { + let err = from_json(r#"{"version": 2, "name": "v", "dirs": {}}"#).unwrap_err(); + assert_eq!( + err.to_string(), + "session format version 2 is newer than this fleetcom (supports 1); \ + load it with a newer build" + ); + } + + /// Version zero is not "newer" — it takes the unreadable-version message, + /// never the false "0 is newer than this fleetcom" claim. + #[test] + fn version_zero_refuses_as_unreadable_not_newer() { + let err = from_json(r#"{"version": 0, "name": "v", "dirs": {}}"#).unwrap_err(); + assert_eq!( + err.to_string(), + "session format version 0 is not one this fleetcom reads \ + (supports 1); load it with a newer build" + ); + } + + /// A non-numeric version is an encoding this build cannot interpret: + /// refuse over guess. + #[test] + fn non_numeric_version_refuses() { + let err = from_json(r#"{"version": "2.0", "name": "v", "dirs": {}}"#).unwrap_err(); + assert_eq!( + err.to_string(), + "session format version \"2.0\" is not one this fleetcom reads \ + (supports 1); load it with a newer build" + ); + } + + /// A refused load is `Err` from `load_in`: no caller holds a config to + /// resave, so the gate also blocks the lossy rewrite. + #[test] + fn refused_load_yields_err_with_nothing_to_resave() { + let dir = temp("session_version_refuse"); + fs::write( + dir.join("future.json"), + r#"{"version": 3, "name": "future", "dirs": {"~/p": ["vim"]}}"#, + ) + .unwrap(); + + let err = load_in(&dir, "future").unwrap_err(); + assert!(err.to_string().contains("version 3"), "{err}"); + assert!(err.to_string().contains("supports 1"), "{err}"); + let _ = fs::remove_dir_all(&dir); + } + + /// In a flat file an accepted numeric `version` member is metadata, not a + /// directory: it must not materialize as an empty-entry directory. + #[test] + fn flat_version_member_does_not_become_a_directory() { + let (name, cfg) = from_json(r#"{"version": 1, "~/proj": ["vim"]}"#).unwrap(); + assert_eq!(name, None); + assert!(!cfg.contains_key("version")); + assert_eq!(cfg["~/proj"], vec![e("vim")]); + } + /// Malformed members are omitted rather than decoded into partial entries. #[test] fn malformed_object_members_drop_without_error() { From 4214cd14d22f0e36df1dfecd63d4616a577c265e Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Mon, 20 Jul 2026 21:58:55 -0700 Subject: [PATCH 09/10] fix: improve documentation and comments for clarity and accuracy --- docs/sessions.md | 6 ++-- src/app.rs | 4 +-- src/daemon.rs | 83 ++++++++++++++----------------------------- src/harness/assets.rs | 59 ++++++++---------------------- src/main.rs | 26 ++++---------- src/session.rs | 69 +++++++++++++---------------------- src/ui.rs | 25 ++++--------- 7 files changed, 82 insertions(+), 190 deletions(-) diff --git a/docs/sessions.md b/docs/sessions.md index efb0cee..45a9fd4 100644 --- a/docs/sessions.md +++ b/docs/sessions.md @@ -14,7 +14,7 @@ A session is a launch recipe, not a process snapshot. It records commands, worki The first save creates the directory. This matches the [configuration path resolution](README.md#config-directory-sessions) used by save, list, and load. -The filename derives from the session name. `fleetcom` trims leading and trailing whitespace, replaces control characters and any of `* " / \ < > : | ? .` with `_`, and caps the result at 250 bytes: `NAME_MAX` is 255 bytes on common Unix filesystems, and the `.json` extension takes the other 5. The cap falls on a character boundary — a multibyte character that would cross it is dropped whole, never split. `my/session` becomes `my_session.json`, and `a.b` becomes `a_b.json`. Replacing `.` prevents the session name from supplying another extension. +The filename derives from the session name. `fleetcom` trims leading and trailing whitespace, replaces control characters and any of `* " / \ < > : | ? .` with `_`, and limits the sanitized stem to 250 UTF-8 bytes so the `.json` filename fits within 255 bytes. The cap falls on a character boundary. `my/session` becomes `my_session.json`, and `a.b` becomes `a_b.json`. Replacing `.` prevents the session name from supplying another extension. ## Format @@ -37,11 +37,11 @@ A session file is a JSON object with three fields. `version` is the format versi ``` - `name` exists because sanitization collapses distinct session names onto one filename: `a/b` and `a.b` both save to `a_b.json`. Saving compares the stored name against the incoming one and refuses a mismatch with an error naming both sessions. The load picker also displays it, so the list shows `a/b`, not `a_b`. -- Keys under `dirs` are directory paths: each task's working directory. Saves write directories under `$HOME` as `~/…`; other paths stay absolute. On load, `~` expands to `$HOME`, and a relative key resolves against the invocation directory of the client loading the session. +- Keys under `dirs` are directory paths: each task's working directory. Saves write directories under `$HOME` as `~/...`; other paths stay absolute. On load, `~` expands to `$HOME`, and a relative key resolves against the invocation directory of the client loading the session. - Values are ordered lists. A string member is a bare shell command; the object form adds the optional group and display name assigned on load. Order is preserved, and each command runs in its own PTY under that directory. - Directories serialize alphabetically. Command order remains stable within each directory. -Files carry a format version. A missing `version` means 1: every file written before the key existed (releases 0.6.0–0.8.0) is structurally version 1, and that absence rule is permanent. A version newer than the running `fleetcom` supports refuses to load — the error names both versions — instead of dropping the members this build does not recognize and rewriting the file on the next save; a newer build loads it. +The `version` field must be an integer from 1 through the newest format supported by the running `fleetcom`. A missing field means version 1. Invalid or unsupported versions fail to load, and the error reports the file's value and the supported version. The shape, not the version, discriminates the schema. An object-valued `dirs` marks the wrapped form shown above. The loader also accepts a flat map whose top-level keys are directories and whose values are entry arrays. In that form, an array-valued key named `dirs` remains a directory entry, but a top-level `version` member is always the format version, never a directory. Flat-map files list by filename stem because they have no stored name. Saving one writes the wrapped form and permits overwriting it without a stored-name collision check. diff --git a/src/app.rs b/src/app.rs index 0b948ab..afb9bdd 100644 --- a/src/app.rs +++ b/src/app.rs @@ -265,8 +265,8 @@ impl App { Box::new(SocketTransport::from_halves(stream, read, wait_tx)) }); app.daemon_backed = true; - // A `--scrollback` that an already-running daemon never saw gets one - // visible status-line notice instead of a silently kept old value. + // Report when a running daemon could not apply this invocation's + // startup-only scrollback setting. app.status = crate::daemon::ignored_scrollback_notice(origin); Ok(app) } diff --git a/src/daemon.rs b/src/daemon.rs index 04f684e..035cb87 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -200,23 +200,17 @@ fn check_hello_ack(kind: u8, payload: &[u8]) -> io::Result<()> { } } -/// How this client's daemon connection came to exist. The distinction matters -/// because startup-only configuration (`--scrollback`) reaches the daemon -/// solely through `spawn_daemon`'s environment: an `AlreadyRunning` daemon -/// never saw this invocation's flags. +/// Whether this invocation reused a daemon or autostarted one. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum DaemonOrigin { - /// The pre-spawn connect succeeded: a daemon predates this invocation. + /// The initial connection attempt succeeded. AlreadyRunning, - /// This invocation spawned the daemon, which inherited its environment. + /// The connection succeeded after autostarting the daemon. Autostarted, } -/// The dashboard notice for a `--scrollback` the daemon never saw, or `None` -/// when the flag took effect (autostart) or was not passed. Kept beside -/// `DaemonOrigin` because the *why* is daemon lifecycle: the flag rides -/// `spawn_daemon`'s env, so an already-running daemon keeps its resolved -/// depth until `--kill` restarts it. +/// Return a notice when a running daemon cannot apply a startup-only +/// `--scrollback` value. fn scrollback_notice(origin: DaemonOrigin, flag: Option) -> Option { match (origin, flag) { (DaemonOrigin::AlreadyRunning, Some(lines)) => Some(format!( @@ -227,18 +221,16 @@ fn scrollback_notice(origin: DaemonOrigin, flag: Option) -> Option Option { scrollback_notice(origin, supervisor::scrollback_flag()) } /// Connect (autostarting if needed) and complete the hello handshake: send /// this process's protocol version and launch context, require the daemon's -/// ack. Every launch this connection makes then runs under *this* client's -/// env, and a version mismatch surfaces as one actionable error here instead -/// of a silently wrong environment later. Also reports whether this -/// invocation autostarted the daemon, so the caller can flag startup-only -/// options an already-running daemon ignored. +/// ack. Every launch this connection makes runs under *this* client's env. +/// Returns the daemon origin so callers can report ignored startup-only +/// options. /// /// The daemon serves one client at a time, so a slow handshake means "queued /// behind another client", not failure: announce it and wait without a @@ -294,8 +286,7 @@ fn busy_daemon_error(e: io::Error) -> io::Error { /// connection) must not wedge the UI either. A timed-out write drops the /// connection, so a partial frame is never read. pub fn connect_ready_bounded() -> io::Result { - // The origin is dropped: the ignored-`--scrollback` notice is a startup - // concern, shown once by `App::connect`, not repeated on every reconnect. + // Scrollback notices apply only to the initial connection. let (mut stream, _) = connect_or_autostart()?; stream.set_write_timeout(Some(HANDSHAKE_TIMEOUT))?; let (kind, payload) = encode_hello(&LaunchContext::here()); @@ -313,9 +304,7 @@ fn connect_or_autostart() -> io::Result<(UnixStream, DaemonOrigin)> { connect_or_autostart_in(&runtime_dir()) } -/// Validate `dir` before the first daemon connection attempt. The returned -/// origin records which branch produced the stream: the pre-spawn connect -/// succeeding is the one signal that a daemon predates this invocation. +/// Validate `dir`, connect or autostart, and report which path succeeded. fn connect_or_autostart_in(dir: &Path) -> io::Result<(UnixStream, DaemonOrigin)> { // Validate before connecting because the hello sends the client's // environment and a successful connection skips daemon-side validation. @@ -402,8 +391,8 @@ pub fn run_kill() -> io::Result<()> { file.read_to_string(&mut pid_str)?; let Some(pid) = pid_str.trim().parse::().ok().filter(|p| *p > 0) else { // Without a usable pid, fall back to a Shutdown frame over the socket. - // Bounded: an attached client keeps the daemon from ever accepting - // this connection, and `--kill` must terminate regardless. + // Bound the fallback because an attached client can keep the daemon + // from accepting this connection. return kill_via_socket(); }; @@ -430,15 +419,10 @@ pub fn run_kill() -> io::Result<()> { )) } -/// Overall budget for the socket-fallback kill exchange, matching the pid -/// path's 10 s flock poll. Without it the fallback inherits the daemon's -/// one-client-at-a-time serving: with another client attached, the hello -/// reply never comes and `--kill` would block forever, silently. +/// Overall budget for the socket-fallback kill exchange. const KILL_SOCKET_TIMEOUT: Duration = Duration::from_secs(10); -/// The one error the socket-fallback deadline produces. The daemon is -/// demonstrably up (the connect succeeded) but never finished the exchange, -/// which with a healthy daemon means it is serving an attached client. +/// Timeout reported when the socket-fallback kill exchange does not finish. fn kill_handshake_timeout() -> io::Error { io::Error::new( ErrorKind::TimedOut, @@ -448,9 +432,7 @@ fn kill_handshake_timeout() -> io::Error { ) } -/// Arm the stream's read timeout with what is left of `deadline`. Errors out -/// when the budget is already spent, because `set_read_timeout` rejects a -/// zero duration and a stale timeout would silently extend the deadline. +/// Set the read timeout to the remaining deadline budget. fn arm_read_deadline(s: &UnixStream, deadline: Instant) -> io::Result<()> { let left = deadline.saturating_duration_since(Instant::now()); if left.is_zero() { @@ -465,8 +447,7 @@ fn kill_via_socket() -> io::Result<()> { kill_via_socket_at(&socket_path(), KILL_SOCKET_TIMEOUT) } -/// `kill_via_socket` against an explicit socket under an injectable budget -/// (tests use a short one against a mute listener). +/// Run the socket-fallback kill exchange at `path` within `budget`. fn kill_via_socket_at(path: &Path, budget: Duration) -> io::Result<()> { match UnixStream::connect(path) { Ok(mut s) => kill_over_stream(&mut s, budget), @@ -477,10 +458,7 @@ fn kill_via_socket_at(path: &Path, budget: Duration) -> io::Result<()> { } } -/// Drive the Shutdown exchange over a connected stream, every blocking step -/// bounded by one overall deadline. Writes share the budget too: an attached -/// client means our connection sits unaccepted, and while the small frames -/// almost surely fit the send buffer, "almost surely" is not a deadline. +/// Drive the Shutdown exchange with one deadline for all blocking I/O. fn kill_over_stream(s: &mut UnixStream, budget: Duration) -> io::Result<()> { let deadline = Instant::now() + budget; s.set_write_timeout(Some(budget))?; @@ -499,10 +477,8 @@ fn kill_over_stream(s: &mut UnixStream, budget: Duration) -> io::Result<()> { let (kind, payload) = encode_command(&Command::Shutdown); write_frame(s, kind, &payload)?; - // Drain until the daemon closes the socket: its exit is the completion - // signal, mirroring the pid path's flock poll. Each pass re-arms the - // remaining budget so the loop cannot outlive the deadline. Non-timeout - // read errors keep the old meaning — the daemon is gone, kill complete. + // Socket closure signals completion. Re-arm each read with the remaining + // budget so the loop cannot outlive the deadline. let mut buf = [0u8; 256]; loop { arm_read_deadline(s, deadline)?; @@ -924,8 +900,7 @@ mod tests { let _ = fs::remove_dir_all(&base); } - /// The ignored-`--scrollback` decision table: only "flag passed" plus - /// "daemon predates this invocation" produces the notice. + /// The notice requires both a flag and an already-running daemon. #[test] fn scrollback_notice_requires_flag_and_preexisting_daemon() { assert_eq!( @@ -939,16 +914,13 @@ mod tests { assert!(notice.contains("--kill"), "{notice}"); } - /// A daemon that accepts the connection but never answers the hello (the - /// attached-client shape: our connection sits in the backlog) must error - /// out on the deadline, not hang `--kill` forever. + /// A missing hello response times out the kill exchange. #[test] fn kill_via_socket_bounds_the_handshake_wait() { let base = temp("kill_socket_mute"); fs::create_dir_all(&base).unwrap(); let sock = base.join("mute.sock"); - // Never accepted: connect still succeeds via the backlog, exactly like - // a live daemon busy serving another client. + // Leave the connection queued in the listener backlog. let _listener = UnixListener::bind(&sock).unwrap(); let start = Instant::now(); let err = kill_via_socket_at(&sock, Duration::from_millis(200)).unwrap_err(); @@ -961,9 +933,7 @@ mod tests { let _ = fs::remove_dir_all(&base); } - /// The post-Shutdown drain is bounded too: a daemon that acks the hello - /// and swallows `Shutdown` without ever closing the socket must not turn - /// the completion wait into a hang. + /// A daemon that keeps the socket open after Shutdown times out the drain. #[test] fn kill_via_socket_bounds_the_drain_wait() { let base = temp("kill_socket_drain"); @@ -976,8 +946,7 @@ mod tests { let (kind, payload) = encode_event(&Event::HelloOk); let _ = write_frame(&mut s, kind, &payload); let _ = read_frame(&mut s); // Shutdown, swallowed - // Hold the socket open until the client gives up and drops its - // end; this read's EOF is the thread's exit signal. + // Hold the socket open until the client drops its end. let _ = s.read(&mut [0u8; 16]); }); let err = kill_via_socket_at(&sock, Duration::from_millis(300)).unwrap_err(); @@ -986,7 +955,7 @@ mod tests { let _ = fs::remove_dir_all(&base); } - /// No socket means no daemon: the fallback stays a friendly no-op. + /// A missing socket makes the fallback a no-op. #[test] fn kill_via_socket_without_a_socket_is_a_noop() { let base = temp("kill_socket_absent"); diff --git a/src/harness/assets.rs b/src/harness/assets.rs index 1be9f62..0c5ac3f 100644 --- a/src/harness/assets.rs +++ b/src/harness/assets.rs @@ -4,10 +4,8 @@ //! `task--.json` path per task run. The nonce isolates concurrent //! processes and prevents PID reuse from selecting an existing namespace. //! -//! Namespace lifecycle: `Drop` removes this process's own namespace; `install` -//! reaps dead-owner siblings. Drop alone cannot cover a SIGKILLed or OOM-killed -//! supervisor, and the cache-directory fallback root persists across boots, so -//! without the reap those namespaces would accumulate forever. +//! Namespace lifecycle: `Drop` removes this process's namespace, while +//! `install` reaps sibling namespaces whose owner no longer exists. //! //! Asset contracts: //! - `claude`: `--settings ` layers a `SessionStart` hook @@ -84,17 +82,7 @@ pub fn runtime_root(override_dir: Option<&Path>) -> Option { dirs::cache_dir().map(|c| c.join("fleetcom").join("run")) } -/// Remove sibling namespaces whose owning supervisor is dead. Drop-only -/// cleanup leaks the namespace of any supervisor that dies without unwinding -/// (SIGKILL, OOM kill), so each install sweeps the root's immediate entries -/// and removes every directory named `-` whose pid probes dead. -/// Removal is best-effort per entry: a failed reap never fails `install`. -/// -/// One reachable corner: a TERM-refusing reparented child of a dead supervisor -/// may still hold `FLEETCOM_CAPTURE_FILE` pointing into a reaped namespace. -/// Its eventual write fails on the missing path, which is harmless — the codex -/// notify script continues to its chain, and the claude hook errors -/// cosmetically. +/// Best-effort removal of namespace directories whose owner no longer exists. fn reap_dead_namespaces(root: &Path) { let Ok(entries) = fs::read_dir(root) else { return; @@ -115,11 +103,7 @@ fn reap_dead_namespaces(root: &Path) { } } -/// Parse a namespace name into its owner pid. The match is strict — split on -/// the first `-`, the pid part is all decimal digits parsing as `i32 > 0` -/// (parse failure covers overflow past `i32::MAX`), and the nonce part is -/// exactly 12 lowercase hex chars. Anything else is not a namespace and must -/// be kept: root-level legacy files and foreign layouts pass through here. +/// Parse `-<12 lowercase hex characters>`. fn namespace_owner(name: &str) -> Option { let (pid, nonce) = name.split_once('-')?; if pid.is_empty() || !pid.bytes().all(|b| b.is_ascii_digit()) { @@ -135,11 +119,7 @@ fn namespace_owner(name: &str) -> Option { pid.parse::().ok().filter(|p| *p > 0) } -/// Whether `pid` provably names no live process. The probe's errors are -/// one-sided by design: only `ESRCH` reads as dead. `Ok` and `EPERM` read as -/// alive (or not ours to judge), and any other errno keeps the entry. A -/// recycled pid therefore misreads only as "alive" — the reaper can keep -/// garbage but can never delete a live supervisor's namespace. +/// Return true only when signal 0 reports that `pid` does not exist. fn owner_is_dead(pid: i32) -> bool { use nix::{errno::Errno, sys::signal::kill, unistd::Pid}; matches!(kill(Pid::from_raw(pid), None), Err(Errno::ESRCH)) @@ -157,9 +137,8 @@ pub struct CaptureAssets { impl CaptureAssets { /// Create `root` and a private `/-` namespace. The /// namespace uses mode `0700`; its Claude settings use `0600`, and its - /// executable Codex notifier uses `0700`. Existing root entries remain - /// unchanged except sibling namespaces whose owner is provably dead: those - /// are reaped best-effort before the new namespace is minted. + /// executable Codex notifier uses `0700`. Dead-owner namespaces are reaped + /// before the new namespace is created; other root entries remain. pub fn install(root: &Path, pid: u32) -> io::Result { fs::DirBuilder::new() .recursive(true) @@ -313,9 +292,7 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// Installation leaves live-owner namespaces and every non-namespace root - /// entry unchanged. Pid 1 is always alive and never ours to signal, so the - /// probe reads it as alive on both its `Ok` and `EPERM` outcomes. + /// Installation retains live-owner namespaces and non-namespace entries. #[test] fn install_never_deletes_live_owner_namespaces_or_legacy_files() { let root = temp("assets_retain"); @@ -349,9 +326,7 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// A matching PID prefix does not cause an existing namespace to be reused - /// or reaped: the planted namespace carries this test's own live pid, so - /// the liveness probe keeps it and the nonce forces a distinct directory. + /// A live matching PID retains its namespace and receives a distinct nonce. #[test] fn install_after_pid_reuse_leaves_the_predecessor_namespace_alone() { let root = temp("assets_reuse"); @@ -390,9 +365,7 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// Spawn and wait a trivial child, returning its now-dead pid. Waiting - /// reaps the zombie — a zombie still accepts signal 0 and would probe as - /// alive. The tiny window for pid reuse before the probe is accepted. + /// Spawn and reap a child, then return its inactive PID. fn dead_pid() -> u32 { let mut child = Command::new("sh").arg("-c").arg("exit 0").spawn().unwrap(); let pid = child.id(); @@ -400,8 +373,7 @@ mod tests { pid } - /// A namespace directory whose owner pid probes dead is reaped at - /// install, contents included. + /// Installation removes a dead owner's namespace and its contents. #[test] fn install_reaps_a_dead_owner_namespace() { let root = temp("assets_reap"); @@ -415,8 +387,7 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// Only directories are namespaces: a root-level file named like a dead - /// owner's namespace is kept. + /// A namespace-shaped file is not reaped. #[test] fn install_keeps_a_file_named_like_a_dead_namespace() { let root = temp("assets_reap_file"); @@ -429,8 +400,7 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// Names outside the strict `-<12 lowercase hex>` shape are - /// never probed or reaped, whatever their pid part would mean. + /// Malformed namespace names are not reaped. #[test] fn install_keeps_directories_with_malformed_namespace_names() { let root = temp("assets_reap_malformed"); @@ -612,8 +582,7 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// Drop removes only the owned namespace and its contents. The sibling - /// carries pid 1 so `install`'s reap keeps it: the test isolates Drop. + /// Drop removes only the owned namespace and its contents. #[test] fn drop_removes_only_the_incarnation_namespace() { let root = temp("assets_drop"); diff --git a/src/main.rs b/src/main.rs index cf32b5d..93ce9c4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -153,29 +153,20 @@ fn parse_args(args: &[String]) -> Result { } } -/// The single stderr shape for a fatal error: the `fleetcom: ` prefix plus -/// the error's `Display` form. Every terminal error goes through this or -/// prints the same prefix by hand with added context (the daemon-connect -/// failure); nothing reaches the runtime's `Debug` handler. +/// Format a fatal error with the program prefix and its `Display` form. fn error_line(e: impl std::fmt::Display) -> String { format!("fleetcom: {e}") } fn main() { - // Matching here instead of returning `io::Result` from `main` is the whole - // point: the runtime's default handler `Debug`-prints an `Err` — raw - // struct noise, no prefix. Every error that propagates this far becomes - // one prefixed, human-readable stderr line with a deliberate exit code. + // Present propagated errors consistently and exit with failure. if let Err(e) = run() { eprintln!("{}", error_line(&e)); std::process::exit(1); } } -/// The whole program, minus error presentation: `main` owns how an `Err` -/// prints and exits. Paths with a more specific report or exit code -/// (usage errors, the daemon-connect failure, the tty guard) print and -/// `exit` directly rather than flattening into the generic exit-1 line. +/// Run the program; `main` presents errors returned from this boundary. fn run() -> io::Result<()> { let args: Vec = std::env::args().skip(1).collect(); let (foreground, session, scrollback) = match parse_args(&args) { @@ -203,12 +194,8 @@ fn run() -> io::Result<()> { } }; - // Refuse a redirected stdout before any terminal setup: crossterm falls - // back to /dev/tty for the control sequences, so `fleetcom > file` would - // leave the user's real terminal raw and blank while frames stream into - // the file. Client path only — `--daemon` is deliberately headless, and - // help/version/`--kill` never touch the terminal. No degraded mode: - // refusing loudly beats rendering into a pipe. + // The interactive client requires stdout for terminal frames. Headless + // and informational modes return before this check. if !io::stdout().is_terminal() { eprintln!("{}", error_line("stdout is not a terminal")); std::process::exit(1); @@ -453,8 +440,7 @@ mod tests { assert!(parse(&["one", "two"]).is_err()); } - /// The boundary contract: fatal errors print the `Display` form behind - /// the `fleetcom: ` prefix — never `Debug`'s struct noise. + /// Fatal error lines use the program prefix and `Display` representation. #[test] fn error_line_prefixes_display_form() { let e = io::Error::new(io::ErrorKind::TimedOut, "daemon did not exit after SIGTERM"); diff --git a/src/session.rs b/src/session.rs index b3fcb92..0735b19 100644 --- a/src/session.rs +++ b/src/session.rs @@ -28,12 +28,11 @@ pub const FLEETCOM_CONFIG_DIR: &str = "FLEETCOM_CONFIG_DIR"; /// Characters replaced with `_` in session filenames. const DISALLOWED: &[char] = &['*', '"', '/', '\\', '<', '>', ':', '|', '?', '.']; -/// Byte cap for sanitized names: `NAME_MAX` (255 bytes) common to Unix -/// filesystems minus the 5-byte `.json` extension `save_in` appends. +/// Sanitized-stem cap that reserves 5 bytes for `.json` in a 255-byte +/// filename component. const MAX_STEM_BYTES: usize = 250; -/// Make `name` safe as a bare filename. The cap counts encoded bytes, not -/// chars: a char that would cross [`MAX_STEM_BYTES`] drops whole, never split. +/// Sanitize `name` and limit its UTF-8 encoding without splitting a character. fn sanitize(name: &str) -> String { let mut out = String::new(); for c in name.trim().chars() { @@ -72,17 +71,12 @@ pub fn sessions_dir(root: Option) -> Option { .map(|base| base.join("sessions")) } -/// On-disk session format version: written by `to_json`, the newest -/// `from_json` accepts. The contract: any change an older reader would decode -/// lossily bumps this number, and a reader refuses versions above its own -/// rather than dropping what it does not recognize and rewriting the file on -/// the next save. A missing `version` key means 1 — every file written by -/// released fleetcom (0.6.0–0.8.0) predates the key and is structurally v1 — -/// and that absence rule is permanent. +/// Session format version written by `to_json` and accepted by `from_json`. +/// Missing versions are interpreted as version 1; unsupported versions fail. const FORMAT_VERSION: u64 = 1; -/// Serialize `{"name": , "dirs": {...}}`. The stored name lets -/// `save_in` distinguish names that sanitize to the same filename. +/// Serialize the versioned wrapped schema. The stored name distinguishes +/// names that sanitize to the same filename. fn to_json(name: &str, cfg: &SessionConfig) -> String { let mut dirs = jzon::JsonValue::new_object(); for (dir, entries) in cfg { @@ -116,13 +110,11 @@ fn to_json(name: &str, cfg: &SessionConfig) -> String { /// Parse wrapped and flat schemas, returning the stored name when present. /// A wrapped file has an object-valued `dirs`; flat files have entry arrays at /// the top level, including when a directory is literally named `dirs`. -/// A top-level `version` above [`FORMAT_VERSION`] refuses to load: this -/// build would drop the members it does not recognize, and the next save -/// would rewrite the file without them. +/// A top-level `version` must be an integer from 1 through [`FORMAT_VERSION`]; +/// a missing version is interpreted as 1. fn from_json(text: &str) -> io::Result<(Option, SessionConfig)> { let parsed = jzon::parse(text).map_err(|e| io::Error::other(e.to_string()))?; - // Gate before schema detection. Absence means version 1: every file - // written by released fleetcom (0.6.0–0.8.0) predates the key. + // Validate version metadata before detecting the schema shape. let version = &parsed["version"]; if !version.is_null() { match version.as_u64() { @@ -133,9 +125,7 @@ fn from_json(text: &str) -> io::Result<(Option, SessionConfig)> { (supports {FORMAT_VERSION}); load it with a newer build" ))); } - // Zero, fractional, negative, or non-numeric: an encoding this - // build cannot interpret — refuse over guess. Zero lands here, - // not in the arm above: "0 is newer" would be a false claim. + // Reject zero, fractional, negative, and non-numeric values. _ => { return Err(io::Error::other(format!( "session format version {} is not one this fleetcom reads \ @@ -153,9 +143,7 @@ fn from_json(text: &str) -> io::Result<(Option, SessionConfig)> { }; let mut cfg = SessionConfig::new(); for (dir, val) in dirs.entries() { - // In a flat file the accepted `version` member sits beside directory - // keys; it is metadata, not a directory. Wrapped iteration reads only - // `dirs`, which never contains it. + // In a flat file, `version` is metadata beside the directory keys. if flat && dir == "version" { continue; } @@ -235,9 +223,8 @@ pub fn save_in(dir: &Path, name: &str, cfg: &SessionConfig) -> io::Result Duration { } /// Split a task row into its leading, preview, and time cells so the preview -/// can be styled independently. Cells are column-exact: `pad` measures -/// terminal columns, so a wide-glyph title or preview (CJK, emoji) fills its -/// budget instead of overflowing it, and the three widths always sum to `cols`. +/// can be styled independently. Each cell is padded to its display-column +/// budget, and the budgets sum to `cols`. fn task_row_parts(v: &TaskView, cols: usize) -> (String, String, String) { let glyph = match v.lifecycle { Lifecycle::Active => "✻", @@ -355,10 +354,7 @@ fn task_row(v: &TaskView, cols: usize) -> String { /// Paint a task row with only its padded preview cell dimmed. fn dim_preview_row(out: &mut impl Write, y: u16, v: &TaskView, cols: usize) -> io::Result<()> { - // Queue the three column-exact cells directly rather than re-splitting a - // composed string: a char-count split desynchronizes from cell boundaries - // as soon as a cell holds wide glyphs. Clamping each cell to the columns - // still open reproduces `put`'s pad-to-`cols` behavior on narrow terminals. + // Clamp each styled cell to the display columns still available. let (lead, preview, time) = task_row_parts(v, cols); let lead = truncate(&lead, cols); let mut rem = cols - lead.width(); @@ -375,9 +371,7 @@ fn dim_preview_row(out: &mut impl Write, y: u16, v: &TaskView, cols: usize) -> i ) } -/// Peek-box top border: `─ label ` extended with `─` fill to exactly -/// `inner_w` columns. The label is measured in display columns — wide glyphs -/// consume two — so the fill always meets the corner. +/// Build a labeled peek-box border exactly `inner_w` display columns wide. fn peek_top_border(label: &str, inner_w: usize) -> String { let mut border = format!("─ {} ", truncate(label, inner_w.saturating_sub(4))); let w = border.width(); @@ -804,9 +798,7 @@ mod tests { ); } - /// Every cell is padded in display columns, so the composed row is - /// exactly `cols` wide and the time cell survives at the right edge for - /// any title/preview content: CJK, emoji, combining marks. + /// Rows remain column-exact with wide and combining glyphs. #[test] fn task_row_is_column_exact_for_wide_glyphs() { let titles = [ @@ -838,9 +830,7 @@ mod tests { } } - /// Cell budgets are fixed by `cols`, not by content: `dim_preview_row` - /// queues the cells separately, so the dim run must cover exactly the - /// preview cell whatever glyphs the cells hold. + /// Row-cell display widths depend on `cols`, not their contents. #[test] fn task_row_cells_hold_their_column_budgets() { let cols = 72; @@ -856,8 +846,7 @@ mod tests { assert_eq!(wl.width() + wp.width() + wt.width(), cols); } - /// The peek top border fills to exactly the inner width for any label, - /// including wide glyphs and labels longer than the border. + /// Peek borders remain column-exact for wide and overlong labels. #[test] fn peek_top_border_fills_to_inner_width() { for label in ["cargo test", "日本語のテスト", "🚀 build", "e\u{0301}", ""] { From 93955055c46b9a0173827cf9e3adfdc9da845e8c Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Mon, 20 Jul 2026 22:24:40 -0700 Subject: [PATCH 10/10] fix: map kill-exchange write timeouts; partition drain-loop read errors --- src/daemon.rs | 70 ++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 14 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index 035cb87..948213e 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -419,7 +419,7 @@ pub fn run_kill() -> io::Result<()> { )) } -/// Overall budget for the socket-fallback kill exchange. +/// Timeout applied to blocking socket-fallback kill operations. const KILL_SOCKET_TIMEOUT: Duration = Duration::from_secs(10); /// Timeout reported when the socket-fallback kill exchange does not finish. @@ -441,13 +441,23 @@ fn arm_read_deadline(s: &UnixStream, deadline: Instant) -> io::Result<()> { s.set_read_timeout(Some(left)) } +/// Convert either platform representation of a socket timeout into the +/// kill-handshake timeout. +fn deadline_mapped(e: io::Error) -> io::Error { + if is_timeout(&e) { + kill_handshake_timeout() + } else { + e + } +} + /// Send `Shutdown` when the lock file has no usable pid. Complete the handshake /// first, then wait for the daemon to close the socket after stopping its tasks. fn kill_via_socket() -> io::Result<()> { kill_via_socket_at(&socket_path(), KILL_SOCKET_TIMEOUT) } -/// Run the socket-fallback kill exchange at `path` within `budget`. +/// Run the socket-fallback kill exchange at `path` using `budget` for I/O. fn kill_via_socket_at(path: &Path, budget: Duration) -> io::Result<()> { match UnixStream::connect(path) { Ok(mut s) => kill_over_stream(&mut s, budget), @@ -458,25 +468,29 @@ fn kill_via_socket_at(path: &Path, budget: Duration) -> io::Result<()> { } } -/// Drive the Shutdown exchange with one deadline for all blocking I/O. +/// Drive the Shutdown exchange with bounded writes and a shared read deadline. fn kill_over_stream(s: &mut UnixStream, budget: Duration) -> io::Result<()> { + let (kind, payload) = encode_hello(&LaunchContext::here()); + kill_exchange(s, budget, kind, &payload) +} + +/// Drive the bounded Shutdown exchange with a pre-encoded hello frame. +fn kill_exchange( + s: &mut UnixStream, + budget: Duration, + hello_kind: u8, + hello_payload: &[u8], +) -> io::Result<()> { let deadline = Instant::now() + budget; s.set_write_timeout(Some(budget))?; - let (kind, payload) = encode_hello(&LaunchContext::here()); - write_frame(s, kind, &payload)?; + write_frame(s, hello_kind, hello_payload).map_err(deadline_mapped)?; arm_read_deadline(s, deadline)?; - let (kind, payload) = read_frame(s).map_err(|e| { - if is_timeout(&e) { - kill_handshake_timeout() - } else { - e - } - })?; + let (kind, payload) = read_frame(s).map_err(deadline_mapped)?; check_hello_ack(kind, &payload)?; let (kind, payload) = encode_command(&Command::Shutdown); - write_frame(s, kind, &payload)?; + write_frame(s, kind, &payload).map_err(deadline_mapped)?; // Socket closure signals completion. Re-arm each read with the remaining // budget so the loop cannot outlive the deadline. let mut buf = [0u8; 256]; @@ -486,7 +500,19 @@ fn kill_over_stream(s: &mut UnixStream, budget: Duration) -> io::Result<()> { Ok(0) => return Ok(()), Ok(_) => {} Err(e) if is_timeout(&e) => return Err(kill_handshake_timeout()), - Err(_) => return Ok(()), + // Retry interrupted reads; the deadline still bounds the loop. + Err(e) if e.kind() == ErrorKind::Interrupted => {} + // The daemon closing mid-drain is completion, same as `Ok(0)`. + Err(e) + if matches!( + e.kind(), + ErrorKind::ConnectionReset | ErrorKind::BrokenPipe | ErrorKind::UnexpectedEof + ) => + { + return Ok(()); + } + // Propagate other errors because they do not confirm daemon exit. + Err(e) => return Err(e), } } } @@ -933,6 +959,22 @@ mod tests { let _ = fs::remove_dir_all(&base); } + /// A blocked hello write reports the kill-handshake timeout. + #[test] + fn kill_exchange_maps_a_write_timeout() { + let base = temp("kill_socket_bigenv"); + fs::create_dir_all(&base).unwrap(); + let sock = base.join("mute.sock"); + let _listener = UnixListener::bind(&sock).unwrap(); + let mut s = UnixStream::connect(&sock).unwrap(); + // The listener never accepts, so this payload fills the send buffer. + let oversized = vec![0u8; 8 * 1024 * 1024]; + let err = kill_exchange(&mut s, Duration::from_millis(200), 0, &oversized).unwrap_err(); + assert_eq!(err.kind(), ErrorKind::TimedOut); + assert!(err.to_string().contains("kill handshake"), "{err}"); + let _ = fs::remove_dir_all(&base); + } + /// A daemon that keeps the socket open after Shutdown times out the drain. #[test] fn kill_via_socket_bounds_the_drain_wait() {