diff --git a/install.ps1 b/install.ps1 index 1617194f1..e51ff0b5b 100644 --- a/install.ps1 +++ b/install.ps1 @@ -104,9 +104,9 @@ function Stop-GitAiManagedProcesses { $pids = @($processes | Sort-Object ProcessId -Unique | Select-Object -ExpandProperty ProcessId) Write-Warning ("Stopping lingering git-ai processes: {0}" -f ($pids -join ', ')) - foreach ($pid in $pids) { + foreach ($managedPid in $pids) { try { - Stop-Process -Id $pid -Force -ErrorAction Stop + Stop-Process -Id $managedPid -Force -ErrorAction Stop } catch { } } @@ -521,6 +521,13 @@ try { Write-Warning "Warning: Failed to set up IDE/agent hooks. Please try running 'git-ai install-hooks' manually." } +# Best-effort restart so detached self-updates can bring the background service back. +try { + & $finalExe bg start *> $null +} catch { + Write-Warning "Warning: Failed to restart git-ai background service automatically." +} + # Update PATH so our shim takes precedence over any Git entries $skipPathUpdate = $env:GIT_AI_SKIP_PATH_UPDATE -eq '1' if ($skipPathUpdate) { diff --git a/src/commands/daemon.rs b/src/commands/daemon.rs index 868824884..cf62e11bb 100644 --- a/src/commands/daemon.rs +++ b/src/commands/daemon.rs @@ -181,14 +181,34 @@ fn handle_run(args: &[String]) -> Result<(), String> { .enable_all() .build() .map_err(|e| e.to_string())?; - runtime + let exit_action = runtime .block_on(async move { crate::daemon::run_daemon(config).await }) .map_err(|e| e.to_string())?; // Daemon is fully dead (lock released, sockets removed, threads joined). - // Now safe to self-update — install.sh can start a fresh daemon. + // Now safe to self-update — the daemon can decide whether a fresh instance + // should be started based on the shutdown reason and install outcome. + #[cfg(not(windows))] + let update_installed = crate::daemon::daemon_run_pending_self_update(); + + #[cfg(windows)] crate::daemon::daemon_run_pending_self_update(); + match exit_action { + crate::daemon::DaemonExitAction::Stop => {} + crate::daemon::DaemonExitAction::Restart => { + ensure_daemon_running(Duration::from_secs(5)).map(|_| ())?; + } + crate::daemon::DaemonExitAction::RestartAfterUpdate => { + #[cfg(not(windows))] + { + if update_installed { + ensure_daemon_running(Duration::from_secs(5)).map(|_| ())?; + } + } + } + } + Ok(()) } diff --git a/src/commands/upgrade.rs b/src/commands/upgrade.rs index 73e122f81..8694f860e 100644 --- a/src/commands/upgrade.rs +++ b/src/commands/upgrade.rs @@ -661,7 +661,7 @@ pub fn run_with_args(args: &[String]) { } fn run_impl(force: bool, background: bool) { - let config = config::Config::get(); + let config = config::Config::fresh(); let channel = config.update_channel(); let skip_install = background && config.auto_updates_disabled(); let _ = run_impl_with_url(force, config.api_base_url(), channel, skip_install); diff --git a/src/config.rs b/src/config.rs index 8629ee10d..6091dde2c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1691,10 +1691,10 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let git_ai = dir.path().join("git-ai"); fs::write(&git_ai, "fake-binary").unwrap(); - let git = dir.path().join("git"); + let _git = dir.path().join("git"); #[cfg(unix)] - fs::hard_link(&git_ai, &git).unwrap(); + fs::hard_link(&git_ai, &_git).unwrap(); #[cfg(unix)] - assert!(path_is_git_ai_binary(&git)); + assert!(path_is_git_ai_binary(&_git)); } } diff --git a/src/daemon.rs b/src/daemon.rs index bf4f7d1ac..6641f98ba 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -58,7 +58,7 @@ use std::io::{BufRead, BufReader, Read, Seek, SeekFrom, Write}; #[cfg(windows)] use std::os::windows::io::{AsRawHandle, FromRawHandle, IntoRawHandle}; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU8, AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; use std::time::{SystemTime, UNIX_EPOCH}; use tokio::sync::{Mutex as AsyncMutex, Notify, mpsc, oneshot}; @@ -3659,6 +3659,7 @@ struct ActorDaemonCoordinator { wrapper_states: Mutex>, wrapper_state_notify: Notify, shutting_down: AtomicBool, + shutdown_action: AtomicU8, shutdown_notify: Notify, shutdown_condvar: std::sync::Condvar, shutdown_condvar_mutex: Mutex<()>, @@ -3670,6 +3671,31 @@ struct WrapperStateEntry { received_at_ns: u128, } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum DaemonExitAction { + Stop, + Restart, + RestartAfterUpdate, +} + +impl DaemonExitAction { + fn as_u8(self) -> u8 { + match self { + Self::Stop => 0, + Self::Restart => 1, + Self::RestartAfterUpdate => 2, + } + } + + fn from_u8(value: u8) -> Self { + match value { + 1 => Self::Restart, + 2 => Self::RestartAfterUpdate, + _ => Self::Stop, + } + } +} + enum TracePayloadApplyOutcome { None, Applied(Box), @@ -3720,6 +3746,7 @@ impl ActorDaemonCoordinator { wrapper_states: Mutex::new(HashMap::new()), wrapper_state_notify: Notify::new(), shutting_down: AtomicBool::new(false), + shutdown_action: AtomicU8::new(DaemonExitAction::Stop.as_u8()), shutdown_notify: Notify::new(), shutdown_condvar: std::sync::Condvar::new(), shutdown_condvar_mutex: Mutex::new(()), @@ -3745,6 +3772,30 @@ impl ActorDaemonCoordinator { self.shutdown_condvar.notify_all(); } + fn request_stop(&self) { + self.shutdown_action + .store(DaemonExitAction::Stop.as_u8(), Ordering::SeqCst); + self.request_shutdown(); + } + + fn request_restart(&self) { + self.shutdown_action + .store(DaemonExitAction::Restart.as_u8(), Ordering::SeqCst); + self.request_shutdown(); + } + + fn request_restart_after_update(&self) { + self.shutdown_action.store( + DaemonExitAction::RestartAfterUpdate.as_u8(), + Ordering::SeqCst, + ); + self.request_shutdown(); + } + + fn shutdown_action(&self) -> DaemonExitAction { + DaemonExitAction::from_u8(self.shutdown_action.load(Ordering::SeqCst)) + } + async fn wait_for_shutdown(&self) { // Register the Notified future BEFORE checking the flag so that a // request_shutdown() racing between the check and the await cannot @@ -7341,7 +7392,7 @@ fn handle_control_connection_actor_reader( reader.get_mut().write_all(b"\n")?; reader.get_mut().flush()?; if shutdown_after_response { - coordinator.request_shutdown(); + coordinator.request_stop(); } } Ok(()) @@ -7635,7 +7686,7 @@ fn daemon_update_check_loop(coordinator: Arc, started_at match check_for_update_available() { Ok(DaemonUpdateCheckResult::UpdateReady) => { debug_log("daemon update check: newer version available, requesting shutdown"); - coordinator.request_shutdown(); + coordinator.request_restart_after_update(); return; } Ok(DaemonUpdateCheckResult::NoUpdate) => { @@ -7649,7 +7700,7 @@ fn daemon_update_check_loop(coordinator: Arc, started_at let uptime_ns = now_unix_nanos().saturating_sub(started_at_ns); if uptime_ns >= daemon_max_uptime_ns() { debug_log("daemon uptime exceeded max, requesting restart"); - coordinator.request_shutdown(); + coordinator.request_restart(); return; } } @@ -7660,7 +7711,7 @@ fn daemon_update_check_loop(coordinator: Arc, started_at /// On Unix the installer atomically replaces the binary via `mv`; on Windows /// the installer is spawned as a detached process that polls until the exe is /// unlocked. -pub(crate) fn daemon_run_pending_self_update() { +pub(crate) fn daemon_run_pending_self_update() -> bool { use crate::commands::upgrade::{ DaemonUpdateCheckResult, check_and_install_update_if_available, }; @@ -7668,17 +7719,20 @@ pub(crate) fn daemon_run_pending_self_update() { match check_and_install_update_if_available() { Ok(DaemonUpdateCheckResult::UpdateReady) => { debug_log("daemon self-update: installation completed successfully"); + true } Ok(DaemonUpdateCheckResult::NoUpdate) => { debug_log("daemon self-update: no update to install"); + false } Err(err) => { debug_log(&format!("daemon self-update: installation failed: {}", err)); + false } } } -pub async fn run_daemon(config: DaemonConfig) -> Result<(), GitAiError> { +pub(crate) async fn run_daemon(config: DaemonConfig) -> Result { sanitize_git_env_for_daemon(); disable_trace2_for_daemon_process(); config.ensure_parent_dirs()?; @@ -7771,7 +7825,7 @@ pub async fn run_daemon(config: DaemonConfig) -> Result<(), GitAiError> { remove_socket_if_exists(&config.control_socket_path)?; remove_pid_metadata(&config)?; - Ok(()) + Ok(coordinator.shutdown_action()) } fn checkpoint_control_timeout_uses_ci_or_test_budget() -> bool { @@ -8246,6 +8300,29 @@ mod tests { assert_eq!(normalized.get("example.txt"), carryover.get("example.txt")); } + #[test] + fn explicit_stop_overrides_prior_restart_intent() { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + + runtime.block_on(async { + let coordinator = ActorDaemonCoordinator::new(); + + coordinator.request_restart_after_update(); + assert_eq!( + coordinator.shutdown_action(), + DaemonExitAction::RestartAfterUpdate + ); + + coordinator.request_stop(); + + assert!(coordinator.is_shutting_down()); + assert_eq!(coordinator.shutdown_action(), DaemonExitAction::Stop); + }); + } + #[test] fn recent_working_log_snapshot_preserves_humans_on_restore() { use crate::authorship::attribution_tracker::LineAttribution; diff --git a/tests/daemon_mode.rs b/tests/daemon_mode.rs index f7e9f3229..e15a574cd 100644 --- a/tests/daemon_mode.rs +++ b/tests/daemon_mode.rs @@ -4045,6 +4045,87 @@ fn spawn_daemon_with_env(repo: &TestRepo, extra_env: &[(&str, String)]) -> Child panic!("daemon did not become ready"); } +/// When the daemon exceeds its configured max uptime, it should restart and +/// make a fresh control socket available. +#[test] +#[serial] +fn daemon_max_uptime_restarts_and_recovers() { + let repo = TestRepo::new_with_mode(GitTestMode::Wrapper); + + let mut child = spawn_daemon_with_env( + &repo, + &[ + ("GIT_AI_DAEMON_UPDATE_CHECK_INTERVAL", "1".to_string()), + ("GIT_AI_DAEMON_MAX_UPTIME_SECS", "5".to_string()), + ], + ); + + let original_pid = child.id(); + let deadline = std::time::Instant::now() + Duration::from_secs(30); + loop { + if let Some(status) = child.try_wait().expect("failed to poll daemon") { + assert!( + status.success(), + "daemon should exit cleanly before restart, got: {}", + status + ); + break; + } + if std::time::Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + panic!("daemon did not exit within 30s after exceeding max uptime"); + } + thread::sleep(Duration::from_millis(100)); + } + + let control_socket_path = daemon_control_socket_path(&repo); + let trace_socket_path = daemon_trace_socket_path(&repo); + let repo_workdir = repo_workdir_string(&repo); + let restarted_pid = loop { + if send_control_request( + &control_socket_path, + &ControlRequest::StatusFamily { + repo_working_dir: repo_workdir.clone(), + }, + ) + .is_ok() + && local_socket_connects_with_timeout(&trace_socket_path, DAEMON_TEST_PROBE_TIMEOUT) + .is_ok() + { + let daemon_config = DaemonConfig::from_home(&repo.daemon_home_path()); + let pid = read_daemon_pid(&daemon_config) + .expect("restarted daemon should publish pid metadata"); + break pid; + } + if std::time::Instant::now() >= deadline { + panic!("daemon did not restart within 30s after max uptime exit"); + } + thread::sleep(Duration::from_millis(100)); + }; + + assert_ne!( + restarted_pid, original_pid, + "daemon restart should produce a new process id" + ); + + let _ = send_control_request(&control_socket_path, &ControlRequest::Shutdown); + for _ in 0..200 { + if send_control_request( + &control_socket_path, + &ControlRequest::StatusFamily { + repo_working_dir: repo_workdir.clone(), + }, + ) + .is_err() + { + return; + } + thread::sleep(Duration::from_millis(25)); + } + panic!("restarted daemon did not shut down cleanly"); +} + /// Config patch JSON that enables version checks and auto-updates (they /// default to disabled in non-OSS debug builds). fn update_enabled_config_patch() -> String { @@ -4058,6 +4139,12 @@ fn update_enabled_config_patch() -> String { /// Verifies the daemon update check loop lifecycle: when a cached update is /// present and the check interval is short, the daemon should detect the /// pending update, request a graceful shutdown, and exit on its own. +/// +/// The test seeds only the on-disk "update available" cache entry; it does not +/// stand up a mock release server. The follow-on self-update attempt may +/// therefore fail after the daemon has already decided to shut down, so the +/// contract we care about here is the shutdown lifecycle rather than a +/// zero-exit install result. #[test] #[serial] fn daemon_update_check_loop_detects_cached_update_and_shuts_down() { @@ -4077,20 +4164,15 @@ fn daemon_update_check_loop_detects_cached_update_and_shuts_down() { // The daemon should self-shutdown after detecting the cached update. // With a 1-second interval the tick is clamped to 1s, so it should // exit within a few seconds. - let deadline = std::time::Instant::now() + Duration::from_secs(15); + let deadline = std::time::Instant::now() + Duration::from_secs(30); loop { - if let Some(status) = child.try_wait().expect("failed to poll daemon") { - assert!( - status.success(), - "daemon should exit cleanly after update-triggered shutdown, got: {}", - status - ); + if child.try_wait().expect("failed to poll daemon").is_some() { break; } if std::time::Instant::now() >= deadline { let _ = child.kill(); let _ = child.wait(); - panic!("daemon did not self-shutdown within 15s after detecting cached update"); + panic!("daemon did not self-shutdown within 30s after detecting cached update"); } thread::sleep(Duration::from_millis(100)); } diff --git a/tests/windows_install_script.rs b/tests/windows_install_script.rs index e8c802069..78a84327a 100644 --- a/tests/windows_install_script.rs +++ b/tests/windows_install_script.rs @@ -427,3 +427,16 @@ fn windows_git_extension_upgrade_requires_direct_git_ai_binary() { combined ); } + +#[test] +fn windows_install_script_does_not_shadow_reserved_pid_variable() { + let script = fs::read_to_string(install_script_path()).expect("failed to read install.ps1"); + assert!( + !script.contains("foreach ($pid in $pids)"), + "install.ps1 should not iterate with the reserved $PID variable name" + ); + assert!( + script.contains("foreach ($managedPid in $pids)"), + "install.ps1 should use a non-reserved loop variable for managed process ids" + ); +}