feat(oci): move container downloads_count on the two S12 read events (repository-column-writers plan: 5/5)

Step 5 of the repository-column-writers plan gives repositories.downloads_count its container writer: a delivered blob or manifest read books one increment through the buffered pipeline Step 4 landed.

What a delivered read means here

S12's event table books a download for a blob GET or HEAD and a manifest GET or HEAD, and its "does not emit" list counts a 206 as the one increment a full 200 is. Six statements carry that across two emitSite values, and where each sits is the part worth reviewing:

  • handleHead (blob.go) and the manifest HEAD arm book on the success status each wrote. Neither has a body to copy, so the status is the whole of what they deliver.
  • serveRedirect books on the 307. It streams no bytes and is still a delivered read; ADR-012's redirect arm is the rule. Its RedirectURL() failure arm cannot fire while IsRedirect() holds, so the placement there rests on the delivered 307 rather than on that guard.
  • serveFull, servePartial and the manifest GET arm book only after copying every byte they declared. ADR-012 states the rule for a proxied read: a partial transfer is not a download. io.Copy returns a nil error for a reader that ends early, so these arms compare the copied count against the declared length rather than trusting the error alone; servePartial's io.CopyN already reported a short read as an error, and the other two now agree with it.

The in-flight cap, and the share the reads take

The plan's Dependencies section rules that this step owns the cap's composition: OCI's counterEmitSem is flat and process-wide, and until this step it carried write-path emits only. Reads exceed writes by orders of magnitude, so saturation stops being rare, and a shed download is permanent where a shed byte delta heals at the next reconciliation pass.

This step takes the mitigation the plan names, a per-repository sub-cap of the downloadSignalMaxPerRepository shape. detach now takes a second, per-repository acquire after the global slot and releases it before, the order downloadSignals.record already used. The two quotas are the one perRepositoryQuota type: the shape existed in remote_download.go and was generalised rather than copied, so there is one implementation and two caps.

The byte deltas deliberately take no share. Bounding them per repository would shed a write delta this cap admits today, which is a new loss, where a share on the reads only changes which read is shed.

counterEmitDownloadMaxPerRepository is 16 of 64, matching the value the remote read path already admits per repository.

Residuals, with owners

  • A shed download names no scope. Both drop arms, the global shed and the per-repository refusal, increment the series and return; only the panic arm logs, and it carries emit_site and column but no namespace or repository. So the paging rule this MR adds tells on-call that increments were lost and not which repository undercounted. A scope-carrying record would have to be rationed, because the arm fires most exactly when the cap is saturated. docs/dev/alerting.md states this, and it sits with the S22 observability follow-ups in #435, whose assignee is @vespian_gl.
  • Maven's vector has no rule. Maven books the same permanent downloads_count loss, but its buffered UPDATE reports ok, so it carries a share and needs a differently shaped rule than the two any-increase rules here. docs/dev/alerting.md names it; #435 owns it, and its assignee is @vespian_gl.
  • No result="ok" arm is added on this column, which the plan keeps a cross-format question and out of this step's scope.
  • The container remote read path books nothing, which is a gap rather than a rule: #292 owns the mechanism decision for it.
  • The N+1 figure in the plan (:1673 and :1818) undercounts a real pull, which also fetches the image config blob over the same /blobs/<digest> route. This MR corrects the figure at its three in-tree sites and leaves the plan alone, because guardrail 4 gives the plan a single writer; the correction there needs its own docs(plans) MR.

Reviewable LOC

1841 added+removed across 33 files, past the 500 the development model asks about. Split by group:

Group LOC
Go tests 1184
Go production 435
Markdown 222

Splitting is not the better shape here. The production change is 435 LOC across nine files, 91 of which are the shared per-repository quota generalised out of remote_download.go, and 64% of the diff is the suite that covers it; a split would open a step MR carrying only red tests, which guardrail 9 allows exactly once and only for a branch that then adds the implementation.

The branch's commits are not split by logical unit either. bc77755e bundles three separable units: the per-repository cap seam, the copied-vs-declared length check, and the documentation sweep, over the 23 files git show --stat reports for it. That commit is no longer the branch tip; later commits have landed on top of it. squash and squash_on_merge are both set on this MR, so those commits never reach main as separate commits and the bundling costs review only. Unbundling it now would mean rewriting the branch's history and force-pushing, which is not worth a cost that ends at review.

Verification

  • go build ./..., go vet ./..., go vet -tags=integration ./internal/format/oci/...: clean.
  • go test ./internal/format/oci/... ./internal/metrics/... ./internal/accounting/... ./cmd/...: pass.
  • golangci-lint run and golangci-lint run --build-tags=integration over the changed packages: 0 issues. The integration-tagged run is the one CI cannot see, per guardrail 7.
  • scripts/ci/check-comment-caps.sh --base origin/main: within caps.
  • Every new guard is falsifiable, checked by breaking each one and re-running: the per-repository acquire, its deferred release, the global-slot handback on a share shed, both length checks, and all six emit statements each redden the suite when removed.

Conformance

No conformance run is owed. Every new statement is a detached counter dispatch placed after the response status is written, and the two added returns replace fall-throughs, so no wire behaviour moves.

e2e

docs/testing/e2e/oci.md gains e2e.oci.consume.repository-downloads-count and its usage-data row. The scenario states the pull total per request class rather than as a formula, names the config blob, and says to drive the abandoned-transfer arm with a body larger than the response buffer, because net/http buffers a small response until the handler returns.

Merge order

docs/testing/e2e/oci.md overlaps !2052 (merged) (sahmed/e2e-caproni-authz), and that path conflicts against this branch while merging cleanly against main, so the conflict is this branch's to coordinate. !2052 (merged) needs a rebase for eight unrelated files regardless, and !2595 (closed) is stacked on it. No pipeline reports this overlap.

Spec coverage

Spec: docs/specs/S12-container-oci-hosted.md

Ordinals are derived at dae4745b8, the branch point: S12 numbers its criteria as a markdown list rather than with literal AC-n labels. P-n is the Properties list, AC-n the Scenarios list. The labels already in this package's comments were derived against earlier revisions of those lists and do not all line up with these: blob_test.go:148 cites AC-25 for the no-Range blob GET, which is the tag listing here, and :841 cites E-18 for INTERNAL, which is E-19 here.

Step 5 gives repositories.downloads_count a writer on S12's two read rows. Every row below that reads "Not this step" names behavior already on main under its own suite; the accounting table at the end is the section this step implements.

Acceptance criteria

# Criterion Tests
P-1 Roundtrip integrity Not this step. TestManifestGet_ByTagAndByDigest owns byte identity; the delivered manifest read's count is TestContainerRead_DeliveredReadBooksOneDownload/a manifest GET.
P-2 Digest correctness Not this step: a write-path property.
P-3 Chunked upload reassembly Not this step: a write-path property.
P-4 Push idempotency Not this step: a write-path property.
P-5 Delete invariant Not this step: a delete-path property.
P-6 Referrers discrimination Not this step. The accounting half is TestDiscoveryRoutes_BookNoDownload/the referrers GET.
AC-1 Blob upload (monolithic) Not this step. Its emit suite now also asserts a zero download sum, through assertTotals.
AC-2 Blob upload (single POST) Not this step, as AC-1.
AC-3 Blob upload (chunked) Not this step, as AC-1.
AC-4 Blob upload (streamed PATCH, no declared length) Not this step, as AC-1.
AC-5 Blob mount Not this step, as AC-1.
AC-6 Blob mount fallback Not this step, as AC-1.
AC-7 Manifest push (OCI Image Manifest by tag) Not this step, as AC-1.
AC-8 Manifest push (OCI Image Index) Not this step, as AC-1.
AC-9 Manifest push (with subject, subject exists) Not this step, as AC-1.
AC-10 Manifest push (with subject, subject missing) Not this step, as AC-1.
AC-11 Manifest push (Docker Schema 2) Not this step, as AC-1.
AC-12 Manifest push (Schema 1 rejection) Not this step: a rejected push books nothing on any column.
AC-13 Manifest push (Docker Schema 2 to oci) Not this step, as AC-1.
AC-14 Manifest push (OCI image manifest to docker) Not this step, as AC-1.
AC-15 Manifest push (non-image artifact to docker) Not this step, as AC-12.
AC-16 Manifest push (referrer to docker) Not this step, as AC-1.
AC-17 Manifest push (cosign signature to docker) Not this step, as AC-1.
AC-18 Manifest push (default buildx index to docker) Not this step, as AC-1.
AC-19 Manifest push (missing blob) Not this step, as AC-12.
AC-20 Manifest push (size limit) Not this step, as AC-12.
AC-21 Manifest push (error accumulation) Not this step, as AC-12.
AC-22 Manifest delete (success) Not this step, as AC-1.
AC-23 Manifest delete (referenced by index) Not this step, as AC-12.
AC-24 Manifest delete (by tag) Not this step, as AC-1.
AC-25 Tag listing (paginated) Accounting half: TestDiscoveryRoutes_BookNoDownload/the tag listing GET. Pagination itself stays with tags_test.go.
AC-26 Referrers (with filter) Accounting half: TestDiscoveryRoutes_BookNoDownload/the referrers GET. The filter stays with referrers_test.go.
AC-27 Referrers (missing subject) Accounting half: TestDiscoveryRoutes_BookNoDownload/the referrers GET, whose digest matches no subject.
AC-28 Zero-byte blob No row of its own: the count is per request, not per byte, which TestBlobGet_PartialContentBooksOneDownload pins over two slice lengths.
AC-29 Content negotiation (Accept mismatch) TestContainerRead_UndeliveredReadBooksNothing/a manifest GET whose Accept header excludes the stored type.
AC-30 Upload recovery Accounting half: TestUploadSessionRoutes_BookNoDownload (session GET and PATCH). The 416-and-retry flow stays with upload_test.go.
AC-31 Blob HEAD TestContainerRead_DeliveredReadBooksOneDownload/a blob HEAD. The header set stays with TestBlobHeadGetParity.
AC-32 Blob range request TestBlobGet_PartialContentBooksOneDownload, both legs. The Content-Range header stays with TestBlobGet_RangeTable.
AC-33 Blob delete Not this step, as AC-1.
AC-34 Upload session GET TestUploadSessionRoutes_BookNoDownload/the session GET reporting the current offset.
AC-35 Upload session DELETE TestUploadSessionRoutes_BookNoDownload/the session DELETE cancelling the upload.
AC-36 Base endpoint TestDiscoveryRoutes_BookNoDownload/the base endpoint GET /v2/. The 401 challenge arm is auth-middleware-owned.
AC-37 Manifest push (by digest only) Not this step, as AC-1.

Error cases

# Condition Tests
E-1 BLOB_UNKNOWN 404: blob not found by digest TestContainerRead_UndeliveredReadBooksNothing: the unlinked-digest GET row and the a blob HEAD for a digest the store does not hold row.
E-2 BLOB_UPLOAD_INVALID 416: out-of-order chunk, corrupted session TestContainerRead_UndeliveredReadBooksNothing: the Range starts past the end row and the refused on the re-open row.
E-3 BLOB_UPLOAD_UNKNOWN 404: session absent, expired, done, cancelled Not this step: no session route books a download on any status, which TestUploadSessionRoutes_BookNoDownload covers on the success arm.
E-4 DIGEST_INVALID 400: digest mismatch or malformed TestContainerRead_UndeliveredReadBooksNothing/a blob GET whose digest does not parse.
E-5 MANIFEST_BLOB_UNKNOWN 400 Not this step: a rejected push.
E-6 MANIFEST_INVALID 400 Not this step: a rejected push.
E-7 MANIFEST_INVALID 413 Not this step: a rejected push.
E-8 MANIFEST_UNKNOWN 404: not found by tag or digest, or not in Accept TestContainerRead_UndeliveredReadBooksNothing: the two resolves to nothing rows and the Accept-mismatch row.
E-9 MANIFEST_REFERENCED 409 Not this step: a blocked delete.
E-10 NAME_UNKNOWN 404: repository or image does not exist TestContainerRead_UndeliveredReadBooksNothing/a blob GET under an image name that resolves to no row.
E-11 SIZE_INVALID 400 Not this step: an upload framing error.
E-12 SIZE_INVALID 413 Not this step: an upload size error.
E-13 MANIFEST_LIMIT_EXCEEDED 400 Not this step: a rejected push.
E-14 TAG_LIMIT_EXCEEDED 400 Not this step: a rejected push.
E-15 PAGINATION_NUMBER_INVALID 400 Not this step: a discovery route, which books nothing on any status per TestDiscoveryRoutes_BookNoDownload.
E-16 NAME_INVALID 400: image grammar or 255-character path Not covered here. The route is rejected before resolution, so no site this step touches is reached; blob_test.go owns the status.
E-17 UNSUPPORTED 405: non-GET on the base endpoint Not covered here. TestDiscoveryRoutes_BookNoDownload drives the GET arm; the base handler holds no sink on any verb.
E-18 UNAVAILABLE 503: GC lock, pool exhaustion, transient conditions Not this step: no read site produces a 503, and a shed emit answers 2xx by design.
E-19 INTERNAL 500: unexpected internal error TestContainerRead_UndeliveredReadBooksNothing: a blob GET whose storage read failed and a manifest GET whose read seam failed.

Security considerations

# Concern Tests
S-1 Bearer token on every endpoint but GET /v2/ Auth-middleware-owned. Not tested in this MR.
S-2 Mount requires dual authorization Not this step: a write path.
S-3 Digest format validated before any lookup TestContainerRead_UndeliveredReadBooksNothing/a blob GET whose digest does not parse pins that the rejection books nothing; blob_test.go owns the validation.
S-4 Tag format Not this step: no tag-format validation runs on the pull path.
S-5 Image name grammar and total path length Not this step, as E-16.
S-6 Manifest payload size-limited to 4 MB, parsed as JSON Not this step: a push-path control.
S-7 Content-Range validation Not this step: an upload-path control.
S-8 Blob size limit (50 GB) Not this step: an upload-path control.
S-9 Manifest size limit (4 MB) before parsing Not this step: a push-path control.
S-10 Max references per manifest (200) Not this step: a push-path control.
S-11 Upload session expiry (24 hours) Not this step: a session-lifecycle control.
S-12 Pagination limits (max 1000 per page) Not this step: a discovery-route control.
S-13 Digest verification on all writes Not this step: a write-path control.
S-14 Referential integrity on delete Not this step: a delete-path control.
S-15 Hard-delete only Not this step: a delete-path control.

Counter and timestamp events (the section this step implements)

Rows are S12's #### Counter and Timestamp Updates event table and its "What does not emit events" list, at :1739-1760.

Row Behavior Tests
Event table: Blob GET or HEAD → downloads_count += 1 One increment per delivered blob read, against the repositories row TestContainerRead_DeliveredReadBooksOneDownload: the full-body GET, the redirect GET and the HEAD rows. TestBlobGet_PartialContentBooksOneDownload for the 206.
Event table: Manifest GET or HEAD → downloads_count += 1 One increment per delivered manifest read TestContainerRead_DeliveredReadBooksOneDownload: the manifest GET and manifest HEAD rows.
Event table: both read rows' last_downloaded_at container_images and container_manifests timestamps Not this step: S18's, with no spec yet. The read sites move no repositories.last_updated_at either, which assertNoStamp pins on every positive row.
Does not emit: failed or 4xx/5xx responses No counter change TestContainerRead_UndeliveredReadBooksNothing, 11 rows across 400, 404, 416 and 500 on both read routes.
Does not emit: Range (206) responses One increment per request, "same as a full 200" TestBlobGet_PartialContentBooksOneDownload, both the offset-0 leg and the re-opened leg.
Does not emit: upload session GET, PATCH, DELETE No counter change TestUploadSessionRoutes_BookNoDownload, one row per method, through a handler carrying the same sink the finalize books on.
Does not emit: tag listing and referrers GET No counter change TestDiscoveryRoutes_BookNoDownload, two rows. Green before the implementation too: those handlers hold no sink, so the case guards against a hook placed in the dispatcher or a shared writer rather than reddening now.
Does not emit: base endpoint GET /v2/ No counter change TestDiscoveryRoutes_BookNoDownload/the base endpoint GET /v2/, on the terms of the row above.
Does not emit: idempotent manifest re-push No counter change, last_updated_at still moves Not this step: a write row. TestEmitPushDeltas_ZeroDeltaTakesNoDispatchSlot owns it, and now asserts a zero download sum with it.
Does not emit: idempotent blob re-push in the same image No counter change Not this step: a write row, as above, under TestCompleteUpload_ZeroDeltaTakesNoDispatchSlot and TestMountBlob_ZeroDeltaTakesNoDispatchSlot.
Plan, Observability acceptance: a shed or panicked read emit meters under the read column column="downloads_count", never size_bytes TestContainerRead_ShedDownloadMetersItsOwnColumn, a shed blob GET and a shed manifest HEAD, each also asserting size_bytes and last_updated_at stay flat.
Plan, Observability acceptance: no result="ok" arm The dispatcher emits no ok on any column Not asserted: the plan keeps the denominator a cross-format question and out of scope, so there is no new behavior to pin.

Closes #1224 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading