fix(runtime): coalesce bulk terminal focus + freeze repro harness - #11841
Conversation
Greptile SummaryIntroduces a generation-aware latest-wins single-flight coalescer (
Confidence Score: 4/5Safe to merge for the bulk-open freeze fix; one correctness hole in the coalescer's success path warrants attention before the change ships under heavy concurrent terminal cycling. The coalescer's pump() discards the result already returned by job.run(ctx) and re-calls resolveSuperseded() when the job was superseded. For the PTY path, resolveSuperseded = livePtyIdentity, which throws terminal_exited if the PTY disconnected after run() started but before resolveSuperseded() is called. In a bulk-focus storm where sessions are simultaneously cycling, both events can land in the same I/O phase — revealTerminalSession resolves and the PTY disconnection is processed — so by the time pump's microtask continuation runs, the PTY is already gone. The superseded job then rejects with terminal_exited instead of resolving with {navigated: false}, which is the wrong outcome for a request that was merely dropped, not failed. The run() callback already returns the correct {navigated: false} result in this situation, so using that result directly closes the hole. The rest of the change — renderer rAF coalescer, CLI message, contract type — is clean. Files Needing Attention: src/main/runtime/terminal-focus-navigation-coalescer.ts lines 106–110: the success path should use the result already returned by run() rather than re-calling resolveSuperseded().
|
| Filename | Overview |
|---|---|
| src/main/runtime/terminal-focus-navigation-coalescer.ts | New generation-aware single-flight coalescer; the success-path calls resolveSuperseded() after run() returns instead of using the already-computed result, which can throw and cause spurious rejections when a PTY disconnects during a storm |
| src/main/runtime/orca-runtime.ts | focusTerminal refactored to route PTY and leaf paths through TerminalFocusNavigationCoalescer with livePtyIdentity/liveLeafIdentity as resolveSuperseded; logic and navigated flag contract look correct |
| src/renderer/src/lib/terminal-focus-ipc-coalescer.ts | RAF-based latest-wins coalescer for IPC storm collapsing on the renderer; dispose/cancel logic and disposed-guard are correct |
| src/renderer/src/hooks/useIpcEvents.ts | Wraps onFocusTerminal in the new IPC coalescer; dispose is pushed to unsubs before the subscription, ensuring clean teardown |
| src/shared/runtime-types.ts | Adds optional navigated?: boolean to RuntimeTerminalFocus; backwards-compatible with existing callers that omit the field |
| src/cli/terminal-format.ts | CLI correctly suppresses Focused message when navigated === false; undefined (old clients) falls through to original message |
| src/main/runtime/terminal-focus-navigation-coalescer.test.ts | Unit tests cover single job, latest-wins drop, storm bound (16 parallel), mid-run supersede, and error propagation; good coverage of the happy paths |
| tests/e2e/helpers/remote-session-bulk-open-oracle.ts | Oracle seeds BULK_OPEN_WORKTREE_COUNT x BULK_OPEN_TABS_PER_WORKTREE remote sessions and measures renderer lag; non-startup tab session records use parentTabId instead of the created tab's own ID |
Sequence Diagram
sequenceDiagram
participant CLI as CLI / Bulk-Open
participant RT as OrcaRuntimeService
participant C as TerminalFocusNavigationCoalescer
participant H as Host Notifier (revealTerminalSession)
participant R as Renderer IPC Coalescer (rAF)
CLI->>RT: focusTerminal(A)
RT->>C: "run({key:A, run:revealA, resolveSuperseded:identityA})"
C->>C: "generation=1, pending=jobA, pump()"
C-->>RT: Promise A
CLI->>RT: focusTerminal(B)
RT->>C: "run({key:B, ...})"
C->>C: "supersede jobA → jobA.resolve(identityA), generation=2, pending=jobB"
Note over C: pump already running — jobB queued
CLI->>RT: focusTerminal(C)
RT->>C: "run({key:C, ...})"
C->>C: "supersede jobB → jobB.resolve(identityB), generation=3, pending=jobC"
C->>H: "revealTerminalSession(A) in-flight gen=1"
H-->>C: "revealed {tabId:tab-a}"
C->>C: "isCurrent()? gen1 != gen3 → navigated:false"
C-->>CLI: "A resolves {navigated:false}"
C->>H: "revealTerminalSession(C) in-flight gen=3"
H-->>C: "revealed {tabId:tab-c}"
C->>C: "isCurrent()? gen3==gen3 → navigated:true"
H->>R: ui:focusTerminal(tabC) via IPC
R->>R: enqueue(tabC) — rAF scheduled
R->>R: next frame flush → activateTab(tabC)
C-->>CLI: "C resolves {navigated:true}"
Reviews (4): Last reviewed commit: "test(freeze): mid-storm status watchdog ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "bench:cold-park-resource": "pnpm run ensure:electron-runtime && node tools/benchmarks/terminal-cold-park-resource-bench.mjs", | ||
| "bench:compare": "node config/scripts/compare-benchmark-artifacts.mjs" | ||
| "bench:compare": "node config/scripts/compare-benchmark-artifacts.mjs", | ||
| "test:e2e:remote-bulk-open-freeze": "pnpm run ensure:electron-runtime && npx playwright test tests/e2e/remote-session-bulk-open-freeze-repro.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1", |
There was a problem hiding this comment.
test:e2e:remote-bulk-open-freeze calls npx playwright test while the Docker-SSH sibling uses pnpm exec playwright test (via run-ssh-docker-bulk-open-freeze-e2e.mjs). In environments where npx doesn't resolve the local binary first it may attempt to download Playwright from the registry, picking up a mismatched version. Prefer pnpm exec for consistency.
| "test:e2e:remote-bulk-open-freeze": "pnpm run ensure:electron-runtime && npx playwright test tests/e2e/remote-session-bulk-open-freeze-repro.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1", | |
| "test:e2e:remote-bulk-open-freeze": "pnpm run ensure:electron-runtime && pnpm exec playwright test tests/e2e/remote-session-bulk-open-freeze-repro.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1", |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
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:
📝 WalkthroughWalkthroughAdded remote freeze metrics, RPC execution, status monitoring, live reproduction scripts, diagnostic reports, package commands, and documentation. Added paired-session and SSH Docker end-to-end freeze tests. Added latest-wins terminal focus coalescing, runtime state revalidation, navigation status reporting, and regression coverage. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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: 6
🧹 Nitpick comments (4)
config/scripts/live-remote-bulk-open-freeze-repro.mjs (1)
333-408: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe scratch-copy failure note never reaches the report.
Line 406 appends to
notes, but the report is already serialized and written at lines 379 and 382. The note is lost. Log the failure to stderr instead, or move the scratch copy above the report write.♻️ Proposed change
} catch (error) { - notes.push(`scratch copy failed: ${String(error).slice(0, 200)}`) + console.warn(`[live-freeze] scratch copy failed: ${String(error).slice(0, 200)}`) }tools/freeze-repro/README.md (1)
62-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument
ORCA_FREEZE_SCRATCH.The harness reads
ORCA_FREEZE_SCRATCHat line 37 ofconfig/scripts/live-remote-bulk-open-freeze-repro.mjsand writes two extra files into that directory. The table omits it.📝 Proposed addition
| `ORCA_FREEZE_CREATE_WT_SPAN` | `16` | How many worktrees to spread creates across | +| `ORCA_FREEZE_SCRATCH` | _(unset)_ | Extra directory for a report copy and an amplification summary |tests/e2e/helpers/remote-session-bulk-open-oracle.ts (1)
51-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared oracle/fixture logic is re-implemented instead of reused by the SSH-Docker (R2) spec. Three separate pieces of logic — shell quoting, the store-interaction probe, and freeze-report build/threshold/write — exist once in the shared modules and once more, duplicated inline, in the SSH-Docker spec. This risks the two topologies' freeze-threshold semantics silently diverging.
tests/e2e/helpers/remote-session-bulk-open-oracle.ts#L51-L70: exportmeasureStoreInteractionMsso it can be imported instead of duplicated.tests/e2e/ssh-docker-bulk-open-freeze-repro.spec.ts#L111-L124: replace the inline interaction-probe block with the exportedmeasureStoreInteractionMs.tests/e2e/helpers/remote-session-bulk-open-oracle.ts#L242-L263: extract the report build/threshold/write sequence into an exported helper parameterized by topology-specific fields (container/worktreeCount).tests/e2e/ssh-docker-bulk-open-freeze-repro.spec.ts#L126-L154: replace the inline report object, threshold checks, and file write with the shared report-builder helper.tests/e2e/helpers/remote-session-bulk-open-fixture.ts#L5-L7: exportshellQuote(or move it to a small shared module) for reuse.tests/e2e/ssh-docker-bulk-open-freeze-repro.spec.ts#L35-L37: import the sharedshellQuoteinstead of redefining it.tests/e2e/remote-session-bulk-open-freeze-repro.spec.ts (1)
116-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the magic number with the actual session-count formula.
toBeGreaterThanOrEqual(8)does not track the real expected session count.seedBulkOpenRemoteSessionscreatesBULK_OPEN_WORKTREE_COUNT * BULK_OPEN_TABS_PER_WORKTREEsessions. Import and use these constants so the assertion tracks the actual seeding logic instead of a stale number.♻️ Proposed fix
-import { - HARD_FREEZE_LAG_MS, - runBulkOpenFreezeOracle, - seedBulkOpenRemoteSessions, - SOFT_FREEZE_LAG_MS -} from './helpers/remote-session-bulk-open-oracle' +import { + BULK_OPEN_TABS_PER_WORKTREE, + BULK_OPEN_WORKTREE_COUNT, + HARD_FREEZE_LAG_MS, + runBulkOpenFreezeOracle, + seedBulkOpenRemoteSessions, + SOFT_FREEZE_LAG_MS +} from './helpers/remote-session-bulk-open-oracle' ... - expect(report.sessionCount).toBeGreaterThanOrEqual(8) - expect(report.worktreeCount).toBe(3) + expect(report.sessionCount).toBeGreaterThanOrEqual( + BULK_OPEN_WORKTREE_COUNT * BULK_OPEN_TABS_PER_WORKTREE + ) + expect(report.worktreeCount).toBe(BULK_OPEN_WORKTREE_COUNT)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec4576c6-f302-46bf-9b16-89b6ad05808e
📒 Files selected for processing (10)
config/scripts/live-remote-bulk-open-freeze-metrics.mjsconfig/scripts/live-remote-bulk-open-freeze-metrics.test.mjsconfig/scripts/live-remote-bulk-open-freeze-repro.mjsconfig/scripts/run-ssh-docker-bulk-open-freeze-e2e.mjspackage.jsontests/e2e/helpers/remote-session-bulk-open-fixture.tstests/e2e/helpers/remote-session-bulk-open-oracle.tstests/e2e/remote-session-bulk-open-freeze-repro.spec.tstests/e2e/ssh-docker-bulk-open-freeze-repro.spec.tstools/freeze-repro/README.md
| const statusProbe = orcaJsonSync(['status'], { local: true }) | ||
| let memoryProbeMs = null | ||
| try { | ||
| const mem = orcaJsonSync(['diagnostics', 'memory'], { local: true, timeoutMs: 120_000 }) | ||
| memoryProbeMs = mem.elapsedMs | ||
| notes.push(`memory diagnostic ms=${mem.elapsedMs.toFixed(0)}`) | ||
| } catch (error) { | ||
| notes.push(`memory diagnostic failed: ${String(error).slice(0, 200)}`) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the status probe so a real freeze still produces a report.
Line 323 calls orcaJsonSync without a try. The probe runs right after the bulk switch, which is the moment the app is most likely to be unresponsive. If the app stalls past the 120s timeout or returns a non-zero status, main() rejects, the process exits 3, and no report is written. The harness then loses the exact data it exists to capture. Treat a failed status probe as a hard freeze signal.
🛡️ Proposed fix
- const statusProbe = orcaJsonSync(['status'], { local: true })
+ let statusProbeMs = 0
+ let statusProbeFailed = false
+ try {
+ statusProbeMs = orcaJsonSync(['status'], { local: true }).elapsedMs
+ } catch (error) {
+ statusProbeFailed = true
+ statusProbeMs = hardMs
+ notes.push(`status probe failed: ${String(error).slice(0, 200)}`)
+ }Then use statusProbeMs at lines 336 and 367, and record statusProbeFailed in the report.
| test('bulk-open many flooding SSH terminals and measure renderer lag @freeze-repro', async ({ | ||
| orcaPage, | ||
| registerPostElectronShutdownCleanup | ||
| }) => { | ||
| test.setTimeout(420_000) | ||
| let target: DockerSshRelayTarget | null = null | ||
| try { | ||
| target = startDockerSshRelayTarget() | ||
| registerPostElectronShutdownCleanup(async () => { | ||
| if (target) { | ||
| cleanupDockerSshRelayTarget(target) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A5 'function startDockerSshRelayTarget' tests/e2e/helpers/docker-ssh-relay-target.ts
rg -n 'startDockerSshRelayTarget\(' -g '*.ts'Repository: stablyai/orca
Length of output: 642
🏁 Script executed:
#!/bin/bash
set -e
printf 'Tracked files matching spec target and sibling:\n'
git ls-files | grep -E '(^|/)ssh-docker-bulk-open-freeze-repro\.spec\.ts$|(^|/)remote-session-bulk-open-freeze-repro\.spec\.ts$' || true
printf '\nTarget file relevant lines:\n'
cat -n tests/e2e/ssh-docker-bulk-open-freeze-repro.spec.ts | sed -n '45,75p'
printf '\nSibling fixture usage:\n'
cat -n tests/e2e/remote-session-bulk-open-freeze-repro.spec.ts | sed -n '45,80p'
printf '\nAll startDockerSshRelayTarget call sites in TS files:\n'
rg -n 'startDockerSshRelayTarget\(' -g '*.ts' || true
printf '\nAll cleanupDockerSshRelayTarget call sites in TS files:\n'
rg -n 'cleanupDockerSshRelayTarget\(' -g '*.ts' || trueRepository: stablyai/orca
Length of output: 3145
Pass testInfo to startDockerSshRelayTarget.
startDockerSshRelayTarget(testInfo: TestInfo) dereferences testInfo.workerIndex for the Docker container name, but this test only destructures orcaPage and registerPostElectronShutdownCleanup, then calls it with no argument. Add testInfo to the fixture destructuring and pass it on line 62.
🐛 Proposed fix
test('bulk-open many flooding SSH terminals and measure renderer lag `@freeze-repro`', async ({
orcaPage,
- registerPostElectronShutdownCleanup
+ registerPostElectronShutdownCleanup,
+ testInfo
}) => {
test.setTimeout(420_000)
let target: DockerSshRelayTarget | null = null
try {
- target = startDockerSshRelayTarget()
+ target = startDockerSshRelayTarget(testInfo)📝 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.
| test('bulk-open many flooding SSH terminals and measure renderer lag @freeze-repro', async ({ | |
| orcaPage, | |
| registerPostElectronShutdownCleanup | |
| }) => { | |
| test.setTimeout(420_000) | |
| let target: DockerSshRelayTarget | null = null | |
| try { | |
| target = startDockerSshRelayTarget() | |
| registerPostElectronShutdownCleanup(async () => { | |
| if (target) { | |
| cleanupDockerSshRelayTarget(target) | |
| } | |
| }) | |
| test('bulk-open many flooding SSH terminals and measure renderer lag `@freeze-repro`', async ({ | |
| orcaPage, | |
| registerPostElectronShutdownCleanup, | |
| testInfo | |
| }) => { | |
| test.setTimeout(420_000) | |
| let target: DockerSshRelayTarget | null = null | |
| try { | |
| target = startDockerSshRelayTarget(testInfo) | |
| registerPostElectronShutdownCleanup(async () => { | |
| if (target) { | |
| cleanupDockerSshRelayTarget(target) | |
| } | |
| }) |
| const report = { | ||
| topology: 'docker-ssh' as const, | ||
| sessionCount: SESSION_SPLITS, | ||
| hiddenFloodMaxLagMs, | ||
| bulkOpenMaxLagMs, | ||
| interactionProbeMs, | ||
| softFreeze: | ||
| bulkOpenMaxLagMs >= SOFT_FREEZE_LAG_MS || interactionProbeMs >= SOFT_FREEZE_LAG_MS, | ||
| hardFreeze: | ||
| bulkOpenMaxLagMs >= HARD_FREEZE_LAG_MS || interactionProbeMs >= HARD_FREEZE_LAG_MS, | ||
| container: target.containerName, | ||
| remoteHostStillStreaming: true | ||
| } | ||
|
|
||
| const { mkdirSync, writeFileSync } = await import('node:fs') | ||
| mkdirSync(REPORT_DIR, { recursive: true }) | ||
| writeFileSync( | ||
| path.join(REPORT_DIR, 'bulk-open-freeze-docker-ssh.json'), | ||
| `${JSON.stringify(report, null, 2)}\n` | ||
| ) | ||
| // eslint-disable-next-line no-console | ||
| console.log('[freeze-repro R2]', JSON.stringify(report, null, 2)) | ||
|
|
||
| // Host still producing frames (Brandon/Tim class: host alive, client stuck). | ||
| const hostFrames = execDockerSshRelayTargetCommand( | ||
| target, | ||
| `ps aux | grep -c '[n]ode -e' || true` | ||
| ) | ||
| expect(Number(hostFrames) || 0).toBeGreaterThan(0) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
remoteHostStillStreaming is hardcoded, not derived from the actual check.
The report sets remoteHostStillStreaming: true as a literal at line 137, before the real check (hostFrames / expect(...).toBeGreaterThan(0)) runs at lines 150-154. The report is written to disk and logged at lines 140-148, before that verification executes. If the host stopped streaming, the persisted JSON still records remoteHostStillStreaming: true, corrupting the diagnostic value of the artifact for exactly the failure this field is meant to catch. Compute the check first and derive the field from its result.
🐛 Proposed fix
+ // Host still producing frames (Brandon/Tim class: host alive, client stuck).
+ const hostFrames = execDockerSshRelayTargetCommand(
+ target,
+ `ps aux | grep -c '[n]ode -e' || true`
+ )
+ const remoteHostStillStreaming = (Number(hostFrames) || 0) > 0
+
const report = {
topology: 'docker-ssh' as const,
sessionCount: SESSION_SPLITS,
hiddenFloodMaxLagMs,
bulkOpenMaxLagMs,
interactionProbeMs,
softFreeze:
bulkOpenMaxLagMs >= SOFT_FREEZE_LAG_MS || interactionProbeMs >= SOFT_FREEZE_LAG_MS,
hardFreeze:
bulkOpenMaxLagMs >= HARD_FREEZE_LAG_MS || interactionProbeMs >= HARD_FREEZE_LAG_MS,
container: target.containerName,
- remoteHostStillStreaming: true
+ remoteHostStillStreaming
}
const { mkdirSync, writeFileSync } = await import('node:fs')
mkdirSync(REPORT_DIR, { recursive: true })
writeFileSync(
path.join(REPORT_DIR, 'bulk-open-freeze-docker-ssh.json'),
`${JSON.stringify(report, null, 2)}\n`
)
// eslint-disable-next-line no-console
console.log('[freeze-repro R2]', JSON.stringify(report, null, 2))
- // Host still producing frames (Brandon/Tim class: host alive, client stuck).
- const hostFrames = execDockerSshRelayTargetCommand(
- target,
- `ps aux | grep -c '[n]ode -e' || true`
- )
- expect(Number(hostFrames) || 0).toBeGreaterThan(0)
+ expect(remoteHostStillStreaming).toBe(true)📝 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 report = { | |
| topology: 'docker-ssh' as const, | |
| sessionCount: SESSION_SPLITS, | |
| hiddenFloodMaxLagMs, | |
| bulkOpenMaxLagMs, | |
| interactionProbeMs, | |
| softFreeze: | |
| bulkOpenMaxLagMs >= SOFT_FREEZE_LAG_MS || interactionProbeMs >= SOFT_FREEZE_LAG_MS, | |
| hardFreeze: | |
| bulkOpenMaxLagMs >= HARD_FREEZE_LAG_MS || interactionProbeMs >= HARD_FREEZE_LAG_MS, | |
| container: target.containerName, | |
| remoteHostStillStreaming: true | |
| } | |
| const { mkdirSync, writeFileSync } = await import('node:fs') | |
| mkdirSync(REPORT_DIR, { recursive: true }) | |
| writeFileSync( | |
| path.join(REPORT_DIR, 'bulk-open-freeze-docker-ssh.json'), | |
| `${JSON.stringify(report, null, 2)}\n` | |
| ) | |
| // eslint-disable-next-line no-console | |
| console.log('[freeze-repro R2]', JSON.stringify(report, null, 2)) | |
| // Host still producing frames (Brandon/Tim class: host alive, client stuck). | |
| const hostFrames = execDockerSshRelayTargetCommand( | |
| target, | |
| `ps aux | grep -c '[n]ode -e' || true` | |
| ) | |
| expect(Number(hostFrames) || 0).toBeGreaterThan(0) | |
| // Host still producing frames (Brandon/Tim class: host alive, client stuck). | |
| const hostFrames = execDockerSshRelayTargetCommand( | |
| target, | |
| `ps aux | grep -c '[n]ode -e' || true` | |
| ) | |
| const remoteHostStillStreaming = (Number(hostFrames) || 0) > 0 | |
| const report = { | |
| topology: 'docker-ssh' as const, | |
| sessionCount: SESSION_SPLITS, | |
| hiddenFloodMaxLagMs, | |
| bulkOpenMaxLagMs, | |
| interactionProbeMs, | |
| softFreeze: | |
| bulkOpenMaxLagMs >= SOFT_FREEZE_LAG_MS || interactionProbeMs >= SOFT_FREEZE_LAG_MS, | |
| hardFreeze: | |
| bulkOpenMaxLagMs >= HARD_FREEZE_LAG_MS || interactionProbeMs >= HARD_FREEZE_LAG_MS, | |
| container: target.containerName, | |
| remoteHostStillStreaming | |
| } | |
| const { mkdirSync, writeFileSync } = await import('node:fs') | |
| mkdirSync(REPORT_DIR, { recursive: true }) | |
| writeFileSync( | |
| path.join(REPORT_DIR, 'bulk-open-freeze-docker-ssh.json'), | |
| `${JSON.stringify(report, null, 2)}\n` | |
| ) | |
| // eslint-disable-next-line no-console | |
| console.log('[freeze-repro R2]', JSON.stringify(report, null, 2)) | |
| expect(remoteHostStillStreaming).toBe(true) |
nwparker
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
config/scripts/live-remote-bulk-open-freeze-metrics.mjs (1)
94-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit that classifies a freeze from probe latency alone.
evaluateFreezeSignalsclassifies based onstatusProbeMsandmemoryProbeMs, so the doc/comment “peaks include probes” is accurate. However, neitherevaluateFreezeSignalsnorevaluateRealisticFreezeSignalshas a focused regression for a high probe value when all latency inputs are below the threshold.config/scripts/live-remote-realistic-freeze-repro.mjs (1)
52-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated harness helpers into a shared module.
orcaJsonSync,orcaJsonAsync,mapPool, andsampleOrcaIfPossibleare defined in bothconfig/scripts/live-remote-bulk-open-freeze-repro.mjsandconfig/scripts/live-remote-realistic-freeze-repro.mjs. Move these helpers into a concrete shared module underconfig/scripts, and have each harness import from it.src/renderer/src/lib/terminal-focus-ipc-coalescer.ts (1)
33-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap
apply(next)in a try/catch.
flush()callsapply(next)without error handling. Ifapplythrows, the exception escapes the microtask with no handler, sincequeueMicrotaskcallbacks are not part of a Promise chain that could catch it.The internal state (
pending,scheduled) is already reset before the call, so the coalescer itself stays consistent. The uncaught exception still surfaces unhandled and could produce unexpected console errors or trigger a global error handler in the renderer.The main-process counterpart (
terminal-focus-navigation-coalescer.ts) wraps its equivalent call (job.run()) in try/catch and routes errors throughjob.reject. Apply the same defensive pattern here for consistency.🛡️ Proposed fix
const flush = (): void => { scheduled = false if (disposed) { pending = null return } const next = pending pending = null if (next) { - apply(next) + try { + apply(next) + } catch (error) { + console.error('terminal-focus-ipc-coalescer: apply failed', error) + } } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e784a08d-92f6-4496-b0fd-eb229d1ee853
📒 Files selected for processing (12)
config/scripts/live-remote-bulk-open-freeze-metrics.mjsconfig/scripts/live-remote-bulk-open-freeze-metrics.test.mjsconfig/scripts/live-remote-realistic-freeze-repro.mjspackage.jsonsrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/terminal-focus-navigation-coalescer.test.tssrc/main/runtime/terminal-focus-navigation-coalescer.tssrc/renderer/src/hooks/useIpcEvents.tssrc/renderer/src/lib/terminal-focus-ipc-coalescer.test.tssrc/renderer/src/lib/terminal-focus-ipc-coalescer.tstools/freeze-repro/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- tools/freeze-repro/README.md
| const envName = process.env.ORCA_FREEZE_ENV || 'awin' | ||
| const scenario = process.env.ORCA_FREEZE_SCENARIO || 'idle-backlog-open' | ||
| const createCount = Math.max(0, Number(process.env.ORCA_FREEZE_CREATE || '8')) | ||
| const openCount = Math.max(2, Number(process.env.ORCA_FREEZE_OPEN_COUNT || '20')) | ||
| const idleMs = Math.max(0, Number(process.env.ORCA_FREEZE_IDLE_MS || '45000')) | ||
| const paceMs = Math.max(0, Number(process.env.ORCA_FREEZE_PACE_MS || '250')) | ||
| const paceJitterMs = Math.max(0, Number(process.env.ORCA_FREEZE_PACE_JITTER_MS || '150')) | ||
| const createWorktreeSpan = Math.max(1, Number(process.env.ORCA_FREEZE_CREATE_WT_SPAN || '12')) | ||
| const softMs = Number(process.env.ORCA_FREEZE_SOFT_MS || DEFAULT_SOFT_MS) | ||
| const hardMs = Number(process.env.ORCA_FREEZE_HARD_MS || DEFAULT_HARD_MS) | ||
| const scratchDir = process.env.ORCA_FREEZE_SCRATCH || '' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a NaN guard for numeric env vars.
Number(process.env.X || 'default') returns NaN for a malformed value (e.g. a typo in ORCA_FREEZE_OPEN_COUNT). Math.max(2, NaN) returns NaN, and NaN then flows into Math.min(openCount, openTargets.length) (Line 327) and .slice(0, NaN), which yields an empty openList. The harness would then silently skip the entire "human-paced sequential open" phase and still write a report, without ever telling the operator that the configuration was invalid.
Validate the parsed number before falling back to a default, or throw when the env var is set but not numeric.
🛡️ Proposed guard
+function envInt(name, fallback) {
+ const raw = process.env[name]
+ if (raw === undefined || raw === '') return fallback
+ const n = Number(raw)
+ if (!Number.isFinite(n)) {
+ throw new Error(`Invalid ${name}=${raw}: expected a number`)
+ }
+ return n
+}
+
-const createCount = Math.max(0, Number(process.env.ORCA_FREEZE_CREATE || '8'))
-const openCount = Math.max(2, Number(process.env.ORCA_FREEZE_OPEN_COUNT || '20'))
-const idleMs = Math.max(0, Number(process.env.ORCA_FREEZE_IDLE_MS || '45000'))
-const paceMs = Math.max(0, Number(process.env.ORCA_FREEZE_PACE_MS || '250'))
-const paceJitterMs = Math.max(0, Number(process.env.ORCA_FREEZE_PACE_JITTER_MS || '150'))
-const createWorktreeSpan = Math.max(1, Number(process.env.ORCA_FREEZE_CREATE_WT_SPAN || '12'))
+const createCount = Math.max(0, envInt('ORCA_FREEZE_CREATE', 8))
+const openCount = Math.max(2, envInt('ORCA_FREEZE_OPEN_COUNT', 20))
+const idleMs = Math.max(0, envInt('ORCA_FREEZE_IDLE_MS', 45000))
+const paceMs = Math.max(0, envInt('ORCA_FREEZE_PACE_MS', 250))
+const paceJitterMs = Math.max(0, envInt('ORCA_FREEZE_PACE_JITTER_MS', 150))
+const createWorktreeSpan = Math.max(1, envInt('ORCA_FREEZE_CREATE_WT_SPAN', 12))| export type TerminalFocusIpcPayload = { | ||
| tabId: string | ||
| worktreeId: string | ||
| leafId?: string | null | ||
| ackPaneKeyOnSuccess?: string | ||
| flashFocusedPane?: boolean | ||
| scrollToBottomIfOutputSinceLastView?: boolean | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace ackPaneKeyOnSuccess and related payload fields from producer to consumer.
set -euo pipefail
rg -nP -C5 '\backPaneKeyOnSuccess\b' --type=ts
rg -nP -C5 '\bflashFocusedPane\b|\bscrollToBottomIfOutputSinceLastView\b' --type=ts
rg -nP -C8 'createTerminalFocusIpcCoalescer' --type=tsRepository: stablyai/orca
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
echo "tracked files matching terminal-focus-ipc-coalescer:"
git ls-files | rg 'terminal-focus-ipc-coalescer|useIpcEvents|IpcEvents|ipc|terminal' || true
echo
echo "repo ts files count:"
git ls-files '*.ts' '*.tsx' | wc -l
echo
echo "search ackPaneKeyOnSuccess all tracked files:"
rg -n -C 5 'ackPaneKeyOnSuccess|flashFocusedPane|scrollToBottomIfOutputSinceLastView|createTerminalFocusIpcCoalescer' . || trueRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="src/renderer/src/lib/terminal-focus-ipc-coalescer.ts"
echo "file exists: $(test -f "$target" && echo yes || echo no)"
if test -f "$target"; then
echo "--- $target lines 1-90 ---"
cat -n "$target" | sed -n '1,90p'
fi
echo
echo "--- precise identifier search in tracked files (max 60 matches) ---"
rg -n -C 3 'ackPaneKeyOnSuccess|flashFocusedPane|scrollToBottomIfOutputSinceLastView|createTerminalFocusIpcCoalescer|terminalFocusIpc' $(git ls-files) | head -n 240Repository: stablyai/orca
Length of output: 24092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- src/renderer/src/hooks/useIpcEvents.ts lines 1790-1835 ---"
cat -n src/renderer/src/hooks/useIpcEvents.ts | sed -n '1790,1835p'
echo
echo "--- producers that pass ackPaneKeyOnSuccess/flashFocusedPane/scrollToBottomIfOutputSinceLastView ---"
python3 - <<'PY'
import subprocess
files = subprocess.check_output(['git','ls-files'], text=True).splitlines()
needle = ['ackPaneKeyOnSuccess', 'flashFocusedPane', 'scrollToBottomIfOutputSinceLastView']
for f in files:
if not f.endswith(('.ts','.tsx','.js','.jsx')):
continue
txt = open(f, encoding='utf-8', errors='replace').read()
if any(n in txt for n in needle):
print(f"\n-- {f} --")
lines = txt.splitlines()
for i,l in enumerate(lines,1):
if any(n in l for n in needle):
start=max(1,i-2); end=min(len(lines),i+3)
for j in range(start,end+1):
print(f"{j}: {lines[j-1]}")
PY
echo
echo "--- source search window.addEventListener focus listeners mentioning coalescer/payload fields ---"
rg -n -C 4 'window\.dispatchEvent|addEventListener|FOCUS_TERMINAL_PANE_EVENT|focusTerminal' src/renderer/src/preload src/preload src/main src/renderer/src | head -n 240Repository: stablyai/orca
Length of output: 43774
Keep the per-request ack fields in the coalesced payload if callers wait for them.
ackPaneKeyOnSuccess currently identifies one request that expects acknowledgeAgents([paneKey]) after its pane resolves. enqueue() overwrites pending, so multiple same-turn focus requests can discard one caller’s ack field before apply() runs. Add a test for superseded ack values, or change the API so the request does not expect delivery just because it was enqueued.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/freeze-repro/README.md (2)
65-76: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the full-app scenario knobs.
The table omits
ORCA_FREEZE_STORM_PARALLELandORCA_FREEZE_FOREVER_WINDOW_MS, although the full-app command sets both at Lines 120-121. Add their defaults and meanings, or state that this table covers only common knobs.
147-155: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the generic “Exit codes (both harnesses)” table.
The README is self-contained and documents realistic codes
0,1,2,3,4, and5. The final0-3table omits codes4and5while claiming it applies to both harnesses.
🧹 Nitpick comments (3)
config/scripts/live-remote-bulk-open-freeze-metrics.mjs (1)
203-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead timestamp-less fallback block.
The loop computes a consecutive unhealthy
runcount intolongest, then Line 215 resetslongest = 0unconditionally. The whole block has no effect on the result. The comment on Line 203 also describes a "count * assumed interval" fallback that the code does not implement.Delete the block, or implement the interval-based fallback and pass the sampling interval in.
Note the related edge case in the same area: if samples arrive without
tMs, everyrunStartandendbase becomes0, solongestbecomes the maximum single-samplemsrather than a continuous window. A single slow probe could then satisfylongest >= foreverWindowMs.startStatusWatchdogalways setstMs, so this path is only reachable for hand-built samples.♻️ Proposed change: drop the no-op block
- // If timestamps missing, fall back to consecutive unhealthy count * assumed interval. - if (longest === 0 && unhealthy.some((s) => s.unhealthy)) { - let run = 0 - for (const s of unhealthy) { - if (s.unhealthy) { - run += 1 - longest = Math.max(longest, run) - } else { - run = 0 - } - } - // Without wall clock, consecutive count alone is not a ms window. - longest = 0 - } -config/scripts/live-remote-realistic-freeze-rpc.mjs (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
result.errorin the synchronous failure message.
spawnSyncreports launch failures and timeouts throughresult.error, withstatus === nullandstderr === null. In that case the thrown message readsfailed (null): null, which hides the real cause (for exampleENOENTwhenorcais not onPATH, orETIMEDOUT). BothorcaJsonSynccalls run at harness startup, so operators see this message first.♻️ Proposed change
- if (result.status !== 0) { + if (result.error || result.status !== 0) { throw new Error( - `orca ${args.join(' ')} failed (${result.status}): ${result.stderr || result.stdout}` + `orca ${args.join(' ')} failed (status=${result.status}${ + result.error ? `, error=${String(result.error)}` : '' + }): ${result.stderr || result.stdout || ''}` ) }config/scripts/live-remote-status-watchdog.mjs (1)
7-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe documented
localoption has no effect.The JSDoc declares
local, butprobealways runsorca status --jsonwithout--environment, so it always measures the local app. A caller that passeslocal: falseto sample the remote host gets local samples instead.evaluateFullAppFreezethen classifies the wrong process.Either implement the option or remove it from the JSDoc.
♻️ Proposed change: implement the option
/** - * `@param` {{ intervalMs?: number, timeoutMs?: number, local?: boolean }} opts + * `@param` {{ intervalMs?: number, timeoutMs?: number, envName?: string }} opts */ export function startStatusWatchdog(opts = {}) { const intervalMs = opts.intervalMs ?? 2000 const timeoutMs = opts.timeoutMs ?? 30_000 + const statusArgs = opts.envName + ? ['status', '--environment', opts.envName, '--json'] + : ['status', '--json']
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb84e949-0900-4379-957f-ae0c7c43c21b
📒 Files selected for processing (16)
config/scripts/live-remote-bulk-open-freeze-metrics.mjsconfig/scripts/live-remote-bulk-open-freeze-metrics.test.mjsconfig/scripts/live-remote-realistic-freeze-repro.mjsconfig/scripts/live-remote-realistic-freeze-rpc.mjsconfig/scripts/live-remote-status-watchdog.mjsconfig/scripts/live-remote-status-watchdog.test.mjssrc/cli/terminal-format.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/terminal-focus-navigation-coalescer.test.tssrc/main/runtime/terminal-focus-navigation-coalescer.tssrc/renderer/src/hooks/useIpcEvents.tssrc/renderer/src/lib/terminal-focus-ipc-coalescer.test.tssrc/renderer/src/lib/terminal-focus-ipc-coalescer.tssrc/shared/runtime-types.tstools/freeze-repro/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/runtime/orca-runtime.test.ts
- src/renderer/src/hooks/useIpcEvents.ts
- src/main/runtime/orca-runtime.ts
| const child = spawn('orca', ['status', '--json'], { | ||
| stdio: ['ignore', 'pipe', 'pipe'] | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find existing spawn patterns for the orca CLI and any Windows-specific resolution helper.
set -uo pipefail
echo '--- spawn/spawnSync/execFile calls naming the orca CLI ---'
rg -nP -C3 "spawn(Sync)?\(\s*['\"]orca" --glob 'config/**' --glob 'tests/**' --glob 'src/**'
echo '--- existing shell:true / win32 / PATHEXT handling around child_process ---'
rg -nP -C3 "shell:\s*(true|process\.platform)|win32|PATHEXT|\.cmd['\"]" --glob 'config/**' --glob 'src/**' | head -80
echo '--- documented environments for the freeze harness ---'
fd -i 'README.md' tools/freeze-repro --exec rg -n -i 'windows|awin|orca (must|binary|cli)|PATH'Repository: stablyai/orca
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo '--- top-level tracked files/dirs ---'
git ls-files | sed -n '1,200p'
echo '--- locate target files ---'
fd -a 'live-remote-status-watchdog\.mjs|live-remote-realistic-freeze-rpc\.mjs|README\.md' . | sed -n '1,200p'
echo '--- target file stats ---'
while IFS= read -r f; do
[ -f "$f" ] || continue
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,220p' "$f"
done < <(fd 'live-remote-status-watchdog\.mjs|live-remote-realistic-freeze-rpc\.mjs' .)
echo '--- search for invoke helper and environment names ---'
rg -n -C3 -i "orca|status-watchdog|freeze-rpc|evaluateFullAppFreeze|foreverUiLockupObserved|awin|windows" --glob '!**/node_modules/**' .Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo '--- locate exact target files ---'
git ls-files | grep -E '(^|/)live-remote-(status-watchdog|realistic-freeze-rpc)\.mjs$' || true
echo '--- target line counts ---'
git ls-files | grep -E '(^|/)live-remote-(status-watchdog|realistic-freeze-rpc)\.mjs$' | while IFS= read -r f; do
echo "$f: $(wc -l < "$f")"
done
echo '--- target contents ---'
git ls-files | grep -E '(^|/)live-remote-(status-watchdog|realistic-freeze-rpc)\.mjs$' | while IFS= read -r f; do
echo "===== $f ====="
cat -n "$f"
done
echo '--- focused references ---'
rg -n -C3 -i 'evaluateFullAppFreeze|foreverUiLockupObserved|live-remote|orca status|spawn\(.*orca|spawnSync\(.*orca|awin|windows|runtime|spawnOptions|shell:\s*process\.platform|PATHEXT' . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | sed -n '1,260p'
echo '--- Node child_process resolvePath behavior probe ---'
node - <<'JS'
const cp = require('child_process');
const os = require('os');
const path = require('path');
console.log(JSON.stringify({
platform: os.platform(),
hasSpawn: typeof cp.spawn,
hasResolve: typeof cp.resolve,
resolveOnCurrentPathExeOnly: process.platform === 'win32'
? path.extname(process.execPath)
: undefined,
nodeVersion: process.version
}, null, 2));
JSRepository: stablyai/orca
Length of output: 32598
🌐 Web query:
Node.js child_process spawn Windows PATHEXT extension shell option documentation
💡 Result:
In Node.js, the child_process.spawn method does not support the Windows PATHEXT environment variable [1][2][3]. When attempting to execute a file on Windows using spawn, you must specify the full filename—including its extension—if it is not an executable (.exe) file [1][2]. For example, you must use spawn('script.bat') rather than spawn('script'), even if.bat is included in the system's PATHEXT variable [2]. The shell option in spawn provides a way to work around this limitation by executing the command through a shell [4][5][6]. When shell is set to true, Node.js runs the command using the shell specified by process.env.ComSpec (typically cmd.exe on Windows) [4][7][8]. Because the shell itself handles command lookup and resolution, it respects the system's PATHEXT settings and allows you to run files without explicitly providing their extension [1][2]. However, using the shell option has important implications: - Security: Enabling shell: true makes the application vulnerable to command injection if the command or arguments contain unsanitized user input [4][5][7]. - Performance: Spawning a shell adds overhead compared to directly executing a process [2][9]. - Alternatives: It is generally recommended to either provide the full path and extension of the executable or to use platform-specific logic to append the correct extension (e.g., checking if the platform is 'win32' and appending.cmd or.bat) when spawning commands cross-platform [2][9]. Many developers prefer using libraries like execa, which handle these cross-platform nuances automatically [2].
Citations:
- 1: child_process.spawn ignores PATHEXT on Windows nodejs/node#6671
- 2: what's broken in spawn? bcoe/awesome-cross-platform-nodejs#26
- 3: child_process.spawn ignores PATHEXT on Windows nodejs/node-v0.x-archive#2318
- 4: https://nodejs.org/api/child_process.html
- 5: https://nodejs.org/dist/latest/docs/api/child_process.html
- 6: https://beta.docs.nodejs.org/child_process.html
- 7: https://github.com/nodejs/node/blob/master/doc/api/child_process.md
- 8: https://github.com/nodejs/node/blob/main/doc/api/child_process.md
- 9: Executing npm by node's spawn on windows is really slow, it takes minutes npm/npm#21148
Resolve the orca executable before spawning it. Node’s spawn/spawnSync does not apply PATHEXT on Windows, so a bare orca shim such as arca.cmd cannot launch unless shell: true or an explicit orca.exe/resolved path is used.
config/scripts/live-remote-status-watchdog.mjs#L21-L23: resolveorcaonce before spawning, and bubbleENOENTas a startup failure instead of recording an unhealthy{ ok: false, error: true }sample.config/scripts/live-remote-realistic-freeze-rpc.mjsbothspawnSyncand barespawncalls require the same resolution approach.
📍 Affects 2 files
config/scripts/live-remote-status-watchdog.mjs#L21-L23(this comment)config/scripts/live-remote-realistic-freeze-rpc.mjs#L9-L17config/scripts/live-remote-realistic-freeze-rpc.mjs#L34-L38
Source: Linters/SAST tools
| it('collects status samples and stops cleanly', async () => { | ||
| // Real path: actually invokes `orca status --json` (must be available in CI/dev with Orca or fail soft). | ||
| const watch = startStatusWatchdog({ intervalMs: 50, timeoutMs: 5_000 }) | ||
| await new Promise((r) => setTimeout(r, 180)) | ||
| const result = await watch.stop() | ||
| expect(result.samples.length).toBeGreaterThanOrEqual(1) | ||
| expect(result.durationMs).toBeGreaterThan(0) | ||
| for (const s of result.samples) { | ||
| expect(typeof s.ms).toBe('number') | ||
| expect(typeof s.ok).toBe('boolean') | ||
| expect(typeof s.hang).toBe('boolean') | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
This unit test spawns the real orca binary and can exceed the default test timeout.
startStatusWatchdog spawns orca status --json immediately. If a probe is still in flight after the 180ms wait, stop() polls until timeoutMs + 1000 (6s here) and then runs one more forced probe. Total runtime can pass Vitest's default 5s test timeout, so the test fails on a loaded machine or when orca status reaches a remote host.
Mock node:child_process so the test is deterministic, or set an explicit timeout above timeoutMs + 1000.
🐛 Proposed change: bound the test explicitly
- const watch = startStatusWatchdog({ intervalMs: 50, timeoutMs: 5_000 })
+ const watch = startStatusWatchdog({ intervalMs: 50, timeoutMs: 1_000 })
await new Promise((r) => setTimeout(r, 180))
const result = await watch.stop()
@@
- })
+ }, 15_000)📝 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.
| it('collects status samples and stops cleanly', async () => { | |
| // Real path: actually invokes `orca status --json` (must be available in CI/dev with Orca or fail soft). | |
| const watch = startStatusWatchdog({ intervalMs: 50, timeoutMs: 5_000 }) | |
| await new Promise((r) => setTimeout(r, 180)) | |
| const result = await watch.stop() | |
| expect(result.samples.length).toBeGreaterThanOrEqual(1) | |
| expect(result.durationMs).toBeGreaterThan(0) | |
| for (const s of result.samples) { | |
| expect(typeof s.ms).toBe('number') | |
| expect(typeof s.ok).toBe('boolean') | |
| expect(typeof s.hang).toBe('boolean') | |
| } | |
| }) | |
| it('collects status samples and stops cleanly', async () => { | |
| // Real path: actually invokes `orca status --json` (must be available in CI/dev with Orca or fail soft). | |
| const watch = startStatusWatchdog({ intervalMs: 50, timeoutMs: 1_000 }) | |
| await new Promise((r) => setTimeout(r, 180)) | |
| const result = await watch.stop() | |
| expect(result.samples.length).toBeGreaterThanOrEqual(1) | |
| expect(result.durationMs).toBeGreaterThan(0) | |
| for (const s of result.samples) { | |
| expect(typeof s.ms).toBe('number') | |
| expect(typeof s.ok).toBe('boolean') | |
| expect(typeof s.hang).toBe('boolean') | |
| } | |
| }, 15_000) |
| | Contract | `RuntimeTerminalFocus.navigated?: boolean` — `false` when superseded / nav skipped | | ||
|
|
||
| **In scope:** concurrent `terminal.focus` / bulk-switch storms. | ||
| **Residual:** sequential soft freezes; reconnect/wake metadata storms — need cheaper activation + scan bounding, not only focus coalescing. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
List all residual focus-related cases.
The residual scope omits abortable reveals and certain activation paths. The same README records Terminal reveal timed out under fan-out at Line 98. Add these cases so the product-fix scope is complete.
a162009 to
543559d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
config/scripts/live-remote-freeze-rpc.mjs (1)
47-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the synchronous JSON parse for consistent error context.
orcaJsonAsyncreports parse failures with the command and a stdout excerpt (Lines 129-135).orcaJsonSyncthrows a bareSyntaxError, so a non-JSON CLI response gives no context in harness logs.♻️ Proposed change
- const parsed = JSON.parse(result.stdout) + let parsed + try { + parsed = JSON.parse(result.stdout) + } catch (error) { + throw new Error( + `${cliCommand} ${args.join(' ')} parse failed: ${String(error)}; stdout=${result.stdout.slice(0, 400)}` + ) + } if (parsed.ok === false) {config/scripts/live-remote-freeze-rpc.test.mjs (1)
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a multi-byte case for the byte limit.
The current input is ASCII, so
Buffer.byteLength(chunk)andchunk.lengthreturn the same value. A multi-byte chunk would lock in the byte-based accounting.♻️ Proposed addition
const overflow = appendOrcaRpcOutput(first.output, '67', first.bytes, 5) expect(overflow).toEqual({ output: '1234', bytes: 6, exceeded: true }) + + // '€' is 3 bytes in UTF-8. + expect(appendOrcaRpcOutput('', '€', 0, 2)).toEqual({ output: '', bytes: 3, exceeded: true }) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 041f4b9e-dcfd-4c0b-89bc-24d239d3903c
📒 Files selected for processing (12)
config/scripts/live-remote-bulk-open-freeze-metrics.mjsconfig/scripts/live-remote-bulk-open-freeze-metrics.test.mjsconfig/scripts/live-remote-bulk-open-freeze-repro.mjsconfig/scripts/live-remote-freeze-rpc.mjsconfig/scripts/live-remote-freeze-rpc.test.mjsconfig/scripts/live-remote-realistic-freeze-repro.mjsconfig/scripts/live-remote-status-watchdog.mjsconfig/scripts/live-remote-status-watchdog.test.mjsconfig/scripts/run-ssh-docker-bulk-open-freeze-e2e.mjspackage.jsonsrc/cli/terminal-format.test.tssrc/cli/terminal-format.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- config/scripts/live-remote-bulk-open-freeze-metrics.test.mjs
- package.json
- src/cli/terminal-format.ts
- config/scripts/live-remote-bulk-open-freeze-repro.mjs
- config/scripts/live-remote-realistic-freeze-repro.mjs
6c91633 to
b16334c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/e2e/remote-session-bulk-open-freeze-repro.spec.ts (1)
185-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReference the shared worktree-count constant instead of a magic number.
expect(report.worktreeCount).toBe(3)hardcodes the expected worktree count.remote-session-bulk-open-oracle.tsdefines the worktree count used byseedBulkOpenRemoteSessions. If that constant changes, this assertion silently diverges and produces a confusing failure instead of tracking the source of truth.Import and compare against the constant instead.
tests/e2e/helpers/headless-paired-runtime-host.unit.test.ts (1)
48-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative-path coverage for
parseHeadlessPairedRuntimePairingOffer.The new tests cover only the two success paths. The parser also returns
nullfor invalid JSON, a wrongtype,pairing.available !== true, and a non-stringpairing.url. This function gates the pairing handshake thatreadPairingOfferdepends on, so covering the rejection paths is worth the small effort.✅ Suggested additional test cases
it('rejects invalid JSON', () => { expect(parseHeadlessPairedRuntimePairingOffer('not-json')).toBeNull() }) it('rejects a mismatched readiness type', () => { expect( parseHeadlessPairedRuntimePairingOffer( JSON.stringify({ type: 'other', pairing: { available: true, url: 'orca://pairing-secret' } }) ) ).toBeNull() }) it('rejects unavailable pairing', () => { expect( parseHeadlessPairedRuntimePairingOffer( JSON.stringify({ type: 'orca_server_ready', pairing: { available: false, url: 'orca://pairing-secret' } }) ) ).toBeNull() }) it('rejects a non-string pairing URL', () => { expect( parseHeadlessPairedRuntimePairingOffer( JSON.stringify({ type: 'orca_server_ready', pairing: { available: true, url: 123 } }) ) ).toBeNull() })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e25101d0-5a08-4529-a1e5-026179e4d834
📒 Files selected for processing (25)
config/scripts/live-remote-bulk-open-freeze-metrics.mjsconfig/scripts/live-remote-bulk-open-freeze-metrics.test.mjsconfig/scripts/live-remote-bulk-open-freeze-repro.mjsconfig/scripts/live-remote-freeze-rpc.mjsconfig/scripts/live-remote-freeze-rpc.test.mjsconfig/scripts/live-remote-realistic-freeze-repro.mjsconfig/scripts/live-remote-status-watchdog.mjsconfig/scripts/live-remote-status-watchdog.test.mjsconfig/scripts/run-ssh-docker-bulk-open-freeze-e2e.mjspackage.jsonsrc/cli/terminal-format.test.tssrc/cli/terminal-format.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/terminal-focus-navigation-coalescer.test.tssrc/main/runtime/terminal-focus-navigation-coalescer.tssrc/shared/runtime-types.tstests/e2e/helpers/headless-paired-runtime-host.tstests/e2e/helpers/headless-paired-runtime-host.unit.test.tstests/e2e/helpers/remote-session-bulk-open-fixture.tstests/e2e/helpers/remote-session-bulk-open-oracle.tstests/e2e/helpers/terminal-host-focus-storm-oracle.tstests/e2e/remote-session-bulk-open-freeze-repro.spec.tstests/e2e/ssh-docker-bulk-open-freeze-repro.spec.tstests/tools/freeze-repro/README.md
🚧 Files skipped from review as they are similar to previous changes (19)
- src/cli/terminal-format.test.ts
- config/scripts/run-ssh-docker-bulk-open-freeze-e2e.mjs
- config/scripts/live-remote-freeze-rpc.test.mjs
- src/main/runtime/orca-runtime.test.ts
- config/scripts/live-remote-status-watchdog.test.mjs
- tests/e2e/ssh-docker-bulk-open-freeze-repro.spec.ts
- src/shared/runtime-types.ts
- tests/e2e/helpers/remote-session-bulk-open-fixture.ts
- src/main/runtime/terminal-focus-navigation-coalescer.test.ts
- config/scripts/live-remote-bulk-open-freeze-repro.mjs
- package.json
- config/scripts/live-remote-bulk-open-freeze-metrics.mjs
- config/scripts/live-remote-status-watchdog.mjs
- src/main/runtime/orca-runtime.ts
- config/scripts/live-remote-realistic-freeze-repro.mjs
- tests/e2e/helpers/remote-session-bulk-open-oracle.ts
- src/main/runtime/terminal-focus-navigation-coalescer.ts
- config/scripts/live-remote-bulk-open-freeze-metrics.test.mjs
- src/cli/terminal-format.ts
b16334c to
0eef23d
Compare
Bound exclusive host navigation to a generation-aware latest-wins single-flight so bulk open and switch fan-out stay responsive on large remote fleets. Add freeze repro harnesses and navigated settlement.
0eef23d to
6fb2039
Compare
1 participant
Summary
Fixes concurrent host terminal-focus storms that can freeze large remote fleets during bulk open or CLI switch fan-out.
The product change is deliberately narrow: a generation-aware, latest-wins single-flight in
OrcaRuntimeService.focusTerminal. Only one exclusive host navigation runs at a time, intermediate pending requests collapse, and obsolete requests settle withnavigated: false.How it works (ELI5)
Imagine many people rapidly yelling, "Show me terminal A! No, B! No, C!" Previously, Orca tried to obey every request at once, and all that expensive remote-session and UI work could freeze the app.
This change adds a receptionist: one focus operation runs at a time, Orca remembers only the newest waiting request, and intermediate requests are marked as superseded. A burst of 100 requests therefore becomes roughly two expensive operations—the one already running and the latest one—instead of 100 competing operations.
Fix
TerminalFocusNavigationCoalescerbounds host navigation to one in flight plus one latest pending requestawaitgeneration check prevents a completed obsolete request from claiming navigationnavigated: falsenavigated: falseinstead of a false successRuntimeTerminalFocus.navigated?: booleanreports whether the request remained the winning applied navigationThe renderer animation-frame coalescer was removed during release review. Main-process single-flight already addresses the storm, while renderer deferral could reorder focus relative to create/reveal events, suspend in a hidden window, and discard richer notification-click payloads.
Change size
The merge candidate has 4,025 additions. Only 284 lines—about 7%—are production code; the remaining 93% is reproduction tooling, tests, fixtures, and documentation.
Repro and safety
See
tests/tools/freeze-repro/README.md.Both live harnesses now default to
ORCA_FREEZE_CREATE=0; creating persistent high-output terminals requires an explicit positive value. Their shared RPC runner:ORCA_CLI_COMMAND, dev, packaged Linux, and other platform CLI names;sampleoutput only after a successful Darwin capture.The headless paired-remote freeze oracle keeps navigation caller-local because a headless host has no renderer. A separate headed-host E2E pairs a second desktop, sends concurrent
terminal.focuscalls withnavigation: hostthroughruntimeEnvironments.call, requires the last request to win, observes superseded receipts, and verifies host-store convergence.Both E2E session seeders close every streaming terminal on success and partial failure. If tab cleanup fails, they fall back to direct PTY close, await every fallback, and surface any terminal that could not be stopped. The post-storm responsiveness probe measures renderer/rAF service directly instead of remounting the terminal view, avoiding software-rendering noise while preserving freeze detection. Headless readiness also treats parsed JSON as untrusted and ignores primitives or malformed shapes.
Scope and residual risk
In scope: concurrent
terminal.focus/ host-navigation storms.Not claimed: reconnect metadata scan cost, sequential activation cost, or cancellation of an already-running renderer reveal. A hung renderer can still hold the single-flight queue until the existing 10-second reveal timeout; notifier loss/replacement can now no longer produce a false success receipt, while abortable renderer protocol work remains separate.
Rebase and overlap audit
Rebased onto
mainat18dbcf001f. Reviewed changes merged during the branch window, including #11890, #11903, #11915, #12081, #12153, #12191, #12197, #12198, #12204, #12206, #12194, #12233, and #12236. Also checked open adjacent work in #11819, #11854, #11864, #11892, #11939, #12149, #12213, #12220, #12238, and #12239. Those changes address projection, reconnect, backpressure, filesystem stalls, close attribution, or send transport; none implements main-process host-focus latest-wins.Test plan
0eef23d9f1: 46 passed, 5 skipped, 0 failed