[ogulcancelik]

If the shell that started the server exits, it can hang the server up, for example herdr server & in zsh. Right now that stops the server and kills every pane. The log also never says why the server stopped, which is what made #4920 hard to read.

• The server now ignores SIGHUP, like tmux. That covers the auto-started daemon, herdr server in a terminal, and the live-handoff replacement. Ctrl+C and SIGTERM still stop it.
• The hangup is ignored through a handler, not by ignoring the signal outright, so anything the server launches still gets normal hangup behavior.
• Every shutdown now logs one line with the reason: SIGTERM, SIGINT, a server.stop request with the calling process and its parent (Linux and macOS), host shutdown, or a Windows console event.
• A server whose terminal closes during startup no longer crashes while printing its ready message.
• Test teardown that stopped servers with SIGHUP now uses SIGTERM.
• If a script of yours stopped the server with SIGHUP, switch it to SIGTERM or herdr server stop.

refs #4920

Validation: just check, plus Windows clippy --all-targets through the cross setup. On macOS, server_headless and the new unit tests. Live on Linux: a zsh job hangup, then kill -HUP with a running pane (both survive), kill -TERM and herdr server stop (both log the reason).

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 597709b1-b739-4845-830b-338c03efc74e

📥 Commits

Reviewing files that changed from the base of the PR and between fabcab1 and 51c2d34.

📒 Files selected for processing (23)

• Cargo.toml
• src/api/server.rs
• src/platform/fallback.rs
• src/platform/linux.rs
• src/platform/macos.rs
• src/platform/mod.rs
• src/platform/unix_common.rs
• src/server/headless.rs
• src/server/headless/bootstrap.rs
• src/server/headless/lifecycle.rs
• src/server/headless/tests/mod.rs
• src/server/mod.rs
• src/server/shutdown.rs
• tests/api_ping.rs
• tests/auto_detect.rs
• tests/cli/harness.rs
• tests/client_mode.rs
• tests/cross_area.rs
• tests/detach_reattach.rs
• tests/live_handoff.rs
• tests/multi_client.rs
• tests/server_headless.rs
• tests/support/mod.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

---

📝 Walkthrough

Walkthrough

The server now shares a ServerStop handle between the API and headless server. It records shutdown reasons for API requests, operating-system signals, and host shutdown. Platform code handles quit signals and retrieves local socket peer details. Tests cover shutdown behavior and use a shared helper to stop spawned processes.

Suggested reviewers: jjliebig

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 51c2d

No identified shutdown or test-workflow issue remains to resolve before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 51c2d

The server now survives terminal hangups and records why shutdown begins. The inspected changes preserve existing stop permissions and isolation. No material security regression was established, but platform-specific and concurrent shutdown edge cases remain incompletely covered.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The shutdown outcome affects the targeted server process and its connected clients and panes. Access to its existing session socket already permitted stopping that server at the base; the inspected PR path does not add cross-session authority or a new privileged operation.

Trust Boundaries and Controls

• observed — The local socket remains the enforcement boundary for API stop access. Unix listener permissions remain user-only, and the existing Windows listener security configuration is retained. Adding caller attribution does not introduce a new credential or substitute process names for access control.

Resilience and Maintainability Implications

• observed — Shutdown remains possible even when reason recording fails, so diagnostic-state failure does not suppress the stop flag. Logging is best-effort and can silently discard writes after failure; the new reason output is therefore operational diagnostics, not a guaranteed durable security audit.

Hardening Proposals

• proposed — Explicitly encode control characters in caller process labels and document attribution as the immediate socket peer. This would make shutdown diagnostics less ambiguous without treating those labels as authenticated identity. No privileged audit consumer or material exploitation path was established.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 22 files. (1 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main changes: keeping the server running on hangup and logging shutdown reasons.
Description check	✅ Passed	The description explains the SIGHUP behavior, shutdown-reason logging, startup fix, and related test changes.
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.

Full details: Docstring Coverage

Explanation

Docstring coverage is 45.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 22 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.}

[kangal-bot]

@coderabbitai review
@greptileai

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[greptile-apps]

Confidence Score: 4/5

[Medium risk] Server lifecycle and signal handling refactored.

The PR appears safe to merge, with a non-blocking gap in shutdown-reason logging for live handoff.

Findings

1.  Handoff exit skips reason log ▶

Summary

The PR makes the server survive SIGHUP, records reasons for signal-, API-, and host-initiated shutdown, and updates test teardown to use SIGTERM.

• The successful live-handoff exit does not pass through the new shutdown-reason log.

_{Reviews (1) · Last reviewed commit: "fix: keep the server running on hangup a..."}

[greptile-apps]

Handoff exit skips reason log

After a successful server.live_handoff, finish_live_handoff_shutdown() sets shutting_down directly. The loop then runs complete_shutdown() without calling initiate_shutdown(), so the old server never emits the new shutdown-reason line. This leaves a gap in the logs when diagnosing why that server stopped.

[ogulcancelik]

intentional: the handoff exit already logs its own reason line, live handoff completed; old server exiting, in finish_live_handoff_shutdown. routing it through initiate_shutdown would also send ServerShutdown to clients that were already handed off, so i kept one line per shutdown there.

[greptile-apps]

That’s a valid distinction. finish_live_handoff_shutdown() already records the handoff-specific exit (live handoff completed; old server exiting), and routing it through initiate_shutdown() would incorrectly notify clients that have already been handed off. I’m withdrawing the finding; one shutdown log line for this path is intentional and sufficient.