Event feed connector: foundations (1/3)

Merged
JE
1mo ago
+11414-325

First half of the SPEC.md §23 event feed connector, split out of #705 so the
state machine can be reviewed on its own. #705 keeps the run loop, catch-up,
recovery and the tier-2 driver, and now stacks on this.

Eight bot rounds on #705 did not converge (12→3→5→2→2→1→3 threads, with late
findings in files no earlier round had touched), and a review pass found a P1
credential defect that all eight missed because it composes two files across a
package boundary. Splitting is the response to that shape.

Size: ~2.7k lines of production code, ~7k with tests.

What is here

Everything the run loop is built from and nothing that runs — each piece
testable without starting a feed:

seams.goTicketMinter / PollSource / CableTransport / CableConn
event.goEvent, Cursor, Page, Signal, Disposition, Observer
errors.goTerminalError and its reason codes
filters.go, digest.gothe filter set, its fail-closed validation, the srv1 key
checkpoint.goCheckpointKey / FlatKey / CanonicalOrigin, the store seam
continuation.gothe §8 same-origin validation algorithm
filestore.gothe built-in FileCheckpointStore
dedupe.go, backoff.go, clock.go, cable.goLRU, reconnect schedule, timers, frame codec
transport.go, websocket_transport.gocable-URL policy, the default transport
feedtest/host-supplied fakes for all four seams

There is no consumer entry point yet: New and the loop are on #705.

The four fixes

1. The cable dial takes no credential it was not given (P1)

The cable origin is chosen by the server — the mint returns a url and the
connector dials it verbatim, cross-host by design — and the short-lived ticket
in its query is the only credential that origin is entitled to. Two paths handed
it more.

WebSocketTransport.HTTPClient, or http.DefaultClient when nil. An
*http.Client carries three credentials invisible at the call site: a
RoundTripper may inject Authorization, a Jar attaches cookies, a
TLSClientConfig may present a client certificate. The DefaultClient fallback
is the same hazard with no call site at all. Deleted rather than validated —
a RoundTripper is opaque, so no runtime inspection could accept one client and
refuse another. Handshakes now run on a package-owned client. (The field had no
callers anywhere, so nothing regressed with it.)

URL userinfo. net/http's send() turns it into a Basic Authorization
header, so a mint whose url carried userinfo made the connector authenticate
to a server-nominated origin with a credential the server chose. Refused before
any network I/O.

The proxy. A wss:// handshake reaches a proxy as CONNECT host:port, so
the ticket stays inside the tunnel; a ws:// handshake is forwarded in absolute
form, putting /cable?ticket=… in the proxy's request line and access log in
the clear. Reachable, not theoretical: §9 admits ws:// for *.localhost, and
net/http's proxy rules exempt the literal localhost and loopback IPs but
not .localhost subdomains. Cleartext dials no longer proxy; TLS dials
still do.

Pre-fix transcript, against un-fixed code:

handshake 0 sent Authorization: "Bearer caller-bearer-token"
handshake 0 sent Proxy-Authorization: "Basic cHJveHk6c2VjcmV0"
handshake 0 sent Cookie: "session=caller-session-cookie"
the server accepted 1 connection(s); the refusal must precede all network I/O

and from the proxy sentinel with the cleartext exemption removed:

a cleartext cable dial reached the proxy 1 time(s):
  ["GET http://app.localhost:9/cable?ticket=sekrit-ticket-value"]

TestCableHTTPClient_IsWiredShut exists because the first mutant written
against the proxy fix survived: Proxy: proxyFromEnvironment captures the
var's value at init, so the behavior tests' sentinel never reached it — meaning
both would pass a regression to Proxy: http.ProxyFromEnvironment. The wiring
assertion holds the shape they cannot observe.

2. A suppressed duplicate is not a delivery

§23 defines the LRU as "actually-delivered event ids", recorded by every
delivery. Seen refreshed recency on a hit, which is the case where the
event is suppressed and no delivery happens. §23 says to expect poll-vs-push
duplication continuously, so a hot id was pinned at the front and evicted ids
delivered once and never seen again — which become eligible for exactly the
re-delivery the LRU prevents.

TestDedupe_HitRefreshesRecency is inverted to ..._HitDoesNotRefreshRecency,
and I am calling that out rather than letting it look like a test edited to
accept a fix. It asserted the negation of the contract; nothing short of
inverting it is honest.

3. The checkpoint store reads a bounded regular file, under one lock

#761: the lock registry was keyed on the exact path spelling. On APFS or
NTFS feed.json and Feed.json are one file, so two stores took two mutexes —
the lost update the registry exists to prevent, reached by two call sites
disagreeing about capitalization. The lock key is now case-folded; the path each
store reads and writes is not.

The read followed whatever the path named. Against the pre-fix read, all four
cases fail:

directory       ... reported "is a directory", not a store-type refusal
FIFO            ... did not return (open blocks until a writer appears)
/dev/zero       ... did not return (reads without end)
8 MiB + 1 file  ... read whole, then failed on a JSON parse error

The FIFO and device cases are hangs, so every assertion runs under a bound. Not
defensive dressing: the first draft bounded only the FIFO, and /dev/zero took
the package's 45s timeout with it, naming nothing.

4. One observer-safe URL redactor

The primitive #705 applies to every URL-bearing observer surface. Reduction is
via CanonicalOrigin rather than truncation at ?, which matters for the case
a naive redactor misses: userinfo is a credential in the authority, so
https://attacker:hunter2@evil.example/steal?ticket=… survives query-stripping
intact and does not survive this.

Verification

Pristine worktree, one pass, clean tree before and after:

  • go build / go vet / go test -race -count=1 / -count=5 — all pass
  • make go-lint — 0 issues
  • gosec -severity high -exclude-dir=pkg/generated on the CI-pinned v2.23.0
    (module hash verified, not a scratchpad binary) — 0 issues
  • full make check — exit 0

Every fix was red-proven before it was written, and every test mutation-checked
after. Three tests were rewritten because mutation showed them vacuous.

One preparatory commit

1646f2c1f relocated the run-coupled declarations out of checkpoint.go and
continuation.go so the two halves fall on file boundaries. The moved function
bodies are byte-identical. It also added direct tests for checkContinuation
and Filters.clone, both of which were only reachable through a full run.

Development history, review threads and proof lineage for every file here are on
#705, preserved at tag pre-split/705-head.

Five more from review

Copilot's rounds on this branch found five further defects; all five are fixed, each
red-proven against the un-fixed code first.

6c6e14f16A null type is not a broadcast. A *string gives the same nil for an absent key and a JSON null, so {"type":null} was liveness-only while {"type":null,"identifier":…,"message":…} was delivered as an event — one wire value in two classes depending on its siblings. Presence is now decoded separately. A present-but-null type takes the ignore branch, not the reject branch: it names no type to recognize, which is §23's unrecognized-type case. BC3's push lane sends {identifier, message} with no type key at all, checked against the current head of bc3 #9659, so the narrowing drops nothing real.
fae998b16Close bounds the read, not just itself. closeGraceBudget stopped Close from waiting out the close handshake, but the socket is what releases a parked read and coder/websocket does not tear it down until its own 5s+5s ends — so a pending ReadFrame stayed blocked four seconds after Close returned. Worse, a background read was uncancellable: the library installs its cancellation hook only when the read context has a Done channel, and ReadFrame(context.Background()) is how a run loop parks a pump. The connection now owns a lifetime context every read and write derives from, cancelled once Close is done waiting — after the budget, never before, so the close frame is still written.
60a28700dThe package docs described the finished connector. There is no Connector, no constructor and no run loop here, and the docs said "runs the whole protocol". Now: foundations only, what has landed, both pending pieces, and that everything below documents the architecture they implement. AGENTS.md's row carried the same overclaim.
791143207The store's write is bound like its read. The read refuses a file past 8 MiB; the write did not, so a Save crossing the cap renamed into place a file nothing can read again — and since Save reads before it writes, Save too. No in-band recovery; the operator must delete the file, discarding every other lineage's cursor. Reaching the cap is accretion, not an adversary: there is no delete, so a filter change leaves the old lineage in the file forever. Refusing degrades to the documented failed-save outcome instead of an unrecoverable one.
b15e8d031Cancellation outranks a local close on entry too. ReadFrame documents the precedence and honored it on the way out but not on the way in, so a cancelled read over a closed connection reported a connection failure — the shutdown a run loop performs. WriteFrame already checked the context first; the two disagreed. The assertion went in the shared transport contract, where the feedtest fake already passed it and the real transport did not.

Two findings were declined on merit with the reasoning in a comment rather than left open, and a third — the symlinked store path versus atomic rename — is flagged for a human call: it is the third round on one file, and every candidate remedy trades away a different documented property of what a store file's identity is.

Summary by cubic

Adds the foundations for SPEC §23’s Go event-feed connector: wire models, seams, codecs, checkpoint storage, the default WebSocket transport, timing and dedupe helpers, and deterministic fakes. This branch still has no constructor or run loop; it provides the pieces for later PRs. The shipped filter contract replaces srv1 with srv2, so existing srv1- checkpoint lineages go cold.

Foundations

  • Defines poll and push events, expanded filters, terminal reasons, continuation validation, backoff, timers, and delivery deduplication.
  • Adds FileCheckpointStore with bounded regular-file I/O, atomic saves, canonical keys, symlink-aware paths, and serialized access.
  • Adds WebSocketTransport over github.com/coder/websocket; proxy egress requires a custom CableTransport.
  • Adds deterministic fakes, shared transport contract tests, and updated conformance schemas and fixtures, including Windows-compatible FIFO coverage.

Contract and safety

  • Uses the published srv2 object digest and vectors, fixed subscription-key ordering, caller-resolved self, and server-owned actor_types vocabulary.
  • Requires exact wire keys and valid UTF-8, rejects duplicate or out-of-range values, and treats malformed checkpoint data as load failure.
  • Keeps credentials and server-controlled URLs, frame contents, close reasons, and dial details out of observer and error renderings.
  • Cancellation, close, and peer failures unblock reads and writes; dead fake connections cannot serve or accept later frames.

Reconciled with the shipped feed contract (54c50caa)

bc3's feed shipped with more than the pre-merge branch this was written against, and the last commit brings the foundations to what the server serves (doc/api/sections/event_feed.md on bc3 master):

  • srv2 digest. A JSON object keyed by present dimension (actor_types, buckets, creators, exclude_performers, performers, reasons, types; bytewise key order; {} for the empty set), first 16 hex of SHA-256. The lineage namespace is srv2-; conformance/event-feed-digest/ carries the doc's eleven vectors, each recomputed independently. srv1 (the positional array) never shipped.
  • Filters gains Performers, ExcludePerformers, ActorTypes and Reasons (the inbox lane's dimension, digested here so its lineage key is well-defined). The subscribe identifier carries the first three after creators. The server's self literal is caller-resolved to an id: the connector performs no wire I/O with which to resolve a principal, and the checkpoint key is the server digest, which is defined over the resolved id (SPEC §23 Consumer Surface says why).
  • Event carries performed_by_id (both lanes, null for a direct action), details (verbatim bytes for the types that publish one — ids keep 64-bit fidelity, new keys survive) and the push-only actor_type. The push decoder requires the shipped eleven-key shape; a shipped frame would otherwise have been refused as an invalid frame on every delivery.
  • PollError carries the 409 body's position_digest / filters_digest.
  • The tier-2 schema and every fixture row take the new keys and dimensions; SPEC §23 is updated normatively for each item.

The inbox lane itself (a separate resource with its own item identity) is a fourth PR stacked on #778; the Layer-1 adapters over the generated operations follow once the spec operations land.