Refuse fixture capacities above the schema's ceilings at load - #900
Merged
Merged
Conversation
conformance/event-feed/schema.json caps liveBufferCapacity and dedupeCapacity at 1,000,000 — a resource ceiling, since a driver may allocate the capacity eagerly — but the Go loader's validateConfig ranged only the duration fields. A fixture naming dedupeCapacity: 2147483647 passed the loader and reached newDedupe, whose index map is sized by the capacity, so running the Go tier-2 tests without the JSON-schema target in front could take the process down in an allocation instead of rejecting the fixture. The capacities now share the durations' three-state decode (absent, null, value) and are ranged to [1, 1000000] at load, refused with the field and the ceiling named; an explicit zero, a negative and a null are refused as the schema does rather than silently read as the default. A self-test pins the ceiling itself loading and everything past it refused through parseScenario alone, before any connector exists, and a second reads schema.json back and compares every bound the loader enforces — the ms maximum, the capacity maximum, their floors — to the constants the range checks use, so the loader's copy cannot drift from the contract.
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
The schema-drift test omits the enforced minimum for advance.ms.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds load-time capacity validation to prevent oversized event-feed fixtures from causing excessive allocations.
Changes:
- Preserves absent/null/value states for capacities.
- Enforces schema capacity limits before connector construction.
- Adds capacity and schema-drift 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 |
|---|---|
scenario_fixture_test.go |
Adds capacity decoding and validation. |
scenario_harness_test.go |
Passes explicitly configured capacities. |
scenario_selftest_test.go |
Tests capacity limits and schema alignment. |
scenario_conformance_test.go |
Defines the schema path for self-tests. |
Review details
- Files reviewed: 4/4 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.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
Member
Author
|
Review threads: 1 resolved (1 fixed, 0 declined). #900 (comment) — the schema drift test now asserts advance.ms's minimum as well as its maximum (5cd9532). |
jeremy
added a commit
that referenced
this pull request
Sep 16, 2026
* Bound both capacities where they are allocated, not only in fixtures 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. * Say where the capacity is actually allocated 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. * Accept the ceiling as well as refuse past it 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. * Declare the capacity ceiling in §23, not only in Go §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. * State what the ceiling's drift check actually covers 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.
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.
Follow-up to #778. Addresses the post-merge finding #778 (comment):
conformance/event-feed/schema.jsoncapsliveBufferCapacityanddedupeCapacityat 1,000,000 (a resource ceiling — a driver may allocate the capacity eagerly), but the Go loader'svalidateConfigranged only the duration fields. A fixture withdedupeCapacity: 2147483647passed the loader and reachednewDedupe, whose index map is sized by the capacity, so running the Go tier-2 tests directly (withoutmake event-feed-fixtures-checkin front) could OOM the test process instead of rejecting the fixture.What changed
go/pkg/basecamp/eventfeed/scenario_fixture_test.gooptionalMsrenamed tooptionalInt, since it now carries capacities and delay-envelope bounds as well): absent means default, JSON null is refused, a value is ranged.maxScenarioCapacity(1,000,000) andcheckScenarioCapacity;validateConfigranges both capacities to[1, 1000000]at load, naming the field and the ceiling in the refusal. An explicit0, a negative andnullare now refused as the schema does, where before they were silently read as "use the default".go/pkg/basecamp/eventfeed/scenario_harness_test.gogo/pkg/basecamp/eventfeed/scenario_selftest_test.goTestScenarioCapacitiesAreRangedAtLoad: for each capacity, the ceiling itself loads and an omitted key stays absent; one past the ceiling,2147483647, the largest int64,1e7,0,-1,nulland a quoted number are refused byparseScenarioalone — before any connector exists to allocate — with the field and the ceiling in the message.TestScenarioConfigBoundsMatchTheSchema: readsschema.jsonback and compares every bound the loader enforces (the five config ms fields, the two capacities,advance.ms,fireTimer.assertDelayMs.{min,max}— minima and maxima) to the loader's constants. The loader's copy of the numbers is now cross-checked against the contract rather than being a second unverified declaration; a mutant ceiling of 2,000,000 fails this test on both capacity fields.go/pkg/basecamp/eventfeed/scenario_conformance_test.goscenarioSchemaPathbesidescenarioFixtureGlob.Commands run
From
go/:go build ./...— exit 0go vet ./...— exit 0go test -race -count=1 ./pkg/basecamp/eventfeed/...— exit 0From the repo root:
make go-lint— 0 issues, exit 0make event-feed-fixtures-check— exit 0 (all fixtures validate, both pin probes verified)Summary by cubic
Rejects fixture capacities above the schema's 1,000,000 ceiling at load. The loader previously ranged only the config duration fields, so a fixture with
dedupeCapacity: 2147483647passed validation and could OOM the Go tier-2 test process when run without the JSON-schema gate in front.schema.jsonback and cross-checks every loader-enforced bound (durations, capacities,advance.ms,assertDelayMsedges) against the loader's constants, so they cannot drift apart silently.Written for commit 5cd9532. Summary will update on new commits.