mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 16:03:20 +00:00
fix(relay): use a monotonic deadline for the DefaultShell probe cooldown
A wall-clock deadline let an NTP correction or VM resume stretch the 30s cooldown into an arbitrarily long window of not honoring the configured shell - a milder version of the bug this branch fixes. performance.now() matches the elapsed-window idiom already used in pty-handler. Also pin the spawn-failure branch: reg.exe failing to spawn carries a string code, so the numeric-exit-code guard is the only thing keeping it retryable, and document that access-denied shares the cached path.
This commit is contained in:
@@ -32,6 +32,13 @@ function timeoutError(): Error {
|
||||
})
|
||||
}
|
||||
|
||||
/** The rejection Node surfaces when reg.exe cannot be spawned at all. */
|
||||
function spawnFailureError(): Error {
|
||||
// Why: spawn failures report killed/signal as `undefined` and a *string* code,
|
||||
// unlike the numeric exit code of a probe that actually ran.
|
||||
return Object.assign(new Error('spawn reg.exe ENOENT'), { code: 'ENOENT' })
|
||||
}
|
||||
|
||||
/** Drive the promisified execFile callback with a per-attempt outcome. */
|
||||
function mockAttempts(outcomes: (string | Error)[]): { attempts: () => number } {
|
||||
let attempt = 0
|
||||
@@ -108,6 +115,20 @@ describe('readOpenSshDefaultShell', () => {
|
||||
expect(attempts()).toBe(1)
|
||||
})
|
||||
|
||||
it('retries when reg.exe could not be spawned at all', async () => {
|
||||
// Why pinned: a spawn failure carries a string `code`, so `typeof code === 'number'`
|
||||
// is the only thing keeping it retryable rather than cached for the process lifetime.
|
||||
vi.useFakeTimers()
|
||||
const { attempts } = mockAttempts([spawnFailureError(), regOutput(POWERSHELL_7)])
|
||||
const { readOpenSshDefaultShell, OPENSSH_DEFAULT_SHELL_RETRY_COOLDOWN_MS } = await loadModule()
|
||||
|
||||
expect(await readOpenSshDefaultShell()).toBe('')
|
||||
vi.advanceTimersByTime(OPENSSH_DEFAULT_SHELL_RETRY_COOLDOWN_MS)
|
||||
|
||||
expect(await readOpenSshDefaultShell()).toBe(POWERSHELL_7)
|
||||
expect(attempts()).toBe(2)
|
||||
})
|
||||
|
||||
it('retries after a transient failure and then returns the real DefaultShell', async () => {
|
||||
// REGRESSION: a single transient reg.exe error used to memoize '' for the
|
||||
// lifetime of the relay process, so the host's configured OpenSSH shell was
|
||||
|
||||
@@ -32,8 +32,15 @@ let inFlight: Promise<string> | undefined
|
||||
* install, where the value simply does not exist — which is authoritative, not a
|
||||
* failure. Only a probe killed by the timeout or that failed to spawn is retryable.
|
||||
* Why exit code and not stderr text: reg.exe messages are localized.
|
||||
*
|
||||
* "Access is denied" also exits non-zero and so caches for the process lifetime.
|
||||
* That matches the pre-async behavior, and a registry ACL does not flap the way a
|
||||
* timeout does.
|
||||
*/
|
||||
function probeCompleted(error: unknown): boolean {
|
||||
// Why loose `== null`: a spawn failure (reg.exe missing) reports killed/signal as
|
||||
// `undefined` rather than false/null, and carries a string `code` like 'ENOENT'.
|
||||
// The `typeof code === 'number'` guard is what keeps those retryable.
|
||||
const { killed, signal, code } = (error ?? {}) as {
|
||||
killed?: boolean
|
||||
signal?: NodeJS.Signals | null
|
||||
@@ -61,7 +68,7 @@ export async function readOpenSshDefaultShell(): Promise<string> {
|
||||
if (probe?.kind === 'resolved') {
|
||||
return probe.shell
|
||||
}
|
||||
if (probe?.kind === 'failed' && Date.now() < probe.retryAtMs) {
|
||||
if (probe?.kind === 'failed' && performance.now() < probe.retryAtMs) {
|
||||
return ''
|
||||
}
|
||||
// Why: share one in-flight probe so concurrent spawns don't each launch reg.exe.
|
||||
@@ -73,7 +80,12 @@ export async function readOpenSshDefaultShell(): Promise<string> {
|
||||
.catch((error: unknown) => {
|
||||
probe = probeCompleted(error)
|
||||
? { kind: 'resolved', shell: '' }
|
||||
: { kind: 'failed', retryAtMs: Date.now() + OPENSSH_DEFAULT_SHELL_RETRY_COOLDOWN_MS }
|
||||
: {
|
||||
kind: 'failed',
|
||||
// Why monotonic: an NTP correction or VM resume must not stretch a 30s
|
||||
// cooldown into an hour of not honoring the configured shell.
|
||||
retryAtMs: performance.now() + OPENSSH_DEFAULT_SHELL_RETRY_COOLDOWN_MS
|
||||
}
|
||||
return ''
|
||||
})
|
||||
.finally(() => {
|
||||
|
||||
Reference in New Issue
Block a user