From c6906bffae449b624d634c1b10ab2c70f900085d Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Tue, 25 Aug 2026 08:29:45 +0100 Subject: [PATCH] feat(arr): make the OpenAI endpoint a live setting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The base URL and model move into a cell the backend re-reads per request, so an operator can repoint it without a restart. Migration 0028 adds the two columns; DESIGN.md §15 calls both database rows. Construction never depends on the API key — llama.cpp serves without one. --- .../migrations/0028_openai_endpoint.sql | 17 ++ crates/arr-subs/src/lib.rs | 8 + crates/arr-subs/src/openai.rs | 97 +++++++--- crates/arr-subs/src/openai_endpoint.rs | 168 ++++++++++++++++++ crates/arr-subs/tests/openai.rs | 55 +++++- 5 files changed, 318 insertions(+), 27 deletions(-) create mode 100644 crates/arr-db/migrations/0028_openai_endpoint.sql create mode 100644 crates/arr-subs/src/openai_endpoint.rs diff --git a/crates/arr-db/migrations/0028_openai_endpoint.sql b/crates/arr-db/migrations/0028_openai_endpoint.sql new file mode 100644 index 0000000..f8e0a07 --- /dev/null +++ b/crates/arr-db/migrations/0028_openai_endpoint.sql @@ -0,0 +1,17 @@ +-- #220. Where the OpenAI-compatible translation backend points, and which +-- model it names (DESIGN.md §15). That backend is not "OpenAI" — it is +-- anything speaking that request shape, `llama.cpp` and a local gateway +-- included — so which endpoint and which model are in use is something the +-- operator tries and changes, not a property of the deployment fixed at +-- start-up. Both replace bootstrap keys #216 added as an explicit stopgap. +-- +-- Only the API key stays in the environment (§10): it is a secret, and an +-- endpoint that needs no key at all is a valid configuration. + +ALTER TABLE subtitle_settings + -- NULL means the backend's own default, `https://api.openai.com/v1/`. + ADD COLUMN openai_base_url TEXT; + +ALTER TABLE subtitle_settings + -- NULL means the backend's own default model. + ADD COLUMN openai_model TEXT; diff --git a/crates/arr-subs/src/lib.rs b/crates/arr-subs/src/lib.rs index c17fee1..1eacaf3 100644 --- a/crates/arr-subs/src/lib.rs +++ b/crates/arr-subs/src/lib.rs @@ -33,6 +33,10 @@ pub mod google; pub mod model; #[cfg(feature = "translate-openai")] pub mod openai; +// Not gated: the settings surface must hold and validate this backend's +// endpoint whether or not the build compiled the backend in — the +// `subtitle_settings` columns exist either way (#220). +pub mod openai_endpoint; pub mod opensubtitles; pub mod podnapisi; pub mod srt; @@ -54,6 +58,10 @@ pub use model::{ }; #[cfg(feature = "translate-openai")] pub use openai::{OpenAi, OpenAiConfig}; +pub use openai_endpoint::{ + OpenAiEndpoint, DEFAULT_BASE_URL as OPENAI_DEFAULT_BASE_URL, + DEFAULT_MODEL as OPENAI_DEFAULT_MODEL, +}; pub use opensubtitles::{moviehash, OpenSubtitles, OpenSubtitlesConfig}; pub use podnapisi::{Podnapisi, PodnapisiBuilder, DEFAULT_BASE_URL as PODNAPISI_DEFAULT_BASE_URL}; pub use srt::Cue; diff --git a/crates/arr-subs/src/openai.rs b/crates/arr-subs/src/openai.rs index 5453997..eb28f7d 100644 --- a/crates/arr-subs/src/openai.rs +++ b/crates/arr-subs/src/openai.rs @@ -3,10 +3,12 @@ //! One HTTP shape — the `/chat/completions` endpoint `OpenAI` defined — reaches //! the largest number of options: `OpenAI` itself, `OpenRouter`, Groq, a //! self-hosted gateway, or a local `llama.cpp` server. The model and base URL -//! are constructor arguments rather than fields on [`OpenAiConfig`]: which -//! engine runs, and where, is a database setting (DESIGN.md §15, issue #198). -//! Only the credential is bootstrap config or environment, per §10 — and even -//! that is optional, since a self-hosted gateway may not ask for one. +//! live in an [`OpenAiEndpoint`] cell re-read per request rather than on +//! [`OpenAiConfig`]: both are database rows the operator edits from +//! `/settings`, and an edit reaches this running backend without a restart +//! (DESIGN.md §15, issues #198 and #220). Only the credential is bootstrap +//! config or environment, per §10 — and even that is optional, since a +//! self-hosted gateway or a local `llama.cpp` may not ask for one. //! //! The batch travels as a JSON array of `{number, text}` objects, and the //! reply is asked for in the same shape: JSON round-trips a cue's embedded @@ -20,17 +22,17 @@ use std::fmt; use std::time::Duration; -use reqwest::{Client, StatusCode, Url}; +use reqwest::{Client, StatusCode}; use serde::{Deserialize, Serialize}; use arr_core::Language; +use crate::openai_endpoint::OpenAiEndpoint; use crate::translate::{ strip_code_fence, system_prompt, Backend, BackendId, Batch, Error, ProbeFuture, Result, TranslateFuture, TranslatedCue, }; -const DEFAULT_BASE_URL: &str = "https://api.openai.com/v1/"; const BACKEND_NAME: &str = "openai"; const USER_AGENT: &str = "arr v0.1.0"; const MAX_ERROR_BODY: usize = 512; @@ -58,35 +60,40 @@ impl fmt::Debug for OpenAiConfig { /// The OpenAI-compatible backend behind the [`Backend`] trait. pub struct OpenAi { http: Client, - base_url: Url, + endpoint: OpenAiEndpoint, config: OpenAiConfig, - model: String, id: BackendId, } // Hand-written: the config must never reach a log line. impl fmt::Debug for OpenAi { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let endpoint = self.endpoint.read(); f.debug_struct("OpenAi") - .field("base_url", &self.base_url.as_str()) - .field("model", &self.model) + .field("base_url", &endpoint.base_url.as_str()) + .field("model", &endpoint.model) .finish_non_exhaustive() } } impl OpenAi { - /// A client against `https://api.openai.com/v1/`. + /// A client at the backend's own defaults, ready to be repointed through + /// [`Self::endpoint`] once the settings row has been read. + /// + /// Construction never depends on the API key: `llama.cpp` serves without + /// authentication, so a base URL and no key is a valid configuration + /// (DESIGN.md §15). /// /// # Errors /// /// Fails if the HTTP client cannot be constructed. - pub fn new(model: impl Into, config: OpenAiConfig) -> Result { - Self::with_base_url(model, config, DEFAULT_BASE_URL) + pub fn new(config: OpenAiConfig) -> Result { + Self::with_endpoint(config, OpenAiEndpoint::new(None, None)?) } /// Point the client at another OpenAI-compatible endpoint — `OpenRouter`, /// Groq, a self-hosted gateway, a local `llama.cpp` server, or `wiremock` - /// in tests. + /// in tests — with a fixed model. /// /// # Errors /// @@ -96,24 +103,44 @@ impl OpenAi { config: OpenAiConfig, base_url: &str, ) -> Result { + let model = model.into(); + Self::with_endpoint( + config, + OpenAiEndpoint::new(Some(base_url), Some(model.as_str()))?, + ) + } + + /// A client re-reading `endpoint` for every request, so the settings API + /// can repoint it without a restart (#220). + /// + /// # Errors + /// + /// Fails if the HTTP client cannot be constructed. + pub fn with_endpoint(config: OpenAiConfig, endpoint: OpenAiEndpoint) -> Result { let id = BackendId::new(BACKEND_NAME); let http = Client::builder().build().map_err(|err| Error::Transport { backend: id.clone(), source: Box::new(err), })?; - let base_url = base_url.parse().map_err(|err| Error::Malformed { - backend: id.clone(), - detail: format!("bad base URL: {err}"), - })?; Ok(Self { http, - base_url, + endpoint, config, - model: model.into(), id, }) } + /// The cell this backend re-reads for every request. + /// + /// Whoever constructs the backend — the daemon, at startup — hands a + /// clone of this to the settings API, which stores `openai_base_url` and + /// `openai_model` into it on every edit. Same shape as #219's + /// `Command::timeout_cell`. + #[must_use] + pub fn endpoint(&self) -> OpenAiEndpoint { + self.endpoint.clone() + } + async fn check_status(&self, response: reqwest::Response) -> Result { match response.status() { StatusCode::OK => Ok(response), @@ -160,8 +187,11 @@ impl OpenAi { detail: format!("could not encode batch: {err}"), })?; + // Re-read per call: the settings API writes this cell when the + // operator edits `openai_base_url` or `openai_model` (#220). + let endpoint = self.endpoint.read(); let body = ChatRequest { - model: &self.model, + model: &endpoint.model, messages: vec![ ChatMessage { role: "system", @@ -174,7 +204,7 @@ impl OpenAi { ], }; - let url = self + let url = endpoint .base_url .join("chat/completions") .map_err(|err| Error::Malformed { @@ -191,7 +221,7 @@ impl OpenAi { request = request.bearer_auth(api_key); } - tracing::debug!(backend = %self.id, model = %self.model, "OpenAI translate request"); + tracing::debug!(backend = %self.id, model = %endpoint.model, "OpenAI translate request"); let response = request.send().await.map_err(|err| Error::Transport { backend: self.id.clone(), source: Box::new(err), @@ -266,9 +296,12 @@ impl Backend for OpenAi { /// `models` is a read-only listing: reachable and key accepted, nothing /// spent (#200). Endpoints that need no key are probed the same way, /// just without the header. + /// + /// The base URL is read from the live cell, not from a start-up copy, so + /// the lamp judges whatever endpoint is configured right now (#220). fn probe(&self) -> ProbeFuture<'_> { Box::pin(async move { - let mut url = self.base_url.clone(); + let mut url = self.endpoint.read().base_url; url.set_path(&format!("{}/models", url.path().trim_end_matches('/'))); let mut request = self.http.get(url); if let Some(key) = &self.config.api_key { @@ -357,11 +390,25 @@ mod tests { use arr_core::Language; use super::OpenAi; + use crate::openai_endpoint::DEFAULT_MODEL; use crate::translate::Backend; use crate::translate::{language_name, strip_code_fence, system_prompt}; fn backend() -> OpenAi { - OpenAi::new("gpt-4o-mini", super::OpenAiConfig { api_key: None }).expect("client builds") + OpenAi::new(super::OpenAiConfig { api_key: None }).expect("client builds") + } + + /// #220: the backend holds the cell, not a copy — a repoint between two + /// calls is what the settings handler relies on. + #[test] + fn the_backend_follows_its_endpoint_cell() { + let backend = backend(); + let cell = backend.endpoint(); + assert_eq!(cell.model(), DEFAULT_MODEL); + cell.set(Some("http://box:8080/v1/"), Some("llama")) + .expect("repoint"); + assert_eq!(backend.endpoint.model(), "llama"); + assert_eq!(backend.endpoint.base_url(), "http://box:8080/v1/"); } #[test] diff --git a/crates/arr-subs/src/openai_endpoint.rs b/crates/arr-subs/src/openai_endpoint.rs new file mode 100644 index 0000000..466a5b9 --- /dev/null +++ b/crates/arr-subs/src/openai_endpoint.rs @@ -0,0 +1,168 @@ +//! Where the OpenAI-compatible translation backend points, and which model +//! it names (`DESIGN.md` §15, issue #220). +//! +//! Its own module, and not behind `translate-openai`, because the settings +//! surface must hold and validate these two values whether or not this build +//! compiled the backend in: the `subtitle_settings` columns exist either way, +//! and an operator who edits them on a binary without the feature should get +//! the same 422 on a base URL that does not parse. + +use std::sync::{Arc, RwLock}; + +use reqwest::Url; + +use crate::translate::{BackendId, Error, Result}; + +const BACKEND_NAME: &str = "openai"; + +/// Where the backend points when `subtitle_settings.openai_base_url` is +/// NULL — the row's own comment calls that "the backend's own default". +pub const DEFAULT_BASE_URL: &str = "https://api.openai.com/v1/"; +/// The model named when `subtitle_settings.openai_model` is NULL. Unlike the +/// base URL this is a genuine guess rather than an obvious address, so it is +/// deliberately the cheap one: an operator who cares picks another. +pub const DEFAULT_MODEL: &str = "gpt-4o-mini"; + +/// Where an [`OpenAi`](crate::openai::OpenAi) backend is pointed and which model it names, in a +/// cell it re-reads for every request (issue #220). +/// +/// Both are `subtitle_settings` rows, not bootstrap config: that backend is +/// not `OpenAI` but anything speaking the same request shape — `llama.cpp`, +/// a local gateway, a hosted provider — and which one is in use is a thing +/// the operator tries and changes (DESIGN.md §15). Whoever constructs the +/// backend hands a clone of this to the settings API, which stores the +/// edited row into it; that is the same path from database to running +/// process #219 built for the remote-command timeout. +/// +/// `None` on either side means the backend's own default — +/// [`DEFAULT_BASE_URL`] and [`DEFAULT_MODEL`]. +#[derive(Clone, Debug)] +pub struct OpenAiEndpoint { + inner: Arc>, +} + +/// One resolved endpoint, cloned out of the lock before any await. +#[derive(Clone, Debug)] +pub(crate) struct Endpoint { + pub(crate) base_url: Url, + pub(crate) model: String, +} + +/// Resolve a stored pair into a usable endpoint. A blank string is absent, +/// not an endpoint named "": the settings form sends an empty field for +/// "use the default". +fn resolve(base_url: Option<&str>, model: Option<&str>) -> Result { + let raw = base_url + .map(str::trim) + .filter(|url| !url.is_empty()) + .unwrap_or(DEFAULT_BASE_URL); + // Every path this backend builds is relative to the base, and + // `Url::join` drops the last segment of a base that does not end in a + // slash — so `http://box:8080/v1` would reach `/chat/completions` + // rather than `/v1/chat/completions`. Normalising here means the + // operator may type it either way. + let normalised = if raw.ends_with('/') { + raw.to_owned() + } else { + format!("{raw}/") + }; + let base_url: Url = normalised.parse().map_err(|err| Error::Malformed { + backend: BackendId::new(BACKEND_NAME), + detail: format!("bad base URL: {err}"), + })?; + let model = model + .map(str::trim) + .filter(|model| !model.is_empty()) + .unwrap_or(DEFAULT_MODEL) + .to_owned(); + Ok(Endpoint { base_url, model }) +} + +impl OpenAiEndpoint { + /// A cell holding `base_url` and `model`, either of which may be `None` + /// for the backend's own default. + /// + /// # Errors + /// + /// [`Error::Malformed`] when `base_url` does not parse. + pub fn new(base_url: Option<&str>, model: Option<&str>) -> Result { + Ok(Self { + inner: Arc::new(RwLock::new(resolve(base_url, model)?)), + }) + } + + /// Repoint the running backend. Validated before anything is stored, so + /// a rejected edit leaves the previous endpoint in place. + /// + /// # Errors + /// + /// [`Error::Malformed`] when `base_url` does not parse. + /// + /// # Panics + /// + /// If a previous writer panicked while holding the lock. + pub fn set(&self, base_url: Option<&str>, model: Option<&str>) -> Result<()> { + let endpoint = resolve(base_url, model)?; + *self.inner.write().expect("endpoint lock poisoned") = endpoint; + Ok(()) + } + + /// The base URL in force right now, as the backend would use it. + #[must_use] + pub fn base_url(&self) -> String { + self.read().base_url.to_string() + } + + /// The model named in force right now. + #[must_use] + pub fn model(&self) -> String { + self.read().model + } + + pub(crate) fn read(&self) -> Endpoint { + self.inner.read().expect("endpoint lock poisoned").clone() + } +} + +#[cfg(test)] +mod tests { + use super::{OpenAiEndpoint, DEFAULT_BASE_URL, DEFAULT_MODEL}; + + /// #220: NULL columns mean the backend's own default, and a base URL the + /// operator typed without a trailing slash still resolves under its own + /// path rather than at the host root. + #[test] + fn an_absent_setting_falls_back_and_a_bare_base_url_keeps_its_path() { + let endpoint = OpenAiEndpoint::new(None, None).expect("defaults resolve"); + assert_eq!(endpoint.base_url(), DEFAULT_BASE_URL); + assert_eq!(endpoint.model(), DEFAULT_MODEL); + + // An empty field is "use the default", not an endpoint named "". + endpoint.set(Some(" "), Some("")).expect("blanks resolve"); + assert_eq!(endpoint.base_url(), DEFAULT_BASE_URL); + assert_eq!(endpoint.model(), DEFAULT_MODEL); + + endpoint + .set(Some("http://127.0.0.1:8080/v1"), Some("local-model")) + .expect("a local endpoint resolves"); + assert_eq!(endpoint.base_url(), "http://127.0.0.1:8080/v1/"); + assert_eq!(endpoint.model(), "local-model"); + } + + /// A rejected edit must leave the running backend where it was, rather + /// than half-applying the model and dropping the URL. + #[test] + fn a_base_url_that_does_not_parse_is_refused_and_changes_nothing() { + let endpoint = + OpenAiEndpoint::new(Some("http://box:8080/v1"), Some("m")).expect("endpoint resolves"); + let error = endpoint + .set(Some("not a url"), Some("other")) + .expect_err("a bad base URL is refused"); + assert!( + matches!(error, crate::translate::Error::Malformed { .. }), + "got {error:?}" + ); + assert_eq!(endpoint.base_url(), "http://box:8080/v1/"); + assert_eq!(endpoint.model(), "m"); + } +} diff --git a/crates/arr-subs/tests/openai.rs b/crates/arr-subs/tests/openai.rs index 5534d5f..ce7e7f2 100644 --- a/crates/arr-subs/tests/openai.rs +++ b/crates/arr-subs/tests/openai.rs @@ -16,7 +16,7 @@ use std::time::Duration; use arr_core::Language; use arr_subs::translate::{translate, Backend, Error}; -use arr_subs::{OpenAi, OpenAiConfig}; +use arr_subs::{OpenAi, OpenAiConfig, OpenAiEndpoint}; use wiremock::matchers::{header, method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -319,9 +319,60 @@ async fn an_unsupported_target_is_refused_before_any_request() { assert!(matches!(error, Error::UnsupportedTarget { .. })); } +/// #220: an edit of `openai_base_url` / `openai_model` reaches the running +/// backend without a restart — the next batch goes to the new endpoint and +/// names the new model, with no reconstruction in between. +#[tokio::test] +async fn repointing_the_endpoint_moves_the_next_batch() { + let first = MockServer::start().await; + let second = MockServer::start().await; + mount_reply( + &first, + ResponseTemplate::new(200).set_body_string(chat_response( + r#""[{\"number\": 1, \"text\": \"do primeiro\"}]""#, + )), + ) + .await; + mount_reply( + &second, + ResponseTemplate::new(200).set_body_string(chat_response( + r#""[{\"number\": 1, \"text\": \"do segundo\"}]""#, + )), + ) + .await; + + let endpoint = OpenAiEndpoint::new(Some(&format!("{}/v1/", first.uri())), Some("first-model")) + .expect("endpoint resolves"); + let backend = + OpenAi::with_endpoint(OpenAiConfig { api_key: None }, endpoint.clone()).expect("builds"); + + let cues = vec![arr_subs::translate::BatchCue { + number: 1, + text: "from the first".to_owned(), + }]; + let batch = arr_subs::translate::Batch { + source: Language::Other("en".to_owned()), + target: Language::PortuguesePortugal, + cues: cues.clone(), + }; + let out = backend.translate(&batch).await.expect("first batch"); + assert_eq!(out[0].text, "do primeiro"); + + endpoint + .set(Some(&format!("{}/v1/", second.uri())), Some("second-model")) + .expect("repoint"); + let out = backend.translate(&batch).await.expect("second batch"); + assert_eq!(out[0].text, "do segundo"); + + let sent = second.received_requests().await.expect("requests recorded"); + let body: serde_json::Value = + serde_json::from_slice(&sent.last().expect("one request").body).expect("json body"); + assert_eq!(body["model"], "second-model"); +} + #[test] fn the_backend_answers_to_openai() { - let id = OpenAi::new("gpt-4o-mini", OpenAiConfig { api_key: None }) + let id = OpenAi::new(OpenAiConfig { api_key: None }) .expect("client builds") .id(); assert_eq!(id.as_str(), "openai");