fix(maven): track the committed push below the success write
What
writeCommitOutcome's committed arm emitted trackArtifactPushed before the two post-commit dispatches and before the success write. A panicking tracker therefore took the dispatches with it and cost the client the response it had durably earned. This moves the tracker last, below the write, and corrects the sidecar passage that justified the old order.
The buffered-quota re-tier this MR originally carried has been dropped; see "Why the quota re-tier is gone" below. All six per-label quotas stay at their merged values.
The dispatch-order defect was found by a branch review of !2399 (merged), which merged while that review was still running.
Stack
Merge in this order. Both branches touch internal/format/maven/upload.go, so the order is required rather than advisory.
| Order | MR | Branch | What |
|---|---|---|---|
| 1 | this MR | dmeshcharakou/maven-last-updated-review-followups |
The committed arm's dispatch order |
| 2 | !2410 (merged) | dmeshcharakou/maven-last-updated-review-followups-docs |
S10 stamp placement, the last_updated_at writer set, test hygiene |
!2410 (merged) currently carries this MR's writeCommitOutcome hunk in its pre-review shape, so it needs a rebase onto this branch's new tip before it merges second. Without one it reinstates the tracker-above-the-write order this MR fixes.
The dispatch order
The committed arm now runs the two post-commit dispatches, then the success write, then trackArtifactPushed. Two distinct facts set that order, and the second is what puts the tracker below the write.
- A panicking tracker unwinds the handler, and LabKit's
PanicRecoveryMiddlewarecatches it above us without resuming us, so any dispatch sequenced below the call never runs. - That middleware writes its own 500 whenever no header has gone out yet (
if !prw.wroteHeaderinhttpserver/recovery.go, labkit v2.39.0). With the tracker above the write, a tracker fault answered a durably committed upload with a 500.
internal/format/npm/publish_commit.go states the first fact on its own equivalent call, and it holds there because its afterCommit runs after the response is produced. This package's own download path already tracks after its write, in serveRedirect and the full-200 arm of serveStream. The errReleaseByteIdentical arm carried the same inversion and moves with it.
upload_commit.md previously borrowed npm's sentence verbatim, which made it claim the response was already written when it was not. It now states the mechanism itself and names the condition that makes the precedent true where it came from.
Measured rather than argued. TestWriteCommitOutcome_StampsBeforeTrackingSoATrackerPanicCannotLoseIt pins three properties, each checked by breaking it in a scratch copy: the tracker back above the write reddens on the recorder's status, the tracker between the two dispatches reddens on the version-size recompute, and a dropped stamp reddens on the stamp. Its primaryUpload previously carried no versionID, so recomputeVersionSizeAfterCommit returned at its nil-version guard and only the stamp was pinned; the case now sets one and records the recompute.
Why the quota re-tier is gone
The re-tier sized the six quotas by how long a shed write stays lost. Review established that this conflates two axes: a quota is a concurrency bound, so it sets how often a shed happens, while loss duration sets what each shed costs. It also gave hosted_last_downloaded_at the smallest quota while, on the hosted path, that label fires on a strict superset of the events downloads_count fires on: bumpAccess runs on every resolved GET and HEAD and ahead of the If-None-Match short-circuit, where bumpArchiveDownloadCount returns early for a non-archive extension and only fires on a delivered archive-class 200 or 302.
Sizing the quotas on the right axis needs the per-label bufferedCounterUpdates{result="dropped"} series from production, which nothing here has read. Landing numbers derived from the wrong axis in the meantime is what this drop avoids.
Reverting the values alone would not have been enough. The per-label quotas must sum to bufferedUpdateMaxInFlight exactly, asserted by TestBufferedUpdate_PerLabelQuotaFitsGlobalCap and claimed at buffered.go:80, at buffered.go:176, and in the sidecar. The merged five other than version_size_bytes already sum to 58 against a cap of 64, so no raise of version_size_bytes fits without a compensating cut or a cap change. A cap change is not free either: twelve sibling caps across the service each admit 64 against the one application pool, a joint draw internal/accounting/reconcile_task.go records as exceeding what any single constant's reasoning assumes (issue 559). So the raise both reviews asked for is a decision about the cap or about another label, not a local edit.
internal/format/maven/buffered.md is deleted rather than rewritten. It was added by this MR to carry the tier rationale; with the tiering no longer argued, a rewritten version would have to justify six merged numbers this MR does not change and cannot justify without that series. Its useful pointers (work items 559 and 1194) already live on the TODO(buffered) markers in buffered.go.
What this does not reach
- The
version_size_bytesunder-protection itself. A shed recompute on an immutable release's last primary file has no repairing upload, so the quota of 6 is plausibly too low. It stays at 6 here, and both that and the six-count re-derivation are recorded on work item 1046 with the sum invariant and the pool over-commitment named as the constraints. buffered.go's merged sizing comment. It still says the quotas are "Sized by what a shed write costs before any repair, not by dispatch rate", which is the same cost-versus-concurrency conflation the review identified. This MR reverts to it rather than editing it, since it now changes no quota; the correction belongs with the re-derivation on work item 1046.- Every other panic-above-an-emit site. The reorder covers
writeCommitOutcome's two tracking arms. It does not audit the rest of the package for the same shape. - The stamp's own row-lock wait.
MarkRepositoryLastUpdatedtakesFOR NO KEY UPDATEon arepositoriesrow a concurrent upload commit holds atFOR SHARE, and nothing bounds that wait but the worker's 5 s context. Same defect family as #1199 (closed), which sets this case aside explicitly.
e2e scenarios
No scenario is added or affected. The reorder changes one response: a panicking usage tracker on a committed upload now returns the 201 or 200 the upload earned, where it previously returned the recovery middleware's 500 over a durable row. That path is reachable only by faulting the tracker, which no scenario can provoke through the protocol, so it is pinned by the unit case above instead. docs/testing/e2e/ gains nothing.
Merge order
git diff --name-only main...HEAD against the 85 open MRs finds one overlapping file, internal/format/maven/upload.go, in two of them.
| MR | Branch | Overlap |
|---|---|---|
| !2351 (merged) | dmeshcharakou/upload-inactivity-deadline-step-3 |
internal/format/maven/upload.go |
| !2410 (merged) | dmeshcharakou/maven-last-updated-review-followups-docs |
internal/format/maven/upload.go |
!2410 (merged) is this stack's second MR: the Stack section above carries its order and the rebase it needs.
!2351 (merged) needs no coordination. Its hunks sit in streamAndCommitPrimary around upload.go:297-360, some 600 lines from this branch's, and git merge-tree --write-tree HEAD origin/mr/2351 reports one conflicting file, docs/dev/configuration-reference.md, which this branch does not touch. The same merge-tree from origin/main conflicts on that file too, so the conflict is !2351 (merged)'s own rather than a product of merge order here. git merge-tree --write-tree for HEAD origin/mr/2410 and for HEAD origin/main both exit 0.
Re-derived at 505202607 against origin/main 5f966401a. An earlier revision of this section claimed the open set holds no other MR touching these files, which was wrong on both !2351 (merged) and !2410 (merged) and contradicted the Stack section.
Size
145 reviewable LOC (117 added, 28 removed) at 505202607, under the 500 in development-model.md. 97 are the dispatch-order test, 18 the sidecar, and 30 production Go.
Related to #1046