mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-22 18:43:45 +00:00
0eb274fed80c2019031e30e84bb3e8eeea51b3b4
1457 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
0eb274fed8 |
test: close the mutation gaps a coverage audit named (BUG-2730, codex round 18)
Round 18 walked every behavioural change in the diff, named the smallest edit that would break it, and listed the ones no test caught. Twelve. All but two are now covered, and each new test was verified against the mutation it exists for: - the activity bus's gap channel coalescing (the watch twin had it, this one did not) - Redis-backed atomic subscribe-and-replay, which reaches the guarantee by a different mechanism than MemoryBus and would break alone - a resuming client being held to the per-workspace limit, so the new API is not a second door past a bound the fresh path enforces - the resume-gap report on the new path, with a fresh-subscription control so it cannot fire on every subscribe and still pass - the Redis drop metric, asserted per DROPPED SUBSCRIBER with two slow subscribers, so a report hoisted out of the fan-out loop halves the count and fails - the gap channel surviving Unsubscribe, since closing it would make a consumer's select spin - every subscribe API returning a non-nil signal - the watch handler incrementing the WATCH counter (countMidStreamResync takes a bool to choose, which is the kind of argument that gets passed the wrong way round), with both wrong-counter legs asserted - the production cooldown, which every handler test overrides, so nothing else would notice it set to zero - the wrapper's gauge on the atomic-resume path, which the previous assertion checked only for non-nil-ness Two left uncovered deliberately: an interleaving test at the handler level for the atomic API (the bus-level tests carry that guarantee and the handler cannot arrange the interleaving), and the same for the handler choosing the atomic call over subscribe-plus-EventsSince. Two existing tests were also repaired rather than kept green by luck: the ordering stress test demanded every published event and a slow reader legitimately loses some, and the new Redis atomicity test resumed against a workspace the bus was not covering. |
||
|
xarmian
|
d6832d6604 |
docs(bus): state the gap seam's invariant for the third cause (BUG-2730, codex round 17)
Asked what a maintainer adding a third reason a subscriber can be signalled would get wrong: nothing stopped them routing a cause with DIFFERENT remediation through the same payload-free channel, passing every test, and silently losing the distinction. Stated as a precondition on both signalGap implementations, with the enforcement named — the channel carries no payload, so a distinction it cannot represent cannot be lost downstream, because it cannot be put in. Coalescing makes the same point from the other side: two causes between two reads become one signal, so per-cause handling was always undecidable here. A condition needing different handling belongs on its own seam. A typed reason was the alternative and is not worth it for two causes with identical remediation — it would buy a field nobody reads and reintroduce a which-reason-wins question that coalescing has no good answer to. |
||
|
xarmian
|
d6480c1f02 |
revert(sse): remove the ordering barrier; its failure mode is worse than the problem (BUG-2730, codex round 16)
Round 16 found the third defect in a row inside the previous round's fix: the gap branch reset gapDrainBudget to the CURRENT queue depth on every signal, so a producer refilling faster than a slow client drains could re-raise the coalesced gap before the budget reached zero and the announcement would never fire — the exact starvation the budget was introduced to prevent, one level up. Rounds 13, 15 and 16 each found a defect in the fix from the round before. That pattern is the signal to stop patching and reassess, so I reassessed the barrier itself rather than fixing it a third time. What it prevented: a client receiving sync_required and then events queued before the hole, whose IDs re-establish a cursor below it. Bounded and self-correcting — the client was told to reconcile, and a later reconnect from such a cursor is refused by the coverage check and told again. What it risked: never announcing at all, on the connection type this whole unit exists for. Unbounded silence. A mechanism whose own failure class is worse than the one it fixes should not ship, so the barrier, its drain budget and its predicate are gone. The announcer and its cooldown stay: they answer a real feedback loop and they latch rather than drop, and their binding to both handlers is tested. The residual ordering behaviour is now documented in docs/deployment.md under what a client should do with sync_required, and in a comment at the gap branch — stated rather than left for a reader to find, which is the same posture as the rest of this unit. |
||
|
xarmian
|
7c03beb24e |
fix(sse): bound the ordering barrier by a count, not by the channel emptying (BUG-2730, codex round 15)
The barrier shipped one commit ago with a comment asserting it could not starve. That was wrong, and wrong in the way that matters: it waited for len(ch) == 0, which never happens while a publisher refills faster than a slow client drains — and the subscriber this whole signal exists for is precisely a slow one on a busy workspace. The announcement it was supposed to make could be deferred indefinitely. The wait is now bounded by the queue depth captured when the gap was latched, decremented once per event taken off the channel. Once that many have gone out, every event that predated the hole has been delivered and anything still queued arrived after it, so the ordering guarantee is satisfied and the announcement goes. Terminating by construction, and exact rather than a timeout. The decrement counts filtered events too — an invisible event occupied a queue slot like any other. An honest note on the instrument, because the first one was no good. I wrote an end-to-end test with a goroutine publishing continuously and it PASSED against the unbounded version: under most schedulings the channel does briefly empty, so the scenario is not reliably reproducible through the handler. The bound is therefore a named predicate, gapReadyToAnnounce, with the starvation case asserted directly — latched, budget spent, channel refilled — where it cannot be scheduled away. The end-to-end test stays for the ordering claim, which it does discriminate. |
||
|
xarmian
|
3a00783557 |
fix(sse): queued events go out before the gap announcement (BUG-2730, codex round 13)
Reading both handlers as state machines: the event channel and the gap channel are two arms of one select, so with both ready Go picks at random. Announcing first and then draining events the subscriber queued BEFORE the hole is the wrong order twice over — the client is told its position is untrustworthy and then immediately handed IDs that re-establish one, below the hole; and on an ID-space change those queued events belong to the space that was just abandoned. The gap is now latched and the announcement waits for an empty channel. Nothing is discarded to achieve it, and that restraint is the load-bearing part on the watch stream: a queued one-shot PUSH cannot be recovered by any reconcile, so dropping it to make the cursor tidy would destroy the only copy. Draining cannot starve the announcement either — one event per iteration, re-checked at the top, so it lands on the first iteration with nothing queued, immediately when the channel was already empty. Pinned by asserting the ORDER of the frames with twenty events queued ahead of the gap. Without the barrier that fails on the first or third event, roughly half the time per run. |
||
|
xarmian
|
fa3710d9da |
docs: scope the metric correlations to the causes that produce them (BUG-2730, codex round 12)
A cross-artifact pass over every claim in the comments, help strings and deployment doc found two, both mine and both the same shape — a correlation stated as general when it holds for one cause: The watch drop metric and the doc row above it pointed operators at pad_event_midstream_resyncs_total, while watch announcements increment pad_watchevents_midstream_resyncs_total. Following either reference led to the wrong series. "drops >= announcements" and "the reset ratio is the fan-out" are each true of one cause and not of the others. A watch sequence gap announces to every subscriber without moving the drop counter; and the no-buffer coverage loss, which the previous round added deliberately, announces while moving NO cause counter at all — there was no coverage to end, but the subscribers still have a hole. That last one is the interesting case to leave written down, because an operator seeing announcements with every cause counter flat would otherwise reasonably conclude the metric was broken. Both counters' descriptions now say ANNOUNCEMENTS rather than clients told, and enumerate which causes correlate how. |
||
|
xarmian
|
b3c5ba95f5 |
style: gofmt the struct-field alignment the new Server field broke
Caught by make lint, after I chained the commit onto the same line as the gate and shipped it on a failing exit code. Same shape as the rule about never piping a gate: read the exit status before the commit runs, not alongside it. |
||
|
xarmian
|
a82bbd6b4f |
test(server): the rate limit has to be tested where it is BOUND (BUG-2730, codex round 11)
A one-survivor pass on the added tests: the announcer was tested directly and each handler test injected a single gap, so a handler that bypassed the limiter entirely and emitted sync_required straight from `case <-gaps:` passed everything — reopening the exact feedback loop the limiter exists to prevent. The same CONVE-19 shape as the wrapper: the component was vouched for, the binding was not. Both handlers now drive a burst through one connection and assert both halves of the bound: exactly ONE announcement inside the window, and one MORE after it. The second leg matters as much as the first — a handler that discarded the extras rather than latching them would satisfy the first and be this fix's own defect one layer up. The cooldown becomes a Server field so the test can narrow it. An integration test that waited out five real seconds per assertion would not have been written, which is how the gap got here. |
||
|
xarmian
|
d54f5236e8 |
docs: say what a client should DO with sync_required (BUG-2730, codex round 10)
Read as a third-party client author with only the wire contract, the frame was ambiguous: an empty id: retires the cursor but does not close the connection or request a reconnect, and the doc described recovery only for the web activity client. Now stated for both endpoints, including the part that is a limitation rather than an instruction: on the watch stream, watch-matched notifications can be re-derived by re-reading the items, but one-shot PUSHES cannot. They are not stored as recoverable state and there is no backfill endpoint, so a push missed during a hole is missed permanently. That endpoint is best-effort for pushes by design, and sync_required on it means the position is untrustworthy, not that a refetch makes the client whole. Also stated: keep the connection open. A client that redials on every sync_required turns one delta into a reconnect storm. |
||
|
xarmian
|
1e839331db |
fix(events): concurrent publishes must deliver in ID order (BUG-2730, codex round 9)
The composition angle found the duplicate this unit did not close. SubscribeAndReplaySince shuts the window between subscribing and reading the replay, but MemoryBus.Publish still assigned the ID under replayMu and fanned out after releasing it — so two concurrent publishes could take N and N+1 and deliver in the other order. A subscriber that sees N+1 then N has a cursor that REGRESSED, and its next reconnect replays N+1 a second time. Same symptom as the window this unit is about, reached by a different route, and closing one while leaving the other open would have been a half-answer. Pre-existing rather than introduced here, and MemoryBus was the outlier: events.RedisBus holds its mutex across append and fan-out, and watchevents.MemoryBus holds its single mutex across both. This one now holds replayMu through the fan-out, which costs an O(subscribers) walk inside a lock a resume read also wants — the trade RedisBus has always made. Pinned by a stress test rather than a seam, because the fix is precisely that there is no longer a point between the two halves to pause at. It fails against the unserialized version on the first of five runs. |
||
|
xarmian
|
8799e7d0cb |
docs: correct the comments this change made wrong (BUG-2730, codex round 7)
A next-maintainer read of every comment against the code it describes found nine, most of them made stale by this branch: - the watch observer and its fan-out still said a subscriber holding a stream open is told nothing about a sequence gap, which is the exact sentence this unit exists to falsify - the events interface described the gap signal as only a full-channel drop, omitting the coverage-loss scope that reaches the same channel - both SubscribeAndReplaySince doc comments still described a two-value return and an eviction-only nil - the InstrumentedBus header said it wraps without changing the interface or its implementations, in a diff that changes both - the SSE handler said a restarted Redis counter is undetectable, which BUG-2736 fixed; what stays silent is narrower And three correctness points about the new metrics, all conceded: - drops and mid-stream announcements are NOT one-to-one. Coalescing and the 5s latch turn a burst on one connection into a single announcement, so the counter measures announcements, not clients, and a large ratio means one client far behind rather than many affected. - the announcement counter increments before the write. Stated rather than changed: counting after would lose every announcement to a client that vanished mid-write, which is the population most worth seeing. - the doc said a connection is told at most once per five seconds. Only the MID-STREAM announcement is bounded; the resume signal is not, and never needed to be. A pass stripping review-history attribution from comments was reverted rather than shipped: it churned 50 files, and the surrounding code uses that attribution style throughout, so removing it here would have made this diff the inconsistent one. |
||
|
xarmian
|
6ce542782d |
docs: say what each stream actually detects, not what the pair does (BUG-2730, codex round 6)
An end-to-end trace of a pub/sub flap found the deployment doc claiming, for BOTH streams, that a reconnect or an undecodable message produces a mid-stream sync_required. True of the activity bus, which subscribes with ChannelWithSubscriptions and ends the workspace's coverage on either. False of the watch bus, which uses a plain Channel() and discards an undecodable payload with a log line — it learns of a hole only when a later notification arrives non-contiguous, so a flap that loses the newest notification with nothing published after it leaves a connected CLI silently stale. That gap is real and pre-existing (BUG-2731 was an activity-bus unit); filed as BUG-2739 rather than folded in, because widening DETECTION is a different claim from announcing what is already detected, and the watch bus's single replay buffer makes "end coverage" a decision rather than a copy. The doc now states the asymmetry and names the item. Also from the same round, both mine: a comment in the activity fan-out still said the drop was silent and that no bus had a channel to a live consumer, three lines above the code that signals one; and two metric descriptions still pointed operators at pad_*_resume_gaps_total for mid-stream signals, which the previous commit deliberately moved to pad_*_midstream_resyncs_total. |
||
|
xarmian
|
b7ae022b6f |
refactor(events): every way of subscribing hands back the gap signal (BUG-2730, codex round 5)
Subscribe allocated and raised a gap channel its callers could not read, which round 5 called dead work. The read is right and the disposition is the other one: an interface method whose subscribers CANNOT be told they missed something is a silent under-delivery waiting for its first production caller, and internal/watchevents' Subscribe already returns the signal, so the asymmetry was the defect rather than the allocation. Subscribe now returns it too, on all three implementations. No production caller changes — the handlers use SubscribeIfAllowed and SubscribeAndReplaySince — so this is a test-call-site sweep plus one signature. |
||
|
xarmian
|
d936464736 |
fix(events): bound the mid-stream signal, and stop it moving existing alerts (BUG-2730, codex round 4)
Three findings from the operator-at-3am angle, all real. A pub/sub outage on a workspace with a subscriber but NO replay buffer yet was silent. dropWorkspaceCoverage returned early before telling anyone, on the reasoning that there was no coverage to end — true of the BUFFER, and beside the point for the SUBSCRIBER, which has the largest possible hole and the least evidence of it. Live subscribers are now signalled on that path while the reset metric stays suppressed: the metric measures coverage endings, the signal measures clients who may have missed something, and those are different questions. The gap channel coalesces, which bounds the queue but not the loop: once the handler consumes a signal the next drop re-arms it, so a slow client could be answered with a delta sync, made slower, and answered again. Both handlers now share a gapAnnouncer that allows one announcement per connection per 5 seconds — a delta-sync round trip, not a tuning knob — and LATCHES rather than drops, so a gap inside the window is announced when the window closes. Suppressing it would be this fix's own defect one layer up. Folding mid-stream signals into pad_*_resume_gaps_total silently changed what every existing alert on those counters measures, and a mixed-version fleet would have reported two populations under one name for the length of a rollout. They go back to counting resumes; the new population gets pad_event_midstream_resyncs_total and pad_watchevents_midstream_resyncs_total, which count CLIENTS TOLD rather than causes — one instance-wide coverage loss moves them once per subscriber while the reset counter moves once, and that ratio is the fan-out an operator wants when judging a storm. |
||
|
xarmian
|
b9dff0072e |
test: close the coverage gaps codex round 3 named (BUG-2730)
Round 3 reviewed the added tests as production code and found four things, all real: - the atomicity test never asserted the replay CARRIED anything, so an implementation that always answered "cannot vouch" and delivered everything live would have satisfied "never both" by never replaying. It now seeds two events and resumes from the first. - both handler tests would have passed for a handler that emitted sync_required and then closed the stream. The activity one now proves an ordinary event still arrives afterwards; the watch one, which has no cheap ordinary event to publish, proves the handler did not return. - the refused-connection test's end state also holds on origin/main, so it argues for nothing about the new ordering. It is a regression test for the defect that ordering introduced, and the comment now says so. - whole paths had no test: the activity RedisBus's drop, coverage-drop and ID-space-reset signalling; the watch bus's epoch-change and counter-backward arms; the InstrumentedBus delegation; the metrics adapter; and the mid-stream counter increment. All covered now. The wrapper tests matter most of the three new files: it is the only implementation that does not originate a gap channel, and in production the bus IS wrapped, so a wrapper returning nil there would have disabled the whole fix while every bus-level test stayed green (CONVE-19). |
||
|
xarmian
|
b2277641cf |
fix(events): a refused connection is not a sync_required (BUG-2730, codex round 1)
Moving the Last-Event-ID parse above the subscribe — which is what makes the atomic subscribe-and-replay possible — put the unreadable-cursor increment on the wrong side of the per-workspace admission check. A connection refused with 429 is sent nothing, and was still counted in pad_event_resume_gaps_total, whose population is signals SENT. Counted at the emission site instead, which is where it effectively was before the parse moved. Pinned with both legs: refused must not count, admitted-with-an-unreadable-cursor must. |
||
|
xarmian
|
db8c5b76ed |
docs(deployment): sync_required is not only a resume answer (BUG-2730)
The signal's documented meaning was resume-shaped in every place it appeared, while the fix widens it to a live subscriber told mid-stream that it has a hole. A widened signal whose docs still state the narrow meaning is a half-shipped contract. Adds a subsection stating both situations and what a client does with each, and corrects the two resume-gap counters' descriptions: they count SIGNALS, not resumes, so a deploy with no reconnects at all can now move them. Documents the new pad_event_events_dropped_total, including that a deploy which starts reporting it may simply be the first that could. |
||
|
xarmian
|
1cee8e615c |
test(server): the quiet-control leg could not see a gap announced at connect (BUG-2730)
A mutation that made the activity handler announce a gap unconditionally at connect SURVIVED both tests. The control asserted the absence only AFTER publishing an ordinary event, and waitForFrameWithEvent reads past every frame that is not the one it wants — so the spurious sync_required went by unexamined and the window that followed was genuinely empty. The absence is now asserted first, before anything else is published, and the wait for the ordinary event refuses a sync_required instead of reading past it. Both legs fail on the mutation now. |
||
|
xarmian
|
af99762815 | test(server): the gap signal must reach the wire, not just the bus (BUG-2730, CONVE-19) | ||
|
xarmian
|
fa2ddf3f22 | test(events): pin the drop signal, its metric, and the subscribe-replay atomicity (BUG-2730) | ||
|
xarmian
|
9f23364b43 | test(watchevents): pin both gap-signal scopes with their counterfactuals (BUG-2730) | ||
|
xarmian
|
4269bdd4cc |
fix(watchevents): the same hole, told to the stream holding it open (BUG-2730)
fanOutLocally already DETECTED a gap in the received notification sequence, logged the exact id range, and raised knownFrom so a client RECONNECTING across it was honestly told sync_required. A client holding the stream OPEN across the same gap was told nothing: it stayed connected, kept receiving everything after the hole, and never saw what went missing. The instance knew; the one consumer whose correctness depended on it did not. Subscribers now carry the same capacity-1 coalescing gap channel the activity bus grew, raised from two different scopes: - per-INSTANCE, to every live subscriber, when this instance discovers a hole in what it received — a sequence gap, a counter that went backwards, an epoch change. Instance-scoped is the whole scope: the ids never arrived HERE, other instances may have them, and every subscriber registered at the moment of detection is exactly the set that was connected across the hole. - per-SUBSCRIBER, to one connection, when its channel was full. The watch SSE handler answers with sync_required mid-stream. The pad CLI monitor already clears its cursor on that event, so the client half needed no change. Refs BUG-2730. |
||
|
xarmian
|
b5615cc1b5 |
fix(events): a live subscriber is told when it has a hole (BUG-2730)
The activity bus dropped an event for a subscriber whose 64-deep channel was full, logged it, and continued. The subscriber was told nothing, so a later delivered event advanced its Last-Event-ID past the dropped ids — after which no replica would ever replay them, because every replica agrees that cursor is current. Only a full reconciliation corrected it. Every subscriber now carries a capacity-1 coalescing gap channel. It is raised when the bus cannot hand that connection an event, and when this instance's coverage of a workspace ends under it (a pub/sub flap, an undecodable message, an ID-space reset) — that second case is per- instance and reaches every subscriber of the affected workspace, because the gap is theirs collectively. The SSE handler selects on it and emits sync_required mid-stream, the same signal a resume we cannot serve gets: the web client already answers it with an incremental /changes delta, so the distinction a new event name would express is one the client would act on identically. Also folded in, because they are the same defect surface: - Subscribe-and-replay is now ONE critical section on both bus implementations (SubscribeAndReplaySince), closing the duplicate window where an event published between the two steps landed in both the replay set and the live channel. MemoryBus takes b.mu across the buffer append and the fan-out to get it; the lock order is b.mu then b.replayMu, everywhere. - internal/events gains the drop report its sibling has had since BUG-2699 (Observer.EventDropped, pad_event_events_dropped_total). The drops were log-only, so the condition this fix makes honest was not countable before it. Both resume-gap counters' help text now names what they actually count — sync_required signals, including the mid-stream ones — rather than resumes alone. Refs BUG-2730. |
||
|
xarmian
|
2a121c32d5 |
Merge pull request #1178 from PerpetualSoftware/fix/events-idspace-migration
fix(events): give every event ID space an identity, behind a two-phase flip (BUG-2736) |
||
|
xarmian
|
6e590b48ff |
docs(events): the straggler window closes per workspace, not globally (BUG-2736)
Codex round 21, correcting a claim I made in round 17 and asserted only in the direction that was convenient. Round 17 said the mixed-roll straggler window 'is one event wide and ends loudly', because the next event from the new space is lower than the straggler's id and trips the counter-backwards check. That check is PER WORKSPACE and the sequence counter is GLOBAL. If other workspaces consume ids past the straggler's value before this one publishes again, this workspace's next id is higher, nothing fires, and the dead-space id stays in the buffer — where a client resuming from just below it is served it as though it followed. My test asserted the closing case and stopped there, which is the shape my own record names: a partial verification stated without its boundary reads as a complete one. The boundary is now its own test, written as a characterization — it asserts that nothing detects this TODAY, so if someone adds the global high-water mark that would close it, the change announces itself there rather than in a deployment. Not closed here. A global comparison fires on interleaves across ANY pair of workspaces during the phase-2 roll, when un-flipped publishers interleave routinely — the storm round 9 armed this check against. It belongs with the other residuals the client cursor's missing epoch would close. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
db7a9317ed |
fix(events): two mechanisms that were each right alone (BUG-2736)
Codex round 20, asked to check the accumulated fixes in COMBINATION rather than one at a time. Both findings are interactions, which is what that angle is for. A STRAGGLER ERASED THE COUNTER-BACKWARDS FLOOR. The straggler drop reused the same argument an epoch change uses, which CLEARS the floor — but a lower generation inside the window is not proof of a new space; that is precisely the question the window exists to defer. The sequence: a counter-backwards reset raises the floor to 100, a straggler clears it, bare traffic repopulates from 51, and a client resuming from 99 is served later ids as though coverage were continuous, silently skipping 51..99. The boolean is now three named intents — clear, raise, keep — because the third had no way to spell itself and so took the wrong branch silently. Each call site now says which it means. A WRONG-TYPED EPOCH KEY DEFEATED THE RECOVERY WRITTEN FOR A CORRUPTED ONE. Round 11 rotated an epoch key holding a bad VALUE; a bare GET on a key holding a list raises WRONGTYPE and aborts the script before that branch can run, so every phase-2 publish failed until someone deleted the key by hand. The script checks TYPE first. THE MUTATION MATRIX EARNED ITS PLACE TWICE HERE. The first version of the wrong-type test passed with the TYPE check deleted: the script's id == 1 branch SETs the epoch unconditionally, and SET replaces a key of any type, so on a fresh counter the wrong-typed key was overwritten before the GET was reached and the branch under test was never entered. The test now publishes once first, and the mutation fails it. The same matrix then found a DEL in the recovery branch that no test could distinguish — redundant for the same reason SET replaces any type — so it is gone rather than left as a line nothing can justify. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
53afc2172c |
fix(events): a generation we cannot vouch for ends coverage, not just the message (BUG-2736)
Codex round 19, inside round 6's own fix. Round 6 made a LOWER generation inside the straggler window discard the message. It left the replay buffers valid — so a client reconnecting during that window was told it was caught up. Harmless if the message really was a straggler, and thirty seconds of silently missed events if the generation had regressed instead, because then the messages being discarded ARE the live stream. A bus that has just decided it cannot classify what it is seeing must not go on claiming it can answer for the span. Coverage now ends on the first lower generation. The CLASSIFICATION still waits out the window — the epoch is not adopted there — so a true straggler does not drag the bus into the dead space. Its cost is one extra drop next to a rotation that had already dropped the buffers, which is nearly free and loud either way. That changes what epoch_regressed means, so its documentation changed with it: it now reports that a lower generation was SEEN, and the two causes are told apart by count rather than at the moment it fires. One alongside an epoch_change is a message in flight during a rotation; a run of them is Redis losing writes. The test asserts both halves — the straggler still does not move the epoch and is still not buffered, AND coverage ends — plus the control that the live generation re-establishes coverage immediately, so this is a resync rather than a dead bus. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
f3aa86503a |
fix(events): assign the in-memory id under the lock that orders the buffer (BUG-2736)
Codex round 18, pointed away from the Redis bus that seventeen rounds had concentrated on. Three findings; one belonged in this unit, one is filed, one is documented. THE ID WAS ASSIGNED BEFORE THE REPLAY LOCK, so two concurrent publishes could take N and N+1 and append in the other order. replayBuffer.since computes oldest and newest by POSITION, so a buffer holding [N+1, N] reports N as its newest and answers a resume from N+1 with sync_required — a client told to resync at the moment it was exactly current. Pre-existing in shape, but it is the same invariant this unit buys for the Redis bus with an atomic publish script, on the same buffer, for the same reason: publish order must equal id order because the buffer's own ordering assumptions are otherwise false. Fixing one and leaving the other would be half an invariant. The test drives 300 concurrent publishes and asserts both the order and its consequence — that every id in the buffer is servable as a cursor, which under the race the newest ones were not. Verified to fail 5 of 5 with the assignment moved back out. FILED, NOT FOLDED IN: BUG-2737. Neither activity bus refuses a subscription after Close, so a handler that subscribes during shutdown holds a channel nobody will close and blocks for the full 30s deadline. It is a shutdown-lifecycle defect rather than an id-space one, it spans both implementations, and internal/watchevents already fixed the identical thing in BUG-2651 — so the fix is porting a decided question, not answering one. DOCUMENTED: the SSE data body carries the event id as a JSON NUMBER, which is now around 1.8e18 and past JavaScript's MAX_SAFE_INTEGER. It cannot be removed — the Redis bus's phase-1 wire form carries the id there and nowhere else — so the frame writer now says that the "id:" field is the one a client may use, and why web's ItemEvent deliberately declares no id. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
a0eb070b00 |
docs(events): name the mixed-roll straggler window, and assert what bounds it (BUG-2736)
Codex round 17. Once a replica has adopted an epoch, a message from an un-flipped instance carries none and is treated as belonging to the current space. It does — unless the sequence counter reset between that publisher assigning its id and publishing it, in which case an id from the dead space lands in a buffer describing the new one. NOT FIXED, because every alternative rule is worse and there is no discriminator. Refusing bare messages once an epoch is adopted would end coverage on every un-flipped publish for the length of the roll, which is a resync storm; delivering without buffering would put holes in the buffer that nothing records. An id from the dead space and an id from an un-flipped publisher are both 'above what we hold' and otherwise identical. What makes it acceptable is a mechanical property rather than an argument, so it is asserted rather than described: the next event from the new space is LOWER than the straggler, which trips the counter-backwards branch, drops the buffers and reports a reset. The exposure is one event wide and it ends loudly. The test also pins the other half — that nothing can detect the straggler ON ARRIVAL — because a reset there would mean the discriminator exists after all and the whole disposition was wrong. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
be47896e13 |
fix(events): hold both wire forms to the same id rule (BUG-2736)
Codex round 16. The positive-id check lived inside the PREFIXED branch of decodePayload, so a bare payload carrying id 0 or a negative was accepted: delivered with no SSE cursor for the client to advance to, and — once an epoch has been adopted — read as the sequence going backwards and used to discard every replay buffer. The check is now a function both branches call, which is the by-construction form rather than a second copy that can drift. Same route as an unreadable payload: this workspace's coverage ends and the next resume says so. The failing case that made this worth finding is the one my own round-11 test did not cover — it exercised the prefixed form only, because that is the form the guard was written next to. Symmetry asserted in the rejections table now, for both forms. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
19f29e1a67 |
fix(events): the channel is the authority on whose event a message is (BUG-2736)
Codex round 15. Two payloads decode without error and are not a usable event,
and both were skipped silently.
"null" and "{}" both unmarshal into a ZERO Event, whose workspace is the empty
string. Fan-out indexes subscriptions by the body's workspace, found none for
"", and returned early WITHOUT ending coverage — so the buffer went on looking
continuous across an event it had skipped, and a later id was replayed after
an earlier cursor as though nothing was missing.
A body naming a DIFFERENT workspace from the channel it arrived on is the same
shape one step further: fan-out would have appended it to that other
workspace's buffer, with an id from a stream those subscribers are not
reading.
The receive path now requires the body's workspace to match the channel's.
Both cases take the same route as an unparseable payload, because they are the
same fact about the world: something reached this channel that this
installation did not publish.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
1e40c972ae |
refactor(events): one home for the asymmetry, one assertion for the count (BUG-2736)
Codex round 14 was a prune pass rather than a defect hunt: 695 comment lines to 243 code lines in internal/events and internal/idspace is a ratio worth looking at, and my day-51 close records what happens when review prose grows under a diff. MOSTLY DECLINED, and the reason matters more than the decision. The list would have stripped this diff to terse definitions -- deleting bug archaeology, rollout tradeoffs and codex-round attributions -- in files whose EXISTING comments are dense narrative and carry exactly those things, "(codex round 6)" attributions included. It also named several blocks on code this diff never touched. Applying it would have made the new code inconsistent with the file it lives in, which is a style change to someone else's codebase wearing a review's clothes. APPLIED, because both match lessons this team already recorded rather than a reviewer's preference: - The numeric-base-vs-travelling-epoch argument existed in FOUR copies, in both packages. The lead's requirement was that the asymmetry declare itself in both buses' comments -- not that the argument be written four times. It now lives once, in internal/idspace's package comment (the shared code both halves depend on), and the four sites state the asymmetry in a sentence and point there. Four copies is four things to update when the deferred follow-on lands. - "Costs at most ONE drop per instance per roll" was a countable claim in prose with nothing asserting it. A comment asserting countable behaviour belongs in the suite: later messages in the same generation must leave the buffers alone, or "bounded" in that sentence is doing no work. Honest measurement, since the point of the round was volume: the net is +47 -41. The prune did not shrink the diff; it removed three copies of one argument and turned one claim into an assertion. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
9fe2839cc2 |
fix(events): an epoch that overflows int64 is corruption too (BUG-2736)
Codex round 13, plain pass, inside round 11's own fix. That fix made the publisher rotate an epoch key holding anything that is not a positive generation, so corruption self-heals instead of dropping every event forever. The pattern match it used accepts a value that is all digits and overflows int64 -- which the RECEIVER parses with strconv.ParseInt, so the payload is rejected on arrival and every event is dropped anyway. The same total, silent, unrecoverable failure by a different route, through the guard written to prevent it. Bounded by length rather than by value, because Lua has no int64 comparison to hand: any 18-digit number fits, and a generation counts an installation's id-space resets, so 18 digits is not a bound anything real approaches. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
5f8252ce88 |
test(events): cover the migration's central claim and three untested branches (BUG-2736)
Codex round 12 asked only "what has no test at all", which is a different question from "is each fix pinned" and found things twelve rounds of the second question had not. THE CENTRAL CLAIM HAD NO TEST. Parsing, publishing and fan-out were each exercised alone; that a phase-1 and a phase-2 instance on one Redis deliver each other's events -- the thing the whole two-phase design rests on -- needed both buses at once and nothing did that. Now both directions, with both receivers asserted, and the premise that the two payload forms really did differ. THE BACKWARDS GUARD IS `<=` AND EVERY TEST USED A STRICTLY LOWER ID. A `<` implementation passed all of them while letting a REPEATED id into the buffer -- a duplicate delivery and a replay that can serve the same id twice, with no reset reported. Mutation-checked. THE ROLLBACK PRECEDENCE HAD NO TEST. The procedure tells an operator to make the EFFECTIVE value false and warns that unsetting the env var is not the same thing; neither half was pinned, so a load order letting the file win over an explicit env-var false would have kept a deployment on phase 2 while its operator believed they had rolled back. AND THE RESIDUAL IS NOW EXECUTABLE. A bus with empty buffers adopts an epoch without dropping, so its first buffer starts at the first id it sees and a client holding the id one below is served. That was described in three places and asserted in none. It is now a characterization test that states the load trade in its own comment, so a future change to it is a decision rather than a side effect -- and the boundary is pinned exactly: adjacent is served, one lower is a gap. Declined with reasons: the startup phase log cannot be driven without standing up a server and its wrongness is visible on first read; the race between a phase-1 epoch delete and a concurrent phase-2 mint is documented, bounded at one extra buffer drop, and not deterministically reproducible; and the cross-Redis failover retry needs two Redises with controlled replication lag, which miniredis cannot represent honestly. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
f243540430 |
fix(events): three failure paths that lost events without saying so (BUG-2736)
Codex round 11 enumerated every Redis call, script step, parse and conversion the diff adds. Three of its findings were silent-loss paths. THE DEDUPE TOKEN WAS WRITTEN IN THE WRONG ORDER. Redis runs Lua atomically against interleaving, NOT with rollback: a script that errors part way through keeps whatever it already wrote. With the token written first, any later failure -- a wrong-typed key, an ACL denial -- left the token behind on a run that never published, and go-redis's retry then declined it. The event lost, permanently, with the caller told it succeeded. It is now CHECKED first and WRITTEN last. A script that dies early leaves no token and the retry does the right thing; a script that completed and merely lost its reply leaves one and the retry declines. The remaining window is an error on the final SET, whose key is a fresh uuid and so cannot be wrong-typed, and whose cost would be a duplicate rather than a loss. AN UNREADABLE MESSAGE WAS DROPPED AND FORGOTTEN. The buffer went on claiming a span that now had a hole in it: the event gone, the ids either side contiguous, and a later resume across it answered "caught up". It now ends that workspace's coverage, so the resume answers sync_required. The workspace comes from the CHANNEL rather than the body, which is what makes that possible when the body is the thing that would not parse. THE PUBLISHER TRUSTED WHATEVER THE EPOCH KEY HELD. Set to something that is not a positive generation -- corrupted, hand-edited, or written by another installation sharing the keyspace -- it was emitted into every prefix, every receiver rejected the payload, and every event was dropped for as long as the key stayed that way. The script now rotates instead: one generation change, one round of resyncs, and the space is identifiable again. Also: decodePayload refuses a non-positive id. The SSE handler omits the id: field for one, so such an event would be delivered with no cursor to advance to and the client would resume from the id before it forever. Both new conditions get their own reason label rather than being folded into an existing one, because an operator acts on undecodable_message differently from anything else here: it means something is publishing onto these channels that is not this installation. Mutation matrix: 4 applied, 4 caught -- but only after two survived the first pass. The dedupe order and the id check had no test that could tell the fixed code from the broken code; the tests that pin them now had to be written to make the mutations fail, which is the point of running the matrix rather than counting the tests. Declined with reasons: the phase-1 assign/publish eviction window is the legacy path this migration exists to replace, and the resume-gap counter's missing cause label is a pre-existing shape. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
6afe683389 |
fix(events): do not arm reset detection where interleave is ordinary traffic (BUG-2736)
Codex round 9, from the 3am-operator angle. Six findings; one of them was a regression this diff would have shipped in the DEFAULT configuration, and the review framed it as a log-volume problem. THE REGRESSION. Phase 1 publishes with a two-call INCR-then-PUBLISH, so on any multi-instance deployment two publishers interleave routinely and a lower ID arrives after a higher one as ordinary traffic. main has no counter-backwards detection at all; this diff added it. Armed unconditionally, it would have fired on that ordinary interleave, dropped EVERY workspace's replay buffer, and resynced every client -- in phase 1, which is where every deployment sits until an operator flips phase 2. The check is now armed only once an epoch has been adopted. What that costs is stated rather than hidden: a genuine counter reset on a never-flipped deployment goes undetected, which is exactly the behaviour before this change and precisely the case phase 2 exists to fix. The new test asserts the gate, and also asserts what is NOT claimed -- the interleaved workspace's own buffer still holds ids out of order, so a cursor at the higher one reads as foreign. That is pre-existing, unchanged here, and strictly less harmful than a global drop; it is asserted rather than described so a future change to since() surfaces there. THE REST ARE THE OPERATOR'S SIGNALS, which were unreadable: - The effective phase was invisible. pad_event_sequence_resets_total cannot be interpreted without it -- a counter_backward rate is expected on phase 1 and an anomaly on phase 2 -- and the setting can arrive from an env var, a TOML file, or neither. It is now on the startup line as id_space_phase. - An unparseable PAD_EVENTS_PUBLISH_EPOCH was silently ignored, so an operator who typed "yes" believed they had flipped. Ignoring it stays the right behaviour; being silent about it does not. - Both publish-failure logs said only "failed to publish". They now say what the operator needs, which differs by phase: phase 1 may or may not have reached subscribers, and phase 2's script is atomic so it did not half-execute, but a lost reply means it may have published anyway -- do not re-publish by hand. - Adopting an epoch with empty buffers is the moment the documented residual becomes possible on that replica, and it happened silently. It now logs at INFO -- not a reset count, deliberately, since counting it would give the reset metric a per-deploy baseline. Declined with reasons: a cause label on the resume-gap counter and a publish failure counter are both pre-existing shapes rather than anything this diff changed, and the straggler log is already bounded by the recovery window. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
378dec5244 |
docs(idspace): name the assumption the incarnation bound rests on (BUG-2736)
Codex round 8, on a fresh angle. The invariant was stated in terms of publish RATE -- an id can repeat across incarnations only if the earlier process published more than 2^20 events per millisecond of its life -- and quietly assumed the other half: that the next start lands in a LATER millisecond. The bases are separated by the clock at millisecond resolution, and the CAS separates only buses built inside one process. A second process starting inside the same millisecond as the first would take the same base and reissue its ids. Not closed, and the reason it is acceptable is physical rather than hopeful: reaching the constructor means the OS reaped the old process and the new one bound its listener, opened its database and ran migrations. Closing it for real needs persistence, which BUG-2736's body rules out for a separate and stronger reason. So it is accepted and NAMED -- in the package comment and in deployment.md -- rather than left for the next reader to find during an incident. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
e12cc5810e |
docs(events): say why each mechanism is here, after a scope review (BUG-2736)
Codex round 7 asked the question I do not reliably ask of my own work: should each of these mechanisms be in this change at all. Five findings, all DECLINED, and the reasons are worth having in the artifacts rather than only in a review log. Two were already the lead's explicit scope for this unit and are not mine to re-open: the two-phase rollout, and removing web's unread id?: number field while it is still unread. One I decline on the argument rather than the authority. The atomic publish script is not an ordering improvement bundled into an ID-space change: the interleave it closes is older than this diff and was merely wrong, but this diff makes it HARMFUL, because counter-backwards detection reads a descending ID as a reset and would fire on every ordinary interleave. And the dedupe token is required BY the script for the same kind of reason -- phase 1 retries a PUBLISH whose payload already carries its ID, so a duplicate arrives under the SAME ID; phase 2's retry re-runs the assignment, so it arrives under a SECOND one, ascending and indistinguishable. Moving assignment into the script is what makes retries worse. Cutting the token while keeping the script would ship a regression. That reasoning is now in the script's comment, where the next person asking this question will find it. One I decline as completing a fix rather than extending scope: the lower-generation recovery exists only because this diff's own straggler rule created a discard-forever state. Cutting it would leave a new unbounded silent failure in a unit whose entire subject is not failing silently. And one is a framing problem rather than a scope problem, which is the useful half of the round. The migration is a substantial MITIGATION and not a closure: it stops a replica mixing two ID spaces in one buffer, and it does not make a client's cursor say which space it came from. That was stated at the end of the deployment section, after the procedure; it is now stated before it, because a reader deciding whether to run the migration should meet the limit before the steps, not after. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
417776ce9b |
fix(events): recover when the generation counter goes backwards and stays (BUG-2736)
Codex round 6 walked four realistic scenarios through the code line by line. Three of its findings were already-filed or already-documented residuals; one was a hole my own round-3 fix had opened. THE HOLE. Round 3 made a LOWER generation mean "a straggler from a space we have left" and discarded the message. That is right for a message in flight at the instant of a rotation. It is wrong, and unrecoverable, for a Redis failover to a replica whose copy of the generation counter predates the rotation: every publisher then mints from the lower number, and this bus discarded every message forever -- nothing delivered, nothing buffered, and the only trace a log line per message. Silent and unbounded is the one outcome this family refuses, and round 3 had traded a loud bounded problem for it without noticing. A persistent regression is now ACCEPTED as a new space: buffers dropped, next resume answered sync_required, delivery resumes. Loud and recoverable. The discriminator is a physical quantity rather than a guess about intent -- a straggler is bounded by pub/sub delivery latency, so a lower generation arriving long after the adoption cannot be one. Both ways of being wrong are loud: too short costs an extra buffer drop, too long costs a few seconds of discards before recovery. It gets its own reason label, epoch_regressed, because an operator acts on it differently from every other reason here: the others are expected, this one means Redis lost writes. The metrics test now drives every reason with DIFFERENT counts, so an adapter that collapsed them onto one series fails there instead of in production. ALSO RECORDED RATHER THAN FIXED, because the review found the claim overstated rather than the code wrong: the publish dedupe token is as durable as Redis replication and no more. A retry that lands on a promoted replica which never received the token publishes a second copy under a second ID, and nothing downstream can tell the two apart. The comment said the token turns a retry into a no-op; it now says which retry. The other three scenario findings are pre-existing and filed: the subscribe-then-replay duplicate window is BUG-2730 and is documented at the site it happens; the empty-buffer replica that serves an adjacent cursor across a cutover is the residual this unit's own comment already names, with the numeric-base design that closes it on BUG-2736's trail; and a held-open SSE connection is not told about a gap detected under it, which is BUG-2730's family too. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
736a8c48f7 |
docs: thirteen claims about code that had moved under them (BUG-2736)
Codex round 5, cross-artifact consistency. Every one was a claim in a comment, help string, doc, or test name that the code no longer supported — and this diff created most of them by moving the code. The ones that would have misled an operator: - pad_event_sequence_resets_total documented ONE reason in both the Go doc comment and the Prometheus help text, and the deployment table said the same. It has emitted three since this branch. An operator reading the help string to build an alert would have alerted on a third of the signal. - deployment.md said every published message carries an epoch prefix. Phase 1 publishes bare JSON — which is the entire point of having two phases. - deployment.md said the first flipped message reaches each replica and every resuming client gets sync_required. A replica learns the epoch only from a message it RECEIVES, so only replicas subscribed to a workspace with traffic see it; and a replica with empty buffers adopts without dropping or counting, deliberately. - deployment.md said a restart's IDs cannot collide. internal/idspace documents a bounded case — the earlier process publishing more than 2^20 events per millisecond of its life. Stated as the bound it is, with the backwards-clock direction named as the safe one. - cmd_watch.go described sync_required as eviction-only. It has had four other causes since BUG-2731 and gained a fifth here. The ones that would have misled the next person editing this code: - bus.go said the Redis half was unwritten and a reset counter could still merge two ID spaces. It is written, three commits back on this branch. - bus.go and watchevents.go said in-memory IDs restart from 1. They count from an incarnation base. - redis_bus.go described this bus's epoch as an opaque uuid equivalent to the watch bus's, twice, after round 3 made it a Redis-minted generation. Only the watch bus still uses uuids. - observer.go said counter_backward happens only during mixed-version rolls. Phase 1's two-call publish produces it in steady state too. - redisns.go said the publish script spans four keys (it is five here now, plus a two-key assign script), and its hand-kept reserved-name inventory never gained event_epoch or event_epoch_gen — so a namespace equal to either would have nested one installation inside another's keyspace unrefused. - A test comment referenced idIncarnationShift, which moved to internal/idspace.Shift when the package was extracted. - Two tests called themselves process-restart tests while constructing successive buses in one process. They test bus incarnations; the comment now says so and says why that is the equivalent thing. And one reasoning error rather than a stale fact: the counter-backwards branch justified raising the floor by asserting the arriving ID is necessarily in the SAME numeric space. It is not — a phase-1 counter reset publishes low IDs with no epoch to explain them, which is a NEW space we cannot see. The behaviour is unchanged and still correct (the lead's day-52 ruling: raise unconditionally, prefer a loud bounded resync loop to a silent skip), but it now says what it actually knows, which is nothing, and names the cost on a real phase-1 reset. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
d393126d80 |
test(events): close the gaps a tests-as-production-code pass found (BUG-2736)
Codex round 4, on the tests themselves. Eight findings, all real; three of them were behaviours in this diff with no test at all. NO TEST AT ALL: - The real receive path. Every reconciliation test drove fanOutFromRedis directly and the publish tests read the wire with a raw subscriber, so a regression that decoded the epoch correctly and then handed 0 to the fan-out would have passed all of them -- reconciliation silently never running in production. Now driven through Subscribe/Publish and back through Redis, with the mutation checked. - The atomic script's ordering claim. Every phase-2 test published once or ran the script sequentially, so a two-call INCR-then-PUBLISH implementation passed them all -- and that ordering is load-bearing, because the receive path reads a descending id as a counter reset. 300 concurrent publishes now assert arrival order equals id order; verified to FAIL 5 of 5 against a two-call implementation and pass 3 of 3 against the script, so the instrument discriminates rather than merely being green. - The TOML tag. The env-var test proved PAD_EVENTS_PUBLISH_EPOCH reaches the field and said nothing about the toml:"events_publish_epoch" tag -- the exact form the rollback procedure warns about, since a file value outlives an unset env var. PASSING FOR THE WRONG REASON: - The production config wiring was still unexecuted: passing an empty config.Config at both RunE call sites compiled and passed everything. The source-text guard that already counts those call sites now also requires them to pass the loaded config. - The phase-2 wire assertions accepted a well-formed payload with an empty event body. They now assert the body survives. - The Redis metrics subtest had no served-resume control, so a bus that refused every resume would have passed. It now round-trips a publish through Redis first. - TestResumeGapIsReportedForBothWaysOfNotServing never proved ws-warm HAD a buffer, so its second half could silently duplicate its first. - internal/server's cold-resume tests still sent a literal 4200, which the incarnation guard now answers before the handler's no-buffer path is reached. My own round-1 sweep of this class stopped at four packages and never looked at internal/server: reviewer-named instances are a sample (team CONVE-18), and so, evidently, are self-named ones. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
c3d485d136 |
fix(events): make the ID space's epoch a monotonic generation (BUG-2736)
Codex round 3, on concurrency. Two findings, and the first says the epoch's TYPE was wrong. AN OPAQUE EPOCH CANNOT BE ORDERED. Each workspace has its own Redis subscription and its own receive goroutine, and Redis orders messages within a channel but not across them. So a message published BEFORE a rotation, on workspace A's channel, can arrive AFTER the rotation was already learned from workspace B's -- and with a uuid there is no way to tell that straggler from a second rotation. The bus flipped back into the dead space, dropped every buffer again, and the "at most one drop per instance per roll" property this unit claimed was simply false. The epoch is now a generation number minted by Redis (INCR on a counter that Pad never deletes), so the two spaces are comparable. A HIGHER generation is adopted; an EQUAL one is steady state; a LOWER one is a straggler from a space we have left, and its message is DISCARDED rather than delivered -- its id belongs to the dead sequence, so buffering it would put two spaces in one buffer, and its subscribers were already told to resync across the change. A wall clock was the other way to order them and is the wrong one: instances have different clocks, so a rotation minted on a lagging machine could carry a lower stamp than the space it replaces and be ignored forever. That is a silent failure where this is a loud one. Minting inside the script also removes the propose-then-SET-NX race: two publishers can no longer both believe they minted the space. THE SECOND FINDING was a TOCTOU in yesterday's phase-1 stale-epoch clear: INCR and DEL as two commands leave a window in which a concurrent flipped publisher mints an epoch between them, and we delete a LIVE one. Phase-1 assignment is now a two-line script, so the restart and the clear are one atomic step. The wire form it publishes is unchanged -- still bare JSON with the id inside, which is the whole point of phase 1. decodePayload now refuses a zero or negative generation. Zero is this package's sentinel for "no ID-space information", so a malformed publisher carrying it would make every receiver stop reconciling while looking healthy. Mutation matrix: 6 applied, 6 caught -- straggler adopted, adoption weakened to any-difference, straggler ignored but still buffered, the phase-1 clear removed, the generation minted as a constant, and the zero-generation guard removed. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
94cc2492fc |
fix(events): a phase-1 counter restart must not leave a live epoch behind (BUG-2736)
Codex round 2, on the rollout angle. Four findings; one was a real silent-loss hole and three were claims in the docs and config comment that the code does not support. THE HOLE. Phase 2 mints an epoch and the counter climbs; the deployment rolls back to phase 1; the seq key is then evicted or deleted; phase-1 publishers climb from 1 again; phase 2 is re-enabled and its SET NX finds the OLD epoch still there. A receiver that had adopted it sees no change, and if its high-water mark is below the new sequence -- a replica that just started, or one whose buffers were empty -- the numeric check does not see the reset either. Two ID spaces merge in one buffer silently, which is the outcome this whole unit exists to prevent. Phase 2's rotation cannot cover it: that rotation fires when the SCRIPT's own INCR returns 1, and by then the counter has climbed past 1 under the phase-1 path. So phase 1 now deletes the epoch when its own INCR returns 1. Deleting rather than rotating, because that path publishes no epoch and has none to propose, and an absent key is what phase 2's SET NX expects. The cost is one extra buffer drop if a phase-1 publisher deletes an epoch a flipped publisher just minted during the phase-2 roll -- loud and bounded, which is the direction this family always chooses over a silent merge. THE THREE CLAIMS. - "Rolling back is symmetric: unset the variable and roll" was true only of the roll back to PHASE 1. Downgrading past it is a second step in reverse order, because a pre-phase-1 binary still cannot parse the prefix, and introducing one while any flipped instance publishes drops events on it. - Unsetting the environment variable is not the same as setting the value false: events_publish_epoch can come from config.toml, whose value stands when the variable is absent. - counter_backward was documented as expected during mixed-version rolls and near zero between them. On phase 1 it can be non-zero at any time: that path keeps the two-call INCR-then-PUBLISH, so instances can interleave. The expectation is now stated per phase. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
a9544a57ba |
test(events): repair the resume tests the base guard made vacuous (BUG-2736)
Codex round 1 named three sites; the class was six, across four packages. Every MemoryBus test that spelled out a cursor as a small literal now passes through the incarnation guard before reaching the branch it is named for. The worst were the two that exist precisely to distinguish branches: the both-ways-of-not-serving observer test would have gone green with BOTH of its branches deleted, and the watch bus's eviction test would have gone green with eviction deleted. Cursors are now base-relative or read back from what the bus issued. Where the test has only the EventBus interface and no access to the base, the cursor is derived from a published event's id instead. Mutation-checked in the direction that matters: with the no-buffer branch, the coverage check, and the eviction check each made inert in turn, the tests named for them fail. Before this commit they did not. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
4a6a748c85 |
feat(events): identify the shared Redis ID space, behind a two-phase flip (BUG-2736)
The activity event counter lives in Redis and is shared by every instance, so no instance can compute an identity for it the way MemoryBus computes its own incarnation base. If that counter is ever reset -- evicted under maxmemory, deleted by hand, a fresh Redis after a restore -- IDs start again from 1, and a replica buffering the old sequence cannot tell the new 101 from the old 101. It merges two ID spaces into one replay buffer and answers a resume across the boundary as though nothing was missed. Numeric detection alone cannot see it. By the time the new sequence passes the replica's high-water mark it looks like ordinary progress -- which is the case the epoch exists for, and the high-water check is what catches the OTHER case (a publisher that never learned the epoch), so both are kept. So the identity travels WITH each message, as an opaque token in a "<epoch>|<id>|<json>" prefix. A prefix rather than an envelope field: an older instance would unmarshal an envelope object SILENTLY -- no matching keys, no error, a zero-valued Event delivered to its clients -- and fails loudly on the prefix instead. TWO PHASES, because the failure is asymmetric. Every instance ACCEPTS both wire forms from this release; only emission is gated, on PAD_EVENTS_PUBLISH_EPOCH. Phase 1 rolls the binary everywhere publishing the historical bare JSON; phase 2 sets the flag and rolls again. Flipping before every instance is upgraded is the one direction that LOSES events rather than resyncing: a pre-phase-1 binary cannot parse the prefix at all. Rollback is symmetric and safe. docs/deployment.md carries the procedure both ways, what the reset counters should read during each roll, and what remains unfixed. Phase 2 also moves ID assignment into one atomic script. The two-call INCR-then-PUBLISH lets two instances interleave, so a receiving instance can append 6 before 5 -- a window older than this change, and already wrong, but load-bearing here because counter-backwards detection reads a descending ID as a reset. The script carries a dedupe token for the same reason internal/watchevents' does: go-redis retries a command whose REPLY was lost, so a publish can happen AND return an error, and the retry would deliver a second copy that looks perfectly valid. THE COUNTER-BACKWARDS FLOOR STAYS, and the earlier hope that this unit would delete it was wrong. Its trigger is mixed-VERSION ordering -- an older binary assigning and publishing in two calls -- not mixed-FORMAT payloads, so publish-old-until-flip removes the format window only. It lives for as long as a deployment can run two publisher versions at once, which is every rolling upgrade, and the code now says so where it fires. THE ASYMMETRY WITH MemoryBus IS DECLARED IN BOTH BUSES, in both packages: an opaque epoch where the counter is shared, a numeric base where one process owns it. They are not two spellings of one idea and must not be symmetrized. A numeric base for Redis would close more -- it would refuse cross-incarnation cursors, which the epoch cannot -- and is deferred rather than rejected: at the flip, IDs would jump to ~1.8e18 in one step and every un-flipped publisher's message would read as a massive backwards jump, dropping every buffer across the whole roll. It is a candidate follow-on once the flip has soaked. What this does NOT fix is stated in the code and the docs rather than implied: the client cursor is still a bare integer with no epoch, so an old and a new ID of the same value remain indistinguishable TO A RESUME even though the buffers can no longer mix them. The flip is read inside newObservedEventBus, which now takes the whole Config. As a hand-picked argument at the two RunE call sites it was untested wiring: replacing it with `false` compiled, passed the entire tree, and left the deployment silently on phase 1 -- indistinguishable from a correct phase-1 deployment, since phase 1 is the default. Mutation-checked in both directions, because a helper that ignores its config and hardcodes either value would pass a one-directional test. Also: the epoch and dedupe keys join the namespace assertions (an epoch shared between two installations is a cross-feed with teeth -- each would read the other's ID-space changes as its own), and this package's four-key EVAL is now recorded on BUG-2724's cluster deferral, which had one call site and now has two. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
c017ad359d |
fix(events): give each in-memory bus incarnation its own ID space (BUG-2736)
Both in-process buses assigned Last-Event-ID values from a counter that restarted at 1 on every process start. A client holding cursor 2 from a previous incarnation could reconnect to a restarted server, pass every coverage check BUG-2731 added, and be replayed the NEW space's 3, 4, 5 as though they followed the OLD space's 2 -- silently missing everything the dead space held above 2. Nothing local could tell the two 2s apart. The cursor carries no epoch, and in internal/events per-workspace IDs are non-consecutive by construction, so "did we issue this ID?" was numerically undecidable. The four adjacent levers were checked rather than assumed: comparing in memory has nothing to compare against; persisting the counter makes single-process Pad carry durable event-bus state and still resets on data loss; refusing cursors we did not issue is the undecidable one; and a nonce on a second channel is unavailable because EventSource echoes Last-Event-ID and nothing else, and cannot rewrite its URL on an automatic reconnect. So the ID space's identity goes in the ID's VALUE while its FORMAT is unchanged: still a bare int64, still ParseInt on the way back. internal/idspace mints a base of processStartUnixMilli<<20 and each bus counts up from it. Two incarnations can only collide if the earlier process published more than 2^20 events per millisecond of its own lifetime -- a deterministic bound, not the probabilistic one BUG-2736's body rules out. A CAS makes bases strictly increasing within a process too, which the clock alone does not do for two buses constructed in the same millisecond. A backwards clock step degrades in the SAFE direction: a lower base puts old cursors ABOVE the new buffer's newest ID, so they are refused rather than answered wrongly. The overflow bound is computed, not estimated: the last start instant that fits is 2248-09-26T15:10:22Z. Each bus then answers the resume question exactly instead of inferring it: a non-zero cursor at or below this incarnation's base was issued by a dead space. That is strictly stronger than the coverage check alone, which serves the ADJACENT cursor on reasoning that only holds within one ID space. In internal/watchevents the check lives in one helper both entry points call. Written inline in EventsSince it was absent from SubscribeAndReplaySince -- the path the SSE handler actually uses -- so the component was fixed and its wiring was not (team CONVE-19). A test now drives both. web's ItemEvent no longer declares `id?: number`. Nothing read it, which is the only reason it was harmless; a base of ~1.8e18 is past JavaScript's MAX_SAFE_INTEGER, so the first reader would have silently got a rounded number. Defused while still unread. Tests that spelled out IDs now read back what the bus assigned -- a literal 1 is a cursor from a dead space, which turned two negative controls into their own opposite. The two watchevents guards (cold buffer, dead incarnation) are tested separately, because a single test covering both would keep passing with either deleted. The Redis half is not here. Its counter is shared across processes, so identifying its ID space needs an epoch travelling with each message; that is the next commit on this branch. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
bc422d0e31 |
Merge pull request #1177 from PerpetualSoftware/fix/events-resume-coverage
fix(events): a resume must not be answered from coverage we never had (BUG-2731) |
||
|
xarmian
|
86b0f7508f |
docs(server): name the subscribe-then-replay window where it lives (BUG-2730)
Codex round 18 was asked to assume exactly one defect survived sixteen rounds and to find it rather than survey. What it returned is the subscribe-then-replay duplicate window — a REAL defect, and one already filed on BUG-2730 by two earlier rounds. That it went that deep and surfaced a known residual rather than a new defect is the useful result. But the code said nothing at the site, so a successor reading handleSSE would have to re-derive it, exactly as three review rounds did. Now stated where it happens: the window, what it costs (a duplicate toast and duplicate work, never a lost event), how internal/watchevents closed the same window with SubscribeAndReplaySince, and why closing it here is its own unit — a new method on events.EventBus across three implementations, folded together with the admission check SubscribeIfAllowed already performs. That is a change about DELIVERY, and this one is about COVERAGE. No behaviour change. Refs BUG-2730, BUG-2731 |
||
|
xarmian
|
f2a037e393 |
docs: nine claims about other people's code that I had not checked (BUG-2731)
Codex round 16, aimed at every factual assertion this diff makes about
code OUTSIDE it — go-redis, the SSE spec, HTTP header handling, the web
client, internal/watchevents, Prometheus. The angle was chosen because
this diff had already been caught twice asserting library behaviour that
was false, and claims about other people's code are the one class no test
in this repo can falsify.
It found nine. Every one is mine, and every one claimed more than I had
verified.
- "no reconnect in 24 seconds of probing" cited an experiment that is
not in the tree — the probe was deleted with the test it belonged to.
The MECHANISM is checkable from the library source and now says so
with the call named; the unretained number is gone.
- "the SSE `id:` field has no room for an ID-space identity" is wrong.
The spec allows an arbitrary UTF-8 event ID. What excludes it is PAD's
own contract — an int64 every deployed client already parses — which
is a stronger and more honest statement of the constraint, and it is
the one BUG-2736 has to argue against.
- "the spec defines an empty header as no position" overstated it. The
spec governs what a client SENDS. What a server does with a value it
cannot use is our policy, and the test now says so.
- "HTTP strips optional whitespace from header values" is too broad: Go
trims on the way OUT, while the incoming MIME parser only TrimLefts.
What I measured was the round trip, and the comment now claims exactly
that.
- "every gap is a full resync / full re-fetch" is wrong in three places.
The web client answers sync_required with an incremental /changes
delta and only falls back to a full refresh after a long absence or a
failure. This one matters beyond wording: the load argument for the
whole fix rests on what a gap costs a client.
- "a wrapper cannot see that a resume gap occurred" — it can see the nil;
what it cannot see is WHY. I had already corrected this in the metrics
adapter and left the overbroad version in the seam it describes.
- internal/watchevents' `since` no longer "mirrors internal/events
exactly" — that stopped being true when knownFrom went into the
latter's `since`. Now states where the two differ and why.
- "the counter returns to baseline" — a Prometheus counter only
increases; its RATE returns to baseline. Two places.
- "the only case where INCR fails while PUBLISH still reaches
subscribers" — an ACL permitting one and denying the other is another.
The test now names the SHAPE as what matters and its arrangement as
one route to it.
No behaviour changes; comments, docs and test prose only.
Separately verified while waiting on this round, and now cited rather than
asserted: the three WHATWG steps that make the empty `id:` cursor
retirement work. That claim was the one thing in the diff I had taken from
memory of a spec rather than read, and it is load-bearing — if wrong, the
feature is theatre.
Refs BUG-2731
|
||
|
xarmian
|
3c6b412b0f |
refactor(events): let go-redis own the redial, and record what neither form detects (BUG-2731)
Codex round 15 raised a P1 against the reconnect handling: a bare pubsub.Receive loop does not start go-redis's health check, which only the Channel* constructors do, so a HALF-OPEN connection — no FIN, no RST, just a route that stopped working — blocks forever with no error, no retry and no coverage drop. That is the exact failure the reconnect handling exists to prevent, reintroduced one layer down by the mechanism meant to fix it. The diagnosis is right. The remedy is not, and the difference is measured rather than argued. PubSub.Ping ONLY WRITES the command — writeCmd, then return; it never reads a reply (go-redis v9.22.0, pubsub.go). So the health check's pingErr is nil for as long as the socket accepts writes, which a half-open socket does until its send buffer fills, and the channel path sets no read deadline. Probed with a TCP proxy that silently stopped forwarding, with ChannelWithSubscriptions in use and the health check running: NO RECONNECT IN 24 SECONDS. A test asserting detection was written, failed, and was deleted rather than left in a shape that cannot pass. So this switches to ChannelWithSubscriptions on the grounds that survive the measurement — it is the supported API, go-redis owns the redial and backoff, and a hand-rolled retry loop is machinery this package does not need to maintain — while the function's comment now states plainly that NEITHER form detects a half-open connection, with the mechanism and the probe result, so nobody re-derives the health check as a solution. The residual is filed on BUG-2730, which already owns "this bus is under- delivering and cannot say so", together with what would actually work (application-level idle tracking) and why it is a decision rather than a patch: it needs a threshold, and too low a threshold resyncs quiet workspaces for no reason — the load-posture inversion this family keeps having to avoid. Mutation: removing the coverage drop on resubscription fails two tests, so the detection still discriminates through the new API. Refs BUG-2731, BUG-2730 |