Uh oh!
There was an error while loading. Please reload this page.
Stop child process spawns and kills from flashing console windows on Windows - #7154
Conversation
🦋 Changeset detectedLatest commit: 04f2049 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
taskkillviaexecFile+windowsHide: true—killProcessGroupandkillProcessGroupOnExitno longer route throughcmd.exe(exec), so Windows process-group kills stop spawning a console window.windowsHideonspawn— console-app children (e.g.git) are spawned hidden unless the caller explicitly setdetached: true, whose new-console intent is preserved.- Changeset —
patchfor@effect/platform-node-shared.
I verified the Windows mechanics against Node/libuv/MSDN sources. The execFile swap is the right call: execFile pipes stdio, so windowsHide maps to CREATE_NO_WINDOW (libuv applies it only when no stdio fd is inherited), suppressing the conhost window for taskkill; a child spawned with stdio: "inherit" from a console-attached parent still renders output because libuv then skips CREATE_NO_WINDOW entirely. Bare taskkill (no extension, no path) resolves via libuv's search_path in execFile exactly as it did through exec, and non-zero-exit error semantics (error.code) are preserved, so the Effect.fail behavior is unchanged. The detached === true exclusion is also sound — MSDN states CREATE_NO_WINDOW is ignored when combined with DETACHED_PROCESS, so hiding a detached child would be a silent no-op anyway. On non-Windows windowsHide is a documented no-op, and both spawn paths in this file (StandardCommand, PipedCommand) route through the changed options, so coverage is complete for this spawner. The node:child_process fork/spawn handler for detached defaults also lines up with the existing computed detached value.
No test was added — Windows-only behavior with no seam for asserting spawn options, and any test would not run on Linux CI; the author already flagged this. Mergeable as-is.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
7f241b5 to
31908c0CompareBundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
There was a problem hiding this comment.
ℹ️ No critical issues — the core Windows-console fix remains sound, and the new
windowsHideoption is deterministic and genuinely exercised by the added tests. Two minor notes inline.
Reviewed changes
- Added
CommandOptions.windowsHide— an independent Windows console-hiding option that defaults to hiding non-detached children (options.windowsHide ?? !detached). - Extracted
buildSpawnOptions(exported helper) and a sharedtaskkillwrapper used by both kill paths. - Added exact-output unit tests for
buildSpawnOptionscovering the Windows defaults and the independentwindowsHideoverride — these assertions can fail, so they pin the behavior. - Rewrote the changeset description to match the new option surface.
I re-traced the defaulting matrix against the previously-approved commit: every Windows case is unchanged, and the only non-Windows shift (windowsHidetrue→false when detached is unset) is a no-op since libuv ignores windowsHide on non-Windows platforms.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Checked the trackers before filing anything, which changed the plan. Both findings were already known in part. The uncached work-tree root is pingdotgg#2037 section 4 — closed in a maintenance sweep on the grounds that the analysed path was rewritten and the proposal was no longer the bottleneck, with an explicit invitation to reopen. The defect survived that rewrite under a new caller and is 86% of subprocess spawns, so reopening with measurements beats filing a fresh duplicate. The taskkill cost is pingdotgg#2537, already traced to the dependency and already fixed by Effect-TS/effect#7154, which ships in @effect/platform-node-shared@4.0.0-beta.107; the catalog here pins beta.103. Verified against the merge commit and the published tarball. What is left unreported is narrower than the earlier draft of this plan claimed: #7154 made each kill cheaper but did not stop the spawner killing a process that had already exited, twice, for any non-zero exit. That part is novel and worth an Effect issue on its own. Adds plans/upstream-reports.md with paste-ready drafts for each destination, and a note on why the technical record belongs on GitHub rather than Discord. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Type
Description
On Windows, spawning a child process or killing a process group can create a console window. In an app that polls providers and shells out to
git, this shows up as bursts of black windows flashing on screen.Two causes in
NodeChildProcessSpawner:spawnnever passedwindowsHide, which Node defaults tofalse, so spawning a console executable such asgitorghcreates aconhostwindow.killProcessGroupandkillProcessGroupOnExitruntaskkillthroughNodeChildProcess.exec, andexecalways wraps its argument in%ComSpec% /d /s /c. Every process group kill therefore spawns acmd.exe.The
execwrapper is easy to confirm:detached: trueis left alone. Windows gives a detached child its own console on purpose, so hiding it would break callers that want a visible console process.detachedalready defaults tofalseon Windows, so the noisy paths are still covered.Validation
pnpm checkpnpm lintpnpm vitest run --project @effect/platform-node-shared: this package already fails 66 of 114 on Windows. The same run on an unmodifiedmainfails identically, so none of those come from this change.I did not add a test. The package has no seam for asserting the options handed to
NodeChildProcess.spawn, and mocking the module looked out of keeping with the surrounding tests. Happy to add one if you would rather have it.Interactive verification in a real Windows console remains external.
Related
Reported downstream in pingdotgg/t3code#2537, where a 5 minute provider refresh and per-command git cleanup produce a repeating burst of flashes.