fix(maven): size the buffered quotas by how long a shed write stays lost

Dzmitry (Dima) Meshcharakourequested to merge
dmeshcharakou/maven-last-updated-review-followups-docs into main

What

Documentation and test residuals from the branch review of !2399 (merged), which merged while that review was still running, plus the buffered quota re-tier.

This is not a docs-only diff: internal/format/maven/buffered.go's per-label quota map changes here, which moves the shed threshold for all six buffered columns in production.

Stack

The stack is resolved. !2409 (merged), the behavioural half of the same review, merged as 5d67d01a1 (fix(maven): track the committed push below the success write) and now sits in this branch's merge base, so nothing gates this MR.

That merge conflicted with this branch on the three files the two shared. Resolved by taking main's side whole, so internal/format/maven/upload.go, internal/format/maven/upload_commit.md and internal/format/maven/upload_repo_updated_test.go are byte-identical to main at 577ecde53 and are no longer in this diff. Nothing was ported back: main's order runs the two post-commit dispatches, then writeUploadSuccess, then trackArtifactPushed last, and being below the write is what stops a tracker panic answering a durably committed upload with a 500.

!2409 (merged)'s squash carries only those three files, so the buffered quota re-tier that an earlier version of this table attributed to it lands here instead.

Rebased onto main at a216c9b7b on 2026-09-09. AGENTS.md was the only conflict, and it is an append collision rather than an overlap: both sides add prose-convention sections immediately above ## Labels, main having gained three of its own there since the old merge base 06d0ab63e. Resolved by keeping both, main's three first, so no line of main's is removed and none of this MR's prose is dropped. The other eleven files merged clean, and every hunk in them is byte-for-byte what 9a83ce97c carried. The rebase rewrote every commit sha on the branch, so the citations below are the post-rebase ones.

Order MR Branch What State
1 !2409 (merged) dmeshcharakou/maven-last-updated-review-followups The committed arm's dispatch order merged as 5d67d01a1
2 this MR dmeshcharakou/maven-last-updated-review-followups-docs S10 stamp placement, the last_updated_at writer set, the buffered quota re-tier, test hygiene open

S10's stamp step sat inside the transaction it must stay out of

docs/specs/S10-maven-hosted.md opens Upload Step 5 with - In a single SQL transaction: and made the timestamp-and-counters step its third child (lines 1149 and 1217 at the merge base 06d0ab63e), so the spec prescribed exactly the placement !2399 (merged) existed to remove.

The step's own tail already routed the emission through the post-commit goroutine path, so the bullet disagreed with its own position rather than being flatly false. The position is the half a reader checks first, and following it reproduces the hang !2399 (merged) measured: the statement takes FOR NO KEY UPDATE on the same repositories row the transaction holds at FOR SHARE, so from inside it waits on a second pooled connection while the holder waits on the client, with no cycle for PostgreSQL to break.

Promoted to a sibling of the transaction list, ordered after it, with the lock reason stated. Guardrail 1 makes the spec the source of truth, so this is the surface that has to agree with the code.

Two review findings on that hunk are fixed on top:

  • The lock reason covered both post-commit writes and it only reaches the stamp. maven.CounterEmitter resolves through mavenCounterEmitter to accounting.Emitter, which records the deltas into the counterbuf buffer and takes no lock on the repositories row at all. The deltas now carry the reason CounterEmitter's own doc comment gives: a delta emitted from inside the transaction records a movement a rollback can still take away (6224fef6c).
  • Two of the four outcome rows described arms the step never reaches. A byte-identical release re-upload and a release conflict both abort the transaction, so writeCommitOutcome takes the errReleaseByteIdentical or errReleaseConflict arm and never reaches the dispatches on the txErr == nil arm above them. The two are folded into one row whose reason is the arm rather than a delta a caller has to suppress, which takes that list from four rows to three (990c5f039).

### Interim implementation of buffered columns still describes the counter emit as a direct UPDATE against repositories. That paragraph is stale against accounting.Emitter independently of these hunks, and correcting it is a separate change.

The last_updated_at writer set

Two comments in internal/format/npm/publish_commit_emit_test.go said npm publish is the tree's only live writer of repositories.last_updated_at. The Maven upload became a second through the same shared statement, and the container writes became a third when !2362 (merged) merged during this branch's life.

The reader those comments mislead is whoever does the S22 swap that takes artifacts_count and size_bytes off the publish's direct write: they are what says which columns must keep advancing, so an "only" there narrows that scope to npm.

Stated without a count, pointing at git grep MarkRepositoryLastUpdated -- '*.go' as what enumerates the callers at any commit. Naming npm and Maven as a pair was the same rotting shape, and it rotted inside the hour, which is why the second commit here replaces it.

That grep reaches a format's own stamp site only where the format's port keeps the method's name. Maven's UploadStore declares MarkRepoLastUpdated (internal/format/maven/upload.go:97) and reaches the shared statement through mavenStoreAdapter (cmd/artifact-registry/wire_maven.go:463), so the plain pattern lands on the adapter and not on Maven's own stamp site. The sidecar names that hop and gives git grep -E 'MarkRepo(sitory)?LastUpdated' -- '*.go', which reaches both spellings.

Correcting the clause in TestPublishEmitIntegration_AllFourCountersMoveAfterDrain's doc comment put that whole twelve-line block under its two-line cap, so its prose moves to internal/format/npm/publish_commit_emit_test.md, the way internal/format/oci already carries emit_test.md.

The buffered quotas were sized by a criterion that does not hold

The six per-label quotas in internal/format/maven/buffered.go were split 16/6/16/6/16/4, and the rationale beside them sorted the labels by a criterion that does not hold.

It placed version_size_bytes among the self-repairing columns and cut it to 6, when what a shed recompute actually costs is set by release immutability: MavenVersionStore.RecomputeVersionSize is reached only from recomputeVersionSizeAfterCommit on a committed primary-file upload, and once a release version's last primary file lands no further upload to the coordinate can commit, so a recompute shed on that file has no successor. Reconciliation recomputes nothing for it either.

That column has two production writers rather than one. MavenFileDeleter.deleteFileTx refolds maven_versions.size_bytes through MavenVersionStore.RefoldVersionSizeTx inside the hosted file delete's own transaction, which docs/dev/storage-accounting.md:43 already records, and UPDATE(table.MavenVersions.SizeBytes) at internal/datastore/maven_version_size.go:375 is the one site that reaches both writers. Whether that refold is the repairing event that would move the column down a tier is not settled here: the remote column one tier below repairs the same way, so making 16 against 12 stand up needs ADR-010 cache retention cited as the remote column's repairing event, and that derivation belongs to #559.

The same criterion held last_downloaded_at at 16 as a loss nothing heals, when MavenRemoteCacheStore.BumpLastDownloadedAtByID (internal/datastore/maven_remote_cache.go:267) sets NOW() under no freshness predicate and the next delivered remote read rewrites it. And it cut hosted_last_downloaded_at, the one row whose statement already declines most writes, on mavenAccessBumpWindow.

Regrouped by how long a shed write stays lost, and sized from that: 16 where nothing repairs it (downloads_count, version_size_bytes), 12 where a later event repairs it but a consumer can act on the stale value first (remote_version_size_bytes, last_downloaded_at), and 4 where the statement's own one-hour window declines most writes (hosted_last_downloaded_at, last_updated_at). Both the old split and the new one sum to bufferedUpdateMaxInFlight, which is 64, and that exact sum is what keeps bufferedUpdateSem's shed arm unreachable by construction.

internal/format/maven/buffered.md is new and carries the per-column reasoning, because the map's inline comment is an ordinary block capped at two lines and the tiers do not fit there.

Review follow-ups on the sidecar and the spec

Three review findings on the sidecar and the spec are fixed on top, each as a doc fix plus the convention it establishes in AGENTS.md:

  • buffered.md asserted a writer set it had not derived, and left a tripwire that could never fire. RecomputeVersionSizeTx is reached only from inside RecomputeVersionSize, and no delete path will ever call it because the delete path takes the refold, so "the day a delete path calls it" is a condition no grep on that symbol can show. Both clauses are deleted rather than rewritten, which leaves the release-immutability case standing on its own; no number, no want map and no tier row moves (86b550d48, convention in 5b9c806d5).
  • The bottom tier's residual was stated for last_updated_at and left open for hosted_last_downloaded_at, and the two do not share one. A shed last_updated_at write misorders a repository in a sort until the next upload; a shed hosted_last_downloaded_at write leaves maven_packages.last_downloaded_at and maven_versions.last_downloaded_at at a value the freshness leg guarantees is NULL or already over an hour old, with no bound above it. Each label now carries its own clause, dated to when keep_last_downloaded_at ships since no Go code reads either column yet. "That bounded cost is what the smallest quota buys" is dropped, having held for the one label and not the other; the quota is unchanged (859e3f937, convention in 1a7b9ea78).
  • S10's "The counter deltas run after it" took the previous sentence's antecedent, the stamp, and so asserted an order the code reverses: the emit is dispatched from commitFileRow as soon as the transaction returns nil (internal/format/maven/upload.go:651), before writeCommitOutcome dispatches the stamp (:987). Both wait on the commit, which is the precondition the reason clause already pointed at, so the clause now names the commit instead of the pronoun. No code moves (6882c7f71, convention in e0b025707).
  • That convention section then misplaced the stamp itself, as "below the response write", which is where the tracker goes and not the stamp. writeCommitOutcome's txErr == nil arm dispatches recomputeVersionSizeAfterCommit and markRepoUpdatedAfterCommit (internal/format/maven/upload.go:986 and :987), then writeUploadSuccess (:994), then trackArtifactPushed (:995), and internal/format/maven/upload_commit.md:51 already states that order, so the two documents disagreed on the one site the section cites as its evidence. One word, and it leaves the point the section makes standing: the two dispatches sit at different sites, which is why the pronoun asserted an order nobody chose (d4237313f).

Test hygiene

internal/format/maven/upload_commit_test.go drives nine PUTs, seven of which commit, and installed no bufferedUpdate wait hook, so each committed one left the version-size recompute and the last_updated_at stamp running untracked past the end of its case. The other two abort the transaction, the byte-identical 200 and the 409, and dispatch nothing.

It sorts before upload_repo_updated_integration_test.go in the same integration binary, and three cases there assert exact deltas on the process-global bufferedCounterUpdates vector, so a late increment can land inside one of those before/after windows and fail a case that did nothing wrong. Nine other test files in the package already install the hook (git grep -l 'withBufferedWaitGroup(t' -- 'internal/format/maven/*_test.go', less this file).

On review, the drain now lives in the fixtures instead of in the cases (798405714). newStreamFixture, newEmitEnv and newUploadFixture each call awaitBufferedDispatches as their last statement, which reaches every case built on one of the three (git grep -l -e newStreamFixture -e newEmitEnv -e newUploadFixture -- 'internal/format/maven/*_test.go' gives nine files at 798405714, all of them serial). The placement is safe because awaitBufferedDispatches is a wait-hook install plus t.Cleanup(wg.Wait) rather than a blocking wait: moving it into the constructor changes when the hook goes in, not when the drain runs, and the registration still lands after newMavenClient's pool teardown, so LIFO drains before the pool closes.

No assertion pins the drain. Deleting it leaves the suite green over three runs and over a forced 300 ms delay inside bufferedUpdateWorker. What does move is the number of dispatches made while bufferedUpdateWaitGroup is nil, which is a goroutine no test drains: over go test -tags=integration -run TestUpload_ ./internal/format/maven/, 44 un-awaited last_updated_at dispatches with no drain anywhere, 37 with the five per-case calls this MR first carried, and 11 with these fixture calls.

repoCounters.lastUpdatedAt in internal/datastore also goes. Retiring the MarkRepoLastUpdated stub took the only subtest that read it, and its doc comment claims it pins a column some case asserts against, which no case in that package now does.

What this does not reach

  • The drain reaches only the cases the three fixtures build. internal/format/maven/upload_usagedata_integration_test.go builds its own client and dispatcher, so it reaches none of them and its committing PUTs stay un-awaited: all 11 of the un-awaited stamps that remain are its. Its cases sort after upload_repo_updated_integration_test.go, so they cannot land in that file's delta windows, but it is a fourth site with the same property. Raised on the review thread rather than closed here.
  • internal/format/maven/upload_commit.md's scope line names commitFileRow and upsertFileRow, and the sidecar also has sections on writeCommitOutcome and on the two post-commit dispatches. Both files are byte-identical to main here, so widening that line is a change against main rather than part of this diff.
  • internal/datastore/maven_repositories.go's ReVerifyRepositoryAlive doc still names the reconciler as its caller, contradicting the type doc !2399 (merged) widened in the same file. !2390 (merged) is rewriting that function's signature and surrounding doc, so it is raised there rather than edited here.
  • docs/dev/storage-accounting.md's three wrong cap line numbers are #1193, already being resolved by !2392 (merged).

e2e scenarios

No scenario is added or affected. The one production change is the buffered quota map, which moves how many concurrent buffered writes each column may hold before one is shed; no route, status, payload, or stored column value moves, and a shed write was already a designed lossy outcome that the next dispatch or reconciliation repairs. The rest of the diff is prose: a spec's bullet nesting and the ordering clause above it, comment corrections in internal/format/npm/publish_commit_emit_test.go and internal/format/maven/buffered.go, two new sidecar .md files, three new convention sections in AGENTS.md, the test drain with its three fixture call sites, and one removed test fixture field.

Merge order

git diff --name-only main...HEAD at d4237313f gives twelve files against main at a216c9b7b. Ten are touched by none of the 62 other open merge requests. Two are, and this MR has to merge before !2020 (merged).

!2020 (merged) (docs(agents): route AGENTS.md detail into lazily-loaded modules) is the order dependency. It removes 37 ## sections from AGENTS.md, ## Labels and every prose-convention section this MR's three sit among included, and re-homes that detail as sentence-case headings in a new docs/dev/writing-discipline.md. Its base is a216c9b7b, main's own head, and it merge-trees clean against main, so its conflict is with this branch's hunks rather than with staleness, and whichever lands second ports rather than merges. No pipeline reports it.

This MR first is the cheaper order: !2020 (merged) then carries the three sections into docs/dev/writing-discipline.md under its own heading casing, which is the move it is already making for the 37 it removes. If !2020 (merged) lands first instead, the three sections have to be re-targeted at that file before this MR can merge, and the ## Labels anchor they sit above no longer exists.

!2077 (merged) needs no order stated against this MR, because it cannot merge before it rebases: it is 120 commits behind main, and git merge-tree --write-tree exits 1 between its head and origin/main too. Its four sections land between the tail of ## A Steady-State Claim Names the Transient It Excludes and ## Labels, which were adjacent in its base dbf8b383f and are not in main, so that rebase is what decides whether it comes near this MR's anchor at all.

git merge-tree --write-tree against origin/main at a216c9b7b exits 0. !2362 (merged), which also edited the last_updated_at surfaces, merged before this branch was pushed.

Size

244 reviewable LOC (200 added, 44 removed) across twelve files, measured at d4237313f against main at a216c9b7b, which is also the merge base. AGENTS.md is 74 of the added lines and internal/format/maven/buffered.md 51, so the prose is most of it. Under the 500 in development-model.md.

Related to #1046

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports