mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 06:55:40 +00:00
main
74 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Matt Faltyn
|
43d94a35d5 | fix(cli): pass PostgreSQL connection strings to backup clients (#1289) | ||
|
Matt Faltyn
|
a3a1d5862b | fix(store): preserve valid item references across workspace import (#1271) | ||
|
xarmian
|
cd51d8f130 |
feat(web): Community link (GitHub Discussions) in the user menu and auth footer (TASK-2888) (#1251)
Dave's day-57 call: no Discord; the repo's GitHub Discussions tab is the community channel. COMMUNITY_URL in $lib/brand/links; Community right after GitHub in the user-menu Resources block (Cloud + self-hosted) and the Cloud auth footer; docs/brand.md §7 order updated; AuthFooter and AuthHeader now take their URLs from the links module and sit under the single-source guard. Four codex rounds (two findings fixed, rounds 3-4 clean on the tip), CI 7/7 green. Claude-Session: https://claude.ai/code/session_015A7n836r64Y9THC8UWsDFF |
||
|
xarmian
|
a1716d8170 |
ci(web): decide the npm audit gate from the report, not the exit code, and run it last (BUG-2881) (#1247)
* ci(web): decide the npm audit gate from the report, not the exit code, and run it last (BUG-2881) `npm audit` exits non-zero identically for "a HIGH/CRITICAL advisory exists" and "the advisory service was unreachable". The Web job ran it before Build / Type check / vitest under `bash -e`, so a registry timeout (main, 03:50Z) and a 503 (#1246, 04:33Z) on 2026-09-04 each produced a red row with every frontend verification step SKIPPED — a lane that read like a failure and had asked nothing. scripts/ci-audit.mjs runs the audit in --json mode and decides from the report: metadata.vulnerabilities present → fail iff high+critical > 0, naming the advisories; an error envelope or unparseable output → a GitHub warning annotation saying the gate did not run, exit 0. The step moves to the end of the job so the frontend's own verdict always exists whatever the audit does. Verified locally against five report shapes (transport timeout envelope, E503 envelope, one high advisory, clean, garbage) and two live runs (the real registry: clean; a dead registry: warning, exit 0). `--input <file>` is the seam those checks use. Fixes BUG-2881 * ci(web): the audit gate fails closed — retry an unreachable advisory service, then fail under its own title Codex round 1 on #1247: the first draft warned and exited 0 when the advisory service could not be asked, which made the only supply-chain gate pass exactly when it had not run. A gate that passes when it cannot run is not a gate. Now: up to three attempts with backoff (registry blips are usually seconds long), then `::error title=npm audit did not run` and exit 1. The title is distinct from `::error title=npm audit` (a real advisory) so the checks tab tells the two apart without opening the log; re-running is the remedy for the first and never for the second. Because the step runs last, Build / Type check / vitest have already produced their result either way — the original blindness is gone regardless of which way this step fails. Verified against the same five saved shapes (transport and E503 envelopes and garbage now exit 1 under the did-not-run title; a high advisory exits 1 under the advisory title; clean exits 0) and two live runs (real registry: clean; dead registry: three attempts logged, exit 1). Refs BUG-2881 * ci(web): the audit gate refuses counts it cannot read, and refuses bad tuning without crashing Codex round 2 on #1247. (1) metadata.vulnerabilities was checked for presence, not for shape: Number("x") + Number(null) > 0 is false, so a malformed count read as a clean audit — a second fail-open, one layer deeper than round 1's. high/critical must now be non-negative integers or the report is unreadable, which is the fail-closed path. (2) The two env knobs are operator-set, but CI_AUDIT_ATTEMPTS=NaN left the retry loop unexecuted and threw a TypeError, and CI_AUDIT_BACKOFF_MS=Infinity parked Atomics.wait forever; both now fall back to the default with a line saying so. Refs BUG-2881 * build: the local preflight runs the same audit gate CI does, and runs it last Codex round 3 on #1247 (blast radius): `make web-check` still chained bare `npm audit && npm run check`, so a registry blip stopped svelte-check locally exactly as it had in CI, and CONTRIBUTING documented the bare command as the way to reproduce the gate. New `web-audit` target runs `npm run audit:ci`; `check` runs it after web-check and web-test, mirroring the Web job's order. CONTRIBUTING and docs/architecture.md say so. Refs BUG-2881 * build: web-audit stands alone — no `web` prerequisite, so `check` runs npm ci once and no new target reaches it Codex round 4 on #1247: `web-audit: web` made `check` run `npm ci` twice (`web` is .PHONY) and added a target CLAUDE.md's worktree rule did not list as reaching `npm ci`. `npm audit` reads the lockfile and needs neither node_modules nor a build — verified by running it with node_modules removed — so the prerequisite goes; CLAUDE.md's safe list gains `web-audit`. Refs BUG-2881 |
||
|
xarmian
|
0363c139a9 |
fix(store,cli): the oracle failed open, and it over-refuses one column (BUG-2810)
Three findings from codex round 5, all real; the third corrected a claim I had
made about the design.
**The suspect path could leave data unrepaired and exit 0.** The CLI printed
SuspectsFailed and then returned nil, checking only the violation bucket. A
script sees success; an operator who trusts the status moves on. Both buckets
now decide the exit code, and the decision is extracted into
nulRepairExitError so it is testable without a database — the bug was in the
decision, not in the repair, and a test that needs a fixture to reach it is a
test nobody writes.
**The destination oracle failed open.** Connection failures, timeouts and
read-back errors were bucketed with "the destination answered, about something
else" — reported and not refused on. So an UNVERIFIED suspect passed the
preflight, which is the defect the suspect class was added to correct arriving
by a different route.
There are now three outcomes rather than two: the server answered with a NUL
code (refuse), the server answered with another complaint about the value
(report, because a NUL preflight that quietly grew into a general one would
block migrations unrelated to this bug), and the server never answered
(REFUSE). ErrDestinationCheckUnavailable carries the third, and
TestDestinationOracleFailsClosedOnAnUnusableConnection pins it against a real
closed pool — with an open-pool control first, since a classifier that answered
"unavailable" for everything would satisfy the assertion and refuse every
migration.
**The oracle is not a perfect model of the migration, and I said it was.**
Codex claimed workspaces.settings is normalised on import, so the cast
over-refuses there. Measured rather than argued, by importing the same
shadowed-duplicate value into three columns against a real server:
workspaces.settings -> import SUCCEEDS, stored as {"a": "clean"}
items.fields -> import FAILS, SQLSTATE 22P05
collections.schema -> import FAILS, SQLSTATE 22P05
CreateWorkspace runs models.NormalizeWorkspaceSettings, a map round-trip that
drops the shadowed member. So the claim was right, and my own runtime demo
earlier on this branch — which used workspaces.settings — was showing a
spurious refusal.
The cast STAYS. That row is a value Layer B refuses on every write today and
exists only because it predates enforcement, so surviving the migration is an
accident of one column's normaliser rather than a property worth preserving,
and repair-nul clears it in one command. Deriving "would this column's writer
normalise it" is a per-column enumeration, which is the shape this cluster
keeps proving unmaintainable.
What changed is the CLAIM. The file header no longer says the oracle is "exact
in both directions" — it is exact about the VALUE and is not a model of the
MIGRATION; the refusal no longer tells an operator PostgreSQL would reject the
row, only that the value carries a NUL jsonb refuses; and the measurement and
the over-refusal are written into CheckJSONBAcceptable's doc comment and
docs/backup.md, which also now states that the check errs toward refusing.
The disposition is flagged to the lead rather than settled here: skipping
normalised columns is a scope call, not mine.
|
||
|
xarmian
|
57b7ca5f48 |
feat(store,cli): ask the destination about suspects instead of dropping them (BUG-2810)
Day-54 lead ruling on PR #1233, and the ruling names the defect precisely: the scan's own SQL pre-filter already surfaces the shadowed-duplicate row as a candidate, and `ParameterRefused` then drops it. So the preflight was discarding information it was holding and going on to promise the migration would go through. I had recorded that as an accepted residual on the grounds that closing it would violate DOC-2823's one-layer rule — but that rule is about what the enforcement layers REFUSE. It says nothing about a preflight throwing away a candidate it had in hand. **The SUSPECT class.** A pre-filter hit the predicate does not refuse. Most are doubled-backslash literals — text that writes ABOUT the escape, which is the false positive this whole predicate family exists to avoid. One member is not: a NUL in a value shadowed by a LITERAL duplicate key, which a map-model decode drops and PostgreSQL refuses. Nothing here can tell them apart, so nothing here tries: `pad db scan-nul` lists them under their own heading, apart from the violations, with what resolves them. **The destination is the oracle.** `pad db migrate-to-pg` casts each suspect on the TARGET connection — `SELECT $1::jsonb`, side-effect-free, and the very cast an INSERT performs — and refuses on 22P05 / 22021. That is exact in both directions precisely because it is not a fourth opinion of ours. Measured against a real server: the literal is ACCEPTED, the shadowed duplicate is REFUSED with 22P05, and a non-JSON value fails for a reason that is reported rather than refused on, because a NUL preflight that quietly grew into a general one would block migrations unrelated to this bug. **The repair had to be measured, not assumed, and the answer changed the design.** `textguard.Repair` leaves the shadowed value completely untouched: its scanner is gated on DocumentDecodesNULAnyShape, a map-model question that answers false for exactly this shape, so it never runs. A preflight that refused the row and printed `pad db repair-nul` would have been printing a command that does nothing to it — a remedy nobody ran (PATTE-135). So the repair reaches the class through the token-level scanner, exported for this, which rewrites the shadowed escape and still leaves the literal byte-identical because it consumes escapes in order. Suspects get their own buckets in the repair report rather than being folded into Repaired, so the dry run's promise and the run's result stay the same number. **Nothing about what any layer REFUSES changed.** textguard.KnownGaps and its pin are untouched, and TestScanNULInheritsTheRecordedKnownGaps still asserts the scan does NOT detect the shape. TestSuspectsCollapseWhenBUG2812Lands fails when the token-walk makes that false, and names every file to delete — the suspect path is a second mechanism that exists only while the predicate is blind. **One defect this found that no test did.** Running the real command against a real Postgres, the refusal announced "0 stored value(s) carry a NUL; nothing was migrated" while listing one — the count used the violations only, and the tests asserted the message CONTAINED "nothing was migrated" without reading the number. Fixed, and the assertion now reads the count. The whole loop is now verified end to end: preflight refuses, `repair-nul` fixes, the migration completes. My own prose from earlier on this branch is corrected with it. ScanNUL's doc comment, the preflight's, and docs/backup.md all said this shape passes the preflight and fails mid-copy, which the same commit makes false. |
||
|
xarmian
|
178b6b5010 |
fix(server,store): two more from codex rounds 3 and 4 (BUG-2810)
**The import repair could silently change what gets imported.** It decodes into map[string]any, where a repeated object member keeps only the LAST value. The TYPED decode that runs next does not agree: encoding/json unmarshals members in order into the same struct field, so two `"workspace"` objects MERGE there and collapse here. A body with duplicate members would therefore import differently with --repair-nul than without, which is outside what a flag by that name may do. It now DECLINES such a body: returns it untouched, lets the gate judge it exactly as it would without the flag, and says why in the refusal — "the payload repeats the member X, and repairing it would change which value is imported". Detection is a token walk, because a decode is what loses the information: by the time there is a map the duplicate is gone. The detector's own test carries the false positive that matters — the same member name in SIBLING objects is not a duplicate, and a single shared set of names would decline every real export, since items all carry `id`, `title`, `slug`. Rewriting such a body faithfully wants a token-preserving pass, which is BUG-2812's token-walk and not a rider on this. A real export cannot contain duplicate members (json.Marshal does not emit them), so declining costs nothing an operator meets by accident. The tally now owns the repair — decodeJSONRepairingNUL takes it and calls Apply — so the count and the declined reason come back through one object instead of a return value a caller has to remember to record. That is the same mistake this branch already made once, when the JSON path dropped the count and the header reported 0 for an import that had rewritten a value. **A row the repair could not address was reported as a failure.** A NUL in a key column the list does not protect, on a row whose violation is elsewhere, makes the address unbindable: Layer A inspects every bound parameter, including a WHERE clause's, so the lookup is refused before SQLite is asked to find the row. It landed in Failed carrying "invalid text parameter: parameter 2" — the same information phrased as a fault in the repair rather than a property of the row. Now detected up front and reported as a skip with the reason, alongside the two skips that already existed. **One finding NOT fixed, deliberately, and recorded instead.** Round 3 raised that the scan misses a NUL in a value shadowed by a LITERAL duplicate key, so such a row passes the migrate-to-pg preflight and then fails during the copy — the exact failure the preflight replaces, surviving for one shape. That is textguard.KnownGaps: a blind spot every layer shares on purpose, which DOC-2823 forbids closing in one layer alone, because layers disagreeing about one value is the defect this cluster is made of. So it is named in ScanNUL's doc comment, in the preflight's, and in docs/backup.md for the operator, and TestScanNULInheritsTheRecordedKnownGaps pins the miss and FAILS when it stops being one — the notification that BUG-2812 has landed and those three prose sites need updating. The consequence is recorded on BUG-2812's trail. Round 2's single finding was refuted rather than fixed: it predicted TestRepairFlagReachesTheNestedAndObliqueForms would fail, on a mechanism that describes the raw-byte scanner this branch had already replaced. The test passes; the outer decode resolves the oblique spelling before the walk sees it. |
||
|
xarmian
|
49bd342e4c |
fix(store,server,cli): three defects from codex round 1 (BUG-2810)
**The import flag could not repair the column it exists for.** `--repair-nul` scanned the RAW body for a live escape, which is right for a value the gate reads at the top level and wrong for the one that actually matters. An item's `fields` blob travels through an export as a STRING: a NUL escape in the stored blob marshals into the body with a DOUBLED backslash, which a raw scan must leave alone because at that layer it is literal text — while the gate refuses it anyway, since it decodes the body and re-parses that string as the document it is. So the repair now walks the DECODED body with the same classing bodyDecodesNUL uses, one verb changed: where the gate asks textguard whether a value decodes to a NUL, this asks textguard to repair it. Two walks of one shape in one package is a real risk, and the mitigation is that they are measured against the same corpus in both directions rather than reviewed for similarity — TestBodyRepairMirrorsTheGateOverTheCorpus drives every case through the body shape and asserts refused-becomes-accepted and accepted-stays-byte-identical. Two consequences worth stating. The walk also reaches the OBLIQUE spelling — the backslash written as its own escape, so the six characters never appear in the raw bytes at all — which the scanner could not, so the test that pinned that limit is replaced by one asserting the capability. And re-encoding is now possible, so it is bounded: UseNumber, so an integer wider than float64 is not silently re-emitted in scientific notation; SetEscapeHTML(false); and a body with nothing to repair is returned byte-identical rather than round-tripped. The mutation that removes UseNumber turns 9007199254740993 into ...992, and a test says so. The header is now X-Pad-Repaired-NUL-Values, because at the decoded layer an escape is not a thing that exists any more and one nested document may have carried several. **The scan could not run on the databases it exists for.** Several protected tables carry a NULLABLE workspace_id — activities, api_tokens, mcp_audit_log — and the scan selected it into a plain *string, which fails with "converting NULL to string is unsupported" and takes the scan, the repair and the migrate-to-pg preflight down with it. Every column is now scanned as sql.NullString: SQLite also permits NULL in a declared PRIMARY KEY that is neither INTEGER PRIMARY KEY nor NOT NULL, which no other engine does, and a NULL key cannot address a row for an UPDATE — such rows are reported and skipped with the reason rather than handed a WHERE that matches nothing. Verified against the unfixed code: the scan returned `scan activities.actor row: sql: Scan error ... converting NULL to string`. It needed a VIOLATING row in such a table, which is why every fixture that planted its rows in `items` missed it. **--force by accident.** The repair skipped the running-server check whenever --from was given — and the most natural --from an operator types is the path `pad db scan-nul` just printed, which IS the live database. The check is now on the resolved path (Abs + EvalSymlinks, so a symlinked data directory or a relative path still matches), and a --from naming an unrelated backup stays unguarded, which is correct: nothing is writing it. The ordering moved with it. `store.New` runs pending migrations, so the refusal now happens BEFORE the database is opened; opening first and refusing second made the guard arrive after the thing it guards against. |
||
|
xarmian
|
63da2f4f5f |
feat(store,server,cli): count and repair the legacy NUL population (BUG-2810)
Layers A and B stop the value being written. Neither makes a row that
already carries one go away, and BUG-2810's filing is what that costs: an
affected workspace exports with a 200 and re-imports with a 400, so a
self-hoster restoring their own backup is blocked with no path forward in
the product, and `pad db migrate-to-pg` fails partway through the copy
against PostgreSQL's jsonb parser rather than up front.
This is DOC-2823's S3, on Dave's day-54 rulings: U+FFFD as the replacement,
repair standalone only with a migrate-to-pg preflight that refuses and
prints the command, `--repair-nul` on import shipping default-strict.
ONE REPAIR, beside the one predicate. textguard.Repair lives next to
ParameterRefused because four layers that agree about what is REFUSED and
disagree about what a repair PRODUCES is this bug family arriving one step
later. Its contract is a property over the same corpus, in both directions:
every refused value becomes one all four layers accept, and every accepted
value comes back IDENTICAL. The second half is the load-bearing one — a
repair that tidies values nobody complained about rewrites
`{"a":"x\\u0000y"}`, six literal characters after a doubled backslash, and
corrupts it.
The JSON arm is a string-literal SCANNER, not decode-walk-remarshal, which
is what the recon write-up proposed before it was written. Re-marshalling
changes four things nobody asked to change — object key order, insignificant
whitespace, integers wider than float64, HTML-ish characters — and silently
drops one of a document's LITERAL duplicate keys, which is a gap BUG-2812
owns and the last thing a repair should do. Scanning copies every byte it
does not deliberately rewrite, so an untouched document is byte-identical
without that having to be argued. A substring replace is not equivalent and
the test that proves it took a mutation to find: a doubled-backslash literal
ALONE never reaches the scanner, so the discriminating fixture is one
document carrying a live escape AND a literal.
THE COUNT IS COMPUTED IN GO. Measured on the read path in this worktree: a
row planted with `bad<NUL>name` reads back into a Go string with all 8 bytes
and the NUL intact, while `length(name)` in the same database answers 3.
TASK-2824 found that C-truncation and concluded no DB-side REPAIR could be
trusted; the same measurement on the read path says no DB-side COUNT can be
either. SQL narrows — `instr(col, char(0))`, plus the escape prefix on
JSON-classed columns, which is textguard's own pre-filter — and never
decides. The decision stays ParameterRefused with isJSON from the shared
86-column list, i.e. Layer B's classing.
Row addressing is read from the live schema rather than a hand-kept map:
39 tables carry protected columns, one (item_wiki_links) declares no primary
key and is addressed by rowid, two have composite keys, and five have a
single key that is not `id`. The repair checks RowsAffected because an
address that stopped selecting its row would otherwise commit an UPDATE that
touched nothing and report it as repaired — the one failure an operator
cannot see in the output.
`email_optouts(email)` is both a protected column and its own primary key.
Repairing it changes the row's identity and can collide with an existing
row, which in that table means somebody starts receiving mail again. It is
reported and skipped, with the reason.
The import flag is NOT an exemption from the gate. `--repair-nul` buys the
body one repair attempt and then runs the same `bodyDecodesNUL` on the
repaired bytes, which still decides — a decode path that skipped the check
is the door BUG-2803 spent thirty rounds closing, on the endpoint carrying
the largest attacker-controlled body in the product. Only the ESCAPE form is
repaired: a raw NUL byte makes the document invalid JSON, and widening what
parses is not this flag's job. Both doors are covered, JSON and tar.gz,
because giving them different answers is how one of them keeps being
forgotten.
Postgres is settled with evidence rather than sent up as a ruling: it cannot
hold either defect (22021, 22P05) and the four-way differential test already
pins that, so the scan reports not-applicable WITH the reason rather than
returning a zero a reader could mistake for a clean database.
Spellings settled here, per the dispatch: `pad db scan-nul` and
`pad db repair-nul` as siblings rather than `repair --nul`, matching
`migrate-to-pg`'s hyphenated compound — a repair verb that errors when given
no flag is a worse shape, and there is no second repair to share it with.
scan-nul IS the dry run, so repair-nul grows no --dry-run. It refuses while
the server is running unless --force, on the `pad db restore` precedent: the
report is a claim about a database, and one somebody else is concurrently
writing makes it a claim about a moment that has passed.
docs/backup.md's section on this is rewritten. It still said the rule lives
in the binary and not the database, which S2 made false, and it pointed at
this item for a preflight and a repair that now exist. Its import examples
also showed `pad workspace import < file`, which has never worked — the file
is an argument.
Closes BUG-2810.
|
||
|
xarmian
|
ba1255881d |
fix(server): refuse a decoded NUL in a JSON request body (BUG-2803) (#1220)
* fix(server): refuse a decoded NUL in a JSON request body (BUG-2803)
The body half of BUG-2782 (path) and BUG-2784 (query). A caller-supplied
string reached a Postgres text parameter, Postgres refused it, and the
handler answered 500 — the honest answer is 400.
WHY THE TRANSPORT RULE CANNOT BE EXTENDED, which is the whole reason this
is a different fix rather than a wider middleware. ValidateQuery works
because a decoded query value is a substring of the raw query with ASCII
substitutions: the bad byte in the raw text IS the bad byte in the value.
That property fails for a JSON body — the reachable NUL arrives as the
six-character escape, all ordinary ASCII — so no request middleware can
find it without decoding the body, which is the handler's job.
MECHANISM, each premise measured against encoding/json rather than
reasoned about:
raw NUL inside a string -> decode ERR (invalid character in string literal)
raw NUL after the value -> decode ERR
the escape in a value -> decodes to a string CONTAINING a NUL
the escape in a KEY -> same
a DOUBLED backslash -> decodes to literal text, NO NUL
the uppercase spelling -> not a JSON escape at all
So the escape is the only vector and its substring is a sound FAST PATH
(absent -> no NUL possible), but not a sufficient test: a doubled
backslash carries the same six characters and decodes to text. In this
product that is not hypothetical — items and documents store markdown,
and a document about JSON escapes is an ordinary thing to write. The
exact step is json.Decoder.Token(), which returns DECODED strings, covers
object keys and arbitrary nesting (an item's fields blob), and needs no
knowledge of the destination type.
NOT REFLECTION over the decoded value, the other obvious design: it sees
[]byte fields AFTER base64 decoding, so a body carrying legitimate binary
({"b":"AQAC"} -> bytes 01 00 02) would be refused for a NUL that is not
text. A token walk sees the base64 characters. No request struct has such
a field today (searched: []byte with a json tag in internal/server and
internal/models, non-test — only models.YjsUpdate.UpdateData, which no
handler decodes from a body); the token walk is chosen so adding one
later cannot silently start rejecting valid requests.
BUFFERING IS NOT A COST. json.Decoder.Decode already holds the whole
top-level value in memory — refill accumulates into dec.buf and grows it
by doubling (encoding/json/stream.go) — so streaming never avoided the
copy. Measured on the 64 MiB workspace-import shape, total allocation:
stream+Decode 354.7 MiB, ReadAll+Unmarshal 256.5 MiB, ReadAll+Decode
512.5 MiB. Peak heap is order-dependent and does not discriminate; the
first run of that measurement showed a 0.77x peak win that vanished when
the legs were swapped, so only the allocation figure is claimed.
POPULATION, measured on Postgres 17 through the real router with a
control leg on every endpoint (92 mutating routes enumerated via
chi.Walk; 13 probed):
before: 12 of 13 DOOR (control 201 / NUL 500, SQLSTATE 22021)
after: 0 of 13 — every NUL leg 400, every control leg unchanged
Confirmed doors: workspace name, collection name, item title, item
content, item fields value, item title via PATCH, comment body, agent
role name, view name, document title, webhook secret, workspace import.
workspace-token name is UNMEASURED, not clean — its control leg 500s on
an unrelated FK in this fixture. The other 79 routes are unprobed, not
claimed clean; the completeness argument is structural instead, and
enforced by a test rather than asserted.
SECOND DEFECT, named rather than slipped in: the six handlers that
decoded straight off r.Body had no http.MaxBytesReader either — the cap
decodeJSON has always applied — so each was an unbounded body read.
Routing them through decodeJSON closes that too.
COMPATIBILITY: json.Unmarshal refuses trailing non-whitespace after the
JSON value where Decode ignored it. Deliberate, same direction as this
fix, and the only behaviour change beyond the refusal. Trailing
whitespace still passes. An EMPTY body still returns a wrapped io.EOF,
because handlers_playbooks.go reads errors.Is(err, io.EOF) as "no
arguments supplied" — caught by TestPlaybookRunAcceptsEmptyBody, which is
exactly the wiring a helper-level change is blind to.
No call site changed for the refusal itself: all 65 decodeJSON callers
already turn a decode error into a 400 carrying err.Error().
Release note: a NUL character in a JSON request body now returns 400
instead of 500 on Postgres deployments.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): follow the NUL refusal into JSON-encoded string fields (BUG-2803)
Codex round 1 on #1220: the check scanned ONE JSON layer, and several
fields cross the wire as JSON-ENCODED STRINGS rather than nested objects
— an item's fields, a collection's schema, a workspace's settings. The
OUTER decode of {"fields":"{...}"} yields the inner document as literal
text, in which the escape is still six ordinary characters and no NUL
exists, so the single-layer token walk passed it.
MEASURED on Postgres 17 with a control leg on each, after the
single-layer check was already in place:
item.fields as a JSON-encoded string 500 control 201
collection.schema as a string 500 control 201
workspace.settings as a string 500 control 201
The error is DIFFERENT from the rest of this family, which is why it is
worth reading rather than assuming:
insert collection: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05)
22P05, not the 22021 the path and query halves produce. The outer string
is pure ASCII so it never trips the text-encoding check; this is
Postgres's own JSON parser refusing the escape inside a document bound
for jsonb, which cannot represent a NUL. After this change all three
answer 400 with their control legs unchanged.
THE FIX: when a decoded string is itself a complete JSON object or array
— the class this API re-parses downstream — walk it too, to a depth
bound of 8. Recursion terminates on its own (each level is a strict
substring of the one above); the bound keeps a hostile body from buying
many full re-parses, and AT the bound the body is refused rather than
passed uninspected, since the escape is known to be present and the walk
has stopped looking.
WHAT THIS OVER-REFUSES, by design and pinned by a test: the rule is
structural, not destination-typed, so a plain TEXT field whose ENTIRE
value is a valid JSON document carrying the escape is refused too, even
though its column would have stored it. Prose ABOUT a JSON escape does
not parse as a bare document, so the case is narrow, and a value of that
shape breaks any consumer that parses it. The destination-typed
alternative — an allow-list of the fields that arrive JSON-encoded — is
exactly correct and goes stale in silence, which is the failure mode
ValidateQuery's comment rejects when it explains why per-site query
validators could not be written.
Tests: nested documents (fields/schema/settings/array/twice-encoded),
with controls for ordinary content, a doubled backslash INSIDE the
nested document, a string that starts like JSON but does not parse, and
prose that merely mentions the escape; the over-refusal pinned as a
decision rather than left as an accident; the depth bound; and a wiring
leg through the real router on SQLite, where the write would otherwise
SUCCEED so a green cannot be the database doing the work. Fixtures build
their JSON-encoded strings with encoding/json rather than hand-written
backslashes, since the escaping rules are the subject under test.
All four new tests fail with the recursion removed.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): build the NUL-bearing timeline fixture through the store (BUG-2803)
TestTimeline_NeverEmitsACursorItWouldRefuse built its fixture through the
API on a premise its own comment stated: "a structured id comes from the
item's fields blob, which nothing validates on write". BUG-2803 made that
false — decodeJSON now refuses a body whose strings decode to a NUL,
including one nested inside a JSON-encoded `fields` string — so the API
can no longer produce the row and the test 400'd on its fixture.
Repaired rather than deleted, because the DEFENCE it covers is still
live: rows in this shape can predate the rule, and the store has no such
check of its own, so a migration, an import or any future non-HTTP writer
can still produce one. The timeline must keep refusing to hand out a
cursor it would then reject.
The fixture now writes the blob directly, injecting the six-character
JSON escape rather than a raw NUL — the blob is JSON text and both
backends reject a raw NUL in it; the NUL comes into existence when Go
DECODES the blob, which is exactly how the timeline ends up with one
inside an entry id. The test is not vacuous under the change: it asserts
the NUL-bearing id took the positional fallback, so an injection that
failed to produce a NUL fails the test rather than passing quietly.
This is the CONVE-23 case — a change that falsifies existing prose owes a
sweep for that prose. The stale sentence was found by the test failing,
not by the sweep, which is the weaker of the two ways to find it.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* docs(server): correct the timeline comment BUG-2803 falsified (CONVE-23)
The entryID fallback's comment said note and decision ids "come from the
item's fields blob and nothing validates them on write". BUG-2803 made
that half false: the HTTP API now refuses a request body whose strings
decode to a NUL, including one nested inside a JSON-encoded `fields`
string. The sentence was true when written and nothing in this branch's
diff pointed at it.
The fallback still has to exist, and the corrected comment says why:
the STORE has no such check, so rows predating the rule — and anything
writing a blob by another path, a migration, an import, a future
non-HTTP writer — can still carry one.
SWEPT AND DELIBERATELY LEFT: two nearby comments
(handlers_timeline_id_collision_test.go, handlers_timeline_structured_test.go)
also say "nothing validates them on write". Both are about id FORMAT and
DUPLICATION — an imported artifact carrying a UUID-shaped id, a
hand-written blob repeating one — and this change validates neither. In
context those sentences remain true, so they are left alone rather than
edited into noise.
Sweep command: grep -rniE "nothing validates|not validated on write|no
validation on write|unvalidated" --include=*.go internal/ cmd/ — six
further hits, all about other subjects (github_pr raw writes, terminal
schema keys, push payload format, decodeJSON's size bound).
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): scope the nested-NUL walk to JSON-encoded fields (BUG-2803)
Codex round 2 on #1220. The nesting check from the previous commit
recursed into ANY string that parsed as a JSON document, on the argument
that a structural test beats a destination-typed one. That argument was
wrong in a way I had written down as an accepted trade and should have
weighed as a defect: a plain-text `content` value holding a JSON snippet
that merely MENTIONS the escape was accepted before this branch, is
stored in a text column that has no problem with it, and was newly
refused — including on RE-IMPORT of an export carrying it.
Refusing input the server itself produced is a worse failure than the
door the unscoped recursion was closing. Measured before the fix: a
workspace whose item content held such a snippet exported 200 and
re-imported 400.
The walk now descends only under keys whose STRING value is a JSON
document something downstream re-parses: config, events, fields,
metadata, phase_data, plan_overrides, schema, settings, tags, traits.
WHY A LIST IS SAFE HERE, when ValidateQuery's comment rejects exactly
this shape for query parameters: there the set of names is unbounded by
design (parseItemListParams turns any unrecognised parameter into a field
filter), so no list could be complete. Here the set is a closed property
of the wire model — a field is JSON-encoded because a Go struct declares
it as a string holding JSON — and
TestJSONEncodedFieldKeysCoversTheModels derives it from internal/models
and fails when a new one appears. The list cannot go stale in silence.
Over-inclusion is the safe direction and the list takes it: a listed key
that is not really JSON-encoded costs one parse attempt and can only
refuse a complete JSON document carrying the escape, while a missing key
reopens a door. `traits` is listed for that reason — it carries JSON but
its declaration has no comment saying so, which is exactly how the
derivation test would have missed it, so the test asserts coverage in one
direction only and the list is allowed to be a superset.
The walk also changed shape: decoding into `any` and walking the value,
rather than a token stream, because key context is needed to know which
subtree is JSON-encoded. The []byte reasoning is unchanged and still
holds — decoding into `any` never produces a []byte, so a base64 field is
seen as its ASCII text rather than as decoded bytes that might contain a
legitimate 0x00.
Tests: text fields carrying a JSON document are ACCEPTED (five keys),
with a leg proving the same document under a JSON-encoded key is still
refused, so the pair differs only in the key; the derivation test; and
the depth-bound fixture now nests under a JSON-encoded key at every
level, since nesting under an ordinary key would never start the
recursion and would have passed for the wrong reason.
STILL OPEN, and the lead holds it: a LEGACY row whose stored fields blob
already carries the escape still exports 200 and re-imports 400. That is
data this fix cannot make importable without weakening the write-side
refusal, and the disposition (repair sweep, flagged import, or documented
acceptance) is a product ruling. Recorded on the item.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): close the three body doors codex round 3 found (BUG-2803)
All three verified before fixing, none taken on the reviewer's word.
1. BUNDLE IMPORT BYPASSED THE REFUSAL (P1). handlers_import_bundle.go
parses pad-export.json itself rather than through decodeJSON, so the
SAME workspace import — reached with Content-Type application/gzip
instead of application/json — walked straight past the NUL check into
Postgres. The bundle's export blob is now checked with bodyDecodesNUL
before ImportWorkspace, answering the same 400. Test drives a real
tar.gz through the router with a clean-bundle control leg, because this
path answers 400 for a dozen unrelated reasons (bad gzip, out-of-order
tar, duplicate entries) and a bare 400 would prove nothing.
2. ONE CALLER SWALLOWED THE NEW ERROR (P2). handlers_admin.go's
test-email endpoint read `if err := decodeJSON(...); err != nil ||
input.To == ""` and fell back to the admin's own address, so a body
carrying a NUL answered 200. An ABSENT body legitimately means "send it
to me"; a body that is present and REFUSED is a different thing, and
collapsing the two turns a validation error into a success. The two
cases are now separated on errors.Is(err, io.EOF).
3. THE COMPLETENESS TEST COULD NOT SEE PAST TWO CALL SHAPES (P2). It
scanned for json.NewDecoder(r.Body) and io.ReadAll(r.Body), so it was
blind to io.ReadAll(io.LimitReader(r.Body, n)) — a shape ALREADY in the
package — and to any alias or helper. A completeness test that misses a
live example is worse than none, because it reads as coverage. It now
scans for the thing that cannot be spelled around, a reference to the
request body at all, and requires every FILE touching one to be
accounted for with a written reason. Both directions are asserted: an
unaccounted file fails because a door may have opened, and an accounted
file that no longer touches a body ALSO fails, so the list cannot rot
into stale excuses that quietly cover a future reader. Verified with a
positive control (an added body reference in an unlisted file fails) and
a negative one (a stale entry fails).
FOUND BY THAT WIDENED SWEEP, and fixed here rather than filed: the raw
artifact import (POST /workspaces/{ws}/import-artifact) takes TEXT, not
JSON, so it never went through decodeJSON and inherited neither the NUL
refusal nor the path/query rule — a body is neither. A raw NUL or
invalid UTF-8 reached the store and Postgres answered 22021, which the
handler turned into a 500 for what is a client error. It now applies
bindableText, the same predicate ValidatePath and ValidateQuery use, and
answers 400 invalid_body. Note the shape difference from the JSON half:
there the ESCAPE is the vector because a decoder rejects a raw NUL;
here the RAW BYTE is, because nothing is in the way.
Each fix has a mutation run against it: disabling the bundle guard fails
the bundle test, disabling the artifact guard fails the artifact test,
and both controls still pass.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): the escape gate was unsound, and YAML has its own (BUG-2803)
Codex round 4, two P1s, both reproduced before fixing.
1. THE FAST PATH LET A REAL NUL THROUGH. bodyDecodesNUL gated on "does
the raw body contain the six-character escape". That is unsound: the
BACKSLASH itself can be written as an escape, so a body carrying
\u0000 contains no literal six-character sequence anywhere in its
raw bytes, while the OUTER decode manufactures one inside the string —
and if that string is re-parsed as a JSON document (jsonEncodedFieldKeys)
the second parse turns it into a real NUL.
Measured through the real router before the fix: the oblique spelling
answered 201 where the direct one answered 400.
The mistake was applying a fact about how a NUL is spelled INSIDE a
decoded string to the RAW BYTES, where the backslash can itself be an
escape. That is the same layer-confusion this whole bug is made of, for
the third round running.
The gate is now a BACKSLASH. Every JSON escape mechanism requires one, so
a body with no backslash has decoded strings byte-identical to its raw
bytes, and a raw NUL cannot survive the decoder — no backslash therefore
means no NUL, at any depth, however spelled. Bodies WITH one pay for an
exact answer, a larger set than before (any nested JSON carries a
backslash-quote), which is the cost of being correct. The same
correction applies to the per-string pre-filter one level down.
2. YAML HAS ITS OWN ESCAPE VOCABULARY. The raw bindableText check added
last commit passes a double-quoted scalar `title: "a\0b"` — no NUL in
the request bytes — and the YAML decode manufactures one. Measured
before the fix: that artifact imported 201 with a NUL in the item title.
The decoded artifact is now checked too: title, body, and every
frontmatter field value, walked because a playbook's `arguments` is a
nested structure rather than a scalar. Keys are checked as well as
values, on the same precautionary grounds ValidateQuery states for
query parameter names.
Same shape as the JSON half in both cases: a value that is harmless
until a SECOND parse, checked at the layer that can see it.
Tests: the oblique spelling joins the nested-document table, and the
YAML escape joins the artifact table. Each is mutation-verified —
reverting the gate to the substring fails the oblique case only, and
disabling the post-decode artifact check fails the YAML case only, with
the raw-byte cases still killed by the raw check. That per-leg
discrimination is the point: it shows each check earns its own keep
rather than being covered by its neighbour.
Prose corrected where this falsified it: jsonNULEscape's "it is the ONLY
spelling" is true of the escape and was being used to justify a filter on
the raw bytes, which is a different claim. Both now say so explicitly.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): multipart text fields and the bundle manifest (BUG-2803)
Codex round 4's two P2s. Both are the same shape as the rest: a
caller-supplied string reaching a text comparison through a door the
earlier fixes did not cover.
1. MULTIPART TEXT FIELDS. The multipart body is deliberately exempt from
the JSON rule — its payload is binary blob content and must not be
scanned for text validity — but its TEXT fields are a different thing.
`item_id` goes to ResolveItem and into a database comparison exactly as
the query-string channel does, and that channel has been validated at
the transport since BUG-2784; the form channel was not. multipartValues
now drops values that are not bindable text, which makes an unusable
value indistinguishable from an absent one — the disposition
resolveUploadItemID already applies to empty values.
The uploaded FILENAME gets the same predicate, with a fallback to a
generic name rather than a refusal: the bytes are fine, only the label
is unusable.
A NEGATIVE RESULT worth recording, because it changed the test: a RAW
NUL in the multipart header is NOT the vector. Go's multipart reader
refuses it as a malformed MIME header line before any handler sees it
(measured: 400, "malformed MIME header line"). The reachable spelling is
the RFC 5987 encoded form, filename*=UTF-8''sh%00ot.png, which the
header parser accepts and percent-decodes afterwards. The first version
of this test used the raw form and was testing a vector that does not
exist.
2. THE BUNDLE ATTACHMENT MANIFEST. A second JSON document inside the
tar.gz, parsed directly like pad-export.json was, so it needed the same
check. Without it a NUL in a manifest string reached
rehydrateAttachment, whose failure is logged and SKIPPED — so the import
reported success while silently dropping the attachment. The
skip-on-failure behaviour is pre-existing and deliberate (a partial
restore beats none); refusing the bad INPUT is what stops it being
reached this way. Left as it is, and named rather than quietly changed.
A VACUOUS ASSERTION THE MUTATION CAUGHT, recorded because the test would
otherwise have shipped as coverage: the filename leg first asserted
`!strings.ContainsRune(body, 0)` on the RESPONSE, which is JSON — a NUL
in the filename comes back as the six-character escape, not as a 0x00,
so the check passed whether or not the fix was present. It did pass with
the fallback disabled. Now it decodes the response and asserts the
replacement name. The item_id leg had the mirror-image weakness: it
asserted "not a 500", which is the Postgres-only symptom, so on SQLite it
would have passed either way; it now asserts the request behaves exactly
like the no-value control.
Every fix in this commit has a mutation against it, and each kills only
its own leg.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* refactor(server): drop the now-unused escape constant (BUG-2803)
The gate became a backslash check, which was the last production use of
jsonNULEscape; golangci-lint's unused check failed on the next run. Its
documentation was load-bearing, so the explanation moved into
bodyDecodesNUL's comment rather than being deleted with the variable —
including the distinction that made the old gate wrong (the escape has
one spelling INSIDE a decoded string, which is not a claim about the raw
bytes).
Caught by re-running lint on the tip after the previous commit rather
than trusting the run from the tip before it.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): rune-safe truncation and User-Agent sanitising (BUG-2803)
Codex round 5 was asked for the POPULATION rather than a confirmation —
"enumerate every remaining way a caller-supplied string can reach a
database text or jsonb parameter without passing a validity check" — and
returned three residual classes with their sinks. Two are fixed here;
the third is filed, because measuring it needs a fixture this unit
should not grow.
1. TRUNCATION CAN UNDO THE VALIDATION. Four sites cut a caller string
with a plain byte slice (name[:120], input.Name[:200]). If the boundary
lands inside a multi-byte rune the result ends in a partial sequence and
is no longer valid UTF-8 — so a value that PASSED the body check a few
frames earlier arrives at the store unbindable, and Postgres answers
22021 for a request the server already accepted.
This is the interesting one, because no input-side round could have
found it: the defect is downstream of validation, and it is invisible
with ASCII fixtures, which is what every test in that area used.
truncateBindableText walks back off continuation bytes and drops the
straddling rune. Tested with 2-, 3- and 4-byte runes so an off-by-one
walk-back cannot pass them all, and with a counterfactual leg asserting
the naive slice really does produce unbindable output for the same
input — without it the cases would pass against an implementation that
did nothing.
2. USER-AGENT REACHES TEXT COLUMNS. It lands in activities.user_agent
(three document paths, the connected-apps revoke) and
sessions.user_agent (three login paths), and no rule here sees a header.
The disposition is SANITISE, not refuse, and that is deliberate: a
header is metadata this server chose to record, not something the caller
asked for, so a malformed one must not turn an otherwise fine request
into a 400. The two sites that HASH the header are left alone — sha256
over arbitrary bytes is well defined, and changing what is hashed would
invalidate every stored UAHash.
The filing's own earlier probe had recorded User-Agent as NOT
reproducing on the item-create path. That was true and did not
generalise; these are different sinks.
3. NOT FIXED, FILED: the OAuth form-encoded bodies
(/oauth/token, /oauth/authorize/decide, /oauth/revoke,
/oauth/introspect) parse url-encoded form data outside the shared body
validator, with connection_name reaching oauth_connections.name and
client_id reaching the oauth_clients.id lookup. This was the ORIGINAL
subject of BUG-2803 before the filing was re-scoped, and it was recorded
then as unreachable without a fosite-backed fixture. That is still true,
and round 5's sink list is far more than the filing had. Filed rather
than guessed at.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): narrow the gate, stop refusing natural-shape fields (BUG-2803)
Codex round 6 plus one measurement of my own. Three changes, one of them
a revert of something I got wrong in the previous commit.
1. THE GATE COST TOO MUCH, so it is narrower and still sound. The
previous commit gated the walk on "does the raw body contain a
backslash", which is correct but catches every body carrying nested JSON
(each `\"` is a backslash). Measured on a ~377 KB import-shaped body:
60106 allocs/op with that gate versus 30073 with the walk disabled — the
walk was running on ordinary traffic.
The gate is now the four bytes that begin any \u escape for a character
below U+0100. The argument: to manufacture the six-character NUL escape
inside a decoded string, each of its characters arrives either literally
from the raw bytes — in which case the raw contains the escape, which
begins with that prefix — or from a \u escape of its own, and the three
characters involved (backslash U+005C, 'u' U+0075, '0' U+0030) all sit
below U+0100, so those escapes begin with it too. Back to 30073
allocs/op, identical to the walk-disabled build.
That argument is the same KIND of reasoning that was wrong two rounds
ago, so it does not stand on its own: a differential test runs the gated
function against an UNGATED walk over a corpus built to attack it —
oblique backslash, upper-case hex, an escaped 'u', an escaped '0', a
doubled backslash — and fails on any disagreement. It also asserts the
corpus contains both answers, since agreement over a one-sided corpus
would be vacuous. Reverting the gate to the old substring fails it.
2. THE CHECK REFUSED THE NATURAL SHAPE OF ITS OWN FIELDS. `tags` and
`fields` accept both a JSON-encoded STRING and their natural array/object
form, and the walk propagated "this subtree is JSON-encoded" into
containers — so a free-form tag whose whole value happened to be a JSON
document was refused, though nothing re-parses it. Measured: refused
before, accepted now, while the JSON-encoded spelling of the same field
is still refused. The flag now marks only a direct STRING child of a
listed key.
3. REVERTED: I wired the three LOGIN paths to the User-Agent sanitiser
last commit, before reading store.CreateSession. It HASHES the header
and stores no text — the round-5 enumeration named "sessions.user_agent"
and I took the name for a column. The change would have been actively
harmful: login would store sha256(sanitised) while middleware_auth still
compares sha256(RAW), so every session from a client with a non-UTF-8
User-Agent would fail validation. A sink named in a review is a pointer
to verify, not a finding. The real sink is activities.user_agent, from
three document paths and the connected-apps revoke.
4. And the wiring leg codex asked for, on that real sink: a request
through the router with a malformed header, reading the STORED value out
of the activities row, with a control asserting an ordinary header is
kept VERBATIM. Unwiring the production call site fails it; the helper's
unit test does not notice.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): apply the key rule at every level, not once (BUG-2803)
Codex round 7, both findings, the first confirmed by measurement.
1. THE RECURSION WENT ONE LEVEL TOO DEEP. Once the walk descended into
a JSON-encoded string it treated the WHOLE subtree below as
JSON-encoded, so a value nested two levels down — an ordinary string
inside a `fields` blob that happens to hold JSON text — was refused.
That is a false rejection, and the measurement says so plainly. With the
depth-2 check disabled, on Postgres 17:
depth 1 (the fields blob itself) -> 400 (correct: Postgres parses it)
depth 2 (a string INSIDE the blob) -> 201 (accepted, no error)
control -> 201
The handler parses `fields` ONCE. The inner text is re-escaped when the
blob is written, so what Postgres receives has a doubled backslash and no
escape at all. Only the document Postgres itself parses can carry a fatal
one.
The nested call now passes false rather than true, which makes this a KEY
RULE APPLIED AT EVERY LEVEL rather than a depth limit: a JSON-encoded key
INSIDE a document still recurses (pinned by a test), an ordinary one does
not. Same correction as round 6's natural-shape fix, one level further in
— I fixed the sibling case and left this one, which is CONVE-18's lesson
about my own enumeration being a sample too.
I checked whether anything re-parses a value inside the blob before
loosening this, rather than assuming: `arguments` was the candidate, and
parsePlaybookArguments asserts it is a native ARRAY (raw.([]any)) rather
than a JSON string, so it is covered by the natural-shape rule and needs
no second parse.
2. AN ERROR MESSAGE THAT SENT CLIENTS THE WRONG WAY. The OAuth dynamic
client registration handler prefixed every decode failure with "Request
body must be JSON". A body carrying a NUL is valid JSON, so that message
sends a client hunting a syntax error it does not have. The two failures
are now distinguished.
Round 7 also reports no break in normal CLI, MCP or web-client request
generation — they marshal JSON and encode paths and query parameters —
which is the first thing any round has said about the client surface.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): complete the artifact check, make the walk path-aware (BUG-2803)
Codex round 8. It confirmed round 7's two fixes, then found two real
defects and two inaccurate comments — the comment half being the angle
the round was asked for.
1. THE ARTIFACT CHECK MISSED TWO REACHABLE FIELDS. artifactIsBindableText
walked the decoded artifact by TYPE, so it never covered Provenance —
whose strings are rendered into a Markdown footer appended to the stored
content — and never matched Arguments, declared []map[string]any, a
concrete slice type the walk's []any case does not match. A YAML NUL
escape in either reached storage.
It now MARSHALS the artifact and searches the output for the escape
encoding/json produces. A type switch over a struct that grows is a list
that goes stale in silence; marshalling covers every exported field,
including ones added later. The one thing it cannot see is invalid UTF-8
(which marshals to U+FFFD), and it does not need to: step 2 rejects that
in the request bytes, and YAML cannot manufacture it from valid input —
its escapes name code points, where \0 names a NUL.
Both new cases fail with the check disabled; the raw-byte cases still
pass, killed by the raw check, so each leg is discriminating.
2. THE WALK WAS NOT PATH-AWARE. A collection may declare a user field
literally named `schema` or `tags`. The walk consulted the wire-key list
at every level, so `{"fields":{"schema":"..."}}` treated a user field
name as a wire key and refused valid text holding a JSON example.
The key list is now consulted only OUTSIDE caller data — not under a
natural `fields` object, not inside an element of a `tags` array, not
inside a re-parsed document. Combined with round 7's fix that makes the
descent exactly one level deep BY CONSTRUCTION, which is why the depth
counter is gone: with the flag no longer inherited, a bound could never
fire, and dead protection reads as protection. The depth-bound test is
replaced by one that pins the property directly — an escape IN the
parsed document is refused, one BELOW it is accepted, and a
wire-key-shaped user field does not restart the descent.
3. THREE COMMENTS CORRECTED, all mine, all of the kind a reader would
believe without checking:
- MaxBytesReader: Close FORWARDS to the underlying body rather than
being a no-op, and with a nil writer there is no automatic 413 — the
cap surfaces as a read error the callers turn into 400. Behaviour
unchanged; only the claim was wrong.
- parseArtifactRequest said "three checks" while implementing five, and
its returns list omitted ErrArtifactUnbindableText. Both added by this
branch, which is exactly the prose a change is most likely to falsify
(CONVE-23).
- errJSONBodyNUL claimed all 65 callers surface its message. The STATUS
is uniform; the wording is not — several substitute a generic string.
4. And one in a test: the timeline fixture said both backends hold a
CHECK constraint a raw NUL violates. items.fields is a plain TEXT column
with no CHECK on SQLite. What was OBSERVED is "SQL logic error:
malformed JSON"; the likely source is an expression index over
json_extract, and that attribution is recorded as NOT verified rather
than asserted.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): a regression this branch introduced, and the same trap again (BUG-2803)
Codex round 9. Both findings are mine, one of them a regression from the
round-8 restructure two commits ago.
1. THE ROUND-8 RESTRUCTURE REOPENED THE ORIGINAL DOOR. Taking the
JSON-encoded branch for a listed key skipped the plain "does this string
contain a NUL" check and asked only "does the document this string
carries hold an escape". Those are different questions. So
{"fields":"a<NUL escape>b"} — a direct NUL in the fields value, the very
first case this whole change closed — was accepted again.
Both checks now run. The test pins all three legs: a direct NUL in the
fields string, an escape inside the fields document, and an ordinary
fields string that must still be accepted, so the first two cannot pass
merely because everything under a listed key is refused.
2. THE ARTIFACT CHECK FELL INTO THE TRAP IT WAS WRITTEN AGAINST. It
searched the MARSHALLED bytes for the escape sequence, and a value
holding the six LITERAL characters marshals to a doubled backslash which
still contains that sequence as a substring — so valid content was
refused. Artifacts are documentation; text about a JSON escape is
exactly what one carries.
Worse than the bug: the comment I wrote asserted the ambiguity "cannot
arise here". It was the same doubled-backslash case bodyDecodesNUL exists
to resolve, one function away, and I wrote a sentence explaining why it
did not apply instead of checking. The marshalled form is now decoded
again and walked with the same machinery — the round trip is what makes
every field reachable without a type switch, the walk is what makes the
answer exact.
Its test asserts literal escape TEXT is accepted in title, body and a
field value, with a counterfactual leg asserting a real NUL in each of
those places is still refused, so acceptance cannot come from the check
doing nothing.
Reverting either fix fails its test and only its test.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* docs(backup): the one case where an export is not importable (BUG-2803)
Codex round 12, an operational pass. It found no migration or config
requirement, and two documentation gaps.
docs/backup.md promises that application-level export/import is portable
across SQLite and PostgreSQL. Since BUG-2803 that has one exception: a
workspace whose stored data contains a NUL exports fine and is refused on
import. It can only affect data written before the rule existed and only
on SQLite, which accepted it — a PostgreSQL instance never stored one.
`pad db migrate-to-pg` has the SAME problem and reports it worse: it
copies rows directly and never passes through the import guard, so a
legacy row fails against PostgreSQL's JSONB parser partway through the
copy rather than being refused up front. That is the likelier way an
operator meets this, since it is the operation that puts an entire old
SQLite database in front of PostgreSQL for the first time. Recorded on
BUG-2810, which owns the preflight and repair.
Round 12's other finding — that the PR's stated release note covered the
JSON 500-to-400 change and none of the rest — is fixed in the PR body
rather than in the tree.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): close two blind spots the tests themselves had (BUG-2803)
Codex round 13, asked whether the new TESTS are sound. Five findings;
these are the two that were self-contained. The other three are recorded
on the item with what each needs.
1. THE COMPLETENESS SCAN WAS BLIND TO FORM BODIES. It matched only
`.Body`, so FormValue / ParseForm / MultipartForm — which read the
request body just as surely — were invisible. It therefore reported full
coverage while the OAuth form-encoded handlers were entirely outside its
view. Widened, and it immediately failed on handlers_oauth.go, which is
the instrument working.
That file is now ACCOUNTED FOR AS A KNOWN GAP rather than as safe: the
OAuth handlers read form-encoded bodies that no rule in this family
covers (the transport rules see the query half of r.Form, not the body
half), tracked as BUG-2811 and needing a fosite-backed fixture to
measure. The test now STATES the gap instead of being blind to it, which
is the difference between a completeness claim and a completeness
appearance.
2. THE TRUNCATION TEST ADMITTED AN IMPLEMENTATION THAT RETURNED "". Its
assertions were: within the limit, bindable text, a prefix of the input.
An empty string satisfies all three. It now also asserts that an input
fitting the limit comes back UNCHANGED, and that no more than one rune
(4 bytes) is lost to the boundary — so a truncator that drops too much
fails, not just one that keeps too much.
Both were found by asking whether a broken implementation would pass,
which is the question CONVE-12 is about and which I had applied to the
production code and not to these two tests.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): the three remaining round-13 gaps (BUG-2803)
Codex round 13's other three findings, all of the same shape: a test
that would stay green with the production change reverted.
1. THE MANIFEST CHECK WAS UNTESTED. The bundle test built archives
containing only pad-export.json, so disabling the INDEPENDENT attachment-
manifest check left the suite green. The new test builds a bundle with
both entries, differing only in the manifest, so a refusal cannot come
from the export half. Verified by disabling each check separately: only
the matching test fails, so the two are independently covered.
2. THE TEST-EMAIL CHANGE HAD NO HANDLER-LEVEL TEST. Every existing leg
exercised decodeJSON, so reverting handlers_admin.go to default EVERY
decode failure to the admin's own address passed them all. The new test
drives the real endpoint with a wired mock sender and pins the
distinction that used to collapse: an ABSENT body still means "send it
to me" (control), an ordinary body still sends (control), and a body that
is present and refused answers 400 rather than being reinterpreted as
the default recipient.
3. THE MULTIPART LEG CHECKED ONE BYTE CLASS. A filter rejecting NULs
while letting malformed UTF-8 through would have passed it. It now drives
both, which matters because invalid UTF-8 is the class that reaches
Postgres as 22021 on a UTF8 database.
Round 13 was asked whether the new TESTS are sound — deterministic,
order-independent, and failing on broken code. It reported the fixtures
isolated and found five ways they were not discriminating. Two were
fixed in the previous commit; these are the rest.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): pin the wiring at every call site, not one (BUG-2803)
Codex round 14 confirmed round 13's five, then found the same shape one
level out: reverting a SINGLE call site back to the unsafe form left the
whole suite green, because the surviving fixtures are ASCII and a
helper's unit test does not care who calls it.
TestTextSafeHelpersAreUsedAtEveryCallSite asserts the wiring STATICALLY
rather than adding a fixture per site (an OAuth connection, a cloud
login, four audit paths). A byte-slice truncation of a caller string
fails it, and so does a raw User-Agent read outside the exempt set. Both
directions are checked: finding none of the SAFE form also fails, so a
scan that silently matched nothing cannot pass forever.
The User-Agent exemptions carry counts rather than being blanket, so a
NEW raw read in an exempt file still fails. All four reads in
handlers_auth.go are exempt because they feed a HASH — CreateSession
hashes the header and stores no text — and sanitising before hashing
would be actively harmful: login would store sha256(sanitised) while the
session check still hashes the RAW header, failing validation for every
client with a non-UTF-8 User-Agent. middleware_request_text.go's one raw
read is requestUserAgent itself.
Verified by reverting one truncation call site and one User-Agent call
site independently; each fails the test.
Round 14's third finding is fixed behaviourally rather than statically,
because the static scan cannot see it — handlers_oauth.go is already
listed for its form-body reads. TestOAuthRegisterRefusesNULBody drives
the real dynamic-registration endpoint with cloud mode and an OAuth
server wired, with a control leg registering successfully, and pins both
the refusal and the message split: the body IS valid JSON, so the answer
must not send a client hunting a syntax error.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): match wire keys the way the decoder does (BUG-2803)
Codex round 16, asked whether this change is consistent with its siblings
in the same file and extensible by someone who did not write it. It found
a live bypass instead.
encoding/json matches an incoming key to a struct field by an exact match
first and a CASE-INSENSITIVE one otherwise, so {"Fields":...} and
{"FIELDS":...} land in ItemCreate.Fields exactly as {"fields":...} does.
The walk looked the key up case-SENSITIVELY, so it skipped the nested
document for a body the handler went on to accept, and the database
answered the original 500.
Measured before the fix: `fields` refused, `Fields` and `FIELDS`
accepted.
This is the same defect shape as everything else in this unit — a check
that agrees with one layer's rules while the layer that actually consumes
the value uses different ones — which is why the fix is a PREDICATE
rather than a wider map: the map is the vocabulary, and the matching RULE
belongs to the consumer. Someone adding a key should not also have to
remember to add its spellings.
The test drives six spellings including mixed case, with a control
asserting an unlisted key stays caller data in any casing, so this is
case-insensitive matching rather than matching everything.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): fold keys the way encoding/json folds them (BUG-2803)
Codex round 17, first of five findings. The previous commit fixed the
ASCII half of key matching and left the Unicode half, which is this
bug's own pattern one more time.
encoding/json matches with Unicode SIMPLE FOLDING, not lower-casing.
U+017F LATIN SMALL LETTER LONG S folds to 's', so "ſchema" reaches the
`schema` struct field while strings.ToLower("ſchema") is unchanged and
missed the allowlist — a nested NUL under that spelling reached the
handler undetected.
Matching is now strings.EqualFold against each canonical key. The test
carries both a lower-case fold spelling and an upper-case one alongside
the ASCII cases, and keeps its control asserting an unlisted key stays
caller data in any casing.
The other four round-17 findings are recorded on the item rather than
patched here: they are genuine layer disagreements (duplicate keys
merging differently in a typed decode than in a map, a scan-failure
disposition on inputs the typed decode tolerates, and unknown-field
policy) whose fixes are design decisions rather than corrections, and
this seat is near its context bar. Each is written up with the
measurement it needs.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): pin that a NUL-bearing manifest refusal keeps the partial workspace (BUG-2803)
Codex round 18. The comment on the manifest NUL branch said refusing the
input "stops it from being reached this way" and stopped there, which
reads as though the refusal undoes the import. It does not.
A plain error with a non-nil workspace keeps the partial workspace,
exactly as every other manifest failure in this loop does — the rollback
branch fires only for *importStatusError, and mid-stream manifest
failures intentionally keep what was imported (TASK-896). Returning a
rollback-shaped error here would give NUL-bearing manifests different
semantics from malformed ones, which is a change to the bundle-import
contract rather than a fix to this bug.
So the behaviour is unchanged and now DELIBERATE: the comment states it,
and the test asserts the persisted state rather than only the HTTP
answer. Mutation: routing the branch through *importStatusError makes
the refusal roll back, and the new assertion fails naming the release
note it would falsify. The pre-existing status/body assertions do not
notice.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): record the four map-model disagreements as dispositions, and pin them (BUG-2803)
Lead ruling day-68 is land-and-follow: this branch lands on its measured
commits, and the token-stream rewrite is the BUG-2812 unit's spec rather
than a late restructure of an 18-commit branch under review pressure.
That makes the four open findings from rounds 16-17 something to WRITE
DOWN precisely, not something to leave in a trail comment.
The doc comment on bodyDecodesNUL now carries all four, with the one
root cause named: this scan decodes into map[string]any and the typed
decode does not agree with that model about keys. Two under-refuse
(duplicate-key merge; scan-failure passthrough) and are BUG-2812's spec
- both dissolve under a walk that never builds values. Two over-refuse
(unknown fields; case-variant duplicates) and are ACCEPTED, because
refusing is the safe direction. The asymmetry is stated rather than
smoothed over: within the map model, (1) and (4) are one defect seen
from two sides and only one of them fails safe.
Finding (3) is an observable compatibility change - a forward-compatible
field carrying a NUL escape now gets a 400 where it got a 200 - so it
goes in the release note as well as here. A qualification only protects
where the actor meets it.
All four are pinned by a test, measured on this tip rather than carried
over from the round-16/17 write-up. The two known-gap legs assert the
WRONG answer on purpose: when BUG-2812 lands they FAIL, naming the doc
comment and the release note as what to update. Both gap legs carry a
premise assertion - the same bodies with the disagreement mechanism
removed ARE detected - without which they would pass against a check
that detected nothing.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* test(server): wire release-note item 10 to the router, with its before-state measured (BUG-2803)
The disposition test proves bodyDecodesNUL RETURNS true for an unknown
field carrying a NUL escape. The release note claims the API answers
400. Those are different claims and only the second one is what an
operator or client author reads - CONVE-19, my own convention: a
direct-call test vouches for the component, not its binding.
Two legs, and the control is the load-bearing one. An unknown field with
an ordinary value must still be ACCEPTED, so this pins "refused for the
NUL" rather than "refused for being unknown". The handler does not
reject unknown fields; if it ever started to, the note's explanation
would be wrong while its status code stayed right, and no
status-code-only assertion could see that.
The before-state is measured rather than asserted from memory. Disabling
the check makes the same request answer 201 - which is main's behaviour,
since decodeJSONWithLimit there unmarshals straight into the typed value
and the key is dropped. So "answers 400 where it answered 200" is a
measurement in both directions, not a recollection of one.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(backup): the NUL rule lives in the binary, not the database (BUG-2803, BUG-2813)
Codex round 19, the fresh-angle deploy/rollback/mixed-version pass.
docs/backup.md said a NUL-bearing row "can only affect data written
before that rule existed, and only on SQLite". The second half is true.
The first half is false, and the reason is the interesting part: the
guard is in decodeJSONWithLimit, so the invariant is a property of the
running BINARY, not of the database.
On SQLite any window where an older binary serves the same database can
still write one - a rollback after upgrading, a staged rollout with an
old and a new instance sharing a database, a second older instance on
the same file. The window closes, the guard returns, and the rows are
already stored, behaving exactly like genuinely old ones. A rollback is
an ordinary operational move, so this is not an exotic path.
The doc now states the binary-version dependence, says which dialect is
affected and why PostgreSQL is not (it refuses a NUL itself, at every
version), and gives the operational answer: drain writes from older
binaries before the new one serves, or roll forward rather than back.
Store-layer enforcement - so the running build stops mattering - is
filed as BUG-2813 rather than added here. It is a dialect-level change
and the day-68 ruling on this unit is land-and-follow.
The same false implication was carried by the PR's release note calling
such a workspace "legacy"; corrected there too.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): cite the ruling in house style, not the team-room day counter (BUG-2803)
"lead ruling day-68" is the internal day counter, which means nothing to
anyone reading this repo and is inconsistent with every other citation
in it - the codebase cites a lead ruling by DATE or by BUG ref, never by
day-N. Replaced with the bug ref, which is the part a reader can
actually follow.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): drop a commit count I had already measured as wrong, and stop asserting a cause I borrowed (BUG-2803)
Two defects in a comment I wrote an hour ago, both of the kind this
unit's trail keeps recording.
"an 18-commit branch" - the branch was 20 commits at
|
||
|
xarmian
|
1933041027 |
fix(events): surface the SUBSCRIBE error and refuse callers instead of admitting a dead stream (BUG-2764) (#1215)
* fix(events): surface the SUBSCRIBE error and refuse callers instead of admitting a dead stream (BUG-2764) * fix(watchevents): surface the SUBSCRIBE error at construction and on resubscribe (BUG-2764) * docs(events): the idle-cycle prose and metric Help now name the third install-nothing reason (BUG-2764) * fix(events): retry uncovered workspaces, wait on in-flight records, refuse after Close (BUG-2764 codex round 1) * fix(events): post-loop check waits on an in-flight record before trusting a live entry (BUG-2764 codex round 2) * test(watchevents): assert what the failed subscribe leaves, not how long it takes (BUG-2764 codex round 3) * docs(events): prose says what the code guarantees — delivery failure or shutdown, replacement refusals do not count as cycles (BUG-2764 codex round 4) * test(server): the 503 mapping is asserted on both subscribe branches; docs scope the refusal to the activity stream (BUG-2764 codex round 5, BUG-2800) * fix(events): uncovered-workspace retry logs are quiet once the bus is closing (BUG-2764 codex round 6) * fix(events): the uncovered-retry log promises a retry only while subscribers remain (BUG-2764 codex round 7) |
||
|
xarmian
|
c73584088f |
fix(watchevents): detect a half-open Redis connection with a bus heartbeat (BUG-2769) (#1199)
* fix(watchevents): detect a half-open Redis connection with a bus heartbeat (BUG-2769) internal/watchevents had the same defect as internal/events did, by the same mechanism: ChannelWithSubscriptions on a connection whose go-redis health check only writes. PubSub.Ping calls writeCmd and returns without reading a reply (v9.22.0), so a route that stops carrying traffic without closing is invisible — the instance blocks on a read forever while its replay buffer goes on looking complete. Named as a class sweep in BUG-2738's filing and deferred there. It became load-bearing when that unit shipped: docs/deployment.md told operators the gap was "closed on the activity stream and still open on the watch stream". This diff falsifies that, which is why the prose sweep is part of it. THE PORT IS SMALLER THAN THE ORIGINAL BY DESIGN. This bus holds ONE process-wide subscription created in its constructor, off any request path, so none of BUG-2747's establishment machinery exists to interact with: no per-workspace map, no establishment record, no single-establisher wall, no concurrency cap, no bounded-parallel recovery, and no per-workspace cycle scoping. Cost is flat too — one frame per instance per interval regardless of workspace count. NO COMPANION COUNTER, and that was CHECKED rather than inherited. internal/events needs pad_event_subscription_cycled_total because its dropWorkspaceCoverage returns early when a workspace has no buffer, so the reset reason under-reports the early-wedge case. dropCoverage here has no such branch: it replaces the buffer and reports unconditionally, so idle_timeout is a complete count on its own and a second metric would be a number needing to be explained against its neighbour for no signal. THE RECEIVE LOOP NOW OWNS ITS SUBSCRIPTION AND CONTEXT. A cycle replaces the subscription under a running bus, and the loop reading the old one must tell "I was replaced" from "the client died" — the second logs an ERROR and moves a counter documented to mean the instance has gone deaf. The cycle cancels that loop's own context before closing its PubSub, so it leaves by the quiet door. Its own test. I PORTED A FLAW ALONG WITH THE STRUCTURE, and the wiring test caught it: both maintenance halves shared one kick channel, so whichever goroutine was waiting consumed it and the other stayed on the stale cadence. internal/events' mutation matrix found exactly that (M11c) and fixed it; the fix did not come across. That is the contamination hazard this port's grounding warned about, in its literal form, caught by the CONVE-19 test rather than by review. Two more found by mutation, both missing tests rather than missing code: nothing asserted that ordinary traffic keeps the instance alive (removing the per-frame stamp survived, because every other test drives idleness through the clock), and nothing asked for a SECOND detection (a replacement inheriting stale stamps gives a detector that works exactly once, which is worse than one that never runs because it looks like it works). The second needed a direct assertion on the install stamps, because the behavioural route re-stamps the field it was meant to be testing. Trio in one commit as required: reason enumeration, the pad_watchevents_sequence_resets_total Help string, and docs/deployment.md — plus the two BUG-2738 sentences this falsifies and a new section explaining how the watch bus differs from the activity one. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): fence stragglers and re-validate before the drop (codex r1) Three findings, and two of them are BUG-2738 fixes I again failed to bring across with the structure. That is now three times in one port: the shared kick channel, the stale idle decision, and the missing generation. The mechanism is the same each time — I ported what the code DOES and not what its review history taught it, and each was caught by a test or a reviewer rather than by me reading the source I was copying. STALE IDLE DECISION. cycleIfIdle decided under one lock and tore down under another; a heartbeat or notification arriving between them left a demonstrably alive subscription being dropped and every client on the instance resynced for nothing. BUG-2738 fixed exactly this at its round 11. Re-validated immediately before the drop, with a positional seam so a test can land the recovery inside the window rather than racing it. NO GENERATION FENCE. Cancelling a receive loop and closing its PubSub does not JOIN the goroutine, and go-redis's channel is buffered, so a frame from a replaced subscription could still stamp the replacement's liveness, append to its buffer, or drop its coverage. On a wedged route that is the worst direction: the dead connection's buffered tail suppressing the detector for its successor. One check at the top of the frame handler covers all three, because the three must agree about whether a frame belongs to the live subscription. The probe stamp is fenced separately, since a slow publish can outlive the subscription it was sent for. A COPIED COST PARAGRAPH THAT CONTRADICTED ITS OWN SECTION. The activity bus's "each workspace has its own subscription, N frames per interval" text sat below the new watch-specific section saying the opposite. Retitled and moved above it. FOUR INSTRUMENT DEFECTS ON THE WAY, all found by mutation: - Nothing asserted ordinary traffic keeps the instance alive — every other test drives idleness through the clock, so removing the per-frame stamp survived. - Nothing asked for a SECOND detection, so a replacement inheriting stale stamps gave a detector that works exactly once — worse than one that never runs, because it looks like it works. Needed a direct assertion on the install stamps, since the behavioural route re-stamps the field under test. - The generation tests asserted the PREDICATE, not that the loop calls it. - And that wiring test could not discriminate on a frozen clock, where a stamp writes the value already there. It advances the clock first now. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): make the generation fence atomic with what it guards (r2) Two P1s, both mine, both the same shape: a check in one lock acquisition and the mutation it guards in another. THE FENCE WAS NOT ATOMIC WITH ITS MUTATIONS. One check at the top of the frame handler read well and guarded nothing reliably — a replacement between that check and stampLastSeen / fanOutFromRedis / dropCoverage let a straggler through to any of them. The generation now travels TO each mutation and is re-checked under the same lock that mutates. A stale notification entering the replacement's buffer is the worst of the three: it makes the instance vouch for a span it never received, which is the false coverage claim this whole family exists to remove. THE OLD GENERATION STAYED CURRENT ACROSS THE REPLACEMENT. subGen was incremented only after the new subscription was confirmed, leaving the cancel, the close, the dial and a round trip during which the OLD generation still passed every fence. Retired at teardown now, so during resubscribe NO generation is current and a late frame is ignored everywhere. That also makes the failure path honest: the "no notifications until restarted" log was false — no generation is current, so the next idle tick tries again. Revalidation and the drop are now ONE critical section rather than two, for the same reason at one level down: a frame arriving between them was silently discarded by a drop already decided on. Also: phase 1 no longer starts the maintenance goroutines, and the watch bus's phase is logged at startup — an operator cannot read an absence of idle_timeout without knowing whether the detector was running, and the two flags are independent. DOCS still described the workspace model in the section that claims to cover both buses: one heartbeat "per subscribed workspace", a phase table naming only PAD_EVENTS_HEARTBEAT, and coverage described as a workspace's. Generalised. Two more instrument gaps, both found by mutation: nothing asserted a straggler cannot enter the replacement's BUFFER (only the stamp was covered), and the phase-1 goroutine gate is untested by design — removing it changes no behaviour, only goroutine count, and the only assertion is a census that would be flaky here. Said out loud rather than left to look like coverage. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(watchevents): prove each generation fence on its own Round 3's fix put a generation check in each of the four places a frame from a replaced subscription can mutate shared state, rather than one check at the top of the receive path — a check in one lock acquisition and a write in another is a TOCTOU, which is what codex blocked. Four checks means four mutations, and the matrix found the first pass of tests could not tell them apart: removing the append's check, or the coverage drop's, left every test green. Not because the guards were redundant — because no test drove those paths with a stale generation. The straggler tests all enter through fanOutFromRedis, whose own guard returns early and hides the one below it, and nothing at all drove dropCoverageForGen with a straggler. So the fences are asserted one at a time, each through the entry point that actually reaches it: epoch bookkeeping fanOutFromRedis with a foreign epoch — the loudest of the four, since an accepted straggler would rewrite the id space and resync every client on the instance buffer append fanOutLocally directly, under the guard above it coverage drop dropCoverageForGen, previously undriven liveness stamp stampLastSeen, which would otherwise let a dead socket's traffic hold detection open Each fails against removal of the single check it names (M16/M17/M19 and the existing stamp mutation), and the four together still pass the end-to-end straggler tests unchanged. Refs BUG-2769 * test(metrics): prove the two new watch signals reach the registry Both were wired and neither was asserted at the metrics layer, which is where docs/deployment.md's claims about them actually live. A reason or a callback that never reaches the registry is a runbook pointing at a series that does not exist, and nothing in internal/watchevents can catch that — its observer is an interface, satisfied by a test double. pad_watchevents_heartbeat_publish_failures_total incremented six times, a count no other assertion in that test uses, so a callback wired to the wrong counter cannot land on the right number by coincidence. Fails when the increment is pointed at a neighbour. sequence_resets_total{reason="idle_timeout"} asserted with the literal label, alongside the four spellings already pinned there and for the same reason BUG-2739's rename left that test behind. Fails when the constant drifts. Also corrects the shared "what happens if you run them out of order" paragraph, which moved under a heading covering both buses while still describing only one: it said the frame travels on "the workspace's event channel" and that an un-upgraded instance resyncs "for every workspace", neither of which is the watch bus, where there is one channel and one buffer per instance. The blast radius differs in scale between the two and the paragraph now says so. Refs BUG-2769 * docs(watchevents): correct three counted claims that stopped being true All three said "three" where the code now has four, and each was accurate when written — the fourth fence (the epoch bookkeeping in fanOutFromRedis) was identified after them, in the pass that found the matrix could not tell the guards apart. That is the whole failure mode: a count is a claim, and a claim written before the last change is wrong afterwards with nothing to notice it. Two of the three sat inside a comment ABOUT how carefully the guards were enumerated, and one names them now instead of counting them, so the next site added has to appear in the list or contradict it visibly. Found by sweeping the branch diff for counted prose rather than by rereading, which is what had already missed them twice. Refs BUG-2769 * test(config): close the other half of the two-flag independence claim The flag tests asserted PAD_WATCH_HEARTBEAT does not move EventsHeartbeat and stopped there, while the comment above them and the deployment doc both claim the two buses roll INDEPENDENTLY. That is a biconditional and one leg does not establish it: a Load() that pointed PAD_EVENTS_HEARTBEAT at both fields passed everything. Now both directions are asserted, and the events leg checks its own premise first, so a fixture that stopped setting the flag fails as a fixture rather than as a pass. Also pins env-over-file precedence for the watch flag, in the direction that actually matters: PAD_WATCH_HEARTBEAT=false over watch_heartbeat=true in config.toml. That is the rollback for a bad phase-2 flip, and an operator reaching for it mid-incident cannot be editing a file on every host. Mutation matrix, each detected: the env var wired to the neighbouring field, the env var never read at all, and the toml tag dropped. Refs BUG-2769 * test(watchevents): fix five tests that passed for the wrong reason Codex round 4 went at test honesty rather than correctness and found no BLOCK, but it found five assertions that hold whether or not the thing they name works. Each is now driven through the path it claims, and each was mutation-checked against the specific defect it exists to catch. the malformed-frame contract only ever called isWatchHeartbeat. The predicate can be perfect while the receive loop routes every "hb|…" payload to the ignore arm without asking it, which is the defect, and the test's name promises coverage ends — a claim about the loop. Now published on the real channel, with a well-formed frame as the control so the assertion cannot be satisfied by a loop that finds everything undecodable. the receive-loop wiring test published, slept 300ms, and asserted nothing had changed. A loop that stalled or never started satisfies that perfectly. There is no natural signal to wait on instead, because a frame the fence refuses is by design invisible — hence a seam that fires after the loop handles a frame whichever arm it took. Bounded, so a stalled loop fails with a message rather than a package timeout, and followed by a control that the same loop still accepts a frame whose generation matches. the quiet-exit test asserted only that no loud exit was reported, which a replaced goroutine that never exits at all also satisfies — a leak, and the worse outcome. Now joins the loop first via a process-wide live-loop count, then checks the counter, so it is a statement about a goroutine that has finished. the maintenance-loop wiring test claimed both halves and observed a heartbeat, which a loop that started only the publisher passes. The idle half cannot be proved there at all: against a live miniredis this bus's own heartbeats come back and refresh liveness every cadence, so wedging it with the loop running is a race against the publisher — which is what my first fix for this turned out to be, flaky at 2 in 3. Renamed to what it proves, pointing at the blackhole end-to-end test, which drives the scanner for real and detects both mutations. the straggler test never delivered a straggler. It incremented subGen by hand, called isCurrentGen, and compared an unchanged timestamp without touching a mutation path — green with every fence removed. Deleted rather than repaired: the four-way per-fence test added earlier covers it properly, and isCurrentGen went with it. Plus two ordering changes in Close/resubscribe that ARE NOT fixes for an observed race, and say so in the test. Making b.pubsub reassignable made Close's unlocked read of it look wrong, and resubscribe's wg.Add outside the lock look like it could land after Close reached Wait. Both windows turn out to be shut already by resubscribe's b.closed check, which sits under the same acquisition as the count — reverting either fix leaves the new Close-during-cycle test green. Kept as defence because the invariant they lean on is three functions away, and documented so nobody later reads them as evidence of a bug that existed. Also corrects the metric help and two comments that said an idle cycle "replaced the connection" when it attempts a replacement that can fail; the deployment doc already said attempted. And the deployment doc's rollback, frame-validation, what-to-watch and startup-log paragraphs, all of which moved under a heading covering both buses while still describing only the activity one. Refs BUG-2769 * refactor(watchevents): drop an always-empty return and the branch reading it dropCoverageIfStillIdle returned (string, bool) where the string was never anything but empty — the reset it reports goes out through the pending/flush path inside the lock, so the caller's `if report != ""` was unreachable. A second reporting path that exists in the signature and never fires is a thing a later change wires up by accident. Refs BUG-2769 * fix(watchevents): a failed re-dial retries without re-dropping coverage Codex round 5, on behaviour across a full Redis outage. No BLOCK; this was its one P2 and it is real. The probe-failure suspension does not cover this case, and the reason is worth stating because the suspension looks like it should. Suspension asks "did our last probe get through", and that can be YES with the route already gone: the last successful publish stamps lastProbeOK, Redis dies before that frame comes back, and lastSeen stays behind it. From there both timestamps are frozen — the probe fails so nothing stamps lastProbeOK, nothing arrives so nothing stamps lastSeen — and the cycle's precondition stays true for the whole outage. Every pass then dropped coverage, announced to every subscriber, and re-dialled. Only the re-dial is owed. The second drop empties an already-empty buffer and re-announces a hole every subscriber has been told about, and it moves pad_watchevents_sequence_resets_total{reason="idle_timeout"} once per cadence — so a five-minute outage read as ten incidents on the series operators are told to alert on. cycleIfIdle now has a retry-only arm ahead of the decision, entered when there is no subscription at all, and the teardown clears b.pubsub / b.subCancel so that state is representable. Clearing them also stops Close closing an already-closed PubSub a second time. Two tests, discriminating in OPPOSITE directions, because the obvious fix for the noise is to suspend the pass and that would trade a noisy outage for one the instance never returns from — retrying the dial IS the recovery path: three passes with Redis away one reset, not three Redis returns after a failed pass the subscription is re-established and the counter does not move again Matrix: removing the retry arm, making it return without retrying, and leaving the torn-down subscription in place are each detected, the middle one only by the recovery test. internal/events has no equivalent defect. Its teardown deletes the workspace's subscription entry, so its next scan finds nothing live and abandons; recovery there runs off the request path. Refs BUG-2769 * fix(watchevents): only one caller may install a replacement subscription Codex round 6, verifying round 5's fix. No BLOCK; this was its P2. Both the cycle and its new retry arm dial with the lock RELEASED, which is deliberate — a Redis round trip under the bus's hot mutex would stall every fan-out on the instance — so two passes can each find no subscription and each dial one. Installing both is wrong twice over: two receive loops would run on the SAME generation, so both accept every frame and each notification is processed twice, and the loser's PubSub would be untracked, closed by nothing including Close. The install is what needs serialising, not the dial, so the loser discards its own connection under the lock rather than the two racing to overwrite b.pubsub. Only the idle scanner calls this today, so this guards an invariant rather than fixing an observed fault. Written down because the invariant lives in a different file from the code relying on it, and because the failure is silent duplication rather than a crash. The test races two resubscribes through the install seam. Two details it needed, both found by running it rather than reading it: the loop count is incremented INSIDE the goroutine, so sampling it right after the constructor returns reads zero — the first version did, and measured every later count against that wrong baseline. It waits for the loop now. the seam release is deferred, because without it the guard's mutation parks both callers in the callback, Close waits on receive loops that cannot start, and the detection arrives as a package-wide hang with no message. That is how the mutation first appeared to pass. Also completes the idle_timeout reason in three comment/help sites that still enumerated four reasons and said "the last two" — the same stale count corrected in the observer contract earlier on this branch, missed in its neighbours because I fixed the one the reviewer named instead of grepping for the claim. Refs BUG-2769 * test(watchevents): count installs instead of waiting for one that never comes Codex round 7 returned no BLOCK and no P2 on the production code, and two NITs on what round 6 added. Both are real. The concurrency test synchronised on a WaitGroup expecting BOTH callers to reach the install seam. Only the winner does — that is the property under test — so in the passing case the goroutine waiting on it blocks forever. A leak inside a test written to prove a leak does not happen is not a shape to leave standing. An atomic the abandoning caller never touches carries the same information and blocks nobody, and it removes the release channel and its deferred close along with it. The final assertion also moved off liveReceiveLoops and onto that count. A loop starts AFTER its install, so reading the loop count can catch a second caller's goroutine before it has begun and see the passing value on a failing run. Both callers have returned by the time the install count is read, so it is final. Detection over ten runs with the guard removed: 10/10, where the loop-count version was a race against a goroutine's first instruction. Also softens the retry arm's log line. It said the instance receives no notifications until an attempt succeeds, which is true for today's single scanner and stale the moment there are two: one caller's dial can fail while another has already installed. It now claims only what the failing call knows. Refs BUG-2769 * test(watchevents): hold both callers at the window, and say what that misses Codex round 8's P2, on the test the previous commit rewrote. Starting two goroutines from a start gate makes overlap likely and guarantees nothing: one can finish resubscribe before the other begins, so the window the install guard closes need never have been open. A seam at the dial/install boundary — connection dialled, lock not yet taken — lets both callers announce their arrival and wait for each other. Now the window is open by construction rather than by luck, and the test fails as a fixture if only one caller ever reaches it, instead of passing on evidence it never gathered. AND IT STILL DOES NOT DETECT EVERYTHING, which the test now says in place of leaving it implied. Measured: guard removed entirely 10 runs, 10 detected guard checked in its own acquisition, then 10 runs, 0 detected the lock retaken to install The second is the regression round 8 asked about, and catching it would mean landing the second caller inside a check-to-install gap that exists only in the mutant — there is nothing to yield on there, and no seam can be placed in code that is not written. So this test covers "a guard exists", not "the guard is in the right critical section". The latter is held by the comment at the guard and by review, and a test comment claiming otherwise would be worth less than the honest note. Refs BUG-2769 * fix(watchevents): make the frame seam and the cycle log tell the truth Codex round 9 was asked whether this should merge and said hold for a cleanup pass. Five findings, no correctness blocker, and every one of them a claim that had stopped matching the code. the frame seam did not fire for every arm, though its comment said so. The arms that decline to act — a heartbeat, an undecodable payload, an unsubscribe confirmation — were `continue` statements, which skipped everything after the switch. A test waiting on the seam for one of those frames would have HUNG rather than failed, which is the worst way to find this out. The switch is now its own method so every arm ends the frame by returning, and a test drives one frame per publisher-reachable arm and counts three. Detected against restoring the skip. the idle-cycle warning was emitted before the revalidation that can abandon the cycle, so it could announce coverage ending and resumes answering sync_required for a subscription that was then left alone — a log line with no counter behind it, and an on-call hunting a bug that is not there. internal/events learned this at its own round 6; the reason did not come across with the port. Moved after the decision is final, still saying "attempting" to replace because the resubscribe can fail. the quiet-exit test sampled liveReceiveLoops instead of waiting for it, so its "the replaced loop left" assertion could be satisfied by a loop that never ran. Same defect fixed in the sibling concurrency test a commit earlier and missed here, because I looked at the test the reviewer named rather than at the pattern. Latent rather than observed: sampling survives 10 runs, so this removes a possibility. the probe-failure log and metric help called an errored Publish a failure to publish. A returned error can also mean the reply was lost after Redis accepted the frame, so the honest claim is that the probe is UNCONFIRMED. It changes no behaviour — an unconfirmed probe is not evidence about the receive path either, so detection suspends the same way — but an operator reading the counter should not be told more than the instance knows. the deployment doc said the watch stream differs in "three things" and listed four, the fourth being the bullet I added last round. Third instance of that species on this branch; the count is gone rather than corrected. Refs BUG-2769 * docs(watchevents): stop one unconfirmed probe standing in for a broken path Codex round 10 confirmed four of round 9's five fixes and held the fifth as partial. It was right on all three residual sites. Renaming the condition to "could not confirm" did not fix the sentences downstream of it. The log still said silence cannot be read as a finding "when we could not ask" — but we may well have asked, and lost only the answer. And both the metric help and the observer contract said an instance in this state "is also failing to deliver its own notifications to every other instance", which is a conclusion about the outbound path drawn from a single call that did not come back. The inference is sound at a SUSTAINED rate and worthless at one increment, so both now say which is which. That distinction is the whole value of the counter to an on-call: a blip is a lost reply, a rate is a broken path, and the same wording for both makes the first look like the second. No behaviour change. An unconfirmed probe suspends detection exactly as a definite failure does, because it is not evidence about the receive path either way. Refs BUG-2769 * docs: sweep the BUG-2738 prose this change makes false BUG-2738 shipped documentation that describes the watch stream as still carrying the half-open defect. Merging this makes those sentences wrong, and I flagged the sweep as owed twice during the groundwork and then did not do it — the lead caught that the package said nothing about it. Five sites, each re-read after editing rather than grepped for, because grepping for a phrasing I chose is how I have twice verified a sweep that had not landed: the residual enumeration opened "One gap remains everywhere, and a second remains on the watch stream only", then described one gap and said it was open on both. The second WAS the half-open case. Now states one gap, on both streams, and says where the second went. the half-open paragraph already said "closed on both streams" — the one site I had fixed — but omitted that each half is behind its own phase-2 flag, so a reader takes it as closed on their deployment when it is closed only once they turn it on. "A third residual" counted the item it followed. With the second gone the ordinal was wrong; it does not need one. "these two gaps" in the closing sentence, same arithmetic. the pad_event_subscription_cycled_total row told an operator to read heartbeat_phase off the startup log. There are now two such fields on two lines under two flags, and only one bears on that counter. It names the line. No code change; suite 28/28 and lint 0 re-run because the branch is under review and a docs commit that skips them is a commit nobody checked. Refs BUG-2769 |
||
|
xarmian
|
effd0199cd |
fix(events): detect a half-open Redis connection with a bus heartbeat (BUG-2738) (#1195)
* fix(events): detect a half-open Redis connection with a bus heartbeat (BUG-2738)
A Redis connection can stop carrying traffic without closing -- no FIN, no
RST, just a route that stopped working. The instance blocks on a read that
never returns, receives nothing, and its replay buffer goes on looking
complete, so every resume is answered "caught up" from a coverage window that
ended when the route did.
go-redis cannot see it: PubSub.Ping writes the command and never reads a
reply (v9.22.0), so its health check reports healthy for as long as the socket
accepts writes. Measured on day-52 against a proxy that silently stopped
forwarding: no reconnect in 24 seconds.
Each subscription now records when it last received ANYTHING -- event,
heartbeat, or subscription acknowledgement -- and a background pass ends the
coverage of any workspace whose stamp goes stale past 3T, then REPLACES the
connection. Drop alone would not recover: the resync it demands is served from
the same dead socket, so the detector fires again on the next pass.
Dave's day-49 ruling dissolves the threshold rather than tuning it. The bus
publishes its own frame every T=30s and fires at 3T=90s, which turns "is this
workspace quiet or is the route dead?" -- unanswerable, deployment-dependent --
into "did our heartbeat arrive?".
TWO PHASES, ORDER NOT OPTIONAL. The frame must travel on the workspace's event
channel, because that connection is what needs proving. A pre-phase-1 binary
cannot classify it: the frame reaches the event decoder, fails, and since
BUG-2739 that is a hole in coverage -- so an early flip makes every un-upgraded
instance drop its buffer and resync all its clients, every 30s, per workspace,
for the length of a mixed deployment. Phase 1 recognises and ignores;
PAD_EVENTS_HEARTBEAT is phase 2, a constructor parameter with no default so
every call site states its phase.
The idle detector is a THIRD actor in a region whose invariants were designed
around request goroutines plus Close. Four rules, each commented at
cycleIdleSubscriptions and each with a test:
1. It refuses to cycle while pendingSubs holds a record, and MINTS the
record itself before tearing anything down -- subscribeAndReplay checks
pendingSubs before wsSubs, so a subscriber arriving mid-cycle joins the
replacement instead of being admitted into the doomed subscription.
2. lastSeen is stamped at INSTALL, not left at the zero value, which reads
as 1970 and would cycle hardest on an unconfirmed admission -- the
workspaces already having a bad time.
3. wsCounts is re-read under the lock that performs the teardown.
4. Re-establishment runs on b.ctx with a nil establisher; the bus has no
subscriber registration of its own to unwind.
Two decisions beyond the plan:
A NEW COUNTER, not just the reset reason. dropWorkspaceCoverage reports a
reset only when a buffer existed to drop, and the incidents this detector
exists for skew hard toward having none -- a route that wedged early on a
quiet workspace. Reading cycles off the reset label alone would under-report
exactly the case it was built to find, so pad_event_subscription_cycled_total
is the dependable count and idle_timeout is corroboration. Both comments say
which is which.
THE CADENCE IS A LIVE TUNABLE -- a timer re-read under b.mu each pass plus a
buffered kick, not a ticker constructed once. A ticker captures the interval
at goroutine start, which makes the field write-once while its comment calls
it a tunable and makes any later write a data race; it also leaves no
deterministic way to test the WIRING other than a test-only constructor.
decodePayload's signature grew a payloadKind. The classification belongs to
the decoder, not the call site, so no future caller can reintroduce the
coverage drop; and the prefix (rather than an exact payload) means a later
frame version needs no third roll.
Also swept, per the team's prose convention: receiveMessages' doc comment and
deployment.md both said this gap was open and needed a decision. Both now say
what closes it -- and deployment.md says the watch stream still has the same
defect by the same mechanism, which is its own unit.
Trio kept together: ResetReasonIdleTimeout, the metric Help strings, and
docs/deployment.md's rollout order with the mixed-fleet failure named.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(events): rebuild the instruments the BUG-2738 matrix showed were blind
The mutation matrix found a defect in the fix itself and three tests that
could not have caught what they were named for.
THE DEFECT: the idle scan skipped a subscription whose lastSeen was the zero
value. That reads as belt-and-braces beside the install-time stamp and is the
opposite -- it makes a subscription that has NEVER received anything
permanently uncyclable, which is the BUG-2747 unconfirmed admission: the one
population the plan singles out as mattering most, and the one where a wedged
route would then be undetectable forever. It was also masking rule 2: with the
skip present, removing the install stamp survived every test. Skip removed;
that mutation is now caught. Re-adding it is undetectable by construction and
the comment says so, because a guard that only acts once a real one has broken
converts a caught defect into a silent one.
THREE INSTRUMENTS THAT WERE NOT MEASURING:
- "Drop only, never cycle" passed because establishSubscription overwrites
wsSubs, so a generation check cannot see a replacement installed WITHOUT
tearing the old connection down -- a leaked PubSub, connection and receive
goroutine per cycle, forever, on exactly the wedged route where they never
die on their own. Now asserted on the receive loop exiting.
- The Close test was vacuous. Close drains wsSubs, so a loop that ignored
b.ctx entirely would find no workspaces and publish nothing: silence after
Close was evidence of nothing. maintenanceStopped makes the goroutine's exit
observable, which is the same reason Observer.ReceiveLoopExited exists.
- The joint test HUNG rather than failing under the drop-only mutation: the
seam never fires, so the joiner goroutine was never spawned and an unbounded
receive waited forever. The harness then aborted mid-run and LEFT THE
MUTATION APPLIED to the working tree, which a grep caught and a green test
run would not have. The wait is bounded and names the failure; the harness
bounds each run, reports a hang as its own status, and restores in a finally.
Added: a direct test that a straggler frame from a replaced generation cannot
refresh its successor's liveness -- on a wedged route, the dead connection's
buffered tail would otherwise suppress the detector for the replacement.
RULE 3 IS AN OPTIMISATION, NOT A CORRECTNESS GUARD, and the matrix says so
rather than an argument: removing the whole second read -- liveness, generation
and count terms together -- survives every test, because
establishSubscription's abandon path already refuses to install for an emptied
workspace and retires the record in the same critical section (BUG-2749). The
first read is redundant more sharply still: reaching zero takes the
subscription down with it, so this loop never sees such a workspace. Both are
kept, because neither DEPENDS on that coupling, and both comments now carry the
per-term reading instead of describing tested defence in depth. The generation
term is unreachable while the establishment record is held, by rule 1's own
mechanism.
Matrix: 16/22 detected, plus 4 follow-ups. Every survivor is documented at its
line with why it survives.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(events): gate idle detection on heartbeat phase 2 (BUG-2738, codex r1)
Codex round 1 found a defect the first draft had shipped WITH A COMMENT
JUSTIFYING IT, plus two coupling hazards.
P2-as-filed, P1 in effect: idle detection ran on every instance from phase 1,
on the reasoning that it could "detect off whatever traffic the deployment
already carries". That holds only for a BUSY workspace. A QUIET one on phase 1
has no events and no heartbeat, so a perfectly healthy subscription crossed
the 90s threshold on every pass and was cycled: replay coverage dropped, every
live subscriber told to resync, indefinitely -- on the DEFAULT configuration
every deployment lands in before it flips anything. A resync storm shipped as
the default, by the feature whose stated purpose is to avoid exactly that load
inversion.
Publishing and detecting are now one switch, which is what they always were:
an instance detects off its OWN frames -- it publishes to the channels it
subscribes to and receives them back -- so it never depended on peers having
flipped, and there was never a reason for the two to be separable. Phase 1 is
"recognise the frame so a phase-2 peer costs you nothing", and nothing else.
Regression test plus its counterfactual, so "no cycles" cannot be satisfied by
a detector that has simply stopped working.
P1: the maintenance loop published heartbeats and scanned for idleness on one
goroutine. publishHeartbeats makes N synchronous Redis publishes, and against
the failure this feature exists to detect those are precisely the calls that
block -- bounded by go-redis's own Dial/Read/WriteTimeout, not by any context
we can pass. A stalled publisher could therefore delay detection for as long
as those timeouts take, on the very instance whose connections had wedged, and
for longer the more workspaces it carried. Two goroutines with their own kick
channels; a stalled publisher now just produces silence, which is what the
detector reads.
P3: the cycle held the workspace's establishment record across a synchronous
observer report, so an Observer callback that subscribed to that workspace
would wait on a record only the reporting goroutine could retire. Moved the
SubscriptionCycled report past establishment. The narrower half is older than
this code -- confirmSubscription's late-acknowledgement path already reported
from inside that window -- so it is documented on the Observer interface as a
contract rather than silently worked around: a callback may publish, read and
unsubscribe; it may not subscribe.
Prose swept for what the gate falsified, per the team convention: the
constructor comment that argued for the defect, config.EventsHeartbeat's
rollback paragraph, the config test's inverted-rationale comment,
ResetReasonIdleTimeout, both metric Help strings, and deployment.md's phase
table and rollback section. All of them now say that phase 1 detects nothing
and that the cycled counter is STRUCTURALLY zero there -- a zero on phase 1
says nothing about whether a route has wedged, which is the reading an
operator would otherwise get wrong.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(events): prove a resuming joiner is told sync_required across a cycle
Codex round 2 raised that a subscriber arriving DURING an idle cycle gets no
gap signal, because dropWorkspaceCoverage only signals subscribers present
when it runs. True, and for a RESUMING caller the gap signal is not what
protects it: the registration mark is. It registers while the workspace has no
buffer, so its mark cannot match whatever buffer exists by the time it reads,
and eventsSinceMarkLocked answers nil -- sync_required rather than a false
"caught up".
A FRESH caller is deliberately not signalled and the finding is DECLINED for
that case, with reasons recorded at the test: it holds no prior position, so
there is no span it could be missing; it is admitted only after the
replacement subscription is acknowledged, because it waits on the cycle's
establishment record which finishPending closes after the confirmation; and on
the unconfirmed-admission path it IS told to reconcile when the acknowledgement
lands. Signalling it anyway would demand a resync of a client with nothing to
reconcile -- the load inversion this unit already had to fix once.
THE FIRST TWO VERSIONS OF THIS TEST DID NOT DISCRIMINATE, which is the part
worth keeping. Version one asserted the empty case: the cycle leaves no buffer,
so eventsSinceMarkLocked returned nil from its `!ok` term and removing the mark
check entirely still passed. Version two published inside
afterSubscriptionConfirmed so a FRESH buffer exists before the joiner reads --
and deleting the `mark.buffer == nil` term still survived, because the keep
arithmetic in that function already reduces to zero for a nil mark. Only
replacing eventsSinceMarkLocked with the unmarked eventsSinceLocked fails the
test, handing the joiner the post-cycle event as though it followed its cursor.
That is the mutation the test is built against, and the redundancy inside
eventsSinceMarkLocked is recorded rather than mistaken for coverage.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(events): only count a cycle that actually replaced the connection (codex r3)
Three findings from a fresh-angle round on shutdown, wire format and doc
accuracy. The wire-format angle came back clean -- events:<workspace> cannot
collide with watchevents under validated namespaces, and no valid activity
payload can be mistaken for an hb| frame.
P3, and the one that stings: config.EventsHeartbeat still said phase 1
"already runs idle detection off whatever traffic exists". That is the exact
sentence the previous commit's sweep existed to remove, in a file that sweep
edited. A grep for the phrasing I remembered writing missed the paraphrase
sitting four lines above the paragraph I did fix.
P3: SubscriptionCycled was reported unconditionally after establishSubscription
returned, but establishment has two reasons to install nothing -- the bus
closed, or the workspace emptied while we dialled. The counter's documented
meaning is "torn down AND replaced", and counting an aborted establishment is
wrong in the direction that matters: an operator reading a non-zero rate
concludes connections are being blackholed, so a shutdown would manufacture
that signal. Now reported only when a replacement is installed, verified by
generation. Both Help strings and deployment.md say "counts replacements, not
teardowns"; the teardown stays visible through the idle_timeout reset reason.
P2: Close does not join the maintenance goroutines. Kept that way and
documented on Close, because the publish half makes synchronous Redis calls
bounded by go-redis's own timeouts -- the calls that stall on exactly the
wedged route this feature detects -- so joining would let a dead network hold
shutdown open. What has to hold instead is that a cycle already past its ctx
check leaves nothing behind, which is now pinned by a test that closes the bus
from inside the cycle's establishment: no subscription installed, no
establishment record stranded, no counter moved.
liveGen moved from the test file into the package -- production needs it now.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(events): restore the coverage the phase gate silently removed
The mutation matrix, re-run against the post-codex code, showed M3 -- removing
the install-time lastSeen stamp -- going from DETECTED back to SURVIVED. The
cause was my own round-1 fix: gating idle detection on heartbeat phase 2 means
a phase-1 bus never scans, and TestAnUnconfirmedAdmissionIsNotCycledAsIdle
built its own phase-1 bus. It was the only test that could observe a zero
lastSeen, because the plain fresh-subscription case is stamped twice over --
at install, and again by the acknowledgement. Flipped to phase 2 and
re-verified: removing the stamp fails it again.
Worth naming the shape rather than just the fix. A behaviour change that
narrows when code runs silently narrows what the tests reach, and nothing in a
green suite says so -- the tests still pass, they just stopped asking. Only
re-running the matrix after the change surfaced it.
Two harness bugs fixed alongside, both of which had been reporting
non-results as if they were readings:
- A mutation that INSERTS keeps its own anchor, so the "did the edit land?"
check read every insertion as ANCHOR-ERROR. It compares the file now.
- The two rule-3 mutations left `sub`/`live` unused and came back BUILD-BREAK
rather than answering the question; they carry the same discard the
follow-up harness already used.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(events): close the wiring and barrier gaps codex round 4 found
Concurrency and lock discipline came back CLEAN -- the establishment record
and the generation checks cover two racing cycles, Unsubscribe, Publish and a
stale resubscription frame, with no lock-order deadlock. The four findings
were all about whether the tests measure what they claim.
P2, and it is the convention I had cited three commits earlier: the heartbeat
flip had no wiring test. internal/events proves a bus built with
publishHeartbeat=true emits frames and detects idleness, and every one of
those tests passes if newObservedEventBus hardcodes false -- the deployment
would simply never detect a wedged connection, which is indistinguishable from
a deployment that has none. Both directions asserted, because a helper that
ignored its config and hardcoded EITHER value passes a one-directional test.
Mutation-checked against exactly that edit.
P2: the metrics adapter test never touched SubscriptionCycled or the
idle_timeout reason, so an adapter that folded the counter into the reset
series -- destroying the very distinction those two are built to keep apart --
would have passed. Both added with counts that differ from their neighbours',
the pattern that file already uses so a label-dropping adapter cannot satisfy
the totals by coincidence.
P3: TestAHeartbeatConsumesNoEventID "waited" on a predicate that returned true
unconditionally. Not a slow wait -- no wait at all: the counter was read with
the publishes still in flight, so a heartbeat that DID consume an id could
land afterwards and the test would still pass. It now waits on the frames
arriving, and fails against a mutation that publishes an event alongside each
heartbeat.
P3: the maintenance goroutines started on phase 1, where both halves are
guaranteed no-ops -- two goroutines and two timers per process waking every
30s for the life of a deployment that asked for none of it, and phase 1 is the
DEFAULT. The flag is constructor-only so the decision is taken once. The
in-function gates stay: those are the correctness ones, and the tests reach
them directly without a loop.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(events): validate the heartbeat frame and stop serialising recovery (r5)
Client-facing behaviour came back CLEAN: an idle cycle signals each local
subscriber, the SSE handler emits an in-band sync_required with an empty id
while holding the connection open, EventSource retires its cursor and the web
client runs the documented reconciliation. Two P2s on the other angles.
FRAME VALIDATION. Accepting any "hb|..." created a silently-ignored class on
the workspace event channel, where before this feature EVERY unreadable
payload ended coverage loudly and moved undecodable_message -- the counter
whose documented job is "suspect a namespace collision". A foreign or buggy
publisher whose bytes happened to start with the prefix slipped through that
signal without a trace. A frame is now hb|<version> plus optional short tokens
under a length cap; anything else wearing the prefix goes back to being a
coverage-ending decode failure, and the forward compatibility the prefix was
chosen for survives for a disciplined future frame.
What this deliberately does NOT try to fix, because it is not a hole: a forged
frame cannot fake liveness. Liveness means "this socket carried traffic", and a
frame that ARRIVES demonstrates exactly that whoever sent it -- which is why
stampLastSeen already fires for undecodable frames. There is no coverage claim
inside a heartbeat to forge.
CADENCE DRIFT, which was self-defeating rather than merely untidy. The timer
restarted after each pass, so the real period was T plus however long the pass
took. For the publisher that means an instance whose publishes are slow emits
heartbeats further apart, its own subscription sees them further apart, and it
can cross its own 3T threshold and cycle connections that were never wedged --
the slowness manufacturing the incident. Scheduling is deadline-based now, and
resets rather than bursting when a pass overruns badly.
SERIAL RECOVERY. One idle pass re-established every due workspace in sequence,
each re-dial bounded by go-redis's own timeouts, so recovery took N x that
timeout with the last workspaces reporting themselves uncovered throughout.
The failure that puts many workspaces on the due list at once is a Redis
failover, so the serial case was the common one. Bounded-parallel at 8 -- each
entry already owns its establishment record so they are independent by
construction, and an unbounded fan-out would answer a struggling Redis with one
dial per workspace at once. Test covers more workspaces than the cap, and
fails against a version that drops the overflow.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(events): idle_timeout means coverage ended, not connection replaced (r6)
Codex round 6 came back clean on the non-Redis path (MemoryBus ignores the
Redis-only flag; EventBus and Close have not drifted), on the rollback
rehearsal (phase-2 to phase-1 and a mixed fleet are safe as documented,
including a bus mid-cycle -- Close cancels it, prevents installation and
retires its pending record), and on the operator surface
(PAD_EVENTS_HEARTBEAT is a server env/TOML setting; `pad configure` is client
connection config and needs no new surface).
The one finding is a contract drift I introduced two commits ago and then
wrote prose for in the same commit. Making SubscriptionCycled mean "replaced"
was right; what I missed is that the idle_timeout RESET REASON is emitted
earlier -- dropWorkspaceCoverage runs before the re-establishment -- so it can
fire when nothing is replaced, which is exactly the shutdown case the counter
was changed to exclude. Three doc sites and one log line said "replaced the
connection" anyway.
They now say what is true at the moment each fires: idle_timeout means
COVERAGE ENDED, only pad_event_subscription_cycled_total proves a replacement,
and the log says "attempting to replace" rather than "replacing". The log
wording matters on its own -- an operator correlating it with the counter
would otherwise find the log without the counter and go hunting a bug that
isn't there.
Third time this unit has produced prose the next change falsified, and each
time a different reviewer angle caught it rather than the sweep I ran at the
time. The pattern is that a behaviour change and the prose describing it land
in one commit, so there is no diff between them to notice.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(cmd): drive the heartbeat wiring test instead of sleeping at it (r7)
Codex round 7 found no leftovers across seven rounds of edits, and confirmed
the mass-cycle case does NOT produce a reconnect storm -- the SSE connections
stay open across a sync_required, so the admission limits are never consulted.
P3, and it is the failure I have been criticising in other people's tests: the
wiring test used a 300ms sleep as its ordering barrier. Under -race or on a
loaded CI box, a phase-1 bus that is correctly silent and a phase-2 goroutine
that merely has not been scheduled yet are indistinguishable, so the test could
pass or fail for reasons unrelated to the flip it exists to check. It now
drives one publish pass synchronously through a named test hook and uses an
ordinary event on the same channel as the barrier, which Redis delivers in
publish order. No timing left. Verified: still fails against the flag being
hardcoded false, and ten consecutive -race runs are green.
That replaces SetMaintenanceCadenceForTest with PublishHeartbeatsForTest rather
than adding to the exported test surface -- the loop's own wiring is covered
inside internal/events, where the unexported setter is available.
P2 is FILED, NOT FIXED, as BUG-2761: a mass coverage drop tells every connected
subscriber of every affected workspace to resync at once, and each browser tab
independently calls /changes with per-tab coalescing but no jitter and no
global budget. The fix is a web-client change plus possibly a wire-format hint,
which is independent of half-open detection and would materially expand this
diff. Worth filing rather than shrugging at because this unit makes the
simultaneous case MORE likely: it adds a third trigger of a class that already
existed (Redis failover, epoch change), and its natural cause is exactly a
network event that wedges many routes at once. deployment.md carries the
residual with the bug ref so an operator meets it before the incident does.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(events): make the tests prove what their comments claim (codex r8)
Round 8 was claim verification rather than bug hunting -- check the diff's
load-bearing assertions against the actual code -- and it was the highest-yield
round of the eight. The go-redis assertions (Ping writes without reading, the
channel path sets no read deadline, TLS dials ignore cancellation) and the four
claims about neighbouring functions all held. Seven other assertions did not.
TESTS THAT DID NOT PROVE THEIR OWN HEADLINE. This is the substance of the
round, and every one of these passed before and after:
- The JOINT TEST -- this unit's flagship -- claimed to discriminate the
two-subscriptions failure and did not. Fan-out is per subscriber, so a joiner
that opened its OWN second subscription still delivers the event to everyone
exactly as the test expected. Nothing separates one subscription from two
except counting them, which it now does at Redis, plus a duplicate-delivery
check for the second receive loop. Fails against the pending record not being
minted in the scan.
- The remedy test said "the old connection must also be gone" and waited for a
receive-loop exit. stopRedisSubscription does two things and the loop exits on
the first alone, so it passed against a version that cancelled the loop and
left the PubSub and its health check open. Counted at Redis now; fails against
exactly that mutation.
- The parallel-recovery test could not tell serial from parallel -- a serial
pass cycles all thirteen workspaces too. It now uses a rendezvous, asserts the
peak concurrency is above one AND within the cap, and fails against a serial
implementation.
- The prefixed-garbage test only exercised the classifier. Whether
receiveMessages ACTS on the error is a different claim, now driven through
the real Redis path.
- The metrics adapter test's comment said "every reason this bus can emit"
while subscription_unconfirmed was missing; its zero-assertion proved
non-leakage, not mapping. Emitted now with a count distinct from its
neighbour's, so a merging adapter cannot satisfy both.
PROSE THAT OUTLIVED THE CODE, again. The latency arithmetic still described the
single shared ticker that round 5 replaced with two independent loops; from
lastSeen [3T,4T) still holds, but from FAULT ONSET it is roughly [2T,4T)
because the publisher has its own phase. And a second "and replaces the
connection" in deployment.md that round 6's sweep missed.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(events): correct three contract statements (codex r9)
Round 9 was cross-artifact conformance: every commitment the plan made was
checked against the code. All met -- wire classifier, lastSeen placement and
locking and install stamp and every-frame stamping, heartbeats bypassing
Publish and the shared counter, the drop-and-cycle remedy under the
single-establisher invariant, all four joint rules, the two-phase rollout with
its inverted-rationale test, and the reason/Help/deployment.md trio with the
rollout order. It also confirmed the three documented mutation survivors are
correctly dispositioned: both wsCounts checks are redundant-but-cheap under the
current invariant, and omitting the lastSeen.IsZero() skip is right because
adding it would mask a regression in the install stamp.
Three statements were wrong.
The env-var contract. My test comment said an unparseable PAD_EVENTS_HEARTBEAT
"must leave the flip off", which is true from a default config and false from a
config file that set it true -- there the value is left alone, as the
precedence test already asserts. The BEHAVIOUR is right and matches the epoch
flag: a typo must not move a migration in either direction, and silently
rolling an operator back to phase 1 would disable detection on a fleet that had
opted in with nothing saying so. Only the prose overclaimed, and it overclaimed
in the direction that invites someone to "fix" the ignore into a fail-closed
reset.
The constructor. NewRedisBusWithKeys documented publishEpoch and said nothing
about publishHeartbeat sitting next to it -- two adjacent booleans of the same
type belonging to two independent migrations, which is a shape that gets
swapped or dropped in a maintenance edit. Both now documented in order, with a
note that any combination is valid.
A stale count. EventSequenceResetsTotal's comment said "Five reasons" and there
are seven; it was already wrong by one before this unit added another. Replaced
with the count plus a pointer to the three artifacts that are authoritative and
move together, since the count itself is the part that goes stale first and is
read last.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(events): make the cadence arithmetic testable, and justify a guard pair
Matrix 5 (29 mutations, 21 detected) surfaced two things the previous run
could not, because both concern code the codex rounds added.
THE DRIFT FIX HAD NO TEST. Restoring the sleep-after-work form survived every
test in the package, and would have kept surviving: the only way to observe
drift through the loop is to time it, and a timing assertion is a flaky
assertion. Extracting nextTick makes the arithmetic checkable without a clock,
and the four cases now pin what the schedule is for -- a slow pass does not
push the next tick out, ten slow passes accumulate no drift, an overrun beyond
one interval resets instead of replaying the missed ticks, and an overrun
WITHIN one interval still catches up rather than re-phasing the schedule
permanently. Both directions mutation-checked.
The property is worth this much because breaking it is self-defeating rather
than merely untidy: an instance whose passes are slow emits heartbeats further
apart, its own subscription sees them further apart, and it crosses its own 3T
threshold and cycles connections that were never wedged.
A GUARD PAIR THAT ONLY DIES TOGETHER, which the team lesson says to treat as a
question rather than a clearance. The loop's ctx.Done select arm and its
post-wait ctx check each survive removal alone. Checked rather than assumed:
they cover disjoint moments and each is independently right -- the select arm
is the exit while WAITING, which is where the goroutine spends its life, and
the post-wait check stops a bus that closed DURING a pass from starting
another one against a cancelled context and a drained wsSubs. Removing BOTH is
detected. Reasoning recorded at the code, and the combined mutation added to
the matrix so the pair cannot quietly become a single point of failure.
Also fixed an ineffassign the lint gate caught in the new test.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(events): state what the detector does not cover (codex r10)
Round 10 was adversarial: refute the unit's central claim rather than look for
defects in it. It partly succeeded, and the corrections are worth more than
most of the bug findings.
The claim was "a wedged connection is detected, coverage is ended, and the
connection is replaced so delivery resumes". Three parts of that were too
strong, and all three limits were checked against go-redis v9.22.0 rather than
argued:
IT IS A RECEIVE-SIDE DETECTOR, not a round-trip health check. It measures
whether frames ARRIVE. A subscription whose outbound direction is broken but
which still receives reads as healthy -- correctly, since nothing is lost, but
that is a narrower claim than "the connection is healthy".
IT CANNOT COVER THE PUBLISH PATH. PUBLISH travels on the client's connPool
while a subscription holds a connection from the separate pubSubPool
(redis.go:363, :1956) -- different sockets, different fates, and a reconnect of
one repairs nothing about the other. An instance whose publish path is wedged
loses its own events for every other instance and this feature will not say so.
That is a real gap in the family's coverage, now written down rather than
implied away.
REPLACEMENT IS ATTEMPTED, NOT GUARANTEED. If the path is still blackholed when
the cycle re-dials, the replacement cannot receive either. Coverage stays ended
so nothing is falsely claimed, but delivery resuming is a statement about the
network rather than about this code.
Filed BUG-2764 rather than folded in: establishSubscription's
`b.client.Subscribe(dialCtx, channel)` silently discards the SUBSCRIBE error,
because go-redis's own Client.Subscribe drops it (`_ = pubsub.Subscribe(...)`,
redis.go). A failed subscribe therefore installs a connection that looks live
and is subscribed to nothing. It is pre-existing, it lives in the establishment
path three bugs have already converged on, and changing how that function
issues its SUBSCRIBE does not belong in a diff about idle detection. Worth
knowing here because it is the one way the replacement can fail on a HEALTHY
network -- and because the detector now cycles it on the next pass, which is
why it self-heals on phase 2 and stays dead forever on phase 1.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(events): do not cycle a workspace that recovered before its turn (r11 P1)
Codex round 11 attacked three claims. Phase-1 safety and rollback safety both
came back clean -- a phase-1 receiver stamps lastSeen and nothing else, touches
no buffer, metric, client, ID or epoch, and its maintenance loop is not started
at all, so that timestamp is inert; heartbeats leave no state in Redis or
across a process replacement, and a mid-cycle shutdown rechecks b.ctx before
installing. The third claim did not survive.
FALSE POSITIVES ON A HEALTHY SYSTEM, which is the property this design cares
about most: cycling a working subscription drops its coverage and resyncs every
one of its subscribers for nothing.
cycleIdleSubscriptions selects its victims under the lock and releases it; the
cycles run afterwards. Its re-checks asked about generation, subscriber count
and bus liveness -- and never re-asked the question the scan had asked. A
subscription that started receiving again in that window was cycled anyway.
The window is not theoretical, and this unit widened it itself: the 8-way
concurrency cap added in round 5 makes a workspace wait behind earlier batches
of slow replacement dials, and a GC or CPU pause leaves a backlog of heartbeats
undrained in the receive loop. Both are ordinary conditions on a loaded box.
cycleOne now validates, ends coverage and tears down WITHOUT RELEASING THE LOCK
in between, which needed dropWorkspaceCoverage split into a locked variant that
returns its reason for the caller to report after unlocking. That also removes
the ordering fragility the previous version documented rather than fixed: there
is no longer any window in which coverage is ended for a workspace this
function then decides to leave alone. The log moved after the decision for the
same reason -- it could previously describe a cycle that then abandoned.
The freshness term is load-bearing and says so, next to the three neighbouring
terms whose mutation survivals are recorded as redundant-but-cheap. Removing it
is detected, by a test that lands the recovery in the exact gap through a new
positional seam.
NTP steps were checked and are not a hazard: time.Time carries a monotonic
reading, so a wall-clock step cannot make a subscription look idle.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* perf(events): take logging and PubSub.Close off the global lock (codex r12)
Round 12 verified round 11's freshness fix: validation, coverage invalidation
and teardown are atomic under b.mu with no lock cycle,
dropWorkspaceCoverageLocked preserved the original semantics exactly including
the no-buffer branch that still signals subscribers, reset reporting happens
after unlocking, and the replacement metric still lands only when a new
generation does. Slow establishment stays outside b.mu, wg.Wait only delays the
next pass, and Close cancellation retires pending records.
Two P2s, both about what round 11 put UNDER that lock:
slog.Warn ran while b.mu was held. slog invokes the installed handler
synchronously, and b.mu is the lock every fan-out and every Subscribe on the
instance contends for -- a slow or custom handler stalls all of them, and one
that calls back into the bus deadlocks. Moved after the unlock; it still has to
come after the DECISION, for round 6's reason, so both constraints are now
stated together at the call.
PubSub.Close ran under b.mu too. It takes go-redis's own mutex, which the
health check can hold across reconnect work, so a network-bound wait sat inside
the instance's hottest lock. That was survivable when teardown only happened as
a workspace lost its last subscriber; the idle detector makes it happen on
every cycle, which is what turned a latent cost into a real one. Handed off to
a goroutine: nothing references the PubSub once the map entry is gone, and
cancel() -- which is what actually stops delivery -- still happens under the
lock.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(events): do not read our own failed probe as a dead peer (codex r13)
Round 13 asked for a production-approval review. Four findings; the second is
the sharpest of the whole run because it is the mirror image of the failure
this feature exists to find.
A FAILED HEARTBEAT PUBLISH WAS READ AS A DEAD SUBSCRIPTION. The detector's
inference is "we published a frame and nothing came back, so the receive path
is dead" -- valid only if the publish actually happened. PUBLISH travels on the
client's connPool while the subscription holds a connection from the separate
pubSubPool, so a publish-side failure (pool exhaustion, a wedged outbound
route, Redis refusing writes) says nothing about whether that subscription can
receive. The detector was reading its own inability to probe as evidence about
the peer, and tearing down healthy connections on a schedule: a resync for
every subscriber of every workspace, every 90s, for as long as the outbound
path stayed broken. The third load inversion this unit has had to fix.
redisSub.lastProbeOK now records the last SUCCESSFUL publish, and detection is
suspended while it is stale -- checked in the scan and again in cycleOne, which
is a pair that only dies together and is therefore justified at the code:
the scan's keeps a workspace off the due list so no record is minted and no
joiner waits, cycleOne's covers the probe failing AFTER selection, a window the
concurrency cap makes real. Neither subsumes the other; removing both is
detected. New counter pad_event_heartbeat_publish_failures_total, documented as
DETECTION DEGRADED rather than as a peer being broken.
THE END-TO-END TEST THAT DID NOT EXIST. Every other test drives this through a
fake clock -- necessary, since the threshold is 90s by construction and
miniredis always answers, but it means they all ASSUME the wedge rather than
produce it. A TCP proxy that stops delivering server->client on the connections
already open, while writes keep succeeding and new connections stay healthy,
produces the real thing. The test asserts both halves of the claim: the wedge
is detected, and the replacement delivers. Both halves mutation-checked
(detector disabled; drop-only with no replacement).
The proxy's first version was vacuous -- a global flag consulted at read time
meant re-enabling delivery for future connections also revived the ones meant
to be dark. Per-connection now, and the comment says why.
Also: PubSub.Close taken off b.mu in Close() too (round 12 fixed only the cycle
path), and the replacement counter now takes an explicit installed result from
establishSubscription rather than inferring one from the live generation --
inference misattributed an unrelated caller's fresh subscription as this
cycle's replacement, and missed a real replacement that had lost its last
subscriber. Both mutation-checked.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(events): bind the probe stamp to a generation; make the proxy test honest
Round 14 returned a BLOCK verdict on two P2s, both mine, both in the fix that
round 13 had just added.
lastProbeOK WAS NOT GENERATION-BOUND. publishHeartbeats snapshots the workspace
list, publishes off the lock -- for as long as go-redis's timeouts allow -- and
then stamped whatever subscription occupied that workspace by the time it
returned. A probe sent for generation A could credit generation B, which never
received one; if later probes then failed, B could be cycled while looking
recently probed. Exactly the hazard stampLastSeen already guards on the same
map, and I did not carry it across. The generation now travels with the
snapshot and is validated before stamping.
THE END-TO-END TEST COULD PASS WITHOUT EXERCISING WHAT IT CLAIMED. It darkened
the receive direction of every open connection, including the ordinary pooled
connection PUBLISH uses -- so the probe may have been failing too, and the run
would then have been exercising the cannot-probe path rather than a half-open
route, which is the very distinction round 13 added the premise check for. The
proxy now classifies connections as it forwards and darkens only one that has
carried a SUBSCRIBE, leaving the publish path healthy, and the test asserts
zero probe failures so a run that drifts back into the other case fails loudly
instead of passing quietly. Still fails against a disabled detector and against
drop-only.
Also covered the new counter's mapping in the metrics adapter test, with a
count distinct from both neighbours -- cycled, idle_timeout and
heartbeat-publish-failure say three different things and an operator acts on
the difference.
Verified by the same round: install-time stamping does not permanently suppress
detection, establishSubscription returns false only on abandon and true on all
three installed paths including the cancelled-establisher goroutine, and
Close's deferred PubSub.Close runs after the unlock.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(events): pin the probe-across-replacement race (closes r15's residual)
Round 15 returned CLEAN and approve-with-comments, naming one residual: the
generation binding on lastProbeOK had no deterministic test, only the argument
that it mirrors stampLastSeen. This closes it with a positional seam between
the publish and the stamp, which is the only place that interleave can be
forced.
TWO INSTRUMENT DEFECTS ON THE WAY, both caught by mutation rather than by
reading:
The first version compared the credited stamp against the PROBE's timestamp.
On a frozen clock the replacement's install stamp and a wrongly-credited probe
are the same value, so it could not tell them apart -- it failed on the install
stamp while claiming a credit had happened, and removing the generation binding
still passed. It now compares against what the replacement was INSTALLED with,
and the clock advances inside the seam so a buggy write lands strictly later.
The second version was FLAKY: 2 failures in 3 runs. The heartbeat that was just
published comes back through miniredis on another goroutine, and if it lands
between the forced-stale write and the scan it refreshes lastSeen, the
workspace is not due, and no replacement happens. Retried until the generation
actually moves. Now 5 of 5 green unmutated and 5 of 5 detected mutated -- which
is the bar, because a 2-in-3 detector reads as coverage while being noise.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(events): on-call signals — log the cycle outcome, correct two claims (r16)
Round 16 read the diff as the person paged at 3am. Four findings.
THE CYCLE LOGGED ITS ATTEMPT AND NEVER ITS OUTCOME. The line says "attempting
to replace", which is correct and, on the one path where the replacement does
not happen, left an on-call with a warning, no counter movement, and no
explanation. Now there is a second line naming the reason.
pad_event_receive_loop_exits_total's documentation was falsified by this unit
and neither doc site said so: every idle cycle stops a receive loop while its
subscribers are still connected, and the comment still claimed exits happen
only at shutdown or when the last subscriber leaves. Both sites corrected, with
the expectation that it tracks the cycle counter during an incident.
A CLAIM I MADE AND THEN COULD NOT SUPPORT, recorded rather than quietly kept.
Round 16 argued the age-based premise check ("has a probe succeeded within the
threshold") failed to suspend detection where an ordering rule ("has a probe
succeeded since anything last arrived") would, and I rewrote the rule on that
argument and wrote a test named for the defect. The mutation matrix then
refused to confirm it: reverting to the age form leaves the test green, and so
does removing both copies of the check, and no case separates the two — on any
healthy path the two stamps advance together, because a probe whose frame
arrives sets both, and they diverge only on the wedge where both forms cycle.
The ordering rule is kept, because it states the intent exactly and is never
weaker. But the test and the comment now say what they actually establish —
that a probe which has started failing stops the detector concluding from
silence, which is the property both forms share and neither had before — rather
than claiming a fixed defect I cannot demonstrate.
The two remaining P2s are already-filed residuals: the cycled counter proves an
install rather than a working replacement (BUG-2764), and repeated cycling
amplifies /changes load with no jitter or global budget (BUG-2761). Both are
documented in deployment.md with their refs.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(events): record what the final matrix actually says about four guards
Final matrix: 34 mutations, 22 detected, baseline restored green. Every
survivor is now documented at its line with why it survives, and two of them
turned out to be instrument defects rather than coverage gaps.
lastProbeOK's INSTALL STAMP IS REDUNDANT and the comment claimed otherwise. It
said a zero value "would permanently disqualify a subscription from ever being
cycled" -- true of the age-based premise it was written for, false under the
ordering rule that replaced it, because a zero value fails
`lastProbeOK.After(lastSeen)` exactly as an install stamp equal to lastSeen
does. Kept, for a reason it earns: it makes the field's invariant true by
construction, so a future rule reasoning about this value's AGE gets a real
timestamp rather than 1970 -- which is the trap the age-based rule fell into
one field over.
THE TWO cycleOne ABANDON GUARDS DIE ONLY TOGETHER AND ARE NOT REDUNDANT, which
took checking rather than assuming. They catch different shapes of the same
recovery: an arrival that has not been re-probed pushes lastSeen past
lastProbeOK so the premise case fires and the freshness case is unreachable --
that is the shape the test produces, and it is why removing either alone stays
green. But the publisher runs on its own goroutine at its own cadence and can
land a successful probe between the arrival and the decision, putting
lastProbeOK ahead again; there only the freshness case stops a healthy
subscription being torn down. Deleting it on the strength of the matrix would
remove the second shape's only guard.
Close's off-the-lock PubSub.Close is UNTESTED BY DESIGN, recorded rather than
papered over. It is a contention property, and the only assertion that
separates it is a timing one, which in this suite is a flaky one.
TWO HARNESS DEFECTS, both of which produced false survivors that would have
gone into the evidence package as findings. M11a inserted its mutation AFTER
the gate it was meant to disable -- unique anchor, wrong placement, so the
early return still fired and nothing changed; with a correct anchor it is
detected. M20 left variables unused and came back BUILD-BREAK rather than
answering; in compiling form it genuinely survives, consistent with
establishSubscription's abandon path already covering it.
The lesson worth keeping: when I rewrote all 34 anchors against current source
I verified each matched exactly ONCE, and uniqueness is not placement. An
anchor can be unique and still land somewhere that changes no behaviour.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(events): barrier the probe test on delivery — it was flaky, CI caught it
Go (PostgreSQL) failed on
|
||
|
xarmian
|
650d9df270 |
fix(events): bound the replay by append POSITION and buffer identity
Codex round 3. Both P1s real. The ceiling used lastAppendedID as if it were a time boundary. This bus's ids come from a counter shared across workspaces and a phase-1 publish assigns and publishes in two calls, so arrival order and numeric order genuinely disagree. Against an id-valued bound both directions break at once: a straggler arriving after registration is replayed although it also went to the caller's channel, and a pre-registration event carrying a higher id is filtered out and never replayed at all. replayBuffer now counts its appends, and the bound is a position — the entries to withhold are simply the final (appends - mark) of whatever since() returned, which may trim from the front but never the back. The mark also carries the BUFFER, not just a position in it. An ID-space reset during the wait replaces the buffer wholesale; a position in the old one describes nothing in the new one, and knownFrom may still accept an adjacent cursor, so the mismatch does not announce itself. Also corrected, all found by the same round and all mine: the Observer comment claimed this counter never reaches SequenceReset, which the late-confirmation path contradicts; the reason enumeration in metrics.go, its Help string and docs/deployment.md were never updated for the sixth reason; and both the metric and its comment said every increment is a client when it is one establishment however many subscribers were waiting. Accepted, not fixed: since() evaluates eviction over the whole buffer including post-registration appends, so a flood inside the wait can evict a cursor that missed nothing and force a sync_required. It costs a spurious resync, never silent loss, which is the direction this family chooses every time. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
0accca7a40 |
test(events,docs): pin the detection chain a colliding repair actually relies on (BUG-2740, codex round 5)
The repair seeds from wall-clock seconds, which is above any COUNTED history and is not a monotonicity guarantee. Corrupt the key twice inside one second and both repairs seed the same value, so two genuinely different id spaces carry the identical epoch — and an equal epoch means 'same space' by design, so neither epoch_change nor epoch_regressed fires. It is still not silent, and the reason is worth pinning because it is not the one the epoch mechanism suggests: a merge needs ids REUSED at a receiver, reuse needs the sequence to go BACKWARDS, and backwards is detected whatever the epoch says. counter_backward drops the affected buffers and refuses cursors below the discarded high-water mark. That was folklore until it was measured. The test drives the whole sequence — two repairs seeding the same value, a sequence reset between them — and asserts its own premise first (the two spaces really do share an epoch), that no epoch-based reason fires, that counter_backward does, and that the old cursor is refused rather than replayed the new space's events. Mutation-verified: stop reporting counter_backward and it fails. The docs carry the chain as a quoted rule, plus the two cases that look like it and are not — a counter set FORWARD is a jump inside one space with no reuse, and a receiver that never held the colliding range experiences a gap, which is BUG-2735's pre-existing class rather than anything this introduces. Lead re-ruled on the probed fact: residual ACCEPTED because it is detected, attribution corrected from epoch_regressed to counter_backward. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
383c6dc8d8 |
docs(events,metrics): finish the epoch_regressed claim and fix the heading level (BUG-2740, codex round 5)
Three of the round's four findings, all mine. THE SAME CLAIM IN THREE PLACES, TWO OF WHICH I HAD FIXED. Round 4 updated the struct comment above the counter and the new operator section; the EXPORTED Prometheus Help string and the deployment table row still told operators that epoch_regressed means Redis lost writes. The Help string is the one a person reads at the scrape endpoint, so it was the worst of the three to leave. Third time this run that a claim lived in more places than I enumerated before editing. 'CLIENTS RESYNC ONCE' was too strong. A repaired generation that lands BELOW one a receiver already holds is discarded as a straggler for that instance's 30-second window rather than adopted, so the same space can be disclaimed again when it is finally taken up. Bounded by the window, and distinguishable because it surfaces as epoch_regressed rather than epoch_change. The new section was a ### under a ## , which adopted every following #### section — including Event ID-space migration — as its children. It is a #### now, a sibling of the sections around it. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
8c05b02c7d |
docs(events,metrics): document the repair path an operator will actually meet (BUG-2740, codex round 4)
The fix changed an operator-visible outcome and nothing said so. A corrupted generation counter used to burn a sequence ID and fail every publish forever; it now repairs, publishes, and reseeds from wall-clock seconds — which the existing prose contradicted in two places, calling the generation 'monotonic' and telling operators that epoch_regressed means a failover to a replica that lost writes. Both corrected, and a new section states the two consequences: a repair can surface as epoch_change or, if a collision had pushed the counter higher, as epoch_regressed; and clients resync ONCE, not in a loop, because the repaired key is valid and the next rotation increments it normally. THE TELL IS THE VALUE, and saying so is the honest version of a claim I very nearly shipped instead. My first draft told operators to distinguish a repair from a failover 'by the neighbouring WARN log line naming the key'. There is no such line — the repair happens inside a Lua script, which cannot log through slog and does not change the counter's label. What actually distinguishes them is that a repaired generation LOOKS like a unix timestamp, ten digits around 1.7e9, rather than a small count of ID-space resets. That is a deliberate property of the seed, and it is now what the docs point at. The section also went in BETWEEN two rows of the metrics table on the first attempt, splitting it exactly as the previous unit's rollout note did. The check I wrote after that one looked for blank lines between rows and could not see a whole section inserted between them; the check is now 'is each table one contiguous run', which catches both. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
f4d1616078 |
docs(deployment): name the resume-boundary residual for operators too (BUG-2739)
The residual list covered the two that leave an OPEN stream stale (BUG-2735, BUG-2738) and not the one that affects RESUMES: a counter restart with the epoch intact leaves the two ID spaces overlapping, so a Last-Event-ID inside the overlap cannot be attributed to either, and a client holding an old-space cursor there can be handed new-space notifications as though they followed it. Filed as BUG-2743 during this branch's review. Named here because an operator deciding whether a counter reset is safe should see it alongside the other two, together with the thing that actually prevents it: rotating the epoch. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
e4167a9160 |
docs(deployment): five corrections from reading the doc as a document (BUG-2739, codex round 20)
- THE NAMESPACE GUIDANCE WAS WRONG, and this is the substantive one. It said a moving undecodable_message suggests two Pad installations sharing a Redis. It does not: two CURRENT installations publish the same wire format, so their messages decode fine and the damage is cross-feeding real notifications between installations while this counter stays flat — a worse and quieter failure, and the one PAD_REDIS_NAMESPACE actually prevents. The counter indicates genuinely unreadable input: a non-Pad publisher, a mixed-version wire format mid-upgrade, or corruption. The wording was inherited from internal/events without checking that it transferred. - THE FLOOD COSTS WERE OVERSTATED AS SELF-BOUNDING. Heap growth and the announcement are bounded; per-message CPU and allocation are not — a fresh replay buffer plus a pass over every subscriber, on the single goroutine that also delivers real notifications, so a sustained flood is receive-loop starvation as much as it is garbage collection. - THE CUTOVER SECTION described every reconnecting client running a /changes delta. True of the web activity client; pad watch --stream clears its cursor and keeps the connection open, refetching nothing. The doc contradicted its own watch-stream paragraph fifty lines later, which this branch added. - 'A reconnecting client is covered in both cases' was too absolute: the shared-counter check reads at one instant and cannot see a notification published after the read, which resumeOutrunsLocalView and cmd_watch.go both already document as an at-most-once residual. - The rollout note said a reason-specific alert on either surviving reason is unaffected. False for counter_backward, whose spelling changed — which is the entire reason that paragraph exists. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
bc87aee6b5 |
fix(docs,metrics): repair the metrics table I split, and bound the fan-out claim (BUG-2739, codex round 19)
Two real findings from an unconstrained fresh-eyes pass. THE TABLE WAS BROKEN. Round 11's rollout note was inserted BETWEEN two rows of the metrics table, so every row after it — eight of them, including all the pad_event_* counters and the presence failures — rendered as plain pipe-delimited text rather than a table. A documentation change that silently breaks the page it documents is worse than the omission it fixed, and no gate in this repo renders Markdown. The note now sits after the table, and a check across the whole file confirms no blank line or prose splits any of its eight tables. THE FAN-OUT CLAIM WAS STILL TOO STRONG, in both the metric help and the docs row: 'each moves pad_watchevents_midstream_resyncs_total once per such subscriber'. The gap signal is capacity-1 and coalescing, so a second cause firing before a client has acted on the first adds no announcement. It is AT MOST one per subscriber, and reading the fan-out off the two counters needs a reset observed in isolation against idle clients. Round 5 narrowed the aggregate version of this claim and left the per-event one standing. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
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 |