Repository navigation
fix(server): stop the session when the server exits - #90
Merged
isac322 merged 6 commits intoOct 10, 2026
Merged
Conversation
launch_app started each app in the server's own process group, so session_stop could only signal the direct child: Session.stop sent SIGTERM/SIGKILL to that one process and LiveSession.stop sent SIGTERM with a 3s wait and no escalation at all. A launched app's descendants (a shell script's background jobs, a browser's zygote/crashpad helpers) survived and kept writing into the isolated home, or stayed on a live desktop where nothing else cleans them up. Each launched app now runs in its own process group, and both the virtual and live session_stop stop all launched apps together: SIGTERM every group, one shared 3s wait, SIGKILL every group that still has live members, one shared 2s wait. Each leader stays unreaped (probed with waitid WNOWAIT) until after the last group signal, so its pid, which is the group id, stays reserved and killpg cannot reach a recycled pid. KWin and apps predating a live session_connect are never signalled. Generalises the session's zombie-aware group liveness into shared helpers reused by both the compositor and the app-group teardown.
![haechibot[bot]](https://avatars.githubusercontent.com/in/5118831?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: BLOCKING (PR-ready: false) reviewed at e923117.
The shutdown path addresses the session leak, but a filesystem cleanup failure defeats the bounded exit when a tool is still running.
Blocking
- P1
src/kwin_mcp/server.py:1176:terminate_all()can raise when_remove_tree()encounters an inaccessible app-created subdirectory. That exception bypassesos._exit(), leaving Python's executor finalizer waiting for the in-flight tool after shutdown signals have been ignored. Guarantee the final exit despite fallback failures, and handle directory removal errors individually so remaining cleanup continues.
Verified
- Ran
python3 /tmp/issue-agent/review-pr-90/validation-cleanup-permissions.py. The isolated harness executes AST-extracted head shutdown and registry code with a real executor and filesystem permissions; MCP and engine interfaces are stubbed. With the drain shortened to 0.1 seconds and a 3-second tool, the healthy control exited 0 in 0.15 seconds. A realchmod(0)subdirectory causedPermissionError; the process waited 3.04 seconds for the tool and exited 1. A permanently blocked tool would prevent exit indefinitely. - Isolated registry checks passed for terminating an owned process, excluding a reaped leader, and refusing spawn after closure.
- Reviewed process ownership, tool-thread serialization, CLI signal deferral, and the full base diff, including the dependency from #89. No closing issues or previous review threads were present.
- Head remained unchanged. GitHub reports MERGEABLE and the Semantic PR check passed.
Not verified
- Full KWin E2E and quality suites: the host lacks the required Python environment and the Docker socket is absent.
- Baseline-versus-head KWin reproduction was not run.
- Required-check configuration could not be read because the installation token received HTTP 403.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
Replace the main-thread session_stop fallback (which violated the engine's single-owner rule while a tool was running) with an owned-process registry plus a tool-thread-only stop. On stdin EOF the server submits the stop to the tool executor and waits a short drain window for it to start. If the tool thread is free the stop runs fully (session_stop); if it is busy the stop is cancelled and the exit path falls back to the registry, which SIGTERMs then SIGKILLs every process group it spawned and removes the virtual-session temp dirs it created. Live-session KWin is never signalled - only launched apps are stopped. The registry holds the owned Popen for each leader (not a bare pgid) so it never signals a group whose leader has already been reaped, and every reap site unregisters the entry. The busy-exit directory cleanup mirrors session_stop's retention semantics: the registry records exactly the temp dirs session_stop would remove. The exit outcome line and the stdio flushes are best-effort: a client that closed its stderr reader before disconnecting can no longer make the BrokenPipeError skip the forced exit and leave interpreter shutdown joining a busy tool thread. A failure of the exit cleanup itself is reported on stderr instead of raised, and the registry removes each temp dir independently: a directory an app made inaccessible can no longer stop the other removals or skip the forced exit.
superbiche
force-pushed
the
fix/stop-session-on-server-exit
branch
from
October 7, 2026 21:36
e923117 to
0848f63
Compare
There was a problem hiding this comment.
Previous findings
- Closed: P1 cleanup errors bypassed forced exit.
server.py:1191–1194now catches cleanup exceptions, andsession.py:420–421handles directory removal errors independently. The previous permission-error reproducer passes on this head.
Verdict: GREEN (PR-ready: false: full validation and required-check configuration remain unverified) re-reviewed at 0848f63. No blocking findings.
The fix preserves tool-thread serialization and queued-stop cancellation while ensuring ordinary cleanup exceptions cannot bypass the final exit. No new findings.
Verified
- Ran
python3 /tmp/issue-agent/review-pr-90/validation-cleanup-permissions.pyagainst both pinned revisions. With a real executor, a 3-second tool, and a shortened 0.1-second drain, the inaccessible-directory case waited 3.06 seconds and exited 1 on e923117; this head exited 0 in 0.74 seconds. Healthy controls exited 0 promptly. - An injected registry exception waited 3.04 seconds on the previous head; this head exited 0 in 0.14 seconds and reported the error.
- Isolated registry checks confirmed that an inaccessible directory does not prevent removal of a subsequent directory.
- Independent code review found no regression in the fix or its added tests. The recorded head remains unchanged, GitHub reports MERGEABLE, and Semantic PR passed.
Not verified
- The harness extracts actual revision code but stubs MCP and engine interfaces; it does not exercise KWin or the full MCP transport.
- Full E2E and quality suites remain unavailable in this environment. The author's reported suite results were not independently reproduced.
- Required-check configuration remains unverified because repository protection reads were denied to the installation token.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
Address review findings on the server/CLI exit path: - server: the signal handler no longer raises inside the asyncio loop. It records the first SIGTERM/SIGHUP/SIGINT and wakes a daemon watcher thread that runs the once-only exit routine, so a signal landing while a request task runs no longer waits for the client to close stdin and exits 128+signum instead of 0. SIGINT is now handled the same way. - session: while terminate_leaders escalates a leader, poll/wait/kill observe its exit with waitid(WNOWAIT) instead of reaping it, so a concurrent poll cannot make the escalation skip SIGKILL. - session: register_temp_dir refuses (and removes the dir) after close, so a dir created after terminate_all's snapshot cannot leak. - session: spawn takes explicit keyword parameters, dropping the ty: ignore and cast. - cli: every exit path sweeps owned process groups that were never attached to a session (e.g. a launch interrupted between spawn and AppInfo registration). - tests: regression tests for each case; strengthen the spawn/close atomicity test. - docs: README, CONTRIBUTING, docker/README, CHANGELOG.
There was a problem hiding this comment.
Previous findings
- Closed: P1 filesystem cleanup errors bypassed forced exit. The exception guard remains in
_finishatserver.py:1183–1186, and registry directory removal still handles individualOSErrors.
Verdict: BLOCKING (PR-ready: false) re-reviewed at 260d7cd.
The signal watcher improves shutdown during active requests, but blocking output operations can still prevent the server from exiting.
New findings
- P1
src/kwin_mcp/server.py:1195–1197: if a client stops reading stdout while a response writer fills the pipe, the watcher blocks instdout.flush()after cleanup completes. It never reachesos._exit(), and subsequent shutdown signals are ignored. Remove unconditional forced-exit flushing and make the final stderr diagnostic bounded or nonblocking as well.
Verified
- Ran
python3 /tmp/issue-agent/review-pr-90/validation-pipe.pyusing actual head functions extracted through AST, a successful cleanup stub, and a fake transport writer sending 1 MB into a real undrained stdout pipe. After SIGTERM, the cleanup diagnostic appeared but the process remained alive after 2 seconds and required SIGKILL. - The same harness with
--drainexited 143 promptly. With--no-flush, it also exited 143 promptly despite undrained stdout, establishing the flush as the cause. - Independent reviews found no additional blocking defects in registry escalation, deferred reaping, late directory registration, or CLI residual cleanup.
- The head remains unchanged. Semantic PR passed; GitHub reports a merge conflict with
main.
Not verified
- Full MCP transport, KWin E2E, and quality suites were not executed. The pipe reproduction isolates the actual shutdown functions with stubbed transport and cleanup.
- No baseline comparison was run for this new finding; required-check configuration remains unverified.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
The forced exit flushed stdout and printed its outcome to stderr before os._exit. A client that stopped draining stderr left that print blocked forever: the process never exited and later signals were ignored. Stdout is no longer flushed on the way out; the transport flushes each message itself. The stderr report (the cleanup outcome, plus the traceback on a server failure) is written raw to fd 2 from a daemon thread joined for at most 1 s, so neither a full nor a closed pipe can delay or skip the exit. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The undrained-stdout check waited for the unread byte count to reach the pipe's advertised capacity, which Linux page accounting does not always reach before the writer blocks. It now shrinks the server's stdout to one page and sends a single tools/list, whose response is far larger, so the transport writer is blocked without a byte-count guess. The full-stderr regression now covers stdin EOF as well as SIGTERM. Co-Authored-By: Claude Opus 5.5 <[email protected]>
There was a problem hiding this comment.
Previous findings
- Closed: P1 cleanup errors bypassed forced exit. The cleanup exception guard and independent directory-error handling remain intact; the injected cleanup-failure control passes.
- Closed: P1 blocked output prevented forced exit.
_finishno longer flushes stdout, and_report_exitwrites stderr on a daemon thread with a one-second maximum wait. Fail-before/pass-after pipe checks confirm the fix.
Verdict: GREEN (PR-ready: false: E2E checks are pending or failing) re-reviewed at 49c4500. No new blocking findings.
Verified
- Ran
python3 /tmp/issue-agent/review-pr-90/validation-pipe-rereview.pyagainst 260d7cd and this head. The previous head hung with undrained stdout or stderr. This head exited 143 on SIGTERM in 0.30 seconds with blocked stdout and 1.32 seconds with blocked stderr; EOF with blocked stderr exited 0 in 1.22 seconds, and a transport exception exited 1 in 1.22 seconds. - Healthy EOF and injected cleanup-exception controls passed. The harness uses actual revision shutdown functions with real threads and pipes, while stubbing transport and cleanup.
- Independent code review found no regression in the bounded reporting fix. Added integration tests cover unread stdout and full stderr on signal and EOF paths.
- The head remains unchanged and is MERGEABLE. CI lint across Python 3.12–3.15, type-check, build, documentation consistency, and Semantic PR passed.
Not verified
- Full KWin/MCP integration was not run locally.
- Several E2E jobs remain in progress. The openSUSE amd64 job failed at “Build the KWin test image”; quality checks and tests were skipped. The underlying image-build failure was not diagnosed.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
Owner
|
Merged, thanks @superbiche! For the record, here's what I changed in 260d7cd:
Each fix has a regression test. Your bounded exit-report follow-up (85fba7a) covered the pipe-blocking case. Nice catch. |
5 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When the MCP server process ends, it now stops the active session the way
session_stopwould. The same holds forkwin-mcp-clionSIGTERMandSIGHUP.Builds on #89: this branch contains its commit, so please review only the last commit,
fix(server): stop the session when the server exits.Before this change,
server.mainonly calledmcp.run(). When a client closed stdin, or a supervisor sentSIGTERMorSIGHUP, the server exited without stopping its session. The virtual KWin compositor, itsdbus-run-sessionwrapper and every launched app kept running.SIGTERM,SIGHUPandSIGINT. A clean EOF still exits 0; a signal exits with 128+signum (143 forSIGTERM). Once shutdown begins, further signals are ignored.session_stopruns on the same single tool thread as tool calls, never on the main thread. When that thread is free, the fullsession_stopruns. An explicitsession_stopbefore exit makes it a no-op.session_stopwould have removed for the session's retention settings. The exit does not wait for the tool to finish.session_connectare never recorded and never signalled.kwin-mcp-clistops the session onSIGTERMandSIGHUPas it already did on Ctrl-C. A signal arriving during teardown is deferred until the teardown finishes, so it cannot interrupt the app-kill loop.[Unreleased]entries under Fixed. No tool or parameter changes.Known limits, documented in the CHANGELOG:
SIGKILLstill runs no cleanup.Motivation
MCP clients usually end a server by closing stdio or sending
SIGTERM. Each such exit leaked a full KWin session plus the apps it launched, and they kept running until someone killed them by hand.How to Test
scripts/run-e2e-docker.sh: 380 passed, 1 skipped.The runner's quality step currently fails on
maintoo. The E2E image installs the dev group without the lockfile, so a fresh build resolves ty 0.0.85. That version reports 9deprecatedcontextmanager annotations in existing files. This branch adds none of them, and the lock-pinned ty 0.0.84 passes. I ran the runner's pytest step on the same image.New tests:
tests/e2e/test_exit_cleanup.pyhas 22 tests:SIGTERMand CLISIGTERMwith a virtual session and app;session_start, during a running tool, and during a tool that outlives the drain (also with the client's stderr closed);SIGTERMduring cleanup;tests/e2e/test_cli_signal_deferral.pycovers the CLI signal deferral.The EOF and
SIGTERMtests (server and CLI) fail onmain: the session process group outlives the server. The overrun, closed-stderr and registry tests each failed on the intermediate implementation they guard.Checklist
uv run ruff check .passesuv run ruff format --check .passesuv run ty checkpasses (ty check src testswith the locked ty 0.0.84; on the host,ty checkalso reports one pre-existing diagnostic indocker/e2e-environment.pyonmain)@superbiche · user · drafted with Claude Opus 5.5, reviewed before posting; the voice and decisions are mine.