mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 23:15:40 +00:00
bc6d3541cb034d38e654dc4b2daabbd717d33ff0
54 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
bc6d3541cb |
refactor(watchevents,docs): order the startup test, move operator prose out of the code (BUG-2739, codex round 16)
A future-maintainer round, three of whose four findings were fair. THE REAL DEFECT: TestNoCoverageIsDroppedAtStartup slept 500ms and hoped the receive goroutine had had its chance. No happens-before, so it could miss the regression or turn scheduler-sensitive. It now publishes and waits for DELIVERY instead: the pub/sub channel is FIFO, so a startup confirmation — if the constructor stopped consuming it — is queued AHEAD of that notification and has necessarily been processed by the time it comes out the other end. Deterministic, strictly stronger, and 0.00s instead of 0.50s. Re-verified against its mutation: removing the constructor's Receive still fails it 3/3. COMMENT ACCRETION: dropCoverage had 76 lines of commentary over 30 of code, including a threat model and a per-message cost breakdown that are operator decisions. Those moved to docs/deployment.md, where operators actually read, and the code keeps the invariants and the one design question a maintainer will ask (why not gate on the shared counter). 45 lines now, and nothing was deleted — only relocated to the artifact whose audience it was written for. THE TIME BOMB: the rollout note said 'this paragraph expires at the next tag' with nothing enforcing it. A claim about release state that goes stale silently is exactly what this branch has spent nine rounds removing, so it now carries the three commands to re-derive it instead of asking to be trusted. DECLINED: extracting fanOutLocally's switch into a coverage-state transition helper. The accretion is real and predates this branch — the switch, its four fields and their reset duplication are the existing design, to which this added one arm. A state-machine refactor of the receive path is its own change with its own review and its own mutation matrix; folding it into a bug fix at round 16 is how a fix's blast radius stops matching its claim. Worth filing if a third reviewer raises it. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
988bea658c |
docs(deployment): state the watch reset metric has no released contract (BUG-2739, codex round 11)
A mixed-version-fleet round raised two things — the label spelling, and the
metric widening from 'the ID space changed' to 'replay coverage was dropped'
with two new reasons — as a rollout hazard where old and new instances report
two shapes under one name.
Both collapse to one fact, verified rather than assumed: the entire metric was
introduced by
|
||
|
xarmian
|
720182a445 |
docs(watchevents): sweep the undecodable overclaim as a class, with its boundary (BUG-2739, codex round 9)
Third round running on the same claim, each time in a place the previous
instance fix had not looked — which is CONVE-18's failure mode in miniature,
committed by me twice after writing the sweep into two commit messages.
THE POPULATION, enumerated this time instead of chased: grep for
'undecodable' across the tree. Within internal/watchevents and the artifacts
this branch owns, three sites still said a notification was LOST — the test's
headline sentence (contradicted by its own closing paragraph eight lines
below), the receive-loop comment ('the id of what we missed is unknown BY
DEFINITION'), and the deployment table's trailing summary ('part of it was
simply missed'). All three now say what the instance actually knows:
something arrived that it could not read, it cannot tell whether that was
ours, and it stops vouching BECAUSE it cannot tell — a claim about our own
evidence rather than about the stream.
THE SEARCH BOUNDARY: internal/watchevents, internal/metrics and
docs/deployment.md — the artifacts this change owns. internal/events carries
the same wording for its own undecodable arm (observer.go, bus.go, the
pad_event_* Help string and its docs row) and is NOT touched here. Whether
that wording is equally overclaimed is an open question about a package this
PR otherwise leaves alone, not a cleared one.
Round 9 also read the branch's commits in order for cross-commit interaction
and found none.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
76aa111e53 |
docs(metrics,deployment): finish the undecodable wording and answer the label question in the artifact (BUG-2739, codex round 6)
Round 6's end-to-end failover trace matched the comments step for step, so both findings were prose again. The struct comment above WatchSequenceResetsTotal still grouped undecodable_message with subscription_resumed as 'we simply lost part of it'. Only the second is demonstrable: for the first, the instance knows an unreadable message arrived and cannot tell whether it was ours. Round 5 corrected the exported Help string and the docs but not this one — the same claim in a fourth place, which is what a class-wide sweep is supposed to prevent. The counter_backward rename has now been raised three rounds running, each time because the answer lives in a commit message and a reviewer reads artifacts. It is answered where an operator with a broken dashboard would look: no tagged release emitted the plural, so there is nothing to migrate. Re-derived at 40b0db06 rather than restated. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
2e9ace4194 |
docs: nine overclaims across code, metrics, docs and the CLI (BUG-2739, codex round 5)
A cross-artifact pass, which is the angle that keeps paying on this family. Every item below was a statement of mine that was false or unsupported; the code did not change. WRONG FACTS: - Watch epochs are opaque UUIDs, not numeric generations. I had copied internal/events' wording, where they ARE numeric — the distinction is the subject of internal/idspace's package comment. - undecodable_message was described as proof a notification was missed. The instance knows only that something it could not read arrived on its channel; it cannot tell whether that was ours. It stops vouching BECAUSE it cannot tell, which is a different and weaker claim. Corrected in four places. - The failover-cost paragraph said every SSE client on the instance reconciles. Wrong twice: a watch-bus resubscription ends the WATCH stream's coverage (activity coverage is per-workspace), and the one client that uses that stream today — pad watch --stream — answers sync_required by clearing its cursor and keeping the connection open, so it issues no request at all. Verified in cmd_watch.go rather than assumed. - The midstream/reset ratio is not fan-out in aggregate: the announcement counter also carries gaps and slow-subscriber drops and coalesces per connection. Only a reset observed in isolation reads that way. - 'The watch stream's only signal was a later non-contiguous notification' is true for a client HOLDING A STREAM OPEN. A reconnecting client was always covered, because a resume asks the shared counter instead of local state. Scoped in the doc and in the test header. - The dropped-confirmation fallback said coverage still ends. Usually, not necessarily: with no traffic during the outage nothing was lost, and if the drops continue through whatever would expose the hole and the stream goes quiet, nothing ever does — BUG-2727's boundary. Named both. - The new Observer Close warning was overbroad: reports run on the receive goroutine only on the RedisBus receive path, while a ResumeGap runs on the caller's and MemoryBus has no such goroutine. The rule stays unconditional, since a callback cannot tell which case it is in, but it now says why. STALE AFTER THIS BRANCH: - metrics.go's WatchSequenceResetsTotal comment listed two reset reasons. - observer_test.go said 'both reset reasons'. - The constructor comment named Channel() after the loop moved to ChannelWithSubscriptions, and did not say the Receive beneath it is load-bearing for that loop having no skip-the-first flag. It does now, and names the test that fails if it goes away. - cmd_watch.go's sync_required cause list predated BUG-2739 (and did not mention the mid-stream delivery BUG-2730 added). Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
5d5450a43d |
docs(watchevents,deployment): name the failover cost and the dropped-confirmation road (BUG-2739, codex round 2)
Two operator-angle findings, both real and neither a code defect. A subscription confirmation goes through the SAME bounded channel as messages — go-redis v9.22.0 initAllChan handles `case *Subscription, *Message:` identically, chanSize 100, chanSendTimeout 1 minute — so under sustained load the resubscription marker can be dropped like any message. Checked in the library rather than argued. Coverage still ends by the other road: a full channel means traffic, the outage left a hole in the ids, and the gap arm raises it on the next message consumed. The operator gets a less specific label for the same truth. BUG-2727's standing boundary (a drop whose hole no later notification exposes) is unchanged in both directions. And detection is not free: a resubscription ends coverage for the whole instance, so every connected SSE client reconciles at once — up to PAD_SSE_MAX_CONNECTIONS of them, since per-connection coalescing smooths repeats within a wave and not the wave itself. Named in the deployment doc with the ratio that measures it, because an operator meeting this for the first time during a failover should not have to derive it. Round 2 also re-raised the counter_backward rename and the half-open connection. The first is answered by the ancestry evidence in 40b0db06 — nothing released carries either spelling. The second is BUG-2738, already named in this doc as a surviving residual and rulings-first per the lead. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
3b2df81032 |
docs(deployment): the activity stream cannot detect ID-sequence holes (BUG-2739, codex round 1)
The rewritten paragraph claimed both streams now detect the same three things, ID-sequence holes included. They do not, and the paragraph directly below it said so — its per-workspace IDs come from a counter shared across workspaces, so holes in them are the normal state and no arithmetic on them means anything. That is why pad_watchevents_sequence_gaps_total has no pad_event_* counterpart, which is now stated where an operator looking for the missing counter would look. What BUG-2739 actually equalises is the two DIRECT detections: a pub/sub resubscription and an undecodable message. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
a3a98b249a |
fix(watchevents,metrics): unify the reset label on counter_backward (BUG-2739)
The two buses spelled the same condition one letter apart: internal/watchevents emitted counter_backwards, internal/events emits counter_backward. Same metric family, same meaning — so an operator writing one alert expression across both gets silence from one of them. Singular wins because it is the majority and the documented one: internal/events' constant, both metric help strings, and docs/deployment.md (the reasons table, the ID-space migration section, and the phase notes) all say counter_backward. watchevents' plural, added in BUG-2727, is the lone deviation. CONTRACT-SAFE, and this is the load-bearing half rather than a nicety, since renaming an emitted metric label ordinarily breaks any alert built on it. Nothing released carries either spelling. Re-derived in this session rather than carried from the ruling's date: git describe --tags --abbrev=0 origin/main -> v0.14.0 git rev-list --count v0.14.0..origin/main -> 128 git merge-base --is-ancestor |
||
|
xarmian
|
5184aba852 |
fix(watchevents): detect the two holes the watch bus could not see (BUG-2739)
The watch bus learned of a hole ONLY when a later notification arrived with a
non-contiguous id. So a Redis flap that lost the NEWEST notification, on a
stream that then went quiet, left every connected CLI silently stale
indefinitely: nothing later ever arrived to be non-consecutive with. The
activity bus has detected both of these directly since BUG-2731; this ports
them.
Two conditions now end this instance's coverage:
- a pub/sub RESUBSCRIPTION. go-redis reconnects and re-subscribes silently,
and whatever was published during the outage never reaches us. Requires
ChannelWithSubscriptions, which surfaces the confirmations Channel hides.
- an UNDECODABLE message. It is not enough that this bus's ids are
consecutive by construction so the gap arm would catch it next time —
that detection needs a next time, and the case that matters is an
undecodable newest message on a quiet stream.
NO "SKIP THE FIRST CONFIRMATION" FLAG, which is the one place a port of
internal/events' loop would have been wrong. That package's receive loop is
handed a fresh PubSub nobody has read from, so its initial confirmation
arrives on the channel and must be skipped. Ours does not:
NewRedisBusWithKeys calls pubsub.Receive before the goroutine starts and that
Receive consumes the initial confirmation — verified with a probe, which saw
zero subscriptions on the channel at startup. Copying the flag would have
swallowed the first GENUINE resubscription, i.e. shipped this bug wearing a
fix. TestNoCoverageIsDroppedAtStartup is the enforcement for that dependency,
not a comment: it fails if the constructor's Receive is ever removed.
dropCoverage resets replay, lastAppendedID and knownFrom TOGETHER. Clearing
the buffer and knownFrom while leaving lastAppendedID stale makes the next
notification read as contiguous, so no arm of fanOutLocally's switch fires,
knownFrom is never re-established, and replaySince refuses every resume on
that instance forever — correct-looking and permanently broken. The recovery
test was written before the refusal test for exactly that reason: a bricked
bus refuses too, so asserting only the refusal cannot tell them apart.
epochJustChanged is deliberately not set: both conditions are a hole in our
view of the SAME id space, so the cold-start arm's ordinary knownFrom = n.ID
is right. The +1 exists only for the ambiguity between two id spaces.
Live subscribers are told through signalAllLocked, which BUG-2730 left in
place for this shape — so the client holding the stream open across the flap
gets sync_required mid-stream, which is the whole point of the unit.
tcpCutter is ported from internal/events' reconnect test for the reason its
header gives: nothing short of a real severed connection produces a
resubscription, so testing the decision logic alone would leave the wiring
claim unproven (CONVE-19).
docs/deployment.md's paragraph stating this asymmetry as a known gap is
rewritten rather than deleted, and now names both surviving residuals:
BUG-2735 (a message lost in transit with the connection intact) and BUG-2738
(a half-open connection, which nothing here can see because go-redis's
pub/sub health check writes without reading).
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
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
|
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
|
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
|
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
|
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
|
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
|
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
|
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
|
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
|
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
|
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
|
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
|
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
|
b4989aa2f0 |
fix(server): retire a cursor we just refused, and stop trusting one we cannot read (BUG-2731)
Three handler changes and the documentation the coverage fix made wrong. sync_required NOW RETIRES THE CLIENT'S CURSOR, carrying an empty `id:` which per the EventSource spec clears the last event ID. Without it the client keeps the cursor that was just declared unservable, so every later reconnect on a quiet workspace is answered sync_required again and re-runs a full delta sync — a loop that only ends when a live event happens to arrive. Survivable while the response was rare (buffer eviction only); the coverage check makes it common, so this is a load consequence of that fix and belongs to it. AN UNREADABLE Last-Event-ID IS A GAP, not a fresh connection. Only a parseable positive value reached the replay path, so "-1", "not-a-number", a quoted number, or an integer too large for int64 silently dropped everything published before that subscription. The same lie this fix exists to end, arriving through the parser rather than the buffer. A genuinely fresh client sends no header and is unaffected — asserted, because the fix is one `if` away from resyncing everyone on connect. Not a case, and the test says why rather than omitting it silently: a whitespace-only value. HTTP strips optional whitespace from header values, so the handler sees an empty string, which the spec defines as "no position". Measured, not assumed. HANDLER-LEVEL GAPS ARE COUNTED. A cursor no one can parse never reaches a bus, so without Server.countResumeGap the counters would undercount exactly the resyncs an operator is most likely to be asked about: a client looping on a cursor nobody can read. BOTH SSE HANDLERS GET ALL THREE, because introducing them on one stream is how parallel surfaces silently diverge. The pad CLI masks the cursor difference by clearing its own — verified by reading its parser, which handles the empty-value form — so the consumer this would bite is a generic SSE client, the one nobody tests. DOCS. Two comments described mechanisms that had changed: the handler's own "gap too large — buffer evicted" (eviction is now one of several) and internal/config's claim that the activity stream silently misses a namespace cutover. And docs/deployment.md's cutover note said resync is honest on the watch stream and silent on the activity one; it is now honest on both, with the edge that a cursor exactly one below a replica's first-seen ID is served rather than refused, tracked as BUG-2736. The sync_required reason text changes from "Event buffer exceeded" to what actually happened. Keeping it was defended earlier BECAUSE the client never reads it, which is the same reason correcting it is free. Refs BUG-2731 |
||
|
xarmian
|
9f88e94832 |
fix(events): a resume must not be answered from coverage we never had (BUG-2731)
internal/events answered a Last-Event-ID resume with an empty-but-non-nil
slice whenever the workspace's replay buffer could not speak to the span
being asked about. The SSE handler reads that as "caught up", so the client
sat on a live stream believing it was current while everything between its
cursor and now was silently gone.
COVERAGE. replayBuffer gains knownFrom: the lowest event ID from which this
instance's coverage of a workspace can be vouched for. A resume from below
it answers nil, which the handler already turns into sync_required. Covers
a buffer that does not exist (cold start, restart, scale-up, or simply the
first connection to a workspace on this instance), a buffer that exists but
starts above the cursor — NOT full and NOT empty, reachable on any
multi-instance deployment with no eviction and no restart — and a non-zero
cursor from a previous incarnation of a single process.
knownFrom here means RECEIVING-continuity, never ID-contiguity, and the
defining comment says so with the measurement attached.
internal/watchevents has a field of the same name that ALSO detects holes
by noticing a non-consecutive ID; porting that would have been a serious
regression, because this bus has a global counter and per-workspace
buffers, so a workspace's buffer holds non-consecutive IDs by construction
(four publishes alternating across two workspaces measure as W=[1 4],
X=[2 3]). An ID-contiguity check would fire on nearly every append and turn
every resume into sync_required — the false-positive inversion of this bug.
LIFECYCLE. Coverage now ends where it really ends:
- a stopped workspace subscription drops its replay buffer. Keeping it
"in case they come back" looks like a free win and is the bug: events
published elsewhere never enter it while it goes on looking complete.
- subscriptions are generation-numbered, so a straggler from an ended
subscription cannot re-create a buffer and vouch for coverage that
ended with it — including the case where the workspace has already been
resubscribed under the stale goroutine.
- a pub/sub reconnect ends that workspace's coverage. PubSub.Channel
resubscribes transparently, so a Redis failover left a hole the buffer
had no idea about; the loop reads pubsub.Receive instead. It must
RECOVER rather than exit — returning on a transient error would leave
an instance publishing fine and receiving nothing — and it drops ONE
workspace's buffer, since a dropped subscription says nothing about any
other channel.
Subscribers are indexed by workspace because the replay buffers moved under
the same mutex (necessary for the straggler race): scanning every local
subscriber under that lock would make one hot workspace the serialization
point for every other workspace's fan-out and every resume.
Also removes Publish's local-counter fallback on a failed INCR, which
minted an ID from a process-local space and published it — every receiving
instance reads that as the counter having been reset. It bought nothing:
this bus has no local fan-out path, so an event that does not reach Redis
reaches no subscriber here either.
SIBLING. internal/watchevents had the identical cold-resume defect on its
MemoryBus — its RedisBus guards it, MemoryBus reached the buffer directly —
so a single-process instance answered a post-restart resume as caught up.
Found by a cross-artifact review pass; the guard goes in `since` so both
implementations inherit it, and is tested through SubscribeAndReplaySince
as well as EventsSince because that is the path the handler uses.
Refs BUG-2731
|
||
|
xarmian
|
bb003dd6bb |
fix: five claims the final comment-truth round found (BUG-2724, BUG-2726)
The bounded process the lead set: N rounds, an author prune pass, one final comment-truth round. This is that round's output, and the loop stops here. Two were mechanisms I had wrong, and both are the kind a reader would reuse without re-deriving: - "Different Redis DB numbers do not help" was half true. Ordinary keys ARE DB-scoped, so two installations on different DBs keep separate presence registries; it is pub/sub that ignores DBs entirely, which is why the buses cross-feed regardless. Stating it as "does not help" made the namespace look like the only fix for a problem it only half is. - A namespace cutover's client resync was attributed to the epoch check. That check needs an OLD epoch to compare against and a freshly namespaced bus has none — the resync comes from the cold replay-buffer coverage check instead (knownFrom is zero, so every resume falls below it). Same honest outcome, different mechanism, and the mechanism is what someone reasoning about a cutover would use. Three were stale or over-general after earlier changes: the admission comment still said the global limit is passed to the bus as 0 (that parameter is gone), `pad watch --help` and the plugin monitor description lumped a missing .pad.toml's hourly retry in with the 5s-to-5min backoff, and CLAUDE.md said clients must back off without the browser exception docs/deployment.md spells out. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
35e564298b |
fix: seven more prose claims, one real metric gap, and a flaky test of my own (codex round 17)
The prose angle again, and it is still finding things — which is itself the finding: this diff's comment density is generating wrong beliefs faster than the review is removing them, in the one dimension where the defect is a reader's understanding rather than the program's behaviour. Everything below was a claim I wrote. ONE WAS A REAL GAP, not just wording. pad_watchevents_resume_gaps_total was documented as counting every unservable resume, and counted only the half decided by the shared counter. The LOCAL half — a cursor below what this instance can vouch for, from a hole or a cold start — returns nil from replaySince, becomes sync_required for the client, and reported nothing. Now counted, on the deferred path so it fires with the lock released. Its test needed a second pass to be an instrument: the first version arranged a hole and asserted the counter moved, but the shared counter disagreed too, so resumeOutrunsLocalView reported and the mutation survived. It now sets the counter to AGREE with what the instance has seen, which is the only arrangement that isolates the local path. The prose corrections, swept by grep rather than by instance this time: - MemoryBus's comment said a single-process deployment never wires an observer. cmd_server wires one, deliberately — that is what makes the drop counter meaningful there, which is a claim I had just added elsewhere. - "Every write path works with Redis down" was too strong in three places. Push answers 503 for an unresolvable targeted push and 502 push_unconfirmed on publish failure — the paths whose job IS cross-instance delivery. - Presence-failure consequences were stated as certainties in four more places after round 16 fixed one. A failure means an error was REPORTED; Redis can fail a pipeline after applying it. - The deployment metrics table still described pad_eventbus_publish_total as "Events published" after the Help string had been corrected to attempts. - The reserved-namespace rationale called prefix nesting a "collision". It is nesting; an exact collision would need the namespace to match a workspace UUID. Refused anyway, and now for the reason that is true. - A presence cutover was described as stranding one renewal interval of stale entries. It is the full 90s TTL — three intervals. AND A FLAKE OF MY OWN, caught by the full suite rather than by the targeted runs: the activity-bus namespace test asserted subscription state immediately, but that bus subscribes ASYNCHRONOUSLY (the watch bus waits for confirmation; the two differ). It now polls, and the asymmetry is named in both tests so the next reader does not assume symmetry the way I did. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
7c8ed3c815 |
fix: nine false or overstated claims in this diff's own prose (codex round 15)
An angle worth naming, because it found more than several code-shaped
ones did: check the COMMENTS against the CODE. This diff is
comment-heavy and its comments make specific factual claims. Nine were
wrong.
The one that mattered most was a false argument for a correct rule.
redisns.Parse rejects colons, and justified it with a collision example
that does not happen: ns "a:events" builds pad:a:events:events:<ws>, not
pad:a:events:<ws>, because the suffix is appended too. The rule stands on
its own grounds (a colon spans segments and makes the keyspace ambiguous
to read back) — but a false example is worse than none, because the next
reader trusts it.
Chasing that turned up a REAL collision needing no colon: a namespace
equal to one of Pad's own first segments nests this installation inside
the default one's keyspace. Namespace "events" puts every key under
pad:events:*, which is the default installation's activity channel space
— the exact cross-feed the namespace exists to prevent, arriving through
the namespace. Now rejected, with a control leg asserting that names
merely CONTAINING a reserved word ("events-eu", "prod-session") stay
valid.
The other eight:
- "The three keyspaces cannot drift" — overstated. Each constructor takes
its own Keys; a source-reading test is what enforces it, which is
weaker than a compiler and now says so.
- Two docs claimed both SSE endpoints incur a presence registration. Only
the watch stream registers.
- The Redis metrics section said they "stay at zero" without Redis, while
pad_redis_up is deliberately unregistered — the section contradicted
the field three lines below it.
- The presence-failure metric's HELP string still carried the blanket
"leaves sessions unlisted and untargetable" that the field comment had
already been corrected away from. Two of the four ops fail in the
opposite direction.
- A nil from MGET was described as proof the process died. Eviction, a
restart and a manual DEL produce the same nil, and this file's own doc
says eviction is indistinguishable from expiry.
- A test comment claimed to cover both corrupt-entry shapes; the second
is unreachable and the subtest is deliberately absent, as the note ten
lines down already said.
- "Enumerates every refusal path" covered per-instance and per-workspace
and not per-user — the same undercount as round 13's, one round later.
Both per-user paths added.
- The Observer contract said a go-redis drop is reported as a sequence
gap. Only if a LATER notification arrives to expose the hole: drop the
newest message on a bus that then goes quiet and nothing is reported.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
3e3170e915 |
fix(server,cli,docs): the consumer contract, per codex round 12 (BUG-2726)
An angle no earlier round took: what does a CLIENT see. Two of the five
findings were about consumers I had never opened.
- `pad project watch` returned "event stream returned 429: {json}" and
exited, which sends the reader looking for a bug rather than at a
limit. It now says what happened and which knobs govern it, and names
the fact that those knobs cover this stream and the agent watch stream
together. It still exits rather than backing off — it is interactive,
and a human can decide — unlike the unattended monitor, which already
folds 429 into its ladder.
- Both endpoints now answer a refusal through one helper: same status,
same code, same message, plus `Retry-After`. `/api/v1/events` was
setting `Content-Type: text/event-stream` BEFORE the admission check,
so its 429 carried the JSON error envelope under an SSE content type —
a different contract from its sibling's for the same refusal. Admission
moved above the headers, which is where it belonged anyway.
- The anonymous-caller rule was documented as if it applied to both
endpoints. It applies to `/api/v1/events` only; the watch stream
requires a resolved user and answers 401 without one.
- docs/architecture.md described one SSE endpoint and one bus. It now has
the table: two streams, two buses, different scopes and consumers, one
shared connection budget, one Redis namespace.
FILED, not fixed: the web UI's `EventSource` cannot see a 429 or a
`Retry-After` — the spec exposes neither to the page — so a refused
browser tab reconnects at a constant rate while the CLI backs off. That
asymmetry means reaching the limit sheds load from the population that
respects it and not from the one that grows fastest under it. No
server-side change closes it; the fix is a client-side reconnect wrapper.
BUG-2733, and docs/deployment.md warns operators to size the limit with
it in mind.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
a790810bd6 |
docs: close the cross-artifact gaps codex round 6 found (BUG-2724, BUG-2726, BUG-2727)
The angle no earlier round probed: which artifacts a human or agent CONSUMES should have changed and did not. Five, and the pattern is the one my own record keeps naming — the caveat existed in the artifacts I was editing and not in the ones that get read. - .env.example had neither new variable and still described PAD_SSE_MAX_CONNECTIONS with its old single-endpoint meaning. It is the file an operator copies; docs/deployment.md being right does not help someone who never opens it. - docs/deployment.md called the readiness endpoint /health/ready. The route is /api/v1/health/ready, so every instruction to go read the new redis block pointed at a 404. Corrected there and in four code comments, and the Health Check section now actually shows the three endpoints, the healthy payload, and the degraded one — it previously demonstrated only /api/v1/health, which is the build-info endpoint and says nothing about readiness. - CLAUDE.md listed /api/v1/events and not /api/v1/events/stream at all, so the endpoint this unit bounds was undocumented in the file agents read first. Added, with the limits and the 429 contract. - `pad watch --stream --help` said silence means "no workspace linked or padd unreachable". A capacity refusal now produces the same silence through the same backoff, so the help was enumerating a set that had quietly grown. - The plugin skill told agents "silence means nothing changed" — now false in the same way, and worse, because an agent repeats it to a user as though the quiet were evidence. Rewritten to say what silence does and does not prove. The plugin monitor description had the same enumeration and got the same fix. Checked rather than assumed: there are two SKILL.md files, and only the plugin copy carries a notifications section — the embedded one has no monitor guidance to correct. NOT changed, and raised with the lead instead: deploy/k8s/deployment.yaml points both probes at /api/v1/health, so the readiness endpoint is never consumed. Fixing it is right but it changes rollout behaviour for anyone using the shipped manifest (a database blip would start pulling pods from the load balancer), which is a deployment-posture call rather than part of this unit. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
9afedbe1a0 |
fix(server,metrics,watchevents): seven codex round-4 findings — operator and next-author angle (BUG-2727, BUG-2724)
Round 4 read the diff as the operator of a running deployment and as the author of the next change. Five findings were claims my own text made that the code does not support, which is the failure mode this angle is for. 1. The degradation list said Redis loss costs "cross-instance activity events". It costs ALL of them: events.RedisBus.Publish logs its failure and returns without a local fan-out, so subscribers on the originating instance stop receiving too. A responder told only about cross-instance delivery would have looked elsewhere. Corrected in the health payload, both prober log lines, and the docs. 2. config.go promised that connected clients resync after a namespace change. True of the watch stream, false of the activity stream, whose cold replay buffer answers a resume as "caught up" (BUG-2731). The docs already carried the asymmetry; the comment did not, and the comment is what the next author reads. 3. Resume-detected gaps were counted nowhere. They are the only gap shape that is always USER-VISIBLE — the client gets sync_required — so an incident reading pad_watchevents_sequence_gaps_total would have missed the failure mode with the clearest symptom. New pad_watchevents_resume_gaps_total, kept separate rather than folded in because the two are diagnosed differently: one is a delivery fault, the other is any cursor this instance cannot vouch for. 4. The presence-failure metric's doc said every failure leaves sessions unlisted and untargetable. Two of the four ops fail in the OPPOSITE direction — a failed deregister leaves a dead session listed, so a push aimed at it is accepted and reaches nobody — and a generic alert on the total would send a responder the wrong way. Now documented per op, in the code and in the docs table. 5. The go-redis log bridge levels everything at WARN, and the comment justified that with "benign reconnect chatter" I had never enumerated. Enumerated now: the stream carries genuine failures, state changes and informational fallbacks with no severity attached. WARN stays — INFO would bury the dropped-message line the bridge exists for, and classifying by message TEXT would make Pad's log levels depend on go-redis's prose — and a component=go-redis field makes it routable instead. 6. internal/redisns centralizes key construction but cannot stop a future contributor wiring one bus with a different Keys than another: every package compiles, every unit test passes, and the deployment runs split across two keyspaces while looking configured. Adds a wiring drift guard that reads cmd_server.go and fails if the three constructors do not share one Parse-produced value. The rule was already written down in a package comment; this is its enforcement step. 7. The limits are per-process and the startup log, log fields and gauge Help called them "global". Renamed to per-instance / per-principal throughout, with the no-shared-counter caveat in the startup line. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
c03a4851bd |
fix(server,redisns): two codex round-3 findings — DoS via legacy tokens, blank namespace (BUG-2724, BUG-2726)
1. Callers with no user id skipped the per-user bound entirely, so one legacy workspace-scoped token could fill the global budget and 429 everyone else — a denial of service through a deprecated auth path. My own comment argued for the skip on the grounds that bucketing every anonymous caller under one empty string would make unrelated callers evict each other. That was right about the empty-string bucket and wrong about the conclusion: the fix is a better key, not no key. They are now bucketed by workspace, the finest granularity actually available — from the token's own workspace id where it has one, from the resolved workspace otherwise. The residual trade (two legacy tokens for one workspace share a bucket) is stated in the code and in the docs rather than left for a reader to discover. 2. PAD_REDIS_NAMESPACE=" " trimmed to Default, so a broken template substitution silently restored the historical keyspace and collided with the installation the namespace was set to separate from — the exact leak, arriving through the mechanism meant to prevent it. Only a genuinely unset value is Default now; whitespace-only is a startup error naming both alternatives. The first fix needed a second instrument. Mutating the handler to pass currentUserID instead of streamPrincipal SURVIVED the unit tests, which drive the helper directly — the same defect shape as day-49's batch-id finding: testing a knob at the layer that consumes it proves the knob, while the caller passing it is a separate claim. The new handler-level test drives the fresh-install no-auth window through HTTP and fails by name when that wiring is reverted. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
03518466ab |
fix(server,metrics,docs): five codex round-2 findings (BUG-2724, BUG-2726)
Round 2 probed angles round 1 did not: rolling upgrade and rollback, request cancellation, and whether any operator-facing text now contradicts the code. Four of the five were the latter. 1. The admission slot was held through the Redis presence cleanup. Defers run LIFO, so the acquire-site release ran LAST — after Remove's round trip, bounded by presenceOpTimeout (5s) and a wait on the renewal goroutine. A reconnect arriving inside that window could be refused by a bound the connection had already stopped consuming, and the window is widest during a Redis outage, which is when clients reconnect most. A second deferred release, registered later so it runs first, closes it; the acquire-site defer stays as the safety net for early returns, and release is idempotent so deferring twice releases once. 2. pad_sse_connections_active is written by the events.EventBus wrapper, so it has only ever counted the workspace stream. That was every SSE connection Pad had a limit for until this branch; it no longer is, so an operator watching it against the global limit would be reading one endpoint's share of a two-endpoint budget. Adds pad_stream_connections_active, driven by the admission gate itself, and both Help strings now name their population. Wired from either SetMetrics or SetSSELimits (either can land first) and from the lazily-built gate, each covered by a test — a gauge stuck at zero while streams are held is the same shape of lie as a metric that is not registered at all. 3. The limits are enforced in-process and the docs called them "Global". With the shipped k8s manifest's two replicas, 1000 admits ~2000 and a user can hold 50 per pod. Documented as per-instance, with the multiply-by-replicas note and a pointer at the new gauge. 4. A namespace cutover partitions a rolling upgrade — namespaced and un-namespaced replicas are two installations for the length of the rollout — and rolling back with the variable still set silently restores the split. Both now stated, with the env var and the binary having to move together in both directions. 5. Client resync across that cutover is honest on the watch stream (the epoch key detects the changed id space) and SILENT on the activity stream, whose cold replay buffer answers a resume as "caught up". Documented, and filed as BUG-2731 rather than fixed here: it is pre-existing, fires on any replica restart, and the minimal fix changes reconnect behaviour for every deployment, which wants a ruling rather than a quiet patch. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
0877b260c1 |
feat(redis): namespace every Redis keyspace from one shared config value (BUG-2724)
Every Redis key and channel Pad uses was flat — pad:events:, pad:event_seq, pad:watchevents*, pad:session:* — so two Pad installations pointed at one Redis endpoint cross-feed each other's notifications and merge each other's session-presence registries. Different logical DB numbers do not help: Redis pub/sub is not namespaced by DB at all. The exposure is narrow but real. Delivery is filtered per caller on user id, and user ids are per-installation UUIDs, so cross-feed needs the same id in both installations — a CLONED database, such as a staging environment restored from a production dump. For that case it is a genuine cross-tenant leak: foreign sessions listed in the picker, and a private push deliverable across installations. Fixed the way internal/watchevents' existing ruling demanded: not by one package growing a prefix the others lack, but through internal/redisns — one value parsed in cmd/pad/cmd_server.go and passed into all three constructors. The three cannot drift because there is nothing to drift from, and the operator rule is stateable in one sentence for every keyspace. PAD_REDIS_NAMESPACE defaults to empty, which reproduces the historical names byte for byte, so an existing deployment keeps addressing its own replay buffers, counters and presence entries across the upgrade. Tests assert both directions per keyspace — present under the namespace AND absent under the historical names — because an implementation that wrote both would still cross-feed while passing a one-directional test. Namespaces are validated at startup, and a colon is rejected specifically: it is Pad's own separator, so namespace "a:events" would build pad:a:events:<ws> and collide with installation "a"'s channel — reintroducing the cross-feed through the mechanism meant to fix it. Names are built through a function rather than assembled from a literal at each site, and redisns' doc says why: "pad:" also begins Pad's OAuth SCOPE values (pad:read / pad:write / pad:admin) in four files, so a grep-driven prefix sweep would break authorization. Not included, deliberately: hash tags for Redis Cluster. BUG-2724's trail recommended shipping them alongside on cost-sharing grounds; that premise is falsified by publishScript, which spans four keys in one EVAL and fails CROSSSLOT exactly as presence's MGET does. There is no cheap half, and no cluster client here to exercise tagged keys against, so they would ship untested by construction. Cluster stays documented as unsupported and the future unit is named on the trail. Renaming is a CUTOVER for the buses (the seq and epoch keys carry Last-Event-ID meaning, so connected clients resync) and free for presence (90s TTL). Both stated in docs/deployment.md and at the constructors. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
ea139272ce |
fix(server,watchevents): shared session presence + honest push acceptance (BUG-2698, BUG-2699) (#1175)
Two coupled defects in the push path, fixed as one unit because 2699's honest-acceptance signature is the substrate 2698's fix reports through. BUG-2699 — Bus.Publish reports acceptance. The endpoint returned 200 pushed:true for a publish that was dropped, because Publish returned nothing and swallowed every failure. An error is two outcomes and they are kept apart: ErrBusClosed proves nothing was published (503 unavailable, safe to resend), while any other error means UNCONFIRMED — go-redis retries a command whose reply was lost, which is why the publish script already carries a dedupe token — and gets 502 push_unconfirmed, deliberately off the web client's safe-to-resend list. MemoryBus was the worse case, not the exempt one: neither implementation checked `closed`, and the in-process one dropped silently with no log at all. Seven production call sites, not the six the item named; the six best-effort producers discard through one named helper, and an AST-based test fails when a new producer publishes directly. BUG-2698 — RedisSessionPresence. A session-targeted push was resolved against the answering replica's presence registry, and the handler skips the publish when the target is absent, so a POST landing on A for a session held on B dropped the instruction and answered delivered_sessions:0. Fixed at the REGISTRY rather than the gate: a shared registry makes the snapshot right, which makes the picker complete and restores the gate's original premise, so the existing skip becomes correct for the reason it was written. Entry and index are written atomically under a TTL renewed by a goroutine that lives exactly as long as the connection; a crashed process stops renewing and Redis clears it. Staleness is unchanged and now stated in full: ~30s for a dropped client, ~90s for a dead instance. delivered_sessions becomes nullable — null means published-but-uncountable, never zero — documented as three states at every consumer. 35 Codex review rounds. Notable: a per-user registry cap was added and then removed after three consecutive rounds found defects inside it and a fourth was asked whether it belonged in this PR at all; a context bound was documented, disproved by its own test (go-redis does not apply a command context to connection establishment — 5.0s measured against a 150ms ctx), and rewritten to say what is true. Every fix was mutation-checked; one instrument was deleted for passing on broken code and one for not asserting its own premise. Filed rather than folded in: BUG-2724 (Redis keyspace namespacing + Cluster), BUG-2725 (delivered_sessions is an estimate with error in both directions), BUG-2726 (no concurrent-connection limit on the watch stream), BUG-2727 (Redis absent from readiness/metrics; silent subscriber loss), BUG-2728 (epoch-reset resume lead). Gates: build · make lint 0 issues · go test ./... (25 pkgs) · svelte-check 0 errors · vitest 1738 passed · CI 7/7 including Go (PostgreSQL) and Nix. |
||
|
xarmian
|
8c609be2e3 |
feat(store): guard against schema-ahead downgrade + pre-migration snapshot + upgrade docs (TASK-2006) (#843)
The migration runner only applied missing embedded migrations and never detected a DB that was AHEAD of the binary, so a brew/docker downgrade silently ran old code against a newer schema. It also took no backup before migrating, and there were zero upgrade docs. - guardSchemaAhead: refuse to start when schema_migrations contains a version that sorts after the highest embedded migration (a downgrade). Escape hatch: 'pad start --force' / PAD_ALLOW_SCHEMA_AHEAD=1. Applied to both the SQLite and Postgres migration paths. - snapshotBeforeMigrate (SQLite only): copy the DB file to <db>.pre-<VERSION> before applying pending migrations, but only when upgrading an existing DB (pending AND already-applied migrations). WAL-checkpointed, atomic temp+rename copy, and preserves an existing snapshot on retry so a failed multi-step upgrade can't clobber the original rollback point. Postgres is skipped (pg_dump/PITR is the DBA's). - Docs: 'Upgrading Pad' in README + an 'Upgrading' section in docs/deployment.md (forward-only rule, guard behavior, snapshot, flow). |
||
|
xarmian
|
9be8e96cfd |
fix(cli): safe SQLite db backup/restore (config path + VACUUM INTO) (#837)
* fix(cli): safe SQLite db backup/restore (config path + VACUUM INTO)
pad db backup/restore hardcoded ~/.pad/pad.db, so `docker exec pad db
backup` (container sets PAD_DATA_DIR=/data) and Windows layouts broke,
and the SQLite path did a torn io.Copy of pad.db + separate -wal/-shm
copy that could lose or tear in-flight WAL writes.
- Resolve the SQLite path via the server's config loader (PAD_DB_PATH >
PAD_DATA_DIR/pad.db > ~/.pad/pad.db) instead of os.Getenv("HOME").
Covers backup, restore, and migrate-to-pg's --from default.
- Replace the file copy with an online-safe `VACUUM INTO` through the
embedded modernc.org/sqlite driver: one self-contained file, no
-wal/-shm juggling, safe while the server is live.
- Restore refuses when a live server is detected (a running WAL
checkpoint could clobber the restored file); --force overrides.
- docs/backup.md: `pad db backup -o <file>` is the canonical SQLite
path (+ the `docker exec <container> pad db backup -o /data/backup.db`
form); dropped the "PostgreSQL-only" mislabel.
PostgreSQL pg_dump/psql paths are unchanged.
Fixes BUG-1996.
Claude-Session: https://claude.ai/code/session_01BoPkYhKqMiWPYmxQigeWsA
* fix(cli): fail restore on stale sidecar removal + drop unsafe backup doc
Address Codex review P2s:
- Restore: treat a failure to remove a stale -wal/-shm at the target as
fatal (was silently ignored). With single-file VACUUM INTO backups a
leftover sidecar would replay old WAL state over the restored DB.
- docs/backup.md: the SQLite strategy block still recommended a raw
`cp pad.db` daily; point it at `pad db backup --cron` instead.
Claude-Session: https://claude.ai/code/session_01BoPkYhKqMiWPYmxQigeWsA
|
||
|
xarmian
|
616a6d2a0a |
feat(auth): localhost password recovery for locked-out self-host admins (#760)
Add a loopback-only account-recovery path so a self-hosted operator who
forgot their password (with no email provider configured) can recover
without editing the database by hand.
- POST /api/v1/auth/local-reset: loopback-gated, non-cloud, no auth
required (same trust model as bootstrap). Returns a single-use reset
link, or a temporary password with {"temp_password": true}.
- pad auth reset-password <email> [--temp-password]: talks to the local
server over loopback directly (not the configured public URL), so the
command works on the server host regardless of CLI config. Prints the
server's shareable reset_url when a public base URL is known.
- Web /forgot-password reads email_configured from the session and shows
host-recovery instructions instead of a dead "we emailed you a link"
when no provider is configured.
- forgot-password server log emits the reset path on non-cloud instances
so operators can also recover straight from the logs.
- Docs: CLAUDE.md + docs/deployment.md recovery sections.
Tests cover the loopback/cloud gates, the shareable reset_url, and both
output modes (reset link + temp password).
|
||
|
xarmian
|
94ebe5a83d |
test(screenshots): capture in dark mode (Pad's default theme) (#319)
The README screenshot capture script ran in light mode because Playwright's headless Chromium reports prefers-color-scheme: light by default. The Pad layout's onMount logic explicitly forces data-theme="light" when matchMedia matches 'light' — so the captures came out light-themed even though Pad defaults to dark when no user preference exists. Two effects made this misleading: 1. README screenshots showed a theme most Pad users never see by default. The first impression in the README didn't match the first impression of the running app. 2. The screenshots could not be reused in the getpad.dev marketing site (dark themed) without visible whiplash. TASK-918 (PLAN-911) needs them on the homepage; light-mode captures would have looked like screenshots of some other product. Fix: pass colorScheme: 'dark' via test.use(). Chromium then reports prefers-color-scheme: dark to the page; the layout's matchMedia check no longer matches 'light', so it leaves the document on the default theme — which is dark. Also fixed a typo in the re-run instruction in the docstring (the PAD_SCREENSHOTS=1 env var was attached to the wrong command). Re-captured all three screenshots (dashboard, board, list) under the new config. Docstring updated to call out the theme rationale so future maintainers don't accidentally flip it back. |
||
|
xarmian
|
10309fc599 |
fix(config): read PUBLIC_URL for emailed link generation (BUG-899) (#318)
* fix(config): read PUBLIC_URL for emailed link generation (BUG-899) The Pad Cloud deployment binds pad to 0.0.0.0 (Dockerfile, k8s configmap, pad-cloud's docker-compose) and never set PAD_URL on the pad service, so cfg.BaseURL() fell through to "http://0.0.0.0:7777" — that string ended up in password-reset (and invite + share-link + admin-invitation) emails and was unreachable to recipients. Adds a PUBLIC_URL env var read by the server only (does not flip CLI to remote mode the way PAD_URL does — PUBLIC_URL is a generic env var name commonly set in unrelated deployment contexts). Stored in a separate Config.PublicURL field consulted by BaseURL() as a fallback after URL. Resolution order in BaseURL(): PAD_URL > PUBLIC_URL > host:port. Also logs a WARN at server startup if the resolved base URL has an unspecified bind-all host (0.0.0.0, ::, [::]) — a backstop that would have caught BUG-899 the first time email went out. Tests cover the precedence ladder, mode-not-flipping, PAD_URL-beats- PUBLIC_URL, and the BUG-899 repro shape (Host=0.0.0.0 with no URL set yields the broken http://0.0.0.0 URL). Companion change in pad-cloud/docker-compose.yml passes PUBLIC_URL through to the pad service so the Cloud deployment stops shipping broken email links. Parent: BUG-899 (TASK-908). * fix(config): keep PUBLIC_URL out of IsConfigured() per Codex review (round 2) PUBLIC_URL was setting LoadedFromEnv = true, which IsConfigured() consults to decide whether the CLI has explicit configuration. A generic PUBLIC_URL in the environment (very common name) would have made any host appear "configured" to the CLI and skipped the not-configured / setup branch — the exact footgun the separate-field design was supposed to avoid. PUBLIC_URL is purely a server-side fact; LoadedFromEnv is purely a CLI affordance. Stop conflating them. Adds a focused regression test pinning the IsConfigured() invariant. * fix(config): split PublicLinkBaseURL from BaseURL per Codex review (round 3) Round 2's BaseURL fall-through to PublicURL leaked PUBLIC_URL into ~20 CLI-client call sites (cli.NewClientFromURL(cfg.BaseURL()) patterns across cmd/pad/main.go, init.go, server_info.go, configure.go) — same footgun the separate-field design was meant to avoid: a developer with a host-level PUBLIC_URL set for unrelated reasons would have their CLI silently route requests to that URL instead of the local server. Restore BaseURL() to its original CLI-only contract (URL > host:port). Add PublicLinkBaseURL() with the URL > PublicURL > host:port ladder that's used at exactly the two server-side call sites that build emailed-link targets: - cmd/pad/main.go:279 srv.SetBaseURL(cfg.PublicLinkBaseURL()) - cmd/pad/main.go:464 email.NewSender(..., cfg.PublicLinkBaseURL()) Tests pin both contracts: BaseURL() ignores PublicURL even when set; PublicLinkBaseURL() honors the precedence ladder. PAD_URL still wins in both, preserving back-compat. * fix(config): drop public_url toml tag to prevent CLI persistence per Codex review (round 4) Round 3 left PublicURL serializable to ~/.pad/config.toml via toml: "public_url". A CLI user who runs `pad init` or `pad configure` on a host where PUBLIC_URL is set for unrelated reasons would end up with that URL persisted into their config file, surviving any later unset of the env var and contaminating server-side emailed link generation indefinitely (server reads ~/.pad/config.toml on the next boot). Switch the field to toml:"-". PUBLIC_URL is a deployment-time fact (env var / docker-compose / k8s); operators who want a config-file equivalent already have `url` (the PAD_URL path), which serializes properly. Adds a regression test pinning that Save() never writes PublicURL to the file. |
||
|
xarmian
|
de873d8a01 |
docs(brand): add brand spec defining cohesion contract (TASK-904) (#312)
Foundation doc for PLAN-900 (Cohesive UX between getpad.dev and Pad
Cloud). Defines the visual contract for surfaces that border between
marketing and product so the two codebases (this repo's web/ and
../pad-web) can converge intentionally rather than drift accidentally.
Central thesis (Section 1): cohesion applies at the SEAMS — auth pages
in Cloud mode, error pages, transactional emails — not in the deep
app. Self-hosted installs stay neutral throughout. Every parity
decision is gated on the existing cloud_mode flag (no new env var).
Concrete decisions baked in:
- Canonical color tokens anchored on pad-web/src/app.css; the app
side moves toward those values for bordering surfaces. Accent
palette (blue/green/amber/purple) is already aligned and stays.
- Type families: Inter + JetBrains Mono on bordering surfaces only;
workspace shell keeps system-ui (intentional — system feel inside
a tool).
- Header pattern (fixed top, blur backdrop, max-w-6xl, hamburger
spec) and footer pattern (link order, copyright format) specified
byte-level so a developer can rebuild either from this doc alone.
- Header link list deliberately differs between marketing and auth
pages (marketing carries Login CTA; auth pages don't); footer link
list and order are identical.
Includes a known-drift note flagging --text-muted: #666666 in
web/src/app.css as failing WCAG AA — pad-web's #8a8a93 passes. Out of
scope for this doc; tracked as a fast-follow.
No code changes — pure documentation. docs/ is not embedded in the Go
binary so this doesn't affect builds.
|
||
|
xarmian
|
7cda0d7896 |
feat: rebrand to Perpetual Software + new tagline (IDEA-832) (#273)
Migrates from xarmian/pad to PerpetualSoftware/pad across the entire
repo and updates the product subtitle to "Collaborate with your AI
agents".
Go module rename
- go.mod: github.com/xarmian/pad → github.com/PerpetualSoftware/pad
- All Go imports updated across cmd/pad, internal/{cli,server,store,
models,collections,items,events,metrics,webhooks} (~130 files)
- Test fixtures with the literal repo slug ("xarmian/pad" in JSON
shapes, SSH/HTTPS git URL strings, workspace_context fixtures)
also updated, including the secondary repo entry
(xarmian/pad-web → PerpetualSoftware/pad-web — pad-web was also
moved to the org per branch context)
Docs / config
- README badges, install instructions, brew tap, Docker image, source
build path, sponsor link (sponsor link kept as personal @xarmian)
- Subtitle: "Project management for developers and AI agents." →
"Collaborate with your AI agents." (README, manifests, web layout
meta, .goreleaser homebrew description)
- CONTRIBUTING.md, SECURITY.md, skills/INSTALL.md
- .goreleaser.yaml: homebrew_casks owner, GHCR image, release github
owner, cosign cert-identity regex, comments
- .github/workflows/release.yml: tap/release comments
- deploy/k8s/deployment.yaml: container image
- docs/deployment.md: clone URL
- web/static/{site.webmanifest,manifest.json}: description
- web/src/routes/+layout.svelte: meta description + og:description
Brew tap path is PerpetualSoftware/tap/pad (CamelCase, matches
GitHub user case). GHCR image is ghcr.io/perpetualsoftware/pad
(lowercased per GHCR's URL normalization). CODEOWNERS @xarmian and
FUNDING.yml github: xarmian intentionally retained — those are the
personal maintainer / sponsor account, separate from the org repo.
Verification: go build ./..., go test ./... (all pkgs pass), web
build, and make install all clean (TASK-844, TASK-845).
|
||
|
xarmian
|
afe721d202 |
feat(cli): add Cloud mode to pad init, drop Docker option (TASK-837, TASK-838) (#272)
Merging despite Go (PostgreSQL) red — those failures (TestListItems_FTS_HyphenatedSearchTerm/task-five + TestAdminBillingStats_SidecarSidecarError_DegradesToLocalOnly TempDir cleanup race) are pre-existing on main and tracked in BUG-842. Codex reviewed in 3 rounds (round 1 clean → round 2 found a real semantic bug → fix → round 3 clean). Tests, vet, and lint all green; remaining check failures are documented pre-existing. |
||
|
xarmian
|
29f720c996 |
docs: add real README screenshots (dashboard + board views) (#257)
The README had two TODO placeholders for screenshots that have been
sitting commented-out since the project started. With the launch
imminent, fill them in.
Captures:
- docs/screenshots/dashboard.png — workspace dashboard with Active
Work cards, Active Plans (v0.2 — Collaboration with progress),
collection summaries, recent activity.
- docs/screenshots/board.png — tasks board view, four columns
(Open / In-Progress / Done / Cancelled) with realistic task cards.
- docs/screenshots/list.png — list view (not currently referenced
from the README, but kept as part of the reproducible asset set).
Reproducibility:
web/e2e/screenshots.spec.ts is a gated Playwright spec (skipped
unless PAD_SCREENSHOTS=1) that uses the existing e2e fixture
infrastructure to:
1. Spin up a fresh pad binary against a clean data dir.
2. Bootstrap an admin + workspace seeded with the startup template.
3. Add a realistic demo dataset (1 active plan, 7 tasks across
open/in-progress/done with mixed priorities, 2 ideas).
4. Navigate + capture three views at 1440x900.
To regenerate:
make build
cd web && PAD_SCREENSHOTS=1 PAD_E2E_PORT=17801 \\
npx playwright test screenshots --project=desktop-chromium
Notes:
- Table view (?view=table) was originally in scope but the URL
parser only accepts list/board today; setting via toggle would
require localStorage manipulation. Three screenshots already
cover the README's needs; revisit if/when table view becomes
URL-reachable.
- Dark/light variants were also in scope but the web UI is dark-
mode-only at present, so the captures are dark-only.
Refs: TASK-673
|