From 6ec034fb1d4933d034fb042ce2853c24ee0f99b2 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Tue, 7 Jul 2026 21:49:05 +0200 Subject: [PATCH] refactor(apply): unify divergence detection and drop the plan-path N+1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review #3 (LOW). `divergence_preview` (plan path) compared parent slugs case-sensitively from its own `get_by_slug`, while the apply path (`reconcile_group_structure_tx`) compares resolved parent ids. On a mixed-case parent slug the two could disagree — `pic plan` reporting `diverged` while `pic apply` hard-errors "parent does not exist". `resolve_existing_group_ids` becomes `load_existing_groups`, returning the full `Group` rows it already loads; `plan_tree` derives the id map from them and passes the map to `divergence_preview`, which now reads each node's own row without a per-node `get_by_slug` (kills the N+1), resolves an in-tree parent's slug from the same map, and compares parent slugs case-insensitively so the preview agrees with the apply path's id-based check. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_service.rs | 50 ++++++++++++++---------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index f830c48..676af0c 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -1679,7 +1679,9 @@ impl ApplyService { bundle: &TreeBundle, project: Option<&ProjectDecl>, ) -> Result { - let resolved = self.resolve_existing_group_ids(bundle).await?; + let existing = self.load_existing_groups(bundle).await?; + let resolved: HashMap = + existing.iter().map(|(k, g)| (k.clone(), g.id)).collect(); let (prepared, to_create, token) = self.prepare_tree(bundle, &resolved).await?; // §6/§7 M3: attach-point ceiling preview (consistent with apply_tree). if let Some(pg_slug) = project.and_then(|p| p.parent_group.as_deref()) { @@ -1694,7 +1696,7 @@ impl ApplyService { let ownership = self.ownership_preview(p.owner, declared).await?; // §6 M2: for an existing group node, preview a structural divergence // (server parent ≠ manifest parent) before apply. - let structure = self.divergence_preview(p.node).await?; + let structure = self.divergence_preview(p.node, &existing).await?; nodes.push(NodePlan { kind: p.node.kind, slug: p.node.slug.clone(), @@ -1932,12 +1934,14 @@ impl ApplyService { Ok(report) } - /// §6: resolve every GROUP node's slug → id on the pool, skipping ones that - /// don't exist yet (they are to-create, previewed by the plan path). - async fn resolve_existing_group_ids( + /// §6: load every EXISTING group node (lowercased slug → its full row) on + /// the pool, skipping ones that don't exist yet (to-create, previewed by the + /// plan path). Returns the whole `Group` so the divergence preview reads + /// `parent_id` (and resolves an in-tree parent's slug) without re-fetching. + async fn load_existing_groups( &self, bundle: &TreeBundle, - ) -> Result, ApplyError> { + ) -> Result, ApplyError> { let mut map = HashMap::new(); for n in &bundle.nodes { if n.kind == NodeKind::Group { @@ -1947,7 +1951,7 @@ impl ApplyService { .await .map_err(|e| ApplyError::Backend(e.to_string()))? { - map.insert(n.slug.to_lowercase(), g.id); + map.insert(n.slug.to_lowercase(), g); } } } @@ -1957,32 +1961,36 @@ impl ApplyService { /// §6 M2: preview an EXISTING group node's structural divergence — `Some` /// only when the server's parent differs from the manifest's declared /// (directory-nesting) parent. `None` for an app node, a to-create group, or - /// an in-sync group. + /// an in-sync group. Reads the node's own row from the pre-loaded `existing` + /// map (no per-node `get_by_slug`), resolves the server parent's slug from + /// that map when the parent is another tree node (else one `get_by_id`), and + /// compares parent slugs **case-insensitively** so this preview agrees with + /// the apply path's id-based check (which lowercases) on a mixed-case slug. async fn divergence_preview( &self, node: &TreeNode, + existing: &HashMap, ) -> Result, ApplyError> { if node.kind != NodeKind::Group { return Ok(None); } - let Some(group) = self - .groups - .get_by_slug(&node.slug) - .await - .map_err(|e| ApplyError::Backend(e.to_string()))? - else { + let Some(group) = existing.get(&node.slug.to_lowercase()) else { return Ok(None); // to-create — not a divergence }; let server_parent = match group.parent_id { - Some(pid) => self - .groups - .get_by_id(pid) - .await - .map_err(|e| ApplyError::Backend(e.to_string()))? - .map(|g| g.slug), + Some(pid) => match existing.values().find(|g| g.id == pid) { + Some(g) => Some(g.slug.clone()), + None => self + .groups + .get_by_id(pid) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))? + .map(|g| g.slug), + }, None => None, }; - if server_parent.as_deref() == node.parent.as_deref() { + let norm = |s: Option<&str>| s.map(str::to_lowercase); + if norm(server_parent.as_deref()) == norm(node.parent.as_deref()) { return Ok(None); // in-sync } Ok(Some(StructurePreview {