fix: nine false or overstated claims in this diff's own prose (codex round 15)

An angle worth naming, because it found more than several code-shaped
ones did: check the COMMENTS against the CODE. This diff is
comment-heavy and its comments make specific factual claims. Nine were
wrong.

The one that mattered most was a false argument for a correct rule.
redisns.Parse rejects colons, and justified it with a collision example
that does not happen: ns "a:events" builds pad:a:events:events:<ws>, not
pad:a:events:<ws>, because the suffix is appended too. The rule stands on
its own grounds (a colon spans segments and makes the keyspace ambiguous
to read back) — but a false example is worse than none, because the next
reader trusts it.

Chasing that turned up a REAL collision needing no colon: a namespace
equal to one of Pad's own first segments nests this installation inside
the default one's keyspace. Namespace "events" puts every key under
pad:events:*, which is the default installation's activity channel space
— the exact cross-feed the namespace exists to prevent, arriving through
the namespace. Now rejected, with a control leg asserting that names
merely CONTAINING a reserved word ("events-eu", "prod-session") stay
valid.

The other eight:

- "The three keyspaces cannot drift" — overstated. Each constructor takes
  its own Keys; a source-reading test is what enforces it, which is
  weaker than a compiler and now says so.
- Two docs claimed both SSE endpoints incur a presence registration. Only
  the watch stream registers.
- The Redis metrics section said they "stay at zero" without Redis, while
  pad_redis_up is deliberately unregistered — the section contradicted
  the field three lines below it.
- The presence-failure metric's HELP string still carried the blanket
  "leaves sessions unlisted and untargetable" that the field comment had
  already been corrected away from. Two of the four ops fail in the
  opposite direction.
- A nil from MGET was described as proof the process died. Eviction, a
  restart and a manual DEL produce the same nil, and this file's own doc
  says eviction is indistinguishable from expiry.
- A test comment claimed to cover both corrupt-entry shapes; the second
  is unreachable and the subtest is deliberately absent, as the note ten
  lines down already said.
- "Enumerates every refusal path" covered per-instance and per-workspace
  and not per-user — the same undercount as round 13's, one round later.
  Both per-user paths added.
- The Observer contract said a go-redis drop is reported as a sequence
  gap. Only if a LATER notification arrives to expose the hole: drop the
  newest message on a bus that then goes quiet and nothing is reported.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

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,

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.
 

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(

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:<ws>
-// 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:<ws> and collide with installation "a"'s channel, when the
+// suffix is appended too and the result is pad:a:events:events:<ws>. 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 == '_':

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:<ws> 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 {

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()
 

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
 		}

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 {

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.
 //