fix(client): handle remote terminal hangup

[ogulcancelik]

Summary

• make final client diagnostics best-effort when the controlling terminal has disappeared
• treat simultaneous terminal and transport loss as clean shutdown without hiding ordinary connection failures
• add deterministic PTY coverage for the remote shutdown race

Verification

• reproduced SIGABRT/core dump before the fix with herdr --remote rohan under a disposable tmux PTY
• repeated the same smoke test after the fix: no core dump, remote server remained running and reconnectable
• regression test passed 10/10
• just check

refs #2424

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d240c07-813a-46ea-992e-b1a5fec9ac03

📥 Commits

Reviewing files that changed from the base of the PR and between 272b8ef and 236d177.

📒 Files selected for processing (1)

• tests/client_mode.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/client_mode.rs

---

📝 Walkthrough

Walkthrough

The client now performs idempotent terminal restoration, propagates cleanup errors, and handles terminal and transport closure during shutdown. A PTY integration test covers concurrent hang-up, and the changelog records the fix.

Changes

Terminal cleanup and shutdown

Layer / File(s)	Summary
Terminal restoration state and results src/client/mod.rs	TerminalGuard tracks restoration state. Explicit restoration returns combined terminal and postlude results. Drop cleanup runs only when needed and ignores errors.
Shutdown and panic cleanup src/client/mod.rs	Panic cleanup ignores restoration errors. Normal shutdown records restoration failures and classifies concurrent connection loss as a clean exit.
Concurrent hang-up regression coverage tests/client_mode.rs, docs/next/CHANGELOG.md	The integration test covers simultaneous PTY and transport closure. The changelog records the client core dump fix.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🔵 Low · up to 236d1

The change improves shutdown handling when the remote terminal disappears, but a panic path may still restore terminal state twice, which could leave the local terminal in an inconsistent state during failures. The PR is mergeable with explicit owner awareness or follow-up on this bounded risk.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Transport
  participant TerminalGuard

  Transport-->>Client: Connection loss
  Client->>TerminalGuard: Explicit terminal restoration
  TerminalGuard-->>Client: Restoration result
  Client-->>Client: Classify shutdown as clean

Loading

Possibly related PRs

• herdrdev/herdr#2041: Both changes cover TerminalGuard restoration and PTY-based client shutdown tests.
• herdrdev/herdr#2347: Both changes make terminal cleanup idempotent across explicit, panic, and drop paths.
• herdrdev/herdr#2427: Both changes address terminal hang-up handling and clean client termination.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the client fix for remote terminal hangups, which is the main change in the pull request.
Description check	✅ Passed	The description accurately explains the terminal and transport shutdown handling, test coverage, and verification steps.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue/2424-remote-terminal-hangup

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 77d9ff0e-20f2-4e34-be64-3fa1c1e77466

📥 Commits

Reviewing files that changed from the base of the PR and between 9e6c2b4 and 272b8ef.

📒 Files selected for processing (3)

• docs/next/CHANGELOG.md
• src/client/mod.rs
• tests/client_mode.rs

[greptile-apps]

Greptile Summary

The PR makes terminal restoration and final diagnostics best-effort when a remote client loses its controlling terminal, while preserving ordinary connection-loss errors. It also adds deterministic PTY regression coverage for simultaneous terminal and transport hangup and documents the fix.

• Adds an explicit, result-returning terminal restoration path guarded against duplicate restoration.
• Treats connection loss as clean shutdown only when terminal restoration also fails.
• Adds a Unix PTY test that freezes the client, removes both transport and terminal, then verifies successful exit.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness or security defects identified.

The changed shutdown path preserves reporting for ordinary connection failures, handles concurrent terminal and transport disappearance as intended, and is covered by a bounded end-to-end PTY regression test.

Important Files Changed

Filename	Overview
src/client/mod.rs	Adds single-use terminal restoration with observable failure handling and narrowly combines restoration failure with transport loss for clean shutdown.
tests/client_mode.rs	Adds deterministic Unix PTY coverage for concurrent terminal and server-transport loss with bounded process cleanup.
docs/next/CHANGELOG.md	Documents that closing a remote-client terminal no longer causes a local core dump.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Client loop ends] --> B[Restore terminal]
  B -->|Success| C{Client result}
  B -->|Failure| D{Connection lost?}
  D -->|Yes| E[Clean shutdown]
  D -->|No| C
  C -->|Detached| E
  C -->|Ordinary error| F[Report error and fail]
  C -->|Success| E

Loading

_{Reviews (1): Last reviewed commit: "fix(client): handle remote terminal hang..." | Re-trigger Greptile}