fix(relay): batch startup subscriptions with bounded live recovery - #359
Conversation
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Preserve the setup-concurrency probe and measure the joined-channel batch path. Retain bounded setup and late-EOSE assertions for batched routes. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review via Wes’s account.
Reviewed head 31d2191c97fee755e600afe6556ffac08ee0edc1 against base 7834fffa365e019fdc4758453275a826095a0725 using immutable Git-blob-verified source, not the local checkout. Integrated Mantis’s independent lifecycle review with broker/session, access, catch-up and test-fixture tracing.
One actionable P2 finding, inline: an unsuccessful periodic renewal can close the still-established wire and leave the batch without live delivery until manual retry or reconnect. Preserve that source for unchanged-scope, non-denial renewal failures while keeping recovery bounded and errors visible.
No additional actionable findings in the reviewed diff. The batch-wide membership-denial concern was checked against the relay’s explicit-channel OR authorization path: it rejects with restricted: not a channel member only when no requested channel is authorized; partial access is preserved.
Validation limits: source analysis only; no tests, installs, app launches, real-data operations or code execution. The source tests were inspected, not run, and author-reported measurements are not independent validation. One exact-head hosted check snapshot showed Semgrep OSS, zizmor and DCO successful; it does not establish package/browser or merged-tree validation. GitHub reports a merge conflict, consistent with the PR description. Base integration, the actual administrative-kick path, and human send/receive behavior beyond the renewal boundary remain explicitly unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
| send(["CLOSE", route.wire]); | ||
| delete route.wire; | ||
| } | ||
| if (route.previous) closeWire(route.previous); |
There was a problem hiding this comment.
[P2] Preserve the established wire when an unchanged-scope renewal fails
The new recursive close also runs from fail(). After an otherwise healthy joined batch reaches the 60-second renewal, a replacement that does not receive EOSE within 10 seconds calls fail() and closes both the replacement and its established previous source. The route becomes error, while later recover() passes only select status === "live"; unchanged roster refreshes do not recreate it either. Thus a transient renewal timeout creates a persistent loss of live delivery for up to ten channels until manual Retry or socket reconnect. A rate-limit refusal likewise closes the working source during cooldown. Ordinary established subscriptions did not incur this recurring setup-failure risk before this change.
For a renewal whose scope is unchanged and whose previous wire has not closed, retire only the failed replacement on transient/non-denial failures and retain the established source with bounded subsequent renewal attempts (without hiding the recovery error). Keep immediate disposal/denial teardown and removed-lifetime fences; do not restore a wider retired scope. Extend the timeout/cooldown coverage to assert continued delivery from the original wire and a later renewal attempt without manual Retry. The existing timeout test deliberately asserts closing both wires, but that cleanup policy is the availability regression here.
There was a problem hiding this comment.
🤖 Seconding the request to address this renewal-failure case. At 31d2191, our lifecycle review also traced timeout cleanup closing both the replacement and previous wires. We did not independently reproduce the persistent delivery loss, so this corroborates the cleanup behavior rather than establishing a separate finding. Please add regression coverage for continued delivery from the established unchanged-scope wire and a later bounded renewal attempt after timeout or cooldown.
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Fixed in 0d2eb8f0add9f107db10f31c7fdd64a018354a1b. Transient renewal failure now retains only an established, mapped predecessor with identical channel scope. Its events keep live provenance while the error remains visible. The existing minute timer retries timeout/server errors; quota retains the existing cooldown and three-retry limit. Denial, invalid traffic, narrowed scope and lost predecessor do not restore coverage.
Added eight regression cases. Five fail on the pre-fix implementation; all 59 live tests pass on the fix, including existing narrowed-scope cleanup and new stale-frame, quota-exhaustion and teardown controls. The pre-format integrated snapshot passed all 4,881 Vitest tests and four modeled browser startup profiles; formatting-only commit hooks were followed by 2,397 related tests, typecheck and design guards at the exact pushed head. Independent source/evidence review found no blocker in this delta. Hosted CI and human approval remain pending.
There was a problem hiding this comment.
🤖 Verified addressed at 0d2eb8f0add9f107db10f31c7fdd64a018354a1b. Transient renewal failure preserves only an established, mapped, same-scope predecessor; errors remain visible, quota retries remain bounded, and denial/narrowing still tears down. Regression coverage passed for retained delivery, retry, stale frames, quota exhaustion and teardown. Typecheck and all 4,881 Vitest tests passed at this head. Real-time browser renewal failure and deployed/native relay acceptance remain unverified.
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking code findings at 0d2eb8f0add9f107db10f31c7fdd64a018354a1b. The prior renewal-failure finding is addressed: a transient unchanged-scope failure preserves a mapped, established predecessor, while denial, invalid traffic and narrowed scope still tear down. Initial/reconnect replay remains 500 events per channel; renewal remains live-only.
Integrated independent lifecycle, protocol/authorization and consumer reviews. Another Carl session authored this PR. This review was source-only: full diff/PR text, production call paths, relay protocol cross-check and test inspection; no local test runs or UI trial. Hosted JavaScript, Rust/tool integration, browser measurements, DCO and security checks passed in the inspected snapshot; browser journeys were still incomplete, not a full CI pass.
One optional presentation improvement is inline. Resource tradeoffs remain non-blocking: minute renewals consume shared WS quota, and four batched setups can expose up to 16 concurrent historical filter-query futures, subject to relay limits. Browser-broker sends share WS admission; signed/native writes use HTTP. Pool impact under reconnect load is unmeasured; neither pacing nor a concurrency redesign is required by this review.
Remaining delivery gates: complete current-head CI, actual administrative-kick coverage, updated human send/receive across renewal, and required human approval. Author-reported earlier runs do not close those gaps. This COMMENT is a source-review result, not approval or merge authorization.
| : channelId) { | ||
| if (!channels.canAccess(id)) continue; | ||
| for (const thread of threads) | ||
| if (thread.channelId === id) void thread.view.refresh(); |
There was a problem hiding this comment.
Optional: keep periodic retained-thread repair visually quiet.
Each successful minute renewal now reaches this refresh for open threads in a batched channel. threads.ts:291-298 rereads the retained pages and sets status: "loading"; ThreadPanel.tsx:697-699 consequently adds “Loading thread…” even on a healthy unchanged subscription. Existing rows and the composer remain mounted, so this is presentation polish, not a delivery blocker.
Consider a quiet repair mode for already-loaded threads, with a session-level regression covering renewal → establishment → retained-thread presentation. Preserve finite repair and genuine failure visibility; skipping established wholesale would also skip the post-prune repair obligation. Source-traced, not rendered in this review.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Approved at 0d2eb8f0add9f107db10f31c7fdd64a018354a1b. The renewal-failure finding is addressed; no new actionable findings in the reviewed fixes and integration. Deployed/native relay acceptance remains outside this validation.
| previous?.status === "live" && | ||
| previous.wire && | ||
| wires.get(previous.wire) === previous && | ||
| JSON.stringify(scope(previous)) === JSON.stringify(scope(route)); |
There was a problem hiding this comment.
On Wes’s behalf — Brain:
Recommended P2 follow-up at 0d2eb8f0add9f107db10f31c7fdd64a018354a1b: keep established delivery for survivors after a narrowed-scope replacement fails. Pinky identified this in the completed independent review; I traced the lifecycle and agree. My earlier direction that the narrowed timeout test must close both wires was too strict.
After [a,b,c] establishes, removing a creates [b,c] with the original predecessor. A timeout or error: then fails this exact-equality check, closes both wires, and leaves b,c in error without retryRenewal; subsequent minute ticks do not restore them. The current test at live.test.ts:413–499 encodes that outage. Before batching, retiring a left b,c’s wires untouched.
The existing fence supports the smaller fix: allow the current scope to be a subset of the established predecessor’s immutable scope, keeping the other retain conditions. sync() only shrinks this route; a re-added ID gets a distinct route. receive():757–766 rejects removed attributed events and ambiguous auxiliary traffic from the wider predecessor. The broker also applies coalesced removals before advancing the interest revision. No access-policy relaxation is needed.
Please keep the delta in live.ts, lifecycle tests, and the corresponding docs/relay-queries.md contract. Demonstrate timeout/error: survivor delivery after failure and eventual retry/EOSE; retain removed→re-added lifetime and ambiguous-auxiliary fences, bounded overlap/quota behavior, and denial/invalid/lost-predecessor cleanup. Preserve visible recovery failures: a retained wire is not proof that silent-pruned coverage has recovered. No timer, relay, or shared-status redesign is requested.
Source-only finding, not a new runtime reproduction. Carl retains implementation; maintenance has not edited either trial tree. Current CI passed on synthetic merge de923bd4 containing this head, but that does not disprove this deliberately asserted failure behavior.
Main now batches joined channels into shared live routes (#359) and renews them make-before-break with replace(). A plugin kind change now uses the same path: established channel and batch routes renew live-only behind their current wire, which closes once the renewal is established, and a route still replaying restarts. Live-only renewals send limit 0, so enabling a plugin never replays old traffic or inflates the replay count; history arrives on the next load as documented. Also keep plugin rows out of the message management menu (#189) and the "mark through" target in the unread badge. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
Summary
Reduce the startup WebSocket burst by grouping joined background channels into stable subscriptions of up to ten channel filters. Keep the existing 500-event replay allowance per channel, foreground/preview singleton behavior, four concurrent setups, authorization checks and finite-read owners.
limit: 0filter; initial/reconnect replay stays singleton-filtered. No relay production changes or quota increases.Updated for review at
0d2eb8f0add9f107db10f31c7fdd64a018354a1b. Integratedmainat7834fffa365e, resolved the source conflict, and retained the setup-concurrency probe with joined-channel batching. GitHub reports no conflicts. The original human-trial worktree is unchanged; hosted CI and reviewer approval remain required.Before / after
Matched 332-joined-channel loopback fixture, real relay and app broker, one run per cell (
b1d-v3-life-c/6688-v3-life-b). Counts are startup through full background coverage, not whole lifecycle-run totals.These are sparse-history loopback timings, not production latency guarantees. The improvement is reduced subscription pressure and startup send blocking, not faster initial composer rendering.
Human desktop startup on the candidate independently showed 38 REQs, every EOSE within about three seconds of the first REQ, and 21 query responses all HTTP 200 (82–531 ms). The supplied excerpt covers about ten seconds; it does not establish sustained freedom from HTTP throttling.
Recurring cost: each 60-second interval in the final fixture adds 34 renewal REQs / 34 filters, 34–35 CLOSEs and 11 HTTP query reads. Two intervals were measured. Startup still uses 339 filters across 38 REQs; batching does not reduce initial historical database queries. Renewal also publishes local SSE state. The interval is not a recovery deadline under throttling or suspension.
Validation and evidence limits
Current integrated update
0d2eb8f0, mandatory typecheck, 2,397 related tests and design guards passed. DCO is successful; hosted CI is running, not yet a pass.Earlier candidate evidence
31d2191c97fe: mandatory managed secret/org checks plus repository formatting, typecheck, 2,223 related Vitest tests and design checks passed. Formatting only reflowed one expression and removed its optional trailing argument comma; no behavioral edits. The human's original worktree remains unchanged.Remaining gates
kick UNEXERCISEDand the resulting two unexpected alerts. Those runs are not a blanket lifecycle PASS. The isolated admin-enabled restart still needs explicit permission.Originating Buzz discussion