Repository navigation
fix(windows): skip stale child process cleanup - #16850
Krarilotus wants to merge 1 commit into
Conversation
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped Windows process-cleanup bug fix delivered as a pinned dependency patch. It preserves tree termination for live leaders and POSIX behavior, while the workspace and lockfile changes only make the patch reproducible; no product defaults or static-analysis diagnostics are changed. You can add or adjust custom eligibility rules. Learn more. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughWindows task cleanup now skips PID-based tree termination if the process leader has exited. The workspace applies the patch to ChangesWindows task cleanup
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The Windows cleanup change is ready to merge after normal checks; no unresolved merge-blocking issue is established. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Full details: ApprovabilityExplanation This pull request patches the dependency
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Correction to my earlier policy acknowledgment: #17018 removed the custom Approvability check because “needs maintainer review” was misleadingly reported as requested changes. I verified the fresh upstream configuration and rebased this PR onto main ( The patch remains identical, and the pinned Effect The linked matched evidence is from the original commits, not a new measurement: Git volume stays at 40 per window, taskkill starts fall from 40 to 0, and measured runner/direct-command CPU falls from 3.907 to 1.414 seconds, excluding whole-PC/WMI CPU. Live cancellation still terminates descendants; the detached-orphan limitation is unchanged. Frozen-lockfile validation and integrity-verified published-package patch application passed again after the rebase. No further production change is indicated by the reviews so far. |
35f6135 to
9fc073b
Compare
|
Tested this on Windows 11 (Ryzen 7 5800X, git 2.55, Node 24.13) and it fixes a real stall. On the current nightly, a 90 s CPU profile of the desktop server showed the main thread blocked inside Benchmark: 60 runs of
The machine was at 60 to 70% CPU from other work, so absolute times are noisy. The launch counts aren't: every failing git call costs two extra The guard also matches what #16007 makes the same change against Benchmark script// Runs failing git commands through Effect's NodeChildProcessSpawner (as T3's git layer does)
// and counts taskkill launches plus main-thread time blocked inside spawn/execFile.
// usage: node --experimental-import-meta-resolve spawner-bench.mjs <repo root> <runs>
import cp from 'node:child_process';
import { syncBuiltinESMExports } from 'node:module';
import { pathToFileURL } from 'node:url';
const [root, runs = '60'] = process.argv.slice(2);
let taskkills = 0, blockedMs = 0, spawns = 0;
let depth = 0;
const timed = (name, fn) => function (...args) { if (depth > 0) return fn.apply(this, args); depth++; const s = performance.now(); try { return fn.apply(this, args); } finally { depth--; blockedMs += performance.now() - s; if (args[0] === "taskkill") taskkills++; else spawns++; } };
cp.execFile = timed('execFile', cp.execFile); cp.spawn = timed('spawn', cp.spawn); syncBuiltinESMExports();
const parent = pathToFileURL(root + '/apps/server/package.json').href;
const load = (s) => import(import.meta.resolve(s, parent));
const { Effect, Layer, Stream } = await load('effect');
const { ChildProcess, ChildProcessSpawner } = await load('effect/process').catch(() => load('effect/unstable/process'));
const NodeServices = await load('@effect/platform-node/NodeServices');
const program = Effect.gen(function* () {
const spawner = yield* ChildProcessSpawner.ChildProcessSpawner;
for (let i = 0; i < Number(runs); i++) {
yield* Effect.scoped(Effect.gen(function* () {
const child = yield* spawner.spawn(ChildProcess.make('git', ['config', '--get', 'no.such.key'], { cwd: root }));
yield* Stream.runDrain(child.all);
yield* child.exitCode;
}));
}
});
const t0 = performance.now();
const spawnerLayer = process.env.SPAWNER ? (await import(pathToFileURL(process.env.SPAWNER).href)).layer : null;
const layer = spawnerLayer ? Layer.merge(NodeServices.layer, spawnerLayer.pipe(Layer.provide(NodeServices.layer))) : NodeServices.layer;
await Effect.runPromise(program.pipe(Effect.provide(layer)));
await new Promise((r) => setTimeout(r, 3000)); // let fire-and-forget taskkills from exit handlers start
console.log(JSON.stringify({ spawner: process.env.SPAWNER ? "unpatched copy" : "installed", root, gitRuns: Number(runs), taskkillLaunches: taskkills, otherSpawns: spawns, mainThreadBlockedInLaunchMs: Math.round(blockedMs), wallMs: Math.round(performance.now() - t0) }));Run from the repo root with |
|
Rerun on a fresh boot of the same machine. The numbers in my comment above came from a machine with a kernel token leak that made every process launch slow, so their absolute times are inflated. Same benchmark, 60 runs of a failing
Each failing git command takes about 9 times as long without the patch, because the scope's release step waits for |
This branch has not been deployed
2 participants
Windows commands that exit nonzero currently launch
taskkill /T /Ftwice after Node has observed their exit: once from the Effect spawner's exit listener and once from scoped release. Expected Git probes such as a missingshow-ref --verify --quiettherefore incur process creation and cleanup overhead despite already being complete.Guard the shared Windows
taskkillhelper whenexitCodeorsignalCodeis non-null, completing its callback without launching a command. The owner is@effect/platform-node-shared, re-exported by@effect/platform-nodeand used by both T3's server and desktop. This uses T3's existing pinned-dependency patch mechanism rather than adding per-caller workarounds. The three changed files contain only the source/runtime patch, patch registration, and generated lockfile references; no dependency versions change and no tests or measurement scripts are added.Credit: @Alb11747 already proposed this guard in #16007, targeting
4.0.0-rc.115. That PR is currently conflicted with main. This current-main implementation targets the pinned4.0.1and supplies an independent matched Windows reproduction. Maintainers may prefer to use this evidence to update the earlier PR.This qualifies as the small focused obvious-bug exception in CONTRIBUTING.md: skip redundant cleanup of completed Windows children, preserve running-tree termination and all POSIX behavior, add no settings or polling-policy changes. Detached descendants that outlive their leader are not reclaimed by PID-based cleanup in either version. The guard avoids knowingly using a completed leader's stale PID; it does not eliminate the race if a live leader exits between the check and taskkill.
Validation on Windows 11, Ryzen 7800X3D / 16 logical threads, Node 24.19.0:
Disposable runtime experiment through the unchanged T3
ProcessRunner: completed exits 1/128 spawn 2 → 0 taskkill calls; exit 0 remains zero. Explicit kill after exit also stops launching stale cleanup. Live explicit kill, scope release, fiber interruption, and timeout all stop owned descendant heartbeats, with one taskkill instead of redundant post-exit calls. A detached/ignored-stdio orphan survives leader exit in both variants and is disposed by the experiment.Matched ABBA measurement: two 22-second windows per variant, 20 one-second ticks, each issuing
git rev-parse --is-inside-work-tree(exit 0) andgit show-ref --verify --quiet refs/heads/absent(exit 1) in the same synthetic repository. Imports/warmup excluded. All 240 direct command processes were captured with retained native handles andGetProcessTimes; runner CPU usesprocess.cpuUsage. Means per window:611132c171f335f6135456dbThis is a 63.8% reduction in measured direct workload CPU, not a whole-machine or installed-app comparison. WMI, Defender, conhost, secondary processes, and unrelated jobs are excluded; wall times vary with scheduling. No Git polling change is justified by this experiment. The installed app was not modified, restarted, or replaced.
Existing
apps/server/src/processRunner.test.tsandapps/server/src/vcs/VcsProcess.test.ts: 43 passed, using an external minimal Vitest configuration with the patched published dependency. Selected existing vendored spawner checks for spawn options, unread stdout, nonzero exit, invalid command/cwd: 6 passed, 72 unselected. No new tests were written or committed.pnpm install --lockfile-only --frozen-lockfile --ignore-scripts,git diff --check, and applying the patch against the exact published4.0.1package withgit apply --check: passed. Strict source typechecking with repository-pinned@types/node@24.12.4reports the same existingExecException/ExecFileExceptioncallback mismatch on baseline and fixed source. No full application build, repo-wide checks, or POSIX runtime run.Per-run numbers and reproducible disposable experiment. Evidence is hosted in the fork as release assets; no PR-only assets are committed to the production tree. Only synthetic technical diagnostics are included, with no raw ETL or private application data.
Implementation and verification: GPT-6.1-Sol, high reasoning effort, through the Codex harness in a visible T3 Code agent thread.
Rebase update (2026-10-08): rebased onto upstream main
a4c9494b0e36; the PR head is now9fc073bd67a2. The production patch is identical (git range-diff), and the pinned Effect package, shared spawner, T3 command runner, and relevant existing tests are unchanged. Frozen-lockfile validation and patch application against the integrity-verified published@effect/platform-node-shared@4.0.1passed again. The measurements and existing runtime checks above remain evidence from the original611132c171f3/35f6135456dbcommits; they have not been relabeled or represented as a new benchmark. The rebase also incorporates the upstream review-policy change in #17018; no review policy is changed by this PR.