codex/bound-transcode-segments
1
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
90f5d117c8 |
feat(events): let websocket clients declare channels on connect (#525)
* feat(events): let websocket clients declare channels on connect Observing the events hub over `/api/v1/events/ws` costs more than it should. A read-only consumer has to send a `subscribe` frame within five seconds or be closed with a policy violation, which means implementing the handshake and holding the write half of the socket open purely to satisfy it. That cost is contract, not transport: `subscribe` is the only inbound message this endpoint accepts. Accept the selection on the URL instead. `?channels=catalog,user_state` subscribes on connect, answers with the same `subscribed` frame and per-channel snapshots the handshake produces, and is never put on the grace-period clock. A connection that declares nothing is unchanged — it still owes a subscribe frame within five seconds. Two supporting changes: - Channel selection now resolves through one shared function used by both paths, so the URL and handshake cannot drift on who may subscribe to what. Role, profile-binding, and validity checks are unchanged. - An unrecognized channel name is reported in the existing `rejected` array as `unknown_channel` rather than closing the connection. Closing took down every other channel the client held over one bad name, and a client cannot always know which channels its role allows before asking. Forbidden and profile-scoped channels were already handled this way. `required_action` in the hello frame is `"none"` for a declared connection and `"subscribe"` otherwise; the web type is widened to match. No wire field changes type or disappears, so this stays additive under the v1 rules. Part of #523 AI disclosure: tool Claude Code, model claude-opus-5, fully AI-generated, reviewed and verified by the author before submission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(events): address review findings on declared-channel subscriptions Three findings from automated review, all verified against the code before acting on them. **Start the reader before declared-channel snapshots (regression).** configureWebSocket installs an absolute read deadline that only pongs extend, and gorilla processes pongs solely inside ReadMessage (conn.go:950, reached only via advanceFrame). The declared path built its snapshots before starting the reader goroutine, so a snapshot slower than the deadline — a loaded jobs/sessions/scans/history query — would kill an otherwise healthy connection the instant reading began. The handshake path never had this problem because its snapshots run downstream of an active reader. Regression test stalls a tasks snapshot past the deadline; it fails with the previous ordering. **Advertise the feature through a capability endpoint.** Adding a client-visible subscription mode without one leaves a read-only client unable to tell, before connecting, whether ?channels= will be honored: an older server ignores it and closes the connection after the grace period, so the client must retain the very handshake this removes. GET /api/v1/events/capability reports both modes, the grace period the handler actually enforces, and the known channels, following the existing per-subsystem convention. **Deduplicate rejections, not just acceptances.** Asking twice for one forbidden channel produced two identical `rejected` entries. Pre-existing — the dedup check sat after the rejection branches — but cheap to correct in the function this PR extracted. Part of #523 AI disclosure: tool Claude Code, model claude-opus-5, fully AI-generated, reviewed and verified by the author before submission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(events): frame the capability endpoint as a staleness probe Clients are expected to run a current build rather than negotiate down to an old server, so the endpoint is not a branch-on-capability contract. Its value is letting a client distinguish "this server does not do that" from "the connection failed" — the two are indistinguishable from the socket alone, since an older server ignores ?channels= and then closes on the grace period — so it can tell the user the deployment is out of date instead of failing opaquely. Comment-only; no behavior or wire change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(events): bound the subscribed answer and reap unsubscribed connections Second review pass on the declared-channel path. Four issues, all at the edges of the new URL surface rather than in the design itself. Rejections amplified the request. Making an unknown channel non-fatal removed the brake that used to close the connection on the first bad name, and every refusal quotes the name it refuses — so a large ?channels= of distinct garbage produced a far larger `subscribed` frame, buffered server-side. Cap a selection at 32 distinct channels, report the overrun once instead of per name, truncate an echoed name at 64 bytes, and set a 64 KiB read limit on the socket so an oversize frame cannot be buffered whole before it is rejected. The grace period was disarmed by declaring, not by subscribing. Both `?channels=` with no names and a non-admin naming only an admin channel came up subscribed to nothing and were never reaped, each holding a hub subscriber, two goroutines, and an envelope channel that every published event fans into. Disarm on holding a subscription instead. Selection now resolves before the hello frame — it is pure, so nothing moves ahead of the reader — which lets required_action say "subscribe" when the connection really does still owe one. Repeating the parameter dropped channels silently. `?channels=a&channels=b` honored only the first and reported nothing rejected. Read every occurrence. The capability endpoint advertised `plugins`, which is host-to-plugin runtime dispatch and is granted to no role. An admin following the endpoint's stated purpose got `forbidden` while already being admin, and the hardcoded "Admin access required" made it a dead end rather than a soft failure. Split evt.ClientChannels out of AllChannels, advertise that, and word the refusal so it does not promise a remedy that does not exist. A test pins that an admin can subscribe to everything the endpoint names. Each guard was verified to bite by reverting it and watching the test fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |