Browse Source

web: fix recipes-empty-after-load + dead log streams

Two live-reported bugs:

1. Recipe select stayed empty after initial load. The recipes fetch
   re-rendered via onState(state.snap) with the SAME object —
   sameSnapshot (anti-blink) always skipped it, so the action bar kept
   the empty select from the first render until an unrelated fleet
   change. Now a force re-render (same data, no animation —
   state.panel unchanged). Live: 33 recipes after load, zero
   interaction.

2. Second log click did nothing. openLog's 'window already open'
   guard just re-fronted the window — invisibly when it was already
   in front, and uselessly when the stream had died (container
   stopped, SSH drop). Now:
   - front + visible flash (feedback either way)
   - re-attach (fresh 200-line history + follow) when the attach is
     done — driven by Snapshot.attaches[].done
   - 'stream ended — click ↻' note in the window (updateLogNotes
     runs on every state push, including no-churn ticks; onState now
     adopts the fresh snapshot before the early return so attachDone
     never reads stale attaches)
   - ↻ refresh button in the log bar
   - server: the stream task calls push_state when the stream ends,
     so attaches[].done + fleet changes reach the UI immediately
     instead of on the next 30 s tick

Playwright +2 checks (dead-stream note on the instant-dying fake
fleet, second-click flash + no duplicate). Live-verified on dgx1:
flash + z-raise on second click, stream still flowing (200→202
lines).
Alexander Korolev 2 months ago
parent
commit
b1854d10ab
4 changed files with 93 additions and 25 deletions
  1. 1 2
      src/web/api.rs
  2. 18 19
      src/web/logs.rs
  3. 50 4
      src/web/ui.html
  4. 24 0
      src/web/ui.playwright.mjs

+ 1 - 2
src/web/api.rs

@@ -221,8 +221,7 @@ pub(crate) async fn logs_attach(
     Json(req): Json<LogsReq>,
 ) -> Result<Json<serde_json::Value>, ApiError> {
     let (id, history) = super::logs::attach(
-        &app.config,
-        &app.bus,
+        &app,
         &app.log_attaches,
         &req.cluster,
         &req.node,

+ 18 - 19
src/web/logs.rs

@@ -10,7 +10,6 @@ use std::sync::{Arc, Mutex};
 
 use tokio::task::JoinHandle;
 
-use crate::config::Config;
 use crate::ssh::shq;
 use crate::web::events::Event;
 use crate::web::ssh_stream::stream_lines;
@@ -89,14 +88,14 @@ pub(crate) fn push_line(ring: &Mutex<VecDeque<String>>, line: &str) {
 /// Idempotent per (cluster, node, recipe, rank): a live stream for the same
 /// key is reused; a finished one is replaced.
 pub fn attach(
-    config: &Config,
-    bus: &tokio::sync::broadcast::Sender<Event>,
+    app: &crate::web::AppState,
     attaches: &Mutex<Vec<LogAttach>>,
     cluster: &str,
     node: &str,
     recipe: &str,
     rank: u32,
 ) -> Result<(String, Vec<String>), String> {
+    let config = &app.config;
     let cl = config
         .clusters
         .iter()
@@ -136,13 +135,14 @@ pub fn attach(
     let done = Arc::new(AtomicBool::new(false));
     let cmd = logs_stream_cmd(recipe, rank, TAIL_LINES);
     let (h, s, d) = (history.clone(), stop.clone(), done.clone());
-    let (cfg, node_obj, bus) = (config.clone(), node_cfg.clone(), bus.clone());
+    let (cfg, node_obj, bus) = (config.clone(), node_cfg.clone(), app.bus.clone());
     let (idc, cluster_s, node_s, recipe_s) = (
         id.clone(),
         cl.name.clone(),
         node_cfg.name.clone(),
         recipe.to_string(),
     );
+    let appc = app.clone();
     let task = tokio::spawn(async move {
         let _ = stream_lines(&cfg, &node_obj, cmd, s, |line| {
             push_line(&h, line);
@@ -156,8 +156,11 @@ pub fn attach(
             });
         })
         .await;
-        // Stream errors (node offline, container gone) just end the window.
+        // Stream errors (node offline, container gone) just end the window —
+        // refresh the board so attaches[].done (and fleet changes) reach the
+        // UI immediately instead of on the next 30 s tick.
         d.store(true, Ordering::Relaxed);
+        crate::web::push_state(&appc).await;
     });
 
     let attach = LogAttach {
@@ -214,7 +217,6 @@ pub fn by_key(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::web::events::Event;
 
     fn ring() -> Arc<Mutex<VecDeque<String>>> {
         Arc::new(Mutex::new(VecDeque::new()))
@@ -267,26 +269,25 @@ mod tests {
 
     #[tokio::test]
     async fn attach_is_idempotent_and_bounded() {
-        let (tx, _rx) = tokio::sync::broadcast::channel::<Event>(8);
+        let app = crate::web::tests::app_state(None);
         let attaches: Mutex<Vec<LogAttach>> = Mutex::new(Vec::new());
-        let cfg = crate::web::tests::fake_config();
         // 8 attaches fill the cap; the 9th is refused.
         for i in 0..MAX_ATTACHES {
-            let (id, _) = attach(&cfg, &tx, &attaches, "fake", "n0", &format!("r{i}"), 0).unwrap();
+            let (id, _) = attach(&app, &attaches, "fake", "n0", &format!("r{i}"), 0).unwrap();
             assert_eq!(id, format!("l{}", i + 1));
         }
-        let err = attach(&cfg, &tx, &attaches, "fake", "n0", "overflow", 0).unwrap_err();
+        let err = attach(&app, &attaches, "fake", "n0", "overflow", 0).unwrap_err();
         assert!(err.contains("max"), "{err}");
         // Unknown cluster / node are 400-class errors.
-        assert!(attach(&cfg, &tx, &attaches, "nope", "n0", "r", 0).is_err());
-        assert!(attach(&cfg, &tx, &attaches, "fake", "nope", "r", 0).is_err());
+        assert!(attach(&app, &attaches, "nope", "n0", "r", 0).is_err());
+        assert!(attach(&app, &attaches, "fake", "nope", "r", 0).is_err());
         // by_key finds what exists, misses what doesn't.
         assert!(by_key(&attaches, "fake", "n0", "r0", 0, 10).is_some());
         assert!(by_key(&attaches, "fake", "n0", "r0", 7, 10).is_none());
         // Detach removes + frees a slot; re-attach after detach works.
         assert!(detach(&attaches, "l1"));
         assert!(!detach(&attaches, "l1"));
-        attach(&cfg, &tx, &attaches, "fake", "n0", "new", 0).unwrap();
+        attach(&app, &attaches, "fake", "n0", "new", 0).unwrap();
         assert_eq!(attaches.lock().unwrap().len(), MAX_ATTACHES);
     }
 
@@ -310,12 +311,11 @@ mod tests {
 
     #[tokio::test]
     async fn attach_reuses_live_stream_for_same_key() {
-        let (tx, _rx) = tokio::sync::broadcast::channel::<Event>(8);
+        let app = crate::web::tests::app_state(None);
         let list: Mutex<Vec<LogAttach>> = Mutex::new(Vec::new());
         list.lock().unwrap().push(fake_attach("l1", false));
-        let cfg = crate::web::tests::fake_config();
         // Same key, live stream -> the existing attach is reused, no new task.
-        let (id, history) = attach(&cfg, &tx, &list, "fake", "n0", "r", 0).unwrap();
+        let (id, history) = attach(&app, &list, "fake", "n0", "r", 0).unwrap();
         assert_eq!(id, "l1");
         assert!(history.is_empty());
         assert_eq!(list.lock().unwrap().len(), 1, "no second attach spawned");
@@ -323,12 +323,11 @@ mod tests {
 
     #[tokio::test]
     async fn attach_replaces_finished_stream_for_same_key() {
-        let (tx, _rx) = tokio::sync::broadcast::channel::<Event>(8);
+        let app = crate::web::tests::app_state(None);
         let list: Mutex<Vec<LogAttach>> = Mutex::new(Vec::new());
         list.lock().unwrap().push(fake_attach("l1", true));
-        let cfg = crate::web::tests::fake_config();
         // Same key, finished stream -> dropped and replaced with a new task.
-        attach(&cfg, &tx, &list, "fake", "n0", "r", 0).unwrap();
+        attach(&app, &list, "fake", "n0", "r", 0).unwrap();
         let g = list.lock().unwrap();
         assert_eq!(g.len(), 1, "old attach dropped");
         assert_eq!(g[0].id, "l1", "slot id may be reused");

+ 50 - 4
src/web/ui.html

@@ -241,6 +241,7 @@
     border-radius: 5px; padding: 1px 5px; font: inherit; font-size: 12px; height: 22px;
   }
   .logsbtn { margin-left: auto; height: 20px; font-size: 11px; padding: 0 8px; flex-shrink: 0; }
+  .win.flash { outline: 2px solid var(--accent); outline-offset: -1px; }
   .dockmenu {
     position: fixed; bottom: 44px; right: 12px; display: flex; flex-direction: column; gap: 4px;
     background: var(--panel); border: 1px solid var(--border); border-radius: 10px; padding: 8px;
@@ -635,7 +636,11 @@ function abtn(id, label, fn, cls) {
 }
 function onState(snap, force) {
   const prev = state.snap;
-  if (!force && prev && sameSnapshot(prev, snap)) return; // nothing changed — no DOM churn
+  updateLogNotes(snap); // runs even on no-churn ticks (attaches change out-of-band)
+  if (!force && prev && sameSnapshot(prev, snap)) {
+    state.snap = snap; // adopt the fresh attaches state; no DOM churn
+    return; // nothing else changed — no rebuild
+  }
   state.snap = snap;
   state.jobs = new Map(snap.jobs.map((j) => [j.id, j]));
   if (!snap.clusters.find((c) => c.name === state.active)) {
@@ -756,6 +761,30 @@ function onJob(ev) {
   renderJobs();
 }
 
+// Stream-end notes: the snapshot carries attaches[].done; surface a
+// re-attach hint in each log window whose stream died.
+function updateLogNotes(snap) {
+  for (const w of logWins.values()) {
+    if (!w.attach || !snap.attaches) continue;
+    const a = snap.attaches.find((x) => x.id === w.attach);
+    const note = w.win.querySelector(".lognote");
+    if (!note) continue;
+    if (a && a.done && !note.classList.contains("dead")) {
+      note.textContent = "stream ended — click ↻ to re-attach";
+      note.classList.add("dead");
+    }
+  }
+}
+function attachDone(attachId) {
+  if (!attachId) return true;
+  const a = state.snap && state.snap.attaches ? state.snap.attaches.find((x) => x.id === attachId) : null;
+  return a ? a.done : false;
+}
+function flashWin(win) {
+  win.classList.add("flash");
+  setTimeout(() => win.classList.remove("flash"), 700);
+}
+
 // --- log windows -------------------------------------------------------------------
 const MAX_PRE_LINES = 1000;
 // ponytail: trimPre re-splits the whole text per line (O(n)); fine at log
@@ -785,9 +814,15 @@ async function openLog(cluster, node, recipe) {
   const c = snap ? snap.clusters.find((x) => x.name === cluster) : null;
   const n = c ? c.nodes.find((x) => x.name === node) : null;
   if (!n) return;
-  // A window for this node is already open and no specific recipe asked for → front it.
+  // A window for this node is already open and no specific recipe asked for
+  // → front it with feedback; if its stream died, re-attach (fresh history).
   if (!recipe) {
-    for (const w of logWins.values()) if (w.cluster === cluster && w.node === node) { front(w.win); return; }
+    for (const w of logWins.values()) if (w.cluster === cluster && w.node === node) {
+      front(w.win);
+      flashWin(w.win);
+      if (w.attach && attachDone(w.attach)) reattachLog(w); // dead stream → fresh
+      return;
+    }
   }
   recipe = recipe || (n.containers[0] ? n.containers[0].recipe : "");
   const win = makeWin({
@@ -798,6 +833,7 @@ async function openLog(cluster, node, recipe) {
       <select title="container on ${esc(node)}"></select>
       <label><input type="checkbox" class="logpause"> pause</label>
       <button class="abtn logcopy">copy</button>
+      <button class="abtn logrefresh" title="re-attach (fresh history + stream)">↻</button>
       <span class="lognote"></span>
     </div><pre class="logpre"></pre>`,
     extraCls: "logwin",
@@ -811,6 +847,7 @@ async function openLog(cluster, node, recipe) {
   win.body.querySelector(".logcopy").onclick = async () => {
     try { await navigator.clipboard.writeText(w.pre.textContent); } catch {}
   };
+  win.body.querySelector(".logrefresh").onclick = () => { if (w.attach) reattachLog(w); };
   const pauseCb = win.body.querySelector(".logpause");
   pauseCb.onchange = () => {
     w.paused = pauseCb.checked;
@@ -855,6 +892,7 @@ async function attachLog(w, recipe) {
   logWins.set(post.attach_id, w);
   const note = w.win.querySelector(".lognote");
   note.textContent = "";
+  note.classList.remove("dead");
 }
 function detachLog(attachId, hide = true) {
   const w = logWins.get(attachId);
@@ -864,6 +902,14 @@ function detachLog(attachId, hide = true) {
   if (hide && w.win && w.win.isConnected) { w.win.style.display = "none"; renderDock(); }
 }
 
+// Re-attach an existing window: drop the old stream (window stays) and
+// start a fresh one (new 200-line history + follow).
+async function reattachLog(w) {
+  const recipe = w.recipe;
+  if (w.attach) detachLog(w.attach, false);
+  await attachLog(w, recipe);
+}
+
 // --- raw content windows ---------------------------------------------------------
 // One window, a kind selector: config / effective / recipe:<name>. Content is
 // verbatim from the server (effective is pretty JSON); copy grabs the text.
@@ -1102,7 +1148,7 @@ async function boot() {
     if (res.ok) {
       const d = await res.json();
       state.recipes = (d.recipes || []).sort((a, b) => a.name.localeCompare(b.name));
-      if (state.snap) onState(state.snap); // re-render with recipes
+      if (state.snap) onState(state.snap, true); // force: sameSnapshot would skip the identical fleet
       if (state.onRecipesReady) { const f = state.onRecipesReady; delete state.onRecipesReady; f(); }
     }
   } catch {}

+ 24 - 0
src/web/ui.playwright.mjs

@@ -197,7 +197,31 @@ ok(
   (await page.locator(".logwin").count()) === 0,
 );
 
+// --- 11b. dead stream: 'stream ended' note + second click re-attaches -----------
+// (fake fleet: the SSH stream to the refused port dies instantly, so the full
+// dead-stream lifecycle is deterministic here)
+await page.evaluate(() => openLog("fake", "n0", "deepseek-v4-flash"));
+await page.waitForSelector(".logwin", { timeout: 10000 });
+await page.waitForFunction(
+  () => (document.querySelector(".logwin .lognote").textContent || "").includes("stream ended"),
+  { timeout: 8000 },
+);
+ok("dead stream shows the 'stream ended' note", true);
+// (dispatchEvent — the open log window overlaps the state window and would
+// swallow a physical click at the chip's coordinates)
+await page.locator("#clusters .chip").nth(0).dispatchEvent("click"); // back to cluster 'fake' (check 4 left it on 'fake2')
+await page.locator("#clusters .node").first().dispatchEvent("click");
+const flashed = await page.evaluate(() => document.querySelector(".logwin").classList.contains("flash"));
+ok(
+  "second click gives feedback (flash) without a duplicate window",
+  flashed && (await page.locator(".logwin").count()) === 1,
+);
+
 // --- 12. node click: log window with no containers ---------------------------------
+for (const btn of await page.locator(".logwin .wbtn[data-act=close]").all()) {
+  await btn.click(); // clear any windows left by earlier checks
+}
+await page.locator("#clusters .chip").nth(1).click(); // fake2 / n1
 await page.click("#clusters .node");
 ok(
   "node click opens a log window (no containers note)",