mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
Record the launched app-server executable identity in PID files (#43552)
## Why An installer can retarget the selected executable symlink or junction while an app server is running. Recording the launch-time identity preserves which binary was started after that path changes. ## What changed - Resolve the executable path before launching on all platforms and store an optional executable identity in app-server PID records. Continue accepting older records without this field and expose the identity only for an active process. - Use BLAKE3 for executable identity digests and make `ExecutableIdentity` serializable. - Schedule updater checks with an explicit deadline in `tokio::select!`, preserving persisted update intervals, settings-error retries, and termination handling. ## Testing Add a Unix test that verifies the recorded identity matches the resolved launch binary, differs from a retargeted symlink's binary, and remains optional when reading legacy PID records. GitOrigin-RevId: 4821130b9a5e75b417a8e39c583b30427a872ff8
This commit is contained in:
Generated
+1
-1
@@ -2299,6 +2299,7 @@ name = "codex-app-server-daemon"
|
||||
version = "0.0.0"
|
||||
dependencies = [
|
||||
"anyhow",
|
||||
"blake3",
|
||||
"codex-app-server-protocol",
|
||||
"codex-app-server-transport",
|
||||
"codex-http-client",
|
||||
@@ -2309,7 +2310,6 @@ dependencies = [
|
||||
"pretty_assertions",
|
||||
"serde",
|
||||
"serde_json",
|
||||
"sha2 0.10.9",
|
||||
"tempfile",
|
||||
"tokio",
|
||||
"tokio-tungstenite",
|
||||
|
||||
@@ -323,6 +323,7 @@ aws-types = "1"
|
||||
axum = { version = "0.8", default-features = false }
|
||||
base64 = "0.22.1"
|
||||
bitflags = "2.13.1"
|
||||
blake3 = "1.8.2"
|
||||
bm25 = "2.3.2"
|
||||
bytes = "1.10.1"
|
||||
chardetng = "0.1.17"
|
||||
|
||||
@@ -23,7 +23,7 @@ futures = { workspace = true }
|
||||
libc = { workspace = true }
|
||||
serde = { workspace = true, features = ["derive"] }
|
||||
serde_json = { workspace = true }
|
||||
sha2 = { workspace = true }
|
||||
blake3 = { workspace = true }
|
||||
tokio = { workspace = true, features = [
|
||||
"fs",
|
||||
"io-util",
|
||||
|
||||
@@ -5,6 +5,7 @@ use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
|
||||
use crate::managed_install::ExecutableIdentity;
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use anyhow::bail;
|
||||
@@ -39,6 +40,8 @@ pub(crate) struct PidBackend {
|
||||
struct PidRecord {
|
||||
pid: u32,
|
||||
process_start_time: String,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
executable_identity: Option<ExecutableIdentity>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
@@ -75,6 +78,17 @@ enum PidCommandKind {
|
||||
}
|
||||
|
||||
impl PidBackend {
|
||||
// Used by the stacked manual-update command to compare the running image.
|
||||
#[allow(dead_code)]
|
||||
pub(crate) async fn running_executable_identity(&self) -> Result<Option<ExecutableIdentity>> {
|
||||
match self.read_pid_file_state().await? {
|
||||
PidFileState::Running(record) if self.record_is_active(&record).await? => {
|
||||
Ok(record.executable_identity)
|
||||
}
|
||||
_ => Ok(None),
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn new(codex_bin: PathBuf, pid_file: PathBuf, remote_control_enabled: bool) -> Self {
|
||||
let lock_file = pid_file.with_extension("pid.lock");
|
||||
Self {
|
||||
|
||||
@@ -7,6 +7,7 @@ use super::PidCommandKind;
|
||||
use super::PidFileState;
|
||||
use super::PidRecord;
|
||||
use super::read_process_start_time;
|
||||
use crate::managed_install::executable_identity;
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use anyhow::bail;
|
||||
@@ -67,14 +68,17 @@ impl PidBackend {
|
||||
}
|
||||
}
|
||||
}
|
||||
// Pin the Windows image path across installer junction retargeting.
|
||||
#[cfg(windows)]
|
||||
// Pin the selected release across installer symlink/junction retargeting.
|
||||
let codex_bin = fs::canonicalize(&self.codex_bin)
|
||||
.await
|
||||
.unwrap_or_else(|_| self.codex_bin.clone());
|
||||
#[cfg(not(windows))]
|
||||
let codex_bin = &self.codex_bin;
|
||||
let mut command = Command::new(codex_bin);
|
||||
let launched_identity =
|
||||
if matches!(self.command_kind, super::PidCommandKind::AppServer { .. }) {
|
||||
executable_identity(&codex_bin).await.ok()
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let mut command = Command::new(&codex_bin);
|
||||
let stderr_log = match self.open_stderr_log().await {
|
||||
Ok(stderr_log) => stderr_log,
|
||||
Err(err) => {
|
||||
@@ -229,6 +233,7 @@ impl PidBackend {
|
||||
Ok(process_start_time) => PidRecord {
|
||||
pid,
|
||||
process_start_time,
|
||||
executable_identity: launched_identity,
|
||||
},
|
||||
Err(err) => {
|
||||
let _ = self.terminate_process(pid);
|
||||
|
||||
@@ -167,10 +167,12 @@ async fn stale_record_cleanup_preserves_replacement_record() {
|
||||
let stale = PidRecord {
|
||||
pid: 1,
|
||||
process_start_time: "old".to_string(),
|
||||
executable_identity: None,
|
||||
};
|
||||
let replacement = PidRecord {
|
||||
pid: 2,
|
||||
process_start_time: "new".to_string(),
|
||||
executable_identity: None,
|
||||
};
|
||||
tokio::fs::write(
|
||||
&pid_file,
|
||||
@@ -188,6 +190,62 @@ async fn stale_record_cleanup_preserves_replacement_record() {
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn pid_record_captures_the_resolved_launch_binary() {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
|
||||
let temp = TempDir::new().expect("temp dir");
|
||||
let original = temp.path().join("original-codex");
|
||||
let replacement = temp.path().join("replacement-codex");
|
||||
for (path, bytes) in [
|
||||
(&original, b"#!/bin/sh\nexec sleep 30\n".as_slice()),
|
||||
(
|
||||
&replacement,
|
||||
b"#!/bin/sh\n# replacement\nexec sleep 30\n".as_slice(),
|
||||
),
|
||||
] {
|
||||
std::fs::write(path, bytes).expect("binary");
|
||||
std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)).expect("executable");
|
||||
}
|
||||
let selected = temp.path().join("current-codex");
|
||||
std::os::unix::fs::symlink(&original, &selected).expect("selected binary");
|
||||
let backend = PidBackend::new(
|
||||
selected.clone(),
|
||||
temp.path().join("app-server.pid"),
|
||||
/*remote_control_enabled*/ false,
|
||||
);
|
||||
backend.start().await.expect("start daemon");
|
||||
let record: PidRecord =
|
||||
serde_json::from_slice(&std::fs::read(&backend.pid_file).expect("PID record"))
|
||||
.expect("parse PID record");
|
||||
assert_eq!(
|
||||
record.executable_identity,
|
||||
Some(
|
||||
crate::managed_install::executable_identity(&original)
|
||||
.await
|
||||
.expect("original digest")
|
||||
)
|
||||
);
|
||||
std::fs::remove_file(&selected).expect("remove link");
|
||||
std::os::unix::fs::symlink(&replacement, &selected).expect("retarget link");
|
||||
assert_ne!(
|
||||
record.executable_identity,
|
||||
Some(
|
||||
crate::managed_install::executable_identity(&selected)
|
||||
.await
|
||||
.expect("new digest")
|
||||
)
|
||||
);
|
||||
assert_eq!(
|
||||
serde_json::from_str::<PidRecord>(r#"{"pid":1,"processStartTime":"old"}"#)
|
||||
.expect("legacy PID record")
|
||||
.executable_identity,
|
||||
None
|
||||
);
|
||||
backend.stop().await.expect("stop daemon");
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn stop_reaps_untracked_app_server_child() {
|
||||
@@ -204,6 +262,7 @@ async fn stop_reaps_untracked_app_server_child() {
|
||||
let record = PidRecord {
|
||||
pid,
|
||||
process_start_time: read_process_start_time(pid).await.expect("start time"),
|
||||
executable_identity: None,
|
||||
};
|
||||
tokio::fs::write(
|
||||
&pid_file,
|
||||
@@ -270,6 +329,7 @@ async fn stopping_updater_signals_its_installer_process_group() {
|
||||
serde_json::to_vec(&PidRecord {
|
||||
pid,
|
||||
process_start_time: read_process_start_time(pid).await.expect("start time"),
|
||||
executable_identity: None,
|
||||
})
|
||||
.expect("serialize pid"),
|
||||
)
|
||||
@@ -302,6 +362,7 @@ async fn exited_unreaped_updater_is_reaped() {
|
||||
process_start_time: read_process_start_time(child.id())
|
||||
.await
|
||||
.expect("start time"),
|
||||
executable_identity: None,
|
||||
};
|
||||
let backend =
|
||||
PidBackend::new_update_loop(temp.path().join("codex"), temp.path().join("updater.pid"));
|
||||
@@ -404,6 +465,7 @@ async fn stale_creation_time_never_stops_reused_pid() {
|
||||
let record = PidRecord {
|
||||
pid: std::process::id(),
|
||||
process_start_time: "stale".into(),
|
||||
executable_identity: None,
|
||||
};
|
||||
tokio::fs::write(&backend.pid_file, serde_json::to_vec(&record).unwrap())
|
||||
.await
|
||||
@@ -431,6 +493,7 @@ async fn failed_updater_handoff_preserves_predecessor_record() {
|
||||
process_start_time: super::read_process_start_time(std::process::id())
|
||||
.await
|
||||
.unwrap(),
|
||||
executable_identity: None,
|
||||
};
|
||||
for record in [
|
||||
record.clone(),
|
||||
@@ -491,12 +554,14 @@ async fn updater_readiness_and_post_publication_failure_preserve_ownership() {
|
||||
process_start_time: super::read_process_start_time(pid)
|
||||
.await
|
||||
.expect("creation time"),
|
||||
executable_identity: None,
|
||||
};
|
||||
let predecessor = PidRecord {
|
||||
pid: std::process::id(),
|
||||
process_start_time: super::read_process_start_time(std::process::id())
|
||||
.await
|
||||
.expect("creation time"),
|
||||
executable_identity: None,
|
||||
};
|
||||
tokio::fs::write(&backend.pid_file, serde_json::to_vec(&successor).unwrap())
|
||||
.await
|
||||
@@ -595,6 +660,7 @@ fn inaccessible_reused_pid_is_stale_without_hiding_process_open_errors() {
|
||||
let record = PidRecord {
|
||||
pid: std::process::id(),
|
||||
process_start_time: "stale".into(),
|
||||
executable_identity: None,
|
||||
};
|
||||
assert!(
|
||||
crate::backend::windows::Process::open(record.pid)
|
||||
|
||||
@@ -42,6 +42,7 @@ impl PidBackend {
|
||||
let record = PidRecord {
|
||||
pid: std::process::id(),
|
||||
process_start_time: read_process_start_time(std::process::id()).await?,
|
||||
executable_identity: None,
|
||||
};
|
||||
self.start_inner(Some(record)).await?;
|
||||
Ok(())
|
||||
|
||||
@@ -8,8 +8,8 @@ use std::time::Duration;
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use anyhow::anyhow;
|
||||
use sha2::Digest;
|
||||
use sha2::Sha256;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
use tokio::fs;
|
||||
use tokio::process::Command;
|
||||
use tokio::time::timeout;
|
||||
@@ -127,7 +127,7 @@ pub(crate) async fn managed_codex_version(codex_bin: &Path) -> Result<String> {
|
||||
parse_codex_version(&stdout)
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
pub(crate) struct ExecutableIdentity {
|
||||
digest: [u8; 32],
|
||||
}
|
||||
@@ -141,7 +141,7 @@ pub(crate) async fn executable_identity(executable: &Path) -> Result<ExecutableI
|
||||
|
||||
pub(crate) fn executable_identity_from_bytes(bytes: &[u8]) -> ExecutableIdentity {
|
||||
ExecutableIdentity {
|
||||
digest: Sha256::digest(bytes).into(),
|
||||
digest: *blake3::hash(bytes).as_bytes(),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -22,7 +22,9 @@ use tokio::signal::unix::Signal;
|
||||
use tokio::signal::unix::SignalKind;
|
||||
#[cfg(unix)]
|
||||
use tokio::signal::unix::signal;
|
||||
use tokio::time::Instant;
|
||||
use tokio::time::sleep;
|
||||
use tokio::time::sleep_until;
|
||||
|
||||
use crate::Daemon;
|
||||
use crate::RestartIfRunningOutcome;
|
||||
@@ -65,57 +67,38 @@ pub(crate) async fn run(http_client_factory: HttpClientFactory) -> Result<()> {
|
||||
http_client_factory,
|
||||
ClientRouteClass::Other,
|
||||
);
|
||||
if sleep_or_terminate(INITIAL_UPDATE_DELAY, &mut terminate).await {
|
||||
return Ok(());
|
||||
}
|
||||
let daemon = Daemon::from_environment()?;
|
||||
let mut next_check = Instant::now() + INITIAL_UPDATE_DELAY;
|
||||
loop {
|
||||
match UpdaterSettings::load(&daemon.settings_file).await {
|
||||
Ok(settings) if !settings.auto_update_enabled => return Ok(()),
|
||||
Err(_) => {
|
||||
if sleep_or_terminate(Duration::from_secs(60), &mut terminate).await {
|
||||
return Ok(());
|
||||
tokio::select! {
|
||||
_ = sleep_until(next_check) => {
|
||||
match UpdaterSettings::load(&daemon.settings_file).await {
|
||||
Ok(settings) if !settings.auto_update_enabled => return Ok(()),
|
||||
Err(_) => {
|
||||
next_check = Instant::now() + Duration::from_secs(60);
|
||||
continue;
|
||||
}
|
||||
Ok(_) => {}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
Ok(_) => {}
|
||||
}
|
||||
// Failed successor cleanup leaves its PID published. The predecessor
|
||||
// must stop instead of installing again without ownership.
|
||||
#[cfg(windows)]
|
||||
updater.wait_for_ownership().await?;
|
||||
match update_once(&http, &running_updater_identity, &mut terminate).await {
|
||||
Ok(UpdateLoopControl::Continue) | Err(_) => {}
|
||||
Ok(UpdateLoopControl::Stop) => return Ok(()),
|
||||
}
|
||||
match wait_for_next_check(
|
||||
&daemon.settings_file,
|
||||
Duration::from_secs(60),
|
||||
&mut terminate,
|
||||
)
|
||||
.await
|
||||
{
|
||||
Ok(true) => return Ok(()),
|
||||
Ok(false) => {}
|
||||
Err(_) => {
|
||||
if sleep_or_terminate(Duration::from_secs(60), &mut terminate).await {
|
||||
return Ok(());
|
||||
// Failed successor cleanup leaves its PID published. The predecessor
|
||||
// must stop instead of installing again without ownership.
|
||||
#[cfg(windows)]
|
||||
updater.wait_for_ownership().await?;
|
||||
match update_once(&http, &running_updater_identity, &mut terminate).await {
|
||||
Ok(UpdateLoopControl::Continue) | Err(_) => {}
|
||||
Ok(UpdateLoopControl::Stop) => return Ok(()),
|
||||
}
|
||||
next_check = Instant::now() + match UpdaterSettings::load(&daemon.settings_file).await {
|
||||
Ok(settings) if !settings.auto_update_enabled => return Ok(()),
|
||||
Ok(settings) => settings.update_interval(Duration::from_secs(60)),
|
||||
Err(_) => Duration::from_secs(60),
|
||||
};
|
||||
}
|
||||
_ = terminate.recv() => return Ok(()),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn wait_for_next_check(
|
||||
settings_file: &Path,
|
||||
minute: Duration,
|
||||
terminate: &mut Signal,
|
||||
) -> Result<bool> {
|
||||
let settings = UpdaterSettings::load(settings_file).await?;
|
||||
Ok(!settings.auto_update_enabled
|
||||
|| sleep_or_terminate(settings.update_interval(minute), terminate).await)
|
||||
}
|
||||
|
||||
async fn sleep_or_terminate(duration: Duration, terminate: &mut Signal) -> bool {
|
||||
tokio::select! {
|
||||
_ = sleep(duration) => false,
|
||||
|
||||
@@ -3,16 +3,12 @@ use std::sync::Mutex;
|
||||
use std::time::Duration;
|
||||
|
||||
use pretty_assertions::assert_eq;
|
||||
#[cfg(unix)]
|
||||
use tempfile::TempDir;
|
||||
|
||||
use super::INSTALL_URL;
|
||||
use super::InstallerHttp;
|
||||
use super::InstallerResponse;
|
||||
use super::fetch_installer_script;
|
||||
use super::update_modes_for_identities;
|
||||
#[cfg(unix)]
|
||||
use super::wait_for_next_check;
|
||||
use crate::RestartMode;
|
||||
use crate::UpdaterRefreshMode;
|
||||
use crate::managed_install::executable_identity_from_bytes;
|
||||
@@ -42,43 +38,6 @@ fn changed_updater_forces_refresh_even_when_version_may_match() {
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn recurring_wait_reads_persisted_interval_and_enabled_state() {
|
||||
let temp = TempDir::new().expect("temp dir");
|
||||
let settings_file = temp.path().join("settings.json");
|
||||
tokio::fs::write(&settings_file, r#"{"updater":{"updateIntervalMinutes":2}}"#)
|
||||
.await
|
||||
.expect("write settings");
|
||||
let mut terminate = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate())
|
||||
.expect("install signal handler");
|
||||
let started = tokio::time::Instant::now();
|
||||
assert_eq!(
|
||||
tokio::time::timeout(
|
||||
Duration::from_secs(1),
|
||||
wait_for_next_check(&settings_file, Duration::from_millis(10), &mut terminate),
|
||||
)
|
||||
.await
|
||||
.expect("recurring wait should finish")
|
||||
.expect("load persisted settings"),
|
||||
false
|
||||
);
|
||||
assert!(started.elapsed() >= Duration::from_millis(20));
|
||||
|
||||
tokio::fs::write(
|
||||
&settings_file,
|
||||
r#"{"updater":{"autoUpdateEnabled":false,"updateIntervalMinutes":2}}"#,
|
||||
)
|
||||
.await
|
||||
.expect("disable updates");
|
||||
assert_eq!(
|
||||
wait_for_next_check(&settings_file, Duration::from_millis(10), &mut terminate)
|
||||
.await
|
||||
.expect("reload settings"),
|
||||
true
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn installer_fetch_uses_exact_url_and_preserves_bytes() {
|
||||
let script = b"#!/bin/sh\nprintf 'update bytes'\n".to_vec();
|
||||
|
||||
Reference in New Issue
Block a user