fix(#3412): swarm-controller answers errors as RFC 9457 problem+json
Its three error paths returned a bare string with a status code, which forces a caller to treat the whole body as prose. hive-c0re converted some time ago, so swarm-controller was the last backend on the old shape -- and it is the one behind the hive status page's 503, where the body is frequently the entire diagnosis rather than a summary. Adds the commitment to docs/conventions.md, since it was implied by the code in one daemon and written down nowhere: an endpoint of ours answering with a bare string is a bug to file, not something callers work around. The test asserts the rendered response -- media type plus an addressable detail -- rather than the problem_details value, because a handler that built the value correctly and then returned it as a string would satisfy a test written against the type alone.
This commit is contained in:
parent
1d9a06482a
commit
58588a6866
4 changed files with 90 additions and 8 deletions
1
Cargo.lock
generated
1
Cargo.lock
generated
|
|
@ -4591,6 +4591,7 @@ dependencies = [
|
||||||
"hive-jobq",
|
"hive-jobq",
|
||||||
"hive-jobq-wire",
|
"hive-jobq-wire",
|
||||||
"hive-types",
|
"hive-types",
|
||||||
|
"problem_details",
|
||||||
"reqwest 0.13.1",
|
"reqwest 0.13.1",
|
||||||
"serde",
|
"serde",
|
||||||
"serde_json",
|
"serde_json",
|
||||||
|
|
|
||||||
|
|
@ -318,6 +318,28 @@ refactor's concern. The dashboard frontend parses via
|
||||||
`util.js::epochSec` wherever it needs arithmetic and feeds the string
|
`util.js::epochSec` wherever it needs arithmetic and feeds the string
|
||||||
straight to `new Date(s)` for display.
|
straight to `new Date(s)` for display.
|
||||||
|
|
||||||
|
### HTTP error bodies
|
||||||
|
|
||||||
|
Every HTTP API in this repo answers failures with **RFC 9457
|
||||||
|
`application/problem+json`** (`{ type, title, status, detail }`), with the
|
||||||
|
human-readable cause in `detail`. An endpoint of ours returning a bare string
|
||||||
|
or a bespoke error shape is a **bug to file against the backend**, not
|
||||||
|
something for the caller to work around.
|
||||||
|
|
||||||
|
Use the `problem_details` crate (`features = ["axum"]`), which the daemons
|
||||||
|
already depend on: type a handler `Result<_, ProblemDetails>` and hand
|
||||||
|
`ProblemDetails::from_status_code(...).with_detail(...)` to `Err`.
|
||||||
|
|
||||||
|
The reason is the consumer, not tidiness. The UIs show errors through one
|
||||||
|
shared component with a copy button, so a caller has to know **which part of
|
||||||
|
the body is the message**. A bare string forces it to treat the whole payload
|
||||||
|
as prose, which is the difference between offering "copy the cause" and
|
||||||
|
dumping a response — and the cause is frequently the entire diagnosis (a
|
||||||
|
JetStream permission refusal, a TLS chain failure) rather than a summary.
|
||||||
|
|
||||||
|
Not in scope: the `hivectl` host-admin and in-agent unix sockets. Those are a
|
||||||
|
JSON-line protocol with their own result types; RFC 9457 is an HTTP format.
|
||||||
|
|
||||||
## Tool groups
|
## Tool groups
|
||||||
|
|
||||||
The MCP tool surface an agent receives is derived from a set of named
|
The MCP tool surface an agent receives is derived from a set of named
|
||||||
|
|
|
||||||
|
|
@ -27,6 +27,10 @@ forgejo-api.workspace = true
|
||||||
# raw bytes.
|
# raw bytes.
|
||||||
base64.workspace = true
|
base64.workspace = true
|
||||||
futures-util.workspace = true
|
futures-util.workspace = true
|
||||||
|
# RFC 9457 `application/problem+json` error bodies. Same version + `axum`
|
||||||
|
# feature as hive-c0re: the two daemons answer the same operator UIs, so a
|
||||||
|
# reader that handles one's failures has to handle the other's.
|
||||||
|
problem_details = { version = "0.9.0", features = ["axum"] }
|
||||||
# The graph itself, held directly rather than behind a c0re-style wrapper
|
# The graph itself, held directly rather than behind a c0re-style wrapper
|
||||||
# module — that layering (`hive-c0re::job_queue`) is partially legacy (predates
|
# module — that layering (`hive-c0re::job_queue`) is partially legacy (predates
|
||||||
# `hive-jobq`'s extraction into its own crate) and this daemon does not need it
|
# `hive-jobq`'s extraction into its own crate) and this daemon does not need it
|
||||||
|
|
|
||||||
|
|
@ -428,14 +428,29 @@ async fn get_links(State(state): State<AppState>) -> Json<Vec<ServiceLink>> {
|
||||||
/// body because a bare 503 on an operator-facing diagnostic is how a
|
/// body because a bare 503 on an operator-facing diagnostic is how a
|
||||||
/// misconfiguration costs an afternoon; it is a queue/JetStream error
|
/// misconfiguration costs an afternoon; it is a queue/JetStream error
|
||||||
/// string, and this surface is already behind the swarm's SSO.
|
/// string, and this surface is already behind the swarm's SSO.
|
||||||
|
///
|
||||||
|
/// It travels in `detail` of an RFC 9457 `application/problem+json` body
|
||||||
|
/// rather than as a bare string, so the cause is an addressable field
|
||||||
|
/// instead of the whole payload — see `error_problem` below.
|
||||||
struct StatusUnavailable(String);
|
struct StatusUnavailable(String);
|
||||||
|
|
||||||
impl axum::response::IntoResponse for StatusUnavailable {
|
impl axum::response::IntoResponse for StatusUnavailable {
|
||||||
fn into_response(self) -> axum::response::Response {
|
fn into_response(self) -> axum::response::Response {
|
||||||
(axum::http::StatusCode::SERVICE_UNAVAILABLE, self.0).into_response()
|
error_problem(axum::http::StatusCode::SERVICE_UNAVAILABLE, &self.0).into_response()
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Every error this daemon returns, in one shape.
|
||||||
|
///
|
||||||
|
/// RFC 9457 `application/problem+json` is the hive-wide contract for HTTP
|
||||||
|
/// error bodies (`docs/conventions.md`), and the operator UIs read `detail`
|
||||||
|
/// for display. A bare string forces the reader to treat the entire body as
|
||||||
|
/// the message, which is the difference between a UI that can offer "copy the
|
||||||
|
/// cause" and one that can only dump a response.
|
||||||
|
fn error_problem(status: axum::http::StatusCode, detail: &str) -> problem_details::ProblemDetails {
|
||||||
|
problem_details::ProblemDetails::from_status_code(status).with_detail(detail)
|
||||||
|
}
|
||||||
|
|
||||||
/// What each hive last said about itself, read from the swarm queue at
|
/// What each hive last said about itself, read from the swarm queue at
|
||||||
/// request time.
|
/// request time.
|
||||||
///
|
///
|
||||||
|
|
@ -522,17 +537,17 @@ struct CreateAgentResponse {
|
||||||
request_body = CreateAgentRequest,
|
request_body = CreateAgentRequest,
|
||||||
responses(
|
responses(
|
||||||
(status = 200, description = "job chain queued", body = CreateAgentResponse),
|
(status = 200, description = "job chain queued", body = CreateAgentResponse),
|
||||||
(status = 400, description = "`name` is not a valid identifier", body = String),
|
(status = 400, description = "`name` is not a valid identifier (problem+json)", body = String),
|
||||||
(status = 500, description = "the job chain could not be queued", body = String),
|
(status = 500, description = "the job chain could not be queued (problem+json)", body = String),
|
||||||
),
|
),
|
||||||
tag = "agents"
|
tag = "agents"
|
||||||
)]
|
)]
|
||||||
async fn create_agent(
|
async fn create_agent(
|
||||||
State(state): State<AppState>,
|
State(state): State<AppState>,
|
||||||
Json(req): Json<CreateAgentRequest>,
|
Json(req): Json<CreateAgentRequest>,
|
||||||
) -> Result<Json<CreateAgentResponse>, (axum::http::StatusCode, String)> {
|
) -> Result<Json<CreateAgentResponse>, problem_details::ProblemDetails> {
|
||||||
let agent = hive_types::Ident::parse(&req.name)
|
let agent = hive_types::Ident::parse(&req.name)
|
||||||
.map_err(|reason| (axum::http::StatusCode::BAD_REQUEST, reason.to_owned()))?
|
.map_err(|reason| error_problem(axum::http::StatusCode::BAD_REQUEST, reason))?
|
||||||
.into_string();
|
.into_string();
|
||||||
let repo = agent.clone();
|
let repo = agent.clone();
|
||||||
|
|
||||||
|
|
@ -562,7 +577,12 @@ async fn create_agent(
|
||||||
.after_ok(create_repo);
|
.after_ok(create_repo);
|
||||||
vec![create_identity.guid()]
|
vec![create_identity.guid()]
|
||||||
})
|
})
|
||||||
.map_err(|e| (axum::http::StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?;
|
.map_err(|e| {
|
||||||
|
error_problem(
|
||||||
|
axum::http::StatusCode::INTERNAL_SERVER_ERROR,
|
||||||
|
&e.to_string(),
|
||||||
|
)
|
||||||
|
})?;
|
||||||
let [id] = ids[..] else {
|
let [id] = ids[..] else {
|
||||||
unreachable!("exactly one handle was asked for");
|
unreachable!("exactly one handle was asked for");
|
||||||
};
|
};
|
||||||
|
|
@ -770,11 +790,46 @@ async fn main() -> Result<()> {
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::{
|
use super::{
|
||||||
DEFAULT_SOCKET, HIVES_ENV, HiveEntry, LINKS_ENV, ServiceLink, SwarmNodeKind, WorkerDeps,
|
DEFAULT_SOCKET, HIVES_ENV, HiveEntry, LINKS_ENV, ServiceLink, StatusUnavailable,
|
||||||
load_hives, load_links, run_swarm_node,
|
SwarmNodeKind, WorkerDeps, load_hives, load_links, run_swarm_node,
|
||||||
};
|
};
|
||||||
use std::path::Path;
|
use std::path::Path;
|
||||||
|
|
||||||
|
/// The 503 this route returns is what the operator UIs' shared error
|
||||||
|
/// component renders, so assert the RENDERED response rather than the
|
||||||
|
/// `problem_details` crate: the contract a UI depends on is the content
|
||||||
|
/// type plus a `detail` it can address, and a handler that built the
|
||||||
|
/// value and returned it as a bare string would satisfy any test
|
||||||
|
/// written against the type alone.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn status_unavailable_renders_problem_json_with_the_cause_in_detail() {
|
||||||
|
use axum::response::IntoResponse as _;
|
||||||
|
|
||||||
|
let cause = "listing status bucket keys: timed out";
|
||||||
|
let resp = StatusUnavailable(cause.to_owned()).into_response();
|
||||||
|
|
||||||
|
assert_eq!(resp.status(), axum::http::StatusCode::SERVICE_UNAVAILABLE);
|
||||||
|
let ct = resp
|
||||||
|
.headers()
|
||||||
|
.get(axum::http::header::CONTENT_TYPE)
|
||||||
|
.and_then(|v| v.to_str().ok())
|
||||||
|
.unwrap_or_default()
|
||||||
|
.to_owned();
|
||||||
|
assert!(
|
||||||
|
ct.starts_with("application/problem+json"),
|
||||||
|
"RFC 9457 media type, got {ct:?}"
|
||||||
|
);
|
||||||
|
|
||||||
|
let bytes = axum::body::to_bytes(resp.into_body(), 64 * 1024)
|
||||||
|
.await
|
||||||
|
.expect("body reads");
|
||||||
|
let v: serde_json::Value = serde_json::from_slice(&bytes).expect("problem+json parses");
|
||||||
|
assert_eq!(v["status"], 503);
|
||||||
|
// The cause is an addressable field, not the entire payload — that
|
||||||
|
// distinction is the point of the change, so it is what is asserted.
|
||||||
|
assert_eq!(v["detail"], cause);
|
||||||
|
}
|
||||||
|
|
||||||
/// Drives `SwarmNodeKind::CreateRepo` through the real
|
/// Drives `SwarmNodeKind::CreateRepo` through the real
|
||||||
/// `hive_jobq::scheduler::Scheduler` claim → run → complete path,
|
/// `hive_jobq::scheduler::Scheduler` claim → run → complete path,
|
||||||
/// rather than only through `create_agent`'s endpoint test (there
|
/// rather than only through `create_agent`'s endpoint test (there
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue