Skip to content

fix(desktop): stop blocking the renderer during topology polling - #19300

Open
daniyal-sh wants to merge 1 commit into
pingdotgg:mainfrom
daniyal-sh:fix/desktop-async-topology-sol61
Open

daniyal-sh wants to merge 1 commit into
pingdotgg:mainfrom
daniyal-sh:fix/desktop-async-topology-sol61

Conversation

@daniyal-sh

Copy link
Copy Markdown
Contributor

Problem

Periodic desktop topology reads call ipcRenderer.sendSync. A busy main process therefore blocks the renderer and keystroke painting even when the topology handler itself is fast. Maintainer-triaged #12689 documents a roughly one-second wait in the synchronous bootstrap read while the browser client remains smooth.

Change

Keep a cached preload snapshot for synchronous startup consumers and refresh it through asynchronous invoke during topology polling. Concurrent refreshes share one request. Failed refreshes preserve the last successful topology; authoritative empty results still remove stopped backends, and unchanged topology retains array identity. Cache the local-environment-enabled flag, which changes through relaunch.

The optional bridge method, main-process handler, preload, and web connection reader are needed for the same desktop IPC fix. Older bridges retain their fallback. The web/mobile connection protocols and visual layout are unchanged.

Scope and approval

Addresses #12689 in the direction requested by maintainer triage: async invoke plus a last-known snapshot. That triage establishes the bug and intended behavior; it is not a claim of merge approval. The earlier #12698 is closed and unmerged. This implementation retains synchronous startup compatibility and targets recurring reads.

Verification

  • Focused preload snapshot, desktop-local topology, platform connection, window IPC, and local-environment IPC tests were run. A held asynchronous refresh allows 1,000 synchronous cached reads with one startup sync read and one shared asynchronous request. Tests cover failed-refresh retention, recovery, successful empty topology, old-bridge fallback, and relaunch-only configuration.
  • A seeded isolated web client passed typing and scrolling checks in the T3 Browser panel; 23 individual keys measured 11.4 ms median and 14.7 ms p95 to the next animation-frame callback. No provider message was sent. Measurements and limitations.
  • Native Electron 44.4.5 / Chrome 152 on Windows x64: sandboxed preloads from current main and this branch, isolated hidden fixture renderers, thirty bootstrap/enabled read pairs with each main-process response deliberately delayed 30 ms. Three interleaved runs per version. Median read-loop time 2,342.2 → 0.4 ms; median longest five-millisecond heartbeat gap 2,343.3 → 6.2 ms. Calls per run, including two startup warmups: 62 sync/0 async → 2 sync/1 async. Native results, CPU profiles, and reproducible harness.
  • This controlled native IPC benchmark proves recurring reads do not wait for a pending reply. The imposed delay and tight loop are synthetic, so their ratio is not a desktop speedup claim. Natural-workload native typing/FPS and live WSL secondary backends were not measured. Browser timing does not exercise Electron IPC. The issue's attached renderer profile supplies the original real-world stall evidence. No visual appearance changes are proposed.

Post-rebase validation on this branch at 8b460a3c (Effect 4.0.4): 42 focused tests passed, one existing platform skip. vp run --filter @t3tools/desktop --filter @t3tools/web --filter @t3tools/contracts typecheck, changed-file lint/format, and patch whitespace checks passed. The native capture above bundles the actual before/after preloads from these rebased sources. Production server/web/desktop bundles and the sandboxed preload verifier also passed on the original benchmark checkout with these fixes, before upstream's dependency upgrade.

Implemented and validated using GPT-6.1 Sol in T3 Code through the Codex harness.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 14, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 1def7bb

Macroscope's review found this PR approvable — This is a focused desktop performance fix that moves recurring topology polling off synchronous renderer-blocking IPC while preserving startup behavior, failure handling, snapshot identity, and older-bridge compatibility. The implementation is localized, additive at the contract boundary, and covered by targeted tests.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 14, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 421cc5db-f925-4a4f-b477-e3ba02b8d146
📥 Commits

Reviewing files that changed from the base of the PR and between 8855a6d and 1def7bb.

📒 Files selected for processing: 10
  • apps/desktop/src/ipc/DesktopIpcHandlers.ts
  • apps/desktop/src/ipc/methods/window.ts
  • apps/desktop/src/preload.ts
  • apps/desktop/src/preloadSnapshot.test.ts
  • apps/desktop/src/preloadSnapshot.ts
  • apps/web/src/connection/desktopLocal.test.ts
  • apps/web/src/connection/desktopLocal.ts
  • apps/web/src/connection/platform.ts
  • docs/internals/performance-regressions.md
  • packages/contracts/src/ipc.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

The desktop now provides asynchronous local-environment bootstrap reads alongside the existing synchronous method. The preload and web readers use asynchronous refreshes while retaining cached topology for synchronous reads and older desktop bridges.

Changes: Local-environment topology refresh

Layer / File(s) Summary
Add asynchronous bootstrap IPC
packages/contracts/src/ipc.ts, apps/desktop/src/ipc/methods/window.ts, apps/desktop/src/ipc/DesktopIpcHandlers.ts
DesktopBridge adds an optional asynchronous bootstrap method. The desktop registers it on the existing channel and shares bootstrap snapshot construction with the synchronous method.
Cache preload bootstrap snapshots
apps/desktop/src/preloadSnapshot.ts, apps/desktop/src/preload.ts, apps/desktop/src/preloadSnapshot.test.ts
The preload snapshot helper caches the initial value, shares in-flight refreshes, and replaces the value after successful refreshes. The preload bridge uses the helper for bootstrap reads and caches the enabled setting. Tests cover pending reads, failures, recovery, and asynchronous startup results.
Refresh web topology through the bridge
apps/web/src/connection/desktopLocal.ts, apps/web/src/connection/desktopLocal.test.ts, apps/web/src/connection/platform.ts, docs/internals/performance-regressions.md
The web reader refreshes asynchronously when supported and falls back to the synchronous method otherwise. Failed reads leave the prior snapshot intact. Platform registration awaits the topology read, and the documentation describes asynchronous polling.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Renderer
  participant WebReader as DesktopSecondaryBootstrapsReader
  participant Preload as DesktopBridge
  participant MainIPC as readLocalEnvironmentBootstraps
  participant Snapshot as readLocalEnvironmentBootstrapSnapshot
  Renderer->>WebReader: refresh topology
  WebReader->>Preload: readLocalEnvironmentBootstraps
  Preload->>MainIPC: asynchronous IPC request
  MainIPC->>Snapshot: read bootstrap snapshot
  Snapshot-->>MainIPC: bootstrap list
  MainIPC-->>Preload: bootstrap list
  Preload-->>WebReader: refreshed bootstrap list
  WebReader-->>Renderer: topology read result
Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 1def7

This change moves recurring desktop topology reads off synchronous IPC so the renderer is no longer blocked. It keeps startup reads synchronous and keeps the fallback for older bridges. No concrete merge-blocking risk was found.

Security architecture risk: 🔵 Low · up to 1def7

The change preserves the existing permissions and credential flow without demonstrating a new access path. Remaining risk concerns stalled refreshes leaving connection information stale.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The differential exposure is confined to bootstrap information already available to callers of the desktop bridge. The new method reads the same desktop-owned backend pool and configuration, without accepting attacker-selected URLs, backend identifiers, or credentials.

Trust Boundaries and Controls

  • observed — The renderer-to-main boundary retains schema validation: the asynchronous request has a void payload and the same bootstrap result schema as the synchronous request. Neither inspected registration wrapper explicitly allowlists senders; that behavior predates this PR and does not establish untrusted reachability without runtime isolation evidence.

Resilience and Maintainability Implications

  • observed — Settled failures retain only unexpired secondary registrations, with endpoint matching required after credential-refresh failure. A request that never settles is different: the preload pending slot remains occupied and the platform poll cannot emit its next reconciliation set. This does not itself demonstrate extended credential authority or an introduced authorization bypass.

Hardening Proposals

  • proposed — Consider a bounded refresh deadline and recovery policy that releases a stalled pending request, ignores superseded late responses, and keeps transport failure distinct from authoritative empty topology. This would strengthen freshness and failure containment rather than remediate a verified security finding.
Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main change: preventing renderer blocking during desktop topology polling.
Description check Passed The description is complete. It explains the problem, implementation, scope, linked issue and maintainer direction, focused verification, benchmark limits, and test results.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@otavio

otavio commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Thanks @daniyal-sh — I built on this in #19511. Your commit is cherry-picked unchanged as the first commit, and a second commit makes the desktop push topology changes into your preload cache, so the renderer no longer polls the main process at all (it also fixes the trace flooding in #19505). That made the async readLocalEnvironmentBootstraps refresh unnecessary, so #19511 removes it. Before/after desktop traces are in the PR body.

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

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants