fix(dashboard): make the first launch honest and survivable on root-owned global installs - #1234
Conversation
…wned global installs A global npm install (distro Node, sudo) left the dashboard dead on first launch with no error anywhere: the init/install launcher spawns the server detached with its output discarded and printed the success line unconditionally, while scripts/dashboard.js tried an on-demand npm install inside the installed package's dashboard/ directory, which dies with EACCES on a root-owned prefix (#1233, reported and diagnosed in the field on Ubuntu 26.04 in #1218). Three changes, smallest bridge fix while the dashboard's future is decided: - express joins the root dependencies (ws already was one), so the global install ships everything dashboard/server.js resolves walking up from dashboard/ and the on-demand install stops being needed in the global-install case entirely - scripts/dashboard.js now gates that install on dependency resolution instead of a node_modules directory existing, and refuses honestly (with the exact command to run) when the package directory is not writable instead of letting npm die with a buried EACCES - the init/install launcher polls the detached server for readiness: on success it opens the browser only once the server answers, on failure it prints 'EGC Dashboard did not start.' plus the foreground command that reveals the real error Reported-by: ankit <rathaurankit501@gmail.com> Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com>
…#1234) Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ng it The dashboard server is plain node:http and never requires express: the declaration in dashboard/package.json was a phantom that forced the first-launch on-demand install (and briefly pushed express into the root dependencies on this branch). Removing the phantom leaves ws as the only dashboard dependency, which the root already ships, so a global install resolves everything with no on-demand install at all. The resolution gate in scripts/dashboard.js now reads the dependency list straight from dashboard/package.json, so the two can never drift apart again. Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/scripts/dashboard-launch.test.js">
<violation number="1" location="tests/scripts/dashboard-launch.test.js:56">
P3: The 'launcher polls for readiness' and 'CLI gates on resolution' cases verify behavior only by scanning the source file's text (regex/string matches for `waitForDashboard(`, `W_OK`, `require.resolve(...)`, and the absence of `setTimeout(openBrowser`), without ever executing `launchDashboard` or `dashboard.js`. A refactor that keeps these strings but breaks the actual wiring (e.g. opening the browser before the poll resolves, or dropping the poll from the launch path) would still pass the suite, so the tests offer no real regression protection for the #1233 failure they are meant to lock in. Consider exercising the functions under test (e.g. call `launchDashboard` against a mocked/non-responding ping and assert `launchDashboard` returns false and the failure line is logged) rather than grepping source. Since the other tests in this repo inspect source text too, this is a maintainability/effectiveness tradeoff worth confirming rather than a blocker.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // discarded; without a readiness poll, a server that dies during | ||
| // startup leaves the success line as the last word while the port | ||
| // refuses connections. | ||
| assert.ok(/waitForDashboard\(/.test(source), 'launcher must poll the server after the detached spawn'); |
There was a problem hiding this comment.
P3: The 'launcher polls for readiness' and 'CLI gates on resolution' cases verify behavior only by scanning the source file's text (regex/string matches for waitForDashboard(, W_OK, require.resolve(...), and the absence of setTimeout(openBrowser), without ever executing launchDashboard or dashboard.js. A refactor that keeps these strings but breaks the actual wiring (e.g. opening the browser before the poll resolves, or dropping the poll from the launch path) would still pass the suite, so the tests offer no real regression protection for the #1233 failure they are meant to lock in. Consider exercising the functions under test (e.g. call launchDashboard against a mocked/non-responding ping and assert launchDashboard returns false and the failure line is logged) rather than grepping source. Since the other tests in this repo inspect source text too, this is a maintainability/effectiveness tradeoff worth confirming rather than a blocker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/scripts/dashboard-launch.test.js, line 56:
<comment>The 'launcher polls for readiness' and 'CLI gates on resolution' cases verify behavior only by scanning the source file's text (regex/string matches for `waitForDashboard(`, `W_OK`, `require.resolve(...)`, and the absence of `setTimeout(openBrowser`), without ever executing `launchDashboard` or `dashboard.js`. A refactor that keeps these strings but breaks the actual wiring (e.g. opening the browser before the poll resolves, or dropping the poll from the launch path) would still pass the suite, so the tests offer no real regression protection for the #1233 failure they are meant to lock in. Consider exercising the functions under test (e.g. call `launchDashboard` against a mocked/non-responding ping and assert `launchDashboard` returns false and the failure line is logged) rather than grepping source. Since the other tests in this repo inspect source text too, this is a maintainability/effectiveness tradeoff worth confirming rather than a blocker.</comment>
<file context>
@@ -0,0 +1,93 @@
+ // discarded; without a readiness poll, a server that dies during
+ // startup leaves the success line as the last word while the port
+ // refuses connections.
+ assert.ok(/waitForDashboard\(/.test(source), 'launcher must poll the server after the detached spawn');
+ assert.ok(
+ source.includes('EGC Dashboard did not start.'),
</file context>
There was a problem hiding this comment.
Fair point. waitForDashboard is now exported and covered behaviorally: an ephemeral HTTP server on a free port must flip it to true, and a dead port must exhaust the budget to false. The structural checks stay as guardrails on top.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…and share the deps gate Review round of #1234: the 4s readiness poll would declare a false failure whenever the child's first launch legitimately runs a full npm install (writable git checkout), so the launcher now runs the same dependency-resolution check before spawning, through a shared helper (scripts/lib/dashboard-deps.js, list read from dashboard/package.json), and widens the budget to 60s when an install is ahead, announcing it. The timeout line no longer claims the server is dead, only that it has not responded within the budget. A broken or missing dashboard manifest now produces a clear reinstall message instead of a raw stack trace, and the readiness poll gained behavioral tests (ephemeral HTTP server and dead-port cases) on top of the structural checks. Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com>
Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com>
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/scripts/dashboard-launch.test.js">
<violation number="1" location="tests/scripts/dashboard-launch.test.js:119">
P3: The behavioral tests reuse a released ephemeral port: a probe server binds port 0, is closed, then the same port is rebound for the positive case and assumed free for the negative case. Between the probe's close()/the first test's server.close() and the subsequent waitForDashboard polls, the OS can hand the freed port to another process; if that process answers 200 on /ping, the 'resolves false' test fails spuriously. Consider asserting on a port that stays reserved for the suite's lifetime (e.g. keep one bound server and register whether a /ping answer comes from it), or at least documenting the race instead of relying on the released port staying free.</violation>
</file>
<file name="scripts/lib/dashboard-launch.js">
<violation number="1" location="scripts/lib/dashboard-launch.js:89">
P2: The readiness budget now widens to 60s whenever deps are missing OR the manifest is broken, but in the root-owned, non-writable global-prefix case (the exact #1233 scenario) no install can actually run — the spawned child `scripts/dashboard.js` detects the missing deps, sees the directory is not writable, prints its EACCES-style refusal, and calls `process.exit(1)` within milliseconds. `installAhead` only predicts "a long install may run", so this fast-fail case now blocks `egc init`/`egc install` polling a dead port for the full 60s before printing "did not respond within 60s", where the previous reviewed code reported "did not start" in 4s. A broken manifest (`manifestError !== null`) likewise can never install yet also forces the full 60s wait. That's the opposite of the "honest and survivable" goal for the exact failure the PR targets. Consider probing writability in the launcher too and failing fast (or treating `manifestError` separately) when no install can or will run, rather than spending the whole budget.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const depsReport = checkDashboardDeps(path.join(__dirname, '..', '..', 'dashboard')); | ||
| const installAhead = depsReport.manifestError !== null || depsReport.missing.length > 0; | ||
| if (installAhead) log('First launch may install dashboard dependencies; giving it up to a minute.'); | ||
| const budgetMs = installAhead ? 60000 : 4000; |
There was a problem hiding this comment.
P2: The readiness budget now widens to 60s whenever deps are missing OR the manifest is broken, but in the root-owned, non-writable global-prefix case (the exact #1233 scenario) no install can actually run — the spawned child scripts/dashboard.js detects the missing deps, sees the directory is not writable, prints its EACCES-style refusal, and calls process.exit(1) within milliseconds. installAhead only predicts "a long install may run", so this fast-fail case now blocks egc init/egc install polling a dead port for the full 60s before printing "did not respond within 60s", where the previous reviewed code reported "did not start" in 4s. A broken manifest (manifestError !== null) likewise can never install yet also forces the full 60s wait. That's the opposite of the "honest and survivable" goal for the exact failure the PR targets. Consider probing writability in the launcher too and failing fast (or treating manifestError separately) when no install can or will run, rather than spending the whole budget.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/dashboard-launch.js, line 89:
<comment>The readiness budget now widens to 60s whenever deps are missing OR the manifest is broken, but in the root-owned, non-writable global-prefix case (the exact #1233 scenario) no install can actually run — the spawned child `scripts/dashboard.js` detects the missing deps, sees the directory is not writable, prints its EACCES-style refusal, and calls `process.exit(1)` within milliseconds. `installAhead` only predicts "a long install may run", so this fast-fail case now blocks `egc init`/`egc install` polling a dead port for the full 60s before printing "did not respond within 60s", where the previous reviewed code reported "did not start" in 4s. A broken manifest (`manifestError !== null`) likewise can never install yet also forces the full 60s wait. That's the opposite of the "honest and survivable" goal for the exact failure the PR targets. Consider probing writability in the launcher too and failing fast (or treating `manifestError` separately) when no install can or will run, rather than spending the whole budget.</comment>
<file context>
@@ -77,13 +77,23 @@ function launchDashboard({ log = () => {} } = {}) {
+ const depsReport = checkDashboardDeps(path.join(__dirname, '..', '..', 'dashboard'));
+ const installAhead = depsReport.manifestError !== null || depsReport.missing.length > 0;
+ 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 => {
if (ready) {
</file context>
There was a problem hiding this comment.
| // 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 => { |
There was a problem hiding this comment.
P3: The behavioral tests reuse a released ephemeral port: a probe server binds port 0, is closed, then the same port is rebound for the positive case and assumed free for the negative case. Between the probe's close()/the first test's server.close() and the subsequent waitForDashboard polls, the OS can hand the freed port to another process; if that process answers 200 on /ping, the 'resolves false' test fails spuriously. Consider asserting on a port that stays reserved for the suite's lifetime (e.g. keep one bound server and register whether a /ping answer comes from it), or at least documenting the race instead of relying on the released port staying free.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/scripts/dashboard-launch.test.js, line 119:
<comment>The behavioral tests reuse a released ephemeral port: a probe server binds port 0, is closed, then the same port is rebound for the positive case and assumed free for the negative case. Between the probe's close()/the first test's server.close() and the subsequent waitForDashboard polls, the OS can hand the freed port to another process; if that process answers 200 on /ping, the 'resolves false' test fails spuriously. Consider asserting on a port that stays reserved for the suite's lifetime (e.g. keep one bound server and register whether a /ping answer comes from it), or at least documenting the race instead of relying on the released port staying free.</comment>
<file context>
@@ -46,44 +61,91 @@ function runTests() {
+ // 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', () => {
</file context>
There was a problem hiding this comment.
Also addressed in #1239: the positive case now keeps its server alive on the bound port while waitForDashboard runs against it, so there is no release-and-rebind window; the negative case reuses the port only after the close completes.
|
…ngle the deps gate (#1239) * fix(dashboard): fast refusal budget when no install can run, and untangle the deps gate Post-merge review of #1234 plus the Sonar complexity finding on main: - the launcher's 60s readiness budget now requires the dashboard directory to be writable: in the root-owned #1233 scenario the child refuses within a second, so the long budget only delayed the honest failure line by a minute; the writability probe moved into the shared helper so both callers see the same answer - the dependency/install gate in scripts/dashboard.js moved out of start() into ensureDashboardDeps(), resolving the cognitive complexity finding the merge introduced - the behavioral poll tests now keep the positive-case server alive on its bound port while waitForDashboard runs (no release-and-rebind race), and the helper gains an unwritable-directory case on POSIX Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com> * test(dashboard): skip the unwritable-directory case when running as root Root bypasses the W_OK access probe (accessSync succeeds on a 0o555 directory under uid 0), so the case would fail in root-running Docker or CI environments; same skip family as the existing Windows guard. Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com> --------- Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com>
1 participant
Fixes #1233
Summary
Bridge fix for the dashboard's first launch on root-owned global installs, intentionally minimal while the component's longer-term architecture is decided:
expressjoins the root dependencies (wsalready was one), so a globalnpm install -g @egchq/egcships everythingdashboard/server.jsresolves walking up fromdashboard/; the on-demandnpm installinside the package directory stops being needed in the global-install case entirelyscripts/dashboard.jsnow gates that on-demand install on dependency resolution (require.resolvefromdashboard/) instead of anode_modulesdirectory existing, and when the package directory is not writable it refuses honestly, printing the exact command to run, instead of letting npm die with a buriedEACCESscripts/lib/dashboard-launch.js) polls the detached server for readiness: on success the browser opens only once the server actually answers; on failure it printsEGC Dashboard did not start.plus the foreground command that reveals the real error, instead of leavingstarting at ...as the last word over a refused portWhy
The v1.1.18 Linux testing round (#1218) hit this end to end on Ubuntu 26.04 with distro Node and a sudo global install:
egc initannounced the dashboard URL, the detached process died instantly, and the port refused connections with no error anywhere. A guided foreground run confirmed the root cause:EACCES: permission denied, mkdir '/usr/local/lib/node_modules/@egchq/egc/dashboard/node_modules'from the on-demand dependency install. Full dossier in #1233.Credit
Reported, reproduced and root-cause-confirmed in the field by @rathaur-ankit in #1218 (his second Linux testing report, same round that produced #1231).
Testing
node --checkon both changed scriptsnode tests/scripts/dashboard-launch.test.js: 3/3 (new: root ships dashboard deps; launcher readiness poll and honest failure; resolution-gated install with writability refusal)node tests/scripts/install-onboarding.test.js: 11/11 (wrapper contract intact)node tests/scripts/npm-publish-surface.test.js: 2/2 (publish surface intact)Summary by cubic
Fixes the dashboard’s first launch on root-owned global installs. Removes a phantom dependency, avoids unwritable on-demand installs, and makes startup checks honest and resilient.
expressfromdashboard/package.json; the server uses Nodehttpandwsonly. Rootdependenciesalready includews, so global installs resolve everything without an on-demand install.scripts/dashboard.jsnow gates installs through a shared helper that readsdashboard/package.jsonand resolves deps fromdashboard/; if missing and the directory is unwritable it exits with a clear message and the exact one-time install command, and if the manifest is missing it points to reinstall.scripts/lib/dashboard-launch.jspolls for readiness and opens the browser only when the server answers; it widens the budget to 60s when a first-time install may run (otherwise 4s), and on timeout prints “did not respond within Ns” plus the foreground command to see the real error.Written for commit eb2a824. Summary will update on new commits.