## What this covers Ten comment and documentation sites across the storage-accounting surface state a fact that a merged change made false. None is a code defect. Every one of them tells a reader, or an operator reading a metric, something the tree contradicts. They are one sweep because they are one class, one reviewer, and one merge request. Each of the three issues folded here argued in its own words that grouping was cheaper than handling the sites one at a time, and that argument holds across them. AGENTS.md's "Comments Do Not Cite Process State" and "A comment must describe the code as the same change leaves it" are the governing rules. | Group | Sites | Was tracked in | | --- | --- | --- | | Dirty-set comments left behind by !1892 | 4 | this issue | | npm and Maven comments claiming no reconciliation pass runs | 5 | work item 840 | | The `storage_accounting` proto comment and its generated copy | 1 comment, 2 files | work item 693 | Two of the ten are worth more than tidiness, and both sit in group 2. `internal/format/npm/counter_emit_dispatch.go` and `internal/format/maven/upload_emit_dispatch.go` each meter a dropped delta, and their comments tell an operator how to read that metric. Today they say a drop is permanent. An operator who takes that at face value either raises an incident or builds a manual repair for a loss the reconciliation pass already corrects. --- # Group 1: dirty-set comments left behind by !1892 !1892 gave `counterbuf` a segmented dirty-set pair for test binaries, but left several comments and one doc block describing the pre-change behavior they now contradict. None of these gated the merge (all non-blocking per the [final review](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1892#note_3727252330)), but per AGENTS.md's "Comments Do Not Cite Process State" and "A comment must describe the code as the same change leaves it" guardrails, they should be swept together. ## Sites to fix 1. **`internal/accounting/drain_trigger_integration_test.go:65-82`** — the paragraph opens "Process-global understates it in CI, and nothing in this file closes the gap" and describes `./cmd/artifact-registry/...` claiming out of "these same two keys". !1892 moved both halves of this suite onto the binary's own segmented pair (`counterbuf.WithDirtySets(testDirtySets)` at line 192, `DirtySets: testDirtySets` at line 220), so the hazard the paragraph describes can no longer occur here. The paragraph reads as current because it's untouched and specific — a developer meeting a 0 count gets a diagnostic recipe that rules out the real cause. The rule in the paragraph above it still holds and should stay (a cardinality assertion still empties the set first, since the segment is per binary and this binary's cases share it) — only the "cross-suite interference" framing is stale. 2. **`internal/format/oci/emit_integration_test.go:585-592`** — `dirtyScopes`'s doc says the set "can hold no per-test prefix" and justifies the per-member read with "a cardinality would also count the members a suite running beside this one marked". !1892 added `emitDirtySets` (line 54) and routed `scopeIsDirty` through `emitDirtySets.KeyFor(scope)` (line 626), so the key now does carry a per-binary segment. The surviving reason for the per-member read is that cases *inside this binary* still share the pair — that's what needs to be written down instead. 3. **`internal/accounting/chunk_worker.go:92`** — `ChunkWorkerDeps`'s doc says "Buffer resolves each claimed scope's delta, and Merge is the only method reached through it here." !1892 added `deps.Buffer.DirtySets()` calls at lines 318 and 326. A reader (or a stub built to the doc's contract) who takes this literally treats a Buffer supplying `Merge` alone as sufficient, and a stub returning a zero `DirtySets` sends the recovery `SADD` into the deployment's pair instead of the buffer's own. Name both methods and what each is read for. 4. **`internal/accounting/drain_trigger.go:53`** — says "production wiring leaves it unset", where `drainTriggerDepsFrom` (added in !1892) now sets it explicitly. The value is the deployment's zero pair either way, so nothing behaves differently, but the sentence is no longer accurate about the wiring path. ## Why one issue The review note that raised these said they're "one comment-freshness sweep over comments the diff left behind, so they are cheaper to handle together than one at a time" — same reasoning applies to tracking them. Related to !1892. --- # Group 2: npm and Maven comments claiming no reconciliation pass runs Folded in from work item 840. ## Summary Five comments in `internal/format/npm` and `internal/format/maven` state that no reconciliation pass runs. A pass does run, so each of the five is false rather than merely stale. The Container/OCI copies of the same claim were corrected in [!1895](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1895). This item carries the copies that merge request did not touch. ## The five sites | File | What it says | | --- | --- | | `internal/format/npm/counter_emit_dispatch.go:161-163` | "A reconciliation pass would repair it ... While nothing dispatches such a pass, a drop here stands for as long as the counter row does" | | `internal/format/npm/publish_commit.go:369` | "While nothing dispatches a reconciliation pass, any of the three ..." | | `internal/format/npm/unpublish_package.go:250` | "While nothing dispatches a reconciliation pass, any of the three ..." | | `internal/format/maven/upload_emit.go:169` | "While nothing schedules a pass — `accounting.RegisterAsynqHandlers` ..." | | `internal/format/maven/upload_emit_dispatch.go:236-238` | "`accounting.RegisterAsynqHandlers` has an empty body and ..." | The two maven sites are the worse pair, because each names a symbol whose body decides the claim, and that body is no longer empty. ## Why the claim is false `accounting.RegisterAsynqHandlers` in `internal/accounting/register.go` builds a `NamespaceReconciler` and registers its `Reconcile` method with the asynq client. `accounting.RegisterRiverJobs` in the same file registers `ReconciliationTriggerArgs` as a periodic on `Config.ReconciliationInterval`. `wireJobs` in `cmd/artifact-registry/wire_jobs.go` calls `RegisterAsynqHandlers`, and `wireAccounting` contributes the River registration through its job-registration entries. The whole chain runs, so what bounds a lost delta is the configured reconciliation interval rather than the counter row's lifetime. ## The corrected form `internal/format/oci` states the bound instead of the absence. `internal/format/oci/emit.go` carries the model sentence, and `emit_dispatch.go`'s shed arm carries the metric-facing version of it. Copy that shape rather than inventing a new one, so the five read the same way as the OCI sites a reader arrives from. ## Why this is worth more than tidiness `internal/format/npm/counter_emit_dispatch.go` and `internal/format/maven/upload_emit_dispatch.go` each meter a dropped delta. Their comments tell an operator how to read that metric, and today they say a drop is permanent. An operator who takes that at face value either raises an incident or builds a manual repair for a loss the reconciliation pass already corrects. Related to https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/758 _This is a bot message 🤖 — /mergeable_ --- # Group 3: the `storage_accounting` proto comment Folded in from work item 693. ## The claim `proto/artifactregistry/config/v1/config.proto` says of the storage-accounting message that the per-field comments "describe the contract each knob will hold rather than behavior already in the binary". `gen/artifactregistry/config/v1/config.pb.go` carries the generated copy of that comment. That trailing clause is false. The preceding clause in the same comment is time-indexed — "As of this message's introduction no consumer reads this block" — and stays true, so the correction is to the trailing clause rather than to the paragraph. ## Why it is false Two readers of the `storage_accounting` block exist in the binary today. Reconciliation reads it through `accounting.NewBacklogCollector`, which takes `ReconciliationInterval` and `ReconciliationOrphanSweepInterval` at scrape time. That arrived in `cc07cd99a`. The drain reads it through `wireAccounting`, which passes the settings into `counterbuf.New`, where `DrainKeyTTLMultiplier` multiplied by `DrainInterval` sets the Redis expiry that carries buffered counters, and `DrainChunkStaleTimeout` gates the chunk admission check. That arrives with the job-registration seam. ## What has already been corrected, and what has not Three sibling sentences made the same claim and are corrected in the merge request that adds the drain reader: - `config.example.yaml` - `docs/dev/configuration-reference.md` - `cmd/artifact-registry/main.go` The proto comment and its generated copy were deliberately left out of that change set. They need `mise run proto:generate`, so they land a regenerated hunk, and that is a different review question from a comment edit. ## What closes this A change that edits the proto comment's trailing clause and runs `mise run proto:generate`, so the regenerated `gen/` hunk travels with it. While no such change is merged, a reader who greps for the claim finds it in those two files and finds the corrected text at the three sites named above. Once it merges, all five agree. ## Why it is worth doing An operator who reads the block's comment and concludes the values change nothing may omit the block or leave a value unconsidered. The values are not inert: one interval paces an anti-join over two tables, and another sets the expiry on the Redis keys that hold buffered counters. `docs/dev/configuration-reference.md` is additionally governed by a rule that it must document as-is behavior and avoid forward-looking language, which is why its own copy of the claim was corrected rather than deferred. The proto comment is not covered by that rule, which is why deferring this one is a scheduling choice rather than a rule violation. --- ## Done when All ten sites read true against `main`, in one merge request. Group 3 carries a regenerated `gen/` hunk, because its fix needs `mise run proto:generate`. That is the only part of the sweep with a review question of its own, and it is not a reason to split it back out.