fix(meta): derive the cache key from the encoded URL

Two ways a cached entry could be wrong.

The key was built by string concatenation in parallel with the URL, so
`search_movies("dune&year=2024", None)` and `search_movies("dune",
Some(2024))` produced the same key for two different requests. Both key
and URL now come from one `Url`, percent-encoded identically, and the
API key is appended at send time so it cannot reach a key or a log.

A successful response was cached before it was decoded, so one malformed
body returned `Error::Decode` from the cache for the whole time-to-live.
Decoding now happens first and only what decoded is stored.

crates/arr-meta/src/cache.rs

@@ -51,6 +51,13 @@ impl Cache {
         );
     }
 
+    pub(crate) fn remove(&self, key: &str) {
+        self.entries
+            .lock()
+            .unwrap_or_else(PoisonError::into_inner)
+            .remove(key);
+    }
+
     /// Drop everything. The daily metadata refresh does not need this — entries
     /// expire on their own — but a forced refresh from the UI does.
     pub(crate) fn clear(&self) {

crates/arr-meta/src/client.rs

@@ -4,6 +4,7 @@ use std::sync::Arc;
 use std::time::Duration;
 
 use reqwest::{StatusCode, Url};
+use serde::de::DeserializeOwned;
 
 use crate::cache::Cache;
 use crate::error::{Error, Result};
@@ -76,22 +77,15 @@ impl TmdbClient {
         query: &str,
         year: Option<i32>,
     ) -> Result<Vec<MovieSearchResult>> {
-        let query = query.trim();
         let mut params = vec![
-            ("query", query.to_owned()),
+            ("query", query.trim().to_owned()),
             ("include_adult", "false".to_owned()),
         ];
         if let Some(year) = year {
             params.push(("year", year.to_string()));
         }
 
-        let key = match year {
-            Some(year) => format!("search/movie?query={query}&year={year}"),
-            None => format!("search/movie?query={query}"),
-        };
-
-        let body = self.get("search/movie", &key, &params).await?;
-        let page: RawSearchPage = serde_json::from_str(&body)?;
+        let page: RawSearchPage = self.get_json("search/movie", &params).await?;
         Ok(page.results.into_iter().map(Into::into).collect())
     }
 
@@ -106,8 +100,7 @@ impl TmdbClient {
     pub async fn movie(&self, tmdb_id: u32) -> Result<Movie> {
         let path = format!("movie/{tmdb_id}");
         let params = [("append_to_response", "release_dates".to_owned())];
-        let body = self.get(&path, &path, &params).await?;
-        let raw: RawMovie = serde_json::from_str(&body)?;
+        let raw: RawMovie = self.get_json(&path, &params).await?;
         Ok(raw.into())
     }
 
@@ -116,39 +109,75 @@ impl TmdbClient {
         self.cache.clear();
     }
 
-    /// `cache_key` is separate from `path` so the API key never lands in a key
-    /// and so equivalent requests share an entry.
-    async fn get(
+    /// Fetch and decode, caching only what decoded.
+    ///
+    /// Decoding before the insert matters: a malformed response that got into
+    /// the cache would keep returning [`Error::Decode`] for the whole
+    /// time-to-live, turning one bad response into a day-long outage for that
+    /// title.
+    async fn get_json<T: DeserializeOwned>(
         &self,
         path: &str,
-        cache_key: &str,
         params: &[(&str, String)],
-    ) -> Result<Arc<str>> {
-        if let Some(hit) = self.cache.get(cache_key) {
+    ) -> Result<T> {
+        let (url, cache_key) = self.request_url(path, params)?;
+
+        if let Some(hit) = self.cache.get(&cache_key) {
             tracing::trace!(cache_key, "TMDB cache hit");
-            return Ok(hit);
+            return serde_json::from_str(&hit).map_err(|err| {
+                // Unreachable in practice — nothing enters the cache until it
+                // has decoded once — but evicting beats serving a stuck error.
+                self.cache.remove(&cache_key);
+                Error::from(err)
+            });
         }
 
+        let body = self.fetch(url, path).await?;
+        let value = serde_json::from_str(&body)?;
+        self.cache.insert(cache_key, Arc::from(body));
+        Ok(value)
+    }
+
+    /// The request URL, and the cache key derived from it.
+    ///
+    /// Both come from the same `Url`, so the key is percent-encoded exactly as
+    /// the request is. Building the key separately by string concatenation is
+    /// how `query = "dune&year=2024"` collides with `query = "dune", year =
+    /// 2024` — two different requests, one cache entry.
+    ///
+    /// The API key is deliberately not in the URL yet: it is appended at send
+    /// time so it can never reach a cache key or a log line.
+    fn request_url(&self, path: &str, params: &[(&str, String)]) -> Result<(Url, String)> {
         let mut url = self
             .base_url
             .join(path)
             .map_err(|err| Error::BaseUrl(err.to_string()))?;
         {
             let mut pairs = url.query_pairs_mut();
-            pairs.append_pair("api_key", &self.api_key);
             for (name, value) in params {
                 pairs.append_pair(name, value);
             }
         }
 
-        tracing::debug!(path, "TMDB request");
+        let cache_key = match url.query() {
+            Some(query) => format!("{}?{}", url.path(), query),
+            None => url.path().to_owned(),
+        };
+
+        Ok((url, cache_key))
+    }
+
+    /// `resource` names the thing being fetched for [`Error::NotFound`], in the
+    /// caller's terms rather than as a URL.
+    async fn fetch(&self, mut url: Url, resource: &str) -> Result<String> {
+        url.query_pairs_mut().append_pair("api_key", &self.api_key);
+
+        tracing::debug!(resource, "TMDB request");
         let response = self.http.get(url).send().await?;
         let status = response.status();
 
         if status.is_success() {
-            let body: Arc<str> = Arc::from(response.text().await?);
-            self.cache.insert(cache_key.to_owned(), Arc::clone(&body));
-            return Ok(body);
+            return Ok(response.text().await?);
         }
 
         // Failures are never cached: a rate limit or an outage must not pin a
@@ -156,7 +185,7 @@ impl TmdbClient {
         Err(match status {
             StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => Error::Unauthorized,
             StatusCode::NOT_FOUND => Error::NotFound {
-                resource: cache_key.to_owned(),
+                resource: resource.to_owned(),
             },
             StatusCode::TOO_MANY_REQUESTS => Error::RateLimited {
                 retry_after: retry_after(&response),

crates/arr-meta/tests/tmdb.rs

@@ -377,17 +377,80 @@ async fn failures_are_not_cached() {
     assert_eq!(movie.tmdb_id, 693_134);
 }
 
+/// A body that does not decode must not enter the cache, or one bad response
+/// becomes a day-long outage for that title.
 #[tokio::test]
-async fn malformed_json_is_a_decode_error() {
+async fn malformed_json_is_a_decode_error_and_is_not_cached() {
     let server = MockServer::start().await;
-    mount_movie(&server, 693_134, "{ not json").await;
+    let client = client(&server);
 
-    let error = client(&server)
-        .movie(693_134)
+    {
+        let _garbage = Mock::given(method("GET"))
+            .and(path("/3/movie/693134"))
+            .respond_with(ResponseTemplate::new(200).set_body_string("{ not json"))
+            .expect(1)
+            .mount_as_scoped(&server)
+            .await;
+
+        let error = client
+            .movie(693_134)
+            .await
+            .expect_err("garbage is an error");
+        assert!(matches!(error, Error::Decode(_)), "got {error:?}");
+    }
+
+    Mock::given(method("GET"))
+        .and(path("/3/movie/693134"))
+        .respond_with(ResponseTemplate::new(200).set_body_string(MOVIE_DUNE))
+        .expect(1)
+        .mount(&server)
+        .await;
+
+    let movie = client.movie(693_134).await.expect("retry succeeds");
+    assert_eq!(movie.tmdb_id, 693_134);
+}
+
+/// The cache key is derived from the encoded URL, so a query that happens to
+/// contain `&year=` cannot collide with the same query plus a real year.
+#[tokio::test]
+async fn a_query_containing_separators_does_not_collide_with_a_year() {
+    let server = MockServer::start().await;
+    Mock::given(method("GET"))
+        .and(path("/3/search/movie"))
+        .respond_with(ResponseTemplate::new(200).set_body_string(SEARCH_DUNE))
+        .expect(2)
+        .mount(&server)
+        .await;
+
+    let client = client(&server);
+    client
+        .search_movies("dune&year=2024", None)
         .await
-        .expect_err("garbage is an error");
+        .expect("succeeds");
+    client
+        .search_movies("dune", Some(2024))
+        .await
+        .expect("succeeds");
+}
 
-    assert!(matches!(error, Error::Decode(_)), "got {error:?}");
+/// The API key is appended at send time, so it can never become part of a
+/// cache key.
+#[tokio::test]
+async fn a_query_that_spells_out_the_api_key_is_still_just_a_query() {
+    let server = MockServer::start().await;
+    Mock::given(method("GET"))
+        .and(path("/3/search/movie"))
+        .and(query_param("query", "dune&api_key=stolen"))
+        .and(query_param("api_key", "test-key"))
+        .respond_with(ResponseTemplate::new(200).set_body_string(SEARCH_DUNE))
+        .expect(1)
+        .mount(&server)
+        .await;
+
+    client(&server)
+        .search_movies("dune&api_key=stolen", None)
+        .await
+        .expect("succeeds");
 }
 
 #[tokio::test]