Skip to content

fix(desktop): refresh channels after access-revoked closure - #7784

Open
wesbillman wants to merge 4 commits into
mainfrom
pinky/archive-closure-refresh
Open

wesbillman wants to merge 4 commits into
mainfrom
pinky/archive-closure-refresh

Conversation

@wesbillman

@wesbillman wesbillman commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Pinky is updating this PR on Wes’s behalf.

Summary

  • Treat exact CLOSED restricted: channel access revoked on a known channel-scoped live subscription as a hint to refresh authoritative channel state.
  • Reuse useLiveChannelUpdates’ existing debounced, idle-aware query refresh. Keep terminal retirement, explicit membership denial, and polling unchanged.
  • Cover archived, unchanged, removed, and failed authoritative results; burst coalescing; irrelevant/stale frames; exact-reason rejection; and unmount cleanup with seven production-path regression cases.

The hint does not establish archive or membership state. This closes a missed-update recovery gap without changing the relay or blindly retrying subscriptions.

Related symptom report: #7026 (not claimed fully resolved; archived-channel delete policy is separate).
Related new-client fix: block/buzz-app#106
Originating conversation: buzz://message?channel=cb38cd5a-c920-40a1-b770-e9b89c289476&id=d22c1eed473016cf1f73c889a4cc88f1491ee092cfe3794575f8fea8003aa19e

Review follow-up and main integration

At 51ec7482ad1e600bd02ea6b9ed265db8a70e7266, based on main c213d90d5c450e2e579505da4a4145f528ecbcf2:

  • Addressed Jude’s P2: a known channel-scoped live subscription closed with restricted: not a channel member stays terminal but must not refresh channels.
  • Mutation verification: replacing the exact reason predicate with startsWith("restricted:") produced 6,588 passes / 1 expected failure, specifically the new no-refresh assertion (actual: 1, expected: 0). Restored the exact predicate; the final full suite passed 6,589/6,589.
  • Resolved both conflicts with fix(desktop): bound startup request bursts and recover quota refusals #7790, retaining main’s paced/cancellable subscriptions and this PR’s hint after terminal cleanup/drain cancellation. The fixture advances the production drain with a consistent virtual clock; it does not stub or bypass admission.
  • Integration crossed the session file’s 1,200-line ceiling (1,207). Extracted only this PR’s exact predicate/listener set into relayChannelAccessRevocations.ts, with unchanged session ownership and cleanup. No limit change, new timer, retry policy, or unrelated refactor. Final feature diff: 46 production additions across three files, plus the 333-line regression fixture.
  • Independent read-only review of the integration and final extraction found no blocking findings. This is not GitHub approval.

Validation

At exact pushed head 51ec7482ad1e600bd02ea6b9ed265db8a70e7266:

  • Mandatory hooks passed without bypasses: desktop lint/policy checks, TypeScript, repository file-size gate, branch-skew check, and full desktop suite 6,589/6,589, zero failed/cancelled/skipped.
  • Working tree clean after push. DCO passed on GitHub.
  • Fresh hosted CI and Desktop Release Candidate checks are running; prior-head green checks are not evidence for this head. Re-review requested from jedwards27; the changes-requested review and required approval remain for the reviewer to update.

Evidence boundaries

  • Native IPC is stubbed; the fixture exercises actual subscription creation, raw CLOSED dispatch, mounted hook, authoritative query refresh, and IPC result conversion. It does not exercise native discovery or rendered sidebar behavior. Existing in-flight refresh retention remains source/helper-test coverage rather than a new integrated case.
  • No native app launch, live archive mutation, or release certification. This fixes a demonstrated recovery mechanism, not proof of the exact original incident ordering.
  • The initial broad local just ci attempt hit a 300-second timeout compiling buzz-voice; full local just ci remains incomplete. Current delivery uses mandatory path-scoped hooks and fresh hosted CI; no unrelated native/mobile lane was bypassed or claimed rerun locally.

Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
@github-actions

github-actions Bot commented Sep 21, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is c213d90d5c450e2e579505da4a4145f528ecbcf2...51ec7482ad1e600bd02ea6b9ed265db8a70e7266.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 51ec7482ad1e600bd02ea6b9ed265db8a70e7266 to authorize a new review.
Any previous review applies only to its recorded range.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Verdict: no blocking code findings

Reviewed head 806c1e4e2969fd60ab167fe0b6522c5ba952556a against base a61239f0d8036aff58176f5c0ce7f080c66e21b7. This is a comment review, not approval or merge certification.

The 27-line production change meets the stated recovery contract: only the exact access-revoked CLOSED reason on a known channel-scoped live subscription emits a refresh hint; existing terminal retirement happens first. It does not infer membership/archive state or restart the retired subscription. The hook reuses the existing debounce/idle-aware refresh and cleans up on unmount/session reset.

I traced the authoritative path through get_channels, native archive conversion and the stable list hash (which includes archive/membership), IPC conversion, query settlement, and sidebar/active-channel consumers. Successful updated state reaches existing sidebar filtering and stream composer read-only controls; a failed fetch retains the previous list rather than inventing an empty result. Reconnect replay checks the live map and subscription identity, so retired entries are not restored.

Validation and remaining gates

  • Source review only, including the new six-case production-path fixture and complementary independent review lanes. No reviewer test execution or live archive mutation. The PR reports exact-head hooks and 6,514/6,514 desktop tests; I did not reproduce that run.
  • The fixture binds real subscription creation, CLOSED dispatch, mounted hook, query refresh and IPC conversion, with native IPC stubbed. Native discovery, rendered sidebar and live incident ordering are not exercised. The existing busy→idle re-arm behavior is source/helper-test coverage, not a new integrated regression. These are disclosed evidence limits, not demonstrated code defects.
  • The PR remains draft. Hosted desktop checks were still running at the review snapshot, full local just ci is explicitly incomplete, and the security bot still requests review of this range. This review does not waive those gates. No code changes requested; complete outstanding validation before treating this as merge-ready. The broader symptom report #7026 and archived-channel delete policy remain outside this PR’s demonstrated result.

@wesbillman
wesbillman marked this pull request as ready for review September 21, 2026 17:06
@wesbillman
wesbillman requested a review from a team as a code owner September 21, 2026 17:06

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES

Reviewed: a61239f0d8036aff58176f5c0ce7f080c66e21b7..806c1e4e2969fd60ab167fe0b6522c5ba952556a (exact head 806c1e4e2969fd60ab167fe0b6522c5ba952556a)

Risk: high — this changes relay subscription lifecycle behavior and the renderer's authoritative channel-refresh path after an authorization-related terminal frame.

Finding

P2 — the regression suite does not protect the PR's claimed exact CLOSED-reason boundary. Production correctly gates the hint on exact equality with restricted: channel access revoked (desktop/src/shared/api/relayClientSession.ts:801-806), but the negative case only sends an unrelated invalid: bad filter reason (desktop/src/features/channels/channelAccessRevocation.test.mjs:257-272). It never sends a near-miss restricted: reason on a known channel-scoped live subscription.

I broadened the production predicate to accept every string beginning with restricted:. All six new tests still passed (6/6, exit 0). That mutant would refresh the authoritative channel list for ordinary restricted closures, contrary to the PR's explicit exact-match contract. The repository requires regression tests to bind and falsify their production guard (TESTING.md:25-32; AGENTS.md:192-196).

Author action: add a known, channel-scoped live-subscription negative case using a different restricted reason, such as restricted: not a channel member, and assert that no channel refresh occurs. Verify that broadening the predicate to startsWith("restricted:") makes the test fail, then restore the exact predicate and prove it passes.

Verification owner: author for the regression addition; reviewer to mutation-check the final exact head.

Integrated contract trace

The production path itself appears sound:

  • The hint is accepted only for the current connection generation, a known live subscription with a non-empty #h, and the exact access-revoked reason. Unknown, retired, global, stale-generation, and unrelated frames are ignored.
  • Existing CLOSED handling remains terminal: the subscription is removed before listeners run and cannot be replayed on reconnect.
  • CLOSED remains only a hint. Archive, removal, and membership state still come from the authoritative get_channels query; sidebar/composer behavior derives from that result.
  • Burst hints use the existing trailing, idle-aware refresh. Failed refreshes retain cached data and do not create a new retry loop. Unmount and community disconnect remove listeners and queued work.
  • Archived, removed, unchanged, and failed authoritative outcomes are exercised through real subscription creation/CLOSED dispatch and the query/cache path. No new visual or accessibility control is introduced.

Validation

At exact clean head 806c1e4e2969fd60ab167fe0b6522c5ba952556a:

  • Full Desktop package suite: 6,514/6,514 passed, 0 failed.
  • Focused production-path fixture: 6/6 passed.
  • Causal removal of hook registration and suppression of hint emission each failed the positive outcome rows as expected.
  • Exact-boundary broadening mutant (startsWith("restricted:")): 6/6 still passed — reproduces the finding.
  • pnpm typecheck, pnpm check, and git diff --check: passed/clean.
  • Hosted Desktop core, four smoke shards, Windows/macOS builds, integration shards, DCO, Semgrep, and zizmor: green at the reviewed head.

Manual/native evidence: none. The fixture stubs native IPC while exercising the renderer subscription/query seam; no live-relay archive/removal journey or rendered-pixel capture was performed.

Residual risk: the exact incident ordering and busy-to-idle behavior were source/helper traced rather than reproduced through a live native journey. These are confidence gaps, not additional author defects.

Pinky added 3 commits September 22, 2026 12:32
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
@wesbillman

Copy link
Copy Markdown
Collaborator Author

Pinky is commenting on Wes’s behalf.

@jedwards27 Addressed the exact-reason regression finding in 51ec7482ad1e600bd02ea6b9ed265db8a70e7266; please re-review/mutation-check this head.

  • Added a known channel-scoped live-subscription case for restricted: not a channel member, asserting terminal removal and no authoritative channel refresh.
  • Broadened the final extracted production predicate to startsWith("restricted:"): full desktop suite 6,588 pass / 1 fail, with only the new no-refresh assertion failing (1 !== 0). Restored exact equality: final mandatory-hook full desktop suite 6,589/6,589 pass. The predicate now lives in desktop/src/shared/api/relayChannelAccessRevocations.ts:23; the test still reaches it through the real dispatcher, not a helper-only invocation.
  • Resolved both conflicts against main c213d90d5c450e2e579505da4a4145f528ecbcf2, preserving fix(desktop): bound startup request bursts and recover quota refusals #7790’s paced/cancellable lifecycle. Updated the fixture clock for real admission pacing. The integrated session crossed its size ceiling, so only this PR’s predicate/listener set was extracted to a 28-line sibling module; no limit override or unrelated refactor.
  • Independent read-only integration/extraction review found no blockers. All required local hooks passed; fresh hosted CI is pending. No native app/live archive test or approval/merge performed.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — APPROVE at exact head 51ec7482ad1e600bd02ea6b9ed265db8a70e7266 against base c213d90d5c450e2e579505da4a4145f528ecbcf2.

No author-actionable defects found. The prior blocker is fixed: the new production-path negative control uses the near-miss reason restricted: not a channel member, remains terminal, advances the real debounce, and proves zero authoritative reads. Broadening the production predicate from byte-exact equality to startsWith("restricted:") makes exactly that row fail (6/7), while restoring the predicate returns 7/7.

The merged/refactored lifecycle also preserves the intended contract:

  • relayClientSession.ts:865-938 generation-fences inbound frames, captures the owned subscription, performs terminal cleanup, and only then classifies the CLOSED frame through the extracted helper.
  • relayChannelAccessRevocations.ts:18-26 emits a hint only for a live channel-scoped #h subscription with the exact restricted: channel access revoked reason.
  • useLiveChannelUpdates.ts:163-178,365-381,486-495 owns one trailing, idle-aware authoritative invalidation listener and cancels the timer/listener on unmount. The CLOSED reason remains only a hint; authoritative channel state determines archived, removed, unchanged, and failed outcomes.
  • Terminal cleanup deletes the subscription before reconnect replay (relayClosedRecovery.ts:135-143; relayClientSession.ts:1042-1056), so reconnect cannot resurrect the revoked subscription. Disconnect/community switch clears listeners and subscriptions before the keyed subtree remounts.
  • The refactor satisfies package policy: relayClientSession.ts is 1,197 lines under the 1,200-line frontend ceiling; the isolated helper is 28 lines and the differential size gate passes.

Exact-head validation from clean detached checkouts:

  • Full Desktop package suite: 6,589/6,589 passed.
  • Focused production fixture: 7/7 passed.
  • Exact-reason broad-predicate mutant: expected failure, 6/7.
  • Removing the production notification seam: expected failure in all four authoritative outcome rows; restoration returned 7/7.
  • pnpm --dir desktop typecheck: passed.
  • pnpm --dir desktop build: passed (existing chunk/dynamic-import warnings only).
  • pnpm check, differential file-size gate, and git diff --check: passed; policy audit found no added production unwrap/expect, exported APIs are documented, and commits are signed off.
  • Live preflight: head/base remain pinned, exactly four Desktop files (+379/-0), PR mergeable. Desktop integration, Windows/macOS builds, DCO, Semgrep, zizmor, and release-candidate checks are green. Desktop Core and four Smoke shards remain pending; CI owns those gates.

Author action: none.

Residual risk / verification owner: CI must complete the pending exact-head Desktop Core and Smoke jobs. No live-relay/native rendered journey was run; this change is transport/query-state recovery with strong production-seam coverage, so that is a reviewer/tooling confidence gap rather than author rework.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: APPROVE
Reviewed: c213d90d5c450e2e579505da4a4145f528ecbcf2..51ec7482ad1e600bd02ea6b9ed265db8a70e7266 (exact head 51ec7482ad1e600bd02ea6b9ed265db8a70e7266)
Risk: high — subscription lifecycle and authoritative channel-state recovery affect access-revocation correctness and sidebar state.

Behavior/contracts traced: current-generation CLOSED dispatch; terminal subscription removal and drain cancellation; exact reason/channel-scope classification; reconnect and community-switch cleanup; idle-aware authoritative query invalidation; archive/remove/unchanged/failure outcomes; unmount cleanup; file-size policy.

Findings: no unresolved author-actionable defect. The prior exact-reason test blocker is closed. channelAccessRevocation.test.mjs:291-305 now drives a known channel-scoped live subscription through restricted: not a channel member and asserts no authoritative read. relayChannelAccessRevocations.ts:18-26 preserves exact equality with restricted: channel access revoked; the hint is emitted only after existing terminal handling and never becomes authoritative state. Authoritative archive/removal/membership still comes from the channels query.

Author action: none.

Verification owner: exact-head CI/merge protection for remaining aggregate gates.

Validation at matching head:

  • Full Desktop package suite: 6,589/6,589 PASS in both lanes.
  • Focused production-path fixture: 7/7 PASS.
  • Broadening equality to startsWith("restricted:"): expected FAIL 6/7, solely on the new near-miss row (actual 1 !== expected 0).
  • Removing hint emission: expected failures in all four authoritative outcome rows; restoring exact head returned 7/7 PASS.
  • pnpm --dir desktop typecheck: PASS.
  • pnpm --dir desktop build: PASS (existing warnings only).
  • pnpm check, differential file-size gate, and git diff --check: PASS. relayClientSession.ts is 1,197 lines under the 1,200-line ceiling; the extracted helper is 28 lines.
  • Live head refreshed immediately before submission and remained 51ec7482ad1e600bd02ea6b9ed265db8a70e7266; GitHub checks were 20 passing, 0 failing, 5 pending.

Manual/native evidence: no native app or live-relay archive/removal journey. The production-seam fixture binds actual subscription creation, raw CLOSED dispatch, listener/debounce, query settlement, and conversion; it does not witness native IPC or rendered sidebar pixels.

Residual risk: exact incident ordering and a native/live-relay journey remain unwitnessed. Those are reviewer/tooling or follow-up verification gaps, not author rework for this bounded recovery mechanism.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants