diff --git a/docs/architecture.md b/docs/architecture.md index ada6bc0d..0ce0b00b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -93,8 +93,9 @@ the distinction matters for anything touching either: | Consumers | the web UI | `pad watch --stream`, the agent monitor | | Auth | a resolved user, a legacy workspace token, or the fresh-install window | a resolved user, always | -They cost the same process resources — a goroutine, a bus subscription, -and (with Redis) a session-presence registration — so they share ONE +They cost the same process resources — a goroutine and a bus subscription +each, plus (with Redis) a session-presence registration for the watch +stream, which is the only one that registers — so they share ONE admission budget, enforced per instance by `internal/server/stream_admission.go` before either subscribes: `PAD_SSE_MAX_CONNECTIONS` and `PAD_SSE_MAX_PER_USER` cover both, diff --git a/docs/deployment.md b/docs/deployment.md index e58e8e16..0a443e30 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -93,9 +93,9 @@ All configuration is via environment variables or a config file (`~/.pad/config. Pad has two SSE endpoints and they share one budget. `/api/v1/events` is workspace-scoped (the web UI's activity stream); `/api/v1/events/stream` is user-scoped (agent watch notifications, `pad watch --stream`). A held -connection costs the same goroutine, subscription and — with Redis — presence -registration whichever one opened it, so `PAD_SSE_MAX_CONNECTIONS` and -`PAD_SSE_MAX_PER_USER` bound them together. Only `PAD_SSE_MAX_PER_WORKSPACE` +connection costs a goroutine and a bus subscription whichever one opened it — +and, on the watch stream only, a session-presence registration in shared Redis — +so `PAD_SSE_MAX_CONNECTIONS` and `PAD_SSE_MAX_PER_USER` bound them together. Only `PAD_SSE_MAX_PER_WORKSPACE` is endpoint-specific, because the watch stream has no workspace to count against. diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index c80a8076..67932548 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -29,9 +29,11 @@ type Metrics struct { // Redis operability metrics (BUG-2727). Wired from // cmd/pad/cmd_server.go and only meaningful on a deployment with - // PAD_REDIS_URL set — a single-process binary never touches Redis, so - // these stay at their zero values there, which is the honest reading - // rather than an absence. + // PAD_REDIS_URL set. On a single-process binary the COUNTERS below + // stay at zero, which is the honest reading — nothing was dropped, + // nothing was missed. pad_redis_up is the exception and is not + // registered at all there, because a zero on a GAUGE named "up" + // asserts something false; see its own comment. // // RedisUp is written by internal/server's health prober, NOT sampled // on scrape: a collector that dials on every scrape turns a monitoring @@ -298,7 +300,7 @@ func New() *Metrics { sessionPresenceFailuresTotal := prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "pad_session_presence_failures_total", - Help: "Failed session-presence operations by op. Failures leave sessions unlisted and untargetable.", + Help: "Failed session-presence operations by op. READ THE LABEL — register/renew leave a live session unlisted and untargetable, deregister leaves a DEAD one listed, list returns 503, prune is benign.", }, []string{"op"}) reg.MustRegister( diff --git a/internal/redisns/redisns.go b/internal/redisns/redisns.go index 419d3299..2c8feb1a 100644 --- a/internal/redisns/redisns.go +++ b/internal/redisns/redisns.go @@ -16,8 +16,13 @@ // none, because an operator rule that holds for two of three keyspaces is // harder to state than the flat one it replaces. This package is that // "at once": one value, built once in cmd/pad/cmd_server.go, passed into -// all three constructors. The three cannot drift because there is nothing -// to drift from. +// all three constructors. +// +// That is a CONVENTION, not a guarantee — each constructor takes its own +// Keys and nothing in the type system stops a future caller passing a +// different one. cmd/pad/redis_keyspace_wiring_test.go is what enforces +// it, and it does so by reading the wiring source, which is a weaker +// instrument than a compiler and is named as such where it lives. // // HAZARD FOR ANYONE SWEEPING THE PREFIX. The string "pad:" also begins // Pad's OAuth SCOPE values — pad:read, pad:write, pad:admin — in @@ -46,6 +51,27 @@ import ( // shared Redis's keyspace is still legible to an operator running KEYS. const prefix = "pad:" +// reservedNames are Pad's own first path segments. A namespace equal to +// one of them nests this installation inside the DEFAULT installation's +// keyspace — "events" being the sharp case, since pad:events:* is the +// activity channel space — which is the collision the namespace exists to +// prevent, reached through the namespace itself. +// +// Kept in sync by hand with the suffixes the three packages pass to Name. +// A drift here is not silent: it costs a namespace that should have been +// rejected, which is exactly the case the reserved list is for. +var reservedNames = map[string]bool{ + "events": true, + "event_seq": true, + "watchevents": true, + "watchevents_seq": true, + "watchevents_epoch": true, + "session": true, + "sessions": true, +} + +const reservedList = "events, event_seq, watchevents, watchevents_seq, watchevents_epoch, session, sessions" + // Keys builds namespaced names. The zero value is Default — today's exact // names — which is what makes an existing deployment's replay buffers, // counters and presence entries survive an upgrade untouched. @@ -73,9 +99,22 @@ var Default = Keys{} // The character set is deliberately narrow — lowercase letters, digits, // hyphen, underscore — because a namespace ends up inside key names, in // Lua KEYS arguments, and in operator-facing logs. A colon is rejected -// specifically: it is Pad's own separator, and allowing one would let a -// namespace forge a key path (ns "a:events" would make pad:a:events: -// collide with installation "a"'s channel). +// specifically: it is Pad's own separator, so a namespace containing one +// spans segments and makes the keyspace ambiguous to anyone (or anything) +// reading KEYS output back. +// +// An earlier version of this comment justified that with a collision +// example that was simply WRONG — it claimed ns "a:events" would produce +// pad:a:events: and collide with installation "a"'s channel, when the +// suffix is appended too and the result is pad:a:events:events:. The +// rule is right; the reasoning was not, and a false example is worse than +// none because the next reader trusts it. +// +// A REAL collision exists and needs no colon at all: reservedNames below +// rejects namespaces equal to Pad's own first path segments. Namespace +// "events" would put every key of this installation inside +// pad:events:*, which is the default installation's activity CHANNEL +// space. func Parse(ns string) (Keys, error) { if ns == "" { return Default, nil @@ -83,6 +122,9 @@ func Parse(ns string) (Keys, error) { if strings.TrimSpace(ns) == "" { return Keys{}, fmt.Errorf("redis namespace %q is whitespace only: leave it unset for the default keyspace, or give it a real name — a blank value would silently share the default keyspace with another installation", ns) } + if reservedNames[ns] { + return Keys{}, fmt.Errorf("redis namespace %q is one of Pad's own key segments (%s): it would place this installation's keys inside the default installation's keyspace", ns, reservedList) + } for _, r := range ns { switch { case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '-', r == '_': diff --git a/internal/redisns/redisns_test.go b/internal/redisns/redisns_test.go index 49ec4659..87b4e69e 100644 --- a/internal/redisns/redisns_test.go +++ b/internal/redisns/redisns_test.go @@ -64,10 +64,10 @@ func TestNamespacedNames(t *testing.T) { func TestParseRejectsNamesThatCouldForgeAKeyPath(t *testing.T) { t.Parallel() - // The colon case is the load-bearing one: Pad's own separator. Without - // it, namespace "a:events" would build pad:a:events: and collide - // with installation "a"'s channel — reintroducing the cross-feed this - // package exists to prevent, through the mechanism meant to fix it. + // A colon is Pad's own separator, so a namespace containing one spans + // segments and makes the keyspace ambiguous to read back. (An earlier + // version of this comment justified the rule with a collision example + // that was wrong — see Parse.) for _, bad := range []string{"a:events", "Staging", "with space", "emoji-🐦", "tab\there", "sub/path", "quote'"} { if _, err := Parse(bad); err == nil { t.Errorf("Parse(%q) accepted an invalid namespace", bad) @@ -90,6 +90,27 @@ func TestParseRejectsNamesThatCouldForgeAKeyPath(t *testing.T) { } } + // The REAL collision the character set cannot catch: a namespace + // equal to one of Pad's own first segments nests this installation + // inside the default one's keyspace. "events" is the sharp case — + // pad:events:* is the default installation's activity channel space, + // so namespace "events" would put every key of this installation + // inside it. + for _, reserved := range []string{"events", "event_seq", "watchevents", "watchevents_seq", "watchevents_epoch", "session", "sessions"} { + if _, err := Parse(reserved); err == nil { + t.Errorf("Parse(%q) accepted a namespace equal to one of Pad's own key segments", reserved) + } + } + + // The control leg: names that merely CONTAIN a reserved word are fine. + // Without it a build that rejected anything containing "session" would + // pass the loop above while refusing perfectly good namespaces. + for _, ok := range []string{"events-eu", "my-events", "sessions2", "prod-session"} { + if _, err := Parse(ok); err != nil { + t.Errorf("Parse(%q) rejected a namespace that only contains a reserved word: %v", ok, err) + } + } + // Only a genuinely UNSET value yields Default. k, err := Parse("") if err != nil { diff --git a/internal/server/session_presence_observer_test.go b/internal/server/session_presence_observer_test.go index 2c5fdca2..e29aedb0 100644 --- a/internal/server/session_presence_observer_test.go +++ b/internal/server/session_presence_observer_test.go @@ -105,9 +105,10 @@ func TestPresenceReportsFailedOps(t *testing.T) { // under-report precisely the case an operator is least likely to notice // any other way — a dead Redis is obvious, a corrupt row is not. // -// Drives real corrupt rows through miniredis rather than calling the -// reporter, and covers BOTH shapes: a non-JSON string, and a value that -// is not a string at all. +// Drives a real corrupt row through miniredis rather than calling the +// reporter. ONE shape, not both: the non-string arm is unreachable +// through MGET and the subtest that would have driven it is absent by +// design — see the note where it would have been. func TestPresenceReportsCorruptEntriesAsListFailures(t *testing.T) { t.Parallel() diff --git a/internal/server/session_presence_redis.go b/internal/server/session_presence_redis.go index b2fa1d3d..c8040b0b 100644 --- a/internal/server/session_presence_redis.go +++ b/internal/server/session_presence_redis.go @@ -707,9 +707,20 @@ func (p *RedisSessionPresence) ListForUser(userID string) ([]LiveSession, error) var expired []string for i, v := range values { if v == nil { - // The session key's TTL lapsed — its process stopped renewing, - // i.e. it died without deregistering. Prune the index member so - // a crashed instance's leftovers don't accumulate. + // The session key is GONE. Usually that means its TTL lapsed + // because the process stopped renewing — it died without + // deregistering — but it is not proof of that: an eviction + // under maxmemory, a Redis restart, or a manual DEL produce + // exactly the same nil, and this type's own doc comment says + // eviction is indistinguishable from expiry. Nothing here + // depends on telling them apart, since the response is the + // same either way: treat the session as gone and prune the + // index member so leftovers do not accumulate. + // + // The pruning itself is CONDITIONAL for precisely this + // reason — a live session whose key was evicted can be + // rewritten by its own renewal between this read and the + // prune. See pruneIndexScript. expired = append(expired, ids[i]) continue } diff --git a/internal/server/stream_admission_test.go b/internal/server/stream_admission_test.go index d70d13c8..b73ed507 100644 --- a/internal/server/stream_admission_test.go +++ b/internal/server/stream_admission_test.go @@ -712,14 +712,28 @@ func TestStreamLimitRefusalContractIsIdenticalOnBothEndpoints(t *testing.T) { closePW := holdAuthedSSE(ctx, t, pwTS.URL, pwSlug, pwTok.Token) defer closePW() + // And the PER-USER bound, which the first two servers disable — round + // 15 caught the "every refusal path" claim omitting it, which is the + // same undercount as round 13's, one round later. + perUserSrv := testServerWithWatchEvents(t) + perUserSrv.SetEventBus(events.New()) + perUserSrv.SetSSELimits(0, 0, 1) // per-user only + puTS := httptest.NewServer(perUserSrv) + defer puTS.Close() + puSlug, _, puTok, _ := setupWatchTestUser(t, perUserSrv) + closePU := holdAuthedSSE(ctx, t, puTS.URL, puSlug, puTok.Token) + defer closePU() + for _, tc := range []struct { name string url string token string }{ - {"workspace stream, admission bound", ts.URL + "/api/v1/events?workspace=" + slug, tok.Token}, - {"watch stream, admission bound", ts.URL + "/api/v1/events/stream", tok.Token}, + {"workspace stream, per-instance bound", ts.URL + "/api/v1/events?workspace=" + slug, tok.Token}, + {"watch stream, per-instance bound", ts.URL + "/api/v1/events/stream", tok.Token}, {"workspace stream, per-workspace bound", pwTS.URL + "/api/v1/events?workspace=" + pwSlug, pwTok.Token}, + {"workspace stream, per-user bound", puTS.URL + "/api/v1/events?workspace=" + puSlug, puTok.Token}, + {"watch stream, per-user bound", puTS.URL + "/api/v1/events/stream", puTok.Token}, } { req, err := http.NewRequest("GET", tc.url, nil) if err != nil { diff --git a/internal/watchevents/observer.go b/internal/watchevents/observer.go index c4384850..371f7753 100644 --- a/internal/watchevents/observer.go +++ b/internal/watchevents/observer.go @@ -18,12 +18,22 @@ import "sync" // WHAT THIS DOES AND DOES NOT SEE (BUG-2727). These callbacks report what // PAD detects. They do NOT hook go-redis, which has its own silent drop: // it buffers 100 messages per subscription and discards further ones after -// a 60s send timeout, logging only through its own internal logger. That -// drop is not reported here directly — it is reported by its CONSEQUENCE, -// as a SequenceGap, because a discarded message leaves a hole in the id -// sequence exactly like a message lost across a reconnect does. So -// SequenceGap counts "this instance missed something", not "Redis was at -// fault"; do not read a gap as evidence of any particular cause. +// a 60s send timeout, logging only through its own internal logger. +// +// Such a drop is USUALLY visible here by its CONSEQUENCE, as a +// SequenceGap: the discarded message leaves a hole in the id sequence +// exactly like one lost across a reconnect. Two boundaries on that, both +// real: +// +// - Detection needs a LATER notification to expose the hole. Drop the +// newest message on a bus that then goes quiet and no gap is ever +// reported, because nothing arrives to be non-consecutive with. +// - A gap says "this instance missed something", NOT "Redis was at +// fault". Reconnects produce them too. Do not read a gap as evidence +// of any particular cause. +// +// go-redis's own log line is the only direct evidence of that drop, which +// is why cmd/pad routes its logger into slog. // // Implementations must be safe for concurrent use and must not block. //