Repository navigation
perf(process-table): name macOS terminals from tdev= instead of ps tty= - #24890
giorgio-zamparelli wants to merge 1 commit into
Conversation
On macOS the full process-table capture asked `ps` for `tty=`, which names each process's terminal with devname(3): a walk of /dev per process. On a 1,854-process Mac with 143 ptys that column alone cost 1.3s of system time, and the whole capture 1.5-1.8 CPU-seconds, run nearly back to back by the daemon. Ask for `tdev=` (major/minor) instead and translate it back to the exact name `tty=` prints from a /dev index of tty* and console entries, built once and rebuilt when a capture names a terminal it has not seen. Consumers keep receiving identical `tty` strings: on the measuring host the translated capture matched `ps -o tty=` for all 1,969 processes, in 356ms against 6,559ms for the old argv under the same load. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ewbhm4qQrr8ssHPL4gfo43
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)📝 WalkthroughWalkthroughOn macOS, the process-table capture now requests Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to A reused terminal device number may continue to appear as a number rather than a terminal name. Address the cache behavior before merging, or accept this bounded macOS edge case. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change stays within macOS process inspection, but terminal-name caching can retain failed lookups after recovery. Existing process-identity and process-group checks limit the impact; no security bypass was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 6adc423a-4dce-4f7c-81af-634bdba5e998
📒 Files selected for processing (5)
src/shared/darwin-terminal-names.test.tssrc/shared/darwin-terminal-names.tssrc/shared/process-table-snapshot-reader.tssrc/shared/process-table-snapshot.tssrc/shared/shell-foreground-snapshot.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| const device = `${(stat.rdev >>> 24) & 0xff}/${stat.rdev & 0xffffff}` | ||
| if (!built.has(device)) { | ||
| built.set(device, name) | ||
| } | ||
| }) | ||
| return built | ||
| } | ||
|
|
||
| return { | ||
| resolve: async (devices) => { | ||
| let current = names ?? (names = await build()) | ||
| const missing = [...devices].filter((device) => !current.has(device) && !unnamed.has(device)) | ||
| if (missing.length > 0) { | ||
| current = names = await build() | ||
| unnamed = new Set(missing.filter((device) => !current.has(device))) | ||
| } | ||
| return current | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,180p' src/shared/darwin-terminal-names.ts
sed -n '95,180p' src/shared/darwin-terminal-names.test.tsRepository: stablyai/orca
Length of output: 7593
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- changed file and tests ---'
wc -l src/shared/darwin-terminal-names.ts src/shared/darwin-terminal-names.test.ts
sed -n '1,260p' src/shared/darwin-terminal-names.test.ts
printf '%s\n' '--- call sites and related contract text ---'
rg -n -C 3 'nameDarwinTerminals|createDarwinTerminalNameIndex|translateDarwinTerminalDevices|listDarwinTerminalDevices|darwin-terminal|tdev=|tty=' --glob '!node_modules' --glob '!dist' .
printf '%s\n' '--- PR diff summary and focused diff ---'
git diff --stat cc73c8e1a72b0e9ee9c29e57458ce307f5f019c2 c34f637ffa1ca9a90ced7d5c979aecdef563212d -- src/shared/darwin-terminal-names.ts src/shared/darwin-terminal-names.test.ts
git diff --unified=30 cc73c8e1a72b0e9ee9c29e57458ce307f5f019c2 c34f637ffa1ca9a90ced7d5c979aecdef563212d -- src/shared/darwin-terminal-names.ts src/shared/darwin-terminal-names.test.tsRepository: stablyai/orca
Length of output: 41931
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings
Length of output: 47480
🌐 Web query:
Darwin macOS pseudo terminal /dev/ttys device minor number reuse after close normal lifecycle dev_t allocation
💡 Result:
**Yes—PTY device-number reuse is a normal lifecycle behavior on Darwin.** A PTY opened through `/dev/ptmx` is a cloned device: the kernel assigns a minor number to the new PTY, and after the PTY is fully released that slot can be assigned to another PTY. So a saved `dev_t` is **not** a durable, unique identity for a PTY across close/reopen cycles.
One useful distinction: **closing one file descriptor does not necessarily free the PTY** if another descriptor or process still holds the master or slave open. Reuse is possible after the PTY itself is torn down, not simply after any individual `close()`.
Darwin’s `dev_t` encodes major and minor numbers; the XNU header’s `minor()` extracts the low 24 bits. That tells you how to inspect the number, but not how long a particular PTY minor remains assigned. ([github.com](https://github.com/apple/darwin-xnu/blob/main/bsd/sys/types.h?utm_source=openai))
**Caveat:** the available XNU source references are not all the same release, and the search results don’t establish an allocation-order or reuse guarantee for every macOS version. Treat the minor as an identifier for the *current* PTY instance, not as a persistent ID. ([newosxbook.com](https://www.newosxbook.com/src.php?file=%2Fbsd%2Fconf%2Ffiles&tree=xnu&utm_source=openai))
Citations:
- 1: https://github.com/apple/darwin-xnu/blob/main/bsd/sys/types.h?utm_source=openai
- 2: https://www.newosxbook.com/src.php?file=%2Fbsd%2Fconf%2Ffiles&tree=xnu&utm_source=openai
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- process-table types and reader ---'
rg -n -C 5 'interface .*ProcessTable|type .*ProcessTable|tty:|terminal|createProcessTableCapture|nameDarwinTerminals|\.tty\b' src/shared src/main --glob '*.{ts,tsx}' | head -n 260
printf '%s\n' '--- focused reader ---'
sed -n '240,292p' src/shared/process-table-snapshot-reader.ts
printf '%s\n' '--- terminal matching consumers ---'
rg -n -C 6 'processTable|process table|ProcessTable|terminal.*tty|tty.*terminal|row\.tty|\.tty ===|\.tty !==|tty:' src/main src/shared --glob '*.{ts,tsx}' | head -n 320Repository: stablyai/orca
Length of output: 41328
🌐 Web query:
site:github.com/apple/darwin-xnu pty minor reuse dev_t tty allocation
💡 Result:
The XNU search results **don’t establish the PTY minor-allocation or reuse behavior**. The closest relevant source is [`bsd/kern/tty.c`](https://github.com/apple/darwin-xnu/blob/main/bsd/kern/tty.c): `ttyopen()` records the supplied `dev_t`, and `ttyclose()` increments the TTY generation counter to help pending I/O detect that a TTY was recycled. That shows TTY-state recycling, but not how a PTY minor is allocated or reused. ([github.com](https://github.com/apple/darwin-xnu/blob/main/bsd/kern/tty.c?utm_source=openai))
XNU’s `dev_t` helpers encode the minor in the low 24 bits; that explains representation, not PTY allocation policy. ([github.com](https://github.com/apple/darwin-xnu/blob/main/bsd/sys/types.h?utm_source=openai))
**Unresolved:** whether closing a PTY makes its minor available for reuse, and what allocator selects the next minor. The search didn’t locate the PTY driver/allocation code needed to verify that.
Citations:
- 1: https://github.com/apple/darwin-xnu/blob/main/bsd/kern/tty.c?utm_source=openai
- 2: https://github.com/apple/darwin-xnu/blob/main/bsd/sys/types.h?utm_source=openai
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- exact process-table files ---'
git ls-files '*process-table*'
rg -n -C 5 'tty' src/shared/process-table-snapshot.ts src/shared/process-table-snapshot-reader.ts src/main/daemon src/main/pty --glob '*.{ts,tsx}' || true
printf '%s\n' '--- all consumers of exported process-table readers/types ---'
rg -n -C 4 'readProcessTable|readProcessTableFresh|readStrictProcess|ProcessTableRow|StrictProcessTable|createProcessTableCapture|\.rows\b|\.processes\b' src --glob '*.{ts,tsx}' | head -n 300
printf '%s\n' '--- direct XNU source probes ---'
for p in bsd/kern/pty.c bsd/kern/tty.c bsd/sys/ttycom.h; do
url="https://raw.githubusercontent.com/apple/darwin-xnu/main/$p"
echo "### $p"
curl -L --fail --silent --show-error "$url" | rg -n -C 3 'minor|dev_t|alloc|free|pty|clone|reuse|ttyclose' | head -n 120 || true
doneRepository: stablyai/orca
Length of output: 41870
🌐 Web query:
Darwin XNU pty.c pseudo terminal minor allocation reuse clone device source
💡 Result:
In current XNU, the PTY code is split across **`bsd/kern/tty_pty.c`** and **`bsd/kern/tty_dev.c`**—not a standalone `pty.c`.
- **Legacy PTYs:** `pty_init()` in `tty_pty.c` creates fixed `/dev/pty…` and `/dev/tty…` nodes for minor numbers `0` through `n_ptys - 1` (capped at `NPTY`). This is a fixed pool, not per-open minor allocation. ([github.com](https://github.com/apple-oss-distributions/xnu/blob/main/bsd/kern/tty_pty.c))
- **PTY state and reuse:** `tty_dev.c` dispatches by device major/minor to the registered driver. The legacy PTY driver’s `open` callback retrieves state from `pt_ioctl[minor]`; its `free` callback is optional. The `tty_dev_t` interface defines those callbacks. ([github.com](https://github.com/apple-oss-distributions/xnu/blob/main/bsd/kern/tty_dev.c))
- **Clone PTYs:** For dynamically cloned PTYs, follow the `/dev/ptmx` path in `tty_ptmx.c` and `tty_dev.c`; `tty_pty.c` describes the fixed-node implementation. The available source pages identify these files, but don’t establish the exact allocation/reuse algorithm for a particular XNU version. ([newosxbook.com](https://newosxbook.com/code/xnu-8792.81.2/bsd/kern/))
So, for **minor allocation/reuse**, start with `tty_ptmx.c` and the clone-device handling in `tty_dev.c`; for the **legacy fixed minor pool**, start with `pty_init()` in `tty_pty.c`. The linked source is current `main` where noted; no specific XNU release or commit was specified.
Citations:
- 1: https://github.com/apple-oss-distributions/xnu/blob/main/bsd/kern/tty_pty.c
- 2: https://github.com/apple-oss-distributions/xnu/blob/main/bsd/kern/tty_dev.c
- 3: https://newosxbook.com/code/xnu-8792.81.2/bsd/kern/
🏁 Script executed:
set -o pipefail
for p in bsd/kern/tty_pty.c bsd/kern/tty_ptmx.c bsd/kern/tty_dev.c; do
url="https://raw.githubusercontent.com/apple-oss-distributions/xnu/main/$p"
echo "### $p"
curl -L --fail --silent --show-error "$url" |
rg -n -C 5 'minor|ptmx|clone|alloc|free|open|close|pty_init|pt_ioctl|n_ptys|unit|slot' |
head -n 240
doneRepository: stablyai/orca
Length of output: 18562
🏁 Script executed:
set -o pipefail
url='https://raw.githubusercontent.com/apple-oss-distributions/xnu/main/bsd/kern/tty_ptmx.c'
tmp=$(mktemp)
curl -L --fail --silent --show-error "$url" > "$tmp"
printf '%s\n' '--- ptmx allocation ---'
sed -n '269,390p' "$tmp"
printf '%s\n' '--- ptmx release ---'
rg -n -C 12 'ptmx_free_ioctl|pis_free|ptmx_clone' "$tmp"
rm -f "$tmp"Repository: stablyai/orca
Length of output: 13118
🏁 Script executed:
curl -L --fail --silent --show-error \
'https://raw.githubusercontent.com/apple-oss-distributions/xnu/main/bsd/kern/tty_ptmx.c' |
sed -n '463,515p'Repository: stablyai/orca
Length of output: 1575
Do not permanently cache unresolved Darwin device IDs.
When a PTY is released, Darwin can reuse its minor number for a new /dev/ttysXXX node. After the first lookup misses, unnamed prevents resolve from rebuilding the index for that number. Later captures can therefore keep the numeric major/minor value instead of the tty= name required by the process-table contract.
Suggested fix
let names: Map<string, string> | null = null
- let unnamed = new Set<string>()
return {
resolve: async (devices) => {
let current = names ?? (names = await build())
- const missing = [...devices].filter((device) => !current.has(device) && !unnamed.has(device))
+ const missing = [...devices].filter((device) => !current.has(device))
if (missing.length > 0) {
current = names = await build()
- unnamed = new Set(missing.filter((device) => !current.has(device)))
}
return current
}
}![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestion inline — the
/devindex'sunnamedbookkeeping drifts from its documented intent.
Reviewed changes
- macOS
psargv —PS_ARGSnow requeststdev=instead oftty=on darwin (Linux unchanged), with a rationale comment. - Device→name translation — new
src/shared/darwin-terminal-names.tsmapsmajor/minorback to the/devnametty=would print, behind a cached/devindex and amajor/minorfallback. - Reader wiring —
process-table-snapshot-reader.tsresolves names on darwin before parsing, best effort if/devis unreadable. - Tests — new
darwin-terminal-names.test.ts(translation, index walks, fallbacks) and an updated argv matcher inshell-foreground-snapshot.test.ts.
I verified the format against Apple's ps source (tdev prints "%d/%d", major(dev), minor(dev) and ?? for NODEV) and the Darwin dev_t layout, and confirmed every row.tty consumer only compares tty within a single snapshot, so the normal-case strings stay identical and the major/minor fallback preserves per-terminal grouping.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
| const missing = [...devices].filter((device) => !current.has(device) && !unnamed.has(device)) | ||
| if (missing.length > 0) { | ||
| current = names = await build() | ||
| unnamed = new Set(missing.filter((device) => !current.has(device))) |
There was a problem hiding this comment.
unnamed is rebuilt from missing rather than from the devices the rebuild still could not name, so a device learned as unnameable is forgotten the moment a genuinely new terminal triggers a rebuild. The next capture naming the vanished device walks /dev again, which contradicts the "remembered so it does not cost a walk of /dev per capture" contract on lines 57-58 and 100. The test at line 120 never re-resolves 16/80 after the new terminal, so it cannot catch this.
Technical details
# `unnamed` loses previously-learned devices on rebuild
## Affected sites
- `src/shared/darwin-terminal-names.ts:87-90` — `unnamed = new Set(missing.filter(...))` drops any device not in this capture's `missing` set.
- `src/shared/darwin-terminal-names.ts:57-58`, `100` — comments promise an unnameable device "does not cost a walk of /dev per capture".
- `src/shared/darwin-terminal-names.test.ts:120-139` — stops before re-resolving the vanished `16/80` after `ttys081` appears.
## Required outcome
- A device a rebuild cannot name stays remembered across later rebuilds, so a live process holding a gone terminal node does not re-trigger a `/dev` walk every time a new terminal opens.
- Re-resolving the vanished device after a rebuild (the test at line 136) must not add a walk.
## Suggested approach (optional)
- Union instead of replace: `unnamed = new Set([...unnamed, ...missing].filter((device) => !current.has(device)))`.
- Secondary: on first use, any unnameable device makes `resolve` build twice back-to-back (`names ?? (names = await build())` then the `missing.length > 0` rebuild). The first build already reflects `/dev` now, so the immediate rebuild adds nothing; the rebuild should only run when `names` is a cached, potentially stale map.
|
Thanks @giorgio-zamparelli. This approach (naming macOS terminals from |
ELI5
Orca regularly asks macOS for the list of every running process, including which terminal each one belongs to. Asking for the terminal's name makes macOS search its device folder once per process, which on a Mac with many processes and terminals takes over a second of CPU every time, and Orca asks every couple of seconds. This PR asks for the terminal's number instead, which is instant, and looks the names up itself once, so Orca sees exactly the same names at a small fraction of the cost.
What Changed
PS_ARGSon macOS askspsfortdev=(major/minor) instead oftty=. Linux is unchanged.src/shared/darwin-terminal-names.tsturns eachmajor/minorback into the exact nameps -o tty=prints (16/79becomesttys079,??stays??). It keeps a/devindex oftty*andconsoleentries, built on first use and rebuilt only when a capture names a terminal it has not seen (one opened since). A device a fresh walk still cannot name (a process outliving its terminal node) keeps itsmajor/minorspelling and is remembered, so it does not cost a walk per capture.process-table-snapshot-reader.tsapplies it to the full capture on macOS. Naming is best effort: if/devcannot be read, the capture keeps itsmajor/minorspellings rather than turning a readable table into a failed one.shell-foreground-snapshot.test.tsrecognised the full capture bytty=in its argv;PS_ARGSfollows the real host, so on a macOS host it now matchesPS_ARGSitself.Every consumer of
row.tty(agent-foreground-process-remote-evidence.ts,spawn-file-foreground-process.ts,spawn-file-child-processes.ts, the posix fence) receives identical strings.Why
Measured on macOS 26.6.2, Apple M5 Max, 1,854 processes and 143 ptys (#24889):
ps -axo pid=,ppid=,pgid=,tpgid=,stat=,tty=,lstart=,command=: 1.5–1.8 CPU-seconds per run, 1.3 s of it system time spent naming terminals; 6.6 s wall at load ~67.tdev=: 0.19 CPU-seconds.PROCESS_TABLE_EVIDENCE_BUDGET_MS(1.2 s).tty=is slow because devname(3) walks/devfor every process, so the cost grows with processes times/deventries. The existing comment already measuredtty=as 0.29 s of a 0.34 s capture; with more terminals open it dominates.Alternatives considered: dropping the terminal column (consumers need it to tell foreground groups on one terminal apart); naming only pane roots (changes the row contract every consumer relies on); generating
/var/run/dev.dbwithdev_mkdb(needs root, per machine, and macOS does not maintain it).Linked Issue
Fixes #24889
Visual Proof
N/A: no UI or interaction change. This changes how the daemon reads the macOS process table, not what Orca shows.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
New
darwin-terminal-names.test.ts(9 tests): the translation rewrites only the device column (an argv containing1/2is untouched), keeps unknown devices asmajor/minor, leaves legacy row shapes alone; the index stats onlytty*/console, lets the first entry win, rebuilds for a newly opened terminal, walks only once for a vanished one; the reader skips/devfor captures without terminals and falls back cleanly when/devis unreadable.Real host, macOS 26.6.2: a throwaway test (not committed) ran the real reader against
ps -o tty=. It found 0 mismatches over 1,969 processes, 664 of them with a terminal; capture 356 ms against 6,559 ms for the old argv under the same load.vitest run src/shared src/main/pty src/main/providers src/main/daemon: 1,221 files pass. The 4 tests in 3 files that fail on this machine fail identically on unmodifiedmain:node-pty-fd-leak(native module not built, I installed with--ignore-scripts),local-pty-provider-spawn-sessioncoalescing, and the live zsh startup test.tsc --noEmit -p config/tsconfig.node.jsonpasses;oxlintandoxfmt --checkare clean on the changed files.Linux and Windows: behaviour unchanged. The new path runs only when the host is darwin (
HOST_IS_DARWINfor the argv,process.platform === 'darwin'in the reader).AI Disclosure
Written with Claude Code (Claude Opus 5.5) and reviewed and tested as described above.
Review
HOST_IS_DARWINargv guard and a darwin check in the reader. Linux keepstty=; Windows never runs these argv./dev.ttystrings as before.readdirplus anlstatpertty*entry on first use and when a new terminal appears./deventry metadata only; no new process execution or file writes.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Not addressed here, and possibly worth a follow-up: the snapshot TTL starts after capture completion, so on a host where the capture is slow it runs almost continuously. With this change a capture is short enough that this matters far less.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred): ran oxlint/oxfmt on the changed files, the node typecheck and the affected test directories locally; leaving the full lint, test and build to CI🤖 Generated with Claude Code
https://claude.ai/code/session_01Ewbhm4qQrr8ssHPL4gfo43