Repository navigation
perf(macos): avoid ps during terminal repaint signals - #26985
outof-place wants to merge 25 commits into
Conversation
…g ps Pane polls, SIGWINCH delivery, launch-paste checks and cwd lookups each forked a setuid-root `ps` or an `lsof`. The terminal daemon alone started about 150 of them a minute with a few panes open. native/proc-info-darwin is a small C N-API addon, built universal by build:proc-info-macos (part of build:native, build:mac and build:mac:release) and shipped as Resources/native/orca-proc-info.node. A beforePack guard fails macOS packaging if it is missing or lacks the target slice. It answers with sysctl(KERN_PROC_ALL/PID/TTY), KERN_PROCARGS2 for one terminal's processes only, and proc_pidinfo for cwd. Its fields match what `ps` prints, including lstart in the user's locale. Four call sites read it first and keep their `ps`/`lsof` path as the fallback: the cheap process-table tier, the SIGWINCH foreground-group lookup, the launch-paste terminal rows, and process cwd. The full evidence capture stays on `ps`, because only root can read other users' argv. Cheap tier 16.4 ms + spawn -> 1.0 ms; SIGWINCH lookup 1.9 ms -> 0.01 ms; terminal rows 2.2 ms -> 0.11 ms; cwd 35-126 ms -> 0.001 ms. Fixes #26984 Refs #24889 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to An interactive macOS run of the event-loop test can terminate on the uncaught heartbeat error. Production lookups retain their fallback, so the impact is bounded to that test workflow. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
The PR adds native full evidence capture through readNativeFullProcessTable and uses it for the strict process-table reader. It also adds readNativeShellForegroundRows for the shell-foreground sna… |
Remove or defer the full evidence and shell-foreground capture changes, including their dedicated tests, to a PR linked to the evidence-capture work. Keep the [#26984] changes for the cheap process table, targeted foreground-group lookup, t… |
| Docstring Coverage | |
Docstring coverage is 45.31% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 64 functions across 37 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
-
Check name Status Explanation Linked Issues check The PR meets the coding requirements in directly linked issue [ #26984]. The native path covers the cheap process table, foreground-group lookup, terminal rows, and process cwd lookup. The readers reta…Title check The title clearly describes a primary performance change: avoiding ps during macOS terminal repaint signal handling. Description check The description includes all required sections and provides detailed motivation, scope, testing, performance data, alternatives, risks, and checklist status. However, it understates the implementation…
Full details: Out of Scope Changes check
-
Explanation
The PR adds native full evidence capture through
readNativeFullProcessTableand uses it for the strict process-table reader. It also addsreadNativeShellForegroundRowsfor the shell-foreground snapshot. Issue [#26984] explicitly excludes the full evidence capture associated with [#24889]. The related full-capture and shell-foreground tests are also outside that issue's stated scope.Resolution
Remove or defer the full evidence and shell-foreground capture changes, including their dedicated tests, to a PR linked to the evidence-capture work. Keep the [
#26984] changes for the cheap process table, targeted foreground-group lookup, terminal rows, cwd lookup, fallbacks, packaging, and required tests.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 45.31% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 64 functions across 37 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
The full `ps -axo ...,tty=,lstart=,command=` capture behind #24889 needs every process's argv. The kernel withholds argv (and PROC_PIDTBSDINFO) from other users' processes, including the pane root login(1). proc_pidpath still answers for them (158 of 158 root-owned processes on macOS 27), and kinfo_proc already carries ppid, pgid, tpgid, tty and uid. listProcessesWithCommands reads argv wherever the kernel allows and the executable path for the rest. It escapes control bytes and trims padding the way ps does under a UTF-8 locale. The full and shell-foreground tiers now read from it, and a withheld-argv process is named by its path. A capture still forks ps only when such a process holds a terminal and is an interpreter or an agent, whose argv only a root ps can read. Full capture: 107-203 ms -> 6.8-15 ms (about 1,000 processes, load 20-50). Agent verdicts for the 11 live panes were identical to ps. Refs #24889 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Follow-up commit 6c193bb also moves the full evidence capture from #24889 onto the kernel read, without root.
|
ps prints lstart with the locale's %c, and Orca's parsers expect the en_US/C shape (#27004). The addon followed the user's locale to stay byte-equal with an unpinned ps. With ps now pinned to en_US.UTF-8 in #27005, the addon formats in C, which prints the same string, so kernel rows and the ps fallback compare equal under any LANG. Refs #27004 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
7ef79b4e-ef06-4f9c-8b31-53a623092fd4
📒 Files selected for processing (27)
config/build-plugins/plain-node-entry-guard.tsconfig/scripts/build-proc-info-macos.mjsconfig/scripts/dev-proc-info-addon.mjsconfig/tsconfig.cli.jsonconfig/tsconfig.tc.web.jsonelectron.vite.config.tsnative/proc-info-darwin/src/proc_api_arguments.cnative/proc-info-darwin/src/proc_api_arguments.hnative/proc-info-darwin/src/proc_info.cnative/proc-info-darwin/src/proc_tty_names.cnative/proc-info-darwin/src/proc_tty_names.hsrc/main/native-process-snapshot-client.tssrc/main/native-process-snapshot-event-loop.test.tssrc/main/native-process-snapshot-protocol.tssrc/main/native-process-snapshot-worker-entry.tssrc/main/ports/port-scan-command-client.test.tssrc/main/pty/posix-pty-foreground-group.test.tssrc/main/pty/posix-pty-foreground-group.tssrc/main/runtime/terminal-foreground-group-native.test.tssrc/main/runtime/terminal-foreground-group.tssrc/main/worker-thread-entry-path.tssrc/shared/cheap-process-table-snapshot-reader.tssrc/shared/native-process-info.test.tssrc/shared/native-process-info.tssrc/shared/native-process-table.test.tssrc/shared/native-process-table.tssrc/shared/process-table-snapshot-reader.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| } else if (!format_cached_process_tty_name(proc.kp_eproc.e_tdev, tty, sizeof(tty))) { | ||
| napi_throw_error(env, "ORCA_PROC_INFO_TTY_UNKNOWN", "terminal name is not cached"); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '34,110p' src/main/runtime/terminal-foreground-group.ts
sed -n '375,413p' native/proc-info-darwin/src/proc_info.c
sed -n '91,105p' native/proc-info-darwin/src/proc_tty_names.cRepository: stablyai/orca
Length of output: 4364
Populate the main-thread terminal cache before using the fallback.
On a cold terminalOf lookup, readProcess(pid) can miss the main-thread cache, throw, and make the caller invoke ps. This adds an avoidable process spawn before the native terminal-row path runs. Populate the cache when the terminal name is not cached, while preserving the fallback for unknown devices.
🐛 Suggested fix
--- "a/native/proc-info-darwin/src/proc_info.c"
+++ "b/native/proc-info-darwin/src/proc_info.c"
@@ -404,10 +404,13 @@
} else {
memmove(tty, name, strlen(name) + 1);
}
- } else if (!format_cached_process_tty_name(proc.kp_eproc.e_tdev, tty, sizeof(tty))) {
- napi_throw_error(env, "ORCA_PROC_INFO_TTY_UNKNOWN", "terminal name is not cached");
- return NULL;
+ } else if (!format_cached_process_tty_name(proc.kp_eproc.e_tdev, tty, sizeof(tty))) {
+ format_process_tty_name(proc.kp_eproc.e_tdev, tty, sizeof(tty));
+ if (strcmp(tty, "??") == 0) {
+ napi_throw_error(env, "ORCA_PROC_INFO_TTY_UNKNOWN", "terminal name is not cached");
+ return NULL;
+ }
}
return make_row(env, &proc, NULL, tty);
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (!format_cached_process_tty_name(proc.kp_eproc.e_tdev, tty, sizeof(tty))) { | |
| napi_throw_error(env, "ORCA_PROC_INFO_TTY_UNKNOWN", "terminal name is not cached"); | |
| return NULL; | |
| } | |
| } else if (!format_cached_process_tty_name(proc.kp_eproc.e_tdev, tty, sizeof(tty))) { | |
| format_process_tty_name(proc.kp_eproc.e_tdev, tty, sizeof(tty)); | |
| if (strcmp(tty, "??") == 0) { | |
| napi_throw_error(env, "ORCA_PROC_INFO_TTY_UNKNOWN", "terminal name is not cached"); | |
| return NULL; | |
| } | |
| } |
| const heartbeat = setInterval(() => { | ||
| const now = performance.now() | ||
| maxGap = Math.max(maxGap, now - last) | ||
| last = now | ||
| ticks++ | ||
| native.readProcess(process.pid) | ||
| }, 5) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,125p' src/main/native-process-snapshot-event-loop.test.ts
sed -n '370,415p' native/proc-info-darwin/src/proc_info.c
sed -n '80,106p' native/proc-info-darwin/src/proc_tty_names.cRepository: stablyai/orca
Length of output: 6595
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- changed-file diff ---'
git diff --no-ext-diff --unified=80 ce9eb662bb8a9a16cbd16307ecaa762f52283d09 f3b5fba03f0609a37b898474237c23dee797c648 -- src/main/native-process-snapshot-event-loop.test.ts native/proc-info-darwin/src/proc_info.c native/proc-info-darwin/src/proc_tty_names.c
printf '%s\n' '--- TTY cache symbols and callers ---'
rg -n -F --glob '*.{c,h,ts,mjs}' -- 'cache_tty_name' native src config || test "$?" -eq 1
rg -n -F --glob '*.{c,h,ts,mjs}' -- 'format_process_tty_name' native src config || test "$?" -eq 1
rg -n -F --glob '*.{c,h,ts,mjs}' -- 'format_cached_process_tty_name' native src config || test "$?" -eq 1
printf '%s\n' '--- readProcess callers and launch environment ---'
rg -n -F --glob '*.{ts,mjs,json,yml,yaml}' -- 'readProcess(' src config test tests package.json .github 2>/dev/null || test "$?" -eq 1
rg -n -F --glob '*.{ts,mjs,json,yml,yaml}' -- 'native-process-snapshot-event-loop' . 2>/dev/null || test "$?" -eq 1Repository: stablyai/orca
Length of output: 28311
🏁 Script executed:
git diff --no-ext-diff --unified=40 ce9eb662bb8a9a16cbd16307ecaa762f52283d09 f3b5fba03f0609a37b898474237c23dee797c648 -- src/main/native-process-snapshot-event-loop.test.ts native/proc-info-darwin/src/proc_info.c native/proc-info-darwin/src/proc_tty_names.c
printf '%s\n' '--- cache references ---'
rg -n -F --glob '*.{c,h}' -- 'cache_tty_name' native || test "$?" -eq 1
rg -n -F --glob '*.{c,h}' -- 'format_process_tty_name' native || test "$?" -eq 1
printf '%s\n' '--- relevant callers ---'
rg -n -F --glob '*.ts' -- 'readProcess(' src || test "$?" -eq 1Repository: stablyai/orca
Length of output: 27771
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- native API wrapper ---'
sed -n '1,220p' src/shared/native-process-info.ts
printf '%s\n' '--- focused addon tests ---'
sed -n '135,255p' src/shared/native-process-info.test.ts
printf '%s\n' '--- native argument parser ---'
sed -n '1,220p' native/proc-info-darwin/src/proc_api_arguments.c
sed -n '1,180p' native/proc-info-darwin/src/proc_api_arguments.hRepository: stablyai/orca
Length of output: 12866
Keep the guard around the heartbeat probe.
getNativeProcessInfo() sends list calls through NativeProcessSnapshotClient, but readProcess remains a direct addon call. The TTY cache is thread-local, so a worker preload does not populate the main-thread cache. With a controlling terminal, the heartbeat can therefore throw ORCA_PROC_INFO_TTY_UNKNOWN outside main().catch(...).
Using format_process_tty_name in native readProcess would remove this specific failure. Keep the guard for the current test, and treat that native change as separate follow-through.
🐛 Suggested fix
- native.readProcess(process.pid)
+ try {
+ native.readProcess(process.pid)
+ } catch {
+ // Keep heartbeat probe failures from escaping the interval callback.
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const heartbeat = setInterval(() => { | |
| const now = performance.now() | |
| maxGap = Math.max(maxGap, now - last) | |
| last = now | |
| ticks++ | |
| native.readProcess(process.pid) | |
| }, 5) | |
| const heartbeat = setInterval(() => { | |
| const now = performance.now() | |
| maxGap = Math.max(maxGap, now - last) | |
| last = now | |
| ticks++ | |
| try { | |
| native.readProcess(process.pid) | |
| } catch { | |
| // Keep heartbeat probe failures from escaping the interval callback. | |
| } | |
| }, 5) |
orcad's bundle names native-process-snapshot-worker-entry.js, but the artifact manifest and the child-entry build did not include it, so a packaged orcad slot would lack the worker and orcad-runtime-entry-artifacts.test.ts failed. Add it to ORCAD_ARTIFACTS, ORCAD_CHILD_ENTRY_POINTS and knip's entry list, as for the port-scan command worker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…/perf-review-26985 # Conflicts: # src/main/pty/posix-pty-foreground-group.ts
|
Trade-offs of the current design ( Setup: macOS on an M4 Max, against a
Lookups happen on PTY resizes, and the renderer fires two per revealed pane, so today each reveal can block main for about 6–18 ms.
|
|
The targeted resolver improvement is real, but this adds a native module and release-packaging surface for a narrow repaint path. Real signed-release loading remains unvalidated, and no complete interaction measurement establishes a gain worth that maintenance and merge risk. The larger process-table improvement has landed separately in #27146 with contributor credit. Closing this proposal as not planned; #26984 describes broader work and remains open. |
ELI5
On macOS, restored terminals can find their repaint signal target without launching
ps.What Changed
A small macOS native addon checks one known process and its known terminal device. Existing process-group and own-terminal safeguards remain; a missing addon or failed lookup keeps the existing
psfallback. Whole-process enumeration and cwd lookup keep their existing implementations.Why
The process and terminal are already known. A targeted kernel lookup avoids a subprocess without introducing a worker, process-table parser or retained native cache.
Reduced the PR from 1,665 to 812 added lines. Runtime adds about 246 lines, including the 129-line C addon. Removed the copied-app signing harness, standalone CI YAML and lengthy experimental reliability entry. Focused native tests now run in the existing Mac ARM/Intel Headless Server lanes.
Linked Issue
Related: #26984, #24889.
Scope distinction: #24889 concerns repeated full-process-table captures. This narrowed PR handles one pane’s repaint lookup and does not fix that whole-table cost. Giorgio’s #24890 addresses the full capture with numeric terminal devices.
Visual Proof
N/A — the same terminal receives the same signal; contents and layout are unchanged.
Testing
ps; invalid PID/device and own-terminal cases are refused.c7cb1c9: PR Checks, Headless Server, Mobile, Computer-use and test-size checks. This includes the Mac ARM/Intel native checks, all unit shards, static/typecheck, and Linux/Windows packaging.Performance measurements
Actual repaint-target resolver, Apple M4 Max / Node 24.20, 2,244 processes; six alternating fresh processes, same owned terminal and checked result:
pspathNative calls launched zero subprocesses. First-call native range was 1.22–35.04 ms on the busy host. This measures explicit repaint-target lookup during restore/tab return; it is not a normal PTY resize or frame-rate measurement.
Alternatives checked against #24889
The separate full-capture implementation is now available as draft #27146. It preserves fresh terminal names and keeps the existing targeted repaint path separate. Giorgio and out-of-place are credited as co-authors in its commit.
Seven alternating full captures on this Mac (~2,229 processes) measured 1,314.7 ms with
tty=versus 114.3 ms withtdev=and a fresh device-name index;psCPU was 1,300 versus 90 ms. Fresh indexing cost 3.74 ms. Names matched for the same PID/start-time cohorts. A persistent index is unnecessary for that gain and has stale-name/negative-cache cases in #24890 that require correction. This is a separate, higher-impact fix. Credit Giorgio Zamparelli’s proposal and retainCo-authored-by: Giorgio Zamparelli <giorgio.zamparelli@gmail.com>if adopting it. No implementation from #24890 has been incorporated here.A single-query microbenchmark measured 4.012 → 2.033 ms with
tdev=, but it excludes the fresh own-terminal read required for the safe replacement. That result does not establish a complete-resolver gain. Extending the existingnode-ptymodule could retain zero subprocesses; its descriptor lifetime and old-module fallback still need dedicated validation. Neither alternative has been added to this PR.The concrete smaller prototype (
287cf573043) was tested against the full resolver: 24 alternating fresh processes and 192 warm calls per implementation, Mac M4 Max / Node 24.20, ~2,259 processes, host load 7–10 on 16 logical cores. Medians:pscallstty=with cached own rowtdev=with fresh own-device rowc7cb1c9The smaller prototype is not proposed for adoption: its warm median regressed about 3%, and resolver-process CPU increased 1.206 → 2.194 ms. That CPU measurement excludes spawned
psCPU and is not an app-wide or battery measurement. Native first-call data retains one 202.020 ms outlier; none were discarded.The prototype passed full typechecks, changed-code quality, 67 focused tests (three Linux-only skips), real owned-PTY SIGWINCH acknowledgment and own-terminal refusal. Both out-of-place and Giorgio are credited with actual co-author trailers in the prototype commit. All owned PTYs/processes exited. The prototype remains unpublished because the safe warm gain disappeared.
AI Disclosure
Original contribution: Claude Code. Maintainer revision and independent review: Codex.
Review
Reviewed PID validation, terminal-device ownership, own-terminal refusal, trusted paths, fallback and packaging. No negative user-facing behavior identified within the narrowed scope. The native dependency adds a small packaging surface.
Original author: @outof-place; their commits remain in history. Preserve
Co-authored-by: outof-place <contact@outofplace.space>when squashing.Agent skill upstream boundary
Notes
Developer ID is Apple's existing certificate for signing distributed Mac applications. Orca already uses it. This PR adds no credentials, entitlement changes, hardened-runtime exceptions or new signing policy. The existing release signer includes the addon under app resources.
Actual Developer-ID-signed release loading has not been tested. The removed ad-hoc signing fixture could not prove it; addon load failure preserves
ps. This remains a release-validation gap, explicitly accepted rather than hidden behind a new test harness. Windows, Linux, SSH and plain Node hosts retain existing behavior.Checklist