fix(ssh): refuse to publish a native-deps tree whose cloexec patch did not take

src/main/ssh/ssh-relay-deploy.ts

@@ -729,6 +729,29 @@ const NODE_PTY_VERSION = '1.1.0'
 const NODE_PTY_CONSOLE_LIST_PATCH_FILENAME = 'node-pty-1.1.0-console-list-agent-patch.cjs'
 const NODE_PTY_MASTER_CLOEXEC_PATCH_FILENAME = 'node-pty-1.1.0-master-cloexec-patch.cjs'
 const NODE_PTY_CLOEXEC_STATUS_PREFIX = 'ORCA-NPTY-CLOEXEC:'
+/**
+ * Whether the tree the patch left behind still leaks the pty master into every later child.
+ * `fixed` is the only outcome a shared cache entry may be published from.
+ */
+type NodePtyMasterCloexecOutcome = 'fixed' | 'unfixed'
+/**
+ * The statuses that leave a non-leaking tree. Deliberately an allowlist, not a `failed:` denylist:
+ * the script's `skipped:` family is mixed. `skipped:not-linux` is a platform that never leaks, but
+ * `skipped:earlier-attempt-failed`, `skipped:no-compiled-build`, `skipped:unexpected-source` and
+ * the two `skipped:<errno>` forms all mean the patch was refused and the leaky build is still on
+ * disk -- indistinguishable from `failed:` as far as what gets published.
+ */
+const NODE_PTY_CLOEXEC_FIXED_STATUSES: ReadonlySet<string> = new Set([
+  'patched',
+  // The rebuild ran from patched source; only the isolation check could not observe the result.
+  // An unobservable check is not a failed patch, and treating it as one would disable the shared
+  // cache on every host without `lsof`.
+  'patched-unverified',
+  'already-patched',
+  // Unreachable while the platform gate below short-circuits first, but it is the one `skipped:`
+  // that means "nothing to fix" rather than "would not fix it".
+  'skipped:not-linux'
+])
 // Exported for the relay-native-dependency-coverage test, which asserts every
 // native addon the relay bundle imports is either installed here or explicitly
 // declared as degrading without it.
@@ -1220,14 +1243,30 @@ async function installNativeDeps(
   // published -- and by contract immutable -- shared cache entry. Patching afterwards would write
   // through the link, and `.deps-complete` would already have published an unpatched tree that
   // every later host links and skips.
-  if (probe.available) {
-    await applyNodePtyMasterCloexecPatch(conn, remoteDir, platform, hostPlatform, nodePath, signal)
-  }
+  const cloexec = probe.available
+    ? await applyNodePtyMasterCloexecPatch(
+        conn,
+        remoteDir,
+        platform,
+        hostPlatform,
+        nodePath,
+        signal
+      )
+    : 'unfixed'
 
   // Why promotion is gated on the probe and not on npm's exit code: an entry is shared, so the
   // only evidence worth publishing is this host having loaded both addons out of that tree.
+  // Why it is gated on the patch too: a refused or rolled-back patch leaves the pre-patch leaky
+  // build in place, and the cache key hashes this patch's bytes -- so publishing it would hand
+  // every later host on the machine a tree that links, probes loadable, and skips patching.
   if (probe.available && cacheContext && cache) {
-    await promoteRelayNativeDepsCache(conn, cacheContext, cache.key)
+    if (cloexec === 'fixed') {
+      await promoteRelayNativeDepsCache(conn, cacheContext, cache.key)
+    } else {
+      console.warn(
+        `[ssh-relay][NPTY-CLOEXEC-UNSHARED] keeping the native deps at ${remoteDir} (${platform}) private; the tree still leaks the pty master, so it is not publishable as ${cache.key}`
+      )
+    }
   }
 
   // MISSING is non-fatal by design: the relay still serves fs/git/preflight; only native-backed ops fail on hosts that can't build the addons.
@@ -1251,6 +1290,9 @@ async function installNativeDeps(
  *
  * Why a shared cache entry never reaches here: the caller returns as soon as a linked tree probes
  * loadable, so this only ever rewrites a `node_modules` the relay directory still owns privately.
+ *
+ * Returns whether the tree that is left behind still leaks, which is what decides publishability.
+ * The script exits 0 on every outcome by design, so the status line is the only evidence there is.
  */
 async function applyNodePtyMasterCloexecPatch(
   conn: SshConnection,
@@ -1259,11 +1301,11 @@ async function applyNodePtyMasterCloexecPatch(
   hostPlatform: RemoteHostPlatform,
   nodePath: string,
   signal?: AbortSignal
-): Promise<void> {
+): Promise<NodePtyMasterCloexecOutcome> {
   // Linux is the only relay platform that takes forkpty()'s no-O_CLOEXEC path; macOS and Windows
   // ship prebuilds, so forcing a rebuild there would add a first compile to fix nothing.
   if (isWindowsRemoteHost(hostPlatform) || !platform.startsWith('linux')) {
-    return
+    return 'fixed'
   }
   try {
     const command = commandWithNodePath(
@@ -1282,7 +1324,16 @@ async function applyNodePtyMasterCloexecPatch(
         .map((line) => line.trim())
         .find((line) => line.startsWith(NODE_PTY_CLOEXEC_STATUS_PREFIX))
         ?.slice(NODE_PTY_CLOEXEC_STATUS_PREFIX.length) ?? 'no-status'
+    if (!NODE_PTY_CLOEXEC_FIXED_STATUSES.has(status)) {
+      // Warn, not log: the script exits 0 on a refusal too, so this line is the only thing that
+      // says the relay directory will leak a master into every child for its whole life.
+      console.warn(
+        `[ssh-relay][NPTY-CLOEXEC-UNFIXED] pty master still leaks at ${remoteDir} (${platform}): ${status}`
+      )
+      return 'unfixed'
+    }
     console.log(`[ssh-relay][NPTY-CLOEXEC] ${remoteDir} (${platform}): ${status}`)
+    return 'fixed'
   } catch (err) {
     signal?.throwIfAborted()
     // Never fatal: the script restores the working build itself, and a leaky relay beats none. An
@@ -1290,6 +1341,9 @@ async function applyNodePtyMasterCloexecPatch(
     console.warn(
       `[ssh-relay][NPTY-CLOEXEC-FAIL] pty master cloexec patch failed at ${remoteDir} (${platform}): ${(err as Error).message}`
     )
+    // An exec that never answered cannot say which build is on disk, and a tree nobody can vouch
+    // for is exactly the one not to share.
+    return 'unfixed'
   }
 }
 

src/main/ssh/ssh-relay-native-deps-cache.ts

@@ -26,7 +26,9 @@
  *
  * Linux's `node-pty-1.1.0-master-cloexec-patch.cjs` also mutates in place, but it stays inside rule
  * 1: the deploy path runs it before promotion, and returns early on a linked entry, so it only ever
- * touches a private tree. Its bytes are in the key, so a patched build never links a pre-patch entry.
+ * touches a private tree. Its bytes are in the key, so a patched build never links a pre-patch
+ * entry -- and a tree whose patch was refused or rolled back is not promoted at all, because under
+ * that same key it would publish the leak to every later host on the machine.
  */
 import { createHash } from 'node:crypto'
 import { RELAY_REMOTE_DIR } from './relay-protocol'

src/main/ssh/ssh-relay-native-deps-install-fixture.ts

@@ -199,7 +199,8 @@ export function makeExecResponses(opts: {
   }
   // Publication is gated on the probe: only a tree this host actually loaded is shared.
   if (loadable) {
-    // The cloexec patch runs first, so what gets published is already patched.
+    // The cloexec patch runs first, and publication is gated on its status, so `patched` is what
+    // makes the promote exec below reachable at all.
     slots.push(`${NODE_PTY_CLOEXEC_STATUS_PREFIX}patched\n`)
     slots.push('') // promote the private tree into the shared native-deps cache
   }

src/main/ssh/ssh-relay-pty-master-cloexec-install.test.ts

@@ -131,6 +131,34 @@ describe('relay pty-master close-on-exec patch on the install path', () => {
       .filter((command) => command.includes(PATCH_ASSET))
   }
 
+  /** Whether this deploy elected itself publisher of the shared entry. */
+  function promoted(): boolean {
+    return vi
+      .mocked(execCommand)
+      .mock.calls.some(([, command]) => command.includes('mkdir "$cache"'))
+  }
+
+  /**
+   * A cache-miss first install whose patch reports `status`. The promote slot is fed either way,
+   * so a run that wrongly promotes reads a valid response rather than falling off the end -- the
+   * assertion has to be the absence of the command itself, not a downstream crash.
+   */
+  function firstInstallReporting(status: string): ExecResponse[] {
+    return [
+      ...makeStagedFirstInstallExecPrefix(),
+      '', // npm install native deps
+      '', // chmod prebuilds
+      'ORCA-NPTY-PROBE-OK\n',
+      '', // rm probe stderr
+      `ORCA-NPTY-CLOEXEC:${status}\n`,
+      '', // promote into the shared native-deps cache, if this deploy still gets that far
+      '', // clean stage root
+      'DEAD',
+      '', // publish the per-launch credential
+      'READY'
+    ]
+  }
+
   it('runs the patch on a Linux relay once node-pty is proven loadable', async () => {
     const conn = makeMockConnection(sftpCapture)
     feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' }))
@@ -160,6 +188,61 @@ describe('relay pty-master close-on-exec patch on the install path', () => {
     expect(patchAt).toBeLessThan(promoteAt)
   })
 
+  it('does not publish a tree whose patch failed and rolled back', async () => {
+    // The script rolls `pty.cc` and `build/Release` back to the pre-patch, still-leaky build and
+    // reports `failed:` with exit 0, so nothing throws. Publishing that tree would be worse than
+    // the leak this PR closes: the key hashes the patch's bytes, so every later host on the
+    // machine links the entry, probes it loadable, and skips patching. Stay private instead.
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
+    try {
+      const conn = makeMockConnection(sftpCapture)
+      feed(firstInstallReporting('failed:npm rebuild node-pty failed: gyp ERR! not found: make'))
+
+      await deployAndLaunchRelay(conn)
+
+      expect(patchCommands()).toHaveLength(1)
+      expect(promoted()).toBe(false)
+      expect(warn.mock.calls.map((args) => String(args[0] ?? '')).join('\n')).toContain(
+        '[ssh-relay][NPTY-CLOEXEC-UNSHARED]'
+      )
+    } finally {
+      warn.mockRestore()
+    }
+  })
+
+  it('does not publish a tree the patch refused to touch', async () => {
+    // `skipped:` is not one verdict. Every form except `skipped:not-linux` means the patch was
+    // declined and the leaky build is still on disk, which is indistinguishable from `failed:`
+    // as far as what would get published.
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
+    try {
+      const conn = makeMockConnection(sftpCapture)
+      feed(firstInstallReporting('skipped:earlier-attempt-failed'))
+
+      await deployAndLaunchRelay(conn)
+
+      expect(promoted()).toBe(false)
+      // A refusal exits 0, so the warn is the only signal that this host stayed leaky.
+      expect(warn.mock.calls.map((args) => String(args[0] ?? '')).join('\n')).toContain(
+        '[ssh-relay][NPTY-CLOEXEC-UNFIXED]'
+      )
+    } finally {
+      warn.mockRestore()
+    }
+  })
+
+  it('still publishes a tree that was patched but whose isolation check could not run', async () => {
+    // `patched-unverified` rebuilt from patched source; only the check that watches a later child
+    // could not observe the result. An unobservable check is not a failed patch, and refusing to
+    // publish here would disable the shared cache on every host without `lsof`.
+    const conn = makeMockConnection(sftpCapture)
+    feed(firstInstallReporting('patched-unverified'))
+
+    await deployAndLaunchRelay(conn)
+
+    expect(promoted()).toBe(true)
+  })
+
   it('never patches through a symlink into an entry another relay already published', async () => {
     // A linked entry was built under a key that hashes this patch's bytes, so it is already
     // patched; re-running the patch would rebuild inside the shared tree.