From 635a651beef8f3c2a90dedd5639cff93bdd51e69 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Tue, 25 Aug 2026 01:24:02 +0100 Subject: [PATCH 1/2] feat(arr): add Syncer::settle as the alass seam Folds an implausible result and an unusable alass binary into one SyncState both the grab handler (#199) and reconcile loop (#196) can record without re-deriving the same match arms. --- crates/arr-subs/src/lib.rs | 4 +- crates/arr-subs/src/sync.rs | 123 +++++++++++++++++++++++++++++++++++- 2 files changed, 125 insertions(+), 2 deletions(-) diff --git a/crates/arr-subs/src/lib.rs b/crates/arr-subs/src/lib.rs index 4518d32..bb94dbe 100644 --- a/crates/arr-subs/src/lib.rs +++ b/crates/arr-subs/src/lib.rs @@ -51,7 +51,9 @@ pub use openai::{OpenAi, OpenAiConfig}; pub use opensubtitles::{moviehash, OpenSubtitles, OpenSubtitlesConfig}; pub use podnapisi::{Podnapisi, PodnapisiBuilder, DEFAULT_BASE_URL as PODNAPISI_DEFAULT_BASE_URL}; pub use srt::Cue; -pub use sync::{Outcome, Rejection, Syncer, DEFAULT_BINARY as ALASS_DEFAULT_BINARY}; +pub use sync::{ + Outcome, Rejection, Settled, SyncState, Syncer, DEFAULT_BINARY as ALASS_DEFAULT_BINARY, +}; pub use translate::{Backend, BackendId, Batch, BatchCue, TranslateFuture, TranslatedCue}; // `unused_crate_dependencies` is a per-target lint. Every provider and diff --git a/crates/arr-subs/src/sync.rs b/crates/arr-subs/src/sync.rs index 79e53d4..e47b575 100644 --- a/crates/arr-subs/src/sync.rs +++ b/crates/arr-subs/src/sync.rs @@ -131,6 +131,48 @@ impl Syncer { }) } + /// Run [`Self::sync`] and settle the result into a form neither caller + /// has to branch on twice. + /// + /// #199's manual grab handler and #196's reconcile loop both write a + /// fetched or translated subtitle, then call this before recording the + /// row — the seam issue #214 asked for, so the sync-then-classify logic + /// exists once. An [`Error`] from `sync` — a missing binary, a timeout, a + /// non-zero exit — degrades to [`SyncState::NotRun`] with a warning + /// logged, rather than failing the caller: a subtitle arr already has + /// beats one it refuses because `alass` could not run. + pub async fn settle(&self, video: impl AsRef, subtitle: impl AsRef) -> Settled { + let subtitle = subtitle.as_ref(); + match self.sync(video, subtitle).await { + Ok(Outcome::Accepted { content }) => Settled { + content: Some(content), + state: SyncState::Synced, + }, + Ok(Outcome::Rejected(reason)) => { + tracing::warn!( + subtitle = %subtitle.display(), + %reason, + "alass sync rejected as implausible, keeping the unsynced original" + ); + Settled { + content: None, + state: SyncState::Rejected, + } + } + Err(error) => { + tracing::warn!( + subtitle = %subtitle.display(), + %error, + "alass sync could not run, leaving the subtitle unsynced" + ); + Settled { + content: None, + state: SyncState::NotRun, + } + } + } + } + /// Spawn `alass` and wait for it to write its output. async fn run(&self, video: &Path, subtitle: &Path, output: &Path) -> Result<()> { let mut command = Command::new(&self.binary); @@ -177,6 +219,33 @@ impl Syncer { } } +/// What settling a sync ended up recording, once an unusable `alass` and an +/// implausible result are both folded into "did not change anything" (§15). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum SyncState { + /// `alass` could not be run to a plausible conclusion — binary missing, + /// timed out, or exited non-zero. Not the same as [`Self::Rejected`]: + /// nothing here says the *original* is implausible, only that nothing + /// checked. + NotRun, + /// Ran, and the result was accepted. + Synced, + /// Ran, and the result was implausible. The unsynced original stands and + /// the file is flagged. + Rejected, +} + +/// The result of [`Syncer::settle`]: what belongs on disk, and what to +/// record. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Settled { + /// The synced text, only when it differs from what is already at the + /// subtitle path. `None` means the file `settle` was given is already + /// correct — the original, untouched. + pub content: Option, + pub state: SyncState, +} + /// What one sync run decided. #[derive(Clone, Debug, PartialEq, Eq)] pub enum Outcome { @@ -250,7 +319,7 @@ mod tests { use tokio::io::AsyncWriteExt; - use super::{plausible, Outcome, Rejection, Syncer, MAX_SHIFT}; + use super::{plausible, Outcome, Rejection, SyncState, Syncer, MAX_SHIFT}; use crate::error::Error; use crate::srt::Cue; @@ -505,6 +574,58 @@ mod tests { ); } + #[tokio::test] + async fn settle_returns_the_synced_content_to_write() { + let dir = tempfile::tempdir().expect("test setup and fake binary succeed"); + let binary = fake_binary( + dir.path(), + "printf '1\\n00:00:06,000 --> 00:00:07,500\\nola\\n\\n2\\n00:00:08,000 --> 00:00:09,000\\ntext\\n' > \"$3\"\n", + ) + .await; + let subtitle = write_subtitle(dir.path(), "in.srt", PLAIN).await; + + let settled = Syncer::new() + .with_binary(&binary) + .settle(dir.path().join("video.mkv"), &subtitle) + .await; + + assert_eq!(settled.state, SyncState::Synced); + assert!(settled.content.is_some()); + } + + #[tokio::test] + async fn settle_on_rejection_keeps_the_original_and_flags_it() { + let dir = tempfile::tempdir().expect("test setup and fake binary succeed"); + let binary = fake_binary( + dir.path(), + "printf '1\\n00:01:40,000 --> 00:01:41,500\\nola\\n\\n2\\n00:01:42,000 --> 00:01:43,000\\ntext\\n' > \"$3\"\n", + ) + .await; + let subtitle = write_subtitle(dir.path(), "in.srt", PLAIN).await; + + let settled = Syncer::new() + .with_binary(&binary) + .settle(dir.path().join("video.mkv"), &subtitle) + .await; + + assert_eq!(settled.state, SyncState::Rejected); + assert_eq!(settled.content, None); + } + + #[tokio::test] + async fn settle_on_a_missing_binary_degrades_to_not_run() { + let dir = tempfile::tempdir().expect("test setup and fake binary succeed"); + let subtitle = write_subtitle(dir.path(), "in.srt", PLAIN).await; + + let settled = Syncer::new() + .with_binary("alass-not-installed") + .settle(dir.path().join("video.mkv"), &subtitle) + .await; + + assert_eq!(settled.state, SyncState::NotRun); + assert_eq!(settled.content, None); + } + #[test] fn rejections_render_as_a_sentence() { assert_eq!( From f06e0e94bcfefdb232d4966adb4163a05a094fd1 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Tue, 25 Aug 2026 01:24:09 +0100 Subject: [PATCH 2/2] feat(arr): sync manually grabbed subtitles with alass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The grab handler runs alass before recording the row, replacing the sidecar with the synced text on acceptance and flagging it as rejected otherwise (§15). Wires a Syncer into AppState, defaulting to alass on PATH; the daemon binary points it at config.alass_path. --- crates/arr-api/src/state.rs | 16 ++++- crates/arr-api/src/subtitles.rs | 113 +++++++++++++++++++++++++++++++- crates/arr-daemon/src/main.rs | 3 +- 3 files changed, 128 insertions(+), 4 deletions(-) diff --git a/crates/arr-api/src/state.rs b/crates/arr-api/src/state.rs index d79c72e..b03ce1d 100644 --- a/crates/arr-api/src/state.rs +++ b/crates/arr-api/src/state.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use std::time::Duration; use arr_db::Db; -use arr_subs::{Backend, Provider}; +use arr_subs::{Backend, Provider, Syncer}; use tokio::sync::mpsc; /// The TMDB API root. Not a bootstrap setting (DESIGN.md §10) — only the key @@ -79,6 +79,7 @@ pub struct AppState { pending_metadata_commands: Arc>>, subtitle_providers: Arc>>, translation_backends: Arc>>, + syncer: Syncer, } /// Work explicitly requested through the movie API. @@ -151,6 +152,7 @@ impl AppState { pending_metadata_commands: Arc::new(tokio::sync::Mutex::new(pending_metadata_commands)), subtitle_providers: Arc::new(Vec::new()), translation_backends: Arc::new(Vec::new()), + syncer: Syncer::default(), }) } @@ -201,6 +203,18 @@ impl AppState { .find(|backend| backend.id().as_str() == id) } + /// Attach the `alass` binary this deployment runs (§15). Defaults to + /// resolving `alass` from `PATH`. + #[must_use] + pub fn with_syncer(mut self, syncer: Syncer) -> Self { + self.syncer = syncer; + self + } + + pub(crate) fn syncer(&self) -> &Syncer { + &self.syncer + } + /// Wait for the next manual movie action in the daemon's reconcile loop. /// /// # Errors diff --git a/crates/arr-api/src/subtitles.rs b/crates/arr-api/src/subtitles.rs index a62a3e8..3747dc1 100644 --- a/crates/arr-api/src/subtitles.rs +++ b/crates/arr-api/src/subtitles.rs @@ -645,13 +645,19 @@ pub async fn grab( let text = srt_text(&fetched)?; write_sidecar(&destination, &text).await?; + let sync = state.syncer().settle(&target.path, &destination).await; + if let Some(synced) = &sync.content { + write_sidecar(&destination, synced).await?; + } + let mut record = arr_db::NewSubtitleFile::fetched( target.media_file_id, &language.to_string(), &input.provider, &input.candidate_id, &destination.to_string_lossy(), - ); + ) + .sync(db_sync_state(sync.state)); if input.forced { record = record.forced(); } @@ -860,6 +866,19 @@ fn srt_text(fetched: &arr_subs::Fetched) -> Result { .map_err(|error| ApiError::SubtitleUpstream(error.to_string())) } +/// Map `arr_subs`'s three-state sync result onto the column pair `arr_db` +/// stores it as. `Syncer::settle` already folded an unusable `alass` and an +/// implausible result together into "nothing changed" — this is just the +/// vocabulary switch between the crate that ran `alass` and the one that +/// persists what it decided. +const fn db_sync_state(state: arr_subs::SyncState) -> arr_db::SubtitleSync { + match state { + arr_subs::SyncState::NotRun => arr_db::SubtitleSync::NotRun, + arr_subs::SyncState::Synced => arr_db::SubtitleSync::Synced, + arr_subs::SyncState::Rejected => arr_db::SubtitleSync::Rejected, + } +} + /// Write a sidecar whole or not at all, so Jellyfin never reads a half file. async fn write_sidecar(destination: &Path, text: &str) -> Result<(), ApiError> { let failure = |path: &Path, error: std::io::Error| { @@ -906,6 +925,7 @@ async fn finish( #[cfg(test)] #[allow(clippy::too_many_lines)] mod tests { + use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; use std::sync::Arc; @@ -915,6 +935,7 @@ mod tests { ProviderId, SearchFuture, SearchRequest, SubtitleFormat, TranslateFuture, TranslatedCue, }; use axum::http::StatusCode; + use tokio::io::AsyncWriteExt; use crate::{router, AppState, Upstreams}; @@ -1096,6 +1117,22 @@ mod tests { async fn application( providers: Vec>, backends: Vec>, + ) -> Fixture { + build_fixture(providers, backends, arr_subs::Syncer::default()).await + } + + async fn application_with_syncer( + providers: Vec>, + backends: Vec>, + syncer: arr_subs::Syncer, + ) -> Fixture { + build_fixture(providers, backends, syncer).await + } + + async fn build_fixture( + providers: Vec>, + backends: Vec>, + syncer: arr_subs::Syncer, ) -> Fixture { let dir = tempfile::tempdir().expect("tempdir"); let database = arr_db::Db::connect(dir.path().join("arr.db")) @@ -1137,7 +1174,8 @@ mod tests { .expect("state") .with_database(database) .with_subtitle_providers(providers) - .with_translation_backends(backends); + .with_translation_backends(backends) + .with_syncer(syncer); let listener = tokio::net::TcpListener::bind("127.0.0.1:0") .await @@ -1350,6 +1388,77 @@ mod tests { assert_eq!(fixture.subtitle_rows().await.len(), 1); } + /// Write an executable fake `alass`. Its body receives the subtitle, + /// video and output paths as `$1`, `$2`, `$3`. + async fn fake_alass(dir: &std::path::Path, body: &str) -> PathBuf { + let path = dir.join("alass"); + let mut file = tokio::fs::File::create(&path).await.expect("fake alass"); + file.write_all(b"#!/bin/sh\n").await.expect("fake alass"); + file.write_all(body.as_bytes()).await.expect("fake alass"); + file.sync_all().await.expect("fake alass"); + drop(file); + tokio::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + .await + .expect("fake alass"); + path + } + + /// §15: `alass` runs on every fetched subtitle (#214), and an accepted + /// shift replaces the sidecar's content before the row is recorded. + #[tokio::test] + async fn a_grab_writes_the_synced_content_when_alass_accepts_it() { + let dir = tempfile::tempdir().expect("tempdir"); + // The fetched cue starts at 1s (`SRT`); a 5s shift is within §15's + // 60-second bound. + let binary = fake_alass( + dir.path(), + "printf '1\\n00:00:06,000 --> 00:00:07,000\\nola\\n' > \"$3\"\n", + ) + .await; + let fixture = application_with_syncer( + vec![Arc::new(StubProvider::new("opensubtitles"))], + vec![], + arr_subs::Syncer::new().with_binary(&binary), + ) + .await; + + let (status, body) = grab(&fixture, pt()).await; + assert_eq!(status, StatusCode::CREATED, "{body}"); + assert_eq!(body["sync"], "synced"); + let sidecar = PathBuf::from(body["path"].as_str().expect("path")); + assert_eq!( + tokio::fs::read_to_string(&sidecar).await.expect("sidecar"), + "1\n00:00:06,000 --> 00:00:07,000\nola\n" + ); + } + + /// §15: an implausible shift keeps the unsynced original and flags the + /// file rather than failing the grab. + #[tokio::test] + async fn a_grab_keeps_the_original_when_alass_is_implausible() { + let dir = tempfile::tempdir().expect("tempdir"); + let binary = fake_alass( + dir.path(), + "printf '1\\n00:05:00,000 --> 00:05:01,000\\nola\\n' > \"$3\"\n", + ) + .await; + let fixture = application_with_syncer( + vec![Arc::new(StubProvider::new("opensubtitles"))], + vec![], + arr_subs::Syncer::new().with_binary(&binary), + ) + .await; + + let (status, body) = grab(&fixture, pt()).await; + assert_eq!(status, StatusCode::CREATED, "{body}"); + assert_eq!(body["sync"], "rejected"); + let sidecar = PathBuf::from(body["path"].as_str().expect("path")); + assert_eq!( + tokio::fs::read_to_string(&sidecar).await.expect("sidecar"), + SRT + ); + } + /// Sidecars are SRT (§15), so a provider serving ASS or VTT is converted /// on the way to disk (#213) rather than refused. #[tokio::test] diff --git a/crates/arr-daemon/src/main.rs b/crates/arr-daemon/src/main.rs index 09ec5f3..45cf3f4 100644 --- a/crates/arr-daemon/src/main.rs +++ b/crates/arr-daemon/src/main.rs @@ -150,7 +150,8 @@ async fn run() -> Result<(), Error> { config.opensubtitles_api_key, config.opensubtitles_username, config.opensubtitles_password, - )); + )) + .with_syncer(arr_subs::Syncer::new().with_binary(config.alass_path.clone())); let app = arr_api::router(state.clone()) .merge(arr_compat::router(compat))