mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 00:03:15 +00:00
Neil
2ee507d744
* fix(ssh): move Windows file writes off PowerShell 5.1 stdin onto sftp #16432 was fixed by chunking writes to 32KB, on the belief that a `DefaultShell=cmd.exe` host caps one stdin at roughly 50KB. Re-measured on Windows 11 26200.9168 / OpenSSH_for_Windows_10.0p2, that premise is wrong in both directions, and the chunking does not fix the hang. The real constraint: a read on Windows PowerShell 5.1's redirected-stdin handle over a non-pty ssh exec can die permanently when it finds the stream momentarily empty, taking both the remaining data and the EOF with it. It is probabilistic per such read — not a size threshold, and not certain on the first one. Measured by swapping the copy loop for a counting reader: a 1.5s gap before any byte -> 0 bytes received, 6 of 6 1 byte, 1.5s gap, then 32767 -> exactly 1 byte 32768, 1.5s gap, then 32768 -> exactly 32768 a continuous 2MB -> 167936 / 270336 / 372736 Those three 2MB figures are one payload run three times under the same conditions, which is what rules out a threshold. Independently reproduced by a second harness where one 1.9MB counted read completed through 39 reads and another died after 11. A payload that fits one burst usually presents only one read that can find the stream empty, which is why 32KB mostly works — and it still failed 15 times in 120 under load, and 1 in 40 on a quiet host. Neither rate survives the 62 execs a 1.9MB file needs: even 2.5% compounds to about four uploads in five failing. No chunk size helps, because the defect is per blocking read, not per byte. Three controls on the same host, same DefaultShell, rule out both a size limit and cmd.exe: `findstr` took 2,016,000 bytes through one exec's stdin, sftp moved 1.9MB 5/5, and PowerShell 7 took 2MB in one exec. Windows writes now go over the sftp subsystem, whose batch script is read by the *local* client, so no remote process reads a pipe at all. PowerShell 7 is the fallback where sftp is unavailable, and Windows PowerShell 5.1 is last, still bounded, and now reports the host limitation and its remedy instead of a bare timeout. Measured on the same host, through this code: 1.9MB x20 all succeeded, hash-verified, median 315ms, against 0/6 before. 32KB x120 zero hangs, against 15/120. Also: - Stage under a unique name per attempt. An abandoned write leaves a remote process that may still hold the staging file, and losing contact is not evidence it died (docs/reference/ssh-execution-boundary.md), so a retry must not reuse a name its predecessor may own. Sweep is best-effort and never treated as proof of anything. - Create upload directories over sftp too; the JSON mkdir batch rode the same defective read. - Cover makeWindowsWriteFileCommand and the publish command against the 8000-char budget, which F11 flagged as untested. * fix(ssh): replace the staged Windows write atomically, and translate ssh -l Three review findings, all on the failure path that the success-path measurements say nothing about. CodeRabbit, Critical: the publish deleted the destination before moving the staged file onto it, so a failed move destroyed the user's existing file and left a window where a reader saw no file at all. That is worse than the truncated partial the staging discipline exists to prevent. Now File.Replace (Win32 ReplaceFile, atomic), falling back to a plain Move only when the destination is absent — and that race is safe, because a destination appearing in between makes Move throw with the staged file preserved. The exclusive branch already had it right: Move throwing on an existing destination is the exclusive contract. Append stays non-atomic and now says why. buildSshArgs can emit '-l <username>' for a config alias no Host block claims, and the translator threw on it. isSftpUnavailableError read that throw as 'this host cannot do sftp', so those hosts fell back to the defective PowerShell 5.1 path and had the refusal cached against them for 30 minutes, silently. '-l' now maps to '-o User=', with a test for the exact argument shape buildSshArgs produces in that case. CodeRabbit, minor: two assertions passed on an absent observation — an unmatched regex yields '' and every() is true of an empty list. Both now assert the positive form first, and the same audit was applied to the three other some()/every() assertions in the file. The temp-file test now asserts mode 0600 rather than only that the file is cleaned up. * fix(ssh): keep a path sftp cannot spell from becoming a verdict about the host Audit of isSftpUnavailableError, prompted by the '-l' gap having the same shape: a per-operation condition being written into a per-host cache that holds for 30 minutes. It had a second instance, and this one was mine. UnsupportedSftpPathError was classified as 'this host cannot do sftp', but it is thrown for a UNC or relative destination and for any path sftp's batch lexer cannot quote -- including a *local* filename containing a newline, which POSIX clients allow. One such file would have routed every later Windows write to that host down the defective PowerShell 5.1 path for the rest of the cache window. The host verdict is now only the errors that really are host-scoped: a refused subsystem, a client that will not start, and an untranslatable argument list. A path refusal falls back for that one write and leaves the cache alone, in both the file-write and directory-creation paths. Revert-tested. Removing the operation-scoped catch fails all three new tests, whether or not the predicate is also widened. Widening the predicate alone does not fail them, correctly: with the catch in place the predicate no longer gates that path, so keeping it narrow is defence-in-depth rather than the live mechanism. Flag audit at the same time: -F, -o, -T, -S, -p, -i, -J, -l and -- are now the complete set buildSshArgs can emit, and all are handled. * fix(ssh): make the atomic publish actually run, and unroll the mkdir batch Two runtime defects that only a real host could surface. Both were invisible to unit tests that assert the shape of the generated command string, because both are PowerShell rejecting an argument at execution time. File.Replace was passed a bare $null for destinationBackupFileName. PowerShell coerces $null to an empty string when binding a .NET string parameter, and Replace rejects that with 'The path is not of a legal form' -- so every create-mode publish failed. The Critical fix was inert as shipped. Now [NullString]::Value, which is the construct that exists for this. Measured on awin, same staging-file lock, opposite outcomes: old publish rc=1 destination MISSING <- prior contents destroyed new publish rc=1 destination PRESENT, sha 7f06b7e0... unchanged control, destination present, no lock rc=0 replaced exactly control, destination absent, no lock rc=0 Move fallback created it End-to-end through the real uploader afterwards: 1.9MB x15 all hashes exact, median 303ms; overwrite of an existing destination exact both times. Separately, the PowerShell mkdir fallback could not create a tree of more than one directory. '@($json | ConvertFrom-Json)' wraps the parsed array in another array, so the loop variable binds to the whole thing and [string] of it is the paths joined by spaces. It only ever worked for a one-element batch, where stringifying a single-element array happens to yield the element -- which is why no existing test caught it. Pre-existing on main; fixed here because this PR puts that command on the fallback tier and claims the ladder works. Both tiers now verified live against a three-directory tree.
80 lines
3.2 KiB
TypeScript
80 lines
3.2 KiB
TypeScript
/**
|
|
* Whether a Windows host has an sftp subsystem is a fact about that host, so the cache is keyed by
|
|
* the endpoint that executes rather than by Orca's target id — otherwise a hardened host is
|
|
* re-probed once per file, and two targets pointing at one machine learn the same fact twice.
|
|
*/
|
|
import { afterEach, describe, expect, it } from 'vitest'
|
|
import type { SshTarget } from '../../shared/ssh-types'
|
|
import {
|
|
clearWindowsRemoteWriteCapabilitiesForTests,
|
|
getWindowsRemoteWriteCapabilities,
|
|
getWindowsRemoteWriteExecutionHostKey
|
|
} from './system-ssh-windows-write-capabilities'
|
|
|
|
const asTarget = (fields: Partial<SshTarget>): SshTarget => fields as SshTarget
|
|
|
|
afterEach(() => {
|
|
clearWindowsRemoteWriteCapabilitiesForTests()
|
|
})
|
|
|
|
describe('getWindowsRemoteWriteExecutionHostKey', () => {
|
|
it('gives two targets on one endpoint the same key', () => {
|
|
const first = asTarget({ id: 'a', host: 'win.example', username: 'dev', port: 22 })
|
|
const second = asTarget({ id: 'b', host: 'win.example', username: 'dev', port: 22 })
|
|
|
|
// A target re-created under a new id has not changed what the host supports.
|
|
expect(getWindowsRemoteWriteExecutionHostKey(first)).toBe(
|
|
getWindowsRemoteWriteExecutionHostKey(second)
|
|
)
|
|
})
|
|
|
|
it('separates hosts, ports and users', () => {
|
|
const base = { id: 'a', host: 'win.example', username: 'dev', port: 22 }
|
|
const keys = [
|
|
asTarget(base),
|
|
asTarget({ ...base, host: 'other.example' }),
|
|
asTarget({ ...base, port: 2222 }),
|
|
asTarget({ ...base, username: 'ops' })
|
|
].map(getWindowsRemoteWriteExecutionHostKey)
|
|
|
|
expect(new Set(keys).size).toBe(4)
|
|
})
|
|
|
|
it('keys a config alias by the alias, since ssh_config decides where it lands', () => {
|
|
const alias = asTarget({ id: 'a', host: 'stale.example', configHost: 'winbox' })
|
|
|
|
expect(getWindowsRemoteWriteExecutionHostKey(alias)).toBe('config:winbox')
|
|
})
|
|
})
|
|
|
|
describe('getWindowsRemoteWriteCapabilities', () => {
|
|
it('shares one cache across targets that reach the same host', () => {
|
|
const first = asTarget({ id: 'a', host: 'win.example', username: 'dev', port: 22 })
|
|
const second = asTarget({ id: 'b', host: 'win.example', username: 'dev', port: 22 })
|
|
|
|
getWindowsRemoteWriteCapabilities(first).rememberUnsupported('sftp-subsystem')
|
|
|
|
expect(getWindowsRemoteWriteCapabilities(second).shouldTry('sftp-subsystem')).toBe(false)
|
|
})
|
|
|
|
it('does not let one host answer for another', () => {
|
|
const hardened = asTarget({ id: 'a', host: 'hardened.example', username: 'dev', port: 22 })
|
|
const ordinary = asTarget({ id: 'b', host: 'ordinary.example', username: 'dev', port: 22 })
|
|
|
|
getWindowsRemoteWriteCapabilities(hardened).rememberUnsupported('sftp-subsystem')
|
|
|
|
expect(getWindowsRemoteWriteCapabilities(ordinary).shouldTry('sftp-subsystem')).toBe(true)
|
|
})
|
|
|
|
it('keeps the two capabilities independent', () => {
|
|
const target = asTarget({ id: 'a', host: 'win.example', username: 'dev', port: 22 })
|
|
const capabilities = getWindowsRemoteWriteCapabilities(target)
|
|
|
|
capabilities.rememberUnsupported('pwsh')
|
|
|
|
// No PowerShell 7 says nothing about whether the host will serve sftp.
|
|
expect(capabilities.shouldTry('sftp-subsystem')).toBe(true)
|
|
expect(capabilities.shouldTry('pwsh')).toBe(false)
|
|
})
|
|
})
|