Repository navigation
Pin the live buffer's two cost claims as assertions - #923
Conversation
The MaxCapacity comment says the live buffer grows to its capacity lazily and pays only for the events it admits, and DefaultLiveBufferCapacity says it is deliberately decoupled from the dedupe capacity. Both are measurable, and neither was measured. TestLiveBufferPaysOnlyForTheEventsItAdmits measures bytes, not allocation count: make([]Event, 0, capacity) is one allocation at any size, so a count is identical in exactly the case that must fail. Capacity and admitted count vary independently, because a diagonal cannot tell "pays for its capacity" from "pays for what it admits". TestLiveBufferCapacityIsDecoupledFromTheDedupeCapacity asserts behavior rather than bytes, and the reason is the first test: the live buffer's cost does not move with its own capacity, so a byte grid would stay green under newLiveBuffer(cfg.dedupeCapacity). What is observable is which capacity bounds which population, sampled off the diagonal in both orderings — the shipped defaults are the same number, so the equal cell discriminates nothing.
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. |
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
The updated public documentation inaccurately describes the bounded, lane-specific dedupe LRU.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds assertions that live-buffer allocation depends on admitted events and that live-buffer and dedupe capacities remain independently wired.
Changes:
- Adds allocation and capacity-decoupling tests.
- Links capacity documentation to those tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
go/pkg/basecamp/eventfeed/connector.go |
Documents capacity rationale and associated assertions. |
go/pkg/basecamp/eventfeed/buffer_test.go |
Tests lazy allocation and independent capacity wiring. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
|
Fixed in e5f337f. The clause now reads "the dedupe LRU is a window over the lane's most recent deliveries that every delivery path records into: poll page, drain and streaming alike" — bounded, per-lane, and the contrast with the buffer (only event-bearing frames) is the part that carries the decoupling. @codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
|
Review threads: 1 resolved (1 fixed, 0 declined).
|
Follow-on to #921, same treatment for the two remaining cost claims in the event-feed package's capacity comments.
The claims
MaxCapacity: "The live buffer grows to its capacity lazily and pays only for the events it admits."DefaultLiveBufferCapacity: "deliberately decoupled from the dedupe capacity — only event-bearing frames are buffered."Both justify a published constraint by naming a cost or an independence. Neither was measured.
TestLiveBufferPaysOnlyForTheEventsItAdmitsBytes (
runtime.MemStats.TotalAlloc), nottesting.AllocsPerRun's count, for the reason #921 paid for:make([]Event, 0, capacity)is one allocation whether the capacity is 1 or a million, so a count is identical in exactly the case that must fail.TestSustainedOverflowRetainsTheBackingnext to it counts allocations because its claim is about how many happen; this claim is about how big one is.Capacity and admitted count vary independently — a grid over admitted ∈ {0, 1, 16, 1000} × capacity ∈ {n, n+1, 10n, 10 000,
MaxCapacity} — because a diagonal cannot tell "pays for its capacity" from "pays for what it admits", which is the entire claim. Every cell is compared against the smallest capacity that still holds the admitted events, so a fixed-size allocation added tonewLiveBuffermoves all the cells together and stays green. The rising floor across admitted counts is the other half: the cost does move, with the events, which is what makes the invariance an observation rather than a measurement of nothing.Events are built outside the measured region and carry no strings, so the only allocations inside it are the
liveBufferstruct andappend's growth of the store. The grid never overflows — a drop would charge the measurement foradd's dropped-ids slices instead of the store.TestLiveBufferCapacityIsDecoupledFromTheDedupeCapacityBehavior rather than bytes, and the choice is not arbitrary — it follows from the first test. Because the live buffer's cost does not move with its own capacity, a byte grid over the two capacities would stay green under
newLiveBuffer(cfg.dedupeCapacity, …), which is the single most likely way to break this claim. Bytes are the wrong instrument here.What is observable is which capacity bounds which population: the buffer drops at its own configured capacity and the LRU evicts at its own, each while the other is set somewhere else. Sampled off the diagonal in both orderings (64/3, 3/64, 1/8, 8/1) plus the shipped defaults — and the defaults cell is labelled in the table as the one that discriminates nothing, since
DefaultLiveBufferCapacityandDefaultDedupeCapacityare the same number.What this test cannot reach, stated rather than papered over
Writing
DefaultLiveBufferCapacity = DefaultDedupeCapacitywould make the two constants formally coupled and leave every observation in the test unchanged — verified, it passes under that mutation — because untyped constants carry no evidence of where they came from. That half of the claim is a statement about intent and stays a comment. The half that is mechanical — that nothing in the wiring derives, swaps or clamps one by the other — is what the test holds.Mutation checks
Each mutation applied to
loop.go, built, both tests run, reverted.T1= pays-only-for-admitted,T2= decoupled.newLiveBuffer→events: make([]Event, 0, capacity)newLiveBuffer→events: make([]Event, 0, capacity/2+1)(half-sized, still eager)newLiveBufferaddrather than in the constructornewLiveBuffernewLoophands the dedupe capacity to the live buffernewLoophands the live buffer capacity to the dedupenewLoopclamps the live buffer by the dedupe capacitynewLoopclamps the dedupe by the live buffer capacityDefaultLiveBufferCapacity = DefaultDedupeCapacityM5 is the control that matters for the byte test: an unrelated allocation must not break it, or the test would become a tripwire on every edit to
newLiveBuffer. M1–M4 are four different eager-allocation shapes, including the exact one that exists indedupe.gotoday.Gates
go build ./...,go vet ./...,go test ./... -race -count=1,make go-lint(0 issues). The two new tests also pass-race -count=5.Summary by cubic
Adds two tests pinning the event-feed capacity comments' two cost claims: the live buffer pays only for events admitted, and its capacity is decoupled from the dedupe capacity. Also expands those comments to describe the dedupe LRU as a bounded per-lane window and reference the tests.
Tests
TestLiveBufferPaysOnlyForTheEventsItAdmitsmeasures bytes across a grid of capacity × admitted count, so cost must not move with capacity and must move with admitted events; bytes rather than allocation count, sincemake([]Event, 0, capacity)is one allocation at any size.TestLiveBufferCapacityIsDecoupledFromTheDedupeCapacityasserts which capacity bounds which population rather than bytes, because the first test shows cost doesn't move with capacity, so a byte grid would stay green undernewLiveBuffer(cfg.dedupeCapacity).Written for commit e5f337f. Summary will update on new commits.