feat(oci): container probe and the probe-deadline split (S32 plan: 9/20)

The container Probe, and the deadline split it needs.

oci.VirtualProber issues its own HEAD rather than wrapping remote.Prober. The 401 rule forces that: the spec asks for opposite answers to a 401 on a cached token and a 401 on a freshly minted one, and remote.ProbeResult carries no token-cache verdict a wrapper could read. So this Probe builds through BuildRequestWithAuth, sends the HEAD through the shared remote.UpstreamDoer, maps the statuses itself, and reuses RetryUnauthorized for the one re-exchange.

Duplicating the status mapping is what that costs. internal/remote/remotetest/probe_outcome.go is the mitigation: one shared conformance assertion both implementations run, 401 excluded by name because they disagree on it by design. It also pins two properties virtual_probe.go inherits from nothing, since it reimplements the call — that the response body is closed, and that the HEAD goes out under the BodyKindMetadata cap.

resolution.probe_timeout moves off probeOne and onto remote.ProbeOptions, so each implementation applies it at its own HEAD leg. That is the scope S13 already fixes for the knob. A failed token handshake becomes a position failure carrying remote.ErrCredentialsUnreadable rather than a resolution-terminating error, so one upstream with a dead token endpoint no longer answers 500 for every layer of every pull through the repository.

The split also changes the per-probe worst case, and the figure the defaults were sized against was too low. A container probe pays two credential acquisitions and two HEADs, not one of each: 2 * probe_timeout + 2 * token_exchange_timeout, 30s a probe and 60s a sweep at the defaults. UpstreamAuthenticator.Authorize bounds a whole acquisition, and acquire's unknown-verdict arm spends a discovery HEAD inside that bound and can still report the token cache's own hit, which is what earns the retry — so the first acquisition is free only on a bare cache hit. Four sites stated the one-acquisition figure and move together: the resolve_timeout default's comment, the test pinning it, configuration-reference.md's row, and S13's post-split derivation. The reference row's exchange-knob boundary moves with them, from above 35s to above 17.5s, because the sweep now scales fourfold with that knob.

A 401 the re-exchange cannot clear now books unauthorized on oci_remote_upstream_responses_total. It previously booked nothing, so total credential rot — the shape that outcome value exists to show — read as a flat probe series.

This lands ahead of its caller. Nothing composes oci.VirtualProber until Step 14, and virtual.NewResolver has no production caller either, so no route reaches this code. A local instance seeded with a kind=virtual npm repository answered 501 on all three virtual read routes while the kind=remote control on the same upstream served the real packument. Do not read this as repairing live behavior. remote.HealthMonitor.Probe is a different type and method and internal/remote/health.go is untouched, so the health sweep and both connection-test endpoints are unaffected; both connection-test arms were driven and hold.

This overturns a ruling the merged plan states

The plan on main carries a subsection titled ### The worst-case probe is 20s, not 30s, and merged S13 says 30s, which rules "No probe can pay both" and derives 20s a probe and 40s a sweep. Its premise is that RemoteTokenHit is returned only by a cache hit, so a probe reporting it had spent no wire leg. UpstreamAuthenticator.Authorize bounds a whole acquisition, and acquire's UpstreamAuthUnknown arm spends a discovery HEAD inside that bound and still reports the cache's hit, which is what earns the retry.

S13's own resolve_timeout paragraph is re-derived here rather than having its figures patched: four sentences around them gated the arrangement on a split this branch performs, and the superseded per-probe and per-sweep figures are dropped instead of restated. Merged S13 and main's own TestVirtualRepositories_ResolveTimeoutClearsTheContainerProbeSweep both already said 60s. This branch had lowered that test to match the plan; that is reverted. !2708 (merged) retires the subsection, its heading, and the five passages citing it. Guardrail 4 bars correcting the plan from here, which is why the merge order below names it.

The credential log line's text changes

virtual: upstream position's credential refused; failed without probing it becomes virtual: upstream position's credential could not be acquired; position failed, because the third credential marker made "refused" wrong for two of the three classes. Any saved log query keyed on the old string stops matching. credential_failure_class carries refused, unopenable or unreadable, so group on that attribute rather than on the message text.

Merge order

Two spec gates are now discharged; two orderings remain, and no pipeline checks either.

  • Both spec gates have merged. !2738 (merged) widened S16's declared operation domain to {fetch, probe}, which this branch books; two spec-mirror tests in internal/format/oci pin that domain as a literal, so a green run here never said anything about whether the owner spec agreed. !2724 (merged) moved S13's credential-failure criterion from "never consulted its upstream" to "established no absence", which is what lets a 401 refusal after the HEAD stay positional.
  • !2708 (merged) must not merge after this. It rewrites Step 9's Acceptance and Tests bullets from a nil Go error to the non-nil ErrCredentialsUnreadable marker this code returns, adds three omitted Files paths, and records both merged amendments as Step 9's gates. The plan's own Scope paragraph already agreed with the code.
  • !2708 (merged) also carries the retired worst-case ruling, so it is a content gate and not only an ordering one: merged after this, the plan states 20s and 40s for a code path that produces 30s and 60s.
  • !2745 (merged) conflicts with this branch and not with main. git merge-tree --write-tree origin/main <!2745> exits 0; against this branch it exits 1 on nine paths: config.example.yaml, docs/dev/configuration-reference.md, gen/artifactregistry/config/v1/config.pb.go, internal/config/virtualrepositories.go, internal/virtual/resolve.go, internal/virtual/resolve_budget.md, internal/virtual/resolve_errors.md, internal/virtual/resolve_test.go and proto/artifactregistry/config/v1/config.proto. Whichever lands second resolves them, and config.pb.go wants a buf generate rather than a hand merge. resolve.go is the one that needs care: it holds the only production ProbeOptions construction, so the budget hand-down has to be re-derived rather than text-merged. Measured at this branch 3a021f0f4, !2745 (merged) df3a50b3b, main ed16a3f4c.
  • remote.ProbeOptions.HeadTimeout becomes required, and ErrProbeHeadTimeoutUnset is refused before any request goes out. Neither symbol is on main, so main is intact and the collision is prospective: any branch that composes remote.NewProber without a budget breaks at runtime while compiling clean, and git merge-tree returns exit 0 because the two branches need share no file. This branch fixes the one caller that exists, and at ed16a3f4c no open MR adds another.

Size

2,654 reviewable added lines, past the 500 ceiling, and splitting would not help. Derived at 3a021f0f4 with git diff --numstat origin/main...HEAD.

Group Added Removed
Source 758 116
Test 1,653 211
Doc and config 241 74
Generated 2 2

Source concentrates in two files: internal/format/oci/virtual_probe.go (+213) and internal/remote/remotetest/probe_outcome.go (+439). The plan's own argument covers the first — a status mapping with no Probe behind it has nothing to map, and the 401 arms are why the mapping is here at all. The second is a Create in a non-_test.go file, so its lines count as source, and it cannot ship later than the Probe it constrains: "cannot drift" is the entire mitigation for the duplication, and an assertion that arrives after both implementations have diverged mitigates nothing. Test concentrates in internal/format/oci/virtual_probe_test.go (+976), which the plan's Tests bullet enumerates case by case.

Commit history

The branch carries one test(oci) commit, the test-first authorship bypass guardrail 10 allows. It previously carried two; they are folded, because the guardrail permits exactly one and the second met every other clause of the carve-out but the count. The fold leaves the final tree byte-identical.

Two residuals recorded rather than fixed

Neither is this step's to close, and both are written down where the code is read.

  • A durably refusing upstream reports healthy. S13 counts 401 and 403 as reachable, so an upstream whose credential is revoked keeps its position and contributes nothing while a sibling's 404 answers the client. The not-found veto is correct as it stands — the data-error mark is for a failure on this service's side of the wire, never an upstream's answer, and remote.Prober maps the same statuses the same way for Maven and npm. The health question is on #291 (closed), and internal/virtual/resolve_errors.md points there from the residual paragraph.
  • The fan-out's post-winner cancel reaches the token handshake. probeOne hands the prober the errgroup context, which runPhase2 cancels at every determined winner, so a losing probe that led a cold exchange can fail a concurrent kind=2 pull sharing the token-cache key. S32's Probe section forbids the obvious fix by name — "detaching the Probe's context inside the Prober would also detach it from the fan-out's post-winner cancellation", and "Whatever construction is chosen has to keep that cancellation intact" — so the Prober is not where this closes. Re-electing a follower belongs to remote_tokencache.go. internal/virtual/resolve_budget.md carries the window and names Step 14, which wires the route that first puts the fan-out beside live proxy traffic.

Testing

Both suites call the shared assertion, and the Prober suite gains the 401 case it never had. Diffed against the mirrored internal/remote Prober suite for dropped subtests: all six status rows, the four transport rows, the cancellation case, the body-close case and the metadata-cap case survive the move into remotetest, and the deleted "probe deadline" row is now the own-budget case of assertProbeWhoseContextEnded.

No docs/testing/ scenario is added or affected: both container catalog sections are owned by Step 18, and no route composes this Probe until Step 14. The OCI distribution-spec conformance suite exercises a hosted repository, so it cannot reach the kind=1 arm at all.

One comment edit was declined. internal/remote/remotetest/fakes.go's package doc enumerates two seam-conformance families and this branch adds a third, but the block predates the caps at 12 lines against a cap of 3, so naming the third family would force deleting nine lines of seam documentation. The assertion carries its own doc instead.

Related to #291 (closed)

Edited by Radamanthus Batnag

Merge request reports

Loading
Loading