From 57108f7d3c20a01d2e6d00280ba020b77205389c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 2 Apr 2026 10:45:37 -0700 Subject: [PATCH] Remove unnecessary comments, use bail! consistently in server start - Delete WHAT comments that restate the code - Replace eprintln! + process::exit(1) with bail! in daemon "already running" path for consistency with foreground mode Co-Authored-By: Claude Opus 4.6 (1M context) --- .../fabro-cli/src/commands/server/foreground.rs | 2 -- .../fabro-cli/src/commands/server/start.rs | 17 +++-------------- .../fabro-cli/src/commands/server/stop.rs | 5 ----- 3 files changed, 3 insertions(+), 21 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/server/foreground.rs b/lib/crates/fabro-cli/src/commands/server/foreground.rs index 009c01dbf..fe30837f0 100644 --- a/lib/crates/fabro-cli/src/commands/server/foreground.rs +++ b/lib/crates/fabro-cli/src/commands/server/foreground.rs @@ -18,14 +18,12 @@ pub(crate) async fn execute( let _ = fabro_proc::title_init(); fabro_proc::title_set(&format!("fabro: server {bind}")); - // Ensure serve_args carries the resolved bind so the server binds correctly. serve_args.bind = Some(bind.to_string()); let _record_guard = scopeguard::guard(record_path, |path| { record::remove_server_record(&path); }); - // If Unix socket, clean up socket file on exit let _socket_guard = if let Bind::Unix(ref path) = bind { let path = path.clone(); Some(scopeguard::guard(path, |p| { diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index 085732728..70fbc05c1 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -18,8 +18,6 @@ pub(crate) async fn execute( storage_dir: PathBuf, styles: &'static Styles, ) -> Result<()> { - // Ensure serve_args carries the resolved bind address so the server knows - // what to listen on. serve_args.bind = Some(bind.to_string()); if foreground { @@ -65,7 +63,6 @@ async fn execute_foreground( record::remove_server_record(&path); }); - // Clean up Unix socket on exit let _socket_guard = if let Bind::Unix(ref path) = bind { let path = path.clone(); Some(scopeguard::guard(path, |p| { @@ -87,11 +84,11 @@ fn execute_daemon(bind: &Bind, serve_args: &ServeArgs, storage_dir: &Path) -> Re let _lock_file = lock_file; // keep alive until function returns if let Some(existing) = record::active_server_record(storage_dir) { - eprintln!( + bail!( "Server already running (pid {}) on {}", - existing.pid, existing.bind + existing.pid, + existing.bind ); - std::process::exit(1); } // Rotate logs @@ -99,7 +96,6 @@ fn execute_daemon(bind: &Bind, serve_args: &ServeArgs, storage_dir: &Path) -> Re let prev_path = log_path.with_extension("log.prev"); let _ = std::fs::rename(&log_path, &prev_path); - // Spawn child: `fabro server __serve --record-path --bind ...` let record_path = record::server_record_path(storage_dir); let log_file = std::fs::File::create(&log_path)?; let stdout_log = log_file.try_clone()?; @@ -112,7 +108,6 @@ fn execute_daemon(bind: &Bind, serve_args: &ServeArgs, storage_dir: &Path) -> Re .arg("--bind") .arg(bind.to_string()); - // Forward optional serve args if let Some(ref model) = serve_args.model { cmd.args(["--model", model]); } @@ -132,7 +127,6 @@ fn execute_daemon(bind: &Bind, serve_args: &ServeArgs, storage_dir: &Path) -> Re cmd.arg("--config").arg(config); } - // Forward global --storage-dir cmd.arg("--storage-dir").arg(storage_dir); cmd.env_remove("FABRO_JSON"); @@ -145,7 +139,6 @@ fn execute_daemon(bind: &Bind, serve_args: &ServeArgs, storage_dir: &Path) -> Re let mut child = cmd.spawn()?; - // Write server record with child PID record::write_server_record( &record_path, &record::ServerRecord { @@ -156,7 +149,6 @@ fn execute_daemon(bind: &Bind, serve_args: &ServeArgs, storage_dir: &Path) -> Re }, )?; - // Check if the child already exited if let Ok(Some(status)) = child.try_wait() { record::remove_server_record(&record_path); let tail = read_log_tail(&log_path, 20); @@ -166,7 +158,6 @@ fn execute_daemon(bind: &Bind, serve_args: &ServeArgs, storage_dir: &Path) -> Re bail!("Server exited immediately with status {status}"); } - // Poll-connect until the server is ready let poll_interval = Duration::from_millis(50); let timeout = Duration::from_secs(5); let mut elapsed = Duration::ZERO; @@ -177,7 +168,6 @@ fn execute_daemon(bind: &Bind, serve_args: &ServeArgs, storage_dir: &Path) -> Re return Ok(()); } - // Check that the child hasn't died while we poll if let Ok(Some(status)) = child.try_wait() { record::remove_server_record(&record_path); if let Bind::Unix(ref path) = *bind { @@ -194,7 +184,6 @@ fn execute_daemon(bind: &Bind, serve_args: &ServeArgs, storage_dir: &Path) -> Re elapsed += poll_interval; } - // Timed out waiting for connection record::remove_server_record(&record_path); if let Bind::Unix(ref path) = *bind { let _ = std::fs::remove_file(path); diff --git a/lib/crates/fabro-cli/src/commands/server/stop.rs b/lib/crates/fabro-cli/src/commands/server/stop.rs index 6815adbed..a26506b43 100644 --- a/lib/crates/fabro-cli/src/commands/server/stop.rs +++ b/lib/crates/fabro-cli/src/commands/server/stop.rs @@ -14,7 +14,6 @@ pub(crate) fn execute(storage_dir: &Path, timeout: Duration) { fabro_proc::sigterm(record.pid); - // Poll for process exit let poll_interval = Duration::from_millis(100); let mut elapsed = Duration::ZERO; while elapsed < timeout { @@ -25,18 +24,14 @@ pub(crate) fn execute(storage_dir: &Path, timeout: Duration) { elapsed += poll_interval; } - // Escalate to SIGKILL if still alive if fabro_proc::process_alive(record.pid) { fabro_proc::sigkill(record.pid); - // Brief wait for SIGKILL to take effect thread::sleep(Duration::from_millis(100)); } - // Clean up record file let record_path = record::server_record_path(storage_dir); record::remove_server_record(&record_path); - // Clean up Unix socket file if applicable if let Bind::Unix(ref path) = record.bind { let _ = std::fs::remove_file(path); }