Repository navigation
Conversation
|
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⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe change adds a watcher that tracks runtime environments in the user-data store. Shared-control connections check for removal during opening, liveness checks, and reconnect handling, then retire removed environments. Tests cover watcher behavior, connection retirement, and CLI removal in an Electron fixture. The SQLite database runtime test include list also adds the agent-launch instant-tab test. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The PR adds runtime-environment retirement coverage and SQLite test discovery. The Electron test may remove its fixture while spawned descendants remain alive, creating a bounded test-cleanup risk; no runtime product failure was established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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:
cdcac82d-27c8-4ea0-a1d6-71a4978f2a7b
📥 Commits
Reviewing files that changed from the base of the PR and between 6aa12c3 and 4e0188e10542a28c4e0ea83064479509a024d7d3.
📒 Files selected for processing (15)
src/main/ipc/runtime-environment-cli-removal.electron.test.tssrc/main/ipc/runtime-environment-removal-watch.test.tssrc/main/ipc/runtime-environment-removal-watch.tssrc/main/ipc/runtime-environment-request-connections.test.tssrc/main/ipc/runtime-environment-request-connections.tssrc/main/ipc/runtime-environments.tssrc/main/runtime/agent-session-record-store-slot.test.tssrc/shared/remote-runtime-shared-control-connection.test.tssrc/shared/remote-runtime-shared-control-connection.tssrc/shared/remote-runtime-shared-control-diagnostics-inputs.tssrc/shared/remote-runtime-shared-control-open.tssrc/shared/remote-runtime-shared-control-reconnect.test.tssrc/shared/remote-runtime-shared-control-reconnect.tssrc/shared/remote-runtime-shared-control-test-server.tssrc/shared/remote-runtime-shared-control-types.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| // Why open() instead of a bare return: its guard declines the dial *and* retires, and a socket | ||
| // closing after the removal has no liveness tick left to do that. | ||
| if (args.environmentRemoved()) { | ||
| args.open() | ||
| return | ||
| } | ||
| // Why the fire is not gated on removal either: open() re-asks the same predicate on every entry, | ||
| // so short-circuiting the timer would strand the transport it must evict. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '45,105p' src/shared/remote-runtime-shared-control-reconnect.ts
sed -n '145,315p' src/shared/remote-runtime-shared-control-connection.ts
rg -n 'invalidateRuntimeEnvironmentTransport|capabilityPaused|scheduleAfterSocketClose' src/main/ipc/runtime-environment-request-connections.ts src/shared/remote-runtime-shared-control-connection.tsRepository: stablyai/orca
Length of output: 8465
🏁 Script executed:
printf '%s\n' '--- relevant changed diff ---'
git diff 28ed544da87be18df7dea85a6ab0c5664f4d14e6 4e0188e10542a28c4e0ea83064479509a024d7d3 -- src/shared/remote-runtime-shared-control-reconnect.ts src/shared/remote-runtime-shared-control-connection.ts src/main/ipc/runtime-environment-request-connections.ts
printf '%s\n' '--- connection class close/constructor and registry binding ---'
sed -n '1,180p' src/shared/remote-runtime-shared-control-connection.ts
sed -n '270,370p' src/shared/remote-runtime-shared-control-connection.ts
printf '%s\n' '--- removal owner and relevant callers ---'
rg -n -C 5 'invalidateRuntimeEnvironmentTransport|onEnvironmentRemoved|isEnvironmentRemoved|isCapabilityPaused|remove.*Environment|environmentRemoved' src/main/ipc/runtime-environment-request-connections.ts src/shared
printf '%s\n' '--- targeted tests ---'
rg -n -C 4 'scheduleAfterSocketClose|capabilityPaused|environmentRemoved|retireRemovedEnvironment|onEnvironmentRemoved' test src --glob '*test*' --glob '*spec*' || test "$?" -eq 1Repository: stablyai/orca
Length of output: 41891
🏁 Script executed:
printf '%s\n' '--- environment removal implementation ---'
sed -n '1,260p' src/main/ipc/runtime-environment-removal-watch.ts
printf '%s\n' '--- connection registry and owner cleanup ---'
sed -n '1,290p' src/main/ipc/runtime-environment-request-connections.ts
printf '%s\n' '--- socket close cleanup contract ---'
sed -n '1,240p' src/shared/remote-runtime-shared-control-socket-close.ts
printf '%s\n' '--- capability pause producer ---'
sed -n '1,260p' src/main/ipc/runtime-environment-capability-evidence.ts
printf '%s\n' '--- close path / paused tests ---'
sed -n '1,145p' src/shared/remote-runtime-shared-control-reconnect.test.ts
rg -n -C 7 'capabilityPaused|isCapabilityPaused|pauseStandingRetry|subscriptionCount: 0|environmentRemoved' src/shared/remote-runtime-shared-control-connection.test.ts src/shared/remote-runtime-shared-control-reconnect.test.tsRepository: stablyai/orca
Length of output: 42286
🏁 Script executed:
printf '%s\n' '--- removal watch registration and retirement binding ---'
rg -n -C 5 'setRuntimeEnvironmentRemovalWatch|retireRemovedRuntimeEnvironment|retire:.*environment|closeRemoteRuntimeRequestConnection' src/main src/shared
printf '%s\n' '--- delegated socket state cleanup ---'
sed -n '1,240p' src/shared/remote-runtime-shared-control-state.ts
printf '%s\n' '--- exact line-numbered scheduler and registry blocks ---'
nl -ba src/shared/remote-runtime-shared-control-reconnect.ts | sed -n '55,88p'
nl -ba src/shared/remote-runtime-shared-control-connection.ts | sed -n '150,185p;275,340p'
nl -ba src/main/ipc/runtime-environment-request-connections.ts | sed -n '85,145p;205,245p'
printf '%s\n' '--- exact capability outcome consumer context ---'
rg -n -C 4 'applyRuntimeEnvironmentCapabilityVerdict|isRuntimeEnvironmentCapabilityPaused' src/mainRepository: stablyai/orca
Length of output: 41895
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- exact removal-watch registration ---'
rg -l -F -- 'setRuntimeEnvironmentRemovalWatch' src/main src | while IFS= read -r f; do
echo "FILE $f"
rg -n -C 12 -F -- 'setRuntimeEnvironmentRemovalWatch' "$f"
done
printf '%s\n' '--- exact transport invalidation ---'
rg -n -C 12 -F -- 'invalidateRuntimeEnvironmentTransport' src/main src
printf '%s\n' '--- exact delegated shared-control cleanup ---'
rg -n -C 10 -F -- 'function closeSharedControlSocketState' src/shared
rg -n -C 10 -F -- 'closeSharedControlSocketState({' src/shared/remote-runtime-shared-control-state.ts src/shared
printf '%s\n' '--- exact scheduler and close callback ---'
nl -ba src/shared/remote-runtime-shared-control-reconnect.ts | sed -n '55,88p'
nl -ba src/shared/remote-runtime-shared-control-connection.ts | sed -n '275,340p']Repository: stablyai/orca
Length of output: 42620
Check removal before the paused/no-subscription return.
When capability evidence reports absent, a socket close with no subscriptions returns before checking environmentRemoved(). If the environment was removed, this path calls neither open() nor onEnvironmentRemoved. The cached shared-control connection can remain in sharedControlConnections indefinitely.
Move the removal check after the intentional/manual-close guard but before the capability-pause guard.
Suggested fix
- if (
- args.intentionallyClosed ||
- args.manuallyDisconnected ||
- (args.subscriptionCount === 0 && args.capabilityPaused)
- ) {
+ if (args.intentionallyClosed || args.manuallyDisconnected) {
return
}
- // Why open() instead of a bare return: its guard declines the dial *and* retires, and a socket
- // closing after the removal has no liveness tick left to do that.
if (args.environmentRemoved()) {
+ // Why open() instead of a bare return: its guard declines the dial *and* retires, and a socket
+ // closing after the removal has no liveness tick left to do that.
args.open()
return
}
+ if (args.subscriptionCount === 0 && args.capabilityPaused) {
+ return
+ }
4e0188e to
ce363b9
Compare
ce363b9 to
22de4bc
Compare
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:
09885def-b83f-451c-9ea8-cb716949e801
📥 Commits
Reviewing files that changed from the base of the PR and between ce363b97e1f47f63b6e67796051506f30a5d09d8 and 857d9db.
📒 Files selected for processing (4)
config/scripts/vitest-node-runtime-files.mjsconfig/scripts/vitest-sqlite-database-runtime-files.mjssrc/main/ipc/runtime-environment-cli-removal.electron.test.tssrc/main/runtime/structured-agent-session-codex-stopped-send-order.test.ts
💤 Files with no reviewable changes (1)
- src/main/runtime/structured-agent-session-codex-stopped-send-order.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| afterAll(async () => { | ||
| child?.kill('SIGKILL') | ||
| await closeSharedControlTestServers() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n 25,60p src/main/ipc/runtime-environment-cli-removal.electron.test.ts
rg -n "xvfb|spawn\(|detached|taskkill|treeKill|killProcessTree" src/main/ipc/runtime-environment-cli-removal.electron.test.ts
rg -ln "taskkill|killProcessTree|tree-kill" src config | headRepository: stablyai/orca
Length of output: 1899
🏁 Script executed:
sed -n '1,260p' src/main/ipc/runtime-environment-cli-removal.electron.test.ts
printf '\\n--- process termination matches ---\\n'
rg -n -i "taskkill|killprocess(tree)?|kill.*tree|tree.*kill|terminate.*(child|process)|child.*(terminate|kill)|detached" src --glob '!**/*.test.ts' || test "$?" -eq 1
printf '\\n--- candidate helper sources ---\\n'
sed -n '1,240p' src/shared/ephemeral-vm-recipe-process.ts
sed -n '1,220p' src/cli/handlers/interactive-login-interruption.ts
sed -n '1,240p' src/main/pty-descendant-termination.test.tsRepository: stablyai/orca
Length of output: 45655
🏁 Script executed:
printf '%s\\n' '--- test launch and teardown ---'
nl -ba src/main/ipc/runtime-environment-cli-removal.electron.test.ts | sed -n '20,42p;112,157p'
printf '%s\\n' '--- electron launch resolver ---'
nl -ba src/main/browser/electron-probe-display-launch.ts | sed -n '1,220p'
printf '%s\\n' '--- child-process run-process API ---'
nl -ba src/shared/child-process/run-process.ts | sed -n '1,260p'
printf '%s\\n' '--- child-process helper files ---'
rg --files src/shared/child-process
printf '%s\\n' '--- helper exports/usages ---'
rg -n "terminateWindowsLoginProcessTree|killRecipeProcess|process-tree-kill" src/shared src/cli src/main/ipc src/main/browserRepository: stablyai/orca
Length of output: 20306
🏁 Script executed:
printf '%s\\n' '--- process tree termination ---'
nl -ba src/shared/child-process/process-tree-termination.ts | sed -n '1,300p'
printf '%s\\n' '--- process-tree kill gate ---'
nl -ba src/shared/child-process/process-tree-kill-gate.ts | sed -n '1,240p'
printf '%s\\n' '--- process spec ---'
nl -ba src/shared/child-process/process-spec.ts | sed -n '1,220p'
printf '%s\\n' '--- spawn resolution ---'
nl -ba src/shared/child-process/spawn-resolution.ts | sed -n '1,220p'
printf '%s\\n' '--- focused helper tests ---'
rg -n "forceTerminateProcessTree|signalProcessTree|taskkill|tree" src/shared/child-process/process-tree-termination.test.tsRepository: stablyai/orca
Length of output: 20606
🏁 Script executed:
nl -ba src/main/ipc/runtime-environment-cli-removal.electron.test.ts | sed -n '32,50p;158,215p'
printf '%s\\n' '--- tree helper completion tests ---'
nl -ba src/shared/child-process/process-tree-termination.test.ts | sed -n '30,100p'Repository: stablyai/orca
Length of output: 5881
Terminate the process tree before removing the fixture.
child.kill('SIGKILL') targets only the spawned process and does not wait for termination. When Linux uses xvfb-run because DISPLAY is unset, Electron is a child of the process this test kills. On Windows, Chromium descendants can retain profile handles while rmSync runs. Use the process-tree helper, and spawn detached on POSIX so the helper can terminate the whole group. Skip fixture removal if termination is not verified.
Suggested fix
+import { forceTerminateProcessTree } from '../../shared/child-process/process-tree-termination'
+
afterAll(async () => {
- child?.kill('SIGKILL')
+ const treeTerminated = child ? await forceTerminateProcessTree(child) : true
await closeSharedControlTestServers()
if (process.env.ORCA_KEEP_FIXTURE === '1') {
return
}
+ if (!treeTerminated) {
+ throw new Error('Could not verify Electron process-tree termination; fixture cleanup skipped')
+ }
for (const root of roots) {
rmSync(root, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 })
}
})
...
env: { ...env, ORCA_BACKGROUND_LAUNCH: '1' },
- stdio: ['ignore', 'pipe', 'pipe']
+ stdio: ['ignore', 'pipe', 'pipe'],
+ detached: process.platform !== 'win32'
})
There was a problem hiding this comment.
Compared with CC’s #21048. Confirmed removal is scoped to the same previously observed catalog. Missing/corrupt/unsupported catalogs preserve connections. Existing heartbeat, request-open and reconnect paths retire transports once; a callback cannot leave the socket alive. Native CLI removal proves the standing socket closes without another dial. No remote process is stopped. Merge before #25994.
Final assembled verification: 6,313 tests pass (five skips) in 563 suites, plus 48 upstream authority tests and 81 serial compatibility tests. Typecheck and changed-code quality pass (147 files, zero findings). Fresh hidden macOS and isolated Windows/Linux each pass all four native flows. Windows: https://github.com/stablyai/orca/actions/runs/37556420437. Linux: https://github.com/stablyai/orca/actions/runs/37557129429. Current-head CI is green on 63260970ae36; merge recommended. Contributor originals remain open until their replacements merge.
Co-authored-by: CC <zhoucc922@gmail.com>
6326097 to
1371f3e
Compare
nwparker
left a comment
nwparker
left a comment
There was a problem hiding this comment.
Adversarial self-review of rebased head 1371f3eda0ad.
Rebased onto main 00984ccb2504, including #24863 and #26088. The standalone parent passes 62 tests across five suites, including the real hidden Electron/separate-CLI socket-retirement test. Its fixture now leaves ssh2 external, matching application packaging, and provides the new startup session census. No archive behavior is required from #25994 for this parent to work. Low risk once current-head CI passes; merge before #25994.
Validation of the assembled rebased application tree: 6,117 tests passed (four skips), full typecheck, zero changed-code quality findings across 151 files, fresh app/CLI builds, and all four hidden macOS flows. The final main refresh is CI-only and its 82 planner tests pass. Earlier Windows/Linux native runs were on the previous base; current-head CI must pass before merge.
Contributor references and coauthor trailers are preserved. The original damaged Windows+WSL profile and a native nested SSH recovery/save journey remain unverified; this does not justify closing #21489. No PRs were merged or superseded contributor PRs closed during this rebase.
This branch has not been deployed
1 participant
ELI5
Removing a host with
orca environment rmused to leave Orca's cached connection alive. It could keep reconnecting to a host the user had removed. The connection now retires when the existing heartbeat or next reconnect observes a confirmed removal.What Changed
Records that the app's own environment catalog contained a host, then checks that same catalog with the validated, required-file reader. A corrupt, missing, unsupported, or different catalog provides no removal evidence. Confirmed removal closes the connection, rejects waiting requests and runs the existing cache/subscription/browser cleanup once. Reconnect timers, direct request-driven opens and already-open sockets all obey the same retirement decision.
Why
Adapted from CC's #21048, with CC credited as coauthor. Review additionally fixed cross-catalog observation leakage and made the connection close itself even if its retirement callback does not. This uses the existing ten-second heartbeat rather than adding a timer or inferring remote process death from lost contact.
Linked Issue
Refs #21489 and #20995. Replaces #21048 after this replacement merges. Leave that contributor PR open until then. Historical workspace partition cleanup is a separate fix.
Visual Proof
N/A — background connection teardown has no new rendered interface. The native Electron/CLI test below directly observes server-side live sockets and reconnects.
Testing
1371f3eda0ad; wait for current-head checks before merging.Review
Compared with CC’s #21048. Confirmed removal is scoped to the same previously observed catalog. Missing/corrupt/unsupported catalogs preserve connections. Existing heartbeat, request-open and reconnect paths retire transports once; a callback cannot leave the socket alive. Native CLI removal proves the standing socket closes without another dial. No remote process is stopped. Merge before #25994.
The series includes local/direct SSH isolation, folder workspaces, and remote authority coverage. Real Windows+WSL reproduction of the original damaged profile and a native nested SSH recovery/save journey remain unverified. #25938 and the independent CI memory fix #26049 are merged.
Rebased onto main
00984ccb2504, including #24863 and #26088. The standalone parent passes 62 tests across five suites, including the real hidden Electron/separate-CLI socket-retirement test. Its fixture now leaves ssh2 external, matching application packaging, and provides the new startup session census. No archive behavior is required from #25994 for this parent to work. Low risk once current-head CI passes; merge before #25994.Agent skill upstream boundary
Checklist