mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
Classify daemon auto-start failures in TUI telemetry (#59058)
## What changed Add a `failure_reason` tag to failed TUI auto-start events in `codex.daemon.start`, distinguishing `install`, `spawn`, `early_child_exit`, `readiness_timeout`, and `other` failures. Preserve the original error diagnostics through a classified startup API. Detect managed child exits during process identity recording and distinguish exits before readiness from readiness timeouts. ## Testing Extend daemon tests to assert classification of installation failures, spawn failures, early child exits, and readiness timeouts. GitOrigin-RevId: 7c626f278e9e7f2ecd61b3fd1a90829a113eb232
This commit is contained in:
@@ -10,6 +10,7 @@ use super::PidFileState;
|
||||
use super::PidRecord;
|
||||
use super::read_process_start_time;
|
||||
use crate::managed_install::executable_identity;
|
||||
use crate::telemetry::StartFailure;
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use anyhow::bail;
|
||||
@@ -215,7 +216,7 @@ impl PidBackend {
|
||||
#[cfg(not(windows))]
|
||||
let child = command.spawn().map_err(anyhow::Error::from);
|
||||
let child = crate::diagnostics::result("process_spawn", started, child);
|
||||
let child = match child {
|
||||
let mut child = match child {
|
||||
Ok(child) => child,
|
||||
Err(err) => {
|
||||
if replacement.is_none() {
|
||||
@@ -227,10 +228,10 @@ impl PidBackend {
|
||||
} else {
|
||||
""
|
||||
};
|
||||
format!(
|
||||
StartFailure::Spawn(format!(
|
||||
"failed to spawn detached app-server process using {}{job_hint}",
|
||||
self.codex_bin.display()
|
||||
)
|
||||
))
|
||||
});
|
||||
}
|
||||
};
|
||||
@@ -259,14 +260,21 @@ impl PidBackend {
|
||||
let record = match crate::diagnostics::result("process_identity", started, record) {
|
||||
Ok(record) => record,
|
||||
Err(err) => {
|
||||
let _ = self.terminate_process(pid);
|
||||
let exited = managed_app_server && matches!(child.try_wait(), Ok(Some(_)));
|
||||
if !exited {
|
||||
let _ = self.terminate_process(pid);
|
||||
}
|
||||
let mut context =
|
||||
format!("failed to record pid-managed app-server process {pid} startup");
|
||||
super::super::append_stderr_log_tail_context(&self.pid_file, &mut context).await;
|
||||
if replacement.is_none() {
|
||||
let _ = fs::remove_file(&self.pid_file).await;
|
||||
}
|
||||
return Err(err).context(context);
|
||||
return if exited {
|
||||
Err(err).context(StartFailure::EarlyChildExit(context))
|
||||
} else {
|
||||
Err(err).context(context)
|
||||
};
|
||||
}
|
||||
};
|
||||
let contents = serde_json::to_vec(&record).context("failed to serialize pid record")?;
|
||||
|
||||
@@ -273,6 +273,10 @@ async fn start_retries_stale_empty_pid_file_under_its_own_lock() {
|
||||
err.to_string()
|
||||
.starts_with("failed to spawn detached app-server process using ")
|
||||
);
|
||||
assert_eq!(
|
||||
crate::telemetry::StartError::from(err).failure_reason(),
|
||||
"spawn"
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
|
||||
@@ -156,11 +156,19 @@ pub(crate) fn ensure_detached_launch(executable: &Path) -> Result<()> {
|
||||
command.creation_flags(CREATE_SUSPENDED | DETACHED_PROCESS);
|
||||
let child = match command.spawn() {
|
||||
Ok(child) => child,
|
||||
Err(_) => return Err(err).context("cannot launch detached daemon"),
|
||||
Err(_) => {
|
||||
return Err(err).context(crate::telemetry::StartFailure::Spawn(
|
||||
"cannot launch detached daemon".into(),
|
||||
));
|
||||
}
|
||||
};
|
||||
(child, Err(err).context(DetachedLaunchRestricted))
|
||||
}
|
||||
Err(err) => return Err(err).context("cannot launch detached daemon"),
|
||||
Err(err) => {
|
||||
return Err(err).context(crate::telemetry::StartFailure::Spawn(
|
||||
"cannot launch detached daemon".into(),
|
||||
));
|
||||
}
|
||||
};
|
||||
child
|
||||
.kill()
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
use crate::Daemon;
|
||||
use crate::LifecycleOutput;
|
||||
use crate::ensure_supported_platform;
|
||||
use crate::telemetry::StartError;
|
||||
use anyhow::Result;
|
||||
use anyhow::ensure;
|
||||
use std::collections::BTreeMap;
|
||||
@@ -13,6 +14,15 @@ use std::collections::BTreeMap;
|
||||
/// Callers must check the running server's configuration before using it.
|
||||
/// Startup diagnostics go to tracing, so a caller with a live TUI can keep rendering.
|
||||
pub async fn start_with_features(features: &BTreeMap<String, bool>) -> Result<LifecycleOutput> {
|
||||
start_with_features_classified(features)
|
||||
.await
|
||||
.map_err(|err| err.error)
|
||||
}
|
||||
|
||||
/// Starts the daemon and classifies failures for the TUI startup counter.
|
||||
pub async fn start_with_features_classified(
|
||||
features: &BTreeMap<String, bool>,
|
||||
) -> Result<LifecycleOutput, StartError> {
|
||||
ensure_supported_platform()?;
|
||||
#[cfg(windows)]
|
||||
crate::backend::windows::ensure_not_elevated()?;
|
||||
@@ -67,7 +77,7 @@ impl Daemon {
|
||||
let previous = settings.feature_overrides.clone();
|
||||
settings.feature_overrides.extend(features.clone());
|
||||
if settings.feature_overrides == previous {
|
||||
return self.start(&previous).await;
|
||||
return self.start(&previous).await.map_err(|err| err.error);
|
||||
}
|
||||
self.restart_with_settings(settings).await
|
||||
}
|
||||
|
||||
@@ -14,6 +14,7 @@ mod install_lock;
|
||||
mod launch;
|
||||
pub use launch::restart_with_features;
|
||||
pub use launch::start_with_features;
|
||||
pub use launch::start_with_features_classified;
|
||||
mod managed_install;
|
||||
mod prepare_install;
|
||||
mod release_pruning;
|
||||
@@ -46,6 +47,8 @@ use managed_install::managed_codex_version;
|
||||
use serde::Serialize;
|
||||
use settings::DaemonSettings;
|
||||
use settings::MAX_SHUTDOWN_GRACE_SECONDS;
|
||||
use telemetry::StartError;
|
||||
use telemetry::StartFailure;
|
||||
use tokio::time::sleep;
|
||||
|
||||
const START_POLL_INTERVAL: Duration = Duration::from_millis(50);
|
||||
@@ -420,7 +423,10 @@ impl Daemon {
|
||||
let _operation_lock = self.acquire_operation_lock().await?;
|
||||
let selected = self.current_installation()?;
|
||||
match command {
|
||||
LifecycleCommand::Start => selected.start(&BTreeMap::new()).await,
|
||||
LifecycleCommand::Start => selected
|
||||
.start(&BTreeMap::new())
|
||||
.await
|
||||
.map_err(|err| err.error),
|
||||
LifecycleCommand::Restart { feature_overrides } => {
|
||||
selected.restart(&feature_overrides).await
|
||||
}
|
||||
@@ -435,7 +441,10 @@ impl Daemon {
|
||||
}
|
||||
}
|
||||
|
||||
async fn start(&self, feature_overrides: &BTreeMap<String, bool>) -> Result<LifecycleOutput> {
|
||||
async fn start(
|
||||
&self,
|
||||
feature_overrides: &BTreeMap<String, bool>,
|
||||
) -> Result<LifecycleOutput, StartError> {
|
||||
let mut managed = self.clone();
|
||||
let mut settings = self.load_settings().await?;
|
||||
let (status, backend, pid, info) = if let Ok(info) = client::probe(&self.socket_path).await
|
||||
@@ -460,9 +469,15 @@ impl Daemon {
|
||||
"warning: failed to clear stale daemon recovery before start: {err}"
|
||||
));
|
||||
}
|
||||
prepare_install::prepare(self, &settings).await?;
|
||||
managed.managed_codex_bin = self.current_managed_codex_bin()?;
|
||||
managed.ensure_managed_codex_bin()?;
|
||||
prepare_install::prepare(self, &settings)
|
||||
.await
|
||||
.map_err(StartError::install)?;
|
||||
managed.managed_codex_bin = self
|
||||
.current_managed_codex_bin()
|
||||
.map_err(StartError::install)?;
|
||||
managed
|
||||
.ensure_managed_codex_bin()
|
||||
.map_err(StartError::install)?;
|
||||
// Only a fresh launch may replace these settings. Keep them for restarts
|
||||
// and updates, without changing the user's config or a running daemon.
|
||||
if settings.feature_overrides != *feature_overrides {
|
||||
@@ -705,19 +720,24 @@ impl Daemon {
|
||||
let exited = backend::wait_for_process_exit(&self.pid_file);
|
||||
tokio::pin!(exited);
|
||||
let deadline = tokio::time::Instant::now() + START_TIMEOUT;
|
||||
let err = loop {
|
||||
let (err, exited_before_ready) = loop {
|
||||
match client::probe(&self.socket_path).await {
|
||||
Ok(info) => return Ok(info),
|
||||
Err(err) if tokio::time::Instant::now() < deadline => {
|
||||
tokio::select! {
|
||||
() = &mut exited => break err,
|
||||
() = &mut exited => break (err, true),
|
||||
() = sleep(START_POLL_INTERVAL) => {}
|
||||
}
|
||||
}
|
||||
Err(err) => break err,
|
||||
Err(err) => break (err, false),
|
||||
}
|
||||
};
|
||||
Err(err).context(self.app_server_not_ready_context().await)
|
||||
let context = self.app_server_not_ready_context().await;
|
||||
Err(err).context(if exited_before_ready {
|
||||
StartFailure::EarlyChildExit(context)
|
||||
} else {
|
||||
StartFailure::ReadinessTimeout(context)
|
||||
})
|
||||
}
|
||||
|
||||
async fn app_server_not_ready_context(&self) -> String {
|
||||
@@ -754,7 +774,10 @@ impl Daemon {
|
||||
let _ = selected
|
||||
.set_remote_control_locked(RemoteControlMode::Enabled)
|
||||
.await?;
|
||||
let output = selected.start(&BTreeMap::new()).await?;
|
||||
let output = selected
|
||||
.start(&BTreeMap::new())
|
||||
.await
|
||||
.map_err(|err| err.error)?;
|
||||
return Ok(RemoteControlStartOutput::Start(output));
|
||||
}
|
||||
|
||||
@@ -1570,5 +1593,9 @@ mod tests {
|
||||
stderr_log.display()
|
||||
)
|
||||
);
|
||||
assert_eq!(
|
||||
super::StartError::from(error).failure_reason(),
|
||||
"early_child_exit"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -468,6 +468,8 @@ async fn broken_selection_is_not_a_missing_installation() {
|
||||
.to_string()
|
||||
.contains("repair the existing installation")
|
||||
);
|
||||
let start_error = daemon.start(&BTreeMap::new()).await.unwrap_err();
|
||||
assert_eq!(start_error.failure_reason(), "install");
|
||||
assert_eq!(
|
||||
std::fs::read_link(current).unwrap(),
|
||||
PathBuf::from("missing-release")
|
||||
|
||||
@@ -3,6 +3,63 @@
|
||||
|
||||
use std::path::Path;
|
||||
|
||||
/// Failure of a foreground auto-start, with the original diagnostic kept separate from the metric.
|
||||
#[derive(Debug)]
|
||||
pub struct StartError {
|
||||
pub error: anyhow::Error,
|
||||
during_install: bool,
|
||||
}
|
||||
|
||||
impl StartError {
|
||||
pub fn failure_reason(&self) -> &'static str {
|
||||
#[cfg(windows)]
|
||||
if self.error.is::<crate::DetachedLaunchRestricted>() {
|
||||
return "spawn";
|
||||
}
|
||||
match self.error.downcast_ref::<StartFailure>() {
|
||||
Some(StartFailure::Spawn(_)) => "spawn",
|
||||
Some(StartFailure::EarlyChildExit(_)) => "early_child_exit",
|
||||
Some(StartFailure::ReadinessTimeout(_)) => "readiness_timeout",
|
||||
None if self.during_install => "install",
|
||||
None => "other",
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn install(error: anyhow::Error) -> Self {
|
||||
Self {
|
||||
error,
|
||||
during_install: true,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl From<anyhow::Error> for StartError {
|
||||
fn from(error: anyhow::Error) -> Self {
|
||||
Self {
|
||||
error,
|
||||
during_install: false,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Typed anyhow context preserves both the original diagnostic and downcasts through it.
|
||||
#[derive(Debug)]
|
||||
pub(crate) enum StartFailure {
|
||||
Spawn(String),
|
||||
EarlyChildExit(String),
|
||||
ReadinessTimeout(String),
|
||||
}
|
||||
|
||||
impl std::fmt::Display for StartFailure {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
match self {
|
||||
Self::Spawn(message)
|
||||
| Self::EarlyChildExit(message)
|
||||
| Self::ReadinessTimeout(message) => f.write_str(message),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Suppress child reporting when the foreground TUI handoff owns the observation.
|
||||
pub const HANDOFF_ENV: &str = "CODEX_DAEMON_TELEMETRY_HANDOFF";
|
||||
|
||||
|
||||
@@ -839,6 +839,12 @@ async fn readiness_waits_for_a_slow_managed_daemon() {
|
||||
backend.start().await.unwrap();
|
||||
|
||||
tokio::time::pause();
|
||||
// The process is alive but has no listening socket throughout the first deadline.
|
||||
let error = daemon.wait_until_ready().await.unwrap_err();
|
||||
assert_eq!(
|
||||
crate::telemetry::StartError::from(error).failure_reason(),
|
||||
"readiness_timeout"
|
||||
);
|
||||
let (ready, listener) = tokio::join!(daemon.wait_until_ready(), async {
|
||||
// Model a cold start that takes longer than the old Linux readiness deadline.
|
||||
tokio::time::sleep(Duration::from_secs(15)).await;
|
||||
|
||||
@@ -25,7 +25,10 @@ impl<F: FnOnce(&'static str) -> LaunchTags> Drop for Launch<F> {
|
||||
|
||||
pub(super) async fn record_start(
|
||||
config: &Config,
|
||||
result: &anyhow::Result<codex_app_server_daemon::LifecycleOutput>,
|
||||
result: &Result<
|
||||
codex_app_server_daemon::LifecycleOutput,
|
||||
codex_app_server_daemon::telemetry::StartError,
|
||||
>,
|
||||
) {
|
||||
use codex_app_server_daemon::LifecycleStatus;
|
||||
let outcome = match result {
|
||||
@@ -59,6 +62,9 @@ pub(super) async fn record_start(
|
||||
("initiation_source", "tui_auto_start"),
|
||||
("outcome", outcome),
|
||||
]);
|
||||
if let Err(error) = result {
|
||||
tags.push(("failure_reason", error.failure_reason()));
|
||||
}
|
||||
let _ = metrics.counter("codex.daemon.start", /*inc*/ 1, &tags);
|
||||
}
|
||||
let _ = otel
|
||||
|
||||
@@ -614,9 +614,10 @@ pub(super) async fn run_main_inner(
|
||||
// responsive while it checks the running server or prepares an installation.
|
||||
// Preserve saved daemon settings until any mismatch is confirmed in recovery.
|
||||
let result =
|
||||
codex_app_server_daemon::start_with_features(&Default::default()).await;
|
||||
codex_app_server_daemon::start_with_features_classified(&Default::default())
|
||||
.await;
|
||||
daemon_telemetry::record_start(&config, &result).await;
|
||||
match result {
|
||||
match result.map_err(|err| err.error) {
|
||||
Ok(output) => Ok(Some(output)),
|
||||
#[cfg(windows)]
|
||||
Err(err) if err.is::<codex_app_server_daemon::DetachedLaunchRestricted>() => {
|
||||
|
||||
Reference in New Issue
Block a user