refactor(remote-bridge): incremental channel add/remove (stop restarting every channel) - #1454
Conversation
…ing every channel) Connecting or disconnecting one remote channel no longer tears down the shared PawWork event stream and every other channel. The whole-bridge restart was the root cause of the UI flap that #1404 papered over with suppression; this makes per-channel lifecycle a first-class operation and removes the suppression. - supervisor.ts: PlatformSupervisor registry replaces the one-shot Promise.all. Each platform runs under its own child AbortController linked to the run signal; add()/remove()/stopAll() start, stop, and await one platform without touching the others. Each entry carries a generation token so a retired/replaced loop's late status can't clobber a newer entry under the same name. The entry is registered before the loop starts so its first synchronous "starting" passes the token guard. A thin supervisePlatforms() wrapper is kept for fixed-set callers. - gateway.ts: App is now a platform registry. It holds the injected factory and a desiredPlatforms map (source of truth). addPlatform(config) runs the same hasRemoteAudience gate as cold start, builds via the factory, registers with the Engine, and supervises it — so an incrementally-added channel can't bypass the audience check; a same-name re-pair retires the old loop but keeps its pointers. removePlatform(name) retires the loop and prunes that platform's session pointers. run() seeds the supervisor synchronously after hydrate and freezes the cold-start onReady set to that snapshot. The message handler drops an inbound from a platform that is no longer the live instance. - engine.ts: unregisterPlatform(name) drops the platform from the reconstruct index and discards active deliveries targeting it. - session-pointers.ts: clearPlatform(name) prunes one platform's mappings by remote-key prefix, keeping the event cursor and other platforms — so a disconnect→reconnect of the same platform can't resurrect a stale session. - desktop remote-bridge.ts: confirm/disconnect go incremental on a live bridge; full startBridge() is kept only for cold start, last-channel teardown, and recovery after a fatal stream. The live bridge is one {app, ac, runPromise} handle cleared whenever run() settles, so a post-fatal confirm rebuilds instead of adding onto a dead app. Both flap-suppressions (the startBridge pre-mark skip and the onStatus connected->connecting guard) are deleted. Tests assert "nothing restarted" via call counts, not UI suppression: the shared stream is stood up once across a single-channel add/remove, only the removed platform is stopped, and a fatal stream rebuilds. supervisor 174 / desktop 375 unit tests pass; both typecheck clean; eslint clean. Closes #1414. Claude-Session: https://claude.ai/code/session_01WVUVErxQ1AEf2mYT2bJ5hT
📝 WalkthroughWalkthroughImplements Wave 2 incremental per-channel add/remove: ChangesIncremental per-channel add/remove on live bridge
Sequence Diagram(s)sequenceDiagram
participant UI
participant Runtime as RemoteBridgeRuntime
participant BridgeApp
participant PlatformSupervisor
participant Engine
rect rgba(100, 149, 237, 0.5)
note over Runtime,Engine: Cold start — full rebuild
UI->>Runtime: confirmPairing(first channel)
Runtime->>Runtime: startBridge → mark all connecting
Runtime->>BridgeApp: app.run()
BridgeApp->>PlatformSupervisor: add(platform1)
BridgeApp->>Engine: registerPlatform(platform1)
end
rect rgba(144, 238, 144, 0.5)
note over Runtime,Engine: Incremental add on live bridge
UI->>Runtime: confirmPairing(second channel)
Runtime->>BridgeApp: addPlatform(config, beforeCommit)
BridgeApp->>BridgeApp: beforeCommit() saves credentials
BridgeApp->>Engine: registerPlatform(platform2)
BridgeApp->>PlatformSupervisor: add(platform2) — platform1 unaffected
end
rect rgba(255, 165, 0, 0.5)
note over Runtime,Engine: Incremental remove on live bridge
UI->>Runtime: disconnect(platform2)
Runtime->>BridgeApp: removePlatform(platform2)
BridgeApp->>PlatformSupervisor: remove(platform2)
BridgeApp->>Engine: unregisterPlatform(platform2)
Note over BridgeApp: platform1 keeps serving, stream stays up
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Astro-Han
added
the
task
github-actions
Bot
added
platform
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/desktop-electron/src/main/remote-bridge.test.ts, packages/desktop-electron/src/main/remote-bridge.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
Two correctness gaps surfaced by the PR #1454 review: - addPlatform built the replacement before retiring a live same-name instance, so a factory failure left the old loop serving while the caller had already switched the saved account/UI to the new identity. Retire first, then build — matching the method's own doc. - PlatformSupervisor.remove awaited a platform's stop()/wind-down with no bound, so a wedged stop() could hang a disconnect or re-pair and wedge the desktop's serial lifecycle queue (the channel never leaving the screen). Bound the wait with removeTimeoutMs; the entry is already dropped and the loop aborted before the wait, so timing out just detaches the stop(). Claude-Session: https://claude.ai/code/session_01WVUVErxQ1AEf2mYT2bJ5hT
Review follow-ups on PR #1454: - addPlatform ran the audience gate before retiring a live same-name instance, so a same-name re-pair with an invalid audience threw at the gate and left the old loop serving under the already-committed new identity. Retire first — before both the gate and the factory build — so any failed re-pair drops the old instance. A new name that fails the gate still leaves existing channels untouched. - Tighten the bounded-remove test to assert < 250ms (was < 2000ms) so it proves remove() honors removeTimeoutMs rather than passing under a hypothetical hardcoded second-scale wait. Claude-Session: https://claude.ai/code/session_01WVUVErxQ1AEf2mYT2bJ5hT
PR #1454 review. Supersedes the retire-first approach from the two earlier review rounds, which destroyed a working connection on a failed re-pair: - A live re-pair now builds and connects the new channel BEFORE anything else; only on success does it retire the old loop and persist the new credential. gateway addPlatform reverts to prepare-first (build, then retire-on-success). desktop confirmPairing swaps the in-memory account, awaits addPlatform, saves the credential only on success, and on failure rolls the swap back and rethrows — leaving the working channel connected and its stored credential untouched. The earlier "old instance live under the new label" concern is resolved at the root by not committing the new account before the new channel is up, rather than by tearing the old down. - Test harness: appWith's addPlatform/removePlatform now throw by default, so a test that unexpectedly drives an incremental path fails loudly instead of passing through a silent no-op (incremental tests override). Claude-Session: https://claude.ai/code/session_01WVUVErxQ1AEf2mYT2bJ5hT
A live re-pair / second-channel add swapped the gateway's live platform set inside app.addPlatform and only then saved the credential, so a save failure (locked keyring, unwritable file) left a live new channel backed by a stale stored credential while the in-memory account list rolled back — a half-commit. Add an optional beforeCommit hook to App.addPlatform that runs after the build but before retire/register/supervise; the desktop saves the credential there. A throwing hook aborts the swap with the old channel still serving and nothing persisted. Cover both the re-pair and second-channel save-failure paths. Claude-Session: https://claude.ai/code/session_01WVUVErxQ1AEf2mYT2bJ5hT
…onnect disconnect() removed the platform from this.accounts and only then saved the trimmed list and called removePlatform, with no try/catch — so a save failure (locked keyring, unwritable file) left the channel gone from memory while its loop kept running live and its status froze. Apply the same prepare-first order as add/re-pair: compute nextAccounts, persist first, and only on success swap this.accounts and remove the live channel; a save failure leaves the channel connected and the disconnect retryable. The last-channel teardown is unchanged (it revokes via clear(), which needs no encryption). Claude-Session: https://claude.ai/code/session_01WVUVErxQ1AEf2mYT2bJ5hT
…ess replace) A re-pair retires the old loop and supervises the new platform once it is built and persisted, not once it is serving. Add a gateway test that re-pairs a name with a replacement whose start() rejects, asserting the old channel is retired, the new platform is the live one, and it surfaces degraded (the supervisor retries it) rather than the old channel being kept. FakePlatform gains a startErr option to model a connect-then-fail platform. Claude-Session: https://claude.ai/code/session_01WVUVErxQ1AEf2mYT2bJ5hT
|
@coderabbitai review |
✅ Action performedReview finished.
|
…e tears down A fatal shared-stream error during an in-flight confirmPairing/disconnect could land in the window where App.run() has cleared the supervisor but the desktop runtime's handle is not yet cleared. addPlatform/removePlatform then resumed and silently no-op'd the live supervise (supervisor?.add on a null supervisor), returning success — so the runtime reported a paired/disconnected channel that never started, or whose survivors were never rebuilt. Give the App an explicit tearingDown flag, set in the same synchronous step as supervisor=null, and have addPlatform/removePlatform throw BridgeClosedError at their commit point when it is set (a null supervisor during startup stays valid: the initial snapshot adopts the platform). The runtime now caches the whole handle and, on BridgeClosedError after the credential is committed, rebuilds the bridge from the persisted accounts; a pre-commit failure still rolls back memory. Tests: a gateway race where the build finishes after teardown now throws instead of silently succeeding; desktop re-pair and disconnect interrupted by a fatal stream recover by rebuilding from the persisted accounts. Claude-Session: https://claude.ai/code/session_01WVUVErxQ1AEf2mYT2bJ5hT
…hannel ops Two P2 races surfaced by review on the incremental add/remove path: 1. A user quit lands stop()'s synchronous handle abort while a re-pair or disconnect is parked in addPlatform/removePlatform. The abort makes the op throw BridgeClosedError, and both desktop catch blocks rebuilt the bridge unconditionally — standing up a fresh event stream during shutdown only to tear it down again. Skip the rebuild when the handle was aborted (the credential / trimmed accounts are already persisted; the next launch builds from them); a fatal stream, which does not abort the handle, still rebuilds. 2. removePlatform threw a pointer-store write failure past its live retire. The retire is the disconnect's commit (the channel has stopped serving), so forgetting its session pointers is best-effort cleanup: log a failed write instead of throwing, which otherwise surfaced as a failed disconnect and stranded the UI showing an already-stopped channel. A stale on-disk pointer self-heals on the next pointer save. Tests cover each seam and were verified to fail when the guard is removed. Claude-Session: https://claude.ai/code/session_01Y3Z6Hbny6bzGoZJg8Xrjjr
|
Addressed both P2 items in P2 (stop during in-flight add/remove rebuilds the bridge mid-shutdown). Confirmed: P2 (non-last disconnect leaves a half-state when pointer cleanup fails). Confirmed: Note on a residual: a fatal stream that races a stop within the in-flight window can still rebuild once during shutdown (the fatal path nulls |
…comments Review follow-up (P3 + verification): - Add a file-backed SessionPointers test for the write-failure restart chain: a clearPlatform whose atomic write fails leaves the in-memory map pruned but the on-disk snapshot intact, so a restart (and a reconnect of the same platform, since addPlatform keeps pointers) revives the stale mapping. This pins the accepted best-effort behavior as deliberate rather than a latent bug. The harm is bounded (a reconnected channel's first message may hit an old session; the engine surfaces an error and the user resends) and the trigger is a rare IO-failure conjunction, so a self-healing tombstone is left as follow-up, not blocking the incremental add/remove path. The forced-failure step skips under root, where a read-only dir cannot block writes. - Compress the commit/rebuild invariant comments in addPlatform, removePlatform, confirmPairing, and disconnect to one-line statements now that tests cover the races; correct removePlatform's comment, which overclaimed the stale pointer was "never resurfaced". No logic change. Claude-Session: https://claude.ai/code/session_01Y3Z6Hbny6bzGoZJg8Xrjjr
|
Addressed both items in 待验证 — does a failed pointer write revive a stale pointer on restart? Verified: yes. Added a file-backed Decision: accept best-effort for this PR. The harm is bounded (a reconnected channel's first message may route to an old session; if it's gone the engine surfaces an error and the user resends, starting fresh — no data loss, no security impact) and the trigger is a rare conjunction (clear write fails ∧ no later save heals the file ∧ immediate exit ∧ later reconnect of the same platform). The clean durable fix can't be "a fresh connect clears pointers" — cold start is also a non-re-pair P3 — trimmed the invariant comments. Compressed the multi-paragraph commit/rebuild explanations in remote-bridge 181 tests / desktop-electron 586 / both typecheck / eslint in-scope all green. |
… root The stale-pointer revival test forces a clearPlatform write failure with a POSIX chmod 0o500 on the directory. That cannot make a directory unwritable on Windows (chmod has no effect there) nor under root (which bypasses permission bits), so on those platforms clearPlatform would not reject and the assertion would fail or pass implicitly. Move the guard into the test declaration as test.skipIf(win32 || getuid()===0), matching the existing owner-only-permissions test, so both cases render as an explicit skip instead of an in-body early return. Claude-Session: https://claude.ai/code/session_01Y3Z6Hbny6bzGoZJg8Xrjjr
|
Fixed in P1 confirmed: the revival test forced the write failure with POSIX remote-bridge 181 tests (1 skipped on Windows/root) / both typecheck / prettier clean. |
Release bump for the 2026.6.11 production hotfix build.\n\nThis updates the desktop package version and matching lockfile entry from 2026.6.10 to 2026.6.11 so the release workflow can publish a new build containing #1461, which fixes the packaged Settings IPC regression reported in #1460. The release also includes #1454, which landed after v2026.6.10.\n\nVerification before merge:\n- Local: bun install --frozen-lockfile, bun install --lockfile-only, package version readback, git diff --check, and packages/desktop-electron typecheck.\n- PR #1462: CodeQL, dependency-review, dev-dep-audit, desktop-smoke, e2e-artifacts, ci, windows-advisory, pr-triage, and title lint all passed.\n- Pre-bump dev validation for #1461 merge commit c509faf: ci, desktop-smoke, CodeQL, and windows-advisory passed; focused WebSearchAuth AppRuntime status smoke passed locally.\n\nRelated: #1460, #1461, #1454.
1 participant
Summary
Connecting or disconnecting one remote channel no longer restarts the shared PawWork event stream or the other channels — only the affected channel starts or stops. Per-channel lifecycle becomes a first-class operation across three layers, and the UI flap-suppression that #1404 added as an interim is removed now that unaffected channels genuinely stay up.
remote-bridge/supervisor.ts— aPlatformSupervisorregistry replaces the one-shotPromise.all. Each platform runs under its own childAbortControllerlinked to the run signal;add()/remove()/stopAll()start, stop, and await a single platform without touching the others. Each entry carries a generation token so a retired or replaced loop's late status can't clobber a newer entry under the same name. A thinsupervisePlatforms()wrapper is kept for fixed-set callers.remote-bridge/gateway.ts—Appis now a platform registry holding the injected factory and adesiredPlatformssource-of-truth map.addPlatform(config)runs the samehasRemoteAudiencegate as cold start (so an incrementally-added channel can't bypass it), builds via the factory, registers with the Engine, and supervises it; a same-name re-pair retires the old loop but keeps its pointers.removePlatform(name)retires the loop and prunes that platform's session pointers.run()seeds the supervisor synchronously after hydrate and freezes the cold-startonReadyset to that snapshot. The message handler drops an inbound from a platform that is no longer the live instance.remote-bridge/engine.ts—unregisterPlatform(name)drops the platform from the reconstruct index and discards active deliveries targeting it.remote-bridge/session-pointers.ts—clearPlatform(name)prunes one platform's mappings by remote-key prefix, keeping the event cursor and other platforms. On a successful write this lets a disconnect→reconnect of the same platform start fresh. The cleanup is best-effort: clearPlatform prunes in memory and then writes atomically, so a write failure leaves the on-disk snapshot intact and a stale pointer can revive after an immediate restart — an accepted behavior pinned by asession-pointerstest, with a self-healing tombstone/retry left as follow-up.desktop-electron/remote-bridge.ts—confirmPairing/disconnectgo incremental on a live bridge; a fullstartBridge()is kept only for cold start, last-channel teardown, and recovery after a fatal stream. The live bridge is one{ app, ac, runPromise }handle cleared wheneverrun()settles, so a post-fatalconfirmPairingrebuilds instead of adding onto a dead app. Both flap-suppressions (thestartBridgepre-mark skip and theonStatusconnected→connecting guard) are deleted.Why
The whole-bridge restart on every connect/disconnect was the root cause of the UI flap that #1404 suppressed at the UI layer. Suppression only hid the blink; it could not fix the real cost — restarting the shared event stream and re-draining every channel whenever one is added or removed (a window in which messages can be missed). This is the root fix #1404's review deferred to Wave 2: don't restart the unaffected channels, and the suppression becomes unnecessary.
A second-AI design pass (first-principles review) surfaced three correctness points folded in here: the audience gate must run on incremental add (kept in the gateway, not bypassable); a removed/replaced platform's in-flight inbound must be dropped (gateway message-handler liveness guard) and its routing/active delivery dropped (
unregisterPlatform); and a single-platform disconnect prunes that platform's session pointers so a same-platform reconnect normally starts fresh (best-effort: a failed pointer write can revive a stale pointer after a restart, pinned by tests).Related Issue
Closes #1414. Part of #1188. Follow-up sibling: #1426 (WeChat session auto-relogin) is intentionally out of scope here.
Human Review Status
Pending
Review Focus
PlatformSupervisorlifecycle correctness (supervisor.ts): the entry is registered before the loop starts (so the first synchronousstartingpasses the token guard) anddoneis filled on the same tick (soremove/stopAllalways await the real loop); the generation token drops a retired loop's late callbacks.run()settle (desktop remote-bridge.ts): this is what makes "is a bridge live?" reliable, so a fatal stream rebuilds rather than adds onto a dead app.App.addPlatform(gateway.ts): an incrementally-added channel goes through the samehasRemoteAudiencecheck as cold start.addPlatformvsremovePlatformpointer semantics: re-pair (addPlatform replacing a live same-name channel) keeps session pointers; disconnect (removePlatform) prunes them.Risk Notes
removePlatformnow prunes a single platform's session pointers viaclearPlatform(orphaned parent links are left as-is; no remaining remote key references them). The last-channel teardown (stop bridge →credentials.clear()→rmSync(statePath)) is unchanged.bun run dev:desktopwalk — with Telegram connected, connect/disconnect WeChat and confirm Telegram keeps serving (its poll is never aborted) — is not yet run, as it requires a live WeChat pairing, same constraint as feat(remote): WeChat iLink adapter with one-step QR pairing (#1188) #1404. The behavior is covered by unit tests asserting the shared stream is never rebuilt and the survivor is never stopped.How To Verify
Screenshots or Recordings
No visible UI change (main-process / gateway logic only).
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.https://claude.ai/code/session_01WVUVErxQ1AEf2mYT2bJ5hT
Summary by CodeRabbit
Release Notes