diff --git a/apps/desktop/electron/backend-connection-state.test.ts b/apps/desktop/electron/backend-connection-state.test.ts index 674b96a819..c4cf43c0ce 100644 --- a/apps/desktop/electron/backend-connection-state.test.ts +++ b/apps/desktop/electron/backend-connection-state.test.ts @@ -1,7 +1,10 @@ import assert from 'node:assert/strict' +import { type ChildProcess, spawn } from 'node:child_process' +import { once } from 'node:events' import { test } from 'vitest' +import { waitForBackendExit } from './backend-child' import { createBackendConnectionState } from './backend-connection-state' type FakeProcess = { id: string } @@ -106,6 +109,116 @@ test('an invalidated attempt cannot attach a late-spawned process', () => { assert.equal(state.getProcess(), null) }) +test('a failed primary stop retains its child and blocks a replacement until retry exits', async () => { + const state = createBackendConnectionState() + + const child = spawn(process.execPath, ['-e', 'process.stdout.write("ready"); setInterval(() => {}, 1000)'], { + stdio: ['ignore', 'pipe', 'ignore'], + windowsHide: true + }) + + try { + await once(child.stdout!, 'data') + const attempt = state.startAttempt() + state.setPromise(attempt, Promise.resolve('ready')) + const owner = state.attachProcess(attempt, child) + assert.ok(owner) + const failure = new Error('primary is still running') + const calls: ChildProcess[] = [] + + const fail = async (current: ChildProcess): Promise => { + calls.push(current) + throw failure + } + + const stopping = state.stopProcess(fail) + assert.equal(state.getProcess(), null) + assert.equal(state.getPromise(), null) + assert.throws(() => state.startAttempt(), /has not stopped/) + assert.equal(state.stopProcess(fail), stopping) + await assert.rejects(stopping, error => error === failure) + state.invalidate() + assert.throws(() => state.startAttempt(), /has not stopped/) + assert.equal(child.exitCode, null) + assert.equal(child.signalCode, null) + + await state.stopProcess(async current => { + calls.push(current) + current.kill() + await waitForBackendExit(current, { + forceKillProcessTree: (): void => { + current.kill('SIGKILL') + }, + killGroup: (): void => { + current.kill('SIGKILL') + } + }) + }) + assert.deepEqual(calls, [child, child]) + assert.ok(child.exitCode !== null || child.signalCode !== null) + const replacement = state.startAttempt() + const connection = Promise.resolve('new') + assert.equal(state.setPromise(replacement, connection), true) + assert.equal(state.clearForCurrentProcess(owner), false) + assert.equal(state.getPromise(), connection) + } finally { + if (child.exitCode === null && child.signalCode === null) { + const closed = once(child, 'close') + child.kill() + await closed + } + } +}, 15_000) + +test('shutdown sees a spawned child while its persistent claim is still pending', async () => { + const state = createBackendConnectionState() + + const child = spawn(process.execPath, ['-e', 'process.stdout.write("ready"); setInterval(() => {}, 1000)'], { + stdio: ['ignore', 'pipe', 'ignore'], + windowsHide: true + }) + + const claim = deferred() + + try { + await once(child.stdout!, 'data') + const attempt = state.startAttempt() + + const claiming = state.claimProcess(attempt, child, async current => { + assert.equal(current, child) + assert.equal(state.getProcess(), child) + await claim.promise + }) + + assert.equal(state.getProcess(), child) + await state.stopProcess(async current => { + assert.equal(current, child) + current.kill() + await waitForBackendExit(current, { + forceKillProcessTree: (): void => { + current.kill('SIGKILL') + }, + killGroup: (): void => { + current.kill('SIGKILL') + } + }) + }) + assert.ok(child.exitCode !== null || child.signalCode !== null) + claim.resolve() + assert.equal(await claiming, null, 'a completed claim cannot revive the stopped generation') + assert.equal(state.getProcess(), null) + assert.doesNotThrow(() => state.startAttempt()) + } finally { + claim.resolve() + + if (child.exitCode === null && child.signalCode === null) { + const closed = once(child, 'close') + child.kill() + await closed + } + } +}, 15_000) + test('distinguishes a pending connection attempt from a cached settled descriptor', async () => { const state = createBackendConnectionState() const connection = deferred() diff --git a/apps/desktop/electron/connection-apply.test.ts b/apps/desktop/electron/connection-apply.test.ts index e19e90b4a2..2fe3a57ce6 100644 --- a/apps/desktop/electron/connection-apply.test.ts +++ b/apps/desktop/electron/connection-apply.test.ts @@ -4,6 +4,7 @@ import { applyConnectionChange, commitConnectionFailure, resolveTerminalConnection, + sshQuitShouldBlock, teardownSshState } from './connection-apply' @@ -91,6 +92,48 @@ describe('resolveTerminalConnection', () => { }) }) +describe('sshQuitShouldBlock', () => { + it('waits when connections exist and teardown has not finished', () => { + expect(sshQuitShouldBlock({ teardownDone: false, connectionCount: 1, bootstrapPending: 0, inFlight: null })).toBe( + true + ) + }) + + it('waits when bootstrap is still running', () => { + expect(sshQuitShouldBlock({ teardownDone: false, connectionCount: 0, bootstrapPending: 1, inFlight: null })).toBe( + true + ) + }) + + it('waits when the map is empty but a remote kill is already in flight', () => { + expect( + sshQuitShouldBlock({ + teardownDone: false, + connectionCount: 0, + bootstrapPending: 0, + inFlight: Promise.resolve() + }) + ).toBe(true) + }) + + it('does not block a second quit after teardown finished', () => { + expect( + sshQuitShouldBlock({ + teardownDone: true, + connectionCount: 1, + bootstrapPending: 1, + inFlight: Promise.resolve() + }) + ).toBe(false) + }) + + it('does not block quit when there is nothing to tear down', () => { + expect(sshQuitShouldBlock({ teardownDone: false, connectionCount: 0, bootstrapPending: 0, inFlight: null })).toBe( + false + ) + }) +}) + describe('teardownSshState', () => { it('terminates the owned remote backend before closing its tunnel and SSH transport', async () => { const events: string[] = [] diff --git a/apps/desktop/electron/connection-apply.ts b/apps/desktop/electron/connection-apply.ts index 03ad29741b..7180ecde24 100644 --- a/apps/desktop/electron/connection-apply.ts +++ b/apps/desktop/electron/connection-apply.ts @@ -61,6 +61,31 @@ async function resolveTerminalConnectionForSender(webContentsId, getTarget, ensu ) } +/** A second before-quit must still wait for an in-flight remote kill. + * + * teardownSshConnection deletes the sshConnections entry first, then + * SSH-execs kill. backendShutdown's finally() calls app.quit() and + * re-enters before-quit with an empty map. Without `inFlight`, Electron + * exits while disconnect is running and the detached serve --isolated + * stays at pid 1 (post-#95085 leftover on #91668: window X on Windows). */ +function sshQuitShouldBlock({ + teardownDone, + connectionCount, + bootstrapPending, + inFlight +}: { + teardownDone: boolean + connectionCount: number + bootstrapPending: number + inFlight: Promise | null +}): boolean { + if (teardownDone) { + return false + } + + return connectionCount > 0 || bootstrapPending > 0 || Boolean(inFlight) +} + async function teardownSshState(state, { cleanupRemote }) { // Remote process first, while the SSH channel can still exec kill. // Then drop the local forward and close the transport. Each step is @@ -91,5 +116,6 @@ export { commitConnectionFailure, resolveTerminalConnection, resolveTerminalConnectionForSender, + sshQuitShouldBlock, teardownSshState } diff --git a/apps/desktop/electron/update-count.test.ts b/apps/desktop/electron/update-count.test.ts new file mode 100644 index 0000000000..d3281f126c --- /dev/null +++ b/apps/desktop/electron/update-count.test.ts @@ -0,0 +1,302 @@ +import assert from 'node:assert/strict' +import { execFileSync } from 'node:child_process' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { test } from 'vitest' + +import { + compareApiUrl, + parseCompareBehindCount, + resolveBehindCount, + resolveCommitLogSelection, + shouldCountCommits +} from './update-count' + +function createTempGitRepo() { + const cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'hermes-update-count-')) + const git = (...args: string[]) => execFileSync('git', args, { cwd, encoding: 'utf8', timeout: 10_000 }).trim() + + try { + git('init', '--quiet') + git('config', 'commit.gpgSign', 'false') + git('config', 'core.hooksPath', '.git/no-hooks') + git('config', 'user.name', 'Hermes Test') + git('config', 'user.email', 'hermes@example.invalid') + + return { cwd, git } + } catch (error) { + fs.rmSync(cwd, { recursive: true, force: true }) + throw error + } +} + +// FAIL-BEFORE: pre-fix the function did `Number.parseInt(countStr) || 0` +// unconditionally, so a shallow checkout with no merge-base surfaced the bogus +// rev-list count (e.g. 12104) — #51922. Later the branch returned the sentinel +// `1`, which the UI rendered as a literal "1 change included" even when the +// true count was far higher (e.g. 90, or the real-world 61 in #84591). An +// update IS available here, but its exact size is unknown — the only honest +// value is `null`. +test('shallow checkout with no merge-base reports null (unknown count), not a fake 1', () => { + assert.equal( + resolveBehindCount({ + countStr: '12104', + currentSha: 'aaa', + targetSha: 'bbb', + isShallow: true + }), + null + ) +}) + +test('shallow checkout with no merge-base but identical SHA reports up-to-date', () => { + assert.equal( + resolveBehindCount({ + countStr: '12104', + currentSha: 'abc', + targetSha: 'abc', + isShallow: true + }), + 0 + ) +}) + +test('shallow local-ahead checkout reports up-to-date when origin is a known ancestor', () => { + assert.equal( + resolveBehindCount({ + countStr: '', + currentSha: 'local-child', + targetSha: 'origin-parent', + isShallow: true, + targetIsAncestorOfHead: true + }), + 0 + ) +}) + +test('shallow Git graph proves the remote tip is an ancestor of a local commit', () => { + const { cwd, git } = createTempGitRepo() + + try { + git('commit', '--allow-empty', '-m', 'origin tip') + + const targetSha = git('rev-parse', 'HEAD') + + git('update-ref', 'refs/remotes/origin/main', targetSha) + fs.writeFileSync(path.join(cwd, '.git', 'shallow'), `${targetSha}\n`) + git('commit', '--allow-empty', '-m', 'local child') + + const currentSha = git('rev-parse', 'HEAD') + + git('merge-base', '--is-ancestor', 'origin/main', 'HEAD') + assert.notEqual(currentSha, targetSha) + assert.equal( + resolveBehindCount({ + countStr: '', + currentSha, + targetSha, + isShallow: true, + targetIsAncestorOfHead: true + }), + 0 + ) + } finally { + fs.rmSync(cwd, { recursive: true, force: true }) + } +}, 30_000) + +test('shallow checkout with a merge-base does not trust an inflated rev-list count', () => { + const { cwd, git } = createTempGitRepo() + + try { + git('commit', '--allow-empty', '-m', 'root') + git('commit', '--allow-empty', '-m', 'ancestor') + + const redundantParent = git('rev-parse', 'HEAD') + + git('commit', '--allow-empty', '-m', 'installed head') + + const currentSha = git('rev-parse', 'HEAD') + const tree = git('rev-parse', 'HEAD^{tree}') + + const targetSha = execFileSync('git', ['commit-tree', tree, '-p', currentSha, '-p', redundantParent], { + cwd, + encoding: 'utf8', + input: 'remote merge\n', + timeout: 10_000 + }).trim() + + git('update-ref', 'refs/remotes/origin/main', targetSha) + + const completeCount = git('rev-list', 'HEAD..origin/main', '--count') + + assert.equal(completeCount, '1') + + fs.writeFileSync(path.join(cwd, '.git', 'shallow'), `${currentSha}\n`) + + assert.equal(git('rev-parse', '--is-shallow-repository'), 'true') + assert.equal(git('merge-base', 'HEAD', 'origin/main'), currentSha) + + const shallowCount = git('rev-list', 'HEAD..origin/main', '--count') + + assert.ok(Number.parseInt(shallowCount, 10) > Number.parseInt(completeCount, 10)) + assert.equal( + resolveBehindCount({ + countStr: shallowCount, + currentSha, + targetSha, + isShallow: true + }), + null + ) + } finally { + fs.rmSync(cwd, { recursive: true, force: true }) + } +}, 30_000) + +test('shallow checkout with a merge-base still uses presence-only status', () => { + assert.equal( + resolveBehindCount({ + countStr: '3', + currentSha: 'aaa', + targetSha: 'bbb', + isShallow: true + }), + null + ) +}) + +test('full (non-shallow) clone keeps the exact count path unchanged', () => { + assert.equal( + resolveBehindCount({ + countStr: '7', + currentSha: 'aaa', + targetSha: 'bbb', + isShallow: false + }), + 7 + ) +}) + +test('up-to-date full clone reports 0', () => { + assert.equal( + resolveBehindCount({ + countStr: '0', + currentSha: 'x', + targetSha: 'x', + isShallow: false + }), + 0 + ) +}) + +test('non-numeric count falls back to 0 (defensive, unchanged behaviour)', () => { + assert.equal( + resolveBehindCount({ + countStr: '', + currentSha: 'aaa', + targetSha: 'bbb', + isShallow: false + }), + 0 + ) +}) + +// shouldCountCommits gates the expensive `rev-list --count` in checkUpdates(). +// Every shallow graph is incomplete, so a visible merge-base is not enough to +// prove that the count is exact. +test('shallow checkouts skip the rev-list count', () => { + assert.equal(shouldCountCommits({ isShallow: true }), false) +}) + +test('full (non-shallow) clones run the rev-list count', () => { + assert.equal(shouldCountCommits({ isShallow: false }), true) +}) + +test('shallow commit logs select only the fetched remote tip', () => { + assert.deepEqual(resolveCommitLogSelection({ branch: 'main', isShallow: true }), { + limit: 1, + revision: 'origin/main' + }) +}) + +test('full-clone commit logs keep the complete behind range', () => { + assert.deepEqual(resolveCommitLogSelection({ branch: 'release', isShallow: false }), { + limit: 40, + revision: 'HEAD..origin/release' + }) +}) + +// The skip path produces an empty countStr; resolveBehindCount must NOT trust +// it and must fall through to the SHA compare (mirrors the live call site). +test('skipped-count path resolves via SHA compare, never via empty countStr', () => { + assert.equal( + resolveBehindCount({ + countStr: '', + currentSha: 'aaa', + targetSha: 'bbb', + isShallow: true + }), + null + ) + assert.equal( + resolveBehindCount({ + countStr: '', + currentSha: 'same', + targetSha: 'same', + isShallow: true + }), + 0 + ) +}) + +// --- compare-API recovery: the accuracy half of the class fix (#84591) --- + +const SHA_A = 'a'.repeat(40) +const SHA_B = 'b'.repeat(40) + +test('compareApiUrl builds the GitHub compare URL for HTTPS origins', () => { + assert.equal( + compareApiUrl({ + currentSha: SHA_A, + originUrl: 'https://github.com/NousResearch/hermes-agent.git', + targetSha: SHA_B + }), + `https://api.github.com/repos/NousResearch/hermes-agent/compare/${SHA_A}...${SHA_B}` + ) +}) + +test('compareApiUrl handles SSH origin forms', () => { + for (const originUrl of [ + 'git@github.com:NousResearch/hermes-agent.git', + 'ssh://git@github.com/NousResearch/hermes-agent.git', + 'git@github.com:NousResearch/hermes-agent' + ]) { + assert.equal( + compareApiUrl({ currentSha: SHA_A, originUrl, targetSha: SHA_B }), + `https://api.github.com/repos/NousResearch/hermes-agent/compare/${SHA_A}...${SHA_B}` + ) + } +}) + +test('compareApiUrl refuses non-GitHub remotes and partial SHAs', () => { + assert.equal(compareApiUrl({ currentSha: SHA_A, originUrl: 'https://gitlab.com/x/y.git', targetSha: SHA_B }), null) + assert.equal(compareApiUrl({ currentSha: 'abc123', originUrl: 'https://github.com/x/y.git', targetSha: SHA_B }), null) + assert.equal(compareApiUrl({ currentSha: SHA_A, originUrl: '', targetSha: SHA_B }), null) +}) + +test('parseCompareBehindCount returns ahead_by (the behind count)', () => { + assert.equal(parseCompareBehindCount({ ahead_by: 61, status: 'ahead' }), 61) + assert.equal(parseCompareBehindCount({ ahead_by: 0, status: 'behind' }), 0) +}) + +test('parseCompareBehindCount rejects malformed payloads', () => { + assert.equal(parseCompareBehindCount(null), null) + assert.equal(parseCompareBehindCount({}), null) + assert.equal(parseCompareBehindCount({ ahead_by: -2 }), null) + assert.equal(parseCompareBehindCount({ ahead_by: '61' }), null) + assert.equal(parseCompareBehindCount({ ahead_by: 1.5 }), null) + assert.equal(parseCompareBehindCount([]), null) +}) diff --git a/apps/desktop/electron/update-count.ts b/apps/desktop/electron/update-count.ts new file mode 100644 index 0000000000..ebca95271a --- /dev/null +++ b/apps/desktop/electron/update-count.ts @@ -0,0 +1,115 @@ +// Whether `git rev-list HEAD..origin/ --count` produces a meaningful +// number worth computing. Installer checkouts are shallow (`--depth 1`), so +// their visible graph is incomplete even when `merge-base` happens to find a +// common commit. A merge can expose ancestry that the local shallow boundary +// hides from HEAD, inflating the count with old commits. Exact counts are only +// trustworthy in full clones; shallow checkouts use presence-only status plus +// any positively proven local-ahead ancestry. +function shouldCountCommits({ isShallow }: { isShallow: boolean }): boolean { + return !isShallow +} + +// Resolve how many commits the local checkout is behind origin for the desktop +// update indicator. Shallow checkouts use SHA equality plus any positively +// proven local-ahead ancestry; exact counts remain exclusive to full clones. +function resolveBehindCount({ + countStr, + currentSha, + targetSha, + isShallow, + targetIsAncestorOfHead = false +}: { + countStr: string + currentSha: string + targetSha: string + isShallow: boolean + targetIsAncestorOfHead?: boolean +}): number | null { + if (!shouldCountCommits({ isShallow })) { + if (currentSha && targetSha && (currentSha === targetSha || targetIsAncestorOfHead)) { + return 0 + } + + // An update IS available, but its size is unknowable without a merge-base. + // Return null — never a numeric sentinel: the UI used to render the old + // `1` as a literal "1 change included" even when the true distance was + // far larger. null lets every surface say "update available" honestly. + return null + } + + return Number.parseInt(countStr, 10) || 0 +} + +// Shallow history can also contaminate the changelog range. Trust the fetched +// remote tip itself, but do not walk its ancestry. Full clones retain the +// detailed range used by the existing update overlay. +function resolveCommitLogSelection({ branch, isShallow }: { branch: string; isShallow: boolean }): { + limit: number + revision: string +} { + const remote = `origin/${branch}` + + return isShallow ? { limit: 1, revision: remote } : { limit: 40, revision: `HEAD..${remote}` } +} + +// When the local graph can't count (behind === null), the GitHub compare API +// still can: `GET /repos///compare/...` returns +// `ahead_by` — how many commits the remote tip is ahead of the local HEAD, +// i.e. exactly the behind count the shallow clone lost. Unauthenticated, no +// clone depth required. Pure URL builder + response parser here; the network +// call lives with the caller. +function compareApiUrl({ + currentSha, + originUrl, + targetSha +}: { + currentSha: string + originUrl: string + targetSha: string +}): string | null { + const sha = /^[0-9a-f]{40}$/i + + if (!sha.test(currentSha || '') || !sha.test(targetSha || '')) { + return null + } + + // Only GitHub remotes have a compare API. Reuse the canonical form the + // official-remote check produces: `github.com//`. + const canonical = canonicalRemoteForCompare(originUrl) + + if (!canonical) { + return null + } + + return `https://api.github.com/repos/${canonical}/compare/${currentSha}...${targetSha}` +} + +function canonicalRemoteForCompare(originUrl: string): string | null { + const value = String(originUrl || '').trim() + + const match = + /^git@github\.com:([^/]+\/[^/]+?)(?:\.git)?\/?$/i.exec(value) || + /^(?:ssh:\/\/git@|https:\/\/|http:\/\/)github\.com\/([^/]+\/[^/]+?)(?:\.git)?\/?$/i.exec(value) + + return match ? match[1] : null +} + +// `ahead_by` counts target commits not reachable from current — the behind +// count. `status` is "ahead" / "behind" / "diverged" / "identical" relative to +// current...target; any shape surprise returns null so the caller keeps the +// honest "update available" fallback instead of trusting a partial answer. +function parseCompareBehindCount(payload: unknown): number | null { + if (!payload || typeof payload !== 'object') { + return null + } + + const ahead = (payload as { ahead_by?: unknown }).ahead_by + + if (typeof ahead !== 'number' || !Number.isInteger(ahead) || ahead < 0) { + return null + } + + return ahead +} + +export { compareApiUrl, parseCompareBehindCount, resolveBehindCount, resolveCommitLogSelection, shouldCountCommits }