Repository navigation
fix(terminal): make remote stop retirement durable - #11804
revofusion wants to merge 2 commits into
Conversation
Greptile SummaryThis PR fixes two independent gaps in remote terminal lifecycle management. First,
Confidence Score: 5/5Safe to merge; both fixes are narrowly scoped, the stale-host fallback is conservative (requires exactly one unambiguous owner), and all three new regression tests pass end-to-end lifecycle scenarios. The stale-host routing change adds a bounded search over persisted sessions and returns the preferred host whenever the result is ambiguous, keeping existing behavior intact for healthy state. The durable-stop change replaces a fire-and-forget with an awaited call and catches provider errors per-PTY, so neither the happy path nor failure cases can leave the loop in a worse state than before. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/main/runtime/orca-runtime.ts | Two targeted fixes: stale-host routing in getWorkspaceSessionHostIdForWorktree (falls back to the unique persisted session owner when repo metadata points at an obsolete partition), and durable stop in stopTerminalsForWorktree (always awaits stopAndWait instead of fire-and-forget kill, with per-PTY error isolation via try/catch). |
| src/main/runtime/orca-runtime.test.ts | Adds three new regression tests pinning the stale-host lookup order, durable PTY retirement before RPC return, and per-PTY error isolation during a worktree sweep; existing stopPty callback mocks updated to match the narrowed Promise<boolean> return type. |
Reviews (2): Last reviewed commit: "fix(terminal): isolate worktree stop fai..." | Re-trigger Greptile
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe runtime now resolves a worktree to a unique persisted workspace-session host when the preferred host does not own the worktree. PTY shutdown now awaits provider retirement through 🚥 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/runtime/orca-runtime.ts (1)
25786-25809: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIsolate per-terminal stop failures so one rejection does not abort the whole worktree sweep.
PTY stopping is now asynchronous. It waits for provider exit through
stopAndWaitwhen available, passes the teardown RPC deadline when applicable, falls back tokill, and awaits the default stop result. This replaces a synchronous, non-throwingkill()call with anawait this.ptyController.stopAndWait(...)call that can reject (for example, on a transient SSH or relay error).In the branch used when
options.stopPtyis not supplied,await stop()at Line 25803 has notry/catch. IfstopAndWaitrejects for oneptyId, the exception propagates out of theforloop andstopTerminalsForWorktreestops processing the remainingptyIdsinptyIds. Other terminals in the same worktree stay unstopped.Elsewhere in this class,
sleepResolvedWorktreeTerminalsusesPromise.allSettledfor the equivalentstopAndWaitfan-out specifically to isolate per-PTY failures. Apply the same isolation here, since this path backs worktree-wide terminal teardown.🔧 Proposed fix: catch stop failures per PTY
const stop = async (): Promise<boolean> => { if (options.deadline !== undefined && Date.now() >= options.deadline) { return false } - // Why: terminal.stop is a durable lifecycle receipt; wait for provider exit so onPtyExit de-persists the tab before returning. - if (this.ptyController?.stopAndWait) { - if (options.deadline !== undefined) { - return await this.ptyController.stopAndWait(ptyId, { - deadlineMs: teardownRpcDeadline(options.deadline) - }) + try { + // Why: terminal.stop is a durable lifecycle receipt; wait for provider exit so onPtyExit de-persists the tab before returning. + if (this.ptyController?.stopAndWait) { + if (options.deadline !== undefined) { + return await this.ptyController.stopAndWait(ptyId, { + deadlineMs: teardownRpcDeadline(options.deadline) + }) + } + return await this.ptyController.stopAndWait(ptyId) } - return await this.ptyController.stopAndWait(ptyId) + return Boolean(this.ptyController?.kill(ptyId)) + } catch (error) { + console.error(`[runtime] failed to stop terminal ${ptyId}`, error) + return false } - return Boolean(this.ptyController?.kill(ptyId)) }Wrapping inside
stop()protects both call paths: the direct{ stopped: await stop(), owner: true }branch, and theoptions.stopPty(ptyId, stop)branch, in case that external callback also letsstop()reject unguarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0901042-f12a-4ec3-9be1-f2aaacea9770
📒 Files selected for processing (2)
src/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.ts
|
@greptile-apps @coderabbitai Addressed the remaining automated-review findings in 306eed5. The |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Summary
terminal.stopawait provider exit so host-side tab retirement and forced persistence finish before the RPC returnsCloses #11803.
This stays separate from #11801: that PR repairs paired-viewer null-PTY reconciliation, while this change owns host runtime session routing and durable PTY stop receipts.
Root cause
A restarted remote runtime could retain repo metadata whose
executionHostIdnamed an obsolete runtime partition, while the worktree's tabs remained uniquely owned by the local workspace session. Whole-tab close followed the stale catalog owner and returnedtab_not_found.Bulk stop had a second gap: its direct path called fire-and-forget
kill, so the RPC could acknowledge beforeonPtyExitremoved and flushed the persisted terminal surface. Restarting the relay during that gap restored the tabs and respawned PTYs.Verification
pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts— 955 passed, 1 skippedpnpm typecheckpnpm run check:code-quality:changedpnpm run check:max-lines-ratchet