From 0d9440e7a605974d46767303d0c5a5f22c47b89e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 16 Sep 2026 15:05:47 -0700 Subject: [PATCH 1/3] Pin the MaxCapacity comment's cost claim as an assertion The comment on MaxCapacity justifies the ceiling by saying where a configured capacity is actually spent: newDedupe sizes its index map by it eagerly, when the first Events iteration starts. An earlier wording said instead that both capacities were allocated the moment the connector was constructed. New allocates neither. Nothing in the suite disagreed with either version, and the wrong one stood until a reader checked it against the code. A comment that justifies a constraint by asserting a cost at a time is making a measurable claim, so measure it: New's allocation size is compared at the two ends of the permitted capacity range and must be the same at both. Bytes rather than an allocation count. The count cannot carry this claim -- make([]byte, capacity) in New is one allocation at capacity 1 and one at a million, so the count is identical in exactly the case that has to fail. Both that shape and the sized map newDedupe actually uses were introduced into New to confirm the assertion fires; at the ceiling they read 1,009,184 and 37,834,320 bytes against 1,568 unmutated. --- go/pkg/basecamp/eventfeed/connector_test.go | 62 +++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/go/pkg/basecamp/eventfeed/connector_test.go b/go/pkg/basecamp/eventfeed/connector_test.go index 9e6301da5..fafa9d8b4 100644 --- a/go/pkg/basecamp/eventfeed/connector_test.go +++ b/go/pkg/basecamp/eventfeed/connector_test.go @@ -6,6 +6,7 @@ package eventfeed_test import ( "context" "errors" + "math" "runtime" "strings" "sync" @@ -167,6 +168,67 @@ func TestNewAcceptsCapacitiesAtTheCeiling(t *testing.T) { } } +// TestNewAllocationSizeDoesNotVaryWithCapacity is the MaxCapacity comment's +// cost claim, written as an assertion. That comment justifies the ceiling by +// naming where a configured capacity is actually spent — newDedupe sizes its +// index map by it eagerly, on the first Events iteration — and that account +// holds only while New itself spends nothing on it. An earlier wording said +// both capacities were allocated the moment the connector was constructed; +// nothing in the suite disagreed, and a reader caught it instead. +// +// Bytes rather than testing.AllocsPerRun's count, because the count cannot +// carry this claim: sizing anything by the capacity — make([]byte, capacity) +// in New — is ONE allocation whether the capacity is 1 or a million, so the +// count is identical in exactly the case that must fail. +// +// The assertion is the invariance, not the size: the two ends of the +// permitted range are compared against each other, so an unrelated allocation +// added to New stays green while a New that began sizing anything by the +// capacity does not. +func TestNewAllocationSizeDoesNotVaryWithCapacity(t *testing.T) { + atOne := bytesAllocatedByNew(t, 1) + atCeiling := bytesAllocatedByNew(t, eventfeed.MaxCapacity) + + if atOne == 0 { + t.Fatal("New allocated no measurable bytes: the construction was optimized away, so this test can no longer observe it") + } + if atCeiling != atOne { + t.Fatalf("New allocated %d bytes at capacity 1 and %d at the ceiling (%d): construction now sizes something by the capacity, which only the run is supposed to do", atOne, atCeiling, eventfeed.MaxCapacity) + } +} + +// newSink keeps each constructed connector reachable, so the allocation being +// measured cannot be optimized away. +var newSink *eventfeed.Connector + +// bytesAllocatedByNew measures one New at one capacity. TotalAlloc is a +// process-wide counter and this package runs connectors on their own +// goroutines throughout its suite, so an unrelated allocation landing between +// the two reads can only ADD to a measurement. The lowest of several is +// therefore the floor, and the floor is what the two capacities are compared +// at. +func bytesAllocatedByNew(t *testing.T, capacity int) uint64 { + t.Helper() + + minter := feedtest.NewMinter() + polls := feedtest.NewPolls() + lowest := uint64(math.MaxUint64) + for range 5 { + var before, after runtime.MemStats + runtime.ReadMemStats(&before) + c, err := eventfeed.New(testOrigin, "1", minter, polls, + eventfeed.WithDedupeCapacity(capacity), + eventfeed.WithLiveBufferCapacity(capacity)) + runtime.ReadMemStats(&after) + if err != nil { + t.Fatalf("New at capacity %d: %v", capacity, err) + } + newSink = c + lowest = min(lowest, after.TotalAlloc-before.TotalAlloc) + } + return lowest +} + func TestNewValidConfigurations(t *testing.T) { minter := feedtest.NewMinter() polls := feedtest.NewPolls() From 080ab26da96cff4968b3f7ec856571c5c44edd5f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 16 Sep 2026 15:17:19 -0700 Subject: [PATCH 2/3] Vary the two capacities independently Moving both options together samples only (1, 1) and (max, max), so an allocation taken from the capacities' relationship rather than from either alone -- one that fires only when the live buffer capacity exceeds the dedupe capacity, say -- reads the same at both samples and leaves the assertion green. Hold each option at 1 while the other goes to the ceiling, in both directions, and compare all three against the smallest configuration. Both relationship-shaped mutations now fail, in the row that isolates the larger capacity; the two single-option mutations fail in two rows each. --- go/pkg/basecamp/eventfeed/connector_test.go | 47 +++++++++++++-------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/go/pkg/basecamp/eventfeed/connector_test.go b/go/pkg/basecamp/eventfeed/connector_test.go index fafa9d8b4..0dee10ece 100644 --- a/go/pkg/basecamp/eventfeed/connector_test.go +++ b/go/pkg/basecamp/eventfeed/connector_test.go @@ -181,19 +181,32 @@ func TestNewAcceptsCapacitiesAtTheCeiling(t *testing.T) { // in New — is ONE allocation whether the capacity is 1 or a million, so the // count is identical in exactly the case that must fail. // -// The assertion is the invariance, not the size: the two ends of the -// permitted range are compared against each other, so an unrelated allocation -// added to New stays green while a New that began sizing anything by the -// capacity does not. +// The assertion is the invariance, not the size: every configuration is +// compared against the smallest one, so an unrelated allocation added to New +// moves them all together and stays green while a New that began sizing +// anything by a capacity does not. The two capacities move independently, +// including in both orderings, so an allocation taken from their relationship +// rather than from either alone is caught too. func TestNewAllocationSizeDoesNotVaryWithCapacity(t *testing.T) { - atOne := bytesAllocatedByNew(t, 1) - atCeiling := bytesAllocatedByNew(t, eventfeed.MaxCapacity) - - if atOne == 0 { + smallest := bytesAllocatedByNew(t, 1, 1) + if smallest == 0 { t.Fatal("New allocated no measurable bytes: the construction was optimized away, so this test can no longer observe it") } - if atCeiling != atOne { - t.Fatalf("New allocated %d bytes at capacity 1 and %d at the ceiling (%d): construction now sizes something by the capacity, which only the run is supposed to do", atOne, atCeiling, eventfeed.MaxCapacity) + + for _, tc := range []struct { + name string + dedupe, liveBuffer int + }{ + {"both capacities at the ceiling", eventfeed.MaxCapacity, eventfeed.MaxCapacity}, + {"dedupe capacity alone at the ceiling", eventfeed.MaxCapacity, 1}, + {"live buffer capacity alone at the ceiling", 1, eventfeed.MaxCapacity}, + } { + t.Run(tc.name, func(t *testing.T) { + got := bytesAllocatedByNew(t, tc.dedupe, tc.liveBuffer) + if got != smallest { + t.Fatalf("New allocated %d bytes at dedupe %d / live buffer %d and %d at 1/1: construction now sizes something by a capacity, which only the run is supposed to do", got, tc.dedupe, tc.liveBuffer, smallest) + } + }) } } @@ -201,13 +214,13 @@ func TestNewAllocationSizeDoesNotVaryWithCapacity(t *testing.T) { // measured cannot be optimized away. var newSink *eventfeed.Connector -// bytesAllocatedByNew measures one New at one capacity. TotalAlloc is a -// process-wide counter and this package runs connectors on their own +// bytesAllocatedByNew measures one New at one pair of capacities. TotalAlloc +// is a process-wide counter and this package runs connectors on their own // goroutines throughout its suite, so an unrelated allocation landing between // the two reads can only ADD to a measurement. The lowest of several is -// therefore the floor, and the floor is what the two capacities are compared +// therefore the floor, and the floors are what the configurations are compared // at. -func bytesAllocatedByNew(t *testing.T, capacity int) uint64 { +func bytesAllocatedByNew(t *testing.T, dedupe, liveBuffer int) uint64 { t.Helper() minter := feedtest.NewMinter() @@ -217,11 +230,11 @@ func bytesAllocatedByNew(t *testing.T, capacity int) uint64 { var before, after runtime.MemStats runtime.ReadMemStats(&before) c, err := eventfeed.New(testOrigin, "1", minter, polls, - eventfeed.WithDedupeCapacity(capacity), - eventfeed.WithLiveBufferCapacity(capacity)) + eventfeed.WithDedupeCapacity(dedupe), + eventfeed.WithLiveBufferCapacity(liveBuffer)) runtime.ReadMemStats(&after) if err != nil { - t.Fatalf("New at capacity %d: %v", capacity, err) + t.Fatalf("New at dedupe %d / live buffer %d: %v", dedupe, liveBuffer, err) } newSink = c lowest = min(lowest, after.TotalAlloc-before.TotalAlloc) From fb5cae31914dea199d7365978ca3b4bd6bafb486 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 16 Sep 2026 15:40:17 -0700 Subject: [PATCH 3/3] Point the MaxCapacity comment at the test that proves its cost claim The test catches the code drifting from the comment. It cannot catch the comment being rewritten wrongly against unchanged code, which is the direction that actually failed in #919 -- and a person editing that comment has no signal the claim is checked at all. Naming the test is the only thing in reach that puts the proof in front of them. Nothing short of review catches a wrongly-rewritten comment; the pointer is not an attempt to. --- go/pkg/basecamp/eventfeed/connector.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/go/pkg/basecamp/eventfeed/connector.go b/go/pkg/basecamp/eventfeed/connector.go index f03720906..892f0821b 100644 --- a/go/pkg/basecamp/eventfeed/connector.go +++ b/go/pkg/basecamp/eventfeed/connector.go @@ -34,9 +34,11 @@ const ( // iteration cannot decline. The live buffer grows to its capacity lazily // and pays only for the events it admits; it carries the same ceiling // because the two are one published contract, not because it allocates up - // front. #900 refused the value at fixture load, after a scenario asking - // for 2,147,483,647 took the test process down. The options are the same - // request on a path the loader does not cover. + // front. That New itself spends nothing on either capacity is proven + // rather than asserted here: TestNewAllocationSizeDoesNotVaryWithCapacity + // measures it. #900 refused the value at fixture load, after a scenario + // asking for 2,147,483,647 took the test process down. The options are + // the same request on a path the loader does not cover. MaxCapacity = 1_000_000 // handshakeDeadline (EVENT_FEED_HANDSHAKE_DEADLINE, 10s) spans