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 manifestHEADarm book on the success status each wrote. Neither has a body to copy, so the status is the whole of what they deliver.serveRedirectbooks on the307. It streams no bytes and is still a delivered read; ADR-012's redirect arm is the rule. ItsRedirectURL()failure arm cannot fire whileIsRedirect()holds, so the placement there rests on the delivered307rather than on that guard.serveFull,servePartialand the manifestGETarm 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.Copyreturns 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'sio.CopyNalready 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_siteandcolumnbut 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.mdstates 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_countloss, but its bufferedUPDATEreportsok, so it carries a share and needs a differently shaped rule than the two any-increase rules here.docs/dev/alerting.mdnames 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+1figure in the plan (:1673and: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 owndocs(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 runandgolangci-lint run --build-tags=integrationover 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)