diff --git a/api-model/src/buck2/api.rs b/api-model/src/buck2/api.rs index a8f0dce84..53d1f22fd 100644 --- a/api-model/src/buck2/api.rs +++ b/api-model/src/buck2/api.rs @@ -16,13 +16,17 @@ use crate::buck2::{status::Status, types::ProjectRelativePath}; /// Parameters required to build a task. #[derive(Debug, Deserialize, Serialize, ToSchema)] pub struct TaskBuildRequest { - /// The repository base path + /// The Buck2 project path within the monorepo (for example `/jupiter/callisto`). pub repo: String, /// The change list link (URL) pub cl_link: String, //TODO: for old database only, delete after updated pub cl_id: i64, - /// The list of file diff changes + /// The list of changed files, expressed relative to the monorepo root. + /// + /// Example values: + /// - `jupiter/callisto/src/access_token.rs` + /// - `common/lib.rs` pub changes: Vec>, /// Buck2 target path (e.g. //app:server). Optional for backward compatibility. #[serde(default, alias = "targets_path")] @@ -41,6 +45,7 @@ pub struct RetryBuildRequest { pub build_id: String, pub cl_link: String, pub cl_id: i64, + /// The list of changed files, expressed relative to the monorepo root. pub changes: Vec>, pub targets: Option>, } diff --git a/api-model/src/buck2/types.rs b/api-model/src/buck2/types.rs index 5755493ad..e80ca5c22 100644 --- a/api-model/src/buck2/types.rs +++ b/api-model/src/buck2/types.rs @@ -11,6 +11,11 @@ pub enum TaskPhase { RunningBuild, } +/// Slash-separated relative path used in Buck2 payloads. +/// +/// The exact base directory is defined by the surrounding API. For task/build +/// requests, see the field-level docs to determine whether the path is relative +/// to the monorepo root or some other project root. #[derive(Clone, Debug, Hash, PartialEq, Eq, Display, Deserialize, Serialize, ToSchema)] pub struct ProjectRelativePath(String); diff --git a/api-model/src/buck2/ws.rs b/api-model/src/buck2/ws.rs index 7325a13f8..df8688397 100644 --- a/api-model/src/buck2/ws.rs +++ b/api-model/src/buck2/ws.rs @@ -14,8 +14,10 @@ pub enum WSMessage { // Server -> Worker messages TaskBuild { build_id: String, + /// The Buck2 project path within the monorepo (for example `/jupiter/callisto`). repo: String, cl_link: String, + /// The list of changed files, expressed relative to the monorepo root. changes: Vec>, }, diff --git a/ceres/src/build_trigger/changes_calculator.rs b/ceres/src/build_trigger/changes_calculator.rs index 11bf71ea4..006ebbb95 100644 --- a/ceres/src/build_trigger/changes_calculator.rs +++ b/ceres/src/build_trigger/changes_calculator.rs @@ -41,6 +41,8 @@ impl ChangesCalculator { &self, cl_diff_files: Vec, ) -> Result>, MegaError> { + // Orion task requests carry change paths relative to the monorepo root, + // so we preserve repository-relative diff paths here. let to_project_relative = |path: &PathBuf| -> Result { let rel = path .to_string_lossy() diff --git a/orion-server/src/service/api_v2_service.rs b/orion-server/src/service/api_v2_service.rs index a69ea9f46..4cdacd309 100644 --- a/orion-server/src/service/api_v2_service.rs +++ b/orion-server/src/service/api_v2_service.rs @@ -1,4 +1,4 @@ -use std::{convert::Infallible, pin::Pin, time::Duration}; +use std::{collections::HashSet, convert::Infallible, pin::Pin, time::Duration}; use api_model::{ buck2::{ @@ -47,6 +47,29 @@ type MessageErrorResponse = (StatusCode, Json); type JsonValueErrorResponse = (StatusCode, Json); type LogSseStream = Pin> + Send>>; +/// Normalize incoming task changes to the repo-root-relative contract. +/// +/// The protocol no longer asks workers to guess whether a path was expressed +/// relative to the selected sub-project. We only perform lossless cleanup here: +/// trim accidental leading slashes and drop exact duplicates. +fn normalize_repo_root_changes( + changes: Vec>, +) -> Vec> { + // Avoid reserving memory directly from request-controlled input length. + let mut normalized = Vec::new(); + let mut seen = HashSet::new(); + + for change in changes { + let normalized_change = + change.into_map(|path| ProjectRelativePath::new(path.as_str().trim_start_matches('/'))); + if seen.insert(normalized_change.clone()) { + normalized.push(normalized_change); + } + } + + normalized +} + pub async fn task_output(state: &AppState, id: &str) -> Result, StatusCode> { if !state.scheduler.active_builds.contains_key(id) { return Err(StatusCode::NOT_FOUND); @@ -584,6 +607,10 @@ pub async fn build_retry( state: &AppState, req: api_model::buck2::api::RetryBuildRequest, ) -> Response { + let req = api_model::buck2::api::RetryBuildRequest { + changes: normalize_repo_root_changes(req.changes), + ..req + }; let old_build_id = match req.build_id.parse::() { Ok(uuid) => uuid, Err(_) => { @@ -881,6 +908,10 @@ async fn handle_immediate_task_dispatch_v2( } pub async fn task_handler_v2(state: &AppState, req: TaskBuildRequest) -> Response { + let req = TaskBuildRequest { + changes: normalize_repo_root_changes(req.changes), + ..req + }; let task_id = Uuid::now_v7(); if let Err(err) = OrionTasksRepo::insert_task(task_id, &req.cl_link, &req.repo, &req.changes, &state.conn) @@ -1020,3 +1051,44 @@ pub async fn get_orion_client_status_by_id( })?; Ok(Json(OrionClientStatus::from_worker_status(&worker))) } + +#[cfg(test)] +mod tests { + use api_model::buck2::{status::Status, types::ProjectRelativePath}; + + use super::normalize_repo_root_changes; + + #[test] + fn test_normalize_repo_root_changes_trims_leading_slashes_and_deduplicates() { + let normalized = normalize_repo_root_changes(vec![ + Status::Modified(ProjectRelativePath::new("/jupiter/callisto/src/main.rs")), + Status::Modified(ProjectRelativePath::new("jupiter/callisto/src/main.rs")), + Status::Removed(ProjectRelativePath::new("//common/lib.rs")), + Status::Removed(ProjectRelativePath::new("common/lib.rs")), + ]); + + assert_eq!( + normalized, + vec![ + Status::Modified(ProjectRelativePath::new("jupiter/callisto/src/main.rs")), + Status::Removed(ProjectRelativePath::new("common/lib.rs")), + ] + ); + } + + #[test] + fn test_normalize_repo_root_changes_keeps_distinct_status_entries() { + let normalized = normalize_repo_root_changes(vec![ + Status::Added(ProjectRelativePath::new("/common/lib.rs")), + Status::Removed(ProjectRelativePath::new("common/lib.rs")), + ]); + + assert_eq!( + normalized, + vec![ + Status::Added(ProjectRelativePath::new("common/lib.rs")), + Status::Removed(ProjectRelativePath::new("common/lib.rs")), + ] + ); + } +} diff --git a/orion/src/buck_controller.rs b/orion/src/buck_controller.rs index aa156dd45..2a4b60a6b 100644 --- a/orion/src/buck_controller.rs +++ b/orion/src/buck_controller.rs @@ -1,5 +1,5 @@ use std::{ - collections::{HashMap, HashSet}, + collections::HashMap, error::Error, io::BufReader, path::{Path, PathBuf}, @@ -20,7 +20,7 @@ use td_util_buck::{ run::{Buck2, targets_arguments}, target_status::{BuildState, EVENT_LOG_FILE, Event, LogicalActionId, TargetBuildStatusUpdate}, targets::Targets, - types::{CellPath, TargetLabel}, + types::TargetLabel, }; use tokio::{ io::AsyncBufReadExt, @@ -290,10 +290,10 @@ fn get_repo_targets(file_name: &str, repo_path: &Path) -> anyhow::Result>, ) -> anyhow::Result> { tracing::info!("Get cells at {:?}", mount_point); @@ -318,18 +318,7 @@ async fn get_build_targets( let base = get_repo_targets("base.jsonl", &old_repo)?; let diff = get_repo_targets("diff.jsonl", &mount_path)?; - let known_paths = collect_known_change_paths(&base, &diff); - let old_repo_root = repo_root_for_project_root(&old_repo, repo_prefix); - let new_repo_root = repo_root_for_project_root(&mount_path, repo_prefix); - let normalized_changes = normalize_changes_for_repo_prefix( - &cells, - repo_prefix, - &old_repo_root, - &new_repo_root, - &known_paths, - mega_changes, - ); - let changes = Changes::new(&cells, normalized_changes)?; + let changes = Changes::new(&cells, mega_changes)?; tracing::debug!("Changes {changes:?}"); tracing::debug!("Base targets number: {}", base.len_targets_upperbound()); @@ -345,162 +334,6 @@ async fn get_build_targets( .collect()) } -fn collect_known_change_paths(base: &Targets, diff: &Targets) -> HashSet { - let mut known_paths = HashSet::new(); - - for targets in [base, diff] { - for target in targets.targets() { - known_paths.extend(target.inputs.iter().cloned()); - } - for import in targets.imports() { - known_paths.insert(import.file.clone()); - known_paths.extend(import.imports.iter().cloned()); - } - } - - known_paths -} - -fn repo_root_for_project_root(project_root: &Path, repo_prefix: &str) -> PathBuf { - let mut repo_root = project_root.to_path_buf(); - for _ in repo_prefix - .trim_matches('/') - .split('/') - .filter(|segment| !segment.is_empty()) - { - repo_root = repo_root - .parent() - .map(Path::to_path_buf) - .unwrap_or(repo_root); - } - repo_root -} - -fn normalize_changes_for_repo_prefix( - cells: &CellInfo, - repo_prefix: &str, - old_repo_root: &Path, - new_repo_root: &Path, - known_paths: &HashSet, - mega_changes: Vec>, -) -> Vec> { - let normalized_prefix = repo_prefix.trim_matches('/'); - let mut normalized_changes = Vec::new(); - let mut seen = HashSet::new(); - - for status in mega_changes { - let candidates = normalize_change_path_candidates( - cells, - normalized_prefix, - old_repo_root, - new_repo_root, - known_paths, - status.get(), - ); - for candidate in candidates { - let normalized_status = status_with_path(&status, candidate); - if seen.insert(normalized_status.clone()) { - normalized_changes.push(normalized_status); - } - } - } - - normalized_changes -} - -fn normalize_change_path_candidates( - cells: &CellInfo, - repo_prefix: &str, - old_repo_root: &Path, - new_repo_root: &Path, - known_paths: &HashSet, - path: &ProjectRelativePath, -) -> Vec { - let raw_path = path.as_str().trim_start_matches('/'); - if repo_prefix.is_empty() - || raw_path == repo_prefix - || raw_path.starts_with(&format!("{repo_prefix}/")) - { - return vec![ProjectRelativePath::new(raw_path)]; - } - - let prefixed_path = format!("{repo_prefix}/{raw_path}"); - let raw_matches = path_matches_repo(cells, known_paths, old_repo_root, new_repo_root, raw_path); - let prefixed_matches = path_matches_repo( - cells, - known_paths, - old_repo_root, - new_repo_root, - &prefixed_path, - ); - - if raw_matches && prefixed_matches { - tracing::warn!( - raw_path, - prefixed_path, - repo_prefix, - "Change path matches both repo-relative and subproject-relative candidates; keeping both" - ); - } - - select_change_path_candidates(raw_path, &prefixed_path, raw_matches, prefixed_matches) -} - -fn path_matches_repo( - cells: &CellInfo, - known_paths: &HashSet, - old_repo_root: &Path, - new_repo_root: &Path, - relative_path: &str, -) -> bool { - path_exists_in_repo(old_repo_root, relative_path) - || path_exists_in_repo(new_repo_root, relative_path) - || path_matches_known_targets(cells, known_paths, relative_path) -} - -fn path_exists_in_repo(repo_root: &Path, relative_path: &str) -> bool { - repo_root.join(relative_path).exists() -} - -fn path_matches_known_targets( - cells: &CellInfo, - known_paths: &HashSet, - relative_path: &str, -) -> bool { - cells - .unresolve(&ProjectRelativePath::new(relative_path)) - .ok() - .is_some_and(|cell_path| known_paths.contains(&cell_path)) -} - -fn select_change_path_candidates( - raw_path: &str, - prefixed_path: &str, - raw_matches: bool, - prefixed_matches: bool, -) -> Vec { - match (raw_matches, prefixed_matches) { - (false, true) => vec![ProjectRelativePath::new(prefixed_path)], - (true, false) => vec![ProjectRelativePath::new(raw_path)], - (true, true) => vec![ - ProjectRelativePath::new(raw_path), - ProjectRelativePath::new(prefixed_path), - ], - (false, false) => vec![ProjectRelativePath::new(raw_path)], - } -} - -fn status_with_path( - status: &Status, - path: ProjectRelativePath, -) -> Status { - match status { - Status::Modified(_) => Status::Modified(path), - Status::Added(_) => Status::Added(path), - Status::Removed(_) => Status::Removed(path), - } -} - #[derive(Debug)] pub struct BuildStatusTracker { pub cancellation: CancellationToken, @@ -708,9 +541,10 @@ pub async fn build( // e.g., repo="/project/git-internal/git-internal" → repo_prefix="project/git-internal/git-internal" let repo_prefix = repo.strip_prefix('/').unwrap_or(&repo); - // Buck2 still resolves cells and target inputs relative to the monorepo root - // even when commands run from a sub-project directory, so normalize incoming - // change paths against `repo_prefix` before target discovery. + // Task build changes are standardized as monorepo-root-relative paths at the + // protocol boundary, even when the selected build repo is a sub-project. + // That lets target discovery pass changes straight into Buck2 without + // guessing whether a path was expressed relative to `repo_prefix`. const MAX_TARGETS_ATTEMPTS: usize = 2; let mut mount_point = None; @@ -750,7 +584,6 @@ pub async fn build( match get_build_targets( old_project_root.to_str().unwrap_or(&old_repo_mount_point), new_project_root.to_str().unwrap_or(&repo_mount_point), - repo_prefix, changes.clone(), ) .await @@ -931,18 +764,15 @@ pub async fn build( #[cfg(test)] mod tests { use std::{ - collections::HashSet, fs, path::{Path, PathBuf}, }; use api_model::buck2::{status::Status, types::ProjectRelativePath}; use serial_test::serial; - use td_util_buck::{cells::CellInfo, types::TargetLabel}; + use td_util_buck::types::TargetLabel; - use super::{ - get_build_targets, normalize_change_path_candidates, select_change_path_candidates, - }; + use super::get_build_targets; struct JsonlCleanupGuard { paths: Vec, @@ -979,95 +809,9 @@ mod tests { path.exists() } - #[test] - fn test_select_change_path_candidates_prefixes_subproject_relative_paths() { - let normalized = select_change_path_candidates( - "src/access_token.rs", - "jupiter/callisto/src/access_token.rs", - false, - true, - ); - - assert_eq!( - normalized, - vec![ProjectRelativePath::new( - "jupiter/callisto/src/access_token.rs" - )] - ); - } - - #[test] - fn test_normalize_change_path_candidates_keeps_repo_relative_paths_idempotent() { - let normalized = normalize_change_path_candidates( - &CellInfo::testing(), - "jupiter/callisto", - &workspace_root(), - &workspace_root(), - &HashSet::new(), - &ProjectRelativePath::new("jupiter/callisto/src/access_token.rs"), - ); - - assert_eq!( - normalized, - vec![ProjectRelativePath::new( - "jupiter/callisto/src/access_token.rs" - )] - ); - } - - #[test] - fn test_select_change_path_candidates_keeps_unrelated_repo_relative_paths_unchanged() { - let normalized = select_change_path_candidates( - "common/src/lib.rs", - "jupiter/callisto/common/src/lib.rs", - true, - false, - ); - - assert_eq!( - normalized, - vec![ProjectRelativePath::new("common/src/lib.rs")] - ); - } - - #[test] - fn test_normalize_change_path_candidates_keeps_existing_repo_relative_paths() { - let normalized = normalize_change_path_candidates( - &CellInfo::testing(), - "jupiter/callisto", - &workspace_root(), - &workspace_root(), - &HashSet::new(), - &ProjectRelativePath::new("common/src/lib.rs"), - ); - - assert_eq!( - normalized, - vec![ProjectRelativePath::new("common/src/lib.rs")] - ); - } - - #[test] - fn test_select_change_path_candidates_keeps_ambiguous_paths_as_both_candidates() { - let normalized = select_change_path_candidates( - "src/access_token.rs", - "jupiter/callisto/src/access_token.rs", - true, - true, - ); - - assert_eq!( - normalized, - vec![ - ProjectRelativePath::new("src/access_token.rs"), - ProjectRelativePath::new("jupiter/callisto/src/access_token.rs"), - ] - ); - } - #[tokio::test] #[serial] - async fn test_get_build_targets_detects_real_subproject_source_change() { + async fn test_get_build_targets_detects_root_relative_subproject_source_change() { let subproject_relative = "jupiter/callisto"; let subproject_root = subproject_root(subproject_relative); assert!( @@ -1084,9 +828,8 @@ mod tests { let targets = get_build_targets( subproject_root.to_str().expect("subproject root path"), subproject_root.to_str().expect("subproject root path"), - subproject_relative, vec![Status::Modified(ProjectRelativePath::new( - "src/access_token.rs", + "jupiter/callisto/src/access_token.rs", ))], ) .await @@ -1117,7 +860,6 @@ mod tests { let targets = get_build_targets( subproject_root.to_str().expect("subproject root path"), subproject_root.to_str().expect("subproject root path"), - subproject_relative, vec![Status::Modified(ProjectRelativePath::new( "orion/tests/fixtures/change_detector_mixed/shared/src/lib.rs", ))], @@ -1135,7 +877,7 @@ mod tests { #[tokio::test] #[serial] - async fn test_get_build_targets_handles_mixed_subproject_and_repo_relative_changes() { + async fn test_get_build_targets_handles_mixed_repo_root_relative_changes() { let subproject_relative = "orion/tests/fixtures/change_detector_mixed/app"; let subproject_root = subproject_root(subproject_relative); assert!( @@ -1152,9 +894,10 @@ mod tests { let targets = get_build_targets( subproject_root.to_str().expect("subproject root path"), subproject_root.to_str().expect("subproject root path"), - subproject_relative, vec![ - Status::Modified(ProjectRelativePath::new("README.md")), + Status::Modified(ProjectRelativePath::new( + "orion/tests/fixtures/change_detector_mixed/app/README.md", + )), Status::Modified(ProjectRelativePath::new( "orion/tests/fixtures/change_detector_mixed/shared/src/lib.rs", )),