From c87299dda358f7dd800b446e469615ca9d8db9ed Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 27 Sep 2026 13:12:14 -0700 Subject: [PATCH] Marketplace: cheaper update checks and single-marketplace refreshes (PR review #10) compute_updates reads each marketplace's head once (snapshot_head, no snapshot clone) and opens each cache once, sharing trees across installs through GitTree::at. refresh_marketplaces(Some(id)) re-pins only that marketplace (mk::set_pins with only) and returns only its snapshot, which the frontend merges by id. Co-Authored-By: Claude Opus 5.5 --- .../src/commands/marketplace_commands.rs | 55 +++--- app/src-tauri/src/marketplace/mod.rs | 167 ++++++++++++++++-- app/src-tauri/src/marketplace/tree.rs | 44 ++++- 3 files changed, 220 insertions(+), 46 deletions(-) diff --git a/app/src-tauri/src/commands/marketplace_commands.rs b/app/src-tauri/src/commands/marketplace_commands.rs index 7e0bae0..0f96436 100644 --- a/app/src-tauri/src/commands/marketplace_commands.rs +++ b/app/src-tauri/src/commands/marketplace_commands.rs @@ -568,34 +568,17 @@ async fn snapshot_blocking( .map_err(|e| format!("Reading the marketplace cache failed: {e}")) } -/// Make each cache's pin refs exactly the commits installs reference, so a -/// pinned version can never be garbage-collected away. Each marketplace's -/// pins are set under that marketplace's repo lock only (pre-flight F11, PR -/// review #5): a concurrent fetch writes refs in the same repo. +/// Make each cache's pin refs exactly the commits installs reference (see +/// [`mk::set_pins`]), for every marketplace. pub(crate) async fn refresh_pins(state: &AppState) { + refresh_pins_of(state, None).await; +} + +/// [`refresh_pins`] for every marketplace, or only `only`. +async fn refresh_pins_of(state: &AppState, only: Option<&str>) { let settings = state.settings_store.get(); - let pins = mk::pins_by_marketplace(&settings, &state.projects_store.list()); - let root = state.marketplace.data_root().to_path_buf(); - for m in &settings.marketplaces { - let lock = state.marketplace.repo_lock(&m.id); - let _repo_guard = lock.lock().await; - let repo = git::cache_path(&root, &m.id); - let commits = pins.get(&m.id).cloned().unwrap_or_default(); - let id = m.id.clone(); - let _ = tokio::task::spawn_blocking(move || { - if !repo.exists() { - return; - } - if let Err(e) = git::set_pins(&repo, &commits) { - log::warn!( - "Could not update the pinned commits of marketplace {}: {}", - id, - e - ); - } - }) - .await; - } + let projects = state.projects_store.list(); + mk::set_pins(&state.marketplace, &settings, &projects, only).await; } /// Forget a marketplace's snapshot and delete its cache, under its repo lock. @@ -640,21 +623,25 @@ pub async fn list_marketplace_snapshots( .map_err(|e| format!("Reading the marketplace caches failed: {e}")) } +/// With `marketplace_id`, refreshes (and re-pins) only that marketplace and +/// returns only its snapshot — the frontend merges snapshots by id — or +/// nothing if it was removed meanwhile. Without, refreshes all and returns +/// every snapshot. #[tauri::command] pub async fn refresh_marketplaces( marketplace_id: Option, state: State<'_, AppState>, ) -> Result, String> { - let settings = state.settings_store.get(); + let current = || state.settings_store.get(); if let Some(id) = &marketplace_id { - find_marketplace(&settings, id)?; + find_marketplace(¤t(), id)?; + let snap = mk::refresh_marketplace(&state.marketplace, ¤t, id).await; + refresh_pins_of(&state, Some(id)).await; + let still_configured = find_marketplace(¤t(), id).is_ok(); + return Ok(if still_configured { vec![snap] } else { vec![] }); } - for m in settings - .marketplaces - .iter() - .filter(|m| marketplace_id.as_deref().is_none_or(|id| id == m.id)) - { - mk::refresh_marketplace(&state.marketplace, &|| state.settings_store.get(), &m.id).await; + for m in current().marketplaces { + mk::refresh_marketplace(&state.marketplace, ¤t, &m.id).await; } refresh_pins(&state).await; list_marketplace_snapshots(state).await diff --git a/app/src-tauri/src/marketplace/mod.rs b/app/src-tauri/src/marketplace/mod.rs index 9474bd3..dd47a48 100644 --- a/app/src-tauri/src/marketplace/mod.rs +++ b/app/src-tauri/src/marketplace/mod.rs @@ -94,6 +94,15 @@ impl MarketplaceManager { .clone() } + /// The in-memory snapshot's head, without copying the snapshot's items. + pub fn snapshot_head(&self, marketplace_id: &str) -> Option { + self.snapshots + .lock() + .unwrap() + .get(marketplace_id) + .and_then(|s| s.head_commit.clone()) + } + pub fn snapshot(&self, marketplace_id: &str) -> Option { self.snapshots.lock().unwrap().get(marketplace_id).cloned() } @@ -195,7 +204,7 @@ impl MarketplaceManager { /// Head commit for a marketplace: the in-memory snapshot's, else the cache's. pub fn head_for(mgr: &MarketplaceManager, m: &Marketplace) -> Option { - mgr.snapshot(&m.id).and_then(|s| s.head_commit).or_else(|| { + mgr.snapshot_head(&m.id).or_else(|| { git::cached_head(&git::cache_path(mgr.data_root(), &m.id)) .ok() .flatten() @@ -341,11 +350,42 @@ pub async fn remove_marketplace_cache(mgr: &MarketplaceManager, marketplace_id: .await; } -fn item_changed(repo: &Path, inst: &MarketplaceInstall, head: &str) -> Result { - let old = GitTree::open(repo, &inst.commit)?; - let new = GitTree::open(repo, head)?; - Ok(item_fingerprint(&old, inst.kind, &inst.key)? - != item_fingerprint(&new, inst.kind, &inst.key)?) +/// One marketplace's side of an update check: its head, read once, and its +/// cache, opened once, with trees shared across installs (PR review #10). +struct UpdateCheck { + head: String, + repo: Option, + trees: HashMap>, +} + +impl UpdateCheck { + fn new(mgr: &MarketplaceManager, m: &Marketplace) -> Option { + let head = head_for(mgr, m)?; + Some(Self { + head, + repo: None, + trees: HashMap::new(), + }) + } + + fn tree(&mut self, repo_path: &Path, commit: &str) -> Result<&GitTree, String> { + if !self.trees.contains_key(commit) { + if self.repo.is_none() { + self.repo = Some(tree::open_repo(repo_path)?); + } + let repo = self.repo.clone().expect("opened above"); + self.trees + .insert(commit.to_string(), GitTree::at(repo, commit)); + } + self.trees[commit].as_ref().map_err(Clone::clone) + } + + fn changed(&mut self, repo_path: &Path, inst: &MarketplaceInstall) -> Result { + let head = self.head.clone(); + let new = item_fingerprint(self.tree(repo_path, &head)?, inst.kind, &inst.key)?; + let old = item_fingerprint(self.tree(repo_path, &inst.commit)?, inst.kind, &inst.key)?; + Ok(old != new) + } } /// Every install (global + all projects) whose item fingerprint at head @@ -356,6 +396,7 @@ pub fn compute_updates( projects: &[Project], ) -> Vec { let mut seen = BTreeSet::new(); + let mut checks: HashMap> = HashMap::new(); let mut out = Vec::new(); let all = settings .global_marketplace_installs @@ -372,18 +413,21 @@ pub fn compute_updates( else { continue; }; - let Some(head) = head_for(mgr, m) else { + let Some(check) = checks + .entry(m.id.clone()) + .or_insert_with(|| UpdateCheck::new(mgr, m)) + else { continue; }; - if head == inst.commit { + if check.head == inst.commit { continue; } let repo = git::cache_path(mgr.data_root(), &m.id); - match item_changed(&repo, inst, &head) { + match check.changed(&repo, inst) { Ok(true) => out.push(ItemUpdate { item: inst.item_ref(), pinned: inst.commit.clone(), - head, + head: check.head.clone(), }), Ok(false) => {} Err(e) => log::debug!("Update check skipped for {}: {}", inst.key, e), @@ -479,6 +523,43 @@ pub fn spawn_project_sync( }); } +/// Make each cache's pin refs exactly the commits installs reference, so a +/// pinned version can never be garbage-collected away — for every configured +/// marketplace, or only `only`. Each marketplace's pins are set under its own +/// repo lock (pre-flight F11, PR review #5): a fetch writes refs there too. +pub async fn set_pins( + mgr: &MarketplaceManager, + settings: &AppSettings, + projects: &[Project], + only: Option<&str>, +) { + let pins = pins_by_marketplace(settings, projects); + for m in settings + .marketplaces + .iter() + .filter(|m| only.is_none_or(|id| id == m.id)) + { + let lock = mgr.repo_lock(&m.id); + let _repo_guard = lock.lock().await; + let repo = git::cache_path(mgr.data_root(), &m.id); + let commits = pins.get(&m.id).cloned().unwrap_or_default(); + let id = m.id.clone(); + let _ = tokio::task::spawn_blocking(move || { + if !repo.exists() { + return; + } + if let Err(e) = git::set_pins(&repo, &commits) { + log::warn!( + "Could not update the pinned commits of marketplace {}: {}", + id, + e + ); + } + }) + .await; + } +} + /// All commits referenced by installs, per marketplace (for `git::set_pins`). pub fn pins_by_marketplace( settings: &AppSettings, @@ -761,6 +842,72 @@ mod tests { assert_eq!(updates[0].head, c2); } + /// PR review #10: one repo open and one head read per marketplace, + /// however many installs (and pinned commits) point into it. + #[tokio::test] + async fn update_check_opens_each_repo_once() { + let Some(fx) = GitFixture::new() else { return }; + let c1 = fx.with_all_kinds(); + fx.write( + "agents/code-reviewer.md", + "---\nname: code-reviewer\ndescription: Reviews code\n---\nReview harder.\n", + ); + let c2 = fx.commit("tweak agent"); + let data = tempfile::tempdir().unwrap(); + let mgr = MarketplaceManager::new(data.path().to_path_buf()); + let mut settings = settings_with(&fx.url()); + settings.global_marketplace_installs = vec![ + install(ItemKind::Agent, "code-reviewer", &c1), + install(ItemKind::Hook, "notify-on-stop", &c1), + ]; + let mut a = crate::models::Project::new("a".into(), vec![]); + a.marketplace_installs = vec![install(ItemKind::Skill, "example-skill", &c1)]; + let mut b = crate::models::Project::new("b".into(), vec![]); + b.marketplace_installs = vec![install(ItemKind::Agent, "code-reviewer", &c2)]; + refresh_marketplace(&mgr, &|| settings.clone(), "m1").await; + + let before = tree::repo_opens(); + let updates = compute_updates(&mgr, &settings, &[a, b]); + assert_eq!(tree::repo_opens() - before, 1, "one open for the whole check"); + + assert_eq!(updates.len(), 1, "{updates:?}"); + assert_eq!(updates[0].item.key, "code-reviewer"); + assert_eq!(updates[0].pinned, c1); + assert_eq!(updates[0].head, c2); + } + + /// PR review #10: refreshing one marketplace sets only its pins, and so + /// never waits on another marketplace's lock. + #[tokio::test] + async fn pins_can_be_set_for_one_marketplace_alone() { + let Some(fx) = GitFixture::new() else { return }; + let c1 = fx.with_all_kinds(); + let data = tempfile::tempdir().unwrap(); + let mgr = MarketplaceManager::new(data.path().to_path_buf()); + let mut settings = settings_with(&fx.url()); + let mut m2 = settings.marketplaces[0].clone(); + m2.id = "m2".into(); + settings.marketplaces.push(m2); + settings.global_marketplace_installs = vec![install(ItemKind::Agent, "code-reviewer", &c1)]; + refresh_marketplace(&mgr, &|| settings.clone(), "m1").await; + refresh_marketplace(&mgr, &|| settings.clone(), "m2").await; + + let other = mgr.repo_lock("m2"); + let _busy = other.lock().await; + tokio::time::timeout( + std::time::Duration::from_secs(20), + set_pins(&mgr, &settings, &[], Some("m1")), + ) + .await + .expect("setting m1's pins must not wait for m2"); + + let refs = git::test_support::git( + &git::cache_path(data.path(), "m1"), + &["for-each-ref", "--format=%(refname)", "refs/triple-c/pins"], + ); + assert_eq!(refs, format!("refs/triple-c/pins/{c1}")); + } + #[test] fn pins_are_grouped_and_deduplicated_per_marketplace() { let a = "a".repeat(40); diff --git a/app/src-tauri/src/marketplace/tree.rs b/app/src-tauri/src/marketplace/tree.rs index 37d3ed0..4a11489 100644 --- a/app/src-tauri/src/marketplace/tree.rs +++ b/app/src-tauri/src/marketplace/tree.rs @@ -92,10 +92,32 @@ pub struct GitTree { tree_id: gix::ObjectId, } +#[cfg(test)] +thread_local! { + static REPO_OPENS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// How many times this thread has opened a cache repo (tests only). +#[cfg(test)] +pub fn repo_opens() -> usize { + REPO_OPENS.with(|c| c.get()) +} + +/// Open a bare cache. Several [`GitTree`]s can share one open repo through +/// [`GitTree::at`] (cloning a `gix::Repository` shares its object store). +pub fn open_repo(repo_path: &std::path::Path) -> Result { + #[cfg(test)] + REPO_OPENS.with(|c| c.set(c.get() + 1)); + gix::open(repo_path).map_err(|e| format!("Could not open the marketplace cache: {}", e)) +} + impl GitTree { pub fn open(repo_path: &std::path::Path, commit: &str) -> Result { - let repo = gix::open(repo_path) - .map_err(|e| format!("Could not open the marketplace cache: {}", e))?; + Self::at(open_repo(repo_path)?, commit) + } + + /// The tree at `commit` of an already open repo. + pub fn at(repo: gix::Repository, commit: &str) -> Result { let oid = gix::ObjectId::from_hex(commit.as_bytes()) .map_err(|e| format!("Invalid commit id {}: {}", commit, e))?; let tree_id = repo @@ -461,6 +483,24 @@ mod tests { assert_eq!(tree.read_file("agents/missing.md", CAP).unwrap(), None); } + #[test] + fn trees_at_several_commits_share_one_open_repo() { + use crate::marketplace::git::test_support::{commit_files, git_available, init_repo}; + if !git_available() { + return; + } + let dir = tempfile::tempdir().unwrap(); + let c1 = init_repo(dir.path(), &[("a.md", "one", false)]); + let c2 = commit_files(dir.path(), &[("a.md", "two", false)], "second"); + let before = repo_opens(); + let repo = open_repo(&dir.path().join(".git")).unwrap(); + let t1 = GitTree::at(repo.clone(), &c1).unwrap(); + let t2 = GitTree::at(repo, &c2).unwrap(); + assert_eq!(repo_opens() - before, 1); + assert_eq!(t1.read_file("a.md", 10).unwrap().unwrap(), b"one"); + assert_eq!(t2.read_file("a.md", 10).unwrap().unwrap(), b"two"); + } + #[test] fn mem_tree_applies_the_same_cap() { let t = MemTree::new().file("a.md", "12345");