Pin the MaxCapacity comment's cost claim as an assertion
Three comments in this fleet were corrected this week for justifying something with a mechanism that does not exist, and all three were caught by a reader, not by a test. This repo's was the MaxCapacity comment (#919, merged in 4523eac7): it said both configurable capacities were "pre-allocated the moment the connector is constructed." New allocates neither. The eager sized allocation is make(map[int64]*list.Element, capacity) in newDedupe, reached from newLoop when an Events iteration starts; the live buffer starts nil and grows in add.
The comment has been fixed. What was missing is the pin.
The rule this establishes: a comment that justifies a constraint by asserting a cost at a time is making a measurable claim, and should be written as a test whose name is the claim.
TestNewAllocationSizeDoesNotVaryWithCapacity sits beside the corrected comment in connector_test.go and measures New at capacity 1 and at eventfeed.MaxCapacity.
Bytes, not an allocation count — the mutation check moved this
The brief asked for allocation count, on a measurement of 8 allocations at capacity 1 and 8 at the ceiling. That measurement reproduces exactly (8.00 at capacities 1, 100, 10,000 and 1,000,000; identical under -race). But the count-based test could not fail. Mutating New to allocate by capacity —
c.mutationProbe = make([]byte, cfg.dedupeCapacity)
— is one allocation whether the capacity is 1 or a million. The count stayed 8 at both ends and the test passed, with New allocating a megabyte at construction. The count is identical in precisely the case that has to fail, so it is the wrong instrument for this claim; the claim is about cost, and cost here is bytes.
The test measures runtime.MemStats.TotalAlloc across the call instead. Re-run against both mutations:
New under test | capacity 1 | capacity 1,000,000 | verdict |
|---|---|---|---|
| unmutated | 1,568 B | 1,568 B | pass |
make([]byte, capacity) | 1,584 B | 1,009,184 B | fail |
make(map[int64]struct{}, capacity) (the newDedupe shape) | 1,616 B | 37,834,320 B | fail |
make([]byte, dedupeCapacity) only | 1,584 B | 1,009,184 B | fail |
make([]byte, liveBufferCapacity) only | 1,584 B | 1,009,184 B | fail |
if liveBuffer > dedupe { make([]byte, liveBuffer) } | 1,568 B | 1,009,184 B | fail |
if dedupe > liveBuffer { make([]byte, dedupe) } | 1,568 B | 1,009,184 B | fail |
(The last four report the row that isolates the larger capacity.)
The assertion is the invariance, not the size
Four configurations, each compared against (1, 1), never against a literal: both capacities at the ceiling, and each one at the ceiling alone while the other stays at 1. An unrelated allocation added to New moves them all together and stays green; a New that starts sizing anything by a capacity does not.
The two options move independently because moving them together samples only the diagonal (raised by Codex, #discussion_r4031265435): an allocation taken from the capacities' relationship — fired only when the live buffer capacity exceeds the dedupe capacity, say — reads identically at (1, 1) and (max, max) and leaves a diagonal-only assertion green. Both orderings of that mutation were green against the first version of this test and fail against this one.
No absolute bound: it would add nothing here — any per-capacity sizing already shows as a difference between configurations — and would make this a tripwire for every unrelated refactor, which is how a pin becomes noise.
One guard that is not the size: the test fails if New measures zero bytes. That is not a bound on the magnitude, it is a check that the measurement is still observing something — a construction optimized away would otherwise pass this test vacuously forever, which is the failure mode the PR exists to argue against. newSink keeps each connector reachable for the same reason.
Stability
testing.AllocsPerRun and TotalAlloc both read process-wide counters, so a goroutine left running by an earlier test inflates a measurement. This package runs connectors on their own goroutines throughout its suite, and CI runs it under -race (go test -race -v ./...). That noise can only add, never subtract, so the helper takes the lowest of five single measurements and compares the two floors.
Evidence it is stable rather than merely passing once:
- a naive single measurement, instrumented inside the full package suite under
-race, read exactly the same value in 64/64 windows; go test -count=5 ./pkg/basecamp/eventfeed/...— green, before and after the widening;go test -race -count=3 ./pkg/basecamp/eventfeed/...— green, before and after the widening;- byte measurement instrumented and repeated under
-race: 18/18 windows at 1,568 both ends.
The min-of-five is kept anyway: the noise source is real and named, it costs four lines, and the alternative to a cheap floor here is a flaky test on the merge gate.
go build ./..., go vet ./... and make go-lint (0 issues) all clean.
The comment now names its own proof
One line added to the MaxCapacity comment in connector.go, naming the test. The reason is the direction this test cannot cover: it catches the code drifting from the comment, not the comment being rewritten wrongly against unchanged code — which is what happened in #919. Without a pointer, the person editing that comment has no signal the claim is checked at all, so the change would defend the direction that did not fail and stay silent on the one that did. Nothing short of review catches a wrongly-rewritten comment, and the pointer does not pretend otherwise; it just puts the proof where that person is looking.
Scope
One claim, one test. Deliberately not sweeping the repo for other cost claims. Two obvious siblings, noted but not touched: the second half of the same MaxCapacity comment ("the live buffer grows to its capacity lazily and pays only for the events it admits" — partly covered by TestSustainedOverflowRetainsTheBacking), and DefaultLiveBufferCapacity's "deliberately decoupled from the dedupe capacity — only event-bearing frames are buffered."