diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index 259386b9..3f388f1c 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -14,6 +14,7 @@ integration: `.ai/contexts/viewer-panel.md` ("Changes mode"). |---|---| | `git-changes.js` | Pure parser — no electron, no DOM, no fs. `require()`-d from `main.js` and from tests, same pattern as `remote-hosts.js` / `derive-project-path.js`. | | `git-changes-runner.js` | Runs the git commands, local or remote, behind one interface. | +| `run-to-exit.js` | The local spawn both runners share: settles on `close`, drains past the stdout cap instead of killing — see "A capped read waits for git to exit". | | `git-changes-target.js` | cwd resolution for the panel's IPCs, extracted out of `main.js` for testability (same rationale as `delete-session-target.js`). | | `git-changes-file.js` | The content pair and the write target behind the editable diff: the `:` guard, the repository-containment check, the read and the write. | | `git-changes-watch.js` | The registry behind `git-changes-watch`: arms `fs.watch`, debounces, re-arms after a rename, and reports the repo-relative path. | @@ -36,7 +37,7 @@ integration: `.ai/contexts/viewer-panel.md` ("Changes mode"). `createGitChangesRunner({kind, cwd, alias, exec, timeoutMs, fsOps})` → `{status(), diff(path, {staged, untracked}), isWorkTree()}`. `status()` runs three commands in parallel (`git status --porcelain=v2 --branch -uall -z`, `git diff --numstat -z`, `git diff --cached --numstat -z`), merges them, and reports `untrackedCollapsed` (see "Untracked files"). `diff()` runs `git diff [--cached] -- ` — or, with `untracked: true`, `git diff --no-index -- /dev/null ` (see "Untracked files") — capped at 512 KB (`MAX_DIFF_BYTES`) measured in UTF-8 bytes and cut on a line boundary, with a `truncated` flag. -- **Local** (`kind: 'local'`): `child_process.execFile('git', args, {cwd, timeout, maxBuffer})` — cwd is `execFile`'s own option, never a `-C` argument. No shell is invoked, so argument content cannot be interpreted as a command regardless of what it contains; timeout 10s. +- **Local** (`kind: 'local'`): `runToExit('git', args, {cwd, timeoutMs, maxBuffer})` (`run-to-exit.js`, a `child_process.spawn` with no shell) — cwd is the spawn's own option, never a `-C` argument. No shell is invoked, so argument content cannot be interpreted as a command regardless of what it contains; timeout 10s. See "A capped read waits for git to exit". - **Remote** (`kind: 'remote'`): the same ssh transport `remote-attach.js` already uses for the tmux probe/restore calls (`buildRemoteCommandArgs`, `defaultRunRemoteCommand`) — `ssh -o BatchMode=yes -o ConnectTimeout=5 -n "git -C '' '--literal-pathspecs' 'diff' '--' '' ..."`. Timeout 20s. This command string DOES run through a shell on the far end. - **`invoke(args, remoteOpts)`** is the single choke point both `status()` and `diff()` go through: it prepends `--literal-pathspecs` (`buildGitArgs`, see "Quoting rule") to every argv/command, and threads `remoteOpts.maxStdoutBytes` to the remote transport only (the local path's `execFile` `maxBuffer` already bounds it). @@ -90,9 +91,9 @@ git's default untracked mode; if the retry succeeds the result comes back `? dir/` rows, and a note saying the untracked listing is coarse. The retry is gated on the failure signature (`isStdoutCapFailure`: the remote -transport's own `stdout exceeded bytes`, or `execFile`'s -`stdout maxBuffer length exceeded` — both non-localized, one ours and one -Node's). Any other failure returns its own error untouched: retrying on every +transport's own `stdout exceeded bytes`, or the local runner's +`stdout maxBuffer length exceeded`, worded as Node's `execFile` words it — both +non-localized). Any other failure returns its own error untouched: retrying on every non-zero exit would tell a user whose repository is unreadable (`could not read directory: Permission denied`) that they have too many untracked files, and discard the real message on the way. If the retry itself @@ -351,7 +352,7 @@ of them produces `reason: 'not-a-repo'`. **A cwd that is gone is ruled out before the corroboration is trusted.** The walk below answers "no `.git` anywhere" for a path that does not exist, so a deleted worktree outside a repository would otherwise be reported as "not a git -repository". `execFile` happens to fail to spawn for such a cwd — code `-1`, not 128 — but the local and +repository". The local spawn happens to fail for such a cwd — code `-1`, not 128 — but the local and remote transports differ here (`git -C ` exits 128), so the check is an outcome of its own rather than something left to a code that happens not to match. It also decides the wording: `spawn git ENOENT` reads as "git is not @@ -723,3 +724,30 @@ hook), the scratch-repo test wrote `tracked.txt` into the outer repository's index and rewrote its local `user.email`; the test helper now drops `GIT_*` / `HUSKY*` for the scratch repo and disables its hooks, and the runner no longer trusts them either. + +## A capped read waits for git to exit + +Both local runners (`defaultRunGit` in `git-changes-file.js`, `defaultLocalExec` +in `git-changes-runner.js`) go through `runToExit` (`run-to-exit.js`), which +settles on the child's `close` and never kills a child for overrunning its +stdout cap. Past the cap it keeps reading and drops the bytes, so git runs to +its own end; the result carries `overflow: true` and the same +`stdout maxBuffer length exceeded` message `execFile` would give. + +The reason is Windows. There, a git that was killed leaves its working +directory busy for a moment after `execFile` has reported its exit: the +repository cannot be removed (`EBUSY` on `rmdir`) although neither the `git.exe` +on `PATH` nor the `mingw64\bin\git.exe` it starts as its own child is still +reported alive. A git that runs to its own end does not. On `windows-2022` the +over-the-cap subtest of `test/git-changes-file-real-git.test.js` hit `EBUSY` in +its cleanup in 24 of 8400 runs while the cap killed git, and in none of 8400 +once git was drained instead. + +A timeout still kills: a git that hangs cannot be waited for. The pipes are +closed on this side first, as `execFile` does, so a grandchild that still holds +them cannot delay `close`. On Windows a timed-out git therefore leaves its +working directory busy for a moment after the call has returned, as above. + +`fs.rmSync`'s `maxRetries` does not cover this failure on Node 20 and 22: their +recursive removal retries only after emptying a directory (`ENOTEMPTY`, +`EPERM`); an `EBUSY` on the first `rmdir` of a directory is thrown at once. diff --git a/git-changes-file.js b/git-changes-file.js index d2b67783..8f3f11e3 100644 --- a/git-changes-file.js +++ b/git-changes-file.js @@ -5,7 +5,7 @@ const realFs = require('fs'); const path = require('path'); const crypto = require('crypto'); -const { execFile } = require('child_process'); +const { runToExit } = require('./run-to-exit'); const { localGitEnv } = require('./git-changes-runner'); const { parseStatusPorcelainV2 } = require('./git-changes'); const { resolveOnDisk, isInsideDir } = require('./resolve-path-on-disk'); @@ -51,23 +51,10 @@ function requireLocalTarget(target) { return target; } -function defaultRunGit(args, { cwd, timeoutMs, maxBuffer }) { - return new Promise((resolve) => { - execFile('git', args, { cwd, env: localGitEnv(), timeout: timeoutMs, maxBuffer, encoding: 'buffer', windowsHide: true }, - (err, stdout, stderr) => { - const out = Buffer.isBuffer(stdout) ? stdout : Buffer.from(stdout || ''); - if (err) { - resolve({ - code: typeof err.code === 'number' ? err.code : -1, - stdout: out, - stderr: String(stderr || err.message || ''), - tooLarge: err.code === 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER', - }); - return; - } - resolve({ code: 0, stdout: out, stderr: '', tooLarge: false }); - }); - }); +async function defaultRunGit(args, { cwd, timeoutMs, maxBuffer }) { + const result = await runToExit('git', args, { cwd, env: localGitEnv(), timeoutMs, maxBuffer }); + if (result.code === 0) return { code: 0, stdout: result.stdout, stderr: '', tooLarge: false }; + return { code: result.code, stdout: result.stdout, stderr: result.stderr || result.message, tooLarge: result.overflow }; } // see .ai/contexts/changes-view.md ("Containment, and which path the write runs on") diff --git a/git-changes-runner.js b/git-changes-runner.js index 1474e6d2..4e1b5e44 100644 --- a/git-changes-runner.js +++ b/git-changes-runner.js @@ -2,7 +2,7 @@ 'use strict'; -const { execFile } = require('child_process'); +const { runToExit } = require('./run-to-exit'); const fs = require('fs'); const path = require('path'); const { defaultRunRemoteCommand } = require('./remote-attach'); @@ -184,17 +184,11 @@ function localGitEnv() { return env; } -function defaultLocalExec(args, { cwd, timeoutMs }) { - return new Promise((resolve) => { - execFile('git', args, { cwd, env: localGitEnv(), timeout: timeoutMs, maxBuffer: LOCAL_MAX_BUFFER, windowsHide: true }, - (err, stdout, stderr) => { - if (err) { - resolve({ code: typeof err.code === 'number' ? err.code : -1, stdout: stdout || '', stderr: stderr || err.message || String(err) }); - return; - } - resolve({ code: 0, stdout: stdout || '', stderr: stderr || '' }); - }); - }); +async function defaultLocalExec(args, { cwd, timeoutMs }) { + const result = await runToExit('git', args, { cwd, env: localGitEnv(), timeoutMs, maxBuffer: LOCAL_MAX_BUFFER }); + const stdout = result.stdout.toString('utf8'); + if (result.code === 0) return { code: 0, stdout, stderr: result.stderr }; + return { code: result.code, stdout, stderr: result.stderr || result.message }; } // Bounds what git wrote — see .ai/contexts/changes-view.md ("Bounded error messages") diff --git a/run-to-exit.js b/run-to-exit.js new file mode 100644 index 00000000..e674e0e1 --- /dev/null +++ b/run-to-exit.js @@ -0,0 +1,76 @@ +'use strict'; + +const { spawn } = require('child_process'); + +// see .ai/contexts/changes-view.md ("A capped read waits for git to exit") +function runToExit(file, args, { cwd, env, timeoutMs, maxBuffer }, spawnFn = spawn) { + return new Promise((resolve) => { + const command = [file, ...args].join(' '); + const stdoutChunks = []; + const stderrChunks = []; + let stdoutBytes = 0; + let stderrBytes = 0; + let overflow = false; + let timedOut = false; + let spawnError = null; + let settled = false; + let timer = null; + + const finish = (exitCode) => { + if (settled) return; + settled = true; + if (timer) clearTimeout(timer); + const stdout = Buffer.concat(stdoutChunks); + const stderr = Buffer.concat(stderrChunks).toString('utf8'); + let message = ''; + if (spawnError) message = spawnError.message; + else if (overflow) message = 'stdout maxBuffer length exceeded'; + else if (exitCode !== 0) message = `Command failed: ${command}`; + const code = spawnError || overflow || timedOut || typeof exitCode !== 'number' ? -1 : exitCode; + resolve({ code, stdout, stderr, overflow, timedOut, message }); + }; + + let child; + try { + child = spawnFn(file, args, { cwd, env, windowsHide: true }); + } catch (err) { + spawnError = err; + finish(null); + return; + } + + child.stdout.on('data', (chunk) => { + if (overflow) return; + if (stdoutBytes + chunk.length > maxBuffer) { + stdoutChunks.push(chunk.subarray(0, maxBuffer - stdoutBytes)); + stdoutBytes = maxBuffer; + overflow = true; + return; + } + stdoutChunks.push(chunk); + stdoutBytes += chunk.length; + }); + child.stderr.on('data', (chunk) => { + if (stderrBytes >= maxBuffer) return; + const kept = chunk.subarray(0, maxBuffer - stderrBytes); + stderrChunks.push(kept); + stderrBytes += kept.length; + }); + if (timeoutMs > 0) { + timer = setTimeout(() => { + timedOut = true; + child.stdout.destroy(); + child.stderr.destroy(); + child.kill(); + }, timeoutMs); + } + child.on('error', (err) => { + if (child.pid !== undefined) return; + spawnError = err; + finish(null); + }); + child.on('close', (exitCode) => finish(exitCode)); + }); +} + +module.exports = { runToExit }; diff --git a/test/git-changes-file-real-git.test.js b/test/git-changes-file-real-git.test.js index 843f5a63..1b910c1a 100644 --- a/test/git-changes-file-real-git.test.js +++ b/test/git-changes-file-real-git.test.js @@ -24,10 +24,7 @@ function mkTmp() { return fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'switchboard-gcf-real-'))); } -// The maxBuffer cap SIGTERMs an overrunning `git cat-file`, and execFile's -// callback runs before that child has been reaped (measured: exitCode null, -// killed true). On Windows a live process holds a handle on its working -// directory, so removing the scratch repo can race it — hence the retries. +// see .ai/contexts/changes-view.md ("A capped read waits for git to exit") function cleanup(dir) { fs.rmSync(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); } diff --git a/test/git-changes-runner-real-git.test.js b/test/git-changes-runner-real-git.test.js index ea6d502a..9e6713bc 100644 --- a/test/git-changes-runner-real-git.test.js +++ b/test/git-changes-runner-real-git.test.js @@ -42,9 +42,7 @@ function mkTmp() { return fs.realpathSync.native(dir); } -// Same race as test/git-changes-file-real-git.test.js: a git child killed by a -// cap is still terminating when the assertion returns, and on Windows it holds -// its working directory until it dies. +// see .ai/contexts/changes-view.md ("A capped read waits for git to exit") function cleanup(dir) { fs.rmSync(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); } diff --git a/test/run-to-exit.test.js b/test/run-to-exit.test.js new file mode 100644 index 00000000..7a128f53 --- /dev/null +++ b/test/run-to-exit.test.js @@ -0,0 +1,83 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawn } = require('child_process'); + +const { runToExit } = require('../run-to-exit'); + +function mkTmp() { + return fs.mkdtempSync(path.join(os.tmpdir(), 'switchboard-rte-')); +} + +function cleanup(dir) { + fs.rmSync(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); +} + +function node(script, opts = {}) { + let child = null; + const spawnFn = (file, args, options) => { + child = spawn(file, args, options); + return child; + }; + const promise = runToExit(process.execPath, ['-e', script], { cwd: opts.cwd || process.cwd(), env: process.env, timeoutMs: opts.timeoutMs || 10_000, maxBuffer: opts.maxBuffer || 1024 * 1024 }, spawnFn); + return { promise, child: () => child }; +} + +test('runToExit: a child that overruns the stdout cap is left to finish, and the call settles only once it has exited', async () => { + const tmp = mkTmp(); + try { + const marker = path.join(tmp, 'finished'); + const script = `process.stdout.write('x'.repeat(8192), () => setTimeout(() => { require('fs').writeFileSync(${JSON.stringify(marker)}, 'done'); }, 300));`; + const run = node(script, { maxBuffer: 1024 }); + const result = await run.promise; + assert.equal(result.overflow, true); + assert.equal(result.code, -1); + assert.equal(result.message, 'stdout maxBuffer length exceeded'); + assert.equal(result.stdout.length, 1024, 'what fits under the cap is kept, the rest is drained and dropped'); + assert.ok(fs.existsSync(marker), 'the child ran to its own end instead of being killed by the cap'); + assert.notEqual(run.child().exitCode, null, 'the child has exited by the time the call settles'); + } finally { cleanup(tmp); } +}); + +test('runToExit: a timeout kills the child and settles only after it has exited', async () => { + const run = node('setInterval(() => {}, 1000);', { timeoutMs: 200 }); + const result = await run.promise; + assert.equal(result.timedOut, true); + assert.equal(result.code, -1); + const child = run.child(); + assert.ok(child.exitCode !== null || child.signalCode !== null, 'the killed child has exited by the time the call settles'); +}); + +test('runToExit: a timeout settles even when a grandchild still holds the output pipes', async () => { + const script = "require('child_process').spawn(process.execPath, ['-e', 'setTimeout(() => {}, 3000)'], { stdio: 'inherit' }); setInterval(() => {}, 1000);"; + const started = Date.now(); + const result = await node(script, { timeoutMs: 300 }).promise; + assert.equal(result.timedOut, true); + assert.ok(Date.now() - started < 2500, `settled after ${Date.now() - started} ms, not when the grandchild let go of the pipes`); +}); + +test('runToExit: a spawn that never ran settles with code -1 and the spawn error', async () => { + const result = await runToExit(process.execPath, ['-e', ''], { cwd: path.join(os.tmpdir(), 'switchboard-rte-missing-dir-does-not-exist'), env: process.env, timeoutMs: 10_000, maxBuffer: 1024 }); + assert.equal(result.code, -1); + assert.match(result.message, /ENOENT/); +}); + +test('runToExit: a clean exit returns stdout and stderr with code 0', async () => { + const result = await node("process.stdout.write('out'); process.stderr.write('warn');").promise; + assert.equal(result.code, 0); + assert.equal(result.stdout.toString('utf8'), 'out'); + assert.equal(result.stderr, 'warn'); + assert.equal(result.overflow, false); + assert.equal(result.message, ''); +}); + +test('runToExit: a non-zero exit keeps its code and stderr', async () => { + const result = await node("process.stderr.write('fatal: nope'); process.exitCode = 3;").promise; + assert.equal(result.code, 3); + assert.equal(result.stderr, 'fatal: nope'); + assert.match(result.message, /^Command failed: /); +});