fix(tracing): reject a dashboard viewer whose ui files are gone

[arelchan]

Summary

The dashboard opened as unstyled HTML that never connected: / and /api/health answered 200 while /app.css and /app.js both 404'd.

The port-reuse guard added in #141 asks the viewer whether it is alive, not whether it can still serve a page. Both the health payload and the HTML shell are produced from memory, while STATIC_DIR is read per request -- so a viewer whose install directory was removed rather than overwritten keeps reporting itself healthy for as long as the process stays up, and the launcher happily reuses it. Observed after the local install changed form (a switch to an editable install rebuilt the environment on a different Python version, deleting the directory the running viewer had been started from); a plain in-place upgrade overwrites the same path and does not trigger it.

Two changes, because either probe alone accepts a viewer that cannot render:

• _viewer_health now also fetches an asset the page cannot work without, and requires 200. This catches any stale viewer regardless of the code it is running -- including one started before this change.
• /api/health now reports the UI as part of health, answering 503 {"ok": false, "ui": "missing"} when its static directory no longer holds app.js, so a fresh process is honest about the same condition to any other consumer.

Type

• Fix
• Feature
• Docs
• CI / tooling
• Refactor
• Other

Verification

uv run pytest tests/test_cli_tracing_commands.py tests/test_tracing_api.py -q
  -> 27 passed

uv run ruff check raven tests            -> All checks passed!
uv run ruff format --check raven tests   -> 814 files already formatted
node --check raven/tracing/viewer/server.js -> ok

The new test is not vacuous -- against the same fake viewer (health 200, assets 404) the previous implementation of _viewer_health returns True and the new one returns False.

End-to-end against a real viewer started with app.js absent from its static directory:

GET /api/health -> 503 {"ok":false,"port":4399,"stateDir":"...","ui":"missing"}
_viewer_health(4399) -> False

And against a healthy viewer, /, /app.css, /app.js and /api/health all return 200 with "ui":"ok".

• Relevant tests pass locally
• Relevant lint / type checks pass locally
• User-facing docs or screenshots are updated when needed

Risk

The health endpoint gains a field and can now return 503. The only in-tree consumer is _viewer_health, which is updated here; an external consumer that treated any 200 as healthy would newly see a 503, which is the intended signal. No change to what the dashboard renders when it is working.

_VIEWER_UI_PROBE adds one localhost request per launch, on the path already taken only when a port is found occupied.

Rollback is reverting the commit: the guard returns to health-only and the endpoint to a bare ok: true.

• Security impact considered
• Backward compatibility considered
• Rollback path is clear for risky changes

Related Issues

N/A -- follow-up to #141, which introduced the guard this hardens.

[claude]

Claude finished @arelchan's task in 2m 22s —— View job

---

Review: no blockers; this can merge

Verified the two probes actually change the outcome (traced through serveStatic/STATIC_DIR in server.js and the urlopen calls in _viewer_health), and confirmed the new test (test_viewer_health_false_when_ui_assets_are_gone) is non-vacuous against the old implementation. Commit message and branch/scope conventions per AGENTS.md check out (tracing is a valid top-level scope, ASCII-only, Co-authored-by trailer present). No asset/large files, no test-naming violations, no weakened tests.

[nit] _viewer_health now also returns False when the port is our tracing viewer but its UI files are gone (the case this PR targets) — not only for a foreign server. The caller's message at raven/cli/tracing_commands.py:140-142 wasn't updated and still says "Port {port} is held by another process (not the tracing viewer)" in that case, which is now inaccurate: it is our viewer, just with a broken install. Behavior is still correct (skip reuse, pick a new port), so this is cosmetic only — worth a follow-up message tweak but not blocking.

Checked and found no issues with:

• /app.js reaching serveStatic correctly (no earlier route intercepts it before line 1024 in server.js)
• The /api/health 503 path being handled by the existing broad except Exception in _viewer_health (an HTTPError for non-2xx is caught the same way 404s already were)
• Startup-wait loop (_open_dashboard line 160) still converges quickly since the extra UI probe only fires once health already reports ok

Branch: fix/tracing_viewer_ui_health

[gloryfromca]

No blockers; this can merge as far as I am concerned.

What I actually verified

The fix changes the outcome, checked against the real viewer (not only the tests). Copied the viewer out of the tree and ran it under node: with ui/app.js present, /api/health answers 200 {"ok":true,...,"ui":"ok"}; after deleting ui/app.js the same server answers 503 {"ok":false,...,"ui":"missing"} and /app.js returns 404. So both halves bite, and they cover different populations - the server-side uiOk covers viewers started after this lands, and the client-side _VIEWER_UI_PROBE covers the case that motivated the change, an already-running viewer from the old build that keeps answering ok:true from memory forever.

The probe target is the right asset. SHELL_HTML is require('./ui/shell.js') at startup, so the shell is in memory and survives its file being deleted, while serveStatic resolves /app.js under STATIC_DIR per request. ui/shell.js loads exactly /app.js, so the probe asserts the asset the page needs. There is no SPA fallback that would make it vacuously pass: serveStatic special-cases only / and /index.html, everything else 404s when missing.

The test is non-vacuous and nothing was weakened. uv run pytest tests/test_cli_tracing_commands.py -q -> 5 passed, 0 skipped. With raven/cli/tracing_commands.py reverted to main and the new test kept, test_viewer_health_false_when_ui_assets_are_gone fails - so it would catch the regression. test_viewer_health_true_for_our_viewer still asserts True via the default ui_ok=True, so the happy path was not loosened to accommodate the new case.

Backward compatibility. An old CLI against a new server gets an HTTPError from the 503 and returns False, which is the direction you want (do not reuse a UI-less viewer). Nothing in ui/ reads /api/health, so the new 503 has no in-page consumer to break.

Repo rules and packaging. Comments are English and explain a hidden constraint (in-memory shell vs per-request asset read) rather than restating the code, per AGENTS.md 1.1/1.2; branch and commit grammar match 2.1/3.1; the test lands in tests/test_cli_tracing_commands.py, which is the file 5.1 requires for raven/cli/tracing_commands.py. raven/tracing/viewer/**/*.js is in the hatch wheel include list, so app.js ships and a normal install will not trip the new probe.

Covered: the diff, the callers (_open_dashboard reuse and startup-wait paths), AGENTS.md, backward compatibility in both directions, and whether the tests were weakened. One nonblocking note inline, about the message the newly-covered False path lands on.

[gloryfromca]

Nonblocking, and about the call site rather than this line.

When this returns False for the newly covered reason - our own viewer, assets gone - _open_dashboard falls into the else branch and prints, at raven/cli/tracing_commands.py:141:

Port {port} is held by another process (not the tracing viewer); starting on {free} instead.

In that case it is the tracing viewer, and the message points the user away from the only action that clears it. The stale process keeps holding the default port, so every later raven tracing dashboard walks to a fresh port and the user never learns why.

The two cases are already distinguishable here (health ok but UI probe failed, vs no health at all), so the branch could say something like "a stale tracing viewer is holding {port} with no UI assets; kill it or pass --port". Not a merge blocker - the fallback still hands the user a working dashboard.