refactor(sock): one socket client, retry as a policy value
Six places in the tree hand-rolled the same connect / write one JSON line / read one JSON line back. Two of them — the harness serve loop's client and the MCP server's — were byte-identical apart from a six-line wrapper, ~145 lines of literal copy-paste. The other four each reimplemented a subset, and the subsets had drifted: some named the socket path in their errors and some did not, one classified transient against fatal failures and the rest retried nothing at all, two drained the response and two decoded it. That duplication was defended when the daemons were split out, on the grounds that a daemon's socket etiquette should stay visible in the crate that depends on it. The etiquette genuinely does differ. The code does not, and five copies is where "each daemon documents its own etiquette" stops paying for itself. `hive-sock-client` now owns the transport once, generic over the request and response types so it is protocol-agnostic: the host-served control socket and the harness's in-agent socket both use it with their own wire-type crates. The two real differences become values instead of forks. Retry is `Retry::RideOutRestart` (2/4/8/16/30s, sized to ride out a service restart) for callers with no natural retry of their own, or `Retry::None` for callers already inside a poll loop where the poll interval is the retry — and the reason each caller picked one is a comment at the call site rather than a reimplementation. The response is either decoded (`request`) or half-closed and drained (`notify`, where the drain exists so the server's write-back doesn't land on a closed socket). Whether a failure propagates or is logged and swallowed stays at the call site, because that is the caller's choice and not a property of the transport. Errors always name the socket path now, everywhere. That detail is load-bearing: a permission problem on a socket that reads as "is the daemon running?" sends the operator to fix the wrong thing. The transient-against-fatal enum is gone rather than moved. Serialising happens before the retry loop and deserialising after it, so only connect, I/O and short-read failures can reach the loop at all — a deterministic failure is now unretryable by construction instead of by classification. It is deliberately a new crate and not part of `hive-agent-sock`. The `*-sock` crates are pure wire types by convention — `hive-agent-sock` depends on serde and nothing else — and the two largest copies talk to the host socket, whose types live in a different crate entirely. A transport in either wire-type crate would drag tokio into it and point the wrong way besides. No wire-format change: same JSON line in, same line out.
This commit is contained in:
parent
bd14cc5c46
commit
7a826f9ee2
23 changed files with 498 additions and 501 deletions
|
|
@ -1,152 +0,0 @@
|
|||
use std::path::Path;
|
||||
use std::time::Duration;
|
||||
|
||||
use anyhow::{Result, anyhow};
|
||||
use serde::Serialize;
|
||||
use serde::de::DeserializeOwned;
|
||||
use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
|
||||
use tokio::net::UnixStream;
|
||||
|
||||
/// Backoff schedule between attempts. Five entries → up to 5 retries on
|
||||
/// top of the initial attempt; total wall-clock cap = 2+4+8+16+30 = 60s.
|
||||
/// Sized to ride out a hive-c0re restart (systemd usually has the unix
|
||||
/// socket back inside ~5s) without the agent-side claude session having
|
||||
/// to handle the transient itself — burning tokens on a tool-error retry
|
||||
/// loop is more expensive than 60s of in-harness sleep.
|
||||
const RETRY_BACKOFFS_MS: &[u64] = &[2_000, 4_000, 8_000, 16_000, 30_000];
|
||||
|
||||
/// Send `req` over the unix socket and decode the single-line JSON
|
||||
/// response, retrying transient connect/IO failures on the backoff
|
||||
/// schedule above and reporting how many retries it took past the initial
|
||||
/// attempt (0 = succeeded first try). MCP tool handlers
|
||||
/// use this so they can append a one-line hint to the tool result when
|
||||
/// retries happened — that way claude knows the prior socket flake
|
||||
/// wasn't a content error and shouldn't trigger an LLM-level retry of
|
||||
/// its own.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns an error if all retries are exhausted, or on a fatal protocol
|
||||
/// error (serialization / deserialization failure).
|
||||
///
|
||||
/// # Panics
|
||||
///
|
||||
/// Panics if `RETRY_BACKOFFS_MS.len()` does not fit in a `u32`, which
|
||||
/// cannot happen with the current compile-time constant.
|
||||
pub async fn request_retried<Req, Resp>(socket: &Path, req: &Req) -> Result<(Resp, u32)>
|
||||
where
|
||||
Req: Serialize + ?Sized,
|
||||
Resp: DeserializeOwned,
|
||||
{
|
||||
let mut last_err: Option<anyhow::Error> = None;
|
||||
let max_retries = u32::try_from(RETRY_BACKOFFS_MS.len()).unwrap();
|
||||
for attempt in 0..=max_retries {
|
||||
match try_once::<Req, Resp>(socket, req).await {
|
||||
Ok(resp) => return Ok((resp, attempt)),
|
||||
Err(RequestError::Fatal(e)) => return Err(e),
|
||||
Err(RequestError::Transient(e)) => {
|
||||
if attempt < max_retries {
|
||||
let sleep_ms = RETRY_BACKOFFS_MS[attempt as usize];
|
||||
tracing::warn!(
|
||||
attempt = attempt + 1,
|
||||
sleep_ms,
|
||||
error = %e,
|
||||
"hive socket attempt failed; retrying"
|
||||
);
|
||||
last_err = Some(e);
|
||||
tokio::time::sleep(Duration::from_millis(sleep_ms)).await;
|
||||
} else {
|
||||
last_err = Some(e);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
// Reaching here means the final attempt returned `Transient`, which always
|
||||
// sets `last_err` — so this is infallible.
|
||||
Err(last_err.expect("a transient failure on the final attempt set last_err"))
|
||||
}
|
||||
|
||||
/// Transient = connect / IO error worth a retry (server restart, broken
|
||||
/// pipe). Fatal = serialization / deserialization / protocol error
|
||||
/// where retrying would just repeat the same failure.
|
||||
enum RequestError {
|
||||
Transient(anyhow::Error),
|
||||
Fatal(anyhow::Error),
|
||||
}
|
||||
|
||||
async fn try_once<Req, Resp>(socket: &Path, req: &Req) -> Result<Resp, RequestError>
|
||||
where
|
||||
Req: Serialize + ?Sized,
|
||||
Resp: DeserializeOwned,
|
||||
{
|
||||
let stream = match UnixStream::connect(socket).await {
|
||||
Ok(stream) => stream,
|
||||
Err(e) => {
|
||||
// A refused or missing socket usually means hive-c0re is
|
||||
// mid-restart (operator redeploy / rebuild) — the socket is
|
||||
// recreated on its boot and `request_retried` rides it out. When
|
||||
// the error *does* surface (retries exhausted, or a non-retried
|
||||
// caller) add that context so claude reads it as a likely
|
||||
// transient rather than a hard failure worth escalating.
|
||||
let restarting = matches!(
|
||||
e.kind(),
|
||||
std::io::ErrorKind::ConnectionRefused | std::io::ErrorKind::NotFound
|
||||
);
|
||||
let mut err = anyhow::Error::new(e).context(format!("connect to {}", socket.display()));
|
||||
if restarting {
|
||||
err = err.context(
|
||||
"hive-c0re may be restarting (e.g. an operator redeploy); \
|
||||
the harness already retried ~60s before surfacing this",
|
||||
);
|
||||
}
|
||||
return Err(RequestError::Transient(err));
|
||||
}
|
||||
};
|
||||
let (read, mut write) = stream.into_split();
|
||||
|
||||
let mut payload = serde_json::to_string(req).map_err(|e| RequestError::Fatal(e.into()))?;
|
||||
payload.push('\n');
|
||||
write
|
||||
.write_all(payload.as_bytes())
|
||||
.await
|
||||
.map_err(|e| RequestError::Transient(e.into()))?;
|
||||
write
|
||||
.flush()
|
||||
.await
|
||||
.map_err(|e| RequestError::Transient(e.into()))?;
|
||||
|
||||
let mut reader = BufReader::new(read);
|
||||
let mut line = String::new();
|
||||
let read_bytes = reader
|
||||
.read_line(&mut line)
|
||||
.await
|
||||
.map_err(|e| RequestError::Transient(e.into()))?;
|
||||
if read_bytes == 0 || line.is_empty() {
|
||||
return Err(RequestError::Transient(anyhow!(
|
||||
"server closed connection without responding"
|
||||
)));
|
||||
}
|
||||
serde_json::from_str(line.trim()).map_err(|e| RequestError::Fatal(e.into()))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::{RequestError, try_once};
|
||||
|
||||
/// A connect to a non-existent socket path (ENOENT → `NotFound`) is
|
||||
/// classified transient AND annotated with the "hive-c0re is restarting"
|
||||
/// hint, so a surfaced tool error reads as the expected transient.
|
||||
#[tokio::test]
|
||||
async fn missing_socket_connect_is_transient_with_restart_hint() {
|
||||
let bogus = std::path::Path::new("/nonexistent/hive/mcp.sock");
|
||||
match try_once::<(), serde_json::Value>(bogus, &()).await {
|
||||
Err(RequestError::Transient(e)) => {
|
||||
let msg = format!("{e:#}");
|
||||
assert!(msg.contains("restarting"), "missing restart hint: {msg}");
|
||||
assert!(msg.contains("connect to"), "missing connect context: {msg}");
|
||||
}
|
||||
Err(RequestError::Fatal(e)) => panic!("expected transient, got fatal: {e:#}"),
|
||||
Ok(_) => panic!("expected connect failure to a non-existent socket"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -16,7 +16,6 @@ use std::path::PathBuf;
|
|||
use anyhow::Result;
|
||||
use clap::Parser;
|
||||
|
||||
mod client;
|
||||
mod mcp;
|
||||
mod paths;
|
||||
mod send_allow;
|
||||
|
|
|
|||
|
|
@ -20,8 +20,6 @@ use std::path::PathBuf;
|
|||
use anyhow::Result;
|
||||
use rmcp::{ServerHandler, handler::server::wrapper::Parameters, tool, tool_handler, tool_router};
|
||||
|
||||
use crate::client;
|
||||
|
||||
mod args;
|
||||
mod render;
|
||||
|
||||
|
|
@ -124,7 +122,15 @@ impl AgentServer {
|
|||
&self,
|
||||
req: hive_core_agent_sock::Request,
|
||||
) -> (Result<hive_core_agent_sock::Response, anyhow::Error>, u32) {
|
||||
match client::request_retried::<_, hive_core_agent_sock::Response>(&self.socket, &req).await
|
||||
// Ride out a hive-c0re restart rather than surface it: the caller
|
||||
// here is claude, and a tool error costs a whole retry turn's
|
||||
// tokens where 60s of in-daemon patience costs nothing.
|
||||
match hive_sock_client::request_retried::<_, hive_core_agent_sock::Response>(
|
||||
&self.socket,
|
||||
&req,
|
||||
hive_sock_client::Retry::RideOutRestart,
|
||||
)
|
||||
.await
|
||||
{
|
||||
Ok((r, n)) => (Ok(r), n),
|
||||
Err(e) => (Err(e), 0),
|
||||
|
|
|
|||
Loading…
Reference in a new issue