diff --git a/src/main/ssh/ssh-relay-deploy-helpers.test.ts b/src/main/ssh/ssh-relay-deploy-helpers.test.ts index acda4594ca2..f17fc858164 100644 --- a/src/main/ssh/ssh-relay-deploy-helpers.test.ts +++ b/src/main/ssh/ssh-relay-deploy-helpers.test.ts @@ -550,6 +550,27 @@ describe('execCommand', () => { expect(channel.stderr.listenerCount('data')).toBe(0) }) + it('hands a zero-exit command stderr to onStderr instead of dropping it', async () => { + // Why: probes fenced with `|| echo MISSING` always exit 0, so the resolve path used to be the + // one place the failure reason was discarded. + const channel = createMockChannel() + const conn = { exec: vi.fn().mockResolvedValue(channel) } + const captured: string[] = [] + const commandPromise = execCommand(conn as never, "(node -e 'x' || echo MISSING)", { + onStderr: (stderr) => captured.push(stderr) + }) + + await Promise.resolve() + channel.stderr.emit('data', Buffer.from('node: --bogus is not allowed in NODE_OPTIONS\n')) + channel.emit('data', Buffer.from('MISSING\n')) + channel.emit('close', 0) + + await expect(commandPromise).resolves.toBe('MISSING\n') + expect(captured).toEqual(['node: --bogus is not allowed in NODE_OPTIONS\n']) + // onStderr must not leak into the SSH exec options. + expect(conn.exec).toHaveBeenCalledWith("(node -e 'x' || echo MISSING)", {}) + }) + it('uses custom command timeouts without forwarding them to SSH exec', async () => { vi.useFakeTimers() try { diff --git a/src/main/ssh/ssh-relay-deploy.ts b/src/main/ssh/ssh-relay-deploy.ts index 911d185b0fd..bac9c2b9b34 100644 --- a/src/main/ssh/ssh-relay-deploy.ts +++ b/src/main/ssh/ssh-relay-deploy.ts @@ -116,12 +116,13 @@ function execHostCommand( conn: SshConnection, hostPlatform: RemoteHostPlatform, command: string, - options?: { timeoutMs?: number; signal?: AbortSignal } + options?: { timeoutMs?: number; signal?: AbortSignal; onStderr?: (stderr: string) => void } ): Promise { return execCommand(conn, command, { wrapCommand: !isWindowsRemoteHost(hostPlatform), timeoutMs: options?.timeoutMs, - signal: options?.signal + signal: options?.signal, + onStderr: options?.onStderr }) } @@ -722,24 +723,34 @@ function nativeDepsProbeJs(successToken: string): string { return `(()=>{const missing=[];try{${loadNodePty}}catch{missing.push("node-pty")}try{require("@parcel/watcher")}catch{missing.push("@parcel/watcher")}if(missing.length){console.log("${NATIVE_DEPS_MISSING_PREFIX}"+missing.join(","));process.exitCode=1}else{console.log(${JSON.stringify(successToken)})}})()` } -function missingNativeDepsFromProbe(output: string): RelayNativeDepName[] { +/** + * Which deps the probe *named* as unloadable, or `undefined` when the answer names none. + * + * Only the probe's own marker line is evidence about the deps. An answer without one (node never + * ran, was killed, exited before the script) says nothing, so it must not be read as "all of them" — + * that inference deleted both native modules on every reconnect of an affected host. + */ +function missingNativeDepsFromProbe(output: string): RelayNativeDepName[] | undefined { const marker = output .split(/\r?\n/) .find((line) => line.trim().startsWith(NATIVE_DEPS_MISSING_PREFIX)) if (!marker) { - return [...RELAY_NATIVE_DEP_NAMES] + return undefined } const reported = marker.trim().slice(NATIVE_DEPS_MISSING_PREFIX.length).split(',') - return RELAY_NATIVE_DEP_NAMES.filter((name) => reported.includes(name)) + const named = RELAY_NATIVE_DEP_NAMES.filter((name) => reported.includes(name)) + return named.length > 0 ? named : undefined } /** - * `ok` — the probe answered and both deps loaded. `blocked` — the probe answered and named deps - * that failed to load. `unverifiable` — the probe never answered, which is evidence about the - * transport, not about the deps. + * `ok` — the probe answered and both deps loaded. `blocked` — the probe answered with a marker + * naming deps that failed to load. `unverifiable` — the probe never answered, or answered nothing + * that names a dep; both are evidence about the probe, not about the deps. * * Why `unverifiable` is not `blocked`: repairing on it does `rm -rf node_modules/node-pty` and a - * node-gyp source build (no Linux prebuild) against a relay that was never shown to be broken. + * node-gyp source build (no Linux prebuild) against a relay that was never shown to be broken. An + * unparseable answer is the worse half of that — it is deterministic and per-host, so a node that + * cannot start (bad NODE_OPTIONS, OOM, exit 127) deleted both modules on every reconnect forever. * Same verdict discipline as `src/main/orcad/node-pty-precondition.ts` and * docs/reference/ssh-execution-boundary.md — loss of contact is not evidence. */ @@ -754,6 +765,7 @@ async function probeRequiredNativeDeps( ): Promise<{ status: RelayNativeDepsProbeStatus; missing: RelayNativeDepName[] }> { const escapedNode = shellEscape(nodePath) const probeJs = nativeDepsProbeJs('ORCA-NATIVE-DEPS-OK') + let probeStderr = '' try { const command = isWindowsRemoteHost(hostPlatform) ? commandWithNodePath( @@ -762,16 +774,32 @@ async function probeRequiredNativeDeps( remoteDir, `try { & ${powerShellLiteral(nodePath)} -e ${powerShellNativeArg(probeJs)} } catch { 'MISSING' }` ) - : commandWithNodePath( + : // Why: no `2>/dev/null` — it discarded the only line that says why node never reached the + // script. stderr stays its own stream so it can't be mistaken for the verdict, mirroring + // src/main/orcad/node-pty-precondition.ts. + commandWithNodePath( hostPlatform, nodePath, remoteDir, - `(${escapedNode} -e ${shellEscape(probeJs)} 2>/dev/null || echo MISSING)` + `(${escapedNode} -e ${shellEscape(probeJs)} || echo MISSING)` ) - const probe = await execHostCommand(conn, hostPlatform, command, { signal }) - return probe.includes('ORCA-NATIVE-DEPS-OK') - ? { status: 'ok', missing: [] } - : { status: 'blocked', missing: missingNativeDepsFromProbe(probe) } + const probe = await execHostCommand(conn, hostPlatform, command, { + signal, + onStderr: (text) => { + probeStderr = text + } + }) + if (probe.includes('ORCA-NATIVE-DEPS-OK')) { + return { status: 'ok', missing: [] } + } + const missing = missingNativeDepsFromProbe(probe) + if (!missing) { + console.warn( + `[ssh-relay][NATIVE-DEPS-PROBE-UNPARSEABLE] Probe at ${remoteDir} answered without naming a dep; launching as-is. stdout=${probe.trim().slice(-200)} stderr=${probeStderr.trim().slice(-500)}` + ) + return { status: 'unverifiable', missing: [] } + } + return { status: 'blocked', missing } } catch { signal?.throwIfAborted() // Why: an unanswered probe says nothing about the deps; reporting MISSING here reset and @@ -1337,7 +1365,8 @@ async function probeInstalledNativeDeps( } return { available: probeOutput.includes(PROBE_OK), - missing: probeOutput.includes(PROBE_OK) ? [] : missingNativeDepsFromProbe(probeOutput), + // A markerless answer names no dep, so it reports none; `available` already carries the failure. + missing: probeOutput.includes(PROBE_OK) ? [] : (missingNativeDepsFromProbe(probeOutput) ?? []), output: probeOutput, stderr: remoteStderr } diff --git a/src/main/ssh/ssh-relay-exec-command.ts b/src/main/ssh/ssh-relay-exec-command.ts index c631cd2d41a..fb0a4fee012 100644 --- a/src/main/ssh/ssh-relay-exec-command.ts +++ b/src/main/ssh/ssh-relay-exec-command.ts @@ -13,6 +13,11 @@ const MAX_EXEC_OUTPUT_CHARS = 1024 * 1024 type ExecCommandOptions = SshExecOptions & { timeoutMs?: number + // Why: a zero-exit command resolves with stdout alone, so the reason a wrapped-in-`|| echo` + // probe failed is discarded. Callers that need that diagnostic opt in here rather than + // folding stderr into stdout, where it would match the probe's own token strings. + // On the system-ssh transport this stream also carries local OpenSSH noise; log-only. + onStderr?: (stderr: string) => void } type SshCommandTerminationError = Error & { @@ -33,7 +38,7 @@ export async function execCommand( command: string, options?: ExecCommandOptions ): Promise { - const { timeoutMs = EXEC_TIMEOUT_MS, ...execOptions } = options ?? {} + const { timeoutMs = EXEC_TIMEOUT_MS, onStderr, ...execOptions } = options ?? {} const signal = options?.signal if (signal?.aborted) { throw createSshOperationAbortError() @@ -151,6 +156,9 @@ export async function execCommand( ) ) } else { + if (stderr && onStderr) { + onStderr(redactRelayInstallMarkerTokens(stderr)) + } settle(resolve, stdout) } } diff --git a/src/main/ssh/ssh-relay-native-deps-install-fixture.ts b/src/main/ssh/ssh-relay-native-deps-install-fixture.ts index e8fc2274583..821ebbc1208 100644 --- a/src/main/ssh/ssh-relay-native-deps-install-fixture.ts +++ b/src/main/ssh/ssh-relay-native-deps-install-fixture.ts @@ -54,6 +54,11 @@ export function makeMockConnection(capture: SftpWriteCapture): SshConnection { export type ExecResponse = string | { reject: string } +// The answer a genuinely broken pair produces: a marker line naming both deps. A bare `MISSING` +// names none, so it is unverifiable and must never stand in for this. +export const BOTH_NATIVE_DEPS_MISSING_PROBE = + 'ORCA-NATIVE-DEPS-MISSING:node-pty,@parcel/watcher\nMISSING' + const STAGE_OWNER = '.sftp-namespace-00000000000000000000000000000000' export function makeStagedFirstInstallExecPrefix(): ExecResponse[] { @@ -71,12 +76,11 @@ export function makeStagedFirstInstallExecPrefix(): ExecResponse[] { // Repair reconnect (isRelayAlreadyInstalled → true) where BOTH native deps are broken and the host // cannot compile node-pty, so the caller's resets must survive into the node-pty-less reinstall. export function makeRepairToolchainSkipExecResponses(): ExecResponse[] { - const bothMissing = 'ORCA-NATIVE-DEPS-MISSING:node-pty,@parcel/watcher\nMISSING' return [ '__ORCA_REMOTE_PLATFORM__ Linux x86_64', '/home/u', - bothMissing, // health probe before lock - bothMissing, // re-probe under the repair lock + BOTH_NATIVE_DEPS_MISSING_PROBE, // health probe before lock + BOTH_NATIVE_DEPS_MISSING_PROBE, // re-probe under the repair lock '', // SFTP-namespace install-owner marker (repair) { reject: 'gyp ERR! stack Error: not found: make' }, 'PKG apk', // toolchain probe: no HAVE lines @@ -139,7 +143,7 @@ export function makeExecResponses(opts: { '', // rm -rf node-pty + reinstall without it // node-pty is always reported missing here; the probe never resolves OK, so cat + rm both run. opts.nodePtySkipWatcher === 'missing' - ? 'ORCA-NATIVE-DEPS-MISSING:node-pty,@parcel/watcher\nMISSING\n' + ? `${BOTH_NATIVE_DEPS_MISSING_PROBE}\n` : 'ORCA-NATIVE-DEPS-MISSING:node-pty\nMISSING\n', '', // cat probe stderr '', // rm -f probe stderr diff --git a/src/main/ssh/ssh-relay-native-deps-install.test.ts b/src/main/ssh/ssh-relay-native-deps-install.test.ts index 5297da7c85a..16144fb2cf3 100644 --- a/src/main/ssh/ssh-relay-native-deps-install.test.ts +++ b/src/main/ssh/ssh-relay-native-deps-install.test.ts @@ -83,6 +83,7 @@ import { import { acquireInstallLock } from './ssh-relay-install-lock' import { tryAcquireRelayRepairLock } from './ssh-relay-repair-lock' import { + BOTH_NATIVE_DEPS_MISSING_PROBE, decodePowerShellCommand, makeExecResponses, makeMockConnection, @@ -677,8 +678,8 @@ describe('installNativeDeps (via deployAndLaunchRelay)', () => { feed([ '__ORCA_REMOTE_PLATFORM__ Linux x86_64', '/home/u', - 'MISSING', // health probe: require() fails - 'MISSING', // re-probe after lock + BOTH_NATIVE_DEPS_MISSING_PROBE, // health probe: require() names both deps + BOTH_NATIVE_DEPS_MISSING_PROBE, // re-probe after lock '', // SFTP-namespace install-owner marker (repair) { reject: 'npm ERR! network ETIMEDOUT' }, // npm install fails (offline) 'DEAD', @@ -701,8 +702,8 @@ describe('installNativeDeps (via deployAndLaunchRelay)', () => { vi.mocked(execCommand) .mockResolvedValueOnce('__ORCA_REMOTE_PLATFORM__ Linux x86_64') .mockResolvedValueOnce('/home/u') - .mockResolvedValueOnce('MISSING') - .mockResolvedValueOnce('MISSING') + .mockResolvedValueOnce(BOTH_NATIVE_DEPS_MISSING_PROBE) + .mockResolvedValueOnce(BOTH_NATIVE_DEPS_MISSING_PROBE) .mockResolvedValueOnce('') // SFTP-namespace install-owner marker (repair) .mockRejectedValueOnce( Object.assign(new Error('npm termination was not confirmed'), { @@ -843,7 +844,7 @@ describe('installNativeDeps (via deployAndLaunchRelay)', () => { feed([ '__ORCA_REMOTE_PLATFORM__ Linux x86_64', '/home/u', - 'MISSING', + BOTH_NATIVE_DEPS_MISSING_PROBE, 'DEAD', '', // remote credential generation without a namespace marker 'READY' diff --git a/src/main/ssh/ssh-relay-native-deps-probe-verdict.test.ts b/src/main/ssh/ssh-relay-native-deps-probe-verdict.test.ts index c67270db4c3..df946dfe346 100644 --- a/src/main/ssh/ssh-relay-native-deps-probe-verdict.test.ts +++ b/src/main/ssh/ssh-relay-native-deps-probe-verdict.test.ts @@ -75,8 +75,11 @@ vi.mock('./ssh-connection-utils', () => ({ import { deployAndLaunchRelay } from './ssh-relay-deploy' import { execCommand, uploadDirectory } from './ssh-relay-deploy-helpers' import { parseUnameToRelayPlatform } from './relay-protocol' +import { resolveRemoteNodePath } from './ssh-remote-node-resolution' import { finalizeInstall, isRelayAlreadyInstalled } from './ssh-relay-versioned-install' import { + BOTH_NATIVE_DEPS_MISSING_PROBE, + decodePowerShellCommand, makeMockConnection, type ExecResponse, type SftpWriteCapture @@ -156,13 +159,67 @@ describe('native-deps repair probe verdicts', () => { expect(outcome, 'lost contact must not abort the connection').not.toBeInstanceOf(Error) }) - it('still resets and repairs when the probe answers without the OK marker', async () => { + it('leaves node_modules intact when the probe answers without naming a dep', async () => { + // The bare `MISSING` a `|| echo MISSING` subshell emits when node never reached the script + // (bad NODE_OPTIONS, OOM kill, exit 127). The shell answered; the answer is not about the deps. const conn = makeMockConnection(sftpCapture) feed([ '__ORCA_REMOTE_PLATFORM__ Linux x86_64', '/home/u', - 'MISSING', // answered, no marker line: both deps are genuinely broken - 'MISSING', // re-probe under the repair lock + 'MISSING', // answered, no marker line: nothing here names a dep + '', // launch namespace marker + 'DEAD', + '', // publish the per-launch credential + 'READY' + ]) + + const outcome = await deployAndLaunchRelay(conn).then( + (result) => result, + (err: Error) => err + ) + + const commands = execCommands() + expect(warnings().some((message) => message.includes('Repairing missing native deps'))).toBe( + false + ) + expect(commands.some((command) => command.includes(NODE_PTY_RESET))).toBe(false) + expect(commands.some((command) => command.includes(WATCHER_RESET))).toBe(false) + expect(commands.some((command) => command.includes('npm install'))).toBe(false) + // One probe only: an unverifiable answer must not fall through to the locked re-probe. + expect(commands.filter((command) => command.includes('ORCA-NATIVE-DEPS-OK'))).toHaveLength(1) + expect(vi.mocked(finalizeInstall)).not.toHaveBeenCalled() + expect(outcome, 'an unparseable answer must not abort the connection').not.toBeInstanceOf(Error) + expect(warnings().some((message) => message.includes('NATIVE-DEPS-PROBE-UNPARSEABLE'))).toBe( + true + ) + }) + + it('carries the probe stderr into the unparseable-answer warning', async () => { + const conn = makeMockConnection(sftpCapture) + vi.mocked(execCommand) + .mockResolvedValueOnce('__ORCA_REMOTE_PLATFORM__ Linux x86_64') + .mockResolvedValueOnce('/home/u') + .mockImplementationOnce((_conn, _command, options) => { + options?.onStderr?.('node: --inspect-brk is not allowed in NODE_OPTIONS') + return Promise.resolve('MISSING') + }) + feed(['', 'DEAD', '', 'READY']) + + await expect(deployAndLaunchRelay(conn)).resolves.toBeDefined() + + // Why: `2>/dev/null` used to drop the one line that says which host config broke node. + expect( + warnings().find((message) => message.includes('NATIVE-DEPS-PROBE-UNPARSEABLE')) + ).toContain('not allowed in NODE_OPTIONS') + }) + + it('still resets both deps when the probe names both', async () => { + const conn = makeMockConnection(sftpCapture) + feed([ + '__ORCA_REMOTE_PLATFORM__ Linux x86_64', + '/home/u', + BOTH_NATIVE_DEPS_MISSING_PROBE, // answered: both deps are genuinely broken + BOTH_NATIVE_DEPS_MISSING_PROBE, // re-probe under the repair lock '', // SFTP-namespace install-owner marker (repair) '', // npm install native deps '', // chmod prebuilds @@ -181,6 +238,62 @@ describe('native-deps repair probe verdicts', () => { expect(vi.mocked(finalizeInstall)).toHaveBeenCalledTimes(1) }) + it('leaves a Windows relay intact when its probe answers without naming a dep', async () => { + // The PowerShell branch has the same hole: `try { & node -e ... } catch { 'MISSING' }` prints + // nothing when node exits non-zero without reaching the script. + vi.mocked(parseUnameToRelayPlatform).mockReturnValueOnce('win32-x64') + vi.mocked(resolveRemoteNodePath).mockResolvedValueOnce('C:/Program Files/nodejs/node.exe') + const conn = makeMockConnection(sftpCapture) + feed([ + '__ORCA_REMOTE_PLATFORM__ Windows AMD64', + 'C:\\Users\\u', + '', // health probe: PowerShell swallowed the native failure, so nothing names a dep + '', // no persisted active pipe marker + 'WAITING', // initial pipe probe + '', // publish the per-launch credential + '', // WMI relay launch + 'READY', // readiness poll + '' // persist active pipe marker + ]) + + await expect(deployAndLaunchRelay(conn)).resolves.toBeDefined() + + const scripts = execCommands().map((command) => decodePowerShellCommand(command) ?? command) + expect(scripts.some((script) => script.includes('node_modules/node-pty'))).toBe(false) + expect(scripts.some((script) => script.includes('node_modules/@parcel/watcher'))).toBe(false) + expect(scripts.some((script) => script.includes('npm install'))).toBe(false) + expect(vi.mocked(finalizeInstall)).not.toHaveBeenCalled() + expect(warnings().some((message) => message.includes('NATIVE-DEPS-PROBE-UNPARSEABLE'))).toBe( + true + ) + }) + + it('resets only the dep the probe names', async () => { + const conn = makeMockConnection(sftpCapture) + const watcherMissing = 'ORCA-NATIVE-DEPS-MISSING:@parcel/watcher\nMISSING' + feed([ + '__ORCA_REMOTE_PLATFORM__ Linux x86_64', + '/home/u', + watcherMissing, + watcherMissing, // re-probe under the repair lock + '', // SFTP-namespace install-owner marker (repair) + '', // npm install native deps + '', // chmod prebuilds + 'ORCA-NPTY-PROBE-OK\n', + '', // rm probe stderr + 'DEAD', + '', // publish the per-launch credential + 'READY' + ]) + + await expect(deployAndLaunchRelay(conn)).resolves.toBeDefined() + + const install = execCommands().find((command) => command.includes('npm install')) ?? '' + expect(install).toContain(WATCHER_RESET) + expect(install).not.toContain(NODE_PTY_RESET) + expect(vi.mocked(finalizeInstall)).toHaveBeenCalledTimes(1) + }) + it('skips repair entirely when the probe answers OK', async () => { const conn = makeMockConnection(sftpCapture) feed([ diff --git a/src/main/ssh/ssh-relay-sftp-namespace-install.test.ts b/src/main/ssh/ssh-relay-sftp-namespace-install.test.ts index 3809af2c3d9..49abdcd6888 100644 --- a/src/main/ssh/ssh-relay-sftp-namespace-install.test.ts +++ b/src/main/ssh/ssh-relay-sftp-namespace-install.test.ts @@ -91,6 +91,7 @@ import { isRelayAlreadyInstalled } from './ssh-relay-versioned-install' import { tryAcquireRelayRepairLock } from './ssh-relay-repair-lock' +import { BOTH_NATIVE_DEPS_MISSING_PROBE } from './ssh-relay-native-deps-install-fixture' import type { SshConnection } from './ssh-connection' import type { SftpNamespacePathMapping } from './sftp-namespace-resolution' @@ -281,8 +282,8 @@ const POSIX_SYSTEM_SSH_FIRST_INSTALL = [ const POSIX_REPAIR = [ '__ORCA_REMOTE_PLATFORM__ Linux x86_64', SHELL_HOME, - 'MISSING', // probe before the repair lock - 'MISSING', // re-probe under the lock + BOTH_NATIVE_DEPS_MISSING_PROBE, // probe before the repair lock: the marker names both deps + BOTH_NATIVE_DEPS_MISSING_PROBE, // re-probe under the lock '', // install-owner marker '', // npm install native deps '', // chmod prebuilds @@ -692,8 +693,8 @@ describe('relay repair writes on a split SFTP namespace', () => { feed([ '__ORCA_REMOTE_PLATFORM__ Linux x86_64', SHELL_HOME, - 'MISSING', - 'MISSING', + BOTH_NATIVE_DEPS_MISSING_PROBE, + BOTH_NATIVE_DEPS_MISSING_PROBE, '', // npm install native deps '', // chmod prebuilds 'ORCA-NPTY-PROBE-OK\n', @@ -716,7 +717,12 @@ describe('relay repair writes on a split SFTP namespace', () => { it('degrades to shell paths when marker creation fails outright', async () => { const conn = makeConnection(capture) - feed(['__ORCA_REMOTE_PLATFORM__ Linux x86_64', SHELL_HOME, 'MISSING', 'MISSING']) + feed([ + '__ORCA_REMOTE_PLATFORM__ Linux x86_64', + SHELL_HOME, + BOTH_NATIVE_DEPS_MISSING_PROBE, + BOTH_NATIVE_DEPS_MISSING_PROBE + ]) vi.mocked(execCommand).mockRejectedValueOnce(new Error('read-only file system')) feed([ '', // npm install native deps @@ -742,7 +748,12 @@ describe('relay repair writes on a split SFTP namespace', () => { it('keeps the repair lock when marker creation has unconfirmed termination', async () => { const conn = makeConnection(capture) - feed(['__ORCA_REMOTE_PLATFORM__ Linux x86_64', SHELL_HOME, 'MISSING', 'MISSING']) + feed([ + '__ORCA_REMOTE_PLATFORM__ Linux x86_64', + SHELL_HOME, + BOTH_NATIVE_DEPS_MISSING_PROBE, + BOTH_NATIVE_DEPS_MISSING_PROBE + ]) vi.mocked(execCommand).mockRejectedValueOnce( Object.assign(new Error('marker teardown unconfirmed'), { sshChannelCloseConfirmed: false }) )