Conversation
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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approved
No unresolved blocking issues were identified.
Pull request overview
Adds a regression test ensuring eventfeed.New allocation cost is independent of configured capacity.
Changes:
- Compares
TotalAllocat minimum and maximum capacities. - Uses repeated measurements and a reachability sink for stability.
File summaries
| File | Description |
|---|---|
go/pkg/basecamp/eventfeed/connector_test.go |
Adds the allocation-invariance regression test. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d9440e7a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
|
Review threads: 1 resolved (1 fixed, 0 declined).
Two things worth flagging beyond the threads, because each changed the shape of the PR. The allocation-count instrument was falsified, not just refined. The original plan was to assert an allocation count, on a measurement of 8 allocations at capacity 1 and 8 at the ceiling. That measurement reproduces exactly. The mutation check is what rejected it — The comment now names the test. fb5cae31 adds one line to the Both reviewers reported on |
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.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
MaxCapacitycomment (#919, merged in 4523eac): it said both configurable capacities were "pre-allocated the moment the connector is constructed."Newallocates neither. The eager sized allocation ismake(map[int64]*list.Element, capacity)innewDedupe, reached fromnewLoopwhen anEventsiteration starts; the live buffer starts nil and grows inadd.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.
TestNewAllocationSizeDoesNotVaryWithCapacitysits beside the corrected comment inconnector_test.goand measuresNewat capacity 1 and ateventfeed.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. MutatingNewto allocate by capacity —— is one allocation whether the capacity is 1 or a million. The count stayed 8 at both ends and the test passed, with
Newallocating 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.TotalAllocacross the call instead. Re-run against both mutations:Newunder testmake([]byte, capacity)make(map[int64]struct{}, capacity)(thenewDedupeshape)make([]byte, dedupeCapacity)onlymake([]byte, liveBufferCapacity)onlyif liveBuffer > dedupe { make([]byte, liveBuffer) }if dedupe > liveBuffer { make([]byte, dedupe) }(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 toNewmoves them all together and stays green; aNewthat 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
Newmeasures 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.newSinkkeeps each connector reachable for the same reason.Stability
testing.AllocsPerRunandTotalAllocboth 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:
-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;-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 ./...andmake go-lint(0 issues) all clean.The comment now names its own proof
One line added to the
MaxCapacitycomment inconnector.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
MaxCapacitycomment ("the live buffer grows to its capacity lazily and pays only for the events it admits" — partly covered byTestSustainedOverflowRetainsTheBacking), andDefaultLiveBufferCapacity's "deliberately decoupled from the dedupe capacity — only event-bearing frames are buffered."