Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions crates/frameshift-server/src/routes/downloads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
18 changes: 16 additions & 2 deletions crates/frameshift-server/src/routes/packs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
48 changes: 48 additions & 0 deletions crates/frameshift-server/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<RwLock<MockState>>, 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
// ---------------------------------------------------------------------------
Expand Down
24 changes: 20 additions & 4 deletions crates/frameshift-server/tests/mocks/catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<u64, CatalogError> {
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).
Expand Down
Loading