(test): drain a capped git instead of killing it, so its repository can be removed on Windows - #336
Conversation
…cannot hold the call
Removes the CI loop and the diagnostic scripts used to measure the EBUSY on windows-2022, and rewrites the changes-view section to what the runs showed: a killed git leaves its working directory busy for a moment after its exit is reported, although neither the PATH launcher nor the mingw64 git it starts is still reported alive; a git drained to its end does not (24 of 8400 runs before, 0 of 8400 after).
devsuitup
left a comment
There was a problem hiding this comment.
Adversarial review at 59103e4 — approved. CI all green on this head.
Equivalence with execFile, checked for every caller: cwd, env: localGitEnv(), windowsHide: true carried over, shell false, same argv; a missing git/cwd gives code -1 with the spawn message, a signal/overflow/timeout -1, a non-zero exit keeps its code; callers read only code, stderr, tooLarge (fed from overflow); defaultRunGit still returns raw Buffers (binary safe), defaultLocalExec decodes the concatenated Buffer (split multibyte safe); stderr is capped at maxBuffer, so no unbounded growth; finish settles once; the timer is always armed on these paths (10 s local, 20 s remote). run-to-exit, git-changes-file-real-git, git-changes-runner-real-git: 79 pass, 2 skipped.
Non-blocking:
- Nothing pins the callers to
runToExit. Withorigin/main'sexecFileversions ofgit-changes-file.jsandgit-changes-runner.jsput back, the two real-git files still pass (73/73) — the EBUSY is 24 in 8400.test/run-to-exit.test.jsproves "drain, don't kill" for the module, not that the runners use it; a revert would go green. One test throughdefaultRunGit/defaultLocalExecwith an injected spawn (or a source pin, the house pattern for wiring) would close that. The CI figures in the body are strong evidence for the fix itself; job links for the before/after runs would make them checkable. - Latency on overrun (
run-to-exit.js:42-51): a capped read used to fail at the cap; it now waits for git to exit, up to the 10 s timeout — a hugestatus -ualldelays the collapsed retry, a big blob delays "too large", by as much. The body says so; worth a line inchanges-view.md, and a test for timeout + overflow together (overflowis checked first infinish, so the retry still triggers — by reading). - Timeout path (
:59-66):child.kill()reaches thebin\git.exewrapper only; if the kill itself fails, nocloseand the promise never settles. Same weakness asexecFile, not a regression; a fallback settle would make it total. - Pre-existing, unchanged:
isStdoutCapFailurereadsstderr || message, so git stderr before an overrun hides the maxBuffer message and skips the retry. Carryingoverflowthrough would be more direct.
Nits: the title says (test): but the change is in the Changes panel's product path — (changes): let a capped git child run to exit would say what lands. Stale mentions of execFile's maxBuffer remain at .ai/contexts/changes-view.md:42 and git-changes-runner.js:212. The cap now counts bytes where the old string-mode local exec counted characters — harmless.
Fixes #332.
Defect
defaultRunGit(git-changes-file.js) anddefaultLocalExec(git-changes-runner.js) ran git throughexecFilewith amaxBuffer. On overrun,execFilekills the child. On Windows, a killed git leaves its working directory busy for a moment afterexecFilehas reported its exit, so a caller that removes the repository right after the call getsEBUSYonrmdir. The subtest "a blob over the cap is refused too…" is the only real-git test that overruns the cap (4096-byte blob, 1025-byte cap), and it is the one that failed its cleanup.The test's
rmSync(…, { maxRetries: 10, retryDelay: 100 })could not absorb it. On Node 20 and 22,fs.rmSyncrecursive goes throughlib/internal/fs/rimraf.js, whose_rmdirSyncretries onlyENOTEMPTY/EEXIST/EPERMafter emptying the directory. AnEBUSYfrom the directory's firstrmdiris thrown at once. The failing stack shows exactly that frame:_rmdirSync (node:internal/fs/rimraf:236:5), which is the firstrmdirSync(path).Evidence (windows-2022, temporary jobs on this branch, since removed)
pwsh, the shell the Test workflow uses, wheregitresolves toC:\Program Files\Git\bin\git.exe: the sameEBUSY: resource busy or locked, rmdir '…\switchboard-gcf-real-XXXX\repo'as in run 36548459342.git-changes-file.jsandgit-changes-runner.jsfrommain. Each job ran the subtestnode --test --test-name-pattern="blob over the cap", 4 processes at a time, on node 20 and 22.If the rate were unchanged (24/8400), the chance of seeing 0 in 8400 runs is about e^-24.
execFilegit killed by its timeout, thenfs.rmdirSync(repo)inside the callback. It gotEBUSYin 5 of 5 tries. The process tree showsbin\git.exe(the PIDexecFileholds) withmingw64\bin\git.exeas its child. At callback timeprocess.kill(pid, 0)reports neither of them alive. The same probe with the drained read gotENOTEMPTY(the directory is free) in 5 of 5 tries.Change
run-to-exit.js: aspawnthat settles onclose. Past the stdout cap it keeps reading and drops the bytes, so git exits on its own. The result carriesoverflow: trueand the samestdout maxBuffer length exceededmessage as before, soisStdoutCapFailureand thetoo-largerefusal are unchanged. A timeout still kills, after destroying the pipes on this side the wayexecFiledoes. Without that, a grandchild that still holds them would delayclose(covered by a test that fails without it).git-changes-file.js,git-changes-runner.js: both local runners go through it. Their result shapes (code, stdout, stderr, tooLarge) are the same as before.test/run-to-exit.test.js: overrun (the child runs to its own end and has exited when the call settles; this fails againstexecFile, which kills it), timeout, a timeout with a grandchild holding the pipes, spawn failure, clean exit, non-zero exit..ai/contexts/changes-view.md: new section "A capped read waits for git to exit", plus the references toexecFileupdated.Not verified
status -uallover 20 MB for the runner. The 10 s timeout still bounds it. Not measured on a large repository.npm run coveragesuite on windows-2022 passed on this branch. That is one run per node version, not a flake-rate measurement of the whole suite.