docs(specs): fix stale refs and under-specified S13 upstream behavior

📜 What this does

Amends docs/specs/S13-virtual-remote-foundation.md so the corrections identified by the S13 plan MR (!954 (merged)), the Step 5 review (!1048 (merged)), the Step 9 review (!1153 (merged)), the Step 11 review (!1290 (merged)), the Step 14 review (!1161 (merged)), the Step 15 review (!1279 (merged)), and the S16 container-remote spec review (!797 (merged)) live in the single source of truth rather than only in plan Research Findings and review threads.

Spec text only. No code, no schema, no configuration.

📌 Changes

Stale references

# Change
1 The S05 and S27 dependency rows read Implemented and name their surfaces (internal/cache/counter.Counter, internal/jobsriver.RegisterPeriodic) instead of describing an interface-and-stub arrangement. The paragraph after the table is rewritten to match.
2 The nine credential-encryption citations repoint to #417 (closed).
3 No change needed — the Fetch section already cites security cover 13 for the max_body_bytes override.
6 No change needed — cover 14 already reads "the smaller of the request's effective body cap and decompression_size_cap".

Under-specified behavior

# Change
4 A raw fetch's live follower window closes on the leader's first forwarded byte; a follower arriving after that reads the committed document, like a transformed follower.
5 The outbound-proxy cover claim is scoped to cover 6. Cover 5 runs best-effort against this process's own resolution, and outbound_allowlist and deny_all_except_allowed keep applying under a proxy.
7 The outbound_allowlist entry grammar is documented as a table: four forms, port scoping semantics, IPv4-mapped normalization, and what fails startup.
8 container_remote_repositories.auth_status joins the behavioral-columns table as container-only, and is named alongside auth_url in the clear-on-url-change rule and its criterion.
9 A new subsection states that a RequestBuilder implementation reads mutable credentials at call time inside BuildRequest, never captures them at construction.
10 The 304 probe mapping is qualified: conditional 304 to ProbeFresh, unsolicited 304 to ProbeHit.
11 The read-time size caps are recorded as a per-flow split rather than a blanket exclusion — see the correction below.
12 health_check.scheduled_interval carries its (0, 1h) bound in the Configuration section.

From the issue's comment thread

Source Change
Health-probe taxonomy The healthy row reads any HTTP response below 500 instead of enumerating 2xx/3xx plus 401/403/404, with why the set is open.
last_health_checked_at Recorded on conclusive probes only. Adds the compare-and-set half: an overtaken probe applies nothing and audits nothing.
last_health_status The row carries CHECK (last_health_status IN (0, 1, 2)) as the shape a new remote slice copies.

Eleven acceptance criteria (eight new, three amended) and eleven Resolutions entries (nine new, two amended) were touched alongside these, counting the review round below.

⚠️ Two corrections to the issue's own text

Both were found while verifying the items against the implementation. The issue is left as written; these are the corrections.

Item 11 records a split, not an exclusion

The issue asks for the read-time cap class to be named a deliberate exclusion from the fallback family. The implementation does not support that, because there are two predicates:

  • remote.FallbackEligible (internal/remote/errors.go) — admits both cap sentinels. This is what virtual resolution (internal/virtual/resolve.go) classifies through, and its own doc lists them under "Eligible".
  • fallbackEligible (internal/remote/standalone.go) — the narrower predicate remote.Standalone pins, which excludes them.

So the spec as written is correct for every caller except the standalone flow. The amendment records the split and the reasoning on both sides. A blanket exclusion would have made the spec wrong for the resolver and for every slice route that drives the flow itself.

The split is per route, not per slice — see the review section below. npm's packument and dist-tags routes compose remote.Standalone and propagate; its tarball route, and the maven and container routes, classify through the exported predicate and serve the cached copy.

The issue's supporting claim that a cap tripping inside the fill surfaces as ErrCacheFill is also not what the code does — the fill's discard site marks the body-read failure with the transport marker and the sentinel travels unwrapped — so that clause is not carried into the spec.

Item 2 retargets to issue 417, not issue 262

The issue asks to repoint issue 68 (https://gitlab.com/gitlab-org/ops/artifact-registry/-/issues/68) to issue 262 (https://gitlab.com/gitlab-org/ops/artifact-registry/-/issues/262). Issue 262 is also closed, and it is the S04-A plan tracker, which explicitly defers the credential-column work. The live tracker is #417 (closed), whose own description names this correction and counts exactly the citations this MR changes: the 7 pointing at issue 68 in S13, plus the 2 pointing at issue 262 already in S13's Follow-ups. All nine now point at issue 417.

The prose around three of them was stale beyond the number and is rewritten: the encryption framework has landed (internal/crypto.RowEncryptor), so what remains deferred is the per-format column wiring, not the framework's design.

🧪 Validation

  • Every item verified against the implementation before writing. Each spec claim traces to a named symbol, constraint, or migration.
  • All internal anchors resolve, including the four new headings.
  • markdownlint-cli2, vale (0 errors), gitlint, gitleaks, and editorconfig pass.
  • Guardrail 13 does not apply: no internal/config/**, proto/**, or config.example.yaml change. docs/dev/configuration-reference.md already documents the allowlist grammar and the (0, 1h) bound as-is; this MR adds the normative half the reference cannot carry by charter.
  • Guardrails 11 and 12 do not apply: no protocol behavior or code changes, so no conformance run and no e2e scenario is affected.
  • No overlap with the two S16 doc MRs in flight (!1831 (merged), !1939 (merged)), which touch only docs/specs/S16-container-remote.md and docs/plans/2026-07-30-container-remote.md.

🔍 From review

Changes landed on this branch in response to review, after the sections above were written. Each was verified against the implementation before editing, same bar as the original items.

Source Change
Duo The two probe acceptance criteria (On-demand probe, Scheduled probe) name the conclusive verdict as the condition on the durable write, and cross-reference the criteria owning the degraded and overtaken cases. They previously asserted the write unconditionally, contradicting the two criteria this MR adds.
Duo The health_check.scheduled_interval upper bound is sourced rather than asserted: where the one-hour horizon comes from (the jobs client overrides neither the client-level timeout nor the rescue interval), and a pointer to the validation error whose doc comment carries the same reasoning. The claim that a live sweep is rescued mid-run and re-fired is softened — the bound exists so the worker's own budget ends the sweep first, which is what keeps it independent of the rescuer.
Review The degraded-probe claim is scoped to the failing arm, in the prose, the criterion, and the Resolutions entry. HealthMonitor.advance degrades only when Increment errors; a passing probe stays conclusive and writes both columns even when Reset fails, which the suite already pins. The passing arm is now stated, along with the stale streak a failed reset actually leaves behind.
Review The RequestBuilder rule's url half becomes checkable: a companion criterion A long-lived builder sends the current url, written to admit maven's rebuild-on-URL-change as well as a per-call read. The "costs nothing" clause is scoped to allocation, since npm pays a datastore round trip per upstream request. A paragraph records that the url half is met today by npm alone and that the container builder does not satisfy the rule as written, pointing at #596.
Review The read-time cap split is stated per route rather than per slice, in the prose, the criterion, the Error Cases rows, and the Resolutions entry. npm straddles it: packument and dist-tags propagate, tarball falls back.
Review The Clear on URL change row takes the health reset — all three formats null last_health_checked_at and set last_health_status to 0 in the same UPDATE as the url write — with the criterion extended to match. The reset does not take the credential arm's exemption: supplying new credentials spares the credential columns and not the health columns.
Review The Clear on URL change row names where credentials_cleared rides — the update response's settings, absent from every other response, never stored — which is the item S17's follow-up entry opens on. The Read redaction row above it is reworded to match: "only a presence flag" had read as excluding that second flag, so the two rows disagreed until one of them named the update response.
Review The stale-streak sentence loses its earliness multiple. "One probe early" holds only at a threshold of 2 and the default is 3; the counter caps nothing on increment, so the offset moves with both the threshold and how far the streak had run. The trigger is the checkable half and is what the monitor's own doc comment states.
Review The claim that the health probe "sends a bare HEAD with no credentials" is corrected in both places it appeared. Maven attaches Basic auth unless both halves are empty, npm attaches the bearer token when one is stored, and only the container probe is bare. What the taxonomy buys is that no probe must acquire credentials first, which is the narrower claim resolution.probe_timeout's scope needs.

⚠️ A third correction, to the S17 follow-up entry

The S17 entry this MR closes describes the URL-change transaction as resetting last_health_status, last_health_checked_at, and the failure counter. The first two are transactional; the counter is not — it lives in S05 state rather than on the row, so its reset runs post-commit in the management handler and is best-effort. A failed or raced reset leaves the old host's counts standing until their TTL, and the update still returns 200. The spec states that split rather than carrying the entry's wording, because a same-transaction reset is not implementable for a value that is not in the database.

Related to #320 (closed)

Edited by David Fernandez

Merge request reports

Loading
Loading