fix(viewer): atomic find-or-reserve so an in-flight window is never read as stale

A second click while the first window was still being built removed its
registry entry, giving a broken window, a duplicate and a cap bypass. The
registry now records when a window is built; `reserve` dedupes, prunes only
built entries whose window is gone (any state, so a leak cannot hold a cap
slot), and enforces the cap in one critical section. Choosing a file already
open elsewhere focuses that window and closes the chooser instead of
resolving a second entry. The not-running sentence names the real action.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

app/src-tauri/src/commands/file_viewer_commands.rs

@@ -12,7 +12,9 @@ use crate::commands::file_commands::{
 };
 use crate::file_viewer::is_viewer_label;
 use crate::file_viewer::poll::{poll_file, ViewerPoll};
-use crate::file_viewer::registry::{Location, ViewerRegistry, ViewerTarget, ViewerTargetState};
+use crate::file_viewer::registry::{
+    Choice, Location, Reservation, ViewerRegistry, ViewerTarget, ViewerTargetState,
+};
 use crate::file_viewer::resolve::{candidate_paths, probe_candidates};
 use crate::file_viewer::window::open_viewer_window;
 use crate::file_viewer::write::{sha256_hex, write_file, MAX_WRITE_BYTES};
@@ -110,17 +112,31 @@ fn project_of(state: &AppState, project_id: &str) -> Result<Project, String> {
         .ok_or_else(|| "This project no longer exists.".to_string())
 }
 
-async fn running_container_of(project: &Project) -> Result<String, String> {
+/// `action` completes "Start the project before …", e.g. "saving this file".
+async fn running_container_of(project: &Project, action: &str) -> Result<String, String> {
     let container_id = project.container_id.clone().ok_or_else(|| {
-        "Start the project before opening files — they live in its container.".to_string()
+        format!("Start the project before {} — files live in its container.", action)
     })?;
-    require_running(&container_id, "opening files").await?;
+    require_running(&container_id, action).await?;
     Ok(container_id)
 }
 
 /// The container of the project a viewer window belongs to, if it is running.
-async fn running_container_for(state: &AppState, target: &ViewerTarget) -> Result<String, String> {
+async fn running_container_for(
-    running_container_of(&project_of(state, &target.project_id)?).await
+    state: &AppState,
+    target: &ViewerTarget,
+    action: &str,
+) -> Result<String, String> {
+    running_container_of(&project_of(state, &target.project_id)?, action).await
+}
+
+/// Raises an existing viewer window and moves it to `location`.
+fn focus_viewer(app: &AppHandle, label: &str, location: Location) {
+    if let Some(existing) = app.get_webview_window(label) {
+        let _ = existing.unminimize();
+        let _ = existing.set_focus();
+        let _ = app.emit_to(label, GOTO_EVENT, location);
+    }
 }
 
 // Nine parameters are fixed by the IPC contract (P10); four injected by Tauri.
@@ -139,7 +155,7 @@ pub async fn open_file_viewer(
 ) -> Result<(), String> {
     require_main(window.label())?;
     let project = project_of(&state, &project_id)?;
-    let container_id = running_container_of(&project).await?;
+    let container_id = running_container_of(&project, "opening files").await?;
 
     let mounts: Vec<String> = project.paths.iter().map(|p| p.mount_name.clone()).collect();
     let candidates = candidate_paths(&path, &mounts)?;
@@ -148,35 +164,35 @@ pub async fn open_file_viewer(
 
     let target_state = match matches.len() {
         0 => ViewerTargetState::NotFound { tried: candidates },
-        1 => {
+        1 => ViewerTargetState::Resolved { container_path: matches[0].clone() },
-            let container_path = matches[0].clone();
-            if let Some(label) = registry.find_open(&project_id, &container_path) {
-                if let Some(existing) = app.get_webview_window(&label) {
-                    let _ = existing.unminimize();
-                    let _ = existing.set_focus();
-                    let _ = app.emit_to(label.as_str(), GOTO_EVENT, initial);
-                    return Ok(());
-                }
-                // Registered but windowless: a stale entry. Drop it and open afresh.
-                registry.remove(&label);
-            }
-            ViewerTargetState::Resolved { container_path }
-        }
         _ => ViewerTargetState::Choose { candidates: matches },
     };
 
     let title = window_title(&path, &project.name);
-    let label = registry.reserve(ViewerTarget {
+    let target = ViewerTarget {
         project_id,
         project_name: project.name.clone(),
         raw_path: path,
         state: target_state,
-        initial,
+        initial: initial.clone(),
-    })?;
+    };
+    // Dedupe, stale pruning and the cap are one registry call, so a second click
+    // while the first window is still being built finds it rather than reading
+    // its not-yet-existing window as stale.
+    let label = match registry.reserve(target, |l| app.get_webview_window(l).is_some())? {
+        Reservation::Reserved(label) => label,
+        // Still being built: it opens at its own location in a moment.
+        Reservation::Existing { built: false, .. } => return Ok(()),
+        Reservation::Existing { label, built: true } => {
+            focus_viewer(&app, &label, initial);
+            return Ok(());
+        }
+    };
     if let Err(e) = open_viewer_window(&app, &label, &title) {
         registry.remove(&label);
         return Err(e);
     }
+    registry.mark_built(&label);
     Ok(())
 }
 
@@ -203,9 +219,19 @@ pub async fn viewer_choose_file(
             .ok_or_else(|| "That choice is no longer available.".to_string())?,
         _ => return Err("This window is not choosing a file.".into()),
     };
-    let updated =
+    let app = window.app_handle();
-        registry.set_state(&label, ViewerTargetState::Resolved { container_path: chosen })?;
+    match registry.choose(&label, chosen, |l| app.get_webview_window(l).is_some())? {
-    Ok(viewer_state_of(&label, updated))
+        Choice::Resolved(updated) => Ok(viewer_state_of(&label, updated)),
+        // Another window already has this file. This window was only ever a
+        // chooser, so hand over to that one and close this one, as a second
+        // click on the same path would have. The error is what this window
+        // shows if the destroy fails.
+        Choice::AlreadyOpen { label: other, .. } => {
+            focus_viewer(app, &other, target.initial);
+            let _ = window.destroy();
+            Err("This file is already open in another window.".into())
+        }
+    }
 }
 
 #[tauri::command]
@@ -217,7 +243,7 @@ pub async fn viewer_read_file(
 ) -> Result<ViewerFile, String> {
     let (_label, target) = own_target(&window, &registry)?;
     let path = resolved_path(&target)?;
-    let container_id = running_container_for(&state, &target).await?;
+    let container_id = running_container_for(&state, &target, "opening files").await?;
     let cap = max_bytes.clamp(1, MAX_READ_BYTES);
     let fetched = fetch_container_file(&container_id, &path, cap).await?;
     let (editable, readonly_reason) = match validate_container_write_path("File", &path) {
@@ -242,7 +268,7 @@ pub async fn viewer_poll_file(
 ) -> Result<ViewerPoll, String> {
     let (_label, target) = own_target(&window, &registry)?;
     let path = resolved_path(&target)?;
-    let container_id = running_container_for(&state, &target).await?;
+    let container_id = running_container_for(&state, &target, "checking this file for changes").await?;
     poll_file(&container_id, &path).await
 }
 
@@ -264,7 +290,7 @@ pub async fn viewer_write_file(
     let bytes = BASE64
         .decode(contents_base64.as_bytes())
         .map_err(|_| "The editor sent malformed content.".to_string())?;
-    let container_id = running_container_for(&state, &target).await?;
+    let container_id = running_container_for(&state, &target, "saving this file").await?;
     write_file(&container_id, &state.exec_manager, &path, &bytes, &base_hash).await
 }
 

app/src-tauri/src/file_viewer/registry.rs

@@ -36,19 +36,87 @@ pub struct ViewerTarget {
     pub initial: Location,
 }
 
+/// What [`ViewerRegistry::reserve`] decided.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum Reservation {
+    /// A window is already registered on this file. `built` is false while that
+    /// window is still being created: it has no `WebviewWindow` to focus yet, and
+    /// it will open at its own location, so the caller should simply return.
+    Existing { label: String, built: bool },
+    /// A new label, registered and counted against the cap; build its window,
+    /// then call [`ViewerRegistry::mark_built`] (or `remove` if building failed).
+    Reserved(String),
+}
+
+/// What [`ViewerRegistry::choose`] decided.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum Choice {
+    /// The caller's entry now points at the chosen file.
+    Resolved(ViewerTarget),
+    /// Another window already has that file; the caller's entry is unchanged.
+    AlreadyOpen { label: String, built: bool },
+}
+
+#[derive(Clone, Debug)]
+struct Entry {
+    target: ViewerTarget,
+    /// Set once the window's `build()` has returned. Until then the label has no
+    /// window by design, so "registered but windowless" means "being built", not
+    /// "stale" — only built entries are ever pruned.
+    built: bool,
+}
+
 #[derive(Default)]
 pub struct ViewerRegistry {
-    entries: Mutex<HashMap<String, ViewerTarget>>,
+    entries: Mutex<HashMap<String, Entry>>,
     next: AtomicU64,
 }
 
+fn same_file(t: &ViewerTarget, project_id: &str, container_path: &str) -> bool {
+    t.project_id == project_id
+        && matches!(&t.state, ViewerTargetState::Resolved { container_path: p } if p == container_path)
+}
+
+fn open_on(
+    entries: &HashMap<String, Entry>,
+    project_id: &str,
+    container_path: &str,
+    except: Option<&str>,
+) -> Option<(String, bool)> {
+    entries
+        .iter()
+        .find(|(label, e)| Some(label.as_str()) != except && same_file(&e.target, project_id, container_path))
+        .map(|(label, e)| (label.clone(), e.built))
+}
+
+/// Drops built entries whose window is gone, whatever their state. `Destroyed`
+/// normally removes an entry; this is the backstop for one it missed, so a leak
+/// can never hold a cap slot for good.
+fn prune(entries: &mut HashMap<String, Entry>, is_live: &dyn Fn(&str) -> bool) {
+    entries.retain(|label, e| !e.built || is_live(label));
+}
+
 impl ViewerRegistry {
-    fn lock(&self) -> std::sync::MutexGuard<'_, HashMap<String, ViewerTarget>> {
+    fn lock(&self) -> std::sync::MutexGuard<'_, HashMap<String, Entry>> {
         self.entries.lock().unwrap_or_else(|e| e.into_inner())
     }
 
-    pub fn reserve(&self, target: ViewerTarget) -> Result<String, String> {
+    /// Finds the window already open on a resolved target, or reserves a label,
+    /// in one critical section, after pruning built entries `is_live` says are
+    /// gone. `is_live` runs under the registry lock and must not call back into
+    /// the registry.
+    pub fn reserve(
+        &self,
+        target: ViewerTarget,
+        is_live: impl Fn(&str) -> bool,
+    ) -> Result<Reservation, String> {
         let mut entries = self.lock();
+        prune(&mut entries, &is_live);
+        if let ViewerTargetState::Resolved { container_path } = &target.state {
+            if let Some((label, built)) = open_on(&entries, &target.project_id, container_path, None) {
+                return Ok(Reservation::Existing { label, built });
+            }
+        }
         if entries.len() >= MAX_VIEWER_WINDOWS {
             return Err(format!(
                 "{} file windows are already open — close one before opening another.",
@@ -57,12 +125,45 @@ impl ViewerRegistry {
         }
         let n = self.next.fetch_add(1, Ordering::SeqCst) + 1;
         let label = format!("{}{}", VIEWER_LABEL_PREFIX, n);
-        entries.insert(label.clone(), target);
+        entries.insert(label.clone(), Entry { target, built: false });
-        Ok(label)
+        Ok(Reservation::Reserved(label))
+    }
+
+    /// Records that `label`'s window exists. A no-op if it was already removed
+    /// (a window destroyed the moment it appeared).
+    pub fn mark_built(&self, label: &str) {
+        if let Some(e) = self.lock().get_mut(label) {
+            e.built = true;
+        }
+    }
+
+    /// Points `label`'s entry at `container_path`, unless another window already
+    /// has that file open — then the entry is left alone, so no two entries are
+    /// ever resolved to the same file.
+    pub fn choose(
+        &self,
+        label: &str,
+        container_path: String,
+        is_live: impl Fn(&str) -> bool,
+    ) -> Result<Choice, String> {
+        let mut entries = self.lock();
+        prune(&mut entries, &is_live);
+        let project_id = entries
+            .get(label)
+            .ok_or_else(|| "This file window is no longer registered.".to_string())?
+            .target
+            .project_id
+            .clone();
+        if let Some((other, built)) = open_on(&entries, &project_id, &container_path, Some(label)) {
+            return Ok(Choice::AlreadyOpen { label: other, built });
+        }
+        let entry = entries.get_mut(label).expect("checked above under the same lock");
+        entry.target.state = ViewerTargetState::Resolved { container_path };
+        Ok(Choice::Resolved(entry.target.clone()))
     }
 
     pub fn get(&self, label: &str) -> Option<ViewerTarget> {
-        self.lock().get(label).cloned()
+        self.lock().get(label).map(|e| e.target.clone())
     }
 
     pub fn set_state(&self, label: &str, state: ViewerTargetState) -> Result<ViewerTarget, String> {
@@ -70,8 +171,8 @@ impl ViewerRegistry {
         let entry = entries
             .get_mut(label)
             .ok_or_else(|| "This file window is no longer registered.".to_string())?;
-        entry.state = state;
+        entry.target.state = state;
-        Ok(entry.clone())
+        Ok(entry.target.clone())
     }
 
     pub fn remove(&self, label: &str) {
@@ -79,13 +180,7 @@ impl ViewerRegistry {
     }
 
     pub fn find_open(&self, project_id: &str, container_path: &str) -> Option<String> {
-        self.lock()
+        open_on(&self.lock(), project_id, container_path, None).map(|(label, _)| label)
-            .iter()
-            .find(|(_, t)| {
-                t.project_id == project_id
-                    && matches!(&t.state, ViewerTargetState::Resolved { container_path: p } if p == container_path)
-            })
-            .map(|(label, _)| label.clone())
     }
 
     pub fn len(&self) -> usize {
@@ -111,15 +206,34 @@ mod tests {
         }
     }
 
+    fn all_live(_: &str) -> bool {
+        true
+    }
+
+    /// Reserves a label that must be new.
+    fn fresh(r: &ViewerRegistry, t: ViewerTarget) -> String {
+        match r.reserve(t, all_live).unwrap() {
+            Reservation::Reserved(label) => label,
+            other => panic!("expected a new label, got {:?}", other),
+        }
+    }
+
+    fn choosing(project: &str, candidates: &[&str]) -> ViewerTarget {
+        ViewerTarget {
+            state: ViewerTargetState::Choose { candidates: candidates.iter().map(|c| c.to_string()).collect() },
+            ..target(project, "a")
+        }
+    }
+
     #[test]
     fn labels_are_sequential_and_never_reused() {
         let r = ViewerRegistry::default();
-        let a = r.reserve(target("p", "/workspace/a")).unwrap();
+        let a = fresh(&r, target("p", "/workspace/a"));
-        let b = r.reserve(target("p", "/workspace/b")).unwrap();
+        let b = fresh(&r, target("p", "/workspace/b"));
         assert_eq!(a, "file-viewer-1");
         assert_eq!(b, "file-viewer-2");
         r.remove(&a);
-        let c = r.reserve(target("p", "/workspace/c")).unwrap();
+        let c = fresh(&r, target("p", "/workspace/c"));
         assert_eq!(c, "file-viewer-3");
         assert_eq!(r.len(), 2);
     }
@@ -128,9 +242,9 @@ mod tests {
     fn the_cap_refuses_the_twenty_first_window() {
         let r = ViewerRegistry::default();
         for i in 0..MAX_VIEWER_WINDOWS {
-            r.reserve(target("p", &format!("/workspace/{}", i))).unwrap();
+            fresh(&r, target("p", &format!("/workspace/{}", i)));
         }
-        let err = r.reserve(target("p", "/workspace/one-more")).unwrap_err();
+        let err = r.reserve(target("p", "/workspace/one-more"), all_live).unwrap_err();
         assert!(err.contains("20"), "{}", err);
         assert_eq!(r.len(), MAX_VIEWER_WINDOWS);
     }
@@ -138,7 +252,7 @@ mod tests {
     #[test]
     fn an_open_resolved_file_is_found_by_project_and_path() {
         let r = ViewerRegistry::default();
-        let label = r.reserve(target("p", "/workspace/a")).unwrap();
+        let label = fresh(&r, target("p", "/workspace/a"));
         assert_eq!(r.find_open("p", "/workspace/a"), Some(label.clone()));
         assert_eq!(r.find_open("other", "/workspace/a"), None);
         // A window still choosing is not "open on" any path.
@@ -159,4 +273,107 @@ mod tests {
         let s = serde_json::to_string(&ViewerTargetState::NotFound { tried: vec!["/x".into()] }).unwrap();
         assert_eq!(s, r#"{"kind":"not_found","tried":["/x"]}"#);
     }
+
+    /// I1: a second click while the first window is still being built must find
+    /// that window, not read it as stale and reserve a second one.
+    #[test]
+    fn a_window_being_built_is_found_not_replaced() {
+        let r = ViewerRegistry::default();
+        let a = fresh(&r, target("p", "/workspace/a"));
+        // No window exists yet for `a`: `is_live` says so, and it must not matter.
+        let second = r.reserve(target("p", "/workspace/a"), |_| false).unwrap();
+        assert_eq!(second, Reservation::Existing { label: a.clone(), built: false });
+        assert!(r.get(&a).is_some());
+        assert_eq!(r.len(), 1);
+
+        r.mark_built(&a);
+        let third = r.reserve(target("p", "/workspace/a"), all_live).unwrap();
+        assert_eq!(third, Reservation::Existing { label: a, built: true });
+        assert_eq!(r.len(), 1);
+    }
+
+    /// A built entry whose window is gone is stale: pruned, and the file reopens.
+    #[test]
+    fn a_built_entry_without_a_window_is_pruned_and_the_file_reopens() {
+        let r = ViewerRegistry::default();
+        let a = fresh(&r, target("p", "/workspace/a"));
+        r.mark_built(&a);
+        let again = r.reserve(target("p", "/workspace/a"), |_| false).unwrap();
+        assert_eq!(again, Reservation::Reserved("file-viewer-2".into()));
+        assert_eq!(r.get(&a), None);
+        assert_eq!(r.len(), 1);
+    }
+
+    /// M2: a leaked entry of any state cannot hold a cap slot once built and gone,
+    /// and an entry still being built always keeps its slot.
+    #[test]
+    fn leaked_entries_of_every_state_free_their_cap_slot() {
+        let r = ViewerRegistry::default();
+        let mut labels = Vec::new();
+        for i in 0..MAX_VIEWER_WINDOWS {
+            let t = match i % 3 {
+                0 => target("p", &format!("/workspace/{}", i)),
+                1 => choosing("p", &["/workspace/x", "/workspace/y"]),
+                _ => ViewerTarget { state: ViewerTargetState::NotFound { tried: vec![] }, ..target("p", "z") },
+            };
+            labels.push(fresh(&r, t));
+        }
+        // All still being built: none may be pruned, so the cap holds.
+        assert!(r.reserve(target("p", "/workspace/new"), |_| false).is_err());
+        for l in &labels {
+            r.mark_built(l);
+        }
+        // Built, and one of each state has lost its window.
+        let dead = [labels[0].clone(), labels[1].clone(), labels[2].clone()];
+        let live = |l: &str| !dead.iter().any(|d| d == l);
+        assert!(matches!(r.reserve(target("p", "/workspace/new"), live), Ok(Reservation::Reserved(_))));
+        assert_eq!(r.len(), MAX_VIEWER_WINDOWS - 2);
+        for d in &dead {
+            assert_eq!(r.get(d), None);
+        }
+    }
+
+    #[test]
+    fn mark_built_on_a_removed_label_is_a_no_op() {
+        let r = ViewerRegistry::default();
+        let a = fresh(&r, target("p", "/workspace/a"));
+        r.remove(&a);
+        r.mark_built(&a);
+        assert_eq!(r.get(&a), None);
+    }
+
+    /// M5: choosing a file another window already has leaves the chooser alone,
+    /// so two entries are never resolved to the same file.
+    #[test]
+    fn choosing_a_file_open_elsewhere_does_not_resolve_a_second_entry() {
+        let r = ViewerRegistry::default();
+        let open = fresh(&r, target("p", "/workspace/x"));
+        r.mark_built(&open);
+        let chooser = fresh(&r, choosing("p", &["/workspace/x", "/workspace/y"]));
+        r.mark_built(&chooser);
+
+        let c = r.choose(&chooser, "/workspace/x".into(), all_live).unwrap();
+        assert_eq!(c, Choice::AlreadyOpen { label: open.clone(), built: true });
+        assert!(matches!(r.get(&chooser).unwrap().state, ViewerTargetState::Choose { .. }));
+
+        match r.choose(&chooser, "/workspace/y".into(), all_live).unwrap() {
+            Choice::Resolved(t) => assert_eq!(t.state, ViewerTargetState::Resolved { container_path: "/workspace/y".into() }),
+            other => panic!("expected Resolved, got {:?}", other),
+        }
+        assert_eq!(r.find_open("p", "/workspace/y"), Some(chooser));
+    }
+
+    #[test]
+    fn choosing_the_same_path_in_another_project_is_not_a_duplicate() {
+        let r = ViewerRegistry::default();
+        fresh(&r, target("other", "/workspace/x"));
+        let chooser = fresh(&r, choosing("p", &["/workspace/x"]));
+        assert!(matches!(r.choose(&chooser, "/workspace/x".into(), all_live), Ok(Choice::Resolved(_))));
+    }
+
+    #[test]
+    fn choose_on_an_unknown_label_is_an_error() {
+        let r = ViewerRegistry::default();
+        assert!(r.choose("file-viewer-9", "/workspace/x".into(), all_live).is_err());
+    }
 }