fix(ssh): stop reset from unlinking the socket it just stopped

Review found the same time-of-check race in reset that the deploy path already
closed: another relay can bind the path between the post-kill check and the
unlink, and reset would then remove a live owner's socket.

Reset does not need to unlink at all. A relay that stops cleanly removes its own
path, and one that had to be killed leaves an inode the next deploy releases under
its identity guard — proven by a new live journey where a killed relay's leftover
socket is taken over by the production launch path.

That also retires the last way reset could read a silent lookup as absence.
This commit is contained in:
Jinwoo-H
2026-08-18 03:46:59 -04:00
parent 7ab1622a6a
commit e891b2f505
3 changed files with 65 additions and 17 deletions
@@ -21,7 +21,9 @@ import { launchRelay } from './ssh-relay-deploy'
import {
LiveRelayFixture,
createLocalShellConnection,
delay,
isProcessAlive,
killProcessTree,
relayBundleDirForHost
} from './ssh-relay-live-daemon-harness'
import { isRelaySocketOwnerLiveError } from './ssh-relay-socket-owner'
@@ -39,6 +41,10 @@ if (process.env.ORCA_REQUIRE_RELAY_BUNDLE === '1' && !BUNDLE_DIR) {
type RelayStatus = { pid: number; ptys: { active: number } }
const hostPlatform = getRemoteHostPlatform(
process.platform === 'darwin' ? 'darwin-arm64' : 'linux-x64'
)
describe.skipIf(!BUNDLE_DIR || process.platform === 'win32')('launchRelay socket ownership', () => {
let fixture: LiveRelayFixture | null = null
let root: string | null = null
@@ -74,9 +80,6 @@ describe.skipIf(!BUNDLE_DIR || process.platform === 'win32')('launchRelay socket
// that rewrote the file after the daemon had already claimed the socket.
live.rotateCredential()
const hostPlatform = getRemoteHostPlatform(
process.platform === 'darwin' ? 'darwin-arm64' : 'linux-x64'
)
const error = await launchRelay(
createLocalShellConnection(),
root,
@@ -92,4 +95,42 @@ describe.skipIf(!BUNDLE_DIR || process.platform === 'win32')('launchRelay socket
expect(isProcessAlive(incumbent.pid)).toBe(true)
expect(live.logMark('replacement')).toBe(0)
})
// Why this pairs with the test above: reset no longer unlinks the socket it stopped, so the
// inode a killed relay leaves behind has to be the deploy path's problem — and it is.
it('takes over the socket a killed relay left behind', async () => {
root = mkdtempSync(join(tmpdir(), 'orca-rel-'))
const live = new LiveRelayFixture(root, BUNDLE_DIR!, REPO_ROOT, TARGET_ID)
fixture = live
live.launchDaemon('incumbent')
expect(await live.waitForSocket()).toBe(true)
const bridge = live.openBridge()
await bridge.waitForSentinel()
const incumbent = await bridge.request<RelayStatus>('relay.status')
bridge.close()
// A relay stopped by reset (or a crash) leaves its socket inode behind.
killProcessTree(incumbent.pid)
for (let attempt = 0; attempt < 50 && isProcessAlive(incumbent.pid); attempt++) {
await delay(100)
}
expect(existsSync(live.sockPath)).toBe(true)
const launched = await launchRelay(
createLocalShellConnection(),
root,
hostPlatform,
process.execPath,
0,
TARGET_ID
)
expect(launched.sockPath).toBe(live.sockPath)
const fresh = live.openBridge()
await fresh.waitForSentinel()
const status = await fresh.request<RelayStatus>('relay.status')
expect(status.pid).not.toBe(incumbent.pid)
fresh.close()
})
})
+3 -3
View File
@@ -187,7 +187,9 @@ describe('forceStopRelayForTarget', () => {
expect(command).toContain('lsof -t -a -U "$1"')
expect(command).toContain('pgrep -f "$sock_name"')
expect(command).toContain('/proc/net/unix')
expect(command).toContain('rm -f "$sock"')
// Why not: a stopped relay unlinks its own socket, and one that was killed leaves an
// inode the next deploy releases under its identity guard (STA-1756).
expect(command).not.toContain('rm -f')
})
it.skipIf(process.platform === 'win32')(
@@ -198,7 +200,6 @@ describe('forceStopRelayForTarget', () => {
expect(result.killCalls.join(' ')).toContain('-TERM')
expect(result.killCalls.join(' ')).not.toContain(UNRELATED_PID)
expect(result.ownerAlive).toBe(false)
expect(result.socketExists).toBe(false)
expect(result.error).toBeNull()
}
)
@@ -210,7 +211,6 @@ describe('forceStopRelayForTarget', () => {
expect(result.pgrepCalls.length).toBeGreaterThan(0)
expect(result.ownerAlive).toBe(false)
expect(result.socketExists).toBe(false)
expect(result.error).toBeNull()
}
)
+18 -11
View File
@@ -10,6 +10,15 @@ import { relaySocketNameForInstanceId } from './ssh-relay-instance-id'
const RESET_NO_OWNER_PROOF_MARKER = 'ORCA-RELAY-RESET-NO-OWNER-PROOF'
const RESET_OWNER_SURVIVED_MARKER = 'ORCA-RELAY-RESET-OWNER-SURVIVED'
/**
* Stop the relay owning this target's socket.
*
* Why it no longer removes the socket: a relay that stops cleanly unlinks its own path, and
* one that had to be killed leaves an inode the next deploy releases under its identity
* guard. Removing it here would need the same guard and would still race a relay binding the
* path between the check and the unlink — for no gain, since the leftover inode blocks
* nothing.
*/
export async function forceStopRelayForTarget(
conn: SshConnection,
relayInstanceId: string
@@ -32,7 +41,7 @@ export async function forceStopRelayForTarget(
' fi',
'}',
// Why an inventory rather than "no pid found": a lookup that returned nothing may
// simply have been unable to look, and reset must not read that silence as absence.
// simply have been unable to look, and reset must not read that silence as a stop.
// 0 = a process holds the socket, 1 = an inventory ran and none does, 2 = none ran.
'socket_listed() {',
' if [ -r /proc/net/unix ]; then',
@@ -46,27 +55,25 @@ export async function forceStopRelayForTarget(
'if [ -d "$base" ]; then',
' for sock in "$base"/relay-*/"$sock_name" "$base"/"$sock_name"; do',
' [ -S "$sock" ] || continue',
' signalled=no',
' find_holder "$sock"',
' if [ -n "$holder" ]; then',
' signalled=yes',
' kill -TERM $holder 2>/dev/null || true',
' sleep 0.2',
' kill -KILL $holder 2>/dev/null || true',
' sleep 0.2',
' fi',
// Why re-check both: a refused kill (permissions, a wedged process) leaves the owner
// listening, and removing its socket then hides it instead of stopping it.
// running, and reporting success then tells the user a relay was stopped that was not.
' socket_listed "$sock"',
' listed=$?',
' find_holder "$sock"',
' if [ "$listed" = 2 ]; then',
` echo ${RESET_NO_OWNER_PROOF_MARKER}`,
' continue',
' fi',
' if [ "$listed" = 0 ] || [ -n "$holder" ]; then',
` echo ${RESET_OWNER_SURVIVED_MARKER}`,
' continue',
' elif [ "$listed" = 2 ] && [ "$signalled" = no ]; then',
` echo ${RESET_NO_OWNER_PROOF_MARKER}`,
' fi',
' rm -f "$sock"',
' done',
'fi'
].join('\n')
@@ -74,14 +81,14 @@ export async function forceStopRelayForTarget(
const output = await execCommand(conn, script)
if (output.includes(RESET_OWNER_SURVIVED_MARKER)) {
throw new Error(
'The remote relay is still running after the reset signal, so its socket was left in ' +
'place rather than stranding it. Stop the relay process on the host, then retry.'
'The remote relay is still running after the reset signal. Stop the relay process on ' +
'the host, then retry.'
)
}
if (output.includes(RESET_NO_OWNER_PROOF_MARKER)) {
throw new Error(
'This host offers no way to see which process owns the relay socket (no /proc/net/unix ' +
'and no lsof), so it was left in place. Stop the relay process on the host, then retry.'
'and no lsof), so nothing was stopped. Stop the relay process on the host, then retry.'
)
}
}