diff --git a/src/relay/node-pty-binding-survey.test.ts b/src/relay/node-pty-binding-survey.test.ts index b51a57fd4a8..8787713c117 100644 --- a/src/relay/node-pty-binding-survey.test.ts +++ b/src/relay/node-pty-binding-survey.test.ts @@ -1,4 +1,12 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { + chmodSync, + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync +} from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import process from 'node:process' @@ -7,6 +15,7 @@ import { isFlattenedNodePtyLoaderMessage } from '../main/orcad/node-pty-loader-d import { collectNodePtyUnavailableDiagnosis, readNodeGypBuildRecord, + resolveNodePtyInstallDir, surveyNodePtyBinding } from './node-pty-binding-survey' import { formatNodePtyUnavailableMessage } from './node-pty-unavailable-diagnosis' @@ -73,6 +82,26 @@ describe('readNodeGypBuildRecord', () => { }) }) +describe('resolveNodePtyInstallDir', () => { + it('finds the install an ancestor node_modules supplies, as the bare import does', () => { + // realpath: the resolver answers the real path, and macOS tmpdir is a link. + const root = realpathSync(mkdtempSync(join(tmpdir(), 'orca-node-pty-'))) + roots.push(root) + const installDir = join(root, 'node_modules', 'node-pty') + mkdirSync(installDir, { recursive: true }) + writeFileSync(join(installDir, 'package.json'), '{}\n') + + expect(resolveNodePtyInstallDir(join(root, 'relay', 'abc123'))).toBe(installDir) + }) + + it('answers nothing when no node_modules up the tree holds node-pty', () => { + const root = mkdtempSync(join(tmpdir(), 'orca-node-pty-')) + roots.push(root) + + expect(resolveNodePtyInstallDir(root)).toBeNull() + }) +}) + describe('collectNodePtyUnavailableDiagnosis', () => { it("recovers the dynamic loader's own words that node-pty threw away", async () => { // The whole defect in one assertion: what reaches the relay is FLATTENED, which names @@ -107,17 +136,61 @@ describe('collectNodePtyUnavailableDiagnosis', () => { } }, 20_000) - it('reports an unlocatable install as unverifiable, not as a diagnosis', async () => { + it('diagnoses a node-pty directory that does not exist instead of calling it unverifiable (#20386)', async () => { + // What a Linux host without a compiler gets: the deploy reinstalls with node-pty removed. + const root = mkdtempSync(join(tmpdir(), 'orca-node-pty-')) + roots.push(root) + const missingDir = join(root, 'node-pty') const diagnosis = await collectNodePtyUnavailableDiagnosis({ - nodePtyDir: null, - error: new Error(FLATTENED) + nodePtyDir: missingDir, + error: new Error(`no node-pty at ${join(missingDir, 'lib', 'index.js')}`) }) - expect(diagnosis.status).toBe('unverifiable') - const text = formatNodePtyUnavailableMessage(diagnosis) - expect(text).toContain('could not establish why') - // It still has to be reportable: the raw error is the only thing an issue can quote. - expect(text).toContain(FLATTENED) - }) + expect(diagnosis.status).toBe('blocked') + expect(['toolchain_missing', 'dependency_missing']).toContain(diagnosis.reason) + expect(diagnosis.survey).toMatchObject({ installed: false, bindingPath: null }) + expect(diagnosis.toolchain === null).toBe(process.platform !== 'linux') + expect(formatNodePtyUnavailableMessage(diagnosis)).toContain( + `node-pty is not installed at ${missingDir}` + ) + }, 20_000) + + it.skipIf(process.platform === 'win32')( + 'treats a node-pty link whose target is gone as not installed', + async () => { + const root = mkdtempSync(join(tmpdir(), 'orca-node-pty-')) + roots.push(root) + const linked = join(root, 'node-pty') + symlinkSync(join(root, 'gone'), linked) + const diagnosis = await collectNodePtyUnavailableDiagnosis({ nodePtyDir: linked }) + expect(diagnosis.survey).toMatchObject({ installed: false, bindingPath: null }) + }, + 20_000 + ) + + it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'keeps a directory it was refused as unverifiable rather than absent', + async () => { + const root = mkdtempSync(join(tmpdir(), 'orca-node-pty-')) + roots.push(root) + const locked = join(root, 'locked') + mkdirSync(join(locked, 'node-pty'), { recursive: true }) + chmodSync(locked, 0o000) + try { + const diagnosis = await collectNodePtyUnavailableDiagnosis({ + nodePtyDir: join(locked, 'node-pty'), + error: new Error(FLATTENED) + }) + expect(diagnosis.status).toBe('unverifiable') + expect(diagnosis.detail).toContain('EACCES') + const text = formatNodePtyUnavailableMessage(diagnosis) + expect(text).toContain('could not establish why') + // It still has to be reportable: the raw error is the only thing an issue can quote. + expect(text).toContain(FLATTENED) + } finally { + chmodSync(locked, 0o755) + } + } + ) it('probes the host toolchain only when nothing was compiled', async () => { const diagnosis = await collectNodePtyUnavailableDiagnosis({ diff --git a/src/relay/node-pty-binding-survey.ts b/src/relay/node-pty-binding-survey.ts index 805527f1aad..16a051c2302 100644 --- a/src/relay/node-pty-binding-survey.ts +++ b/src/relay/node-pty-binding-survey.ts @@ -19,8 +19,9 @@ * Every step is best-effort and failure-tolerant: whatever cannot be established is * reported as unestablished rather than guessed (docs/reference/ssh-execution-boundary.md). */ -import { existsSync, readFileSync } from 'node:fs' -import { join } from 'node:path' +import { existsSync, readFileSync, statSync } from 'node:fs' +import { createRequire } from 'node:module' +import { dirname, join } from 'node:path' import { release } from 'node:os' import process from 'node:process' import { runProcess } from '../shared/child-process/run-process' @@ -85,6 +86,7 @@ export function surveyNodePtyBinding( const built = readNodeGypBuildRecord(nodePtyDir) return { moduleDir: nodePtyDir, + installed: true, bindingPath, searched, builtNodeAbi: built.nodeAbi, @@ -181,6 +183,34 @@ async function probeRelayBuildToolchain( } } +/** The install a bare `node-pty` import from `fromDir` loads — Node walks up, so it can be an ancestor's. */ +export function resolveNodePtyInstallDir(fromDir: string): string | null { + try { + return dirname(createRequire(join(fromDir, 'relay.js')).resolve('node-pty/package.json')) + } catch { + return null + } +} + +// Only ENOENT is absence — the relay observing its own host. `stat` judges the directory the +// loader reads, so a dangling link is absent; `existsSync` would also answer false for EACCES. +function readNodePtyDirPresence( + nodePtyDir: string +): 'present' | 'absent' | { unverifiable: string } { + try { + statSync(nodePtyDir) + return 'present' + } catch (error) { + const code = error instanceof Error && 'code' in error ? String(error.code) : null + if (code === 'ENOENT') { + return 'absent' + } + return { + unverifiable: `the relay could not read its node-pty install directory (${code ?? readErrorMessage(error) ?? 'unknown error'})` + } + } +} + function readErrorMessage(error: unknown): string | null { if (error instanceof Error) { return error.message @@ -193,21 +223,32 @@ function readErrorMessage(error: unknown): string | null { * one's answer. Called only on the failure path, so a spawn that works pays nothing. */ export async function collectNodePtyUnavailableDiagnosis(options: { - nodePtyDir: string | null + nodePtyDir: string error?: unknown }): Promise { const abi = detectNativeHostAbi() const host: NodePtyUnavailableHost = { ...abi, nodeVersion: process.version } const requireError = readErrorMessage(options.error) - if (!options.nodePtyDir) { + const presence = readNodePtyDirPresence(options.nodePtyDir) + if (typeof presence === 'object') { return diagnoseNodePtyUnavailable({ host, survey: null, requireError, - unverifiableBecause: 'the relay could not locate its node-pty install directory' + unverifiableBecause: presence.unverifiable }) } - const survey = surveyNodePtyBinding(options.nodePtyDir, host) + const survey: NodePtyBindingSurvey | null = + presence === 'absent' + ? { + moduleDir: options.nodePtyDir, + installed: false, + bindingPath: null, + searched: [], + builtNodeAbi: null, + builtArch: null + } + : surveyNodePtyBinding(options.nodePtyDir, host) const probed = survey?.bindingPath ? await probeNodePtyLoader(options.nodePtyDir) : {} const toolchain = survey && !survey.bindingPath ? await probeRelayBuildToolchain(host.platform) : null diff --git a/src/relay/node-pty-unavailable-diagnosis.test.ts b/src/relay/node-pty-unavailable-diagnosis.test.ts index 7bed6ef256e..ca661e4bd2e 100644 --- a/src/relay/node-pty-unavailable-diagnosis.test.ts +++ b/src/relay/node-pty-unavailable-diagnosis.test.ts @@ -27,6 +27,7 @@ const SEARCHED = ['build/Release', 'build/Debug', 'prebuilds/linux-x64'] const INSTALLED: NodePtyBindingSurvey = { moduleDir: MODULE_DIR, + installed: true, bindingPath: `${MODULE_DIR}/build/Release/pty.node`, searched: SEARCHED, builtNodeAbi: null, @@ -140,6 +141,53 @@ describe('diagnoseNodePtyUnavailable', () => { }) expect(present.reason).toBe('dependency_missing') expect(formatNodePtyUnavailableMessage(present)).not.toContain('apt-get') + expect(formatNodePtyUnavailableMessage(present)).toContain( + 'build tools needed to compile it are present' + ) + }) + + it('never claims the build tools are present when no toolchain probe answered', () => { + // docs/reference/ssh-execution-boundary.md: a probe that timed out established nothing. + const linuxUnchecked = message({ survey: NOTHING_INSTALLED, toolchain: null }) + expect(linuxUnchecked).not.toContain('are present') + expect(linuxUnchecked).toContain('could not be checked') + expect(linuxUnchecked).toContain('Reconnect to reinstall') + + // Off Linux the probe never runs: node-pty ships prebuilds, so tools are beside the point. + const macos = message({ + host: { ...UBUNTU_2004, platform: 'darwin', libc: 'none', glibcVersion: null }, + survey: NOTHING_INSTALLED, + toolchain: null + }) + expect(macos).not.toContain('build tools') + expect(macos).toContain('Reconnect to reinstall') + }) + + it('names an absent node-pty directory as not installed and still offers the build tools (#20386)', () => { + // The no-toolchain deploy removes node-pty outright, so there is no directory to search. + const text = message({ + survey: { ...NOTHING_INSTALLED, installed: false, searched: [] }, + toolchain: toolchain(['python3']) + }) + expect(text).toContain(`node-pty is not installed at ${MODULE_DIR}`) + expect(text).toContain('sudo apt-get install -y build-essential python3') + expect(text).not.toContain('reconnect to retry') + }) + + it('does not blame the build tools for an absent directory on a host that has them', () => { + // The node-pty-less reinstall also leaves no directory when it fails for a non-toolchain + // reason (ENOSPC, registry unreachable). Naming tools the host already has is the + // confidently-wrong diagnosis #20386's fix must not introduce. + const verdict = diagnose({ + survey: { ...NOTHING_INSTALLED, installed: false, searched: [] }, + toolchain: toolchain(['make', 'g++', 'python3']) + }) + expect(verdict.reason).toBe('dependency_missing') + const text = formatNodePtyUnavailableMessage(verdict) + expect(text).toContain(`node-pty is not installed at ${MODULE_DIR}`) + expect(text).toContain('build tools needed to compile it are present') + expect(text).not.toContain('apt-get') + expect(text).not.toContain('not installed.') }) it('reports a binding that killed the probe as a crash rather than a miss', () => { diff --git a/src/relay/node-pty-unavailable-diagnosis.ts b/src/relay/node-pty-unavailable-diagnosis.ts index 590eedcc375..b8d8fdbdda5 100644 --- a/src/relay/node-pty-unavailable-diagnosis.ts +++ b/src/relay/node-pty-unavailable-diagnosis.ts @@ -33,6 +33,8 @@ import type { TerminalUnavailableCause } from '../shared/terminal-unavailable-ca export type NodePtyBindingSurvey = { /** The node-pty install the relay would load from. */ moduleDir: string + /** False when `moduleDir` itself is absent — the no-toolchain deploy skips node-pty entirely. */ + installed: boolean /** The compiled binding the loader would open, or null when no directory holds one. */ bindingPath: string | null /** Directories checked, so "nothing is installed" is a statement with evidence. */ @@ -312,8 +314,7 @@ function remedyFor(diagnosis: NodePtyUnavailableDiagnosis): string { case 'dependency_missing': return ( `node-pty has no compiled binary on this host (${searchedPhrase(survey)}). ` + - `The C/C++ build tools needed to compile it are present, so reconnect to reinstall ` + - `the relay's native modules.` + `${toolchainPresenceSentence(host, toolchain)}Reconnect to reinstall the relay's native modules.` ) case 'abi_mismatch': return ( @@ -355,7 +356,25 @@ function remedyFor(diagnosis: NodePtyUnavailableDiagnosis): string { } } +// Claims "present" only off a probe that answered; the probe runs on Linux only, since +// node-pty ships prebuilds elsewhere and a reinstall there needs no compiler. +function toolchainPresenceSentence( + host: NodePtyUnavailableHost, + toolchain: BuildToolchainStatus | null +): string { + if (toolchain) { + return 'The C/C++ build tools needed to compile it are present. ' + } + if (host.platform === 'linux') { + return 'Whether the C/C++ build tools needed to compile it are installed could not be checked. ' + } + return '' +} + function searchedPhrase(survey: NodePtyBindingSurvey | null): string { + if (survey && !survey.installed) { + return `node-pty is not installed at ${survey.moduleDir}` + } return survey && survey.searched.length > 0 ? `checked ${survey.searched.join(', ')} under ${survey.moduleDir}` : 'nothing was found where node-pty looks' diff --git a/src/relay/pty-handler-spawn-admission.test.ts b/src/relay/pty-handler-spawn-admission.test.ts index ea5c6ca3486..7e0dc3f70c6 100644 --- a/src/relay/pty-handler-spawn-admission.test.ts +++ b/src/relay/pty-handler-spawn-admission.test.ts @@ -1,11 +1,13 @@ import './mock-descendant-sweep' import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' -import { mkdtempSync, rmSync } from 'node:fs' +import { existsSync, mkdirSync, mkdtempSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' -import { join } from 'node:path' +import { dirname, join } from 'node:path' import * as ptyChildProcessInspection from './pty-child-process-inspection' import * as ptyShellUtils from './pty-shell-utils' import * as processTableSnapshotReader from '../shared/process-table-snapshot-reader' +import * as runProcessModule from '../shared/child-process/run-process' +import * as nodePtyBindingSurvey from './node-pty-binding-survey' const { mockPtySpawn, mockPtyInstance, mockCreateShellPromptReadinessProbe } = vi.hoisted(() => ({ mockPtySpawn: vi.fn(), @@ -325,30 +327,78 @@ describe('PtyHandler', () => { expect(handler.activePtyCount).toBe(0) }) - it('keeps the load error it was handed instead of replacing it with guesses', async () => { - // #17830: the user got three remedies for four possible faults and could verify none. - // The relay must carry what it was actually told, and must not prescribe a toolchain - // install it never probed for. - const thrown = - 'Failed to load native module: conpty.node, checked: build/Release, prebuilds/win32-x64' - mockPtySpawn.mockImplementationOnce(() => { - throw new Error(thrown) - }) + const THROWN_LOAD_ERROR = + 'Failed to load native module: pty.node, checked: build/Release, prebuilds/linux-x64' - const message = await dispatcher.callRequest('pty.spawn', {}).then( - () => '', - (error: Error) => error.message + function failSpawnWithToolchainProbe( + toolchainProbeStdout: string + ): Promise<(Error & { data?: unknown }) | null> { + const realRunProcess = runProcessModule.runProcess + vi.spyOn(runProcessModule, 'runProcess').mockImplementation((spec) => + spec.program === '/bin/sh' + ? Promise.resolve({ + stdout: toolchainProbeStdout, + stderr: '', + code: 0, + signal: null, + timedOut: false + }) + : realRunProcess(spec) ) + mockPtySpawn.mockImplementationOnce(() => { + throw new Error(THROWN_LOAD_ERROR) + }) + return dispatcher.callRequest('pty.spawn', {}).then( + () => null, + (error: Error & { data?: unknown }) => error + ) + } - expect(message).toContain(thrown) - expect(message).not.toContain('install make, a C++ compiler, and python3') - // Nothing here established a cause — the relay's node-pty directory is not on disk in - // this harness — so per docs/reference/ssh-execution-boundary.md it must say so rather - // than pick a diagnosis. Every message still names the host, for the bug report. - expect(message).toContain('could not establish why') + it('diagnoses a relay installed without node-pty instead of asking for a reconnect (#20386)', async () => { + // Relies on no node-pty beside the relay source — what the no-toolchain deploy leaves. Absence + // is observed on the owning host, so it is a diagnosis (docs/reference/ssh-execution-boundary.md). + expect(typeof process.resourcesPath, 'packaged node-pty lookup must be off').not.toBe('string') + expect( + existsSync(join(__dirname, 'node_modules', 'node-pty')), + 'a node-pty beside src/relay would turn this into the installed-but-unbuilt case' + ).toBe(false) + // The checkout's own node_modules holds a node-pty an SSH host's relay dir never has. + vi.spyOn(nodePtyBindingSurvey, 'resolveNodePtyInstallDir').mockReturnValue(null) + + const rejection = await failSpawnWithToolchainProbe('HAVE python3\nPKG apt-get\n') + const message = rejection?.message ?? '' + + // #17830: the load error the relay was handed still travels with the rejection. + // No compiler means no rebuild, so the client must not auto-reconnect on this one. + expect(rejection?.data).toMatchObject({ + reason: 'toolchain_missing', + repairable: false, + rawError: THROWN_LOAD_ERROR + }) + expect(message).not.toContain('could not establish why') + expect(message).toContain('node-pty is not installed at') + expect(message).toContain('sudo apt-get install -y build-essential python3') + // Every message still names the host, for the bug report. expect(message).toMatch(/Host: linux\/\w+, .*Node v[\d.]+ \(ABI \d+\)/) }) + it('diagnoses the node-pty an ancestor node_modules supplied, not the absent one beside the relay', async () => { + const ancestorInstall = join(mkdtempSync(join(tmpdir(), 'orca-node-pty-')), 'node-pty') + mkdirSync(ancestorInstall) + vi.spyOn(nodePtyBindingSurvey, 'resolveNodePtyInstallDir').mockReturnValue(ancestorInstall) + + try { + const message = + (await failSpawnWithToolchainProbe('HAVE make\nHAVE g++\nHAVE python3\nPKG apt-get\n')) + ?.message ?? '' + + expect(message).not.toContain('node-pty is not installed at') + expect(message).toContain(`under ${ancestorInstall}`) + } finally { + rmSync(dirname(ancestorInstall), { recursive: true, force: true }) + } + }) + it('preserves unrelated node-pty spawn failures', async () => { mockPtySpawn.mockImplementationOnce(() => { throw new Error('File not found: missing-shell.exe') diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index cb88537de44..fbad4a212a0 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -128,7 +128,10 @@ import { injectRelayHistoryEnv } from './terminal-history' import { isFlattenedNodePtyLoaderMessage } from '../main/orcad/node-pty-loader-diagnosis' -import { collectNodePtyUnavailableDiagnosis } from './node-pty-binding-survey' +import { + collectNodePtyUnavailableDiagnosis, + resolveNodePtyInstallDir +} from './node-pty-binding-survey' import { describeRelayRuntime } from './relay-runtime-identity' import { relayConptyDllSpawnOptions } from './relay-windows-conpty' import { @@ -670,9 +673,10 @@ export class PtyHandler { * healthy relay never pays for them. */ private async nodePtyUnavailableError(spawnError?: unknown): Promise { - const nodePtyDir = this.relayNodePtyDir() + // Why: diagnose the install the bare import loaded; the bundle's own dir is only the fallback. + const nodePtyDir = resolveNodePtyInstallDir(__dirname) ?? this.relayNodePtyDir() const diagnosis = await collectNodePtyUnavailableDiagnosis({ - nodePtyDir: existsSync(nodePtyDir) ? nodePtyDir : null, + nodePtyDir, error: spawnError ?? this.lastPtyLoadError }) return Object.assign(new Error(formatNodePtyUnavailableMessage(diagnosis)), { diff --git a/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts b/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts index ea5a4dda3aa..e8c0c2caf4f 100644 --- a/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts +++ b/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts @@ -181,6 +181,7 @@ function handleConnectError( context: IpcPtyConnectContext ): PtyConnectResult | undefined { const { connectionId } = context.transportOptions + // Unclamped: host diagnoses put the remedy on later lines, and the pane toast renders them all. const message = readIpcErrorDetail(error) ?? (error instanceof Error ? error.message : String(error)) if (connectionId && options.sessionId && isSshSessionGoneError(message)) { diff --git a/src/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.ts b/src/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.ts index 2ff2c9062a2..6a181e70f7c 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.ts @@ -260,4 +260,23 @@ describe('createIpcPtyTransport', () => { expect(onError).toHaveBeenCalledWith(createTerminalSessionStateSaveFailureMessage()) }) + + it('keeps every line of a multi-line spawn failure for the pane toast (#20386)', async () => { + const { createIpcPtyTransport } = await import('./pty-transport') + const diagnosis = + 'Remote terminals are unavailable: make and a C++ compiler are not installed. Install them on the remote host, then reconnect:\n' + + ' sudo apt-get install -y build-essential python3\n' + + 'Host: linux/x64, glibc 2.36, Node v22.12.0 (ABI 127), prebuild slot linux-x64-glibc.' + vi.mocked(window.api.pty.spawn).mockRejectedValueOnce( + new Error(`Error invoking remote method 'pty:spawn': Error: ${diagnosis}`) + ) + + const onError = vi.fn() + await createIpcPtyTransport({ connectionId: 'ssh-1' }).connect({ + url: '', + callbacks: { onError } + }) + + expect(onError).toHaveBeenCalledWith(diagnosis) + }) })