From 00edb92bff822b1aedc63c9bd16ac8a35749fba4 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:10:05 -0700 Subject: [PATCH] fix(pm): order dependency lifecycle builds --- .../crates/aube/src/commands/install/delta.rs | 151 ++++++++++++++++++ .../aube/src/commands/install/lifecycle.rs | 51 +++++- vendor/aube/crates/aube/tests/e2e.rs | 43 +++++ 3 files changed, 239 insertions(+), 6 deletions(-) diff --git a/vendor/aube/crates/aube/src/commands/install/delta.rs b/vendor/aube/crates/aube/src/commands/install/delta.rs index 6fb7592ee..c52fcacbc 100644 --- a/vendor/aube/crates/aube/src/commands/install/delta.rs +++ b/vendor/aube/crates/aube/src/commands/install/delta.rs @@ -397,6 +397,108 @@ fn tarjan_scc(graph: &LockfileGraph) -> Vec> { out } +/// Group selected dependency builds into children-first phases. +/// +/// The graph walk includes non-building packages only when they connect two +/// selected builds: if `parent -> bridge -> child` and only parent/child have +/// lifecycle scripts, child still has to finish before parent starts. An +/// unrelated non-building child does not delay its parent. Strongly connected +/// components collapse dependency cycles into one phase, matching pnpm's +/// graph sequencer rather than deadlocking on a cycle. +pub(super) fn dependency_build_phases( + graph: &LockfileGraph, + selected: &BTreeSet, +) -> Vec> { + let mut components = tarjan_scc(graph); + for members in &mut components { + members.sort(); + } + + let mut component_by_path = BTreeMap::new(); + for (index, members) in components.iter().enumerate() { + for dep_path in members { + component_by_path.insert(dep_path.as_str(), index); + } + } + + // Edges point parent -> child. A component becomes ready only after all + // child components have drained, so Kahn's algorithm emits leaves first. + let mut children = vec![BTreeSet::new(); components.len()]; + let mut parents = vec![BTreeSet::new(); components.len()]; + for (dep_path, package) in &graph.packages { + let Some(&parent) = component_by_path.get(dep_path.as_str()) else { + continue; + }; + for (child_name, child_tail) in &package.dependencies { + let Some(child_path) = aube_lockfile::resolve_dep_edge(child_name, child_tail, |key| { + graph.packages.contains_key(key) + }) else { + continue; + }; + let Some(&child) = component_by_path.get(child_path.as_str()) else { + continue; + }; + if parent != child && children[parent].insert(child) { + parents[child].insert(parent); + } + } + } + + // pnpm sequences only packages that build and the ancestors connecting + // them. Starting at selected components and walking toward parents gives + // that same projection without letting unrelated graph depth serialize + // independent builds. + let mut relevant = BTreeSet::new(); + let mut pending: Vec = selected + .iter() + .filter_map(|dep_path| component_by_path.get(dep_path.as_str()).copied()) + .collect(); + while let Some(component) = pending.pop() { + if relevant.insert(component) { + pending.extend(parents[component].iter().copied()); + } + } + + let mut remaining_children: Vec = children + .iter() + .map(|component_children| { + component_children + .iter() + .filter(|child| relevant.contains(child)) + .count() + }) + .collect(); + let mut ready: BTreeSet = relevant + .iter() + .copied() + .filter(|index| remaining_children[*index] == 0) + .collect(); + let mut phases = Vec::new(); + while !ready.is_empty() { + let current = std::mem::take(&mut ready); + let mut selected_here = Vec::new(); + for component in current { + selected_here.extend( + components[component] + .iter() + .filter(|dep_path| selected.contains(*dep_path)) + .cloned(), + ); + for &parent in &parents[component] { + remaining_children[parent] -= 1; + if remaining_children[parent] == 0 { + ready.insert(parent); + } + } + } + selected_here.sort(); + if !selected_here.is_empty() { + phases.push(selected_here); + } + } + phases +} + fn dfs_post(start: usize, edges: &[BTreeSet], seen: &mut [bool], order: &mut Vec) { if seen[start] { return; @@ -1045,4 +1147,53 @@ mod tests { assert_eq!(hashes.len(), 2); assert_eq!(hashes["a@1"], hashes["b@1"]); } + + #[test] + fn build_phases_wait_through_non_building_intermediaries() { + let graph = graph_of(&[ + pkg_with_deps("parent", "1", &[("bridge", "1"), ("unused", "1")]), + pkg_with_deps("bridge", "1", &[("child", "1")]), + pkg("child", "1"), + pkg("unused", "1"), + pkg("independent", "1"), + ]); + let selected = [ + "parent@1".to_string(), + "child@1".to_string(), + "independent@1".to_string(), + ] + .into_iter() + .collect(); + + assert_eq!( + dependency_build_phases(&graph, &selected), + vec![ + vec!["child@1".to_string(), "independent@1".to_string()], + vec!["parent@1".to_string()], + ] + ); + } + + #[test] + fn build_phases_keep_independent_jobs_parallel_and_cycles_live() { + let graph = graph_of(&[ + pkg_with_deps("parent", "1", &[("left", "1"), ("right", "1")]), + pkg_with_deps("left", "1", &[("right", "1")]), + pkg_with_deps("right", "1", &[("left", "1")]), + pkg("independent", "1"), + ]); + let selected = graph.packages.keys().cloned().collect(); + + assert_eq!( + dependency_build_phases(&graph, &selected), + vec![ + vec![ + "independent@1".to_string(), + "left@1".to_string(), + "right@1".to_string(), + ], + vec!["parent@1".to_string()], + ] + ); + } } diff --git a/vendor/aube/crates/aube/src/commands/install/lifecycle.rs b/vendor/aube/crates/aube/src/commands/install/lifecycle.rs index 2fcb74572..5e74de80f 100644 --- a/vendor/aube/crates/aube/src/commands/install/lifecycle.rs +++ b/vendor/aube/crates/aube/src/commands/install/lifecycle.rs @@ -422,6 +422,8 @@ pub(crate) async fn run_dep_lifecycle_scripts( // every 200-package graph for a graph that has 3 allowlisted deps. #[derive(Clone)] struct BuildJob { + dep_path: String, + phase: usize, name: String, registry_name: String, version: String, @@ -551,6 +553,8 @@ pub(crate) async fn run_dep_lifecycle_scripts( floor_trusted.push(pkg.spec_key()); } jobs.push(BuildJob { + dep_path: dep_path.clone(), + phase: 0, name: pkg.name.clone(), registry_name: pkg.registry_name().to_string(), version: pkg.version.clone(), @@ -566,6 +570,20 @@ pub(crate) async fn run_dep_lifecycle_scripts( return Ok(0); } + let selected: std::collections::BTreeSet = + jobs.iter().map(|job| job.dep_path.clone()).collect(); + let phases = super::delta::dependency_build_phases(graph, &selected); + let phase_by_path: std::collections::BTreeMap<&str, usize> = phases + .iter() + .enumerate() + .flat_map(|(phase, paths)| paths.iter().map(move |path| (path.as_str(), phase))) + .collect(); + for job in &mut jobs { + job.phase = *phase_by_path + .get(job.dep_path.as_str()) + .expect("every selected lifecycle job must have a build phase"); + } + // Name what the floor let through — the floor must never be a // silent allow path. One line, not per-package, so big graphs // don't drown the install output. Emitted at `warn` with a stable @@ -629,12 +647,11 @@ pub(crate) async fn run_dep_lifecycle_scripts( node_gyp_bootstrap::lazy_shim_bin_dir(&project_bin_dir)? }); - // Pass 2 (parallel, bounded): fan out across `child_concurrency` - // concurrent workers. Inside one job the three hooks - // (preinstall → install → postinstall) still run sequentially — - // pnpm's execution model is "at most N packages building in - // parallel," not "at most N scripts running," so hook ordering - // within a single package is preserved. + // Pass 2 (dependency-ordered, parallel within each phase): all jobs are + // registered up front so the existing first-error cancellation stays + // intact, but a package waits until every selected build in its dependency + // phase has finished. Jobs in one phase remain bounded by + // `child_concurrency`; hooks within one package stay sequential. // // Cancellation on first failure uses `JoinSet`, which aborts every // outstanding task when it's dropped. A plain `Vec` @@ -646,6 +663,13 @@ pub(crate) async fn run_dep_lifecycle_scripts( // waiting for the longest-running one to finish. let concurrency = child_concurrency.max(1); let semaphore = std::sync::Arc::new(tokio::sync::Semaphore::new(concurrency)); + let (phase_tx, phase_rx) = tokio::sync::watch::channel(0usize); + let phase_remaining = std::sync::Arc::new( + phases + .iter() + .map(|phase| std::sync::atomic::AtomicUsize::new(phase.len())) + .collect::>(), + ); let project_dir = project_dir.to_path_buf(); let modules_dir_name = modules_dir_name.to_string(); let should_restore_side_effects_cache = side_effects_cache.should_restore(); @@ -659,7 +683,13 @@ pub(crate) async fn run_dep_lifecycle_scripts( let modules_dir_name = modules_dir_name.clone(); let node_gyp_bin_dir = node_gyp_bin_dir.clone(); let jail_policy = jail_policy.clone(); + let phase_tx = phase_tx.clone(); + let mut phase_rx = phase_rx.clone(); + let phase_remaining = phase_remaining.clone(); let task = crate::dep_chain::scope_current(async move { + while *phase_rx.borrow_and_update() < job.phase { + phase_rx.changed().await.into_diagnostic()?; + } let _permit = sem.acquire().await.unwrap(); if should_restore_side_effects_cache && let Some(cache_entry) = job.cache_entry.clone() { @@ -677,6 +707,12 @@ pub(crate) async fn run_dep_lifecycle_scripts( })?; match restore_result? { SideEffectsCacheRestore::Restored | SideEffectsCacheRestore::AlreadyApplied => { + if phase_remaining[job.phase] + .fetch_sub(1, std::sync::atomic::Ordering::AcqRel) + == 1 + { + let _ = phase_tx.send(job.phase + 1); + } return Ok(0); } SideEffectsCacheRestore::Miss => {} @@ -808,6 +844,9 @@ pub(crate) async fn run_dep_lifecycle_scripts( ); } } + if phase_remaining[job.phase].fetch_sub(1, std::sync::atomic::Ordering::AcqRel) == 1 { + let _ = phase_tx.send(job.phase + 1); + } Ok(ran_here) }); let task = crate::runtime::scope_current(task); diff --git a/vendor/aube/crates/aube/tests/e2e.rs b/vendor/aube/crates/aube/tests/e2e.rs index 4e240bb40..c1d8cc604 100644 --- a/vendor/aube/crates/aube/tests/e2e.rs +++ b/vendor/aube/crates/aube/tests/e2e.rs @@ -335,3 +335,46 @@ fn approve_builds_surfaces_and_runs_a_local_source_dep() { .success() .stdout(predicates::str::contains("No ignored builds")); } + +#[test] +fn dependency_build_finishes_before_its_consumers_build_starts() { + let _guard = e2e_lock(); + let sbx = Sandbox::new(); + + sbx.write_file( + "child/package.json", + r#"{ + "name": "build-child", + "version": "1.0.0", + "scripts": { + "postinstall": "node -e \"setTimeout(() => require('fs').writeFileSync('READY', 'ok'), 300)\"" + } + }"#, + ); + sbx.write_file( + "parent/package.json", + r#"{ + "name": "build-parent", + "version": "1.0.0", + "dependencies": { "build-child": "file:../child" }, + "scripts": { + "postinstall": "node -e \"const fs=require('fs'), p=require('path').join(require('path').dirname(require.resolve('build-child/package.json')), 'READY'); if (!fs.existsSync(p)) process.exit(17); fs.writeFileSync('PARENT_READY', 'ok')\"" + } + }"#, + ); + sbx.write_manifest( + r#"{ + "name": "dependency-build-order-e2e", + "version": "1.0.0", + "dependencies": { "build-parent": "file:./parent" } + }"#, + ); + + sbx.cmd() + .args(["install", "--dangerously-allow-all-builds"]) + .assert() + .success(); + let node_modules = sbx.project.join("node_modules"); + assert!(marker_exists_under(&node_modules, "READY")); + assert!(marker_exists_under(&node_modules, "PARENT_READY")); +}