From aec48606ee78fb279867db6a5cd5d0c07ed393d6 Mon Sep 17 00:00:00 2001 From: ahanot Date: Wed, 29 Apr 2026 15:49:28 -0500 Subject: [PATCH 1/6] feat(ads-client): respect server max-age in cache TTL resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve effective TTL by priority — explicit per-request TTL wins, otherwise the response's Cache-Control: max-age is used, otherwise the configured default_ttl. Result is capped at 7 days for safety. Previously the layer took the min of all three, which collapsed to the short default_ttl in practice and ignored the server's max-age signal. AC-103 --- CHANGELOG.md | 3 + .../ads-client/docs/usage-javascript.md | 18 ++--- components/ads-client/docs/usage-kotlin.md | 18 ++--- components/ads-client/docs/usage-swift.md | 18 ++--- components/ads-client/src/http_cache.rs | 60 +++++++++++++-- .../ads-client/src/http_cache/builder.rs | 4 +- .../src/http_cache/cache_control.rs | 74 +++++++++++++++++-- .../ads-client/src/http_cache/strategy.rs | 11 ++- 8 files changed, 156 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e11857153..3eb7f02d20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ ## ✨ What's New ✨ +### Ads Client +* HTTP cache TTL is now resolved by priority — explicit per-request TTL (if any) wins, otherwise the response's `Cache-Control: max-age` is used, otherwise the configured `default_ttl`. The chosen value is capped at 7 days. Previously the layer took the minimum of all three, which effectively ignored the server's `max-age` signal. + ### Remote Settings * Add uptake telemetry support ([#7288](https://github.com/mozilla/application-services/pull/7288)) * Add v2 routes ([#7339](https://github.com/mozilla/application-services/pull/7339)) diff --git a/components/ads-client/docs/usage-javascript.md b/components/ads-client/docs/usage-javascript.md index aacca4b0c9..2e45de6647 100644 --- a/components/ads-client/docs/usage-javascript.md +++ b/components/ads-client/docs/usage-javascript.md @@ -544,19 +544,17 @@ It reduces redundant network traffic and improves latency for repeated or identi ### Cache Lifecycle -Each network response can be stored in the cache with an associated effective TTL, computed as: +Each network response can be stored in the cache with an associated effective TTL, +resolved by priority (highest to lowest): -``` -effective_ttl = min(server_max_age, client_default_ttl, per_request_ttl) -``` - -where: +1. `per_request_ttl` — caller-provided override on `MozAdsRequestCachePolicy`. +2. `server_max_age` — value of the HTTP `Cache-Control: max-age=N` header on the response. +3. `client_default_ttl` — configured on `MozAdsCacheConfig`. -- `server_max_age` comes from the HTTP `Cache-Control: max-age=N` header (if present), -- `client_default_ttl` is set in `MozAdsCacheConfig`, -- `per_request_ttl` is an optional override set in `MozAdsRequestCachePolicy`. +The chosen value is capped at 7 days for safety. -If the effective TTL resolves to 0 seconds, the response is not cached. +If the effective TTL resolves to 0 seconds (e.g. `Cache-Control: max-age=0`), +the response is not cached. ### Configuring The Cache diff --git a/components/ads-client/docs/usage-kotlin.md b/components/ads-client/docs/usage-kotlin.md index 9db60c6410..df83ad1000 100644 --- a/components/ads-client/docs/usage-kotlin.md +++ b/components/ads-client/docs/usage-kotlin.md @@ -501,19 +501,17 @@ It reduces redundant network traffic and improves latency for repeated or identi ### Cache Lifecycle -Each network response can be stored in the cache with an associated effective TTL, computed as: +Each network response can be stored in the cache with an associated effective TTL, +resolved by priority (highest to lowest): -``` -effective_ttl = min(server_max_age, client_default_ttl, per_request_ttl) -``` - -where: +1. `per_request_ttl` — caller-provided override on `MozAdsRequestCachePolicy`. +2. `server_max_age` — value of the HTTP `Cache-Control: max-age=N` header on the response. +3. `client_default_ttl` — configured on `MozAdsCacheConfig`. -- `server_max_age` comes from the HTTP `Cache-Control: max-age=N` header (if present), -- `client_default_ttl` is set in `MozAdsCacheConfig`, -- `per_request_ttl` is an optional override set in `MozAdsRequestCachePolicy`. +The chosen value is capped at 7 days for safety. -If the effective TTL resolves to 0 seconds, the response is not cached. +If the effective TTL resolves to 0 seconds (e.g. `Cache-Control: max-age=0`), +the response is not cached. ### Configuring The Cache diff --git a/components/ads-client/docs/usage-swift.md b/components/ads-client/docs/usage-swift.md index d3ac5741dc..314fb4743a 100644 --- a/components/ads-client/docs/usage-swift.md +++ b/components/ads-client/docs/usage-swift.md @@ -501,19 +501,17 @@ It reduces redundant network traffic and improves latency for repeated or identi ### Cache Lifecycle -Each network response can be stored in the cache with an associated effective TTL, computed as: +Each network response can be stored in the cache with an associated effective TTL, +resolved by priority (highest to lowest): -``` -effective_ttl = min(server_max_age, client_default_ttl, per_request_ttl) -``` - -where: +1. `per_request_ttl` — caller-provided override on `MozAdsRequestCachePolicy`. +2. `server_max_age` — value of the HTTP `Cache-Control: max-age=N` header on the response. +3. `client_default_ttl` — configured on `MozAdsCacheConfig`. -- `server_max_age` comes from the HTTP `Cache-Control: max-age=N` header (if present), -- `client_default_ttl` is set in `MozAdsCacheConfig`, -- `per_request_ttl` is an optional override set in `MozAdsRequestCachePolicy`. +The chosen value is capped at 7 days for safety. -If the effective TTL resolves to 0 seconds, the response is not cached. +If the effective TTL resolves to 0 seconds (e.g. `Cache-Control: max-age=0`), +the response is not cached. ### Configuring The Cache diff --git a/components/ads-client/src/http_cache.rs b/components/ads-client/src/http_cache.rs index f9d0af7704..3160e36407 100644 --- a/components/ads-client/src/http_cache.rs +++ b/components/ads-client/src/http_cache.rs @@ -28,6 +28,10 @@ pub use self::request_hash::RequestHash; use std::path::Path; use std::time::Duration; +pub(crate) const DEFAULT_TTL: Duration = Duration::from_secs(300); +pub(crate) const MIN_TTL: Duration = Duration::from_secs(1); +pub(crate) const MAX_TTL: Duration = Duration::from_secs(60 * 60 * 24 * 7); // 7 days + pub type HttpCacheSendResult = std::result::Result<(Response, Vec), viaduct::ViaductError>; @@ -84,13 +88,15 @@ impl HttpCache { CachePolicy::CacheFirst { ttl } => CacheFirst { hash, request, - ttl: ttl.unwrap_or(self.default_ttl), + explicit_ttl: *ttl, + default_ttl: self.default_ttl, } .apply(client, &self.store), CachePolicy::NetworkFirst { ttl } => NetworkFirst { hash, request, - ttl: ttl.unwrap_or(self.default_ttl), + explicit_ttl: *ttl, + default_ttl: self.default_ttl, } .apply(client, &self.store), }?; @@ -321,13 +327,13 @@ mod tests { } #[test] - fn ttl_resolution_min_of_server_request_default() { + fn ttl_resolution_explicit_overrides_server_max_age() { viaduct_dev::init_backend_dev(); let _m = mockito::mock("POST", "/ads") .with_status(200) .with_header("content-type", "application/json") - .with_header("cache-control", "max-age=1") // Set max age to 1 second + .with_header("cache-control", "max-age=1") // Server says 1 second .with_body(r#"{"ok":true}"#) .expect(1) .create(); @@ -336,23 +342,61 @@ mod tests { let req = make_post_request(); let hash = RequestHash::new(&req); let policy = CachePolicy::CacheFirst { - ttl: Some(Duration::from_secs(20)), // 20 second ttl specified vs the cache's default of 300s + ttl: Some(Duration::from_secs(20)), // Caller asked for 20s }; let client = make_client(); - // Store ttl should resolve to 1s as specified by response headers + // Caller's explicit TTL wins over the server's max-age. let (_, outcomes) = cache.send_with_policy(&client, req, &policy).unwrap(); assert!(matches!(outcomes.last().unwrap(), CacheOutcome::MissStored)); - // After ~>1s, cleanup should remove it + // Past the server max-age (1s) but still under the explicit TTL (20s) — entry is still there. cache.store.get_clock().advance(2); cache.store.delete_expired_entries().unwrap(); + assert!(cache.store.lookup(&hash).unwrap().is_some()); + + // Past the explicit TTL — entry should be expired. + cache.store.get_clock().advance(20); + cache.store.delete_expired_entries().unwrap(); + assert!(cache.store.lookup(&hash).unwrap().is_none()); + } + + #[test] + fn ttl_resolution_uses_server_max_age_when_no_explicit_override() { + viaduct_dev::init_backend_dev(); + + let _m = mockito::mock("POST", "/ads") + .with_status(200) + .with_header("content-type", "application/json") + .with_header("cache-control", "max-age=2") // Server says 2 seconds + .with_body(r#"{"ok":true}"#) + .expect(1) + .create(); + + // Configured default is 300s — should be ignored in favor of server max-age. + let cache = make_cache_with_ttl(300); + let req = make_post_request(); + let hash = RequestHash::new(&req); + + let client = make_client(); + let (_, outcomes) = cache + .send_with_policy(&client, req, &CachePolicy::default()) + .unwrap(); + assert!(matches!(outcomes.last().unwrap(), CacheOutcome::MissStored)); + + // Still cached at ~1s. + cache.store.get_clock().advance(1); + cache.store.delete_expired_entries().unwrap(); + assert!(cache.store.lookup(&hash).unwrap().is_some()); + // Expired after exceeding server max-age. + cache.store.get_clock().advance(2); + cache.store.delete_expired_entries().unwrap(); assert!(cache.store.lookup(&hash).unwrap().is_none()); } #[test] - fn ttl_resolution_request_overrides_default_when_smaller() { + fn ttl_resolution_explicit_overrides_default() { viaduct_dev::init_backend_dev(); let _m = mockito::mock("POST", "/ads") diff --git a/components/ads-client/src/http_cache/builder.rs b/components/ads-client/src/http_cache/builder.rs index 97f92f004b..4d481fd985 100644 --- a/components/ads-client/src/http_cache/builder.rs +++ b/components/ads-client/src/http_cache/builder.rs @@ -7,18 +7,16 @@ use crate::http_cache::HttpCache; use super::bytesize::ByteSize; use super::connection_initializer::HttpCacheConnectionInitializer; use super::store::HttpCacheStore; +use super::{DEFAULT_TTL, MAX_TTL, MIN_TTL}; use rusqlite::Connection; use sql_support::open_database; use std::path::PathBuf; use std::time::Duration; const DEFAULT_MAX_SIZE: ByteSize = ByteSize::mib(10); -const DEFAULT_TTL: Duration = Duration::from_secs(300); const MIN_CACHE_SIZE: ByteSize = ByteSize::kib(1); const MAX_CACHE_SIZE: ByteSize = ByteSize::mib(100); -const MIN_TTL: Duration = Duration::from_secs(1); -const MAX_TTL: Duration = Duration::from_secs(60 * 60 * 24 * 7); // 7 days #[derive(Debug, thiserror::Error)] pub enum HttpCacheBuilderError { diff --git a/components/ads-client/src/http_cache/cache_control.rs b/components/ads-client/src/http_cache/cache_control.rs index 526549dcaa..409d504082 100644 --- a/components/ads-client/src/http_cache/cache_control.rs +++ b/components/ads-client/src/http_cache/cache_control.rs @@ -6,6 +6,8 @@ use serde::{Deserialize, Serialize}; use std::time::Duration; use viaduct::{header_names, Response}; +use super::MAX_TTL; + #[derive(Clone, Debug, Deserialize, Serialize)] pub struct CacheControl { pub max_age: Option, @@ -51,11 +53,23 @@ impl CacheControl { !self.no_store } - pub fn effective_ttl(&self, requested_ttl: Duration) -> Duration { - match self.max_age { - Some(s) => std::cmp::min(requested_ttl, Duration::from_secs(s)), - None => requested_ttl, - } + /// Resolve the TTL to use when storing a response in the cache. + /// + /// Priority (highest to lowest): + /// 1. `explicit_ttl` — caller-provided per-request override. + /// 2. Server `Cache-Control: max-age` from this response. + /// 3. `default_ttl` — the cache's configured default. + /// + /// The resulting TTL is capped at [`MAX_TTL`] for safety. + pub fn effective_ttl( + &self, + explicit_ttl: Option, + default_ttl: Duration, + ) -> Duration { + let chosen = explicit_ttl + .or_else(|| self.max_age.map(Duration::from_secs)) + .unwrap_or(default_ttl); + std::cmp::min(chosen, MAX_TTL) } } @@ -104,4 +118,54 @@ mod tests { assert!(!directives.no_store); assert!(directives.should_cache()); } + + #[test] + fn effective_ttl_explicit_overrides_max_age_and_default() { + let directives = from_header(Some("max-age=3600")); + let ttl = directives.effective_ttl( + Some(Duration::from_secs(60)), + Duration::from_secs(300), + ); + assert_eq!(ttl, Duration::from_secs(60)); + } + + #[test] + fn effective_ttl_falls_back_to_max_age_when_no_explicit() { + let directives = from_header(Some("max-age=3600")); + let ttl = directives.effective_ttl(None, Duration::from_secs(300)); + assert_eq!(ttl, Duration::from_secs(3600)); + } + + #[test] + fn effective_ttl_falls_back_to_default_when_no_max_age_and_no_explicit() { + let directives = from_header(None); + let ttl = directives.effective_ttl(None, Duration::from_secs(300)); + assert_eq!(ttl, Duration::from_secs(300)); + } + + #[test] + fn effective_ttl_caps_max_age_at_max_ttl() { + // 30 days, well over MAX_TTL (7 days) + let directives = from_header(Some("max-age=2592000")); + let ttl = directives.effective_ttl(None, Duration::from_secs(300)); + assert_eq!(ttl, MAX_TTL); + } + + #[test] + fn effective_ttl_caps_explicit_at_max_ttl() { + let directives = from_header(None); + let ttl = directives.effective_ttl( + Some(Duration::from_secs(60 * 60 * 24 * 30)), + Duration::from_secs(300), + ); + assert_eq!(ttl, MAX_TTL); + } + + #[test] + fn effective_ttl_zero_max_age_yields_zero() { + // max-age=0 should propagate as zero so the strategy emits NoCache. + let directives = from_header(Some("max-age=0")); + let ttl = directives.effective_ttl(None, Duration::from_secs(300)); + assert_eq!(ttl, Duration::ZERO); + } } diff --git a/components/ads-client/src/http_cache/strategy.rs b/components/ads-client/src/http_cache/strategy.rs index 8368e9dc23..755218f962 100644 --- a/components/ads-client/src/http_cache/strategy.rs +++ b/components/ads-client/src/http_cache/strategy.rs @@ -13,7 +13,8 @@ use viaduct::{Client, Request}; pub struct CacheFirst { pub hash: RequestHash, pub request: Request, - pub ttl: Duration, + pub explicit_ttl: Option, + pub default_ttl: Duration, } impl CacheFirst { @@ -28,7 +29,8 @@ impl CacheFirst { let network = NetworkFirst { hash: self.hash, request: self.request, - ttl: self.ttl, + explicit_ttl: self.explicit_ttl, + default_ttl: self.default_ttl, }; let (response, mut network_outcomes) = network.apply(client, store)?; outcomes.append(&mut network_outcomes); @@ -39,7 +41,8 @@ impl CacheFirst { pub struct NetworkFirst { pub hash: RequestHash, pub request: Request, - pub ttl: Duration, + pub explicit_ttl: Option, + pub default_ttl: Duration, } impl NetworkFirst { @@ -47,7 +50,7 @@ impl NetworkFirst { let response = client.send_sync(self.request)?; let cache_control = CacheControl::from(&response); let outcome = if cache_control.should_cache() { - let ttl = cache_control.effective_ttl(self.ttl); + let ttl = cache_control.effective_ttl(self.explicit_ttl, self.default_ttl); if ttl.is_zero() { return Ok((response, vec![CacheOutcome::NoCache])); } From 3566183918e6e660671cebd548a4be17da7d31a5 Mon Sep 17 00:00:00 2001 From: ahanot Date: Wed, 29 Apr 2026 23:39:58 -0500 Subject: [PATCH 2/6] refactor(ads-client): extract TTL resolution from CacheControl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the priority-chain TTL resolution out of `CacheControl::effective_ttl` into a dedicated `ttl` module with a `resolve_ttl(TtlInputs { ... })` free function. The named-fields struct labels each input at the call site, making the source of every value (caller / server / config) clear. `CacheControl` is now back to its original job — parsing the response's Cache-Control header — and exposes `max_age_duration()` for callers that want a `Duration` instead of raw seconds. --- components/ads-client/src/http_cache.rs | 1 + .../src/http_cache/cache_control.rs | 71 +----------- .../ads-client/src/http_cache/strategy.rs | 7 +- components/ads-client/src/http_cache/ttl.rs | 105 ++++++++++++++++++ 4 files changed, 114 insertions(+), 70 deletions(-) create mode 100644 components/ads-client/src/http_cache/ttl.rs diff --git a/components/ads-client/src/http_cache.rs b/components/ads-client/src/http_cache.rs index 3160e36407..5ed4b100e1 100644 --- a/components/ads-client/src/http_cache.rs +++ b/components/ads-client/src/http_cache.rs @@ -11,6 +11,7 @@ mod outcome; mod request_hash; mod store; mod strategy; +mod ttl; use self::{ builder::HttpCacheBuilder, diff --git a/components/ads-client/src/http_cache/cache_control.rs b/components/ads-client/src/http_cache/cache_control.rs index 409d504082..a469ffa312 100644 --- a/components/ads-client/src/http_cache/cache_control.rs +++ b/components/ads-client/src/http_cache/cache_control.rs @@ -6,8 +6,6 @@ use serde::{Deserialize, Serialize}; use std::time::Duration; use viaduct::{header_names, Response}; -use super::MAX_TTL; - #[derive(Clone, Debug, Deserialize, Serialize)] pub struct CacheControl { pub max_age: Option, @@ -53,23 +51,8 @@ impl CacheControl { !self.no_store } - /// Resolve the TTL to use when storing a response in the cache. - /// - /// Priority (highest to lowest): - /// 1. `explicit_ttl` — caller-provided per-request override. - /// 2. Server `Cache-Control: max-age` from this response. - /// 3. `default_ttl` — the cache's configured default. - /// - /// The resulting TTL is capped at [`MAX_TTL`] for safety. - pub fn effective_ttl( - &self, - explicit_ttl: Option, - default_ttl: Duration, - ) -> Duration { - let chosen = explicit_ttl - .or_else(|| self.max_age.map(Duration::from_secs)) - .unwrap_or(default_ttl); - std::cmp::min(chosen, MAX_TTL) + pub fn max_age_duration(&self) -> Option { + self.max_age.map(Duration::from_secs) } } @@ -118,54 +101,4 @@ mod tests { assert!(!directives.no_store); assert!(directives.should_cache()); } - - #[test] - fn effective_ttl_explicit_overrides_max_age_and_default() { - let directives = from_header(Some("max-age=3600")); - let ttl = directives.effective_ttl( - Some(Duration::from_secs(60)), - Duration::from_secs(300), - ); - assert_eq!(ttl, Duration::from_secs(60)); - } - - #[test] - fn effective_ttl_falls_back_to_max_age_when_no_explicit() { - let directives = from_header(Some("max-age=3600")); - let ttl = directives.effective_ttl(None, Duration::from_secs(300)); - assert_eq!(ttl, Duration::from_secs(3600)); - } - - #[test] - fn effective_ttl_falls_back_to_default_when_no_max_age_and_no_explicit() { - let directives = from_header(None); - let ttl = directives.effective_ttl(None, Duration::from_secs(300)); - assert_eq!(ttl, Duration::from_secs(300)); - } - - #[test] - fn effective_ttl_caps_max_age_at_max_ttl() { - // 30 days, well over MAX_TTL (7 days) - let directives = from_header(Some("max-age=2592000")); - let ttl = directives.effective_ttl(None, Duration::from_secs(300)); - assert_eq!(ttl, MAX_TTL); - } - - #[test] - fn effective_ttl_caps_explicit_at_max_ttl() { - let directives = from_header(None); - let ttl = directives.effective_ttl( - Some(Duration::from_secs(60 * 60 * 24 * 30)), - Duration::from_secs(300), - ); - assert_eq!(ttl, MAX_TTL); - } - - #[test] - fn effective_ttl_zero_max_age_yields_zero() { - // max-age=0 should propagate as zero so the strategy emits NoCache. - let directives = from_header(Some("max-age=0")); - let ttl = directives.effective_ttl(None, Duration::from_secs(300)); - assert_eq!(ttl, Duration::ZERO); - } } diff --git a/components/ads-client/src/http_cache/strategy.rs b/components/ads-client/src/http_cache/strategy.rs index 755218f962..d2d4698e4f 100644 --- a/components/ads-client/src/http_cache/strategy.rs +++ b/components/ads-client/src/http_cache/strategy.rs @@ -6,6 +6,7 @@ use super::cache_control::CacheControl; use super::request_hash::RequestHash; use super::store::HttpCacheStore; +use super::ttl::{resolve_ttl, TtlInputs}; use super::{CacheOutcome, HttpCacheSendResult}; use std::time::Duration; use viaduct::{Client, Request}; @@ -50,7 +51,11 @@ impl NetworkFirst { let response = client.send_sync(self.request)?; let cache_control = CacheControl::from(&response); let outcome = if cache_control.should_cache() { - let ttl = cache_control.effective_ttl(self.explicit_ttl, self.default_ttl); + let ttl = resolve_ttl(TtlInputs { + explicit: self.explicit_ttl, + server_max_age: cache_control.max_age_duration(), + default: self.default_ttl, + }); if ttl.is_zero() { return Ok((response, vec![CacheOutcome::NoCache])); } diff --git a/components/ads-client/src/http_cache/ttl.rs b/components/ads-client/src/http_cache/ttl.rs new file mode 100644 index 0000000000..cb92473622 --- /dev/null +++ b/components/ads-client/src/http_cache/ttl.rs @@ -0,0 +1,105 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this +* file, You can obtain one at http://mozilla.org/MPL/2.0/. +*/ + +use std::time::Duration; + +use super::MAX_TTL; + +/// Inputs to [`resolve_ttl`]. +/// +/// Each field comes from a different source: `explicit` from the caller, +/// `server_max_age` from the response's `Cache-Control` header, and +/// `default` from the cache's configuration. +pub(super) struct TtlInputs { + /// Per-request override provided by the caller, if any. + pub explicit: Option, + /// `Cache-Control: max-age` from the server response, if present. + pub server_max_age: Option, + /// The cache's configured default TTL. + pub default: Duration, +} + +/// Resolve the TTL to use when storing a response in the cache. +/// +/// Priority (highest to lowest): +/// 1. `explicit` — caller-provided per-request override. +/// 2. `server_max_age` — value of `Cache-Control: max-age` on the response. +/// 3. `default` — the cache's configured default. +/// +/// The resulting TTL is capped at [`MAX_TTL`] for safety. +pub(super) fn resolve_ttl(inputs: TtlInputs) -> Duration { + let chosen = inputs + .explicit + .or(inputs.server_max_age) + .unwrap_or(inputs.default); + std::cmp::min(chosen, MAX_TTL) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn explicit_overrides_server_max_age_and_default() { + let ttl = resolve_ttl(TtlInputs { + explicit: Some(Duration::from_secs(60)), + server_max_age: Some(Duration::from_secs(3600)), + default: Duration::from_secs(300), + }); + assert_eq!(ttl, Duration::from_secs(60)); + } + + #[test] + fn falls_back_to_server_max_age_when_no_explicit() { + let ttl = resolve_ttl(TtlInputs { + explicit: None, + server_max_age: Some(Duration::from_secs(3600)), + default: Duration::from_secs(300), + }); + assert_eq!(ttl, Duration::from_secs(3600)); + } + + #[test] + fn falls_back_to_default_when_no_explicit_and_no_server_max_age() { + let ttl = resolve_ttl(TtlInputs { + explicit: None, + server_max_age: None, + default: Duration::from_secs(300), + }); + assert_eq!(ttl, Duration::from_secs(300)); + } + + #[test] + fn caps_server_max_age_at_max_ttl() { + let ttl = resolve_ttl(TtlInputs { + explicit: None, + // 30 days, well over MAX_TTL (7 days) + server_max_age: Some(Duration::from_secs(60 * 60 * 24 * 30)), + default: Duration::from_secs(300), + }); + assert_eq!(ttl, MAX_TTL); + } + + #[test] + fn caps_explicit_at_max_ttl() { + let ttl = resolve_ttl(TtlInputs { + explicit: Some(Duration::from_secs(60 * 60 * 24 * 30)), + server_max_age: None, + default: Duration::from_secs(300), + }); + assert_eq!(ttl, MAX_TTL); + } + + #[test] + fn zero_server_max_age_yields_zero() { + // Lets the strategy emit NoCache without a network round-trip. + let ttl = resolve_ttl(TtlInputs { + explicit: None, + server_max_age: Some(Duration::ZERO), + default: Duration::from_secs(300), + }); + assert_eq!(ttl, Duration::ZERO); + } +} From fe6fb4c9324046d745d53ee94c2030fda700da40 Mon Sep 17 00:00:00 2001 From: ahanot Date: Thu, 30 Apr 2026 15:11:06 -0500 Subject: [PATCH 3/6] refactor(ads-client): make resolve a method on TtlInputs Move the TTL resolution from a free function into an inherent method on TtlInputs, and drop pub(super) restrictions in favor of plain pub (visibility is already bounded by the private ttl module). --- .../ads-client/src/http_cache/strategy.rs | 7 +- components/ads-client/src/http_cache/ttl.rs | 64 +++++++++++-------- 2 files changed, 40 insertions(+), 31 deletions(-) diff --git a/components/ads-client/src/http_cache/strategy.rs b/components/ads-client/src/http_cache/strategy.rs index d2d4698e4f..d103d447ea 100644 --- a/components/ads-client/src/http_cache/strategy.rs +++ b/components/ads-client/src/http_cache/strategy.rs @@ -6,7 +6,7 @@ use super::cache_control::CacheControl; use super::request_hash::RequestHash; use super::store::HttpCacheStore; -use super::ttl::{resolve_ttl, TtlInputs}; +use super::ttl::TtlInputs; use super::{CacheOutcome, HttpCacheSendResult}; use std::time::Duration; use viaduct::{Client, Request}; @@ -51,11 +51,12 @@ impl NetworkFirst { let response = client.send_sync(self.request)?; let cache_control = CacheControl::from(&response); let outcome = if cache_control.should_cache() { - let ttl = resolve_ttl(TtlInputs { + let ttl = TtlInputs { explicit: self.explicit_ttl, server_max_age: cache_control.max_age_duration(), default: self.default_ttl, - }); + } + .resolve(); if ttl.is_zero() { return Ok((response, vec![CacheOutcome::NoCache])); } diff --git a/components/ads-client/src/http_cache/ttl.rs b/components/ads-client/src/http_cache/ttl.rs index cb92473622..68450a89f6 100644 --- a/components/ads-client/src/http_cache/ttl.rs +++ b/components/ads-client/src/http_cache/ttl.rs @@ -7,12 +7,12 @@ use std::time::Duration; use super::MAX_TTL; -/// Inputs to [`resolve_ttl`]. +/// Inputs to [`TtlInputs::resolve`]. /// /// Each field comes from a different source: `explicit` from the caller, /// `server_max_age` from the response's `Cache-Control` header, and /// `default` from the cache's configuration. -pub(super) struct TtlInputs { +pub struct TtlInputs { /// Per-request override provided by the caller, if any. pub explicit: Option, /// `Cache-Control: max-age` from the server response, if present. @@ -21,20 +21,22 @@ pub(super) struct TtlInputs { pub default: Duration, } -/// Resolve the TTL to use when storing a response in the cache. -/// -/// Priority (highest to lowest): -/// 1. `explicit` — caller-provided per-request override. -/// 2. `server_max_age` — value of `Cache-Control: max-age` on the response. -/// 3. `default` — the cache's configured default. -/// -/// The resulting TTL is capped at [`MAX_TTL`] for safety. -pub(super) fn resolve_ttl(inputs: TtlInputs) -> Duration { - let chosen = inputs - .explicit - .or(inputs.server_max_age) - .unwrap_or(inputs.default); - std::cmp::min(chosen, MAX_TTL) +impl TtlInputs { + /// Resolve the TTL to use when storing a response in the cache. + /// + /// Priority (highest to lowest): + /// 1. `explicit` — caller-provided per-request override. + /// 2. `server_max_age` — value of `Cache-Control: max-age` on the response. + /// 3. `default` — the cache's configured default. + /// + /// The resulting TTL is capped at [`MAX_TTL`] for safety. + pub fn resolve(&self) -> Duration { + let chosen = self + .explicit + .or(self.server_max_age) + .unwrap_or(self.default); + std::cmp::min(chosen, MAX_TTL) + } } #[cfg(test)] @@ -43,63 +45,69 @@ mod tests { #[test] fn explicit_overrides_server_max_age_and_default() { - let ttl = resolve_ttl(TtlInputs { + let ttl = TtlInputs { explicit: Some(Duration::from_secs(60)), server_max_age: Some(Duration::from_secs(3600)), default: Duration::from_secs(300), - }); + } + .resolve(); assert_eq!(ttl, Duration::from_secs(60)); } #[test] fn falls_back_to_server_max_age_when_no_explicit() { - let ttl = resolve_ttl(TtlInputs { + let ttl = TtlInputs { explicit: None, server_max_age: Some(Duration::from_secs(3600)), default: Duration::from_secs(300), - }); + } + .resolve(); assert_eq!(ttl, Duration::from_secs(3600)); } #[test] fn falls_back_to_default_when_no_explicit_and_no_server_max_age() { - let ttl = resolve_ttl(TtlInputs { + let ttl = TtlInputs { explicit: None, server_max_age: None, default: Duration::from_secs(300), - }); + } + .resolve(); assert_eq!(ttl, Duration::from_secs(300)); } #[test] fn caps_server_max_age_at_max_ttl() { - let ttl = resolve_ttl(TtlInputs { + let ttl = TtlInputs { explicit: None, // 30 days, well over MAX_TTL (7 days) server_max_age: Some(Duration::from_secs(60 * 60 * 24 * 30)), default: Duration::from_secs(300), - }); + } + .resolve(); assert_eq!(ttl, MAX_TTL); } #[test] fn caps_explicit_at_max_ttl() { - let ttl = resolve_ttl(TtlInputs { + let ttl = TtlInputs { explicit: Some(Duration::from_secs(60 * 60 * 24 * 30)), server_max_age: None, default: Duration::from_secs(300), - }); + } + .resolve(); assert_eq!(ttl, MAX_TTL); } #[test] fn zero_server_max_age_yields_zero() { // Lets the strategy emit NoCache without a network round-trip. - let ttl = resolve_ttl(TtlInputs { + let ttl = TtlInputs { explicit: None, server_max_age: Some(Duration::ZERO), default: Duration::from_secs(300), - }); + } + .resolve(); assert_eq!(ttl, Duration::ZERO); } } From 32517f3f7352506436227b43d16c1bed1aa476db Mon Sep 17 00:00:00 2001 From: ahanot Date: Thu, 30 Apr 2026 15:14:56 -0500 Subject: [PATCH 4/6] refactor(ads-client): remove TTL/size bounds from http_cache Drop the shared DEFAULT_TTL/MIN_TTL/MAX_TTL constants and the builder's TTL/max-size range validation. The http_cache module now trusts whatever the caller configures and no longer caps server-supplied max-age values. This keeps http_cache standalone and free of policy decisions; deciding what counts as a reasonable TTL or cache size is the consumer's job. Drops the InvalidTtl and InvalidMaxSize variants from HttpCacheBuilderError as a result. --- CHANGELOG.md | 2 +- .../ads-client/docs/usage-javascript.md | 2 - components/ads-client/docs/usage-kotlin.md | 2 - components/ads-client/docs/usage-swift.md | 2 - components/ads-client/src/ffi/telemetry.rs | 2 - components/ads-client/src/http_cache.rs | 4 - .../ads-client/src/http_cache/builder.rs | 118 +----------------- components/ads-client/src/http_cache/ttl.rs | 33 +---- 8 files changed, 4 insertions(+), 161 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eb7f02d20..273c4b8417 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ ## ✨ What's New ✨ ### Ads Client -* HTTP cache TTL is now resolved by priority — explicit per-request TTL (if any) wins, otherwise the response's `Cache-Control: max-age` is used, otherwise the configured `default_ttl`. The chosen value is capped at 7 days. Previously the layer took the minimum of all three, which effectively ignored the server's `max-age` signal. +* HTTP cache TTL is now resolved by priority — explicit per-request TTL (if any) wins, otherwise the response's `Cache-Control: max-age` is used, otherwise the configured `default_ttl`. Previously the layer took the minimum of all three, which effectively ignored the server's `max-age` signal. ### Remote Settings * Add uptake telemetry support ([#7288](https://github.com/mozilla/application-services/pull/7288)) diff --git a/components/ads-client/docs/usage-javascript.md b/components/ads-client/docs/usage-javascript.md index 2e45de6647..4467fb3f07 100644 --- a/components/ads-client/docs/usage-javascript.md +++ b/components/ads-client/docs/usage-javascript.md @@ -551,8 +551,6 @@ resolved by priority (highest to lowest): 2. `server_max_age` — value of the HTTP `Cache-Control: max-age=N` header on the response. 3. `client_default_ttl` — configured on `MozAdsCacheConfig`. -The chosen value is capped at 7 days for safety. - If the effective TTL resolves to 0 seconds (e.g. `Cache-Control: max-age=0`), the response is not cached. diff --git a/components/ads-client/docs/usage-kotlin.md b/components/ads-client/docs/usage-kotlin.md index df83ad1000..18ffec337e 100644 --- a/components/ads-client/docs/usage-kotlin.md +++ b/components/ads-client/docs/usage-kotlin.md @@ -508,8 +508,6 @@ resolved by priority (highest to lowest): 2. `server_max_age` — value of the HTTP `Cache-Control: max-age=N` header on the response. 3. `client_default_ttl` — configured on `MozAdsCacheConfig`. -The chosen value is capped at 7 days for safety. - If the effective TTL resolves to 0 seconds (e.g. `Cache-Control: max-age=0`), the response is not cached. diff --git a/components/ads-client/docs/usage-swift.md b/components/ads-client/docs/usage-swift.md index 314fb4743a..52cb21282b 100644 --- a/components/ads-client/docs/usage-swift.md +++ b/components/ads-client/docs/usage-swift.md @@ -508,8 +508,6 @@ resolved by priority (highest to lowest): 2. `server_max_age` — value of the HTTP `Cache-Control: max-age=N` header on the response. 3. `client_default_ttl` — configured on `MozAdsCacheConfig`. -The chosen value is capped at 7 days for safety. - If the effective TTL resolves to 0 seconds (e.g. `Cache-Control: max-age=0`), the response is not cached. diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs index 55af52db58..7a707abb61 100644 --- a/components/ads-client/src/ffi/telemetry.rs +++ b/components/ads-client/src/ffi/telemetry.rs @@ -87,8 +87,6 @@ impl Telemetry for MozAdsTelemetryWrapper { match cache_builder_error { HttpCacheBuilderError::EmptyDbPath => "empty_db_path".to_string(), HttpCacheBuilderError::Database(_) => "database_error".to_string(), - HttpCacheBuilderError::InvalidMaxSize { .. } => "invalid_max_size".to_string(), - HttpCacheBuilderError::InvalidTtl { .. } => "invalid_ttl".to_string(), }, format!("{}", cache_builder_error), ); diff --git a/components/ads-client/src/http_cache.rs b/components/ads-client/src/http_cache.rs index 5ed4b100e1..2ae3d07812 100644 --- a/components/ads-client/src/http_cache.rs +++ b/components/ads-client/src/http_cache.rs @@ -29,10 +29,6 @@ pub use self::request_hash::RequestHash; use std::path::Path; use std::time::Duration; -pub(crate) const DEFAULT_TTL: Duration = Duration::from_secs(300); -pub(crate) const MIN_TTL: Duration = Duration::from_secs(1); -pub(crate) const MAX_TTL: Duration = Duration::from_secs(60 * 60 * 24 * 7); // 7 days - pub type HttpCacheSendResult = std::result::Result<(Response, Vec), viaduct::ViaductError>; diff --git a/components/ads-client/src/http_cache/builder.rs b/components/ads-client/src/http_cache/builder.rs index 4d481fd985..0bf2a11478 100644 --- a/components/ads-client/src/http_cache/builder.rs +++ b/components/ads-client/src/http_cache/builder.rs @@ -7,16 +7,13 @@ use crate::http_cache::HttpCache; use super::bytesize::ByteSize; use super::connection_initializer::HttpCacheConnectionInitializer; use super::store::HttpCacheStore; -use super::{DEFAULT_TTL, MAX_TTL, MIN_TTL}; use rusqlite::Connection; use sql_support::open_database; use std::path::PathBuf; use std::time::Duration; const DEFAULT_MAX_SIZE: ByteSize = ByteSize::mib(10); - -const MIN_CACHE_SIZE: ByteSize = ByteSize::kib(1); -const MAX_CACHE_SIZE: ByteSize = ByteSize::mib(100); +const DEFAULT_TTL: Duration = Duration::from_secs(300); #[derive(Debug, thiserror::Error)] pub enum HttpCacheBuilderError { @@ -24,20 +21,6 @@ pub enum HttpCacheBuilderError { EmptyDbPath, #[error("Database error: {0}")] Database(#[from] open_database::Error), - #[error( - "Maximum cache size must be between {min_size} and {max_size}, got {size_bytes} bytes" - )] - InvalidMaxSize { - max_size: String, - min_size: String, - size_bytes: u64, - }, - #[error("TTL must be between {min_ttl} and {max_ttl}, got {ttl} seconds")] - InvalidTtl { - max_ttl: String, - min_ttl: String, - ttl: u64, - }, } pub struct HttpCacheBuilder { @@ -69,27 +52,6 @@ impl HttpCacheBuilder { if self.db_path.to_string_lossy().trim().is_empty() { return Err(HttpCacheBuilderError::EmptyDbPath); } - - if let Some(max_size) = self.max_size { - if max_size < MIN_CACHE_SIZE || max_size > MAX_CACHE_SIZE { - return Err(HttpCacheBuilderError::InvalidMaxSize { - size_bytes: max_size.as_u64(), - min_size: MIN_CACHE_SIZE.to_string(), - max_size: MAX_CACHE_SIZE.to_string(), - }); - } - } - - if let Some(ttl) = self.default_ttl { - if !(MIN_TTL..=MAX_TTL).contains(&ttl) { - return Err(HttpCacheBuilderError::InvalidTtl { - ttl: ttl.as_secs(), - min_ttl: format!("{} seconds", MIN_TTL.as_secs()), - max_ttl: format!("{} seconds", MAX_TTL.as_secs()), - }); - } - } - Ok(()) } @@ -169,82 +131,4 @@ mod tests { let result = make_test_builder(" ").build(); assert!(matches!(result, Err(HttpCacheBuilderError::EmptyDbPath))); } - - #[test] - fn test_validation_max_size_too_small() { - let result = make_test_builder("test.db") - .max_size(ByteSize::b(512)) - .build(); - assert!(matches!( - result, - Err(HttpCacheBuilderError::InvalidMaxSize { - size_bytes: 512, - min_size: _, - max_size: _, - }) - )); - } - - #[test] - fn test_validation_max_size_too_large() { - let result = make_test_builder("test.db") - .max_size(ByteSize::b(2 * 1024 * 1024 * 1024)) - .build(); - assert!(matches!( - result, - Err(HttpCacheBuilderError::InvalidMaxSize { - size_bytes: 2147483648, - min_size: _, - max_size: _, - }) - )); - } - - #[test] - fn test_validation_max_size_boundaries() { - let builder_min = make_test_builder("test.db").max_size(MIN_CACHE_SIZE); - assert!(builder_min.build().is_ok()); - - let builder_max = make_test_builder("test.db").max_size(MAX_CACHE_SIZE); - assert!(builder_max.build().is_ok()); - } - - #[test] - fn test_validation_ttl_too_small() { - let result = make_test_builder("test.db") - .default_ttl(Duration::from_secs(0)) - .build(); - assert!(matches!( - result, - Err(HttpCacheBuilderError::InvalidTtl { - ttl: 0, - min_ttl: _, - max_ttl: _, - }) - )); - } - - #[test] - fn test_validation_ttl_too_large() { - let result = make_test_builder("test.db") - .default_ttl(Duration::from_secs(8 * 24 * 60 * 60)) - .build(); - assert!(matches!( - result, - Err(HttpCacheBuilderError::InvalidTtl { - ttl: 691200, - min_ttl: _, - max_ttl: _, - }) - )); - } - - #[test] - fn test_validation_ttl_boundaries() { - let builder_min = make_test_builder("test.db").default_ttl(MIN_TTL); - assert!(builder_min.build().is_ok()); - - let builder_max = make_test_builder("test.db").default_ttl(MAX_TTL); - assert!(builder_max.build().is_ok()); - } } diff --git a/components/ads-client/src/http_cache/ttl.rs b/components/ads-client/src/http_cache/ttl.rs index 68450a89f6..b64ab24176 100644 --- a/components/ads-client/src/http_cache/ttl.rs +++ b/components/ads-client/src/http_cache/ttl.rs @@ -5,8 +5,6 @@ use std::time::Duration; -use super::MAX_TTL; - /// Inputs to [`TtlInputs::resolve`]. /// /// Each field comes from a different source: `explicit` from the caller, @@ -28,14 +26,10 @@ impl TtlInputs { /// 1. `explicit` — caller-provided per-request override. /// 2. `server_max_age` — value of `Cache-Control: max-age` on the response. /// 3. `default` — the cache's configured default. - /// - /// The resulting TTL is capped at [`MAX_TTL`] for safety. pub fn resolve(&self) -> Duration { - let chosen = self - .explicit + self.explicit .or(self.server_max_age) - .unwrap_or(self.default); - std::cmp::min(chosen, MAX_TTL) + .unwrap_or(self.default) } } @@ -76,29 +70,6 @@ mod tests { assert_eq!(ttl, Duration::from_secs(300)); } - #[test] - fn caps_server_max_age_at_max_ttl() { - let ttl = TtlInputs { - explicit: None, - // 30 days, well over MAX_TTL (7 days) - server_max_age: Some(Duration::from_secs(60 * 60 * 24 * 30)), - default: Duration::from_secs(300), - } - .resolve(); - assert_eq!(ttl, MAX_TTL); - } - - #[test] - fn caps_explicit_at_max_ttl() { - let ttl = TtlInputs { - explicit: Some(Duration::from_secs(60 * 60 * 24 * 30)), - server_max_age: None, - default: Duration::from_secs(300), - } - .resolve(); - assert_eq!(ttl, MAX_TTL); - } - #[test] fn zero_server_max_age_yields_zero() { // Lets the strategy emit NoCache without a network round-trip. From 1f039689ef4a6b25ab4d64734144924f496a61d1 Mon Sep 17 00:00:00 2001 From: ahanot Date: Thu, 30 Apr 2026 15:15:42 -0500 Subject: [PATCH 5/6] refactor(ads-client): rename TtlInputs to EffectiveTtl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The struct represents the concept being computed (the effective TTL), not a parameter pack — `EffectiveTtl { ... }.resolve()` reads more naturally at the call site. --- .../ads-client/src/http_cache/strategy.rs | 4 ++-- components/ads-client/src/http_cache/ttl.rs | 24 +++++++++---------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/components/ads-client/src/http_cache/strategy.rs b/components/ads-client/src/http_cache/strategy.rs index d103d447ea..acf23c8234 100644 --- a/components/ads-client/src/http_cache/strategy.rs +++ b/components/ads-client/src/http_cache/strategy.rs @@ -6,7 +6,7 @@ use super::cache_control::CacheControl; use super::request_hash::RequestHash; use super::store::HttpCacheStore; -use super::ttl::TtlInputs; +use super::ttl::EffectiveTtl; use super::{CacheOutcome, HttpCacheSendResult}; use std::time::Duration; use viaduct::{Client, Request}; @@ -51,7 +51,7 @@ impl NetworkFirst { let response = client.send_sync(self.request)?; let cache_control = CacheControl::from(&response); let outcome = if cache_control.should_cache() { - let ttl = TtlInputs { + let ttl = EffectiveTtl { explicit: self.explicit_ttl, server_max_age: cache_control.max_age_duration(), default: self.default_ttl, diff --git a/components/ads-client/src/http_cache/ttl.rs b/components/ads-client/src/http_cache/ttl.rs index b64ab24176..0183ddfbb8 100644 --- a/components/ads-client/src/http_cache/ttl.rs +++ b/components/ads-client/src/http_cache/ttl.rs @@ -5,12 +5,12 @@ use std::time::Duration; -/// Inputs to [`TtlInputs::resolve`]. +/// The TTL to use when storing a response in the cache, computed from +/// three possible sources. /// -/// Each field comes from a different source: `explicit` from the caller, -/// `server_max_age` from the response's `Cache-Control` header, and -/// `default` from the cache's configuration. -pub struct TtlInputs { +/// `explicit` comes from the caller, `server_max_age` from the response's +/// `Cache-Control` header, and `default` from the cache's configuration. +pub struct EffectiveTtl { /// Per-request override provided by the caller, if any. pub explicit: Option, /// `Cache-Control: max-age` from the server response, if present. @@ -19,10 +19,8 @@ pub struct TtlInputs { pub default: Duration, } -impl TtlInputs { - /// Resolve the TTL to use when storing a response in the cache. - /// - /// Priority (highest to lowest): +impl EffectiveTtl { + /// Resolve the TTL by priority (highest to lowest): /// 1. `explicit` — caller-provided per-request override. /// 2. `server_max_age` — value of `Cache-Control: max-age` on the response. /// 3. `default` — the cache's configured default. @@ -39,7 +37,7 @@ mod tests { #[test] fn explicit_overrides_server_max_age_and_default() { - let ttl = TtlInputs { + let ttl = EffectiveTtl { explicit: Some(Duration::from_secs(60)), server_max_age: Some(Duration::from_secs(3600)), default: Duration::from_secs(300), @@ -50,7 +48,7 @@ mod tests { #[test] fn falls_back_to_server_max_age_when_no_explicit() { - let ttl = TtlInputs { + let ttl = EffectiveTtl { explicit: None, server_max_age: Some(Duration::from_secs(3600)), default: Duration::from_secs(300), @@ -61,7 +59,7 @@ mod tests { #[test] fn falls_back_to_default_when_no_explicit_and_no_server_max_age() { - let ttl = TtlInputs { + let ttl = EffectiveTtl { explicit: None, server_max_age: None, default: Duration::from_secs(300), @@ -73,7 +71,7 @@ mod tests { #[test] fn zero_server_max_age_yields_zero() { // Lets the strategy emit NoCache without a network round-trip. - let ttl = TtlInputs { + let ttl = EffectiveTtl { explicit: None, server_max_age: Some(Duration::ZERO), default: Duration::from_secs(300), From fc40827dc578c0e050d8995e6836ed1da7ca2662 Mon Sep 17 00:00:00 2001 From: ahanot Date: Thu, 30 Apr 2026 15:19:57 -0500 Subject: [PATCH 6/6] revert(ads-client): restore builder validation and telemetry mappings Roll back the bounds-removal changes from this branch. Cache size and TTL validation in HttpCacheBuilder, the InvalidMaxSize/InvalidTtl error variants, and their telemetry codes are restored to match main. Those changes are out of scope for AC-103 (the priority-order TTL resolution) and should be revisited separately. --- components/ads-client/src/ffi/telemetry.rs | 2 + .../ads-client/src/http_cache/builder.rs | 118 ++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs index 7a707abb61..55af52db58 100644 --- a/components/ads-client/src/ffi/telemetry.rs +++ b/components/ads-client/src/ffi/telemetry.rs @@ -87,6 +87,8 @@ impl Telemetry for MozAdsTelemetryWrapper { match cache_builder_error { HttpCacheBuilderError::EmptyDbPath => "empty_db_path".to_string(), HttpCacheBuilderError::Database(_) => "database_error".to_string(), + HttpCacheBuilderError::InvalidMaxSize { .. } => "invalid_max_size".to_string(), + HttpCacheBuilderError::InvalidTtl { .. } => "invalid_ttl".to_string(), }, format!("{}", cache_builder_error), ); diff --git a/components/ads-client/src/http_cache/builder.rs b/components/ads-client/src/http_cache/builder.rs index 0bf2a11478..97f92f004b 100644 --- a/components/ads-client/src/http_cache/builder.rs +++ b/components/ads-client/src/http_cache/builder.rs @@ -15,12 +15,31 @@ use std::time::Duration; const DEFAULT_MAX_SIZE: ByteSize = ByteSize::mib(10); const DEFAULT_TTL: Duration = Duration::from_secs(300); +const MIN_CACHE_SIZE: ByteSize = ByteSize::kib(1); +const MAX_CACHE_SIZE: ByteSize = ByteSize::mib(100); +const MIN_TTL: Duration = Duration::from_secs(1); +const MAX_TTL: Duration = Duration::from_secs(60 * 60 * 24 * 7); // 7 days + #[derive(Debug, thiserror::Error)] pub enum HttpCacheBuilderError { #[error("Database path cannot be empty")] EmptyDbPath, #[error("Database error: {0}")] Database(#[from] open_database::Error), + #[error( + "Maximum cache size must be between {min_size} and {max_size}, got {size_bytes} bytes" + )] + InvalidMaxSize { + max_size: String, + min_size: String, + size_bytes: u64, + }, + #[error("TTL must be between {min_ttl} and {max_ttl}, got {ttl} seconds")] + InvalidTtl { + max_ttl: String, + min_ttl: String, + ttl: u64, + }, } pub struct HttpCacheBuilder { @@ -52,6 +71,27 @@ impl HttpCacheBuilder { if self.db_path.to_string_lossy().trim().is_empty() { return Err(HttpCacheBuilderError::EmptyDbPath); } + + if let Some(max_size) = self.max_size { + if max_size < MIN_CACHE_SIZE || max_size > MAX_CACHE_SIZE { + return Err(HttpCacheBuilderError::InvalidMaxSize { + size_bytes: max_size.as_u64(), + min_size: MIN_CACHE_SIZE.to_string(), + max_size: MAX_CACHE_SIZE.to_string(), + }); + } + } + + if let Some(ttl) = self.default_ttl { + if !(MIN_TTL..=MAX_TTL).contains(&ttl) { + return Err(HttpCacheBuilderError::InvalidTtl { + ttl: ttl.as_secs(), + min_ttl: format!("{} seconds", MIN_TTL.as_secs()), + max_ttl: format!("{} seconds", MAX_TTL.as_secs()), + }); + } + } + Ok(()) } @@ -131,4 +171,82 @@ mod tests { let result = make_test_builder(" ").build(); assert!(matches!(result, Err(HttpCacheBuilderError::EmptyDbPath))); } + + #[test] + fn test_validation_max_size_too_small() { + let result = make_test_builder("test.db") + .max_size(ByteSize::b(512)) + .build(); + assert!(matches!( + result, + Err(HttpCacheBuilderError::InvalidMaxSize { + size_bytes: 512, + min_size: _, + max_size: _, + }) + )); + } + + #[test] + fn test_validation_max_size_too_large() { + let result = make_test_builder("test.db") + .max_size(ByteSize::b(2 * 1024 * 1024 * 1024)) + .build(); + assert!(matches!( + result, + Err(HttpCacheBuilderError::InvalidMaxSize { + size_bytes: 2147483648, + min_size: _, + max_size: _, + }) + )); + } + + #[test] + fn test_validation_max_size_boundaries() { + let builder_min = make_test_builder("test.db").max_size(MIN_CACHE_SIZE); + assert!(builder_min.build().is_ok()); + + let builder_max = make_test_builder("test.db").max_size(MAX_CACHE_SIZE); + assert!(builder_max.build().is_ok()); + } + + #[test] + fn test_validation_ttl_too_small() { + let result = make_test_builder("test.db") + .default_ttl(Duration::from_secs(0)) + .build(); + assert!(matches!( + result, + Err(HttpCacheBuilderError::InvalidTtl { + ttl: 0, + min_ttl: _, + max_ttl: _, + }) + )); + } + + #[test] + fn test_validation_ttl_too_large() { + let result = make_test_builder("test.db") + .default_ttl(Duration::from_secs(8 * 24 * 60 * 60)) + .build(); + assert!(matches!( + result, + Err(HttpCacheBuilderError::InvalidTtl { + ttl: 691200, + min_ttl: _, + max_ttl: _, + }) + )); + } + + #[test] + fn test_validation_ttl_boundaries() { + let builder_min = make_test_builder("test.db").default_ttl(MIN_TTL); + assert!(builder_min.build().is_ok()); + + let builder_max = make_test_builder("test.db").default_ttl(MAX_TTL); + assert!(builder_max.build().is_ok()); + } }