From 888163ad7c119bb82692a1397b0fe83771166700 Mon Sep 17 00:00:00 2001 From: Leo Kettmeir Date: Sat, 29 Aug 2026 12:52:28 +0000 Subject: [PATCH] fix(api): stop re-downloading immutable objects, and bound the all-symbols listing (#1542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tracing showed the R2 download step taking 45-92% of request time on `/docs` and `/source`, spread across a wide, effectively random set of packages, with no download faster than ~130ms. That reads like a storage-latency problem, and the natural conclusion is to put a cache or CDN in front of the buckets. It isn't one, and that fix can't be built as stated. The API reaches the buckets over the S3 API at `.r2.cloudflarestorage.com`, which is not served through Cloudflare's cache and cannot be fronted by one. (A CDN cache in front of the *public* bucket paths already exists — `proxyToR2` serves `jsr.io/@scope/pkg/…` out of `caches.default`. It just isn't in the path the API uses.) The traces are accurate about *where* time goes. What they can't show is that most of those downloads shouldn't be happening at all: the same immutable objects are fetched over and over. Latency per fetch is not really the problem; the number of fetches is. This PR removes the repeats, and fixes two other things found along the way. ## 1. The same immutable object is downloaded on every request `_meta.json` is downloaded once per source-file view, to turn imports into links. It's immutable per version and shared by every file in that version — but nothing cached it. 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. Three other call sites read the same object and also re-download it every time. The README on the package overview is the same shape: immutable per version, re-fetched on every render even when the doc context cache hits. This adds an in-process cache for immutable modules-bucket objects. The S3 path already encodes a published version, so it's a safe key with nothing to invalidate. The package-level `meta.json` is deliberately excluded — it's rewritten on every publish. Two details worth reviewing: - **Absences are not cached.** A 404 leaves the loader as an error so it isn't stored, which keeps an object still being written during publish from being remembered as missing. - **Misses are single-flighted**, so a cold key costs one download rather than one per waiting request. ### On memory The cache is bounded in **bytes**, not entries, and needs no increase to the container's memory limit. `GenerateCtxCache` used `max_capacity(64)`, which counts entries. That had two problems: 64 tiny packages evicted each other exactly as readily as 64 enormous ones (a sample of 2000 docs requests touched 534 distinct packages), and the bytes actually held were bounded by nothing at all — the comment estimated "2-5mb average, 320mb max" but nothing enforced it. It's now weighed by the stored size of the doc nodes each context was built from. So both caches are byte-bounded, and the two together have a smaller worst case than that one cache had on its own. `GenerateCtxCache` misses are single-flighted too. That matters more than for a typical cache: building a context allocates the 10-50 MB that the render permit exists to bound, so concurrent requests for the same cold package multiplied real memory, not just CPU. ## 2. The symbol listing limit doesn't bound the all-symbols page Found while looking at what else was consuming origin capacity, and it turned out to be the larger cost. `SYMBOL_LISTING_LIMIT` (2048) caps how many symbol rows a module listing may contain. Its comment says why it exists: to keep the docs response under Cloud Run's 32 MiB response limit. But it's applied inside `ModuleDocCtx::new`, so it bounds **one module**. `AllSymbolsCtx::new` builds one listing *per entrypoint* and concatenates them, so a package with many entrypoints multiplies straight past the cap. `@ubx/sdk-aws` publishes 1921 exports, putting its ceiling around 3.9 million rows. When the response exceeds the limit, Cloud Run kills the request mid-flight. Every attempt builds the entire listing, spends ~1.6s of CPU, and returns nothing usable. And a 500 carries no cache headers, so retries always reach the origin — a search crawler walking that package's symbol pages produced **871 identical failing requests in one hour**, roughly half of one instance's capacity spent on work that could never succeed. This refuses up front with a **413** instead. The count mirrors what rendering would actually emit, so it errs towards serving rather than refusing. The durable fix belongs in `deno_doc` — the limit should be a total budget rather than a per-module one. When that lands, this guard should stop firing on its own. **Making the refusal cacheable took three changes**, since the outcome is a fixed property of a published version: 1. The API attaches a long-lived `Cache-Control` to the 413 on pinned versions, alongside the existing `EntrypointOrSymbolNotFound` case. 2. `cache_versioned_impl` / `cache_impl` previously attached cache headers only to 404s. 3. `lb/proxy.ts` previously cached only 200s and 404s, so the header would have been inert. ## 3. Both docs search routes were missing `_shared` `docs/search` and `docs/search_structured` are derived purely from the published version, with no permission/member/sudo branch — exactly like `docs` above them, which is already `_shared`. Marking them lets the lb serve them from its shared cache to authenticated callers instead of bypassing the cache whenever auth is present. ## Frontend `LocalSymbolSearch` already disables the input when the index is missing, but `onInput` asserted the index was present. A refused listing now leaves it unset by design, so guard it rather than throw on every keystroke. ## Not in this PR - **The source file bytes and their syntax highlighting.** Both immutable per `(scope, package, version, path)` and both currently uncached, but the working set is much wider than the metadata objects, so it needs a memory budget this PR doesn't try to claim. - **Reading the module graph from Postgres instead of R2.** `source_specifier_links` needs exactly one module's info plus the file list, and Postgres already has the file list. Storing the per-module info at publish would delete the fetch entirely rather than caching it — but that's a migration plus a backfill. ## Testing - 3 new unit tests covering the per-entrypoint accumulation, including the case this fixes: many entrypoints each well under the per-module limit still exceeding the total. - 2 new lb tests: a 413 with a cacheable directive is cached, one without still isn't. - `cargo clippy`, `cargo fmt`, and the lb suite (43 tests) are clean. - The database-backed API tests were not run locally and need CI. --- api/src/api/errors.rs | 16 ++ api/src/api/package.rs | 71 ++++-- api/src/docs.rs | 217 +++++++++++++++--- api/src/main.rs | 6 + api/src/object_cache.rs | 121 ++++++++++ api/src/util.rs | 40 +++- .../package/(_islands)/LocalSymbolSearch.tsx | 6 +- lb/proxy.ts | 8 +- lb/proxy_test.ts | 56 +++++ 9 files changed, 471 insertions(+), 70 deletions(-) create mode 100644 api/src/object_cache.rs 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 };