fix(server,watchevents): shared session presence + honest push acceptance (BUG-2698, BUG-2699) (#1175)

Two coupled defects in the push path, fixed as one unit because 2699's honest-acceptance signature is the substrate 2698's fix reports through.

BUG-2699 — Bus.Publish reports acceptance. The endpoint returned 200 pushed:true
for a publish that was dropped, because Publish returned nothing and swallowed
every failure. An error is two outcomes and they are kept apart: ErrBusClosed
proves nothing was published (503 unavailable, safe to resend), while any other
error means UNCONFIRMED — go-redis retries a command whose reply was lost, which
is why the publish script already carries a dedupe token — and gets 502
push_unconfirmed, deliberately off the web client's safe-to-resend list.
MemoryBus was the worse case, not the exempt one: neither implementation checked
`closed`, and the in-process one dropped silently with no log at all. Seven
production call sites, not the six the item named; the six best-effort producers
discard through one named helper, and an AST-based test fails when a new
producer publishes directly.

BUG-2698 — RedisSessionPresence. A session-targeted push was resolved against
the answering replica's presence registry, and the handler skips the publish
when the target is absent, so a POST landing on A for a session held on B
dropped the instruction and answered delivered_sessions:0. Fixed at the REGISTRY
rather than the gate: a shared registry makes the snapshot right, which makes
the picker complete and restores the gate's original premise, so the existing
skip becomes correct for the reason it was written. Entry and index are written
atomically under a TTL renewed by a goroutine that lives exactly as long as the
connection; a crashed process stops renewing and Redis clears it. Staleness is
unchanged and now stated in full: ~30s for a dropped client, ~90s for a dead
instance.

delivered_sessions becomes nullable — null means published-but-uncountable,
never zero — documented as three states at every consumer.

35 Codex review rounds. Notable: a per-user registry cap was added and then
removed after three consecutive rounds found defects inside it and a fourth was
asked whether it belonged in this PR at all; a context bound was documented,
disproved by its own test (go-redis does not apply a command context to
connection establishment — 5.0s measured against a 150ms ctx), and rewritten to
say what is true. Every fix was mutation-checked; one instrument was deleted for
passing on broken code and one for not asserting its own premise.

Filed rather than folded in: BUG-2724 (Redis keyspace namespacing + Cluster),
BUG-2725 (delivered_sessions is an estimate with error in both directions),
BUG-2726 (no concurrent-connection limit on the watch stream), BUG-2727 (Redis
absent from readiness/metrics; silent subscriber loss), BUG-2728 (epoch-reset
resume lead).

Gates: build · make lint 0 issues · go test ./... (25 pkgs) · svelte-check 0
errors · vitest 1738 passed · CI 7/7 including Go (PostgreSQL) and Nix.

.env.example

@@ -125,8 +125,11 @@ PAD_ENCRYPTION_KEY=
 # PAD_DATABASE_URL=postgres://pad:secret@postgres:5432/pad?sslmode=disable
 
 # ─────────────────────────────────────────────────────────────────────
-# Real-time events — cross-instance SSE fan-out via Redis pub/sub.
-# Without this, SSE stays in-process (fine for single-node).
+# Real-time events — cross-instance fan-out via Redis. Carries three things:
+# SSE activity events, watch/push notifications, and the session-presence
+# registry that `pad push` and the web UI's agent-session picker read.
+# Without this, all three stay in-process (fine for single-node).
+# Use a non-evicting maxmemory-policy — see docs/deployment.md.
 # ─────────────────────────────────────────────────────────────────────
 
 # PAD_REDIS_URL=redis://redis:6379

cmd/pad/cmd_push.go

@@ -1,7 +1,9 @@
 package main
 
 import (
+	"errors"
 	"fmt"
+	"os"
 	"strings"
 
 	"github.com/spf13/cobra"
@@ -34,17 +36,22 @@ func pushCmd() *cobra.Command {
 		Use:   "push <ref>",
 		Short: "Push an item + instruction to your own connected agent session(s)",
 		Long: fmt.Sprintf(`pad push <ref> -m "message"
-    Publish a self-addressed push notification on an item. Every one of
-    your OWN connected plugin-monitor sessions (pad watch --stream
-    --for-session) receives it — fire-and-forget over the watch-events
-    bus, with no durable inbox: a push with no connected session
-    listening is simply not seen (Phase 1 scope).
+    Publish a self-addressed push notification on an item. It reaches
+    your OWN plugin-monitor sessions (pad watch --stream --for-session)
+    that are ACCEPTING pushes and can see the item — a connected session
+    that hasn't opted in, or that lacks access to the item, does not
+    receive it. Fire-and-forget over the watch-events bus, with no
+    durable inbox: a push nothing is listening for is simply not seen
+    (Phase 1 scope).
 
-    On a multi-instance deployment (PAD_REDIS_URL set) a BROADCAST push
-    reaches your sessions on every instance. A push naming one session
-    is still resolved against the server that handles the request, so
-    targeting a session connected to a different instance finds nothing
-    and sends nothing — see BUG-2698.
+    On a multi-instance deployment (PAD_REDIS_URL set) both broadcast
+    and session-targeted pushes reach your sessions on every instance:
+    the notification bus and the session-presence registry are both
+    shared, so a session connected to one server is visible and
+    addressable from any of them. Addressable is not the same as
+    delivered — a session still has to be accepting pushes, and still
+    has to have access to the item — so treat the reported count as
+    what was ADDRESSED, not as a receipt.
 
     -m/--message is required and must not be blank; it is the
     instruction text the receiving agent acts on (load the item first,
@@ -65,6 +72,24 @@ func pushCmd() *cobra.Command {
 
 			result, err := client.PushItem(ws, args[0], trimmed)
 			if err != nil {
+				// AMBIGUOUS FAILURES GET SAID OUT LOUD (codex round 9 on
+				// BUG-2699). The handler publishes BEFORE it writes its
+				// response, so an error is not proof the instruction went
+				// nowhere: a response lost in transit, a truncated body, a
+				// gateway envelope, or the server's own push_unconfirmed all
+				// reach here identically to a clean refusal. A user who reads
+				// "error" and re-runs the command delivers the push twice —
+				// there is no idempotency key, and the receiving agent acts
+				// on each copy.
+				//
+				// The web dialog has drawn this distinction since TASK-2588;
+				// the CLI had not, so the same outcome produced a safe
+				// message in a browser and a misleading one in a terminal.
+				if !pushRefusedBeforePublishing(err) {
+					fmt.Fprintln(os.Stderr,
+						"warning: the push may or may not have been delivered — check your agent session before re-running. "+
+							"Re-sending an instruction that already landed delivers it twice.")
+				}
 				return err
 			}
 
@@ -80,3 +105,65 @@ func pushCmd() *cobra.Command {
 		fmt.Sprintf("instruction text to push (required, max %d characters after whitespace collapse)", maxPushMessageLenForHelp))
 	return cmd
 }
+
+// pushPrePublishRefusalCodes are the error codes the push endpoint can
+// only produce BEFORE it publishes, so the instruction provably did not go
+// out and re-running is safe.
+//
+// Deliberately mirrors web/src/lib/push/dispatch.ts's
+// PUSH_PRE_PUBLISH_ERROR_CODES — the two surfaces answer the same question
+// about the same endpoint, and letting them drift would mean a push that is
+// safe to retry in a browser and unsafe in a terminal, or the reverse.
+// Keep them in step.
+//
+// `unavailable` covers both the no-bus branch and a bus that was already
+// closed; both are refusals with nothing published. `archived` is the 409
+// writeItemResolveError writes when the ref names a soft-deleted item —
+// item resolution happens before anything is published (codex round 12).
+//
+// ENUMERATED from the handler rather than taken one at a time: the codes
+// handlePushToItem and its helpers can write before the publish are
+// bad_request, unauthorized, unavailable, not_found (getWorkspace,
+// requireItemVisible, writeItemResolveError), archived, and internal_error,
+// plus the middleware codes below it. Round 12 named `archived`; the
+// enumeration is what found `internal_error` alongside it.
+//
+// Notably ABSENT, both deliberately:
+//   - push_unconfirmed, which exists precisely to say the outcome is
+//     unknown.
+//   - internal_error. It IS pre-publish today — this handler only reaches
+//     writeInternalError from the item-resolution path — but unlike every
+//     other code here that is a property of where one call sits, not of
+//     what the code means. A future writeInternalError added after the
+//     publish would silently make this entry wrong, and wrong in the
+//     direction that costs a duplicate dispatch. A spurious warning on a
+//     500 costs a sentence.
+var pushPrePublishRefusalCodes = map[string]bool{
+	"bad_request":         true,
+	"unauthorized":        true,
+	"not_found":           true,
+	"forbidden":           true,
+	"permission_denied":   true,
+	"unavailable":         true,
+	"archived":            true,
+	"rate_limited":        true,
+	"plan_limit_exceeded": true,
+	"csrf_error":          true,
+	"email_not_verified":  true,
+}
+
+// pushRefusedBeforePublishing reports whether err is a refusal the server
+// provably wrote before publishing.
+//
+// Defaults to FALSE for anything it cannot identify — a transport error, a
+// non-JSON gateway response, an unrecognised code. That default is the
+// whole point: an unknown outcome must be treated as possibly-delivered,
+// because the cost of being wrong in the other direction is a duplicate
+// dispatch.
+func pushRefusedBeforePublishing(err error) bool {
+	var apiErr *cli.APIError
+	if !errors.As(err, &apiErr) {
+		return false
+	}
+	return pushPrePublishRefusalCodes[apiErr.Code]
+}

cmd/pad/cmd_push_ambiguity_test.go

@@ -0,0 +1,202 @@
+package main
+
+import (
+	"encoding/json"
+	"errors"
+	"net/http"
+	"os"
+	"path/filepath"
+	"regexp"
+	"strings"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/cli"
+)
+
+// BUG-2699 codex round 9 — the CLI's half of "is this safe to re-run?".
+//
+// The push handler publishes BEFORE it writes its response, so an error is
+// not proof that nothing went out. The web dialog has distinguished the
+// two since TASK-2588; the CLI reported every failure identically, so the
+// same server outcome produced a safe message in a browser and a
+// misleading one in a terminal.
+//
+// The default matters more than any single code: anything unrecognised
+// must read as POSSIBLY DELIVERED, because being wrong that way costs a
+// warning while being wrong the other way costs a duplicate dispatch into
+// an agent harness.
+func TestPushRefusedBeforePublishing(t *testing.T) {
+	t.Parallel()
+
+	t.Run("recognised pre-publish refusals are safe to re-run", func(t *testing.T) {
+		for _, code := range []string{
+			"bad_request", "unauthorized", "not_found", "forbidden",
+			"permission_denied", "unavailable", "archived", "rate_limited",
+			"plan_limit_exceeded", "csrf_error", "email_not_verified",
+		} {
+			if !pushRefusedBeforePublishing(&cli.APIError{Code: code, Message: "x"}) {
+				t.Errorf("%s should be recognised as a pre-publish refusal", code)
+			}
+		}
+	})
+
+	t.Run("internal_error is NOT safe, deliberately", func(t *testing.T) {
+		// It is pre-publish today, and left off anyway: that is a property
+		// of where one call currently sits, not of what the code means, so
+		// a writeInternalError added after the publish would silently make
+		// it wrong in the direction that costs a duplicate dispatch.
+		if pushRefusedBeforePublishing(&cli.APIError{Code: "internal_error", Message: "x"}) {
+			t.Fatal("internal_error must stay off the safe list — see pushPrePublishRefusalCodes for why")
+		}
+	})
+
+	t.Run("push_unconfirmed is NOT safe", func(t *testing.T) {
+		// The code the server emits precisely to say it does not know. If
+		// this were ever added to the map, the CLI would tell users to
+		// re-run the one case that is most likely to duplicate.
+		if pushRefusedBeforePublishing(&cli.APIError{Code: "push_unconfirmed", Message: "x"}) {
+			t.Fatal("push_unconfirmed must never be treated as a pre-publish refusal")
+		}
+	})
+
+	t.Run("unknown shapes default to possibly-delivered", func(t *testing.T) {
+		cases := []struct {
+			name string
+			err  error
+		}{
+			{"transport error", errors.New("request failed: connection reset")},
+			{"unrecognised code", &cli.APIError{Code: "bad_gateway", Message: "x"}},
+			{"empty code", &cli.APIError{Code: "", Message: "x"}},
+			{"gateway envelope", errors.New("API error: 502 <html>bad gateway</html>")},
+		}
+		for _, tc := range cases {
+			if pushRefusedBeforePublishing(tc.err) {
+				t.Errorf("%s must default to possibly-delivered", tc.name)
+			}
+		}
+	})
+
+	t.Run("wrapped API errors are still recognised", func(t *testing.T) {
+		// errors.As, not a type assertion: a wrapped refusal is still a
+		// refusal, and treating it as unknown would warn on a case that is
+		// provably safe.
+		wrapped := errors.Join(errors.New("context"), &cli.APIError{Code: "bad_request", Message: "x"})
+		if !pushRefusedBeforePublishing(wrapped) {
+			t.Fatal("a wrapped pre-publish refusal must still be recognised")
+		}
+	})
+}
+
+// TestPushRefusalCodesMatchTheWebAllowList pins the two surfaces together.
+//
+// They answer the same question about the same endpoint, so drift means a
+// push that is safe to retry in a browser and unsafe in a terminal, or the
+// reverse. Checked against the TypeScript source rather than a copy of it,
+// so editing one side without the other fails here.
+func TestPushRefusalCodesMatchTheWebAllowList(t *testing.T) {
+	t.Parallel()
+	webCodes := readWebPrePublishCodes(t)
+	for code := range pushPrePublishRefusalCodes {
+		if !webCodes[code] {
+			t.Errorf("%q is a pre-publish refusal in the CLI but not in web/src/lib/push/dispatch.ts", code)
+		}
+	}
+	for code := range webCodes {
+		if !pushPrePublishRefusalCodes[code] {
+			t.Errorf("%q is a pre-publish refusal in web/src/lib/push/dispatch.ts but not in the CLI", code)
+		}
+	}
+	// POSITIVE CONTROL: a parse that found nothing would make both loops
+	// vacuous and report agreement between a list and an empty set.
+	if len(webCodes) == 0 {
+		t.Fatal("parsed no codes out of the web allow-list — the parser is broken, not the lists")
+	}
+	// WHAT THIS TEST CANNOT DO, stated because agreement reads like
+	// completeness (codex round 12): it proves the two lists MATCH, never
+	// that either is complete. `archived` was missing from both for two
+	// rounds and this test was green throughout. Completeness comes from
+	// enumerating the handler's pre-publish error codes — see
+	// pushPrePublishRefusalCodes' doc comment, which records that
+	// enumeration and which codes were deliberately left off.
+}
+
+// readWebPrePublishCodes extracts the string literals from
+// PUSH_PRE_PUBLISH_ERROR_CODES in the web client.
+func readWebPrePublishCodes(t *testing.T) map[string]bool {
+	t.Helper()
+	src, err := os.ReadFile(filepath.Join("..", "..", "web", "src", "lib", "push", "dispatch.ts"))
+	if err != nil {
+		t.Fatalf("read web dispatch.ts: %v", err)
+	}
+	text := string(src)
+	start := strings.Index(text, "PUSH_PRE_PUBLISH_ERROR_CODES")
+	if start < 0 {
+		t.Fatal("PUSH_PRE_PUBLISH_ERROR_CODES not found — it was renamed, and this pin needs updating with it")
+	}
+	open := strings.Index(text[start:], "([")
+	closeIdx := strings.Index(text[start:], "])")
+	if open < 0 || closeIdx < 0 || closeIdx < open {
+		t.Fatal("could not locate the code list body")
+	}
+	body := text[start+open : start+closeIdx]
+
+	out := map[string]bool{}
+	for _, m := range regexp.MustCompile(`'([a-z_]+)'`).FindAllStringSubmatch(body, -1) {
+		out[m[1]] = true
+	}
+	return out
+}
+
+// TestPushCmd_WarnsOnlyWhenTheOutcomeIsAmbiguous — codex round 30
+// (coverage-gap sweep).
+//
+// The classification helper is tested directly; nothing executed the
+// command and checked what a user actually sees. Both directions matter
+// and they fail differently: a missing warning on an ambiguous failure
+// invites a duplicate dispatch, and a spurious warning on a clean refusal
+// trains people to ignore it.
+func TestPushCmd_WarnsOnlyWhenTheOutcomeIsAmbiguous(t *testing.T) {
+	for _, tc := range []struct {
+		name     string
+		status   int
+		code     string
+		wantWarn bool
+	}{
+		{"ambiguous 502 warns", http.StatusBadGateway, "push_unconfirmed", true},
+		{"unrecognised code warns", http.StatusBadGateway, "bad_gateway", true},
+		// The CONTROLS. A fix that simply always warned would pass the two
+		// cases above and be useless.
+		{"clean refusal does not warn", http.StatusServiceUnavailable, "unavailable", false},
+		{"validation refusal does not warn", http.StatusBadRequest, "bad_request", false},
+		{"archived refusal does not warn", http.StatusConflict, "archived", false},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			setupPushTest(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(tc.status)
+				_ = json.NewEncoder(w).Encode(map[string]any{
+					"error": map[string]any{"code": tc.code, "message": "nope"},
+				})
+			}))
+
+			cmd := pushCmd()
+			cmd.SetArgs([]string{"TASK-5", "-m", "triage this"})
+			cmd.SilenceErrors = true
+			cmd.SilenceUsage = true
+
+			var execErr error
+			stderr := captureStderr(t, func() { execErr = cmd.Execute() })
+			if execErr == nil {
+				t.Fatal("a failed push must still return an error")
+			}
+
+			warned := strings.Contains(stderr, "may or may not have been delivered")
+			if warned != tc.wantWarn {
+				t.Fatalf("warning presence = %v, want %v (stderr: %q)", warned, tc.wantWarn, stderr)
+			}
+			if tc.wantWarn && !strings.Contains(stderr, "twice") {
+				t.Fatalf("the warning must say what re-running costs; got %q", stderr)
+			}
+		})
+	}
+}

cmd/pad/cmd_push_test.go

@@ -107,6 +107,87 @@ type PushResultForTest struct {
 	Workspace string `json:"workspace"`
 	Pushed    bool   `json:"pushed"`
 	Message   string `json:"message"`
+	// A POINTER, mirroring cli.PushResult, because the field is tri-state
+	// and 0 is not the same answer as "unknown" (BUG-2698).
+	DeliveredSessions *int `json:"delivered_sessions"`
+}
+
+// TestPushCmd_FormatJSONCarriesDeliveredSessions — codex round 25.
+//
+// The field was added to cli.PushResult in review round 3 and nothing
+// covered its serialization, so a regression in either direction — the tag
+// renamed, `omitempty` reintroduced — would have passed unnoticed. Both
+// values are driven, because they are DIFFERENT answers: a number is a
+// count, null means the server published but could not count it.
+func TestPushCmd_FormatJSONCarriesDeliveredSessions(t *testing.T) {
+	for _, tc := range []struct {
+		name   string
+		server any
+		want   func(t *testing.T, got *int)
+	}{
+		{
+			name:   "a real count round-trips",
+			server: 3,
+			want: func(t *testing.T, got *int) {
+				if got == nil {
+					t.Fatal("delivered_sessions was dropped; `pad push --format json` cannot see the count")
+				}
+				if *got != 3 {
+					t.Fatalf("delivered_sessions = %d, want 3", *got)
+				}
+			},
+		},
+		{
+			name:   "null survives as null, not as zero",
+			server: nil,
+			want: func(t *testing.T, got *int) {
+				// The wrong behaviour's fingerprint: a 0, which claims
+				// nobody received a broadcast that was in fact published.
+				if got != nil {
+					t.Fatalf("null must not decode to a number, got %d", *got)
+				}
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			body := map[string]any{
+				"ref": "TASK-5", "workspace": "demo", "pushed": true,
+				"message": "triage this", "delivered_sessions": tc.server,
+			}
+			setupPushTest(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.WriteHeader(http.StatusOK)
+				_ = json.NewEncoder(w).Encode(body)
+			}))
+			prevFormat := formatFlag
+			formatFlag = "json"
+			defer func() { formatFlag = prevFormat }()
+
+			cmd := pushCmd()
+			cmd.SetArgs([]string{"TASK-5", "-m", "triage this"})
+			var execErr error
+			out := captureStdout(t, func() { execErr = cmd.Execute() })
+			if execErr != nil {
+				t.Fatalf("execute push: %v", execErr)
+			}
+
+			// The KEY must be present either way — an absent key is a third
+			// signal (a server predating session targeting), so emitting
+			// nothing would collapse two answers into one.
+			var raw map[string]json.RawMessage
+			if err := json.Unmarshal([]byte(out), &raw); err != nil {
+				t.Fatalf("output is not valid JSON: %v\noutput: %s", err, out)
+			}
+			if _, present := raw["delivered_sessions"]; !present {
+				t.Fatalf("delivered_sessions must be present in the CLI's JSON, got: %s", out)
+			}
+
+			var result PushResultForTest
+			if err := json.Unmarshal([]byte(out), &result); err != nil {
+				t.Fatalf("decode: %v\noutput: %s", err, out)
+			}
+			tc.want(t, result.DeliveredSessions)
+		})
+	}
 }
 
 // TestPushCmd_RequiresNonBlankMessage covers the client-side guard: an

cmd/pad/cmd_server.go

@@ -679,20 +679,37 @@ func serveCmd() *cobra.Command {
 			}
 			srv.SetWatchEventsBus(watchBus)
 
-			// Live-session presence registry (PLAN-2558 S1). Wired
-			// unconditionally, and it is now the ONLY in-process piece
-			// of this pair — the watch bus above stopped being one when
-			// PAD_REDIS_URL is set (BUG-2651). So the old "same caveat
-			// as the bus directly above" reading no longer holds: the
-			// bus delivers across instances, while this registry still
-			// reports only the answering instance's connections.
+			// Live-session presence registry (PLAN-2558 S1), on the SAME
+			// PAD_REDIS_URL switch as both buses above (BUG-2698). The
+			// three now cross instance boundaries together, which is the
+			// point: a shared bus with a per-process registry was worse
+			// than either being consistent, because a session-targeted
+			// push was resolved against the answering instance's view and
+			// skipped the publish for a session the bus could have
+			// reached.
 			//
-			// See internal/server/session_presence.go's note for what
-			// that costs (a session picker that under-reports, rather
-			// than a push that lies) before putting the web-UI push
-			// surface (PLAN-2558 S3) in front of a multi-process
-			// deployment.
-			srv.SetSessionPresence(server.NewMemorySessionPresence())
+			// A self-hosted single-process binary keeps the in-memory
+			// registry and never touches Redis, same as the buses.
+			var sessionPresence server.SessionPresence
+			// Declared out here so the shutdown sequence below can close it
+			// at the right point in the order — see there for why that point
+			// is before http.Server.Shutdown rather than after.
+			var redisPresence *server.RedisSessionPresence
+			if watchRedis != nil {
+				redisPresence = server.NewRedisSessionPresence(watchRedis)
+				// Backstop for early-return paths that never reach the
+				// shutdown sequence; Close is idempotent. Deliberately does
+				// NOT delete this instance's entries — a shutdown racing a
+				// reconnect elsewhere would then delete a session that had
+				// already re-registered — so they clear on their TTL.
+				defer redisPresence.Close()
+				sessionPresence = redisPresence
+				slog.Info("Session presence registry using Redis (shared across instances)")
+			} else {
+				sessionPresence = server.NewMemorySessionPresence()
+				slog.Info("Session presence registry using in-memory (single instance)")
+			}
+			srv.SetSessionPresence(sessionPresence)
 
 			// Yjs collab room manager (PLAN-1248). Single-instance only
 			// today; multi-replica fanout via Redis is a deferred IDEA.
@@ -879,6 +896,33 @@ func serveCmd() *cobra.Command {
 				eventBus.Close()
 				slog.Info("Event bus closed")
 
+				// Presence closes FIRST — before the watch bus, and well before
+				// Shutdown. The ordering is load-bearing twice over (codex
+				// rounds 4 and 5 on BUG-2698).
+				//
+				// Remove waits for a session's renewal goroutine. Closing the bus
+				// is what RELEASES the SSE handlers, so each one immediately runs
+				// its deferred Remove — and any Remove that runs before Close
+				// still finds a live renewal to wait for, bypassing the drain
+				// bound entirely and putting that wait in front of Shutdown.
+				// Closing presence first cancels every renewal and drains them,
+				// so the Removes that follow have nothing left to wait on.
+				//
+				// Note it does NOT empty the registry — an earlier version of
+				// this comment said so, and Close deliberately retains the
+				// entries precisely so a concurrent Remove can still find and
+				// await a renewal (codex rounds 6 and 10). Remove's own wait is
+				// bounded by the same drain deadline, so a renewal Close could
+				// not drain cannot hold Shutdown either.
+				//
+				// Nothing here depends on the bus, so there is no cost to going
+				// first. Idempotent, so the deferred Close at the wiring site
+				// stays a harmless backstop for early-return paths.
+				if redisPresence != nil {
+					redisPresence.Close()
+					slog.Info("Session presence registry closed")
+				}
+
 				// Same reasoning for the watch bus, and it matters for the
 				// same reason: GET /api/v1/events/stream is a long-lived
 				// handler blocked on this bus's channel, so leaving it open
@@ -890,17 +934,22 @@ func serveCmd() *cobra.Command {
 				// call inside srv.Stop() is a no-op.
 				//
 				// THE TRADE, which is the same one eventBus above already
-				// makes and is worth naming (codex round 10): closing BEFORE
-				// Shutdown drains handlers means a push already in flight can
-				// Publish into a closed bus, be dropped, and still return
-				// HTTP 200 with pushed:true. Closing AFTER instead would hold
-				// Shutdown for its full 30s deadline on any open stream,
+				// makes and is worth naming: closing BEFORE Shutdown drains
+				// handlers means a push already in flight publishes into a
+				// closed bus and is not delivered. Closing AFTER instead would
+				// hold Shutdown for its full 30s deadline on any open stream,
 				// every time. Neither is free; this side loses a message in a
-				// window measured in milliseconds during a deliberate
-				// shutdown, the other side delays every shutdown by half a
-				// minute. Making the handler actually LEARN the publish was
-				// dropped needs Bus.Publish to report it, which is an
-				// interface change and a different unit.
+				// window measured in milliseconds during a deliberate shutdown,
+				// the other side delays every shutdown by half a minute.
+				//
+				// WHAT IT NO LONGER COSTS is the caller's understanding of it
+				// (BUG-2699). This comment used to end "...and still return HTTP
+				// 200 with pushed:true", and then note that fixing it needed an
+				// interface change and a different unit. That unit landed:
+				// Bus.Publish reports acceptance, so a push publishing into a
+				// closed bus gets ErrBusClosed and answers 503. The message is
+				// still lost; the caller is no longer told it was sent, and can
+				// safely re-send once the server is back.
 				watchBus.Close()
 				slog.Info("Watch notification bus closed")
 

cmd/pad/cmd_watch.go

@@ -520,8 +520,20 @@ func runWatchMonitor(ctx context.Context) error {
 		}
 		// The stream ended (server closed it, network blip, padd
 		// restart). Last-Event-ID (captured in streamWatchEvents)
-		// resumes from here — no re-delivery, no gap silently
+		// resumes from here — no re-delivery, and no gap silently
 		// swallowed beyond the replay buffer's own bounds.
+		//
+		// ONE EXCEPTION on a Redis-backed deployment, stated because this
+		// sentence used to promise more than the implementation delivers
+		// (codex round 6 on BUG-2698). RedisBus's resume check compares
+		// against the shared counter, and its own doc says what it cannot
+		// see: a notification published AFTER that read and missed by the
+		// answering instance is invisible to any check made there. That is
+		// a property of at-most-once pub/sub with no per-connection ack,
+		// not a bound this loop can widen. So the honest promise is "no gap
+		// before the resume goes silently unreported"; a message lost in
+		// the instant after it is not detectable here or anywhere else
+		// short of a durable stream.
 		attempt++
 		if !sleepOrDone(sigCtx, padddBackoff(attempt)) {
 			return nil

docker-compose.prod.yml

@@ -56,7 +56,16 @@ services:
     # In production, consider using a managed PostgreSQL service instead.
 
   redis:
-    command: redis-server --maxmemory 128mb --maxmemory-policy allkeys-lru --requirepass "${REDIS_PASSWORD:-}"
+    # noeviction, NOT allkeys-lru (BUG-2698). Pad's Redis holds the
+    # session-presence registry as well as pub/sub, and evicting a live
+    # session's entry is indistinguishable from its TTL lapsing: the
+    # session disappears from the "push to agent" picker and a push
+    # targeted at it reports delivered_sessions: 0 while it is still
+    # connected. It self-repairs on the next 30s renewal, but the
+    # keyspace is small — a few hundred bytes per connected session plus
+    # two counters — so there is nothing to gain by evicting it. See
+    # docs/deployment.md's Redis configuration notes.
+    command: redis-server --maxmemory 128mb --maxmemory-policy noeviction --requirepass "${REDIS_PASSWORD:-}"
     healthcheck:
       # Override the base healthcheck to authenticate when REDIS_PASSWORD is set.
       # redis-cli reads REDISCLI_AUTH automatically for authentication.

docker-compose.yml

@@ -3,7 +3,8 @@
 #   1. cp .env.example .env            # generates a POSTGRES_PASSWORD if missing
 #   2. docker compose up -d
 #
-# Starts Pad with PostgreSQL for storage and Redis for real-time events.
+# Starts Pad with PostgreSQL for storage and Redis for real-time events,
+# notifications, and the shared session-presence registry.
 # The web UI binds to 127.0.0.1:7777 on the host by default — LAN/internet
 # access requires explicit opt-in (see PAD_BIND_ADDR below or the prod override).
 # First-time setup: visit the UI or run `pad auth setup` from a local CLI.

docs/deployment.md

@@ -24,7 +24,7 @@ Pad is a single Go binary with an embedded web UI. It supports SQLite (default)
 
 - **Pad** serves the REST API and embedded SvelteKit web UI on a single port (default: 7777)
 - **PostgreSQL** stores all data (workspaces, items, users, activity). SQLite works for single-node.
-- **Redis** enables real-time SSE events across multiple Pad instances. Optional for single-node.
+- **Redis** carries real-time events, watch/push notifications, and the shared session-presence registry across multiple Pad instances. Optional for single-node.
 
 ## Quick Start with Docker Compose
 
@@ -82,10 +82,97 @@ All configuration is via environment variables or a config file (`~/.pad/config.
 
 | Variable | Default | Description |
 |----------|---------|-------------|
-| `PAD_REDIS_URL` | — | Redis URL for cross-instance pub/sub. Without Redis, SSE events are in-process only. |
+| `PAD_REDIS_URL` | — | Redis URL for cross-instance pub/sub **and the session-presence registry**. Without Redis, SSE events, watch notifications, and session presence are all in-process only. |
 | `PAD_SSE_MAX_CONNECTIONS` | `1000` | Global maximum SSE connections |
 | `PAD_SSE_MAX_PER_WORKSPACE` | `100` | Per-workspace maximum SSE connections |
 
+#### Redis configuration notes
+
+Pad's Redis integration assumes a **single Redis node** — `redis://…`, not a
+cluster. Both event buses and the session-presence registry use flat key names
+and a non-cluster client; pointing Pad at a Redis Cluster is not supported.
+
+**Avoid an evicting `maxmemory-policy` for Pad's Redis.**
+`docker-compose.prod.yml` sets `noeviction` for this reason; the plain
+`docker-compose.yml` keeps `allkeys-lru` on its 64 MB dev instance, where the
+consequence below is a momentary annoyance rather than a lost instruction —
+change it too if you run that file in anger.
+
+Under an evicting policy Redis may drop live session-presence entries under
+memory pressure. Nothing can distinguish that from a TTL lapsing, so a
+connected agent session briefly disappears from the picker and a push targeted
+at it reports
+`delivered_sessions: 0`. It self-repairs on the session's next 30-second
+renewal, and Pad's keyspace is small — a few hundred bytes per connected
+session plus two counters — so there is nothing to gain by evicting it.
+
+#### If push stops finding a session (on-call)
+
+The most likely Redis-related symptom is a **transient write failure while
+registering a session**. The agent's event stream stays up — the connection is
+never refused over a registry problem — but the session is absent from the
+shared registry, so:
+
+- it does not appear in `GET /api/v1/sessions` or the web picker, and
+- a push **targeted** at it returns `200 pushed:true` with
+  `delivered_sessions: 0` and skips publication, so the instruction is not
+  delivered.
+
+**What you'll see:** `session presence: failed to register session` or `failed
+to renew session entry` warnings (rate-limited to one per minute, carrying
+`failures_since_last_log` — a large count means the replica, a small one means
+a single session), and the session missing from the listing.
+
+**What to do:** restore Redis connectivity, capacity, or ACLs. Registration
+self-heals — each session's renewal re-writes its full entry, so an affected
+session reappears within ~30 seconds without reconnecting. Confirm it is listed
+again before re-sending anything.
+
+**What NOT to do:** do not blindly re-send. A *targeted* push reporting
+`delivered_sessions: 0` is safe to resend, because the server skipped the
+publish. A **broadcast** is always published, and a `502 push_unconfirmed` means
+the outcome is unknown — re-sending either can deliver a second instruction the
+agent acts on twice. Only re-send what the server told you it skipped.
+
+#### Upgrading a multi-instance deployment
+
+`PAD_REDIS_URL` now also backs the **session-presence registry** — the list of
+which agent sessions are connected, which `pad push` and the web UI's "Push to
+agent" picker read to decide where a push goes. Previously that registry was
+per-process even when Redis was configured, so a push aimed at a session held
+by another replica was silently dropped.
+
+**During a rolling upgrade, old and new replicas disagree about presence.** An
+old replica has only its own connections in view, so a push it answers cannot
+see a session held on a new replica, and `GET /api/v1/sessions` returns a
+different list depending on which replica answers. A TARGETED push reports this
+honestly — `delivered_sessions: 0`, and the publish is skipped, so nothing was
+sent — but the instruction is not delivered.
+
+This is the same behaviour every replica had *before* this build, so the
+rollout is not a regression; it is a window in which the fix is only partly in
+effect. Two ways to avoid the window:
+
+- **Blue/green** — bring up the new replicas, cut traffic over, retire the old
+  ones. No mixed period.
+- **Drain first** — scale old replicas out of the load balancer and let agent
+  monitors reconnect (`pad watch --stream` reconnects on its own) before
+  serving pushes from the new set.
+
+If neither is practical, a rolling upgrade is still safe: nothing is corrupted
+and no migration is needed. Targeted pushes may report `delivered_sessions: 0`
+and go undelivered until every replica runs the new build; those are safe to
+re-send once the rollout completes, because a targeted miss skips the publish
+entirely.
+
+**That safety does not extend to broadcasts.** A broadcast push is always
+published, on old and new replicas alike, and the shared notification bus
+carries it across instances regardless of which registry the answering replica
+used — so a broadcast reporting `0` during the rollout may well have been
+delivered. Re-sending one is a second instruction the receiving agent will act
+on twice. Only re-send a push the server told you it skipped. There is no Redis or database migration; the
+registry's keys are transient and expire on their own TTL.
+
 ### Security
 
 | Variable | Default | Description |
@@ -318,7 +405,7 @@ curl -s http://localhost:7777/api/v1/health   # {"status":"ok"}
 ## Production Checklist
 
 - [ ] **Database:** PostgreSQL configured with `PAD_DB_DRIVER=postgres`
-- [ ] **Redis:** Connected for multi-instance SSE (`PAD_REDIS_URL`)
+- [ ] **Redis:** Connected for multi-instance events, notifications, and session presence (`PAD_REDIS_URL`), on a non-evicting `maxmemory-policy`
 - [ ] **TLS:** Reverse proxy with valid certificates
 - [ ] **Secure cookies:** `PAD_SECURE_COOKIES=true` (requires TLS)
 - [ ] **Public URL:** `PAD_URL` set to your public-facing domain

internal/cli/client.go

@@ -461,6 +461,35 @@ type PushResult struct {
 	Workspace string `json:"workspace"`
 	Pushed    bool   `json:"pushed"`
 	Message   string `json:"message"`
+	// DeliveredSessions mirrors the server's field of the same name — how
+	// many of the caller's own live sessions the push's delivery predicate
+	// matched. It was missing here while this struct's doc comment claimed
+	// to mirror the response shape, so `pad push --format json` silently
+	// dropped it (codex round 3 on BUG-2698/2699).
+	//
+	// A POINTER, because the field is genuinely tri-state on the wire:
+	// a number is a real count; NULL means the notification was published
+	// but the presence registry could not be read to count it (BUG-2698);
+	// and an ABSENT key means a server predating session targeting. The
+	// second and third are both `nil` here — a CLI consumer that needs to
+	// tell them apart has to read the raw body, which no caller does. What
+	// matters is that neither is reported as 0, because 0 and "unknown" are
+	// different answers.
+	//
+	// AND 0 IS NOT "reached nobody" ON THIS PATH (codex round 24). That
+	// guarantee is the TARGETED one: the server skips the publish when a
+	// named target is absent, so nothing was sent. `pad push` only ever
+	// broadcasts — internal/cli never sends target_session_id — and a
+	// broadcast is ALWAYS published, so a 0 here means no session was
+	// registered at the moment the count was taken, not that nobody got it.
+	// A session registering in the interval receives it.
+	//
+	// NO omitempty (codex round 10): it would drop the nil case on the way
+	// back OUT, so `pad push --format json` would print no field at all for
+	// "published, count unknown" — silently re-collapsing the distinction
+	// this pointer exists to carry. The field is always present in the
+	// CLI's own JSON, as a number or as null.
+	DeliveredSessions *int `json:"delivered_sessions"`
 }
 
 // PushItem publishes a self-addressed push notification (IDEA-2544

internal/server/handlers_comments.go

@@ -138,18 +138,16 @@ func (s *Server) handleCreateComment(w http.ResponseWriter, r *http.Request) {
 	// Publish SSE event
 	s.publishCommentEvent(sseCommentCreated, workspaceID, item.ID, comment.ID, item.Title, item.CollectionSlug, actor, source)
 
-	if s.watchEvents != nil {
-		s.watchEvents.Publish(watchevents.Notification{
-			WorkspaceID:  workspaceID,
-			ItemID:       item.ID,
-			CollectionID: item.CollectionID,
-			ItemRef:      item.Ref,
-			Kind:         watchevents.KindComment,
-			Actor:        actor,
-			ActorName:    actorNameFromRequest(r),
-			Summary:      truncateForSummary(comment.Body, 120),
-		})
-	}
+	s.publishWatchNotification(watchevents.Notification{
+		WorkspaceID:  workspaceID,
+		ItemID:       item.ID,
+		CollectionID: item.CollectionID,
+		ItemRef:      item.Ref,
+		Kind:         watchevents.KindComment,
+		Actor:        actor,
+		ActorName:    actorNameFromRequest(r),
+		Summary:      truncateForSummary(comment.Body, 120),
+	})
 
 	writeJSON(w, http.StatusCreated, comment)
 }
@@ -371,18 +369,16 @@ func (s *Server) handleCreateReply(w http.ResponseWriter, r *http.Request) {
 	// hook entirely. Same kind=comment notification as a top-level
 	// comment; a watcher shouldn't lose replies just because they landed
 	// one level deeper in the thread.
-	if s.watchEvents != nil {
-		s.watchEvents.Publish(watchevents.Notification{
-			WorkspaceID:  workspaceID,
-			ItemID:       parentComment.ItemID,
-			CollectionID: replyCollID,
-			ItemRef:      replyItemRef,
-			Kind:         watchevents.KindComment,
-			Actor:        actor,
-			ActorName:    actorNameFromRequest(r),
-			Summary:      truncateForSummary(comment.Body, 120),
-		})
-	}
+	s.publishWatchNotification(watchevents.Notification{
+		WorkspaceID:  workspaceID,
+		ItemID:       parentComment.ItemID,
+		CollectionID: replyCollID,
+		ItemRef:      replyItemRef,
+		Kind:         watchevents.KindComment,
+		Actor:        actor,
+		ActorName:    actorNameFromRequest(r),
+		Summary:      truncateForSummary(comment.Body, 120),
+	})
 
 	writeJSON(w, http.StatusCreated, comment)
 }

internal/server/handlers_items.go

@@ -815,12 +815,12 @@ func (s *Server) createItemChecked(r *http.Request, workspaceID string, coll *mo
 	// created item has no "before" state to diff — LastMutation doesn't
 	// apply here — so this is a direct, unconditional check rather than
 	// a call to publishWatchNotifications.
-	if s.watchEvents != nil && item.AssignedUserID != nil && *item.AssignedUserID != "" {
+	if item.AssignedUserID != nil && *item.AssignedUserID != "" {
 		name := item.AssignedUserName
 		if name == "" {
 			name = *item.AssignedUserID
 		}
-		s.watchEvents.Publish(watchevents.Notification{
+		s.publishWatchNotification(watchevents.Notification{
 			WorkspaceID:    workspaceID,
 			ItemID:         item.ID,
 			CollectionID:   item.CollectionID,
@@ -1738,18 +1738,16 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) {
 			// directly, not through POST .../comments) — a bypass Rider
 			// 1's producer audit would otherwise miss. Same
 			// kind=comment notification as the standalone endpoint.
-			if s.watchEvents != nil {
-				s.watchEvents.Publish(watchevents.Notification{
-					WorkspaceID:  workspaceID,
-					ItemID:       updated.ID,
-					CollectionID: updated.CollectionID,
-					ItemRef:      updated.Ref,
-					Kind:         watchevents.KindComment,
-					Actor:        actor,
-					ActorName:    actorNameForUpdate,
-					Summary:      truncateForSummary(comment.Body, 120),
-				})
-			}
+			s.publishWatchNotification(watchevents.Notification{
+				WorkspaceID:  workspaceID,
+				ItemID:       updated.ID,
+				CollectionID: updated.CollectionID,
+				ItemRef:      updated.Ref,
+				Kind:         watchevents.KindComment,
+				Actor:        actor,
+				ActorName:    actorNameForUpdate,
+				Summary:      truncateForSummary(comment.Body, 120),
+			})
 		}
 	}
 

internal/server/handlers_push.go

@@ -1,7 +1,9 @@
 package server
 
 import (
+	"errors"
 	"fmt"
+	"log/slog"
 	"net/http"
 	"strings"
 
@@ -14,13 +16,19 @@ import (
 type pushRequest struct {
 	Message string `json:"message"`
 	// TargetSessionID optionally narrows delivery to one of the caller's
-	// OWN live sessions from the S1 presence registry (PLAN-2558 S5,
+	// OWN live sessions from the S1 presence registry — narrows, within a
+	// set already limited to ARMED sessions that can see the item (PLAN-2558 S5,
 	// TASK-2588; GET /api/v1/sessions is where a caller learns the id).
-	// Omitted (the pre-S5 shape) means broadcast to every one of the
-	// caller's connected sessions, unchanged. API + TS client + web
-	// picker only in this slice, per CONVE-1741 — no CLI flag, no MCP
-	// surface; internal/cli's PushResult mirror simply never sends or
-	// reads this field.
+	// Omitted (the pre-S5 shape) means broadcast to every session in that
+	// same set — armed, item-visible, the caller's own — unchanged. API + TS client + web
+	// picker only in this slice, per CONVE-1741 — no CLI flag and no MCP
+	// surface for SETTING it.
+	//
+	// The read side is no longer true and was corrected in place rather
+	// than left as archaeology (codex round 10): internal/cli's PushResult
+	// now mirrors delivered_sessions, because `pad push --format json` was
+	// silently dropping a field this response documents at length. It
+	// still never SENDS target_session_id.
 	TargetSessionID string `json:"target_session_id,omitempty"`
 }
 
@@ -47,9 +55,10 @@ type pushResponse struct {
 	// (S1 presence registry, `target_session_id`-filtered if one was
 	// given) matched — PLAN-2558 S5, TASK-2588. This is a PREDICTION read
 	// from the registry, not a delivery receipt: it carries the exact
-	// same staleness window as GET /api/v1/sessions (session_presence.go's
+	// same staleness windows as GET /api/v1/sessions (session_presence.go's
 	// LiveSession doc comment — up to ~30s behind an ungracefully-dropped
-	// connection) and there is still no ack from the receiving side. A
+	// CLIENT, and on a Redis-backed deployment up to ~90s behind a dead
+	// server INSTANCE) and there is still no ack from the receiving side. A
 	// vanished or cross-user target_session_id is 0, the same as "nothing
 	// connected" — deliberately not a distinct error, so the CLI's pre-S5
 	// behavior is unchanged by construction. Since PLAN-2613 S1, an
@@ -68,21 +77,56 @@ type pushResponse struct {
 	// target isn't in that snapshot — see its doc comment — so a 0 here
 	// is never a race, it's a guarantee: nothing was sent.
 	//
-	// THAT GUARANTEE IS PER-PROCESS, and in a Redis-backed deployment
-	// that makes this count wrong in BOTH directions for a BROADCAST push
-	// (BUG-2651, codex round 3). The count comes from the local presence
-	// registry while the bus now delivers everywhere: with one armed
-	// session on each of two replicas, the replica handling the POST
-	// reports 1 and two sessions receive it; with none of its own, it
-	// reports 0 while a remote session receives it anyway.
+	// THAT GUARANTEE USED TO BE PER-PROCESS, which made this count wrong
+	// in BOTH directions for a BROADCAST push once BUG-2651 gave the bus
+	// cross-instance reach: the count came from the answering instance's
+	// presence registry while the bus delivered everywhere. With one armed
+	// session on each of two replicas the answering replica reported 1 and
+	// two received it; with none of its own it reported 0 while a remote
+	// session received it anyway.
 	//
-	// Filed as BUG-2698 alongside the targeted-push half. Not corrected
-	// here, because the fix is not a better count — it is the shared-state
-	// SessionPresence that PLAN-2558 S3 already gates on. Any local arithmetic would just be a more elaborate way of
-	// asking one replica what all of them are doing. Targeted pushes do
-	// not have the over-report half of this problem, for the unhappy
-	// reason that the same locality stops them being published at all.
-	DeliveredSessions int `json:"delivered_sessions"`
+	// CLOSED BY BUG-2698 for the INSTANCE-LOCALITY half: with PAD_REDIS_URL
+	// set, s.sessionPresence is the shared RedisSessionPresence, so this
+	// count is read from every instance's sessions rather than one
+	// instance's. It is still a PREDICTION with the staleness above — a
+	// shared registry does not make it a receipt.
+	//
+	// STILL AN UPPER BOUND, not a match count, and an earlier draft of this
+	// comment claimed otherwise (codex round 6). deliveredSessionCount
+	// filters on user, armed, and target id. Actual delivery ALSO applies
+	// each stream's own visibility — watchNotificationVisible's
+	// vis.allows(CollectionID, ItemID), computed per connection from the
+	// credentials that opened it. Two streams of the SAME user can differ
+	// there: a cookie session and a workspace-scoped API token do not see
+	// the same collections. So a session counted here can still drop the
+	// push, and a targeted one at such a session is published and lost
+	// while this field reports 1.
+	//
+	// Pre-existing and unchanged by BUG-2698 — the in-process count had the
+	// identical blind spot — but tracked now rather than left implied
+	// (BUG-2725). Fixing it means the registry carrying per-session
+	// visibility, which has its own staleness question, since access can be
+	// revoked while the connection is held open.
+	//
+	// Note this was never wrong in the over-reporting direction for a
+	// TARGETED push, for the unhappy reason that the same locality stopped
+	// those from being published at all.
+	//
+	// NULL MEANS "PUBLISHED, COUNT UNKNOWN" — never "zero" (codex round 1
+	// P1). It is emitted only when the presence registry could not be READ
+	// (a Redis outage) on a BROADCAST push: the publish still happened,
+	// because presence never gated a broadcast, but there is no honest
+	// number to put here and 0 would claim nobody received it. A TARGETED
+	// push in that state is refused with a 503 instead and never reaches
+	// this struct, so a null is always a broadcast.
+	//
+	// Consumers: treat null as "unknown", not as falsy. Both clients carry
+	// it — internal/cli's PushResult mirrors it as a *int (untagged, so
+	// `pad push --format json` prints an explicit null rather than
+	// omitting the key), and the web's type is `number | null | undefined`
+	// with all three states documented there. An ABSENT key is a third
+	// thing again: a server predating session targeting.
+	DeliveredSessions *int `json:"delivered_sessions"`
 }
 
 // maxPushTargetSessionIDLen bounds target_session_id (dispatcher review
@@ -118,7 +162,9 @@ const maxPushMessageLen = 4096
 // handlePushToItem publishes a self-addressed watchevents.KindPush
 // notification (IDEA-2544 Phase 1) — the "push this to my agent" verb:
 // an explicit, user-authored instruction bound to an item, delivered to
-// every one of the pushing user's OWN connected monitor sessions via
+// each of the pushing user's OWN monitor sessions that is ACCEPTING pushes
+// and can see the item — a connected session that hasn't opted in, or that
+// lacks access, does not receive it — via
 // GET /api/v1/events/stream, or to exactly one of them when the request
 // names a target_session_id (PLAN-2558 S5, TASK-2588). Unlike watch/
 // assignment notifications, this has no durable backing (Dave's product
@@ -219,8 +265,8 @@ func (s *Server) handlePushToItem(w http.ResponseWriter, r *http.Request) {
 	// round 1, codex — see DeliveredSessions' doc comment for the race a
 	// post-publish count had). For a TARGETED push whose id isn't in this
 	// snapshot, the publish is skipped entirely: session ids are
-	// per-connection and never reused (session_presence.go's
-	// MemorySessionPresence.Add mints a fresh uuid per Add call), so a
+	// per-connection and never reused (both SessionPresence
+	// implementations mint a fresh uuid per Add call), so a
 	// target absent right now can never later be matched by the SAME
 	// connection reconnecting under that id, nor by the bus's replay
 	// buffer (which only serves a resumed connection presenting its own
@@ -234,26 +280,71 @@ func (s *Server) handlePushToItem(w http.ResponseWriter, r *http.Request) {
 	// delivery happens, which is the same staleness every presence
 	// answer on this surface already carries.
 	//
-	// THE "GUARANTEED NO-OP" PREMISE IS NOW MEMORYBUS-ONLY (BUG-2651,
-	// codex round 2). It rested on the bus being in-process: a target
-	// this instance cannot see was, by construction, a target nobody
-	// could deliver to. With watchevents.RedisBus the notification would
-	// reach every instance, so a session held on another one WOULD match
-	// it — and this skip is what stops that, turning a deliverable push
-	// into the no-op the comment describes rather than merely declining
-	// to record one.
+	// THAT PREMISE BROKE ONCE AND IS NOW RESTORED, which is worth spelling
+	// out because the skip below looks like a bug at a glance. It rested
+	// on the bus being in-process: a target THIS instance could not see
+	// was, by construction, a target nobody could deliver to. BUG-2651's
+	// RedisBus made the notification reach every instance, at which point
+	// a session held on another one WOULD have matched it — and this skip
+	// was the only thing stopping it, turning a deliverable push into the
+	// no-op the comment describes (BUG-2698).
 	//
-	// Deliberately NOT changed here. Publishing unconditionally would fix
-	// targeted cross-instance delivery and immediately make the
-	// delivered_sessions=0 in the response a lie in the other direction,
-	// which is a question about what that field promises rather than a
-	// bug in this line. It belongs with the shared-state SessionPresence
-	// that PLAN-2558 S3 already gates on — fixing the registry makes the
-	// snapshot right, and then this skip is correct again for the same
-	// reason it was originally.
-	deliveredSessions := deliveredSessionCount(s.sessionPresence, userID, targetSessionID)
+	// BUG-2698 fixed it at the registry rather than here. With
+	// PAD_REDIS_URL set, s.sessionPresence is the shared
+	// RedisSessionPresence, so "absent from this snapshot" means absent
+	// from EVERY instance again, and the skip is correct for exactly the
+	// reason it was originally written. Note that the shortcut — publish
+	// unconditionally for targeted pushes — would have fixed delivery and
+	// immediately made delivered_sessions=0 a lie in the other direction.
+	// The line below is deliberately unchanged by that fix.
+	//
+	// One detail the "never reused" argument now leans on more heavily:
+	// both implementations mint a fresh uuid per Add
+	// (MemorySessionPresence.Add, RedisSessionPresence.Add), so a
+	// reconnecting client never returns under a previous id on either.
+	deliveredSessionsUnknown := false
+	deliveredSessions, presenceErr := deliveredSessionCount(s.sessionPresence, userID, targetSessionID)
+	if presenceErr != nil {
+		// PRESENCE GATES A TARGETED PUSH BUT ONLY COUNTS A BROADCAST, and
+		// that asymmetry is exactly why an unreadable registry gets two
+		// different answers rather than one uniform refusal (codex round 1
+		// P1; dispatcher ruling).
+		//
+		// TARGETED: the gate cannot be evaluated at all. Publishing would
+		// deliver but report a count we do not have; skipping silently is
+		// the original bug. So refuse — 503 `unavailable`, nothing
+		// published, which is true and is already the code the web client
+		// treats as safe to resend.
+		if targetSessionID != "" {
+			slog.Warn("push refused: cannot read session presence to evaluate the target",
+				"item_ref", item.Ref, "error", presenceErr)
+			writeError(w, http.StatusServiceUnavailable, "unavailable",
+				"Push is not available right now — the target session could not be resolved, so nothing was sent")
+			return
+		}
+		// BROADCAST: presence never gated this — the publish is
+		// unconditional and always has been. Refusing here would
+		// manufacture an outage for a delivery that would have succeeded,
+		// coupling push availability to presence availability, which is a
+		// dependency that does not otherwise exist. Publish, and report the
+		// count as UNKNOWN rather than as 0.
+		slog.Warn("push: session presence unreadable, broadcasting with an unknown delivery count",
+			"item_ref", item.Ref, "error", presenceErr)
+		deliveredSessionsUnknown = true
+	}
 	if targetSessionID == "" || deliveredSessions > 0 {
-		s.watchEvents.Publish(watchevents.Notification{
+		// The ONE direct Bus.Publish call in this package, and the only
+		// one that acts on the result (BUG-2699 — see
+		// publishWatchNotification's doc comment for the six that
+		// deliberately don't, and publishSitesAreRuled_test.go for the
+		// check that keeps that split true).
+		//
+		// A push has no durable backing whatsoever: no inbox, nothing to
+		// read back, not even a store row carrying the same fact the way
+		// a comment notification has. If the publish is refused, the
+		// instruction is gone, and the caller is the only one who can do
+		// anything about that.
+		if err := s.watchEvents.Publish(watchevents.Notification{
 			WorkspaceID:     workspaceID,
 			ItemID:          item.ID,
 			CollectionID:    item.CollectionID,
@@ -264,16 +355,22 @@ func (s *Server) handlePushToItem(w http.ResponseWriter, r *http.Request) {
 			Summary:         message,
 			TargetUserID:    userID,
 			TargetSessionID: targetSessionID,
-		})
+		}); err != nil {
+			writePushPublishError(w, err, item.Ref)
+			return
+		}
 	}
 
-	writeJSON(w, http.StatusOK, pushResponse{
-		Ref:               item.Ref,
-		Workspace:         ws.Slug,
-		Pushed:            true,
-		Message:           message,
-		DeliveredSessions: deliveredSessions,
-	})
+	resp := pushResponse{
+		Ref:       item.Ref,
+		Workspace: ws.Slug,
+		Pushed:    true,
+		Message:   message,
+	}
+	if !deliveredSessionsUnknown {
+		resp.DeliveredSessions = &deliveredSessions
+	}
+	writeJSON(w, http.StatusOK, resp)
 }
 
 // deliveredSessionCount answers "how many of userID's own live sessions
@@ -302,11 +399,23 @@ func (s *Server) handlePushToItem(w http.ResponseWriter, r *http.Request) {
 // The publish-skip logic below reads this same filtered count, so a
 // targeted push at an unarmed session is skipped for the identical
 // reason a targeted push at a vanished one is: a guaranteed no-op.
-func deliveredSessionCount(presence SessionPresence, userID, targetSessionID string) int {
+//
+// UNREADABLE PRESENCE IS NOT ZERO (codex round 1, P1). The error is
+// returned rather than folded into a 0, because 0 is a load-bearing
+// answer on this path: it makes the caller SKIP the publish for a
+// targeted push. Reporting 0 for a registry that could not be read would
+// therefore drop the instruction and answer success — the same defect
+// BUG-2698 filed, arriving through the fix for it. A nil registry still
+// yields (0, nil): that is a server built without presence, which is a
+// known configuration rather than an unknown state.
+func deliveredSessionCount(presence SessionPresence, userID, targetSessionID string) (int, error) {
 	if presence == nil {
-		return 0
+		return 0, nil
+	}
+	sessions, err := presence.ListForUser(userID)
+	if err != nil {
+		return 0, err
 	}
-	sessions := presence.ListForUser(userID)
 	count := 0
 	for _, sess := range sessions {
 		if !sess.Armed {
@@ -317,5 +426,60 @@ func deliveredSessionCount(presence SessionPresence, userID, targetSessionID str
 		}
 		count++
 	}
-	return count
+	return count, nil
+}
+
+// pushPublishUnconfirmedCode is the error code for a push whose publish
+// failed in a way that does NOT prove the notification went nowhere
+// (BUG-2699).
+//
+// It is deliberately absent from the web client's
+// PUSH_PRE_PUBLISH_ERROR_CODES allow-list (web/src/lib/push/dispatch.ts),
+// and that absence is the entire behaviour: isPrePublishRefusal routes
+// every unrecognised code to the dialog's outcome-unknown branch, whose
+// copy is "we can't tell whether this was sent — pushing twice would
+// deliver it twice", and which does NOT re-arm the send button. That is
+// the honest UI for this case, so the code must stay off that list. Do
+// not "tidy" it on there.
+const pushPublishUnconfirmedCode = "push_unconfirmed"
+
+// writePushPublishError maps a Bus.Publish failure onto the response,
+// keeping the two outcomes Bus.Publish distinguishes distinguishable all
+// the way to the caller (BUG-2699). Collapsing them is the actual hazard
+// here: one of them is safe to resend and the other can duplicate a
+// dispatch into an agent harness.
+//
+//   - ErrBusClosed — the bus was already shut down, so nothing was
+//     published and nothing could have been. 503 `unavailable`, the SAME
+//     code the nil-bus branch at the top of handlePushToItem already
+//     returns, because the caller's situation is identical: push is not
+//     available right now, nothing went out, try again against a live
+//     server. Reusing that code is also what makes the web surface
+//     correct with no change at all — `unavailable` is already on its
+//     pre-publish-refusal allow-list.
+//   - anything else — UNCONFIRMED. go-redis retries a command whose
+//     reply was lost to a network error (the reason RedisBus's publish
+//     script carries a dedupe token at all), so the script may have run
+//     and published while the call still returned non-nil. 502 with
+//     pushPublishUnconfirmedCode, which is deliberately NOT on that
+//     allow-list.
+//
+// Neither branch writes a pushResponse. `Pushed` documents itself as
+// "accepted and processed", which is exactly what did not happen — and a
+// 200 body with pushed:false would let a caller that reads only the
+// status code (the CLI's `pad push` does: cmd_push.go returns on the
+// error and prints nothing otherwise) treat a lost instruction as sent.
+func writePushPublishError(w http.ResponseWriter, err error, itemRef string) {
+	if errors.Is(err, watchevents.ErrBusClosed) {
+		slog.Warn("push refused: notification bus is closed, nothing was published",
+			"item_ref", itemRef, "error", err)
+		writeError(w, http.StatusServiceUnavailable, "unavailable",
+			"Push is not available right now — the notification was not sent")
+		return
+	}
+	slog.Error("push publish failed with an unconfirmed outcome — the notification may or may not have been delivered",
+		"item_ref", itemRef, "error", err)
+	writeError(w, http.StatusBadGateway, pushPublishUnconfirmedCode,
+		"The push could not be confirmed — it may or may not have been delivered. "+
+			"Check your agent session before sending it again; pushing twice would deliver it twice.")
 }

internal/server/handlers_push_cross_instance_test.go

@@ -0,0 +1,231 @@
+package server
+
+import (
+	"encoding/json"
+	"net/http"
+	"sync"
+	"testing"
+
+	"github.com/alicebob/miniredis/v2"
+	"github.com/redis/go-redis/v9"
+
+	"github.com/PerpetualSoftware/pad/internal/watchevents"
+)
+
+// BUG-2698 at the layer that OWNS the caller.
+//
+// session_presence_redis_test.go proves the registry crosses instances.
+// That is a different claim from "the push endpoint now delivers
+// cross-instance", and conflating the two is the exact mistake the day-49
+// review caught one unit ago: a knob tested where it is CONSUMED proves
+// the knob works, not that the handler turns it.
+//
+// So these drive the real failure scenario through HTTP:
+//
+//	1. A user's agent session connects to replica B and registers there.
+//	2. The load balancer sends their push, naming that session id, to
+//	   replica A.
+//	3. A's registry did not contain the session, so deliveredSessionCount
+//	   returned 0 and the handler SKIPPED the publish entirely, answering
+//	   200 pushed:true delivered_sessions:0. B's session received nothing.
+
+// recordingBus records what the handler put on the bus. Recording rather
+// than delivering is deliberate: the notification's existence and its
+// TargetSessionID are what distinguish "published, for whichever instance
+// holds the target to pick up" from "skipped", and the shared Redis bus
+// (BUG-2651) is what carries it from there. Reproducing that fan-out here
+// would test BUG-2651 again, not this fix.
+type recordingBus struct {
+	stubBus
+	mu      sync.Mutex
+	targets []string
+}
+
+func (b *recordingBus) Publish(n watchevents.Notification) error {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	b.targets = append(b.targets, n.TargetSessionID)
+	return nil
+}
+
+func (b *recordingBus) count() int {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return len(b.targets)
+}
+
+func (b *recordingBus) targetSessionIDs() []string {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return append([]string(nil), b.targets...)
+}
+
+// TestPushToItem_TargetedAtSessionOnAnotherInstance is the defect: with a
+// SHARED registry, replica A must publish a push aimed at a session that
+// only replica B holds, and must count it as delivered.
+func TestPushToItem_TargetedAtSessionOnAnotherInstance(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+
+	// Replica A: the one answering the POST. Its presence registry is
+	// Redis-backed and it has never seen the session.
+	srvA := testServer(t)
+	busA := &recordingBus{}
+	srvA.SetWatchEventsBus(busA)
+	presenceA := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(presenceA.Close)
+	srvA.SetSessionPresence(presenceA)
+
+	slug, item, tok, user := setupWatchTestUser(t, srvA)
+
+	// Replica B: a different process, a different registry object, the same
+	// Redis. This is where the agent's stream is actually held.
+	presenceB := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(presenceB.Close)
+	sessionOnB := presenceB.Add(user.ID, SessionIdentity{Label: "docapp", Armed: true})
+
+	rr := bearerJSON(t, srvA, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token,
+		map[string]interface{}{"message": "triage this", "target_session_id": sessionOnB})
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("push: %d %s", rr.Code, rr.Body.String())
+	}
+	var resp pushResponse
+	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("parse response: %v", err)
+	}
+
+	// The WRONG behaviour's fingerprint, asserted directly (CONVE-12): the
+	// broken handler answered 200 with delivered_sessions:0 having put
+	// nothing on the bus. "The push worked" is not observable here — the
+	// receiving stream lives in another process — so what is asserted is
+	// what the skip would have left behind.
+	if got := busA.count(); got != 1 {
+		t.Fatalf("replica A must publish a push aimed at a session held by B; published %d notifications", got)
+	}
+	if got := busA.targetSessionIDs()[0]; got != sessionOnB {
+		t.Fatalf("published notification targeted %q, want %q", got, sessionOnB)
+	}
+	if deliveredCount(t, resp) != 1 {
+		t.Fatalf("delivered_sessions = %d, want 1 — the count is what the CLI and the web dialog read",
+			deliveredCount(t, resp))
+	}
+}
+
+// TestPushToItem_TargetedAtSessionOnAnotherInstance_MemoryRegistryDrops is
+// the NEGATIVE CONTROL, and it is what makes the test above mean
+// something: the identical scenario against per-process presence must
+// still drop, because that IS the bug. It also documents the behaviour a
+// self-hosted single-process deployment keeps.
+func TestPushToItem_TargetedAtSessionOnAnotherInstance_MemoryRegistryDrops(t *testing.T) {
+	t.Parallel()
+	srvA := testServer(t)
+	busA := &recordingBus{}
+	srvA.SetWatchEventsBus(busA)
+	srvA.SetSessionPresence(NewMemorySessionPresence())
+
+	slug, item, tok, user := setupWatchTestUser(t, srvA)
+
+	// "Replica B" — an entirely separate in-memory registry, which is what
+	// a second process has.
+	presenceB := NewMemorySessionPresence()
+	sessionOnB := presenceB.Add(user.ID, SessionIdentity{Label: "docapp", Armed: true})
+
+	rr := bearerJSON(t, srvA, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token,
+		map[string]interface{}{"message": "triage this", "target_session_id": sessionOnB})
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("push: %d %s", rr.Code, rr.Body.String())
+	}
+	if got := busA.count(); got != 0 {
+		t.Fatalf("per-process presence cannot see B's session, so the publish must still be skipped; got %d", got)
+	}
+}
+
+// TestPushToItem_BroadcastCountsSessionsOnEveryInstance covers the OTHER
+// half BUG-2698 filed: delivered_sessions was a local count describing a
+// global delivery. With a shared bus, a broadcast published on A reaches
+// B's sessions — so a count that only saw A's under-reported.
+func TestPushToItem_BroadcastCountsSessionsOnEveryInstance(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+
+	srvA := testServer(t)
+	busA := &recordingBus{}
+	srvA.SetWatchEventsBus(busA)
+	presenceA := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(presenceA.Close)
+	srvA.SetSessionPresence(presenceA)
+
+	slug, item, tok, user := setupWatchTestUser(t, srvA)
+
+	// One session on the answering replica, two on another.
+	presenceA.Add(user.ID, SessionIdentity{Label: "local", Armed: true})
+	presenceB := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(presenceB.Close)
+	presenceB.Add(user.ID, SessionIdentity{Label: "remote-1", Armed: true})
+	presenceB.Add(user.ID, SessionIdentity{Label: "remote-2", Armed: true})
+
+	// An UNARMED session on B, which must NOT be counted: it cannot receive
+	// a push at all (watchNotificationVisible denies it), and counting it
+	// would trade one honesty gap for another.
+	presenceB.Add(user.ID, SessionIdentity{Label: "remote-unarmed", Armed: false})
+
+	rr := bearerJSON(t, srvA, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token,
+		map[string]interface{}{"message": "triage this"})
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("push: %d %s", rr.Code, rr.Body.String())
+	}
+	var resp pushResponse
+	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("parse response: %v", err)
+	}
+	if deliveredCount(t, resp) != 3 {
+		t.Fatalf("delivered_sessions = %d, want 3 (1 local + 2 remote armed, unarmed excluded)", deliveredCount(t, resp))
+	}
+}
+
+// TestListSessions_ShowsSessionsFromEveryInstance covers the third
+// consumer BUG-2698 names, and the one a user meets first: the web push
+// dialog's target picker reads GET /api/v1/sessions, so a session on
+// another replica was not merely mis-counted — it could not be SELECTED.
+func TestListSessions_ShowsSessionsFromEveryInstance(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+
+	srvA := testServer(t)
+	presenceA := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(presenceA.Close)
+	srvA.SetSessionPresence(presenceA)
+
+	_, _, tok, user := setupWatchTestUser(t, srvA)
+
+	presenceB := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(presenceB.Close)
+	remoteID := presenceB.Add(user.ID, SessionIdentity{Label: "remote", Armed: true})
+
+	rr := bearerCall(t, srvA, "GET", "/api/v1/sessions", tok.Token, nil)
+	if rr.Code != http.StatusOK {
+		t.Fatalf("list sessions: %d %s", rr.Code, rr.Body.String())
+	}
+	if !containsSessionID(t, rr.Body.Bytes(), remoteID) {
+		t.Fatalf("the picker must offer a session held by another replica; body: %s", rr.Body.String())
+	}
+}
+
+func containsSessionID(t *testing.T, body []byte, id string) bool {
+	t.Helper()
+	var payload struct {
+		Sessions []LiveSession `json:"sessions"`
+	}
+	if err := json.Unmarshal(body, &payload); err != nil {
+		t.Fatalf("parse sessions payload: %v (body: %s)", err, body)
+	}
+	for _, s := range payload.Sessions {
+		if s.ID == id {
+			return true
+		}
+	}
+	return false
+}

internal/server/handlers_push_presence_unreadable_test.go

@@ -0,0 +1,196 @@
+package server
+
+import (
+	"encoding/json"
+	"errors"
+	"net/http"
+	"testing"
+)
+
+// BUG-2698 codex round 1 P1 — an unreadable presence registry.
+//
+// THE SPLIT RULE these pin: presence GATES a targeted push but only
+// COUNTS a broadcast, so an unreadable registry gets two different
+// answers rather than one uniform refusal.
+//
+//   - targeted: the gate cannot be evaluated → refuse, publish NOTHING.
+//   - broadcast: the publish was never gated → publish, and report the
+//     count as null (unknown), never as 0.
+//
+// Both assert what the WRONG behaviour would DO, per CONVE-12. For the
+// targeted leg that is a publish that should not exist; for the broadcast
+// leg it is a `0` that would tell the caller nobody received a message
+// that in fact went out.
+
+// unreadablePresence is a SessionPresence whose reads fail. Add/Remove
+// still work, because the failure being modelled is a Redis read outage,
+// not a broken registry object.
+type unreadablePresence struct {
+	MemorySessionPresence
+}
+
+func (p *unreadablePresence) ListForUser(string) ([]LiveSession, error) {
+	return nil, errors.New("redis: connection refused")
+}
+
+func newUnreadablePresence() *unreadablePresence {
+	return &unreadablePresence{MemorySessionPresence: *NewMemorySessionPresence()}
+}
+
+// TestPushToItem_TargetedWithUnreadablePresenceIsRefused: the handler
+// cannot tell whether the named session exists, so it must not guess in
+// either direction. Refuse, publish nothing, and say so with a code the
+// caller can safely resend on.
+func TestPushToItem_TargetedWithUnreadablePresenceIsRefused(t *testing.T) {
+	t.Parallel()
+	srv := testServer(t)
+	bus := &recordingBus{}
+	srv.SetWatchEventsBus(bus)
+	srv.SetSessionPresence(newUnreadablePresence())
+	slug, item, tok, _ := setupWatchTestUser(t, srv)
+
+	rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token,
+		map[string]interface{}{"message": "triage this", "target_session_id": "some-session-id"})
+
+	if rr.Code != http.StatusServiceUnavailable {
+		t.Fatalf("expected 503, got %d (body: %s)", rr.Code, rr.Body.String())
+	}
+	// THE LOAD-BEARING ASSERTION. A 503 alone would also pass for a
+	// handler that published first and then errored — which is precisely
+	// the outcome the code must not produce, because the caller is told
+	// nothing was sent and would resend.
+	if got := bus.count(); got != 0 {
+		t.Fatalf("a refused targeted push must publish nothing; published %d", got)
+	}
+	if code := errorCodeOf(t, rr.Body.Bytes()); code != "unavailable" {
+		t.Fatalf("expected code %q (the web allow-list entry that re-arms the send), got %q", "unavailable", code)
+	}
+}
+
+// TestPushToItem_BroadcastWithUnreadablePresenceStillPublishes: refusing
+// here would manufacture an outage — presence never gated a broadcast, so
+// the delivery would have succeeded. Publish, and report the count as
+// unknown.
+func TestPushToItem_BroadcastWithUnreadablePresenceStillPublishes(t *testing.T) {
+	t.Parallel()
+	srv := testServer(t)
+	bus := &recordingBus{}
+	srv.SetWatchEventsBus(bus)
+	srv.SetSessionPresence(newUnreadablePresence())
+	slug, item, tok, _ := setupWatchTestUser(t, srv)
+
+	rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token,
+		map[string]interface{}{"message": "triage this"})
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d (body: %s)", rr.Code, rr.Body.String())
+	}
+	if got := bus.count(); got != 1 {
+		t.Fatalf("a broadcast must still publish when presence is unreadable; published %d", got)
+	}
+
+	var resp pushResponse
+	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("parse response: %v", err)
+	}
+	// NULL, not 0 — and the two are asserted separately because a `0`
+	// would deserialize into a non-nil pointer and quietly satisfy any
+	// check written as "not more than zero". A 0 here would tell the
+	// caller nobody received a notification that was in fact published.
+	if resp.DeliveredSessions != nil {
+		t.Fatalf("delivered_sessions must be null when the registry could not be read, got %d", *resp.DeliveredSessions)
+	}
+	// And the raw wire, because the Go struct could round-trip a null the
+	// JSON did not actually carry (an absent key unmarshals to nil too).
+	var raw map[string]json.RawMessage
+	if err := json.Unmarshal(rr.Body.Bytes(), &raw); err != nil {
+		t.Fatalf("parse raw: %v", err)
+	}
+	got, present := raw["delivered_sessions"]
+	if !present {
+		t.Fatalf("delivered_sessions must be PRESENT and null, not omitted — an absent key is a different signal to the web client (it means a pre-S5 server); body: %s", rr.Body.String())
+	}
+	if string(got) != "null" {
+		t.Fatalf("delivered_sessions = %s, want null", got)
+	}
+}
+
+// TestPushToItem_ReadablePresenceStillReportsACount is the positive
+// control for both. Without it, a handler that reported null for every
+// push — or refused every targeted one — would pass the pair above.
+func TestPushToItem_ReadablePresenceStillReportsACount(t *testing.T) {
+	t.Parallel()
+	srv := testServer(t)
+	bus := &recordingBus{}
+	srv.SetWatchEventsBus(bus)
+	presence := NewMemorySessionPresence()
+	srv.SetSessionPresence(presence)
+	slug, item, tok, user := setupWatchTestUser(t, srv)
+	sessionID := presence.Add(user.ID, SessionIdentity{Armed: true})
+
+	rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token,
+		map[string]interface{}{"message": "triage this", "target_session_id": sessionID})
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d (body: %s)", rr.Code, rr.Body.String())
+	}
+	var resp pushResponse
+	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("parse response: %v", err)
+	}
+	if resp.DeliveredSessions == nil {
+		t.Fatal("a readable registry must report a real count, not null")
+	}
+	if *resp.DeliveredSessions != 1 {
+		t.Fatalf("delivered_sessions = %d, want 1", *resp.DeliveredSessions)
+	}
+	if got := bus.count(); got != 1 {
+		t.Fatalf("expected 1 publish, got %d", got)
+	}
+}
+
+// TestListSessions_UnreadablePresenceIs503: the same "I cannot tell" must
+// not reach the picker as an empty list. handleListSessions already 503s
+// for a registry that was never wired; a registry that cannot be READ is
+// the same answer to the caller and a worse lie if flattened, since the
+// dialog renders an empty list as "No agent session is connected".
+func TestListSessions_UnreadablePresenceIs503(t *testing.T) {
+	t.Parallel()
+	srv := testServer(t)
+	srv.SetSessionPresence(newUnreadablePresence())
+	_, _, tok, _ := setupWatchTestUser(t, srv)
+
+	rr := bearerCall(t, srv, "GET", "/api/v1/sessions", tok.Token, nil)
+
+	if rr.Code != http.StatusServiceUnavailable {
+		t.Fatalf("expected 503 for an unreadable registry, got %d (body: %s)", rr.Code, rr.Body.String())
+	}
+	// The wrong behaviour's fingerprint: a 200 whose body says zero
+	// sessions.
+	var payload struct {
+		Count int `json:"count"`
+	}
+	if err := json.Unmarshal(rr.Body.Bytes(), &payload); err == nil && rr.Code == http.StatusOK && payload.Count == 0 {
+		t.Fatal("an unreadable registry must not be reported as zero connected sessions")
+	}
+}
+
+// errorCodeOf pulls the error code out of whichever envelope shape the
+// server used, so a test asserts the CODE (what the web client keys on)
+// rather than only the status.
+func errorCodeOf(t *testing.T, body []byte) string {
+	t.Helper()
+	var envelope struct {
+		Error struct {
+			Code string `json:"code"`
+		} `json:"error"`
+		Code string `json:"code"`
+	}
+	if err := json.Unmarshal(body, &envelope); err != nil {
+		t.Fatalf("parse error envelope: %v (body: %s)", err, body)
+	}
+	if envelope.Code != "" {
+		return envelope.Code
+	}
+	return envelope.Error.Code
+}

internal/server/handlers_push_publish_result_test.go

@@ -0,0 +1,240 @@
+package server
+
+import (
+	"encoding/json"
+	"errors"
+	"net/http"
+	"strings"
+	"sync"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/watchevents"
+)
+
+// BUG-2699 — the push endpoint answering honestly for a publish that did
+// not happen, or that it cannot confirm happened.
+//
+// WHY THESE ASSERT WHAT THEY ASSERT (CONVE-12). The end state a naive
+// test would reach for is "the notification did not arrive", and that is
+// worthless here: it is equally true of the BROKEN behaviour, which
+// published nothing and returned 200 pushed:true. The observable
+// difference between fixed and unfixed lives entirely in the RESPONSE —
+// the wrong behaviour's fingerprint is a 200 carrying pushed:true — so
+// that is what these drive, with the status code and the body checked
+// separately rather than one standing in for the other.
+
+// stubBus is a watchevents.Bus whose Publish returns a caller-chosen
+// error. Everything else is inert: these tests never subscribe, and a
+// stub that silently satisfied the read side would invite a future test
+// to believe it had exercised delivery.
+type stubBus struct {
+	mu       sync.Mutex
+	err      error
+	attempts int
+}
+
+func (b *stubBus) Publish(n watchevents.Notification) error {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	b.attempts++
+	return b.err
+}
+
+func (b *stubBus) publishAttempts() int {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return b.attempts
+}
+
+func (b *stubBus) Subscribe() chan watchevents.Notification { return nil }
+func (b *stubBus) SubscribeAndReplaySince(int64) (chan watchevents.Notification, []watchevents.Notification) {
+	return nil, nil
+}
+func (b *stubBus) Unsubscribe(chan watchevents.Notification) {}
+func (b *stubBus) EventsSince(int64) []watchevents.Notification {
+	return nil
+}
+func (b *stubBus) Close() {}
+
+// TestPushToItem_ClosedBusIsRefused: ErrBusClosed proves nothing was
+// published, so the caller gets the same 503 `unavailable` the nil-bus
+// branch already returns — never a 200 with pushed:true.
+//
+// The `unavailable` CODE is asserted, not just the status: the web
+// client's PUSH_PRE_PUBLISH_ERROR_CODES allow-list keys off the code, and
+// it is what tells PushToAgentDialog this send is safe to re-offer. A 503
+// under some other code would silently move the UI onto its
+// outcome-unknown branch.
+func TestPushToItem_ClosedBusIsRefused(t *testing.T) {
+	t.Parallel()
+	srv := testServer(t)
+	bus := &stubBus{err: watchevents.ErrBusClosed}
+	srv.SetWatchEventsBus(bus)
+	slug, item, tok, _ := setupWatchTestUser(t, srv)
+
+	rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token,
+		map[string]interface{}{"message": "triage this"})
+
+	if rr.Code != http.StatusServiceUnavailable {
+		t.Fatalf("expected 503 for a closed bus, got %d (body: %s)", rr.Code, rr.Body.String())
+	}
+	// The wrong behaviour's fingerprint, asserted directly.
+	var resp pushResponse
+	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err == nil && resp.Pushed {
+		t.Fatalf("response claimed pushed:true for a refused publish: %s", rr.Body.String())
+	}
+	var envelope struct {
+		Error struct {
+			Code string `json:"code"`
+		} `json:"error"`
+		Code string `json:"code"`
+	}
+	if err := json.Unmarshal(rr.Body.Bytes(), &envelope); err != nil {
+		t.Fatalf("parse error envelope: %v (body: %s)", err, rr.Body.String())
+	}
+	code := envelope.Code
+	if code == "" {
+		code = envelope.Error.Code
+	}
+	if code != "unavailable" {
+		t.Fatalf("expected code %q (the web allow-list entry that re-arms the send), got %q (body: %s)",
+			"unavailable", code, rr.Body.String())
+	}
+	// Premise of this test, asserted rather than assumed: the handler did
+	// reach the publish. Without this the 503 above would also pass if
+	// some earlier validation had rejected the request, and the test would
+	// be named for a path it never took.
+	if got := bus.publishAttempts(); got != 1 {
+		t.Fatalf("expected exactly 1 publish attempt, got %d", got)
+	}
+}
+
+// TestPushToItem_UnconfirmedPublishIsNotReportedAsSent: any NON-closed
+// publish error means the notification may or may not have gone out (a
+// go-redis retry can publish and still return an error), so the response
+// must not claim success — and must NOT use a code on the web's
+// pre-publish allow-list, since re-offering the send could duplicate a
+// dispatch.
+func TestPushToItem_UnconfirmedPublishIsNotReportedAsSent(t *testing.T) {
+	t.Parallel()
+	srv := testServer(t)
+	bus := &stubBus{err: errors.New("redis publish: connection reset")}
+	srv.SetWatchEventsBus(bus)
+	slug, item, tok, _ := setupWatchTestUser(t, srv)
+
+	rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token,
+		map[string]interface{}{"message": "triage this"})
+
+	if rr.Code != http.StatusBadGateway {
+		t.Fatalf("expected 502 for an unconfirmed publish, got %d (body: %s)", rr.Code, rr.Body.String())
+	}
+	var resp pushResponse
+	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err == nil && resp.Pushed {
+		t.Fatalf("response claimed pushed:true for an unconfirmed publish: %s", rr.Body.String())
+	}
+	var envelope struct {
+		Error struct {
+			Code string `json:"code"`
+		} `json:"error"`
+		Code string `json:"code"`
+	}
+	if err := json.Unmarshal(rr.Body.Bytes(), &envelope); err != nil {
+		t.Fatalf("parse error envelope: %v (body: %s)", err, rr.Body.String())
+	}
+	code := envelope.Code
+	if code == "" {
+		code = envelope.Error.Code
+	}
+	if code != pushPublishUnconfirmedCode {
+		t.Fatalf("expected code %q, got %q (body: %s)", pushPublishUnconfirmedCode, code, rr.Body.String())
+	}
+	// The load-bearing half of this assertion, and the reason the code is
+	// checked at all: `unavailable` would send the web dialog down its
+	// re-arm path, offering a resend for a message that may already have
+	// been delivered.
+	if code == "unavailable" {
+		t.Fatalf("unconfirmed publish must not reuse the pre-publish-refusal code")
+	}
+	if got := bus.publishAttempts(); got != 1 {
+		t.Fatalf("expected exactly 1 publish attempt, got %d", got)
+	}
+	// NEVER retried by the server, for the same reason the CLI and the web
+	// client never retry a push: no idempotency key.
+}
+
+// TestPushToItem_SucceedsWhenPublishAccepted is the positive control for
+// both tests above. Without it, a handler that refused every push would
+// pass them, and they would be evidence of nothing.
+func TestPushToItem_SucceedsWhenPublishAccepted(t *testing.T) {
+	t.Parallel()
+	srv := testServer(t)
+	bus := &stubBus{err: nil}
+	srv.SetWatchEventsBus(bus)
+	slug, item, tok, _ := setupWatchTestUser(t, srv)
+
+	rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token,
+		map[string]interface{}{"message": "triage this"})
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("expected 200 for an accepted publish, got %d (body: %s)", rr.Code, rr.Body.String())
+	}
+	var resp pushResponse
+	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("parse response: %v", err)
+	}
+	if !resp.Pushed {
+		t.Fatalf("expected pushed:true on an accepted publish, got %s", rr.Body.String())
+	}
+	if got := bus.publishAttempts(); got != 1 {
+		t.Fatalf("expected exactly 1 publish attempt, got %d", got)
+	}
+}
+
+// TestBestEffortProducerSucceedsWhenThePublishFails — codex round 30
+// (coverage-gap sweep).
+//
+// publishWatchNotification's whole ruling is that a producer layered on a
+// committed store write DISCARDS a publish failure: the item exists, the
+// comment exists, and failing the caller's request over a lost
+// notification would tell the client its write failed when it did not.
+// Every test covered the SUCCESS path, so an implementation that
+// propagated the error and 500'd a perfectly good comment would have
+// passed.
+//
+// Asserts what the wrong behaviour would DO — a 5xx on the write, and a
+// comment missing afterwards — rather than that a notification was
+// absent, which is equally true of both behaviours.
+func TestBestEffortProducerSucceedsWhenThePublishFails(t *testing.T) {
+	t.Parallel()
+	srv := testServer(t)
+	bus := &stubBus{err: errors.New("redis publish: connection reset")}
+	srv.SetWatchEventsBus(bus)
+	slug, item, tok, _ := setupWatchTestUser(t, srv)
+
+	rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/comments", tok.Token,
+		map[string]interface{}{"body": "this comment must survive a failed notification"})
+
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("a failed watch notification must not fail the write it rides on; got %d (body: %s)",
+			rr.Code, rr.Body.String())
+	}
+
+	// And the write really committed — a 201 with nothing behind it would
+	// satisfy the check above.
+	list := bearerCall(t, srv, "GET", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/comments", tok.Token, nil)
+	if list.Code != http.StatusOK {
+		t.Fatalf("list comments: %d %s", list.Code, list.Body.String())
+	}
+	if !strings.Contains(list.Body.String(), "must survive a failed notification") {
+		t.Fatalf("the comment was not persisted: %s", list.Body.String())
+	}
+
+	// THE PREMISE, asserted rather than assumed (codex round 31, and my own
+	// rule about tests asserting their own premise). Without this, the two
+	// checks above pass for a handler that never publishes at all — no bus,
+	// the producer removed, the notification hook deleted — so the test
+	// would be named for a failed publish it never performed.
+	if got := bus.publishAttempts(); got != 1 {
+		t.Fatalf("expected the producer to attempt exactly 1 publish, got %d — this test proves nothing about a FAILED publish if none was attempted", got)
+	}
+}

internal/server/handlers_push_test.go

@@ -231,8 +231,8 @@ func TestPushToItem_TargetedSessionReceivesOnly(t *testing.T) {
 	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
 		t.Fatalf("parse response: %v", err)
 	}
-	if resp.DeliveredSessions != 1 {
-		t.Fatalf("expected delivered_sessions=1 for a session-targeted push, got %d", resp.DeliveredSessions)
+	if deliveredCount(t, resp) != 1 {
+		t.Fatalf("expected delivered_sessions=1 for a session-targeted push, got %d", deliveredCount(t, resp))
 	}
 
 	ev := waitForWatchEvent(t, chA, 3*time.Second)
@@ -280,8 +280,8 @@ func TestPushToItem_TargetedVanishedSessionMisses(t *testing.T) {
 	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
 		t.Fatalf("parse response: %v", err)
 	}
-	if resp.DeliveredSessions != 0 {
-		t.Fatalf("expected delivered_sessions=0 for a vanished target, got %d", resp.DeliveredSessions)
+	if deliveredCount(t, resp) != 0 {
+		t.Fatalf("expected delivered_sessions=0 for a vanished target, got %d", deliveredCount(t, resp))
 	}
 	if !resp.Pushed {
 		t.Fatal("expected pushed=true — a targeted miss is still a successfully PROCESSED push (matching broadcast's existing no-listeners semantics), even though nothing was published to the bus")
@@ -337,8 +337,8 @@ func TestPushToItem_TargetedUnarmedSessionTreatedAsMiss(t *testing.T) {
 	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
 		t.Fatalf("parse response: %v", err)
 	}
-	if resp.DeliveredSessions != 0 {
-		t.Fatalf("expected delivered_sessions=0 for a real but unarmed target, got %d", resp.DeliveredSessions)
+	if deliveredCount(t, resp) != 0 {
+		t.Fatalf("expected delivered_sessions=0 for a real but unarmed target, got %d", deliveredCount(t, resp))
 	}
 	if !resp.Pushed {
 		t.Fatal("expected pushed=true — an unarmed-target miss is still a successfully PROCESSED push, same shape as a vanished target")
@@ -405,8 +405,8 @@ func TestPushToItem_TargetedSessionOfAnotherUserTreatedAsVanished(t *testing.T)
 	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
 		t.Fatalf("parse response: %v", err)
 	}
-	if resp.DeliveredSessions != 0 {
-		t.Fatalf("expected delivered_sessions=0 for another user's session id — a cross-user id must be indistinguishable from a vanished one, got %d", resp.DeliveredSessions)
+	if deliveredCount(t, resp) != 0 {
+		t.Fatalf("expected delivered_sessions=0 for another user's session id — a cross-user id must be indistinguishable from a vanished one, got %d", deliveredCount(t, resp))
 	}
 	if after := len(srv.watchEvents.EventsSince(0)); after != before {
 		t.Fatalf("expected a cross-user target to skip the publish entirely, but the bus grew from %d to %d entries", before, after)
@@ -459,8 +459,8 @@ func TestPushToItem_TargetSessionIDAtCapAccepted(t *testing.T) {
 	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
 		t.Fatalf("parse response: %v", err)
 	}
-	if resp.DeliveredSessions != 0 {
-		t.Fatalf("expected delivered_sessions=0 — an at-cap id still names no live session, got %d", resp.DeliveredSessions)
+	if deliveredCount(t, resp) != 0 {
+		t.Fatalf("expected delivered_sessions=0 — an at-cap id still names no live session, got %d", deliveredCount(t, resp))
 	}
 }
 
@@ -495,8 +495,8 @@ func TestPushToItem_BroadcastDeliveredSessionsCountsLiveSessions(t *testing.T) {
 	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
 		t.Fatalf("parse response: %v", err)
 	}
-	if resp.DeliveredSessions != 2 {
-		t.Fatalf("expected delivered_sessions=2 for a broadcast with 2 live sessions, got %d", resp.DeliveredSessions)
+	if deliveredCount(t, resp) != 2 {
+		t.Fatalf("expected delivered_sessions=2 for a broadcast with 2 live sessions, got %d", deliveredCount(t, resp))
 	}
 
 	for _, ch := range []<-chan watchSSEEvent{chA, chB} {
@@ -550,8 +550,8 @@ func TestPushToItem_BroadcastDeliveredSessionsCountsArmedOnly(t *testing.T) {
 	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
 		t.Fatalf("parse response: %v", err)
 	}
-	if resp.DeliveredSessions != 1 {
-		t.Fatalf("expected delivered_sessions=1 (armed sessions only) out of 2 connected, got %d", resp.DeliveredSessions)
+	if deliveredCount(t, resp) != 1 {
+		t.Fatalf("expected delivered_sessions=1 (armed sessions only) out of 2 connected, got %d", deliveredCount(t, resp))
 	}
 
 	ev := waitForWatchEvent(t, chArmed, 3*time.Second)
@@ -592,7 +592,24 @@ func TestPushToItem_OmittedTargetSessionIDMatchesPreS5RequestShape(t *testing.T)
 	if resp.Ref != item.Ref || resp.Workspace != slug || !resp.Pushed || resp.Message != "triage this" {
 		t.Fatalf("pre-S5 fields must be unchanged for a pre-S5 request body, got %+v", resp)
 	}
-	if resp.DeliveredSessions != 0 {
-		t.Fatalf("expected delivered_sessions=0 with no presence registry wired, got %d", resp.DeliveredSessions)
+	if deliveredCount(t, resp) != 0 {
+		t.Fatalf("expected delivered_sessions=0 with no presence registry wired, got %d", deliveredCount(t, resp))
 	}
 }
+
+// deliveredCount reads pushResponse.DeliveredSessions, failing the test if
+// it is NULL.
+//
+// Null is not zero on this field — it means "published, but the presence
+// registry could not be read to count it" (BUG-2698, codex round 1 P1).
+// Asserting through this helper makes every existing test state its own
+// premise: it expected a REAL count, and a null would be a different
+// outcome rather than a 0 that happens to compare equal. The tests that
+// deliberately expect null assert on the raw pointer instead.
+func deliveredCount(t *testing.T, resp pushResponse) int {
+	t.Helper()
+	if resp.DeliveredSessions == nil {
+		t.Fatalf("delivered_sessions was null; this test expects a real count")
+	}
+	return *resp.DeliveredSessions
+}

internal/server/handlers_sessions.go

@@ -1,6 +1,9 @@
 package server
 
-import "net/http"
+import (
+	"log/slog"
+	"net/http"
+)
 
 // sessionsResponse is the body of GET /api/v1/sessions.
 //
@@ -66,6 +69,19 @@ func (s *Server) handleListSessions(w http.ResponseWriter, r *http.Request) {
 	//     exists to prevent.
 	w.Header().Set("Cache-Control", "private, no-store")
 
-	sessions := s.sessionPresence.ListForUser(user.ID)
+	sessions, err := s.sessionPresence.ListForUser(user.ID)
+	if err != nil {
+		// The SAME 503 the nil-registry branch above returns, and for the
+		// same reason stated there: a server that cannot tell who is
+		// listening has no degraded answer to give, and answering 200 with
+		// an empty list is a lie in exactly the direction this endpoint
+		// exists to prevent. The nil branch covers "never wired"; this one
+		// covers "wired but unreachable right now" — a distinction only an
+		// out-of-process registry can produce (codex round 1, P1 on
+		// BUG-2698), and one the caller must not have to guess at.
+		slog.Warn("sessions: presence read failed", "error", err, "user_id", user.ID)
+		writeError(w, http.StatusServiceUnavailable, "unavailable", "Session presence is not available")
+		return
+	}
 	writeJSON(w, http.StatusOK, sessionsResponse{Sessions: sessions, Count: len(sessions)})
 }

internal/server/handlers_watch_notify.go

@@ -2,6 +2,7 @@ package server
 
 import (
 	"fmt"
+	"log/slog"
 
 	"github.com/PerpetualSoftware/pad/internal/models"
 	"github.com/PerpetualSoftware/pad/internal/watchevents"
@@ -66,7 +67,7 @@ func (s *Server) publishWatchNotifications(workspaceID string, updated *models.I
 	sig := updated.LastMutation
 
 	if sig.StatusChanged {
-		s.watchEvents.Publish(watchevents.Notification{
+		s.publishWatchNotification(watchevents.Notification{
 			WorkspaceID:    workspaceID,
 			ItemID:         updated.ID,
 			CollectionID:   updated.CollectionID,
@@ -89,7 +90,7 @@ func (s *Server) publishWatchNotifications(workspaceID string, updated *models.I
 			}
 			summary = fmt.Sprintf("assigned to %s", name)
 		}
-		s.watchEvents.Publish(watchevents.Notification{
+		s.publishWatchNotification(watchevents.Notification{
 			WorkspaceID:    workspaceID,
 			ItemID:         updated.ID,
 			CollectionID:   updated.CollectionID,
@@ -111,3 +112,57 @@ func orNoneLabel(status string) string {
 	}
 	return status
 }
+
+// publishWatchNotification is the BEST-EFFORT publish path, and the one
+// ruling behind every non-push producer (BUG-2699).
+//
+// Bus.Publish reports acceptance since BUG-2699, which raised the
+// question of what each of the seven production call sites should DO
+// with that answer. Six of them — comment created, comment reply, item
+// created-with-assignee, comment-on-update, status change, assignment
+// change — publish a notification LAYERED ON A DURABLE STORE WRITE THAT
+// HAS ALREADY COMMITTED. The item exists, the comment exists, the
+// activity row exists, and the SSE event carrying the same fact went out
+// on a different bus. A watch notification that fails to publish costs a
+// subscriber one line about a fact that is still fully recoverable by
+// reading the item. Failing the caller's request over it would be the
+// wrong trade in the other direction: a 500 on a PATCH that already
+// committed tells the client its write failed when it did not.
+//
+// So these six discard the result — deliberately, and here rather than
+// six times over.
+//
+// DISCARDED, BUT NOT UNOBSERVED, and this helper is what makes that true
+// (codex round 10). An earlier version of this comment claimed both bus
+// implementations already log a failed publish, so discarding cost no
+// visibility. That is right for a transport failure and WRONG for the one
+// case most likely to matter: ErrBusClosed returns without logging in
+// either implementation, so a producer publishing into a bus closed during
+// shutdown vanished in silence. The helper logs it here instead — once,
+// at the layer that decided to ignore it.
+//
+// The SEVENTH site, handlePushToItem, does not use this helper and must
+// not: a push has no durable backing at all (no inbox, nothing to read
+// back), so a dropped publish loses the instruction outright and the
+// caller has to hear about it. That asymmetry is enforced structurally —
+// after BUG-2699 the push handler is the ONLY direct s.watchEvents.Publish
+// call in this package besides the one just below, and that split is
+// CHECKED rather than asserted: publish_sites_ruled_test.go enumerates
+// the call sites and fails by name when a new producer publishes
+// directly. A comment saying "don't do X" protects whoever reads it
+// before doing X, which is not the person this needs protecting from.
+//
+// Also absorbs the nil-bus check every one of those sites was repeating.
+func (s *Server) publishWatchNotification(n watchevents.Notification) {
+	if s.watchEvents == nil {
+		return
+	}
+	if err := s.watchEvents.Publish(n); err != nil {
+		// Warn, not Error: the durable write this notification sits on top
+		// of already committed, so nothing is lost that a reader cannot
+		// recover from the item itself. An operator still wants to see it,
+		// because a run of these means the bus is unhealthy.
+		slog.Warn("watch notification not published; the underlying write still committed",
+			"error", err, "kind", n.Kind, "item_ref", n.ItemRef)
+	}
+}

internal/server/publish_sites_ruled_test.go

@@ -0,0 +1,197 @@
+package server
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// BUG-2699 — the enforcement step for an invariant this package's own doc
+// comments assert.
+//
+// publishWatchNotification's doc comment says the push handler is the ONLY
+// direct Bus.Publish caller in this package, because it is the only site
+// whose notification has no durable backing to fall back on. A sentence in
+// a comment protects whoever reads that comment before adding a producer;
+// it does nothing about the producer added by someone who didn't. So the
+// split is checked here instead of asserted there.
+//
+// This is a SOURCE scan rather than a behavioural test on purpose: the
+// thing being protected is a call-site population, and a population claim
+// is checked by enumerating it. A new producer that calls
+// s.watchEvents.Publish directly fails this test by name and has to make
+// a deliberate choice — use the best-effort helper, or extend
+// allowedDirectPublishFiles with a reason.
+//
+// WHAT IT STILL CANNOT SEE, said plainly rather than left to be
+// discovered (codex round 7): a bus obtained indirectly — returned from a
+// method, passed in as a parameter, reached through an interface value
+// stored elsewhere. Catching those needs type information, not syntax.
+// The structural alternative is stronger and is the right answer if this
+// ever has to grow again: make the field unreachable outside the helper
+// rather than detectable. Until then this covers the forms a producer
+// would plausibly be written in, and it FAILS on them — which is more
+// than the doc comment it replaced could do.
+//
+// COUNTS, not just filenames (codex round 10). A filename allow-list lets
+// a future unsafe producer be added to an already-allowed file and pass —
+// while this test's own message claims new producers fail. Pinning the
+// expected number means a new direct publish in handlers_push.go fails
+// here too, and whoever adds it has to say why the count moved.
+type allowedPublishSite struct {
+	count  int
+	reason string
+}
+
+var allowedDirectPublishFiles = map[string]allowedPublishSite{
+	// The one caller that acts on the result: a push has no inbox, no
+	// store row, nothing to read back, so a refused publish loses the
+	// instruction outright and the caller has to be told.
+	// One call, in handlePushToItem.
+	"handlers_push.go": {1, "push has no durable backing; it maps the error onto the response"},
+	// The helper itself — where the best-effort discard is ruled, once,
+	// for every other producer.
+	// One call, inside publishWatchNotification itself.
+	"handlers_watch_notify.go": {1, "publishWatchNotification: the single best-effort discard"},
+}
+
+func TestWatchEventsPublishSitesAreRuled(t *testing.T) {
+	t.Parallel()
+
+	// AST, not a line grep (codex round 2, P2). The first version matched
+	// the literal string `s.watchEvents.Publish(` on a single line, which a
+	// producer could evade without trying: a local alias (`bus :=
+	// s.watchEvents`), a selector split across lines by gofmt, a receiver
+	// named anything else. A scanner that only catches the spelling it
+	// expects reports a clean package while the invariant is broken — the
+	// same class of false green this whole unit is about.
+	//
+	// Parsing resolves calls structurally: any call whose function is a
+	// selector `.Publish` on an expression that mentions the watchEvents
+	// field, however it is spelled or wrapped.
+	// Files enumerated and parsed individually rather than with
+	// parser.ParseDir, which is deprecated as of Go 1.25 (it ignores build
+	// tags). Per-file parsing needs no extra dependency, and this package
+	// has no build-tagged files for the tags to matter to.
+	fset := token.NewFileSet()
+	entries, err := os.ReadDir(".")
+	if err != nil {
+		t.Fatalf("read package dir: %v", err)
+	}
+
+	found := map[string]int{}
+	for _, entry := range entries {
+		name := entry.Name()
+		if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") {
+			continue
+		}
+		{
+			base := filepath.Base(name)
+			file, err := parser.ParseFile(fset, name, nil, 0)
+			if err != nil {
+				t.Fatalf("parse %s: %v", name, err)
+			}
+			ast.Inspect(file, func(n ast.Node) bool {
+				call, ok := n.(*ast.CallExpr)
+				if !ok {
+					return true
+				}
+				sel, ok := call.Fun.(*ast.SelectorExpr)
+				if !ok || sel.Sel.Name != "Publish" {
+					return true
+				}
+				// The receiver expression mentions the watchEvents field —
+				// `s.watchEvents`, `srv.watchEvents`, or anything that
+				// selects it. An ALIAS assigned to a local variable is the
+				// one shape this still cannot see; aliasDeclarations below
+				// covers that separately.
+				if mentionsWatchEventsField(sel.X) {
+					found[base]++
+				}
+				return true
+			})
+			// A local alias defeats the structural check above, so the
+			// alias itself is what gets flagged: any code that reads
+			// s.watchEvents into a variable is treated as a publish site,
+			// because the ruling is about which code may HOLD the bus, not
+			// about how the call is spelled.
+			//
+			// Both binding forms, not just `:=` (codex round 7): a
+			// `var bus = s.watchEvents` is a GenDecl/ValueSpec, not an
+			// AssignStmt, and the first version of this check walked only
+			// the latter.
+			ast.Inspect(file, func(n ast.Node) bool {
+				switch node := n.(type) {
+				case *ast.AssignStmt:
+					for _, rhs := range node.Rhs {
+						if isWatchEventsFieldSelector(rhs) {
+							found[base]++
+						}
+					}
+				case *ast.ValueSpec:
+					for _, v := range node.Values {
+						if isWatchEventsFieldSelector(v) {
+							found[base]++
+						}
+					}
+				}
+				return true
+			})
+		}
+	}
+
+	// POSITIVE CONTROL. Without it, a scanner that matched nothing — wrong
+	// directory, renamed method, a parse that quietly returned no files —
+	// would report a clean bill of health for a package it never read.
+	if found["handlers_push.go"] == 0 {
+		t.Fatal("scanner found no direct Publish call in handlers_push.go — the scan itself is broken, not the invariant")
+	}
+
+	for file, count := range found {
+		allowed, ok := allowedDirectPublishFiles[file]
+		if ok && count != allowed.count {
+			t.Errorf("%s has %d direct watchEvents.Publish site(s), expected %d (%q).\n"+
+				"A new producer in an already-allowed file is still a new producer: route it through "+
+				"s.publishWatchNotification, or update the expected count here with a reason.",
+				file, count, allowed.count, allowed.reason)
+			continue
+		}
+		if !ok {
+			t.Errorf("%s reaches watchEvents.Publish directly (%d site(s)).\n"+
+				"Producers layered on a committed store write must use s.publishWatchNotification, "+
+				"which rules the best-effort discard in one place (BUG-2699).\n"+
+				"If this site genuinely needs to act on the result, add it to allowedDirectPublishFiles with a reason.",
+				file, count)
+		}
+	}
+	for file, allowed := range allowedDirectPublishFiles {
+		if found[file] == 0 {
+			t.Errorf("%s is listed as an allowed direct Publish caller (%q) but has none — "+
+				"stale allow-list entry, remove it", file, allowed.reason)
+		}
+	}
+}
+
+// mentionsWatchEventsField reports whether expr reads the Server's
+// watchEvents field anywhere inside it, so a wrapped or parenthesised
+// receiver is still recognised.
+func mentionsWatchEventsField(expr ast.Expr) bool {
+	seen := false
+	ast.Inspect(expr, func(n ast.Node) bool {
+		if isWatchEventsFieldSelector(n) {
+			seen = true
+			return false
+		}
+		return true
+	})
+	return seen
+}
+
+func isWatchEventsFieldSelector(n ast.Node) bool {
+	sel, ok := n.(*ast.SelectorExpr)
+	return ok && sel.Sel != nil && sel.Sel.Name == "watchEvents"
+}

internal/server/session_presence.go

@@ -24,51 +24,60 @@ import (
 // carry a label (S2, TASK-2560) — turns the same data into the target
 // picker S5 needs.
 //
-// SINGLE-PROCESS LIMITATION, stated up front for the same reason
-// internal/watchevents states its own: MemorySessionPresence tracks
-// connections held open by THIS process only. In a multi-process padd
-// deployment (Pad Cloud) a session connected to instance A is invisible
-// to instance B, so /api/v1/sessions under-reports.
+// WHICH IMPLEMENTATION YOU GET, and what each one can and cannot see.
+// cmd/pad/cmd_server.go picks on PAD_REDIS_URL, the same switch both
+// buses use:
 //
-// THIS NOTE USED TO SAY presence and delivery were blind in the same
-// direction, and that the two had to be fixed together or not at all.
-// That was true when written and is no longer: BUG-2651 shipped
-// watchevents.RedisBus, so DELIVERY is now cross-instance WHEN
-// PAD_REDIS_URL IS SET — a push published on A reaches a stream held on
-// B, and B matches it against its own live sessions, which is the only
-// set B needs. Without that env var the deployment is single-process by
-// definition and none of this applies. What remains per-process in a
-// Redis-backed deployment is exactly this registry, and it is worth
-// being precise about what that costs, because the two halves fail
-// differently:
+//   - No PAD_REDIS_URL — MemorySessionPresence. The deployment is
+//     single-process by definition, so per-process presence is complete
+//     presence and nothing below applies.
+//   - PAD_REDIS_URL set — RedisSessionPresence (BUG-2698,
+//     session_presence_redis.go). Every instance reads and writes one
+//     shared registry, so a session held on B is visible to A.
 //
-//   - BROADCAST DELIVERY (fixed): a push with TargetUserID and no
-//     session id used to reach only the sessions on whichever instance
-//     handled the POST. It now reaches all of that user's sessions,
-//     wherever they are held.
-//   - TARGETED DELIVERY (still open, and NOT fixed by the bus): a push
-//     naming a specific session id is gated on THIS registry before it
-//     is published at all — see handlers_push.go, which skips the
-//     publish when the id is absent from the local snapshot. So a POST
-//     landing on A for a session held on B still delivers nothing. The
-//     bus would carry it; the gate means it is never put on the bus.
-//   - VISIBILITY (still open): GET /api/v1/sessions answers from ONE
-//     instance's registry, so a user whose session is held on B and who
-//     asks A sees nothing to target. Unchanged by BUG-2651 — neither
-//     better nor worse — so nothing regressed.
+// THAT SPLIT USED TO BE A DEFECT, and the history is worth keeping
+// because the three symptoms looked unrelated. BUG-2651 gave watchevents
+// a Redis bus, which made DELIVERY cross-instance while this registry
+// stayed per-process — and a shared bus with a per-process registry was
+// worse than either being consistent:
 //
-// The two open halves are the same defect wearing different clothes (filed
-// together as BUG-2698), and one implementation closes both: a shared-state SessionPresence makes
-// the snapshot right, which makes the picker complete AND makes the
-// push gate's premise true again. Do not put the web-UI push surface
-// (PLAN-2558 S3) in front of a multi-process deployment until it exists.
-// SessionPresence is an interface from day one so it can slot in without
-// touching the stream handler or the endpoint.
+//   - BROADCAST DELIVERY was fixed by the bus alone, but its REPORTED
+//     count was not: delivered_sessions counted the answering instance's
+//     sessions while the bus delivered to all of them.
+//   - TARGETED DELIVERY was not fixed by the bus at all, because
+//     handlers_push.go gates on THIS registry BEFORE publishing and
+//     skipped the publish entirely when the id was absent locally. The
+//     bus would have carried it; it was never put on the bus.
+//   - VISIBILITY was not fixed either: GET /api/v1/sessions answered
+//     from one instance's registry, so a user whose session was held on
+//     B and who asked A had nothing to select in the picker.
 //
-// (An earlier version of this note, written with BUG-2651, claimed
-// targeted delivery was fixed too. It was not: that claim was made from
-// reading the bus and this file without reading the push handler's gate,
-// and codex round 2 caught it.)
+// One shared registry closes all three, and in an order worth noticing:
+// making the registry global makes the snapshot right, which makes the
+// picker complete AND restores the push gate's original premise — the
+// skip then means what it was written to mean ("nothing is listening")
+// rather than "this instance cannot see who is listening". The tempting
+// shortcut of publishing unconditionally for targeted pushes would have
+// fixed delivery while making delivered_sessions:0 a lie in the other
+// direction.
+//
+// (Two earlier versions of this note were wrong in opposite directions —
+// one claimed BUG-2651 fixed targeted delivery, written from reading the
+// bus and this file without reading the push handler's gate; the one
+// before it claimed presence and delivery were blind in the same
+// direction and had to be fixed together. Kept as a reminder that a
+// claim about a path you have not read is a claim, not a caveat.)
+//
+// SessionPresence has been an interface since S1 precisely so the
+// shared-state implementation could slot in without rewriting the stream
+// handler or the endpoint, and that held: the STREAM HANDLER is unchanged.
+//
+// The ENDPOINT is not, and an earlier draft of this note claimed both were
+// (codex round 6). handleListSessions gained a 503 branch, because the
+// out-of-process implementation made "I could not find out" a reachable
+// answer that the in-process one never had — the interface absorbed the
+// implementation swap, not the new failure mode the implementation brought
+// with it. Those are different claims and only the first one was true.
 
 // LiveSession is one currently-connected user-scoped event stream —
 // i.e. one `GET /api/v1/events/stream` connection being held open.
@@ -84,6 +93,24 @@ import (
 // watchEventsKeepaliveInterval is 30s. So this list can name a listener
 // that is already gone, for up to ~30 seconds.
 //
+// A DEAD INSTANCE IS A SECOND, LONGER WINDOW, and it exists only for the
+// shared registry (codex round 1, P2 on BUG-2698). The ~30s above is
+// about a dead CLIENT, detected when the keepalive write fails and the
+// handler's defer deregisters. If the SERVER PROCESS dies instead,
+// MemorySessionPresence loses its entries instantly — they lived in the
+// process that died — while RedisSessionPresence's outlive it and clear
+// only when their TTL lapses, up to ~90s (sessionKeyTTL). During that
+// window a picker can offer a session on an instance that no longer
+// exists, and a push targeted at it will publish and reach nobody while
+// reporting one delivery.
+//
+// Documented rather than shortened, deliberately: a TTL close to the
+// renewal interval would start evicting LIVE sessions on any hiccup — a
+// GC pause, a briefly slow Redis — and an evicted live session is
+// invisible to the picker AND makes the push gate skip a genuinely
+// connected target. Do not fix a staleness window by creating an eviction
+// failure. See sessionKeyTTL for the arithmetic.
+//
 // That bound is acceptable for push, which is fire-and-forget either
 // way: a push to a session that died 5 seconds ago costs a message that
 // would have been lost regardless. It is NOT acceptable as a delivery
@@ -203,12 +230,29 @@ type SessionPresence interface {
 	Remove(userID string, sessionID string)
 	// ListForUser returns userID's live sessions, oldest connection
 	// first. The returned slice is a copy and is safe to retain.
-	ListForUser(userID string) []LiveSession
+	//
+	// THE ERROR IS NOT DECORATION, and an implementation that cannot fail
+	// must still not drop it (codex round 1, P1 on BUG-2698). Without it,
+	// "this user has no sessions" and "I could not find out" are the same
+	// value to every consumer — and they demand OPPOSITE handling: the
+	// first means a push has nothing to deliver to, the second means the
+	// caller must not conclude anything. An out-of-process implementation
+	// makes the difference reachable at runtime (a Redis outage), where
+	// returning an empty list would make handleListSessions answer 200
+	// with no sessions — the precise lie its 503 exists to avoid — and
+	// would make a TARGETED push skip its publish and lose the
+	// instruction while reporting success.
+	//
+	// MemorySessionPresence always returns a nil error; that is a property
+	// of that implementation, not of this contract.
+	ListForUser(userID string) ([]LiveSession, error)
 }
 
-// MemorySessionPresence is the in-process SessionPresence — see this
-// file's SINGLE-PROCESS LIMITATION note before deploying it behind more
-// than one padd process.
+// MemorySessionPresence is the in-process SessionPresence, and the one a
+// deployment gets when PAD_REDIS_URL is unset — which is also the only
+// deployment shape it is correct for. See this file's WHICH IMPLEMENTATION
+// YOU GET note before putting it behind more than one padd process;
+// RedisSessionPresence (session_presence_redis.go) is what that needs.
 type MemorySessionPresence struct {
 	mu sync.RWMutex
 	// byUser is userID -> sessionID -> session. Two levels rather than a
@@ -273,7 +317,7 @@ func (p *MemorySessionPresence) Remove(userID string, sessionID string) {
 // two connections that opened within the same clock tick (a real case
 // under a coarse monotonic clock, and an unstable list order would make
 // the S5 target picker jump around under the user's cursor).
-func (p *MemorySessionPresence) ListForUser(userID string) []LiveSession {
+func (p *MemorySessionPresence) ListForUser(userID string) ([]LiveSession, error) {
 	p.mu.RLock()
 	sessions := p.byUser[userID]
 	out := make([]LiveSession, 0, len(sessions))
@@ -288,5 +332,7 @@ func (p *MemorySessionPresence) ListForUser(userID string) []LiveSession {
 		}
 		return out[i].ConnectedAt.Before(out[j].ConnectedAt)
 	})
-	return out
+	// Always nil: an in-process map read cannot fail. See the interface's
+	// doc comment for why the error is in the signature anyway.
+	return out, nil
 }

internal/server/session_presence_redis.go

@@ -0,0 +1,789 @@
+package server
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"log/slog"
+	"sort"
+	"sync"
+	"time"
+
+	"github.com/google/uuid"
+	"github.com/redis/go-redis/v9"
+)
+
+// RedisSessionPresence is the shared-state SessionPresence (BUG-2698) —
+// the implementation session_presence.go's interface has been waiting for
+// since PLAN-2558 S1, and the one that makes a multi-instance deployment
+// tell the truth about who is listening.
+//
+// WHAT IT FIXES, precisely, because the obvious reading is too broad.
+// BUG-2651 gave watchevents a Redis bus, so a notification PUBLISHED on
+// any instance now reaches a stream held on any other. What stayed broken
+// is everything that depends on knowing WHICH sessions exist:
+//
+//   - A session-targeted push was resolved against the presence registry
+//     of whichever instance handled the POST, and handlers_push.go SKIPS
+//     the publish entirely when the target is absent from that snapshot.
+//     With per-process presence, a session connected to B was invisible to
+//     A, so a POST that the load balancer sent to A dropped the
+//     instruction and answered 200 delivered_sessions:0. Reachable through
+//     the intended flow: GET /api/v1/sessions answered by B lists the
+//     session, and the POST that follows is free to land on A.
+//   - delivered_sessions was wrong in BOTH directions for a broadcast
+//     push — a local count describing a global delivery.
+//   - GET /api/v1/sessions under-reported, so the web push dialog's target
+//     picker could not offer a session on another instance at all.
+//
+// One shared registry closes all three, and the ORDER matters: making the
+// registry global makes the snapshot right, which makes the picker
+// complete AND restores the push gate's original premise — at which point
+// the existing skip is correct for the reason it was written, rather than
+// being something to work around. Publishing unconditionally for targeted
+// pushes would have fixed delivery and immediately made
+// delivered_sessions:0 a lie in the other direction.
+//
+// THE REAPING STORY, which session_presence.go's interface doc requires of
+// any out-of-process implementation and does not let this one inherit.
+// MemorySessionPresence's entries die with the process that wrote them —
+// there is nothing to reap, and Server.Shutdown leaving SSE handlers (and
+// their presence entries) hanging is harmless for it. Redis entries
+// OUTLIVE their writer, so a crash or a hard kill would otherwise strand
+// them permanently, and the list would name sessions belonging to an
+// instance that no longer exists.
+//
+// This implementation reaps with Redis's own expiry rather than with
+// bookkeeping of its own:
+//
+//   - Each session is a key with a TTL (sessionKeyTTL), renewed every
+//     sessionRenewInterval by a goroutine that lives exactly as long as
+//     the connection's Add..Remove span. A process that dies stops
+//     renewing, and Redis deletes the entries — no sweeper, no instance
+//     ownership records, no startup scan.
+//   - The per-user index is a SET carrying the same TTL, written in the
+//     SAME atomic step as the entry it indexes (see writeScript), so a
+//     live session is never briefly absent from the only enumeration of
+//     it. A member whose session key has since EXPIRED is pruned by the
+//     next reader, so the index still self-heals without a background job.
+//
+// Native expiry is deliberate: the alternative — storing an expiry
+// timestamp per entry and filtering at read time — compares a timestamp
+// written by one instance's clock against another instance's clock, and
+// "two instances disagree about presence" is the exact bug class this type
+// exists to end.
+//
+// EVICTION LOOKS EXACTLY LIKE EXPIRY, and that is survivable by design
+// rather than by luck (codex round 15). Under a maxmemory policy that can
+// evict live keys — `allkeys-lru`, which the repo's own docker-compose
+// configures — Redis may drop a session entry or the index while the
+// session is still connected. Nothing here can tell that from a TTL
+// lapsing, so the session goes briefly unlisted and a targeted push at it
+// is skipped.
+//
+// The repair is already the renewal's job: renewLoop re-SETs the full
+// payload rather than issuing a bare EXPIRE, precisely so a renewal
+// RESTORES a vanished entry instead of no-oping against a missing key. So
+// an eviction costs at most one renewal interval of invisibility, and the
+// same mechanism covers a Redis restart. Operators who want to avoid even
+// that should keep Pad's Redis off an evicting policy — see
+// docs/deployment.md.
+//
+// A COMPROMISED REPLICA IS NOT IN THIS TYPE'S THREAT MODEL, and saying so
+// is more useful than implying otherwise (codex round 17). Anything holding
+// the shared Redis credentials can enumerate these keys, delete real
+// entries, insert fake armed sessions, or grow a victim's index without
+// bound — none of which this type can prevent, since it has no ownership
+// or integrity mechanism and adding one would not help against an attacker
+// who also holds the credentials.
+//
+// It is worth being precise that this does not WIDEN that exposure. The
+// same credentials already reach internal/watchevents' shared bus, where an
+// attacker can both READ every notification crossing the deployment —
+// including the instruction text of every user's pushes — and PUBLISH
+// arbitrary ones into any user's session. Reading presence metadata and
+// corrupting a picker are strictly smaller capabilities than injecting
+// instructions into someone's agent. The boundary that matters is the
+// Redis credential, and it is the same boundary it was before this type
+// existed.
+//
+// The authenticated-user case — one user holding unbounded streams — is a
+// different question, and this type does NOT answer it: see BUG-2726 for
+// the stream admission limit that would.
+//
+// SINGLE-NODE REDIS ONLY, like the rest of Pad's Redis integration:
+// cmd/pad/cmd_server.go dials with redis.NewClient, not a cluster client,
+// and this type's keys are not hash-tagged, so a user's index and entries
+// would land in different slots. That is the whole integration's
+// constraint rather than this type's, and changing it belongs with the
+// keyspace work in BUG-2724, not here — one keyspace growing hash tags the
+// others lack is the same divergence that note already forbids.
+//
+// STALENESS IS UNCHANGED, and must not be read as improved. A session
+// still disappears from this list only when its stream handler returns
+// (clean disconnect) or the keepalive write fails (up to ~30s after an
+// ungraceful one) — see LiveSession's doc comment. The TTL is a crash
+// backstop, not a liveness probe: it is deliberately several renewal
+// intervals long, so it never expires a session whose process is merely
+// busy. Consumers must keep treating this list as "connected as far as the
+// server can tell", never as a delivery guarantee.
+type RedisSessionPresence struct {
+	client *redis.Client
+
+	// sessionKeyTTL and renewInterval are fields rather than constants so
+	// tests can drive the expiry path without sleeping through it. Prefer
+	// NewRedisSessionPresence, which sets the production pair.
+	sessionKeyTTL time.Duration
+	renewInterval time.Duration
+	// opTimeout bounds every Redis call, defaulting to presenceOpTimeout.
+	// A field for the same reason the two above are: a test needs to drive
+	// the stalled-Redis path without waiting out the production bound.
+	opTimeout time.Duration
+	// drainTimeout bounds Close's wait for renewal goroutines, defaulting
+	// to closeDrainTimeout. A field so a test can park a renewal and assert
+	// Close still returns — see closeDrainTimeout for why that could not be
+	// asserted while it was a constant.
+	drainTimeout time.Duration
+
+	// onRenewWrite, when non-nil, is called by renewLoop immediately
+	// before each renewal write. Always nil in production — it exists so a
+	// test can hold a renewal INSIDE its write and prove that Remove waits
+	// for it (codex round 1, P2).
+	//
+	// A seam rather than a timing loop, deliberately: the probabilistic
+	// version of that test — 50µs renewal interval, 200 add/remove
+	// iterations — passed 3/3 against the UNFIXED Remove, so it was
+	// evidence of nothing. An instrument that cannot fail on broken code
+	// is not an instrument.
+	onRenewWrite func()
+
+	mu     sync.Mutex
+	closed bool
+	// renewLog rate-limits the renewal-failure warning. A Redis outage
+	// fails EVERY session's renewal on every tick, so at a thousand
+	// sessions the unthrottled version emitted ~33 warnings a second per
+	// replica — enough to page on volume while saying nothing an operator
+	// could act on, and enough to bury the errors that matter (codex round
+	// 33). One line per interval carries the same information plus how many
+	// it stands for.
+	renewLog struct {
+		last       time.Time
+		suppressed int
+	}
+	renewals map[string]*renewal // userID|sessionID -> its live renewal
+	wg       sync.WaitGroup
+}
+
+const (
+	// sessionRenewInterval matches watchEventsKeepaliveInterval (30s), the
+	// cadence at which the stream handler already proves the connection is
+	// alive. Renewing more often would buy nothing: a connection that has
+	// died between keepalives is not detectable here either.
+	sessionRenewInterval = 30 * time.Second
+
+	// sessionKeyTTL is THREE renewal intervals, not one. A TTL close to the
+	// interval would evict live sessions on any hiccup — a GC pause, a
+	// briefly slow Redis, a rescheduled goroutine — and an evicted session
+	// is invisible to the target picker AND causes handlers_push.go to skip
+	// the publish for a session that is genuinely connected, which is
+	// exactly the failure this type exists to remove. Three intervals means
+	// a process must miss three consecutive renewals before its sessions
+	// vanish, while a crashed instance's entries still clear inside 90s
+	// rather than lingering forever.
+	//
+	// THE COST OF THAT CHOICE, stated because it is a real regression
+	// against MemorySessionPresence and codex round 1 was right to raise
+	// it: for up to this long after an instance DIES, its sessions are
+	// still listed, so a picker can offer one that no longer exists and a
+	// push targeted at it publishes and reaches nobody. The in-memory
+	// registry had no such window — its entries died with its process.
+	// The trade is deliberate and it is not close: do not fix a staleness
+	// window by creating an eviction failure. A shorter TTL trades a
+	// bounded window in which a DEAD session looks alive for an unbounded
+	// one in which a LIVE session looks dead, and the second is worse on
+	// both surfaces — the picker hides a working target, and the push gate
+	// skips a genuinely connected one. Documented in LiveSession's
+	// staleness note and in the web dialog's header rather than tuned.
+	sessionKeyTTL = 3 * sessionRenewInterval
+
+	// presenceOpTimeout bounds every Redis call this type makes, including
+	// ADD's registration write, which used to run on context.Background()
+	// (codex round 2, P1).
+	//
+	// WHAT IT DOES NOT BOUND, measured rather than assumed: go-redis does
+	// not apply a command context to CONNECTION ESTABLISHMENT. Against a
+	// listener that accepts and then never answers, a first command with a
+	// 150ms context took 5.0s to return `i/o timeout` — the client's own
+	// DialTimeout, not the context. So this deadline governs commands on an
+	// already-established connection; the first call after a stall is
+	// bounded by the client's dial/read timeouts instead. Both are finite,
+	// which is what shutdown needs, but do not read this constant as the
+	// worst case.
+	presenceOpTimeout = 5 * time.Second
+
+	// closeDrainTimeout bounds how long Close waits for renewal goroutines.
+	//
+	// Close waits on a WaitGroup whose counter includes a goroutine that has
+	// not started yet — Add increments it before the registration write,
+	// deliberately, so a Close racing an Add cannot return before that
+	// session is accounted for. The cost is that a stalled Redis puts Add's
+	// write between Close and its own completion.
+	//
+	// WITH THE PRODUCTION CLIENT CONFIG this deadline never fires:
+	// go-redis's own dial/read timeouts release the write first, and a
+	// mutation test that stretched the deadline to 24 hours stayed green
+	// for exactly that reason. It is still real behaviour rather than
+	// decoration — a client reconfigured with a zero ReadTimeout has
+	// nothing else to release it, and the goroutines own nothing that
+	// outlives the process, so returning beats waiting forever.
+	//
+	// It is now EXERCISED rather than merely reasoned about:
+	// TestRedisSessionPresence_CloseDoesNotWaitForeverOnAParkedRenewal
+	// parks a renewal inside its write through the onRenewWrite seam and
+	// asserts Close still returns; removing the deadline hangs that test.
+	// The first version of this comment claimed no reachable failing case
+	// existed, which was true only of the failing case I had bothered to
+	// construct.
+	closeDrainTimeout = 10 * time.Second
+)
+
+// renewal is one session's renewal goroutine, held so Remove can both
+// STOP it and WAIT for it.
+//
+// The wait is the load-bearing half (codex round 1, P2). Cancelling
+// returns immediately, so a renewal already inside its write could
+// complete AFTER Remove's DEL/SREM and re-create the entry — resurrecting
+// a session that has just disconnected and leaving it in every instance's
+// picker until the TTL lapses. A targeted push at that ghost publishes and
+// reaches nobody while reporting delivery. Waiting is bounded: the
+// goroutine's context is already cancelled when the wait begins, so any
+// in-flight Redis command returns promptly rather than running to its own
+// timeout.
+type renewal struct {
+	stop context.CancelFunc
+	done chan struct{}
+}
+
+// pruneIndexScript removes index members whose session key is GONE,
+// re-checking existence atomically inside Redis.
+//
+// ARGV[1] is the session-key prefix for this user, so the script can
+// rebuild each candidate's key name; ARGV[2..] are the candidate session
+// ids. Written this way rather than passing the full key names as KEYS
+// because the member id is what SREM needs and the key name is derivable
+// from it — passing both would let the two drift.
+var pruneIndexScript = redis.NewScript(`
+local removed = 0
+for i = 2, #ARGV do
+  if redis.call('EXISTS', ARGV[1] .. ARGV[i]) == 0 then
+    removed = removed + redis.call('SREM', KEYS[1], ARGV[i])
+  end
+end
+return removed
+`)
+
+// timeout is opTimeout with the production default applied, so a
+// zero-valued struct (a test that only set the fields it cared about)
+// still bounds its calls instead of blocking forever.
+func (p *RedisSessionPresence) timeout() time.Duration {
+	if p.opTimeout <= 0 {
+		return presenceOpTimeout
+	}
+	return p.opTimeout
+}
+
+// renewLogInterval bounds how often a renewal failure is logged. Long
+// enough that a sustained outage produces a readable trickle, short enough
+// that a recovering one is visible within a minute.
+const renewLogInterval = time.Minute
+
+// warnRenewFailure logs a renewal failure at most once per
+// renewLogInterval, carrying the number of failures it stands for.
+//
+// Deliberately not silent in between: the count is what tells an operator
+// whether this is one flapping session or the whole replica, which is
+// exactly the Redis-problem-vs-Pad-problem question the raw stream could
+// not answer. The session id is included because the un-throttled version
+// omitted it — a warning naming only the user could not be tied to a
+// specific stuck target.
+func (p *RedisSessionPresence) warnRenewFailure(err error, userID, sessionID string) {
+	now := time.Now()
+
+	p.mu.Lock()
+	p.renewLog.suppressed++
+	if !p.renewLog.last.IsZero() && now.Sub(p.renewLog.last) < renewLogInterval {
+		p.mu.Unlock()
+		return
+	}
+	count := p.renewLog.suppressed
+	p.renewLog.suppressed = 0
+	p.renewLog.last = now
+	p.mu.Unlock()
+
+	slog.Warn("session presence: failed to renew session entry; affected sessions are unlisted and untargetable until it recovers",
+		"error", err, "user_id", userID, "session_id", sessionID,
+		"failures_since_last_log", count, "log_interval", renewLogInterval)
+}
+
+// drain is drainTimeout with the production default applied, for the same
+// zero-value reason as timeout.
+func (p *RedisSessionPresence) drain() time.Duration {
+	if p.drainTimeout <= 0 {
+		return closeDrainTimeout
+	}
+	return p.drainTimeout
+}
+
+// userIDKeyPrefix is sessionKey's prefix for one user — everything before
+// the session id. Kept next to sessionKey so the two cannot drift.
+func userIDKeyPrefix(userID string) string {
+	return "pad:session:" + userID + ":"
+}
+
+// sessionKey is the per-session entry: one key, one TTL, one owner.
+func sessionKey(userID, sessionID string) string {
+	return userIDKeyPrefix(userID) + sessionID
+}
+
+// DEPLOYMENT SCOPING, inherited from internal/watchevents/redis_bus.go's
+// note of the same name and restated here because this is a THIRD
+// keyspace living under the same rule (codex round 3).
+//
+// These names are fixed, like `pad:events:` / `pad:event_seq` /
+// `pad:watchevents` before them, so the operational rule is unchanged and
+// unconditional: ONE REDIS ENDPOINT PER PAD INSTALLATION. Selecting
+// different logical DBs does not rescue it — pub/sub is not namespaced by
+// DB at all, so the buses cross-feed regardless, and two installations
+// sharing one DB would merge these session registries too.
+//
+// What that would cost HERE, stated precisely rather than alarmingly:
+// both keys are scoped by user id, and user ids are UUIDs minted per
+// installation, so a merged registry only exposes one installation's
+// sessions to another where the same UUID exists in both — which in
+// practice means a cloned database, not two independent deployments. The
+// same condition gates the bus's cross-feed, since delivery is filtered on
+// TargetUserID. It is a real hazard for a cloned install and not one for a
+// coincidental collision.
+//
+// Deliberately NOT fixed by growing a prefix here: redis_bus.go's own note
+// rules that if the flat names ever need scoping it should happen for
+// every keyspace at once, from shared config, rather than one file growing
+// a prefix the others lack. Adding one here would make the operational
+// rule harder to state, not easier. Tracked as BUG-2724.
+//
+// sessionIndexKey is the per-user SET of that user's session ids. Scoped
+// per user because every read is user-scoped — there is no "list all
+// sessions on this server" consumer and there should not be one (see
+// handleListSessions' doc comment).
+func sessionIndexKey(userID string) string {
+	return "pad:sessions:" + userID
+}
+
+// NewRedisSessionPresence returns a shared registry backed by client.
+//
+// The client is the SAME one the watch bus uses (cmd/pad/cmd_server.go
+// dials once on PAD_REDIS_URL): they address the same server, and one
+// connection pool serving two logical concerns is the shape internal/events
+// already assumes.
+func NewRedisSessionPresence(client *redis.Client) *RedisSessionPresence {
+	return &RedisSessionPresence{
+		client:        client,
+		sessionKeyTTL: sessionKeyTTL,
+		renewInterval: sessionRenewInterval,
+		opTimeout:     presenceOpTimeout,
+		drainTimeout:  closeDrainTimeout,
+		renewals:      make(map[string]*renewal),
+	}
+}
+
+// Add implements SessionPresence.
+//
+// Returns the generated id even when the Redis write fails. That is
+// deliberate and it is the same posture the rest of the push path takes
+// toward presence: the caller is a live SSE handler that is about to serve
+// a real connection, and it must not be torn down because an optional
+// registry is unavailable. A session missing from the registry is
+// under-reporting — the failure mode presence already documents — while
+// refusing the connection would take away delivery the bus can still
+// perform. The id is still the one watchNotificationVisible compares a
+// targeted push against, so a broadcast push reaches this connection
+// regardless.
+func (p *RedisSessionPresence) Add(userID string, ident SessionIdentity) string {
+	id := uuid.NewString()
+	sess := LiveSession{
+		ID:          id,
+		Label:       ident.Label,
+		PID:         ident.PID,
+		Armed:       ident.Armed,
+		ConnectedAt: time.Now().UTC(),
+	}
+	payload, err := json.Marshal(sess)
+	if err != nil {
+		// Cannot happen for this struct; logged rather than swallowed so a
+		// future field that breaks marshalling is visible.
+		slog.Error("session presence: marshal session", "error", err, "user_id", userID)
+		return id
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	rn := &renewal{stop: cancel, done: make(chan struct{})}
+
+	p.mu.Lock()
+	if p.closed {
+		p.mu.Unlock()
+		cancel()
+		return id
+	}
+	p.renewals[renewalKey(userID, id)] = rn
+	p.wg.Add(1)
+	p.mu.Unlock()
+
+	writeCtx, writeCancel := context.WithTimeout(context.Background(), p.timeout())
+	defer writeCancel()
+	if err := p.write(writeCtx, userID, id, string(payload)); err != nil {
+		// Logged, not fatal: renewLoop re-SETs the whole payload rather
+		// than issuing a bare EXPIRE, so the next tick recovers a
+		// registration that failed on a blip.
+		slog.Warn("session presence: failed to register session; it will be missing from the picker until the next renewal",
+			"error", err, "user_id", userID)
+	}
+
+	go p.renewLoop(ctx, rn, userID, id, string(payload))
+	return id
+}
+
+// writeScript stores the session entry and indexes it, ATOMICALLY.
+//
+// Scripted rather than pipelined (codex round 14). A pipeline can apply
+// partially — a dropped connection between commands leaves some sent and
+// some not — and the two halves are not equally harmless, which an earlier
+// version of this comment got wrong. An index member with no session key is
+// pruned by the next reader, fine. But a session KEY with no INDEX MEMBER
+// is a live session that ListForUser cannot see, because the index is the
+// only enumeration: a targeted push at it is skipped and answers a clean
+// delivered_sessions:0. That is precisely the failure BUG-2698 exists to
+// remove, reintroduced by a partial write.
+//
+// It was self-healing — the next renewal re-runs this and re-indexes, so
+// the window was one renewal interval rather than permanent — but a
+// bounded reappearance of the bug is still the bug, and Redis executes a
+// script atomically on its single thread, so there is no reason to accept
+// the window at all.
+
+var writeScript = redis.NewScript(`
+redis.call('SET', KEYS[1], ARGV[1], 'EX', ARGV[2])
+redis.call('SADD', KEYS[2], ARGV[3])
+redis.call('EXPIRE', KEYS[2], ARGV[2])
+return 1
+`)
+
+// write stores the session entry and indexes it, both under the same TTL,
+// in one atomic step. See writeScript.
+func (p *RedisSessionPresence) write(ctx context.Context, userID, sessionID, payload string) error {
+	return writeScript.Run(ctx, p.client,
+		[]string{sessionKey(userID, sessionID), sessionIndexKey(userID)},
+		payload, int(p.sessionKeyTTL.Seconds()), sessionID,
+	).Err()
+}
+
+// renewLoop keeps this connection's entry alive for exactly as long as the
+// connection is held open by THIS process.
+//
+// It re-SETs the payload rather than issuing a bare EXPIRE, so a renewal
+// also RESTORES an entry that expired during a Redis outage or a long
+// pause. A bare EXPIRE against a vanished key is a no-op that returns
+// success, which would leave a live session permanently invisible with
+// nothing in the logs.
+func (p *RedisSessionPresence) renewLoop(ctx context.Context, rn *renewal, userID, sessionID, payload string) {
+	defer p.wg.Done()
+	// Closed AFTER the last write returns, which is what makes Remove's
+	// wait meaningful — see the renewal type's doc comment.
+	defer close(rn.done)
+	ticker := time.NewTicker(p.renewInterval)
+	defer ticker.Stop()
+	for {
+		select {
+		case <-ctx.Done():
+			return
+		case <-ticker.C:
+			if p.onRenewWrite != nil {
+				p.onRenewWrite()
+			}
+			if err := p.write(ctx, userID, sessionID, payload); err != nil && ctx.Err() == nil {
+				p.warnRenewFailure(err, userID, sessionID)
+			}
+		}
+	}
+}
+
+// Remove implements SessionPresence: idempotent, and safe for a session id
+// that was never added (the stream handler's defer runs on paths where Add
+// may not have been reached).
+func (p *RedisSessionPresence) Remove(userID string, sessionID string) {
+	if sessionID == "" {
+		return
+	}
+	p.mu.Lock()
+	rn, ok := p.renewals[renewalKey(userID, sessionID)]
+	if ok {
+		delete(p.renewals, renewalKey(userID, sessionID))
+	}
+	closed := p.closed
+	p.mu.Unlock()
+	// After Close, the entry may already have been handed to Close's own
+	// drain rather than found here. Waiting on the WaitGroup is what covers
+	// that case: it does not return until every renewal goroutine has
+	// finished its last write, which is the property the DEL below depends
+	// on (codex round 6). Close bounds its own drain, so this cannot
+	// outlive shutdown.
+	if closed && !ok {
+		// BOUNDED, like Close's own drain (codex round 10). An unbounded
+		// wait here reintroduces exactly what the drain deadline exists to
+		// prevent: if Close timed out on a parked renewal, that renewal is
+		// still running, so waiting on it without a deadline would hold
+		// this handler — and therefore http.Server.Shutdown — for as long
+		// as the parked write lasts.
+		//
+		// Timing out means proceeding to the DEL below with a renewal
+		// potentially still in flight, i.e. accepting the ghost this wait
+		// exists to prevent. That is the right trade at this point: the
+		// ghost expires on its TTL, while a hung shutdown does not resolve
+		// at all.
+		p.waitForDrain(p.drain())
+	}
+	if ok {
+		// Stop AND wait, in that order and outside the lock. Stopping alone
+		// leaves an in-flight renewal free to re-create the entry after the
+		// delete below (codex round 1, P2).
+		//
+		// BOUNDED, and this is the branch that matters (codex round 11).
+		// Round 10 bounded only the post-Close fallback below, which was
+		// the wrong half: Close RETAINS its renewal entries, so a handler
+		// unwinding during shutdown finds one here and takes this path —
+		// and an unbounded `<-rn.done` against a parked renewal held
+		// http.Server.Shutdown exactly as before. Same deadline, same
+		// trade: a ghost that expires on its TTL beats a shutdown that
+		// never returns.
+		rn.stop()
+		select {
+		case <-rn.done:
+		case <-time.After(p.drain()):
+			slog.Warn("session presence: renewal did not stop before deregistration; the entry may briefly reappear until its TTL",
+				"user_id", userID, "timeout", p.drain())
+		}
+	}
+
+	// A fresh context, NOT the cancelled renewal one: this is the clean
+	// deregistration a disconnect owes the registry, and it has to run
+	// after the renewal has stopped. Reusing the cancelled context would
+	// fail every command and leave the entry to expire on its TTL instead —
+	// turning an immediate disconnect into a 90-second ghost in every other
+	// instance's session picker.
+	ctx, cancel := context.WithTimeout(context.Background(), p.timeout())
+	defer cancel()
+	pipe := p.client.Pipeline()
+	pipe.Del(ctx, sessionKey(userID, sessionID))
+	pipe.SRem(ctx, sessionIndexKey(userID), sessionID)
+	if _, err := pipe.Exec(ctx); err != nil {
+		slog.Warn("session presence: failed to deregister session; it will expire on its TTL",
+			"error", err, "user_id", userID)
+	}
+}
+
+// ListForUser implements SessionPresence, oldest connection first with the
+// session id as a tiebreaker — the same deterministic order
+// MemorySessionPresence produces, because an unstable order would make the
+// web target picker jump around under the user's cursor.
+//
+// A READ FAILURE IS RETURNED, never flattened into an empty list. An
+// earlier version of this method did flatten it, on the reasoning that "a
+// partial read is not an outage" — which conflated a nil registry (a
+// configuration fact, known at startup) with a failed read (a runtime
+// one), and no consumer can tell those apart from an empty slice anyway.
+// Codex round 1 caught it: the flattened version made handleListSessions
+// answer 200 with no sessions during a Redis outage, and made a targeted
+// push skip its publish and lose the instruction while reporting success.
+func (p *RedisSessionPresence) ListForUser(userID string) ([]LiveSession, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), p.timeout())
+	defer cancel()
+
+	ids, err := p.client.SMembers(ctx, sessionIndexKey(userID)).Result()
+	if err != nil {
+		// REPORTED, not swallowed into an empty list (codex round 1, P1).
+		// An empty list means "nobody is listening", which makes a targeted
+		// push skip its publish and lose the instruction; an outage means
+		// "I cannot tell", which must make the caller decline to conclude
+		// anything. Collapsing the two here is the same defect this type
+		// was written to remove, one layer down.
+		slog.Warn("session presence: failed to read session index", "error", err, "user_id", userID)
+		return nil, fmt.Errorf("session presence: read index: %w", err)
+	}
+	if len(ids) == 0 {
+		// EMPTY, not nil (codex round 5). MemorySessionPresence returns a
+		// non-nil empty slice, and handleListSessions marshals whatever it
+		// gets straight into `sessions` — so a nil here serialises as
+		// `"sessions": null` where the other implementation produces
+		// `"sessions": []`. A consumer that maps over the array gets a
+		// runtime error against one implementation and not the other, which
+		// is precisely the kind of cross-implementation divergence this
+		// registry exists to remove.
+		return []LiveSession{}, nil
+	}
+
+	keys := make([]string, 0, len(ids))
+	for _, id := range ids {
+		keys = append(keys, sessionKey(userID, id))
+	}
+	values, err := p.client.MGet(ctx, keys...).Result()
+	if err != nil {
+		slog.Warn("session presence: failed to read session entries", "error", err, "user_id", userID)
+		return nil, fmt.Errorf("session presence: read entries: %w", err)
+	}
+
+	out := make([]LiveSession, 0, len(values))
+	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.
+			expired = append(expired, ids[i])
+			continue
+		}
+		raw, ok := v.(string)
+		if !ok {
+			// Same ruling as the decode failure below: a value we cannot
+			// interpret is an UNKNOWN registry state, not an absent session.
+			return nil, fmt.Errorf("session presence: session entry for %s is not a string", ids[i])
+		}
+		var sess LiveSession
+		if err := json.Unmarshal([]byte(raw), &sess); err != nil {
+			// REPORTED, not skipped (codex round 2, P2). An earlier version
+			// dropped the row and returned a nil error, arguing that the
+			// other sessions in the list were still real. That contradicted
+			// the P1 ruling one round earlier, and in the same direction: a
+			// silently-omitted session makes deliveredSessionCount report a
+			// number that looks complete, so a targeted push at the omitted
+			// session returns delivered_sessions:0 and is SKIPPED — the
+			// instruction dropped while the caller is told nothing was
+			// listening.
+			//
+			// The trade is deliberate: one corrupt entry now blinds this
+			// user's whole listing (503 on the picker, null count on a
+			// broadcast) until the row expires, at most one TTL. That is the
+			// honest answer — we cannot say who is connected — and it
+			// self-heals, where the silent version stayed wrong and looked
+			// right.
+			slog.Warn("session presence: undecodable session entry",
+				"error", err, "user_id", userID, "session_id", ids[i])
+			return nil, fmt.Errorf("session presence: decode entry %s: %w", ids[i], err)
+		}
+		out = append(out, sess)
+	}
+	if len(expired) > 0 {
+		// CONDITIONAL, not a plain SREM (codex round 2, P2). Between the
+		// MGet above and this call, a renewal can restore the very key we
+		// observed missing — which happens exactly when it is most harmful:
+		// a Redis outage long enough to expire entries, followed by
+		// recovery, has every surviving instance rewriting its sessions at
+		// once. An unconditional SREM would then evict a LIVE session from
+		// the index, hiding it from the picker and making targeted pushes to
+		// it skip, for up to a renewal interval. The script re-checks
+		// existence inside Redis, where the check and the removal cannot be
+		// interleaved.
+		//
+		// Still best-effort: a failure costs a retry on the next read, and
+		// the members are already invisible to this listing either way.
+		if err := pruneIndexScript.Run(ctx, p.client,
+			[]string{sessionIndexKey(userID)},
+			append([]interface{}{userIDKeyPrefix(userID)}, toAny(expired)...)...,
+		).Err(); err != nil && !errors.Is(err, redis.Nil) {
+			slog.Debug("session presence: failed to prune expired index members",
+				"error", err, "user_id", userID)
+		}
+	}
+
+	sort.Slice(out, func(i, j int) bool {
+		if out[i].ConnectedAt.Equal(out[j].ConnectedAt) {
+			return out[i].ID < out[j].ID
+		}
+		return out[i].ConnectedAt.Before(out[j].ConnectedAt)
+	})
+	// Everything that could not be interpreted has already RETURNED an
+	// error above — an undecodable entry included (codex round 2 changed
+	// that, and an earlier version of this comment still described the
+	// behaviour it replaced). Reaching here means every indexed session
+	// either decoded or was pruned as expired, so this list is complete
+	// as of the read.
+	return out, nil
+}
+
+// Close stops every renewal goroutine and waits for them. It is NOT part of
+// the SessionPresence interface — MemorySessionPresence has nothing to shut
+// down — so callers hold the concrete type to call it. cmd/pad/cmd_server.go
+// does exactly that during shutdown.
+//
+// Note what it deliberately does NOT do: it does not delete this instance's
+// entries. A shutdown that raced a reconnect elsewhere would then delete a
+// session that had already re-registered. Letting the TTL clear them is
+// slower and correct.
+func (p *RedisSessionPresence) Close() {
+	p.mu.Lock()
+	if p.closed {
+		p.mu.Unlock()
+		return
+	}
+	p.closed = true
+	// STOPPED, NOT DELETED (codex round 6). Deleting here removed the very
+	// lookup Remove uses to decide whether to WAIT: a handler disconnecting
+	// concurrently with Close found no renewal, skipped the wait, and ran
+	// its DEL — leaving an already-cancelled-but-still-in-flight renewal
+	// free to complete afterwards and re-create the key. That is round 1's
+	// resurrection bug arriving by the other door, and the fix there (wait
+	// for the goroutine) only works if Remove can still find it.
+	//
+	// The entries are dropped with the object instead. Close is the
+	// shutdown path, so nothing is accumulating.
+	for _, rn := range p.renewals {
+		rn.stop()
+	}
+	p.mu.Unlock()
+
+	if !p.waitForDrain(p.drain()) {
+		// Bounded, not abandoned: every goroutine has already been
+		// cancelled above, so this only fires when one is parked inside a
+		// Redis call that its own client timeouts have not yet released.
+		// Holding shutdown behind that is worse than leaving them to the
+		// process exit — they own no state that outlives it.
+		slog.Warn("session presence: renewal goroutines did not drain before shutdown; leaving them to process exit",
+			"timeout", p.drain())
+	}
+}
+
+// waitForDrain waits for every renewal goroutine to finish, up to timeout.
+// Reports whether they all finished; false means at least one is still
+// running and the caller is proceeding anyway.
+func (p *RedisSessionPresence) waitForDrain(timeout time.Duration) bool {
+	drained := make(chan struct{})
+	go func() {
+		p.wg.Wait()
+		close(drained)
+	}()
+	select {
+	case <-drained:
+		return true
+	case <-time.After(timeout):
+		return false
+	}
+}
+
+func renewalKey(userID, sessionID string) string { return userID + "|" + sessionID }
+
+func toAny(ss []string) []interface{} {
+	out := make([]interface{}, 0, len(ss))
+	for _, s := range ss {
+		out = append(out, s)
+	}
+	return out
+}

internal/server/session_presence_redis_round2_test.go

@@ -0,0 +1,748 @@
+package server
+
+import (
+	"bytes"
+	"encoding/json"
+	"errors"
+	"log/slog"
+	"net"
+	"strconv"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/alicebob/miniredis/v2"
+	"github.com/redis/go-redis/v9"
+)
+
+// BUG-2698, codex round 2. Three findings, three instruments.
+
+// stalledRedis returns the address of a listener that ACCEPTS connections
+// and then never answers — the shape a hung Redis presents, which a closed
+// port does not: a closed port fails fast, and failing fast was never the
+// problem.
+func stalledRedis(t *testing.T) (addr string, accepted <-chan struct{}) {
+	t.Helper()
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	// Closed on the FIRST accepted connection. Callers synchronise on this
+	// rather than sleeping (codex round 13): a sleep does not establish
+	// that the call under test is in flight, so on a loaded runner the
+	// sequencing can invert and even the BROKEN implementation passes. An
+	// instrument whose discrimination depends on the scheduler is not one.
+	firstAccept := make(chan struct{})
+	var once sync.Once
+
+	var mu sync.Mutex
+	var conns []net.Conn
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		for {
+			conn, err := ln.Accept()
+			if err != nil {
+				return
+			}
+			once.Do(func() { close(firstAccept) })
+			// Held open, never read, never written.
+			mu.Lock()
+			conns = append(conns, conn)
+			mu.Unlock()
+		}
+	}()
+	t.Cleanup(func() {
+		_ = ln.Close()
+		<-done
+		mu.Lock()
+		for _, c := range conns {
+			_ = c.Close()
+		}
+		mu.Unlock()
+	})
+	return ln.Addr().String(), firstAccept
+}
+
+// TestRedisSessionPresence_StalledRedisDoesNotHangShutdown — codex round
+// 2, P1, with the claim narrowed to what is actually true.
+//
+// The first version of this test asserted that Add returns within the
+// op timeout when Redis stalls. IT FAILED, and it was right to: go-redis
+// does not apply a command context to connection establishment, so a
+// 150ms context still took 5.0s (the client's DialTimeout). The bound I
+// had just written into a code comment did not exist. Measured, not
+// reasoned about — which is the only reason the comment now says
+// something true.
+//
+// What IS assertable, and what the finding was actually about: Close must
+// not hang. Its WaitGroup counter includes a goroutine that has not
+// started yet (incremented in Add before the registration write, so a
+// Close racing an Add cannot return early), so a stalled Redis parks
+// Add's write between Close and its own completion. Close therefore
+// drains with a deadline.
+//
+// The assertion is on TIME because that is what the wrong behaviour
+// violates: an unbounded Close does not return a wrong value, it simply
+// never returns.
+//
+// HONEST SCOPE, because the mutation matrix says so and a test that
+// oversells itself is worse than none: replacing closeDrainTimeout with 24
+// hours leaves this test GREEN. go-redis's own dial/read timeouts already
+// bound Add's write, so Close is finite here with or without the deadline.
+// What this pins is the end-to-end property — a stalled Redis blocks
+// neither the connecting handler nor shutdown — not the deadline itself,
+// which is a backstop for a client someone reconfigures without timeouts.
+func TestRedisSessionPresence_StalledRedisDoesNotHangShutdown(t *testing.T) {
+	t.Parallel()
+	addr, accepted := stalledRedis(t)
+	client := redis.NewClient(&redis.Options{Addr: addr, MaxRetries: -1})
+	p := &RedisSessionPresence{
+		client:        client,
+		sessionKeyTTL: time.Minute,
+		renewInterval: time.Hour,
+		opTimeout:     150 * time.Millisecond,
+		renewals:      make(map[string]*renewal),
+	}
+
+	added := make(chan string, 1)
+	go func() { added <- p.Add("user-1", SessionIdentity{Armed: true}) }()
+
+	// Close races the IN-FLIGHT Add rather than waiting for it, because
+	// that is the scenario the finding described: the WaitGroup counter is
+	// already incremented while the write is parked, so Close is queued
+	// behind a Redis call that is going nowhere.
+	//
+	// Synchronised on the listener ACCEPTING the connection, not on a
+	// sleep: this is the moment Add is provably inside a Redis call that
+	// will never answer.
+	<-accepted
+
+	closed := make(chan struct{})
+	start := time.Now()
+	go func() { p.Close(); close(closed) }()
+	select {
+	case <-closed:
+		if elapsed := time.Since(start); elapsed > closeDrainTimeout+5*time.Second {
+			t.Fatalf("Close took %v, past its own drain bound", elapsed)
+		}
+	case <-time.After(closeDrainTimeout + 10*time.Second):
+		t.Fatal("Close did not return while Redis was stalled — graceful shutdown would hang behind a renewal goroutine")
+	}
+
+	// And Add itself must have returned, with an id: a registry write that
+	// failed is under-reporting, not a reason to refuse the connection.
+	select {
+	case id := <-added:
+		if id == "" {
+			t.Fatal("Add must return a session id even when the registry write fails")
+		}
+	case <-time.After(30 * time.Second):
+		t.Fatal("Add never returned against a stalled Redis — the registration write has no bound at all, not even the client's")
+	}
+}
+
+// TestPruneIndexScript_LeavesLiveMembers — codex round 2, P2. The prune
+// re-checks existence INSIDE Redis, because between ListForUser's MGet and
+// its prune a renewal can restore the key it observed missing. That
+// happens exactly when it hurts most: an outage long enough to expire
+// entries, then recovery, has every instance rewriting at once. An
+// unconditional SREM would evict a LIVE session from the index, hiding it
+// from the picker and making targeted pushes skip.
+//
+// Drives the script directly, which is the only way to observe the
+// re-check: through ListForUser the two orderings are indistinguishable
+// from outside.
+func TestPruneIndexScript_LeavesLiveMembers(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+	ctx := t.Context()
+
+	const user = "user-1"
+	live, dead := "live-session", "dead-session"
+	if err := client.SAdd(ctx, sessionIndexKey(user), live, dead).Err(); err != nil {
+		t.Fatalf("seed index: %v", err)
+	}
+	// Only the live one has a session key — the dead one's expired, which
+	// is exactly the state ListForUser observes before pruning.
+	if err := client.Set(ctx, sessionKey(user, live), "{}", time.Minute).Err(); err != nil {
+		t.Fatalf("seed key: %v", err)
+	}
+
+	if err := pruneIndexScript.Run(ctx, client,
+		[]string{sessionIndexKey(user)},
+		userIDKeyPrefix(user), live, dead,
+	).Err(); err != nil && !errors.Is(err, redis.Nil) {
+		t.Fatalf("prune: %v", err)
+	}
+
+	members, err := client.SMembers(ctx, sessionIndexKey(user)).Result()
+	if err != nil {
+		t.Fatalf("read index: %v", err)
+	}
+	// The wrong behaviour's fingerprint: the live member gone because the
+	// prune trusted a stale observation instead of re-checking.
+	if len(members) != 1 || members[0] != live {
+		t.Fatalf("prune must remove only members whose key is gone; index = %v", members)
+	}
+}
+
+// TestRedisSessionPresence_UndecodableEntryIsAnError — codex round 2, P2.
+// A row we cannot interpret is an UNKNOWN registry state, not an absent
+// session. Skipping it silently made deliveredSessionCount report a number
+// that looked complete, so a targeted push at the omitted session returned
+// delivered_sessions:0 and was SKIPPED — the instruction dropped while the
+// caller was told nothing was listening.
+func TestRedisSessionPresence_UndecodableEntryIsAnError(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+	p := NewRedisSessionPresence(client)
+	t.Cleanup(p.Close)
+
+	id := p.Add("user-1", SessionIdentity{Armed: true})
+
+	// CONTROL FIRST: a well-formed registry lists cleanly. Without this the
+	// assertion below would also pass for an implementation that errored on
+	// everything.
+	sessions, err := p.ListForUser("user-1")
+	if err != nil || len(sessions) != 1 {
+		t.Fatalf("precondition: expected 1 session and no error, got %d / %v", len(sessions), err)
+	}
+
+	if err := client.Set(t.Context(), sessionKey("user-1", id), "not json", time.Minute).Err(); err != nil {
+		t.Fatalf("corrupt entry: %v", err)
+	}
+
+	sessions, err = p.ListForUser("user-1")
+	if err == nil {
+		t.Fatalf("an undecodable entry must be reported, not skipped; got %d sessions and no error", len(sessions))
+	}
+	if sessions != nil {
+		t.Fatalf("a failed read must not return a partial list that looks complete; got %d sessions", len(sessions))
+	}
+}
+
+// TestRedisSessionPresence_CloseDoesNotWaitForeverOnAParkedRenewal —
+// codex round 4, P2, and the instrument the round-2 version of this file
+// admitted it did not have.
+//
+// closeDrainTimeout was untestable while it was a constant: under the
+// production client config go-redis's own timeouts always release the
+// write first, so removing the deadline changed nothing observable. That
+// admission was honest but it left a behaviour with no coverage. Parking a
+// renewal INSIDE its write through the onRenewWrite seam produces the one
+// state the deadline exists for — a goroutine that nothing else will
+// release — and makes the difference between bounded and unbounded
+// observable.
+//
+// Fails by HANGING when the deadline is removed, which is what an
+// unbounded Close does; the timeout below is what turns that into a test
+// failure rather than a stuck suite.
+func TestRedisSessionPresence_CloseDoesNotWaitForeverOnAParkedRenewal(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+
+	entered := make(chan struct{})
+	release := make(chan struct{})
+	var once sync.Once
+	p := &RedisSessionPresence{
+		client:        client,
+		sessionKeyTTL: time.Minute,
+		renewInterval: 5 * time.Millisecond,
+		drainTimeout:  200 * time.Millisecond,
+		renewals:      make(map[string]*renewal),
+		onRenewWrite: func() {
+			once.Do(func() {
+				close(entered)
+				<-release
+			})
+		},
+	}
+	t.Cleanup(func() {
+		// JOIN, don't just release (codex round 13). This test
+		// deliberately lets a drain time out, so it finishes with a renewal
+		// goroutine still parked; releasing it without waiting leaves it
+		// unwinding into whatever runs next, contaminating leak checks and
+		// later tests in the package.
+		close(release)
+		p.Close()
+		p.waitForDrain(5 * time.Second)
+	})
+
+	p.Add("user-1", SessionIdentity{Armed: true})
+	<-entered // a renewal is parked and nothing will release it
+
+	closed := make(chan struct{})
+	go func() { p.Close(); close(closed) }()
+
+	select {
+	case <-closed:
+	case <-time.After(10 * time.Second):
+		t.Fatal("Close never returned with a renewal parked inside its write — the drain has no deadline, so shutdown waits forever")
+	}
+}
+
+// TestRedisSessionPresence_EmptyListIsNotNil — codex round 5, P2.
+//
+// handleListSessions marshals whatever ListForUser returns straight into
+// the `sessions` field, so a nil slice serialises as `"sessions": null`
+// where MemorySessionPresence produces `"sessions": []`. A consumer that
+// maps over the array breaks against one implementation and not the other.
+//
+// Asserts the WIRE, not just the Go value: a non-nil check would pass for
+// a value that still marshalled to null through some future indirection.
+func TestRedisSessionPresence_EmptyListIsNotNil(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	p := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(p.Close)
+
+	sessions, err := p.ListForUser("nobody-here")
+	if err != nil {
+		t.Fatalf("list: %v", err)
+	}
+	if sessions == nil {
+		t.Fatal("an empty listing must be an empty slice, not nil — it serialises as null and breaks array consumers")
+	}
+
+	encoded, err := json.Marshal(sessionsResponse{Sessions: sessions, Count: len(sessions)})
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	if !strings.Contains(string(encoded), `"sessions":[]`) {
+		t.Fatalf("expected an empty array on the wire, got %s", encoded)
+	}
+
+	// CONTROL: the in-memory implementation, whose shape this is matching.
+	memSessions, err := NewMemorySessionPresence().ListForUser("nobody-here")
+	if err != nil {
+		t.Fatalf("memory list: %v", err)
+	}
+	if memSessions == nil {
+		t.Fatal("precondition: MemorySessionPresence was expected to return a non-nil empty slice")
+	}
+}
+
+// TestRedisSessionPresence_RemoveAfterCloseStillWaits — codex round 6, P2.
+//
+// Round 1 made Remove wait for its session's renewal so an in-flight write
+// could not re-create the key after the DEL. Close then removed the
+// entries from the map before draining, which deleted the very lookup
+// Remove uses to find that renewal — so a handler disconnecting
+// concurrently with Close found nothing, skipped the wait, and reopened
+// the same resurrection window by the other door.
+//
+// Drives that exact interleaving: a renewal parked inside its write, Close
+// running (and hitting its drain bound), then a Remove arriving after.
+// Remove must not return while that renewal can still write.
+func TestRedisSessionPresence_RemoveAfterCloseStillWaits(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+
+	entered := make(chan struct{})
+	release := make(chan struct{})
+	var once sync.Once
+	p := &RedisSessionPresence{
+		client:        client,
+		sessionKeyTTL: time.Minute,
+		renewInterval: 5 * time.Millisecond,
+		// Long enough that the wait below is observable. The BOUND on that
+		// wait is asserted separately, by the test after this one — the two
+		// properties pull against each other (wait, but not forever) and
+		// conflating them into one deadline would let either failure hide
+		// behind the other.
+		drainTimeout: 5 * time.Second,
+		renewals:     make(map[string]*renewal),
+		onRenewWrite: func() {
+			once.Do(func() {
+				close(entered)
+				<-release
+			})
+		},
+	}
+
+	id := p.Add("user-1", SessionIdentity{Armed: true})
+	<-entered
+
+	closeDone := make(chan struct{})
+	go func() { p.Close(); close(closeDone) }()
+
+	removed := make(chan struct{})
+	go func() {
+		p.Remove("user-1", id)
+		close(removed)
+	}()
+
+	// THE ASSERTION: Remove must still be waiting. The pre-fix version
+	// found an empty map and went straight to its DEL, which the parked
+	// write would then undo.
+	select {
+	case <-removed:
+		t.Fatal("Remove returned after Close while a renewal was still in flight; that renewal can re-create the entry after the delete")
+	case <-time.After(300 * time.Millisecond):
+	}
+
+	close(release)
+	<-closeDone
+
+	select {
+	case <-removed:
+	case <-time.After(5 * time.Second):
+		t.Fatal("Remove never returned after the renewal was released")
+	}
+
+	reader := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(reader.Close)
+	sessions, err := reader.ListForUser("user-1")
+	if err != nil {
+		t.Fatalf("list: %v", err)
+	}
+	if len(sessions) != 0 {
+		t.Fatalf("a ghost session survived Remove-after-Close (%d listed)", len(sessions))
+	}
+}
+
+// TestRedisSessionPresence_RemoveIsBoundedWhenARenewalWillNotStop —
+// codex round 11, and the counterweight to the test above.
+//
+// Those two properties pull against each other: Remove must WAIT for an
+// in-flight renewal (or a ghost survives the delete), and must NOT wait
+// forever (or a parked renewal holds http.Server.Shutdown). Round 10
+// bounded the wrong branch — the post-Close fallback rather than the
+// `<-rn.done` path a shutdown actually takes, because Close RETAINS its
+// entries so the handler finds one. The hang survived its own fix.
+//
+// Asserting the bound needs its own test rather than a shorter deadline on
+// the one above, which would make each failure indistinguishable from the
+// other.
+func TestRedisSessionPresence_RemoveIsBoundedWhenARenewalWillNotStop(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+
+	entered := make(chan struct{})
+	release := make(chan struct{})
+	var once sync.Once
+	p := &RedisSessionPresence{
+		client:        client,
+		sessionKeyTTL: time.Minute,
+		renewInterval: 5 * time.Millisecond,
+		drainTimeout:  200 * time.Millisecond,
+		renewals:      make(map[string]*renewal),
+		onRenewWrite: func() {
+			once.Do(func() {
+				close(entered)
+				<-release
+			})
+		},
+	}
+	t.Cleanup(func() {
+		// JOIN, don't just release (codex round 13). This test
+		// deliberately lets a drain time out, so it finishes with a renewal
+		// goroutine still parked; releasing it without waiting leaves it
+		// unwinding into whatever runs next, contaminating leak checks and
+		// later tests in the package.
+		close(release)
+		p.Close()
+		p.waitForDrain(5 * time.Second)
+	})
+
+	id := p.Add("user-1", SessionIdentity{Armed: true})
+	<-entered // parked, and nothing will release it
+
+	removed := make(chan struct{})
+	go func() {
+		p.Remove("user-1", id)
+		close(removed)
+	}()
+
+	select {
+	case <-removed:
+	case <-time.After(10 * time.Second):
+		t.Fatal("Remove never returned against a renewal that will not stop — a disconnecting handler holds http.Server.Shutdown forever")
+	}
+}
+
+// TestWriteScript_IndexesAtomicallyWithTheEntry — codex round 14.
+//
+// The index is the ONLY enumeration of a user's sessions, so a session key
+// written without its index member is a live session ListForUser cannot
+// see — and a targeted push at it is skipped with a clean
+// delivered_sessions:0, which is the bug BUG-2698 exists to remove,
+// reintroduced by a partial write.
+//
+// Asserts the pair, not just the key: a test that checked only the entry
+// would pass for exactly the broken shape.
+func TestWriteScript_IndexesAtomicallyWithTheEntry(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+	p := NewRedisSessionPresence(client)
+	t.Cleanup(p.Close)
+	ctx := t.Context()
+
+	if err := p.write(ctx, "user-1", "sess-1", `{"id":"sess-1"}`); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+
+	if _, err := client.Get(ctx, sessionKey("user-1", "sess-1")).Result(); err != nil {
+		t.Fatalf("session entry missing: %v", err)
+	}
+	members, err := client.SMembers(ctx, sessionIndexKey("user-1")).Result()
+	if err != nil {
+		t.Fatalf("read index: %v", err)
+	}
+	if len(members) != 1 || members[0] != "sess-1" {
+		t.Fatalf("the index must carry the session the same write created; got %v", members)
+	}
+
+	// BOTH keys carry a TTL. An index that outlives its entries accumulates
+	// dead members for a user who never reconnects; an entry with no TTL
+	// never expires when its instance dies, which is the reaping story this
+	// type owes.
+	entryTTL, err := client.TTL(ctx, sessionKey("user-1", "sess-1")).Result()
+	if err != nil || entryTTL <= 0 {
+		t.Fatalf("session entry must carry a TTL; got %v (err %v)", entryTTL, err)
+	}
+	indexTTL, err := client.TTL(ctx, sessionIndexKey("user-1")).Result()
+	if err != nil || indexTTL <= 0 {
+		t.Fatalf("session index must carry a TTL; got %v (err %v)", indexTTL, err)
+	}
+}
+
+// TestRedisSessionPresence_RenewalRestoresAVanishedEntry — codex round 30
+// (coverage-gap sweep).
+//
+// renewLoop re-SETs the full payload rather than issuing a bare EXPIRE, and
+// that choice is load-bearing: it is what makes an EVICTED or
+// outage-vanished entry come back, which is the entire reason this type
+// tolerates an evicting maxmemory-policy and a Redis restart. Nothing
+// tested it. The crash-expiry test would still pass with a bare EXPIRE
+// (nothing renews there), and the keepalive test would too (its key never
+// vanishes), so the exact regression that matters was invisible.
+//
+// Asserts BOTH halves come back — a renewal that restored the entry but
+// not the index member would leave a live session missing from the only
+// enumeration of it.
+func TestRedisSessionPresence_RenewalRestoresAVanishedEntry(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+	p := &RedisSessionPresence{
+		client:        client,
+		sessionKeyTTL: time.Minute,
+		renewInterval: 10 * time.Millisecond,
+		renewals:      make(map[string]*renewal),
+	}
+	t.Cleanup(p.Close)
+	ctx := t.Context()
+
+	id := p.Add("user-1", SessionIdentity{Label: "docapp", Armed: true})
+
+	// Evict it the way Redis would under memory pressure: entry and index
+	// member both gone, with the session still connected.
+	if err := client.Del(ctx, sessionKey("user-1", id)).Err(); err != nil {
+		t.Fatalf("evict entry: %v", err)
+	}
+	if err := client.SRem(ctx, sessionIndexKey("user-1"), id).Err(); err != nil {
+		t.Fatalf("evict index member: %v", err)
+	}
+	if sessions, err := p.ListForUser("user-1"); err != nil || len(sessions) != 0 {
+		t.Fatalf("precondition: the session should be gone, got %d (err %v)", len(sessions), err)
+	}
+
+	var restored []LiveSession
+	deadline := time.Now().Add(3 * time.Second)
+	for time.Now().Before(deadline) {
+		time.Sleep(20 * time.Millisecond)
+		got, err := p.ListForUser("user-1")
+		if err != nil {
+			t.Fatalf("list: %v", err)
+		}
+		if len(got) == 1 {
+			restored = got
+			break
+		}
+	}
+	if len(restored) != 1 {
+		t.Fatal("a renewal must RESTORE a vanished entry, not merely refresh a TTL — a bare EXPIRE against a missing key is a silent no-op and the session stays invisible")
+	}
+	if restored[0].ID != id {
+		t.Fatalf("restored the wrong session: %q", restored[0].ID)
+	}
+	// The full payload, not a placeholder: a renewal that re-SET an empty
+	// or default value would satisfy a count-only assertion.
+	if restored[0].Label != "docapp" || !restored[0].Armed {
+		t.Fatalf("the restored entry lost its identity: %+v", restored[0])
+	}
+}
+
+// TestRedisSessionPresence_RealRedisReadFailureIsReported — codex round 30.
+//
+// The unreadable-presence tests use a fake SessionPresence, so they prove
+// the HANDLER's behaviour and say nothing about whether the real
+// implementation reports a read failure or flattens it into an empty list.
+// That flattening is the round-1 P1 defect, and until now nothing would
+// have caught its return.
+func TestRedisSessionPresence_RealRedisReadFailureIsReported(t *testing.T) {
+	t.Parallel()
+	// A closed port: every command fails for real rather than by stubbing.
+	client := redis.NewClient(&redis.Options{
+		Addr:        "127.0.0.1:1",
+		DialTimeout: 200 * time.Millisecond,
+		MaxRetries:  -1,
+	})
+	p := NewRedisSessionPresence(client)
+	t.Cleanup(p.Close)
+
+	sessions, err := p.ListForUser("user-1")
+	if err == nil {
+		t.Fatal("an unreachable Redis must be reported, not returned as zero sessions — an empty list makes a targeted push skip and lose the instruction")
+	}
+	if sessions != nil {
+		t.Fatalf("a failed read must not return a list that looks complete; got %d", len(sessions))
+	}
+}
+
+// TestRedisSessionPresence_RenewalFailureLoggingIsRateLimited — codex
+// round 33 (operability).
+//
+// A Redis outage fails EVERY session's renewal on every tick. At a
+// thousand sessions the unthrottled warning emitted roughly 33 lines a
+// second per replica — enough to page on volume while saying nothing
+// actionable, and enough to bury the errors that matter. That flood was
+// introduced by this change, so it is this change's to bound.
+//
+// Asserts the count is CARRIED, not just that lines are dropped:
+// throttling that loses the number would leave an operator unable to tell
+// one flapping session from a whole replica, which is the
+// Redis-problem-vs-Pad-problem question the log exists to answer.
+func TestRedisSessionPresence_RenewalFailureLoggingIsRateLimited(t *testing.T) {
+	t.Parallel()
+	var buf bytes.Buffer
+	var mu sync.Mutex
+	prev := slog.Default()
+	t.Cleanup(func() { slog.SetDefault(prev) })
+	slog.SetDefault(slog.New(slog.NewJSONHandler(&lockedWriter{w: &buf, mu: &mu}, &slog.HandlerOptions{Level: slog.LevelWarn})))
+
+	p := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: "127.0.0.1:1"}))
+	t.Cleanup(p.Close)
+
+	const failures = 50
+	for i := 0; i < failures; i++ {
+		p.warnRenewFailure(errors.New("connection refused"), "user-1", "sess-"+strconv.Itoa(i))
+	}
+
+	mu.Lock()
+	out := buf.String()
+	mu.Unlock()
+
+	lines := 0
+	for _, l := range strings.Split(strings.TrimSpace(out), "\n") {
+		if strings.Contains(l, "failed to renew session entry") {
+			lines++
+		}
+	}
+	if lines != 1 {
+		t.Fatalf("expected exactly 1 logged warning for %d failures inside the interval, got %d\n%s", failures, lines, out)
+	}
+	if !strings.Contains(out, `"session_id":"sess-0"`) {
+		t.Fatalf("the warning must name a session id — a warning naming only the user cannot be tied to a stuck target:\n%s", out)
+	}
+	// The suppressed count must be present and must not silently be 1: a
+	// throttle that drops the number is worse than one that drops lines.
+	if !strings.Contains(out, `"failures_since_last_log":1`) {
+		t.Fatalf("the first warning should stand for exactly itself:\n%s", out)
+	}
+
+	// The NEXT interval reports how many it stood for. Driven by moving the
+	// throttle's clock back rather than sleeping a minute.
+	p.mu.Lock()
+	p.renewLog.last = time.Now().Add(-2 * renewLogInterval)
+	p.mu.Unlock()
+	p.warnRenewFailure(errors.New("connection refused"), "user-1", "sess-final")
+
+	mu.Lock()
+	out = buf.String()
+	mu.Unlock()
+	if !strings.Contains(out, `"failures_since_last_log":50`) {
+		t.Fatalf("the next warning must carry the suppressed count (49 suppressed + itself = 50):\n%s", out)
+	}
+}
+
+// lockedWriter serialises writes from slog, which a rate-limit test drives
+// from one goroutine but which the race detector still checks.
+type lockedWriter struct {
+	w  *bytes.Buffer
+	mu *sync.Mutex
+}
+
+func (l *lockedWriter) Write(p []byte) (int, error) {
+	l.mu.Lock()
+	defer l.mu.Unlock()
+	return l.w.Write(p)
+}
+
+// TestRedisSessionPresence_RenewLoopUsesTheRateLimitedWarning — codex
+// round 34, and the SECOND time in this review I tested a knob and not the
+// wiring.
+//
+// TestRedisSessionPresence_RenewalFailureLoggingIsRateLimited calls
+// warnRenewFailure directly. It proves the throttle and says nothing about
+// whether renewLoop USES it — restoring the original unbounded slog.Warn
+// at the call site leaves that test green while the flood returns. Round
+// 20 caught the identical shape on the cap's admission flag, and I wrote
+// the lesson down before writing this test.
+//
+// So this drives the real goroutine against an unreachable Redis and
+// counts what reaches the log.
+func TestRedisSessionPresence_RenewLoopUsesTheRateLimitedWarning(t *testing.T) {
+	var buf bytes.Buffer
+	var mu sync.Mutex
+	prev := slog.Default()
+	t.Cleanup(func() { slog.SetDefault(prev) })
+	slog.SetDefault(slog.New(slog.NewJSONHandler(&lockedWriter{w: &buf, mu: &mu}, &slog.HandlerOptions{Level: slog.LevelWarn})))
+
+	// Registration succeeds against miniredis, then the client is pointed at
+	// a closed port so every RENEWAL fails — which is the outage shape, and
+	// the only one where the flood happens.
+	mr := miniredis.RunT(t)
+	p := &RedisSessionPresence{
+		// MaxRetries -1 so a failing renewal returns promptly; with
+		// go-redis's default backoff each failure takes long enough that a
+		// short window sees no attempts at all, which cost this test one
+		// false failure before it was measured rather than assumed.
+		client:        redis.NewClient(&redis.Options{Addr: mr.Addr(), MaxRetries: -1, DialTimeout: 50 * time.Millisecond}),
+		sessionKeyTTL: time.Minute,
+		renewInterval: 5 * time.Millisecond,
+		opTimeout:     50 * time.Millisecond,
+		renewals:      make(map[string]*renewal),
+	}
+	t.Cleanup(p.Close)
+	p.Add("user-1", SessionIdentity{Armed: true})
+
+	// Kill Redis out from under the renewal loop.
+	mr.Close()
+
+	// Long enough for many failing ticks — far more than one interval's
+	// worth of allowed lines.
+	time.Sleep(2 * time.Second)
+
+	mu.Lock()
+	out := buf.String()
+	mu.Unlock()
+
+	lines := strings.Count(out, "failed to renew session entry")
+	if lines == 0 {
+		t.Fatalf("expected the renewal loop to report its failures at least once:\n%s", out)
+	}
+	// ~80 ticks in that window; unthrottled the loop would log every one.
+	if lines > 2 {
+		t.Fatalf("renewLoop is not going through the rate-limited warning — %d lines for one interval's worth of failures:\n%s", lines, out)
+	}
+}

internal/server/session_presence_redis_test.go

@@ -0,0 +1,344 @@
+package server
+
+import (
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/alicebob/miniredis/v2"
+	"github.com/redis/go-redis/v9"
+)
+
+// BUG-2698 — the registry half.
+//
+// These drive TWO RedisSessionPresence values against ONE miniredis,
+// because that is what the bug is: two padd processes behind a load
+// balancer, each with its own registry object, disagreeing about who is
+// connected. A single-instance test would pass on the broken code.
+
+func newRedisPresencePair(t *testing.T) (*RedisSessionPresence, *RedisSessionPresence, *miniredis.Miniredis) {
+	t.Helper()
+	mr := miniredis.RunT(t)
+	newOne := func() *RedisSessionPresence {
+		client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+		p := NewRedisSessionPresence(client)
+		t.Cleanup(p.Close)
+		return p
+	}
+	return newOne(), newOne(), mr
+}
+
+// TestRedisSessionPresence_VisibleAcrossInstances is the defect, stated
+// directly: a session registered on instance B must appear in instance A's
+// listing. Before this type, A's ListForUser returned nothing for it, and
+// handlers_push.go skipped the publish on that basis.
+func TestRedisSessionPresence_VisibleAcrossInstances(t *testing.T) {
+	t.Parallel()
+	instanceB, instanceA, _ := newRedisPresencePair(t)
+
+	id := instanceB.Add("user-1", SessionIdentity{Label: "docapp", PID: 4242, Armed: true})
+
+	sessions := mustList(t, instanceA, "user-1")
+	if len(sessions) != 1 {
+		t.Fatalf("instance A must see the session registered on B; got %d sessions", len(sessions))
+	}
+	if sessions[0].ID != id {
+		t.Fatalf("expected session id %q, got %q", id, sessions[0].ID)
+	}
+	// The fields the target picker and the push gate actually read. Armed
+	// especially: deliveredSessionCount drops unarmed sessions, so an entry
+	// that crossed the wire with Armed lost would be counted as absent and
+	// the push skipped — the original bug wearing a different hat.
+	if !sessions[0].Armed {
+		t.Fatal("Armed must survive the round trip: an unarmed-looking session is skipped by the push gate")
+	}
+	if sessions[0].Label != "docapp" || sessions[0].PID != 4242 {
+		t.Fatalf("identity lost in transit: %+v", sessions[0])
+	}
+	if sessions[0].ConnectedAt.IsZero() {
+		t.Fatal("ConnectedAt must survive the round trip — it is the picker's sort key")
+	}
+}
+
+// TestMemorySessionPresence_NotVisibleAcrossInstances is the NEGATIVE
+// CONTROL for the test above, and it is what makes that test evidence
+// rather than a tautology: the same two-registry shape against the
+// in-memory implementation must NOT see across, because that is precisely
+// the behaviour BUG-2698 reports. If this ever passes, the test above has
+// stopped discriminating.
+func TestMemorySessionPresence_NotVisibleAcrossInstances(t *testing.T) {
+	t.Parallel()
+	instanceB := NewMemorySessionPresence()
+	instanceA := NewMemorySessionPresence()
+
+	instanceB.Add("user-1", SessionIdentity{Label: "docapp", Armed: true})
+
+	if got := len(mustList(t, instanceA, "user-1")); got != 0 {
+		t.Fatalf("in-memory presence is per-process by definition; instance A saw %d sessions", got)
+	}
+}
+
+// TestRedisSessionPresence_RemoveIsImmediateAndCrossInstance: a clean
+// disconnect must deregister NOW, not on the TTL. If Remove only stopped
+// the renewal, a closed session would linger in every other instance's
+// picker for the full TTL — and a targeted push at it would be published
+// and delivered to nobody.
+func TestRedisSessionPresence_RemoveIsImmediateAndCrossInstance(t *testing.T) {
+	t.Parallel()
+	instanceB, instanceA, _ := newRedisPresencePair(t)
+
+	id := instanceB.Add("user-1", SessionIdentity{Armed: true})
+	if got := len(mustList(t, instanceA, "user-1")); got != 1 {
+		t.Fatalf("precondition: A should see 1 session, got %d", got)
+	}
+
+	instanceB.Remove("user-1", id)
+
+	if got := len(mustList(t, instanceA, "user-1")); got != 0 {
+		t.Fatalf("Remove must be visible immediately on other instances; A still sees %d", got)
+	}
+}
+
+// TestRedisSessionPresence_RemoveIsIdempotentAndSafeForUnknownIDs pins the
+// interface contract the stream handler depends on: Remove runs from a
+// defer that also fires on paths where Add was never reached.
+func TestRedisSessionPresence_RemoveIsIdempotentAndSafeForUnknownIDs(t *testing.T) {
+	t.Parallel()
+	p, _, _ := newRedisPresencePair(t)
+
+	p.Remove("user-1", "")
+	p.Remove("user-1", "never-added")
+	id := p.Add("user-1", SessionIdentity{Armed: true})
+	p.Remove("user-1", id)
+	p.Remove("user-1", id)
+
+	if got := len(mustList(t, p, "user-1")); got != 0 {
+		t.Fatalf("expected no sessions, got %d", got)
+	}
+}
+
+// TestRedisSessionPresence_CrashedInstanceEntriesExpire is the reaping
+// story session_presence.go's interface doc REQUIRES of any out-of-process
+// implementation — the constraint MemorySessionPresence is exempt from
+// because its entries die with its process.
+//
+// A crash is modelled by the only thing a crash actually is from Redis's
+// side: renewals stop. The registry object is abandoned WITHOUT Close and
+// WITHOUT Remove, exactly as a killed process leaves it, and time is moved
+// past the TTL.
+func TestRedisSessionPresence_CrashedInstanceEntriesExpire(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+
+	// Short TTL so the expiry path is driven for real rather than waited
+	// out. The renewal interval is longer than the TTL here on purpose:
+	// this models the crashed process, which by definition renews nothing.
+	crashed := &RedisSessionPresence{
+		client:        client,
+		sessionKeyTTL: 2 * time.Second,
+		renewInterval: time.Hour,
+		renewals:      make(map[string]*renewal),
+	}
+	// Cleaned up at the END, after the expiry has been observed (codex
+	// round 13). The crash is modelled by the renewal never FIRING — a
+	// one-hour ticker — so joining the goroutine afterwards does not weaken
+	// the scenario, while abandoning it leaks a goroutine and a Redis
+	// client into every subsequent test in the package and into -count
+	// reruns.
+	t.Cleanup(crashed.Close)
+	crashed.Add("user-1", SessionIdentity{Armed: true})
+
+	survivor := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(survivor.Close)
+	if got := len(mustList(t, survivor, "user-1")); got != 1 {
+		t.Fatalf("precondition: the survivor should see the crashed instance's session, got %d", got)
+	}
+
+	mr.FastForward(3 * time.Second)
+
+	if got := len(mustList(t, survivor, "user-1")); got != 0 {
+		t.Fatalf("a crashed instance's entries must expire; still listing %d", got)
+	}
+	// The index must self-heal too, not just the entry. Without the prune,
+	// the SET accumulates a dead member per crashed session forever, and
+	// every future ListForUser pays an MGET for keys that will never exist.
+	members, err := client.SMembers(t.Context(), sessionIndexKey("user-1")).Result()
+	if err != nil {
+		t.Fatalf("read index: %v", err)
+	}
+	if len(members) != 0 {
+		t.Fatalf("expired session left %d stale index member(s): %v", len(members), members)
+	}
+}
+
+// TestRedisSessionPresence_RenewalKeepsALiveSessionListed is the positive
+// control for the expiry test above. Without it, an implementation that
+// dropped every session after the TTL — renewals broken, entries never
+// restored — would pass the crash test and look correct.
+func TestRedisSessionPresence_RenewalKeepsALiveSessionListed(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+	// MARGIN, deliberately generous (codex round 13). The first version ran
+	// a 2s fake TTL forward by 1s every 50ms of wall time, so a ~100ms
+	// scheduler pause could expire the session before a renewal ran and
+	// report "renewal broken" for a loaded CI machine. A 10s TTL tolerates
+	// a half-second pause while the loop still advances fake time PAST the
+	// TTL in total — so a genuinely broken renewal still expires and still
+	// fails. Margin without losing discrimination.
+	live := &RedisSessionPresence{
+		client:        client,
+		sessionKeyTTL: 10 * time.Second,
+		renewInterval: 20 * time.Millisecond,
+		renewals:      make(map[string]*renewal),
+	}
+	t.Cleanup(live.Close)
+	live.Add("user-1", SessionIdentity{Armed: true})
+
+	// Let several renewals run, moving miniredis's clock less far than the
+	// TTL between them, so the session survives only if renewal works.
+	// 20 iterations advance fake time by 20s total, comfortably past the
+	// 10s TTL, so an implementation that never renews cannot survive this
+	// loop.
+	for i := 0; i < 20; i++ {
+		time.Sleep(50 * time.Millisecond)
+		mr.FastForward(time.Second)
+	}
+
+	if got := len(mustList(t, live, "user-1")); got != 1 {
+		t.Fatalf("a renewed session must stay listed; got %d", got)
+	}
+}
+
+// TestRedisSessionPresence_OrderIsDeterministic pins the same ordering
+// MemorySessionPresence guarantees — oldest first, id as tiebreaker. Redis
+// SETs are unordered, so without the explicit sort the web target picker
+// would reorder under the user's cursor between polls.
+func TestRedisSessionPresence_OrderIsDeterministic(t *testing.T) {
+	t.Parallel()
+	p, reader, _ := newRedisPresencePair(t)
+
+	for i := 0; i < 5; i++ {
+		p.Add("user-1", SessionIdentity{Armed: true})
+	}
+
+	first := mustList(t, reader, "user-1")
+	if len(first) != 5 {
+		t.Fatalf("expected 5 sessions, got %d", len(first))
+	}
+	for i := 1; i < len(first); i++ {
+		if first[i].ConnectedAt.Before(first[i-1].ConnectedAt) {
+			t.Fatalf("not oldest-first at %d: %v before %v", i, first[i].ConnectedAt, first[i-1].ConnectedAt)
+		}
+	}
+	for attempt := 0; attempt < 5; attempt++ {
+		again := mustList(t, reader, "user-1")
+		for i := range again {
+			if again[i].ID != first[i].ID {
+				t.Fatalf("listing order changed between reads at index %d", i)
+			}
+		}
+	}
+}
+
+// TestRedisSessionPresence_ScopedPerUser: the cross-user boundary that
+// makes a foreign target_session_id structurally indistinguishable from a
+// vanished one (deliveredSessionCount's doc comment) has to hold for the
+// shared registry too — sharing state across INSTANCES must not leak state
+// across USERS.
+func TestRedisSessionPresence_ScopedPerUser(t *testing.T) {
+	t.Parallel()
+	instanceB, instanceA, _ := newRedisPresencePair(t)
+
+	instanceB.Add("user-1", SessionIdentity{Armed: true})
+
+	if got := len(mustList(t, instanceA, "user-2")); got != 0 {
+		t.Fatalf("user-2 must not see user-1's sessions; got %d", got)
+	}
+}
+
+// TestRedisSessionPresence_RemoveWaitsForAnInFlightRenewal — codex round
+// 1, P2.
+//
+// Remove cancels the renewal goroutine and then DELs the entry.
+// Cancelling returns immediately, so an unfixed Remove leaves a renewal
+// already inside its write free to complete AFTERWARDS and re-create the
+// key — resurrecting a session that has just disconnected and leaving it
+// in every instance's picker until the TTL lapses. A targeted push at
+// that ghost publishes, reaches nobody, and reports one delivery.
+//
+// Driven through a seam that holds a renewal INSIDE its write, rather
+// than by racing it. The probabilistic version of this test (50µs renewal
+// interval, 200 add/remove iterations) passed 3/3 against the unfixed
+// Remove and was deleted: an instrument that cannot fail on broken code
+// proves nothing, and keeping it would have made the mutation matrix a
+// liar.
+func TestRedisSessionPresence_RemoveWaitsForAnInFlightRenewal(t *testing.T) {
+	t.Parallel()
+	mr := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
+
+	entered := make(chan struct{})
+	release := make(chan struct{})
+	var once sync.Once
+	p := &RedisSessionPresence{
+		client:        client,
+		sessionKeyTTL: time.Minute,
+		renewInterval: 5 * time.Millisecond,
+		renewals:      make(map[string]*renewal),
+		onRenewWrite: func() {
+			once.Do(func() {
+				close(entered)
+				<-release
+			})
+		},
+	}
+	t.Cleanup(func() {
+		select {
+		case <-release:
+		default:
+			close(release)
+		}
+		p.Close()
+	})
+
+	id := p.Add("user-1", SessionIdentity{Armed: true})
+	<-entered // a renewal is now parked immediately before its write
+
+	removed := make(chan struct{})
+	go func() {
+		p.Remove("user-1", id)
+		close(removed)
+	}()
+
+	// THE ASSERTION, and the one the unfixed Remove fails: it must still be
+	// waiting, because the renewal it cancelled has not finished. An
+	// unfixed Remove has already returned by now, having deleted the key
+	// that the parked write is about to re-create.
+	select {
+	case <-removed:
+		t.Fatal("Remove returned while a renewal write was still in flight; that renewal can re-create the entry after the delete")
+	case <-time.After(100 * time.Millisecond):
+	}
+
+	close(release)
+
+	select {
+	case <-removed:
+	case <-time.After(5 * time.Second):
+		t.Fatal("Remove never returned after the renewal was released")
+	}
+
+	// And the consequence the wait exists to prevent, asserted from a
+	// DIFFERENT registry object the way another instance would see it.
+	reader := NewRedisSessionPresence(redis.NewClient(&redis.Options{Addr: mr.Addr()}))
+	t.Cleanup(reader.Close)
+	sessions, err := reader.ListForUser("user-1")
+	if err != nil {
+		t.Fatalf("list: %v", err)
+	}
+	if len(sessions) != 0 {
+		t.Fatalf("Remove left a ghost session (%d listed) — a renewal completed after the delete", len(sessions))
+	}
+}

internal/server/session_presence_test.go

@@ -10,7 +10,7 @@ func TestMemorySessionPresence_AddListRemove(t *testing.T) {
 	t.Parallel()
 	p := NewMemorySessionPresence()
 
-	if got := p.ListForUser("u1"); len(got) != 0 {
+	if got := mustList(t, p, "u1"); len(got) != 0 {
 		t.Fatalf("expected no sessions for an unknown user, got %d", len(got))
 	}
 
@@ -22,7 +22,7 @@ func TestMemorySessionPresence_AddListRemove(t *testing.T) {
 		t.Fatal("two connections for the same user must get distinct ids")
 	}
 
-	got := p.ListForUser("u1")
+	got := mustList(t, p, "u1")
 	if len(got) != 2 {
 		t.Fatalf("expected 2 sessions for u1, got %d", len(got))
 	}
@@ -38,11 +38,11 @@ func TestMemorySessionPresence_AddListRemove(t *testing.T) {
 	}
 
 	p.Remove("u1", a)
-	got = p.ListForUser("u1")
+	got = mustList(t, p, "u1")
 	if len(got) != 1 || got[0].ID != b {
 		t.Fatalf("expected only session %s to remain, got %+v", b, got)
 	}
-	if len(p.ListForUser("u2")) != 1 {
+	if len(mustList(t, p, "u2")) != 1 {
 		t.Fatal("removing u1's session disturbed u2's list")
 	}
 }
@@ -61,7 +61,7 @@ func TestMemorySessionPresence_RemoveIsForgiving(t *testing.T) {
 	p.Remove("u1", id)
 	p.Remove("u1", id) // double-remove: idempotent, not a panic
 
-	if got := p.ListForUser("u1"); len(got) != 0 {
+	if got := mustList(t, p, "u1"); len(got) != 0 {
 		t.Fatalf("expected u1 to have no sessions, got %d", len(got))
 	}
 }
@@ -87,7 +87,7 @@ func TestMemorySessionPresence_ListIsOldestFirstAndStable(t *testing.T) {
 	// Same input, repeated reads: the order must not depend on Go's
 	// randomized map iteration.
 	for i := 0; i < 20; i++ {
-		got := p.ListForUser("u1")
+		got := mustList(t, p, "u1")
 		if len(got) != 3 {
 			t.Fatalf("expected 3 sessions, got %d", len(got))
 		}
@@ -108,10 +108,10 @@ func TestMemorySessionPresence_ListReturnsACopy(t *testing.T) {
 	p := NewMemorySessionPresence()
 	id := p.Add("u1", SessionIdentity{Label: "docapp"})
 
-	got := p.ListForUser("u1")
+	got := mustList(t, p, "u1")
 	got[0].Label = "tampered"
 
-	fresh := p.ListForUser("u1")
+	fresh := mustList(t, p, "u1")
 	if fresh[0].Label != "docapp" {
 		t.Fatalf("mutating the returned slice changed the registry: label is now %q", fresh[0].Label)
 	}
@@ -131,7 +131,7 @@ func TestMemorySessionPresence_ArmedRoundTrips(t *testing.T) {
 	armedID := p.Add("u1", SessionIdentity{Label: "docapp", Armed: true})
 	unarmedID := p.Add("u1", SessionIdentity{Label: "voiapp"}) // Armed left at its zero value
 
-	got := p.ListForUser("u1")
+	got := mustList(t, p, "u1")
 	if len(got) != 2 {
 		t.Fatalf("expected 2 sessions, got %d", len(got))
 	}
@@ -161,14 +161,14 @@ func TestMemorySessionPresence_ConcurrentAccess(t *testing.T) {
 			defer wg.Done()
 			for j := 0; j < 50; j++ {
 				id := p.Add("u1", SessionIdentity{})
-				p.ListForUser("u1")
+				mustList(t, p, "u1")
 				p.Remove("u1", id)
 			}
 		}()
 	}
 	wg.Wait()
 
-	if got := p.ListForUser("u1"); len(got) != 0 {
+	if got := mustList(t, p, "u1"); len(got) != 0 {
 		t.Fatalf("every Add was paired with a Remove, expected 0 sessions, got %d", len(got))
 	}
 	// The user's bucket should be reclaimed too, not left as an empty map.
@@ -179,3 +179,21 @@ func TestMemorySessionPresence_ConcurrentAccess(t *testing.T) {
 		t.Fatal("expected the empty user bucket to be reclaimed")
 	}
 }
+
+// mustList reads a registry's session list, failing the test if the read
+// itself failed.
+//
+// ListForUser gained an error when an out-of-process implementation made
+// "I could not find out" a reachable state distinct from "there are none"
+// (BUG-2698, codex round 1 P1). Going through this helper makes every
+// existing test assert its own premise — that the read SUCCEEDED — rather
+// than comparing lengths against a nil slice that an error would also
+// produce.
+func mustList(t *testing.T, p SessionPresence, userID string) []LiveSession {
+	t.Helper()
+	sessions, err := p.ListForUser(userID)
+	if err != nil {
+		t.Fatalf("ListForUser(%q): %v", userID, err)
+	}
+	return sessions
+}

internal/watchevents/publish_result_test.go

@@ -0,0 +1,129 @@
+package watchevents
+
+import (
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/redis/go-redis/v9"
+)
+
+// BUG-2699 — Publish reports acceptance.
+//
+// The pre-fix behaviour these pin is not "an error was swallowed": it is
+// that a post-Close publish on MemoryBus did nothing AT ALL and said
+// nothing about it — no error (there was no channel to return one on)
+// and, unlike RedisBus, not even a log line, while still consuming a
+// sequence id. The single-process deployment had the same shutdown
+// window as the Redis one, with strictly less evidence.
+
+// TestMemoryBusPublishAfterCloseReportsClosed drives the case the
+// shutdown ordering in cmd/pad/cmd_server.go actually creates: the bus is
+// closed before http.Server.Shutdown drains handlers, so a push already
+// in its handler publishes into a closed bus.
+func TestMemoryBusPublishAfterCloseReportsClosed(t *testing.T) {
+	t.Parallel()
+	bus := New()
+
+	// POSITIVE CONTROL first, and it is not decoration: without it a bus
+	// that refused EVERY publish would satisfy the assertion below, and
+	// this test would be evidence that nothing works rather than that
+	// Close is what refuses.
+	if err := bus.Publish(Notification{Kind: KindPush, ItemRef: "TASK-1"}); err != nil {
+		t.Fatalf("publish on a live bus must be accepted, got %v", err)
+	}
+
+	bus.Close()
+
+	err := bus.Publish(Notification{Kind: KindPush, ItemRef: "TASK-1"})
+	if !errors.Is(err, ErrBusClosed) {
+		t.Fatalf("expected ErrBusClosed after Close, got %v", err)
+	}
+}
+
+// TestMemoryBusPublishAfterCloseDoesNotBurnSequence asserts the OTHER
+// half of the old behaviour — the part that is invisible in the return
+// value. The unfixed Publish incremented seq and appended to the replay
+// buffer for a notification no subscriber could ever receive, so a
+// resuming client's Last-Event-ID space contained ids that never
+// corresponded to a delivered notification.
+//
+// Written as an assertion about what the WRONG behaviour DOES (the
+// counter moved) rather than about what the right one leaves looking
+// unchanged (CONVE-12).
+func TestMemoryBusPublishAfterCloseDoesNotBurnSequence(t *testing.T) {
+	t.Parallel()
+	bus := New()
+	if err := bus.Publish(Notification{Kind: KindComment, ItemRef: "TASK-1"}); err != nil {
+		t.Fatalf("live publish: %v", err)
+	}
+
+	before := bus.EventsSince(0)
+	if len(before) != 1 {
+		t.Fatalf("expected 1 buffered notification before Close, got %d", len(before))
+	}
+	highWater := before[len(before)-1].ID
+
+	bus.Close()
+	_ = bus.Publish(Notification{Kind: KindComment, ItemRef: "TASK-2"})
+
+	after := bus.EventsSince(0)
+	if len(after) != 1 {
+		t.Fatalf("post-Close publish must not append to the replay buffer: %d entries", len(after))
+	}
+	if after[len(after)-1].ID != highWater {
+		t.Fatalf("post-Close publish burned a sequence id: %d -> %d", highWater, after[len(after)-1].ID)
+	}
+}
+
+// TestRedisBusPublishAfterCloseReportsClosed pins the distinction the
+// push handler acts on. Close cancels the bus context, so an unfixed
+// post-Close publish fails with a CONTEXT error — indistinguishable, from
+// the caller's side, from a request that lost its reply to the network.
+// One of those proves nothing was published and the other does not, so
+// the sentinel has to come from the closed flag, not from the error text.
+//
+// The client points at a closed port: if the closed-flag check were
+// removed, this would still fail, but with a dial error rather than
+// ErrBusClosed — which is exactly the confusion being ruled out.
+func TestRedisBusPublishAfterCloseReportsClosed(t *testing.T) {
+	t.Parallel()
+	client := redis.NewClient(&redis.Options{
+		Addr:        "127.0.0.1:1",
+		DialTimeout: 200 * time.Millisecond,
+		MaxRetries:  -1,
+	})
+	bus := NewRedisBus(client)
+	bus.Close()
+
+	err := bus.Publish(Notification{Kind: KindPush, ItemRef: "TASK-1"})
+	if !errors.Is(err, ErrBusClosed) {
+		t.Fatalf("expected ErrBusClosed after Close, got %v", err)
+	}
+}
+
+// TestRedisBusPublishFailureIsNotClosed is the discriminating leg: a live
+// bus whose Redis is unreachable must report an error that is NOT
+// ErrBusClosed, because that outcome is UNCONFIRMED (go-redis retries a
+// command whose reply was lost, so the script may have run). If this
+// returned ErrBusClosed the push endpoint would answer 503 `unavailable`,
+// which the web client treats as safe to resend — offering a duplicate
+// dispatch.
+func TestRedisBusPublishFailureIsNotClosed(t *testing.T) {
+	t.Parallel()
+	client := redis.NewClient(&redis.Options{
+		Addr:        "127.0.0.1:1",
+		DialTimeout: 200 * time.Millisecond,
+		MaxRetries:  -1,
+	})
+	bus := NewRedisBus(client)
+	defer bus.Close()
+
+	err := bus.Publish(Notification{Kind: KindPush, ItemRef: "TASK-1"})
+	if err == nil {
+		t.Fatal("expected an error publishing to an unreachable Redis")
+	}
+	if errors.Is(err, ErrBusClosed) {
+		t.Fatalf("an unreachable-Redis failure must not claim the bus was closed: %v", err)
+	}
+}

internal/watchevents/redis_bus.go

@@ -327,7 +327,7 @@ func NewRedisBusWithReplaySize(client *redis.Client, size int) *RedisBus {
 // that fallback could have applied: there is no longer a window in which an id
 // exists but the publish has not happened, so "assign or don't" and "publish
 // or don't" are the same decision.
-func (b *RedisBus) Publish(n Notification) {
+func (b *RedisBus) Publish(n Notification) error {
 	if n.Timestamp == 0 {
 		n.Timestamp = time.Now().UnixMilli()
 	}
@@ -338,7 +338,23 @@ func (b *RedisBus) Publish(n Notification) {
 	data, err := json.Marshal(n)
 	if err != nil {
 		slog.Error("watchevents: failed to marshal notification for Redis", "error", err, "kind", n.Kind)
-		return
+		return fmt.Errorf("watchevents: marshal notification: %w", err)
+	}
+
+	// Checked BEFORE the script call, and reported as ErrBusClosed rather
+	// than as whatever the cancelled context happens to produce (BUG-2699).
+	// Close() cancels b.ctx, so a post-Close publish already failed — but
+	// it failed with a context error indistinguishable from a request that
+	// raced a real cancellation, and the caller's two outcomes turn on
+	// exactly that distinction: closed proves nothing was published, while
+	// a transport error does not. Reading b.closed under the lock is what
+	// makes the proof available; inferring it from the error text would
+	// not.
+	b.mu.Lock()
+	closed := b.closed
+	b.mu.Unlock()
+	if closed {
+		return ErrBusClosed
 	}
 
 	// A fresh token per logical publish — NOT per attempt, which is the
@@ -351,9 +367,27 @@ func (b *RedisBus) Publish(n Notification) {
 	if err := publishScript.Run(b.ctx, b.client,
 		[]string{redisWatchSeqKey, redisWatchChannel, dedupeKey, redisWatchEpochKey},
 		string(data), redisWatchDedupeTTLSeconds, uuid.NewString()).Err(); err != nil {
-		slog.Error("watchevents: dropping notification — Redis publish failed, so no globally ordered ID was assigned",
+		// WORDED AS UNCONFIRMED, not as a drop (codex round 3). The earlier
+		// text said "dropping notification ... no globally ordered ID was
+		// assigned", which contradicts what this error actually means and
+		// contradicted the return path four lines down: go-redis retries a
+		// command whose reply was lost, so the script may already have run
+		// and published. An operator who reads "dropped" and re-sends turns
+		// a possible delivery into a duplicate DISPATCH.
+		slog.Error("watchevents: publish outcome UNCONFIRMED — the Redis call failed, but a lost reply can mean the notification was published anyway; do not re-send without checking",
 			"error", err, "kind", n.Kind, "item_ref", n.ItemRef)
+		// Returned as a plain wrapped error, deliberately NOT ErrBusClosed
+		// and deliberately not described as a drop to the caller, however
+		// the log line above phrases it for an operator. This error means
+		// UNCONFIRMED: go-redis retries a command whose reply was lost to a
+		// network error, which is the entire reason the script carries a
+		// SET-NX dedupe token (codex round 5), so the script may well have
+		// run and published while this call still returns non-nil. A caller
+		// that re-publishes on this error risks a duplicate DISPATCH, not a
+		// repeat of nothing — see Bus.Publish's doc comment.
+		return fmt.Errorf("watchevents: redis publish: %w", err)
 	}
+	return nil
 }
 
 // Subscribe returns a channel receiving every future Notification, with no

internal/watchevents/redis_bus_integration_test.go

@@ -209,3 +209,39 @@ func TestRedisBusCloseStopsReceiving(t *testing.T) {
 		t.Fatalf("Redis still reports %d subscriber(s) after Close", n)
 	}
 }
+
+// TestRedisBusPublishReportsAcceptanceOnASuccessfulRoundTrip — codex round
+// 30 (coverage-gap sweep).
+//
+// publish_result_test.go covers the FAILURE directions — ErrBusClosed, and
+// a transport error that must not claim the bus was closed — against a
+// dead client. Nothing asserted the success direction against a real
+// round trip: every integration test here ignored Publish's return value,
+// so an implementation that published correctly and then returned a
+// non-nil error would have made every push answer 502 push_unconfirmed
+// while the suite stayed green.
+//
+// Asserts BOTH that the call reports acceptance and that the notification
+// actually arrived, because either alone is compatible with the bug: a nil
+// error proves nothing if nothing was published, and an arrival proves
+// nothing about what the caller was told.
+func TestRedisBusPublishReportsAcceptanceOnASuccessfulRoundTrip(t *testing.T) {
+	bus, _ := newMiniredisBus(t, 16)
+	ch := bus.Subscribe()
+
+	if err := bus.Publish(Notification{Kind: KindPush, ItemRef: "TASK-1", Summary: "triage this"}); err != nil {
+		t.Fatalf("a successful publish must report acceptance, got %v", err)
+	}
+
+	select {
+	case got := <-ch:
+		if got.ItemRef != "TASK-1" || got.Summary != "triage this" {
+			t.Fatalf("unexpected notification: %+v", got)
+		}
+		if got.ID == 0 {
+			t.Fatal("a published notification must carry the id the script assigned")
+		}
+	case <-time.After(3 * time.Second):
+		t.Fatal("publish reported acceptance but nothing arrived — the nil error was not evidence of a publish")
+	}
+}

internal/watchevents/watchevents.go

@@ -18,29 +18,52 @@
 // single-process binary keeps MemoryBus and never grows a Redis
 // dependency.
 //
-// Bus was defined as an interface from Phase 1 precisely so this could
-// slot in without touching any producer or the stream handler, and that
-// held: RedisBus changed neither.
+// Bus was defined as an interface from Phase 1 precisely so a second
+// implementation could slot in without touching any producer or the stream
+// handler, and that held for RedisBus: it changed neither.
 //
-// STILL PER-PROCESS, and worth knowing before assuming multi-instance is
-// finished: internal/server's SessionPresence registry. What RedisBus
-// fixes is delivery of what is actually PUBLISHED — a broadcast push, a
-// status change, a comment — which now reaches a stream on any instance.
-// What it does not fix is a SESSION-TARGETED push, because
-// handlers_push.go consults that per-process registry and skips the
-// publish entirely when the named session is not local; the bus would
-// carry it, but it is never put on the bus. The
-// GET /api/v1/sessions listing is per-process for the same reason.
-// See session_presence.go's own note — one shared-state implementation
-// closes both.
+// It did NOT hold for BUG-2699, and the distinction is worth keeping
+// (codex round 6). Making Publish report acceptance changed the INTERFACE,
+// not just the implementations, so every producer had to be revisited —
+// not to change behaviour, but to rule on what each should do with an
+// answer it had never been given. An interface absorbs a new
+// implementation; it does not absorb a new question.
+//
+// PRESENCE IS NO LONGER THE ODD ONE OUT. internal/server's
+// SessionPresence registry used to stay per-process after this bus went
+// shared, and the combination was worse than either being consistent: a
+// SESSION-TARGETED push is gated on that registry BEFORE publishing
+// (handlers_push.go), so a push for a session held on another instance
+// was skipped rather than carried, and GET /api/v1/sessions could not
+// offer it as a target at all. BUG-2698 closed that with
+// RedisSessionPresence, on the same PAD_REDIS_URL switch. All three —
+// event bus, watch bus, presence registry — now cross instance
+// boundaries together or none of them do.
+//
+// See internal/server/session_presence.go for that registry's own
+// reaping story, which this package's Redis bus does not need: a
+// notification is transient, while a presence entry outlives the process
+// that wrote it and has to expire on its own.
 package watchevents
 
 import (
+	"errors"
 	"log/slog"
 	"sync"
 	"time"
 )
 
+// ErrBusClosed is returned by Publish when the bus has already been shut
+// down (BUG-2699). It is the ONE Publish failure that proves the
+// notification was not published: Close has already emptied the
+// subscriber set, so nothing could have received it and nothing can.
+//
+// It exists as a sentinel rather than an ordinary error because callers
+// act on the distinction — see Bus.Publish's doc comment on why every
+// other error means "unconfirmed" instead. Compare with errors.Is; both
+// implementations wrap it with context.
+var ErrBusClosed = errors.New("watchevents: bus is closed")
+
 // Notification kinds, matching DOC-2479's event payload contract
 // (kind ∈ {status-change, assignment, comment, ask}) exactly.
 const (
@@ -62,12 +85,14 @@ const (
 	// right now rather than waiting on assignment/watch semantics. NOT
 	// "publishes exactly one" unconditionally as of PLAN-2558 S5
 	// (TASK-2588): handlePushToItem decides whether to publish at all —
-	// a session-targeted request whose id matches no LOCAL live session
-	// skips the publish entirely. That was a guaranteed no-op under
-	// MemoryBus and is merely a skip under RedisBus, where the session
-	// might be live on another instance; see handlers_push.go's own
-	// comment on that gate, plus TargetSessionID and
-	// pushResponse.DeliveredSessions there.
+	// a session-targeted request whose id matches no live session in the
+	// presence registry skips the publish entirely. That registry is
+	// per-process with MemoryBus and SHARED with RedisBus (BUG-2698), so
+	// the skip means "nothing is listening anywhere" in both shapes — an
+	// earlier version of this sentence said LOCAL, which described the
+	// window between BUG-2651 and BUG-2698 (codex round 24). See
+	// handlers_push.go's own comment on that gate, plus TargetSessionID
+	// and pushResponse.DeliveredSessions there.
 	// KindAsk is push's reserved sibling in the other direction
 	// (harness→human) — see TargetUserID's doc comment for the shared
 	// envelope shape the two are meant to converge on.
@@ -129,9 +154,10 @@ type Notification struct {
 	// event-stream connections (PLAN-2558 S5, TASK-2588). Populated only
 	// on Kind == KindPush, and only when the pusher named a specific
 	// session id from the S1 presence registry (GET /api/v1/sessions);
-	// empty means "every one of TargetUserID's connected sessions" —
-	// broadcast is targeted-with-an-empty-predicate, not a separate
-	// code path. watchNotificationVisible checks this against the
+	// empty means "every one of TargetUserID's sessions that the delivery
+	// predicate admits" — armed, and able to see the item — rather than
+	// every CONNECTED one; broadcast is targeted-with-an-empty-SESSION-
+	// predicate, not a separate code path and not an unfiltered one. watchNotificationVisible checks this against the
 	// SAME session id session_presence.go handed the connection at
 	// Add() time, exactly parallel to how TargetUserID gates against
 	// the connected caller's user id. An id that names no live session
@@ -160,7 +186,31 @@ type Bus interface {
 	// their IDs from separate critical sections could otherwise append
 	// to the replay buffer out of ID order, corrupting since()'s
 	// ordering assumptions.
-	Publish(n Notification)
+	//
+	// REPORTS ACCEPTANCE, NOT DELIVERY (BUG-2699). A nil error means the
+	// bus took ownership of the notification; it says nothing about
+	// whether any subscriber read it, and there is still no ack from the
+	// receiving side. Most producers publish best-effort on top of a
+	// durable store write and should discard the result deliberately —
+	// internal/server's publishWatchNotification helper is where that
+	// ruling lives. The push endpoint is the one caller that acts on it,
+	// because a push has no persistence to recover from.
+	//
+	// A RETURNED ERROR IS TWO DIFFERENT OUTCOMES, and a caller that acts
+	// on one must not collapse them:
+	//
+	//   - ErrBusClosed means nothing was published, provably. The bus was
+	//     already shut down; no subscriber can ever have seen it. Safe to
+	//     report as a clean refusal and safe for the caller to retry
+	//     against a live bus.
+	//   - Any OTHER error means UNCONFIRMED, not "did not happen".
+	//     RedisBus's publish is a Lua script call, and go-redis retries a
+	//     command whose reply was lost to a network error — which is why
+	//     that script carries a dedupe token at all (see redis_bus.go's
+	//     publishScript). The script may have run and published while the
+	//     call still returns an error, so re-publishing risks a DUPLICATE
+	//     dispatch rather than a repeat of nothing.
+	Publish(n Notification) error
 	// Subscribe returns a channel that receives every future
 	// Notification, with NO replay. There is exactly one logical stream
 	// (unlike internal/events.EventBus, which is workspace-scoped) —
@@ -320,7 +370,7 @@ func NewWithReplaySize(size int) *MemoryBus {
 // lock costs nothing (the send is O(1) and non-blocking either way) and
 // closes that window structurally: Unsubscribe/Close can no longer run
 // between "this channel is still a live subscriber" and "send to it".
-func (b *MemoryBus) Publish(n Notification) {
+func (b *MemoryBus) Publish(n Notification) error {
 	if n.Timestamp == 0 {
 		n.Timestamp = time.Now().UnixMilli()
 	}
@@ -328,6 +378,25 @@ func (b *MemoryBus) Publish(n Notification) {
 	b.mu.Lock()
 	defer b.mu.Unlock()
 
+	// BUG-2699: publishing after Close was the SILENT half of this bug,
+	// and the worse half. Close empties b.subscribers, so the loop below
+	// ran over an empty map and returned having done nothing at all — no
+	// error (Publish had no way to say so), and unlike RedisBus not even
+	// a log line, while still burning a sequence id and appending to a
+	// replay buffer nobody will ever read from.
+	//
+	// The window is real for the SINGLE-PROCESS deployment, not only for
+	// Redis: cmd/pad/cmd_server.go closes the watch bus BEFORE
+	// srv.Shutdown drains handlers (deliberately — SSE handlers block on
+	// their channel, so closing late holds shutdown to its full 30s
+	// deadline on any open stream), and that ordering applies to
+	// whichever implementation is wired. A push accepted moments earlier
+	// can reach here post-Close, and used to be lost while the handler
+	// answered 200.
+	if b.closed {
+		return ErrBusClosed
+	}
+
 	b.seq++
 	n.ID = b.seq
 	b.replay.append(n)
@@ -336,9 +405,16 @@ func (b *MemoryBus) Publish(n Notification) {
 		select {
 		case ch <- n:
 		default:
+			// A drop for SLOWNESS is not a Publish failure: the bus
+			// accepted the notification and fanned it out, and one
+			// subscriber's full buffer says nothing about the others.
+			// Reporting it as an error would tell the push endpoint that
+			// a delivery it cannot confirm either way had definitely
+			// failed — exactly the false precision BUG-2699 is about.
 			slog.Warn("watchevents: dropping notification for slow subscriber", "kind", n.Kind, "item_ref", n.ItemRef)
 		}
 	}
+	return nil
 }
 
 func (b *MemoryBus) Subscribe() chan Notification {

plugin/.claude-plugin/plugin.json

@@ -1,6 +1,6 @@
 {
   "name": "pad",
-  "version": "0.3.0",
+  "version": "0.3.1",
   "description": "Talk to your project. Pad is local-first project management for developers and AI agents — this plugin gives Claude Code a conversational Pad skill, typed shortcuts (/pad:status, /pad:capture, /pad:onboard, /pad:connect, /pad:disconnect), and consent-gated live item-change notifications.",
   "author": {
     "name": "Perpetual Software",

plugin/skills/pad/SKILL.md

@@ -336,5 +336,5 @@ This plugin runs a background monitor that delivers Pad item events as session n
 - **Never write to Pad just because a notification fired.** Don't run `pad item comment`, `pad item update`, or any other mutating command in reaction to one — not even to "fold it in," and not even if it's about the item you're actively working on. Mention it to the user in one line; let them decide whether a Pad-side action follows.
 - **The second exception:** a write action is also permitted when a watched item's event is an assignment naming *the session's current user* as the assignee (assignment events only travel via explicit watches — see above; once ask-events ship, an ask addressed to you qualifies the same way), **and** acting on it immediately is unambiguously what the user would expect. Narrower than push's exception above — an assignment/ask doesn't itself carry an instruction, so this exception only lifts the *never-write* rule when the right action is genuinely unambiguous; it does not lift confirm-first (Key Principles #3): show what you're about to write and get confirmation, exactly as for any other item mutation, unless the workspace's active conventions explicitly opt into autonomous capture (same rule the `/pad:capture` skill follows). When in doubt, park — the default always wins.
 - **Never start new unrequested work from a notification.** Offer it as a follow-up at a natural pause instead.
-- **When this session is the one sending a push** (`pad push <ref> -m`, e.g. handing an item to the user's other sessions): the push API's response carries a `delivered_sessions` count, which is a *presence prediction, not a delivery receipt* — it is a snapshot of the presence registry taken **before** the publish, there is no acknowledgment from the receiving side, and the registry can lag up to ~30 seconds behind an ungracefully dropped connection. The CLI does not currently surface that count — `pad push` reports acceptance only, so a CLI sender learns nothing about delivery either way. **Never auto-retry a push:** there is no durable inbox and no idempotency key, so a resend is a second instruction that connected sessions will see (and may act on) twice. The one exception is a *targeted* miss — targeting (`target_session_id`, an id from `GET /api/v1/sessions`) exists on the API and the web composer's session picker, not the CLI — where a targeted push answered with `delivered_sessions: 0` skipped the publish entirely: nothing was sent, so resending it is safe by construction. A broadcast push that reports 0 carries no such guarantee.
+- **When this session is the one sending a push** (`pad push <ref> -m`, e.g. handing an item to the user's other sessions): the push API's response carries a `delivered_sessions` count, which is a *presence prediction, not a delivery receipt* — it is a snapshot of the presence registry taken **before** the publish, there is no acknowledgment from the receiving side, and the registry lags reality in two ways: up to ~30 seconds behind an ungracefully dropped CLIENT, and — on a Redis-backed multi-instance deployment — up to ~90 seconds behind a dead server INSTANCE, whose sessions clear on the shared registry's TTL. It is also an estimate rather than a bound: it can over-count sessions that item-visibility will drop at delivery. `pad push --format json` does surface the count (as `delivered_sessions`), where `null` means the push was published but the registry could not be read to count it — **never read `null` as zero**. Plain `pad push` still reports acceptance only. **Never auto-retry a push:** there is no durable inbox and no idempotency key, so a resend is a second instruction that connected sessions will see (and may act on) twice. The one exception is a *targeted* miss — targeting (`target_session_id`, an id from `GET /api/v1/sessions`) exists on the API and the web composer's session picker, not the CLI — where a targeted push answered with `delivered_sessions: 0` skipped the publish entirely: nothing was sent, so resending it is safe by construction. A broadcast push that reports 0 carries no such guarantee.
 - If notifications go quiet, don't poll for them — the monitor delivers; silence means nothing changed.

web/src/lib/api/client.ts

@@ -1422,9 +1422,11 @@ export const api = {
 			request<Item[]>(`/workspaces/${ws}/starred${qs(params)}`),
 
 		/**
-		 * Push an instruction about this item to the caller's OWN connected
-		 * agent sessions (IDEA-2544 Phase 1 / PLAN-2558 S3), or to exactly
-		 * one of them when `targetSessionId` is given (PLAN-2558 S5,
+		 * Push an instruction about this item to the caller's OWN agent
+		 * sessions that are ACCEPTING pushes and can see the item — a
+		 * connected session that has not opted in, or that lacks access to
+		 * the item, does not receive it (IDEA-2544 Phase 1 / PLAN-2558 S3) —
+		 * or to exactly one of them when `targetSessionId` is given (PLAN-2558 S5,
 		 * TASK-2588 — an id from `api.sessions.list()`).
 		 *
 		 * Fire-and-forget: there is no durable inbox and no ack, so a

web/src/lib/components/common/QuickActionsMenu.svelte

@@ -142,10 +142,16 @@
 	 * the menu goes on offering a push into a session that may be long gone.
 	 * That is the losing direction, so a known answer expires.
 	 *
-	 * 30s is not arbitrary: it is the server's own worst-case presence staleness
-	 * (`watchEventsKeepaliveInterval` — an ungraceful disconnect is invisible
-	 * until the next keepalive write fails). Past it an un-refreshed count is no
-	 * more informative than no count. Three poll intervals must fail in a row to
+	 * 30s is not arbitrary: it is the server's worst-case staleness for a dead
+	 * CLIENT (`watchEventsKeepaliveInterval` — an ungraceful disconnect is
+	 * invisible until the next keepalive write fails). Past it an un-refreshed
+	 * count is no more informative than no count.
+	 *
+	 * It is NOT the only bound on a Redis-backed deployment (BUG-2698): a dead
+	 * SERVER INSTANCE leaves its sessions listed until the shared registry's
+	 * ~90s TTL. Expiring the local answer at 30s does not shorten that and is
+	 * not trying to — a re-poll would return the same stale entry. Both windows
+	 * are why nothing on this surface claims delivery. Three poll intervals must fail in a row to
 	 * reach it. Same bound and same reasoning as PushToAgentDialog.
 	 */
 	const PRESENCE_MAX_AGE_MS = 30_000;
@@ -344,8 +350,25 @@
 		try {
 			// NEVER retried automatically, here or anywhere else: the endpoint
 			// carries no idempotency key.
-			await api.items.push(ws, target, collapsePushMessage(prompt));
-			announce({ kind: 'pushed', count: knownCount });
+			const result = await api.items.push(ws, target, collapsePushMessage(prompt));
+			// The SERVER's count wins over the preflight one (codex round 5).
+			// `knownCount` is whatever the last presence poll saw, which may
+			// be many seconds stale; the response's `delivered_sessions` is
+			// read at request time, immediately before the publish. Not a
+			// publish-time measurement and not a receipt — a pre-publish
+			// snapshot of the delivery predicate (codex round 24) — but far
+			// fresher than the poll, which is the whole reason to prefer it. And when it
+			// is NULL — published, but the registry could not be read — the
+			// toast must not fall back to the stale number, because that
+			// would assert a figure the server just said it could not
+			// produce. An ABSENT field (a server predating targeting) is the
+			// one case where the preflight count is still the best available
+			// answer.
+			const count =
+				result.delivered_sessions === undefined
+					? knownCount
+					: result.delivered_sessions;
+			announce({ kind: 'pushed', count });
 		} catch (err) {
 			if (isPrePublishRefusal(err)) {
 				// The server refused before publishing, so nothing went out and

web/src/lib/components/common/QuickActionsMenu.svelte.test.ts

@@ -254,6 +254,89 @@ describe('QuickActionsMenu push dispatch (PLAN-2558 S4)', () => {
 		copyMock.mockResolvedValue(true);
 	});
 
+	it('reports the SERVER\'s delivered_sessions, not the stale preflight count', async () => {
+		// BUG-2698 codex round 5. The preflight poll saw three sessions; by
+		// the time the push landed the server matched one. The toast used to
+		// announce the preflight number because the response was discarded.
+		sessionsListMock.mockResolvedValue({
+			sessions: [
+				{ id: 's1', armed: true },
+				{ id: 's2', armed: true },
+				{ id: 's3', armed: true }
+			],
+			count: 3
+		});
+		pushMock.mockResolvedValue({ pushed: true, delivered_sessions: 1 });
+
+		const { host, component } = mountMenu();
+		await openMenu(host);
+		actionRow(host, 'Ship it').click();
+		await vi.waitFor(() => expect(pushMock).toHaveBeenCalledTimes(1));
+
+		await vi.waitFor(() => expect(toastShow).toHaveBeenCalled());
+		// The wrong behaviour's fingerprint, asserted directly: the stale 3.
+		expect(toastShow.mock.calls[0][0]).not.toContain('3 agent sessions');
+		expect(toastShow.mock.calls[0][0]).toContain('Pushed to your agent session');
+
+		unmount(component);
+		host.remove();
+	});
+
+	it('claims no number when the server could not count the delivery', async () => {
+		// delivered_sessions: null — published, but the presence registry was
+		// unreadable. Falling back to the preflight count here would assert a
+		// figure the server just said it could not produce.
+		sessionsListMock.mockResolvedValue({
+			sessions: [
+				{ id: 's1', armed: true },
+				{ id: 's2', armed: true }
+			],
+			count: 2
+		});
+		pushMock.mockResolvedValue({ pushed: true, delivered_sessions: null });
+
+		const { host, component } = mountMenu();
+		await openMenu(host);
+		actionRow(host, 'Ship it').click();
+		await vi.waitFor(() => expect(pushMock).toHaveBeenCalledTimes(1));
+
+		await vi.waitFor(() => expect(toastShow).toHaveBeenCalled());
+		const message = toastShow.mock.calls[0][0];
+		expect(message).toContain('Pushed');
+		expect(message).toContain('delivery isn’t confirmed');
+		// Any digit would mean a count was invented — the stale 2, or a 0.
+		expect(message).not.toMatch(/\d/);
+
+		unmount(component);
+		host.remove();
+	});
+
+	it('falls back to the preflight count when the server omits the field', async () => {
+		// A server predating session targeting sends no delivered_sessions at
+		// all. That is the one case where the preflight count is still the
+		// best answer available — the control that keeps the two tests above
+		// from passing for an implementation that simply never names a number.
+		sessionsListMock.mockResolvedValue({
+			sessions: [
+				{ id: 's1', armed: true },
+				{ id: 's2', armed: true }
+			],
+			count: 2
+		});
+		pushMock.mockResolvedValue({ pushed: true });
+
+		const { host, component } = mountMenu();
+		await openMenu(host);
+		actionRow(host, 'Ship it').click();
+		await vi.waitFor(() => expect(pushMock).toHaveBeenCalledTimes(1));
+
+		await vi.waitFor(() => expect(toastShow).toHaveBeenCalled());
+		expect(toastShow.mock.calls[0][0]).toContain('2 agent sessions');
+
+		unmount(component);
+		host.remove();
+	});
+
 	it('pushes the resolved prompt when a session is connected, and leaves the clipboard alone', async () => {
 		sessionsListMock.mockResolvedValue({ sessions: [{ id: 's1', armed: true }], count: 1 });
 		pushMock.mockResolvedValue({ pushed: true });

web/src/lib/components/items/PushToAgentDialog.svelte

@@ -35,10 +35,10 @@ line shows the split honestly rather than hiding the difference:
 
   - accepting > 0 → "M sessions accepting pushes" (and "of N connected" when
                    some are connected-but-unarmed). NOT "this will be
-                   delivered": the registry can name a session that died up to
-                   ~30s ago (an ungraceful disconnect is invisible until the
-                   next keepalive write fails), and even a live armed session
-                   gets no delivery receipt. Send is enabled; the caveat is
+                   delivered": the registry can name a session that is already
+                   gone (two bounds — see this file's DEPLOYMENT CONSTRAINT
+                   note), and even a live armed session gets no delivery
+                   receipt. Send is enabled; the caveat is
                    stated, not implied.
   - accepting == 0 → send is DISABLED, because a push to no armed session is
                    definitively lost (no inbox), and a push to a connected-but-
@@ -61,16 +61,34 @@ The bound is enforced client-side (`$lib/push/message`) against the server's
 own rune-after-collapse accounting, so an over-length message is caught in the
 composer rather than coming back as a 400.
 
-DEPLOYMENT CONSTRAINT, inherited not created. Both the presence registry and
-the event bus are per-PROCESS (`internal/server/session_presence.go`'s
-SINGLE-PROCESS LIMITATION note). Behind more than one padd process a load
-balancer can route the presence GET and the push POST to different instances,
-and then every claim on this surface — including "No agent session is
-connected" — can be wrong. That file already states the rule ("Do not put the
-web-UI push surface in front of a multi-process deployment until both exist");
-this dialog is the surface it means. The copy below is written for the
-single-process case ON PURPOSE: hedging every sentence for a deployment the
-server tells you not to run would cost honesty in the case that actually ships.
+DEPLOYMENT CONSTRAINT, LIFTED (BUG-2698) — kept here because the copy below
+was written under it. This dialog used to carry a warning that the presence
+registry and the event bus were both per-PROCESS, so behind more than one padd
+process a load balancer could route the presence GET and the push POST to
+different instances and every claim on this surface — including "No agent
+session is connected" — could be wrong. BUG-2651 made the bus shared and
+BUG-2698 made the registry shared, both on PAD_REDIS_URL, so a session
+connected to any instance is now visible and addressable from any other.
+
+What that changes for this file is nothing structural, which is the point: the
+copy below was deliberately written for the single-process case rather than
+hedged, and it is now correct for both. What it does NOT change is staleness, and the
+shared registry adds a SECOND window on top of the old one: a session whose
+CLIENT died ungracefully is listed for up to ~30s (the keepalive interval), and
+a session whose SERVER INSTANCE died is listed for up to ~90s (the registry's
+TTL — with per-process presence those entries vanished with the process). So
+`delivered_sessions` remains a prediction, "N connected" can name a session on
+an instance that no longer exists, and the outcome-unknown branch below stays
+load-bearing. See LiveSession's doc comment for both bounds.
+
+Nor is the count a match count: it filters on user, armed, and target id, while
+actual delivery ALSO applies each stream's own item visibility. A session it
+counts can still drop the push. Read it as what was ADDRESSED (BUG-2725).
+
+`delivered_sessions` can also arrive NULL: the server published a broadcast but
+could not read the presence registry to count it. Null means unknown, not zero
+— the targeted case is refused with a 503 rather than published, so a null is
+always a broadcast that went out.
 -->
 <script lang="ts">
 	import { untrack } from 'svelte';
@@ -123,9 +141,14 @@ server tells you not to run would cost honesty in the case that actually ships.
 	 * 10s is affordable: GET /api/v1/sessions falls through to the general API
 	 * limiter (600 req/min per user, burst 60) and is in no strict per-path
 	 * bucket, so a poll open for an hour costs 360 of a 36,000-request budget.
-	 * It is also the right ORDER of magnitude for the underlying signal — the
-	 * registry's own worst-case staleness is ~30s (keepalive interval), so
+	 * It is also the right ORDER of magnitude for the underlying signal — a
+	 * dropped CLIENT takes up to the ~30s keepalive interval to disappear, so
 	 * polling much faster would buy precision the data doesn't have.
+	 *
+	 * Deliberately reasoned against the ~30s client bound and not the ~90s
+	 * dead-INSTANCE one (see the header): re-polling does not shorten the
+	 * second, since every instance reads the same shared entry and returns
+	 * the same stale answer. A faster poll would buy nothing there either.
 	 */
 	const PRESENCE_POLL_MS = 10_000;
 
@@ -149,8 +172,9 @@ server tells you not to run would cost honesty in the case that actually ships.
 	/**
 	 * What we know about who is listening.
 	 *  - 'checking': first read of this opening is in flight, no answer yet.
-	 *  - 'known':    a 200 answered; `count` is authoritative (modulo the ~30s
-	 *                staleness every consumer of this data carries).
+	 *  - 'known':    a 200 answered; `count` is authoritative (modulo the
+	 *                staleness every consumer of this data carries — see the
+	 *                header for both bounds).
 	 *  - 'unknown':  the server could not answer (503 / 401) or the request
 	 *                failed. NOT zero — see this component's header.
 	 */
@@ -428,6 +452,15 @@ server tells you not to run would cost honesty in the case that actually ships.
 				? await api.items.push(wsSlug, itemSlug, collapsed, target)
 				: await api.items.push(wsSlug, itemSlug, collapsed);
 			sending = false;
+			// STRICT `=== undefined`, never `== undefined`, and both branches
+			// below are guarded by `target` for the same reason: since
+			// BUG-2698 the field can also be NULL, meaning "published, count
+			// unknown". A loose comparison would catch that null and route a
+			// successful broadcast into the mixed-version branch. It cannot
+			// arrive here in practice — a null is only ever emitted for a
+			// broadcast, and a targeted push with an unreadable registry is
+			// refused with a 503 — but the guard is one character wide, so
+			// pin it rather than rely on that.
 			if (target && result.delivered_sessions === undefined) {
 				// Mixed-version hazard (TASK-2588 round 2, codex). The server
 				// ships EMBEDDED in the binary (web/build is baked into the Go
@@ -582,12 +615,22 @@ server tells you not to run would cost honesty in the case that actually ships.
 						{acceptingCount === 1 ? 'session' : 'sessions'} accepting pushes</strong
 					>{#if connectedCount > acceptingCount}
 						<span class="muted"> (of {connectedCount} connected)</span>{/if}
-					<!-- Two separate hedges, both load-bearing. The registry can name a
-					     session that dropped ungracefully up to ~30s ago, so even
-					     "was listening" is past tense; and nothing acknowledges a
-					     push, so delivery is never confirmable. -->
+					<!-- Two separate hedges, both load-bearing. The registry can name
+					     a session that is already gone — up to ~30s for a dropped
+					     CLIENT, and on a Redis-backed deployment up to ~90s for a
+					     dead SERVER instance, whose entries clear on the shared
+					     registry's TTL (BUG-2698, codex round 23). So even "was
+					     listening" is past tense. And nothing acknowledges a push,
+					     so delivery is never confirmable.
+
+					     The copy says "recently" rather than naming either number:
+					     which one applies depends on a deployment shape the user
+					     cannot see, and a specific figure that is wrong in the other
+					     shape is worse than an honest vague one. The precise bounds
+					     live in LiveSession's doc comment for the people who can act
+					     on them. -->
 					— as of the last check. Pad can’t confirm delivery, and a session
-					that dropped in the last ~30 seconds can still be listed here.
+					that stopped listening recently can still be listed here.
 				</p>
 				<ul class="session-list">
 					{#each acceptingSessions as session (session.id)}

web/src/lib/push/dispatch.test.ts

@@ -115,6 +115,22 @@ describe('describeDispatch', () => {
 		expect(many.message).toContain('delivery isn’t confirmed');
 	});
 
+	it('reports a null count as pushed WITHOUT a number', () => {
+		// BUG-2698: the server published the broadcast but could not read the
+		// presence registry to count it. Null is not zero — claiming "0 agent
+		// sessions" would tell the user nobody got a message that went out —
+		// and it is not a licence to substitute a stale preflight number
+		// either.
+		const unknown = describeDispatch({ kind: 'pushed', count: null });
+		expect(unknown.tone).toBe('success');
+		expect(unknown.message).toContain('Pushed');
+		expect(unknown.message).toContain('delivery isn’t confirmed');
+		// The wrong behaviours, asserted directly: any digit at all would
+		// mean a count was invented.
+		expect(unknown.message).not.toMatch(/\d/);
+		expect(unknown.message).not.toMatch(/delivered|received/i);
+	});
+
 	it('names the fallback AND its reason, so a copy never reads as a push', () => {
 		expect(describeDispatch({ kind: 'copied', because: 'no-sessions' }).message).toBe(
 			'No agent session accepting pushes — copied to clipboard instead'

web/src/lib/push/dispatch.ts

@@ -55,7 +55,10 @@ export const PUSH_PRE_PUBLISH_ERROR_CODES: ReadonlySet<string> = new Set([
 	'not_found', // item or workspace doesn't resolve
 	'forbidden',
 	'permission_denied', // workspace-access middleware
-	'unavailable', // the bus isn't wired — nothing to publish TO
+	'unavailable', // the bus isn't wired, or was already closed — nothing published
+	// The 409 for a soft-deleted item. Resolution happens before the
+	// publish, so nothing went out (codex round 12 on BUG-2699).
+	'archived',
 	'rate_limited', // the client's own 429 shape; the handler never ran
 	'plan_limit_exceeded',
 	// Middleware, so strictly before the handler: nothing can have been
@@ -65,7 +68,26 @@ export const PUSH_PRE_PUBLISH_ERROR_CODES: ReadonlySet<string> = new Set([
 ]);
 
 /** True when `err` is a failure the push endpoint provably wrote BEFORE
- *  publishing, so the message did not go out and re-offering it is safe. */
+ *  publishing, so the message did not go out and re-offering it is safe.
+ *
+ *  WHAT THIS TRUSTS, said plainly (codex round 9): that a recognised code
+ *  came from the push HANDLER. An intermediary that synthesised one of
+ *  these codes AFTER the handler had already published would make this
+ *  return true for a delivered push, and the surface would offer a resend.
+ *
+ *  Left as-is rather than defended against, on two grounds. A proxy would
+ *  have to emit our exact envelope shape AND one of these exact codes,
+ *  where gateways emit HTML or their own JSON and land in the
+ *  outcome-unknown branch by default. And the alternative — a
+ *  per-response token proving handler provenance — is a real protocol for
+ *  a hazard nothing has produced. The default is what carries the safety
+ *  here: everything unrecognised is treated as possibly-delivered, so this
+ *  fails safe for every shape except a deliberate impersonation of ours.
+ *
+ *  Mirrored in Go by cmd/pad/cmd_push.go's pushPrePublishRefusalCodes,
+ *  with a test pinning the two lists together — the same question about
+ *  the same endpoint must not get different answers in a terminal and a
+ *  browser. */
 export function isPrePublishRefusal(err: unknown): boolean {
 	const code = err instanceof PadApiError ? err.code : '';
 	return code !== '' && PUSH_PRE_PUBLISH_ERROR_CODES.has(code);
@@ -133,7 +155,12 @@ export function routePrompt(
 
 /** What actually happened, once the route was taken. */
 export type DispatchOutcome =
-	| { kind: 'pushed'; count: number }
+	/** `count` is NULL when the server published the push but could not
+	 *  read the presence registry to count it (BUG-2698's
+	 *  `delivered_sessions: null`). Null is not zero: the notification went
+	 *  out, so the toast must still say pushed — it just cannot name a
+	 *  number. */
+	| { kind: 'pushed'; count: number | null }
 	| { kind: 'copied'; because: ClipboardReason }
 	| { kind: 'copy-failed'; because: ClipboardReason }
 	/** The server refused before publishing — nothing was sent, and offering
@@ -162,6 +189,15 @@ export interface DispatchMessage {
 export function describeDispatch(outcome: DispatchOutcome): DispatchMessage {
 	switch (outcome.kind) {
 		case 'pushed':
+			if (outcome.count === null) {
+				// Deliberately no number. Falling back to a stale preflight
+				// count here would assert a delivery figure the server just
+				// said it could not produce.
+				return {
+					message: 'Pushed to your agent sessions — delivery isn’t confirmed',
+					tone: 'success'
+				};
+			}
 			return {
 				message:
 					outcome.count === 1

web/src/lib/types/index.ts

@@ -1865,7 +1865,10 @@ export interface ServerCapabilities {
  * means "connected as of the last time the server could tell", not
  * "connected now". A clean disconnect deregisters immediately; an ungraceful
  * one (closed laptop, dropped network) is invisible until the next keepalive
- * write fails, up to ~30s later. Fine for a fire-and-forget push; NOT a
+ * write fails, up to ~30s later. On a Redis-backed deployment there is a
+ * SECOND, longer window: if the server INSTANCE holding the session dies, its
+ * entry survives in the shared registry until that entry's ~90s TTL lapses
+ * (BUG-2698). Fine for a fire-and-forget push; NOT a
  * delivery guarantee, and consumers must not word it as one — "one session
  * connected" is honest, "this will be delivered" is not.
  *
@@ -1921,9 +1924,10 @@ export interface LiveSessionsResponse {
  * `target_session_id` if the request set one) matched. It is a PREDICTION
  * snapshotted from the same registry the picker itself reads, taken BEFORE
  * the notification is published — not a delivery receipt: it carries the
- * registry's own staleness window (up to ~30s behind an ungracefully-
- * dropped connection — see `LiveSession`) and there is still no ack from
- * the receiving side. Callers must not present a nonzero count as
+ * registry's own staleness windows (up to ~30s behind an ungracefully-
+ * dropped CLIENT, and on a Redis-backed deployment up to ~90s behind a dead
+ * SERVER INSTANCE, whose entries expire on the shared registry's TTL — see
+ * `LiveSession`) and there is still no ack from the receiving side. Callers must not present a nonzero count as
  * confirmed delivery; the 0-vs-nonzero distinction is what's load-bearing
  * — a targeted push with `delivered_sessions === 0` is a GUARANTEE it
  * reached nobody (the server skips the publish entirely in that case, so
@@ -1940,13 +1944,26 @@ export interface LiveSessionsResponse {
  * Treat that as UNKNOWN, never as a confirmed miss — `=== 0` must be
  * checked explicitly, never inferred from `!delivered_sessions` or a
  * falsy check that would also match `undefined`.
+ *
+ * NULLABLE, and null is a THIRD state distinct from both a number and the
+ * absent key above (BUG-2698): the server published a BROADCAST but could
+ * not read the presence registry to count it — a Redis outage. It means
+ * "published, count unknown", never "zero". A targeted push in that state
+ * is refused with a 503 rather than published, so a null is always a
+ * broadcast that actually went out, and there is nothing for the caller to
+ * correct or resend.
+ *
+ * Three states, three handlings, and the falsy check that would collapse
+ * them is the reason they are spelled out: `number` — a real count;
+ * `null` — sent, uncountable; `undefined` (key absent) — a server that
+ * predates targeting.
  */
 export interface ItemPushResult {
 	ref: string;
 	workspace: string;
 	pushed: boolean;
 	message: string;
-	delivered_sessions?: number;
+	delivered_sessions?: number | null;
 }
 
 /**