swarm-controller: answer 404, not 503, for an issue-report on a nonexistent repo

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

@@ -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())

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<forge::IssueReportRow>),
+        (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<AppState>,
     Path((org, repo)): Path<(String, String)>,
-) -> Result<Json<Vec<forge::IssueReportRow>>, StatusUnavailable> {
+) -> Result<Json<Vec<forge::IssueReportRow>>, problem_details::ProblemDetails> {
     let Some(client) = state.forge.as_ref() else {
-        return Err(StatusUnavailable(
+        return Err(error_problem(
-            "no forge is configured on this host".to_owned(),
+            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 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(StatusUnavailable(detail))
+                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::<forgejo_api::ForgejoError>() {
+        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
+        );
+    }
+}