chore(accounting): emit API, metrics seam, and failpoints job (S22 plan: 6/21)

What this step delivers

This step lands the exported surface of internal/accounting, the seam that registers its metrics, and the CI job for its fault-injection suite. The layering fence for the package tree is already on main from step 5.

  • Emitter, with EmitRepoCounters and EmitNamespaceCounters. Neither method returns an error. A failure the pipeline reports is logged at Warn and dropped, so a Redis fault does not fail the operation that issued the emit.
  • Two mis-wirings sit outside that no-error guarantee, because neither is a failure the pipeline reports. counterbuf.New stores its Redis client without a nil guard, so a Buffer over a nil client panics inside the operation. No Emitter method guards its receiver, so an emit through a nil *Emitter panics there too. An emitter that merely carries no buffer is not one of the two: it logs its drop and returns.
  • The accumulate arm reports an unconfirmed write rather than an absent one. counterbuf.Buffer.Increment observes its single-key script through the reply alone. go-redis has stopped retrying by the time an error reaches that arm, and every attempt whose reply was lost still ran the script. So the delta is anywhere from never applied to applied several times, and nothing on that arm tells which. record logs the drop there and returns before it marks the scope, which leaves an accumulated delta on an unmarked scope. A failure between the two Redis calls leaves that same state.
  • A lost reply has a second shape, and it is silent. go-redis retries a failed command at its own default count of three, and internal/redisclient leaves that count unset. counterbuf.Buffer.Increment is additive rather than idempotent, so a retry that reaches a Redis which already ran the script adds the same delta again. A retry that succeeds returns no error, so record marks the scope and a later drain commits the multiplied delta. Nothing is logged, and no error reaches the caller.
  • Every one of those outcomes is corrected the same way. A reconciliation pass recomputes the counter from the caller's committed source rows, and it overwrites whatever the buffered path left. This step writes the multiple-apply shape down at four sites: the guide's ## Emitting a delta, internal/accounting/doc.go, record's doc comment, and the accumulate arm itself.
  • RegisterMetrics(prometheus.Registerer) over a package-level collector list. The list is empty at this step, so the call registers nothing and returns nil.
  • One call to it from registerServiceMetrics in cmd/artifact-registry/main.go:346, beside the oci, npm, storage, and authz calls, under the same panic-on-error contract.
  • Two gofail seams on record, the single path both exported methods reach. emitSuppressed sits before any Redis call. emitBeforeMarkDirty sits after the increment and before the dirty mark.
  • test:accounting-failpoints in .gitlab-ci.yml. It is the only job that compiles the integration,accountingfaults build-tag pair. It brackets the run with go tool gofail enable internal/accounting and a matching disable in after_script. Its Postgres service is pinned at 18, matching the concrete .conformance-realbackend jobs whose shape this job copies. The job's comment names that pin, rather than sending the reader to test:integration's version matrix.
  • Ten test functions across three files. emit_test.go carries four untagged cases, each on the boundary between an emit and the operation that issued it. emit_integration_test.go carries three Redis-backed cases on the keys and the arithmetic. emit_faults_integration_test.go carries the two seam cases and one guard over them.
  • docs/dev/storage-accounting.md, the guide a call site reads. It carries the two caller contracts, and it separates what an emit detaches from what it leaves to the call site. It also gives the rule about where to emit for a delete, and how to test a call site. Its index row lands in docs/dev/README.md, and the integration,accountingfaults lint pass lands in docs/dev/go-testing.md.

This step ships no production caller. grep -rn 'accounting\.NewEmitter' --include='*.go' . matches only files under internal/accounting/, so no production code constructs an emitter. The emit API is reachable from tests alone until the npm, OCI, and Maven call sites arrive at steps 17, 18, and 19.

The fault suite's skip arm is bounded, and not closed. When a seam name does not resolve, armSeam skips, which is what lets the file compile whether or not the package carries the gofail rewrite. When no seam in the file resolves, TestEmitFaultSeams_TheRewrittenPackageCarriesASeam fails, so a run that armed nothing is red instead of a page of skips. One seam that goes missing while the other still resolves stays outside the guard's reach, and shows only as that case's own skip. The run verified the guard in both directions: red against a renamed directive, and green against the directives this branch ships.

The branch also carries a review pass over its own comment and prose. Five commits of that pass corrected the drop-policy comments in emit.go, and the no-error contract in internal/accounting/doc.go and the guide. They also corrected both test-suite headers and one log message. No behavior changed in that pass. The one production string that moved is the accumulate arm's drop message, which now reports the write as unconfirmed rather than as absent. Two later commits are documentation as well: the multiple-apply shape above, and the two plan dependency lines the next section names.

Steps 17 and 19 gain a dependency in the merged plan

This MR adds step 14, the reconciliation task, to what steps 17 and 19 depend on. The plan is already merged, so the reason is stated here as well as in the plan:

The spec's tolerance for a transient double-count rests on reconciliation existing, so an emit caller that ships before Step 14 makes every double-apply the spec calls transient permanent.

That reason is about a class, not about one mechanism. Every double-apply the spec calls transient stays permanent for as long as no recompute stands behind it. The retry above is one path into that class, and the dependency covers the class rather than that path.

The plan states those dependencies in four places, and all four move together:

  • the Depends on line of each step, with the Why text under it,
  • the Mermaid graph, which gains S14 --> S17 and S14 --> S19,
  • the dependency table, whose rows 17 and 19 now read Steps 6, 8, 14,
  • the prose below the graph, which now puts all three call-site steps below step 14.

One encoding corrected without the other three is worse than none. A reader cannot then tell which of the two answers is current. Step 18 already carried the dependency in all four places, so nothing of its own moved.

Two repository-configuration files sit outside the plan's Files: list

The plan's Files: list for step 6 names neither .gitignore nor .golangci.yaml. Both edits are forced rather than chosen, and both are comment rewrites that change no rule.

.gitignore's old comment named test:storage-failpoints as the only producer of gofail bindings. This branch adds a second producer, so that enumeration went stale the moment test:accounting-failpoints landed. The rewrite states the rule and then names every producer. It also corrects an omission that predates this branch. The hidden .conformance-realbackend template runs go tool gofail enable too, and two concrete jobs extend it. Those two are test:integration:conformance:s3-garage and test:integration:conformance:gcs-key-creds.

The *.fail.go rule itself is load-bearing for this MR. It keeps internal/accounting/emit.fail.go out of any commit made while the package source carries the gofail rewrite.

.golangci.yaml's depguard rationale rested on the same kind of claim about the tree's state. It said that internal/accounting/'s Redis subpackage exists and its top-level files do not yet, which is why the top-level glob is listed ahead of them. This MR lands three of those top-level files, so that sentence is true on main and false the moment this merges. The rewrite states the rule the globs enforce instead. A scope's globs are listed whether or not files matching them exist. A pattern that matches nothing costs nothing. An unlisted pattern leaves its tree unfenced for as long as it takes a merge request to add the first file under it. The same rewrite retires an internal/jobsriver/ clause that was already stale on main, since internal/jobsriver/client.go exists.

An emit detaches its context, and runs on the caller's goroutine

Both Redis calls run on context.WithTimeout(context.WithoutCancel(ctx), detachedEmitTimeout), and detachedEmitTimeout is 5s.

Step 6's plan entry, its Files: list, and its Acceptance do not describe this behavior. The context choice was taken and recorded, and this description states it rather than re-argues it. It lands here because the detach is already merged at the first call sites the swap reaches.

Here is what the detach protects against. A caller contract puts the emit after the commit of the rows its delta describes, so the context an emit gets can already be done. go-redis fails a done context inside its connection pool before it dials. Then the delta drops, and the emit logs it the way it logs a Redis fault, with Redis healthy and never asked.

5s is the value the repository's other detached post-outcome writes carry. remote's healthRecordTimeout and detachedDownloadBumpTimeout, npm's and Maven's bufferedUpdateTimeout, and managementapi's bufferedCounterUpdateTimeout are each 5s. What is borrowed from those five is the duration and nothing else. The constant is a tighter bound rather than the only one. go-redis applies its own dial, read, and write timeouts per attempt and retries a failed command, and internal/redisclient leaves each at the library default. So an unreachable or stalled Redis is bounded either way. What the constant buys is a 5s wait in place of the client's own worst case of tens of seconds.

What record does not do is dispatch. Four of those five sibling constants bound a goroutine their call site spawns behind a shedding in-flight cap. npm's and Maven's bufferedUpdate, managementapi's bufferedCounterUpdate, and remote's detached download bump each pair their 5s with a 64-slot semaphore. Each sheds against that semaphore rather than queues on it, so the wait never lands on the goroutine that issued the write. remote's healthRecordTimeout is the one of the five that runs inline, and it bounds the recording phase of a scheduled health probe. record spawns nothing and sheds nothing, so its bound falls on whichever goroutine calls it.

All three npm sites that step 17 swaps sit inside bufferedUpdate today. That wrapper does four things: it detaches, it bounds, it spawns, and it sheds. record reproduces the first two. A call site that replaces one of those wrappers with a bare emitter.EmitRepoCounters(ctx, …) therefore keeps the bound, and drops the goroutine and the cap. An earlier version of this description said the detach preserves merged behavior, without naming which property it preserves. This paragraph is what replaces that sentence: the detach and the bound are preserved, and the other two properties are not.

Whether an emit runs inline or on a goroutine the call site spawns stays the call site's decision. internal/accounting makes neither choice for it. What this MR adds is that cost written down where a call-site author reads it, in internal/accounting/doc.go and in the guide's ## Emitting a delta.

What the metrics seam proves, and what no test can prove

accountingCollectors in internal/accounting/metrics.go is an empty []prometheus.Collector. RegisterMetrics therefore registers nothing and returns nil. Two consequences follow, and both limit what this MR can assert.

A second call returns nil again, so there is no duplicate-registration error to provoke. The composition-root registerer gains no entry, so no test can tell a wired call in main.go from a dropped one. That is why the branch carries no internal/accounting/metrics_test.go.

The run closed that gap outside the test suite, on the booted binary. go tool objdump -s 'main\.registerServiceMetrics' shows main.go:346 CALL internal/accounting.RegisterMetrics, beside its three sibling calls in the same function. The call therefore survives link-time dead-code elimination. GET /-/metrics answered 200, and the 130 KB body carries zero occurrences of the string accounting. That absence is the correct outcome over an empty collector list. A gitlab_artifact_registry_accounting_* series here is the defect.

Neither cmd/artifact-registry/main.go nor internal/accounting/metrics.go changed after that boot, so the evidence describes the registration path this branch ships. Both properties become assertable at the first step with a collector to register, and that step owns their tests. No docs/dev/observability.md row is owed yet, for the same reason: the package publishes no series.

CI enrollment for internal/accounting is non-recursive on purpose

Both test:integration and test:accounting-failpoints list ./internal/accounting without /.... The recursive form re-enrolls internal/accounting/counterbuf, which the three Redis topology jobs already run under their own prefix. That runs one suite four times and splits one package's enrollment across two jobs.

The changes: glob for the same tree stays recursive. internal/accounting imports counterbuf, so a counterbuf-only MR must still run the tests that consume it, and a changes: glob does not cross a /. Both .gitlab-ci.yml sites carry the reason in a comment, so a later reader does not correct one spelling into the other. scripts/ci/check-integration-test-wiring.sh pools every job's entries into one flat allowlist, so a recursive entry in either job vouches for packages neither job compiles.

One departure from the plan's step 6 Files: text belongs here rather than in the plan. That text makes the choice between the two allowlists turn on whether a package needs this job's PostgreSQL. That test sends internal/accounting to the Redis topology matrix. Its integration suite acquires one Redis through testutil.StartIsolatedRedisClient and no database at all. The test this job applies is the one its own internal/remote paragraph states. The matrix is for a suite that needs three Redis topologies. A suite satisfied by one Redis, by PostgreSQL, or by both runs here. internal/accounting qualifies here on that test, and counterbuf sits in the matrix because the single-key counter behavior is topology-covered there.

Spec coverage

Spec: docs/specs/S22-storage-accounting.md

Acceptance criteria

# Criterion (abridged) Tests
AC-1 Repo-scoped increment reaches repositories.artifacts_count/size_bytes after the next drain tick Emit half here: TestEmitCounters_AccumulatesUnderTheFamilyKeyAndMarksTheScopeDirty (repo scoped case). The Postgres half needs a running drain and is Step 10's.
AC-2 Namespace-scoped increment reaches namespace_statistics after the next drain tick Emit half here: same test, namespace scoped case. Postgres half is Step 10's.
AC-3 Concurrent increments to one scope sum exactly, no lost updates Emit half here: TestEmitCounters_ConcurrentEmitsSumExactly. Redis-script half is Step 5's (on main); eventual-Postgres-value half is Step 10's.
AC-4 Re-marked scope: no lost delta, no double count, bounded overlap Not this step. Re-mark clause Step 5; sequential reclaim Step 8; no-lost-delta end to end Step 10; overlap Step 14.
AC-5 Chunk past drain_chunk_stale_timeout bails Step 8.
AC-6 Chunk failing every attempt re-adds its scopes before the terminal error Step 8.
AC-7 Redis unavailable at increment: operation succeeds, delta absent, reconciliation corrects Drop half here, twice over: TestEmitCounters_UnreachableRedisDropsTheDeltaAndLogsAtWarn (live connection failure, plus the Warn-not-Error level) and TestEmitCounters_SuppressedEmitWritesNothing (seam). Restore half is Step 14's, through this step's suppression seam.
AC-8 Reconciliation clears a scope's buffer before the SQL scan Step 14.
AC-9 Per-format soft-delete visibility in the recomputes Steps 11, 12, 13, 14.
AC-10 Positive hit per version-type table and (format, kind) Steps 11, 13.
AC-11 Drift recorded on the unit-matched histogram before the overwrite Step 14.
AC-12 Crash between HINCRBY and SADD leaves the increment un-marked and still captured Seam landed here and its Redis-observable half asserted: TestEmitCounters_CrashBeforeMarkDirtyLeavesTheDeltaUnmarked. The reconciliation-captures-it half is Step 14's, through this seam.
AC-13 Sliding TTL refreshed on every write; dirty sets carry no TTL Step 5 (on main) for the refresh and no-TTL clauses; drained-at-normal-cadence is Step 10's.
AC-14 Migrations apply cleanly and roundtrip Steps 1, 2a, 2b.
AC-15 blob_storage_blobs triggers keep the shadow exactly consistent Step 2b.
AC-16 Every namespace has a zero-valued statistics row by construction Step 1.
AC-17 npm publish increment swapped onto the pipeline; Δartifacts at both unpublish handlers Step 17.
AC-18 OCI increments at CompleteUpload, MountBlob, manifest PUT Step 18.
AC-19 OCI decrements at the delete handlers, not inside the deleters Step 18.
AC-20 Maven upload emits post-commit; the stub is retired, not swapped Step 19.
AC-21 Repository cascade hard-delete emits through the purger (gated on #464 (closed)) Integration contract verified in the S20-A plan's work, not here.
AC-22 One asynq task per namespace candidate, never one per repository Task half Step 14; fan-out half Step 15.
AC-23 Namespace with no statistics row gets one on its first pass (UPSERT) Step 13.
AC-24 Concurrent reconciliation tasks never exceed reconciliation_max_in_flight Step 14.
AC-25 Source-first ordering, asserted per emitting site The contract half ships in this step as documentation (internal/accounting/doc.go and docs/dev/storage-accounting.md), which is prose rather than a test. All seven per-site assertions are Steps 17, 18, 19. The pipeline cannot enforce the ordering from its own side, so no test in this package can assert it.
AC-26 last_reconciled_at stamped only after every repository is written back Step 14.
AC-27 Trigger selects only stale namespaces; orphan sweep reaches the rest Step 15.
AC-28 A namespace already in flight is enqueued at most once at a time Step 15.
AC-29 reconciliation_backlog exposed by a single-writer collector Step 16.
AC-30 Drain chunk skips a scope reconciled since its baseline read Guard predicate Step 7; drain-versus-reconciliation interleaving Step 14.
AC-31 counter_dirty_set_size sampled once per tick, before SPOP Step 10.
AC-32 Config load rejects each invalid configuration Steps 3 (on main) and 3b (on main).
AC-33 The six metrics registered with exact names, types, and bounded label values; three alerts fire Registration half is split across Steps 8, 10, 14, 16. Four of the six reach the registry through the entry point this step lands, from Steps 8, 10 and 14. The other two are reconciliation_backlog and namespaces_missing_statistics, which Step 16 registers through its own cmd/artifact-registry/wire_accounting_metrics.go, because that scrape-time collector needs the cache-purpose Redis client and the datastore pool this call site does not hold. This step registers no vector of its own, so nothing here is assertable: over an empty collector list RegisterMetrics registers nothing, a second call returns nil again, and the composition-root registerer gains no entry. Alert-wiring half lands with #354 (closed).
AC-34 Saturated reconciliation task re-enqueues, never sheds or blocks Step 14.
AC-35 Namespace-scoped chunk with no statistics row drops the delta and deletes :flushed Zero-rows half Step 7; guard-skip half Step 8; reconciliation half Step 14.
AC-36 A failing recovery SADD does not lose the delta Failing-recovery half Step 8; reconciliation half Step 14.
AC-37 Management-API deletes emit once their transaction commits (gated on #313 (closed)) Deferred with #313 (closed); no plan step owns it.
AC-38 A persistently failing reconciliation task leaves its namespace re-enqueueable Step 14.

Error cases

# Condition Tests
E-1 Redis unavailable at increment time TestEmitCounters_UnreachableRedisDropsTheDeltaAndLogsAtWarn and TestEmitCounters_SuppressedEmitWritesNothing. The reconciliation-corrects half is Step 14's.
E-2 Redis unavailable at drain-trigger time Step 10.
E-3 Chunk job's Postgres UPDATE fails Step 8.
E-4 Chunk job's :flushed DEL fails after the UPDATE succeeded Step 8.
E-5 Chunk job exhausts all retry attempts Step 8.
E-6 Recovery SADD itself fails Step 8 (the injected failure) and Step 14 (the reconciliation recovery).
E-7 Chunk job dequeued past drain_chunk_stale_timeout Step 8.
E-8 Worker dies mid-chunk after merging into :flushed Step 14. No test in this step: the loss window opens at the merge, which no emit reaches.
E-9 Two chunks run the same scope concurrently Step 14 (criterion 4's overlap clause).
E-10 Trigger's EnqueueTx fails while the process is alive Step 10.
E-11 Crash between the trigger's SPOP and its EnqueueTx commit or re-SADD Accepted crash-only gap the spec declines to close; reconciliation is the stated backstop (Step 14). No test asserts the gap itself, and the spec asks for none.
E-12 Assigned repository or namespace row hard-deleted before its chunk drains Step 7 (zero-rows half).
E-13 Namespace-scoped chunk drains a namespace with no statistics row Steps 7, 8, 14 (criterion 35).
E-14 Crash between a scope's HINCRBY and its SADD TestEmitCounters_CrashBeforeMarkDirtyLeavesTheDeltaUnmarked asserts the delta survives in the live hash with the scope unmarked. That it is later captured from committed source is Step 14's.
E-15 Crash between reconciliation's pre-scan clear and its write-back Step 14.
E-16 Reconciliation scan races a concurrent increment Step 14.
E-17 A drain chunk and a reconciliation process the same scope concurrently Steps 7 (guard predicate) and 14 (the interleaving).
E-18 Reconciliation finds a discrepancy Step 14.
E-19 Namespace has no statistics row when its reconciliation task runs Step 13.
E-20 Reconciliation task fails before its final UPSERT Step 14.
E-21 A namespace can never be reconciled Steps 14 (re-enqueueable) and 16 (stays counted in the backlog).

Security considerations

# Concern Tests
S-1 Redis keys carry only internal UUID segments, so no key injection and no cross-slot relocation Enforced at this layer by the parameter types: both methods take uuid.UUID, so no caller-supplied free text can reach a key. The exact key an emit writes under is pinned by TestEmitCounters_AccumulatesUnderTheFamilyKeyAndMarksTheScopeDirty and TestEmitCounters_ScopesAndFamiliesDoNotShareAKey, both of which assemble the expected string themselves rather than reading it back from the builder. The identifier-rejection half (ErrInvalidID on :, {, } and the empty string) is Step 5's counterbuf/keys_test.go, on main.
S-2 Counter values are non-secret but feed billing, so a bug has financial impact The stated mitigations are the drift metrics and the standing reconciliation schedule: Steps 14 and 16. Nothing in this step.
S-3 No new credential surface: Redis and Postgres reuse the existing clients The emitter composes over an already-built counterbuf.Buffer and opens no connection of its own. The Redis-backed tests build it over redisclient.NewCacheClient, the cache-purpose client the composition root already wires.

Two tests the criterion rows above do not carry

Neither maps onto a criterion row or an error-case row, so both are named here instead.

  • TestEmitCounters_UnmarkedScopeIsDroppedAndLogsAtWarn covers record's third drop, where the accumulate is taken and the dirty marking is refused. No criterion row names that condition: the spec's SADD failure rows are about the drain's recovery SADD, not the emit's own. The test drives it with a go-redis ProcessHook that fails sadd alone and reports every other command as taken, so it needs no deployment. It asserts the record's message alongside counter_family and counter_scope, because the message is what separates this arm from the other four in a log.
  • TestEmitFaultSeams_TheRewrittenPackageCarriesASeam is a guard over the two fault cases rather than coverage of its own. It protects AC-7's and AC-12's seam halves against a rename that turns both cases green by omission.

TestEmitCounters_EmitterWithNoBufferIsInert also gained assertions. It pins the one Warn record a buffer-less emit leaves, with its message and both keys, in place of asserting only that no panic escapes.

e2e scenario catalogs

Nothing under docs/testing/ changed, and nothing there is affected. The catalog rule binds feat and fix, and this step is chore, so the statement belongs here rather than in an edit.

The run examined the diff for behavior change regardless of the step type and found none. No route is added, removed, or re-dispatched. The diff adds no configuration key, no schema change, and no migration. The one boot-time change registers an empty collector list, so /-/metrics gained zero series. The emit API has no production caller until step 17. So no user-visible behavior exists for a scenario to describe.

Diff size

git diff --shortstat $(git merge-base origin/main HEAD)...HEAD gives 14 files, +1690 −36, measured at head 5c412a61. The base is this branch's merge base with origin/main, which was 253431f9 at that measurement. The recipe names the base as a merge base rather than as a fixed SHA on purpose. origin/main moved many times during this review, and the rebase moved the merge base with it. A pinned SHA then stops measuring this branch, and it still returns a plausible number. The total is past the 500 reviewable-LOC threshold, so docs/dev/development-model.md asks for a split or a justification with the split by file group.

Group Files, with added lines Added Removed
Tests emit_test.go 489, emit_integration_test.go 322, emit_faults_integration_test.go 161 972 0
Go production emit.go 268, doc.go 65, metrics.go 48, main.go 14 395 0
Documentation and the plan storage-accounting.md 161, go-testing.md 7/−12, docs/dev/README.md 1, the S22 plan 16/−9 185 21
CI and repository configuration .gitlab-ci.yml 121/−1, .gitignore 10/−5, .golangci.yaml 7/−9 138 15
Total 14 files 1690 36

A split does not help, and the group sizes are the reason.

  • Tests are 972 of the 1690 added lines, 58% of the diff. They are the floor this step exists to establish, and they cannot ship apart from the code they constrain.
  • Go production is 395 lines across four files, and 300 of those lines are a comment or a blank line. 95 statement lines remain, well inside the threshold on their own.
  • CI and repository configuration is 138 lines, and 89 of them are a comment or a blank line. All 49 lines that remain are in .gitlab-ci.yml: one go list entry, one changes: glob entry, and the 47-line new job. The .gitignore and .golangci.yaml edits are comment rewrites and nothing else, which the counts above show directly — 10 of 10 and 7 of 7.
  • Documentation is the guide the plan's Files: list names for this step, plus a one-line index row in docs/dev/README.md and one lint-pass entry in docs/dev/go-testing.md. The go-testing.md group's 12 removed lines are one bullet, re-flowed to one sentence per line so the new pass and its stand-in rule fit. The plan's 16 added and 9 removed lines are step 6's Status cell, plus four places. Each of those four states the step 14 dependency for steps 17 and 19.

Every group is part of the same contract. An emit API is not reviewable without its tests, without the job that runs its fault suite, or without the guide its call sites read. A split moves lines between MRs, and neither half becomes easier to read.

Merge-order notes

Two of the merge requests these notes were written about have merged, and this branch is rebased onto them. So the notes fall in two groups. The first group records what that rebase settled, because the resolutions are worth reading rather than re-deriving. The second group is a manual merge that still falls to whichever side lands second.

git merge-tree --write-tree origin/main HEAD exits 0 with no conflict region, so nothing on main conflicts with this branch as it stands.

What the rebase settled

  • !1561 (merged) ci(integration): faster test postgres and a migrations job split, merged, had five points of contact with this diff. One point conflicted textually, and the rebase resolved it. The other four landed without a conflict, and two of them appear in no earlier note.
    • The packages= go list line in test:integration, which was the conflict. main carries the tail filter grep -vE '/datastore/(jet|migrations)(/|$)', and a comment above the line explaining it. The line now carries main's filter and this branch's ./internal/accounting entry, and both comment blocks stay, because they explain different things.
    • The &integration-test-files anchor. main appended ".gitlab-ci.yml" at the end of the list this branch extends near the top, so those two edits merged without a conflict. main also anchors that job's rules: as &integration-rules, and its new test:integration:migrations job reuses them. So the "internal/accounting/**/*" entry this branch adds now triggers test:integration:migrations as well.
    • The gocover-cobertura version. main pinned every install to @v1.5.0 while this branch's new test:accounting-failpoints installed @latest, which the rebase left as the only @latest in the file. This branch took the pin rather than leaving it to a later reader, quoting the version token and the back-reference comment from the sibling jobs. The file now holds eight @v1.5.0 installs and no @latest.
    • The *pg-service-options anchor. main appended fsync=off, synchronous_commit=off, full_page_writes=off, and autovacuum=off after dynamic_shared_memory_type=mmap. test:accounting-failpoints references that anchor by name, so it now takes those options with no edit of its own. Nothing in the job opens Postgres today, so the inherited options change no result. They are recorded here for the reviewer of the later step that does open it.
    • Four anchors on test:integration: &pg-test-service, &cobertura-report, &coverage-regex, and &integration-rules. This branch's new job pins its own postgres:18-alpine service, and it spells artifacts, coverage, and rules out inline. So four reusable anchors now exist that the new job does not use.
  • The count in this branch's own .gitlab-ci.yml comment survived !1561 (merged). The comment says that every package under internal/accounting/ is enrolled in exactly one of the two -tags=integration go list allowlists. This file holds exactly two such lines, one in test:integration and one in the .redis-integration-base template the three Redis topology jobs extend. The substantive point survives with the count: ./internal/accounting sits in the first list, and ./internal/accounting/counterbuf/... in the second. The same sentence's "either job" phrase names test:integration and test:accounting-failpoints explicitly, so it needs no change either.
  • What that sentence leaves out is the width of the pool. scripts/ci/check-integration-test-wiring.sh:18 harvests package entries with an unanchored grep -- '-tags=integration', so a job that passes the tag without go list joins the same flat allowlist. test:integration:migrations is one of those jobs, beside test:integration:brand-list-validator and the storage-driver jobs, which predate it. So the two go list sets are the enrollment surface the comment reasons about, and the pool behind the wiring script is wider than two. A reader who counts -tags=integration lines rather than go list ones needs both facts.
  • !1511 (merged) feat(datastore): batched guarded counter UPDATE (S22 plan: 7/21), merged, filled Status row 7 in the S22 plan. Row 7 sits directly below the row 6 cell this branch fills, and the two edits share no line. They still conflicted as one region, because row 7's fill shifts every later line, and the rebase kept both cells. Row 6 carries !1605 (merged) and row 7 carries !1511 (merged). !1604 (merged) fills row 11, five rows down, and it is still open.

Still open, so the order still matters

Every merge request in this group was open when this description was written. Each state was read then, rather than carried from an earlier survey. Each one is an open position rather than a settled one, and this branch matches none of them. A state below can change without this text changing, so read it as a pointer to the merge request rather than as its current state.

  • !877 (merged) test(iam): integration test for ReadRelationships and Health — the same go list line and the same &integration-test-files anchor. It enrolls ./internal/iam/... recursively on both. This branch's non-recursive spelling for ./internal/accounting is deliberate, and the rebase kept that entry on the line, so !877 (merged) still meets it. The two spellings differ on purpose, and neither is a correction of the other. The plan's step 6 text carries the rationale, which is what a reviewer who compares the two lines needs. Its new test:integration:iam-rig job harvests -tags=integration too, and its entry sits outside internal/accounting/, so the comment's claim about that tree holds after it merges.
  • !1542 (merged) and !1535 (merged) — both add a managementapi.RegisterMetrics(appReg) call at this branch's exact insertion point in registerServiceMetrics. They collide with each other as well as with this branch, and whichever of the two lands second drops its own call. The rebase against this branch is mechanical either way round: the calls are independent, and their order inside the function carries no meaning. This branch's own rebase leaves that function unchanged, because cmd/artifact-registry/main.go merges onto main without a conflict.
  • !1027 chore(skills): add e2e-tests skill for catalog-paired end-to-end tests — appends to the .gitignore tail, on the line directly below the gofail comment block this branch rewrites. Adjacent lines only, with no shared line. Its hunk is cut against an older .gitignore than the one on main, so its own rebase is owed whatever the order against this branch.
  • !1559 (merged) proposes that a step MR stop editing the plan file, the Status table included, and that a separate MR fill the rows in batch. It is unmerged, so this branch fills row 6, which is what the rule on main asks for. If !1559 (merged) merges first, whoever rebases decides what happens to that cell.

Five more open MRs reach blocks this diff touches. Each one is worth a look before a rebase, rather than a copy into this branch.

MR Contact with this diff
!818 (closed) Renames test:integration, the job this branch adds a go list entry to, and relocates the block. Draft.
!924 Replaces every job's gocover-cobertura and cobertura-report block with the go-test framework's coverage aggregation. That block is what this branch's new job carries, and the new job then needs a place in post-test:go:coverage's needs. Draft, and the largest structural contact in the set.
!921 (merged) and !1069 Each adds a row to the docs/dev/README.md table this branch adds a row to, in a different region of the table.
!1011 (closed) Inserts into the same registerServiceMetrics function on a much older base, so its hunk relocates on rebase. Draft.

Related to #515

This is a bot message 🤖 — /smurfit

Edited by Pawel Rozlach

Merge request reports

Loading
Loading