Skip to content

Commit f99647b

Browse files
authored
fix: report a clear cli error when no herdr server is running (#1963)
* fix: report a clear cli error when no herdr server is running socket cli commands surfaced a raw io::Error debug string (`Error: Os { code: 2, ... }`) when nothing was listening on the api socket, which read like a bad --cwd path. map dead-socket connect failures to a `server_not_running` json error carrying the resolved socket path, printed once at the edge that surfaces the error; recovering callers (plugin offline registry fallback, agent start polling) recognize the marker and keep their existing behavior. refs #1941 * fix: map dead-socket errors on the unchecked cli request path refs #1941 * fix: make server-not-running guidance session-aware refs #1941
1 parent 26a7bc8 commit f99647b

7 files changed

Lines changed: 255 additions & 20 deletions

File tree

src/cli.rs

Lines changed: 89 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ mod plugin;
1818
mod protocol_guard;
1919
mod runtime;
2020
mod server;
21+
mod server_not_running;
2122
mod spec;
2223
mod status;
2324
mod tab;
@@ -745,17 +746,20 @@ pub(super) fn send_request(request: &Request) -> std::io::Result<serde_json::Val
745746
ensure_server_protocol_compatible(&client, &request.id)?;
746747
client
747748
.request_value(request)
748-
.map_err(api_client_error_to_io)
749+
.map_err(|err| map_server_not_running_or_io(err, &request.id, &client))
749750
}
750751

751752
pub(super) fn send_request_unchecked(request: &Request) -> std::io::Result<serde_json::Value> {
752-
ApiClient::local()
753+
let client = ApiClient::local();
754+
client
753755
.request_value(request)
754-
.map_err(api_client_error_to_io)
756+
.map_err(|err| map_server_not_running_or_io(err, &request.id, &client))
755757
}
756758

757759
fn ensure_server_protocol_compatible(client: &ApiClient, request_id: &str) -> std::io::Result<()> {
758-
let status = client.status().map_err(api_client_error_to_io)?;
760+
let status = client
761+
.status()
762+
.map_err(|err| map_server_not_running_or_io(err, request_id, client))?;
759763
let server_protocol = status
760764
.protocol
761765
.ok_or_else(|| std::io::Error::other("server ping did not include a protocol version"))?;
@@ -778,6 +782,49 @@ pub(crate) fn protocol_mismatch_was_reported(err: &std::io::Error) -> bool {
778782
protocol_guard::was_reported(err)
779783
}
780784

785+
pub(crate) fn server_not_running_was_reported(err: &std::io::Error) -> bool {
786+
server_not_running::was_reported(err)
787+
}
788+
789+
/// Returns the `ErrorResponse` carried by a `server_not_running` marker, if any,
790+
/// so the edge that surfaces the error can print it exactly once (deferred
791+
/// printing: recovering callers like plugin offline fallback print nothing).
792+
pub(crate) fn server_not_running_reported_response(
793+
err: &std::io::Error,
794+
) -> Option<&crate::api::schema::ErrorResponse> {
795+
server_not_running::reported_response(err)
796+
}
797+
798+
/// True when an io::Error indicates nothing is listening on the API socket.
799+
/// Classify by `ErrorKind` only: Windows named pipes surface different raw
800+
/// errno values than Unix domain sockets but the same error kinds.
801+
pub(super) fn server_not_running_error(err: &std::io::Error) -> bool {
802+
matches!(
803+
err.kind(),
804+
std::io::ErrorKind::NotFound | std::io::ErrorKind::ConnectionRefused
805+
)
806+
}
807+
808+
/// Maps an `ApiClientError` from a socket command into the io::Error that
809+
/// bubbles up to `main`. A dead-server connect failure is reported as a
810+
/// friendly `server_not_running` JSON error plus a recognizable marker; all
811+
/// other errors fall through unchanged so existing handling is preserved.
812+
fn map_server_not_running_or_io(
813+
err: ApiClientError,
814+
request_id: &str,
815+
client: &ApiClient,
816+
) -> std::io::Error {
817+
match err {
818+
ApiClientError::Io(io_err) if server_not_running_error(&io_err) => {
819+
server_not_running::reported_error(server_not_running::response(
820+
request_id,
821+
&client.socket_path(),
822+
))
823+
}
824+
err => api_client_error_to_io(err),
825+
}
826+
}
827+
781828
fn api_client_error_to_io(err: ApiClientError) -> std::io::Error {
782829
match err {
783830
ApiClientError::Io(err) => err,
@@ -1036,4 +1083,42 @@ mod tests {
10361083
"env must use KEY=VALUE"
10371084
);
10381085
}
1086+
1087+
#[test]
1088+
fn maps_dead_server_connect_failure_to_friendly_error() {
1089+
use crate::api::client::{ApiClient, ApiClientError};
1090+
1091+
let client = ApiClient::local();
1092+
let socket = client.socket_path().display().to_string();
1093+
1094+
// The helper does NOT print; it returns a recognizable marker carrying
1095+
// the ErrorResponse so the surfacing edge can print it exactly once.
1096+
let mapped = super::map_server_not_running_or_io(
1097+
ApiClientError::Io(std::io::Error::from(std::io::ErrorKind::NotFound)),
1098+
"cli:workspace:create",
1099+
&client,
1100+
);
1101+
1102+
let response = super::server_not_running::reported_response(&mapped)
1103+
.expect("dead-server connect failure should carry a server_not_running response");
1104+
assert_eq!(response.id, "cli:workspace:create");
1105+
assert_eq!(response.error.code, "server_not_running");
1106+
assert!(response.error.message.contains(&socket));
1107+
1108+
// The mapping is recognizable without string matching.
1109+
assert!(super::server_not_running::was_reported(&mapped));
1110+
}
1111+
1112+
#[test]
1113+
fn classifier_ignores_unrelated_io_kinds() {
1114+
use crate::api::client::{ApiClient, ApiClientError};
1115+
1116+
let client = ApiClient::local();
1117+
let mapped = super::map_server_not_running_or_io(
1118+
ApiClientError::Io(std::io::Error::from(std::io::ErrorKind::TimedOut)),
1119+
"cli:workspace:create",
1120+
&client,
1121+
);
1122+
assert!(!super::server_not_running::was_reported(&mapped));
1123+
}
10391124
}

src/cli/agent.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,13 @@ fn print_agent_transport_error(
577577
if super::protocol_mismatch_was_reported(&err) {
578578
return Ok(1);
579579
}
580+
// A dead-server marker reaches here from `send_request` in the agent
581+
// startup path; surface its deferred response exactly once instead of
582+
// printing a second, generic transport-error line.
583+
if let Some(response) = super::server_not_running_reported_response(&err) {
584+
let value = serde_json::to_value(response).map_err(std::io::Error::other)?;
585+
return super::print_response(&value);
586+
}
580587
super::print_response(&cli_agent_error(request_id, code, err.to_string()))
581588
}
582589

src/cli/plugin.rs

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1618,14 +1618,19 @@ fn current_unix_ms() -> u64 {
16181618
}
16191619

16201620
fn is_connection_error(err: &std::io::Error) -> bool {
1621-
matches!(
1622-
err.kind(),
1623-
std::io::ErrorKind::NotFound
1624-
| std::io::ErrorKind::ConnectionRefused
1625-
| std::io::ErrorKind::ConnectionAborted
1626-
| std::io::ErrorKind::ConnectionReset
1627-
| std::io::ErrorKind::BrokenPipe
1628-
)
1621+
// A `server_not_running` marker is a connect failure for recovery purposes:
1622+
// treating it as a connection error lets plugin commands fall back to the
1623+
// offline registry. The marker carries (but does not print) a friendly
1624+
// response, so recovering here prints nothing.
1625+
super::server_not_running_was_reported(err)
1626+
|| matches!(
1627+
err.kind(),
1628+
std::io::ErrorKind::NotFound
1629+
| std::io::ErrorKind::ConnectionRefused
1630+
| std::io::ErrorKind::ConnectionAborted
1631+
| std::io::ErrorKind::ConnectionReset
1632+
| std::io::ErrorKind::BrokenPipe
1633+
)
16291634
}
16301635

16311636
fn print_plugin_response(method: Method) -> std::io::Result<i32> {

src/cli/server_not_running.rs

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
use std::fmt;
2+
use std::path::Path;
3+
4+
use crate::api::schema::{ErrorBody, ErrorResponse};
5+
6+
/// Marker error signalling a dead API socket. Carries the `ErrorResponse` that
7+
/// should be printed at the edge that finally surfaces the error, so callers
8+
/// that recover (e.g. plugin offline fallback) print nothing. Mirrors
9+
/// `ProtocolMismatchReported`, except printing is deferred because several CLI
10+
/// commands recover from a dead server instead of reporting it.
11+
#[derive(Debug)]
12+
pub(super) struct ServerNotRunningReported {
13+
pub(super) response: ErrorResponse,
14+
}
15+
16+
impl fmt::Display for ServerNotRunningReported {
17+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
18+
// Delegate to the carried message so pre-existing paths that
19+
// stringify transport errors still show the actionable text.
20+
f.write_str(&self.response.error.message)
21+
}
22+
}
23+
24+
impl std::error::Error for ServerNotRunningReported {}
25+
26+
/// Builds the friendly `server_not_running` ErrorResponse shown when no
27+
/// server is listening on the resolved API socket.
28+
pub(super) fn response(request_id: &str, socket_path: &Path) -> ErrorResponse {
29+
let attach_command = startup_command(socket_path);
30+
ErrorResponse {
31+
id: request_id.to_string(),
32+
error: ErrorBody {
33+
code: "server_not_running".into(),
34+
message: format!(
35+
"no herdr server is running at {}; run `{attach_command}` to start or attach it",
36+
socket_path.display()
37+
),
38+
},
39+
}
40+
}
41+
42+
fn startup_command(socket_path: &Path) -> String {
43+
let session_socket =
44+
crate::session::api_socket_path_for(crate::session::active_name().as_deref());
45+
if socket_path == session_socket {
46+
crate::session::local_attach_command()
47+
} else {
48+
// A socket override wins over an inherited HERDR_SESSION. Keep the
49+
// command in the current environment so it starts the overridden
50+
// target instead of directing the user to an unrelated session.
51+
"herdr".to_string()
52+
}
53+
}
54+
55+
/// Wraps the response in the recognizable marker WITHOUT printing. The caller
56+
/// that ultimately surfaces the error prints the carried response (see
57+
/// `reported_response`); recovering callers simply drop it.
58+
pub(super) fn reported_error(response: ErrorResponse) -> std::io::Error {
59+
std::io::Error::other(ServerNotRunningReported { response })
60+
}
61+
62+
pub(super) fn was_reported(err: &std::io::Error) -> bool {
63+
err.get_ref()
64+
.and_then(|source| source.downcast_ref::<ServerNotRunningReported>())
65+
.is_some()
66+
}
67+
68+
/// Returns the `ErrorResponse` carried by a `server_not_running` marker, if any,
69+
/// so the surfacing edge can print it exactly once.
70+
pub(super) fn reported_response(err: &std::io::Error) -> Option<&ErrorResponse> {
71+
err.get_ref()
72+
.and_then(|source| source.downcast_ref::<ServerNotRunningReported>())
73+
.map(|reported| &reported.response)
74+
}

src/cli/status.rs

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -159,20 +159,13 @@ fn read_server_runtime_status() -> std::io::Result<ServerRuntimeStatus> {
159159
protocol: status.protocol,
160160
capabilities: status.capabilities,
161161
}),
162-
Err(ApiClientError::Io(err)) if server_not_running_error(&err) => {
162+
Err(ApiClientError::Io(err)) if super::server_not_running_error(&err) => {
163163
Ok(ServerRuntimeStatus::NotRunning)
164164
}
165165
Err(err) => Err(api_client_error_to_io(err)),
166166
}
167167
}
168168

169-
fn server_not_running_error(err: &std::io::Error) -> bool {
170-
matches!(
171-
err.kind(),
172-
std::io::ErrorKind::NotFound | std::io::ErrorKind::ConnectionRefused
173-
)
174-
}
175-
176169
fn api_client_error_to_io(err: ApiClientError) -> std::io::Error {
177170
match err {
178171
ApiClientError::Io(err) => err,

src/main.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -497,6 +497,14 @@ fn main() -> io::Result<()> {
497497
Ok(cli::CommandOutcome::Handled(code)) => std::process::exit(code),
498498
Ok(cli::CommandOutcome::NotCli) => {}
499499
Err(err) if cli::protocol_mismatch_was_reported(&err) => std::process::exit(1),
500+
Err(err) if cli::server_not_running_was_reported(&err) => {
501+
if let Some(response) = cli::server_not_running_reported_response(&err) {
502+
if let Ok(json) = serde_json::to_string(response) {
503+
eprintln!("{json}");
504+
}
505+
}
506+
std::process::exit(1);
507+
}
500508
Err(err) => return Err(err),
501509
}
502510

tests/cli/sessions.rs

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,69 @@ fn named_sessions_use_separate_servers_and_workspace_state() {
184184
cleanup_test_base(&base);
185185
}
186186

187+
#[test]
188+
fn dead_server_cli_reports_one_session_aware_json_line() {
189+
fn assert_server_not_running(
190+
output: std::process::Output,
191+
socket_path: &Path,
192+
attach_command: &str,
193+
) {
194+
assert_eq!(
195+
output.status.code(),
196+
Some(1),
197+
"stdout={} stderr={}",
198+
String::from_utf8_lossy(&output.stdout),
199+
String::from_utf8_lossy(&output.stderr)
200+
);
201+
assert!(output.stdout.is_empty(), "server errors belong on stderr");
202+
203+
let stderr = String::from_utf8(output.stderr).unwrap();
204+
let lines: Vec<_> = stderr.lines().collect();
205+
assert_eq!(lines.len(), 1, "expected exactly one JSON line: {stderr:?}");
206+
207+
let response: serde_json::Value = serde_json::from_str(lines[0]).unwrap();
208+
assert_eq!(response["id"], "cli:workspace:create");
209+
assert_eq!(response["error"]["code"], "server_not_running");
210+
assert_eq!(
211+
response["error"]["message"],
212+
format!(
213+
"no herdr server is running at {}; run `{attach_command}` to start or attach it",
214+
socket_path.display()
215+
)
216+
);
217+
}
218+
219+
let base = unique_test_dir();
220+
let config_home = base.join("config");
221+
let runtime_dir = base.join("runtime");
222+
fs::create_dir_all(&runtime_dir).unwrap();
223+
register_runtime_dir(&runtime_dir);
224+
225+
let named_socket = named_session_socket(&config_home, "foo");
226+
let missing = run_named_cli(
227+
&config_home,
228+
&runtime_dir,
229+
&["--session", "foo", "workspace", "create"],
230+
);
231+
assert_server_not_running(missing, &named_socket, "herdr session attach foo");
232+
233+
let stale_socket = runtime_dir.join("stale.sock");
234+
drop(UnixListener::bind(&stale_socket).unwrap());
235+
let stale = Command::new(env!("CARGO_BIN_EXE_herdr"))
236+
.args(["workspace", "create"])
237+
.env("XDG_CONFIG_HOME", &config_home)
238+
.env("XDG_RUNTIME_DIR", &runtime_dir)
239+
.env("HERDR_SOCKET_PATH", &stale_socket)
240+
.env("HERDR_SESSION", "unrelated")
241+
.env_remove("HERDR_CLIENT_SOCKET_PATH")
242+
.env_remove("HERDR_ENV")
243+
.output()
244+
.unwrap();
245+
assert_server_not_running(stale, &stale_socket, "herdr");
246+
247+
cleanup_test_base(&base);
248+
}
249+
187250
#[test]
188251
fn integration_commands_run_locally_when_server_is_missing() {
189252
let base = unique_test_dir();

0 commit comments

Comments
 (0)