## Problem Round 5 of the [S32 spec review](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/972) found eight things after the MR was already approval-clear. Six of them are wording in `docs/specs/S32-container-virtual.md` that is wrong or imprecise as merged. They were deferred rather than pushed, because pushing resets the approval and none of the six changes what the spec asks anyone to build. Each is a self-contained edit. The replacement text is written out below so the MR applying them needs no rediscovery, and so a reviewer can check the edit without reopening the review threads. The two findings that are **not** in this issue: - The `#792` closing-condition claim in `Security Considerations` was true of neither the work item's description nor its assignee when it was written. Both were fixed on [#792](https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/792) instead, which is where the obligation belongs, so the spec sentence needs no change. - A container-family `repositories` row with no `container_repositories` child passes S32's kind and format checks and fails open to a permanent `404`. That one is not S32's alone: it is [#867](https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/867), which asks the question over S30, S31 and S32 together. ## The six edits ### 1. The data-error list's maintenance directive reads as if a fourth cause exists Under `### Container virtual resolution`. The list is introduced with "**Three things make a position a data error**, and the term carries exactly these three", and the paragraph closing it then states a fourth cause as fact. The conditional marker was lost in an earlier revision. The carve-out also exempts "the passages that refer to a data error by name" without excluding the two that state the count, and those two must change with the list. Current: > A fourth cause is added to this list and to [Acceptance Criteria](#acceptance-criteria), which carries one criterion per cause, and not to the passages that refer to a data error by name. Replacement: > Should a fourth cause ever be added, it is added to this list and to [Acceptance Criteria](#acceptance-criteria), which carries one criterion per cause, and to the two passages that state the count — the `AllUpstreamsIneligible` row of [Error Cases](#error-cases) and the data-error bullet of [Resolutions](#resolutions) — but not to the passages that refer to a data error by name without counting the causes. ### 2. `served_from_cache` leaves out the cold manifest pull Under `### Wide events`. The definitional clause covers a cold manifest `GET` — `### Manifest proxy` states that "a manifest `GET` serves nothing until the fill commits, so the stored value is in hand before any response header is committed" — but the four-case enumeration omits it and `streamed` excludes it. An implementer of the wide event has no value to pick for the primary cold path. Three sentences change, not one. The discriminator above the enumeration turns the choice on "whether the read served an upstream body **to the client**", which points a cold manifest pull at `streamed`, so adding the fifth case alone would leave two sentences disagreeing. Current: > `served_from_cache` and `streamed` are the two success values, and which one a read records turns on whether the read served an upstream body **to the client** — not on whether one crossed the wire from the upstream. > `served_from_cache` covers a winner answered from content this service holds by the time the response is built, in four cases: a Phase 1 fresh Hit; a Probe-confirmed `304` on a stale tag Hit; a hosted winner serving its stored manifest or blob, which never streams because its Lookup is authoritative and its content is local; and a cold client `HEAD`, which serves no body at all and answers from the fill its own Fetch just committed. > `streamed` covers a winner whose Phase 3 Fetch transferred a live upstream body that the read then served to the client while filling the cache; only a remote winner can record it. > The cold `HEAD` is why the value says nothing about what the request cost — a cold `HEAD` is as expensive as the `GET` it mirrors, per [Client HEAD requests](#client-head-requests) — and an operator reading cost off this field alone would read it wrong. Replacement: > `served_from_cache` and `streamed` are the two success values, and which one a read records turns on whether the response body was read out of **this service's own store** or passed through from the wire as it arrived — not on whether a body crossed the wire from the upstream at all, which it does on every cold read. > `served_from_cache` covers a winner answered from content this service holds by the time the response is built, in five cases: a Phase 1 fresh Hit; a Probe-confirmed `304` on a stale tag Hit; a hosted winner serving its stored manifest or blob, which never streams because its Lookup is authoritative and its content is local; a cold client `HEAD`, which serves no body at all and answers from the fill its own Fetch just committed; and a cold manifest pull, which serves the stored value rather than the transfer that produced it, because a manifest `GET` serves nothing until its fill commits, per [Manifest proxy](#manifest-proxy). > `streamed` covers a winner whose Phase 3 Fetch transferred a live upstream body that the read passed to the client while filling the cache; only a remote winner can record it, and on the manifest path nothing can, because the fill commits before any header is written. > The two cold cases are why the value says nothing about what the request cost: a cold `HEAD` is as expensive as the `GET` it mirrors, per [Client HEAD requests](#client-head-requests), and a cold manifest pull paid for a full upstream transfer, so an operator reading cost off this field alone would read it wrong. S31 lists both values and defines neither, so there is nothing to inherit and nothing to keep in step with here. ### 3. The delete-guard clause requires a "same release" no author can check Under `## Dependencies`, with a matching sentence under `## Resolutions`. Two problems: - Releases are cut per push from Conventional Commit types ([docs/dev/releases.md](https://gitlab.com/gitlab-org/ops/artifact-registry/-/blob/main/docs/dev/releases.md)), so whether two merge requests share a version turns on whose `feat:` or `fix:` lands between them. A same-release requirement is not something an author can verify at merge time. S32 is the only spec in `docs/specs/` that states one; S31 states a strict before-ordering, and S20-A and S15 say "the same MR". - "Which #314 step carries the container delete guard is unsettled" is wrong. #314's plan gives the container arm to **Step 42**, whose acceptance is "the container slices of Steps 39-40's criteria"; Step 39 carries the store-level guard as "the Maven arm of the per-format check" and Step 40 carries the `409` end to end. The rule that *is* enforceable is stated over the association row rather than over a release, because the row is what stalls the purger and hand-written SQL is the only thing that can produce one in the window. Current: > **Depends on:** [#314](...)'s container steps and a running repository-delete guard land in the **same release** as this spec's schema MR. > Not before it: #314's own dependency graph gates those steps on this slice's schema, so no ordering that puts them first is achievable ([Resolutions](#resolutions)). > Same release is what the exposure requires. `container_virtual_repository_upstreams`'s `NO ACTION` foreign key stalls S20-A's purger from the moment the first association row exists, and in this window that row arrives by direct SQL rather than through an endpoint, so the table's existence is what opens the exposure and a running delete guard is what closes it. > Which #314 step carries the container delete guard is unsettled: the graph names a delete-side guard only for Maven, gated on the Maven slice. Pinning it is the S17 DRI's, and it is the one part of this clause an implementer cannot act on today. Replacement: > **Depends on:** [#314](...)'s container steps land after this spec's schema MR, and **no `container_virtual_repository_upstreams` row is written until the container arm of the repository-delete guard is running**. > Not before it: #314's own dependency graph gates those steps on this slice's schema, so no ordering that puts them first is achievable ([Resolutions](#resolutions)). > The row is what opens the exposure, not the table. `container_virtual_repository_upstreams`'s `NO ACTION` foreign key stalls S20-A's purger from the moment the first association row exists, and in this window that row arrives by direct SQL rather than through an endpoint — so seeding associations by hand before the guard runs is the only way to reach the stall, and the table standing empty cannot. > The container arm of the guard is #314's **Step 42**, whose acceptance is the container slices of its Steps 39 and 40 — Step 39 carries the store-level guard and Step 40 the `409` end to end — so the guard arrives with the association endpoints rather than needing a pin of its own. > The rule is stated over the row rather than over a release because releases are cut per push from Conventional Commit types ([docs/dev/releases.md](../dev/releases.md)): which two merge requests share a version turns on whose `feat:` or `fix:` lands between them, so a same-release requirement is not something an author can check at merge time. And in the `## Resolutions` bullet on the SQL-only window, replacing its last two lines: > What this spec can require is that no association row is written until the container arm of the delete guard is running, which [Dependencies](#dependencies) now states, and the passages describing SQL-only configuration are correct as they stand rather than pending a rewrite. > The graph's delete-guard node names Maven, but #314's plan gives the container arm to its Step 42 by way of that step's acceptance, so the owning step needs no pin from the S17 DRI. > A later revision than that one required the guard in the same **release** as the schema MR and recorded the owning step as unsettled; both are corrected above, the first because a same-release requirement is uncheckable at merge time under per-push releases and the second because the plan had already assigned the arm. ### 4. The `Retry-After` Resolutions heading counts the rows that escape S13 Under `## Resolutions`. The heading says "S13 fixes the value for all but two rows" while its own enumeration has three rows that do not take an S13-fixed number: the relayed status, the digest mismatch, and the data-error veto. The count is what breaks; the body's "Two rows supply their own" is about handler-supplied rows and is accurate. The relayed row is the interesting half. S13 *does* state the relay rule in prose — relay the upstream's own value where it carried one — so what is genuinely unfixed is only what such a row carries when the upstream sent none, and that is [#687](https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/687). So the fix drops the count and attributes the row rather than counting it. Current heading: > - **Every `503` carries `Retry-After`, and S13 fixes the value for all but two rows.** Replacement heading: > - **Every `503` carries `Retry-After`, and S13 fixes the value for every row but the two that supply their own.** And in the same bullet's first paragraph: > a relayed status relays the upstream's own value where it sent one, and what such a row carries where the upstream sent none is what [#687](https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/687) reconciles across S13, S15, S16 and S31. ### 5. The digest Security bullet does not carry its #433 gate Under `## Security Considerations`. The bullet states "nothing failing it is cached" and "composing upstreams cannot substitute content for a digest" with no qualification, while `## Dependencies` discloses that today's Fetch seam commits a by-digest fill with no expected digest to check against. In the interim a mismatched fill is committed and, because digest-addressed content is never revalidated, served to every later pull that addresses it. Every other gated Security bullet in the corpus carries its gate **in the bullet** — S13 and S15 on credential encryption ([#68](https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/68)), and this spec's own #263 and #417 — so the bullet is the place for it. One correction to the finding as filed: the `Error Cases` digest rows carry no #433 gate. Only the acceptance criteria do. The replacement text says so. Append to the bullet: > **Gated on the two Fetch-seam properties tracked in [#433](https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/433)**, in the form [Acceptance Criteria](#acceptance-criteria) carries: until both hold, a by-digest fill commits with no expected digest to check against and a tag fetch produces no verdict a slice can act on, so a mismatched fill is committed and, because digest-addressed content is never revalidated ([ADR-008](../adr/008_content_addressable_storage.md)), served to every later pull that addresses it. A GA review clearing digest integrity clears it on those two properties, not on this bullet alone. Whether the two `Error Cases` digest rows should carry the gate as well is a separate question this issue does not answer. ### 6. The open question's closer names one write surface where it needs two Under `## Open Questions`, second bullet. "Every candidate above sits on that write surface" is false for the candidate the question is most about: the four position candidates sit on the association surface, the tag-targeted deny pattern sits on rule CRUD, which the same bullet says is reachable only by direct SQL. The conclusion holds on the broad reading, so this is precision rather than a scope change. Current: > Every candidate above sits on that write surface, so S32 needs no change whichever of them lands; … Replacement: > Every candidate above sits on an S17 write surface — the association endpoints for the four position candidates, rule CRUD for the deny-pattern candidate — so S32 needs no change whichever of them lands; … ## Proposal One `docs(specs)` MR applying the six edits above. No behavior changes, no acceptance criteria added or removed, and no other spec is touched. It can open as soon as !972 merges. The `MD013` limit in this repository is 800 characters per line, and edits 4 and 5 push their lines past it, so both need a sentence break at the point the replacement text shows. Related to https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/972