docs(specs): amend S31 with the cross-upstream packument merge
What this MR does
S31 specified a winner-only packument: the document a virtual repository served was the
lowest-position upstream's, and cross-upstream merge sat in ## Follow-ups. The product decision in
!2018 (merged)
reverses that for every format, so this amendment makes the merge S31's specified behavior.
The two npm metadata routes now collect every eligible upstream and serve the union. The tarball route is unchanged and stays single-winner, because a tarball is opaque bytes with nothing to union.
S31 is not a closed-beta blocker, so there is no winner-only first: the merge is the first implementation, delivered through the existing S31 step issues.
This is the npm half of that decision. The container half is !2240, already open.
The merge rules
| Field | Rule |
|---|---|
versions |
Union; a key held at several positions takes the whole object from the lowest position. Never merged field by field. |
dist-tags |
Union; lowest position wins each tag name, latest included and never synthesized. |
time |
Union; lowest position wins a key. modified is the maximum across contributors, created the minimum. |
dist.tarball |
Re-pointed at the virtual repository on every emitted entry, whichever position it came from. |
dist.integrity, dist.shasum |
Relayed byte-for-byte from the contributing position. Never recomputed, never reconciled. |
_id / _rev |
The requested package name / dropped, on the full variant. A merged abbreviated packument carries neither, nor any top-level field outside name, dist-tags, and versions, matching S11's projection. |
GET /-/package/{package}/dist-tags merges by the same dist-tags rule, so the two surfaces cannot
report different tags for one package in one upstream state.
Collection semantics mirror S30's: no Probe, every eligible position contributes, a failed or
unhealthy position contributes its cached copy or is omitted, any contributor gives 200 with
merged or merged_partial, all-absent gives 404, all-failed 503, and a data error vetoes the
404. One arm is not best-effort: a hosted position whose Lookup Hit is followed by a refused
fenced rebuild vetoes the whole read with S11's own code, because answering from the remaining
positions would serve a public remote's versions alone for a name the hosted repository
demonstrably holds. Bounded by resolution.probe_concurrency, whose name is a tarball-route artifact the spec
now flags where an operator reads it.
The merge streams: position 0's entries are emitted first and a later position's already-seen
version key is never re-emitted, so versions never buffers. The duplicate is still read, because
the divergence counter compares its digests, so the retained state is a version-key-to-digest-pair
map rather than a key set. dist-tags, time, and
the top-level scalars are buffered and emitted after, because a value from the last position can
still win. No Content-Length, for the reason the re-point already required one to be omitted.
The ETag is derived from the merge inputs (algorithm version byte, then per position
position, upstream_repository_id, and that input's blob SHA-256), not from the served bytes,
which are never buffered. If-None-Match therefore answers 304 after the database reads alone.
A HEAD is not that cheap: it runs the collection and the merge and discards the body, because
an over-cap or unparseable input is only discovered in the parse, and that is what separates a
merged_partial from a merged and, where it takes the last contributor, a 503 from a 200.
That reverses two clauses of the ## Resolutions entry recording the old rule, which now carries an
*Amended:* note: a merged document has no single blob to digest, and a metadata read carries a
validator even when its only contributor was filled by a cold fetch. Both original clauses stand
unchanged on the tarball route.
The correction this carries
S31 claimed twice that the merge removes the packument/tarball integrity mismatch, "because a
merged document names one tarball per version". It does not. A merged document does name one
version object per key, but the tarball read is still a separate single-winner resolution, and the
re-pointed URL is uniform per ADR-009 and carries no position. So bytes can still arrive from a
position other than the one whose dist.integrity the client holds.
The uniform URL is not on its own what leaves the mismatch open, and the spec says so rather than
stopping there: two independent resolutions over one predicate would agree. The passage names the
two mechanisms that actually make them disagree. They read different sources, since a position
contributes the packument its metadata row holds while its tarball answer is a Lookup against
npm_remote_files or npm_files and, on a remote position, a live Probe. And they run at
different times, which is the arm the winner-change acceptance criterion already pins with a
position health-marked unhealthy between the two reads.
What the merge does add is detection:
gitlab_artifact_registry_npm_virtual_integrity_divergence_total counts a version key whose
contributing candidates disagree on dist.integrity or dist.shasum, which is the
dependency-confusion signal. The counter is not free, and the spec says so: it widens the streamed
pass's retained state from a version-key set to a version-key-to-digest-pair map, roughly 135
bytes per key (95 for a sha512- integrity value, 40 for a hex shasum). Both fields are
retained because either can diverge alone: dist.integrity is optional in a packument and the
abbreviated variant an npm install requests is where most virtual metadata reads land, so a map
holding integrity alone would sit at zero on the busiest routes and read as "no divergence"
rather than "not measured".
Two configuration keys, owed to the S13 DRI
The fan-out cover needs resolution.negative_cache_ttl and resolution.position_wait_timeout. Both
land on S13's shared virtual_repositories block rather than on the npm slice, for the reason
resolve_timeout already sits there: each bounds the resolution rather than an npm leg. S32's
merged collections need both; S30 asks for neither, and covers the same fan-out with a rate
limit over the virtual read path instead, so this is S31 proposing an addition to the foundation
rather than restating a need three slices already share. Their values are recorded as owed to the
S13 DRI in ## Dependencies, so the step MR that implements collection is gated on that
ratification rather than guessing them.
The gate carries its ceiling, because the DRI is choosing inside one. A metadata collection runs
inside resolve_timeout, so the wave count times the per-leg bound has to fit under it. At today's
defaults request_total_timeout supplies that on its own: two waves of 30s inside the 90s
budget, whatever position_wait_timeout is set to, so the new key can only tighten the sweep and
never break it. What does move is S13's own sizing text, which derives npm's worst case as 2 * 5s
of bodyless probes and says in the same passage that the figure holds only while Phase 2 is how a
resolution narrows its candidates. These routes run no Phase 2, so npm's worst case becomes
2 * 30s of full-body fetches. 90s still clears it, so nothing has to change today, and the
stale npm line goes to the S13 DRI alongside the two values.
The negative cache itself is specified in full rather than by its TTL alone, because its key carries a client-supplied package name: it is in-process and per pod, bounded by an entry ceiling with least-recently-used eviction. The ceiling is a fixed per-pod value rather than a third key, because what it bounds is a process's heap rather than any resolution's behavior.
Reviewable LOC
1329 changed lines, 1048 of them insertions, measured at 71553bbae against the merge base
e0de46676:
| File | + | - |
|---|---|---|
docs/specs/S31-npm-virtual.md |
1047 | 280 |
docs/specs/README.md |
1 | 1 |
Past the 500 ceiling
docs/dev/development-model.md sets, and splitting would not
help. The change is one reversal propagated through one document: the design section, the
acceptance criteria, the error table, the observability family, the follow-ups, and seven
## Resolutions entries all state the same fact, and a split that landed any subset would leave S31
asserting winner-only in the sections it did not reach. That is the failure mode the amendment
exists to close, and it would be live on main between the parts. The single README.md line is
that same fact once more, outside the spec file.
What the second commit fixes
da66b5f11 closes review findings against b33399e1d, all of them sites the first commit did not
reach or a mechanism it under-stated:
- The integrity-mismatch passage was self-refuting. It concluded that an object from position 1
is fetched from position 0 "whenever position 0 also holds that
name@version", which the merge rule two sentences above it forbids. It now names the two mechanisms that operate. - Three
## Resolutionsentries still described the winner-only world, byte-identical tomain: theETagentry (which stated an implementable HTTP contract opposite to the amended body), the re-point entry, and the configuration entry, whose amendment note still counted one added foundation key where there are now three. - S13's cache-fallback paragraph claimed a
dist.tarballre-point and aserved_from_fallbackoutcome that this amendment made tarball-only. - Two acceptance criteria and two serving lines still said "the winner's" on a route that elects none, one of them contradicting the packument-merge criterion on the same fact.
- A conditional metadata read had no
resolution_outcome. This amendment barred the winner-named values from the metadata routes, andmerged/merged_partialboth assert a contributor; a304now records what the collection settled, andcontributor_countis defined for it. (71553bbaenarrows that bar from five values to four and names the304's one blind spot.) - The divergence map retained one digest while the rule compares two fields, so the counter
would have sat at zero wherever only a
shasumis published. (The abbreviated-variant reasonda66b5f11gave for this is wrong, and71553bbaereplaces it.) - The negative cache had a TTL but no store, bound, or eviction, on a key carrying a client-supplied package name.
What the third commit fixes
71553bbae closes review findings against da66b5f11. One is a behavior reversal and the rest are
claims that did not survive checking:
- The hosted shed-rebuild arm asserted two opposite behaviors.
da66b5f11routed a refused fenced rebuild to amerged_partial, while the acceptance criterion, the## Resolutionsentry, the## Error Casesintro, and the row's own heading kept the winner-only rule that vetoes the read. The veto wins, because its reason is untouched by the merge: a hosted Lookup Hit is authoritative, so answering from the remaining positions serves a public remote's versions alone for a name the internal repository holds, and the shed is seconds long and self-clearing. Fixed at every site, and the## Resolutionsentry keeps its ruling and gains an*Amended:*note for the route change rather than a reversal. Two collateral errors went with it: the row's heading called itself the tarball route's, when a tarball Lookup readsnpm_filesand runs no rebuild so that route cannot reach it at all; andhosted_rebuild_refusedwas barred from the metadata routes, which left the value unreachable on every route. - The DRI gate named no ceiling. It asked the S13 DRI to set
position_wait_timeoutwithout saying what it has to fit inside. The section above now states the constraint and re-derives S13's own npm figure, which this amendment makes stale. - The residual named the wrong bounds. It said the fan-out has "one read-level bound
(
resolution.probe_concurrency) and no per-position bound, which makes the slowest position set every metadata read's latency".probe_concurrencybounds concurrency, not time, and two time bounds do operate:request_total_timeoutper leg andresolve_timeoutper read. The residual is that a slow position is paid for30sbefore it sheds, not that it is paid forever. - "S30 and S32 need the same two" was false of S30, which is approved on
mainwith the same collection and neither key. Corrected forresolve_timeouttoo, which neither sibling references. - The negative cache claimed it "can never turn a
mergedinto a404". True only against an unchanged upstream state, which the same paragraph's own second sentence denies. A publish at a negatively cached position inside the TTL is now stated and pinned by a criterion. HEADskipped the merge, so it would answer200where itsGETanswers503. It runs the merge and discards the body; the304keeps the blind spot and now says so.- The
versionsbullet said the retained state is a key set and a duplicate is "skipped rather than compared", which the divergence counter three paragraphs later contradicts. - Half the "retain both digests" rationale did not operate. It rested on the abbreviated variant
dropping
dist.integrity; S11's projection carries the wholedistobject. The operative reason, thatintegritypostdatesshasum, was already in the same sentence and now carries it alone. - A merged abbreviated packument would have emitted
_id, which neither S11's projection nor npmjs's own abbreviated documents carry.
What this MR does not cover
#266 has ten checklist items. This MR is items 1 to 7, which are all of the spec text. Three remain, and none belongs in a spec MR:
- Item 8, the shared seam. Collection mode and per-position records in
internal/virtualare Go code that does not exist onmaintoday (internal/virtualholdsdecision.go,resolve.go,result.go,rules.go,upstream.goand no collection mode). S30 books the same seam and has no plan onmain, so this needs its own plan and MR. - Item 9, the step issues. #894, #896, #897, #899, #900, and #893 (closed) are tracker edits.
- Item 10, merge order. A statement, not an artifact.
Separately, docs/plans/2026-08-21-s31-npm-virtual.md
still describes Steps 13, 15, and 16 winner-shaped and is not in #266's checklist. It needs a
companion docs(plans) MR, which this one does not open.
Merge order and file overlap
git diff --name-only against the merge base gives two files. Re-derived at 71553bbae against all
88 open MRs, every head ref fetched and none unchecked:
docs/specs/S31-npm-virtual.md: no overlap. No open MR touches it, so the spec file is conflict-free.docs/specs/README.md: seven open MRs change that file in their own diff, and none touches the S31 row. Three merge cleanly with this branch: !2339 (merged) (adds a monolithS15row), !2317 (merged) (edits the S17 row), and !728 (edits the S28 row). Three conflict withmainas well as with this branch and so need a rebase whichever way this lands: !1011 (closed), !1009 (closed), and !1007 (closed), each inserting an S34 row. !1028 (closed) is stacked on !1011 (closed) and carries that row rather than adding one, so it is diffed against its parent and rebases with it; a base-relative scan counts it as an eighth and is double-counting the parent.- !2240 is the only one whose conflict is with this branch alone (
git merge-treeexits 1 against this head and 0 againstmain). It edits the S32 row, which is the line immediately after the S31 row, so whichever of the pair lands second rebases one line. There is no semantic dependency in either direction and no required merge order.
The Step 12 stack (!2325 (merged), !2326 (merged), !2255 (merged)) touches only internal/format/npm/* and is deliberately
built against the spec as it stands: #893 (closed)'s description records the four parts of that step this
amendment re-opens. No merge order is required between this MR and that stack, in either
direction. Every step issue that implements the merge sits downstream of Step 12 anyway, so the
implementation cannot reach main before Step 12 does.
ADRs
Checked docs/adr/ for a governing decision. No ADR states winner-only semantics for virtual
repositories, so no handbook ADR amendment is owed. ADR-009's uniform-URL rule is load-bearing for
this change rather than in conflict with it: it is why the re-pointed tarball URL carries no
position, which is why the merge cannot bind the tarball read to the contributing position.
Related to #266