feat(managementapi): npm virtual upstream association routes (S17 Phase 6 plan: 40/42)

Why

A virtual repository's only configuration surface is its ordered upstream list, and nothing reaches it. Step 39 landed the junction store and the repository-delete association guard, feat(datastore): npm virtual upstream store and... (!2701 - merged) • Hayley Swimelar • 19.5. This step serves the five routes over them, maps the guard's typed error to the delete's 409, and emits the three mutation audit events.

Until this lands, an npm repository listed as a virtual upstream answers 500 on both destructive values when a junction row is seeded directly. That was measured on the parent branch before the change. This MR closes the window with a static 409 under both values.

Plan step: Step 40 of docs/plans/2026-08-13-s17-phase6-virtual-remote-repositories.md, re-homed onto npm by docs(plans): re-home the phase 6 association su... (!2694 - merged) • Hayley Swimelar • 19.5. Built against c3d86201, whose Step 40 text is byte-identical to main's after that merge.

Merge order

The parent merged as 91ad35c1c in feat(datastore): npm virtual upstream store and... (!2701 - merged) • Hayley Swimelar • 19.5, so this MR targets main directly and nothing gates it.

Depended on Step 39, and the order mattered (Guardrail 20). Step 39 carries the delete-side guard. This step adds the writer routes that create the rows the guard refuses a delete over, plus the handler's 409 mapping. Landing the routes first would have let a caller create a junction row with no delete-side arm to refuse it, which is the 500 window above. The promise recorded on feat(managementapi): serve npm's virtual kind (... (!2665 - merged) • Hayley Swimelar • 19.5 is that per format no writer route lands ahead of its guard arm. Steps 41 and 42 carry arm and routes together.

What

Only the parts the diff does not explain.

The list route reads through a new NpmVirtualUpstreamStore.Upstreams, not the resolution store. NpmVirtualResolutionStore.UpstreamsByVirtualRepository cannot serve it. It drops a soft-deleted upstream, which would serve non-contiguous positions against a contract that promises contiguity and would hide the one association blocking a wedged upstream's delete. It also projects no format, keys on the child surrogate where every mutation keys on the parent repositories.id, and clips the list.

That read carries no LIMIT, a recorded deviation from docs/dev/database-query-patterns.md. No ceiling is derivable: the junction's unique index counts repositories of every format, and checkRepositoryCap filters tombstones, so a bound would have to be picked rather than derived, and a refusing bound would trap the repair path for a list already over the cap. Through every production path the cap check holds the list at 20 under the child-row lock. Reasoning in internal/datastore/npm_virtual_upstreams.md.

The three mutations arm no lock deadline. The child-row lock wait is unbounded, and server.timeouts.write does not bound it, because WriteTimeout arms a socket write deadline and cancels nothing. Arming a request-context deadline waits on the classification #252 owns: logAndWriteInternalError returns without writing on context.DeadlineExceeded, so a fired deadline today yields a bare 200 on a mutation that may or may not have committed. The class is pre-existing, recorded on #902, and accepted for closed beta by S20-A:1001. Reasoning in internal/managementapi/upstream_repositories.md.

Dissociate now returns the row it removed, extending a Step 39 signature. The audit event's vacated position was otherwise read off the pool handle before the transaction, where a concurrent mutation renumbers in between.

Two contract corrections sit outside the plan's file list, both reachable only once these routes exist. All five upstream operations now declare the 503 the authorization middleware already answers. deleteRepository's 409 prose dropped a false claim that the upstream condition applies only under destructive=false. It applies under both, it takes precedence over the non-empty message, and the guard is npm's alone today.

Diff size

694 production Go lines, past the 500 ceiling (Guardrail 18). Split: 529 the five handlers with their resource and error mapping, 120 the datastore read plus the Dissociate return, 28 route registration, 8 the delete 409, and 9 wiring and the query name. Tests are 2267 lines, docs and contract 389. Splitting the handlers from the read they render would ship a route with no data source, and the delete 409 is the other half of AC #110 (closed)'s end-to-end pair.

Deliberately unchanged

internal/virtual/position.go:84 says no production caller reaches the write path that enforces ADR-004's cap. This branch is that caller. feat(virtual): collect every eligible position ... (!2698 - merged) • Dzmitry (Dima) Meshcharakou • 19.5 owns the file and is open, so the line is left for it.

No docs/user/** move (Guardrail 24). docs/user/repositories/_index.md already documents the ordered list, the 20-upstream cap, and the recursion refusal, and docs/user/api.md's resource table is scoped to artifacts, where a repository-configuration sub-resource does not belong.

The unpaginated list deviates from ADR-009's rule that every list endpoint paginates. The spec records that carve-out in its Resolutions and puts it in the Phase 6 ADR-009 amendment batch under Follow-ups, so this MR opens no new escalation.

Rollout

This MR adds the first route that can write an npm_virtual_repository_upstreams row, and maps ErrRepositoryIsVirtualUpstream onto the delete's 409 in the same change. On main the guard is live but unreachable, so the sentinel falls to the logged 500. Two consequences follow, and no code change closes either.

During the rolling deploy, an upstream repository's DELETE answers 409 on a new pod and 500 on an old one. After a binary rollback the junction rows stay, no route lists or removes them, and the 500 is that repository's delete answer until roll-forward. Nothing is lost or corrupted, and rolling forward restores the 409.

Test plan

Integration tests need Postgres with max_locks_per_transaction at 1024 or more. The local default of 64 storms with 53200.

export ARTIFACT_REGISTRY_DATABASE_TEST_DSN="postgres://..."
go test -tags=integration -timeout=30m ./internal/managementapi/ ./internal/datastore/
golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/managementapi/... ./internal/datastore/...
go vet -tags=integration ./...

The end-to-end walk is TestNpmVirtualUpstreamsIntegration_AssociateReorderDissociateWalk. Scenarios are catalogued under ## Upstream associations in docs/testing/e2e/npm.md (Guardrail 12).

.claude/skills/run-artifact-registry/SKILL.md gained a section driving the list by hand (Guardrail 21). driver.sh smoke was re-run against this head on an isolated rig the driver built from it: 74 passed, 0 failed. The run observes kind=virtual on maven is 201, which af9ee273c flipped from the 422 the earlier record on this branch saw, and kind=virtual on npm is 201 beside it. Step 40 adds no check, so the total does not move.

Spec coverage

AC What it requires Covered by
#79 (association slice) Each mutation emits one audit event naming the actor TestNpmUpstreams_{Associate,Update}_EmitsOneAuditEventNamingTheActor, TestNpmUpstreams_Dissociate_EmitsOneAuditEventNamingTheVacatedPosition, TestNpmUpstreams_Mutation_SettledReadFails_Returns500AndKeepsTheAuditEvent, the five-event assertion in the walk
#80 (end-to-end half) An associate and its upstream's repository delete never both succeed TestDeleteHandler_RepositoryIsAVirtualUpstream_Returns409, TestNpmVirtualUpstreamsIntegration_UpstreamDeleteIsRefusedUntilDissociated. The FOR SHARE interlock is Step 39's
#81 (end-to-end half) An associate concurrent with the virtual repository's own delete serializes TestNpmUpstreams_Associate_MapsEveryStoreRefusal/the virtual repository's child row gone. The child-row lock and cascade are Step 39's
#82 (end-to-end half) Validation order: the duplicate 409 beats the cap 422 on a full list TestNpmVirtualUpstreamsIntegration_CapAndDuplicateOrdering, plus TestNpmUpstreams_Associate_MapsEveryStoreRefusal pinning that the handler adds no precedence of its own
#89 (closed) POST associates hosted and remote (201, Location, body), appends on omitted position, inserts and shifts on supplied, and the list is the whole set, contiguous, unpaginated TestNpmUpstreams_Associate_{AppendsWhenPositionIsOmitted,InsertsAtTheRequestedPosition,HostedAndRemoteUpstreams}, TestNpmUpstreams_List_{ReturnsThePositionOrderedSet,EmptyIsAnEmptyArray}, TestNpmUpstreams_Get_ReturnsTheAddressedAssociation, the walk
#90 PATCH moves with the rest contiguous, DELETE closes the gap, an out-of-range position 422 TestNpmUpstreams_Update_MovesAndKeepsTheRestContiguous (six directions), TestNpmUpstreams_Update_RejectsBadRequests, TestNpmUpstreams_Dissociate_Returns204AndClosesTheGap, TestNpmVirtualUpstreamsIntegration_RefusesAnOutOfRangePosition, the walk
#91 Unknown and foreign upstreams share one static 422, a virtual upstream 422 names the recursion rule, an out-of-family upstream 422 TestNpmUpstreams_Associate_{UnknownAndForeignUpstreamShareOneMessage,RefusesSelfReferenceAndTombstonedUpstream,MapsEveryStoreRefusal}, TestNpmVirtualUpstreamsIntegration_RefusesEveryIneligibleUpstream
#92 (closed) Re-associating returns 409, the 21st returns 422 limit_exceeded TestNpmUpstreams_Associate_MapsEveryStoreRefusal, TestNpmVirtualUpstreamsIntegration_CapAndDuplicateOrdering
#93 (closed) Wrong kind 404 on all five routes, and a :format differing from the stored format 404 TestNpmUpstreams_WrongKind_Returns404 (2 kinds x 5 routes), TestNpmUpstreams_FormatSegmentMismatch_Returns404, TestNpmUpstreams_MemberRoutes_UnresolvedID_Returns404, TestContractOperations_AreRegisteredAndServed
#110 (closed) DELETE on a repository that is an upstream returns 409 with a static message and deletes nothing, and succeeds once the association is removed TestDeleteHandler_RepositoryIsAVirtualUpstream_Returns409 (both destructive values), TestDeleteHandler_UpstreamGuard_AndNotEmpty_AnswerDifferentMessages, TestNpmVirtualUpstreamsIntegration_UpstreamDeleteIsRefusedUntilDissociated

AC #80 and #81's end-to-end halves are covered compositionally rather than by a single concurrent-request test: the store halves carry the lock disciplines under feat(datastore): npm virtual upstream store and... (!2701 - merged) • Hayley Swimelar • 19.5, and these assert the HTTP answers over them.

Database Review Evidence

No file under internal/datastore/migrations/sql/ changed, so migration mode did not run.

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17 container (matching GL_PG_CURR_VERSION from .gitlab-ci-other-versions.yml), with synthesized seed data rolled back per query and the container torn down at the end of the run. Numbers reflect moderate cardinality and do not capture production-scale effects. See Database review evidence for seed sizing, methodology, and the anomalies the skill flags. The Index column names the declared index; the plans below name the partition's copy of it, whose number follows the seeded namespace. Expand each row's details for the seed shape, rendered SQL, bound args, and raw plan.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.npmVirtualUpstreamManagementListStmt Sort unique_nvru_ns_id_repository_id_upstream_repository_id, unique_nvr_ns_id_repository_id, pk_repositories 5 / 20 25.84 0.067ms 71 / 0 1/64, 1/64, 1/64

Query notes:

  • datastore.npmVirtualUpstreamManagementListStmt: the management list read carries no LIMIT, which deviates from "Bounding result sets" in docs/dev/database-query-patterns.md. Its predicate is an equality on the virtual repository's own key, so the result is one repository's upstream list rather than a share of the table, and NpmVirtualUpstreamStore holds that list to ADR-004's cap of 20 on every write path. The bound is refused rather than forgotten: no ceiling is derivable from the schema, because the junction's unique index counts repositories of every format and checkRepositoryCap filters tombstones, and a refusing bound would trap the repair path for a list already seeded over the cap outside the store.
datastore.npmVirtualUpstreamManagementListStmt

Summary: Plan matches the method's intent: the virtual repository's child row resolves on the partition's copy of unique_nvr_ns_id_repository_id, its junction rows come from unique_nvru_ns_id_repository_id_upstream_repository_id, and pk_repositories fills each upstream summary, with all three tables pruned to one partition of 64 on the bound namespace literal. The root Sort is real work rather than a missing index: the chosen junction index orders by upstream_repository_id within the virtual repository, not by position, and at the cap of 20 rows the quicksort costs 0.002ms against a 26kB working set, so the planner has no reason yet to prefer unique_nvru_ns_id_repository_id_position, which would supply the order directly. The 5-row estimate against 20 actual rows is a seed artifact, since the seed spreads five upstreams across each of 1000 virtual repositories and tops the read's target up to the cap, and it stays well inside the 10x threshold.

Seed shape: namespaces=1, repositories=5000 (4000 hosted upstream candidates, 1000 virtual), npm_virtual_repositories=1000, npm_virtual_repository_upstreams=5015 (5 per virtual repository, with the read's target topped up to 20)

Rendered SQL:

SELECT npm_virtual_repository_upstreams.id AS "npm_virtual_repository_upstreams.id",
     npm_virtual_repository_upstreams.position AS "npm_virtual_repository_upstreams.position",
     npm_virtual_repository_upstreams.upstream_repository_id AS "npm_virtual_repository_upstreams.upstream_repository_id",
     repositories.name AS "repositories.name",
     repositories.format AS "repositories.format",
     repositories.kind AS "repositories.kind"
FROM public.npm_virtual_repository_upstreams
     INNER JOIN public.npm_virtual_repositories ON ((npm_virtual_repositories.namespace_id = npm_virtual_repository_upstreams.namespace_id) AND (npm_virtual_repositories.id = npm_virtual_repository_upstreams.npm_virtual_repository_id))
     INNER JOIN public.repositories ON ((repositories.namespace_id = npm_virtual_repository_upstreams.namespace_id) AND (repositories.id = npm_virtual_repository_upstreams.upstream_repository_id))
WHERE (((npm_virtual_repository_upstreams.namespace_id = $1::uuid) AND (npm_virtual_repositories.namespace_id = $2::uuid)) AND (npm_virtual_repositories.repository_id = $3::uuid)) AND (repositories.namespace_id = $4::uuid)
ORDER BY npm_virtual_repository_upstreams.position ASC;

Bound args: [01930000-0000-7000-8000-000000000001, 01930000-0000-7000-8000-000000000001, 02000000-0000-7000-8000-000000000001, 01930000-0000-7000-8000-000000000001]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Sort  (cost=25.83..25.84 rows=5 width=64) (actual time=0.065..0.067 rows=20 loops=1)
  Sort Key: npm_virtual_repository_upstreams."position"
  Sort Method: quicksort  Memory: 26kB
  Buffers: shared hit=71
  ->  Nested Loop  (cost=0.84..25.77 rows=5 width=64) (actual time=0.031..0.056 rows=20 loops=1)
        Buffers: shared hit=71
        ->  Nested Loop  (cost=0.56..23.73 rows=5 width=52) (actual time=0.022..0.028 rows=20 loops=1)
              Buffers: shared hit=11
              ->  Index Scan using npm_virtual_repositories_p32_namespace_id_repository_id_idx on npm_virtual_repositories_p32 npm_virtual_repositories  (cost=0.28..8.29 rows=1 width=32) (actual time=0.009..0.009 rows=1 loops=1)
                    Index Cond: ((namespace_id = '01930000-0000-7000-8000-000000000001'::uuid) AND (repository_id = '02000000-0000-7000-8000-000000000001'::uuid))
                    Buffers: shared hit=3
              ->  Index Scan using npm_virtual_repository_upstr_namespace_id_npm_virtual_rep_idx32 on npm_virtual_repository_upstreams_p32 npm_virtual_repository_upstreams  (cost=0.28..15.38 rows=5 width=68) (actual time=0.012..0.017 rows=20 loops=1)
                    Index Cond: ((namespace_id = '01930000-0000-7000-8000-000000000001'::uuid) AND (npm_virtual_repository_id = npm_virtual_repositories.id))
                    Buffers: shared hit=8
        ->  Index Scan using repositories_p32_pkey on repositories_p32 repositories  (cost=0.28..0.40 rows=1 width=60) (actual time=0.001..0.001 rows=1 loops=20)
              Index Cond: ((id = npm_virtual_repository_upstreams.upstream_repository_id) AND (namespace_id = '01930000-0000-7000-8000-000000000001'::uuid))
              Buffers: shared hit=60
Planning:
  Buffers: shared hit=97 read=1
Planning Time: 0.827 ms
Execution Time: 0.095 ms

Timings: planning 0.827ms, execution 0.095ms, total 0.922ms.

Closes #979 (closed)

Closes #1400 (closed)

Related to #314 (closed)

Context for LLM agents

Rationale

  • Serve the list from NpmVirtualResolutionStore.UpstreamsByVirtualRepository. Rejected: it drops soft-deleted upstreams, projects no format, keys on the child surrogate, and clips.
  • Bound the new management read with a LIMIT. Rejected: no ceiling is derivable from the schema, and a refusing bound traps the repair path for a list already over the cap.
  • Arm a request-context deadline on the three mutations. Rejected: a fired deadline yields a bare 200 until the classification on #252 settles, and the 503 these operations declare already means AuthorizationServiceUnavailable.
  • Chosen: a management-side unbounded read and no handler deadline, both deviations written into the package sidecars rather than left implicit.

Consequences: a later bulk-associate route has no schema-derived bound to rely on and would have to pick one.

Non-goals

  • Maven and container association routes. Steps 41 and 42, each carrying its own guard arm.
  • Per-association allow and deny rules. Deferred by the spec's Out of Scope.
  • Bounding the lock wait. #902 records the class and #252 owns the deadline classification.
  • Correcting docs/user/repositories/_index.md's beta note. Step 37 falsified its create half, so it belongs in its own docs(user) MR.
Edited by Hayley Swimelar

Merge request reports

Loading
Loading