90f5d117c8
* 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>
81 lines
2.9 KiB
Go
81 lines
2.9 KiB
Go
package handlers
|
|
|
|
import (
|
|
"encoding/json"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"testing"
|
|
|
|
evt "github.com/Silo-Server/silo-server/internal/events"
|
|
)
|
|
|
|
func TestEventsCapabilityReportsDeclaredChannelSupport(t *testing.T) {
|
|
handler := &EventsHandler{}
|
|
rec := httptest.NewRecorder()
|
|
handler.HandleCapability(rec, httptest.NewRequest(http.MethodGet, "/events/capability", nil))
|
|
|
|
if rec.Code != http.StatusOK {
|
|
t.Fatalf("status = %d, want 200", rec.Code)
|
|
}
|
|
if ct := rec.Header().Get("Content-Type"); ct != "application/json" {
|
|
t.Errorf("Content-Type = %q, want application/json", ct)
|
|
}
|
|
|
|
var got eventsCapabilityResponse
|
|
if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil {
|
|
t.Fatalf("decoding capability: %v (%s)", err, rec.Body.String())
|
|
}
|
|
|
|
if !got.DeclaredChannels {
|
|
t.Error("declared_channels = false; a client cannot detect ?channels= support")
|
|
}
|
|
if !got.SubscribeFrame {
|
|
t.Error("subscribe_frame = false; the handshake is still supported")
|
|
}
|
|
if got.SchemaVersion != 1 {
|
|
t.Errorf("schema_version = %d, want 1", got.SchemaVersion)
|
|
}
|
|
|
|
// The advertised grace period must be the one the handler actually
|
|
// enforces, or a client will size its handshake timeout against fiction.
|
|
if want := int(subscribeGracePeriod.Seconds()); got.SubscribeGracePeriodSeconds != want {
|
|
t.Errorf("subscribe_grace_period_seconds = %d, want %d", got.SubscribeGracePeriodSeconds, want)
|
|
}
|
|
|
|
if got.MaxRequestedChannels != maxRequestedChannels {
|
|
t.Errorf("max_requested_channels = %d, want %d", got.MaxRequestedChannels, maxRequestedChannels)
|
|
}
|
|
|
|
if len(got.Channels) != len(evt.ClientChannels) {
|
|
t.Errorf("channels = %v, want all %d client channels", got.Channels, len(evt.ClientChannels))
|
|
}
|
|
}
|
|
|
|
// TestEventsCapabilityAdvertisesOnlySubscribableChannels is the point of
|
|
// publishing the list at all: a client that requests everything the endpoint
|
|
// names must not be refused any of it. The plugins channel is the case — it is
|
|
// in evt.AllChannels but granted to no role, admin included, so advertising it
|
|
// would send a client to a request that can only fail.
|
|
func TestEventsCapabilityAdvertisesOnlySubscribableChannels(t *testing.T) {
|
|
handler := &EventsHandler{}
|
|
rec := httptest.NewRecorder()
|
|
handler.HandleCapability(rec, httptest.NewRequest(http.MethodGet, "/events/capability", nil))
|
|
|
|
var got eventsCapabilityResponse
|
|
if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil {
|
|
t.Fatalf("decoding capability: %v (%s)", err, rec.Body.String())
|
|
}
|
|
|
|
// An admin, on a profile-bound connection, is the most permissive caller
|
|
// there is. Every advertised channel must resolve for them.
|
|
_, accepted, rejected := resolveChannelSelection(
|
|
got.Channels, allowedChannelsForRole("admin"), "profile-1")
|
|
|
|
if len(rejected) != 0 {
|
|
t.Errorf("capability advertises channels an admin cannot subscribe to: %v", rejected)
|
|
}
|
|
if len(accepted) != len(got.Channels) {
|
|
t.Errorf("accepted %d of %d advertised channels", len(accepted), len(got.Channels))
|
|
}
|
|
}
|