From 282cd7b019378746cb87bd91a95d8b4bcae12aa3 Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 24 Sep 2026 12:56:09 +0000 Subject: [PATCH] Stabilize Rosetta test timing and retry delay telemetry (#47832) ## Why Cold Rosetta translation and cache invalidation from hard-linked executables can interfere with timed CLI tests. Sampling the clock twice also makes local retry telemetry report a delay shorter than the selected backoff. ## What changed - Warm CLI fixtures with `--version` on macOS x86_64 before timed checks, and use copies or Unix symlinks to avoid invalidating the shared executable's Rosetta cache. - Use one clock sample to calculate the response retry deadline and recorded delay. - Include the retry telemetry event in delay-range assertion failures for the headerless rate-limit stream test. GitOrigin-RevId: fc573bfff912e999cf74b94a1c6e7ae1be07974b --- codex-rs/cli/tests/app_server_daemon.rs | 16 ++++++++++++++++ codex-rs/cli/tests/daemon_startup.rs | 15 +++++++++++++++ codex-rs/cli/tests/doctor_path_safety.rs | 20 ++++++++++++++++++-- codex-rs/cli/tests/worktree.rs | 15 +++++++++++++++ codex-rs/core/src/responses_retry.rs | 8 ++++---- codex-rs/core/tests/suite/retry_after.rs | 5 ++++- 6 files changed, 72 insertions(+), 7 deletions(-) diff --git a/codex-rs/cli/tests/app_server_daemon.rs b/codex-rs/cli/tests/app_server_daemon.rs index 74151b41d8..a2626b04b2 100644 --- a/codex-rs/cli/tests/app_server_daemon.rs +++ b/codex-rs/cli/tests/app_server_daemon.rs @@ -37,6 +37,22 @@ impl TestDaemon { .join(&release_name) .join("bin/codex"); std::fs::create_dir_all(managed.parent().context("managed bin parent")?)?; + // Preserve the installed path without invalidating the shared CLI's Rosetta cache. + #[cfg(all(target_os = "macos", target_arch = "x86_64"))] + { + std::fs::copy(&codex_source, &managed)?; + // Translate the fixture before timed daemon capability and readiness checks. + ensure!( + Command::new(&managed) + .env("CODEX_HOME", home.path()) + .arg("--version") + .output()? + .status + .success(), + "failed to prepare managed test executable" + ); + } + #[cfg(not(all(target_os = "macos", target_arch = "x86_64")))] std::fs::hard_link(&codex_source, &managed) .or_else(|_| std::fs::copy(&codex_source, managed).map(|_| ()))?; std::fs::write(standalone.join("auto-update-version"), &release_name)?; diff --git a/codex-rs/cli/tests/daemon_startup.rs b/codex-rs/cli/tests/daemon_startup.rs index c8976c080c..828ccf1d11 100644 --- a/codex-rs/cli/tests/daemon_startup.rs +++ b/codex-rs/cli/tests/daemon_startup.rs @@ -73,6 +73,17 @@ async fn daemon_startup(command: &str) -> Result<()> { let home = tempfile::Builder::new().tempdir_in("/tmp")?; #[cfg(not(unix))] let home = tempfile::tempdir()?; + // Keep cold Rosetta translation outside the timed startup assertions. + #[cfg(all(target_os = "macos", target_arch = "x86_64"))] + anyhow::ensure!( + Command::new(&codex) + .env("CODEX_HOME", home.path()) + .arg("--version") + .output()? + .status + .success(), + "failed to prepare CLI test executable" + ); fs::write( home.path().join("config.toml"), format!( @@ -124,6 +135,10 @@ async fn daemon_startup(command: &str) -> Result<()> { .path() .join("packages/app-server-daemon/current/bin/codex"); fs::create_dir_all(home.path().join("packages/app-server-daemon/current/bin"))?; + // Hard links change the executable's ctime and invalidate Rosetta's translation cache. + #[cfg(unix)] + std::os::unix::fs::symlink(&codex, &managed)?; + #[cfg(not(unix))] fs::hard_link(&codex, &managed).or_else(|_| fs::copy(&codex, &managed).map(|_| ()))?; fs::create_dir(home.path().join("app-server-daemon"))?; fs::write( diff --git a/codex-rs/cli/tests/doctor_path_safety.rs b/codex-rs/cli/tests/doctor_path_safety.rs index 95795935bf..db2df64a82 100644 --- a/codex-rs/cli/tests/doctor_path_safety.rs +++ b/codex-rs/cli/tests/doctor_path_safety.rs @@ -34,22 +34,38 @@ struct Fixture { impl Fixture { fn new() -> Result { let root = TempDir::new()?; + let home = root.path().join("home"); + std::fs::create_dir(&home)?; // Cargo-built paths deliberately ignore npm provenance. Launch outside // target/ so this fixture also exercises the packaged-install checks. let program = root .path() .join(format!("codex{}", std::env::consts::EXE_SUFFIX)); let source = codex_utils_cargo_bin::cargo_bin("codex")?; + // Hard-link setup and teardown invalidate other tests' Rosetta translations. + #[cfg(all(target_os = "macos", target_arch = "x86_64"))] + { + std::fs::copy(&source, &program)?; + // Translate the fixture before the timed diagnostic command. + anyhow::ensure!( + std::process::Command::new(&program) + .env("CODEX_HOME", &home) + .arg("--version") + .output()? + .status + .success(), + "failed to prepare diagnostic test executable" + ); + } + #[cfg(not(all(target_os = "macos", target_arch = "x86_64")))] if std::fs::hard_link(&source, &program).is_err() { std::fs::copy(&source, &program)?; } let workspace = root.path().join("workspace"); let bin = workspace.join("node_modules/.bin"); - let home = root.path().join("home"); let marker = root.path().join("helper-ran"); std::fs::create_dir_all(&bin)?; std::fs::create_dir_all(workspace.join(".git"))?; - std::fs::create_dir(&home)?; std::fs::write( home.join("config.toml"), r#" diff --git a/codex-rs/cli/tests/worktree.rs b/codex-rs/cli/tests/worktree.rs index fef13c27fa..1ec06aa306 100644 --- a/codex-rs/cli/tests/worktree.rs +++ b/codex-rs/cli/tests/worktree.rs @@ -234,6 +234,17 @@ trust_level = "trusted" codex_config::types::AuthCredentialsStoreMode::File, )?; let program = codex_utils_cargo_bin::cargo_bin("codex")?; + // Keep cold Rosetta translation outside the timed startup assertions. + #[cfg(all(target_os = "macos", target_arch = "x86_64"))] + anyhow::ensure!( + Command::new(&program) + .env("CODEX_HOME", &home) + .arg("--version") + .output()? + .status + .success(), + "failed to prepare CLI test executable" + ); let mut env: HashMap = std::env::vars().collect(); env.insert("CODEX_HOME".into(), home.display().to_string()); env.insert("CODEX_SQLITE_HOME".into(), home.display().to_string()); @@ -266,6 +277,10 @@ trust_level = "trusted" let bin = home.join("packages/app-server-daemon/current/bin"); fs::create_dir_all(&bin)?; let managed = bin.join(if cfg!(windows) { "codex.exe" } else { "codex" }); + // Hard links change the executable's ctime and invalidate Rosetta's translation cache. + #[cfg(unix)] + std::os::unix::fs::symlink(program.canonicalize()?, &managed)?; + #[cfg(not(unix))] fs::hard_link(&program, &managed).or_else(|_| fs::copy(&program, &managed).map(|_| ()))?; fs::create_dir(home.join("app-server-daemon"))?; fs::write( diff --git a/codex-rs/core/src/responses_retry.rs b/codex-rs/core/src/responses_retry.rs index 6094395754..64f5112382 100644 --- a/codex-rs/core/src/responses_retry.rs +++ b/codex-rs/core/src/responses_retry.rs @@ -132,10 +132,10 @@ pub(crate) async fn handle_response_stream_error( ) .await; } - let retry_at = retry_after - .map(RetryAfter::deadline) - .unwrap_or_else(|| Instant::now() + delay); - let delay = retry_at.saturating_duration_since(Instant::now()); + // Use one clock sample so local backoff telemetry retains the selected delay. + let now = Instant::now(); + let retry_at = retry_after.map(RetryAfter::deadline).unwrap_or(now + delay); + let delay = retry_at.saturating_duration_since(now); codex_client::record_retry!(retry_count, delay, operation); tokio::time::sleep_until(retry_at).await; return Ok(()); diff --git a/codex-rs/core/tests/suite/retry_after.rs b/codex-rs/core/tests/suite/retry_after.rs index 76b79f841e..bc4f1f0360 100644 --- a/codex-rs/core/tests/suite/retry_after.rs +++ b/codex-rs/core/tests/suite/retry_after.rs @@ -1103,7 +1103,10 @@ async fn sse_failure_without_retry_after_exhausts_stream_retries(code: &str) -> submit_user_input(&test, "exhaust the headerless rate-limited stream").await?; let retry = telemetry.next_retry().await; - assert!((FIRST_RETRY_MIN_DELAY..FIRST_RETRY_MAX_DELAY).contains(&retry.delay)); + assert!( + (FIRST_RETRY_MIN_DELAY..FIRST_RETRY_MAX_DELAY).contains(&retry.delay), + "{retry:?}", + ); assert_eq!( retry, RetryTelemetryEvent {