docs(plans): add the S31 npm virtual implementation plan
🎯 Summary
The implementation plan for S31 npm virtual, decomposed into 19 MR-sized steps off five roots.
S31 is the first of the three virtual formats to clear its spec gate. S13's resolver, S15's remote legs, and S11's hosted reads all exist; nothing composes them. This plan is the composition.
Merging this MR is the approval gate. No step MR opens before it lands.
🧭 How the shape was chosen
Two measurements drove the decomposition, both in Research Findings.
Step sizing comes from diffing all 32 merged S15 npm remote step MRs against their first parents: 89,814 added lines, bucketed 37% generated / 43% test / 20% reviewable source, for an aggregate 2.15:1 test-to-source ratio and a median 499.5 reviewable source per MR. Every step here states Source, Test, and Total against that ratio rather than sizing implementation alone. The three schema steps are exempted on the partition-DDL argument instead, because their own ratios are 1.19 to 1.37, not 2.15.
Step shape comes from S15's own post-mortem. That plan cut its three proxy handlers as three steps and left the per-repository flight holder, the composition-root threading, and the shared error codes and metric labels unowned, so three parallel stacks each built all of them, and git merge-tree reported 12 to 15 conflicting files per stack pair. S31 has the same shape, three routes over one engine, so every shared artifact is a numbered step ahead of the routes and Step 14 wires all three dispatch slots to the interim 501 through separate per-route builders before any handler exists.
Separate builders are necessary and not sufficient, which the review made concrete. Step 14 is also the single writer of the npmVirtualWiring struct, of every slot-builder signature, and of the run recipe's hard-coded smoke-check count, because a sibling that widens any of those puts three edits on adjacent lines. Steps 15, 16, and 17 are true siblings on that basis.
🩹 What the review changed
The review found four blocking and twenty-three warning items, all of them clustered at the seam between the merged internal/virtual foundation and the slice that first composes it. The DAG, the five roots, and the 19 steps are unchanged; what changed is which step owns which seam, and which of the spec's claims the plan repeats rather than checks.
Four collaborators that no step owned.
| Gap | Where it now lands |
|---|---|
The resolver reads upstream health once per remote position, serially, through a seam fixed at NewResolver, so the list read's last_health_status column cannot pay for it and a 20-upstream read costs 20 round trips |
Step 11 gains a request-scoped memoizing remote.HealthStatusReader; criteria #15 and #16 (closed) move there from Step 4, because a store-level statement count passes while the composed read fails |
virtual.ResolveResult exposes no per-position detail, so positions[], probes_total, and positions_consulted have no producer |
An S13 observation amendment, recorded as the plan's one step-external gate, pacing Step 18 only. Step 18 specifies the reduced form it ships without one |
No production remote.UpstreamExclusionAuditSink exists and NewResolver rejects a nil one, so Step 14 could not have built the resolver. The only implementation is a fake in a package with no build tag |
Step 14 lands the production sink and asserts the wired type is not the fake |
NewRemoteUpstream panics without a Fetcher (which must be the shared per-repository SingleFlight) and a Cache, and neither came from Steps 4 to 7 |
Step 11 names both as injected seams and acquires each flight lazily, so a denied position neither takes the registry's process-wide lock nor materializes its upstream's plaintext credential. Step 14 supplies the values |
Nine claims that did not survive checking, each now corrected at the point it arises rather than silently fixed:
npm_packageskeys onnpm_repositories, notrepositories, so Step 4 gains a third join without which Step 5 has nothing to batch against.- The hosted authoritative tables carry
soft_deleted_atand the spec's lookup paragraph omits the predicate. Without it an unpublished package is served through a virtual route to a wider audience than the hosted repository's own. - The legacy
GET /{package}/{tag}route rests on a premise S15 never landed. No pattern inbuildMuxmatches it and the S15 plan holds the same question open, so no step here registers it and the criterion is deferred to a ruling. remote_document_serve.goholds ten functions, not five. The reusable half is five, and three of those are the credential-redaction and error-envelope covers the virtual arm needs for the same reason the remote arm does.- Step 8's seam returns a reader, a validator, and a delivery decision, not bytes: the code it extracts has a
307arm, streams rather than buffers, and sheds load with two503s that Step 12 now maps. - Step 12 maps fourteen sentinels, not twelve.
ErrSingleFlightTimeoutandErrHeaderInjectionreach a caller unchanged and want different answers. - The
outcomemetric label collides with an already pinned 13-value closed set and becomesresolution_outcome, which is what the spec's own wide-event field is called. ALTER TABLE ... ADD CONSTRAINTadmits noIF NOT EXISTS, so the one statement Step 2 makes mandatory is the one that breaks the file's own re-runnability rule. ADROP CONSTRAINT IF EXISTSprecedes it.- The
/db-review-prepexemption the plan leaned on belongs to a different anomaly, and ADR-004's 20-upstream cap is enforced by no code, so Step 4 carries a defensiveLIMITand the evidence is expected to raise the fan-out anomalies rather than come back clean.
Step types are now 17 feat, 1 refactor, 1 test, with the rule stated in Approach. The previous 13 chore split would have ridden a new partitioned table set on a maintenance-only version bump, exempted those MRs from the related-issue guardrail, and labelled them type::maintenance, which is a SOX control. Every S15 step MR carrying that plan's marker is a feat and none is a chore, and docs/dev/conventions.md's worked example is feat(maven): maven remote repositories schema.
Smaller corrections: the identifier abbreviation scheme applies to all three tables rather than only Step 2's; the squawk ignore set is per step, and Step 2's disallowed-unique-constraint is the repo's first use; Schema #4 (closed)'s rules link moves to Step 3, which creates that table; Conditional requests #7 (closed) gains a primary owner in Step 13 rather than only a re-assertion in Step 17; Steps 16 and 17 carry the dropped-subtest diff instruction; Step 9 names the fourth NewResolver call site that would otherwise leave the package uncompilable, and renames its finder to avoid a second unrecorded collision with S17 Phase 6 Step 37; Step 10 names buildMux, reuses the existing 405 writer instead of duplicating it, and records that the arm stays outside the instrument seam so the pinned code budget is untouched.
⚠️ Three rulings, none of which blocks this MR
## Open Questions is no longer "None". Each entry names the step it paces and what ships in the meantime.
- The S13 per-position observation amendment, or a narrowed
positions[]. Paces Step 18 only; Step 18 specifies both forms. Owner: the S13 DRI. - Whether the legacy
GET /{package}/{tag}route lands, and for which kinds. Blocks nothing. Registering a shared-mux route from one format's virtual slice is how S15 produced three conflicting stacks, so the recommendation is that it lands in S15's plan for all three kinds at once or the sentence is dropped from both specs. Owner: the S15 spec author. - Whether S17 Phase 6 keeps its npm delete guard and its association route in one MR. The spec's stated merge order for the guard cannot be satisfied: S17 Phase 6 Step 39 carries the guard and is gated on the S30 Maven virtual schema, which does not exist. This plan ships the slice, and the confirmation sought is the narrow invariant that Step 41 must not be split so that the association route merges before the guard, since Step 41 sits transitively behind Step 39 and the whole safety argument rests on the two staying together. Recorded in #314. Owner: the S17 Phase 6 DRI.
The metric-label rename, the unreachable skipped_soft_deleted outcome, the two hosted inline-build 503s missing from the Error Cases table, and the ADR-007 ON DELETE correction are not open questions. Each has one correct answer, the plan takes it, and the spec and handbook amendments ride behind the work rather than gating it.
📏 Size
3,482 lines, all documentation, in two files: the plan (3,481) and one docs/plans/README.md index row. Splitting it would break the plan-MR gate, which requires one plan to land in one MR before its steps open. The length is Research Findings, which carries the two measurements above, the seam inventory for internal/virtual (this plan writes its first importer), the schema conventions for the first *_virtual_* table set in the repository, which S30 and S32 will copy, and the corrections listed above.
✅ Review
Two /review-branch passes in group mode across all thirteen phases. The first is addressed in 49090722. The second, against 48c1ecfd, returned four blocking and seven warning items and is addressed in ffac0565 and 31126c67; all of them sit at the seam between the merged internal/virtual foundation and the slice that first composes it, and the DAG, the five roots, and the 19 steps are unchanged by both.
What the second pass changed:
| Finding | Correction |
|---|---|
No step owned virtual.Request.Fetch. Its zero BodyKind is BodyKindMetadata, so every virtual tarball would have run under the metadata cap; and because the virtual leg fetches through the shared per-repository SingleFlight, whose tee writes the npm_remote_metadata_files rows the standalone remote route serves from, the fetch-time transform's base decides whether one cache row stays correct for both routes |
Step 12 owns the options; Step 11's per-position fetcher wrapper builds the fill-time rewriter on the winning position's own remote base, because Request.Fetch is request-global while the base is per position, and the virtual re-point is Step 13's, at serve time. Step 15 gains a multi-remote cache-parity composition test, Step 17 a large-tarball case |
Step 4's defensive LIMIT was sized off ADR-004's 20-upstream cap on both reads. The rules read's row count is rules across associations, which ADR-004 and the spec both decline to cap, so no such bound "never truncates a legal list"; truncating it drops deny rules, the fail-open internal/virtual/rules.go calls one that "emits no log and no audit event" |
The rules read fails closed on its own dimension, and the test asserts the error rather than asserting that 20 upstreams fit, which passes at a bound of 21 |
Step 12's taxonomy stopped at fourteen values by the same reasoning it used to reject twelve. remote.FallbackEligible excludes seven sentinels and fetchWinner returns through default: whatever it excludes |
The arms are derived from that predicate: nineteen values, adding ErrLeaderCanceled, ErrTransformRejected, and ErrUpstreamDigestMismatch, grouping ErrNotImplemented with the wiring defects, naming the synchronous ErrFillAbandoned arm the derived table cannot reach, and a test that fails when the predicate grows |
positions[] was said to be unbuildable from the foundation, gating Step 18 on an unstarted S13 amendment. RemoteUpstreamParams takes Lookup, Prober, Fetcher, and Cache from this slice |
Step 18 observes them directly. The plan now has no step-external gate, and probes_total ships with the routes, which matters because the spec accepts the probe-amplification risk conditionally on that counter existing |
Seven further corrections: npm_tags carries no soft_deleted_at, so Step 5's dist-tags predicate goes through the package join and its Miss test seeds a soft-deleted package row; Step 5's hosted packument Lookup becomes an EXISTS probe rather than a join that fans out over ADR-004's 25,000 versions, and Steps 5 and 6 state the bound they inherit; the lazy-flight credential claim is replaced by the control that works, Step 4 omitting the token column, since Flight never reads it; a kind=2 position with no npm_remote_repositories row is excluded as a data error instead of panicking, and a NULL last_health_status is pinned to unknown rather than aborting the resolution; Step 12 gains a resolution deadline behind a new virtual_repositories.resolution.resolve_timeout; upstream_rejected joins the error-code inventory; and WithForceStream is supplied on both winner arms in Step 13, since the one in singleflight.go covers only the fill's internal read-back.
Smaller: the S15 feat count is 26 with the grep that reproduces it, the two outcome label sets are not disjoint (both carry cancelled, which is why the union is 25), the identifier-prefix list gains bsa, the Resolution cost citation points at the section that holds the quote, the single-package steps are named as the exceptions to the multi-package smell, and each step over the source ceiling is justified rather than split.
Every citation was re-verified against the tree in both passes; all self-anchors and relative links resolve, and markdownlint-cli2, lychee, and vale pass on both files.
A third round, from @mkhalifa3's nine blocking: review notes, is addressed in a4adc696 through e0f69ad3, one commit per note. The DAG, the five roots, and the 19 steps are again unchanged. What moved:
| Finding | Correction |
|---|---|
Step 11 routed a kind=2 position with no npm_remote_repositories row through virtual.NewUnexpectedKindUpstream, which panics on an in-range kind by contract, so the bullet shipped the recovered panic it exists to prevent. Fabricating an out-of-range kind would make the audit stream record unexpected_kind for a genuinely remote row |
virtual.NewMissingRemoteBindingUpstream, the parallel remote.ExclusionRemoteBindingMissing, and the ReasonRemoteBindingMissing classification and loadAndFilter routing that emit it, landed inside Step 11 as the plan's one accepted foundation touch and additive, so nothing is gated. A follow-up note caught that the first two alone could not emit the reason, since EligibleUpstreams keys on the unexpected-kind mark and loadAndFilter hardcodes ExclusionUnexpectedKind for it. Step 11 becomes multi-package and its source moves to ~575, which puts a second step over the ceiling |
Step 15 said the Accept variant is "chosen once and carried through Lookup, Probe, and Fetch", but no step named the mechanism, and the foundation is kind-keyed. A kind-agnostic implementation of Steps 6, 8, 12, and 13 passed every named test while cross-serving full and abbreviated documents with the wrong Content-Type |
Step 12's facade owns the kind-encoded npmremote.Target whose Path() is the resolver's Request.Path; Step 6 filters on its MetadataKind, Step 8's seam takes it as the document kind, Step 13 asserts the served Content-Type per kind, and Steps 6, 8, and 15 gain cross-variant negatives |
Step 11's rewriter base named one of its three inputs, leaving no delivery path for npm.public_registry_url or the namespace slug. A wrong base embeds wrong dist.tarball values into a shared, persistent cache row |
The base is spelled out as packumentBaseURL(publicRegistryURL, <request namespace slug>, repositories.name) with an owner per input, and Step 12's facade takes the namespace resolution rather than a bare id |
The 40s resolve_timeout default and three claims derived from it were pinned in prose while the spec amendment is allowed to overturn them |
The sizing paragraph marks the value and its four dependent surfaces as amendment-contingent, following Step 10's re-derive convention |
Three inventories disagreed with the steps they describe: the projection count (five columns in Research, six in Step 4), Step 13's serve-time clause crediting Step 12 with the fill-time transform, and the accepted-smell span paragraph omitting internal/format/npm for Steps 15 to 17 and cmd/artifact-registry for Step 18 |
Each corrected at the point it arises |
The eighth note asked for an author ruling on whether the deadline test's Retry-After: 1 follower assertion is deterministically reachable. It is not: a metadata fetch carries the rewriter Transform, so liveEligible is false and every follower parks on the leader's terminal, and whichever of two 40s timers fires first decides between the pinned 1 and remote.RetryAfterSeconds. The ruling is to pin the assertion to the leader's facade, where the deadline is the only canceler, and Step 12 now records why forcing the follower ordering would still miss the arm: SingleFlight.waitError yields a bare wrapped context.DeadlineExceeded with no server-initiated marker, so that route needs two mechanisms rather than one.
Note that GitLabDuo's earlier "nothing to comment on" note reviewed the first commit only: it cross-checks a "13 chore/5 feat/1 test" split that 49090722 replaced.
Related to #290