Bugs in dashboard for triage

[SimonHeybrock]

Results from audit that need triage below.

---

I'll audit the dashboard code for subtle correctness bugs. Let me start by mapping the dashboard structure.

Now I'll launch five parallel audit agents, each covering a subsystem cluster, focused strictly on correctness bugs.

All five auditors are running. Meanwhile I'll review the remaining uncovered files myself (config adapters, registries, misc utilities).

Those two look fine. Checking the other uncovered files.

The utility files I reviewed myself (configuration_adapter, data_roles, plotter_registry, derived_devices, format_utils, save_filename, range_hook) look correct — no findings there. The five audit agents are still working through the core pipeline, buffers, orchestration, plotting, and widgets/session code; I'll verify and consolidate their findings as they report back.

The orchestration auditor reported back with several significant findings. Let me verify the key claims against the code before accepting them.

Confirmed — extractors are only ever added, never removed. Two of five auditors are in and verified; the orchestration findings (unguarded cross-thread mutation, mid-replay persistence truncation, extractor accumulation) and the buffer findings (datetime64 trim crash, live ring-buffer views) all check out against the code. Waiting on the core-pipeline, plotting, and widgets auditors before writing up the consolidated report.

The plotting auditor found a high-severity ROI-wipe bug. Verifying it and the slicer tuple/list comparison directly:

Confirmed — _initial_rois is parsed once from config and never updated, so every new browser session/reload republishes the config-time ROI set and wipes user-drawn ROIs on the backend. Four of five audits are now in and the major findings are verified. Waiting on the final widgets/session auditor before writing the consolidated report.

All five auditors are done. Let me spot-verify the widgets auditor's two high-severity claims before consolidating.

Both widget claims are confirmed by direct reading. Two last spot-checks on the remaining medium claims:

All findings are verified. The eviction claim and the DataService race are confirmed by direct reading (__setitem__ on an existing key doesn't refresh insertion order; register_subscriber/_get_extractors touch shared state locklessly across threads). Here is the consolidated audit.

---

Dashboard correctness audit

I audited all 81 files under src/ess/livedata/dashboard/ (five parallel deep-read passes over the data pipeline, buffers, orchestration, plotting, and widgets/session code, plus my own review of the utility modules). Every finding below was re-verified against the source; the two buffer bugs were additionally reproduced numerically against scipp 26.3.1. Line numbers refer to the current main checkout.

The overall picture: the single-writer designs (transactions in DataService, version-polling widgets, the notification queue) are solid. The bugs cluster in three areas — cross-thread paths that bypass the ingestion guard, buffer/time-coord edge cases, and UI state keyed by list position.

Critical / High

1. TemporalBuffer trim crashes on datetime64 time coords — permanent data loss once the buffer fills — temporal_buffers.py:440-443
cutoff = latest_time - timespan subtracts a float64 scalar (from sc.to_unit(sc.scalar(..., unit='s'), ...)) from a datetime64 coord, which scipp rejects (DTypeError: datetime arithmetic requires int64). datetime64 time coords do reach these buffers (ToNXlog emits sc.epoch('ns') + times; da00 decoding preserves datetime64), and extractors.py:189-191 handles this exact case with an explicit int64 cast. Reproduced: once a windowed buffer reaches max_capacity, the trim path raises and every subsequent add() for that key fails forever — the plot silently stops updating. Existing tests only use int64 coords.

2. Subscribing a plot to a running workflow races the ingestion thread — lost samples and dead plots — job_orchestrator.py:1042-1044 → data_service.py:135-156, temporal_buffer_manager.py:118-136 (found independently by two audit passes)
The workflow-commit path registers subscribers under ingestion_guard (job_orchestrator.py:459-461), but the immediate on_started callback fired when adding a plot layer to an already-running workflow has no guard. DataService.register_subscriber then appends to _subscribers and can trigger a Single→Temporal buffer migration (state.buffer swap) concurrently with the ingestion thread's state.buffer.add(data). Consequences: an in-flight sample lands in the orphaned pre-migration buffer and is silently lost; and _get_extractors (data_service.py:102, plain list iteration) racing unregister_subscriber's list.remove can skip a subscriber, creating the buffer as SingleValueBuffer for a windowed extractor — after which every extraction for that subscriber raises (swallowed at data_service.py:196-197) and the plot stays empty until recreated.

3. JobOrchestrator workflow state is mutated from both the ingestion and UI threads with no lock — job_orchestrator.py:837-862, 810-835 vs. 358-470, 782-808
on_job_status_updated → _deactivate_workflow runs on the ingestion thread (wired at dashboard_services.py:249, driven by the orchestrator-update thread), while commit_workflow/stop_workflow/subscribe_to_workflow run on the UI thread; only lines 459-461 are guarded. Concrete interleaving: the user restarts a workflow while the backend's final "stopped" heartbeats arrive → the UI thread's commit path (lines 392-414) and the ingestion thread's _deactivate_workflow interleave so that the retained previous-job buffers are wrongly evicted and state.previous becomes None; if the stop notification lands after _notify_workflow_available, layers are marked STOPPED although the new job runs, and data_arrived only transitions out of WAITING_FOR_DATA (plot_data_service.py:187-199) — plots stay blank until another restart. Separately, _notify_workflow_stopped_to_subscribers (line 974) and _notify_workflow_available (line 944) iterate the live subscription set while the UI thread adds/removes entries → RuntimeError: Set changed size during iteration aborts the notify loop mid-way, so remaining layers never learn the job stopped.

4. Every new browser session (or reload) republishes the config-time ROI set, wiping user-drawn ROIs — roi_request_plots.py:648-674, via session_layer.py:87
_create_edit_handler() ends with self._publish_rois(dict(self._initial_rois)), and it is called from create_presenter(), which runs once per session. _initial_rois is parsed once from config (line 550) and never updated on edits, and publishing an empty dict "clears all" (roi_publisher.py:48). So: user A draws ROIs, backend accumulates ROI spectra; user B opens the dashboard (or A reloads) → the initial (typically empty) set is published → the backend deletes all ROIs while A's browser still displays the rectangles. UI and backend are silently inconsistent until A's next edit resurrects the ROIs.

5. Disabled grids skew the tab↔grid index mapping — visible plots freeze, wrong tab removed — widgets/plot_grid_tabs.py:283-310, 335-363
_reconcile_grid_tabs keeps disabled grids in _grid_widgets but creates no tab for them, while _get_active_grid_id and _remove_grid_tab map tab positions to list(self._grid_widgets.keys()) positions. With grids A (disabled), B (enabled): viewing B resolves the "active grid" to A, so update_pipe() never runs and the interest token is released — all plots on the visible tab silently freeze (the compute gate may stop computing them entirely). Removing a grid pops the wrong tab (or raises IndexError, swallowed at plot_orchestrator.py:1730-1739, leaving a dead grid's tab on screen in every session). Existing tests only disable the last grid, missing the skew.

6. Cell-properties modal silently erases a user-defined cell title on Save — widgets/cell_properties_modal.py:94-99, 143-145
The title field is pre-filled via TextInput(value=...), but _persist_title() reads value_input, which Panel only populates on browser keystrokes. Opening the modal on a cell titled "Foo" and clicking Save (or "Add layer…") without typing persists None — the custom title is cleared in all sessions and in plot_configs.yaml.

Medium

7. TemporalBuffer.get() returns live views into the ring buffer — temporal_buffers.py:222, 245-247, 361-372. Reproduced: after the buffer fills, the next add() left-shifts in place (drop's memmove), silently mutating data already handed to extractors/subscribers — a previously extracted "latest" value changes under the consumer, and a session flushing a pipe payload concurrently can render shifted/torn data.

8. Startup replay persists mid-load — an exception permanently truncates the saved plot config — plot_orchestrator.py:1566-1595 with add_grid persisting at line 482. The first add_grid overwrites plot_grids in the store with a single grid; if any later add_layer raises (e.g. params-schema drift making a multi-layer cell non-overlayable), the catch-all at 1594 aborts the rest and the store retains only the grids applied so far — the user's remaining tabs are gone for good. A crash mid-load does the same. (Related latent hazard: shutdown() at 1794-1807 removes grids one by one, persisting each step, leaving {'grids': []} — currently only tests call it.)

9. Compute-gate race: an older build can overwrite a newer one — plot_data_service.py:248-332, plot_orchestrator.py:1084-1127, plots.py:656-697. The gate lock protects stash consumption, but plotter.compute runs outside it, deliberately on two different threads (activation on the polling thread, flush on the ingestion thread). A slow compute of delta v1 can finish after a fast compute of v2 and overwrite _cached_state — all sessions display older data; if the stream then goes quiet or stops, the stale frame persists indefinitely. The non-atomic _range_targets/_time_bounds/_cached_state writes can also tear.

10. ROI identity is list position — roi_request_plots.py:117-133, 237-246. BoxEdit/PolyDraw shapes carry no stable identity; index = enumeration position. Deleting rectangle A of {0: A, 1: B} publishes {0: B} — the backend treats B as an update of ROI 0, spectra and readback colors silently rebind to the wrong geometry.

11. Stale-session reaper never runs PlotGridTabs.shutdown() — reduction.py:187, session_registry.py:113-125, plot_grid_tabs.py:216-224. Only pn.state.on_session_destroyed is wired; the heartbeat reaper (which exists for browser-crash cases) never unsubscribes the dead session's PlotGridTabs from PlotOrchestrator._lifecycle_subscribers. The leaked strong reference defeats the weakref.finalize token fallback in LayerStateMachine.set_active — layers last viewed by a dead session are computed forever, and every grid mutation keeps rebuilding dead sessions' widgets. Fix shape: session_updater.register_cleanup_handler(plot_grid_tabs.shutdown), as already done for LogProducerWidget.

12. Slicer flatten mode: tuple-vs-list comparison makes the coord-preserving branch dead code — slicer_plotter.py:251-257. coord.dims == flatten_dims compares a tuple to a list — always False — so the documented "keep the 2-D coord" path never runs and the flattened axis always shows meaningless 0..N-1 indices.

13. Correlation histogram time-matching drops or fabricates points — correlation_plotter.py:142-157, 239-252. When all data timestamps precede the first axis reading, the 'nearest' fallback correlates every point to an axis value measured entirely after the data; once the streams overlap, the mode flips to 'previous' and those early points silently vanish from the histogram (NaN coords are dropped by hist), so displayed totals change discontinuously.

14. No overlap validation on cells across sessions — plot_orchestrator.py:643-673, widgets/plot_grid.py:287-313. Overlap prevention is per-session button state computed at first-click time; two sessions (or one slow wizard) can persist overlapping cells. Panel's GridSpec then silently deletes the overlapped plot, and the collision replays on every reload of plot_configs.yaml.

Low (verified, brief)

• Config-store "LRU" is FIFO-by-first-insertion (config_store.py:86-103): __setitem__ on an existing key doesn't refresh recency, so a config updated seconds ago can be evicted from the YAML while stale entries survive.
• Extractors accumulate on buffers forever (data_service.py:158-176, temporal_buffer_manager.py:134-138): set_extractors has zero callers; every plot edit / workflow restart appends, buffers never downgrade, history retention only shrinks on job cleanup.
• per_second normalization blanks single-sample streams (correlation_plotter.py:246-250): median of an empty interval array is NaN → all-NaN histogram until the second sample; also each point is divided by the forward interval (off by one sample on irregular streams).
• Log-scale clim for constant positive data forced to (1, 10) (plots.py:627-654): vmax <= vmin also catches vmax == vmin, so a uniform image (e.g. all 1e6) renders saturated with a colorbar reading 1–10.
• JobService.job_statuses hands the live dict across threads (job_service.py:40-48): a heartbeat insert during a widget refresh iteration → RuntimeError, refresh pass lost.
• Grid creation hijacks every session's active tab (plot_grid_tabs.py:312-322): lifecycle callbacks fire in all sessions; there is no "acting session" discrimination.
• _job_states leaks one entry per source on every restart-commit (job_orchestrator.py:392-414); pending commands never expire (pending_command_tracker.py:94-99 — timestamp written, never read; a dead worker leaves the entry and the user gets no feedback); buffer memory accounting ignores variances (temporal_buffers.py:402-408, ~2× the configured limit); slicer kdims frozen at first render (slicer_plotter.py:79-166, only bites if coords/shape change mid-job).

Checked and found correct (highlights)

DataService transaction nesting, exception handling, and the snapshot in _notify_subscribers; initial-data delivery on registration (once, no duplication); downsample_timeseries (verified numerically — latest point kept, no duplicates, idempotent); VariableBuffer.drop ring-shift boundaries; scipp→holoviews 2D dim/axis mapping (no transposition); 1D bin-edge vs midpoint handling; NotificationQueue cursor arithmetic under deque overflow; SessionRegistry lock discipline and double-cleanup guard; LayerSubscription stop dedup and full unsubscribe on layer removal; Kafka transport fatal-error escalation and flush discipline; time_utils timezone/precision handling.

Suggested priorities

If you fix only a few: #1 (one-line dtype cast, mirrors extractors.py:189-191), #4 (drop the initial publish or key it to first-presenter-only), #5/#6 (small, user-visible, multi-session), and then decide on a locking strategy for the ingestion/UI boundary that covers #2/#3/#9 together — they share the root cause that the ingestion_guard protects only the commit path, not the add-layer-while-running and status-heartbeat paths.

I can turn any of these into fixes with regression tests on the claude/esslivedata-dashboard-audit-gd0dkn branch — say which ones.

[SimonHeybrock]

Triage aid: the high-severity findings here cluster by root cause, now consolidated in #1046 / draft ADR #1048:

• Findings 2, 9 (register-while-running race, compute-gate stale overwrite) — plane 1, structurally removed by the pull-on-frame inversion Invert DataService delivery to pull-on-frame #1045.
• Finding 3 (JobOrchestrator cross-thread mutation, notify loops over live sets) — plane 2, the racy machinery is deleted by Assessment: Job identity and the JobOrchestrator/PlotOrchestrator complexity #1042 Option B rather than locked.
• Findings 5, 11 and "grid creation hijacks every session's active tab" (low) — plane 3 (PlotOrchestrator shared state has no thread-safety discipline (background thread vs UI; cross-session widget push) #1040, Dashboard: stale-session cleanup mutates Bokeh document from a background thread (unsafe) #955): version-polling for grid widgets + IOLoop-confined teardown.
• Extractor ratchet (low) — plane 1 follow-up (unregister_subscriber symmetry, noted in Audit results: dashboard data-delivery waste and bottlenecks in long-running operation #1044).

Independent of the program and worth fixing directly regardless: #1 (datetime64 trim crash — one-line cast), #4 (ROI wipe on new session), #6 (cell title erased on Save), #12 (tuple-vs-list dead branch), and the remaining mediums/lows.

---

Generated by Claude Code

[SimonHeybrock]

Decision for finding 4 (ROI wipe), verified against roi_request_plots.py: the root problem is that ROI state and the initial publish live in a per-session closure (_create_edit_handler, line 648: current_rois = dict(self._initial_rois); line 672: unconditional _publish_rois at presenter creation). Fix:

1. Hoist current_rois to plotter-instance state (self._current_rois, seeded once from _parse_initial_geometry()). The plotter is per-layer and shared across sessions; presenters/edit handlers read and update the shared state. (All session callbacks run on the single IOLoop thread per PlotOrchestrator shared state has no thread-safety discipline (background thread vs UI; cross-session widget push) #1040's threading-model note, so no new locking is needed today.)
2. Publish at most once per plotter lifetime, on the first compute() where _data_key becomes available (guarded by a flag) — i.e. when the plot layer is created — never in create_presenter()/_create_edit_handler(). A new browser session or reload then only renders the current shared state; it never republishes.
3. Seed each presenter's initial_stream_data from self._current_rois (not _initial_rois), so a session opened after edits shows the edited ROIs instead of the config-time set — fixing the silent UI/backend divergence noted in the finding.

Not addressed here (out of scope, tracked in #583): ROI identity is still list position (finding 10); the shared-state fix makes cross-session behavior consistent but doesn't give shapes stable IDs.

---

Generated by Claude Code

[SimonHeybrock]

Two corrections to my earlier comments on this issue, from adversarial review:

Finding 5 classification. The triage comment above assigned finding 5 (disabled-grid tab↔index skew) to plane 3 of #1046. That was wrong — it is a deterministic index-arithmetic bug, not a race or leak, and waiting for the plane-3 refactor would delay a user-visible fix for no reason. It goes in the immediate point-fix list (Wave 1.5 in the #1046 guide), which supersedes the plane-3 assignment.

Finding 4 (ROI) decision, strengthened. The earlier decision (hoist state to plotter level, publish once per plotter lifetime) has a hole the review caught: the plotter is recreated on every workflow commit (and on dashboard restart), so "once per plotter lifetime" ≈ "once per commit" — the recommit/restart wipe class would survive at behavior-parity with today, only the per-session wipe would die. Strengthened decision:

1. Hoist ROI state to plotter level and remove the per-session publish (as before).
2. Seed from the ROI readback stream instead of config: the plotter already receives readback data as its primary role in compute(). On first compute, if readback shows existing ROIs, adopt them as _current_rois and publish nothing; only when readback is empty publish the config-declared initial set (once). This kills the recommit wipe and the dashboard-restart wipe (including against a still-running backend job) in the same fix, and makes the backend the source of truth — consistent with where Unify ROI request/readback streams with shape ID #583 is headed.
3. Threading note, corrected: the justification "all session callbacks run on one IOLoop" was wrong for the new publish site — compute() runs on the ingestion/polling path, not the IOLoop. The fix is still safe (GIL-atomic dict replacement; the Kafka producer is thread-safe), but the hoisted _current_rois is now touched from two threads and should be documented as such.

Also for the record: finding 7 (live ring-buffer views) is expected to be subsumed by the #1009 stack (copy-out-of-buffer discipline for all DataService consumers) — verify on landing and close with that justification rather than fixing separately; findings 8 and 13 are assigned to the Wave 1.5 point-fix list in #1046, and the low "pending commands never expire" is promoted to Wave 1.5 because it is the only recovery signal for the commit send-failure mode noted on #1042.

---

Generated by Claude Code

[SimonHeybrock]

Finding 4 (ROI) decision, round 3 — the readback-seeding guard is broken; final shape below

A targeted verification of the round-2 decision traced the actual readback semantics and found the "publish config ROIs only when readback is empty" guard unsound:

Verified mechanics (file:line on main):

• Readback is re-emitted on every finalize cycle while detector data flows (roi_rectangle/roi_polygon are ordinary target_keys, handlers/detector_view/factory.py:242-243; finalize runs whenever primary data arrives, core/job.py:396-397). So a restarted dashboard does see current ROIs within one cycle — scenario (c) works.
• But ROI requests are per-job context keys ('{job_id}/roi_rectangle', factory.py:246-257, with job_number in the stream name) — a recommitted job can never see the old job's requests, its context defaults to None, and its first readback is empty (handlers/detector_view/roi.py:316-322). So on recommit the guard always reads "empty" and publishes the config set. Empty readback cannot distinguish "fresh job" from "user deleted all ROIs" — any auto-publish keyed on it resurrects ROIs the user explicitly cleared (empty-dict publish is a deliberate clear-all, roi_publisher.py:48).
• Additional gap in rounds 1–2: presenters and per-session edit baselines are seeded from _initial_rois (roi_request_plots.py:747-750, 648), and BoxEdit/PolyDraw send the session's complete shape list on first edit — a session seeded with stale config ROIs wipes the real set the moment the user drags anything.

Final decision:

1. Hoist ROI state to the plotter and remove the per-session publish (unchanged from round 1).
2. Seed presenters and session edit baselines from the plotter's live ROI state (adopted from readback when non-empty), never from _initial_rois after startup.
3. No auto-publish at all. Config-declared initial ROIs are published once at explicit user action — plot-config creation/edit — not inferred from readback state. A recommitted job therefore starts with no ROIs and the dashboard displays none (honest: dashboard and backend agree), and deleted ROIs stay deleted.
4. Consequence, documented rather than papered over: user-drawn ROIs do not survive a workflow recommit — this is backend behavior (per-job request keys), true today and unchanged by this fix. ROI continuity across recommit is a real feature request, but it needs either backend persistence of the request context across job restarts or deliberate dashboard republish of last-known state — that belongs with Unify ROI request/readback streams with shape ID #583 (stable ROI identity) and the slice (i) ROI resolver work in Assessment: Job identity and the JobOrchestrator/PlotOrchestrator complexity #1042, not hidden inside a bug fix.

If operations would rather have "recommit re-applies the configured initial ROIs" (deterministic seeding at every job start), that is a defensible alternative to (3) — but it must be chosen knowingly, since it re-imposes config ROIs over user deletions on every recommit. Default is (3).

---

Generated by Claude Code