Skip to content

cli: Kill the serve-web process tree on idle timeout - #333719

Open
Kie (Kidsunbo) wants to merge 1 commit into
microsoft:mainfrom
Kidsunbo:main
Open

cli: Kill the serve-web process tree on idle timeout#333719
Kie (Kidsunbo) wants to merge 1 commit into
microsoft:mainfrom
Kidsunbo:main

Conversation

@Kidsunbo

Copy link
Copy Markdown

Fixes #332764

serve-web previously killed only the direct launcher process when reaching
the idle timeout, potentially leaving the Node.js server and its descendants
running.

Use kill_tree to terminate the complete process tree, wait for the launcher
to exit, and fall back to a forced kill after a timeout.

Copilot AI balanced review requested due to automatic review settings September 1, 2026 06:45
@Kidsunbo

Copy link
Copy Markdown
Author

Kie (Kie (@Kidsunbo)) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates serve-web idle-timeout shutdown to reduce orphaned server processes.

Changes:

  • Uses kill_tree for process-tree termination.
  • Waits five seconds before attempting a forced kill.
Suppressed comments (1)

cli/src/commands/serve_web.rs:947

  • kill_tree only sends SIGTERM, but this timeout waits for the launcher alone. If the shell exits while a descendant ignores or hangs on SIGTERM, child.wait() succeeds immediately and the forced-kill path is skipped; even on timeout, child.kill() targets only the launcher. The shutdown can therefore still leave server-main.js running. The fallback needs to force-kill the captured process tree/process group (and verify descendants exit), not just the direct child.
					if tokio::time::timeout(REAP_TIMEOUT, child.wait()).await.is_err() {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cli/src/commands/serve_web.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cli/src/commands/serve_web.rs:947

  • The timeout only waits for the launcher, not the descendants that kill_tree signaled. Because the shell can exit immediately while server-main.js is still shutting down or ignores SIGTERM, child.wait() can succeed before the five seconds elapse; the fallback is then skipped. Even when the timeout does fire, child.kill() force-kills only that launcher, recreating the orphan scenario this change is intended to prevent. The graceful tree operation needs to retain/observe the descendant set and force-kill the whole tree after the grace period, rather than keying escalation solely on the launcher's exit.
					if tokio::time::timeout(REAP_TIMEOUT, child.wait()).await.is_err() {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VSCode server LEAKs server-main.js process.

3 participants