Repository navigation
Conversation
|
Run failed. View the logs →
|
ba75847 to
7d43b39
Compare
Greptile SummaryThe PR strengthens SSH PTY restoration, identity fencing, relay backpressure handling, remote workspace normalization, split routing, cold parking, and tab cycling.
Confidence Score: 4/5The PR is not yet safe to merge because split terminal cycling can still be swallowed when hydration leaves the active unified-tab pointer null or unmatched. The current fallback derives both the cursor and active-target check from the terminal entity ID when no exact active unified tab is available; distinct split tabs sharing that entity therefore remain indistinguishable and cycling can return without activating the sibling. Files Needing Attention: src/renderer/src/hooks/ipc-tab-switch.ts
|
| Filename | Overview |
|---|---|
| src/main/providers/ssh-pty-session-reattach.ts | Refines SSH reattach restoration with bounded retries and evidence-preserving liveness outcomes. |
| src/relay/relay-pty-source-publication.ts | Updates source publication and client-delivery behavior to isolate disconnected or backpressured viewers. |
| src/relay/pty-handler.ts | Adds PTY delivery cleanup and resume behavior around client detachment. |
| src/renderer/src/hooks/ipc-tab-switch.ts | Makes terminal cycling unified-tab aware and adds hydration fallbacks for worktree terminal order. |
| src/renderer/src/components/terminal-pane/terminal-pane-split-request-routing.ts | Makes terminal split requests aware of exact worktree and unified-tab routing. |
| tests/e2e/ssh-docker-transport-drop-recovery.spec.ts | Adds Docker-backed coverage for SSH transport interruption and terminal recovery. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[SSH PTY transport drops] --> B{PTY liveness evidence}
B -->|Live| C[Bounded source restore]
B -->|Unverifiable| D[Retain recoverable ownership]
B -->|Exited| E[Publish terminal exit]
C --> F[Generation-fenced relay delivery]
F --> G{Viewer attached?}
G -->|Yes| H[Resume ordered output]
G -->|No| I[Drop client backlog and resume PTY]
Reviews (11): Last reviewed commit: "fix(ssh): honor cancellation during reat..." | Re-trigger Greptile
|
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 configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between f290ccf5037083e5904af8a08b80cc14549317f2 and 00e30b49949ad7f1994fa5e6da5547bf0364d60f. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe changes centralize SSH PTY failure tokens and refine identity-mismatch, session-expiry, and source-restore handling. Reattach flows now retry bounded source restoration and await lease rollback. Relay delivery ownership now includes transport generation, and undeliverable output is released after client detach. Renderer terminal routing now handles worktree-scoped split requests, hydration gaps, and normalized workspace status. New Docker SSH E2E tests cover transport recovery, memory bounds, and output continuity. Merge Risk: 🔵 Low · up to The change improves SSH recovery, relay backpressure, and terminal routing behavior, with no current merge-blocking check failure. A bounded terminal-cycling issue remains possible when stale and active terminal identifiers collide, so owner follow-up is still warranted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is mostly complete and explains the changes, motivation, verification, residual risk, and checklist status. It uses equivalent sections for testing and issue context, but it does not provide the required 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: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 2629599b-30ae-4dc7-bb38-a42ec66afd3c
📥 Commits
Reviewing files that changed from the base of the PR and between a5796ec and 7d43b3961a90d861058feff231c402b19d820a81.
📒 Files selected for processing (55)
config/reliability-gates.jsoncconfig/scripts/pr-e2e-gate-contract.test.mjsconfig/scripts/pr-e2e-source-routing.mjsconfig/scripts/run-ssh-docker-e2e.mjssrc/main/daemon/daemon-native-pty-exception.test.tssrc/main/daemon/daemon-native-pty-exception.tssrc/main/ipc/pty/ipc/spawn-execute.tssrc/main/ipc/pty/runtime/spawn-execute.tssrc/main/providers/ssh-pty-errors.tssrc/main/providers/ssh-pty-provider-reattach-incarnation.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/providers/ssh-pty-relay-absence-verdict.test.tssrc/main/providers/ssh-pty-session-reattach-for-spawn.tssrc/main/providers/ssh-pty-session-reattach-restore-retry.test.tssrc/main/providers/ssh-pty-session-reattach.tssrc/main/ssh/__tests__/ssh-connection-test-client.tssrc/relay/dispatcher-capacity-signals.tssrc/relay/dispatcher-contract.tssrc/relay/dispatcher-rpc-routing.tssrc/relay/pty-handler-output-streaming.test.tssrc/relay/pty-handler.tssrc/relay/relay-pty-source-delivery-record.tssrc/relay/relay-pty-source-publication.tssrc/relay/relay-pty-source-restore-required.tssrc/relay/relay-pty-source-send-scheduler.tssrc/relay/relay-pty-source-transport-generation.test.tssrc/renderer/src/app-shell/remote-workspace-unplaced-upload-suppression.test.tsxsrc/renderer/src/app-shell/use-app-session-persistence.tssrc/renderer/src/components/terminal-pane/TerminalErrorToast.test.tssrc/renderer/src/components/terminal-pane/TerminalErrorToast.tsxsrc/renderer/src/components/terminal-pane/TerminalPane.tsxsrc/renderer/src/components/terminal-pane/ipc-pty-connect.tssrc/renderer/src/components/terminal-pane/pty-connection-direct-ssh-reattach-retry.test.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-attach.tssrc/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-connect.tssrc/renderer/src/components/terminal-pane/pty-connection/pty-connect-limits.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-failure-classification.test.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-failure-classification.tssrc/renderer/src/components/terminal-pane/pty-connection/ssh-session-connect.tssrc/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport-expired-pane-recovery.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/components/terminal-pane/terminal-pane-split-request-routing.test.tssrc/renderer/src/components/terminal-pane/terminal-pane-split-request-routing.tssrc/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.tssrc/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.tssrc/renderer/src/hooks/ipc-tab-switch.test.tssrc/renderer/src/hooks/ipc-tab-switch.tssrc/renderer/src/hooks/remote-workspace-push-status.tssrc/shared/ssh-pty-failure-tokens.tstests/e2e/headless-paired-remote-terminal-stall-recovery.spec.tstests/e2e/helpers/docker-ssh-relay-faults.tstests/e2e/helpers/paired-terminal-parking-fixture.tstests/e2e/ssh-docker-transport-drop-recovery.spec.tstests/e2e/terminal-parked-cli-split.spec.ts
💤 Files with no reviewable changes (2)
- src/renderer/src/components/terminal-pane/pty-connection/pty-connect-limits.ts
- src/renderer/src/components/terminal-pane/pty-connection/ssh-session-connect.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
e73cd72 to
8a998fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 3c107d01-21a1-40fb-926e-c47366beeb1f
📥 Commits
Reviewing files that changed from the base of the PR and between e73cd722b14b0a6d338fc31f7798c450c0f6b679 and 8a998fc5203b1d220abaee86a995d87d74170ad6.
📒 Files selected for processing (11)
src/main/providers/ssh-pty-provider-exit-race.test.tssrc/main/providers/ssh-pty-session-reattach-for-spawn.tssrc/main/providers/ssh-pty-session-reattach-restore-retry.test.tssrc/main/providers/ssh-pty-session-reattach.tssrc/relay/relay-pty-source-publication.tssrc/relay/relay-pty-source-transport-generation.test.tssrc/renderer/src/components/terminal-pane/pty-connection/reattach-failure-classification.tssrc/renderer/src/hooks/ipc-tab-switch.test.tssrc/renderer/src/hooks/ipc-tab-switch.tstests/e2e/ssh-docker-transport-drop-recovery.spec.tstests/e2e/terminal-parked-cli-split.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: f7991a92-5876-4bc4-8aba-428ef75ad383
📥 Commits
Reviewing files that changed from the base of the PR and between 8a998fc5203b1d220abaee86a995d87d74170ad6 and f290ccf5037083e5904af8a08b80cc14549317f2.
📒 Files selected for processing (2)
SSH-HANDOFF.mdsrc/renderer/src/i18n/locales/en.json
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta since the prior review (e73cd72 → 8a998fc): the provisional source-delivery rollback is now awaited on every SSH reattach/spawn failure path, the relay fences stale activations, the reattach-failure copy is localized, and terminal cycling honors the unified active tab.
- Awaited provisional delivery rollback —
requestSshPtyAttach,reattachSshPtySessionWithExitFence, andreattachSshPtySessionForSpawnnowawaitthe lease rollback (preserving the original error) instead of fire-and-forget, so a failed spawn cannot outlive its own cancellation or let a retry overlap the old lease. The ledger's cachedrollbackSettlementkeeps repeated calls safe. - Fenced stale relay activations —
RelayPtySourcePublication.activate()now returnsfalse(releasing the rotation fence) whencontext.isStale(), so a superseded request cannot retire or rotate the delivery opened by its replacement. - Localized the reattach-failure message — the "reconnecting" copy routes through
translate()with the existing English fallback. - Honored the unified active tab in terminal cycling —
handleSwitchTerminalTabanchors the cycle on the active group's unifiedtabIdand only treats single-terminal as a no-op when a terminal is already active, fixing stale legacy-id collisions during hydration. - Expanded focused coverage — new unit tests pin settle-before-reject timing, stale-activation fencing, and unified-tab cycle direction; e2e flood-line and parking-fixture assertions were tightened.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
A personal work-handoff document was committed to the repo root and should be removed before merging.
Reviewed changes
Reviewed the two commits pushed after the prior review (8a998fc → f290ccf): a localization-catalog entry for the new SSH restore message, and a new root-level handoff document.
- Registered the SSH restore message —
src/renderer/src/i18n/locales/en.jsonnow carriesreattachFailure.outputStreamRestoring, matching thetranslate()key added inreattach-failure-classification.ts. Correct and complete. - Added
SSH-HANDOFF.md— a machine-specific working note that records a local worktree path, references untracked.llm-counsel/and.ui-proof/scratch directories, and lists open review-thread follow-ups. This is agent scratch, not repo documentation, and contradicts the PR checklist's "no scratch files" claim.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
00e30b4 to
a417ff9
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta since the prior review (f290ccf → 00e30b4): the root-level handoff note was removed, a stale relay activation can no longer release a replacement transport's rotation fence, and the closed-delivery retirement check was inlined on the publish path.
- Removed
SSH-HANDOFF.md— the machine-specific scratch note flagged in the prior review is deleted from the repo root, resolving the only open finding. - Fenced the stale-activation rotation release —
activate()now releases the rotation fence throughreleaseOwnedPtySourceRotationFence, which gates the release onsameClientTransport(record, context), so a superseded request from an old transport generation cannot release the fence a replacement transport armed. Legacy id-only behavior is preserved when both generations areundefined. - Inlined the closed-delivery retirement check — the
deliveryClosedUnderRecordhelper was folded into directptySourceDeliveryClosed(this.session, ...)calls at both the existing-delivery andpublish()sites, preserving the check. - Hardened e2e drop/flood markers — the transport-drop spec now emits markers via
printf '..._%s\n'instead of shell-interpolatedecho. - Added a deterministic regression test —
does not release a replacement fence from a stale activationasserts the stale request leaves the replacement fence armed (sopublish()stays false) and that the owning transport can later release it.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the three commits pushed after the prior review (00e30b4 → a417ff9): the relay link is now declared lost when it delivers nothing inbound instead of wedging on a saturated writer, an unanswered native-deps probe is no longer misread as missing deps, and terminal cycling ignores stale non-terminal entity ids.
- Declared a wedged relay link lost —
SshChannelMultiplexerdrops thewriterSaturatedfield and no longer suppresses the dead-link check while the writer is saturated. The check still requires both no inbound data and a stale unacked frame, so a slow-but-alive peer whose relay keepalives still arrive stays connected, while a half-open link that delivers nothing inbound is torn down withconnection_lost. Drain still rebases both health clocks. - Stopped treating an unanswered native-deps probe as proof the deps are missing —
probeRequiredNativeDepsnow returns anavailable | missing | unverifiableverdict; a throw (host never answered) maps tounverifiable, andrepairInstalledNativeDepsonly authorizes an npm install/rebuild on a confirmedmissing. Aborts still propagate viasignal?.throwIfAborted().execCommandtimeout errors also gained a machine-readableSSH_EXEC_TIMEOUT_CODE. - Ignored stale non-terminal ids during terminal cycling —
handleSwitchTerminalTabapplies the entity-id fallback only when a terminal is active, so a stale editor/browser id that collides with a terminal entity during hydration starts the cycle at the edge instead of at the colliding row.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
3442662 to
087d356
Compare
087d356 to
b7d4c16
Compare
A departed client wedged the shell it was watching. Output backpressure pauses the pty itself and only resumes once pending bytes drain below the low watermark - which needs a client to drain to. When the transport dies mid-stream there is none, so the pty stayed paused, the shell blocked on a pty nobody was reading, and those bytes were never produced at all. Measured on both sides before the fix. Client, instrumenting pty.attach: replayLen=1427 seqCount=120 first=1 last=120 sourceRecovery=no Relay, instrumenting appendReplayBuffer in-container: [relay-probe] ingress saw SEQ up to 100 ... and nothing after So the reattach replay was not returning too little - for a scrolling shell it could not contain the outage, because the outage was never recorded. The pending queue is per-client delivery state and unbounded, which is why pausing was the safe move. With no client it is also undeliverable, so it is dropped rather than carried; buffered is the reattach replay source and is capacity-bounded. Oracle: 459 of 600 lines lost -> 0, across 10 runs. Bounded under 48MB of undeliverable output, with reconnect-and-rebind measured at ~11.9s. Also adds the fault-injection harness this needed (the repo had none) and wires six Docker-SSH specs to SSH source changes - 12 of 17 previously ran only when their own spec file changed.
5accc6f to
ac961d0
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the single commit pushed since the prior review (a417ff9 → 5accc6fd9f): cancellation is now honored throughout the SSH reattach restore-retry path.
- Forwarded the spawn signal to the attach request —
requestSshPtyAttachacceptssignal?: AbortSignaland passes it through tomux.request, which already supports abort and emitsrpc.cancelto the relay, so an in-flightpty.attachis canceled relay-side rather than only dropped client-side. - Fenced each retry iteration against cancellation —
reattachSshPtySessioncallsargs.options.signal?.throwIfAborted()before each attach attempt and again after the provisional-delivery rollback, so a spawn canceled mid-reattach does not burn another attach round-trip. The rollback check was refactored into astaleDeliveryCanceledboolean with unchanged fail-closed semantics. - Added regression coverage —
honors spawn cancellation before a restore retryasserts a single attach round-trip and anAbortErrorrejection; the focused suite passes (7 tests).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta since the prior review (5accc6fd9f → ac961d0c0a): a rebase plus one mechanical relocation.
- Relocated the reattach-error-clear block into the title-state hook — the
describeReattachFailure-awareonPtyErrorClearedRefbody moved verbatim fromTerminalPane.tsxintouse-terminal-pane-title-state.ts, whichuse-terminal-pane-controller.tsalready composes, so the ref reaches the same consumers (terminal-pane-pty-deps.ts,transport-output-callbacks.ts,session-reconcile-dispose.ts). No behavior change.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Review verdict: salvage parts, discard others — not a rebase-and-mergeThree independent reviews of this PR against Keep — these are real and fix user-visible bugs
The Also reported as genuinely failing on main for the right reason: the multiplexer wedge, the daemon stack-frame, the relay backlog-drop, the native-deps probe, and the reattach-retry core. Discard — the split-routing rework guards a state production cannot produceEvery new construct in The mechanism itself is sound and not racy — the generation fence, the pending-flag clear before the size re-check, and the re-read from the map are all correct. It simply has no reachable trigger. Four of the five new tests here fail on main for a reason that cannot occur in production. Two defects in the same file regardless: Either delete this machinery and narrow Fix before any of it lands — a new error vocabulary with unreachable consumers
Outside the two Related, and the shape that most concerns me: the Two tests that pass unchanged on
|
Correction and expansion — I under-weighted the core fix in my comment aboveA deeper pass changes the emphasis. My previous comment led with The core bug, and nothing else addresses itOn That status proves the PTY is live — only its output delivery was retired. And So this is a respawn authorised by a successful RPC that proved the process alive — not by loss of contact. It is the exact inversion I checked: #17957, #17965, #17966, #17969 and #18013 do not touch The fix here is small and correct — a distinct A second real root cause: delivery ownership keyed on client id alone
Also worth taking regardless of this PR's fate
And Superseded — drop these
One thing that should not be in any PR
Revised recommendationClose this PR as a unit. Open three focused ones:
Everything else — the split-routing rework, |
|
Closing per the review above, under the original brief's explicit option to "take it over… OR throw it all away." The salvage is done and landed as two focused PRs:
#18017 found a defect on Why the rest is not being rebasedOn the one file where this PR and the sweep actually met, this PR's version is worse. Verbatim duplicates of open work: the mux Fixes that do not work as landed: the delivery-ownership guard is applied at 1 of 3 sites in A new error vocabulary shipped without its consumers. Test issues: two tests pass unchanged on Out of scope: And Credit where dueThree genuinely valuable ideas came out of this PR and are preserved: the restore-required vocabulary split, the transport-generation ownership key, and the Docker fault taxonomy — the last of which is the strongest test artifact in the whole sweep and had no equivalent anywhere. The Reopen if you disagree with any of the above — the analysis is on this PR and in #18016/#18017. |
|
thanks for reporting. looking into it |
ELI5
An SSH terminal can be alive even when its output stream or network path is temporarily unavailable. This change keeps that distinction intact, retries only the recoverable source-restore case, and prevents a disconnected viewer from wedging the relay indefinitely.
What changed
livevsunverifiablevsexitedoutcomes.Why
This is the follow-up for #12448 and the remaining SSH/tab reliability findings from #17159. It also covers the long-lived SSH slowdown report by ensuring an abandoned client cannot keep a relay PTY paused forever.
Verification
pnpm tcpnpm run check:code-quality:changedpnpm exec vitest run --config config/vitest.config.tsfocused SSH/relay/tab matrix: 15 files, 178 testsorigin/mainat1efd4e1a97dResidual risk
Physical WSL reboot/sleep-resume remains unavailable in this environment. #17748 is related but separate: its timeout fix is plausible, though its current Docker gate is non-green after an Electron startup context failure.
Checklist
main.llm-counselscratch files includedAI-assisted change; all behavior claims are backed by the listed tests or host runs.