Conversation
Development builds now record click-to-content by source (memory, disk, disk-late, network), cache-hit rate, long tasks during background work, signature-check and fold time, /query count and bytes per connect or reconnect, and live-route coverage. Settings > Developer shows the summary and exports it as JSON. Production and unit tests get a no-op. `pnpm probe:live` compares live subscription setup strategies against a real relay: the app's subscriber at any concurrency, packed filters and multi-channel #h, plus a canary for head-of-line delay. Refs BOT-2081, BOT-2092 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Recommendation: fix before merge. Reviewed head 0e3fd25ff80d548f3e660d0cd37f5682c0be3864 against base f5c49be04b0f4cdabc926580caf73e6035ea39d8. Posting a COMMENT, as AGENTS.md requires; this is not an approval. Seven P2 measurement defects are detailed inline.
P1: remove internal infrastructure details from the public PR. The “Early probe numbers” paragraph names an internal relay hostname. The same hostname appears in this commit’s author email and Signed-off-by identity. Replace the description with a generic environment label and arrange owner-controlled cleanup using verified public-safe attribution. This is an information-disclosure issue, not a request to change who authored the work. The changed-file diff contains no added screenshots or internal infrastructure references that I found.
Validation: hosted required checks are green at this head; Windows native validation is skipped. Locally, the three complete recorder/probe/live test files plus two temporary adversarial files passed: 61 assertions, including seven that deliberately assert the current incorrect behavior. An additional deterministic cross-session recorder replay confirmed phase contamination. The memory/disk reproduction used the actual session/store with signed fixtures and no persistence. No production source edits, real-relay probes, or native UI runs; temporary test files were removed.
Exit criteria: remove the public internal-environment references and correct the inline lifecycle, provenance, session-isolation, and probe-result defects, with regression coverage. Existing CI does not exercise these scenarios.
Non-blocking scope/validation notes: the PR description promises background attribution during live setup, but only disk hydration and background HTTP reads open intervals; live-frame signature verification is not in the reported verify stages. Narrow that claim unless implementing it is intended. Desktop JSON export remains unverified here. Private-roster and live-delivery checks are already explicitly deferred; this review does not expand the probe into those features.
Each channel open now splits into wait (until the store holds rows: a disk read or relay request) and render (rows in memory to on screen). Settings > Developer shows one bar per source (memory, disk, server) with those medians, the cache-hit rate, and the app-open subscription setup as a row of numbers. Main-thread and read counts move into a collapsed "Main thread and reads" section with plain labels. Refs BOT-2081 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11771dab69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (id) channels.add(id); | ||
| } else if ( | ||
| frame[1] === "roster" && | ||
| (frame[0] === "EOSE" || frame[0] === "CLOSED") |
There was a problem hiding this comment.
Reject CLOSED roster reads instead of accepting them
When the relay refuses or aborts the roster subscription—for example due to the quota pressure this probe is intended to measure—discoverRoster treats CLOSED exactly like EOSE and proceeds with an empty or partial channel list. The resulting strategy timings silently describe the wrong roster, so handle CLOSED as a failed discovery and only resolve after EOSE.
AGENTS.md reference: AGENTS.md:L10-L12
Useful? React with 👍 / 👎.
| state(snapshot) { | ||
| clientMetrics.live(snapshot); | ||
| callbacks.state(snapshot); |
There was a problem hiding this comment.
Track live setup as background work
During startup or reconnect while live routes are pending, this wrapper only forwards snapshots to clientMetrics.live; the only clientMetrics.background() owners in the change cover finite background reads and disk hydration. Consequently, long tasks caused by live replay verification and folding are classified as foreground rather than live-setup/background work, defeating the stated main-thread split. Add a setup interval tied to each subscription/reconnect and close it on settlement, error, and disposal.
Useful? React with 👍 / 👎.
| current.routeErrors = channels.filter( | ||
| (route) => route.status === "error", | ||
| ).length; |
There was a problem hiding this comment.
Count capacity-limited routes as failures
For rosters above LIVE_CHANNEL_CAPACITY (1,022 channels, or one fewer with an observer), subscribeRelayTraffic marks omitted routes as limited, and the coverage predicate here treats them as settled, but routeErrors counts only error. The dashboard therefore reports “All … live” with zero failures even though some channels have no live subscription; include limited routes in the failure count or expose them separately.
Useful? React with 👍 / 👎.
…essions Address review of the client metrics and live setup probe: - Leaving the channel pane drops its unfinished open, so returning is a new open. StrictMode's immediate remount keeps the click's open. - The store tracks rows restored from IndexedDB apart from freshness, so rows staled by a dropped socket no longer count as disk hits. - Live coverage and reads follow the first relay session; other retained communities no longer read as that session reconnecting. - Probe: coverage only when every channel is live, and refused runs suppress the median; canary refusals are reported, not timed; only channel EOSE moves the coverage clock; outgoing CLOSE retires a tapped request so late frames cannot revive it. Refs BOT-2081 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
The probe's roster read treated CLOSED like EOSE, and a capped read only warned, so a refused or truncated roster was measured as if complete. Both now fail. Live coverage counted routes over the live capacity as settled but not failed, and the panel labelled them all live; they now count as failed and the panel says settled. Refs BOT-2081 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
877a5df to
3c2b790
Compare
Long tasks and CPU batches were checked against token intervals opened by disk restores and background reads. Per-stage CPU (verify.read, verify.restore, fold) already shows what warming costs, so drop the interval tracking and keep long-task count and total. Also reuse percentile in the live setup probe and drop the unused cpu.calls. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
b9bd330 to
0ccb42f
Compare
Dev-only client instrumentation, plus a probe that measures live subscription setup strategies.
What changes
In the app (development builds only). Metrics stay in memory. Nothing is uploaded. Production builds and unit tests get a no-op recorder.
memory,disk,disk-lateornetwork, which gives the cache-hit rate. Opens with nothing to paint are counted as Not timed instead of being timed: empty or failed channels, and rows kept offscreen by a saved scroll position./querycount and decoded bytes for the app open and for each reconnect.memory. Leaving the pane (for Settings, say) drops an unfinished open, and coming back is a new open.__buzzClientMetrics.summary()returns the same data in the console.pnpm probe:live(dev/live-setup-probe.mjs) opens fresh authenticated sockets against a real relay. It compares the app's own subscriber at any concurrency (client:K, default 4) with up to 10 filters perREQ(filters:F) and multi-channel#h(multi-h:M). For each strategy it reports medians for time to full coverage, per-REQtime, and a canaryREQ's head-of-line delay compared withidle, plus refusals by reason. Coverage counts only when every channel reached EOSE, and only channel REQs move its clock. Runs that time out or leave any channel refused are counted asincomplete, and that strategy then gets no coverage median. A refused canary is reported as a refusal and suppresses the canary median. OutgoingCLOSEretires a tapped request, so a late EOSE can't revive a timed-out route.live.tsgets an optionalsetupConcurrencyso the probe can drive the real subscriber. The default stays 4. No relay changes, no change to default setup behavior, no FOUNDATION files.Early probe numbers
Test relay environment, 62 open channels, medians of 3 runs:
client:1client:4(today)rate-limited: quota exceededretriesclient:16client:allfilters:10multi-h:10multi-h:allA single
REQwhose one filter lists all 62 channels in#his accepted today. These runs only check EOSE on open channels. They don't check live event delivery, or private and member-only channels.Testing
Tested locally by baxen in the desktop dev app against the test relay.
vitest run: 392 files, 4400 tests pass.tsc --noEmitandbiome checkare clean.Browser:
settings-developer.spec.mjspasses in Chromium and WebKit. It's the one new browser case, and it proves that App routing, the real click path and the paint hook record an open by source, split wait from render, and export it. Unit tests can't cover that wiring.Review fixes (877a5df) each have a regression test: leave → return and StrictMode remount, no-disk reconnect attributed to
memorythrough the real session, two retained sessions, partial and all-channel refusal, refused canary, delayed global EOSE, and late EOSE after CLOSE. The probe tests fail on the previous head.Unit coverage for each fixed measurement bug: coverage not overwritten by resubscribes or joins, retry-after-error starting a new phase, hidden-page lag, empty and scrolled-away opens, the probe ending when the subscriber gives up locally, and timed-out runs.
Deferred: live-event delivery check for multi-
#h, and a probe run on a member roster with private channels.🤖 Generated with Claude Code