Skip to content

perf(runtime): remove timer clamps from cooperative yields - #10908

Merged
nwparker merged 2 commits into
mainfrom
perf/remove-clamped-event-loop-yields
Jul 27, 2026
Merged

nwparker merged 2 commits into
mainfrom
perf/remove-clamped-event-loop-yields

Conversation

@nwparker

@nwparker nwparker commented Jul 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up audit to #10892 found the same timer-clamp issue in 11 cooperative-yield implementations across clipboard validation, large editor pastes, terminal input queues, browser text insertion, and the OpenCode usage scanner.

  • Add one shared yield primitive: setImmediate in Node and MessageChannel in renderer/web runtimes, with a timer fallback for Vitest and unsupported environments.
  • Migrate only true cooperative yields. Lifecycle deferrals, ordering-sensitive drains, and PTY paste/accepted-write pacing remain unchanged.
  • A maximum 16 MiB editor payload split into 16 KiB chunks performs 1,023 between-chunk yields. A three-round headless Chromium benchmark measured medians of 4,567.2 ms for setTimeout(0) and 1.7 ms for MessageChannel: about 4.57 seconds of artificial wait removed at the limit. With 0.1 ms of simulated work per batch, concurrent timer latency improved slightly (5.1 ms to 4.4 ms), so the faster path does not achieve its gain by starving other timer work.

Screenshots

No visual change.

Testing

  • pnpm lint — changed-file lint, switch exhaustiveness, reliability gates, and max-lines ratchet pass; the full command fails only on six pre-existing unlocalized Ghostty search keywords in unrelated settings files.
  • pnpm typecheck
  • pnpm test — focused post-rebase suite run instead: 14 files, 205 tests passing.
  • pnpm build
  • Added or updated high-quality tests that would catch regressions, or explained why tests were not needed

Focused coverage includes the shared scheduler, clipboard bounds, browser insertion, OpenCode scanning, text controls, primary selection, dictation, Monaco, rich Markdown, terminal paste planning, PTY input queueing, and PTY transport.

AI Review Report

Three independent adversarial agents reviewed scheduler correctness, all migrated call sites, performance claims, tests, security boundaries, and cross-platform behavior. They found and verified fixes for two material issues:

  • The first indexed resolver queue retained completed callbacks until global quiescence under sustained concurrent producers. Renderer yields now use token-keyed Map entries deleted before resolution, with a two-producer replenishment regression test.
  • Terminal paste and accepted-write timers provide pacing where SSH/local PTYs lack true drain acknowledgement. Those two sites were restored to their original timer behavior; only the PTY input queue keeps MessageChannel, where existing code explicitly identifies the nested timer clamp as harmful.

CodeRabbit's consistency nit was also addressed by routing OpenCode scanning through the shared scheduler. All agents re-reviewed the fixes and reported no remaining findings.

Cross-platform review covered macOS, Linux, Windows, WSL, SSH, remote-runtime, and folder-workspace behavior. The scheduling APIs are runtime-based rather than OS-based, no shortcuts/labels/paths/shell commands changed, and existing remote acknowledgements plus intentional PTY pacing remain intact.

Security Audit

No dependencies, auth, secrets, commands, or path handling changed. Existing input size limits, control-sequence sanitization, target ownership checks, IPC chunk bounds, PTY backpressure/pacing, timeouts, and bracketed-paste cleanup remain in place. The change only replaces the scheduling mechanism between existing bounded operations where timer duration has no behavioral role.

Notes

The full-lint localization failure is unrelated to this diff and is reproducible on main. Intentional zero-delay timer sites remain in place after the adversarial audit.

ELI5

Some zero-delay timers still wait about four milliseconds in browsers. Large operations that pause hundreds of times therefore spend seconds waiting for clocks, not doing work. The shared yield uses the event loop mechanism designed for immediate cooperation, while the few PTY sites where delay is intentional keep their timers.

Proof of performance improvement

A maximum 16 MiB editor payload performs 1,023 between-chunk yields. In three headless Chromium rounds, the median scheduler time fell from 4,567.2 ms with setTimeout(0) to 1.7 ms with MessageChannel. With 0.1 ms of simulated work per batch, concurrent timer latency improved from 5.1 ms to 4.4 ms, so the gain did not come from starving timer work.

Proof of zero regression

Only true cooperative yields moved. Adversarial review restored terminal paste and accepted-write timers because those sites use delay as pacing; bounded clipboard, paste, queue, sanitizer, timeout, and remote acknowledgement behavior remains unchanged. Evidence: 14 focused files with 205 tests, full typecheck, green PR verify, packaged Windows crash-survival, Windows terminal-restart E2E, and explicit macOS, Linux, Windows, WSL, SSH, remote-runtime, and folder-workspace review.

Review loop count + verdict

2 loops with 3 independent adversarial reviewers. Round 1 found a retained-callback queue and two incorrectly migrated pacing sites. Round 2 verified the token-keyed callback cleanup, restored PTY timers, shared OpenCode scheduler use, and reported no remaining findings.

Verdict: ship.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a shared yieldToEventLoop utility with environment-specific scheduling, including setImmediate, MessageChannel, and timer fallbacks. Adds tests for Node-like and renderer-like scheduling behavior. Updates browser insertion, OpenCode scanning, renderer paste and input flows, clipboard measurement, and PTY queue handling to use the shared utility instead of local timer-based implementations.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the main change: replacing cooperative yield timers to remove clamp-induced latency.
Description check ✅ Passed The description matches the template and includes the required summary, screenshots, testing, AI review, security audit, and notes sections.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

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

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
src/main/opencode-usage/scanner.ts (1)

147-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the shared yieldToEventLoop helper here.

This migration only changes the local implementation to setImmediate, so the scanner remains outside the shared runtime/fake-timer contract. Import ../../shared/event-loop-yield and remove this local helper to keep all cooperative yields consistent.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 00052801-dc06-4273-8828-55338df81924

📥 Commits

Reviewing files that changed from the base of the PR and between fb26254 and 1148cf5.

📒 Files selected for processing (15)
  • src/main/browser/browser-text-insertion.ts
  • src/main/opencode-usage/scanner.ts
  • src/renderer/src/components/dictation/dictation-insertion-target.ts
  • src/renderer/src/components/editor/monaco-large-text-paste.ts
  • src/renderer/src/components/editor/rich-markdown-large-text-paste.ts
  • src/renderer/src/components/terminal-pane/pty-input-write-queue.ts
  • src/renderer/src/components/terminal-pane/pty-transport.ts
  • src/renderer/src/components/terminal-pane/terminal-paste-executor.ts
  • src/renderer/src/lib/agent-draft-paste-content.ts
  • src/renderer/src/lib/paste-payload-metadata.ts
  • src/renderer/src/lib/primary-selection-paste.ts
  • src/renderer/src/lib/text-control-paste.ts
  • src/shared/clipboard-text.ts
  • src/shared/event-loop-yield.test.ts
  • src/shared/event-loop-yield.ts

nwparker added 2 commits July 27, 2026 12:19
Renderer paste and input loops can schedule more than a thousand zero-delay timer yields for a maximum-size payload. Chromium clamps nested timers to 4ms, adding seconds of idle wall time.

Use MessageChannel tasks in renderer runtimes and setImmediate in Node while retaining a timer fallback for tests and unsupported environments.
Adversarial review found that concurrent producers could retain resolved callbacks until global quiescence. Route renderer yields by token and delete each resolver before resuming its producer.

Keep timer pacing in terminal paste and accepted-write loops where SSH and local PTYs do not provide drain acknowledgement. Use the shared scheduler for the OpenCode scanner.
@nwparker
nwparker force-pushed the perf/remove-clamped-event-loop-yields branch from 1148cf5 to 239d89b Compare July 27, 2026 19:19
@nwparker
nwparker merged commit 8b154d6 into main Jul 27, 2026
3 checks passed
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
…10908)

* perf(runtime): remove timer clamps from cooperative yields

Renderer paste and input loops can schedule more than a thousand zero-delay timer yields for a maximum-size payload. Chromium clamps nested timers to 4ms, adding seconds of idle wall time.

Use MessageChannel tasks in renderer runtimes and setImmediate in Node while retaining a timer fallback for tests and unsupported environments.

* fix(runtime): preserve pacing and release yield callbacks

Adversarial review found that concurrent producers could retain resolved callbacks until global quiescence. Route renderer yields by token and delete each resolver before resuming its producer.

Keep timer pacing in terminal paste and accepted-write loops where SSH and local PTYs do not provide drain acknowledgement. Use the shared scheduler for the OpenCode scanner.
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.

1 participant