diff --git a/apps/desktop/electron/bundle-skew.test.ts b/apps/desktop/electron/bundle-skew.test.ts index d08aa97018..252fdd02f6 100644 --- a/apps/desktop/electron/bundle-skew.test.ts +++ b/apps/desktop/electron/bundle-skew.test.ts @@ -5,7 +5,7 @@ import { dirname, join } from 'node:path' import { afterAll, describe, expect, it } from 'vitest' -import { detectBundleSkew, isFallbackCommit, type RunGit, RUNTIME_PATHS } from './bundle-skew' +import { createBundleSkewChecker, detectBundleSkew, isFallbackCommit, type RunGit, RUNTIME_PATHS } from './bundle-skew' const REPO = '/repo' const STAMP = { commit: 'a'.repeat(40), source: 'ci' } @@ -44,6 +44,54 @@ function gitCounting(count: string): RunGit { return gitAnswering({ 'merge-base': { code: 0 }, 'rev-list': { stdout: count } }).git } +it('coalesces polls, backs off failures, and skips checks while an update owns the checkout', async () => { + let updating = false + let now = 0 + let finish: (value: { code: number; stderr: string; stdout: string }) => void + const calls: string[][] = [] + + const git: RunGit = (args, options) => { + calls.push(args) + expect(options.timeoutMs).toBeGreaterThan(0) + expect(options.timeoutMs).toBeLessThanOrEqual(5000) + expect(options.env?.GIT_NO_LAZY_FETCH).toBe('1') + + return new Promise(resolve => { + finish = resolve + }) + } + + const check = createBundleSkewChecker(STAMP, git, { isUpdating: () => updating, now: () => now }) + const quiet = { desktopCommitsBehind: null, outOfSync: false } + + const polls = Array.from({ length: 40 }, () => check(REPO)) + expect(calls).toHaveLength(1) + finish!({ code: 128, stderr: 'missing tree', stdout: '' }) + expect(await Promise.all(polls)).toEqual(polls.map(() => quiet)) + await check(REPO) + expect(calls).toHaveLength(1) + + now += 60_000 + const retry = check(REPO) + expect(calls).toHaveLength(2) + finish!({ code: 128, stderr: '', stdout: '' }) + await retry + + updating = true + expect(await check(REPO)).toEqual(quiet) + expect(calls).toHaveLength(2) + updating = false + const refreshed = check(REPO) + expect(calls).toHaveLength(3) // update invalidates the cached failure + finish!({ code: 128, stderr: '', stdout: '' }) + await refreshed + + const other = check('/another-checkout') + expect(calls).toHaveLength(4) + finish!({ code: 128, stderr: '', stdout: '' }) + await other +}) + describe('isFallbackCommit', () => { it('matches the all-zero placeholder at any stamp length', () => { expect(isFallbackCommit('0'.repeat(40))).toBe(true) diff --git a/apps/desktop/electron/bundle-skew.ts b/apps/desktop/electron/bundle-skew.ts index ecb4cae7a5..2a389e132a 100644 --- a/apps/desktop/electron/bundle-skew.ts +++ b/apps/desktop/electron/bundle-skew.ts @@ -56,8 +56,8 @@ export interface BundleSkewResult { export type RunGit = ( args: string[], - options: { cwd: string } -) => Promise<{ code: number; stderr: string; stdout: string }> + options: { cwd: string; env?: NodeJS.ProcessEnv; timeoutMs?: number } +) => Promise<{ code: number | null; stderr: string; stdout: string }> /** * The paths that actually reach the user: renderer sources, main-process @@ -82,6 +82,52 @@ export const RUNTIME_PATHS = [ const NOT_STALE: BundleSkewResult = { desktopCommitsBehind: null, outOfSync: false } +/** About/version polls share one check per checkout, including failed checks. */ +export function createBundleSkewChecker( + stamp: BundleSkewStamp | null, + runGit: RunGit, + { isUpdating, now = Date.now }: { isUpdating: () => boolean; now?: () => number } +): (repoRoot: string) => Promise { + const pending = new Map>() + const cached = new Map() + + return repoRoot => { + if (isUpdating()) { + cached.clear() + + return Promise.resolve(NOT_STALE) + } + + const running = pending.get(repoRoot) + + if (running) { + return running + } + + const previous = cached.get(repoRoot) + + if (previous && now() - previous.at < 30_000) { + return Promise.resolve(previous.result) + } + + const check = detectBundleSkew(stamp, runGit, repoRoot) + .then(result => { + if (isUpdating()) { + return NOT_STALE + } + + cached.set(repoRoot, { result, at: now() }) + + return result + }) + .finally(() => pending.delete(repoRoot)) + + pending.set(repoRoot, check) + + return check + } +} + /** Matches write-build-stamp.mjs's all-zero placeholder for non-git builds. */ export function isFallbackCommit(commit: string): boolean { return /^0{7,40}$/.test(commit) @@ -97,6 +143,15 @@ export async function detectBundleSkew( } try { + // A path-filtered walk of a tree:0 clone can otherwise fetch its entire + // missing history. Older Git ignores NO_LAZY_FETCH, so also bound and reap + // the process tree in execGit, not just the promise waiting for it. + const options = { + cwd: repoRoot, + env: { ...process.env, GIT_NO_LAZY_FETCH: '1', GIT_TERMINAL_PROMPT: '0', GCM_INTERACTIVE: 'Never' }, + timeoutMs: 5000 + } + // Exit 0 = ancestor, 1 = unrelated or diverged, anything else = git could // not answer (unknown object, shallow clone, not a repo). Only the first // makes the commit count below a statement about skew, and the other two @@ -108,17 +163,13 @@ export async function detectBundleSkew( // would be told "app build out of date" backwards. Ancestry is what makes // this a proof that the renderer PREDATES the tree, which is the claim the // warning actually makes. - const ancestry = await runGit(['merge-base', '--is-ancestor', stamp.commit, 'HEAD'], { - cwd: repoRoot - }) + const ancestry = await runGit(['merge-base', '--is-ancestor', stamp.commit, 'HEAD'], options) if (ancestry.code !== 0) { return NOT_STALE } - const result = await runGit(['rev-list', '--count', `${stamp.commit}..HEAD`, '--', ...RUNTIME_PATHS], { - cwd: repoRoot - }) + const result = await runGit(['rev-list', '--count', `${stamp.commit}..HEAD`, '--', ...RUNTIME_PATHS], options) if (result.code !== 0) { return NOT_STALE diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 749ccd0a52..6460d6613e 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -114,7 +114,7 @@ import { BROWSER_WINDOW_WIDTH, buildBrowserWindowUrl } from './browser-windows' -import { detectBundleSkew } from './bundle-skew' +import { createBundleSkewChecker } from './bundle-skew' import { detectBundleSwap, readBundleSwapStamp } from './bundle-swap' import { registerChatOnboardingWindow } from './chat-onboarding-window' import { provisionCliLinks } from './cli-provision' @@ -368,7 +368,7 @@ import { } from './native-oauth' import { runNativeLogin } from './native-oauth-login' import { loadNativeTokenSet, type NativeTokenStoreIo, persistNativeTokenSet } from './native-token-store' -import { planNoConsoleGitSpawn, setNoConsoleGitRoots, windowsGitHost } from './no-console-git' +import { execGit, planNoConsoleGitSpawn, setNoConsoleGitRoots, windowsGitHost } from './no-console-git' import { registerNativeNotifications } from './notification-ipc' import { isExpectedOauthNavigationAbort } from './oauth-navigation' import { serializeJsonBody, setJsonRequestHeaders } from './oauth-net-request' @@ -531,7 +531,7 @@ import { windowOpacityFor, windowOpacityOptions } from './translucency' -import { waitForUpdateClearance } from './update-gate' +import { updateGateReason, waitForUpdateClearance } from './update-gate' import { readLiveUpdateMarker, updateHandoffConflict, writeUpdateMarker } from './update-marker' import { resolveUpdaterMechanism, @@ -18265,8 +18265,14 @@ function resolveHermesVersion(scope: { connectionId?: string; profile?: string } // apps/desktop/, and warn when the running renderer is provably behind. // Fail-quiet: dev runs (no stamp), non-git builds, and shallow-clone gaps all // report in-sync rather than risk a false "your install is torn" warning. +const checkRendererSkew = createBundleSkewChecker( + INSTALL_STAMP, + (args, options) => execGit(resolveGitBinary(), args, options), + { isUpdating: () => updateGateReason(updateGateDeps()) !== null } +) + async function detectRendererSkew() { - return detectBundleSkew(INSTALL_STAMP, runGit, resolveUpdateRoot()) + return checkRendererSkew(resolveUpdateRoot()) } // Re-resolve the live Hermes version and push it into the native About panel diff --git a/apps/desktop/electron/no-console-git.test.ts b/apps/desktop/electron/no-console-git.test.ts index 11c21042ed..3c5cbd1b80 100644 --- a/apps/desktop/electron/no-console-git.test.ts +++ b/apps/desktop/electron/no-console-git.test.ts @@ -1,13 +1,15 @@ import assert from 'node:assert/strict' import { execFileSync } from 'node:child_process' import fs from 'node:fs' +import { connect } from 'node:net' import os from 'node:os' import path from 'node:path' -import { test } from 'vitest' +import { expect, test, vi } from 'vitest' import { CREATE_NO_WINDOW, + execGit, NO_CONSOLE_GIT_SCRIPT, planNoConsoleGitSpawn, resolveNoConsolePython, @@ -17,6 +19,67 @@ import { const gitArgs = ['-c', 'windows.appendAtomically=false', 'merge-base', '--is-ancestor', 'origin/main', 'HEAD'] const gitBin = 'C:\\Program Files\\Git\\cmd\\git.exe' +test('a timed-out git command reaps descendants, including the Windows Python host', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'git-timeout-')) + const pids = path.join(dir, 'pids.json') + const fixture = path.join(dir, 'hang.cjs') + + const listening = (port: number): Promise => + new Promise(resolve => { + const socket = connect(port, '127.0.0.1') + + const done = (alive: boolean) => { + socket.destroy() + resolve(alive) + } + + socket.once('connect', () => done(true)) + socket.once('error', () => done(false)) + socket.setTimeout(2000, () => done(true)) + }) + + fs.writeFileSync( + fixture, + ` + const { spawn } = require('node:child_process'); + const worker = String.raw\`const fs = require('node:fs'); + const server = require('node:net').createServer(socket => socket.end()); + server.listen(0, '127.0.0.1', () => fs.writeFileSync(process.argv[1], + JSON.stringify({ pids: [Number(process.argv[2]), process.pid], port: server.address().port })));\`; + spawn(process.execPath, ['-e', worker, process.argv[2], String(process.pid)], { stdio: 'inherit' }); + setInterval(() => {}, 1000); + ` + ) + + try { + if (process.platform === 'win32') { + const python = execFileSync('python3', ['-c', 'import sys; print(sys.executable)'], { encoding: 'utf8' }).trim() + vi.stubEnv('HERMES_DESKTOP_PYTHON', python) + } + + const success = await execGit(process.execPath, ['-e', 'process.stdout.write("ok")'], { timeoutMs: 5000 }) + expect(success).toMatchObject({ code: 0, stdout: 'ok' }) + + const error = await execGit(process.execPath, [fixture, pids], { timeoutMs: 5000 }).catch(error => error) + const { port } = JSON.parse(fs.readFileSync(pids, 'utf8')) + await vi.waitFor(async () => expect(await listening(port)).toBe(false), { timeout: 3000 }) + expect(error).toMatchObject({ code: 'ETIMEDOUT' }) + } finally { + if (fs.existsSync(pids)) { + for (const pid of JSON.parse(fs.readFileSync(pids, 'utf8')).pids) { + try { + process.kill(pid, 'SIGKILL') + } catch { + /* already reaped */ + } + } + } + + vi.unstubAllEnvs() + fs.rmSync(dir, { recursive: true, force: true }) + } +}, 20_000) + test('windows git spawn uses a CREATE_NO_WINDOW host and does not rewrite git argv', () => { const plan = planNoConsoleGitSpawn({ gitBin, diff --git a/apps/desktop/electron/no-console-git.ts b/apps/desktop/electron/no-console-git.ts index e969556651..dfeffe0e75 100644 --- a/apps/desktop/electron/no-console-git.ts +++ b/apps/desktop/electron/no-console-git.ts @@ -8,7 +8,7 @@ // unchanged. simple-git cannot take a creation flag, so its binary tuple is // [python, this host script]. -import { spawn, type SpawnOptions } from 'node:child_process' +import { execFile, spawn, type SpawnOptions } from 'node:child_process' import fs from 'node:fs' import os from 'node:os' import path from 'node:path' @@ -252,6 +252,9 @@ export function execGit( const spec = hiddenGitSpawnSpec(gitBin, args, { cwd: options.cwd, env: options.env, + // Timed commands own a POSIX group so a promisor fetch cannot outlive + // the git process. On Windows taskkill follows the Python host's tree. + detached: Boolean(options.timeoutMs) && process.platform !== 'win32', stdio: ['ignore', 'pipe', 'pipe'] }) @@ -260,6 +263,7 @@ export function execGit( let stdout = '' let stderr = '' let settled = false + let timeoutError: (NodeJS.ErrnoException & { stderr?: string }) | undefined const finish = (error?: Error, code: number | null = child.exitCode) => { if (settled) { @@ -268,6 +272,10 @@ export function execGit( settled = true + if (timer) { + clearTimeout(timer) + } + if (error) { reject(error) @@ -279,11 +287,40 @@ export function execGit( const timer = options.timeoutMs ? setTimeout(() => { - child.kill() - const error = new Error('git timed out') as NodeJS.ErrnoException & { stderr?: string } + timeoutError = Object.assign(new Error('git timed out'), { code: 'ETIMEDOUT', stderr }) - error.stderr = stderr - finish(error) + const done = () => { + // Pipes held by descendants must not keep Electron alive after a + // failed tree kill. The timeout remains a failure, never exit 0. + child.stdout?.destroy() + child.stderr?.destroy() + finish(timeoutError) + } + + if (process.platform === 'win32' && child.pid) { + execFile( + 'taskkill', + ['/PID', String(child.pid), '/T', '/F'], + { windowsHide: true, timeout: 5000 }, + error => { + if (error) { + child.kill('SIGKILL') + } + + done() + } + ) + } else { + try { + if (child.pid) { + process.kill(-child.pid, 'SIGKILL') + } + } catch { + child.kill('SIGKILL') + } + + done() + } }, options.timeoutMs) : null @@ -305,7 +342,10 @@ export function execGit( clearTimeout(timer) } - finish(undefined, code) + // On Windows wait for taskkill's completion, not just the root's exit. + if (!timeoutError) { + finish(undefined, code) + } }) }) }