From 2357e72113ef3b5751147572781f42706ba4a978 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Tue, 25 Aug 2026 05:33:43 +0100 Subject: [PATCH] fix(api): give an expired subtitle candidate a typed error A grab naming a stale candidate_id now fails as ApiError::SubtitleCandidateExpired (404, code candidate_expired) instead of the generic upstream 503 string the panel had to pattern-match for 'not found'. --- crates/arr-api/src/movies.rs | 63 ++++++++++++++++++++++++--------- crates/arr-api/src/subtitles.rs | 27 +++++++++++++- web/src/main.ts | 7 ++-- web/src/releases.ts | 15 ++++++++ web/src/subtitles.ts | 11 ++++-- 5 files changed, 100 insertions(+), 23 deletions(-) diff --git a/crates/arr-api/src/movies.rs b/crates/arr-api/src/movies.rs index 0209707..76c5b32 100644 --- a/crates/arr-api/src/movies.rs +++ b/crates/arr-api/src/movies.rs @@ -145,6 +145,11 @@ pub struct Accepted { #[derive(Debug, Clone, Serialize, ToSchema)] pub struct ErrorBody { pub error: String, + /// A machine-readable discriminant, set only where a client needs to + /// branch on the failure rather than display it (issue #221). `None` + /// everywhere else — the message is for the operator, not the client. + #[serde(skip_serializing_if = "Option::is_none")] + pub code: Option, } #[derive(Debug)] @@ -166,6 +171,12 @@ pub enum ApiError { /// four engines are configurable at once and an unnamed failure is /// unactionable. SubtitleUpstream(String), + /// A grab named a `candidate_id` the provider no longer recognises + /// (issue #221): a search's results outlive the search itself only in + /// the client's memory, and the provider can expire one at will. Kept + /// apart from [`Self::SubtitleUpstream`] so the panel can offer "search + /// again" from a `code`, not from matching the message text. + SubtitleCandidateExpired, /// The §9.6 chip outcome: the title exists upstream but has no trailer. /// Ordinary, so it must stay distinguishable from an upstream failure. NoTrailer, @@ -186,41 +197,61 @@ pub enum ApiError { impl IntoResponse for ApiError { fn into_response(self) -> Response { - let (status, error) = match self { - Self::NotFound => (StatusCode::NOT_FOUND, "movie not found".to_string()), - Self::SeriesNotFound => (StatusCode::NOT_FOUND, "series not found".to_string()), - Self::SeasonNotFound => (StatusCode::NOT_FOUND, "season not found".to_string()), - Self::EpisodeNotFound => (StatusCode::NOT_FOUND, "episode not found".to_string()), - Self::OwnerNotFound => (StatusCode::NOT_FOUND, "owner not found".to_string()), - Self::PolicyNotFound => (StatusCode::NOT_FOUND, "policy not found".to_string()), - Self::RootNotFound => (StatusCode::NOT_FOUND, "root not found".to_string()), - Self::MediaFileNotFound => (StatusCode::NOT_FOUND, "media file not found".to_string()), - Self::SubtitleNotFound => (StatusCode::NOT_FOUND, "subtitle not found".to_string()), - Self::SubtitleUpstream(error) => (StatusCode::SERVICE_UNAVAILABLE, error), - Self::NoTrailer => (StatusCode::NOT_FOUND, "no trailer".to_string()), - Self::Conflict(error) => (StatusCode::CONFLICT, error), - Self::Invalid(error) => (StatusCode::UNPROCESSABLE_ENTITY, error), + let (status, error, code) = match self { + Self::NotFound => (StatusCode::NOT_FOUND, "movie not found".to_string(), None), + Self::SeriesNotFound => (StatusCode::NOT_FOUND, "series not found".to_string(), None), + Self::SeasonNotFound => (StatusCode::NOT_FOUND, "season not found".to_string(), None), + Self::EpisodeNotFound => (StatusCode::NOT_FOUND, "episode not found".to_string(), None), + Self::OwnerNotFound => (StatusCode::NOT_FOUND, "owner not found".to_string(), None), + Self::PolicyNotFound => (StatusCode::NOT_FOUND, "policy not found".to_string(), None), + Self::RootNotFound => (StatusCode::NOT_FOUND, "root not found".to_string(), None), + Self::MediaFileNotFound => ( + StatusCode::NOT_FOUND, + "media file not found".to_string(), + None, + ), + Self::SubtitleNotFound => ( + StatusCode::NOT_FOUND, + "subtitle not found".to_string(), + None, + ), + Self::SubtitleUpstream(error) => (StatusCode::SERVICE_UNAVAILABLE, error, None), + Self::SubtitleCandidateExpired => ( + StatusCode::NOT_FOUND, + "candidate no longer exists — search again".to_string(), + Some("candidate_expired".to_string()), + ), + Self::NoTrailer => (StatusCode::NOT_FOUND, "no trailer".to_string(), None), + Self::Conflict(error) => (StatusCode::CONFLICT, error, None), + Self::Invalid(error) => (StatusCode::UNPROCESSABLE_ENTITY, error, None), Self::Unavailable => ( StatusCode::SERVICE_UNAVAILABLE, "database unavailable".into(), + None, ), Self::Upstream(name) => ( StatusCode::SERVICE_UNAVAILABLE, format!("{name} unavailable"), + None, ), Self::Database(error) => { tracing::error!(%error, "API database error"); - (StatusCode::INTERNAL_SERVER_ERROR, "database error".into()) + ( + StatusCode::INTERNAL_SERVER_ERROR, + "database error".into(), + None, + ) } Self::Filesystem(error) => { tracing::error!(%error, "API filesystem error"); ( StatusCode::INTERNAL_SERVER_ERROR, format!("files not removed: {error}"), + None, ) } }; - (status, Json(ErrorBody { error })).into_response() + (status, Json(ErrorBody { error, code })).into_response() } } diff --git a/crates/arr-api/src/subtitles.rs b/crates/arr-api/src/subtitles.rs index b7503af..d5f4c5e 100644 --- a/crates/arr-api/src/subtitles.rs +++ b/crates/arr-api/src/subtitles.rs @@ -968,7 +968,10 @@ pub async fn grab( let fetched = provider .download(&CandidateId::new(input.candidate_id.clone())) .await - .map_err(|error| ApiError::SubtitleUpstream(error.to_string()))?; + .map_err(|error| match error { + arr_subs::Error::NotFound { .. } => ApiError::SubtitleCandidateExpired, + other => ApiError::SubtitleUpstream(other.to_string()), + })?; let text = srt_text(&fetched)?; write_sidecar(&destination, &text).await?; @@ -2301,6 +2304,28 @@ mod tests { .exists()); } + /// A candidate id does not outlive the search that produced it (issue + /// #221): the panel gets a typed `code` to switch on, not a string it has + /// to pattern-match against a provider-chosen message. + #[tokio::test] + async fn a_grab_of_an_expired_candidate_is_a_typed_404() { + let fixture = application(vec![Arc::new(DeadProvider)], vec![]).await; + + let (status, body) = grab( + &fixture, + serde_json::json!({ + "provider": "dead", + "candidate_id": "stale", + "language": "pt-PT" + }), + ) + .await; + + assert_eq!(status, StatusCode::NOT_FOUND, "{body}"); + assert_eq!(body["code"], "candidate_expired"); + assert!(fixture.subtitle_rows().await.is_empty()); + } + #[tokio::test] async fn a_grab_naming_an_unconfigured_provider_is_a_422() { let fixture = stub_application().await; diff --git a/web/src/main.ts b/web/src/main.ts index b4d53c1..9ddb322 100644 --- a/web/src/main.ts +++ b/web/src/main.ts @@ -1320,9 +1320,10 @@ function subtitleSection(status: SubtitleStatus, refresh: () => void): SubtitleS say(note, "fetching, syncing, writing…"); const outcome = await grabSubtitle(mediaFileId, candidate); if (outcome.kind === "error") { - const stale = /not found/i.test(outcome.detail) - ? " — search again, a candidate id does not outlive its search" - : ""; + const stale = + outcome.code === "candidate_expired" + ? " — search again, a candidate id does not outlive its search" + : ""; say(note, `grab failed — ${outcome.detail}${stale}`, "fault"); return false; } diff --git a/web/src/releases.ts b/web/src/releases.ts index 527bc48..be4f5a1 100644 --- a/web/src/releases.ts +++ b/web/src/releases.ts @@ -463,3 +463,18 @@ export async function errorDetail(response: Response): Promise { return `http ${response.status}`; } } + +/** + * Like {@link errorDetail}, but also surfaces `ErrorBody.code` — the + * machine-readable discriminant a client switches on instead of matching the + * message text (issue #221). `code` is `undefined` on every error that has + * none, which is most of them. + */ +export async function errorBody(response: Response): Promise<{ detail: string; code?: string }> { + try { + const body = (await response.json()) as { error?: string; code?: string }; + return { detail: body.error ?? `http ${response.status}`, code: body.code }; + } catch { + return { detail: `http ${response.status}` }; + } +} diff --git a/web/src/subtitles.ts b/web/src/subtitles.ts index c86ad24..667ac40 100644 --- a/web/src/subtitles.ts +++ b/web/src/subtitles.ts @@ -2,7 +2,7 @@ // schemas — same reasoning as search.ts: the generated client (src/api/) is // uncommitted, so CI's tsc cannot see it. -import { errorDetail } from "./releases"; +import { errorBody, errorDetail } from "./releases"; /** One subtitle arr knows about, as `/subtitles` and `/subtitles/status` render it. */ export interface Subtitle { @@ -201,12 +201,16 @@ export async function searchSubtitles( export type SubtitleWriteOutcome = | { kind: "done"; subtitle: Subtitle } - | { kind: "error"; detail: string }; + | { kind: "error"; detail: string; code?: string }; /** * One click: the daemon fetches the candidate, runs `alass` over it and * writes the sidecar (§15). The candidate's own flags travel with it — * nothing on the server remembers a search. + * + * A grab can fail because the candidate itself expired between the search + * and the click (issue #221) — the server reports that as `code: + * "candidate_expired"`, not by wording the message a particular way. */ export async function grabSubtitle( mediaFileId: number, @@ -225,7 +229,8 @@ export async function grabSubtitle( }), }); if (!response.ok) { - return { kind: "error", detail: await errorDetail(response) }; + const { detail, code } = await errorBody(response); + return { kind: "error", detail, code }; } return { kind: "done", subtitle: (await response.json()) as Subtitle }; } catch {