## :dart: Summary `internal/virtual`'s `Contributor` seam documents a two-context split that no contributor can actually implement, because the fetch seam beneath it takes one context. A collection position's own work is meant to be bounded by `resolution.position_wait_timeout` while a wait behind a single-flight leader stays on `resolution.single_flight_wait_timeout` alone. Both bounds are real keys, but a contributor has no way to apply them to the two phases separately. Raised in review of https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/2745, which introduced the per-position bound: https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/2745#note_3851078458 ## :mag: The gap `Contributor.Contribute` receives two contexts and its doc says `ctx` carries the position bound while `waitCtx` is detached from it. Underneath, `SingleFlight.Fetch` takes a single `ctx` and derives the follower wait from it (`internal/remote/singleflight.go:256`): ```go waitCtx, cancel := context.WithTimeout(ctx, s.waitTimeout) ``` So a contributor picks one and loses the other: | It passes | Result | | --- | --- | | the position context | The follower wait is clamped to the position bound. At the defaults that is 20s against a 5m wait, and 20s also sits below the 30s `http_client.request_total_timeout`, so a follower is shed before a leader that succeeds can finish | | the detached wait context | `position_wait_timeout` bounds nothing on a remote position | No value on either key closes this. S31 fixes the key's useful range at `(0, request_total_timeout]` — `docs/specs/S31-npm-virtual.md:101-104` states that above `30s` it never fires — and the follower wait it would have to clear is `5m`. Lowering `single_flight_wait_timeout` globally is not available either: it is read by four `NewSingleFlight` sites across Maven, npm and OCI, it bounds a follower's wait for a leader's commit on blobs capped at 5GB, and `ErrSingleFlightTimeout` is not fallback-eligible, so shortening it converts coalesced large-layer reads into `503`s. ## :straight_ruler: Impact :information_source: Latent, not live. Nothing wires `virtual.Collect` today and no `Contributor` exists outside the package's own tests, so no request is served under either choice. The first slice to wire a contributor is the point this becomes reachable, which is why it is recorded rather than deferred silently. While it is open, a coalescing position can exceed `position_wait_timeout` without being shed. `logPositionOutranWaitBound` reports exactly that case, so the condition is observable rather than silent. ## :bulb: Proposed shape Carry the wait budget **per call on `FetchOptions`** rather than per `SingleFlight` instance: ```go // FetchOptions // WaitBudget, when positive, overrides the instance's single-flight wait for // this call alone. WaitBudget time.Duration // joinFollower wait := s.waitTimeout if opts.WaitBudget > 0 { wait = opts.WaitBudget } waitCtx, cancel := context.WithTimeout(ctx, wait) ``` Why this shape over the two alternatives considered: - **Against widening `Fetch`'s signature to take both contexts:** `FetchOptions` already reaches all 10 declaration sites and every call site, so a new field changes only the callers that set it. The signature change would touch five packages and roughly 170 call sites, most of them tests sharing `remotetest.FakeFetcher`. - **Against a context field on `FetchOptions`:** a `context.Context` struct field trips `containedctx`, which is enabled here under `default: all`. A `time.Duration` does not. - **Against a separate `SingleFlight` for collection reads:** ruled out by the usage contract at `internal/remote/singleflight.go:25-33`. A virtual upstream is a remote repository, so the same `(remote_repository_id, path)` is read by both standalone and collection flows; a second instance stops them coalescing and doubles `max_concurrent_fills_per_repo`. Both regressions are visible only as upstream traffic. `joinFollower` currently takes `(ctx, fl, path)` and would need `opts` passed through — a private method, one line. ## :question: What this issue still has to decide The shape above bounds the wait. It does not re-arm the position bound for what follows the wait: `joinFollower` waits for the leader's outcome, may wait again for the leader's commit, and then `readCommitted` opens and streams the blob, all inside `Fetch` where an outside caller cannot bound it separately. A second field covering the post-wait read is one option. Two questions need a real contributor to answer against: 1. What should a collection follower's wait be? Collection documents are metadata-shaped, under the 16MB `body_size_cap_metadata`, not the 5GB blob cap the `5m` default is sized for. 1. Should the post-wait committed read be bounded by the position key, or left to the transport bounds it already sits under? ## :white_check_mark: Acceptance - [ ] A contributor can bound its own upstream work by `resolution.position_wait_timeout` without clamping a single-flight follower wait. - [ ] Coalescing and the per-repository fill cap are unchanged: one `SingleFlight` per remote repository, standalone and virtual alike. - [ ] `Contributor.Contribute`'s doc and the `position_wait_timeout` entry in the configuration reference describe what the seam enforces, with the scoping this issue's residual added removed. - [ ] A test proves a follower is not shed by the position bound while a position's own upstream read still is. Related to https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/1249