Skip to content

fix: enforce mount_path semantics and stabilize builds #1836

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
benjamin-747 merged 2 commits into from
Feb 1, 2026
Merged

fix: enforce mount_path semantics and stabilize builds #1836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6692372
fix: enforce mount_path semantics and stabilize builds
jerry609 Jan 23, 2026
2885c51
test(ceres): fix clippy lints in tests
jerry609 Jan 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 27 additions & 27 deletions ceres/src/api_service/buck_tree_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,15 +762,15 @@ mod tests {

// Find direct children in dir_trees (paths whose parent == "a")
for (child_path, child_tree) in &dir_trees {
if let Some(parent) = child_path.parent() {
if parent == dir_path {
let child_name = child_path.file_name().unwrap().to_str().unwrap();
items.push(TreeItem {
mode: TreeItemMode::Tree,
id: child_tree.id,
name: child_name.to_string(),
});
}
if let Some(parent) = child_path.parent()
&& parent == dir_path
{
let child_name = child_path.file_name().unwrap().to_str().unwrap();
items.push(TreeItem {
mode: TreeItemMode::Tree,
id: child_tree.id,
name: child_name.to_string(),
});
}
}

Expand Down Expand Up @@ -803,15 +803,15 @@ mod tests {

// Find direct children in dir_trees (paths whose parent == "")
for (child_path, child_tree) in &dir_trees {
if let Some(parent) = child_path.parent() {
if parent.as_os_str().is_empty() {
let child_name = child_path.file_name().unwrap().to_str().unwrap();
items.push(TreeItem {
mode: TreeItemMode::Tree,
id: child_tree.id,
name: child_name.to_string(),
});
}
if let Some(parent) = child_path.parent()
&& parent.as_os_str().is_empty()
{
let child_name = child_path.file_name().unwrap().to_str().unwrap();
items.push(TreeItem {
mode: TreeItemMode::Tree,
id: child_tree.id,
name: child_name.to_string(),
});
}
}

Expand Down Expand Up @@ -880,15 +880,15 @@ mod tests {

// Simulate the child directory linking logic
for (child_path, child_tree) in &dir_trees {
if let Some(parent) = child_path.parent() {
if parent == current_dir.as_path() {
let child_name = child_path.file_name().unwrap().to_str().unwrap();
items.push(TreeItem {
mode: TreeItemMode::Tree,
id: child_tree.id,
name: child_name.to_string(),
});
}
if let Some(parent) = child_path.parent()
&& parent == current_dir.as_path()
{
let child_name = child_path.file_name().unwrap().to_str().unwrap();
items.push(TreeItem {
mode: TreeItemMode::Tree,
id: child_tree.id,
name: child_name.to_string(),
});
}
}

Expand Down
8 changes: 3 additions & 5 deletions ceres/src/api_service/mono_api_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2949,12 +2949,12 @@ mod test {
let commit_hash = ObjectHash::Sha1([3u8; 20]);

let item1 = TreeItem {
id: id1.clone(),
id: id1,
name: "file1.txt".into(),
mode: TreeItemMode::Blob,
};
let item2 = TreeItem {
id: id2.clone(),
id: id2,
name: "file2.txt".into(),
mode: TreeItemMode::Blob,
};
Expand All @@ -2976,7 +2976,7 @@ mod test {
};

let commit_a = Commit {
id: commit_hash.clone(),
id: commit_hash,
tree_id: ObjectHash::Sha1([8u8; 20]),
parent_commit_ids: vec![],
author: fake_sig.clone(),
Expand Down Expand Up @@ -3389,7 +3389,6 @@ mod test {
let diff_output: Vec<DiffItem> =
GitDiff::diff(old_blobs, new_blobs, Vec::new(), read_content)
.into_iter()
.map(DiffItem::from)
.collect();

// Verify diff output contains expected content
Expand Down Expand Up @@ -3437,7 +3436,6 @@ mod test {
let diff_output: Vec<DiffItem> =
GitDiff::diff(old_blobs, new_blobs, Vec::new(), read_content)
.into_iter()
.map(DiffItem::from)
.collect();

assert!(
Expand Down
4 changes: 1 addition & 3 deletions ceres/src/merge_checker/gpg_signature_checker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ F5MtAwnDBeT2Qg==
=Q/C5
-----END PGP PUBLIC KEY BLOCK-----
"#;
let pub_key = SignedPublicKey::from_string(&pk)
let pub_key = SignedPublicKey::from_string(pk)
.expect("unable to parse key")
.0;
let sig = StandaloneSignature::from_string(&sig)
Expand All @@ -280,6 +280,4 @@ F5MtAwnDBeT2Qg==
sig.signature
.verify(&pub_key, bytes)
.expect("unable to verify");

assert!(true);
}
2 changes: 1 addition & 1 deletion ceres/src/pack/monorepo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1085,7 +1085,7 @@ impl MonoRepo {

let req: OrionBuildRequest = OrionBuildRequest {
cl_link: link.clone(),
repo: path_str.to_string(),
mount_path: path_str.to_string(),
cl: cl_info.id,
builds: vec![BuildInfo {
changes: counter_changes,
Expand Down
32 changes: 21 additions & 11 deletions common/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,45 +986,55 @@ mod test {

#[test]
fn test_buck_config_validate_upload_limit_zero() {
let mut config = BuckConfig::default();
config.upload_concurrency_limit = 0;
let config = BuckConfig {
upload_concurrency_limit: 0,
..Default::default()
};
let result = config.validate();
assert!(result.is_err());
assert!(result.unwrap_err().contains("upload_concurrency_limit"));
}

#[test]
fn test_buck_config_validate_large_file_limit_zero() {
let mut config = BuckConfig::default();
config.large_file_concurrency_limit = 0;
let config = BuckConfig {
large_file_concurrency_limit: 0,
..Default::default()
};
let result = config.validate();
assert!(result.is_err());
assert!(result.unwrap_err().contains("large_file_concurrency_limit"));
}

#[test]
fn test_buck_config_validate_max_files_zero() {
let mut config = BuckConfig::default();
config.max_files = 0;
let config = BuckConfig {
max_files: 0,
..Default::default()
};
let result = config.validate();
assert!(result.is_err());
assert!(result.unwrap_err().contains("max_files"));
}

#[test]
fn test_buck_config_validate_session_timeout_zero() {
let mut config = BuckConfig::default();
config.session_timeout = 0;
let config = BuckConfig {
session_timeout: 0,
..Default::default()
};
let result = config.validate();
assert!(result.is_err());
assert!(result.unwrap_err().contains("session_timeout"));
}

#[test]
fn test_buck_config_validate_valid_values() {
let mut config = BuckConfig::default();
config.upload_concurrency_limit = 100;
config.large_file_concurrency_limit = 20;
let config = BuckConfig {
upload_concurrency_limit: 100,
large_file_concurrency_limit: 20,
..Default::default()
};
assert!(config.validate().is_ok());
}
}
6 changes: 3 additions & 3 deletions orion-server/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ Orion Server is a Buck2 build task scheduling service written in Rust. It provid
- **Request Body:**
```json
{
"repo": "string",
"mount_path": "string", // monorepo mount path (buck project root), e.g. "/" or "/third-party/mega"
"target": "string",
"args": ["string", ...], // optional
"cl": "string" // optional, Change List number
Expand All @@ -65,7 +65,7 @@ Orion Server is a Buck2 build task scheduling service written in Rust. It provid
curl -X POST http://localhost:8004/task \
-H "Content-Type: application/json" \
-d '{
"repo": "buck2-rust-third-party",
"mount_path": "/third-party/mega",
"target": "root//:rust-third-party",
"args": [""],
"cl": "123"
Expand All @@ -78,7 +78,7 @@ curl -X POST http://localhost:8004/task \
- **Request Body:**
```json
{
"repo": "string",
"mount_path": "string", // monorepo mount path (buck project root), e.g. "/" or "/third-party/mega"
"buck_hash": "string",
"buckconfig_hash": "string",
"args": ["string", ...], // optional
Expand Down
4 changes: 3 additions & 1 deletion orion-server/bellatrix/src/orion_client/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ pub struct BuildInfo {
#[derive(Serialize, Debug)]
pub struct OrionBuildRequest {
pub cl_link: String,
pub repo: String,
/// Monorepo mount path (Buck2 project root or subdirectory)
#[serde(rename = "mount_path", alias = "repo", alias = "path")]

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alias attribute in serde only works for deserialization. Since OrionBuildRequest is used for serialization (sending requests to Orion server), the alias attributes here have no effect. When this struct is serialized to JSON, only "mount_path" will be used as the field name. If backward compatibility is needed for clients consuming this serialized data, you need a different approach (like adding a deprecated field similar to BuildDTO). However, since this appears to be for internal API calls within the system, the aliases may be unnecessary.

Suggested change
#[serde(rename = "mount_path", alias = "repo", alias = "path")]

Copilot uses AI. Check for mistakes.
pub mount_path: String,
pub cl: i64,
pub builds: Vec<BuildInfo>,
}
Expand Down
2 changes: 1 addition & 1 deletion orion-server/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,12 @@ pub enum TaskStatusEnum {
/// Request structure for creating a task
#[derive(Debug, Clone, Deserialize, ToSchema)]
pub struct TaskRequest {
#[serde(rename = "repo", alias = "mount_path", alias = "path")]
pub repo: String,
Comment on lines +83 to 84

Copilot AI Feb 1, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The serde attribute is incorrect. According to the PR description, the field should be renamed to mount_path with repo as an alias for backward compatibility. Currently it's backwards - the field is named repo with mount_path as an alias. This should be:

#[serde(rename = "mount_path", alias = "repo", alias = "path")]
pub mount_path: String,

This inconsistency exists in contrast to OrionBuildRequest in orion-server/bellatrix/src/orion_client/mod.rs which correctly uses mount_path as the field name with repo as an alias.

Suggested change
#[serde(rename = "repo", alias = "mount_path", alias = "path")]
pub repo: String,
#[serde(rename = "mount_path", alias = "repo", alias = "path")]
pub mount_path: String,

Copilot uses AI. Check for mistakes.
pub cl_link: String,
pub cl: i64,
pub builds: Vec<scheduler::BuildRequest>,
}

/// Request structure for Retry a build
#[derive(Debug, Clone, Deserialize, ToSchema)]
pub struct RetryBuildRequest {
Expand Down
2 changes: 2 additions & 0 deletions orion-server/src/scheduler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ pub struct PendingTask {
pub task_id: Uuid,
pub cl_link: String,
pub build_id: Uuid,
/// Monorepo mount path (Buck2 project root or subdirectory)
pub repo: String,
pub target_id: Uuid,
pub target_path: String,
Expand Down Expand Up @@ -158,6 +159,7 @@ pub struct BuildInfo {
pub build_id: String,
pub target_id: String,
pub target_path: String,
/// Monorepo mount path (Buck2 project root or subdirectory)
pub repo: String,
pub start_at: DateTimeUtc,
pub changes: Vec<Status<ProjectRelativePath>>,
Expand Down
1 change: 1 addition & 0 deletions orion/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ thiserror = { workspace = true }
utoipa.workspace = true
wiremock = "0.6.5"
serial_test = { workspace = true }
tempfile = { workspace = true }
2 changes: 1 addition & 1 deletion orion/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{buck_controller, repo::sapling::status::Status, ws::WSMessage};
/// Parameters required to execute a buck build operation.
#[derive(Debug)]
pub struct BuildRequest {
/// Repository path or identifier
/// Monorepo mount path (Buck2 project root or subdirectory)
pub repo: String,
/// Change List identifier for context
pub cl: String,
Expand Down
Loading