From 76926de369ddaf1e86b56a6dc3f4a94e9e93d9c5 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Tue, 7 Jul 2026 19:21:03 +0200 Subject: [PATCH] fix(apply): bound the blast-radius up-walk; fix two stale comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second-review follow-ups on the ownership cleanups: - The new `group_blast_radius` `app_chain` recursion walked app→root with no depth cap, unlike the `ancestors()` CTE it replaced (`WHERE c.depth < 64`). Add the same `ac.depth < 64` guard so a (should-be-impossible) cycle in `groups.parent_id` can't spin the recursive CTE unbounded and hang a plan. - Drop the now-false "memoized per group" line from `group_blast_radius`'s doc (it is a single set-based query now). - Correct the `PlanRequest` flatten comment: apply is deliberately NOT flattened (its pre-§7 wire was already `{bundle, …}`), so the two endpoints are asymmetric by history — the old comment wrongly claimed parity. Verified: the blast-radius journey + ownership journeys still pass; the INNER JOIN to `projects` is safe (owner_project FK is ON DELETE SET NULL, so a non-NULL owner always references a live row — no dangling drop). Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_api.rs | 12 +++++++----- crates/manager-core/src/apply_service.rs | 5 ++--- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/manager-core/src/apply_api.rs b/crates/manager-core/src/apply_api.rs index 8355ae8..7de7c91 100644 --- a/crates/manager-core/src/apply_api.rs +++ b/crates/manager-core/src/apply_api.rs @@ -183,11 +183,13 @@ async fn group_extension_points_handler( /// `[project]`, so the plan can preview the ownership outcome + attach ceiling. #[derive(Deserialize)] pub struct PlanRequest { - // Flattened so the bundle fields sit at the JSON top level: an older `pic` - // (pre-§7) that POSTs a bare bundle still deserializes (`project` defaults - // to `None`), and a newer client sends the same bundle fields plus a - // top-level `project` key. Keeps the plan wire compatible in both skew - // directions, matching the additive compatibility of the apply request. + // Flattened so the bundle fields sit at the JSON top level — restoring the + // pre-§7 plan wire, which took a BARE `Json`. An older `pic` that + // POSTs a bare bundle still deserializes (`project` defaults to `None`), and + // a newer client sends the same bundle fields plus a top-level `project` + // key. (Apply is deliberately NOT flattened — its pre-§7 wire was already + // `{bundle, prune, …}`, so flattening it would instead break old apply + // clients; the two endpoints are asymmetric because their histories are.) #[serde(flatten)] pub bundle: Bundle, #[serde(default)] diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index ce4c01e..593773c 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -2192,8 +2192,7 @@ impl ApplyService { /// §7 M3.2: descendant apps of `gid` grouped by their nearest-claimed- /// ancestor project, EXCLUDING `exclude` (the planning project) — the apps /// a change to `gid`'s inherited config fans out to across other repos. - /// Ancestor resolution is memoized per group, so the cost is one walk per - /// distinct descendant group, not per app. + /// Resolved in one set-based recursive query (see below). async fn group_blast_radius( &self, gid: GroupId, @@ -2218,7 +2217,7 @@ impl ApplyService { UNION ALL SELECT ac.app_id, g.parent_id, ac.depth + 1 FROM app_chain ac JOIN groups g ON g.id = ac.gid - WHERE g.parent_id IS NOT NULL + WHERE g.parent_id IS NOT NULL AND ac.depth < 64 ), nearest AS ( SELECT DISTINCT ON (ac.app_id) ac.app_id, g.owner_project