From d51b54774b5e242d4f6c8edc91a37cec4cc50ae8 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 27 Sep 2026 13:04:51 -0700 Subject: [PATCH] Marketplace: edit only the marketplace fields of a project (PR review #2) ProjectsStore gains update_marketplace_fields / update_all_marketplace_fields, which read-modify-write installs and opt-outs under the store's own lock. Install, uninstall, update, forget and set_global_item_disabled use them instead of writing back a whole Project read earlier, so a concurrent start's status/container_id change is no longer overwritten. Co-Authored-By: Claude Opus 5.5 --- .../src/commands/marketplace_commands.rs | 76 ++++++---- app/src-tauri/src/storage/projects_store.rs | 137 ++++++++++++++++++ 2 files changed, 181 insertions(+), 32 deletions(-) diff --git a/app/src-tauri/src/commands/marketplace_commands.rs b/app/src-tauri/src/commands/marketplace_commands.rs index bad3858..583dda1 100644 --- a/app/src-tauri/src/commands/marketplace_commands.rs +++ b/app/src-tauri/src/commands/marketplace_commands.rs @@ -761,16 +761,12 @@ pub async fn forget_marketplace_installs( .global_marketplace_installs .retain(|i| i.marketplace_id != marketplace_id); state.settings_store.update(settings)?; - for mut p in state.projects_store.list() { - let before = (p.marketplace_installs.len(), p.marketplace_disabled.len()); - p.marketplace_installs - .retain(|i| i.marketplace_id != marketplace_id); - p.marketplace_disabled - .retain(|r| r.marketplace_id != marketplace_id); - if (p.marketplace_installs.len(), p.marketplace_disabled.len()) != before { - state.projects_store.update(p)?; - } - } + state + .projects_store + .update_all_marketplace_fields(|installs, disabled| { + installs.retain(|i| i.marketplace_id != marketplace_id); + disabled.retain(|r| r.marketplace_id != marketplace_id); + })?; refresh_pins(&state).await; Ok(()) } @@ -807,9 +803,12 @@ pub async fn install_marketplace_item( state.settings_store.update(s)?; } InstallScope::Project { project_id } => { - let mut p = find_project(&state, &project_id)?; - ops::upsert_install(&mut p.marketplace_installs, inst); - state.projects_store.update(p)?; + state + .projects_store + .update_marketplace_fields(&project_id, |installs, _| { + ops::upsert_install(installs, inst); + Ok(()) + })?; } } refresh_pins(&state).await; @@ -830,19 +829,23 @@ pub async fn uninstall_marketplace_item( } state.settings_store.update(s)?; // An opt-out of an item that is no longer global means nothing. - for mut p in state.projects_store.list() { - if p.marketplace_disabled.contains(&item) { - ops::set_disabled(&mut p.marketplace_disabled, &item, false); - state.projects_store.update(p)?; - } - } + state + .projects_store + .update_all_marketplace_fields(|_, disabled| { + ops::set_disabled(disabled, &item, false) + })?; } InstallScope::Project { project_id } => { - let mut p = find_project(&state, &project_id)?; - if !ops::remove_install(&mut p.marketplace_installs, &item) { - return Err(format!("That item is not installed in \"{}\".", p.name)); - } - state.projects_store.update(p)?; + let name = find_project(&state, &project_id)?.name; + state + .projects_store + .update_marketplace_fields(&project_id, |installs, _| { + if ops::remove_install(installs, &item) { + Ok(()) + } else { + Err(format!("That item is not installed in \"{name}\".")) + } + })?; } } refresh_pins(&state).await; @@ -857,9 +860,13 @@ pub async fn set_global_item_disabled( state: State<'_, AppState>, ) -> Result { validate_item(&item)?; - let mut p = find_project(&state, &project_id)?; - ops::set_disabled(&mut p.marketplace_disabled, &item, disabled); - state.projects_store.update(p) + let ((), saved) = state + .projects_store + .update_marketplace_fields(&project_id, |_, list| { + ops::set_disabled(list, &item, disabled); + Ok(()) + })?; + Ok(saved) } // ───────────────────────────────────────────────────────────────────────────── @@ -924,11 +931,16 @@ pub async fn update_marketplace_item( state.settings_store.update(s)?; } InstallScope::Project { project_id } => { - let mut p = find_project(&state, &project_id)?; - if !ops::repin(&mut p.marketplace_installs, &item, &head) { - return Err(format!("That item is not installed in \"{}\".", p.name)); - } - state.projects_store.update(p)?; + let name = find_project(&state, &project_id)?.name; + state + .projects_store + .update_marketplace_fields(&project_id, |installs, _| { + if ops::repin(installs, &item, &head) { + Ok(()) + } else { + Err(format!("That item is not installed in \"{name}\".")) + } + })?; } } refresh_pins(&state).await; diff --git a/app/src-tauri/src/storage/projects_store.rs b/app/src-tauri/src/storage/projects_store.rs index 338da50..7c09b23 100644 --- a/app/src-tauri/src/storage/projects_store.rs +++ b/app/src-tauri/src/storage/projects_store.rs @@ -2,6 +2,7 @@ use std::fs; use std::path::{Path, PathBuf}; use std::sync::Mutex; +use crate::models::marketplace::{MarketplaceInstall, MarketplaceItemRef}; use crate::models::Project; /// The sticky marker for `projects.json`: `projects.json.corrupt`, beside it. @@ -256,6 +257,60 @@ impl ProjectsStore { } } + /// Read-modify-write of one project's marketplace installs and opt-outs + /// under the store's lock, touching nothing else (PR review #2): the + /// marketplace commands must not write back a whole record read before a + /// start changed its status or container id. When `f` fails nothing is + /// saved. Returns `f`'s value and the saved project. + pub fn update_marketplace_fields( + &self, + project_id: &str, + f: impl FnOnce( + &mut Vec, + &mut Vec, + ) -> Result, + ) -> Result<(T, Project), String> { + let mut projects = self.lock(); + let p = projects + .iter_mut() + .find(|p| p.id == project_id) + .ok_or_else(|| format!("Project {} not found", project_id))?; + let mut installs = p.marketplace_installs.clone(); + let mut disabled = p.marketplace_disabled.clone(); + let out = f(&mut installs, &mut disabled)?; + p.marketplace_installs = installs; + p.marketplace_disabled = disabled; + p.updated_at = chrono::Utc::now().to_rfc3339(); + let saved = p.clone(); + self.save(&projects)?; + Ok((out, saved)) + } + + /// [`Self::update_marketplace_fields`] over every project at once, in one + /// save. Projects `f` leaves as they were are not touched at all. + pub fn update_all_marketplace_fields( + &self, + mut f: impl FnMut(&mut Vec, &mut Vec), + ) -> Result<(), String> { + let mut projects = self.lock(); + let mut changed = false; + for p in projects.iter_mut() { + let mut installs = p.marketplace_installs.clone(); + let mut disabled = p.marketplace_disabled.clone(); + f(&mut installs, &mut disabled); + if installs != p.marketplace_installs || disabled != p.marketplace_disabled { + p.marketplace_installs = installs; + p.marketplace_disabled = disabled; + p.updated_at = chrono::Utc::now().to_rfc3339(); + changed = true; + } + } + if changed { + self.save(&projects)?; + } + Ok(()) + } + pub fn set_container_id(&self, project_id: &str, container_id: Option) -> Result<(), String> { let mut projects = self.lock(); if let Some(p) = projects.iter_mut().find(|p| p.id == project_id) { @@ -410,4 +465,86 @@ mod tests { fs::remove_dir_all(&dir).ok(); } + + fn market_install(key: &str) -> crate::models::marketplace::MarketplaceInstall { + crate::models::marketplace::MarketplaceInstall { + marketplace_id: "m1".into(), + kind: crate::models::marketplace::ItemKind::Agent, + key: key.into(), + commit: "a".repeat(40), + } + } + + #[test] + fn marketplace_edits_keep_a_concurrent_status_and_container_change() { + // PR review #2: a marketplace install/uninstall used to write back a + // whole record read before a start flipped status and container_id, + // leaving the project stuck at Starting with no container. + let dir = temp_dir("marketplace-fields"); + let project = Project::new("demo".to_string(), Vec::new()); + let id = project.id.clone(); + let store = store_over(&dir, vec![project]); + + // The start flow moves on while a marketplace command is running. + store.set_container_id(&id, Some("cid-1".into())).unwrap(); + store.update_status(&id, crate::models::ProjectStatus::Starting).unwrap(); + + let (added, saved) = store + .update_marketplace_fields(&id, |installs, disabled| { + installs.push(market_install("a")); + disabled.push(market_install("g").item_ref()); + Ok(installs.len()) + }) + .unwrap(); + assert_eq!(added, 1); + assert_eq!(saved.container_id.as_deref(), Some("cid-1")); + assert_eq!(saved.status, crate::models::ProjectStatus::Starting); + let on_disk: Vec = + serde_json::from_str(&fs::read_to_string(dir.join("projects.json")).unwrap()).unwrap(); + assert_eq!(on_disk[0].container_id.as_deref(), Some("cid-1")); + assert_eq!(on_disk[0].marketplace_installs, vec![market_install("a")]); + + // A refusal inside the closure writes nothing. + let before = fs::read_to_string(dir.join("projects.json")).unwrap(); + let err = store + .update_marketplace_fields(&id, |installs, _| { + installs.clear(); + Err::<(), _>("not installed".to_string()) + }) + .unwrap_err(); + assert_eq!(err, "not installed"); + assert_eq!(store.get(&id).unwrap().marketplace_installs.len(), 1); + assert_eq!(fs::read_to_string(dir.join("projects.json")).unwrap(), before); + assert!(store.update_marketplace_fields("nope", |_, _| Ok(())).is_err()); + + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn marketplace_edits_across_all_projects_touch_only_those_fields() { + let dir = temp_dir("marketplace-all"); + let mut a = Project::new("a".to_string(), Vec::new()); + a.marketplace_installs = vec![market_install("x")]; + let b = Project::new("b".to_string(), Vec::new()); + let (a_id, b_id) = (a.id.clone(), b.id.clone()); + let store = store_over(&dir, vec![a, b]); + store.set_container_id(&b_id, Some("cid-b".into())).unwrap(); + store.update_status(&a_id, crate::models::ProjectStatus::Running).unwrap(); + + let b_updated_at = store.get(&b_id).unwrap().updated_at; + store + .update_all_marketplace_fields(|installs, _| { + installs.retain(|i| i.marketplace_id != "m1") + }) + .unwrap(); + + let a = store.get(&a_id).unwrap(); + assert!(a.marketplace_installs.is_empty()); + assert_eq!(a.status, crate::models::ProjectStatus::Running); + let b = store.get(&b_id).unwrap(); + assert_eq!(b.container_id.as_deref(), Some("cid-b")); + assert_eq!(b.updated_at, b_updated_at, "an untouched project is not rewritten"); + + fs::remove_dir_all(&dir).ok(); + } }