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 <noreply@anthropic.com>
This commit is contained in:
2026-09-27 13:12:14 -07:00
co-authored by Claude Opus 5.5
parent e62ca8795a
commit c87299dda3
3 changed files with 220 additions and 46 deletions
+157 -10
View File
@@ -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<String> {
self.snapshots
.lock()
.unwrap()
.get(marketplace_id)
.and_then(|s| s.head_commit.clone())
}
pub fn snapshot(&self, marketplace_id: &str) -> Option<MarketplaceSnapshot> {
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<String> {
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<bool, String> {
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<gix::Repository>,
trees: HashMap<String, Result<GitTree, String>>,
}
impl UpdateCheck {
fn new(mgr: &MarketplaceManager, m: &Marketplace) -> Option<Self> {
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<bool, String> {
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<ItemUpdate> {
let mut seen = BTreeSet::new();
let mut checks: HashMap<String, Option<UpdateCheck>> = 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);