## Problem `remote.RetryAfterSeconds` derives the value a `503` for an unavailable upstream carries in its `Retry-After` header: `health_check.scheduled_interval` rounded up to the next whole second, never below `1`, so `300` under the default `5m`. It lives in `internal/remote/retryafter.go` and merged in !1678, as item 3 of #609. When this issue was filed nothing called it, so the first proxy path to render one of the covered `503`s would pick its own number. The npm remote read routes have since taken it. No other path has: each one either has no covered `503` to render yet, or renders one from a value of its own. Two such values sit in `internal/format/oci` and `internal/format/maven` today. ## Proposal Every `503` a proxy path answers for an unavailable upstream takes its value from `remote.RetryAfterSeconds`, applied to the configured `health_check.scheduled_interval`. The rule covers the verdicts behind `remote.ErrUpstreamUnavailable`, `remote.ErrAllUpstreamsUnhealthy` and `remote.ErrRemoteUnhealthy`, per S13's [`Retry-After` on an unavailable upstream](https://gitlab.com/gitlab-org/ops/artifact-registry/-/blob/main/docs/specs/S13-virtual-remote-foundation.md). The derivation is done. What each path still needs is the configured interval reachable at the point it writes the header, and no second helper deriving it again. That wiring is what this issue tracks. ## Blocked by This issue closes only when four other slices ship, so it is a tracking umbrella rather than a unit of work. The linked blockers, and which item each one gates: | Item | Blocked by | Why | | --- | --- | --- | | 2. Container remote fill | #288 | The fill that renders the first covered `503` is an S16 step, not a change this issue can make on its own. | | 4. npm virtual mapping | #290 | No npm virtual slice exists: `internal/format/npm/handler.go` answers `kind=1` with a blanket `notImplemented`, so there is no mapping to give a value to. | | 5. Container virtual | #291 | Blocked in turn by #239. `docs/specs/` holds no `S32-*.md`, and `docs/specs/README.md` lists S32 as `Planned`. | | 5. Maven virtual | #289 | Blocked in turn by #284. S30 has no spec file at all, and `docs/specs/README.md` lists it as `Planned`. The furthest-out item here. | Items 1, 3 and 6 are not blocked and can proceed in any order. Two of them contend with open merge requests on the files they touch, which is a rebase to sequence rather than a blocker: item 1 with #820 and with the Maven remote cache-fill accounting work, and item 3 with the open S16 wave on `internal/format/oci/remote_relay.go` and `internal/format/oci/remote_list.go`. Item 6's spec file also carries an open edit, at a hunk far from the `## Follow-ups` entry. ## Already landed Measured at `f50fb5ed`. Symbols rather than line numbers, so the claims stay checkable by `grep`. 1. **npm remote (S15).** All three read routes take the value: `internal/format/npm/remote_packument.go`, `remote_disttags.go` and `remote_tarball.go` each build their `remoteReadPolicy` with `remote.RetryAfterSeconds(deps.HealthCheckInterval)`, and `writeRemoteProxyEnvelope` in `internal/format/npm/remote_read_errors.go` stamps the header on a `503` only. The interval reaches them from `cmd/artifact-registry/wire.go` through `wire_npm_remote.go`. Asserted in the construction tests (`internal/format/npm/remote_packument_internal_test.go`, `remote_disttags_internal_test.go`), the route tests, and the composition-root boot test `cmd/artifact-registry/wire_npm_remote_boot_integration_test.go`. 1. **The npm interim value is gone.** `remoteProxyRetryAfter = 5 * time.Second` is no longer anywhere in the tree; the renderer takes the seconds as a parameter instead. 1. **First correction to the S13 follow-up entry**, in !1708. Item 6 below is the second pass it now needs. ## What still needs to be done ### 1. Maven remote (S14): collapse the duplicate helper `upstreamRetryAfterSeconds` in `internal/format/maven/remote_artifact.go` re-derives what `remote.RetryAfterSeconds` already derives (ceiling to the whole second, floor of `1`), with a different formula and the same result. Maven's behavior is therefore already correct, which makes this a de-duplication rather than a fix, and it is the item most likely to be mistaken for done. Call sites: `remote_artifact.go`'s relayed-value fallback and `writeArtifactFillUnavailable`, plus the matching pair in `internal/format/maven/remote_metadata.go`. The interval already reaches all four: `cmd/artifact-registry/wire.go` passes `HealthCheck.ScheduledInterval` through `wire_root_dispatcher.go` and `wire_maven.go` onto `maven.Deps.UpstreamRetryAfter`, held as the handler's `upstreamRetryAfter`. Note that Maven applies its helper at render time on every `503`, where npm computes once at construction; either is fine, but the shapes differ, so this is not a copy of the npm diff. Removing the helper also removes `TestUpstreamRetryAfterSecondsRounding` in `internal/format/maven/remote_artifact_dispatch_test.go`, which duplicates `internal/remote/retryafter_test.go` case for case. **Overlap to settle before opening an MR.** #820 rewrites the same four call sites to bound the relayed upstream value, and it is a security issue, so its MR goes to the security mirror rather than here. Whichever change lands second rebases onto the first, and no pipeline reports the collision. ### 2. Container remote (S16): the fill arm's unavailable `503` No OCI path renders a covered `503` yet. The two wired read arms serve from cache and answer an interim `501` on anything else (`writeRemoteReadFillNotImplemented`, reached from `internal/format/oci/remote_serve.go` and `remote_blob_serve.go`), and `internal/format/oci/remote_stub.go` still answers `501` for the unfilled tag-list and referrers slots. The decision layer is written and has no production caller: `MapFetchOutcome` and `respondUnavailable` in `internal/format/oci/remote_errors.go` deliberately leave `UpstreamRetryAfter` empty for a renderer to supply. Nothing in `internal/format/oci` reads `health_check.scheduled_interval`, so the interval is not yet wired into the package at all. !1894 wires the first container remote blob fill. While it is open, no covered `503` is reachable in OCI; once it merges, that fill's unavailable arm is the first one, so it is the MR that owes both the interval wiring and the header assertion. Blocked by #288. ### 3. Container: split `defaultRemoteRetryAfter` `internal/format/oci/remote_relay.go` holds `defaultRemoteRetryAfter = "5"`. It has two kinds of caller, and only one of them is this issue's: - **In scope.** `writeRemoteReadUnavailable` in the same file, and `writeRemoteListUnavailable` and `writeRemoteListUnusableResponse` in `internal/format/oci/remote_list.go`, write it on a `503` for an upstream that produced nothing to serve. That is the covered verdict, and the value should come from the interval. - **Out of scope.** `remoteRetryAfter` in the same file falls back to it when a *relayed* upstream status carried no `Retry-After`, or one in neither RFC 9110 form. That is the relayed case under **Out of scope** below, which #687 reconciles. So the fix is to split the constant, not to replace it: a shared rename would drag the relayed fallback into a rule that does not cover it. The doc comment's "a floor rather than an estimate" rationale belongs to the relayed half. None of the in-scope writers is reachable from production today: `remoteReadSubject.writeUnavailable` has no caller, and the two list routes are unwired. This is a hardcode to replace before a caller arrives, not a shipped defect, and it is the OCI twin of the npm `remoteProxyRetryAfter` that item 2 of **Already landed** removed. ### 4. npm virtual (S31): the resolution error mapping `internal/format/npm/handler.go` answers `kind=1` with a blanket `notImplemented`, so there is no mapping to give a value to. This item opens when the npm virtual slice does. Blocked by #290. ### 5. Container virtual (S32) and the Maven virtual paths Neither slice exists. `internal/format/maven/handler.go` answers `kind=1` through `writeNotImplemented` in `internal/format/maven/remote_stub.go`, and OCI has no virtual dispatch. Same shape as item 4: the slice's own MR carries the value and the assertion. Blocked by #291 for the container half and #289 for the Maven half. **`remote.ErrAllUpstreamsUnhealthy` has no HTTP mapping anywhere in production.** It is raised in `internal/virtual/resolve.go` and documented in `internal/remote/errors.go`, and no format package matches it. It is one of the three verdicts this rule covers, and it is reachable only from a virtual path, so whichever of items 4 and 5 lands first owes it a mapping and a `Retry-After` assertion, not just the two verdicts a remote path sees. A virtual slice that maps only `ErrUpstreamUnavailable` leaves this item open. ### 6. The S13 `## Follow-ups` entry The entry in `docs/specs/S13-virtual-remote-foundation.md` that names this work is deleted by the last of the items above. Before then it needs a second correction, on top of the one !1708 made, because two of its claims no longer hold: - "no handler calls it" is falsified by the three npm remote routes in item 1 of **Already landed**. - "no path reaches ... the blocked Fetch path on an unhealthy remote" is falsified by npm's tarball health gate in `internal/format/npm/remote_tarball.go`, which feeds the `ErrRemoteUnhealthy` arm of `internal/format/npm/remote_read_errors.go`. Still accurate in that entry, and to be kept: the Maven-local-helper sentence, "No virtual-resolution path sets the header", and "no path reaches the `AllUpstreamsUnhealthy` verdict". That correction is its own docs MR and does not wait for any path above. ## Not a gap A `503` mapped from `remote.ErrLeaderCanceled` or `remote.ErrSingleFlightTimeout` carries `Retry-After: 1`, per S13's `#### Retry-After on a coalescing failure`. No probe has to run before that condition can clear, so the sweep interval does not apply. npm's `remoteCoalescingRetryAfterSeconds` and the literal `"1"` in `internal/format/maven/remote_errors.go` are that rule rather than a violation of this one. #825 tracks the npm remote tarball route answering the sweep interval on that condition instead of `1`. ## Out of scope A `503` that relays a status the upstream itself produced. S13, S15, S16 and S31 disagree on what such a response carries, and `remote.RetryAfterSeconds`'s doc comment records the disagreement rather than settling it. Reconciling the four specs is #687. OCI's `remoteRetryAfter` in `internal/format/oci/remote_relay.go` is that relay path (RFC 9110 grammar check, one-hour clamp), and #820 covers Maven's, which relays unbounded. ## Scope note Each item lands with the slice or fill-handler MR that renders the `503`, not as a separate sweep, and asserts the header value in that MR's own tests. #609 states the same rule. Items 1, 3 and 6 are the exceptions: no new path is involved, so each is its own MR. Related to #609.