fix(dashboard): fast refusal budget when no install can run, and untangle the deps gate

scripts/dashboard.js

@@ -58,21 +58,14 @@ function readPid() {
   try { return Number.parseInt(fs.readFileSync(PID_FILE, 'utf8').trim(), 10); } catch (_) { return null; } // NOSONAR: missing or unreadable PID file simply means not running
 }
 
-async function start() {
-  if (!fs.existsSync(SERVER_SCRIPT)) {
-    console.error('EGC Dashboard not found. Expected: ' + SERVER_SCRIPT);
-    process.exit(1);
-  }
-
-  const PORT = getPort();
-
-  // Dependency gate shared with the init/install launcher: the list comes
-  // from dashboard/package.json (never drifts from the manifest) and a dep
-  // counts as present when require() can resolve it walking up from
-  // dashboard/. Only when something does not resolve is an on-demand
-  // install attempted, and only where it can succeed: inside a root-owned
-  // global prefix, npm install can only die with EACCES behind the
-  // detached spawn, which is how #1233 was hit.
+// Dependency gate shared with the init/install launcher: the list comes
+// from dashboard/package.json (never drifts from the manifest) and a dep
+// counts as present when require() can resolve it walking up from
+// dashboard/. Only when something does not resolve is an on-demand
+// install attempted, and only where it can succeed: inside a root-owned
+// global prefix, npm install can only die with EACCES behind the
+// detached spawn, which is how #1233 was hit.
+function ensureDashboardDeps() {
   const { checkDashboardDeps } = require(path.join(__dirname, 'lib', 'dashboard-deps'));
   const depsReport = checkDashboardDeps(DASHBOARD_DIR);
   if (depsReport.manifestError) {
@@ -82,22 +75,30 @@ async function start() {
     console.error('The installed package looks damaged. Reinstall with: npm install -g @egchq/egc');
     process.exit(1);
   }
-  if (depsReport.missing.length > 0) {
-    let writable = true;
-    try { fs.accessSync(DASHBOARD_DIR, fs.constants.W_OK); } catch (_) { writable = false; } // NOSONAR: probe only
-    if (!writable) {
-      console.error('Dashboard dependencies are missing and the package directory is not writable:');
-      console.error('  ' + DASHBOARD_DIR);
-      console.error('Update EGC to a release that ships them preinstalled, or install them once with:');
-      console.error('  sudo npm --prefix "' + DASHBOARD_DIR + '" install');
-      process.exit(1);
-    }
-    console.log('Installing dashboard dependencies...');
-    const r = spawnSync(process.platform === 'win32' ? 'npm.cmd' : 'npm', ['install'], {
-      cwd: DASHBOARD_DIR, stdio: 'inherit',
-    });
-    if (r.status !== 0) { console.error('npm install failed.'); process.exit(1); }
+  if (depsReport.missing.length === 0) return;
+  if (!depsReport.writable) {
+    console.error('Dashboard dependencies are missing and the package directory is not writable:');
+    console.error('  ' + DASHBOARD_DIR);
+    console.error('Update EGC to a release that ships them preinstalled, or install them once with:');
+    console.error('  sudo npm --prefix "' + DASHBOARD_DIR + '" install');
+    process.exit(1);
   }
+  console.log('Installing dashboard dependencies...');
+  const r = spawnSync(process.platform === 'win32' ? 'npm.cmd' : 'npm', ['install'], {
+    cwd: DASHBOARD_DIR, stdio: 'inherit',
+  });
+  if (r.status !== 0) { console.error('npm install failed.'); process.exit(1); }
+}
+
+async function start() {
+  if (!fs.existsSync(SERVER_SCRIPT)) {
+    console.error('EGC Dashboard not found. Expected: ' + SERVER_SCRIPT);
+    process.exit(1);
+  }
+
+  const PORT = getPort();
+
+  ensureDashboardDeps();
 
   const already = await isRunning();
   if (already) {

scripts/lib/dashboard-deps.js

@@ -10,17 +10,23 @@ const path = require('node:path');
 // dashboard/node_modules (git checkout with a local install) or in the
 // package root's node_modules (global npm install ships them at the root).
 function checkDashboardDeps(dashboardDir) {
+  // Whether an on-demand `npm install` inside dashboardDir could even
+  // succeed. In a root-owned global prefix it cannot (#1233), and callers
+  // use this to pick between a long install budget and a fast refusal.
+  let writable = true;
+  try { fs.accessSync(dashboardDir, fs.constants.W_OK); } catch (_) { writable = false; } // NOSONAR: probe only
+
   let deps;
   try {
     const manifest = JSON.parse(fs.readFileSync(path.join(dashboardDir, 'package.json'), 'utf8'));
     deps = Object.keys(manifest.dependencies || {});
   } catch (err) {
-    return { deps: [], missing: [], manifestError: err };
+    return { deps: [], missing: [], manifestError: err, writable };
   }
   const missing = deps.filter(dep => {
     try { require.resolve(dep, { paths: [dashboardDir] }); return false; } catch (_) { return true; } // NOSONAR: an unresolved dep is the datum being collected
   });
-  return { deps, missing, manifestError: null };
+  return { deps, missing, manifestError: null, writable };
 }
 
 module.exports = { checkDashboardDeps };

scripts/lib/dashboard-launch.js

@@ -78,13 +78,16 @@ function launchDashboard({ log = () => {} } = {}) {
     // startup (missing deps in a root-owned prefix, #1233) leaves the
     // success line as the last word while the port refuses connections.
     // The budget covers the child's own path: when every dashboard dep
-    // already resolves the server answers in well under 4s, but a first
-    // launch in a writable checkout may run a full npm install first, so
+    // already resolves the server answers in well under 4s, and a first
+    // launch in a WRITABLE checkout may run a full npm install first, so
     // announcing failure at 4s there would be a false verdict (PR #1234
-    // review).
+    // review). When deps are missing but the directory is not writable
+    // (the exact #1233 scenario) no install can run at all: the child
+    // refuses within a second, so the long budget would only delay the
+    // honest failure line by a minute (post-merge review of #1234).
     const { checkDashboardDeps } = require(path.join(__dirname, 'dashboard-deps'));
     const depsReport = checkDashboardDeps(path.join(__dirname, '..', '..', 'dashboard'));
-    const installAhead = depsReport.manifestError !== null || depsReport.missing.length > 0;
+    const installAhead = depsReport.missing.length > 0 && depsReport.writable && depsReport.manifestError === null;
     if (installAhead) log('First launch may install dashboard dependencies; giving it up to a minute.');
     const budgetMs = installAhead ? 60000 : 4000;
     return waitForDashboard(budgetMs).then(ready => {

tests/scripts/dashboard-launch.test.js

@@ -64,29 +64,36 @@ async function runTests() {
   if (test('both the CLI and the launcher gate through the shared dependency helper', () => {
     const launcherSource = fs.readFileSync(LAUNCHER, 'utf8');
     const cliSource = fs.readFileSync(DASHBOARD_CLI, 'utf8');
+    const helperSource = fs.readFileSync(DEPS_HELPER, 'utf8');
 
     // One source of truth: the dependency list comes from
     // dashboard/package.json via scripts/lib/dashboard-deps.js, never a
-    // hardcoded array that can drift from the manifest.
+    // hardcoded array that can drift from the manifest, and the
+    // writability probe lives in the helper so both callers see it.
     assert.ok(cliSource.includes("require(path.join(__dirname, 'lib', 'dashboard-deps'))"), 'dashboard.js must use the shared deps helper');
     assert.ok(launcherSource.includes("require(path.join(__dirname, 'dashboard-deps'))"), 'dashboard-launch.js must use the shared deps helper');
     assert.ok(!/\[\s*'express'\s*,\s*'ws'\s*\]/.test(cliSource), 'no hardcoded dependency list may remain');
-    assert.ok(/W_OK/.test(cliSource), 'the on-demand install must be gated on the directory being writable');
+    assert.ok(/W_OK/.test(helperSource), 'the writability probe must live in the shared helper');
     assert.ok(cliSource.includes('not writable'), 'the unwritable case must be reported honestly');
     assert.ok(cliSource.includes('manifest missing or unreadable'), 'a broken manifest must produce a clear message, not a raw stack trace');
   })) passed++; else failed++;
 
-  if (test('launcher polls for readiness with a budget that covers a first-time dependency install', () => {
+  if (test('launcher widens the poll budget only when an install can actually run', () => {
     const source = fs.readFileSync(LAUNCHER, 'utf8');
 
     assert.ok(/waitForDashboard\(/.test(source), 'launcher must poll the server after the detached spawn');
     assert.ok(source.includes('did not respond within'), 'a dead server must be reported without claiming a false verdict');
     assert.ok(source.includes('See the startup error with:'), 'the failure line must point at the foreground command');
     assert.ok(!/setTimeout\(openBrowser/.test(source), 'the browser must only open after readiness, not on a blind timer');
-    assert.ok(/60000/.test(source) && /4000/.test(source), 'the poll budget must widen when an on-demand install is ahead (PR #1234 review)');
+    // The 60s budget exists for a real on-demand install (writable
+    // checkout). In the unwritable #1233 scenario the child refuses within
+    // a second, so the launcher must keep the short budget there instead
+    // of stalling a minute before the honest failure line.
+    assert.ok(/missing\.length > 0 && depsReport\.writable/.test(source), 'the long budget must require the directory to be writable');
+    assert.ok(/60000/.test(source) && /4000/.test(source), 'both budgets must exist');
   })) passed++; else failed++;
 
-  if (test('checkDashboardDeps reports resolvable, missing, and broken-manifest states', () => {
+  if (test('checkDashboardDeps reports resolvable, missing, broken-manifest, and writability states', () => {
     const { checkDashboardDeps } = require(DEPS_HELPER);
 
     const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'egc-dash-deps-'));
@@ -95,6 +102,7 @@ async function runTests() {
       const broken = checkDashboardDeps(tmp);
       assert.ok(broken.manifestError, 'a missing manifest must surface as manifestError');
       assert.deepStrictEqual(broken.deps, [], 'no deps can be known without a manifest');
+      assert.strictEqual(broken.writable, true, 'a fresh tmpdir must probe as writable');
 
       // Unresolvable dependency.
       fs.writeFileSync(path.join(tmp, 'package.json'), JSON.stringify({
@@ -108,41 +116,48 @@ async function runTests() {
       fs.writeFileSync(path.join(tmp, 'package.json'), JSON.stringify({ dependencies: {} }));
       const clean = checkDashboardDeps(tmp);
       assert.deepStrictEqual(clean.missing, []);
+
+      // Unwritable directory (POSIX only: chmod cannot revoke directory
+      // write access on Windows, matching how install-sh.test.js skips;
+      // also skipped as root, which bypasses the W_OK probe entirely, so
+      // the case would fail in root-running Docker/CI).
+      if (process.platform !== 'win32' && typeof process.getuid === 'function' && process.getuid() !== 0) {
+        fs.chmodSync(tmp, 0o555);
+        try {
+          const readonly = checkDashboardDeps(tmp);
+          assert.strictEqual(readonly.writable, false, 'a read-only directory must probe as not writable');
+        } finally {
+          fs.chmodSync(tmp, 0o755);
+        }
+      }
     } finally {
       fs.rmSync(tmp, { recursive: true, force: true });
     }
   })) passed++; else failed++;
 
-  // Behavioral coverage of the poll itself (PR #1234 review asked for more
-  // than source scanning): run waitForDashboard against a throwaway HTTP
-  // server on a free port, then against the same port once it is closed.
-  const freePort = await new Promise(resolve => {
-    const probe = http.createServer();
-    probe.listen(0, '127.0.0.1', () => {
-      const { port } = probe.address();
-      probe.close(() => resolve(port));
-    });
+  // Behavioral coverage of the poll itself (PR #1234 review): the positive
+  // case binds port 0 and keeps that server ALIVE while waitForDashboard
+  // runs against it (no release-and-rebind race); the negative case then
+  // reuses the port only after the server is closed.
+  const server = http.createServer((req, res) => {
+    res.writeHead(200, { 'Content-Type': 'application/json' });
+    res.end('{"ts":0}');
   });
+  await new Promise(resolve => server.listen(0, '127.0.0.1', resolve));
+  const livePort = server.address().port;
 
-  process.env.EGC_PORT = String(freePort);
+  process.env.EGC_PORT = String(livePort);
   delete require.cache[require.resolve(LAUNCHER)];
   delete require.cache[require.resolve(path.join(__dirname, '..', '..', 'dashboard', 'port'))];
   const { waitForDashboard } = require(LAUNCHER);
 
-  if (await asyncTest('waitForDashboard resolves true once the server answers /ping', async () => {
-    const server = http.createServer((req, res) => {
-      res.writeHead(200, { 'Content-Type': 'application/json' });
-      res.end('{"ts":0}');
-    });
-    await new Promise(resolve => server.listen(freePort, '127.0.0.1', resolve));
-    try {
-      assert.strictEqual(await waitForDashboard(2000), true);
-    } finally {
-      await new Promise(resolve => server.close(resolve));
-    }
+  if (await asyncTest('waitForDashboard resolves true while the server answers /ping', async () => {
+    assert.strictEqual(await waitForDashboard(2000), true);
   })) passed++; else failed++;
 
-  if (await asyncTest('waitForDashboard resolves false when nothing listens within the budget', async () => {
+  await new Promise(resolve => server.close(resolve));
+
+  if (await asyncTest('waitForDashboard resolves false once nothing listens within the budget', async () => {
     const started = Date.now();
     assert.strictEqual(await waitForDashboard(700), false);
     assert.ok(Date.now() - started >= 600, 'the poll must keep trying until the budget is spent');