test: reap the brokers and temp dirs a suite run leaves behind - #579
Open
xoonjaeho wants to merge 2 commits into
Open
test: reap the brokers and temp dirs a suite run leaves behind#579xoonjaeho wants to merge 2 commits into
xoonjaeho wants to merge 2 commits into
Conversation
`ensureBrokerSession` keys the app-server broker by cwd and spawns it detached, and `makeTempDir` hands every test a fresh cwd -- so a run strands one broker tree per test that reaches the app server. A broker exits only on a shutdown RPC or a signal, and the SessionEnd hook that would normally reap it never fires under `npm test`, so nothing cleans up. Repeated runs on one host reached 2257 processes against a ~330 baseline, plus 1443 orphaned mkdtemp dirs. Track the dirs `makeTempDir` creates and, at process exit, shut down any broker registered against them and remove the dir.
An EBUSY on Windows -- the broker's handle is not always released the instant taskkill returns -- would otherwise throw out of the exit handler and fail a test file that had passed.
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
1 participant
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.
makeTempDirgives every test a fresh cwd, andensureBrokerSessionkeys the app-server broker by cwd and spawns itdetached+unref. So every test that reaches the app server spawns a brand-new broker, the reuse branch can never hit, and nothing shuts any of them down: a broker exits only on abroker/shutdownRPC or a signal, and theSessionEndhook that would normally reap it never fires undernpm test.Measured on one Windows host after repeated suite runs:
node.exewith the test temp-dir prefixcodex-plugin-test-*temp dirscxc-*broker session dirsEach leaked test broker holds a 4-process unit (
broker → bash -c "codex app-server" → env node <fixture> → conhost).The change:
makeTempDirrecords the dirs it creates, and aprocess.on("exit")handler shuts down any broker registered against them and removes the dir.Verification — same file, same suite, on Windows:
node.execxc-*dirsZero growth held over three consecutive full-suite runs.
Known limits, both deliberate:
exitonly, so an interrupted (Ctrl-C) run still leaks. A signal handler here would also have to re-raise, and every normal run goes throughexit.terminateProcessTreereachestaskkillthrough$SHELL; under Git Bash the/PIDflag is rewritten into a path and the kill silently fails. fix: make cancel work under Git Bash on a non-English Windows (#423) #577 fixes that. This PR is still correct without it — the temp dirs go either way, and the kill works on Linux/macOS and from cmd — but on a Git Bash host it needs fix: make cancel work under Git Bash on a non-English Windows (#423) #577 to actually reap.