diff --git a/src/main/ssh/system-ssh-file-transfer.ts b/src/main/ssh/system-ssh-file-transfer.ts index 052ac5a79a3..506fa6ec384 100644 --- a/src/main/ssh/system-ssh-file-transfer.ts +++ b/src/main/ssh/system-ssh-file-transfer.ts @@ -27,7 +27,11 @@ import { WINDOWS_STDIN_WRITE_TIMEOUT_MS, writeBufferViaSystemSsh } from './system-ssh-file-binary-transfer' -import { isSftpUnavailableError, makeDirectoriesViaSftp } from './system-ssh-sftp-transfer' +import { + isSftpPathUnsupportedError, + isSftpUnavailableError, + makeDirectoriesViaSftp +} from './system-ssh-sftp-transfer' import { getWindowsRemoteWriteCapabilities } from './system-ssh-windows-write-capabilities' type SystemSshOperationOptions = SystemSshBuildArgsOptions & { @@ -189,7 +193,17 @@ async function createWindowsUploadDirectories( throwIfAborted(options.signal) await getWindowsRemoteWriteCapabilities(target).runWithFallback( 'sftp-subsystem', - () => makeDirectoriesViaSftp(target, pending, options), + async () => { + try { + await makeDirectoriesViaSftp(target, pending, options) + } catch (error) { + // A directory sftp cannot address is this batch's problem, not the host's verdict. + if (!isSftpPathUnsupportedError(error)) { + throw error + } + await createWindowsUploadDirectoriesViaPowerShell(target, payload, options) + } + }, () => createWindowsUploadDirectoriesViaPowerShell(target, payload, options), isSftpUnavailableError ) diff --git a/src/main/ssh/system-ssh-sftp-transfer.ts b/src/main/ssh/system-ssh-sftp-transfer.ts index 2e5c6f6dcb1..c50375fa8eb 100644 --- a/src/main/ssh/system-ssh-sftp-transfer.ts +++ b/src/main/ssh/system-ssh-sftp-transfer.ts @@ -25,16 +25,31 @@ export class SftpSubsystemUnavailableError extends Error { } /** - * True for the errors that mean "this host cannot do sftp", as opposed to "this transfer failed". - * Deliberately narrow: a permission denial or a missing directory is a real failure that must - * surface, not a reason to retry the whole upload down a slower path. + * True for the errors that mean "this host cannot serve sftp at all". + * + * Host-scoped, and therefore the only errors safe to remember: a capability cache keyed by host + * turns anything it accepts into a verdict about every later write to that host. Deliberately + * narrow — a permission denial or a missing directory is a real failure that must surface, not a + * reason to retry the whole upload down a slower path. */ export function isSftpUnavailableError(error: unknown): boolean { - return ( - error instanceof SftpSubsystemUnavailableError || - error instanceof SftpArgTranslationError || - error instanceof UnsupportedSftpPathError - ) + return error instanceof SftpSubsystemUnavailableError || error instanceof SftpArgTranslationError +} + +/** + * True when *this path* cannot be spelled for sftp, which says nothing about the host. + * + * Kept apart from the host verdict on purpose. A UNC destination, or a local file whose name + * contains a newline, is a property of one operation; caching it would degrade every subsequent + * write to that host for the cache's whole retry window on the strength of one odd filename. + */ +export function isSftpPathUnsupportedError(error: unknown): boolean { + return error instanceof UnsupportedSftpPathError +} + +/** Neither kind of refusal moves a byte, so a staged file cannot exist to sweep. */ +export function isSftpRefusalBeforeStaging(error: unknown): boolean { + return isSftpUnavailableError(error) || isSftpPathUnsupportedError(error) } function systemSftpCandidates(sshPath: string | null, platform: NodeJS.Platform): string[] { diff --git a/src/main/ssh/system-ssh-windows-upload.test.ts b/src/main/ssh/system-ssh-windows-upload.test.ts index 6ba8d777507..dbfb053cb89 100644 --- a/src/main/ssh/system-ssh-windows-upload.test.ts +++ b/src/main/ssh/system-ssh-windows-upload.test.ts @@ -436,6 +436,51 @@ describe('Windows upload over sftp', () => { expect(fileWrites()).toHaveLength(0) }) + it('does not let one unaddressable path become a verdict about the host', async () => { + writeFileSync(join(localDir, 'relay.js'), 'x') + + // A UNC destination has no settled mapping in sftp's drive-rooted namespace, so this write + // falls back — but the host still serves sftp perfectly well for every other path. + await uploadFileViaSystemSsh( + target, + join(localDir, 'relay.js'), + '//fileserver/share/relay.js', + { hostPlatform } + ) + + expect(fileWrites().length).toBeGreaterThan(0) + expect(sftpBatches).toHaveLength(0) + // The 30-minute capability cache is keyed by host; caching this would send every later write + // to the same machine down the defective path on the strength of one odd destination. + expect(getWindowsRemoteWriteCapabilities(target).shouldTry('sftp-subsystem')).toBe(true) + }) + + it('keeps using sftp for the next file after one path it could not spell', async () => { + writeFileSync(join(localDir, 'relay.js'), 'x') + + await uploadFileViaSystemSsh(target, join(localDir, 'relay.js'), '//fileserver/share/a.js', { + hostPlatform + }) + await uploadFileViaSystemSsh(target, join(localDir, 'relay.js'), `${remoteRoot}/b.js`, { + hostPlatform + }) + + expect(putLines()).toHaveLength(1) + expect(putDestination(putLines()[0]!)).toContain('/C:/Users/dev/.orca-remote/b.js') + }) + + it('does not let a local filename sftp cannot quote become a verdict either', async () => { + // POSIX clients allow a newline in a filename, and sftp's batch lexer would read it as the end + // of one command and the start of another. + const awkward = join(localDir, 'two\nlines.js') + writeFileSync(awkward, 'x') + + await uploadFileViaSystemSsh(target, awkward, `${remoteRoot}/relay.js`, { hostPlatform }) + + expect(fileWrites().length).toBeGreaterThan(0) + expect(getWindowsRemoteWriteCapabilities(target).shouldTry('sftp-subsystem')).toBe(true) + }) + it('translates the ssh argument list rather than passing it to a client that reads it differently', async () => { writeFileSync(join(localDir, 'relay.js'), 'x') diff --git a/src/main/ssh/system-ssh-windows-write-strategy.ts b/src/main/ssh/system-ssh-windows-write-strategy.ts index 7df9f800925..f2cdca12516 100644 --- a/src/main/ssh/system-ssh-windows-write-strategy.ts +++ b/src/main/ssh/system-ssh-windows-write-strategy.ts @@ -6,7 +6,12 @@ import { throwIfAborted, waitForChannelClose } from './system-ssh-operation-lifecycle' -import { isSftpUnavailableError, runSftpBatch } from './system-ssh-sftp-transfer' +import { + isSftpPathUnsupportedError, + isSftpRefusalBeforeStaging, + isSftpUnavailableError, + runSftpBatch +} from './system-ssh-sftp-transfer' import { quoteSftpBatchArgument, toSftpRemotePath } from './system-ssh-sftp-path' import { getWindowsRemoteWriteCapabilities } from './system-ssh-windows-write-capabilities' import { @@ -108,7 +113,30 @@ async function stageThenPublish( } } -function writeViaSftp( +/** + * A path sftp cannot address falls back for this write alone, without touching the host verdict. + * + * The distinction matters because the capability cache is keyed by host and holds for half an hour: + * routing one UNC destination, or one local filename containing a newline, into + * `rememberUnsupported` would send every later write to that host down the defective path too. + */ +async function writeViaSftp( + target: SshTarget, + remotePath: string, + source: WindowsWriteSource, + options: WindowsWriteOptions +): Promise { + try { + await attemptSftpWrite(target, remotePath, source, options) + } catch (error) { + if (!isSftpPathUnsupportedError(error)) { + throw error + } + await writeViaRemoteStdin(target, remotePath, source, options) + } +} + +function attemptSftpWrite( target: SshTarget, remotePath: string, source: WindowsWriteSource, @@ -133,7 +161,7 @@ function writeViaSftp( options ) ), - isSftpUnavailableError + isSftpRefusalBeforeStaging ) }