From e4167a916065da06feb496038900d50c76798b35 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 23 Aug 2026 13:56:42 +0000 Subject: [PATCH] docs(deployment): five corrections from reading the doc as a document (BUG-2739, codex round 20) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - THE NAMESPACE GUIDANCE WAS WRONG, and this is the substantive one. It said a moving undecodable_message suggests two Pad installations sharing a Redis. It does not: two CURRENT installations publish the same wire format, so their messages decode fine and the damage is cross-feeding real notifications between installations while this counter stays flat — a worse and quieter failure, and the one PAD_REDIS_NAMESPACE actually prevents. The counter indicates genuinely unreadable input: a non-Pad publisher, a mixed-version wire format mid-upgrade, or corruption. The wording was inherited from internal/events without checking that it transferred. - THE FLOOD COSTS WERE OVERSTATED AS SELF-BOUNDING. Heap growth and the announcement are bounded; per-message CPU and allocation are not — a fresh replay buffer plus a pass over every subscriber, on the single goroutine that also delivers real notifications, so a sustained flood is receive-loop starvation as much as it is garbage collection. - THE CUTOVER SECTION described every reconnecting client running a /changes delta. True of the web activity client; pad watch --stream clears its cursor and keeps the connection open, refetching nothing. The doc contradicted its own watch-stream paragraph fifty lines later, which this branch added. - 'A reconnecting client is covered in both cases' was too absolute: the shared-counter check reads at one instant and cannot see a notification published after the read, which resumeOutrunsLocalView and cmd_watch.go both already document as an at-most-once residual. - The rollout note said a reason-specific alert on either surviving reason is unaffected. False for counter_backward, whose spelling changed — which is the entire reason that paragraph exists. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X --- docs/deployment.md | 69 +++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 26 deletions(-) diff --git a/docs/deployment.md b/docs/deployment.md index 27f0f5b1..8630f0b1 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -172,8 +172,11 @@ cross-tenant leak. > its cold replay-buffer coverage check rather > than an epoch comparison — a freshly namespaced bus has no old epoch to > compare against. Expect a burst of client reconciliation as they reconnect -> — an incremental `/changes` delta each, not a full page load; -> that is the cutover being paid for, and it is bounded by the number of +> — for ACTIVITY-stream clients (the web UI) an incremental `/changes` delta +> each, not a full page load. WATCH-stream clients cost less: `pad watch +> --stream` answers `sync_required` by clearing its cursor and keeping the +> connection open, so it refetches nothing. Either way that is the cutover +> being paid for, and it is bounded by the number of > reconnecting clients — each RESUME is counted, so a client that > reconnects several times counts several times. > @@ -271,23 +274,31 @@ reading the metrics below, and for anyone writing a third-party consumer: ever becomes a capacity problem the answer is fewer connections per instance, not a quieter bus. - **Who can force a resync, and what a flood of them costs.** The - undecodable-message detection is reachable by anyone who can `PUBLISH` onto - the watch channel, which sounds worse than it is: the same access allows - publishing FORGED notifications, so a channel writer is outside the threat - model already. The realistic cause is two Pad installations sharing a Redis - without `PAD_REDIS_NAMESPACE` set — which is why the metric's reason points - at a namespace collision. Under a flood, three of the four per-message costs - are self-bounding: the announcement is a non-blocking send onto a - capacity-1 flag that is already raised, so it collapses to nothing; the - metric increment IS the alarm you want, at full rate; and the receive loop - is serial, so buffer allocations are one at a time against a GC rather than - a growing heap. **Log volume is the residual** — one ERROR line per message - — and bounding it needs a rate threshold, which is a deployment decision - this code declines to make on your behalf. Payload size is deliberately not - capped in Pad, because go-redis has read the whole message into memory - before Pad sees it; bound it with Redis's `proto-max-bulk-len` and with who - holds `PUBLISH`. + **What `undecodable_message` actually indicates.** Genuinely unreadable + input on the watch channel: a non-Pad publisher on the key, a wire format + from a mixed-version fleet mid-upgrade, or corruption. **It does NOT + usually mean two current Pad installations sharing a Redis** — those publish + the same wire format, so their messages DECODE, and the damage is + cross-feeding real notifications between installations while this counter + stays flat. That is the failure `PAD_REDIS_NAMESPACE` exists to prevent, and + it is both worse and quieter than the one this counter reports. + + **Who can force a resync with it, and what a flood costs.** Anyone who can + `PUBLISH` onto the watch channel — which sounds worse than it is, since the + same access allows publishing FORGED notifications, so a channel writer is + outside the threat model already. Under a flood, what IS bounded: the + announcement, a non-blocking send onto a capacity-1 flag that is already + raised, so it collapses to nothing after the first; and heap GROWTH, since + each discarded replay buffer is garbage immediately and the receive loop is + serial. What is NOT bounded: per-message CPU and allocation — a fresh replay + buffer plus a pass over every subscriber, per malformed message, on the + single goroutine that also delivers real notifications, so a sustained flood + is receive-loop starvation as much as it is garbage collection. And log + volume, one ERROR line per message. Bounding either needs a rate threshold, + which is a deployment decision this code declines to make on your behalf. + Payload size is deliberately not capped in Pad, because go-redis has read the + whole message into memory before Pad sees it; bound it with Redis's + `proto-max-bulk-len` and with who holds `PUBLISH`. **Two gaps in that detection remain, and an operator should know both.** A message lost in transit with the connection intact — no flap, no decode @@ -303,9 +314,13 @@ reading the metrics below, and for anyone writing a third-party consumer: threshold, which is a deployment decision rather than an implementation detail. - A RECONNECTING client is covered in both cases on the watch stream anyway, - because a resume consults the shared counter rather than local state alone. - It is the client holding a stream open that these two can leave stale. + A RECONNECTING client is largely covered on the watch stream anyway, because + a resume consults the shared counter rather than local state alone. Not + entirely: that check reads the counter at one instant, so a notification + published AFTER the read and missed is invisible to it — an at-most-once + pub/sub residual with no per-connection ack, documented on + `resumeOutrunsLocalView` and again in the CLI. What these two gaps reliably + leave stale is the client holding a stream OPEN. The second case is newer — before it, a held-open stream that missed events was never told, and a later delivered event advanced its cursor past the missing @@ -380,7 +395,7 @@ Alert on these instead: | `pad_watchevents_midstream_resyncs_total` | Watch-stream subscribers told MID-STREAM that they missed notifications, on a connection that stayed open. New in BUG-2730 | | `pad_watchevents_notifications_missed_total` | How many notifications those gaps spanned | | `pad_watchevents_notifications_dropped_total` | Received but not delivered to a local subscriber — that connection's buffer was full. Since BUG-2730 that subscriber is told (`sync_required`, mid-stream) rather than silently under-served, so a rise here produces a rise in `pad_watchevents_midstream_resyncs_total`, one client at a time | -| `pad_watchevents_sequence_resets_total` | Watch replay coverage dropped, by reason. `epoch_change` — the watch epoch token changed, so the IDs now come from a different sequence; the token is an opaque UUID here, not a numeric generation. `counter_backward` — an ID arrived at or below the high-water mark with the epoch unchanged. (This label was spelled `counter_backwards` while BUG-2739 was in development. If you are reading a dashboard that uses the plural, it was built against an unreleased build — see the note below.) `subscription_resumed` — a pub/sub connection dropped and re-subscribed, so whatever was published during the outage never arrived; expect these during a Redis failover and expect them to stop afterwards. `undecodable_message` — a message on the watch channel could not be parsed. The instance cannot tell whether that was a notification it should have had or something foreign, and it stops vouching because it cannot tell; expect zero, and suspect a namespace collision. The first two mean the ID space changed under this instance. `subscription_resumed` means it did not and something demonstrably went missing. `undecodable_message` means neither is established — only that coverage can no longer be proved. Each also announces to the watch subscribers connected at that moment, so each moves `pad_watchevents_midstream_resyncs_total` by AT MOST one per such subscriber — at most, because the signal is capacity-1 and coalescing, so a second cause firing before a client has acted on the first adds no announcement. For the same reason the announcement counter is not a ratio against this one in aggregate: it also counts gaps and slow-subscriber drops, and only a reset observed in isolation, against idle clients, lets you read the fan-out off the two | +| `pad_watchevents_sequence_resets_total` | Watch replay coverage dropped, by reason. `epoch_change` — the watch epoch token changed, so the IDs now come from a different sequence; the token is an opaque UUID here, not a numeric generation. `counter_backward` — an ID arrived at or below the high-water mark with the epoch unchanged. (This label was spelled `counter_backwards` while BUG-2739 was in development. If you are reading a dashboard that uses the plural, it was built against an unreleased build — see the note below.) `subscription_resumed` — a pub/sub connection dropped and re-subscribed, so whatever was published during the outage never arrived; expect these during a Redis failover and expect them to stop afterwards. `undecodable_message` — a message on the watch channel could not be parsed. The instance cannot tell whether that was a notification it should have had or something foreign, and it stops vouching because it cannot tell; expect zero, and suspect a namespace collision. The first two mean the ID space changed under this instance. `subscription_resumed` means it did not and something demonstrably went missing. `undecodable_message` means neither is established — only that coverage can no longer be proved. Each also announces to the watch subscribers connected at that moment, so each moves `pad_watchevents_midstream_resyncs_total` by AT MOST one per such subscriber — at most, because the signal is capacity-1 and coalescing, so a second cause firing before a client has acted on the first adds no announcement. For the same reason the announcement counter is not a ratio against this one in aggregate: it also counts gaps and slow-subscriber drops, and only a reset observed in isolation, against idle clients, lets you read the fan-out off these two counters | | `pad_watchevents_receive_loop_exits_total` | Non-zero outside shutdown means an instance publishes but receives nothing | | `pad_event_resume_gaps_total` | The ACTIVITY stream's (`/api/v1/events`) twin of the watch resume counter above. **Expect a step around a deploy, with the RATE settling back to baseline** (the counter itself only ever increases) — each instance starts with no replay coverage, so an early resume against a workspace it has not seen yet is a warranted resync. It counts RESUMES, not clients: a deploy with no reconnects does not move it at all, and a client that reconnects several times is counted several times. A rate that does not settle is the thing to alert on | | `pad_event_midstream_resyncs_total` | Activity-stream subscribers told MID-STREAM that they missed events, on a connection that stayed open. New in BUG-2730, and the counter to watch when judging whether that fix is costing more resyncs than it is worth. It counts ANNOUNCEMENTS, not causes and not distinct clients: a reset that drops buffers moves it once per live subscriber (and that ratio against `pad_event_sequence_resets_total` is the fan-out); a burst of drops on ONE connection moves it once, because signals coalesce and are rate-limited per connection; and a coverage loss on a workspace with no buffer yet moves it while every cause counter stays flat, because there was no coverage to end but the subscribers still have a hole | @@ -397,9 +412,11 @@ development deployment can be alerting on it. Two things about it changed on that branch: the `counter_backward` label lost a trailing `s`, and the metric widened from "the ID space changed" to "replay coverage was dropped", which added the `subscription_resumed` and `undecodable_message` reasons. A -reason-specific alert on either surviving reason is unaffected; an alert on -the unlabelled total now counts more things, which is the metric doing what -its name says rather than a regression. During a rolling deploy an instance on +reason-specific alert on `epoch_change` is unaffected; one on +`counter_backward` must have its expression updated for the spelling, which is +the whole reason this paragraph exists. An alert on the unlabelled total now +counts more things, which is the metric doing what its name says rather than a +regression. During a rolling deploy an instance on the older build reports neither new reason and keeps the old spelling — so a mixed fleet reports two shapes under one name for the rollout's length, which is acceptable precisely because no released version is in that fleet.