Event feed conformance: three corrections to what tier 2 claims (3/3) - #778
Event feed conformance: three corrections to what tier 2 claims (3/3)#778jeremy wants to merge 27 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Tightens tier-2 event-feed conformance claims and closes a streaming deduplication coverage gap.
Changes:
- Clarifies redirect coverage at the poll seam.
- Rejects scheduling-dependent virtual-time advances.
- Adds fixture 31 for lower-ID live stragglers.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
go/pkg/basecamp/eventfeed/scenario_selftest_test.go |
Tests the new advance guard. |
go/pkg/basecamp/eventfeed/scenario_conformance_test.go |
Implements guarded virtual-time advancement. |
conformance/event-feed/README.md |
Revises tier-2 guarantees and fixture matrix. |
conformance/event-feed/fixtures/31-post-snapshot-straggler-below-served-id.json |
Adds streaming dedupe coverage. |
conformance/event-feed/fixtures/30-continuation-redirect-cross-origin.json |
Corrects the redirect fixture description. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
039aad6 to
4c8446c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
go/pkg/basecamp/eventfeed/scenario_conformance_test.go:494
- This observes only the net counts per timer name, not whether
NewTimerran. A timer can fire and be rearmed under the same name beforesettlesamples it (the connector does same-name rearming, e.g.repair-pollatcatchup.go:852), leaving the map equal tobeforeand allowing the scheduling-dependent advance. Conversely, removal alone is labeled as an arm. Track a monotonic timer-arm/firing generation infeedtest.Clockand add a same-name-rearm self-test so this guard detects activity independently of the snapshot schedule.
before := timerCounts(d.h.clock)
armed := false
d.h.clock.AdvanceSettling(millis(step.Ms), func() {
if !maps.Equal(timerCounts(d.h.clock), before) {
armed = true
conformance/event-feed/README.md:360
- The sentinel obligation is still present verbatim in the shared schema (
schema.json:906), and the fixture inventory at README line 239 still presents “zero foreign egress” without the seam qualification. As a result, the merged contract continues to make the exact tier-2 claim this PR says is withdrawn. Remove the sentinel requirement and qualify the inventory/schema text as driver construction rather than connector proof.
An earlier revision of this row claimed a harness obligation to "bind the
foreign origin to a sentinel listener whose any-request fails the scenario".
That is withdrawn. No implementation met it, and meeting it would prove
nothing: the foreign origin is unreachable **by construction of the harness**,
conformance/event-feed/README.md:131
- The shared schema still defines
advanceas firing timers armed inside the window (schema.json:1208) and does not mention this mandatory rejection. Drivers generated from or validated against that contract are therefore still instructed to implement the behavior this paragraph makes unscriptable. Update the schema description alongside the README so the cross-language contract is consistent.
Every driver must therefore FAIL an `advance` during which the connector arms
anything, naming `fireTimer` as the deterministic alternative — it fires one
named timer without moving the clock, so no re-selection is involved. This is
3eaf48a to
cf1d06b
Compare
4c8446c to
cfd9b3d
Compare
cfd9b3d to
be5a1c7
Compare
Round summary — 3 findings, 3 fixed, 0 openSwept all three surfaces. Both review threads answered and resolved; all three suppressed comments from the round on head are addressed, since two of them carried findings no thread did.
The one that matteredFinding 1 was a test that could not fail, and the review's sharper half was in the suppressed block rather than the thread. Proven by mutation rather than argued: with the connector's Findings 2 and 3 are one mechanism — a correction that reached the README but not Not absorbed here: #789 (arrival-strict matching)#789 is the same class as finding 1 — "the driver cannot fail the contract it enforces" — and it is now the third known instance. I am deliberately not fixing it in this PR, and the reason is mechanism, not appetite. Finding 1's remedy is a per-step verdict read from an instrument the clock already had: additive, locally provable, no new concurrency. #789's remedy is a harness-wide state machine — an active-step tracked under So it should land as its own change on top of this stack. #705 made the same call for the same reason, which is why it recorded rather than patched. One piece of design input I do owe #789, posted there: this PR's finding is evidence about the shape of the remedy. Stack / rebaseRebased twice —
Gates
No finding in this round argued from bc3 server behaviour. I re-checked anyway, since the pins moved today: between |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
go/pkg/basecamp/eventfeed/scenario_selftest_test.go:365
- This control makes the new guard timing-dependent. After
Advancedelivers the due timer, the connector handles it on another goroutine; the driver only pollsArmCountuntilscenarioWatchdogexpires (scenario_conformance_test.go:508-519). If that goroutine is delayed past the watchdog, the scenario passes and can arm a timer afteradvancereturns—the exact cross-language divergence this rule is meant to eliminate. A timeout cannot prove that no arm is coming; either reject everyadvancethat fires a timer (while still allowing truly quiet windows), or add an explicit synchronous completion protocol.
{"expectConnect":{"url":"{{CABLE_URL:2}}"}}],
"finally":{"state":"awaiting_welcome"}}`
be5a1c7 to
81414d7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
conformance/event-feed/README.md:356
- Adding row 16 leaves the matrix summary inaccurate in two ways: the heading still says “fifteen,” and the introductory sentence says every listed mutation is killed even though row 15 is now explicitly only partial/below-seam. Update the heading and introduction so this conformance document does not continue overstating the suite’s proof.
| 15 | `follow-cross-origin-redirect` (follows a 302 to a foreign Location) | 30 — **partially**, and the boundary is below the seam. See the note under this table. |
| 16 | `discard-live-id-at-or-below-served-id` (streaming lane orders live ids against the highest poll-served id) | 31 — and 31 alone: verified to pass all of 01–30, because every other straggler either arrives with nothing yet served (20) or is buffered pre-cut (01, 12, 19) |
Stopping on the advance guard — third round, one classThe suppressed comment on the latest review ( The three rounds
Rounds 2 and 3 are not the same bug, but they are the same class: the driver cannot observe the quantity the rule names. Round 2 observed the wrong quantity; round 3 cannot observe the right one at all. A fourth selector on the same wait would be the treadmill. The real questionThe rule says: reject an advance during which the connector arms a timer. That predicate is decidable in a single-threaded driver and undecidable in a concurrent one — you can only ever conclude "nothing armed within N seconds". Which means the current rule has the disease it was written to cure. A rule that TypeScript decides exactly and Go decides by waiting is itself a cross-language divergence: the same fixture can be admitted in one language and rejected in another. So the question is not "how long should the wait be" — it is "what predicate can every driver decide identically and synchronously?" The candidate answer, and the measurement that makes it actionableCopilot's first alternative is the sound one: key the rule on FIRING, not arming. Every driver decides "did this advance fire a timer" synchronously, inside its own advance, on its own thread, with no waiting and no observation of the connector at all. It is sound as a conservative over-approximation because the reentrant divergence requires a firing to trigger it: no firing in the window ⇒ nothing can be armed in response ⇒ no divergence, provably, with no wait. It is strictly coarser than the true predicate, and the coarseness is the price of decidability. Its cost is that it forbids a shape currently legal — an advance whose window expires a timer that is not replaced. I measured that cost against the merged suite and it is zero. Fixture 05 is the only Both timers sit beyond the window, exactly as the fixture intends. Why I am not making the change hereIt rewrites the normative rule in What is shipping meanwhile, stated plainlyThe guard as it stands is conservative in the safe direction. Related: #789 is the same family — "the tier-2 driver cannot fail the contract it enforces" — and this is a further instance of its root shape, a predicate the driver cannot actually observe. If the firing-based rule is accepted, it should land with #789's work rather than as a fourth pass here. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (5)
conformance/event-feed/README.md:358
- Calling this a “partial kill” contradicts the next sentence, which says the redirect-follow mutation lives below the seam and no tier-2 harness can reach it. Describe it as unreachable/not killed so the explanatory note agrees with the mutation matrix.
**Row 15 is the family's one partial kill, and the reason is structural.** In
conformance/event-feed/README.md:355
- The table still says fixture 30 partially kills
follow-cross-origin-redirect, but the note below and the PR description establish that this mutation is below the poll seam and cannot be reached at tier 2. In a mutation kill matrix, an unreachable mutant is not partially killed; fixture 30 instead covers separate verdict-handling failures above the seam. Mark this row as not killed at tier 2.
This issue also appears on line 358 of the same file.
| 15 | `follow-cross-origin-redirect` (follows a 302 to a foreign Location) | 30 — **partially**, and the boundary is below the seam. See the note under this table. |
conformance/event-feed/README.md:366
- Fixture 30 cannot detect Location over-echoing. The driver reduces the scripted
LocationtoCanonicalOriginand supplies a generic safe cause (scenario_conformance_test.go:1065-1081), while this fixture asserts only the terminal reason. A connector that echoed all data it received would therefore still pass. Remove this kill claim; the separate hostile-cause unit test owns the redaction proof.
and no tier-2 harness can reach it. What fixture 30 does kill is the half above
the seam: a connector that mishandles the verdict — retrying it, classifying it
as anything but Terminal(`invalid_continuation`), or echoing more of the
`Location` than its origin — diverges on `finally` and fails.
conformance/event-feed/fixtures/30-continuation-redirect-cross-origin.json:3
- The fixture description claims it kills Location over-echoing, but the driver strips the Location to its origin before constructing
PollErrorand the fixture asserts only the terminal reason. Thus an implementation that unsafely preserves its entire seam input still passes this scenario. Remove the redaction claim or extend the contract and driver with an assertion that exposes a secret-bearing cause.
"description": "A VALIDATED same-origin `next` answers 302 with a cross-origin Location: the poll seam suppresses automatic redirect-following, and the foreign Location is Terminal(invalid_continuation) with zero egress to the foreign origin. {{NEXT:1}} substitutes same-origin, so the pre-poll validation PASSES and the second poll seam call is made (contrast fixture 26, where no request reaches the URL at all) — the redirect answer is where the per-hop rule bites. The Location host is literal, never substituted, and never served. NOTE: at tier 2 the poll lane is a seam, so the driver forms the redirect-refused verdict and the connector never sees a Location header \u2014 this fixture kills the half above the seam (mishandling the verdict: retrying it, misclassifying it, or echoing more of the Location than its origin), not follow-cross-origin-redirect itself, which lives below the seam and is proven by the Layer-1 adapter's own 302 test. See the row-15 note in README.md.",
conformance/event-feed/schema.json:906
- This contract description still overclaims redaction coverage. The tier-2 driver passes the connector only the canonical origin plus
errors.New("poll refused a redirect"), and fixture 30 checks onlyinvalid_continuation; no path/query text reaches the connector for the fixture to prove it is not echoed. Remove that item from the pinned behaviors.
"description": "Redirect on a continuation (fixture 30): the seam suppresses automatic following; a cross-origin/downgraded Location is Terminal(invalid_continuation). At tier 2 the poll lane is a SEAM, so the driver forms the redirect-refused verdict and the connector never sees a Location header: this pins the half above the seam (mishandling the verdict — retrying it, misclassifying it, or echoing more of the Location than its origin), not redirect-following itself, which lives below the seam and is proven by the Layer-1 adapter's own 302 test. No sentinel-listener obligation is imposed on the harness: the foreign origin is unreachable by construction of the harness, so a silent sentinel would be a statement about the driver rather than about the connector. See the row-15 note in README.md.",
Round 3's second suppressed finding — fixed in
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
SPEC.md:2855
- This enumeration does not bound the connector's total retained payload.
PollSource.Pollcan return an uncappedPollPage.Eventsslice (go/pkg/basecamp/eventfeed/seams.go:117-137), andwalkretains that whole page while delivering it (catchup.go:294-308); those events are neither cable frames bounded byMAX_FRAME_BYTESnor counted in this formula. Either scope this explicitly to cable-frame/live-buffer retention, or add a poll-page/body cap and include that holder before publishing it as the worst-case connector-memory ceiling.
bounded multiplicatively — every retained item is itself bounded by
`EVENT_FEED_MAX_FRAME_BYTES`, and retention is an enumeration by HOLDER, which is what
closes the count: a frame lives in the hand-off queue (≤ pump depth), in the live
buffer (≤ `EVENT_FEED_LIVE_BUFFER_CAPACITY`), in the single deferral slot (≤ 1), or in
the hands of one of the exactly two goroutines that touch frames — one in-hand frame
spec/tracking-issues.yml:50
- The registry still omits active #578 ownership promises in
typescript/tests/services/todolists.test.ts:214and the three shared merge-safe modules (ruby/.../merge_safe.rb:35,python/.../_merge_safe.py:30, andtypescript/.../merge-safe.ts:30). These non-canonical forms are explicitly outside the scanner, so without rows they can remain after the registered sites are removed and #578 will no longer be checked. Register each file as required by this file's one-entry-per-claim contract.
file: "typescript/src/services/todolists-extensions.ts"
Copilot's third round: one detector gap, one SPEC overclaim, four more unregistered sites. All three taken. * The sweep's comment leader is matched by CLASS, not by count. `//` consumed exactly two slashes, so a Swift/Kotlin doc comment's third left `/ separately in #N` unmatchable and read the wrapped promise as absence — the same failure shape the wrap tolerance exists to close, one mark deeper. Now `#+`, two-or-more slashes, `*+`, and `--+` (with `#+` still refusing a following digit, so an issue reference is never eaten as a leader). Red-proven: the new Swift `///` corpus case was run against the previous detector first and failed with "expected FAILURE but checker passed"; all 23 cases pass after the widening. * The retention formula is scoped to what it counts: the CABLE lane. It was published as "worst-case connector memory", but every counted item is a socket frame or a buffered live event, while `PollSource.Poll` returns one page decoded whole and the walk retains that page until its rows are delivered — `PollPage.Events` carries no cap in the seam contract, §23, or the pagination sections (page size is the server's decision, which `EVENT_FEED_MAX_FRAME_BYTES` does not govern). The honest statement ships instead of an invented cap: the formula is the cable lane's bound; the poll lane holds at most one live page per walk by shape (sequential polls; a superseded attempt's in-flight poll may briefly hold another before its result is discarded), unbounded in size by this contract on purpose. The three Go comment mirrors and both test comments now say "cable lane" too. * The four named #578 sites are registered (typescript/tests todolists.test.ts, ruby merge_safe.rb, python _merge_safe.py, typescript merge-safe.ts) — and, three rounds being the signal that the set had to be closed rather than chased, the whole tree was enumerated for the three issues the registry knows: git grep -nI -e '#578' -e '#589' -e '#775' -- \ ':!spec/api-gaps' ':!spec/tracking-issues.yml' \ ':!scripts/test-check-known-defect-issues-open' 17 mention lines; after registering the three additional #775 sites it surfaced (kotlin Pagination.kt and PaginationTest.kt — "#775 carries the six-SDK table" — and typescript errors.ts), the residual is ZERO unregistered [file, issue] pairs. A fourth round on these three issues has nothing left to find; the registry holds 15 entries and the gate verifies every one. Gates: tracking-issues self-test 23/23; the live checker sweeps clean and verifies all 15 registered claims (#589, #775, #578 open); doc-constants-check green under LC_ALL=C; both eventfeed Go packages green.
|
Covering note for the two suppressed findings in Copilot's review of
|
…ent-feed-conformance-driver * origin/event-feed-go-connector: Event feed: verdicts that lost coin flips, and a driver blind to eras Fix the filters-clone comment that stated the opposite of the code Event feed: a policy reason never echoes what the server wrote Event feed: wakes are not verdicts, and three more places the order was the bug Make the fake transport honour the oversize sentinel it mirrors Police the endpoints a discovered issuer names, not only the issuer (#810) Refuse redirects on the signed download hop in every SDK (#809) Event feed: the oversize verdict survived the seam but not the sanitizer Event feed: close the rendering-cap class, type the oversize rejection Event feed: cancellation could not reach the one exit, and the origin was checked after the mangle Event feed: own the ledger's bytes, cap the dial rendering, test the store fake Quiet known-noise CodeQL alerts without losing coverage (#807) Judge the advertised OAuth issuer's address, not just its spelling (#804) Let Go's raw GET retry loop see the Retry-After it already parses (#796) Deflake three tests that raced a wall clock, and gate the class that produced two of them (#794) SPEC §6: decide which statuses honour Retry-After, and how each loop composes it (#793) Pin the conformance runners' fixture reads, and give CI a leg that can see them break (#791) Report an anonymous embed the timestamp walk cannot resolve, instead of skipping it (#790)
Merging origin/event-feed-go-connector brought SPEC §6's Retry-After decision (#793) and its follow-on text, and the discovery sweep caught the first consequence on its first run: SPEC.md:640 now says "Python and Ruby still truncate, tracked in #799" — a canonical claim with no registry entry. That is the gate working across a merge, not a defect. Reconciled against the merged tree: * Seven new entries for the promises the merge introduced: #799 (SPEC.md, MIGRATING.md, go/pkg/basecamp/client.go, client_retry_after_test.go — the rounding/over-range halves of the divergence), #798 (SPEC.md, MIGRATING.md — the generated parser stays Atoi until its template change lands), and one #775 the audit surfaced in client.go:704 (status-set widening is #775's). * Every pre-merge entry stays valid: each registered file still carries live sentences for its issue (SPEC.md's #775 sentences moved but did not go; MIGRATING.md:252 likewise). Audit on the merged tree, per the round-3 protocol extended to the two new issues: git grep -nI -e '#578' -e '#589' -e '#775' -e '#798' -e '#799' -- \ ':!spec/api-gaps' ':!spec/tracking-issues.yml' \ ':!scripts/test-check-known-defect-issues-open' 33 mention lines, residual ZERO unregistered [file, issue] pairs. Gates on the merged tree: both eventfeed Go packages green, plain and -race (the era-scoped occupancy scan and the AdvanceIfQuiet advance merged without conflict — disjoint hunks in the driver); tracking self-test 23/23; the live checker sweeps clean and verifies all 22 entries (#578, #589, #775, #798, #799 all open); doc-constants-check green under LC_ALL=C.
|
Stack note: merged the moved base (event-feed-go-connector at 86ec2cd) — zero conflicts; the anticipated driver-file collision never materialized (the era scan and the AdvanceIfQuiet advance are disjoint hunks, both verified present). The merge also gave the new tracking detector its first live catch: #793's rewrite of SPEC §6 introduced a canonical "tracked in #799" promise the registry did not carry, and the live checker failed closed on it. Reconciled in cd68f23 — seven new registry rows (#799 ×4, #798 ×2, one more #775 the audit surfaced), all verified open; the machine cross-check reports zero unregistered pairs across all five issues (33 mention lines). |
…ent-feed-conformance-driver * origin/event-feed-go-connector: Event feed: the parser cannot say zero, and my throttled fix forgot to ask it Event feed: the redactor renders configured text or a placeholder, never the server's
…ss claim
Copilot's fourth round: four findings, all taken.
* DueWithin's doc claimed "exactly the set an Advance(d) would fire",
and Advance fires more than that: it re-evaluates after each fire, so
a timer armed reentrantly by a firing's recipient fires inside the
window without ever appearing in DueWithin's answer —
TestClock_AdvanceFiresATimerArmedByAFiringsRecipient demonstrates it.
The doc now names what is actually read: the INITIAL due set, a floor
when non-empty and exact when empty (no first firing means no
reentrant arm), which is the asymmetry the advance guard stands on.
AdvanceIfQuiet's return doc carried the same overclaim one step
removed ("the due set") and now says "the initially due set" with the
same reasoning.
* The registry's `site` and `file` were validated through to_s, which
coerces [] and {} and 123 into non-empty text — a row with no
human-readable location reached the issue check, and a non-String
`file` could never match the sweep's [file, issue] pair: a promise
registered into a shape nothing reads. Both now require a non-blank
String, with the offending value named in the error. Red-proven: the
two new self-test cases were run against the previous checker first
and both failed with "expected FAILURE but checker passed"; all 25
cases pass after the fix.
* §23's zero-egress paragraph led with "Zero egress to the foreign
origin is a Layer-1 obligation, not tier-2 coverage" — a subject that
swept all three hostile-URL cases while its justification described
only the redirect. Three review rounds read it the same wrong way,
which makes the sentence the defect regardless of the defense: the
family README (placeholder rule, rows 26/27) affirmatively requires
tier-2 zero-request coverage for connector-visible hostile targets,
and tier 2 does own it — a connector that follows one hands the URL
to the poll seam, which the driver observes and fails. The paragraph
now splits at the seam: fixtures 26/27's structural zero-request
coverage is tier 2's, and only the redirect Location — which the
driver reduces to an origin before the connector sees anything — is
the Layer-1 obligation, recorded as pending in the row-15 note. This
narrows the earlier decline of the same finding: the operative
artifacts were never in conflict, but a sentence that needs this much
defending three rounds running earns the rewrite.
Gates: tracking-issues self-test 25/25; the live checker sweeps clean
and verifies all 22 entries (five issues open); doc-constants-check
green under LC_ALL=C; both eventfeed Go packages green; gofmt clean.
…ent-feed-conformance-driver * origin/event-feed-go-connector: Event feed: the teardown's courtesy call, and three more places Close lost to an announcement
…ent-feed-conformance-driver * origin/event-feed-go-connector: Fix three comments still stating rules the code has since outgrown
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
spec/tracking-issues.yml:96
- The registry still omits an active ownership promise:
conformance/tests/retry.json:73says “#780 tracks giving the harness a clock so this fixture can cover it.” That is exactly the non-canonical phrasing the checker says must be registered by hand. As written, closing #780 would leave this promise stale while the new gate stays green. Add the fixture’s[file, issue]entry.
site: "MIGRATING.md — the generated copy of the parse is untouched pending #798"
Copilot's fifth round: two findings, both taken. * The by-holder ceiling missed the codec's copy. parseFrame unmarshals a message frame's payload into a json.RawMessage — a copy — while the original bytes are still in hand, so the state machine's in-hand term briefly costs two frame-sized allocations, not one, for a near-limit message. Verified at head: all four parseFrame call sites are state-machine side (catchup.go, loop.go dispatch), so the pump's term is untouched and the copy is not a fourth party — it is a WEIGHT on a holder the enumeration already counts, which is why the closure argument survives: each holder is now counted at its worst-case weight. The ceiling becomes (pump depth + 4 + EVENT_FEED_LIVE_BUFFER_CAPACITY) × MAX_FRAME_BYTES, the +4 being the deferral slot, the pump's in-hand frame, and the state machine's in-hand frame at its decode-time weight of two — the copy transient and bounded by one extra frame. The catchup.go mirror and both test comments carry the same figure. * The gate's "no-op when the allowlist references no issues, which is today's state" rationale predated the seeded registry: every run now makes live lookups regardless of the allowlist. The script header, the Makefile's test-target comment, and the workflow step comment now say what is true — a green live run proves only that today's referenced issues are open, and the offline self-test is what covers the failure branches (closed issues, malformed rows, the fail-closed paths) a green run never exercises. Gates: both eventfeed Go packages green, gofmt clean; tracking-issues self-test 25/25; the live checker sweeps clean and verifies all 22 entries (five issues open); doc-constants-check green under LC_ALL=C.
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
| holds one frame; the state machine's weighs up to TWO frame-sized allocations while a | ||
| message frame is decoded, because `parseFrame` unmarshals the payload into a | ||
| `json.RawMessage` — a copy — while the original bytes are still in hand. The ceiling | ||
| is | ||
| (pump depth + 4 + `EVENT_FEED_LIVE_BUFFER_CAPACITY`) × `EVENT_FEED_MAX_FRAME_BYTES` |
| # or dropped in an edit reads as nil and used to pass as empty — the same | ||
| # fail-open path as a deleted file, one typo later. Only the explicit empty | ||
| # list says "no promises" on purpose. | ||
| unless tracking.key?('prose_tracking_issues') |
…ent-feed-conformance-driver * origin/event-feed-go-connector: Annotate resolveStorePath's deliberate nilerr fall-through Annotate the symlink walk's deliberate error swallow for the linter Event feed: save through a symlinked store path instead of replacing it Event feed: an invalid frame limit fails closed, not open into no limit
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
SPEC.md:3374
- The revised ceiling still misses one frame-sized allocation.
parseFrameretains the original frame plus itsjson.RawMessagecopy, anddecodeMessageEventthen unmarshalskind,event_type, andactioninto separately allocated strings while both remain live (cable.go:261-269,311-356). Those decoded strings can collectively approach the frame limit, so the state-machine holder peaks at three frame-sized allocations and the formula needs+ 5, not+ 4; please update this explanation and its Go mirrors together.
holds one frame; the state machine's weighs up to TWO frame-sized allocations while a
message frame is decoded, because `parseFrame` unmarshals the payload into a
`json.RawMessage` — a copy — while the original bytes are still in hand. The ceiling
is
(pump depth + 4 + `EVENT_FEED_LIVE_BUFFER_CAPACITY`) × `EVENT_FEED_MAX_FRAME_BYTES`
| // the pump has read and not yet handed off, and the frame the scan itself has | ||
| // dequeued and not yet disposed of — the very receive that lets a blocked | ||
| // pump refill the queue behind it, counted at TWO, its decode-time weight, | ||
| // because parseFrame's json.RawMessage payload copy briefly coexists with the | ||
| // original bytes. All are retained WHILE the queue is full, which is why they |
| // buffer's memory ceiling. SPEC.md §23 publishes the cable lane's worst case | ||
| // as (pump depth + 4 + EVENT_FEED_LIVE_BUFFER_CAPACITY) × EVENT_FEED_MAX_FRAME_BYTES; |
| // is a bound on events HELD AT ONCE — SPEC §23 sizes the cable lane's whole | ||
| // memory ceiling off it, "(pump depth + 4 + EVENT_FEED_LIVE_BUFFER_CAPACITY) × | ||
| // EVENT_FEED_MAX_FRAME_BYTES" — so a drain must not be able to hold a batch |
Third of the split. Stacked on #705, which is stacked on #777 — review after
those. Three corrections to the tier-2 conformance family, all of them about the
suite claiming more than it proves.
1. Row 15's kill claim is partial, and the sentinel obligation is withdrawn
The mutation table said fixture 30 kills
follow-cross-origin-redirectpartlythrough "the harness obligation that the fixture's foreign origin is bound to a
sentinel listener whose any-request fails the scenario". Fixture 30's own
description said the same.
No implementation meets that obligation, and meeting it would prove nothing.
At tier 2 the poll lane is a seam: the driver receives the fixture's scripted
302 and hands the connector an already-formed redirect-refused verdict. The Go
driver says so itself — "the driver is the seam, so the foreign origin is
unreachable by construction". The connector never sees a
Locationheader andnever decides whether to follow one, so a silent sentinel is a statement about
the driver, not the connector.
That makes the mutation unreachable at this tier — it lives below the seam.
What fixture 30 does kill is the half above it: mishandling the verdict by
retrying it, misclassifying it, or echoing more of the
Locationthan itsorigin. Real zero-egress proof is the Layer-1 adapter's own 302 test, tracked
for G1b.
The fixture's description is amended too. Fixtures are merged contract and are
not edited lightly, but leaving the identical false claim in the file the other
five SDKs read would make this a half-correction. Prose only — no assertion
changed, both pin probes still verify.
This also corrects #705's own kill-matrix, which inherited the claim.
2. An advance that arms a timer is rejected, in every driver
The driver ran
advancethrough the plain clockAdvance, with a commentnoting that a fixture wanting a chained firing "would pass
AdvanceSettlingtherendezvous". That is an opt-in, and the next fixture author is exactly who would
not take it.
Stress-testing the settle killed it, for reasons that are fundamental rather
than effort:
does not consume a staleness firing promptly — a window closing during a
delivery is latched and observed later, which is what §23 requires. A clock
that waited would hang against the behavior the spec mandates.
distinguishes "has not armed yet" from "will not arm".
So the reentrant clause is not settleable where the connector runs
concurrently, and the divergence is made unscriptable instead: an advance
during which the connector arms anything now fails, naming
fireTimeras thedeterministic alternative. Unconditional — a schema field would let an author
take the divergence rather than avoid it.
The family README carries the constraint, because it binds all six SDKs and they
are split across single-threaded and concurrent test clocks.
Self-tested both ways: the mutant (an advance across the handshake deadline,
which arms
backoffinside the window) is rejected, and the control (a quietwindow) still passes — without which the guard would be rejecting every advance,
including fixture 05's.
3. Fixture 31 — a straggler below the entry page's served id
Closes the hole where a dedupe ordering live ids against the highest poll-served
id passes the whole suite.
Fixture 20 already proves a post-snapshot straggler is delivered, but its entry
page serves no events, so a highest-served-id implementation passes it too:
with nothing served the mark is unset and every id clears it. Every other
straggler in the suite is buffered pre-cut.
31 serves id 99 on the entry page and pushes 41 afterwards, in the streaming
lane. 41 must be delivered — dedupe tracks actually-delivered ids and 41 was
never served by poll — while a re-push of 99 is still suppressed, which keeps
this a dedupe fixture rather than a no-dedupe one.
The kill was verified, and the first two attempts were wrong in instructive
ways. A mutant in
admitLivesurvived — that is the buffer-admission path, anda streaming straggler never goes through it. A mutant in the shared
deliver()gate failed six fixtures, so it was broader than the hole and would not have
justified a new fixture. The mutant that matches the claim — the ordering applied
only to the streaming lane — passes all of 01–30 and is killed by 31 alone.
Verification
go build/go vet/go test -race -count=1pass; 23/23 fixtures green;5 driver self-test groups pass;
make event-feed-fixtures-checkclean with bothpin probes verified;
make go-lint0 issues.Summary by cubic
Aligns tier‑2 event‑feed conformance with what the suite actually proves and fixes the cable‑lane memory ceiling. Blocks scheduling‑dependent virtual‑time advances, narrows row‑15’s redirect claim to above‑seam behavior, adds a live‑lane dedupe fixture, gates “Tracked in #N” promises behind a fail‑closed registry/detector, and hardens storage/limits (symlinked store path honored; invalid frame limit now fails closed).
Migration
Written for commit d2b895d. Summary will update on new commits.