fix(hopper): shut down the Linux demo helper orderly so its X lock is removed (#4966)

* fix(hopper): shut down the Linux demo helper orderly so its X lock is removed

Teardown SIGKILLed the helper's process group, so run_launch's finally
never ran stop_process on Xvfb and each session orphaned its
/tmp/.X<n>-lock. Later startups count orphans as address_collision.

Ask the helper to stop first: closing its stdin trips OwnerLifetime,
and its finally stops Hopper and Xvfb with SIGTERM, which removes the
lock. Anything still alive after the grace period is reaped by the
token-verified group cleanup.

Fixes #4921

Co-authored-by: Saim Shafique <128062316+sx4im@users.noreply.github.com>

* fix(hopper): notice signal-exited demo helpers during orderly shutdown

A helper that already died from a signal keeps exitCode null, so the
orderly-shutdown path treated it as still running and sat out the full
grace period before the group cleanup. Check signalCode alongside
exitCode in both the cleanup guard and the exit wait, and add a
regression test for an already signal-exited helper.

Also hardens the ignored-shutdown test: it now proves the fixture
started (waits for its lock), asserts the helper was reaped by the
group SIGKILL, and bounds the wait by the configured 500 ms grace
instead of a vacuous 30 s ceiling.

src/hopper/BridgeLauncher.ts

@@ -19,8 +19,13 @@ import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js";
 import { AnalysisCleanupLeaseGroup } from "../artifacts/AnalysisCleanupLease.js";
 import { AnalysisError } from "../domain/analysisErrorBase.js";
 import { cleanupOwnedProcessGroup } from "../process/ProcessOwnership.js";
+import type {
+  OwnedProcessGroup,
+  ProcessCleanupResult,
+} from "../process/ProcessOwnership.js";
 import { execFileOutput } from "../process/ExecFileOutput.js";
 import {
+  type ProviderProcessHandle,
   type ProviderProcessLaunch,
   spawnOwnedProviderProcess,
 } from "../process/ProviderProcess.js";
@@ -49,6 +54,12 @@ import { retryLinuxHopperPublisherCleanup } from "../application/LinuxHopperPubl
 
 const execFileAsync = promisify(execFile);
 const HOPPER_BACKGROUND_STARTUP_MS = 5_000;
+/**
+ * Bound for the Linux demo helper's orderly shutdown during teardown.
+ * The helper stops Hopper and Xvfb with SIGTERM (up to 2s each in
+ * stop_process); Xvfb removes its /tmp/.X<n>-lock on that clean exit.
+ */
+const LINUX_DEMO_ORDERLY_SHUTDOWN_GRACE_MS = 10_000;
 
 const bindRetainedArtifactCleanup = (
   error: AnalysisError,
@@ -148,6 +159,11 @@ export interface HopperApplicationLauncherDependencies {
     readonly helperPath: string;
     readonly signal?: AbortSignal;
   }) => Promise<LinuxPrivateDisplaySelection>;
+  /**
+   * How long teardown waits for the Linux demo helper's orderly shutdown
+   * (stdin EOF → SIGTERM to Hopper and Xvfb) before the process-group kill.
+   */
+  readonly demoOrderlyShutdownGraceMs?: number;
 }
 
 /**
@@ -386,7 +402,14 @@ export class HopperApplicationLauncher implements BridgeLauncher {
       });
       if (started === null) return err(hopperCancelledBySignal(signal));
       launched = true;
-      const cleanup = () => cleanupOwnedProcessGroup(started.ownership);
+      const cleanup = () =>
+        cleanupHopperLauncherSession({
+          helper: linuxDemo === undefined ? undefined : started.process,
+          ownership: started.ownership,
+          orderlyShutdownGraceMs:
+            this.dependencies.demoOrderlyShutdownGraceMs ??
+            LINUX_DEMO_ORDERLY_SHUTDOWN_GRACE_MS,
+        });
       const ownedLauncher = {
         ...(this.options.preparedImage === undefined
           ? {}
@@ -656,6 +679,60 @@ const hopperLaunchFailure = (
     ? new HopperCancelledError(signal.reason)
     : new HopperStartError({ cause });
 
+/**
+ * Shut down a launched Hopper session, asking the Linux demo helper to stop
+ * orderly before the process-group kill.
+ *
+ * The helper treats stdin EOF as its shutdown signal (OwnerLifetime): its
+ * run_launch finally block then stops Hopper and Xvfb with SIGTERM, and Xvfb
+ * removes its /tmp/.X<n>-lock on that clean exit. SIGKILLing the group first
+ * orphans the lock, and later startups count each orphan as an
+ * address_collision. Anything still alive after the grace period is reaped by
+ * the token-verified group cleanup, so a stuck helper cannot hang teardown.
+ */
+const cleanupHopperLauncherSession = async (input: {
+  readonly helper: ProviderProcessHandle | undefined;
+  readonly ownership: OwnedProcessGroup;
+  readonly orderlyShutdownGraceMs: number;
+}): Promise<ProcessCleanupResult> => {
+  const helper = input.helper;
+  if (
+    helper !== undefined &&
+    helper.exitCode === null &&
+    helper.signalCode === null
+  ) {
+    try {
+      helper.stdin?.end();
+    } catch {
+      // Already gone; the group cleanup below covers the remainder.
+    }
+    await waitForHelperExit(helper, input.orderlyShutdownGraceMs);
+  }
+  return cleanupOwnedProcessGroup(input.ownership);
+};
+
+/** Wait for the helper's exit, giving up after the grace period. */
+const waitForHelperExit = (
+  helper: ProviderProcessHandle,
+  timeoutMs: number,
+): Promise<void> =>
+  new Promise((resolve) => {
+    if (helper.exitCode !== null || helper.signalCode !== null)
+      return resolve();
+    const timer = setTimeout(
+      () => {
+        helper.off("exit", onExit);
+        resolve();
+      },
+      Math.max(0, timeoutMs),
+    );
+    const onExit = () => {
+      clearTimeout(timer);
+      resolve();
+    };
+    helper.once("exit", onExit);
+  });
+
 /** Select the publisher-verified demo adapter from the caller's explicit launch mode. */
 export const usesLinuxDemo = (
   options: HopperApplicationLauncherOptions,

tests/boundary/providers/hopper/bridgeLauncher.test.ts

@@ -547,3 +547,217 @@ it.skipIf(process.platform !== "linux")(
     expect(selectPrivateDisplay).not.toHaveBeenCalled();
   },
 );
+
+it.skipIf(process.platform !== "linux")(
+  "asks the demo helper to shut down orderly so its X lock is removed",
+  async () => {
+    const directory = await createTestTempDirectory("rea-hopper-orderly-");
+    const lockPath = join(directory, ".X199-lock");
+    const markerPath = join(directory, "orderly-shutdown");
+    const launcher = new HopperApplicationLauncher(
+      {
+        ...options(process.execPath),
+        launchMode: "verified_linux_demo",
+        environment: { REA_FIXTURE_LOCK_DIR: directory },
+        demoHelperPath: fileURLToPath(
+          new URL(
+            "../../../fixtures/hopperOrderlyShutdown.py",
+            import.meta.url,
+          ),
+        ),
+      },
+      {
+        verifyLinuxLauncher: async () =>
+          ok({
+            sha256: verifiedSha256,
+            version: "fixture",
+            primaryFingerprint: "0".repeat(40),
+            signingFingerprint: "1".repeat(40),
+            source: "signed_package_cache",
+          }),
+        selectPrivateDisplay: readyDirectDisplay,
+      },
+    );
+    const launched = await launcher.launch({
+      directory,
+      socketPath: join(directory, "bridge.sock"),
+      token: "fixture-token",
+      runId: randomUUID(),
+    });
+    if (!launched.ok) throw launched.error;
+    const supervisor = new ProviderProcessSupervisor(launched.value);
+    try {
+      const deadline = Date.now() + 5_000;
+      for (;;) {
+        try {
+          await access(lockPath);
+          break;
+        } catch {
+          if (Date.now() >= deadline)
+            throw new Error("lock file never appeared");
+          await new Promise((resolve) => setTimeout(resolve, 50));
+        }
+      }
+      expect(await supervisor.stop()).toMatchObject({
+        status: "verified-cleanup",
+      });
+      // The helper shut down orderly: it removed its own lock before exiting.
+      await expect(access(lockPath)).rejects.toMatchObject({ code: "ENOENT" });
+      await expect(access(markerPath)).resolves.toBeUndefined();
+    } finally {
+      await supervisor.stop();
+      await launched.value.releaseLease?.();
+    }
+  },
+);
+
+it.skipIf(process.platform !== "linux")(
+  "still cleans up when the demo helper ignores the orderly shutdown request",
+  async () => {
+    const directory = await createTestTempDirectory(
+      "rea-hopper-orderly-ignored-",
+    );
+    const lockPath = join(directory, ".X199-lock");
+    const launcher = new HopperApplicationLauncher(
+      {
+        ...options(process.execPath),
+        launchMode: "verified_linux_demo",
+        environment: {
+          REA_FIXTURE_LOCK_DIR: directory,
+          REA_FIXTURE_IGNORE_SHUTDOWN: "1",
+        },
+        demoHelperPath: fileURLToPath(
+          new URL(
+            "../../../fixtures/hopperOrderlyShutdown.py",
+            import.meta.url,
+          ),
+        ),
+      },
+      {
+        verifyLinuxLauncher: async () =>
+          ok({
+            sha256: verifiedSha256,
+            version: "fixture",
+            primaryFingerprint: "0".repeat(40),
+            signingFingerprint: "1".repeat(40),
+            source: "signed_package_cache",
+          }),
+        selectPrivateDisplay: readyDirectDisplay,
+        demoOrderlyShutdownGraceMs: 500,
+      },
+    );
+    const launched = await launcher.launch({
+      directory,
+      socketPath: join(directory, "bridge.sock"),
+      token: "fixture-token",
+      runId: randomUUID(),
+    });
+    if (!launched.ok) throw launched.error;
+    const supervisor = new ProviderProcessSupervisor(launched.value);
+    try {
+      // The fixture really started: its lock is on disk before we ask it to
+      // shut down, so an early fixture failure cannot fake a passing test.
+      const deadline = Date.now() + 5_000;
+      for (;;) {
+        try {
+          await access(lockPath);
+          break;
+        } catch {
+          if (Date.now() >= deadline)
+            throw new Error("lock file never appeared");
+          await new Promise((resolve) => setTimeout(resolve, 50));
+        }
+      }
+      const started = Date.now();
+      expect(await supervisor.stop()).toMatchObject({
+        status: "verified-cleanup",
+      });
+      // Bounded by the configured 500 ms grace, not a hardcoded 10 s wait:
+      // the group kill must follow the ignored shutdown request promptly.
+      expect(Date.now() - started).toBeLessThan(5_000);
+      // The fallback path ran: the helper was reaped by the group SIGKILL,
+      // and its lock survives because the orderly path never ran.
+      expect(launched.value.process.signalCode).toBe("SIGKILL");
+      await expect(access(lockPath)).resolves.toBeUndefined();
+    } finally {
+      await supervisor.stop();
+      await launched.value.releaseLease?.();
+    }
+  },
+);
+
+it.skipIf(process.platform !== "linux")(
+  "does not wait out the orderly grace when the demo helper already died from a signal",
+  async () => {
+    const directory = await createTestTempDirectory("rea-hopper-signal-dead-");
+    const lockPath = join(directory, ".X199-lock");
+    const launcher = new HopperApplicationLauncher(
+      {
+        ...options(process.execPath),
+        launchMode: "verified_linux_demo",
+        environment: {
+          REA_FIXTURE_LOCK_DIR: directory,
+          REA_FIXTURE_SIGNAL_DEATH: "1",
+        },
+        demoHelperPath: fileURLToPath(
+          new URL(
+            "../../../fixtures/hopperOrderlyShutdown.py",
+            import.meta.url,
+          ),
+        ),
+      },
+      {
+        verifyLinuxLauncher: async () =>
+          ok({
+            sha256: verifiedSha256,
+            version: "fixture",
+            primaryFingerprint: "0".repeat(40),
+            signingFingerprint: "1".repeat(40),
+            source: "signed_package_cache",
+          }),
+        selectPrivateDisplay: readyDirectDisplay,
+      },
+    );
+    const launched = await launcher.launch({
+      directory,
+      socketPath: join(directory, "bridge.sock"),
+      token: "fixture-token",
+      runId: randomUUID(),
+    });
+    if (!launched.ok) throw launched.error;
+    const supervisor = new ProviderProcessSupervisor(launched.value);
+    try {
+      // The fixture wrote its lock, then killed itself with SIGKILL before
+      // the launcher asked for cleanup.
+      const lockDeadline = Date.now() + 5_000;
+      for (;;) {
+        try {
+          await access(lockPath);
+          break;
+        } catch {
+          if (Date.now() >= lockDeadline)
+            throw new Error("lock file never appeared");
+          await new Promise((resolve) => setTimeout(resolve, 50));
+        }
+      }
+      const exitDeadline = Date.now() + 5_000;
+      for (;;) {
+        if (launched.value.process.signalCode !== null) break;
+        if (Date.now() >= exitDeadline)
+          throw new Error("signal exit never observed");
+        await new Promise((resolve) => setTimeout(resolve, 50));
+      }
+      const started = Date.now();
+      expect(await supervisor.stop()).toMatchObject({
+        status: "verified-cleanup",
+      });
+      // No grace wait: the helper was already gone, so cleanup must not sit
+      // out the 10 s orderly shutdown window.
+      expect(Date.now() - started).toBeLessThan(5_000);
+      expect(launched.value.process.signalCode).toBe("SIGKILL");
+    } finally {
+      await supervisor.stop();
+      await launched.value.releaseLease?.();
+    }
+  },
+);

tests/fixtures/hopperOrderlyShutdown.py

@@ -0,0 +1,46 @@
+#!/usr/bin/env python3
+"""Demo-helper stand-in: shuts down orderly when the owner pipe closes.
+
+Mirrors the production helper contract: the launcher owns this process group
+and asks for an orderly shutdown by closing stdin (OwnerLifetime). The
+production run_launch finally block then stops Hopper and Xvfb with SIGTERM;
+here the fixture removes its fake X lock file instead, which is what Xvfb
+does for /tmp/.X<n>-lock on a clean exit.
+"""
+
+import importlib.util
+import os
+import signal
+import sys
+import time
+from pathlib import Path
+
+spec = importlib.util.spec_from_file_location(
+    "hopper_demo_x11", Path(__file__).resolve().parents[2] / "scripts/hopper-demo-x11.py"
+)
+assert spec is not None and spec.loader is not None
+helper = importlib.util.module_from_spec(spec)
+spec.loader.exec_module(helper)
+assert "--owner-stdin" in sys.argv
+
+lock_dir = Path(os.environ["REA_FIXTURE_LOCK_DIR"])
+lock = lock_dir / ".X199-lock"
+# Xvfb writes its PID right-justified in a 10-character field.
+lock.write_text(f"{os.getpid():10d}\n")
+
+if os.environ.get("REA_FIXTURE_IGNORE_SHUTDOWN") == "1":
+    while True:
+        time.sleep(60)
+
+if os.environ.get("REA_FIXTURE_SIGNAL_DEATH") == "1":
+    # Die from a signal before the launcher asks for cleanup: teardown must
+    # notice the signal exit instead of waiting out the orderly grace period.
+    os.kill(os.getpid(), signal.SIGKILL)
+
+owner = helper.OwnerLifetime(True)
+while owner.alive():
+    time.sleep(0.05)
+# Orderly shutdown path: remove our own lock, like Xvfb does on SIGTERM.
+lock.unlink(missing_ok=True)
+(lock_dir / "orderly-shutdown").write_text("ok")
+sys.exit(0)