diff --git a/apps/server/src/process/taskkillAfterExit.test.ts b/apps/server/src/process/taskkillAfterExit.test.ts new file mode 100644 index 000000000..ce7e6049f --- /dev/null +++ b/apps/server/src/process/taskkillAfterExit.test.ts @@ -0,0 +1,77 @@ +// @effect-diagnostics nodeBuiltinImport:off - the test records what node:child_process starts. +import * as NodeChildProcess from "node:child_process"; + +import * as NodeServices from "@effect/platform-node/NodeServices"; +import { it } from "@effect/vitest"; +import * as Effect from "effect/Effect"; +import * as ChildProcess from "effect/unstable/process/ChildProcess"; +import * as ChildProcessSpawner from "effect/unstable/process/ChildProcessSpawner"; +import { HostProcessPlatform } from "@t3tools/shared/hostProcess"; +import { describe, expect } from "vite-plus/test"; + +interface InternalChildProcessPrototype { + spawn: (this: unknown, options: { readonly file?: string }) => unknown; +} + +/** Runs `effect` while recording the executable of every child process it starts. */ +const recordingStarts = (effect: Effect.Effect) => + Effect.acquireUseRelease( + Effect.sync(() => { + const target = NodeChildProcess.ChildProcess + .prototype as unknown as InternalChildProcessPrototype; + const inner = target.spawn; + const files: Array = []; + target.spawn = function record(this: unknown, options) { + files.push(options.file ?? ""); + return inner.call(this, options); + }; + return { files, restore: () => (target.spawn = inner) }; + }), + (recorder) => Effect.map(effect, (value) => ({ value, files: recorder.files })), + (recorder) => Effect.sync(recorder.restore), + ); + +const runNode = (script: string) => + Effect.scoped( + Effect.gen(function* () { + const spawner = yield* ChildProcessSpawner.ChildProcessSpawner; + const child = yield* spawner.spawn( + ChildProcess.make(process.execPath, ["-e", script], { stdout: "ignore", stderr: "ignore" }), + ); + return yield* child.exitCode; + }), + ); + +// Effect's spawner stops a process tree on Windows with `taskkill /pid /T /F`. +// patches/@effect__platform-node-shared@4.0.0-rc.115.patch carries the guard from +// Effect 4.0.4: no `taskkill` once the leader has exited, because its pid may +// already belong to an unrelated process. Without it, every child that exits +// non-zero started two `taskkill` runs against its dead pid. +describe.runIf(HostProcessPlatform.defaultValue() === "win32")("taskkill on Windows", () => { + it.live("does not run for a child that already exited with a failure", () => + Effect.gen(function* () { + const { value: code, files } = yield* recordingStarts(runNode("process.exit(3)")); + expect(code).toBe(3); + expect(files.filter((file) => file === "taskkill")).toEqual([]); + }).pipe(Effect.provide(NodeServices.layer)), + ); + + it.live("still stops a child that is running when its scope closes", () => + Effect.gen(function* () { + const { files } = yield* recordingStarts( + Effect.scoped( + Effect.gen(function* () { + const spawner = yield* ChildProcessSpawner.ChildProcessSpawner; + yield* spawner.spawn( + ChildProcess.make(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + stdout: "ignore", + stderr: "ignore", + }), + ); + }), + ), + ); + expect(files.filter((file) => file === "taskkill")).toEqual(["taskkill"]); + }).pipe(Effect.provide(NodeServices.layer)), + ); +}); diff --git a/apps/server/src/provider/testFixtures/sessionProcessTree.mjs b/apps/server/src/provider/testFixtures/sessionProcessTree.mjs index fba5eedfa..a0e2e7008 100644 --- a/apps/server/src/provider/testFixtures/sessionProcessTree.mjs +++ b/apps/server/src/provider/testFixtures/sessionProcessTree.mjs @@ -21,9 +21,12 @@ NodeFS.appendFileSync( `${role === "opencode" ? "provider" : role} ${process.pid}\n`, ); +// Hidden, because the detached dev server has no console: without it, the +// watcher it starts would open a console window of its own on Windows. const start = (childRole, options = {}) => NodeChildProcess.spawn(process.execPath, [import.meta.filename, childRole], { stdio: "ignore", + windowsHide: true, ...options, }); diff --git a/apps/server/src/vcs/VcsProcess.test.ts b/apps/server/src/vcs/VcsProcess.test.ts index 1d3705a9f..a697aceb9 100644 --- a/apps/server/src/vcs/VcsProcess.test.ts +++ b/apps/server/src/vcs/VcsProcess.test.ts @@ -283,6 +283,49 @@ describe("VcsProcess.run", () => { }), ); + it.effect("turns off GitHub CLI telemetry for every gh it starts, and only for gh", () => + Effect.gen(function* () { + const seen: Array = []; + const service = yield* VcsProcess.make.pipe( + Effect.provideService( + ProcessRunner.ProcessRunner, + ProcessRunner.ProcessRunner.of({ + run: (input) => + Effect.sync(() => { + seen.push(input); + return { + stdout: "", + stderr: "", + code: ChildProcessSpawner.ExitCode(0), + timedOut: false, + stdoutTruncated: false, + stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, + }; + }), + }), + ), + Effect.provide(GitWorkDepth.layerWith(1)), + ); + + yield* service.run({ ...baseInput, command: "gh", args: ["api", "user"] }); + yield* service.run({ + ...baseInput, + command: "gh", + args: ["api", "user"], + env: { GH_DEBUG: "" }, + }); + yield* service.run(baseInput); + + expect(seen.map((input) => input.env)).toEqual([ + { GH_TELEMETRY: "0" }, + { GH_DEBUG: "", GH_TELEMETRY: "0" }, + undefined, + ]); + }), + ); + it.effect("collects stdout", () => Effect.gen(function* () { const result = yield* run({ diff --git a/apps/server/src/vcs/VcsProcess.ts b/apps/server/src/vcs/VcsProcess.ts index 6a5e29334..4dcfce0bd 100644 --- a/apps/server/src/vcs/VcsProcess.ts +++ b/apps/server/src/vcs/VcsProcess.ts @@ -131,6 +131,11 @@ export const make = Effect.gen(function* () { argumentCount: input.args.length, }; + // `gh` 2.92 can start a detached `gh send-telemetry` child. On Windows that + // child has no console, so the `tzutil` it runs opens a visible console + // window. The server's own `gh` calls opt out of telemetry. + const env = input.command === "gh" ? { ...input.env, GH_TELEMETRY: "0" } : input.env; + const result = yield* processRunner .run({ command: input.command, @@ -139,7 +144,7 @@ export const make = Effect.gen(function* () { ...(input.spawnCwd !== undefined ? { spawnCwd: input.spawnCwd } : {}), ...(input.stdin !== undefined ? { stdin: input.stdin } : {}), ...(input.onStdoutChunk !== undefined ? { onStdoutChunk: input.onStdoutChunk } : {}), - ...(input.env !== undefined ? { env: input.env } : {}), + ...(env !== undefined ? { env } : {}), timeout: input.timeoutMs ?? DEFAULT_TIMEOUT_MS, maxOutputBytes: input.maxOutputBytes ?? DEFAULT_MAX_OUTPUT_BYTES, outputMode: input.outputMode ?? "truncate", diff --git a/docs/internals/windows-console-windows.md b/docs/internals/windows-console-windows.md index d93854b23..2f9afefc6 100644 --- a/docs/internals/windows-console-windows.md +++ b/docs/internals/windows-console-windows.md @@ -42,9 +42,10 @@ Node exposes no way to ask for only the first. Forcing the flag everywhere would the editor windows `externalLauncher.ts` opens, `explorer.exe`, and an `electron-updater` NSIS installer - none of which pass any spawn options of their own, so none of which could opt out. -So the hook decides per target. It resolves the executable the way `CreateProcess` would -(`PATH`, then `PATHEXT`), reads the `Subsystem` field out of the PE optional header, and applies -`windowsHide` only for `IMAGE_SUBSYSTEM_WINDOWS_CUI`. Anything graphical, or anything it cannot +So the hook decides per target. It resolves the executable the way `CreateProcess` would, with +the `PATH` and `PATHEXT` of the child's own environment, since a caller may add a directory only +the child sees (the device host does, for `adb`). It reads the `Subsystem` field out of the PE +optional header, and applies `windowsHide` only for `IMAGE_SUBSYSTEM_WINDOWS_CUI`. Anything graphical, or anything it cannot parse, is passed through untouched. Results are cached per resolved path. There is also an explicit opt-out for a console program that genuinely wants a window: diff --git a/fork-features.json b/fork-features.json index ed572b193..68d74cb25 100644 --- a/fork-features.json +++ b/fork-features.json @@ -606,6 +606,23 @@ ], "test": "apps/web/src/cloud/forkUnlinkThisPc.test.tsx", "patchesUpstream": true + }, + { + "name": "gh-telemetry-off", + "description": "Every gh the server starts runs with GH_TELEMETRY=0. gh 2.92 can start a detached gh send-telemetry child; on Windows that child has no console, so the tzutil it runs opened a visible console window. The user's own gh configuration is left alone.", + "keep": "The GH_TELEMETRY=0 entry that runUnbounded adds to the environment of every gh in VcsProcess.ts survives, with its comment; take upstream for the rest of the file.", + "files": ["apps/server/src/vcs/VcsProcess.ts"], + "test": "apps/server/src/vcs/VcsProcess.test.ts", + "testName": "turns off GitHub CLI telemetry for every gh it starts, and only for gh", + "patchesUpstream": true + }, + { + "name": "effect-taskkill-after-exit", + "description": "On Windows, Effect's child process spawner no longer runs taskkill against a pid whose process has already exited. Before, every child that exited non-zero started two taskkill runs on its dead pid, each with its own console host, and the pid could already belong to an unrelated process. The patch backports the guard Effect 4.0.4 ships to 4.0.0-rc.115.", + "keep": "The patch file and its patchedDependencies line in pnpm-workspace.yaml are ours whole while the fork is on @effect/platform-node-shared 4.0.0-rc.115; when upstream moves the fork to 4.0.4 or later, which carries the same guard, take upstream and drop both.", + "files": ["patches/@effect__platform-node-shared@4.0.0-rc.115.patch", "pnpm-workspace.yaml"], + "test": "apps/server/src/process/taskkillAfterExit.test.ts", + "patchesUpstream": true } ] } diff --git a/packages/shared/src/windowsConsole.test.ts b/packages/shared/src/windowsConsole.test.ts index 5c1d94ea2..fdd8dd331 100644 --- a/packages/shared/src/windowsConsole.test.ts +++ b/packages/shared/src/windowsConsole.test.ts @@ -145,6 +145,13 @@ describe("hideWindowsConsoleWindows", () => { expect(spawnOptionsFor(consoleExe).windowsHide).toBe(true); }); + it("hides a console child found only on the PATH its caller passed", () => { + // How `adb` starts: the device host puts the Android SDK on the child's + // PATH, which is the one Windows searches, not on this process's. + const env = { PATH: fixtures, PATHEXT: NodePath.extname(consoleExe) }; + expect(spawnOptionsFor("t3-console-fixture", { env }).windowsHide).toBe(true); + }); + it("leaves a graphical child untouched, so its window still appears", () => { // The whole point of reading the header. `windowsHide` is two libuv flags, // and the second one hides GUI windows: forcing it here would make an editor diff --git a/packages/shared/src/windowsConsole.ts b/packages/shared/src/windowsConsole.ts index 8ae1a8c51..7f48fbc25 100644 --- a/packages/shared/src/windowsConsole.ts +++ b/packages/shared/src/windowsConsole.ts @@ -73,10 +73,12 @@ export const allowConsoleWindow: unique symbol = Symbol.for( * * `file` is the executable as the caller named it, *not* resolved against * `PATH`. For `shell: true` Node has already rewritten it to `cmd.exe`. + * `envPairs` is the child's environment as `KEY=value` strings. */ interface InternalSpawnOptions { readonly file?: string | undefined; readonly cwd?: string | undefined; + readonly envPairs?: ReadonlyArray | undefined; readonly windowsHide?: boolean | undefined; readonly [allowConsoleWindow]?: boolean | undefined; } @@ -239,6 +241,26 @@ export const windowsExecutableSubsystem = ( return subsystem; }; +/** + * The `PATH` and `PATHEXT` of the child's own environment. Windows searches + * the child's `PATH`, not the parent's, so a caller that adds a directory - + * the Android SDK's `platform-tools` for `adb`, say - must be resolved there. + */ +const childSearchEnvironment = ( + envPairs: ReadonlyArray | undefined, +): NodeJS.ProcessEnv | undefined => { + if (envPairs === undefined) return undefined; + const environment: NodeJS.ProcessEnv = {}; + for (const pair of envPairs) { + // Start at 1: Windows keeps per-drive entries such as `=C:=C:\`. + const separator = pair.indexOf("=", 1); + if (separator === -1) continue; + const key = pair.slice(0, separator).toUpperCase(); + if (key === "PATH" || key === "PATHEXT") environment[key] = pair.slice(separator + 1); + } + return environment; +}; + /** Test seam: forgets what has been read, so a fixture path can be reused. */ export const clearWindowsSubsystemCache = (): void => { subsystemCache.clear(); @@ -275,7 +297,8 @@ export const hideWindowsConsoleWindows = ( if (options.windowsHide === true) return inner.call(this, options); if (options[allowConsoleWindow] === true) return inner.call(this, options); if (typeof options.file !== "string") return inner.call(this, options); - if (windowsExecutableSubsystem(options.file, options.cwd) !== "console") { + const environment = childSearchEnvironment(options.envPairs); + if (windowsExecutableSubsystem(options.file, options.cwd, environment) !== "console") { return inner.call(this, options); } return inner.call(this, { ...options, windowsHide: true }); diff --git a/patches/@effect__platform-node-shared@4.0.0-rc.115.patch b/patches/@effect__platform-node-shared@4.0.0-rc.115.patch new file mode 100644 index 000000000..f9ee8589e --- /dev/null +++ b/patches/@effect__platform-node-shared@4.0.0-rc.115.patch @@ -0,0 +1,49 @@ +diff --git a/dist/NodeChildProcessSpawner.js b/dist/NodeChildProcessSpawner.js +index 95d97c58cd42c5f10c445c289f792df668b1d153..6ca3473cf5e71cfb6fd178203bc424d5ea53e3f3 100644 +--- a/dist/NodeChildProcessSpawner.js ++++ b/dist/NodeChildProcessSpawner.js +@@ -44,9 +44,16 @@ const isProcessAlive = (childProcess, exitSignal) => { + return false; + } + }; +-const taskkill = (childProcess, onExit = () => {}) => NodeChildProcess.execFile("taskkill", ["/pid", String(childProcess.pid), "/T", "/F"], { +- windowsHide: true +-}, onExit); ++const taskkill = (childProcess, onExit = () => {}) => { ++ // After the leader exits, its PID may be reused by an unrelated process. ++ if (childProcess.exitCode !== null || childProcess.signalCode !== null) { ++ onExit(null); ++ return; ++ } ++ NodeChildProcess.execFile("taskkill", ["/pid", String(childProcess.pid), "/T", "/F"], { ++ windowsHide: true ++ }, onExit); ++}; + const make = /*#__PURE__*/Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; +diff --git a/src/NodeChildProcessSpawner.ts b/src/NodeChildProcessSpawner.ts +index 2077379773283926b715e29d6b6431bf7ba20ff9..bfb5c5f2f81f3733688087b943df3b565bff20b1 100644 +--- a/src/NodeChildProcessSpawner.ts ++++ b/src/NodeChildProcessSpawner.ts +@@ -95,13 +95,19 @@ const isProcessAlive = (childProcess: NodeChildProcess.ChildProcess, exitSignal: + const taskkill = ( + childProcess: NodeChildProcess.ChildProcess, + onExit: (error: NodeChildProcess.ExecException | null) => void = () => {} +-) => ++) => { ++ // After the leader exits, its PID may be reused by an unrelated process. ++ if (childProcess.exitCode !== null || childProcess.signalCode !== null) { ++ onExit(null) ++ return ++ } + NodeChildProcess.execFile( + "taskkill", + ["/pid", String(childProcess.pid!), "/T", "/F"], + { windowsHide: true }, + onExit + ) ++} + + const make = Effect.gen(function*() { + const fs = yield* FileSystem.FileSystem diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index edaf3adaa..53aef1220 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -91,6 +91,7 @@ packageExtensionsChecksum: sha256-k/dT9NFDl5hihRPaoFKeY11hzyutMFs5psfZLFiKJic= patchedDependencies: '@clerk/expo@4.6.8': a82bb41039ee88a290a87d4ab58be7be4c49fa338603b17ea646e1ec140a1cfb + '@effect/platform-node-shared@4.0.0-rc.115': a9500e78bfde6dcab2f9dc8d8ff54eeec04b95c0270f473eb240dfbb3c349242 '@effect/vitest@4.0.0-rc.115': 172bd95ef73a13d322888984415d5d18d9d2aa8e13fa529eac235efac837e902 '@expo/metro-config@57.0.12': 96f1a75347e6ea02dc4b7034ace815d8ee39e18b8166ebfb573d9e58328f0dc2 '@ff-labs/fff-node@0.9.4': c4e3cc2420ceb9dc650f9d189e9c24baf7e83f342998bacda5f459a4ce7927a8 @@ -512,7 +513,7 @@ importers: version: 4.0.0-rc.115(bufferutil@4.1.0)(effect@4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5))(redis@6.2.1)(utf-8-validate@6.0.6) '@effect/platform-node-shared': specifier: 4.0.0-rc.115 - version: 4.0.0-rc.115(bufferutil@4.1.0)(effect@4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5))(utf-8-validate@6.0.6) + version: 4.0.0-rc.115(patch_hash=a9500e78bfde6dcab2f9dc8d8ff54eeec04b95c0270f473eb240dfbb3c349242)(bufferutil@4.1.0)(effect@4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5))(utf-8-validate@6.0.6) '@ff-labs/fff-node': specifier: 0.9.4 version: 0.9.4(patch_hash=c4e3cc2420ceb9dc650f9d189e9c24baf7e83f342998bacda5f459a4ce7927a8) @@ -12340,7 +12341,7 @@ snapshots: transitivePeerDependencies: - encoding - '@effect/platform-node-shared@4.0.0-rc.115(bufferutil@4.1.0)(effect@4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5))(utf-8-validate@6.0.6)': + '@effect/platform-node-shared@4.0.0-rc.115(patch_hash=a9500e78bfde6dcab2f9dc8d8ff54eeec04b95c0270f473eb240dfbb3c349242)(bufferutil@4.1.0)(effect@4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5))(utf-8-validate@6.0.6)': dependencies: '@types/ws': 8.18.1 effect: 4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5) @@ -12351,7 +12352,7 @@ snapshots: '@effect/platform-node@4.0.0-rc.115(bufferutil@4.1.0)(effect@4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5))(redis@6.2.1)(utf-8-validate@6.0.6)': dependencies: - '@effect/platform-node-shared': 4.0.0-rc.115(bufferutil@4.1.0)(effect@4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5))(utf-8-validate@6.0.6) + '@effect/platform-node-shared': 4.0.0-rc.115(patch_hash=a9500e78bfde6dcab2f9dc8d8ff54eeec04b95c0270f473eb240dfbb3c349242)(bufferutil@4.1.0)(effect@4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5))(utf-8-validate@6.0.6) effect: 4.0.0-rc.115(patch_hash=0dfc4bb8ebd80fb3e06b91ef61346f5259517ab0f2437644fe95ae531084b1f5) redis: 6.2.1 undici: 8.10.2 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 0ccd9e443..d92b746f8 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -179,6 +179,7 @@ packageExtensions: patchedDependencies: "@clerk/expo@4.6.8": patches/@clerk__expo@4.6.8.patch + "@effect/platform-node-shared@4.0.0-rc.115": patches/@effect__platform-node-shared@4.0.0-rc.115.patch "@effect/vitest@4.0.0-rc.115": patches/@effect__vitest@4.0.0-rc.115.patch "@expo/metro-config@57.0.12": patches/@expo__metro-config@57.0.12.patch "@ff-labs/fff-node@0.9.4": patches/@ff-labs__fff-node@0.9.4.patch