From 804213a51796d27ab8c7c2d9f0f93095f000e478 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 22 Sep 2026 21:22:58 -0700 Subject: [PATCH] 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) --- .../src/commands/file_viewer_commands.rs | 86 ++++-- app/src-tauri/src/file_viewer/registry.rs | 259 ++++++++++++++++-- 2 files changed, 294 insertions(+), 51 deletions(-) diff --git a/app/src-tauri/src/commands/file_viewer_commands.rs b/app/src-tauri/src/commands/file_viewer_commands.rs index 388c4ce..f7ab869 100644 --- a/app/src-tauri/src/commands/file_viewer_commands.rs +++ b/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 { .ok_or_else(|| "This project no longer exists.".to_string()) } -async fn running_container_of(project: &Project) -> Result { +/// `action` completes "Start the project before …", e.g. "saving this file". +async fn running_container_of(project: &Project, action: &str) -> Result { 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 { - running_container_of(&project_of(state, &target.project_id)?).await +async fn running_container_for( + state: &AppState, + target: &ViewerTarget, + action: &str, +) -> Result { + 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 = 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 => { - 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 } - } + 1 => ViewerTargetState::Resolved { container_path: matches[0].clone() }, _ => 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 = - registry.set_state(&label, ViewerTargetState::Resolved { container_path: chosen })?; - Ok(viewer_state_of(&label, updated)) + let app = window.app_handle(); + match registry.choose(&label, chosen, |l| app.get_webview_window(l).is_some())? { + 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 { let (_label, target) = own_target(&window, ®istry)?; 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 { let (_label, target) = own_target(&window, ®istry)?; 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 } diff --git a/app/src-tauri/src/file_viewer/registry.rs b/app/src-tauri/src/file_viewer/registry.rs index 0f86e13..14adfb5 100644 --- a/app/src-tauri/src/file_viewer/registry.rs +++ b/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>, + entries: Mutex>, 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, + 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, is_live: &dyn Fn(&str) -> bool) { + entries.retain(|label, e| !e.built || is_live(label)); +} + impl ViewerRegistry { - fn lock(&self) -> std::sync::MutexGuard<'_, HashMap> { + fn lock(&self) -> std::sync::MutexGuard<'_, HashMap> { self.entries.lock().unwrap_or_else(|e| e.into_inner()) } - pub fn reserve(&self, target: ViewerTarget) -> Result { + /// 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 { 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); - Ok(label) + entries.insert(label.clone(), Entry { target, built: false }); + 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 { + 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 { - self.lock().get(label).cloned() + self.lock().get(label).map(|e| e.target.clone()) } pub fn set_state(&self, label: &str, state: ViewerTargetState) -> Result { @@ -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; - Ok(entry.clone()) + entry.target.state = state; + 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 { - self.lock() - .iter() - .find(|(_, t)| { - t.project_id == project_id - && matches!(&t.state, ViewerTargetState::Resolved { container_path: p } if p == container_path) - }) - .map(|(label, _)| label.clone()) + open_on(&self.lock(), project_id, container_path, None).map(|(label, _)| label) } 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 b = r.reserve(target("p", "/workspace/b")).unwrap(); + let a = fresh(&r, target("p", "/workspace/a")); + 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()); + } }