mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 11:03:41 +00:00
xarmian
d8b098fbed
* fix(watchevents): bound the resume settle window by the request (BUG-2751) GET /api/v1/events/stream takes a global AND a per-user admission slot before subscribing (BUG-2726) and releases them by defer when the handler returns. The resume path could WAIT inside that window: resumeOutrunsLocalView does a Redis GET, waits 250ms for propagation, then does a second GET -- and its select waited on b.ctx, the BUS's lifetime, never the request's. A resuming client that disconnected mid-window held both slots for the remainder of it plus two round trips. The connection was gone; the capacity was not. The context is threaded through SubscribeAndReplaySince -> resumeOutrunsLocalView -> sharedCounter and MERGED with the bus context rather than replacing it. Swapping to the caller's alone would trade one leak for another: b.ctx is what lets Close cut a wait short, so dropping it leaves shutdown blocked behind a client that is still perfectly connected. internal/events lost exactly that half in its own first draft. Both endings are reasons to stop, and each has its own test that fails against the other one's implementation. ENDING THE WAIT EARLY IS HALF A FIX (codex round 1). resumeOutrunsLocalView answers false on cancellation, which reads as an ordinary converged resume, so the rest of the call went on to register a subscriber and build a replay slice for a connection that was unwinding. A cancelled caller is now declined outright, returning the same shape as the closed-bus branch -- a closed channel, never nil, because the handler treats nil as "fall back to plain Subscribe" and would have re-registered the very caller being declined. MemoryBus was CHECKED rather than assumed clear, which is the scope note this bug carries and also how it was found (BUG-2749's filing asked for this package to be checked). It has no bounded wait and no I/O, so there is nothing for a cancelled caller to stop paying for -- but it declines one too, because the two implementations must not disagree about whether a departed client ends up registered. That divergence is invisible on a single-process deployment right up until it is a leak on a clustered one. Six tests, each failing against the specific thing it names: bus-ctx-only, caller-ctx-only, an implementation that never settles, no decline on RedisBus, no decline on MemoryBus, and -- the binding one, in internal/server -- a handler that passes context.Background(). The last is what CONVE-19 asks for: the bus tests vouch for the bus honouring cancellation and say nothing about whether the handler ever hands it one. The mid-settle cancellation lands through a new positional seam rather than a sleep. A sleep-timed cancellation that arrives late does not fail safe here; it silently measures the already-cancelled path instead. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): decline a cancelled resume before it touches Redis (r2) Codex round 2 verified round 1's fixes and came back clean on the two dimensions that could have made this change dangerous: every caller handles the declined closed channel correctly (the SSE handler falls back to plain Subscribe only on NIL, and the CLI treats EOF as reconnect), and there is no path where a LIVE caller's context is cancelled -- the route has no timeout middleware, and bus shutdown stays separately bounded through the merged context. Its P3 was a real one: the cancellation check sat AFTER resumeOutrunsLocalView, so an already-cancelled caller still entered it and made the first Redis GET. That fails on the dead context and logs "could not read the sequence counter to validate a resume; answering from local knowledge only" at WARN -- a line that means "Redis is unhealthy" to whoever reads it. Ordinary disconnect churn would have fired it on every client that hung up a moment before its resume landed. Moved ahead of the settle path. A METRIC WAS DECLINED, with the reasoning at the code. The finding asked for a cancellation counter so operators could distinguish disconnect churn from no resume activity. A client hanging up during its own resume is ORDINARY on a mobile network, so that counter would be a number nobody can act on, sitting next to pad_watchevents_resume_gaps_total where it would read as a fault. The condition an operator does act on -- capacity held by connections that no longer exist -- is already visible in the admission counts, and this change is what keeps those honest. Debug log instead. THE FIRST INSTRUMENT FOR THIS MEASURED NOTHING. I asserted "no Redis reads" as a proxy for "no misleading log", and go-redis short-circuits a cancelled context before it touches the wire -- so no GET reaches miniredis whether or not the early decline exists, and removing it survived. The assertion is on the LOG now, through a capture handler, because the log is the only thing that distinguishes the two. Fails with the exact WARN quoted back. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): our own cancellation is not a Redis fault (codex r3) Round 3 approved with comments and found the twin of round 2's finding. The entry-side decline catches a caller that was ALREADY gone; a caller can also leave while the first counter GET is in flight, and the error that comes back is context.Canceled -- indistinguishable, at the sequence-counter WARN, from Redis being unreachable. On a stream where clients hang up mid-resume that line would manufacture exactly the alarm an operator would chase. Suppressed to Debug when ctx is already dead, with a test that intercepts the GET from inside miniredis's command hook -- cancelling there and failing the command is what makes it the in-flight case rather than the entry case, without any timing. Fails with the exact WARN quoted back. Also cleared by that round, recorded because each was a real question rather than a rubber stamp: no self-sustaining state; slog.SetDefault is restored and the capturing test is non-parallel so it cannot bleed into package t.Parallel() tests; and whicheverEndsFirst is a correct second package-local copy of internal/events.mergeCancellation rather than a candidate for extraction -- a four-line shared utility would be coupling two packages for nothing. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): classify the caller-gone case on the error, not the context Codex round 4 BLOCKED, correctly. Round 3's suppression asked ctx.Err() rather than what the error actually was, so a GENUINE Redis failure arriving while the context happened to be dead would be downgraded to Debug and disappear — the signal an operator most needs, hidden by the change meant to reduce noise. My own new test demonstrated it: it returned a real server error and asserted no WARN. Now a predicate on the error alone, callerIsGone(err), and extracting it is the substance rather than tidiness. The two integration legs I had written CANNOT tell the two forms apart: both agree on every case a live client can be made to produce on demand — a cancelled context yields a context error, a live one yields a server error — so that pair passed against the blocked implementation too. Verified by mutation rather than assumed; reverting to ctx.Err() left them green. They disagree on exactly one case, a Redis failure whose error arrives while the context is already dead, and staging that through a client is a race by construction because go-redis decides by timing which error it returns. As a predicate there is no timing, and that case is a table row. Kept all three: the predicate table for the classification, and the two integration legs for the wiring — a cancelled read stays quiet, a genuine failure still warns. The second is the control without which "no WARN" is satisfied by a bus that has stopped reporting Redis trouble at all. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X