From a8b83ca63a4af8fb40328f28d8a29075494ecab9 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sat, 14 Nov 2026 22:41:29 -0800 Subject: [PATCH] fix(aws): follow checks the launch slot under the same lock it installs in The work-folder registry is a pure Slots whose launch, follow and clean-up transitions each run under one lock acquisition, with tests; HeadObject errors name the action as s3:GetObject (HeadObject). Co-Authored-By: Claude Opus 5.5 --- .../src/commands/environment_commands.rs | 194 ++++++++++++------ .../commands/environment_commands/tests.rs | 61 ++++++ 2 files changed, 195 insertions(+), 60 deletions(-) diff --git a/app/src-tauri/src/commands/environment_commands.rs b/app/src-tauri/src/commands/environment_commands.rs index 5833413..b071f57 100644 --- a/app/src-tauri/src/commands/environment_commands.rs +++ b/app/src-tauri/src/commands/environment_commands.rs @@ -573,21 +573,113 @@ pub(crate) async fn follow_and_report( // --------------------------------------------------------------------------- // Plumbing -/// What this process is doing with a work folder. -enum Slot { +/// What this process is doing with a work folder. `T` is the follower +/// task's handle (generic so the registry is unit-testable). +pub(crate) enum Slot { /// A launch is between its checks and its start; nothing else may /// materialize, start or follow the folder. Launching, /// One task follows the folder's run under `op_id`. - Following { - op_id: String, - task: tauri::async_runtime::JoinHandle<()>, - }, + Following { op_id: String, task: T }, } -static SLOTS: OnceLock>> = OnceLock::new(); +/// Why a folder's slot refused. +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum SlotBusy { + Launching, + Following, +} -fn slots() -> std::sync::MutexGuard<'static, HashMap> { +impl SlotBusy { + fn error(&self, label: &str) -> AwsError { + other(match self { + SlotBusy::Launching => format!("A deployment is starting for {label}; wait for it"), + SlotBusy::Following => format!( + "The run for {label} is still being followed; wait for it to finish" + ), + }) + } +} + +/// Per work folder: a launch in progress or the one follower. Every +/// check-and-change happens in one method call, i.e. under one acquisition +/// of the registry's lock. +pub(crate) struct Slots(HashMap>); + +impl Default for Slots { + fn default() -> Self { + Slots(HashMap::new()) + } +} + +impl Slots { + /// Claim `dir` for a launch: refused if anything holds it. + pub(crate) fn begin_launch(&mut self, dir: &Path) -> Result<(), SlotBusy> { + match self.0.get(dir) { + Some(Slot::Launching) => Err(SlotBusy::Launching), + Some(Slot::Following { .. }) => Err(SlotBusy::Following), + None => { + self.0.insert(dir.to_path_buf(), Slot::Launching); + Ok(()) + } + } + } + + /// A launch is over without a follower: clear `Launching`, never a + /// follower's slot. + pub(crate) fn end_launch(&mut self, dir: &Path) { + if matches!(self.0.get(dir), Some(Slot::Launching)) { + self.0.remove(dir); + } + } + + /// Install a follower made by `spawn`. From a launch (`from_launch`, the + /// caller holds `Launching`) it replaces the launch's slot; from a + /// follow it is refused while a launch holds the folder, and otherwise + /// replaces any earlier follower, whose handle is returned to abort. + /// `spawn` runs only once the check has passed, under the same lock. + pub(crate) fn install_follower( + &mut self, + dir: &Path, + op_id: &str, + from_launch: bool, + spawn: impl FnOnce() -> T, + ) -> Result, SlotBusy> { + if !from_launch && matches!(self.0.get(dir), Some(Slot::Launching)) { + return Err(SlotBusy::Launching); + } + let task = spawn(); + let old = self.0.insert( + dir.to_path_buf(), + Slot::Following { + op_id: op_id.to_string(), + task, + }, + ); + Ok(match old { + Some(Slot::Following { task, .. }) => Some(task), + _ => None, + }) + } + + /// A follower ended: remove the slot only if it is still that one's. + pub(crate) fn finish_follower(&mut self, dir: &Path, op_id: &str) { + if matches!(self.0.get(dir), Some(Slot::Following { op_id: id, .. }) if id == op_id) { + self.0.remove(dir); + } + } + + #[cfg(test)] + pub(crate) fn is_free(&self, dir: &Path) -> bool { + !self.0.contains_key(dir) + } +} + +type FollowerTask = tauri::async_runtime::JoinHandle<()>; + +static SLOTS: OnceLock>> = OnceLock::new(); + +fn slots() -> std::sync::MutexGuard<'static, Slots> { SLOTS .get_or_init(Default::default) .lock() @@ -600,28 +692,14 @@ struct LaunchGuard(PathBuf); impl LaunchGuard { fn take(dir: &Path, label: &str) -> Result { - let mut map = slots(); - match map.get(dir) { - Some(Slot::Launching) => Err(other(format!( - "A deployment is starting for {label}; wait for it" - ))), - Some(Slot::Following { .. }) => Err(other(format!( - "The run for {label} is still being followed; wait for it to finish" - ))), - None => { - map.insert(dir.to_path_buf(), Slot::Launching); - Ok(LaunchGuard(dir.to_path_buf())) - } - } + slots().begin_launch(dir).map_err(|b| b.error(label))?; + Ok(LaunchGuard(dir.to_path_buf())) } } impl Drop for LaunchGuard { fn drop(&mut self) { - let mut map = slots(); - if matches!(map.get(&self.0), Some(Slot::Launching)) { - map.remove(&self.0); - } + slots().end_launch(&self.0); } } @@ -736,7 +814,7 @@ async fn boundary_state_exists(ctx: &Ctx) -> Result, AwsError> { Err(e) => match e.raw_response().map(|r| r.status().as_u16()) { Some(404) => Ok(Some(false)), Some(403) => Ok(None), - _ => Err(crate::aws::classify_sdk_error("s3:GetObject", &e)), + _ => Err(crate::aws::classify_sdk_error("s3:GetObject (HeadObject)", &e)), }, } } @@ -764,9 +842,17 @@ fn emit_progress(app: &AppHandle, p: &DeployProgress) { /// Start the folder's follower under a new op id, replacing (aborting) any /// follower this process already has for it, so a re-follow (a reloaded /// window) replays the whole log. The follower emits nothing until the -/// returned [`Gate`] is opened. The caller has checked the folder isn't -/// launching, or holds its [`LaunchGuard`]. -fn install_follower(app: AppHandle, wd: Workdir, key: String, can_cancel: bool) -> (String, Gate) { +/// returned [`Gate`] is opened. A follow (`from_launch == false`) is refused +/// while a launch holds the folder; the check, the spawn and the slot change +/// happen under one lock acquisition, so no launch can slip in between. +fn install_follower( + app: AppHandle, + wd: Workdir, + key: String, + label: &str, + can_cancel: bool, + from_launch: bool, +) -> Result<(String, Gate), AwsError> { let op_id = uuid::Uuid::new_v4().to_string(); let (gate, rx) = Gate::new(); let base = DeployProgress { @@ -778,28 +864,22 @@ fn install_follower(app: AppHandle, wd: Workdir, key: String, can_cancel: bool) result: None, }; let dir = wd.dir.clone(); - let mut map = slots(); - // Spawned under the lock, so the task's own clean-up (which takes it) - // can't run before its slot is in place. let id = op_id.clone(); - let task = tauri::async_runtime::spawn(async move { - let dir = wd.dir.clone(); - follow_and_report(rx, wd, base, FOLLOW_POLL, |p| emit_progress(&app, p)).await; - let mut map = slots(); - if matches!(map.get(&dir), Some(Slot::Following { op_id, .. }) if *op_id == id) { - map.remove(&dir); - } - }); - if let Some(Slot::Following { task: old, .. }) = map.insert( - dir, - Slot::Following { - op_id: op_id.clone(), - task, - }, - ) { + // The task's own clean-up takes the same lock, so it can't run before + // its slot is in place. + let old = slots() + .install_follower(&dir, &op_id, from_launch, move || { + tauri::async_runtime::spawn(async move { + let dir = wd.dir.clone(); + follow_and_report(rx, wd, base, FOLLOW_POLL, |p| emit_progress(&app, p)).await; + slots().finish_follower(&dir, &id); + }) + }) + .map_err(|b| b.error(label))?; + if let Some(old) = old { old.abort(); } - (op_id, gate) + Ok((op_id, gate)) } /// Ensure OpenTofu and the state bucket, write the work folder and start a @@ -844,7 +924,7 @@ async fn launch( .map_err(|e| other(format!("The deploy could not be started: {e}")))? .map_err(|e| start_error(e, &label))?; let can_cancel = cfg!(unix) && started.pid.is_some(); - Ok(install_follower(app, wd, state_key(d), can_cancel)) + install_follower(app, wd, state_key(d), &label, can_cancel, true) } /// Open a launch's gate and hand back its op id: the last thing a command @@ -998,7 +1078,8 @@ pub async fn unlock_aws_state( Err(e) if e.raw_response().map(|r| r.status().as_u16()) == Some(404) => { return Err(other("The state lock is already gone; detect again")); } - Err(e) => return Err(crate::aws::classify_sdk_error("s3:GetObject", &e)), + // HeadObject is authorised by s3:GetObject. + Err(e) => return Err(crate::aws::classify_sdk_error("s3:GetObject (HeadObject)", &e)), }; let etag = head .e_tag() @@ -1056,17 +1137,10 @@ pub async fn follow_aws_deployment( key.label() ))); } - if matches!(slots().get(&wd.dir), Some(Slot::Launching)) { - return Err(other(format!( - "A deployment is starting for {}; wait for it", - key.label() - ))); - } let can_cancel = tofu::is_running(&wd) && cancellable(&wd); - // A launch can't start here between the check and the install: it - // needs a runner-free folder or an empty slot, and either way the - // follower then follows that run from byte 0. - let (op_id, gate) = install_follower(app, wd, state_key, can_cancel); + // Refused inside install_follower, under the registry lock, while a + // launch holds the folder. + let (op_id, gate) = install_follower(app, wd, state_key, &key.label(), can_cancel, false)?; gate.open(); Ok(op_id) } diff --git a/app/src-tauri/src/commands/environment_commands/tests.rs b/app/src-tauri/src/commands/environment_commands/tests.rs index 031e464..63ab0f3 100644 --- a/app/src-tauri/src/commands/environment_commands/tests.rs +++ b/app/src-tauri/src/commands/environment_commands/tests.rs @@ -498,3 +498,64 @@ fn an_existing_boundary_without_state_here_is_refused() { assert!(boundary_refusal(&Knowable::No, Some(false)).is_ok()); assert!(boundary_refusal(&Knowable::Unknown, Some(false)).is_ok()); } + +#[test] +fn a_follow_is_refused_while_a_launch_holds_the_folder() { + let dir = Path::new("/w/env/dev"); + let mut slots: Slots = Slots::default(); + slots.begin_launch(dir).unwrap(); + let mut spawned = false; + let r = slots.install_follower(dir, "f1", false, || { + spawned = true; + 1 + }); + assert_eq!(r.unwrap_err(), SlotBusy::Launching); + assert!(!spawned, "nothing is spawned for a refused follow"); + // The launch's own follower replaces its Launching slot. + assert_eq!(slots.install_follower(dir, "l1", true, || 2).unwrap(), None); + slots.end_launch(dir); // the guard dropping afterwards + assert!(!slots.is_free(dir), "end_launch must not clear a follower"); +} + +#[test] +fn a_launch_is_refused_while_a_follower_or_launch_holds_the_folder() { + let dir = Path::new("/w/env/dev"); + let mut slots: Slots = Slots::default(); + slots.install_follower(dir, "f1", false, || 1).unwrap(); + assert_eq!(slots.begin_launch(dir).unwrap_err(), SlotBusy::Following); + slots.finish_follower(dir, "f1"); + slots.begin_launch(dir).unwrap(); + assert_eq!(slots.begin_launch(dir).unwrap_err(), SlotBusy::Launching); + slots.end_launch(dir); + assert!(slots.is_free(dir)); + // Other folders are independent. + slots.begin_launch(dir).unwrap(); + slots.begin_launch(Path::new("/w/env/prod")).unwrap(); +} + +#[test] +fn a_finishing_follower_only_removes_its_own_slot() { + let dir = Path::new("/w/env/dev"); + let mut slots: Slots = Slots::default(); + slots.install_follower(dir, "old", false, || 1).unwrap(); + // A re-follow replaces it and hands back the old task to abort. + assert_eq!(slots.install_follower(dir, "new", false, || 2).unwrap(), Some(1)); + slots.finish_follower(dir, "old"); + assert!(!slots.is_free(dir), "the old follower must not remove the new one's slot"); + slots.finish_follower(dir, "new"); + assert!(slots.is_free(dir)); + // Nor a launch's slot. + slots.begin_launch(dir).unwrap(); + slots.finish_follower(dir, "new"); + assert_eq!(slots.begin_launch(dir).unwrap_err(), SlotBusy::Launching); +} + +#[test] +fn ending_a_launch_never_clears_a_following_slot() { + let dir = Path::new("/w/env/dev"); + let mut slots: Slots = Slots::default(); + slots.install_follower(dir, "f", false, || 1).unwrap(); + slots.end_launch(dir); + assert_eq!(slots.begin_launch(dir).unwrap_err(), SlotBusy::Following); + slots.end_launch(Path::new("/w/env/none")); // nothing there: no-op +}