fix(functions): don't orphan the Python discovery server - #10893
Open
IzaakGough wants to merge 5 commits into
Open
fix(functions): don't orphan the Python discovery server#10893IzaakGough wants to merge 5 commits into
IzaakGough wants to merge 5 commits into
Conversation
The Python delegate's admin server shutdown could never force-kill a wedged server: the SIGKILL timer was cleared immediately after being set, the pid it held was the venv shell rather than Python underneath it, and nothing ran at all when the CLI itself was killed. A stuck server then left the deploy waiting on an exit event that never came. Spawn the server detached and kill its process group, bound every wait in the shutdown path, and force-kill tracked children on SIGINT, SIGTERM and SIGHUP. Fixes #10847
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request resolves an issue where the Python discovery admin server was left running after a killed or wedged deploy, causing subsequent deploys to hang. It introduces detached process spawning for the admin server, process group tracking, and a robust shutdown sequence that escalates to force-killing the process group on termination signals or timeouts. The reviewer provided valuable feedback, identifying a potential 15-second hang if the child process exits before shutdown is initiated, and suggesting a guard against zero or negative PIDs in killProcessTree to prevent the CLI from accidentally terminating itself.
Attach the exit/error listeners at spawn time rather than in shutdownAdmin. Neither event replays, so a discovery server that died before shutdown ran (a venv that fails to activate, a missing interpreter) left a listener that could never fire, stalling shutdown for the full 15s timeout, skipping the untrack, force-killing a reaped pid, and logging a misleading "survived being force-killed". Also guard killProcessTree against a zero or negative pid: callers filter those today, but the function is exported and process.kill(-0, ...) would signal the CLI's own process group.
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
2 participants
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.
Fixes #10847
The Python discovery admin server (
serving.py) could survive a deploy:serveAdmin's shutdown logic cleared its force-kill timer immediately after setting it, so theSIGKILLfallback never ran, and it waited onexit/errorwith no timeout. It also only signalled the shell thatrunWithVirtualEnvspawns, not the Python process underneath. An orphaned server kept the discovery port bound, so later deploys hung onconnect ETIMEDOUT.Shutdown now escalates properly:
/__/quitquitquitwith a timeout, then a force-kill of the child's process group (taskkill /T /Fon Windows), with an overall cap so a wedged server can never hang the deploy. The child is spawned detached so the whole group can be killed, and is tracked soSIGINT/SIGTERM/SIGHUPand process exit clean it up if the CLI itself goes away, which is what left servers behind on CI cancellation.