feat(oci): refuse a manifest read to a remote gated off as unhealthy (S16 plan: 14/26)

Why

Step 14-1 of the S16 container remote plan, the first of the step's four MRs. Step 11b landed the manifest arm's cache-hit half and left the miss path answering an interim 501. Step 14 replaces that 501 with a real fill; this part lands the refusal surface the fill will render every error through, ahead of the fill itself, so the fill adds no rule of its own about what a 503 carries.

What

The unhealthy-remote gate. A manifest read whose repository row carries last_health_status = unhealthy is refused 503 UNAVAILABLE before any upstream request is built, with the probe-sweep interval as its Retry-After and no cache row written. The gate sits inside the miss branch, after the cache lookup, so a fresh cache hit is served as before and a stale tag is refused — which is what the spec's row for that column asks for. An unknown or healthy verdict still answers the interim 501.

RemoteFetchOutcome.SelfDecided. Status alone cannot separate a 503 the upstream produced from one this service minted: both arrive as 503 UNAVAILABLE with an empty UpstreamRetryAfter, and the mapping mints a 503 from an upstream status as well as from an error, so a nil error sits behind both. The field records which side chose the status. It is unit-tested here and has no production reader until 14-2 fetches; the one self-decided 503 reachable today is the unhealthy refusal, which does carry the sweep interval.

The retry-hint rule, settled here. A status the upstream produced relays its own value bounded by the one-hour clamp; a 503 this service decided offers the probe-sweep interval; a 401, a 403 and a propagated 500 carry none. Whatever number a caller hands it, writeRemoteReadUnavailable floors the seconds at 1, so no minted 503 can answer Retry-After: 0 and send the client straight back at an upstream that just produced nothing. remoteRetryAfter splits into relayedRetryAfter so the clamp is reachable from a value the arm already holds rather than only from a header it does not have.

Composition root. wireOCI and wireOCIWithStore carry the health-check config down to the arm, threaded from wireServices. The boot suite asserts the hop with a deliberately non-round 137-second interval, which is the one case no test inside internal/format/oci can report: there the handler is constructed with whatever schedule the test hands it.

This is also the first production caller of Step 16-1's relay writers.

Plan changes are in a separate MR

Per guardrail 5, step MRs do not edit the plan file. This branch carries none.

Review this against the Step 14 entry in !1831, not main's. main's entry still costs Step 14 as one MR and has no per-MR table, so it gives no Files list, no criteria allocation, and no account of the four-way split this MR is part 1 of. !1831 changes only docs/plans/, this changes nothing there, so the two cannot conflict and can land in either order.

Merge order

This branch is on main and targets it, and the rest of Step 14 stacks in a line behind it: 14-2 targets this branch, 14-3 targets 14-2, and 14-4 will target 14-3. The merge order is the step order — each later part retargets main only once the one below it merges.

Step MR
14-1 this MR
14-2 !1872 (merged)
14-3 !1873 (merged)
14-4 TBD

Two other open MRs touch cmd/artifact-registry/wire.go (!1841 (merged) and !1754 (merged)) — the shared composition root, where this MR's hunk is a single argument. Whichever lands second rebases; no pipeline reports it. Two more did: !1803 (merged) merged as f15ef2439, which is this branch's merge base, and !1798 (merged) as 5a712d372.

Reviewable size

17 files, +1,300 −77, measured three-dot against the merge base after merging main in. Past the 500-line ceiling in development-model.md, so guardrail 17 wants the split written out:

Group Lines
Production Go +261 −46 — remote_relay.go 79/19, remote_errors.go 74/15, remote_manifest.go 51/0, remote_serve.go 32/6, the three wire*.go 19/5, remote_list.go 6/1
Tests +994 −27
Run recipe (.claude/skills/) +45 −4

remote_list.go is the merge's doing rather than the gate's. The shared live list relay landed on main after this branch last saw it, calling writeRemoteReadUnavailable with the arity this MR changes; the merge is clean and the compile is not, so the call site takes the hint as a number. It carries the same Retry-After: 5 it does on main.

Why not split further. The behavior this MR adds is a five-line gate. Four fifths of the diff is its coverage, and every remaining piece is something that gate cannot be reviewed without: the outcome field that separates the two 503s, the clamp extraction that lets the arm bound a value it already holds, and the wiring hunks that carry the sweep interval down to the arm. Cutting anywhere separates the gate from the thing that proves it. This is already part 1 of a four-way split of Step 14 — the step measured 1,883 reviewable lines as one MR — and the split runs along which surface gets its first production caller, not along the fill.

The sweep is the only thing that clears the verdict

Worth a reviewer's attention because this MR is what makes it reachable on the OCI manifest route. The gate reads container_remote_repositories.last_health_status with no staleness bound, so only the probe sweep can move a repository back off unhealthy. buildHealthSweepSources in cmd/artifact-registry/wire_remote.go logs-and-skips a container source that fails to build, and returns nil outright when cacheRedis is nil, so a boot can leave the sweep dead while the gate stays armed — an indefinite 503 on every uncached manifest read for that repository.

This is inherited rather than introduced: the behavior matches the S16 spec and both shipped siblings (remote.Standalone and npm's RemoteTarballHandler gate the same way off the same column). Flagging it rather than fixing it here.

Guardrails with nothing to add

  • Conformance tests. The OCI conformance suite runs against hosted repositories and cannot reach a kind=2 route, so it cannot cover this behavior.
  • e2e scenario catalogs. docs/testing/e2e/oci.md puts virtual and remote repositories out of scope until the capability ships, so no scenario is added or affected.
  • Configuration reference. No schema, loader, or config.example.yaml change — the health-check knob this MR reads is already documented.
  • Bruno / OpenAPI. No operation added, renamed, or removed.

Run recipe

.claude/skills/run-artifact-registry/SKILL.md gains #### Driving the manifest arm's unhealthy refusal: how to force the column the sweep will not set on a box with no Redis, and the four answers to expect.

driver.sh smoke does not assert those rows. Every remote assertion it makes is a blob read, and the blob arm reads no health column, so forcing the column takes nothing smoke checks out of reach — the manifest rows are simply unasserted today. The recipe text as it lands here gives a different reason, that forcing the column would put the blob table's 501 row out of reach. That reason is wrong, and the correction lands in !1872 (merged) together with the rest of this review's non-blocking notes.

Related to #288

Edited by Radamanthus Batnag

Merge request reports

Loading
Loading