Deep Review: 20260326-130316-pr-10018

If K8s restarts quickly before the old Steve process emits its close event, isRunning is still true when start() is called with a new port. The method silently returns at lines 48-52, and background.ts proceeds to update the proxy to the new (unused) port. In practice, K8s restart cycles take many seconds while Steve exits promptly on SIGINT, making this window negligible.

  public async start(httpsPort: number) {
    const { pid } = this.process || { };

    if (this.isRunning && pid) {
      console.debug(`Steve is already running with pid: ${ pid }`);

      return;
    }

    this.httpsPort = httpsPort;

    const osSpecificName = /^win/i.test(os.platform()) ? 'steve.exe' : 'steve';

Fix: either make Steve.stop() return a promise that resolves on process exit, or guard against mismatched ports:

 if (this.isRunning && pid) {
+  if (this.httpsPort !== httpsPort) {
+    throw new Error(`Steve is shutting down on port ${ this.httpsPort }`);
+  }
   console.debug(`Steve is already running with pid: ${ pid }`);
   return;
 }

getAvailablePorts() explicitly documents a TOCTOU window: ports are released before returning (lines 20-22 of networks.ts). Rebinding returned ports in the test depends on no other process claiming them first, which can cause intermittent CI failures on busy hosts.

  it('returns usable ports', async() => {
    const ports = await getAvailablePorts(2);

    // Verify both returned ports can actually be bound.
    for (const port of ports) {
      const server = net.createServer();

      await new Promise<void>((resolve, reject) => {
        server.once('error', reject);
        server.listen(port, '127.0.0.1', resolve);
      });

      await new Promise<void>((resolve) => {
        server.close(() => resolve());
      });
    }
  });

Fix: test deterministic properties only:

-  it('returns usable ports', async() => {
+  it('returns distinct positive ports', async() => {
     const ports = await getAvailablePorts(2);
-    for (const port of ports) {
-      // ... rebind attempt
-    }
+    expect(new Set(ports).size).toBe(ports.length);
+    for (const port of ports) {
+      expect(port).toBeGreaterThan(0);
+    }
   });

The docstring says "Call this before each Steve start," but background.ts:1287 calls it after Steve.start() succeeds (which is correct — proxies should point to where Steve is listening).

  /**
   * Update the Steve HTTPS port and recreate proxies with the new
   * target.  Call this before each Steve start.
   */
  public setStevePort(stevePort: number) {
    this.stevePort = stevePort;
    this.createProxies();
  }

Fix:

-   * target.  Call this before each Steve start.
+   * target.  Call this after each successful Steve start.

When Steve stops, DashboardServer still holds proxy middleware targeting the old ephemeral port. If another process binds that port, requests would be proxied to the wrong service. In practice, when K8s is STOPPING the dashboard button is disabled, so no user-initiated requests reach the proxy. Defense-in-depth, not a real exposure.

      if (state === K8s.State.STOPPING) {
        Steve.getInstance().stop();
      }

Fix: DashboardServer.getInstance().setStevePort(0) after Steve.stop().

The .once('error') listener at lines 109-111 is meant to catch spawn failures. If the process spawns successfully, this listener remains attached and would silently swallow a later runtime error by calling reject() on an already-resolved promise. Harmless in practice (Node.js ignores duplicate promise resolutions).

    await new Promise<void>((resolve, reject) => {
      this.process.once('spawn', () => {
        this.isRunning = true;
        console.debug(`Spawned child pid: ${ this.process.pid }`);
        resolve();
      });
      this.process.once('error', (err) => {
        reject(new Error(`Failed to spawn Steve: ${ err.message }`, { cause: err }));
      });
    });

Fix: remove the listener upon successful spawn.

The error message calculates 60 * 500 / 1000 = 30 seconds, but each loop iteration also awaits isPortReady() which has its own 1000ms timeout. If every attempt times out, actual elapsed time is up to 90 seconds, not 30.

      await setTimeout(delayMs);
    }

    throw new Error(`Steve did not become ready after ${ maxAttempts * delayMs / 1000 } seconds`);

Fix:

-throw new Error(`Steve did not become ready after ${ maxAttempts * delayMs / 1000 } seconds`);
+throw new Error(`Steve did not become ready after ${ maxAttempts } attempts`);

Electron.session.fromPartition() returns the same session for a given partition name. The setCertificateVerifyProc call at line 160 overwrites the previous callback on every iteration (up to 60 times in waitForReady). The function is identical each time, so this is harmless but redundant.

    return new Promise((resolve) => {
      const session = Electron.session.fromPartition('steve-healthcheck', { cache: false });

      session.setCertificateVerifyProc((request, callback) => {
        if (request.hostname === '127.0.0.1') {
          // We do not have any more information to narrow down the certificate;
          // given that we're doing this in a private partition, it should be
          // safe to allow all localhost certificates.  In particular, we do not
          // get access to the port number, and all the Steve certificates have
          // generic fields (e.g. subject).
          callback(0);
        } else {
          // Unexpected request; not sure how this could happen in a new session,
          // but we can at least pretend to do the right thing.
          callback(-3); // Use Chromium's default verification.
        }
      });

Fix: set the callback once in the constructor or on first call.