diff --git a/api/src/api/errors.rs b/api/src/api/errors.rs index 369a4b155..ce9fb6f95 100644 --- a/api/src/api/errors.rs +++ b/api/src/api/errors.rs @@ -60,6 +60,10 @@ errors!( status: NOT_FOUND, "Documentation is only available for the latest version of a package.", }, + DocsSymbolListingTooLarge { + status: PAYLOAD_TOO_LARGE, + "This package has too many symbols to list them all at once.", + }, EntrypointOrSymbolNotFound { status: NOT_FOUND, "The requested entrypoint or symbol was not found.", @@ -432,6 +436,18 @@ impl } } +impl From for ApiError { + fn from(error: crate::object_cache::ObjectCacheError) -> ApiError { + anyhow::Error::from(error).into() + } +} + +impl From for ApiError { + fn from(error: crate::docs::GenerateCtxCacheError) -> ApiError { + anyhow::Error::from(error).into() + } +} + impl From for ApiError { fn from(error: crate::docs::DocNodeCacheError) -> ApiError { anyhow::Error::from(error).into() diff --git a/api/src/api/package.rs b/api/src/api/package.rs index 53c53af08..db3d6e056 100644 --- a/api/src/api/package.rs +++ b/api/src/api/package.rs @@ -211,8 +211,12 @@ pub fn package_router() -> Router { ), ) .get( + // `_shared`: like `docs` above, both search payloads are derived purely + // from the published version, with no permission/member/sudo branch, so + // the lb may serve them from its shared cache to authenticated callers + // instead of bypassing on auth. "/:package/versions/:version/docs/search", - util::cache_versioned( + util::cache_versioned_shared( CacheDuration::FIVE_MINUTES, CacheDuration::THIRTY_DAYS, util::json(get_docs_search_handler), @@ -220,7 +224,7 @@ pub fn package_router() -> Router { ) .get( "/:package/versions/:version/docs/search_structured", - util::cache_versioned( + util::cache_versioned_shared( CacheDuration::FIVE_MINUTES, CacheDuration::THIRTY_DAYS, util::json(get_docs_search_structured_handler), @@ -1482,7 +1486,14 @@ pub async fn get_docs_handler( version.readme_path.as_ref().unwrap(), ) .into(); - Either::Left(buckets.modules_bucket.download(s3_path)) + let object_cache = req + .data::() + .unwrap() + .clone(); + let modules_bucket = buckets.modules_bucket.clone(); + Either::Left(async move { + object_cache.download(&modules_bucket, s3_path).await + }) } else { Either::Right(futures::future::ready(Ok(None))) }; @@ -1635,6 +1646,10 @@ pub async fn get_docs_search_handler( ApiError::InternalServerError })?; + if crate::docs::all_symbols_listing_too_large(&ctx) { + return Err(ApiError::DocsSymbolListingTooLarge); + } + let _permit = crate::docs::acquire_doc_render_permit().await; let search_index = deno_doc::html::generate_search_index(&ctx); @@ -1709,6 +1724,10 @@ pub async fn get_docs_search_structured_handler( ApiError::InternalServerError })?; + if crate::docs::all_symbols_listing_too_large(&ctx) { + return Err(ApiError::DocsSymbolListingTooLarge); + } + let _permit = crate::docs::acquire_doc_render_permit().await; let docs = crate::docs::render_docs_html( &ctx, @@ -1778,9 +1797,10 @@ pub async fn get_source_handler( } else if path == format!("{}_meta.json", version.version) { let source_file_path = crate::s3_paths::version_metadata(&scope, &package, &version.version); - buckets - .modules_bucket - .download(source_file_path.into()) + req + .data::() + .unwrap() + .download(&buckets.modules_bucket, source_file_path.into()) .await? } else if path != "/" { let package_path = PackagePath::try_from(path.as_str()).map_err(|err| { @@ -1815,6 +1835,7 @@ pub async fn get_source_handler( &path, &file, buckets, + req.data::().unwrap(), req.data::().unwrap().0.as_str(), ) .await; @@ -1937,6 +1958,7 @@ pub async fn get_source_handler( /// The ranges come from the `moduleGraph2` recorded at publish time, so /// nothing is re-parsed here. Links are a nicety: anything missing or /// unreadable just yields a file view without them. +#[allow(clippy::too_many_arguments)] async fn source_specifier_links( scope: &ScopeName, package: &PackageName, @@ -1944,32 +1966,37 @@ async fn source_specifier_links( path: &str, source: &str, buckets: &Buckets, + object_cache: &crate::object_cache::ObjectCache, registry_url: &str, ) -> Vec { if !crate::source_links::is_module_path(path) { return Vec::new(); } + // Immutable per version and shared by every file in it, so this goes + // through the object cache: without it each source-file view re-downloaded + // the whole manifest just to read one module's entry. let metadata_path = crate::s3_paths::version_metadata(scope, package, version); - let metadata = - match buckets.modules_bucket.download(metadata_path.into()).await { - Ok(Some(bytes)) => { - match serde_json::from_slice::(&bytes) - { - Ok(metadata) => metadata, - Err(err) => { - error!("failed to parse version metadata for links: {err}"); - return Vec::new(); - } + let metadata = match object_cache + .download(&buckets.modules_bucket, metadata_path.into()) + .await + { + Ok(Some(bytes)) => { + match serde_json::from_slice::(&bytes) { + Ok(metadata) => metadata, + Err(err) => { + error!("failed to parse version metadata for links: {err}"); + return Vec::new(); } } - Ok(None) => return Vec::new(), - Err(err) => { - error!("failed to download version metadata for links: {err}"); - return Vec::new(); - } - }; + } + Ok(None) => return Vec::new(), + Err(err) => { + error!("failed to download version metadata for links: {err}"); + return Vec::new(); + } + }; let Some(module_info) = metadata.module_graph_2.get(path) else { return Vec::new(); diff --git a/api/src/docs.rs b/api/src/docs.rs index 197833053..fe3736bff 100644 --- a/api/src/docs.rs +++ b/api/src/docs.rs @@ -37,6 +37,7 @@ use std::io::Read; use std::io::Write; use std::sync::Arc; use std::sync::OnceLock; +use std::time::Duration; use tracing::instrument; use url::Url; @@ -146,6 +147,22 @@ pub async fn download_doc_nodes( version: &Version, bucket: &crate::s3::Buckets, ) -> Result, DocNodeCacheError> { + Ok( + download_doc_nodes_sized(scope, package, version, bucket) + .await? + .map(|(doc_nodes, _)| doc_nodes), + ) +} + +/// Like [`download_doc_nodes`], but also reports how many stored bytes the +/// nodes were decoded from. [`GenerateCtxCache`] weighs its entries by that +/// figure, so it needs the size the download already knows. +async fn download_doc_nodes_sized( + scope: &ScopeName, + package: &PackageName, + version: &Version, + bucket: &crate::s3::Buckets, +) -> Result, DocNodeCacheError> { let v2_path = crate::s3_paths::docs_v2_path(scope, package, version); let v2_result = bucket .docs_bucket @@ -153,7 +170,7 @@ pub async fn download_doc_nodes( .await?; if let Some(bytes) = v2_result { - return Ok(Some(deserialize_doc_nodes_v2(&bytes)?)); + return Ok(Some((deserialize_doc_nodes_v2(&bytes)?, bytes.len()))); } let v1_path = crate::s3_paths::docs_v1_path(scope, package, version); @@ -167,6 +184,7 @@ pub async fn download_doc_nodes( }; let doc_nodes = deserialize_doc_nodes_v1(&bytes)?; + let stored_bytes = bytes.len(); // Best-effort migration: re-upload as v2 and delete v1. Failures are // logged but not propagated — the doc nodes were already read successfully. @@ -198,24 +216,68 @@ pub async fn download_doc_nodes( } } - Ok(Some(doc_nodes)) + Ok(Some((doc_nodes, stored_bytes))) +} + +/// Total weight the cache may hold, measured in the *stored* (gzipped +/// MessagePack) size of the doc nodes each context was built from. +/// +/// This replaced a flat 64-entry capacity. Counting entries meant 64 tiny +/// packages evicted each other exactly as readily as 64 enormous ones, while +/// the memory actually held was bounded by nothing at all. Weighing by stored +/// size bounds memory instead, which both caps the worst case and lets far more +/// of the long tail stay resident — a production sample saw 534 distinct +/// packages across 2000 docs requests against those 64 slots. +/// +/// Stored size is a proxy: the built context is roughly an order of magnitude +/// larger in memory than the bytes it was decoded from. The budget is set +/// conservatively for that reason, and pinning the real ratio needs cache +/// instrumentation that does not exist yet. +const MAX_CACHED_DOC_NODE_BYTES: u64 = 16 * 1024 * 1024; + +/// Release contexts that go unread for this long, so a burst across many +/// packages does not pin memory for the rest of the process's life. +const CACHED_CTX_TIME_TO_IDLE: Duration = Duration::from_secs(30 * 60); + +#[derive(Clone)] +struct CachedCtx { + ctx: Arc, + /// Stored size of the doc nodes this was built from; the cache weight. + stored_bytes: u32, } +/// Loader failure. `Absent` never reaches a caller — returning it as an error +/// is how a package with no doc nodes escapes `try_get_with` without moka +/// caching the miss. +#[derive(Debug, thiserror::Error)] +enum CtxLoadError { + #[error(transparent)] + DocNodes(#[from] DocNodeCacheError), + #[error("no doc nodes stored for this version")] + Absent, +} + +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct GenerateCtxCacheError(Arc); + /// Cache for fully-built GenerateCtx. Keyed by /// `scope/package/version/is_latest/has_readme/runtime_compat` so concurrent /// requests for the same doc page share a single GenerateCtx without /// rebuilding. #[derive(Clone)] pub struct GenerateCtxCache { - cache: moka::future::Cache>, + cache: moka::future::Cache, } impl GenerateCtxCache { pub fn new() -> Self { Self { - // estimated 2-5mb for the average package (based on std packages). - // 5*64 = 320mb estimated max average. - cache: moka::future::Cache::builder().max_capacity(64).build(), + cache: moka::future::Cache::builder() + .max_capacity(MAX_CACHED_DOC_NODE_BYTES) + .weigher(|_key, cached: &CachedCtx| cached.stored_bytes) + .time_to_idle(CACHED_CTX_TIME_TO_IDLE) + .build(), } } @@ -232,7 +294,7 @@ impl GenerateCtxCache { runtime_compat: RuntimeCompat, registry_url: &str, bucket: &crate::s3::Buckets, - ) -> Result>, DocNodeCacheError> { + ) -> Result>, GenerateCtxCacheError> { // runtime_compat is part of the key because the usage instructions baked // into the GenerateCtx depend on it, and it is editable in the package // settings. @@ -241,38 +303,93 @@ impl GenerateCtxCache { runtime_compat ); - if let Some(cached) = self.cache.get(&key).await { - return Ok(Some(cached)); - } + // `try_get_with` single-flights the miss: concurrent requests for the same + // cold key wait on one download and one context build instead of each + // paying both. That matters more here than for a typical cache, because + // building a context allocates the 10-50 MB the render permit exists to + // bound, so a stampede on a popular cold package multiplied real memory as + // well as CPU. + let loaded = self + .cache + .try_get_with(key, async { + let Some((doc_nodes, stored_bytes)) = + download_doc_nodes_sized(scope, package, version, bucket).await? + else { + return Err(CtxLoadError::Absent); + }; + + let docs_info = get_docs_info(exports, None); + let ctx = get_generate_ctx( + "/doc".to_string(), + doc_nodes, + docs_info.main_entrypoint, + docs_info.rewrite_map, + scope.clone(), + package.clone(), + version.clone(), + version_is_latest, + github_repository, + has_readme, + runtime_compat, + has_create_export(exports), + registry_url.to_string(), + None, + ); - let Some(doc_nodes) = - download_doc_nodes(scope, package, version, bucket).await? - else { - return Ok(None); - }; + Ok(CachedCtx { + ctx: Arc::new(ctx), + stored_bytes: stored_bytes.try_into().unwrap_or(u32::MAX), + }) + }) + .await; - let docs_info = get_docs_info(exports, None); - let ctx = get_generate_ctx( - "/doc".to_string(), - doc_nodes, - docs_info.main_entrypoint, - docs_info.rewrite_map, - scope.clone(), - package.clone(), - version.clone(), - version_is_latest, - github_repository, - has_readme, - runtime_compat, - has_create_export(exports), - registry_url.to_string(), - None, - ); + match loaded { + Ok(cached) => Ok(Some(cached.ctx)), + Err(err) if matches!(&*err, CtxLoadError::Absent) => Ok(None), + Err(err) => Err(GenerateCtxCacheError(err)), + } + } +} + +/// Upper bound on the number of symbol rows the "all symbols" listing may +/// contain across *every* entrypoint. +/// +/// [`SYMBOL_LISTING_LIMIT`] already bounds each module's own listing, but the +/// all-symbols listing concatenates one per entrypoint, so a package with many +/// entrypoints multiplies straight past it: `@ubx/sdk-aws` publishes 1921 +/// exports, putting its ceiling near 3.9 million rows. The resulting response +/// exceeded Cloud Run's 32 MiB cap, so every request built the whole thing, +/// spent ~1.6s of CPU, and was then killed mid-flight — 871 times in one hour +/// from a single crawler, and never cacheable because a 500 is not. +/// +/// Refusing up front turns that into a cheap, cacheable error. The durable fix +/// is for the limit in deno_doc to be a total budget rather than a per-module +/// one, at which point this guard should stop firing on its own. +const ALL_SYMBOLS_LISTING_LIMIT: usize = 10_000; + +/// Whether the all-symbols listing for `ctx` would exceed +/// [`ALL_SYMBOLS_LISTING_LIMIT`]. +/// +/// Mirrors what rendering will actually emit: each entrypoint contributes at +/// most [`SYMBOL_LISTING_LIMIT`] rows. Counting top-level doc nodes can only +/// over-estimate a module's rendered rows, so this errs towards refusing. +pub fn all_symbols_listing_too_large(ctx: &GenerateCtx) -> bool { + listing_rows_exceed_limit(ctx.doc_nodes.values().map(|nodes| nodes.len())) +} - let ctx = Arc::new(ctx); - self.cache.insert(key, ctx.clone()).await; - Ok(Some(ctx)) +/// The counting half of [`all_symbols_listing_too_large`], split out so the +/// per-entrypoint accumulation can be tested without building a `GenerateCtx`. +fn listing_rows_exceed_limit( + module_symbol_counts: impl Iterator, +) -> bool { + let mut total = 0usize; + for count in module_symbol_counts { + total = total.saturating_add(count.min(SYMBOL_LISTING_LIMIT)); + if total > ALL_SYMBOLS_LISTING_LIMIT { + return true; + } } + false } pub type URLRewriter = @@ -1670,6 +1787,36 @@ mod tests { use super::*; use deno_doc::html::ShortPath; + #[test] + fn one_module_under_the_per_module_limit_is_allowed() { + // The common case: a single entrypoint, however symbol-heavy, is capped by + // SYMBOL_LISTING_LIMIT and must still render. + assert!(!listing_rows_exceed_limit(std::iter::once(usize::MAX))); + } + + #[test] + fn many_small_entrypoints_can_exceed_the_total_limit() { + // The regression this guard exists for. Every module here is far under + // SYMBOL_LISTING_LIMIT, so the per-module cap never trims anything, yet the + // all-symbols listing concatenates one per entrypoint. @ubx/sdk-aws + // publishes 1921 exports; at 100 symbols each that is 192_100 rows, which + // is what pushed the response past Cloud Run's 32 MiB cap. + assert!(listing_rows_exceed_limit(std::iter::repeat_n(100, 1921))); + } + + #[test] + fn a_package_at_the_limit_is_still_served() { + // Exactly at the budget is allowed; only exceeding it refuses. + assert!(!listing_rows_exceed_limit(std::iter::repeat_n( + 1, + ALL_SYMBOLS_LISTING_LIMIT + ))); + assert!(listing_rows_exceed_limit(std::iter::repeat_n( + 1, + ALL_SYMBOLS_LISTING_LIMIT + 1 + ))); + } + #[test] fn renders_heading_permalinks_through_the_sanitizer() { // deno_doc emits heading permalinks as an `anchorable` heading wrapping an diff --git a/api/src/main.rs b/api/src/main.rs index 330418b27..f1d12cbd2 100644 --- a/api/src/main.rs +++ b/api/src/main.rs @@ -18,6 +18,7 @@ mod ids; mod jemalloc_profiling; mod metadata; mod npm; +mod object_cache; mod provenance; mod publish; mod s3; @@ -71,6 +72,7 @@ pub struct MainRouterOptions { database: Database, buckets: Buckets, generate_ctx_cache: crate::docs::GenerateCtxCache, + object_cache: crate::object_cache::ObjectCache, registry_metadata_cache: crate::api::RegistryMetadataCache, github_client: auth::github::Oauth2Client, gitlab_client: auth::gitlab::Oauth2Client, @@ -104,6 +106,7 @@ pub(crate) fn main_router( database, buckets, generate_ctx_cache, + object_cache, registry_metadata_cache, github_client, gitlab_client, @@ -129,6 +132,7 @@ pub(crate) fn main_router( .data(database) .data(buckets) .data(generate_ctx_cache) + .data(object_cache) .data(registry_metadata_cache) .data(github_client) .data(gitlab_client) @@ -370,12 +374,14 @@ async fn main() { let license_store = util::license_store(); let generate_ctx_cache = crate::docs::GenerateCtxCache::new(); + let object_cache = crate::object_cache::ObjectCache::new(); let registry_metadata_cache = crate::api::RegistryMetadataCache::new(); let router = main_router(MainRouterOptions { database, buckets, generate_ctx_cache, + object_cache, registry_metadata_cache, github_client, gitlab_client, diff --git a/api/src/object_cache.rs b/api/src/object_cache.rs new file mode 100644 index 000000000..3e3393d93 --- /dev/null +++ b/api/src/object_cache.rs @@ -0,0 +1,121 @@ +// Copyright 2024 the JSR authors. All rights reserved. MIT license. + +//! In-process cache for immutable objects in the modules bucket. +//! +//! Every path cached here already encodes a specific published version, so the +//! bytes behind it never change: a new publish writes a new path. That makes +//! the S3 path a safe cache key with no invalidation story to get wrong. The +//! package-level `meta.json` is *not* eligible — it is rewritten on every +//! publish — and neither is anything else addressed without a version. +//! +//! It exists because these reads were the largest source of repeated R2 round +//! trips. `_meta.json` is fetched once per source-file view, to turn +//! imports into links, but is shared by every file in the version: in a +//! one-hour production sample 76% of the fetches were re-reads of an object +//! already pulled that same hour, and the busiest package fetched the identical +//! object 202 times. +//! +//! Misses are single-flighted, so concurrent requests for the same cold path +//! wait on one download instead of each issuing their own. Absent objects are +//! deliberately *not* cached: a 404 leaves the loader as an error so moka +//! discards it, which stops an object still being written during publish from +//! being remembered as missing. + +use crate::s3::BucketWithQueue; +use crate::s3::S3Error; +use bytes::Bytes; +use std::sync::Arc; +use std::time::Duration; + +/// Total bytes held across all entries. +/// +/// Deliberately small. The objects here are version manifests and READMEs, so +/// this still holds many hundreds of them, and it has to share the API +/// container's memory limit with the doc-node cache. Both are now bounded in +/// bytes, where the doc-node cache used to be bounded only in entry count, so +/// the two together have a smaller worst case than that one cache did alone — +/// this needs no increase to the container's memory limit. +const MAX_TOTAL_BYTES: u64 = 32 * 1024 * 1024; + +/// Objects larger than this are served but not retained. Without a ceiling one +/// outsized object could evict most of the working set to store a single entry. +const MAX_ENTRY_BYTES: usize = 2 * 1024 * 1024; + +/// Drop entries that go unread for this long, so a burst of traffic across many +/// packages does not pin memory for the rest of the process's life. +const TIME_TO_IDLE: Duration = Duration::from_secs(30 * 60); + +#[derive(Debug, thiserror::Error)] +enum LoadError { + #[error(transparent)] + S3(S3Error), + /// Never surfaces to a caller: it is how a 404 escapes `try_get_with` + /// without being cached. + #[error("object not found")] + Absent, +} + +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct ObjectCacheError(Arc); + +#[derive(Clone)] +pub struct ObjectCache { + cache: moka::future::Cache, Bytes>, +} + +impl Default for ObjectCache { + fn default() -> Self { + Self::new() + } +} + +impl ObjectCache { + pub fn new() -> Self { + Self { + cache: moka::future::Cache::builder() + .max_capacity(MAX_TOTAL_BYTES) + .weigher(|_path, bytes: &Bytes| { + bytes.len().try_into().unwrap_or(u32::MAX) + }) + .time_to_idle(TIME_TO_IDLE) + .build(), + } + } + + /// Download `path` from `bucket`, serving it from memory when it has been + /// read before. `Ok(None)` means the object does not exist. + /// + /// Only call this for paths whose contents are immutable — see the module + /// docs. A mutable path (the package-level `meta.json`, an npm packument) + /// would be served stale until it fell out of the cache. + pub async fn download( + &self, + bucket: &BucketWithQueue, + path: Arc, + ) -> Result, ObjectCacheError> { + let loaded = self + .cache + .try_get_with(path.clone(), async { + match bucket.download(path.clone()).await { + Ok(Some(bytes)) => Ok(bytes), + Ok(None) => Err(LoadError::Absent), + Err(err) => Err(LoadError::S3(err)), + } + }) + .await; + + match loaded { + Ok(bytes) => { + // The fetcher, and anyone who joined its single flight, still gets the + // bytes; they just are not kept. + if bytes.len() > MAX_ENTRY_BYTES { + self.cache.invalidate(&path).await; + } + Ok(Some(bytes)) + } + Err(err) if matches!(&*err, LoadError::Absent) => Ok(None), + Err(err) => Err(ObjectCacheError(err)), + } + } +} diff --git a/api/src/util.rs b/api/src/util.rs index 223e2bc80..a54856b4c 100644 --- a/api/src/util.rs +++ b/api/src/util.rs @@ -209,7 +209,8 @@ fn error_response( res } -/// Short negative-cache `Cache-Control` for a `404` on a cached route. Anonymous +/// Short negative-cache `Cache-Control` for a cacheable error (see +/// [`is_cacheable_error_status`]) on a cached route. Anonymous /// (and identity-independent `shared`) requests get a brief `public` window; /// other authenticated requests are never cached (the lb skips its shared cache /// when an `Authorization` header or `token=` cookie is present, so a `public` @@ -226,18 +227,36 @@ fn short_negative_cache_control(public: bool) -> header::HeaderValue { .unwrap() } -/// A missing entrypoint/symbol on an immutable (non-"latest") version can never -/// appear later, so it is as cacheable as a normal `200` for that version — -/// unlike other 404s (package/version not found, or anything on "latest"), which -/// only get the brief negative-cache window. Returns the long-lived value to use -/// for such an error, or `None` to fall back to short negative caching. +/// Errors that are a deterministic property of an immutable (non-"latest") +/// version can never resolve differently later, so they are as cacheable as a +/// normal `200` for that version — unlike other 404s (package/version not +/// found, or anything on "latest"), which only get the brief negative-cache +/// window. Returns the long-lived value to use for such an error, or `None` to +/// fall back to short negative caching. fn immutable_miss_cache_control( err: &ApiError, is_latest: bool, long_lived: impl FnOnce() -> header::HeaderValue, ) -> Option { - (matches!(err, ApiError::EntrypointOrSymbolNotFound) && !is_latest) - .then(long_lived) + let deterministic_for_version = matches!( + err, + // The symbol is absent from this version's docs. + ApiError::EntrypointOrSymbolNotFound + // This version has more symbols than the listing can carry; that is a + // property of its published contents, so it will not change. + | ApiError::DocsSymbolListingTooLarge + ); + (deterministic_for_version && !is_latest).then(long_lived) +} + +/// Statuses whose error responses carry `Cache-Control` on a cached route. +/// +/// `404` has always been here. `413` joined it because the docs symbol-listing +/// refusal is deterministic per version: without a cache header every retry +/// reached the origin, which is how one crawler produced 871 identical failing +/// requests in an hour. +fn is_cacheable_error_status(status: StatusCode) -> bool { + status == StatusCode::NOT_FOUND || status == StatusCode::PAYLOAD_TOO_LARGE } /// Cache an immutable-ish response for `duration`. See [`cache_shared`] for the @@ -312,7 +331,7 @@ where req.param("version").map(|v| v == "latest").unwrap_or(false); let mut res = match handler(req).await { Ok(res) => res, - Err(err) if err.status_code() == StatusCode::NOT_FOUND => { + Err(err) if is_cacheable_error_status(err.status_code()) => { let long_lived = || if public { value } else { private_value }; let cc = immutable_miss_cache_control(&err, is_latest, long_lived) .unwrap_or_else(|| short_negative_cache_control(public)); @@ -425,7 +444,7 @@ where req.param("version").map(|v| v == "latest").unwrap_or(true); let mut res = match handler(req).await { Ok(res) => res, - Err(err) if err.status_code() == StatusCode::NOT_FOUND => { + Err(err) if is_cacheable_error_status(err.status_code()) => { let long_lived = || { if public { versioned_value @@ -1126,6 +1145,7 @@ pub mod test { database: db, buckets: buckets.clone(), generate_ctx_cache: crate::docs::GenerateCtxCache::new(), + object_cache: crate::object_cache::ObjectCache::new(), registry_metadata_cache: crate::api::RegistryMetadataCache::new(), github_client: github_oauth2_client.clone(), gitlab_client: gitlab_oauth2_client.clone(), diff --git a/frontend/routes/package/(_islands)/LocalSymbolSearch.tsx b/frontend/routes/package/(_islands)/LocalSymbolSearch.tsx index 10ef38a4f..2147d4ed8 100644 --- a/frontend/routes/package/(_islands)/LocalSymbolSearch.tsx +++ b/frontend/routes/package/(_islands)/LocalSymbolSearch.tsx @@ -164,9 +164,13 @@ export function LocalSymbolSearch( }, []); async function onInput(e: JSX.TargetedEvent) { + // The index is absent when the symbol listing could not be fetched — a + // package with too many symbols to list is refused by the API. Typing then + // has nothing to search, so do nothing rather than throw on every keystroke. + if (!db.value) return; if (e.currentTarget.value) { const term = e.currentTarget.value; - const searchResult = await search(db.value!, { + const searchResult = await search(db.value, { term, properties: ["name", "description"], threshold: 0.2, diff --git a/lb/proxy.ts b/lb/proxy.ts index 7d2dc4af8..cb790d1ee 100644 --- a/lb/proxy.ts +++ b/lb/proxy.ts @@ -429,7 +429,10 @@ async function cachedFetch( // Only cache responses the origin explicitly marked cacheable: a `max-age` or // `s-maxage` directive, and never `private`/`no-store`. This applies to both - // 200s and (negatively-cached) 404s. Previously an unmarked 200 was cached by + // 200s, (negatively-cached) 404s, and the 413 the API returns for a symbol + // listing too large to serve — that refusal is a deterministic property of a + // published version, and leaving it uncacheable meant every crawler retry + // reached the origin. Previously an unmarked 200 was cached by // default, which silently cached dynamic endpoints that forgot to opt out — // e.g. the publish-status poll (`util::json`, no `Cache-Control`), pinning a // stale "pending"/"processing" status so `deno publish` hung until the entry @@ -440,7 +443,8 @@ async function cachedFetch( cacheControl.includes("no-store"); const hasCacheableDirective = cacheControl.includes("max-age") || cacheControl.includes("s-maxage"); - const cacheable = (res.ok || res.status === 404) && + const cacheableStatus = res.ok || res.status === 404 || res.status === 413; + const cacheable = cacheableStatus && !explicitlyUncacheable && hasCacheableDirective; // An authenticated request may only write an identity-independent response — // a viewer-specific authed response must never land in the shared cache. diff --git a/lb/proxy_test.ts b/lb/proxy_test.ts index 00e1761e6..2b40cfedb 100644 --- a/lb/proxy_test.ts +++ b/lb/proxy_test.ts @@ -984,6 +984,62 @@ Deno.test("proxyToBackend negatively caches 404s with a public TTL", async () => } }); +Deno.test("proxyToBackend caches a 413 symbol-listing refusal", async () => { + const cache = createFakeCache(); + (globalThis as any).caches = { default: cache }; + + // A package with more symbols than the listing can carry is refused with 413. + // That is a fixed property of the published version, so the API stamps it + // cacheable — without this, one crawler produced 871 identical failing + // requests to the origin in an hour. + const restore = setupFetchStub( + new Response("Payload Too Large", { + status: 413, + headers: { + "Cache-Control": "public, max-age=60, s-maxage=2592000", + }, + }), + ); + + try { + const request = new Request( + "https://jsr.io/api/scopes/s/packages/p/versions/1.0.0/docs/search_structured", + { method: "GET" }, + ); + const response = await proxyToBackend(request, BACKEND_URL); + + assertEquals(response.status, 413); + assertEquals(cache.putCalls.length, 1); + } finally { + restore(); + (globalThis as any).caches = { default: undefined }; + } +}); + +Deno.test("proxyToBackend does not cache a 413 without a cacheable directive", async () => { + const cache = createFakeCache(); + (globalThis as any).caches = { default: cache }; + + // Only the API's explicit opt-in makes an error cacheable; an unmarked 413 + // from anywhere else must still reach the origin next time. + const restore = setupFetchStub( + new Response("Payload Too Large", { status: 413 }), + ); + + try { + const request = new Request("https://jsr.io/api/too-big", { + method: "GET", + }); + const response = await proxyToBackend(request, BACKEND_URL); + + assertEquals(response.status, 413); + assertEquals(cache.putCalls.length, 0); + } finally { + restore(); + (globalThis as any).caches = { default: undefined }; + } +}); + Deno.test("proxyToBackend does not cache 404s with no-store", async () => { const cache = createFakeCache(); (globalThis as any).caches = { default: cache };