From 244db367c4110bc3feaf3a3f4745d94e24936592 Mon Sep 17 00:00:00 2001 From: atlas Date: Thu, 24 Sep 2026 16:26:40 +0200 Subject: [PATCH] swarm-controller: answer 404, not 503, for an issue-report on a nonexistent repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit get_issue_report mapped every error from Client::issue_report to 503 (StatusUnavailable) unconditionally. issue_report calls issue_list_issues(org, repo, ...) first, and a Forgejo 404 there — a typo'd or deleted repo — was flattened into the same 503 a genuine forge outage produces, which tells a client to retry a request that will never succeed. Downcast the anyhow error back to forgejo_api::ForgejoError (same pattern main.rs's wanted_error_status uses for swarm_queue_client::Error) and check it structurally against the ApiErrorKind::NotFound / UnexpectedStatusCode(404) shapes forgejo's generated client produces for a 404, rather than string-matching the rendered message. Only that case answers 404, naming the org/repo; every other forge failure still answers 503. Updates the route's OpenAPI response list to document the 404. Closes #4701 --- swarm-controller/src/forge.rs | 21 +++++ swarm-controller/src/issue_report.rs | 117 +++++++++++++++++++++++++-- 2 files changed, 131 insertions(+), 7 deletions(-) diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 21961310..23bbdf22 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -1103,6 +1103,27 @@ fn folds_into_success(e: &ForgejoError, existing: bool) -> bool { is_confirmed_conflict(e) || (is_ambiguous_validation_failure(e) && existing) } +/// Whether a forge call failed because the named repo itself doesn't exist — +/// forgejo answers a plain HTTP 404 for `owner/repo` not found, which the +/// generated client surfaces as `ApiErrorKind::NotFound`. Checked +/// defensively against `ApiErrorKind::Other`/`UnexpectedStatusCode` too, +/// same dual-check shape [`is_already_exists`] uses for 409/422 — belt and +/// braces against a response whose body didn't parse as the expected shape. +/// Used by [`crate::issue_report::get_issue_report`] to tell "no such repo" +/// (404, retrying won't help) apart from every other forge failure (503, +/// might clear up) — see that module for why the distinction matters. +pub(crate) fn is_repo_not_found(e: &ForgejoError) -> bool { + match e { + ForgejoError::ApiError(api) => match api.error_kind() { + ApiErrorKind::NotFound { .. } => true, + ApiErrorKind::Other(s) => *s == StatusCode::NOT_FOUND, + _ => false, + }, + ForgejoError::UnexpectedStatusCode(s) => *s == StatusCode::NOT_FOUND, + _ => false, + } +} + #[cfg(test)] fn validation_failed_error() -> ForgejoError { ForgejoError::ApiError(ApiErrorKind::ValidationFailed.into()) diff --git a/swarm-controller/src/issue_report.rs b/swarm-controller/src/issue_report.rs index 12309b6f..edc0a6b8 100644 --- a/swarm-controller/src/issue_report.rs +++ b/swarm-controller/src/issue_report.rs @@ -12,7 +12,7 @@ use axum::{ extract::{Path, State}, }; -use super::{AppState, StatusUnavailable}; +use super::{AppState, StatusUnavailable, error_problem}; use crate::forge; /// Repos with at least one open issue, as `owner/name` full names — the @@ -84,6 +84,13 @@ pub async fn get_issue_report_all( /// See [`forge::Client::issue_report`] for how `blocked` / /// `depended_on_by_count` are computed and why this has to be a /// server-side pass rather than something swarm-ui resolves itself per row. +/// +/// Unlike [`get_repos`] / [`get_issue_report_all`], a failure here isn't +/// automatically 503: `issue_report` calls `issue_list_issues(org, repo, +/// ...)` first, and a Forgejo 404 there — a typo'd or deleted repo — means +/// "this will never work", not "try again later". See +/// [`issue_report_error_status`] for how that's told apart from a genuine +/// forge failure, which does stay 503. #[utoipa::path( get, path = "/api/repos/{org}/{repo}/issue-report", @@ -93,6 +100,7 @@ pub async fn get_issue_report_all( ), responses( (status = 200, description = "every open issue in this repo, blocked/dependent-count pre-resolved", body = Vec), + (status = 404, description = "no such repo (problem+json)", body = String), (status = 503, description = "no forge is configured on this host, or the report could not be built", body = String), ), tag = "repos" @@ -100,18 +108,113 @@ pub async fn get_issue_report_all( pub async fn get_issue_report( State(state): State, Path((org, repo)): Path<(String, String)>, -) -> Result>, StatusUnavailable> { +) -> Result>, problem_details::ProblemDetails> { let Some(client) = state.forge.as_ref() else { - return Err(StatusUnavailable( - "no forge is configured on this host".to_owned(), + return Err(error_problem( + axum::http::StatusCode::SERVICE_UNAVAILABLE, + "no forge is configured on this host", )); }; match client.issue_report(&org, &repo).await { Ok(rows) => Ok(Json(rows)), Err(e) => { - let detail = format!("{e:#}"); - tracing::warn!(error = %detail, %org, %repo, "building issue report failed"); - Err(StatusUnavailable(detail)) + let status = issue_report_error_status(&e); + if status == axum::http::StatusCode::NOT_FOUND { + tracing::info!(%org, %repo, "issue report requested for a repo that doesn't exist"); + Err(error_problem(status, &format!("{org}/{repo} not found"))) + } else { + let detail = format!("{e:#}"); + tracing::warn!(error = %detail, %org, %repo, "building issue report failed"); + Err(error_problem(status, &detail)) + } } } } + +/// The status [`get_issue_report`]'s forge error should answer with. +/// +/// `Client::issue_report` propagates through `anyhow`, so the concrete +/// [`forgejo_api::ForgejoError`] survives underneath but is not the type in +/// hand — downcasting back to it, same pattern `main.rs`'s +/// `wanted_error_status` uses for `swarm_queue_client::Error`. A 404 there +/// (`forge::is_repo_not_found`) means the repo itself doesn't exist; +/// anything else — including a 404 on some *other* call this function +/// doesn't cover, an outage, auth failure, etc. — stays 503, same as before +/// this existed. +fn issue_report_error_status(e: &anyhow::Error) -> axum::http::StatusCode { + match e.downcast_ref::() { + Some(fe) if forge::is_repo_not_found(fe) => axum::http::StatusCode::NOT_FOUND, + _ => axum::http::StatusCode::SERVICE_UNAVAILABLE, + } +} + +#[cfg(test)] +mod tests { + use forgejo_api::{ApiErrorKind, ForgejoError}; + + // ── a repo-not-found forge error maps to 404, everything else stays + // 503 ─────────────────────────────────────────────────────────── + + /// `issue_report_error_status` as a pure function first: build the + /// exact `ForgejoError` a 404 from `issue_list_issues` decodes to (see + /// `forge::is_repo_not_found`'s doc comment for the trace through + /// forgejo-api's generated client), wrap it the same way `?` inside + /// `Client::issue_report`'s `.with_context(...)` does, and confirm it + /// downcasts back to the status the route is meant to answer. + #[test] + fn repo_not_found_maps_to_404() { + let e: anyhow::Error = + ForgejoError::ApiError(ApiErrorKind::NotFound { errors: None }.into()).into(); + let e = e.context("list open issues in some-org/some-repo"); + assert_eq!( + super::issue_report_error_status(&e), + axum::http::StatusCode::NOT_FOUND + ); + } + + /// Belt-and-braces case: a 404 that arrives as `UnexpectedStatusCode` + /// rather than the parsed `ApiErrorKind::NotFound` shape (the body + /// didn't decode as forgejo's not-found JSON) still maps to 404 — see + /// `forge::is_repo_not_found`'s dual check. + #[test] + fn unparsed_404_still_maps_to_404() { + let e: anyhow::Error = + ForgejoError::UnexpectedStatusCode(axum::http::StatusCode::NOT_FOUND).into(); + assert_eq!( + super::issue_report_error_status(&e), + axum::http::StatusCode::NOT_FOUND + ); + } + + /// A *different* `ForgejoError` variant — a real forge failure, not + /// "repo doesn't exist" — stays 503. Proves the match looks at which + /// kind of forge error this is, not just "was `ForgejoError` involved + /// at all". + #[test] + fn a_different_forge_error_stays_503() { + let e: anyhow::Error = ForgejoError::ApiError(ApiErrorKind::Forbidden.into()).into(); + assert_eq!( + super::issue_report_error_status(&e), + axum::http::StatusCode::SERVICE_UNAVAILABLE + ); + } + + /// Invert-proof for the 404 cases above: an `anyhow::Error` that does + /// not wrap a `ForgejoError` at all (the shape a genuine non-forge + /// failure takes) must NOT downcast to it, and must stay 503. Without + /// this, an `issue_report_error_status` that answered 404 + /// unconditionally would still pass the tests above. This is also the + /// status quo this change preserves — before this change, *every* + /// error on this route (including the 404 case above) answered 503 + /// unconditionally; this test alone can't distinguish "old + /// unconditional 503" from "new, correctly-scoped 503", the 404 tests + /// above do that. + #[test] + fn a_non_forge_error_stays_503() { + let e = anyhow::anyhow!("list open issues in some-org/some-repo"); + assert_eq!( + super::issue_report_error_status(&e), + axum::http::StatusCode::SERVICE_UNAVAILABLE + ); + } +}