mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-22 02:23:46 +00:00
d4e7be4a246378e3631efaa83163a62f7431df2b
1531 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
d4e7be4a24 |
feat(web): Live view on the activity page — the feed folded into episodes (IDEA-2755)
An episode is a run of consecutive events by one actor on one item, split on a 30m gap: audit-grain rows become work-grain cards. The Live/Audit toggle persists per browser; server HTML and the hydration pass both render the 'live' default and the stored choice applies in onMount, strictly after hydration. Liveness is claimed only from event age. Live cards enrich with the newest comment's first line (best-effort, first four only, no polling) — the trail's checkpoint discipline is what makes that line worth showing. Seat identity: the fold reads metadata.agent (the X-Pad-Agent stamp); generic client ids render as 'agent', and a seat that sends its own name lights up its label with no further change — concept B's lanes want exactly that. Design canvas and decision record on IDEA-2755. Review loop: 4 rounds, 5 findings fixed (wire-contract phantom, agent metadata field, Node-25 localStorage guard, hydration mismatch, cross-type fixture bleed). Claude-Session: https://claude.ai/code/session_01HvAuiZ7JaWyCqqyV99LyWt |
||
|
xarmian
|
ad0deacb43 |
fix(web): Activity's item_id was a phantom — the wire field is document_id
internal/models/activity.go serializes the referenced item's UUID as document_id (the audit trail predates the document→item rename); the TS Activity type declared item_id, which no server response ever carries. Nothing read it until the episode fold tried to — its primary key never fired and ref-less rows would have folded into one workspace episode. The timeline test fixture carried the same phantom field, internally consistent with the type and unlike any real payload. Note the deliberate asymmetry: Comment's wire field IS item_id (models/comment.go) — the two types genuinely differ, which is exactly how the phantom survived review. Claude-Session: https://claude.ai/code/session_01HvAuiZ7JaWyCqqyV99LyWt |
||
|
xarmian
|
5003718802 |
fix(push): apply delivery's visibility gate to delivered_sessions (BUG-2725) (#1187)
deliveredSessionCount applied three of watchNotificationVisible's four gates, missing the first thing delivery checks: vis.allows(CollectionID, ItemID). Broadcast over-reported. Targeted was worse — the publish-skip reads this count, so the gate passed, the push went out, the stream dropped it on visibility, and the response said delivered_sessions: 1. An instruction lost behind a success. Per Dave's day-49 ruling, visibility is RE-RESOLVED at push time rather than snapshotted: membership and grants are revocable, so a value cached at connect goes wrong exactly when revocation is what makes it matter. The one input that cannot be re-resolved is the target connection's auth transport — computeWatchAccessVisibility consults isBearerAuth exactly once, inside the admin bypass, and the pushing request only knows its own. So SessionOrigin.BearerAuth is recorded at Add(). That is NOT the snapshot the ruling rejected: auth transport is a property of the connection, fixed when it opened and not revocable while held, so it cannot go stale. Armed is the precedent. SessionOrigin is kept separate from SessionIdentity because that type documents itself as self-declared and never verified; folding a server-derived security fact in there would silently retract the warning for one field. Both comments state the rule for future extenders: connection properties are admissible, derived authorization state never is. computeWatchAccessVisibility now takes a bool instead of an *http.Request, which makes the per-connection input visible in the signature and lets the count answer for a connection it is not serving. COST: "re-resolve per counted session" reads like N access checks per push. It is at most TWO, and sessionVisibility's memo makes that true by construction rather than by careful calling — every other input is per-user and identical across the sessions counted, so one varying boolean bounds the answers at two. Pinned by a test with 50 sessions. Codex round 1 (P1): the first version swallowed store errors into "not visible", reintroducing BUG-2698 through this fix — a targeted push reporting 0 SKIPS the publish, so a DB blip would drop the instruction and answer 200, in a function whose own doc comment says why 0 is load-bearing. Round 2 (P1): the same class one layer down — computeWatchAccessVisibility collapsed FOUR store failures into a denial, two discarded into underscores. Fixed as a class per CONVE-18. Resolution and policy are now separate: stream-side callers discard the error explicitly with reasons, only the counting caller propagates. Round 3 CLEAN. CONVE-23 sweep found three consumer-facing artifacts still describing the old mechanism, none on a line this diff touched: the plugin skill doc, the web push dialog, and pad push --help. All three corrected to name what actually remains rather than deleting the caveat. Plugin 0.3.1 -> 0.3.2, since installed plugins are version-pinned at install. NOT fixed, deliberately: the UNDER-count. A stream past maxSessionsPerUser receives broadcasts while never entering the registry. delivered_sessions remains an estimate with error in both directions, and every consumer-facing description now says so. Two coverage gaps recorded rather than rounded off: mutation M11 survives (the reporting test reaches only the first of four store calls, because closing the DB fails it first), and no test drives the whole chain store-fault-to-503 (the DB-close instrument kills the request earlier, so such a test would have gone green against the wrong 500 — deleted rather than relaxed). Also lands the BUG-2752 refutation sentinel: that item claimed the OAuth workspace allow-list went unenforced on /api/v1/events/stream. Refuted — no allow-list-bearing credential can authenticate to /api/v1/* at all. The test guards that format gate, so if it ever widens, the refutation's premise fails loudly instead of silently reopening a leak. Gates on the merged tip: make test 27 pkgs, make lint 0 issues, full Postgres suite 27 pkgs, govulncheck, codex CLEAN, CI 7/7. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86 |
||
|
xarmian
|
72336aacb5 |
fix(events): release SSE admission slots when a client leaves mid-establishment (BUG-2749) (#1186)
`GET /api/v1/events` reserved its admission slot, then blocked in `SubscribeAndReplaySince` while the workspace's Redis subscription was dialled and — since BUG-2747 — acknowledged. Nothing propagated the request's cancellation into that wait, so a client that disconnected during establishment left a process-wide slot, a per-principal slot and a per-workspace slot held for the whole of it. The connection was gone; the capacity was not. Cancellation is now DEREGISTRATION, and `wsCounts` — which already answers "is anyone still here" — decides everything downstream. No ownership hand-off and no reaper: the arbiter already existed. (One thing IS handed off, and only one — the remainder of the confirmation wait; see below.) The two cancellation positions take different paths, and only one of them owes the joiners anything: - Before the install: the existing post-dial critical section already abandons and retires correctly when nobody is left. It needed one ordering rule — the departed establisher stops being counted IN THAT SAME SECTION, before the count is read. If joiners registered while we dialled, the count is still non-zero and they get the subscription; that is the hand-off the filing asked about, expressed as a count rather than a transfer of ownership. - During the confirmation wait: the subscription is already installed with its receive loop running, so the connection is not at risk — but the WAIT is what releases the joiners, and dropping it would admit them into a subscription Redis has not acknowledged while telling them nothing. That is BUG-2747's defect re-created at the seam between the two designs. So the remainder of the wait moves to a goroutine that finishes exactly as the caller would have: same arms, same `markUnconfirmedAdmission` on the bound, same `finishPending`. Bounded by `confirmTimeout`; no reaper needed, because teardown stays count-driven. A departure is not a refusal. `ok bool` is replaced by a `SubscribeOutcome` enum across the three `EventBus` Subscribe methods, so `SubscribeWorkspaceLimit` and `SubscribeCancelled` cannot be collapsed: answering a departed client with 429 would have written a limit refusal into the logs and counters that anyone would use to tune that limit. An enum rather than a second bool or an error because the switch has to name the case — by construction rather than by argument. Caller population, with its search boundary: 3 production implementations (events.MemoryBus, events.RedisBus, metrics.InstrumentedBus), 1 test double (server.gapEventBus, which embeds the interface), 2 production call sites (both in handlers_events.go). Searched this repo four ways — the three method names, `.Subscribe(`, method declarations, and interface embedding. collab.OpBus and watchevents.Bus are different interfaces and are out of scope; no other repo links this package. WHAT THIS DOES NOT FIX, verified in go-redis v9.22.0 rather than inferred from its doc comment (which says Subscribe "does not wait on a response from Redis" and so reads as though no dial happens on the request path — it does; only the reply is unawaited). On plaintext, dialConn derives its per-attempt deadline from the caller's context and the default dialer is net.Dialer.DialContext, so cancellation aborts the dial. Under TLS the same dialer calls tls.DialWithDialer, which takes no context, so the dial stays bounded by DialTimeout alone. On a TLS deployment this shrinks the held slot from (dial + confirm bound) to (dial), not to zero. Review round 2 (codex) found a P1 in this unit's own first draft, of exactly the shape the filing warned about. A cancellation check at the top of the establish loop could return while the caller still OWNED an unretired establishment record: section 1 had already named it the establisher, so the record stayed in pendingSubs with nobody behind it, its done channel never closed. The next subscriber for that workspace would join it and wait forever — and its own registration keeps wsCounts non-zero, so no later caller would establish either. A permanently dead stream that looks alive, produced by a guard whose only purpose was to save a dial. The guard is gone: a cancelled caller now goes THROUGH establishSubscription, which is the only code that knows how to put the record down. Regression test included, and reinstating the guard turns it red. Round 2 also found a P2 shutdown regression: routing the dial to the caller's context alone took away Close()'s ability to interrupt a stalled dial, which it had before. The dial now runs on a context ended by EITHER the caller or the bus, and each half is pinned by its own test — dropping either one is detected. Review round 1 (codex): no P1. One nit fixed as a class — three comments elsewhere in the file asserted the dial was "NOT bounded by the context we pass", which this change falsified; the sweep found and corrected all three (establishSubscription, defaultSubscribeConfirmTimeout, Subscribe). The TLS half of its P2 is filed as BUG-2754: the fix belongs at client construction, where it covers every Redis call rather than this one. Class sweep filed separately as BUG-2751 (lead-ruled: one region, one design per diff): internal/watchevents has no per-request establishment, but its resume path blocks on a 250ms settle window bound to the bus's context rather than the request's, while /api/v1/events/stream holds the same admission slots across it. Tests: five cancellation cases in internal/events (before install, during the wait alone, during the wait with a joiner, a cancelled joiner, an already-dead caller), a dial-binding assertion, and the handler-level binding in internal/server asserting the admission slot itself is released — the half of the bug that does not live in the bus. Mutation matrix, 8 mutations: 7 detected, each by the test named for it. The one survivor is the ctx term in the retry re-decide, and it survives because it is an OPTIMISATION rather than a correctness guard — a departed caller that mints a second record still establishes, deregisters and retires correctly; the term only saves a pointless dial. The code says so rather than implying the guard is load-bearing. The earlier draft's entry guard and loop-top break formed a redundant pair the matrix could only detect when both were removed. That redundancy was the smell, and round 2 found the substance under it: one of the two was not redundant, it was wrong. With it gone the entry guard is detected on its own. |
||
|
xarmian
|
692b3e1a84 |
fix(store): erase a deleted account's user id from frozen outbox payloads (TASK-2719) (#1185)
* fix(store): erase a deleted account's user id from frozen outbox payloads DeleteAccountAtomic's de-identify posture reached only LIVE rows; outbox payloads froze user ids at emit time, so a deleted user's id stayed legible in undispatched and dispatched-retained rows — in workspaces they didn't own — until TASK-2714's retention window closed on the row. Dave's ruling on TASK-2719: 'delete my account' means prompt erasure, not a bounded window. ScrubOutboxUserRefsTx runs inside the deletion transaction: a key-scoped, value-matched recursive rewrite (assigned_user_id / user_id / uploaded_by, any depth — covers item_batch member nesting) plus a value-equality NULL of the subject_id column, whose only user-valued rows are member events. Scrub uniformly, delete nothing (lead ruling): erasure is this pass's job, row lifecycle stays with retention. A scrubbed member row degrades to a parseable resync signal — verified against the drain (opaque bytes) and memberEventPayload (absent user_id unmarshals to ""), so SPEC-3's tombstone branch is not needed. Population per CONVE-18: five payload families enumerated at outboxUserRefKeys; boundary stated (fields-blob interiors not entered, matching the live-row posture). scrubItemPII's emit-time keep of assigned_user_id is now SUPERSEDED at deletion time — both comments name the winner. Rewrite is Go, not SQL (dual-dialect: payload is JSONB on PG, TEXT on SQLite; the row-finding LIKE casts for the same reason). Read-fully-then- write on the one transaction (BUG-2409 shape). json.Number preserves numeric literals across the rewrite. TASK-2719 Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86 * fix(store): CAS the outbox scrub rewrite; pin key-scoping and number safety Codex round 1 on TASK-2719. The rewrite is now a compare-and-swap conditioned on the payload we read: two concurrent account deletions can hold one bulk payload (naming both users) on Postgres READ COMMITTED, and the later blind write would reintroduce the earlier deletion's id from its stale copy. Zero rows matched means re-read and redo against the fresh bytes; SQLite's single writer never takes the path; bounded loudly at 5. Documented rather than fixed, with the mechanics: the residual concurrent-emit window (FK KEY SHARE serializes every path that would CREATE a reference to the dying user; what survives is a re-freeze of an existing one, e.g. a title update on a still-assigned item, bounded by TASK-2714 retention — closing it needs a table lock on a once-per-account path), and the two prefilter invariants (newID() uuids carry no LIKE metacharacters and nothing JSON escapes; writeOutboxTx's json.Valid gate makes multi-value payloads unrepresentable). New tests pin what the existing set couldn't fail on: a decoy field whose VALUE is the deleted id under a non-target key survives (key-scoping), a 2^53+1 seq literal crosses the rewrite verbatim (json.Number), and the CAS retry leg is driven deterministically with a stale payload copy, asserting the stored bytes win and the stale copy's ids are not reintroduced. TASK-2719 Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86 * docs(store): state the outbox-scrub residual window honestly Codex round 2: the round-1 note overclaimed. On Postgres the escape is not just the re-freeze case — a KEY SHARE acquired before the deletion reaches DELETE FROM users means the DELETION waits and the emit commits first, and attachments.uploaded_by has no FK at all (migrations 047/026), so post- commit emits are not structurally clean either. All three paths stay retention-bounded; the comment now enumerates them instead of asserting cleanliness. TASK-2719 Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86 |
||
|
xarmian
|
308ed994b0 |
Merge pull request #1184 from PerpetualSoftware/fix/subscribe-confirmation-window
fix(events): await Redis registration before admitting an SSE subscriber (BUG-2747, BUG-2748) |
||
|
xarmian
|
cbf2dd29c8 |
test(events,metrics): assert exactly-once, cover the new counter, drop an overclaim
Codex round 7. The confirmation bound's comment said it bounds establishment. It does not: the dial and go-redis's HELLO/AUTH handshake run inside client.Subscribe before the timer starts and are bounded by the CLIENT's DialTimeout instead, so the worst case composes to roughly DialTimeout plus this. Anyone reasoning about connect latency needs both numbers. The concurrency test asserted topology — subscriber count and pendingSubs — while reading one event per channel, so a duplicate from a second establishment could sit in the channel undetected. It now asserts exactly-once, which is the behaviour the topology was standing in for. The new Observer method, counter and deployment contract had no adapter test. Added, including the half that matters: the count must NOT also land on the reset series, since an adapter that merged them would pass a total-only assertion while destroying the distinction an operator acts on. And the abandonment test now says out loud that it fabricates its state, because the establishing caller is blocked inside Subscribe and nobody can unsubscribe it — the same reason the retry it exercises is defence in depth rather than a reachable path. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
4d84453f41 |
fix(events): apply the registration bound to the buffer, before the cursor filter
Codex round 5. The P1 is the same two-spaces mistake the ID-valued ceiling made, committed again inside the fix for it: asking since() for the events above the cursor and then dropping the last (appends - mark) of them mixes a filtered list with an unfiltered count. A post-registration straggler whose id falls at or below the cursor is absent from the slice but still counted in the drop, so the count eats a legitimate pre-registration event instead. Pre-mark [5 30 20], post-mark [6 40], cursor 10 handed the caller [30] and lost 20. replayBuffer.sinceBounded applies the window to the BUFFER and shares every coverage rule with since(), which now delegates to it, so the two cannot disagree about what cannot-vouch means. This also closes the residual round 3 left accepted: if the wait's appends evict everything the buffer held at registration, keep goes to zero and the span is refused rather than partially served. Three overclaiming comments corrected — the wait is bounded, and saying Subscribe returns only once Redis has acknowledged is false on the timeout path. And the ceiling test's own claim: it appends straight to the buffer, so it pins the boundary arithmetic and says nothing about replay-XOR-channel, which is a different test. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
f5ca67cbca |
perf(events): shorten the subscribe-confirmation bound to a measured 1s
Lead ruling on codex round 3's second finding: file the ctx-plumbing as its own unit (BUG-2749), bound the exposure here. The bound is not a guess at how fast Redis is. Establishment either completes in single-digit milliseconds or does not complete at all, so past the top of the fast mode waiting longer buys nothing and only holds an SSE admission slot, global and per-workspace, for a client that may already be gone. Measured on a containerised Redis over loopback, 300 establishments, timing the whole of Subscribe: p50 388us / p99 679us / max 1.73ms idle; p50 693us / p90 5.1ms / p99 12.1ms / max 18.5ms under 24 busy loops on 8 cores. One second is ~54x the loaded maximum. Being too short costs an admission whose coverage this instance cannot describe — counted, logged, and reconciled to the client when the acknowledgement lands. Waiting is the silent direction. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
650d9df270 |
fix(events): bound the replay by append POSITION and buffer identity
Codex round 3. Both P1s real. The ceiling used lastAppendedID as if it were a time boundary. This bus's ids come from a counter shared across workspaces and a phase-1 publish assigns and publishes in two calls, so arrival order and numeric order genuinely disagree. Against an id-valued bound both directions break at once: a straggler arriving after registration is replayed although it also went to the caller's channel, and a pre-registration event carrying a higher id is filtered out and never replayed at all. replayBuffer now counts its appends, and the bound is a position — the entries to withhold are simply the final (appends - mark) of whatever since() returned, which may trim from the front but never the back. The mark also carries the BUFFER, not just a position in it. An ID-space reset during the wait replaces the buffer wholesale; a position in the old one describes nothing in the new one, and knownFrom may still accept an adjacent cursor, so the mismatch does not announce itself. Also corrected, all found by the same round and all mine: the Observer comment claimed this counter never reaches SequenceReset, which the late-confirmation path contradicts; the reason enumeration in metrics.go, its Help string and docs/deployment.md were never updated for the sixth reason; and both the metric and its comment said every increment is a client when it is one establishment however many subscribers were waiting. Accepted, not fixed: since() evaluates eviction over the whole buffer including post-registration appends, so a flood inside the wait can evict a cursor that missed nothing and force a sync_required. It costs a spurious resync, never silent loss, which is the direction this family chooses every time. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
cadf0fab33 |
docs(events): name the joiner retry as defence in depth, and make it loud
The mutation matrix could not reach it: no mutation of the surrounding code makes a test take that path, because the same-lock retire in the abandon path makes the strand unreachable rather than recoverable. A joiner increments wsCounts under b.mu before the establisher's count check reads it, so a registered joiner prevents the abandon; a joiner arriving after the check cannot find the record, because it is gone in that same section. That is an argument, not a measurement, so the retry stays — a permanently dead stream that looks alive is worth one wasted pass in a case that should never happen — but it now logs when it fires, so a wrong argument surfaces in production instead of limping silently. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
1f4ab9b549 |
fix(events): abandon and teardown must not strand a joiner or leak a PubSub
Codex round 2, lifecycle angle. Both findings real, plus one my own fix introduced. P1 — Close cancels the context before it takes the lock and drains wsSubs, so an establishment that locked afterwards installed into a map Close had already emptied. Its receive loop exits on the cancelled context and neither subCancel nor pubsub.Close ever runs: the PubSub and its health-check goroutine outlive the bus. establishSubscription now refuses to install into a closing bus, and Close clears wsCounts alongside the subscribers it counts, so the two structures cannot disagree. P1 — abandoning because the workspace emptied retired the establishment record in a separate critical section from the decision. A subscriber arriving in between registered, waited on a promise nobody would keep, and returned with a channel wired to nothing — permanently, since its own registration keeps wsCounts non-zero so no later caller establishes either. The record is now retired under the same lock as the decision, and a joiner verifies a live subscription afterwards rather than assuming one, taking the establishment over once if there is none. And one I introduced writing that: the re-check created a pending record on the loop's final pass with nobody left to establish behind it, which is a worse version of the same defect. Records are now created only at the top of an iteration that will use them. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
c719bf44b1 |
test(events): make the confirm-versus-timer race deterministic
The repetition version caught the mutation that removes the confirmClosed re-check in 0 of 10 runs at 500 establishments each: a near-zero bound makes the timer win outright far more often than it ties, and winning outright is the ordinary timeout path, not the race. A one-in-ten detector reads as coverage and is not. beforeUnconfirmedMark holds the mark until the acknowledgement has landed, reproducing the interleave every time. 10 of 10 against the same mutation. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
703e746922 |
fix(events): bound the replay at registration instead of withholding fan-out
Codex round 1, three findings, all real. P1 — a subscriber arriving mid-establishment was admitted immediately. establishSubscription installs wsSubs and only THEN waits for the acknowledgement, so for that interval the workspace looks live and is not. Reading wsSubs first let a second subscriber straight into the unconfirmed window this change exists to close. pendingSubs is now checked first. P1 — withholding fan-out from a not-yet-admitted subscriber dropped events for the very population BUG-2747 is about. A fresh subscriber (sinceID == 0) reads no replay at all, so an event skipped on the theory that the replay would carry it was skipped and then never replayed. Replaced with a replay CEILING captured at registration: the subscriber is live in fan-out from the moment it registers and receives everything after that on its channel, while its replay is bounded above by what the buffer held then. That is the same division the single critical section gave for free, generalised to the case where a wait separates the two halves. P2 — the confirm timer could set unconfirmedAdmitted after the acknowledgement had already cleared it, leaving a subscriber counted as unconfirmed and never told to reconcile. markUnconfirmedAdmission now checks confirmClosed under the lock that closes it. Two tests added for the two P1s. The double-delivery test moved from the establishing caller to a JOINER, because the establisher can never exercise it: establishing implies no live subscription, losing the last subscriber deletes the buffer, so its replay is always nil and there is nothing to duplicate. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
739f573b3b |
docs(events): the namespace helper's comment described a window this fix closed
It said the wait was covering up a production window with no remedy, pointing at BUG-2747 as where it was tracked. That is now the fixed thing, so the comment was teaching the next reader something false about the bus they were looking at. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
4148278ef6 |
test(events): drive the double-delivery window through the gap it actually opens
The first version hung the publish off afterSubscribeRegister, which now runs under b.mu in the second critical section — so the fan-out it was meant to race could not happen until the lock was released and the subscriber was already admitted. It passed against a mutation that delivers to unadmitted subscribers, which means it was not testing the flag at all. afterSubscriptionConfirmed opens at the real gap: acknowledged, so events arrive, and before the establishing caller re-acquires b.mu to read its replay. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
ad8853f0bc |
test(events): assert the widened-window premise after the defect assertion
Checked before the publish, it races the unfixed code — which returns from Subscribe while the SUBSCRIBE is still on its way into the proxy — so the test reported a broken instrument instead of the defect. Last, it can only fire on a vacuous pass, which is the thing it is for. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
548b31588b |
fix(events): await Redis registration before admitting an SSE subscriber
RedisBus.startRedisSubscription wrote the SUBSCRIBE and returned. Events Redis processed before it registered the subscription reached nobody — and since this bus has no local fan-out, that includes events published by the very instance serving the client's stream. A resuming client was already protected: a first subscription has no replay buffer, so eventsSinceLocked returns nil for sinceID > 0 and the handler emits sync_required. The silent loss reached only sinceID == 0 — the SubscribeIfAllowed path — whose buffer coverage begins at the first event that DOES arrive, leaving the hole below anything the buffer ever claimed. Subscription establishment now runs OUTSIDE b.mu (BUG-2748) and Subscribe does not return until Redis has acknowledged it. All three entry points share one body, subscribeAndReplay, so none can drift. The confirmation is signalled from inside receiveMessages rather than by a Receive placed ahead of it. That loop treats its first *redis.Subscription as the initial acknowledgement and every later one as a RESUBSCRIPTION that ends coverage (BUG-2739); consuming the first with an earlier Receive would leave it swallowing the first genuine resubscription instead. Splitting the register and the replay read into two critical sections put SubscribeAndReplaySince's replay-XOR-channel guarantee at risk, so a subscriber is now REGISTERED but not ADMITTED across the wait: fan-out appends to the buffer and skips its channel, and the replay read that follows delivers it. The flag's zero value is admitted, so MemoryBus and the already-live fast path are unaffected by construction. Failure path admits rather than refuses — every subscriber was admitted into an unconfirmed subscription before this, so refusing would be new strictness that turns a Redis blip into failed connects. Instead the span is reconciled to the client through BUG-2730's mid-stream signal when the acknowledgement lands, and counted for the operator via a new pad_event_subscription_unconfirmed_total. BUG-2747, BUG-2748 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
4b0d41c17d |
Merge pull request #1182 from PerpetualSoftware/fix/generation-key-corruption-guard
fix(events): guard the generation counter the way the epoch key beside it is guarded (BUG-2740) |
||
|
xarmian
|
86277915e1 | Merge branch 'main' into fix/generation-key-corruption-guard | ||
|
xarmian
|
5ff0cc9b59 |
Merge pull request #1183 from PerpetualSoftware/fix/events-load-fragile-tests
fix(events): stop the load-fragile tests failing for reasons that are not defects (BUG-2742) |
||
|
xarmian
|
9a8e41f8ab |
test(events): keep invariants in the comments and process history in the trail (BUG-2742, codex round 12)
Asked to critique the change for scope and restraint. Accepted: several comments carried trial-specific numbers and round labels — catch rates, CI counts, which review round corrected whom. Those belong in commit messages and on the item, which is where someone looking for the history will go; a code comment that quotes a measurement acquires a maintenance obligation nobody will honour. Trimmed to the invariant and the reason, with a pointer to BUG-2742 for the figures and, where it matters, the instruction not to trade rounds back for a bigger burst. Accepted: subscriberChanDepth's comment described a test premise before it described what the number means in production. Reversed. Declined, three, with reasons since two of them argue against earlier rounds of this same review: - That the waits on the two key-only namespace tests are unnecessary and add a failure mode. They are unnecessary for those tests' own assertions, which is why round 10 asked for them: those are the shortest Redis-bus tests in the package and therefore the ones a newcomer copies. The failure mode added is "registration never happens", which is a defect rather than a flake. - That this should be several units. It is one bug covering one family in one package, and the item asks for the family swept together rather than test-by-test. The steps are separate COMMITS, which is the separation that helps a reader. - That the compile-time premise check is over-engineered next to a runtime one. Its whole value is failing EARLIER than a run: the failure it guards is the test going silently vacuous, which a runtime check inside the vacuous test cannot reliably announce. |
||
|
xarmian
|
bfaa1d88a9 |
test(events): make the nearest example safe to copy (BUG-2742, codex round 10)
Asked whether this package now makes the correct thing easy and the flaky thing hard for someone adding a Redis-backed delivery test. It did not. The two shortest Redis-bus tests — the ones a newcomer copies — still read Subscribe, then Publish, with the registration wait afterwards or absent. Neither NEEDS the wait, because both assert on Redis keys, which a publish writes whether or not anyone is listening. But being correct for a reason particular to themselves does not stop them teaching the race to whoever copies their shape, so both now wait immediately after subscribing and say why in one line. The usage rule was also in the wrong place: waitForSubscribers' own comment described what it polls, while the warning about WHEN to call it sat on pollSubscriberCount, an unexported helper no caller has a reason to open. The rule now leads the comment on the function people actually call, along with the case that does not need it, so the exception is visible rather than inferred from a test that omits it. |
||
|
xarmian
|
7378707350 |
test(events): prove the replacement subscription is live by getting an event back (BUG-2742, codex round 9)
Round 8 left the post-reconnect publish protected only by the 2s poll above it, documented as margin by accident. Asked whether anything in the package could still redden CI, Codex went straight there: on a slow, -race or single-core runner the reconnect may still be pending after 2s, the publish is dropped, and drain times out. A real flake, and documenting it is not fixing it. The construction that works is the one the failure itself suggests. Counting subscribers cannot distinguish the stale registration from the replacement, and waiting for the count to fall can hang — both measured in round 8. But an event ARRIVING is proof that cannot be satisfied by the dead connection. So the test publishes until one comes back: instant when the subscription is already live, and incapable of passing early when it is not. The extra events some rounds publish are harmless. What the rest of the test needs is a buffer with something in it, not a buffer with exactly one thing in it, and the assertion that follows — that a reconnect WITH a buffer does report a coverage break — is unchanged. This also removes a vacuity I flagged in round 8 and had put out of scope: the 2s poll was the only thing establishing that a reconnect had happened at all, so a slower reconnect would have made the first assertion pass without ever reconnecting. The loop makes that premise explicit and fails loudly when it does not hold. |
||
|
xarmian
|
6ba8e8dd79 |
test(events): retract the reconnect wait, which guaranteed nothing (BUG-2742, codex round 8)
Two findings from reviewing the new test code as production code. The phase-two collector read `msg := <-incoming` without checking whether the channel was still open. A closed pubsub channel hands back a nil *Message, so the next line panicked the test binary with a stack instead of naming the failure — in the one goroutine whose entire job is to report what went wrong. It reports the close now. The second retracts my own round-4 change. I added waitForSubscribers before the post-reconnect publish and claimed it turned safety by accident into safety by construction. Measured, that claim is false: across the cut the subscriber count goes 1 -> 0 -> 1 over a couple of milliseconds, because the STALE registration still reads 1 until miniredis notices the closed socket. A count-based wait placed after the cut returns immediately on the dead subscription and guarantees nothing. Waiting for the 1 -> 0 transition instead is not available either: in one trial of five the count never dropped inside 3s, so that wait can hang. What actually protects that publish is the 2s poll above it, which runs to completion whenever no reset is reported — the case this test asserts. That is margin by accident, and the comment now says exactly that, with the measurement, rather than carrying a wait that reads as a guarantee. Worth its own note: that same 2s poll is how the test establishes a reconnect happened at all, so if a reconnect ever took longer the test would pass vacuously. Real, out of scope here, recorded on BUG-2742. |
||
|
xarmian
|
f6a58b6b80 |
test(events): finish the depth migration, close the go-redis bound, correct a rationale (BUG-2742, codex round 7)
Asked to judge the accumulated result rather than the sequence. The depth-constant migration was HALF DONE. I converted the two tests the previous round named and never enumerated the rest, leaving four literals in two more tests — including the fanOutLocally boundary cases, whose whole point is the event at depth+1. A partial migration is worse than none: it leaves two groups of tests disagreeing about the same number. All six sites derive from subscriberChanDepth now, and a grep for the literal comes back with only the prose that quotes the CI failures. The phase-two ordering test kept a bound I had described as unavoidable. It is not: ps.Channel takes redis.WithChannelSize, so sizing it above the message count makes truncation structurally impossible instead of merely unlikely, which is the same standard applied to the sibling test rather than a weaker one. The early reader stays, because the two defend different things — the size stops the buffer truncating the sample, starting early stops the test pretending a burst is consumed instantly. Re-measured against the two-call INCR-then-PUBLISH mutation after the change: 8 of 8, unchanged. And a rationale of mine was simply false. I wrote that the old probe-publishing helper was harmless because every call site ran before the first publish. Four of them run after one, so it really was ending coverage on live buffers. It was harmless for a thinner reason — those four go on to assert about Redis KEYS rather than about coverage — which is a property of what each caller happens to assert next, and stops holding the moment someone adds a coverage assertion after a wait. The comment says the true thing, which makes the switch to PubSubNumSub better motivated than my own argument for it was. |
||
|
xarmian
|
0d501761b9 |
test(events): derive the drop-boundary tests from the depth, and name what the waits stop testing (BUG-2742, codex round 6)
Asked what a rewrite might have silently REMOVED. Declined, on checked grounds: that the new ordering test no longer overflows the subscriber channel. That path is covered better elsewhere than the old test covered it — TestSlowConsumerDropsEvents asserts exactly a buffer's worth survives an overfill, and TestDropSignalsOnlyTheSubscriberItHappenedTo asserts the boundary itself, that filling the channel raises nothing and one more raises the flag on the slow subscriber and not the fast one. The old ordering test exercised drops incidentally and asserted nothing about them. Both of those, however, restated the depth as a literal 64 — the duplicated premise the new constant exists to remove, in the two tests whose entire claim is about the boundary AT that depth. Derived now. Declined: that the phase-two test's per-message bound went 5s to 30s and so can no longer detect a 5-30s stall. It never asserted latency, and detecting a stall that way has the same load-dependence this whole change removes. The bound exists to fail a hang. Accepted and recorded rather than fixed: the registration waits mean these tests no longer exercise the lost-publish window at all. That window is real in production, where nothing waits, and the honest place for it is a filing rather than tests that fail once in about a hundred loaded runs — a detector nobody can act on. pollSubscriberCount now says so, and points at BUG-2747, which carries the asymmetry with internal/watchevents. |
||
|
xarmian
|
6c81cf923c |
test(events): make three comments true of the code they sit on (BUG-2742, codex round 5)
Read as the next maintainer, three claims did not survive. waitForSubscribers' own doc comment still described the mechanism this branch REMOVED — that a "probe" payload is published and the resulting unmarshal error is the confirmation. The helper reads PubSubNumSub now and publishes nothing. It also said Subscribe returns before the SUBSCRIBE reaches the server; go-redis writes it synchronously and simply does not await the server's confirmation. I corrected that mechanism in my own comments one round earlier and did not carry the correction to the pre-existing comment sitting on the function I had just rewritten. "Draining concurrently keeps the buffer from ever being the constraint" was overstated: a starved reader can still fall 100 behind. Unlike the sibling test, that bound belongs to go-redis rather than to us, so it cannot be made structurally impossible the same way — the comment now says so, and says to raise ChannelSize rather than lower the assertion if it ever does flake. "Load-independent" was too strong for the ordering test. The ASSERTION path is load-independent; detection is not, since whether a broken bus interleaves on a given round is still up to the scheduler. Scoped to the half that is true, which is also the half that fixed the CI failures. Declined: that the two earlier commit messages still contain the claims later commits correct. They do, and each correction names the claim it reverses, so a git log reads as the sequence it was. Rewriting them would erase a review round finding something real, and this branch's history is meant to show that. Then, applying the correction-must-travel rule to the repo rather than to the files a reviewer named: grepping the claim string turned up the same sentence in internal/watchevents/redis_bus.go, where it introduces the FIX — that bus waits for its SUBSCRIBE to be confirmed at construction. Which retires a bug I filed earlier today claiming watchevents shared this exposure (it does not; that is why it survived the load that broke this package) and sharpens the remaining one from an open design question into a named asymmetry. See BUG-2746 (wontfix, retracted) and BUG-2747. |
||
|
xarmian
|
8597d60884 |
test(events): close the two remaining instances of both shapes (BUG-2742, codex round 4)
My own sweep found the second shape exhaustively and anchored the first on the test CI had named. Codex, asked to look for both plus a third, found one more of each. Neither reproduced in 50 invocations under the load that made the already-fixed four fail 5 of 6 rounds, so these are shape fixes rather than observed-failure fixes. TestConcurrentPhaseTwoPublishesArriveInIDOrder published all 300 messages before anything drained ps.Channel(), which go-redis bounds at ~100. That makes the arrival count a function of how much CPU the reader gets — the same defect as the sibling test, with go-redis's buffer as the bound instead of the bus's. The reader now starts before the publishes, so the buffer is never the constraint. Still one reader, so recorded order is still arrival order, and the ordering assertion is unchanged. Re-measured against the two-call INCR-then-PUBLISH mutation this test exists to catch: 8 of 8, so the restructure did not weaken it. TestAReconnectOnAnIdleWorkspaceIsNotAReset publishes after a cut, which means after a re-subscription, with no wait for the replacement registration. It is safe today only because the 2s poll above it leaves room — and that poll exists to prove a reset was NOT reported, so it exits early the moment that changes, taking the margin with it. Safety by accident becomes safety by construction for the cost of a few milliseconds. Third shape: Codex found none, agreeing with my own enumeration. |
||
|
xarmian
|
79b0f23c45 |
test(events): strengthen the ordering test and correct the registration mechanism (BUG-2742, codex rounds 2-3)
Round 2, on the rewritten ordering test: - Deriving `each` from subscriberChanDepth by integer division silently yields 0 the day the depth drops below the publisher count, making total 0 and passing `count == total` as 0 == 0. The test would go permanently vacuous with no signal. The numbers are stated and a constant declaration fails the BUILD when they stop being compatible; verified by setting each = 4 and watching vet refuse it. - The assertion checked increasing ids, which a bus that dropped some of these and delivered something else with a bigger id also satisfies. One bus, one workspace and nothing else publishing means the assigned ids are exactly base+1..base+total, so the delivered sequence must be CONTIGUOUS. Re-measured against both mutations after strengthening: 25/25 each. - Two comments claimed FEWER events than the channel is deep; total equals the depth exactly. Still safe, since the channel starts empty and every publish fits, but the stated premise was wrong. Round 3 corrects a mechanism I asserted without reading go-redis. The SUBSCRIBE is NOT sent by the receive goroutine: Client.Subscribe calls PubSub.subscribe synchronously, which writes the command. What it does not do is wait for the server to confirm it, and the publish that follows travels on a different connection — so Redis can process the publish first. Same window, same fix, but the comment now says the true thing. Declined from round 2: that the test only samples scheduler behaviour rather than forcing the interleave. That is true and is why the comment quotes a measured catch rate rather than claiming a proof. Forcing it would need a pause point in Publish, and the property under test is precisely that no such point exists. |
||
|
xarmian
|
e1f256b48c |
test(events): wait for the subscription to register before publishing (BUG-2742)
RedisBus.Subscribe returns before Redis knows about the subscription: the SUBSCRIBE command goes out from the receive goroutine, which has not necessarily been scheduled yet. RedisBus.Publish has no local fan-out -- local subscribers are served by the receive path like any other instance's -- so a publish that lands inside that window is lost OUTRIGHT. Nothing replays a pub/sub message nobody was listening for. That is why these failures survive every timeout: the observed round trip is bimodal, either tens of milliseconds or never, and no bound distinguishes the second case from a hang. Under a busy-loop load the affected shape lost about 1 arrival in 100 and lost none once the wait was in place. Four tests published immediately after subscribing and then waited for arrival. CI named three of them; the fourth, TestAMixedPhaseDeploymentDelivers- BothWays, is in the same class and had not fired yet. It needs a COUNT rather than a presence check: its claim is that two replicas see each other's events, and waiting for "a subscriber" is satisfied by whichever registers first, so waitForSubscriberCount is added for it. waitForSubscribers stops inferring registration by publishing a probe and reads the server's state instead. The probe was a trap for the next caller: "probe" is not a decodable payload, so on a bus that reads the wire prefix it is an UNDECODABLE MESSAGE -- the exact condition several tests here assert about, injected by the helper they would call to arrange themselves. It was harmless only because an undecodable message ends coverage that EXISTS and every call site happened to run before the first publish. That is a property of the call sites, not of the helper, and adding call sites is what this commit does. |
||
|
xarmian
|
63c45bc651 |
test(events): buy ordering-race detection with repetition, not a load-dependent sample (BUG-2742)
TestConcurrentPublishesDeliverInIDOrder published 320 events while a goroutine drained concurrently, then guarded against vacuity with "at least half must arrive". That guard is a bet on runner speed. The bus DELIBERATELY drops for a subscriber that cannot keep up, so the arrival count is a continuous function of how much CPU the reader gets: across four CI instances on three PRs it reported 64, 65, 144 and 150 of 320, and 64 is exactly the subscriber channel depth -- one buffer-full and nothing after it. Any "require N of 320" line is a bet on the same variable, which is why this is not a threshold change. The sample is made untruncatable instead of large. No reader runs during the publishes and fewer events are published than the channel is deep, so a drop cannot occur; the vacuity guard becomes count == total, an equality rather than a threshold. Nothing in the assertion path waits on a goroutine being scheduled, so the result no longer depends on load at all. Detection power then comes from repeating the round, which is what the bounded channel could not provide. Measured against the mutation the test exists to catch (release replayMu after the append so fan-out runs outside it): the version replaced here caught it 9 times in 30 runs; this one caught it 25 times in 25, at about 10ms against the old fixed 0.5s. subscriberChanDepth is named rather than inlined so the test states its no-drop premise in terms of the real depth. A test that hardcoded 64 next to it would silently stop proving what it claims the day the depth changed. |
||
|
xarmian
|
a04912197a |
test(events): the clamp table needs no Redis (BUG-2740)
Five subtests each stood up a bus and a miniredis to assert arithmetic. The clamp's behaviour is a pure function of an int64; the servers bought nothing and cost five of them in a package whose timing-sensitive tests are already load-fragile (BUG-2742). Split into a pure table plus ONE end-to-end case, which is what stops the table being a unit test for a function nothing calls. Zero is the case driven end to end, because it is the one that reproduced the original failure: '0' fails the epoch guard's ^[1-9] and sent the script back into the repair for the same bad seed. Mutation-verified after the split: removing the clamp and removing its floor are both still caught, the first by the wired case and the second by both. This is a footprint reduction, NOT a fix for #1182's Go leg. See the trail — that failure is BUG-2742's family and I have not established that this branch causes it. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
161e60334e |
docs(events): why per-workspace backward detection is enough for a global counter (BUG-2740, codex round 8)
Two separate review rounds have now raised the same shape: the sequence counter is shared across workspaces, but counter_backward compares only against the affected workspace's high-water mark, so a restarted counter whose reissued ids are consumed by OTHER workspaces appears to bypass detection. Refuted by probe rather than by argument, and the reasoning is now where the next reviewer will ask it. The unit of coherence is the per-workspace buffer and the per-workspace cursor: an id can only corrupt a buffer by colliding with one already in it, and an id at or below that workspace's high-water mark is precisely what this arm catches. A reissued id landing in a different workspace collides with nothing. Both shapes measured. A counter DELETED restarts at 1, which takes publishScript's id == 1 branch and rotates the epoch unconditionally — the probe reported epoch_change and a refused cursor, so this arm is never reached. A counter SET backwards above 1 skips that rotation, and then the workspace whose own ids are reissued sees them as backward and lands here, while a workspace that never held them keeps a strictly increasing stream and correctly needs no reset. No code change; this is BUG-2736's mechanism, not BUG-2740's, and it is unmodified. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
99cb56f50a |
fix(events): a broken host clock must not make the repair fatal (BUG-2740, codex round 7)
The seed is an INPUT, and nothing validated it. An unset or misconfigured host clock reports zero or a negative second; the repair then SET that at the generation key and returned it as the epoch. The epoch guard rejects anything not matching ^[1-9][0-9]*$, so it rotated, called back into the repair, received the SAME bad seed, and assigned it to the epoch WITHOUT revalidating — putting an unparseable epoch on the wire, which every receiver rejects. A total, permanent drop, reached through the mechanism written to prevent one. clampGenerationSeed forces the seed into the shape the guards accept: a positive integer of at most 17 digits. The floor is 1 rather than anything cleverer, and it gives up the ruling's property deliberately — a broken clock cannot deliver 'above any counted history' at all, so the choice is between a value that is merely LOW and one that is FATAL. A low generation is detected as epoch_regressed and costs a round of resyncs; an invalid one drops every event until a human intervenes. The ceiling exists for next_gen's reason: a value over 17 digits is not usable as a generation, so seeding one would repair the key into a state the next rotation rejects. THE FIRST VERSION OF THE TEST WAS VACUOUS IN THREE OF FIVE CASES, and only the other two exposed it. Without clearing the epoch no rotation branch fires, so the repair never ran and the second publish just reused the epoch the fixture minted — which is 1, matching three of the expected clamps exactly. The two cases that expected something else failed and gave it away. Matrix: 3 applied, 3 caught — no clamp, floor removed, ceiling removed, each killed by the cases that specifically exercise it. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
0accca7a40 |
test(events,docs): pin the detection chain a colliding repair actually relies on (BUG-2740, codex round 5)
The repair seeds from wall-clock seconds, which is above any COUNTED history and is not a monotonicity guarantee. Corrupt the key twice inside one second and both repairs seed the same value, so two genuinely different id spaces carry the identical epoch — and an equal epoch means 'same space' by design, so neither epoch_change nor epoch_regressed fires. It is still not silent, and the reason is worth pinning because it is not the one the epoch mechanism suggests: a merge needs ids REUSED at a receiver, reuse needs the sequence to go BACKWARDS, and backwards is detected whatever the epoch says. counter_backward drops the affected buffers and refuses cursors below the discarded high-water mark. That was folklore until it was measured. The test drives the whole sequence — two repairs seeding the same value, a sequence reset between them — and asserts its own premise first (the two spaces really do share an epoch), that no epoch-based reason fires, that counter_backward does, and that the old cursor is refused rather than replayed the new space's events. Mutation-verified: stop reporting counter_backward and it fails. The docs carry the chain as a quoted rule, plus the two cases that look like it and are not — a counter set FORWARD is a jump inside one space with no reuse, and a receiver that never held the colliding range experiences a gap, which is BUG-2735's pre-existing class rather than anything this introduces. Lead re-ruled on the probed fact: residual ACCEPTED because it is detected, attribution corrected from epoch_regressed to counter_backward. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
383c6dc8d8 |
docs(events,metrics): finish the epoch_regressed claim and fix the heading level (BUG-2740, codex round 5)
Three of the round's four findings, all mine. THE SAME CLAIM IN THREE PLACES, TWO OF WHICH I HAD FIXED. Round 4 updated the struct comment above the counter and the new operator section; the EXPORTED Prometheus Help string and the deployment table row still told operators that epoch_regressed means Redis lost writes. The Help string is the one a person reads at the scrape endpoint, so it was the worst of the three to leave. Third time this run that a claim lived in more places than I enumerated before editing. 'CLIENTS RESYNC ONCE' was too strong. A repaired generation that lands BELOW one a receiver already holds is discarded as a straggler for that instance's 30-second window rather than adopted, so the same space can be disclaimed again when it is finally taken up. Bounded by the window, and distinguishable because it surfaces as epoch_regressed rather than epoch_change. The new section was a ### under a ## , which adopted every following #### section — including Event ID-space migration — as its children. It is a #### now, a sibling of the sections around it. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
8c05b02c7d |
docs(events,metrics): document the repair path an operator will actually meet (BUG-2740, codex round 4)
The fix changed an operator-visible outcome and nothing said so. A corrupted generation counter used to burn a sequence ID and fail every publish forever; it now repairs, publishes, and reseeds from wall-clock seconds — which the existing prose contradicted in two places, calling the generation 'monotonic' and telling operators that epoch_regressed means a failover to a replica that lost writes. Both corrected, and a new section states the two consequences: a repair can surface as epoch_change or, if a collision had pushed the counter higher, as epoch_regressed; and clients resync ONCE, not in a loop, because the repaired key is valid and the next rotation increments it normally. THE TELL IS THE VALUE, and saying so is the honest version of a claim I very nearly shipped instead. My first draft told operators to distinguish a repair from a failover 'by the neighbouring WARN log line naming the key'. There is no such line — the repair happens inside a Lua script, which cannot log through slog and does not change the counter's label. What actually distinguishes them is that a repaired generation LOOKS like a unix timestamp, ten digits around 1.7e9, rather than a small count of ID-space resets. That is a deliberate property of the seed, and it is now what the docs point at. The section also went in BETWEEN two rows of the metrics table on the first attempt, splitting it exactly as the previous unit's rollout note did. The check I wrote after that one looked for blank lines between rows and could not see a whole section inserted between them; the check is now 'is each table one contiguous run', which catches both. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
e00b1d2a1a |
fix(events): the generation ceiling is one digit under the epoch ceiling (BUG-2740, codex round 4)
The two guards interact, and accepting 18 digits here made them fight. The value next_gen accepts is about to be INCREMENTED, and the result becomes the EPOCH — which the guard further down rejects above 18 digits. So a counter at 999999999999999999 was accepted, incremented into a 19-digit epoch, and the epoch guard fired: the script rotated a SECOND time inside one publish, found a 19-digit generation, repaired it to the wall-clock seed, and published a generation far below the one receivers hold. Measured rather than reasoned: seeded at 18 digits the published epoch came back as the seed; at 17 it came back as the ordinary increment. The ceiling for what is USABLE has to sit one digit under the ceiling for what is PUBLISHABLE, and it is now derived from that rather than chosen to match. THE FIRST VERSION OF THE TEST DID NOT CATCH IT. Reverting the ceiling to 18 passed, because 999999999999999999 lands on the seed either way — once by a single repair, once by an increment plus a second rotation. Identical end state, different mechanism, which is CONVE-12 exactly. The discriminating row is an 18-digit value whose increment STAYS 18 digits (100000000000000000): over the line at 17 and repaired, accepted at 18 and merely incremented, with no second rotation to disguise the difference. The precision test moved down with the ceiling, and its magnitude is now chosen rather than incidental: doubles are exact only to 2^53 (9007199254740992, 16 digits), so tostring divergence begins well BELOW the 17-digit ceiling — measured at 99999999999999998, which increments to 99999999999999999 in the key while tostring renders it 100000000000000000. The GET fix is load-bearing across much of the admitted range, not just at its edge. Also corrects the BUG-2744 note, which said the script's returned id is compared numerically on the Go side. It is not: this caller discards the result with .Err(), so a wrong id reaches the wire with nothing Go-side to notice. assignScript's id IS consumed, which is why the remedy spans both paths and belongs to that item. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
729abfd3b2 |
test(events): isolate the id==1 rotation branch, and record the ruling on the seed (BUG-2740, codex round 3)
The 'sequence starting at 1' case cleared the sequence AND the epoch, which is the obvious setup and does not isolate anything: with the epoch gone, the absent-epoch branch fires instead and produces an identical result, so the case passed with the id == 1 body deleted outright. It now leaves a LIVE epoch in place and asserts that as a fixture precondition — which is also the real shape, since event_seq is what gets evicted and the epoch is what survives pointing at the abandoned space. Mutation-verified: removing the branch now fails it with 'got 1'. Same lesson as N6 one round earlier, from the other side: a table that clears state uniformly only ever drives one branch, whether the uniformity hides a missing guard or a missing branch. Also records the lead's day-55 ruling against max(seed, current_epoch + 1), with the deciding reason rather than just the verdict: a repair path that reads a NEIGHBOURING shared key to compute its seed takes a dependency on that neighbour's health in exactly the state where neighbours are suspect. The residual is accepted as inside the ruling's intent — bounded to one resync, detected as epoch_regressed, never merges id spaces — and is reversible in one line. The sequence id's identical stringification hazard is filed as BUG-2744 and noted at the line, with the reachability corrected: it is reached by corruption, not by counting. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
d81f67a7a6 |
fix(events): publish the generation Redis holds, not Lua's rendering of it (BUG-2740, codex round 2)
Redis hands an integer reply to Lua as a NUMBER, and Lua 5.1 numbers are doubles printed with %.14g — so tostring() stops being faithful BELOW the 18 digits this guard admits. Measured at the boundary rather than reasoned about: a counter at 999999999999999998 increments to 999999999999999999, and tostring() renders that 1000000000000000000 while GET returns it exactly. The published epoch and the stored generation would disagree, and the receiver would adopt a generation the publisher does not hold. next_gen now reads the value back with GET. Pinned by a test at the guard's own limit, and mutation-verified: reverting to tostring fails it. 18 digits is not a magnitude a generation reaches by counting — it is the magnitude a hand-edited or collided key arrives at, which is exactly the class of event this guard exists for. The same hazard is pre-existing on the sequence id concatenated two lines later. Not touched here: ids reach that magnitude only after 1e18 events, and it is a different claim. Also documents what the wall-clock seed does NOT guarantee. 'Strictly above any increment-from-1 history' is the ruling's premise and is not the same as monotonic: a backwards clock step, or the key corrupted twice within one second, can seed at or below the generation receivers hold. Bounded two ways — consecutive repairs are already safe without the clock, since the first leaves a valid counter for the next publisher to increment; and a backwards generation is DETECTED as epoch_regressed rather than silently merging two id spaces. The stronger max(seed, epoch+1) repair is noted and deliberately not taken unilaterally, because the seed value is a ruling and that refinement only helps on the branches where the epoch is readable. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
483ac6e097 | test(events): drive all three rotation branches, not one (BUG-2740) | ||
|
xarmian
|
538b0d5ed4 |
fix(events): guard the generation counter the way the epoch key beside it is guarded (BUG-2740)
Every branch of publishScript that rotates the id space INCRs
pad:event_epoch_gen, and none checked the key first. The epoch key next to it
does, and its comment says exactly why — but the generation key never got the
same treatment.
An INCR against a corrupted key ABORTS THE SCRIPT, after the sequence INCR has
already landed. Redis is atomic against interleaving but does not roll back a
script's earlier writes, so the failure burns an id (a hole to every
receiver), repeats on the next publish, and never self-heals: the branch that
would rotate the generation is the branch that cannot run.
FOUR ABORT MODES, not the two the filing named. Measured against the pinned
miniredis rather than assumed:
list -> WRONGTYPE
hash -> WRONGTYPE
string 'abc' -> ERR value is not an integer or out of range
string '9223372036854775807' -> ERR increment or decrement would overflow
A TYPE check alone — the fix the filing describes — would have covered half of
them. The value is validated on the same terms the epoch guard uses: a
positive integer of at most 18 digits, which is what the receiver's
strconv.ParseInt can read back.
RESTART AT WALL-CLOCK SECONDS, per Dave's day-49 ruling, read from the item's
trail rather than from the dispatch message. Generations only have to be
orderable among themselves, and the corrupted key is the only witness to the
previous one, so nothing can be derived from it. Restarting at 1 would put the
new generation BELOW ones receivers have already adopted, which BUG-2736's
design reads as a regression.
The seed is passed as ARGV[3] rather than read with redis.call('TIME'), and
not for a replication reason — TIME does work here, measured. It keeps the
script deterministic and lets a test inject a fixed seed and assert the EXACT
repaired value, which is what tells 'repaired' apart from 'repaired to the
wrong thing'. Clock skew between publishers is harmless: the script is atomic,
so the first to reach a corrupted key repairs it and the rest simply
increment; no two seeds are ever compared.
Repair is SET, not DEL — SET replaces a key of any type (measured), and
BUG-2736's mutation matrix already established here that a DEL is removable
without any test noticing.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
1ff79af03f |
Merge pull request #1181 from PerpetualSoftware/fix/watch-bus-undetected-holes
fix(watchevents): detect the two holes the watch bus could not see (BUG-2739) |
||
|
xarmian
|
f4d1616078 |
docs(deployment): name the resume-boundary residual for operators too (BUG-2739)
The residual list covered the two that leave an OPEN stream stale (BUG-2735, BUG-2738) and not the one that affects RESUMES: a counter restart with the epoch intact leaves the two ID spaces overlapping, so a Last-Event-ID inside the overlap cannot be attributed to either, and a client holding an old-space cursor there can be handed new-space notifications as though they followed it. Filed as BUG-2743 during this branch's review. Named here because an operator deciding whether a counter reset is safe should see it alongside the other two, together with the thing that actually prevents it: rotating the epoch. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
47a83ccdd2 |
docs(watchevents): scope round 21's boundary claim to what it actually closes (BUG-2739, codex round 22)
A class sweep over every knownFrom assignment found that the previous commit's +1 is a strict improvement and NOT the complete fix its comment implied. replaySince serves any cursor at or above knownFrom-1, so n.ID+1 admits n.ID itself — and if the old space also reached n.ID, that cursor is still ambiguous, as is every old-space id up to the old high water mark. The epoch arm's identical +1 has the same residual, and a subsequent GAP overwrites the boundary with the ordinary knownFrom = n.ID, which can lower it back into the overlap. No constant closes this. It needs a boundary that remembers the OLD space's extent — refuse everything at or below it until the new space climbs past — which is a resume-semantics change touching the epoch path too. Filed as BUG-2743 with that design (an ambiguousUntil field guarded in replaySince rather than more arithmetic per arm, which also fixes the gap-overwrite for free), and referenced from both the code and the test so neither reads as more than it is. The honest framing, now stated where the arm lives: this is mitigation. Two id spaces with overlapping integers cannot be told apart by arithmetic on those integers, which is exactly why redisWatchEpochSuffix exists. A counter reset with the epoch intact is the case the epoch cannot see. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
c854c5248d |
fix(watchevents): refuse the ambiguous cursor after a counter reset (BUG-2739, codex round 21)
Both backward arms set knownFrom to the first id of the RESTARTED sequence, which admits a resume from one below it — and one below it is exactly the ambiguous cursor. The two id spaces overlap, so a client presenting n.ID-1 may be holding the OLD sequence's copy of that id, and serving it replays the new space's notifications as though they followed the client's old cursor. That is the corruption these arms exist to prevent, reached one line later. Concretely: hold up to 200, the counter is evicted, ids 1-99 of the new space are missed, and the first we receive is 100. A client at old-space 99 was handed new-space 100 as its successor and told nothing, having actually missed old-space 100 through 200. The shared-counter check cannot see it either — after the reset the remote counter and our high-water mark agree on the new space's value. n.ID + 1 in both arms, which is the SAME reasoning the cold-start arm already applies after an epoch change, for the same reason: an id space changed under us. The epoch arm had the guard and the backward arms did not, which was a gap rather than a distinction — a counter evicted WITHOUT an epoch rotation is precisely the case the epoch cannot see, so it is the case that needed it most. PRE-EXISTING ON THE CONNECTED PATH, and verified as such rather than assumed: the repro was run in a scratch worktree on unmodified origin/main, where it fails identically. Fixed here anyway rather than filed, because this branch added the second arm and shipping it with the known-wrong value — or leaving the two arms disagreeing about the same condition — is worse than a one-character change in code the fix already touches. Cost: one refused resume for a client genuinely at n.ID-1 of the NEW space, reachable only by having been served by another instance. The conservative direction, and the trade the epoch arm already accepts. Mutation matrix: 3 applied, 3 caught — including the over-correction that refuses everything after a reset, which the test's new-space leg exists to catch. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
e4167a9160 |
docs(deployment): five corrections from reading the doc as a document (BUG-2739, codex round 20)
- THE NAMESPACE GUIDANCE WAS WRONG, and this is the substantive one. It said a moving undecodable_message suggests two Pad installations sharing a Redis. It does not: two CURRENT installations publish the same wire format, so their messages decode fine and the damage is cross-feeding real notifications between installations while this counter stays flat — a worse and quieter failure, and the one PAD_REDIS_NAMESPACE actually prevents. The counter indicates genuinely unreadable input: a non-Pad publisher, a mixed-version wire format mid-upgrade, or corruption. The wording was inherited from internal/events without checking that it transferred. - THE FLOOD COSTS WERE OVERSTATED AS SELF-BOUNDING. Heap growth and the announcement are bounded; per-message CPU and allocation are not — a fresh replay buffer plus a pass over every subscriber, on the single goroutine that also delivers real notifications, so a sustained flood is receive-loop starvation as much as it is garbage collection. - THE CUTOVER SECTION described every reconnecting client running a /changes delta. True of the web activity client; pad watch --stream clears its cursor and keeps the connection open, refetching nothing. The doc contradicted its own watch-stream paragraph fifty lines later, which this branch added. - 'A reconnecting client is covered in both cases' was too absolute: the shared-counter check reads at one instant and cannot see a notification published after the read, which resumeOutrunsLocalView and cmd_watch.go both already document as an at-most-once residual. - The rollout note said a reason-specific alert on either surviving reason is unaffected. False for counter_backward, whose spelling changed — which is the entire reason that paragraph exists. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
bc87aee6b5 |
fix(docs,metrics): repair the metrics table I split, and bound the fan-out claim (BUG-2739, codex round 19)
Two real findings from an unconstrained fresh-eyes pass. THE TABLE WAS BROKEN. Round 11's rollout note was inserted BETWEEN two rows of the metrics table, so every row after it — eight of them, including all the pad_event_* counters and the presence failures — rendered as plain pipe-delimited text rather than a table. A documentation change that silently breaks the page it documents is worse than the omission it fixed, and no gate in this repo renders Markdown. The note now sits after the table, and a check across the whole file confirms no blank line or prose splits any of its eight tables. THE FAN-OUT CLAIM WAS STILL TOO STRONG, in both the metric help and the docs row: 'each moves pad_watchevents_midstream_resyncs_total once per such subscriber'. The gap signal is capacity-1 and coalescing, so a second cause firing before a client has acted on the first adds no announcement. It is AT MOST one per subscriber, and reading the fan-out off the two counters needs a reset observed in isolation against idle clients. Round 5 narrowed the aggregate version of this claim and left the per-event one standing. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
603c7f77c3 |
docs(watchevents): bring the shared Bus contract up to what RedisBus now does (BUG-2739, codex round 18)
A parity round asked whether MemoryBus and RedisBus are still describable by one contract. They are — MemoryBus has no transport to lose anything in, so its sequence is contiguous by construction and it raises only the per-subscriber signal — but the interface DOCS had drifted: Subscribe listed the per-instance causes as 'a hole in what it received from Redis', and SubscribeAndReplaySince's nil-replay list predated both new causes. The interface is what a caller reads, so it now names the two scopes explicitly, says the per-instance causes are all RedisBus's because they are all transport faults, and warns that the absence of an instance-wide signal is evidence about which bus you have and nothing else. SubscribeAndReplaySince's wording also gets the distinction this branch has been correcting everywhere else: a nil replay means the span cannot be vouched for, NOT that something was necessarily lost. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |