feat(npm): tarball download serve + Cache-Control (S11 Step 9, 3/6)

📦 What

S11 Step 9 (tarball download) is a six-MR stack, in dependency order (each targets the one above; GitLab auto-retargets to main as they merge):

  • !535 (merged) - two-id Resolution for npm read handlers. (merged)
  • !543 (merged) - NpmVersionByPackageAndVersion datastore finder (read-side prerequisite). (merged)
  • !536 (merged) - tarball download handler: serve + Cache-Control. <- this MR
  • !562 (merged) - last_downloaded_at bump on a served download.
  • !563 (merged) - perf: resolve the version by an indexed point lookup.
  • !537 (merged) - conditional GET + strong validators.

This MR now targets main, rebased on main after !535 (merged) and !543 (merged) merged.

Slice 1 - the core serve path. Implements DownloadHandler for GET/HEAD /{slug}/npm/{repository_name}/{package_name}/-/{file_name}:

  • Resolves the npm_files row through package -> versions -> files (keyset-paginated), keyed on Resolution.NpmRepositoryID.
  • Rejects path-traversing/malformed {file_name} (400 file_name_invalid) before any lookup; 404 package_not_found / file_not_found for missing or soft-deleted rows (the blob is not served even if it survives in CAS).
  • Serves the blob via storage.BlobStore.OpenBlob (307 redirect or 200 stream; S06 owns the wire format); the 200 stream sets Content-Length from the blob size instead of chunked encoding.
  • Sets the immutable, visibility-varying Cache-Control (public, max-age=31536000, immutable for public repositories, private, ... otherwise) and Vary: Authorization on the 200-stream and HEAD success branches — not the 307 redirect (whose signed Location expires; matches the OCI handler), and never on a 404/500 envelope (RFC 9111).
  • Hardens the tarball serve path with X-Content-Type-Options: nosniff + Content-Disposition: attachment (matches the OCI blob handler).
  • Answers HEAD via BlobStore.BlobInfo (existence + size, not OpenBlob): sets Content-Length and the same hardening headers, and 404s when the blob is absent from CAS so HEAD agrees with GET.

The last_downloaded_at bump (!562 (merged)), the indexed point-lookup optimization (!563 (merged)), and the conditional-GET 304 short-circuit (!537 (merged)) layer on in the stacked follow-ups; this slice resolves the version with the keyset scan and performs no bump. Because that keyset scan is O(versions) on the hot path, !536 (merged) must not merge to production ahead of !563 (merged) - land them in the same merge train.

Spec coverage

Behaviour Test
AC 17 - served via BlobStore.OpenBlob TestDownloadHandler_GET_ServesTarballWithCacheHeaders, TestDownloadHandlerIntegration_GET_ServesTarball, TestDownloadHandler_GET_RedirectMode_ServesRedirect
AC 18 - soft-deleted file -> 404 file_not_found TestDownloadHandler_SoftDeletedFile_Returns404, TestDownloadHandlerIntegration_SoftDeletedFile_Returns404
AC 34 - package missing -> 404 package_not_found TestDownloadHandler_MissingPackage_Returns404, TestDownloadHandlerIntegration_MissingPackage_Returns404
AC 53 - ..// in {file_name} -> 400 file_name_invalid TestDownloadHandler_PathTraversalFileName_Returns400
AC 56 - OpenBlob transient failure deferred to S07 fault-injection (TODO(fault-injection) at the surfacing site)
Cache-Control varies by repositories.visibility TestDownloadHandler_CacheControl_VariesByVisibility
HEAD existence check (BlobInfo) + Content-Length, no body TestDownloadHandler_HEAD_NoBody
HEAD 404s when the blob is absent from CAS (agrees with GET) TestDownloadHandler_HEAD_BlobMissingFromCAS_Returns404
multi-page keyset resolution (version + file-name cursor advance) TestDownloadHandler_MultiPageKeyset_ResolvesFileOnSecondPage
finder / visibility errors -> 500, no raw-error leak TestDownloadHandler_InternalErrors_Return500
error envelopes (404/500) omit the immutable Cache-Control TestDownloadHandler_ErrorResponses_OmitImmutableCacheControl
fail-fast on nil deps TestNewDownloadHandler_PanicsOnNilDependencies

⚠️ Known gap - handler not yet wired into the route table

DownloadHandler is fully implemented and tested, but nothing mounts it: internal/format/npm/handler.go still serves the 501 Not Implemented placeholder for the tarball route, and neither handler.go nor wire_npm.go constructs the handler. The tests drive it directly, so a live npm tarball fetch still returns 501.

This is an unassigned plan-level gap: the npm hosted plan creates handler.go with placeholders in Step 8 and never assigns the placeholder->real-handler swap to any handler step. It surfaces no later than Step 23 (real npm-CLI conformance) and will be closed via a plan amendment, not bolted onto this MR. Deliberately out of scope here.

🧪 Testing

  • go test ./internal/format/npm/... - pass
  • golangci-lint run ./internal/format/npm/... - 0 issues
  • go vet -tags integration ./internal/format/npm/... - clean (integration build compiles)
  • Integration tests (//go:build integration) run in the merged-results pipeline.

Related to #127 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading