fix: stop spawning a child process per file, and bound the codex version probe - #582
Merged
Merged
Conversation
churn.stripRepoPrefix answered "does this path exist" by shelling out to
`test -f`, once for every indexed file on every enrichment pass, and
gated the whole helper on `exec.LookPath("git")` for good measure. The
identical helper in internal/blame was already converted to os.Stat; its
comment even says "without spawning a process for every lookup". This
copy was missed.
Two consequences:
- Process fan-out. A large workspace pays thousands of child-process
spawns per pass to answer a question os.Stat answers inline. On
Windows each of those children is also handed a fresh console
window, because the call site never went through
platform.ConfigureBackgroundCommand.
- Silently wrong on Windows. There is no `test` executable there, so
every lookup failed, multi-repo paths were never stripped, and the
enricher walked away with no churn data at all.
os.Stat + Mode().IsRegular reproduces the `test -f` semantics exactly
(regular files only, symlinks followed), and needs nothing on PATH.
The LookPath gate goes with it: path stripping has nothing to do with
git being installed, and EnrichGraph already fails at its rev-parse
before reaching this code when git is absent.
The new test pins the property that broke on Windows — resolution works
with an empty PATH — rather than the incidental fact that `test` happens
to exist on POSIX CI.
The adapter resolved `codex` on PATH and ran `--version` with no timeout. A version banner is instant, so any delay means a wedged binary — and an unbounded Output() call blocks the install forever while leaving behind a child process that never exits and, on Windows, a console window the user has to close by hand. Give the probe a 5s deadline via CommandContext so a stuck binary is killed rather than waited on, and route it through platform.ConfigureBackgroundCommand so it does not allocate a console of its own when the caller has none. The failure posture is unchanged and already correct: a probe error reads as "unknown install" and leaves direct tool exposure on, so the deadline degrades to the existing safe default instead of silently downgrading the install. Covered by a test so it stays that way.
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.
Investigated #581 and fixed the two real defects it surfaced.
On the reported
findstr.exeGortex never invokes
findstr. It is not in the source, and there is no grep/ripgrep search fallback to degrade into:search_textis a trigram index inside the daemon, and every code-search path is in-process. The onergshell-out in the tree is a benchmark baseline adapter underbench/, which the daemon does not link.So the report's specific attribution does not hold. But the shape it describes — a daemon fanning out short-lived children with no console suppression and no deadline — is real, and two instances of it are fixed here.
1.
churnspawnedtest -fonce per indexed filestripRepoPrefixanswered "does this path exist" by shelling out:The identical helper in
internal/blamewas already converted toos.Stat, and its comment says so explicitly — "without spawning a process for every lookup". This copy was missed, sointernal/churnkept the old behaviour.Consequences:
os.Statanswers inline. The call site never went throughplatform.ConfigureBackgroundCommand, so on Windows each child also gets its own console window — matching the reported symptom of console windows appearing during activity, concurrent with thegitfan-out.testexecutable on Windows, so every lookup failed, multi-repo paths were never stripped, and the enricher produced no churn data at all.os.Stat+Mode().IsRegular()reproduces thetest -fsemantics exactly (regular files only, symlinks followed) and needs nothing on PATH. Theexec.LookPath("git")gate goes with it — path stripping does not depend on git being installed, andEnrichGraphalready fails at itsrev-parsebefore reaching this code when git is absent.2.
codex --versionprobe could hang foreverThe Codex adapter ran
exec.Command(path, "--version").Output()with no deadline. A version banner is instant, so any delay means a wedged binary — and the unbounded call blocks the install indefinitely while leaving a child that never exits. That is the issue's suggested fix #2, and the process signature it describes (zero CPU since start, neverexits).
Now bounded by a 5s
CommandContextdeadline and routed throughConfigureBackgroundCommand. The failure posture is unchanged and already correct: a probe error reads as "unknown install" and leaves direct tool exposure on, so the deadline degrades into the existing safe default rather than silently downgrading the install.Not changed
CREATE_NO_WINDOWon every spawn site. Already handled — 17 call sites route throughplatform.ConfigureBackgroundCommand, including everygitinvocation (gitcmd.Run), LSP servers, and the LLM subprocess providers. The two sites fixed here were the daemon-side gaps.gortex.exeprocesses. Left alone — no defect identified, and the reporter flagged it without claiming one. Worth a separate issue if it recurs with process-tree evidence.Verification
Both tests were confirmed to fail against the pre-fix code:
TestStripRepoPrefix_NeedsNoExternalBinariesfails withgot "myrepo/internal/foo.go", want "internal/foo.go"— the exact Windows breakage, reproduced on any platform by emptying PATH rather than by relying ontesthappening to exist on POSIX CI.TestCodexVersionOutput_TimesOutdoes not compile pre-fix; there was no deadline to assert on.go build ./...,go vet,golangci-lint run(0 issues), andgo test -raceacrossinternal/churn,internal/blame,internal/agents/..., andcmd/gortexall pass.internal/churn,internal/blame, andinternal/platformalso cross-compile forGOOS=windows.Closes #581