Repository navigation
remote-daemon: tear a PTY session down once - #8643
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesPTY termination lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (24 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
TestTerminateProcessesRunsOnlyOnce has failed since it landed in manaflow-ai#8438. The test asserts teardown is idempotent, but the guard it was written against was never added, so the foreground-group lookup and both SIGKILLs run again on every call. Two paths reach teardown for the same session. waitSessionProcess runs it once the session leader exits, and the hub runs it on a client close frame, when a non-persistent attachment's connection ends, on closeAll, and on an idle reap. The hub paths exclude each other by removing the session from the hub map under h.mu, but waitSessionProcess sits outside that bookkeeping, so a session that outlives its leader and is then closed tears down twice. A second pass can only run after cmd.Wait has returned, so the leader pid it signals has already been reaped. It also repeats the member scan, which reads every /proc/<pid>/stat on Linux and forks ps on macOS. Guard the body with a sync.Once, matching closeTTYOnce and closePTYOnce on the same struct.
abe046e to
a79dfd6
Compare
|
@codex review @coderabbitai review Please review exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested='a79dfd6cc6e634bf0ec7ab4e1f69abcb25015f6e'
echo '== checked-out commit =='
git rev-parse HEAD
echo '== requested commit exists =='
git rev-parse "${requested}^{commit}"
echo '== PR metadata =='
gh pr view 8643 --repo manaflow-ai/cmux --json number,headRefOid,baseRefOid,files --jq '{number, headRefOid, baseRefOid, files: [.files[].path]}'
echo '== diff against first parent =='
git diff --find-renames --unified=80 "${requested}^" "${requested}" -- \
'daemon/remote/cmd/cmuxd-remote/*'
echo '== candidate source and test files =='
fd -t f -e go . daemon/remote/cmd/cmuxd-remote | sort
echo '== structural outline =='
ast-grep outline daemon/remote/cmd/cmuxd-remote --items all --type struct,function,method \
--match 'wsPTYSession|terminateProcessesWithForegroundGroupLookup|waitSessionProcess|closeTTYOnce|closePTYOnce|terminatePTYSessionMembers'
echo '== teardown and lifecycle references =='
rg -n -C 5 \
'\b(terminateProcessesWithForegroundGroupLookup|terminateProcesses|terminatePTYSessionMembers|waitSessionProcess|closeTTYOnce|closePTYOnce|wsPTYSession)\b' \
daemon/remote/cmd/cmuxd-remote --glob '*.go'Length of output: 52730 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== session start / attach / reconnect lifecycle =='
sed -n '720,970p' daemon/remote/cmd/cmuxd-remote/ws_pty.go
echo '== hub teardown paths =='
sed -n '1080,1270p' daemon/remote/cmd/cmuxd-remote/ws_pty.go
sed -n '1489,1645p' daemon/remote/cmd/cmuxd-remote/ws_pty.go
sed -n '1848,1885p' daemon/remote/cmd/cmuxd-remote/ws_pty.go
echo '== POSIX process-session member enumeration =='
cat -n daemon/remote/cmd/cmuxd-remote/ws_pty_session_processes_linux.go
cat -n daemon/remote/cmd/cmuxd-remote/ws_pty_session_processes_darwin.go
echo '== relevant termination tests =='
sed -n '700,830p' daemon/remote/cmd/cmuxd-remote/ws_pty_test.go
rg -n -C 12 \
'TestWebSocketPTYCleanupTerminatesEverySessionProcessGroup|shielded|trap.*HUP|terminateProcesses|waitSessionProcess|closeAll|reapIdleSession' \
daemon/remote/cmd/cmuxd-remote --glob '*_test.go'
echo '== all wsPTYSession constructions and cmd assignments =='
rg -n -C 3 \
'wsPTYSession\s*\{|\.cmd\s*=|cmd:\s*' \
daemon/remote/cmd/cmuxd-remote --glob '*.go'Length of output: 78863
The The teardown reasoning also holds. All hub teardown paths and No findings from this focused review. |
Greptile SummaryThis PR makes PTY process teardown run once per session. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "remote-daemon: tear a PTY session down o..." | Re-trigger Greptile |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
TestTerminateProcessesRunsOnlyOncehas failed on main since it was added in #8438:The test asserts that a PTY session is torn down once, but the guard it was written against never landed, so
terminateProcessesWithForegroundGroupLookupredoes the foreground-group ioctl and re-sends both SIGKILLs on every call.Two paths reach teardown for the same session.
waitSessionProcessruns it after the session leader exits. The hub runs it when a client sends a close frame, when a non-persistent attachment's connection ends, oncloseAll, and on an idle reap. Those four hub paths cannot collide with each other, because each one removes the session from the hub map underh.mubefore it signals anything, butwaitSessionProcessis outside that bookkeeping. A session that outlives its leader and is then closed runs teardown twice.This wraps the body in a
sync.Once, matchingcloseTTYOnceandclosePTYOncealready on the struct. AwsPTYSessionowns oneexec.Cmdfor its whole life and nothing reassigns it, and reconnecting to a live session only adds an attachment, so the guard covers exactly the one process group it is meant to. Once a session is closed the attach path builds a freshwsPTYSessionwith its ownsync.Once.What a duplicate pass actually costs is a second pair of SIGKILLs and a second walk of the process table.
terminatePTYSessionMembersreads every/proc/<pid>/staton Linux and forkspson macOS, and it rescans until the discovered member set stops growing. A second pass can also only run aftercmd.Waithas returned, so the leader pid it signals has already been reaped and no longer belongs to the daemon.The guard does change one thing. When a hub path tears the session down first, the sweep
waitSessionProcessdoes aftercmd.Waitbecomes a no-op, and that sweep's own comment says it is there to catch a SIGHUP-shielded background job still holding the PTY slave. I do not believe anything is lost in practice. The hub pass runs the same rescanning member sweep, the leader is a member of its own session so it gets killed inside that sweep, and a process cannot fork once SIGKILL has been delivered to it, which is what makes the scan converge.TestWebSocketPTYCleanupTerminatesEverySessionProcessGroupalready drives that exact case from all three hub paths withset -m; trap '' HUP; sleep 300 &, and with this guard in place it is the hub pass alone that has to clean up. I could not construct a member that the hub pass misses and the skipped pass would have caught, but it is one fewer sweep than before, so please check that reasoning rather than take it from me.I left the stale-pid signalling alone. Signalling a pid the daemon has already reaped is wrong on the first pass too:
waitSessionProcesscallsterminateProcessesaftercmd.Wait, so the leader is gone by then, and async.Oncecannot suppress a first call. The window is narrow, because Linux keeps a pid allocated while any process still has it as a session id, so recycling requires the session to be empty and thenKill(-pid, SIGKILL)just returns ESRCH. Fixing it properly means capturing the member set beforecmd.Waitinstead of rederiving it from the leader pid afterwards, which is a bigger change than this test fix and belongs in its own PR.I could not compile or run any of this locally, since the machine I wrote it on has no Go toolchain. The linux
remote-daemon-testsjob is the gate. An earlierworkflow_dispatchrun of this branch on my fork reportedok github.com/manaflow-ai/cmux/daemon/remote/cmd/cmuxd-remote 1.783s.This failure also blocked CI that has nothing to do with the daemon.
linux-preflightfails when a routed linux job with a true route fails, the macOS jobs gate onlinux-preflightsucceeding, and any non-pull_requestevent marks every route true. So while this test was red, every manually dispatchedci.ymlrun skipped the entire macOS matrix.Summary by CodeRabbit