Address (AI) review comments

- E2E: set up watching for dashboard open before trying to open it.
- Steve: don't record the port until everything is ready.
- Steve: set a timeout for port being ready.
- Steve: guard against concurrent calls to `start()`.
- Steve: track running by actually looking at the process.
- Steve: always clear port on stop (even if it's not running).
- Dashboard UI: get steve port via IPC.
- Dashboard UI: don't enable if steve port is unset.
- Dashboard UI: don't open if there is no port.
- Dashboard UI: open explorer view directly.
- Vuex store: plugins use the root store.
- Dependencies: ignore files that definitely not a steve archive.

Signed-off-by: Mark Yen <mark.yen@suse.com>

Author: mook-as

e2e/dashboard.e2e.spec.ts

@@ -17,14 +17,19 @@ test.describe.serial('Dashboard', () => {
   });
 
   test.afterAll(({ colorScheme }, testInfo) => teardown(electronApp, testInfo));
+  test.afterAll(() => dashboardPage?.close()?.catch(() => { /* ignore */ }));
+  test.afterAll(() => kubectl('delete', '--ignore-not-found', 'pod', 'e2e-nginx').catch(() => { /* ignore */ }));
 
   test('should allow opening the dashboard', async() => {
     const navPage = new NavPage(page);
 
     await navPage.progressBecomesReady();
+
+    // Set up the promise before clicking, to ensure we don't miss the event.
+    const dashboardPromise = electronApp.waitForEvent('window', page => page.url().includes('/c/local/explorer'));
     await navPage.dashboardButton.click();
 
-    dashboardPage = await electronApp.waitForEvent('window', page => page.url().includes('/c/local/explorer'));
+    dashboardPage = await dashboardPromise;
   });
 
   test('kubernetes should be available', async() => {
@@ -71,9 +76,4 @@ test.describe.serial('Dashboard', () => {
     const logs = windowManager.locator('*[id="panel-default/e2e-nginx-logs"] .logs-container');
     await expect(logs).toContainText('docker-entrypoint.sh');
   });
-
-  test('cleanup', async() => {
-    await dashboardPage.close();
-    await kubectl('delete', 'pod', 'e2e-nginx');
-  });
 });

e2e/extensions.e2e.spec.ts

@@ -457,7 +457,7 @@ test.describe.serial('Extensions', () => {
             await expect(result).resolves.toContain(contents);
           });
         } finally {
-          server.close();
+          await new Promise<Error | void>(resolve => server.close(resolve));
         }
       });
       test.describe('can post values', () => {

pkg/rancher-desktop/backend/steve.ts

@@ -1,16 +1,18 @@
 import { ChildProcess, spawn } from 'child_process';
 import os from 'os';
 import path from 'path';
-import { setTimeout } from 'timers/promises';
 
 import Electron from 'electron';
 
 import K3sHelper from '@pkg/backend/k3sHelper';
+import { getIpcMainProxy } from '@pkg/main/ipcMain';
 import Latch from '@pkg/utils/latch';
 import Logging from '@pkg/utils/logging';
 import paths from '@pkg/utils/paths';
+import { send } from '@pkg/window';
 
 const console = Logging.steve;
+const ipcMainProxy = getIpcMainProxy(console);
 
 /**
  * @description Singleton that manages the lifecycle of the Steve API.
@@ -19,11 +21,17 @@ export class Steve {
   private static instance: Steve;
   private process:         ChildProcess | undefined;
 
-  private isRunning: boolean;
+  // Promise to prevent multiple simultaneous calls to start() from causing
+  // multiple instances of Steve from being created.
+  private pendingStart: Promise<number> | undefined;
+
   #port = 0;
 
   private constructor() {
-    this.isRunning = false;
+    send('steve-port', 0);
+    ipcMainProxy.on('steve-port', () => {
+      send('steve-port', this.port);
+    });
   }
 
   /**
@@ -42,14 +50,34 @@ export class Steve {
    * @description Starts the Steve API if one is not already running.
    * Returns only after Steve is ready to accept connections.
    * @returns The port Steve is listening on.
+   * @note Concurrent calls are serialized.
    */
   public async start(): Promise<number> {
-    const { pid } = this.process || { };
+    // Prevent multiple concurrent calls to start().
+    const promise = this.pendingStart || this.startInternal();
+    this.pendingStart = promise;
+    try {
+      return await promise;
+    } finally {
+      this.pendingStart = undefined;
+    }
+  }
 
-    if (this.isRunning && pid) {
-      console.debug(`Steve is already running with pid: ${ pid }`);
+  /**
+   * This is the implementation of `start()`; it should always be called via
+   * `start()`, as it does not guard against concurrent calls.
+   */
+  private async startInternal(): Promise<number> {
+    if (this.isRunning) {
+      if (this.port) {
+        console.debug(`Steve is already running with port: ${ this.port }`);
 
-      return this.#port;
+        return this.port;
+      }
+      // If the process is running, but we don't have a port, suspect that Steve
+      // is in a bad state and restart it.
+      console.warn(`Steve process is running without a port. Restarting...`);
+      this.stop();
     }
 
     const osSpecificName = /^win/i.test(os.platform()) ? 'steve.exe' : 'steve';
@@ -62,10 +90,11 @@ export class Steve {
       // do nothing
     }
     console.debug(`Starting Steve with KUBECONFIG=${ env.KUBECONFIG }`);
-    this.process = spawn(stevePath, ['--context', 'rancher-desktop'],
+    const childProcess = spawn(stevePath, ['--context', 'rancher-desktop'],
       { env, stdio: ['ignore', 'pipe', 'pipe'], windowsHide: true });
+    this.process = childProcess;
 
-    const { stdout, stderr } = this.process;
+    const { stdout, stderr } = childProcess;
     let portBuffer = '';
     const portLatch = Latch<number>();
 
@@ -75,7 +104,7 @@ export class Steve {
       throw new Error(`Failed to start Steve: could not get output`);
     }
 
-    // Steve has been modified to output the port to stdout and then immediate
+    // Steve has been modified to output the port to stdout and then immediately
     // close it, leaving stderr open for logs.
     console.debug('Waiting for Steve to output port...');
     stdout.on('data', (data) => {
@@ -90,33 +119,70 @@ export class Steve {
       }
     });
 
+    // Set up a handler for the port latch erroring in case we never get to the
+    // point of waiting for it.
+    portLatch.catch((err) => {
+      console.error(err);
+      try {
+        // Kill the child process if it's still alive.
+        childProcess.kill();
+      } catch { /* ignore */ }
+    });
+
     stderr.on('data', (data) => {
       console.error(`stderr: ${ data }`);
     });
 
-    this.process.on('exit', (code, signal) => {
-      console.log(`child process exited with code ${ code } and signal ${ signal }`);
-      this.isRunning = false;
+    childProcess.on('exit', (code, signal) => {
+      if (childProcess !== this.process) {
+        // A stale process has exited; ignore.
+        console.debug(`Stale steve process exited with code ${ code } and signal ${ signal }`);
+        return;
+      }
+      console.log(`Steve process exited with code ${ code } and signal ${ signal }`);
+      this.#port = 0;
+      send('steve-port', 0);
       portLatch.reject(new Error(`Steve process exited unexpectedly with code ${ code } and signal ${ signal }`));
     });
 
     await new Promise<void>((resolve, reject) => {
-      this.process?.once('spawn', () => {
-        this.isRunning = true;
-        console.debug(`Spawned child pid: ${ this.process?.pid }`);
+      const timeout = setTimeout(() => {
+        const error = new Error('Timed out waiting for Steve to start');
+        portLatch.reject(error); // Kills the child process.
+        reject(error);
+      }, 10_000);
+      childProcess.once('spawn', () => {
+        clearTimeout(timeout);
+        console.debug(`Spawned child pid: ${ childProcess.pid }`);
         resolve();
       });
-      this.process?.once('error', (err) => {
+      childProcess.once('error', (err) => {
+        clearTimeout(timeout);
         reject(new Error(`Failed to spawn Steve: ${ err.message }`, { cause: err }));
       });
-      setTimeout(10_000).then(() => reject(new Error('Timed out waiting for Steve to start')));
     });
-    this.#port = await portLatch;
-    console.debug(`Steve is listening on port: ${ this.#port }`);
+    // Set a timeout in case Steve fails to listen to a port.
+    const portTimeout = setTimeout(() => {
+      portLatch.reject(new Error('Timed out waiting for Steve port'));
+    }, 30_000);
+    try {
+      const port = await portLatch;
+      console.debug(`Steve is listening on port: ${ port }`);
 
-    await this.waitForReady(this.#port);
+      await this.waitForReady(port);
+      this.#port = port;
+      send('steve-port', port);
 
-    return this.#port;
+      return port;
+    } finally {
+      clearTimeout(portTimeout);
+    }
+  }
+
+  private get isRunning() {
+    const { pid, exitCode, signalCode } = this.process || { };
+
+    return !!pid && exitCode === null && signalCode === null;
   }
 
   public get port() {
@@ -141,7 +207,7 @@ export class Steve {
         return;
       }
 
-      await setTimeout(delayMs);
+      await new Promise(resolve => setTimeout(resolve, delayMs));
     }
 
     throw new Error(`Steve did not become ready after ${ maxAttempts * delayMs / 1000 } seconds`);
@@ -172,11 +238,10 @@ export class Steve {
    * Stops the Steve API.
    */
   public stop() {
-    if (!this.isRunning) {
-      return;
-    }
-
-    this.process?.kill('SIGINT');
     this.#port = 0;
+    send('steve-port', 0);
+    if (this.isRunning) {
+      this.process?.kill('SIGINT');
+    }
   }
 }

pkg/rancher-desktop/components/DashboardOpen.vue

@@ -1,19 +1,20 @@
 <script lang="ts">
 import { defineComponent } from 'vue';
-import { mapGetters } from 'vuex';
 
 import { State as K8sState } from '@pkg/backend/backend';
+import { mapTypedGetters, mapTypedState } from '@pkg/entry/store';
 
 export default defineComponent({
   name:     'dashboard-open',
   computed: {
-    ...mapGetters('preferences', ['getPreferences']),
-    ...mapGetters('k8sManager', { k8sState: 'getK8sState' }),
+    ...mapTypedGetters('preferences', ['getPreferences']),
+    ...mapTypedGetters('k8sManager', { k8sState: 'getK8sState' }),
+    ...mapTypedState('steve', ['port']),
     kubernetesEnabled(): boolean {
       return this.getPreferences.kubernetes.enabled;
     },
-    kubernetesStarted(): boolean {
-      return this.k8sState === K8sState.STARTED;
+    dashboardReady(): boolean {
+      return this.k8sState === K8sState.STARTED && this.port > 0;
     },
   },
   methods: {
@@ -27,7 +28,7 @@ export default defineComponent({
 <template>
   <button
     v-if="kubernetesEnabled"
-    :disabled="!kubernetesStarted"
+    :disabled="!dashboardReady"
     class="btn role-secondary btn-icon-text"
     @click="openDashboard"
   >

pkg/rancher-desktop/entry/store.ts

@@ -14,6 +14,7 @@ import * as Preferences from '../store/preferences';
 import * as Prefs from '../store/prefs';
 import * as ResourceFetch from '../store/resource-fetch';
 import * as Snapshots from '../store/snapshots';
+import * as Steve from '../store/steve';
 import * as TransientSettings from '../store/transientSettings';
 
 const modules = {
@@ -31,16 +32,21 @@ const modules = {
   prefs:               Prefs,
   'resource-fetch':    ResourceFetch,
   snapshots:           Snapshots,
+  steve:               Steve,
   transientSettings:   TransientSettings,
 };
 
+export type Modules = typeof modules;
+
+export type RootState = {
+  [K in keyof Modules]: ReturnType<Modules[K]['state']>;
+};
+
 export default createStore<any>({
   modules: Object.fromEntries(Object.entries(modules).map(([k, v]) => [k, { namespaced: true, ...v }])),
-  plugins: Object.values(modules).flatMap(v => 'plugins' in v ? v.plugins : []),
+  plugins: Object.values(modules).flatMap<Plugin<RootState>>(v => 'plugins' in v ? v.plugins : []),
 });
 
-export type Modules = typeof modules;
-
 /**
  * mapTypedGetters is a wrapper around mapGetters that is aware of the types of
  * the Vuex stores we have availabile, and returns the correctly typed values.

pkg/rancher-desktop/store/diagnostics.ts

@@ -6,6 +6,7 @@ import { Plugin } from 'vuex';
 import { ActionTree, MutationsType } from './ts-helpers';
 
 import { CURRENT_SETTINGS_VERSION } from '@pkg/config/settings';
+import { RootState } from '@pkg/entry/store';
 import type { DiagnosticsResult, DiagnosticsResultCollection } from '@pkg/main/diagnostics/diagnostics';
 import ipcRenderer from '@pkg/utils/ipcRenderer';
 
@@ -165,7 +166,7 @@ export const actions = {
   },
 } satisfies ActionTree<DiagnosticsState, any, typeof mutations>;
 
-export const plugins: Plugin<DiagnosticsState>[] = [
+export const plugins: Plugin<RootState>[] = [
   // Vuex plugin used to refresh diagnostics on command from the backend.
   function(store) {
     ipcRenderer.on('diagnostics/update', () => {

pkg/rancher-desktop/store/steve.ts

@@ -0,0 +1,32 @@
+import { Plugin } from 'vuex';
+
+import { MutationsType } from './ts-helpers';
+
+import { RootState } from '@pkg/entry/store';
+import ipcRenderer from '@pkg/utils/ipcRenderer';
+
+interface SteveState {
+  port: number;
+}
+
+export const state: () => SteveState = () => (
+  {
+    port: 0,
+  }
+);
+
+export const mutations = {
+  SET_PORT(state, port) {
+    state.port = port;
+  },
+} satisfies MutationsType<SteveState>;
+
+export const plugins: Plugin<RootState>[] = [
+  // Vuex plugin to monitor for Steve port updates.
+  function({ commit }) {
+    ipcRenderer.on('steve-port', (event, port) => {
+      commit('steve/SET_PORT', port);
+    });
+    ipcRenderer.send('steve-port');
+  },
+];

pkg/rancher-desktop/typings/electron-ipc.d.ts

@@ -26,6 +26,8 @@ export interface IpcMainEvents {
   'k8s-integration-set':   (name: string, newState: boolean) => void;
   'factory-reset':         (keepSystemImages: boolean) => void;
   'update-network-status': (status: boolean) => void;
+  /** Trigger the corresponding IPC renderer event. */
+  'steve-port':            () => void;
 
   // #region main/update
   'update-state': () => void;
@@ -192,6 +194,7 @@ export interface IpcRendererEvents {
   'k8s-integrations':          (integrations: Record<string, boolean | string>) => void;
   'service-changed':           (services: ServiceEntry[]) => void;
   'service-error':             (service: ServiceEntry, errorMessage: string) => void;
+  'steve-port':                (port: number) => void;
   'kubernetes-errors-details': (
     titlePart: string,
     mainMessage: string,