Bound the connector's capacities where they are allocated - #919
Merged
Merged
Conversation
A fixture asking for a dedupe capacity of 2,147,483,647 could take the test process with it, because the dedupe index sizes its map to the capacity the moment the connector is constructed. #900 closed that at the fixture loader, so no conformance scenario can ask for it any more. The same request still arrives through the public options. WithDedupeCapacity and WithLiveBufferCapacity were checked for positivity and nothing else, so any consumer of the package can hand the constructor a number that is not a slow run but an allocation the process cannot decline — a typo, or a value that meant bytes rather than events. Both are now bounded by the ceiling the conformance schema already declares, in the constructor that owns the allocation and beside every other bound it enforces. The tests assert the constructor's error rather than the behaviour past it: a test that allocates to prove the allocation is the thing being prevented.
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.
🟡 Changes recommended
Allocation documentation is inaccurate, public option contracts omit the new ceiling, and the valid upper boundary is untested.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Bounds event-feed connector capacities to prevent excessive allocations.
[!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.
Changes:
- Adds a shared 1,000,000 capacity ceiling.
- Rejects oversized dedupe and live-buffer configurations.
- Adds validation tests for rejected values.
File summaries
| File | Description |
|---|---|
connector.go |
Adds and enforces capacity limits. |
connector_test.go |
Tests oversized capacity rejection. |
Review details
Suppressed comments (1)
go/pkg/basecamp/eventfeed/connector.go:434
WithLiveBufferCapacity's public GoDoc still documents only the positive-value requirement, so it omits this newly enforced upper bound. Please state that values must not exceedMaxCapacity.
if cfg.liveBufferCapacity > MaxCapacity {
return usageError(fmt.Sprintf("live buffer capacity must be at most %d, got %d", MaxCapacity, cfg.liveBufferCapacity))
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 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: 4bc17feca3
ℹ️ 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".
The comment justifying MaxCapacity described an allocation that does not happen. New builds the config, validates it, and returns a Connector; it allocates neither structure. The dedupe index sizes its map to the capacity in newDedupe, reached from newLoop when an Events iteration starts, and the live buffer starts as a nil slice and grows to its capacity lazily. The limit is still right and the failure that prompted it was real — the allocation is eager, it just happens on the first iteration rather than at construction. But a comment that justifies a limit with a mechanism that does not exist is how the next person removes the limit for a good-sounding reason, so both it and the test's version now say what holds: construction-time validation refuses the value before it can reach the run loop. The test comment also claimed this test goes through the fixture loader, when it calls New directly. Both option GoDocs documented only positivity, so a caller could not validate an input without first eating a construction error. They name the ceiling now.
Every capacity case asserted a refusal, so a comparison that slipped from > to >= would narrow the published bound with the whole suite still green: each refusal case is satisfied by a ceiling one lower. Construction now has to accept MaxCapacity exactly, from both options. Flipping the comparison fails those two cases and nothing else. The schema-bound test pinned the loader's own maxScenarioCapacity, a second literal 1,000,000 that could drift from the exported one silently. It is eventfeed.MaxCapacity by declaration now, so the test that compares the schema's `maximum` against it pins the schema to the public contract too.
§23's public validation contract said these options must be positive and set no upper bound; the JSON-schema `maximum` constrains conformance fixtures only. So the Go reference was enforcing a limit the shared contract did not state, and the pending ports could legitimately accept 2,147,483,647 and diverge from it with nobody finding out until a conformance run or a bug report. The ceiling is right — an unbounded capacity a driver may honor eagerly is a footgun in every port — so it is stated where the ports will read it: EVENT_FEED_MAX_CAPACITY, a shared API constraint every SDK's construction-time validation applies, refused with the `usage` code like the positivity check beside it. Appendix A carries the constant with the other EVENT_FEED_* rows. The bound is written in three places, and they are tied rather than independently maintained: the Go loader's constant is the exported one by declaration, the existing schema-bounds test compares the schema's `maximum` against that, and the schema's own descriptions name the constant. It is tagged [static] rather than [conformance]: the schema's `maximum` means no fixture can carry an over-ceiling capacity, so the refusal is verified by build checks, not by the fixture suite.
Member
Author
|
Review threads: 7 resolved (7 fixed, 0 declined with the reasoning in each thread). |
There was a problem hiding this comment.
🟡 Changes recommended
The SPEC claims an unenforced three-way drift invariant, and both updated schema descriptions contain a grammatical error.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
conformance/event-feed/schema.json:662
- This sentence is missing a verb/link between “bound” and “validation,” making the schema description grammatically incomplete.
"description": "Default 10000 delivered ids. Deliberately decoupled from liveBufferCapacity. Capped at 1,000,000 — SPEC §23's EVENT_FEED_MAX_CAPACITY, a resource ceiling, since a driver may allocate the capacity eagerly. The same bound the connector's own construction-time validation applies, so no fixture can ask for what a driver must refuse.",
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
The sentence claimed the three statements of 1,000,000 cannot drift. Only two of them are machine-tied: the tier-2 loader's bound is the exported constant by declaration and the schema's `maximum` is compared against it, but SPEC's own number and Appendix A's row are hand-maintained, as Appendix A says every `EVENT_FEED_*` row is. That is the same defect this PR started with — a claim resting on a mechanism that does not reach as far as the claim — so it says what holds and names the editing obligation the rest is left to. Gating SPEC's number instead would mean a fifth doc-constants line kind, in a gate whose own header calls that set deliberately bounded, for a value that has not moved; Appendix A's rule is to gate a row when it starts moving. The schema descriptions carried a sentence fragment. Fixed.
Merged
jeremy
added a commit
that referenced
this pull request
Sep 16, 2026
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.
jeremy
added a commit
that referenced
this pull request
Sep 18, 2026
* 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. * 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. * 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Codex raised this on #778 three minutes after that PR merged, so the finding landed in
mainunaddressed. Half of it has since been fixed by #900, which refuses fixture capacities above the schema's ceiling at load. This is the half that is still open.What is still reachable
conformance/event-feed/schema.jsonboundsdedupeCapacityandliveBufferCapacityto[1, 1000000].validateConfigchecks both for positivity and nothing else, so the public options are unbounded:newDedupethen sizes its index withmake(map[int64]*list.Element, capacity). That is not a slow run — it is an allocation the process cannot decline, and it takes the caller with it rather than returning a usage error. The plausible route is not a hostile value but an ordinary one: a typo, or a number that meant bytes rather than events.The allocation is eager but it is not at construction:
Newvalidates and returns, and the capacity reachesnewDedupethroughnewLoopwhen the firstEventsiteration starts. Construction is where it is cheap to refuse — before the validated config can reach a run — which is the reason for the bound, not the allocation happening earlier than it does. The live buffer starts as a nil slice and grows lazily; it carries the same ceiling because the two capacities are one published contract.After #900 no conformance fixture can ask for this. Any consumer of the package still can.
The change
Both capacities are bounded by
MaxCapacityinvalidateConfig, beside every other bound it already enforces — the filter-list cap, the positive-id checks, the positive durations.The ceiling is declared in SPEC §23 rather than enforced only in Go. §23's validation contract said these options must be positive and set no upper bound, and the JSON-schema
maximumconstrains conformance fixtures only — so a reference implementation enforcing an undeclared limit would let the pending ports legitimately accept 2,147,483,647 and diverge, with nobody finding out until a conformance run. It is nowEVENT_FEED_MAX_CAPACITY, a shared API constraint every SDK's construction-time validation applies, with the constant in Appendix A.Two of the places the number is written are machine-tied: the Go tier-2 loader's
maxScenarioCapacityiseventfeed.MaxCapacityby declaration, and the existing schema-bounds test compares the schema'smaximumagainst that constant, so the schema cannot drift from the option contract. SPEC's own number and Appendix A's row are hand-maintained, as Appendix A says everyEVENT_FEED_*row is — the §23 text says so rather than implying the tie covers it, and the schema's field descriptions name the constant so an editor lands on the cross-reference.Tests pin the bound from both sides. The refusal cases are each capacity one past the ceiling plus a dedupe capacity at
int32max, the value from the original report; they assert the constructor's error rather than the behaviour past it. The acceptance cases construct at exactlyMaxCapacity, so a comparison that slipped to>=fails instead of silently narrowing the published bound — mutation-checked.go build ./...,go vet ./...,go test -race ./pkg/basecamp/eventfeed/...,make go-lint,make doc-constants-checkandmake event-feed-fixtures-checkall pass.