From b68a84ee8e46d7172308cf8a95a3374262db3f46 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 10 Feb 2026 19:07:01 -0800 Subject: [PATCH] Remove `deterministic_process_ids` feature to avoid duplicate `codex-core` builds (#11393) ## Why `codex-core` enabled `deterministic_process_ids` through a self dev-dependency. That forced a second feature-resolved build of the same crate, which increased compile time and test latency. ## What Changed - Removed the `deterministic_process_ids` feature from `codex-rs/core/Cargo.toml`. - Removed the self dev-dependency on `codex-core` that enabled that feature. - Removed the Bazel `deterministic_process_ids` crate feature for `codex-core`. - Added a test-only `AtomicBool` override in unified exec process-id allocation. - Added a test-support setter for that override and re-exported it from `codex-core`. - Enabled deterministic process IDs in integration tests via `core_test_support` ctor. ## Behavior - Production behavior remains random process IDs. - Unit tests remain deterministic via `cfg(test)`. - Integration tests remain deterministic via explicit test-support initialization. ## Validation - `just fmt` - `cargo test -p codex-core unified_exec::` - `cargo test -p codex-core --test all unified_exec -- --test-threads=1` - `cargo tree -p codex-core -e features` (verified the removed feature path) --- codex-rs/Cargo.lock | 2 +- codex-rs/core/BUILD.bazel | 2 +- codex-rs/core/Cargo.toml | 4 --- codex-rs/core/src/lib.rs | 2 ++ codex-rs/core/src/unified_exec/mod.rs | 5 +++ .../core/src/unified_exec/process_manager.rs | 35 ++++++++++++++++--- codex-rs/core/tests/common/Cargo.toml | 1 + codex-rs/core/tests/common/lib.rs | 6 ++++ 8 files changed, 47 insertions(+), 10 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 2c6a8904d..4e2877a2c 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1660,7 +1660,6 @@ dependencies = [ "codex-arg0", "codex-async-utils", "codex-client", - "codex-core", "codex-execpolicy", "codex-file-search", "codex-git", @@ -2639,6 +2638,7 @@ dependencies = [ "codex-protocol", "codex-utils-absolute-path", "codex-utils-cargo-bin", + "ctor 0.6.3", "futures", "notify", "pretty_assertions", diff --git a/codex-rs/core/BUILD.bazel b/codex-rs/core/BUILD.bazel index f1649b36c..6e5ce680a 100644 --- a/codex-rs/core/BUILD.bazel +++ b/codex-rs/core/BUILD.bazel @@ -6,7 +6,7 @@ codex_rust_crate( # TODO(mbolin): Eliminate the use of features in the version of the # rust_library() that is used by rust_binary() rules for release artifacts # such as the Codex CLI. - crate_features = ["deterministic_process_ids", "test-support"], + crate_features = ["test-support"], compile_data = glob( include = ["**"], exclude = [ diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index e84f61725..26e7f17cc 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -114,7 +114,6 @@ wildmatch = { workspace = true } zip = { workspace = true } [features] -deterministic_process_ids = [] test-support = [] @@ -150,9 +149,6 @@ keyring = { workspace = true, features = ["sync-secret-service"] } assert_cmd = { workspace = true } assert_matches = { workspace = true } codex-arg0 = { workspace = true } -codex-core = { path = ".", default-features = false, features = [ - "deterministic_process_ids", -] } codex-otel = { workspace = true, features = [ "disable-default-metrics-exporter", ] } diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 4479e24c3..85095bc5f 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -145,6 +145,8 @@ pub use file_watcher::FileWatcherEvent; pub use safety::get_platform_sandbox; pub use tools::spec::parse_tool_input_schema; pub use turn_metadata::build_turn_metadata_header; +#[cfg(any(test, feature = "test-support"))] +pub use unified_exec::set_deterministic_process_ids_for_tests; // Re-export the protocol types from the standalone `codex-protocol` crate so existing // `codex_core::protocol::...` references continue to work across the workspace. pub use codex_protocol::protocol; diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index c1c7b8707..bbc7790c6 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -42,6 +42,11 @@ mod head_tail_buffer; mod process; mod process_manager; +#[cfg(any(test, feature = "test-support"))] +pub fn set_deterministic_process_ids_for_tests(enabled: bool) { + process_manager::set_deterministic_process_ids_for_tests(enabled); +} + pub(crate) use errors::UnifiedExecError; pub(crate) use process::UnifiedExecProcess; diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 8576bf77c..674085009 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -5,6 +5,7 @@ use std::collections::HashSet; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; use tokio::sync::Notify; use tokio::sync::mpsc; use tokio::time::Duration; @@ -61,6 +62,32 @@ const UNIFIED_EXEC_ENV: [(&str, &str); 10] = [ ("CODEX_CI", "1"), ]; +#[cfg(any(test, feature = "test-support"))] +/// Test-only override for deterministic unified exec process IDs. +/// +/// In production builds this value should remain at its default (`false`) and +/// must not be toggled. +static FORCE_DETERMINISTIC_PROCESS_IDS: AtomicBool = AtomicBool::new(false); + +#[cfg(any(test, feature = "test-support"))] +pub(super) fn set_deterministic_process_ids_for_tests(enabled: bool) { + FORCE_DETERMINISTIC_PROCESS_IDS.store(enabled, Ordering::Relaxed); +} + +#[cfg(any(test, feature = "test-support"))] +fn deterministic_process_ids_forced_for_tests() -> bool { + FORCE_DETERMINISTIC_PROCESS_IDS.load(Ordering::Relaxed) +} + +#[cfg(not(any(test, feature = "test-support")))] +fn deterministic_process_ids_forced_for_tests() -> bool { + false +} + +fn should_use_deterministic_process_ids() -> bool { + cfg!(test) || deterministic_process_ids_forced_for_tests() +} + fn apply_unified_exec_env(mut env: HashMap) -> HashMap { for (key, value) in UNIFIED_EXEC_ENV { env.insert(key.to_string(), value.to_string()); @@ -85,10 +112,7 @@ impl UnifiedExecProcessManager { loop { let mut store = self.process_store.lock().await; - let process_id = if !cfg!(test) && !cfg!(feature = "deterministic_process_ids") { - // production mode → random - rand::rng().random_range(1_000..100_000).to_string() - } else { + let process_id = if should_use_deterministic_process_ids() { // test or deterministic mode let next = store .reserved_process_ids @@ -99,6 +123,9 @@ impl UnifiedExecProcessManager { .unwrap_or(1000); next.to_string() + } else { + // production mode → random + rand::rng().random_range(1_000..100_000).to_string() }; if store.reserved_process_ids.contains(&process_id) { diff --git a/codex-rs/core/tests/common/Cargo.toml b/codex-rs/core/tests/common/Cargo.toml index 1c76e5a16..1fb43bb76 100644 --- a/codex-rs/core/tests/common/Cargo.toml +++ b/codex-rs/core/tests/common/Cargo.toml @@ -15,6 +15,7 @@ codex-core = { workspace = true, features = ["test-support"] } codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-cargo-bin = { workspace = true } +ctor = { workspace = true } futures = { workspace = true } notify = { workspace = true } regex-lite = { workspace = true } diff --git a/codex-rs/core/tests/common/lib.rs b/codex-rs/core/tests/common/lib.rs index 3feadb072..0144eeec5 100644 --- a/codex-rs/core/tests/common/lib.rs +++ b/codex-rs/core/tests/common/lib.rs @@ -1,6 +1,7 @@ #![expect(clippy::expect_used)] use codex_utils_cargo_bin::CargoBinError; +use ctor::ctor; use tempfile::TempDir; use codex_core::CodexThread; @@ -17,6 +18,11 @@ pub mod streaming_sse; pub mod test_codex; pub mod test_codex_exec; +#[ctor] +fn enable_deterministic_unified_exec_process_ids_for_tests() { + codex_core::set_deterministic_process_ids_for_tests(true); +} + #[track_caller] pub fn assert_regex_match<'s>(pattern: &str, actual: &'s str) -> regex_lite::Captures<'s> { let regex = Regex::new(pattern).unwrap_or_else(|err| {