From bc6d3541cb034d38e654dc4b2daabbd717d33ff0 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 23 Aug 2026 13:27:16 +0000 Subject: [PATCH] refactor(watchevents,docs): order the startup test, move operator prose out of the code (BUG-2739, codex round 16) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A future-maintainer round, three of whose four findings were fair. THE REAL DEFECT: TestNoCoverageIsDroppedAtStartup slept 500ms and hoped the receive goroutine had had its chance. No happens-before, so it could miss the regression or turn scheduler-sensitive. It now publishes and waits for DELIVERY instead: the pub/sub channel is FIFO, so a startup confirmation — if the constructor stopped consuming it — is queued AHEAD of that notification and has necessarily been processed by the time it comes out the other end. Deterministic, strictly stronger, and 0.00s instead of 0.50s. Re-verified against its mutation: removing the constructor's Receive still fails it 3/3. COMMENT ACCRETION: dropCoverage had 76 lines of commentary over 30 of code, including a threat model and a per-message cost breakdown that are operator decisions. Those moved to docs/deployment.md, where operators actually read, and the code keeps the invariants and the one design question a maintainer will ask (why not gate on the shared counter). 45 lines now, and nothing was deleted — only relocated to the artifact whose audience it was written for. THE TIME BOMB: the rollout note said 'this paragraph expires at the next tag' with nothing enforcing it. A claim about release state that goes stale silently is exactly what this branch has spent nine rounds removing, so it now carries the three commands to re-derive it instead of asking to be trusted. DECLINED: extracting fanOutLocally's switch into a coverage-state transition helper. The accretion is real and predates this branch — the switch, its four fields and their reset duplication are the existing design, to which this added one arm. A state-machine refactor of the receive path is its own change with its own review and its own mutation matrix; folding it into a bug fix at round 16 is how a fix's blast radius stops matching its claim. Worth filing if a third reviewer raises it. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X --- docs/deployment.md | 36 +++++++++++-- internal/watchevents/redis_bus.go | 57 +++++--------------- internal/watchevents/redis_reconnect_test.go | 27 +++++----- 3 files changed, 60 insertions(+), 60 deletions(-) diff --git a/docs/deployment.md b/docs/deployment.md index 47db17b5..a155000b 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -271,6 +271,24 @@ 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`. + **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 failure, just a message that never arrived (BUG-2735): on the watch stream a @@ -376,10 +394,20 @@ 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. **This -paragraph expires at the next tag**: once a release ships either spelling, the -next change to this metric is a real contract break and needs the versioned -treatment instead. +is acceptable precisely because no released version is in that fleet. + +**Re-derive that rather than trusting this paragraph**, because it is a claim +about release state and release state changes without anyone editing this file: + +``` +git describe --tags --abbrev=0 origin/main # the latest tag +git log --reverse --format=%H -S pad_watchevents_sequence_resets_total \ + -- internal/metrics/metrics.go | head -1 # the commit that introduced it +git merge-base --is-ancestor # non-zero exit => still unreleased +``` + +Once a release does ship this metric, the next change to it is a real contract +break and needs versioned treatment instead of a note here. | `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 | diff --git a/internal/watchevents/redis_bus.go b/internal/watchevents/redis_bus.go index 938c9857..7e85d57d 100644 --- a/internal/watchevents/redis_bus.go +++ b/internal/watchevents/redis_bus.go @@ -1184,9 +1184,12 @@ func (b *RedisBus) fanOutLocally(n Notification) { // makes the NEXT notification look contiguous — no arm of fanOutLocally's // switch fires, so knownFrom is never re-established, stays 0, and // replaySince refuses EVERY resume on this instance from then on. It reads -// correct and bricks resumes permanently. TestCoverageIsReestablishedByTheNextNotification -// is what holds this, and it is why the recovery case was written before the -// refusal case. +// correct and bricks resumes permanently. +// TestCoverageIsReestablishedByTheNextNotification is what holds this, and it +// is why the recovery case was written before the refusal case. +// +// highWaterID deliberately SURVIVES, because backward-counter detection needs +// a mark that a coverage drop does not erase — see its field comment. // // epochJustChanged is deliberately NOT set: these conditions are a hole in our // view of the SAME id space, so the cold-start arm's ordinary knownFrom = n.ID @@ -1203,47 +1206,13 @@ func (b *RedisBus) fanOutLocally(n Notification) { // subscriber on the instance. Ending coverage locally is the record that // survives Redis being unreadable, which is the whole point of keeping one. // -// THE COST OF THAT CHOICE, stated rather than glossed: when the outage lost -// NOTHING — no publish happened while we were away — this discards a buffer -// that was still complete, and a client reconnecting before the next -// notification is answered sync_required instead of being replayed. That is -// the package's standing trade (chatty-but-correct beats quiet-but-lossy, -// the lead's ruling recorded on resumeOutrunsLocalView), and it is bounded: -// the next notification re-establishes coverage, and a LIVE subscriber loses -// nothing either way because it stays connected. -// -// WHY DROP AT ALL, given the resume path independently catches a real loss -// (a counter ahead of our high-water mark) and the gap arm catches it on the -// next notification? Because both of those need something to happen — a -// reachable Redis, or a later publish. The live subscriber on a stream that -// then goes quiet has neither, and it is the client this whole unit exists -// for. -// -// THREAT MODEL for the undecodable arm, since it lets a WRITER to the channel -// force a resync at will: anyone able to publish onto this channel can -// already publish forged notifications, which is strictly worse than making -// us disclaim coverage. The realistic cause is not an attacker but two Pad -// installations sharing a Redis without PAD_REDIS_NAMESPACE set (BUG-2724), -// which is why the metric help points at a namespace collision. Rate-limiting -// the drop would need a threshold, and a threshold is a deployment decision -// rather than an implementation detail — the same reason BUG-2738 is not -// fixed here. -// -// WHAT A FLOOD OF SUCH MESSAGES COSTS, since "out of threat model" is not the -// same as "free" (codex round 10). Per message: one ERROR log, one metric -// increment, one replay-buffer allocation, and one pass over the subscriber -// map. Three of those four are self-bounding — the subscriber pass 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 an operator would -// want at full rate; and the receive loop is serial, so the allocations are -// one at a time against a GC rather than a growing heap. The unbounded one is -// LOG VOLUME, which is the residual: a flood writes one line per message, and -// bounding it is the same threshold decision as above. -// -// Payload size is deliberately not capped here, because a cap at this layer -// would not do anything: go-redis has already read the whole message into -// memory before decodePayload sees it. Bounding that belongs to the Redis -// deployment (proto-max-bulk-len) and to who holds PUBLISH. +// The cost of that choice, and the reason we drop at all when the resume path +// and the gap arm each catch a real loss independently, are stated in +// docs/deployment.md under "What a failover now COSTS" — they are operator +// decisions and belong where operators read. The short version: both of those +// other detections need something to HAPPEN (a reachable Redis, or a later +// publish), and the live subscriber on a stream that then goes quiet has +// neither. That client is what this function exists for. func (b *RedisBus) dropCoverage(reason string) { // Registered FIRST so it runs LAST, after the Unlock — reports fire with // no bus lock held. See Observer. diff --git a/internal/watchevents/redis_reconnect_test.go b/internal/watchevents/redis_reconnect_test.go index 7e0e0a75..77260721 100644 --- a/internal/watchevents/redis_reconnect_test.go +++ b/internal/watchevents/redis_reconnect_test.go @@ -421,10 +421,21 @@ func TestNoCoverageIsDroppedAtStartup(t *testing.T) { ch, gaps := b.Subscribe() defer b.Unsubscribe(ch) - // Long enough for a startup confirmation to have arrived on the channel - // if the constructor were no longer consuming it. The probe that - // established this behaviour saw zero within 1.5s. - time.Sleep(500 * time.Millisecond) + // ORDERED, NOT TIMED (codex round 16). The first version slept 500ms and + // hoped. This publishes and waits for DELIVERY instead, which gives a real + // happens-before: the pub/sub channel is FIFO, so a startup confirmation — + // if the constructor were no longer consuming it — is queued AHEAD of this + // notification and has necessarily been processed by the time the + // notification comes out the other end. Receiving it therefore proves the + // loop has already had its chance to mishandle the confirmation. + // + // It also makes the assertions' premise explicit: a subscription that + // delivers is a LIVE one, not a dead loop reporting nothing because it + // receives nothing. + if err := b.Publish(Notification{Kind: KindComment, ItemRef: "TASK-1"}); err != nil { + t.Fatalf("publish: %v", err) + } + drainOne(t, ch) if got := obs.snapshot(); len(got.resets) != 0 { t.Fatalf("a bus that has merely started must not drop coverage, got %v — "+ @@ -433,14 +444,6 @@ func TestNoCoverageIsDroppedAtStartup(t *testing.T) { if raised(gaps) { t.Fatal("a subscriber on a freshly started bus must not be told it missed anything") } - - // And it still works: the premise of the assertions above is a LIVE - // subscription, not a dead one that reports nothing because it receives - // nothing. - if err := b.Publish(Notification{Kind: KindComment, ItemRef: "TASK-1"}); err != nil { - t.Fatalf("publish: %v", err) - } - drainOne(t, ch) } // drainOne reads one notification or fails.