From b39816dabea757e2ff819ef08b90f30137b7e00a Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Mon, 29 Jun 2026 15:12:46 -0400 Subject: [PATCH] fix(server): increment total_downloads on pack download The catalog `total_downloads` counter shown on the marketplace was never incremented by any route: the download handler only recorded a `pack_downloads` event (which feeds the 7-day Trending ranking), while the counter that backs `total_downloads` is bumped exclusively by `increment_download_counter`, which no handler called. As a result every pack reported 0 total downloads. Wire `increment_download_counter` into `download_pack_bytes` alongside the existing `record_download` call, as a best-effort step that warns and continues on failure so a download the client already received is never failed. Document why the signed `/dl/{hash}` path cannot increment the counter (it has only the content hash, not name/version; the official client uses the direct route). Add an integration test asserting a successful download increments the counter. --- .../frameshift-server/src/routes/downloads.rs | 7 +++ crates/frameshift-server/src/routes/packs.rs | 18 ++++++- crates/frameshift-server/tests/integration.rs | 48 +++++++++++++++++++ .../frameshift-server/tests/mocks/catalog.rs | 24 ++++++++-- 4 files changed, 91 insertions(+), 6 deletions(-) diff --git a/crates/frameshift-server/src/routes/downloads.rs b/crates/frameshift-server/src/routes/downloads.rs index 273ac6e..e6a8d1e 100644 --- a/crates/frameshift-server/src/routes/downloads.rs +++ b/crates/frameshift-server/src/routes/downloads.rs @@ -253,5 +253,12 @@ pub async fn stream_signed_download( // Count successful signed-download responses (alongside direct pack downloads). state.metrics.pack_downloads_total.inc(); + // NOTE: total_downloads is NOT incremented here. This path has only a + // content hash -- pack name and version are not available, so + // increment_download_counter cannot be called. The official client + // (frameshift-client) downloads via `/v1/packs/{name}/versions/{version}/pack`, + // which does increment total_downloads. Callers that want counted downloads + // should use that route. + Ok(response) } diff --git a/crates/frameshift-server/src/routes/packs.rs b/crates/frameshift-server/src/routes/packs.rs index a7a9d74..894973f 100644 --- a/crates/frameshift-server/src/routes/packs.rs +++ b/crates/frameshift-server/src/routes/packs.rs @@ -893,11 +893,25 @@ pub async fn download_pack_bytes( // Count successful direct-download responses. state.metrics.pack_downloads_total.inc(); - // Record the download for trending ranking. Best-effort: a failure here must - // not fail the download the client already received. + // Record the download event for trending ranking -- feeds the 7-day velocity + // used by SortMode::Trending. Best-effort: a failure here must not fail the + // download the client already received. if let Err(e) = state.catalog.record_download(&name, &version).await { tracing::warn!(pack = %name, version = %version, error = %e, "record_download failed"); } + // Increment the cumulative download counter -- feeds `total_downloads` on + // the pack record shown on the marketplace catalog page. Best-effort: same + // policy as record_download above; warn and continue on failure. NotFound + // is unreachable here (the version record was fetched above), but the + // best-effort pattern handles it safely regardless. + if let Err(e) = state + .catalog + .increment_download_counter(&name, &version) + .await + { + tracing::warn!(pack = %name, version = %version, error = %e, "increment_download_counter failed"); + } + Ok(response) } diff --git a/crates/frameshift-server/tests/integration.rs b/crates/frameshift-server/tests/integration.rs index 638ed82..e84866c 100644 --- a/crates/frameshift-server/tests/integration.rs +++ b/crates/frameshift-server/tests/integration.rs @@ -307,6 +307,54 @@ async fn download_pack_502_when_blob_missing_from_objects() { assert_eq!(body["error"], "upstream backend mismatch"); } +/// `GET /v1/packs/{name}/versions/{version}/pack` calls `increment_download_counter` +/// on a successful 200 response. +/// +/// Regression test: before the fix, `download_pack_bytes` only called +/// `record_download` (trending) and never `increment_download_counter`, so +/// `total_downloads` on every pack was permanently 0. +#[tokio::test] +async fn download_pack_increments_download_counter() { + let blob = b"counted".to_vec(); + let hash = ObjectHash::of(&blob); + let author_key = Ed25519PublicKey([5u8; 32]); + + let catalog = MockCatalog::new(); + { + let mut state = catalog.state.write().unwrap(); + state + .packs + .insert("counted-pack".to_string(), make_pack("counted-pack", author_key)); + state.versions.insert( + ("counted-pack".to_string(), "1.0.0".to_string()), + make_version("counted-pack", "1.0.0", hash, author_key), + ); + } + // Clone before moving into make_state -- both sides share the same + // Arc>, so writes through AppState are visible here. + let catalog_observer = catalog.clone(); + + let objects = MockPackStore::new(); + objects.insert(hash, blob.clone()); + + let state = make_state(catalog, objects); + let resp = oneshot_get(state, "/v1/packs/counted-pack/versions/1.0.0/pack").await; + assert_eq!(resp.status(), StatusCode::OK, "download should succeed"); + + let increments = catalog_observer + .state + .read() + .unwrap() + .download_counter_increments + .get(&("counted-pack".to_string(), "1.0.0".to_string())) + .copied() + .unwrap_or(0); + assert_eq!( + increments, 1, + "increment_download_counter must be called exactly once per successful download" + ); +} + // --------------------------------------------------------------------------- // /v1/authors // --------------------------------------------------------------------------- diff --git a/crates/frameshift-server/tests/mocks/catalog.rs b/crates/frameshift-server/tests/mocks/catalog.rs index 1efee37..300b575 100644 --- a/crates/frameshift-server/tests/mocks/catalog.rs +++ b/crates/frameshift-server/tests/mocks/catalog.rs @@ -48,6 +48,12 @@ pub struct MockState { /// When `true`, the next mutating call returns `CatalogError::Conflict`. pub inject_conflict: bool, + + /// Number of `increment_download_counter` calls per `(pack_name, version)`. + /// + /// Tests read this to assert that the cumulative download counter was + /// incremented after a successful download response. + pub download_counter_increments: HashMap<(String, String), u64>, } /// In-memory [`CatalogBackend`] for integration tests. @@ -259,13 +265,23 @@ impl CatalogBackend for MockCatalog { Ok(results) } - /// Increment download counter (no-op in mock). + /// Increment the download counter for a pack version. + /// + /// Records the call in `state.download_counter_increments` so tests can + /// assert that `download_pack_bytes` actually invoked this method. async fn increment_download_counter( &self, - _name: &str, - _version: &str, + name: &str, + version: &str, ) -> Result { - Ok(0) + let mut state = self + .state + .write() + .map_err(|e| CatalogError::BackendError(e.to_string().into()))?; + let key = (name.to_string(), version.to_string()); + let count = state.download_counter_increments.entry(key).or_insert(0); + *count += 1; + Ok(*count) } /// Tombstone a pack version (no-op in mock).