refactor(npm): split the publish charge into a per-blob gate (S11 Provenance Attestations plan: 2/20)
Summary
The publish charge reads one blob's two size verdicts inside the same function that draws the dispatch slot. This MR separates them: blobSizeDeltas computes one blob's repository- and namespace-scoped size deltas, and emitPublishCounters takes the summed pair. Every emitted value, label, and dispatch count is unchanged, and a publish still draws one slot. It is Step 2 of docs/plans/2026-09-11-npm-provenance-attestations.md, landed by docs(plans): add the S11 npm provenance attesta... (!2648 - merged) • Hayley Swimelar • 19.4.
Summing before the call is what keeps a second blob on one dispatch later. dispatchCounterEmit takes the publish site's own quota of 10 before the 64-wide process pool, and a dispatch past that quota is dropped with no retry, recorded only as npm_buffered_counter_updates_total{column="size_bytes",result="dropped"}. That quota is sized for one dispatch per publish carrying both scopes, so a second dispatch would double the draw and could shed on its own, leaving one scope short the bytes the other charged. A summed call keeps both halves on the one dispatch that either lands or sheds as a unit.
Three calls the diff does not explain:
- The two-adjacent-boolean swap hazard moves rather than disappears.
emitPublishCounterscarried it before andblobSizeDeltascarries it now, and the two verdicts trade places between the old signature and the new one, so a swapped call site compiles and charges each column the other's verdict. The integration emit suite already caught that onmain, but it needs PostgreSQL and Redis, soTestAfterCommit_HandsEachVerdictToItsOwnScopemoves the guard intogo_unittests. Applying the swap was verified to leave the whole npm unit suite green without that test and to fail both of its rows with it. - The row counts stay literal
1s inside the closure rather than becoming parameters. A publish commits one version however many blobs it charges, so a caller has nothing to say about them. - The new cases live in
publish_commit_emit_internal_test.gorather than thepublish_commit_emit_test.gothe plan's Step 2 names. That file is//go:build integrationin packagenpm_test, so it can reach neither unexported symbol, and the only external route isexport_test.go, which Step 3 of this plan declares. The named file is byte-identical on this branch, which is the behavior-preservation evidence its acceptance criterion asks for.
Governing ADRs
scripts/adr-freshness.sh reports the mirror one upstream commit behind, on 012_usage_data_collection.md alone, plus one internal ADR that never mirrors locally. Both were read at their current source.
- ADR 023 keeps
database/sqlout of the format layer and enforces format isolation through depguard.blobSizeDeltasis an unexported pure function that imports nothing, so none of the three rules has a target. - ADR 007 fixes what a repository's
size_bytesand a namespace's deduplicated size sum over. The emitted deltas are identical to the ones they replace, so the reconciliation contract is untouched. - ADR 002 scopes deduplication to organizations and ADR 008 fixes SHA-256 content addressing. Both verdicts keep their meaning and their gating, which is the property the asymmetric test rows pin.
- ADR 012 governs the usage-data event catalog. This diff adds, changes, and removes no event, so the one stale mirror file does not govern it.
- No ADR governs internal function decomposition inside a format package.
Testing
go test ./internal/format/npm/... -count=1 passes. Five unit tests cover the change, all in publish_commit_emit_internal_test.go:
TestBlobSizeDeltasholds each size half to its own verdict across five rows, including the two asymmetric ones a swapped pair of arguments fails and an empty tarball.TestAfterCommit_HandsEachVerdictToItsOwnScopecalls the realafterCommitwithrebuildSemsaturated, so the rebuild sheds inline and detaches no worker, and asserts the full recorded emit on both scopes under each verdict alone.TestEmitPublishCounters_CarriesTheSummedSizeDeltasToBothScopessums two blobs with different verdicts into 640 and 600, a pair no single blob produces, and pins one call per scope with a row count of one. The repository sum needs both blobs, and a swapped pair of arguments fails on 640 against 600.TestEmitPublishCounters_TakesOneDispatchSlotPerPublishsaturates the dispatch pool and asserts the dropped series moves by exactly one, which is what separates one dispatch from one per scope.TestEmitPublishCounters_NilEmitterDispatchesNothingpins the seam guard: an unwired handler neither panics nor moves the dropped or panic series.
The integration emit suite in publish_commit_emit_test.go is unchanged and compiles against the new signature. It needs PostgreSQL and Redis, so CI runs it rather than this branch.
Behavior is unchanged, so no docs/testing/ scenario is added or affected, and no docs/user/ page is either: both new symbols are unexported and no customer-visible surface moves.
Context for LLM agents
Spec coverage
Spec: docs/specs/S11-npm-hosted.md. Plan: docs/plans/2026-09-11-npm-provenance-attestations.md, Step 2.
Step 2 acceptance
| # | Criterion | Tests |
|---|---|---|
| A-1 | publish_commit_emit_test.go passes unchanged in what it asserts |
That file is byte-identical to main (zero diff lines) and compiles against the four-parameter signature, verified by go test -tags=integration -run XXX_none. Its five cases need PostgreSQL and Redis, so they are compile-verified here and execution-verified by CI's integration job. |
| A-2 | A case that blobSizeDeltas zeroes each size half under its own verdict and neither half under the other's |
TestBlobSizeDeltas, five rows: neither verdict (bytes, bytes), repositoryHeld alone (0, bytes), deduplicated alone (bytes, 0), both (0, 0), and a zero-byte blob. The two single-verdict rows are the asymmetric ones. |
| A-3 | A case that two blobs' size deltas summed into one call reach the emitter in one dispatch with both row counts moving by one | Split across two tests, because one cannot carry both halves. TestEmitPublishCounters_CarriesTheSummedSizeDeltasToBothScopes covers the summed deltas and the row counts. It cannot cover "one dispatch": a split into one dispatch per scope still yields exactly one repository call and one namespace call, so that mutation leaves it green. TestEmitPublishCounters_TakesOneDispatchSlotPerPublish covers the dispatch count instead, by saturating the pool and asserting the dropped series moves by one. |
| A-4 | Behavior unchanged, one slot drawn per publish | A-1 plus A-3's second test. emitPublishCounters has exactly one production caller before and after, afterCommit. |
| beyond | The call site's verdict-to-scope mapping | TestAfterCommit_HandsEachVerdictToItsOwnScope. Not in the plan's acceptance, and added because the refactor reverses the two verdicts' positions and nothing else in go_unittests covers that line. |
Rationale
Accepted deviations from the plan's Step 2 section:
- The plan's
Tests:field is not satisfiable as written. It namespublish_commit_emit_test.go, extended. That file is//go:build integrationin packagenpm_test, and both new cases need package-npmaccess, sinceblobSizeDeltasandemitPublishCountersare unexported. The summed-two-blob case is further unreachable through the HTTP handler until Step 20 charges a second blob. The only external route isexport_test.go, which Step 3 declares and a sibling branch owns, so exporting was rejected over a merge collision and over putting two pure-function assertions behind a build tag that needs a database. The cases went into a newpublish_commit_emit_internal_test.go, following the package's existing*_internal_test.goconvention. docs/dev/storage-accounting.mdline 540 is reframed, one clause. That sentence attributed the first-attach gate toemitPublishCounters, which this refactor moved toblobSizeDeltas, so it now namesblobSizeDeltasas the gate andemitPublishCountersas what emits the figures it is given, matching theemitRebuildCountersprecedent the same paragraph gives at line 545. Line 543 already states the posture, and lines 564 and 2900 still hold. Four live MRs hold that file open and none of them touches lines 535 to 550, so the one-line edit does not collide. Step 20 charges a second blob and will make the same sentence wrong about the blob count, which is that step's correction to make.- The plan's LOC estimate is off in both directions. It forecast about 40 source and about 120 test. Actual: 24 source and 327 test. The source figure overshoots because the refactor moves existing lines rather than adding logic. The test figure is about 2.7 times the forecast, because the table test, the recording double with its three interface methods and mutex, the four emit cases, and the call-site table each carry their own assertion messages.
blobSizeDeltas' two adjacent booleans were not reordered, though they are the structural cause of the swap hazard. The merged plan names the signature verbatim, so reordering would deviate from an approved plan for a readability gain. The anti-swap test is the mitigation.
Non-goals
- Consolidating the four-way duplication of this computation across formats (
npm.blobSizeDeltas,oci.attachDeltaandoci.detachDelta,oci.committedRemoteFillDeltas, and the inline form in Maven andnpmremote). ADR 023's format-isolation depguard rule means a shared helper must land aboveinternal/format/, which is a subsystem decision rather than a step of this plan. - Reworking
packument_cache.go's assign-then-zero delta form around lines 1391 to 1393, beside its ownEmitNamespaceCountersdispatch. A one-hunk follow-up if the package should be internally consistent, and out of this step's declared file list. - The 66 integration-tagged lint findings across eleven untouched
*_integration_test.gofiles in this package. CI compiles no//go:build integrationfile, so they are a standing blind-spot backlog rather than this branch's regression. Neither of this MR's two files carries one. - Charging a second blob's bytes. That is Step 20, which adds the bundle's pair to the tarball's and passes the sums into this same call.
- Editing the plan's Status table. Row 1 was recorded on
mainby docs(plans): record the merged step MRs in seve... (!2797 - merged) • David Fernandez • 19.5, and row 2 belongs to the workstream's table-owner MR rather than to a step branch.
Related to #1273