From 5634daefc60992265ea2cc1aaabb9c5e22ff2c18 Mon Sep 17 00:00:00 2001 From: Srikrishna Veturi Date: Tue, 1 Sep 2026 16:02:19 -0600 Subject: [PATCH 1/6] Report eBPFSvc, GuestProxyAgent service status, and surface eBPF/GPA errors immediately - Add eBPFSvc as a required member of the eBPF substatus alongside EbpfCore and NetEbpfExt (Windows only); the substatus is now Success only when all three services are Running. - Decouple eBPF and GuestProxyAgent service runtime-status checks onto their own ~2-minute polling cadence (SERVICE_STATUS_POLL_INTERVAL_SECS), independent of the 15s aggregate-status loop, while still refreshing the status file every 15s from cached results. - Add a new cross-platform ProxyAgentServiceStatus substatus reporting the GuestProxyAgent service's own runtime status, via a new proxy_agent_shared::service::check_service_run_status abstraction backed by the Windows SCM and by systemctl on Linux. - Immediately override the top-level extension status/message (bypassing the existing debounce state machine) whenever the eBPF substatus (Windows) or the GuestProxyAgent service substatus (both platforms) reports Error, including the last known status timestamp and current time, so operators see the real root cause instead of a generic stale-status message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- proxy_agent_extension/src/constants.rs | 7 + proxy_agent_extension/src/service_main.rs | 668 ++++++++++++++---- proxy_agent_shared/src/service.rs | 85 +++ .../src/service/linux_service.rs | 131 ++++ .../src/service/windows_service.rs | 37 + 5 files changed, 801 insertions(+), 127 deletions(-) diff --git a/proxy_agent_extension/src/constants.rs b/proxy_agent_extension/src/constants.rs index 1e8ffb24..99edeaaa 100644 --- a/proxy_agent_extension/src/constants.rs +++ b/proxy_agent_extension/src/constants.rs @@ -72,8 +72,15 @@ pub const MAX_TIME_BEFORE_STALE_STATUS_SECS: u64 = 5 * 60; pub const EBPF_CORE: &str = "EbpfCore"; pub const EBPF_EXT: &str = "NetEbpfExt"; +pub const EBPF_SVC: &str = "eBPFSvc"; pub const EBPF_SUBSTATUS_NAME: &str = "EbpfStatus"; +pub const PROXY_AGENT_SERVICE_SUBSTATUS_NAME: &str = "ProxyAgentServiceStatus"; + +// Cadence for polling the eBPF (Windows) and GuestProxyAgent (cross-platform) service +// runtime status, decoupled from the main monitor_thread loop_interval. +pub const SERVICE_STATUS_POLL_INTERVAL_SECS: u64 = 2 * 60; + pub const MAX_CONNECTION_SUMMARY_LEN: usize = 100; pub const MAX_FAILED_AUTH_SUMMARY_LEN: usize = 50; // Max KB of substatus string for connection summary and failed authentication summary diff --git a/proxy_agent_extension/src/service_main.rs b/proxy_agent_extension/src/service_main.rs index 7205b70f..d4e7227e 100644 --- a/proxy_agent_extension/src/service_main.rs +++ b/proxy_agent_extension/src/service_main.rs @@ -212,6 +212,19 @@ async fn monitor_thread() { let mut restored_in_error = false; let mut proxy_agent_update_reported: Option = None; let loop_interval = Duration::from_secs(15); + // Decoupled cache/cadence for the eBPF (Windows only) and GuestProxyAgent service + // (cross-platform) runtime status checks - these are (re)queried only every + // SERVICE_STATUS_POLL_INTERVAL_SECS (~2 minutes), independent of loop_interval, while the + // cached substatus is still re-appended to the status object (and written to the status + // file) on every loop_interval tick. + #[cfg(windows)] + let mut last_ebpf_substatus: Option = None; + let mut last_gpa_service_substatus: Option = None; + let mut last_service_status_poll: Option = None; + // Last known timestamp (as reported by the GPA aggregate status itself) used to annotate + // the immediate eBPF/GPA-service overrides below; kept from the previous iteration whenever + // the current iteration's fetch fails outright. + let mut last_known_status_timestamp = String::new(); loop { let current_seq_no: String = common::get_current_seq_no(&exe_path); @@ -287,13 +300,16 @@ async fn monitor_thread() { } // Step 3: Read and evaluate the proxy agent aggregate status - report_proxy_agent_aggregate_status( + if let Some(status_timestamp) = report_proxy_agent_aggregate_status( &proxy_agent_file_version_in_extension, &mut status, &mut status_state_obj, &mut service_state, ) - .await; + .await + { + last_known_status_timestamp = status_timestamp; + } // Step 4: Restore (on error) or purge (on success) the backed-up proxy agent, once if !restored_in_error { @@ -313,13 +329,59 @@ async fn monitor_thread() { proxy_agent_update_reported = None; } - // Step 6: Report eBPF driver status (Windows only) + // Step 6: Poll eBPF (Windows only) and GuestProxyAgent service (cross-platform) runtime + // status on a decoupled ~2-minute cadence, independent of loop_interval. The cached + // substatus values are re-appended every iteration so the status file (written every + // loop_interval) always reflects the latest known state. + if should_poll( + last_service_status_poll, + std::time::Instant::now(), + Duration::from_secs(constants::SERVICE_STATUS_POLL_INTERVAL_SECS), + ) { + #[cfg(windows)] + { + last_ebpf_substatus = Some(compute_ebpf_substatus()); + } + last_gpa_service_substatus = Some(compute_gpa_service_substatus()); + last_service_status_poll = Some(std::time::Instant::now()); + } #[cfg(windows)] - { - report_ebpf_status(&mut status); + if let Some(ebpf_substatus) = &last_ebpf_substatus { + status.substatus.push(ebpf_substatus.clone()); + } + if let Some(gpa_service_substatus) = &last_gpa_service_substatus { + status.substatus.push(gpa_service_substatus.clone()); } - // Step 7: Write the final status file and sleep + // Step 7: Apply immediate overrides when eBPF (Windows only, highest priority - an + // unhealthy eBPF is frequently the root cause of the GuestProxyAgent service failing to + // start) or the GuestProxyAgent service itself (both platforms) is reporting Error. + // These bypass the debounce state machine and take priority over whatever status/message + // Steps 3-5 produced (stale/version-mismatch/connectivity-error/success), because a + // definitively-known local service failure is a complete, actionable, immediate answer + // on its own - there is no reason to wait for slower generic detection to catch up. + #[cfg(windows)] + let overridden = match &last_ebpf_substatus { + Some(ebpf_substatus) => apply_ebpf_status_override( + &mut status, + ebpf_substatus, + &last_known_status_timestamp, + ), + None => false, + }; + #[cfg(not(windows))] + let overridden = false; + if !overridden { + if let Some(gpa_service_substatus) = &last_gpa_service_substatus { + apply_gpa_service_status_override( + &mut status, + gpa_service_substatus, + &last_known_status_timestamp, + ); + } + } + + // Step 8: Write the final status file and sleep common::report_status( status_folder_path.to_path_buf(), &cache_seq_no.to_string(), @@ -330,6 +392,19 @@ async fn monitor_thread() { } } +/// Returns true when a poll is due: either no poll has happened yet, or at least `interval` +/// has elapsed since the last one. Pure/testable helper for the decoupled service-status cadence. +fn should_poll( + last: Option, + now: std::time::Instant, + interval: Duration, +) -> bool { + match last { + None => true, + Some(last) => now.duration_since(last) >= interval, + } +} + fn write_state_event( state_key: &str, state_value: &str, @@ -354,58 +429,33 @@ fn write_state_event( fn build_ebpf_substatus( core: &proxy_agent_shared::service::ServiceStatusInfo, ext: &proxy_agent_shared::service::ServiceStatusInfo, + svc: &proxy_agent_shared::service::ServiceStatusInfo, ) -> SubStatus { use proxy_agent_shared::service::ServiceState; - let (status, code, message) = match (&core.state, &ext.state) { - (Some(core_state), Some(ext_state)) => { - let both_running = - *core_state == ServiceState::Running && *ext_state == ServiceState::Running; - if both_running { - ( - constants::SUCCESS_STATUS.to_string(), - constants::STATUS_CODE_OK, - format!( - "EbpfCore: {}, NetEbpfExt: {}", - core.summary(), - ext.summary() - ), - ) - } else { - ( - constants::ERROR_STATUS.to_string(), - constants::STATUS_CODE_NOT_OK, - format!( - "EbpfCore: {}, NetEbpfExt: {}", - core.summary(), - ext.summary() - ), - ) - } - } - (None, None) => ( - constants::ERROR_STATUS.to_string(), - constants::STATUS_CODE_NOT_OK, - "EbpfCore: unsuccessfully queried, NetEbpfExt: unsuccessfully queried.".to_string(), - ), - (None, _) => ( - constants::ERROR_STATUS.to_string(), - constants::STATUS_CODE_NOT_OK, - format!( - "EbpfCore: unsuccessfully queried, NetEbpfExt: {}", - ext.summary() - ), - ), - (_, None) => ( + let all_running = matches!(core.state, Some(ServiceState::Running)) + && matches!(ext.state, Some(ServiceState::Running)) + && matches!(svc.state, Some(ServiceState::Running)); + + let (status, code) = if all_running { + ( + constants::SUCCESS_STATUS.to_string(), + constants::STATUS_CODE_OK, + ) + } else { + ( constants::ERROR_STATUS.to_string(), constants::STATUS_CODE_NOT_OK, - format!( - "EbpfCore: {}, NetEbpfExt: unsuccessfully queried.", - core.summary() - ), - ), + ) }; + let message = format!( + "EbpfCore: {}, NetEbpfExt: {}, eBPFSvc: {}", + core.summary(), + ext.summary(), + svc.summary() + ); + SubStatus { name: constants::EBPF_SUBSTATUS_NAME.to_string(), status, @@ -418,16 +468,104 @@ fn build_ebpf_substatus( } #[cfg(windows)] -fn report_ebpf_status(status_obj: &mut StatusObj) { +fn compute_ebpf_substatus() -> SubStatus { let core_status = service::check_service_status(constants::EBPF_CORE); logger::write(format!("check_service_status: {}", core_status.message())); let ext_status = service::check_service_status(constants::EBPF_EXT); logger::write(format!("check_service_status: {}", ext_status.message())); - let mut substatus = status_obj.substatus.clone(); - substatus.push(build_ebpf_substatus(&core_status, &ext_status)); - status_obj.substatus = substatus; + let svc_status = service::check_service_status(constants::EBPF_SVC); + logger::write(format!("check_service_status: {}", svc_status.message())); + + build_ebpf_substatus(&core_status, &ext_status, &svc_status) +} + +/// Builds the cross-platform `ProxyAgentServiceStatus` substatus for the GuestProxyAgent +/// service itself (Windows SCM service or Linux systemd unit). +fn build_proxy_agent_service_substatus( + info: &proxy_agent_shared::service::ServiceRuntimeStatus, +) -> SubStatus { + let (status, code) = if info.is_running { + ( + constants::SUCCESS_STATUS.to_string(), + constants::STATUS_CODE_OK, + ) + } else { + ( + constants::ERROR_STATUS.to_string(), + constants::STATUS_CODE_NOT_OK, + ) + }; + + SubStatus { + name: constants::PROXY_AGENT_SERVICE_SUBSTATUS_NAME.to_string(), + status, + code, + formattedMessage: FormattedMessage { + lang: constants::LANG_EN_US.to_string(), + message: format!( + "{}: {}", + constants::PROXY_AGENT_SERVICE_NAME, + info.summary() + ), + }, + } +} + +fn compute_gpa_service_substatus() -> SubStatus { + let info = + proxy_agent_shared::service::check_service_run_status(constants::PROXY_AGENT_SERVICE_NAME); + logger::write(format!("check_service_run_status: {}", info.message())); + build_proxy_agent_service_substatus(&info) +} + +/// If `ebpf_substatus` reports Error, unconditionally overrides `status`'s top-level +/// status/code/message to surface the eBPF detail plus the last known status timestamp and the +/// current time. Bypasses the debounce state machine intentionally. Returns true if it applied +/// the override (used by the caller to give this priority over the GPA-service override). +#[cfg(windows)] +fn apply_ebpf_status_override( + status: &mut StatusObj, + ebpf_substatus: &SubStatus, + last_known_status_timestamp: &str, +) -> bool { + if ebpf_substatus.status != constants::ERROR_STATUS { + return false; + } + status.status = constants::ERROR_STATUS.to_string(); + status.code = constants::STATUS_CODE_NOT_OK; + status.formattedMessage.message = format!( + "{}. Last status timestamp: {}, Current time: {}", + ebpf_substatus.formattedMessage.message, + last_known_status_timestamp, + misc_helpers::get_current_utc_time() + ); + true +} + +/// If `gpa_service_substatus` reports Error, unconditionally overrides `status`'s top-level +/// status/code/message to surface the GuestProxyAgent service detail plus the last known status +/// timestamp and the current time. Bypasses the debounce state machine intentionally, mirroring +/// `apply_ebpf_status_override`. Cross-platform (Windows and Linux). Returns true if it applied +/// the override. +fn apply_gpa_service_status_override( + status: &mut StatusObj, + gpa_service_substatus: &SubStatus, + last_known_status_timestamp: &str, +) -> bool { + if gpa_service_substatus.status != constants::ERROR_STATUS { + return false; + } + status.status = constants::ERROR_STATUS.to_string(); + status.code = constants::STATUS_CODE_NOT_OK; + status.formattedMessage.message = format!( + "{}. Last status timestamp: {}, Current time: {}", + gpa_service_substatus.formattedMessage.message, + last_known_status_timestamp, + misc_helpers::get_current_utc_time() + ); + true } fn backup_proxy_agent(setup_tool: &String) { @@ -533,12 +671,15 @@ async fn get_proxy_agent_aggregate_status( } } +/// Reads and evaluates the proxy agent aggregate status, returning the raw status timestamp +/// (formatted) it observed when the fetch succeeded at all, or `None` when the fetch failed +/// outright (callers should keep whatever timestamp they last observed in that case). async fn report_proxy_agent_aggregate_status( proxy_agent_file_version_in_extension: &String, status: &mut StatusObj, status_state_obj: &mut common::StatusState, service_state: &mut ServiceState, -) { +) -> Option { let proxy_agent_aggregate_status_top_level: GuestProxyAgentAggregateStatus; // Attempt to get the proxy agent aggregate status from the GPA Proxy Server. // If the GPA Proxy Server is not available, fall back to reading the status from the file. @@ -566,6 +707,10 @@ async fn report_proxy_agent_aggregate_status( service_state, ); proxy_agent_aggregate_status_top_level = proxy_agent_aggregate_status; + let status_timestamp = proxy_agent_aggregate_status_top_level + .get_status_timestamp() + .ok() + .map(|ts| ts.to_string()); extension_substatus( proxy_agent_aggregate_status_top_level, proxy_agent_file_version_in_extension, @@ -573,6 +718,7 @@ async fn report_proxy_agent_aggregate_status( status_state_obj, service_state, ); + status_timestamp } Err(e) => { let error_message = format!("{e}"); @@ -617,6 +763,7 @@ async fn report_proxy_agent_aggregate_status( }, ] }; + None } } } @@ -1220,7 +1367,7 @@ mod tests { #[tokio::test] #[cfg(windows)] - async fn test_report_ebpf_status() { + async fn test_compute_ebpf_substatus() { let mut status = make_test_status_obj( constants::SUCCESS_STATUS, constants::STATUS_CODE_OK, @@ -1256,7 +1403,7 @@ mod tests { }, ]; - super::report_ebpf_status(&mut status); + status.substatus.push(super::compute_ebpf_substatus()); assert_eq!( status.substatus[0].name, constants::PLUGIN_CONNECTION_NAME.to_string() @@ -1274,42 +1421,28 @@ mod tests { constants::EBPF_SUBSTATUS_NAME.to_string() ); - // Verify the eBPF substatus message includes service status info + // Verify the eBPF substatus message includes all three services, and that status/code + // are internally consistent (adaptive to whatever eBPF-for-Windows state the test + // runner happens to have installed). let ebpf_substatus = &status.substatus[3]; let ebpf_message = &ebpf_substatus.formattedMessage.message; - if ebpf_message.contains("unsuccessfully queried") { - // At least one service not installed — status should be Error - assert_eq!( - ebpf_substatus.status, - constants::ERROR_STATUS, - "Expected Error status when a service is not installed" - ); + assert!( + ebpf_message.contains("EbpfCore:"), + "Expected message to contain 'EbpfCore:', got: {ebpf_message}" + ); + assert!( + ebpf_message.contains("NetEbpfExt:"), + "Expected message to contain 'NetEbpfExt:', got: {ebpf_message}" + ); + assert!( + ebpf_message.contains("eBPFSvc:"), + "Expected message to contain 'eBPFSvc:', got: {ebpf_message}" + ); + if ebpf_substatus.status == constants::SUCCESS_STATUS { + assert_eq!(ebpf_substatus.code, constants::STATUS_CODE_OK); } else { - // Both services found — message should contain status details for each driver - assert!( - ebpf_message.contains("EbpfCore:"), - "Expected message to contain 'EbpfCore:', got: {ebpf_message}" - ); - assert!( - ebpf_message.contains("NetEbpfExt:"), - "Expected message to contain 'NetEbpfExt:', got: {ebpf_message}" - ); - // Status depends on whether both services are running - if ebpf_message.contains("Running") && !ebpf_message.contains("Stopped") { - assert_eq!( - ebpf_substatus.status, - constants::SUCCESS_STATUS, - "Expected Success when both services are running" - ); - assert_eq!(ebpf_substatus.code, constants::STATUS_CODE_OK); - } else { - assert_eq!( - ebpf_substatus.status, - constants::ERROR_STATUS, - "Expected Error when at least one service is not running" - ); - assert_eq!(ebpf_substatus.code, constants::STATUS_CODE_NOT_OK); - } + assert_eq!(ebpf_substatus.status, constants::ERROR_STATUS); + assert_eq!(ebpf_substatus.code, constants::STATUS_CODE_NOT_OK); } } @@ -1331,23 +1464,61 @@ mod tests { } } - // 1. Both not installed + let running = || Some(ServiceState::Running); + let stopped = || Some(ServiceState::Stopped); + + // 1. All three not installed let sub = super::build_ebpf_substatus( &make_info(constants::EBPF_CORE, None), &make_info(constants::EBPF_EXT, None), + &make_info(constants::EBPF_SVC, None), ); - assert_eq!(sub.status, constants::ERROR_STATUS, "Both not installed"); + assert_eq!(sub.status, constants::ERROR_STATUS, "All not installed"); assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); let msg = &sub.formattedMessage.message; assert!( - msg.contains(constants::EBPF_CORE) && msg.contains(constants::EBPF_EXT), - "Expected both driver names in message, got: {msg}" + msg.contains(constants::EBPF_CORE) + && msg.contains(constants::EBPF_EXT) + && msg.contains(constants::EBPF_SVC), + "Expected all three service names in message, got: {msg}" ); - // 2. Core not installed, Ext running + // 2. Core+Ext running, eBPFSvc not installed → still Error (all three required) + let sub = super::build_ebpf_substatus( + &make_info(constants::EBPF_CORE, running()), + &make_info(constants::EBPF_EXT, running()), + &make_info(constants::EBPF_SVC, None), + ); + assert_eq!( + sub.status, + constants::ERROR_STATUS, + "eBPFSvc not installed should still be Error even if Core+Ext are healthy" + ); + assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); + let msg = &sub.formattedMessage.message; + assert!( + msg.contains("eBPFSvc: NotInstalled"), + "Expected eBPFSvc: NotInstalled in message, got: {msg}" + ); + + // 3. Core+Ext running, eBPFSvc stopped → Error + let sub = super::build_ebpf_substatus( + &make_info(constants::EBPF_CORE, running()), + &make_info(constants::EBPF_EXT, running()), + &make_info(constants::EBPF_SVC, stopped()), + ); + assert_eq!( + sub.status, + constants::ERROR_STATUS, + "eBPFSvc stopped should be Error even if Core+Ext are healthy" + ); + assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); + + // 4. Core not installed, Ext+Svc running → Error let sub = super::build_ebpf_substatus( &make_info(constants::EBPF_CORE, None), - &make_info(constants::EBPF_EXT, Some(ServiceState::Running)), + &make_info(constants::EBPF_EXT, running()), + &make_info(constants::EBPF_SVC, running()), ); assert_eq!(sub.status, constants::ERROR_STATUS, "Core not installed"); assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); @@ -1358,72 +1529,315 @@ mod tests { ); assert!( msg.contains("Running"), - "Expected Ext summary (Running) in message, got: {msg}" + "Expected Ext/Svc summary (Running) in message, got: {msg}" ); - // 3. Core running, Ext not installed + // 5. Ext not installed, Core+Svc running → Error let sub = super::build_ebpf_substatus( - &make_info(constants::EBPF_CORE, Some(ServiceState::Running)), + &make_info(constants::EBPF_CORE, running()), &make_info(constants::EBPF_EXT, None), + &make_info(constants::EBPF_SVC, running()), ); assert_eq!(sub.status, constants::ERROR_STATUS, "Ext not installed"); assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); - let msg = &sub.formattedMessage.message; - assert!( - msg.contains("Running"), - "Expected Core summary (Running) in message, got: {msg}" - ); - assert!( - msg.contains(constants::EBPF_EXT), - "Expected NetEbpfExt in message, got: {msg}" - ); - // 4. Both running → success + // 6. All three running → Success let sub = super::build_ebpf_substatus( - &make_info(constants::EBPF_CORE, Some(ServiceState::Running)), - &make_info(constants::EBPF_EXT, Some(ServiceState::Running)), + &make_info(constants::EBPF_CORE, running()), + &make_info(constants::EBPF_EXT, running()), + &make_info(constants::EBPF_SVC, running()), ); - assert_eq!(sub.status, constants::SUCCESS_STATUS, "Both running"); + assert_eq!(sub.status, constants::SUCCESS_STATUS, "All three running"); assert_eq!(sub.code, constants::STATUS_CODE_OK); let msg = &sub.formattedMessage.message; assert!( - msg.contains("EbpfCore:") && msg.contains("NetEbpfExt:"), - "Expected both driver labels in message, got: {msg}" + msg.contains("EbpfCore:") && msg.contains("NetEbpfExt:") && msg.contains("eBPFSvc:"), + "Expected all three driver labels in message, got: {msg}" ); - // 5. Core stopped, Ext running → error + // 7. Core stopped, Ext+Svc running → Error let sub = super::build_ebpf_substatus( - &make_info(constants::EBPF_CORE, Some(ServiceState::Stopped)), - &make_info(constants::EBPF_EXT, Some(ServiceState::Running)), + &make_info(constants::EBPF_CORE, stopped()), + &make_info(constants::EBPF_EXT, running()), + &make_info(constants::EBPF_SVC, running()), ); assert_eq!( sub.status, constants::ERROR_STATUS, - "Core stopped, Ext running" + "Core stopped, Ext+Svc running" ); assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); - // 6. Core running, Ext stopped → error + // 8. Core running, Ext stopped, Svc running → Error let sub = super::build_ebpf_substatus( - &make_info(constants::EBPF_CORE, Some(ServiceState::Running)), - &make_info(constants::EBPF_EXT, Some(ServiceState::Stopped)), + &make_info(constants::EBPF_CORE, running()), + &make_info(constants::EBPF_EXT, stopped()), + &make_info(constants::EBPF_SVC, running()), ); assert_eq!( sub.status, constants::ERROR_STATUS, - "Core running, Ext stopped" + "Core running, Ext stopped, Svc running" ); assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); - // 7. Both stopped → error + // 9. All three stopped → Error let sub = super::build_ebpf_substatus( - &make_info(constants::EBPF_CORE, Some(ServiceState::Stopped)), - &make_info(constants::EBPF_EXT, Some(ServiceState::Stopped)), + &make_info(constants::EBPF_CORE, stopped()), + &make_info(constants::EBPF_EXT, stopped()), + &make_info(constants::EBPF_SVC, stopped()), ); - assert_eq!(sub.status, constants::ERROR_STATUS, "Both stopped"); + assert_eq!(sub.status, constants::ERROR_STATUS, "All three stopped"); assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); } + #[test] + fn test_build_proxy_agent_service_substatus() { + use proxy_agent_shared::service::ServiceRuntimeStatus; + + // Running → Success + let info = ServiceRuntimeStatus { + service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), + is_installed: true, + is_running: true, + state_display: "Running".to_string(), + start_type_display: "AutoStart".to_string(), + }; + let sub = super::build_proxy_agent_service_substatus(&info); + assert_eq!(sub.name, constants::PROXY_AGENT_SERVICE_SUBSTATUS_NAME); + assert_eq!(sub.status, constants::SUCCESS_STATUS); + assert_eq!(sub.code, constants::STATUS_CODE_OK); + assert_eq!( + sub.formattedMessage.message, + format!( + "{}: Running, AutoStart", + constants::PROXY_AGENT_SERVICE_NAME + ) + ); + + // Stopped → Error + let info = ServiceRuntimeStatus { + service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), + is_installed: true, + is_running: false, + state_display: "Stopped".to_string(), + start_type_display: "AutoStart".to_string(), + }; + let sub = super::build_proxy_agent_service_substatus(&info); + assert_eq!(sub.status, constants::ERROR_STATUS); + assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); + assert_eq!( + sub.formattedMessage.message, + format!( + "{}: Stopped, AutoStart", + constants::PROXY_AGENT_SERVICE_NAME + ) + ); + + // Disabled (installed but not running, start type Disabled) → Error + let info = ServiceRuntimeStatus { + service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), + is_installed: true, + is_running: false, + state_display: "Stopped".to_string(), + start_type_display: "Disabled".to_string(), + }; + let sub = super::build_proxy_agent_service_substatus(&info); + assert_eq!(sub.status, constants::ERROR_STATUS); + assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); + + // Not installed → Error, "NotInstalled" summary + let info = ServiceRuntimeStatus { + service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), + is_installed: false, + is_running: false, + state_display: "NotInstalled".to_string(), + start_type_display: "NotInstalled".to_string(), + }; + let sub = super::build_proxy_agent_service_substatus(&info); + assert_eq!(sub.status, constants::ERROR_STATUS); + assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); + assert_eq!( + sub.formattedMessage.message, + format!("{}: NotInstalled", constants::PROXY_AGENT_SERVICE_NAME) + ); + } + + #[test] + #[cfg(windows)] + fn test_apply_ebpf_status_override() { + let make_ebpf_sub = |status: &str, message: &str| SubStatus { + name: constants::EBPF_SUBSTATUS_NAME.to_string(), + status: status.to_string(), + code: if status == constants::ERROR_STATUS { + constants::STATUS_CODE_NOT_OK + } else { + constants::STATUS_CODE_OK + }, + formattedMessage: FormattedMessage { + lang: constants::LANG_EN_US.to_string(), + message: message.to_string(), + }, + }; + + // eBPF Error overrides an otherwise-Success status + let mut status = make_test_status_obj( + constants::SUCCESS_STATUS, + constants::STATUS_CODE_OK, + "ProxyAgent extension is reporting successful status.", + ); + let ebpf_sub = make_ebpf_sub( + constants::ERROR_STATUS, + "EbpfCore: Running, AutoStart, NetEbpfExt: Stopped, AutoStart, eBPFSvc: Running, AutoStart", + ); + let overridden = super::apply_ebpf_status_override( + &mut status, + &ebpf_sub, + "2026-08-21 8:13:38.104 +00:00:00", + ); + assert!(overridden); + assert_eq!(status.status, constants::ERROR_STATUS); + assert_eq!(status.code, constants::STATUS_CODE_NOT_OK); + assert!(status + .formattedMessage + .message + .contains("NetEbpfExt: Stopped")); + assert!(status + .formattedMessage + .message + .contains("Last status timestamp: 2026-08-21 8:13:38.104 +00:00:00")); + assert!(status.formattedMessage.message.contains("Current time:")); + + // eBPF Error overrides an already-Error stale message too + let mut status = make_test_status_obj( + constants::ERROR_STATUS, + constants::STATUS_CODE_NOT_OK, + "Proxy agent aggregate status file is stale. Status timestamp: ..., Current time: ...", + ); + let overridden = super::apply_ebpf_status_override( + &mut status, + &ebpf_sub, + "2026-08-21 8:13:38.104 +00:00:00", + ); + assert!(overridden); + assert!(!status.formattedMessage.message.contains("stale")); + assert!(status + .formattedMessage + .message + .contains("NetEbpfExt: Stopped")); + + // eBPF healthy leaves the existing message untouched + let mut status = make_test_status_obj( + constants::SUCCESS_STATUS, + constants::STATUS_CODE_OK, + "ProxyAgent extension is reporting successful status.", + ); + let healthy_ebpf_sub = make_ebpf_sub( + constants::SUCCESS_STATUS, + "EbpfCore: Running, AutoStart, NetEbpfExt: Running, AutoStart, eBPFSvc: Running, AutoStart", + ); + let overridden = + super::apply_ebpf_status_override(&mut status, &healthy_ebpf_sub, "irrelevant"); + assert!(!overridden); + assert_eq!(status.status, constants::SUCCESS_STATUS); + assert_eq!( + status.formattedMessage.message, + "ProxyAgent extension is reporting successful status." + ); + } + + #[test] + fn test_apply_gpa_service_status_override() { + let make_gpa_sub = |status: &str, message: &str| SubStatus { + name: constants::PROXY_AGENT_SERVICE_SUBSTATUS_NAME.to_string(), + status: status.to_string(), + code: if status == constants::ERROR_STATUS { + constants::STATUS_CODE_NOT_OK + } else { + constants::STATUS_CODE_OK + }, + formattedMessage: FormattedMessage { + lang: constants::LANG_EN_US.to_string(), + message: message.to_string(), + }, + }; + + // GPA-service Error overrides an otherwise-Success status immediately (no gating on + // top-level already being Error) + let mut status = make_test_status_obj( + constants::SUCCESS_STATUS, + constants::STATUS_CODE_OK, + "ProxyAgent extension is reporting successful status.", + ); + let gpa_sub = make_gpa_sub( + constants::ERROR_STATUS, + &format!( + "{}: Stopped, AutoStart", + constants::PROXY_AGENT_SERVICE_NAME + ), + ); + let overridden = super::apply_gpa_service_status_override( + &mut status, + &gpa_sub, + "2026-08-21 8:13:38.104 +00:00:00", + ); + assert!(overridden); + assert_eq!(status.status, constants::ERROR_STATUS); + assert_eq!(status.code, constants::STATUS_CODE_NOT_OK); + assert!(status + .formattedMessage + .message + .contains("Stopped, AutoStart")); + assert!(status + .formattedMessage + .message + .contains("Last status timestamp: 2026-08-21 8:13:38.104 +00:00:00")); + assert!(status.formattedMessage.message.contains("Current time:")); + + // GPA-service healthy leaves the existing message untouched + let mut status = make_test_status_obj( + constants::SUCCESS_STATUS, + constants::STATUS_CODE_OK, + "ProxyAgent extension is reporting successful status.", + ); + let healthy_gpa_sub = make_gpa_sub( + constants::SUCCESS_STATUS, + &format!( + "{}: Running, AutoStart", + constants::PROXY_AGENT_SERVICE_NAME + ), + ); + let overridden = + super::apply_gpa_service_status_override(&mut status, &healthy_gpa_sub, "irrelevant"); + assert!(!overridden); + assert_eq!( + status.formattedMessage.message, + "ProxyAgent extension is reporting successful status." + ); + } + + #[test] + fn test_should_poll() { + use std::time::{Duration, Instant}; + + let interval = Duration::from_secs(120); + let now = Instant::now(); + + // Never polled before -> should poll + assert!(super::should_poll(None, now, interval)); + + // Polled recently -> should not poll yet + assert!(!super::should_poll(Some(now), now, interval)); + + // Polled long enough ago -> should poll again + let long_ago = now - Duration::from_secs(121); + assert!(super::should_poll(Some(long_ago), now, interval)); + + // Exactly at the interval boundary -> should poll (>=) + let exactly_at_interval = now - interval; + assert!(super::should_poll(Some(exactly_at_interval), now, interval)); + } + #[tokio::test] async fn get_top_proxy_connection_summary_tests() { let mut summary = Vec::new(); diff --git a/proxy_agent_shared/src/service.rs b/proxy_agent_shared/src/service.rs index a3a0815c..e7513466 100644 --- a/proxy_agent_shared/src/service.rs +++ b/proxy_agent_shared/src/service.rs @@ -182,6 +182,51 @@ pub use windows_service::ServiceState; #[cfg(windows)] pub use windows_service::ServiceStatusInfo; +/// Cross-platform runtime status of a system service (Windows SCM or Linux systemd), +/// used for reporting service health that is meaningful on both platforms (e.g. the +/// GuestProxyAgent service itself). Unlike `ServiceStatusInfo` (Windows-only, used for +/// the Windows-specific eBPF driver/service substatus), this type has an implementation +/// on every platform. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ServiceRuntimeStatus { + pub service_name: String, + pub is_installed: bool, + pub is_running: bool, + /// Human-readable running state, e.g. "Running", "Stopped", "Failed". + pub state_display: String, + /// Human-readable start type, e.g. "AutoStart", "OnDemand", "Disabled". + pub start_type_display: String, +} + +impl ServiceRuntimeStatus { + /// Human-readable summary, e.g. "Running, AutoStart" or "NotInstalled". + pub fn summary(&self) -> String { + if self.is_installed { + format!("{}, {}", self.state_display, self.start_type_display) + } else { + "NotInstalled".to_string() + } + } + + /// Log-friendly message including the service name and summary. + pub fn message(&self) -> String { + format!("service: {} status: {}", self.service_name, self.summary()) + } +} + +/// Checks the runtime status (running state + start type) of a service in a cross-platform +/// way. Uses the Windows SCM on Windows and `systemctl` on Linux. +pub fn check_service_run_status(service_name: &str) -> ServiceRuntimeStatus { + #[cfg(windows)] + { + windows_service::query_service_run_status(service_name) + } + #[cfg(not(windows))] + { + linux_service::check_service_run_status(service_name) + } +} + #[cfg(test)] mod tests { #[test] @@ -266,4 +311,44 @@ mod tests { _ = super::stop_and_delete_service(service_name).await.unwrap(); } } + + #[test] + fn test_check_service_run_status_not_installed() { + // Cross-platform: a service name that certainly does not exist should report + // not-installed/not-running on both Windows and Linux. + let status = super::check_service_run_status("gpa-test-service-that-does-not-exist"); + assert!(!status.is_installed); + assert!(!status.is_running); + assert_eq!(status.summary(), "NotInstalled"); + assert!(status.message().contains("NotInstalled")); + } + + #[tokio::test] + async fn test_check_service_run_status_windows() { + #[cfg(windows)] + { + let service_name = "test_check_service_run_status"; + // try delete the service if it exists + _ = super::stop_and_delete_service(service_name).await; + + let exe_path = std::env::current_exe().unwrap(); + let result = super::install_service(service_name, service_name, vec![], exe_path); + assert!(result.is_ok()); + + let status = super::check_service_run_status(service_name); + assert!(status.is_installed); + // The test exe cannot actually run as a service, so it should be reported as + // installed-but-not-running. + assert!(!status.is_running); + assert_eq!(status.state_display, "Stopped"); + let summary = status.summary(); + assert!( + summary.contains("AutoStart"), + "Expected summary to contain 'AutoStart', got: {summary}" + ); + + // clean up + super::stop_and_delete_service(service_name).await.unwrap(); + } + } } diff --git a/proxy_agent_shared/src/service/linux_service.rs b/proxy_agent_shared/src/service/linux_service.rs index 3e71bb9c..197cdfdb 100644 --- a/proxy_agent_shared/src/service/linux_service.rs +++ b/proxy_agent_shared/src/service/linux_service.rs @@ -187,3 +187,134 @@ pub fn check_service_installed(service_name: &str) -> (bool, String) { (false, message) } } + +/// Maps the trimmed stdout of `systemctl is-active ` to (is_running, state_display). +/// Pure function so it is unit-testable without shelling out to `systemctl`. +fn map_is_active_output(output: &str) -> (bool, String) { + match output.trim() { + "active" => (true, "Running".to_string()), + "inactive" => (false, "Stopped".to_string()), + "failed" => (false, "Failed".to_string()), + "activating" => (false, "Activating".to_string()), + "deactivating" => (false, "Deactivating".to_string()), + other => (false, capitalize_first(other)), + } +} + +/// Maps the trimmed stdout of `systemctl is-enabled ` to a start-type display string, +/// using Windows-like vocabulary ("AutoStart"/"Disabled") so the reported message shape is +/// consistent across platforms. Pure function so it is unit-testable without shelling out. +fn map_is_enabled_output(output: &str) -> String { + match output.trim() { + "enabled" | "enabled-runtime" => "AutoStart".to_string(), + "disabled" => "Disabled".to_string(), + "masked" => "Disabled".to_string(), + "static" => "OnDemand".to_string(), + other => capitalize_first(other), + } +} + +fn capitalize_first(s: &str) -> String { + if s.is_empty() { + return "Unknown".to_string(); + } + let mut chars = s.chars(); + match chars.next() { + Some(first) => first.to_uppercase().collect::() + chars.as_str(), + None => "Unknown".to_string(), + } +} + +/// Checks a service's runtime status (running state + start type) using `systemctl`, +/// in the cross-platform `ServiceRuntimeStatus` shape. +pub fn check_service_run_status(service_name: &str) -> crate::service::ServiceRuntimeStatus { + let (is_installed, _) = check_service_installed(service_name); + if !is_installed { + return crate::service::ServiceRuntimeStatus { + service_name: service_name.to_string(), + is_installed: false, + is_running: false, + state_display: "NotInstalled".to_string(), + start_type_display: "NotInstalled".to_string(), + }; + } + + let (is_running, state_display) = + match misc_helpers::execute_command("systemctl", vec!["is-active", service_name], -1) { + Ok(output) => map_is_active_output(&output.stdout()), + Err(e) => { + logger_manager::write_info(format!( + "check_service_run_status: failed to query is-active for {service_name}: {e}" + )); + (false, "Unknown".to_string()) + } + }; + + let start_type_display = + match misc_helpers::execute_command("systemctl", vec!["is-enabled", service_name], -1) { + Ok(output) => map_is_enabled_output(&output.stdout()), + Err(e) => { + logger_manager::write_info(format!( + "check_service_run_status: failed to query is-enabled for {service_name}: {e}" + )); + "Unknown".to_string() + } + }; + + crate::service::ServiceRuntimeStatus { + service_name: service_name.to_string(), + is_installed: true, + is_running, + state_display, + start_type_display, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn map_is_active_output_test() { + assert_eq!( + map_is_active_output("active\n"), + (true, "Running".to_string()) + ); + assert_eq!( + map_is_active_output("inactive\n"), + (false, "Stopped".to_string()) + ); + assert_eq!( + map_is_active_output("failed\n"), + (false, "Failed".to_string()) + ); + assert_eq!( + map_is_active_output("activating\n"), + (false, "Activating".to_string()) + ); + assert_eq!( + map_is_active_output("unknown\n"), + (false, "Unknown".to_string()) + ); + } + + #[test] + fn map_is_enabled_output_test() { + assert_eq!(map_is_enabled_output("enabled\n"), "AutoStart".to_string()); + assert_eq!(map_is_enabled_output("disabled\n"), "Disabled".to_string()); + assert_eq!(map_is_enabled_output("masked\n"), "Disabled".to_string()); + assert_eq!(map_is_enabled_output("static\n"), "OnDemand".to_string()); + assert_eq!( + map_is_enabled_output("some-other-state\n"), + "Some-other-state".to_string() + ); + } + + #[test] + fn check_service_run_status_not_installed_test() { + let status = check_service_run_status("gpa-test-service-that-does-not-exist"); + assert!(!status.is_installed); + assert!(!status.is_running); + assert_eq!(status.summary(), "NotInstalled"); + } +} diff --git a/proxy_agent_shared/src/service/windows_service.rs b/proxy_agent_shared/src/service/windows_service.rs index b8f552c5..9a42b889 100644 --- a/proxy_agent_shared/src/service/windows_service.rs +++ b/proxy_agent_shared/src/service/windows_service.rs @@ -221,6 +221,43 @@ pub fn query_service_config(service_name: &str) -> Result { .map_err(|e| Error::WindowsService(e, std::io::Error::last_os_error())) } +/// Queries a service's runtime status in the cross-platform `ServiceRuntimeStatus` shape, +/// re-mapping the same data already fetched by `check_service_status`/`query_service_config`. +pub fn query_service_run_status(service_name: &str) -> crate::service::ServiceRuntimeStatus { + match query_service_status(service_name) { + Ok(status) => { + let start_type_display = match query_service_config(service_name) { + Ok(config) => format!("{:?}", config.start_type), + Err(e) => { + logger_manager::write_info(format!( + "Failed to query config for service '{service_name}': {e}", + )); + "Unknown".to_string() + } + }; + crate::service::ServiceRuntimeStatus { + service_name: service_name.to_string(), + is_installed: true, + is_running: status.current_state == ServiceState::Running, + state_display: format!("{:?}", status.current_state), + start_type_display, + } + } + Err(e) => { + logger_manager::write_info(format!( + "Failed to query status for service '{service_name}': {e}. Treating as not installed.", + )); + crate::service::ServiceRuntimeStatus { + service_name: service_name.to_string(), + is_installed: false, + is_running: false, + state_display: "NotInstalled".to_string(), + start_type_display: "NotInstalled".to_string(), + } + } + } +} + pub fn update_service( service_name: &str, service_display_name: &str, From c769e7b459b75203c1081d2ec99df3ef18d74d3a Mon Sep 17 00:00:00 2001 From: Srikrishna Veturi Date: Wed, 2 Sep 2026 14:45:28 -0600 Subject: [PATCH 2/6] Address code review: avoid false-positive overrides during transient service states - Add Running/Transitioning/Down state classification (new ServiceRuntimeStatus.is_transitioning field, new classify_service_state helper on Windows, updated Linux map_is_active_output) instead of treating any non-Running state as a confirmed failure. Windows StartPending/ContinuePending and Linux systemd ctivating are now classified as Transitioning rather than Error, so a service that is simply still starting up (e.g. during boot or an extension-triggered restart) no longer immediately flips the top-level extension status to Error. - build_ebpf_substatus and build_proxy_agent_service_substatus now report the existing TRANSITIONING_STATUS instead of ERROR_STATUS for these benign transitional states, so apply_ebpf_status_override/apply_gpa_service_status_override (unchanged) naturally do not fire on them. - Add should_force_recompute and wire it into monitor_thread so the cached eBPF/GPA-service substatus is recomputed immediately whenever the aggregate-status success/failure result changes, instead of waiting out the full ~2-minute poll interval. This prevents a stale cached Error substatus from continuing to override a just-recovered aggregate status (and vice versa for a newly-broken service) for up to 2 minutes. - Add/extend unit tests for the new classification logic on both platforms, the Transitioning branch of both substatus builders, regression tests confirming Transitioning does not trigger either override, a test for should_force_recompute, and a backfilled test for compute_gpa_service_substatus (introduced in the previous commit without dedicated coverage). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- proxy_agent_extension/src/service_main.rs | 257 +++++++++++++++++- proxy_agent_shared/src/service.rs | 14 + .../src/service/linux_service.rs | 46 +++- .../src/service/windows_service.rs | 60 +++- 4 files changed, 352 insertions(+), 25 deletions(-) diff --git a/proxy_agent_extension/src/service_main.rs b/proxy_agent_extension/src/service_main.rs index d4e7227e..15394e55 100644 --- a/proxy_agent_extension/src/service_main.rs +++ b/proxy_agent_extension/src/service_main.rs @@ -225,6 +225,12 @@ async fn monitor_thread() { // the immediate eBPF/GPA-service overrides below; kept from the previous iteration whenever // the current iteration's fetch fails outright. let mut last_known_status_timestamp = String::new(); + // Tracks whether the aggregate-status check (Step 3) was successful on the previous + // iteration, so a fresh transition (success<->failure) can force an immediate recompute of + // the cached eBPF/GPA service substatus below, instead of waiting out the full poll + // interval - this avoids a stale cached Error substatus continuing to override a + // just-recovered aggregate status (or vice versa) for up to the poll interval. + let mut prev_aggregate_status_ok: Option = None; loop { let current_seq_no: String = common::get_current_seq_no(&exe_path); @@ -311,6 +317,17 @@ async fn monitor_thread() { last_known_status_timestamp = status_timestamp; } + // Detect an aggregate-status success/failure transition since the previous iteration, + // so Step 6 can force an immediate recompute of the cached eBPF/GPA service substatus + // instead of waiting out the full poll interval. Without this, a stale cached Error + // substatus could keep overriding a just-recovered aggregate status (or a stale cached + // healthy substatus could keep masking a newly-broken service) for up to the poll + // interval. + let aggregate_status_ok = status.status == *constants::SUCCESS_STATUS; + let force_service_status_recompute = + should_force_recompute(prev_aggregate_status_ok, aggregate_status_ok); + prev_aggregate_status_ok = Some(aggregate_status_ok); + // Step 4: Restore (on error) or purge (on success) the backed-up proxy agent, once if !restored_in_error { restored_in_error = restore_purge_proxy_agent(&mut status); @@ -330,14 +347,17 @@ async fn monitor_thread() { } // Step 6: Poll eBPF (Windows only) and GuestProxyAgent service (cross-platform) runtime - // status on a decoupled ~2-minute cadence, independent of loop_interval. The cached - // substatus values are re-appended every iteration so the status file (written every - // loop_interval) always reflects the latest known state. + // status on a decoupled ~2-minute cadence, independent of loop_interval (or immediately, + // regardless of cadence, when the aggregate-status result just transitioned - see + // `force_service_status_recompute` above). The cached substatus values are re-appended + // every iteration so the status file (written every loop_interval) always reflects the + // latest known state. if should_poll( last_service_status_poll, std::time::Instant::now(), Duration::from_secs(constants::SERVICE_STATUS_POLL_INTERVAL_SECS), - ) { + ) || force_service_status_recompute + { #[cfg(windows)] { last_ebpf_substatus = Some(compute_ebpf_substatus()); @@ -405,6 +425,16 @@ fn should_poll( } } +/// Returns true when `current` differs from the previously observed value, indicating the +/// aggregate-status success/failure state has just changed since the prior iteration. Returns +/// false on the very first call (when `prev` is `None`), since there is nothing yet to compare +/// against. Pure/testable helper used to force an immediate eBPF/GPA-service-status recompute +/// at meaningful transitions, without abandoning the steady-state decoupled polling cadence +/// the rest of the time. +fn should_force_recompute(prev: Option, current: bool) -> bool { + prev.is_some_and(|previous| previous != current) +} + fn write_state_event( state_key: &str, state_value: &str, @@ -431,22 +461,37 @@ fn build_ebpf_substatus( ext: &proxy_agent_shared::service::ServiceStatusInfo, svc: &proxy_agent_shared::service::ServiceStatusInfo, ) -> SubStatus { - use proxy_agent_shared::service::ServiceState; + use proxy_agent_shared::service::classify_service_state; + + let (core_running, core_transitioning) = classify_service_state(core.state.as_ref()); + let (ext_running, ext_transitioning) = classify_service_state(ext.state.as_ref()); + let (svc_running, svc_transitioning) = classify_service_state(svc.state.as_ref()); - let all_running = matches!(core.state, Some(ServiceState::Running)) - && matches!(ext.state, Some(ServiceState::Running)) - && matches!(svc.state, Some(ServiceState::Running)); + let all_running = core_running && ext_running && svc_running; + // "Down" means confirmed not-running and not actively transitioning toward Running. + let any_down = (!core_running && !core_transitioning) + || (!ext_running && !ext_transitioning) + || (!svc_running && !svc_transitioning); let (status, code) = if all_running { ( constants::SUCCESS_STATUS.to_string(), constants::STATUS_CODE_OK, ) - } else { + } else if any_down { ( constants::ERROR_STATUS.to_string(), constants::STATUS_CODE_NOT_OK, ) + } else { + // None are confirmed down, but at least one is still starting up (StartPending / + // ContinuePending) - a normal, usually brief condition during boot or a restart. + // Report Transitioning instead of Error so the immediate top-level override + // (`apply_ebpf_status_override`) does not fire on this benign condition. + ( + constants::TRANSITIONING_STATUS.to_string(), + constants::STATUS_CODE_OK, + ) }; let message = format!( @@ -491,6 +536,15 @@ fn build_proxy_agent_service_substatus( constants::SUCCESS_STATUS.to_string(), constants::STATUS_CODE_OK, ) + } else if info.is_transitioning { + // Actively starting up (Windows StartPending/ContinuePending, or systemd + // "activating") - a normal, usually brief condition during boot or a restart. + // Report Transitioning instead of Error so the immediate top-level override + // (`apply_gpa_service_status_override`) does not fire on this benign condition. + ( + constants::TRANSITIONING_STATUS.to_string(), + constants::STATUS_CODE_OK, + ) } else { ( constants::ERROR_STATUS.to_string(), @@ -1440,12 +1494,50 @@ mod tests { ); if ebpf_substatus.status == constants::SUCCESS_STATUS { assert_eq!(ebpf_substatus.code, constants::STATUS_CODE_OK); + } else if ebpf_substatus.status == constants::TRANSITIONING_STATUS { + // A service could legitimately be caught mid-start on the test runner; code stays + // OK while Transitioning, consistent with the existing set_error/set_success + // code/status coupling convention used elsewhere in this file. + assert_eq!(ebpf_substatus.code, constants::STATUS_CODE_OK); } else { assert_eq!(ebpf_substatus.status, constants::ERROR_STATUS); assert_eq!(ebpf_substatus.code, constants::STATUS_CODE_NOT_OK); } } + #[test] + fn test_compute_gpa_service_substatus() { + // Cross-platform (unlike compute_ebpf_substatus, not gated to Windows): exercises the + // real check_service_run_status call (SCM on Windows, systemctl on Linux) against + // whatever GuestProxyAgent service state the test runner happens to have, and verifies + // the result is well-formed and internally consistent regardless of that state. This + // backfills test coverage for a function introduced in the prior commit that previously + // had no dedicated test (only its pure `build_proxy_agent_service_substatus` helper was + // tested). + let substatus = super::compute_gpa_service_substatus(); + assert_eq!( + substatus.name, + constants::PROXY_AGENT_SERVICE_SUBSTATUS_NAME + ); + assert!( + substatus + .formattedMessage + .message + .starts_with(&format!("{}: ", constants::PROXY_AGENT_SERVICE_NAME)), + "Expected message to start with '{}: ', got: {}", + constants::PROXY_AGENT_SERVICE_NAME, + substatus.formattedMessage.message + ); + if substatus.status == constants::SUCCESS_STATUS { + assert_eq!(substatus.code, constants::STATUS_CODE_OK); + } else if substatus.status == constants::TRANSITIONING_STATUS { + assert_eq!(substatus.code, constants::STATUS_CODE_OK); + } else { + assert_eq!(substatus.status, constants::ERROR_STATUS); + assert_eq!(substatus.code, constants::STATUS_CODE_NOT_OK); + } + } + #[test] #[cfg(windows)] fn test_build_ebpf_substatus() { @@ -1589,6 +1681,49 @@ mod tests { ); assert_eq!(sub.status, constants::ERROR_STATUS, "All three stopped"); assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); + + // 10. Core starting up (StartPending), Ext+Svc running → Transitioning, not Error. + // Regression test: a service mid-boot/mid-restart must not immediately flip the + // top-level extension status to Error (see apply_ebpf_status_override, which only + // fires on ERROR_STATUS). + let sub = super::build_ebpf_substatus( + &make_info(constants::EBPF_CORE, Some(ServiceState::StartPending)), + &make_info(constants::EBPF_EXT, running()), + &make_info(constants::EBPF_SVC, running()), + ); + assert_eq!( + sub.status, + constants::TRANSITIONING_STATUS, + "Core starting up should be Transitioning, not Error" + ); + assert_eq!(sub.code, constants::STATUS_CODE_OK); + + // 11. Svc resuming (ContinuePending), Core+Ext running → Transitioning, not Error. + let sub = super::build_ebpf_substatus( + &make_info(constants::EBPF_CORE, running()), + &make_info(constants::EBPF_EXT, running()), + &make_info(constants::EBPF_SVC, Some(ServiceState::ContinuePending)), + ); + assert_eq!( + sub.status, + constants::TRANSITIONING_STATUS, + "Svc resuming should be Transitioning, not Error" + ); + assert_eq!(sub.code, constants::STATUS_CODE_OK); + + // 12. Core starting up (StartPending) AND Ext confirmed stopped → Error wins over + // Transitioning, since at least one service is confirmed down. + let sub = super::build_ebpf_substatus( + &make_info(constants::EBPF_CORE, Some(ServiceState::StartPending)), + &make_info(constants::EBPF_EXT, stopped()), + &make_info(constants::EBPF_SVC, running()), + ); + assert_eq!( + sub.status, + constants::ERROR_STATUS, + "A confirmed-down service should still report Error even if another is transitioning" + ); + assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); } #[test] @@ -1600,6 +1735,7 @@ mod tests { service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), is_installed: true, is_running: true, + is_transitioning: false, state_display: "Running".to_string(), start_type_display: "AutoStart".to_string(), }; @@ -1620,6 +1756,7 @@ mod tests { service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), is_installed: true, is_running: false, + is_transitioning: false, state_display: "Stopped".to_string(), start_type_display: "AutoStart".to_string(), }; @@ -1639,6 +1776,7 @@ mod tests { service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), is_installed: true, is_running: false, + is_transitioning: false, state_display: "Stopped".to_string(), start_type_display: "Disabled".to_string(), }; @@ -1651,6 +1789,7 @@ mod tests { service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), is_installed: false, is_running: false, + is_transitioning: false, state_display: "NotInstalled".to_string(), start_type_display: "NotInstalled".to_string(), }; @@ -1661,6 +1800,33 @@ mod tests { sub.formattedMessage.message, format!("{}: NotInstalled", constants::PROXY_AGENT_SERVICE_NAME) ); + + // Starting up (StartPending on Windows / "activating" on Linux) → Transitioning, not + // Error. Regression test: a service mid-boot/mid-restart must not immediately flip the + // top-level extension status to Error (see apply_gpa_service_status_override, which + // only fires on ERROR_STATUS). + let info = ServiceRuntimeStatus { + service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), + is_installed: true, + is_running: false, + is_transitioning: true, + state_display: "StartPending".to_string(), + start_type_display: "AutoStart".to_string(), + }; + let sub = super::build_proxy_agent_service_substatus(&info); + assert_eq!( + sub.status, + constants::TRANSITIONING_STATUS, + "A service starting up should be Transitioning, not Error" + ); + assert_eq!(sub.code, constants::STATUS_CODE_OK); + assert_eq!( + sub.formattedMessage.message, + format!( + "{}: StartPending, AutoStart", + constants::PROXY_AGENT_SERVICE_NAME + ) + ); } #[test] @@ -1744,6 +1910,30 @@ mod tests { status.formattedMessage.message, "ProxyAgent extension is reporting successful status." ); + + // eBPF Transitioning (e.g. a service mid-boot/mid-restart) must NOT trigger the + // override - regression test for the reviewer finding that this override previously + // fired immediately on any non-Running state, including benign transitional ones. + let mut status = make_test_status_obj( + constants::SUCCESS_STATUS, + constants::STATUS_CODE_OK, + "ProxyAgent extension is reporting successful status.", + ); + let transitioning_ebpf_sub = make_ebpf_sub( + constants::TRANSITIONING_STATUS, + "EbpfCore: Running, AutoStart, NetEbpfExt: StartPending, AutoStart, eBPFSvc: Running, AutoStart", + ); + let overridden = + super::apply_ebpf_status_override(&mut status, &transitioning_ebpf_sub, "irrelevant"); + assert!( + !overridden, + "Transitioning eBPF substatus must not trigger the immediate override" + ); + assert_eq!(status.status, constants::SUCCESS_STATUS); + assert_eq!( + status.formattedMessage.message, + "ProxyAgent extension is reporting successful status." + ); } #[test] @@ -1814,6 +2004,36 @@ mod tests { status.formattedMessage.message, "ProxyAgent extension is reporting successful status." ); + + // GPA-service Transitioning (e.g. mid-boot/mid-restart) must NOT trigger the override - + // regression test for the reviewer finding that this override previously fired + // immediately on any non-Running state, including benign transitional ones. + let mut status = make_test_status_obj( + constants::SUCCESS_STATUS, + constants::STATUS_CODE_OK, + "ProxyAgent extension is reporting successful status.", + ); + let transitioning_gpa_sub = make_gpa_sub( + constants::TRANSITIONING_STATUS, + &format!( + "{}: StartPending, AutoStart", + constants::PROXY_AGENT_SERVICE_NAME + ), + ); + let overridden = super::apply_gpa_service_status_override( + &mut status, + &transitioning_gpa_sub, + "irrelevant", + ); + assert!( + !overridden, + "Transitioning GPA-service substatus must not trigger the immediate override" + ); + assert_eq!(status.status, constants::SUCCESS_STATUS); + assert_eq!( + status.formattedMessage.message, + "ProxyAgent extension is reporting successful status." + ); } #[test] @@ -1838,6 +2058,25 @@ mod tests { assert!(super::should_poll(Some(exactly_at_interval), now, interval)); } + #[test] + fn test_should_force_recompute() { + // First observation ever (no previous value) -> never force, nothing to compare against + assert!(!super::should_force_recompute(None, true)); + assert!(!super::should_force_recompute(None, false)); + + // No change since previous iteration -> don't force + assert!(!super::should_force_recompute(Some(true), true)); + assert!(!super::should_force_recompute(Some(false), false)); + + // Aggregate status just recovered (was failing, now succeeding) -> force recompute so + // a stale cached Error substatus doesn't keep overriding the fresh success. + assert!(super::should_force_recompute(Some(false), true)); + + // Aggregate status just broke (was succeeding, now failing) -> force recompute so a + // stale cached healthy substatus doesn't keep masking the new failure. + assert!(super::should_force_recompute(Some(true), false)); + } + #[tokio::test] async fn get_top_proxy_connection_summary_tests() { let mut summary = Vec::new(); diff --git a/proxy_agent_shared/src/service.rs b/proxy_agent_shared/src/service.rs index e7513466..c3367da4 100644 --- a/proxy_agent_shared/src/service.rs +++ b/proxy_agent_shared/src/service.rs @@ -175,6 +175,8 @@ pub fn check_service_status(service_name: &str) -> windows_service::ServiceStatu } } +#[cfg(windows)] +pub use windows_service::classify_service_state; #[cfg(windows)] pub use windows_service::set_default_failure_actions; #[cfg(windows)] @@ -192,6 +194,12 @@ pub struct ServiceRuntimeStatus { pub service_name: String, pub is_installed: bool, pub is_running: bool, + /// True when the service is actively transitioning *toward* a running state (e.g. + /// Windows `StartPending`/`ContinuePending`, or systemd `activating`). This is a normal, + /// usually brief condition during boot or a service restart and is intentionally treated + /// as distinct from a confirmed failure (`is_running == false && is_transitioning == + /// false`), so callers don't have to treat "still starting up" the same as "actually down". + pub is_transitioning: bool, /// Human-readable running state, e.g. "Running", "Stopped", "Failed". pub state_display: String, /// Human-readable start type, e.g. "AutoStart", "OnDemand", "Disabled". @@ -319,6 +327,10 @@ mod tests { let status = super::check_service_run_status("gpa-test-service-that-does-not-exist"); assert!(!status.is_installed); assert!(!status.is_running); + assert!( + !status.is_transitioning, + "A not-installed service must not be reported as transitioning" + ); assert_eq!(status.summary(), "NotInstalled"); assert!(status.message().contains("NotInstalled")); } @@ -340,6 +352,8 @@ mod tests { // The test exe cannot actually run as a service, so it should be reported as // installed-but-not-running. assert!(!status.is_running); + // Stopped is a confirmed-down state, not a transitioning one. + assert!(!status.is_transitioning); assert_eq!(status.state_display, "Stopped"); let summary = status.summary(); assert!( diff --git a/proxy_agent_shared/src/service/linux_service.rs b/proxy_agent_shared/src/service/linux_service.rs index 197cdfdb..3f0e37f4 100644 --- a/proxy_agent_shared/src/service/linux_service.rs +++ b/proxy_agent_shared/src/service/linux_service.rs @@ -188,16 +188,22 @@ pub fn check_service_installed(service_name: &str) -> (bool, String) { } } -/// Maps the trimmed stdout of `systemctl is-active ` to (is_running, state_display). +/// Maps the trimmed stdout of `systemctl is-active ` to +/// (is_running, is_transitioning, state_display). +/// `activating` mirrors Windows `StartPending`/`ContinuePending`: the unit is heading *toward* +/// active and this is a normal, usually brief condition during boot or a restart, so it is +/// intentionally distinguished from a confirmed failure. `deactivating` mirrors Windows +/// `StopPending`: the unit is heading *away* from active, which is treated as a confirmed down +/// state (not transitioning), since it's actionable information worth surfacing immediately. /// Pure function so it is unit-testable without shelling out to `systemctl`. -fn map_is_active_output(output: &str) -> (bool, String) { +fn map_is_active_output(output: &str) -> (bool, bool, String) { match output.trim() { - "active" => (true, "Running".to_string()), - "inactive" => (false, "Stopped".to_string()), - "failed" => (false, "Failed".to_string()), - "activating" => (false, "Activating".to_string()), - "deactivating" => (false, "Deactivating".to_string()), - other => (false, capitalize_first(other)), + "active" => (true, false, "Running".to_string()), + "inactive" => (false, false, "Stopped".to_string()), + "failed" => (false, false, "Failed".to_string()), + "activating" => (false, true, "Activating".to_string()), + "deactivating" => (false, false, "Deactivating".to_string()), + other => (false, false, capitalize_first(other)), } } @@ -234,19 +240,20 @@ pub fn check_service_run_status(service_name: &str) -> crate::service::ServiceRu service_name: service_name.to_string(), is_installed: false, is_running: false, + is_transitioning: false, state_display: "NotInstalled".to_string(), start_type_display: "NotInstalled".to_string(), }; } - let (is_running, state_display) = + let (is_running, is_transitioning, state_display) = match misc_helpers::execute_command("systemctl", vec!["is-active", service_name], -1) { Ok(output) => map_is_active_output(&output.stdout()), Err(e) => { logger_manager::write_info(format!( "check_service_run_status: failed to query is-active for {service_name}: {e}" )); - (false, "Unknown".to_string()) + (false, false, "Unknown".to_string()) } }; @@ -265,6 +272,7 @@ pub fn check_service_run_status(service_name: &str) -> crate::service::ServiceRu service_name: service_name.to_string(), is_installed: true, is_running, + is_transitioning, state_display, start_type_display, } @@ -278,23 +286,30 @@ mod tests { fn map_is_active_output_test() { assert_eq!( map_is_active_output("active\n"), - (true, "Running".to_string()) + (true, false, "Running".to_string()) ); assert_eq!( map_is_active_output("inactive\n"), - (false, "Stopped".to_string()) + (false, false, "Stopped".to_string()) ); assert_eq!( map_is_active_output("failed\n"), - (false, "Failed".to_string()) + (false, false, "Failed".to_string()) ); + // "activating" is transitioning toward Running - not a confirmed failure. assert_eq!( map_is_active_output("activating\n"), - (false, "Activating".to_string()) + (false, true, "Activating".to_string()) + ); + // "deactivating" is heading away from Running - treated as a confirmed down state, + // consistent with Windows StopPending, since it's actionable to know immediately. + assert_eq!( + map_is_active_output("deactivating\n"), + (false, false, "Deactivating".to_string()) ); assert_eq!( map_is_active_output("unknown\n"), - (false, "Unknown".to_string()) + (false, false, "Unknown".to_string()) ); } @@ -315,6 +330,7 @@ mod tests { let status = check_service_run_status("gpa-test-service-that-does-not-exist"); assert!(!status.is_installed); assert!(!status.is_running); + assert!(!status.is_transitioning); assert_eq!(status.summary(), "NotInstalled"); } } diff --git a/proxy_agent_shared/src/service/windows_service.rs b/proxy_agent_shared/src/service/windows_service.rs index 9a42b889..faff2905 100644 --- a/proxy_agent_shared/src/service/windows_service.rs +++ b/proxy_agent_shared/src/service/windows_service.rs @@ -221,6 +221,23 @@ pub fn query_service_config(service_name: &str) -> Result { .map_err(|e| Error::WindowsService(e, std::io::Error::last_os_error())) } +/// Classifies a Windows service state into (is_running, is_transitioning). +/// `StartPending`/`ContinuePending` are transitioning *toward* Running - a normal, usually +/// brief condition during boot or a service restart, distinct from a confirmed failure. +/// `StopPending`/`PausePending`/`Paused`/`Stopped` (and no state at all) are treated as a +/// confirmed down state, since they are heading away from - or already away from - Running. +/// Pure function so it is unit-testable without a real SCM service. Takes `Option<&ServiceState>` +/// (rather than owning it) so callers don't need `ServiceState` to implement `Copy`/`Clone`, and +/// so it can be reused as-is by `proxy_agent_extension`'s eBPF substatus classification (see +/// `pub use` re-export below). +pub fn classify_service_state(state: Option<&ServiceState>) -> (bool, bool) { + match state { + Some(ServiceState::Running) => (true, false), + Some(ServiceState::StartPending) | Some(ServiceState::ContinuePending) => (false, true), + _ => (false, false), + } +} + /// Queries a service's runtime status in the cross-platform `ServiceRuntimeStatus` shape, /// re-mapping the same data already fetched by `check_service_status`/`query_service_config`. pub fn query_service_run_status(service_name: &str) -> crate::service::ServiceRuntimeStatus { @@ -235,10 +252,13 @@ pub fn query_service_run_status(service_name: &str) -> crate::service::ServiceRu "Unknown".to_string() } }; + let (is_running, is_transitioning) = + classify_service_state(Some(&status.current_state)); crate::service::ServiceRuntimeStatus { service_name: service_name.to_string(), is_installed: true, - is_running: status.current_state == ServiceState::Running, + is_running, + is_transitioning, state_display: format!("{:?}", status.current_state), start_type_display, } @@ -251,6 +271,7 @@ pub fn query_service_run_status(service_name: &str) -> crate::service::ServiceRu service_name: service_name.to_string(), is_installed: false, is_running: false, + is_transitioning: false, state_display: "NotInstalled".to_string(), start_type_display: "NotInstalled".to_string(), } @@ -373,6 +394,43 @@ mod tests { use std::{path::PathBuf, process::Command}; use windows_service::service::ServiceState; + #[test] + fn classify_service_state_test() { + // Running -> healthy + assert_eq!( + super::classify_service_state(Some(&ServiceState::Running)), + (true, false) + ); + // StartPending/ContinuePending -> transitioning toward Running, not a confirmed failure + assert_eq!( + super::classify_service_state(Some(&ServiceState::StartPending)), + (false, true) + ); + assert_eq!( + super::classify_service_state(Some(&ServiceState::ContinuePending)), + (false, true) + ); + // Stopped/StopPending/PausePending/Paused -> confirmed down, not transitioning + assert_eq!( + super::classify_service_state(Some(&ServiceState::Stopped)), + (false, false) + ); + assert_eq!( + super::classify_service_state(Some(&ServiceState::StopPending)), + (false, false) + ); + assert_eq!( + super::classify_service_state(Some(&ServiceState::PausePending)), + (false, false) + ); + assert_eq!( + super::classify_service_state(Some(&ServiceState::Paused)), + (false, false) + ); + // No state (not installed / query failed) -> confirmed down, not transitioning + assert_eq!(super::classify_service_state(None), (false, false)); + } + #[tokio::test] async fn test_install_service() { const TEST_SERVICE_NAME: &str = "test_nt_service"; From 3fe55e4f26b5d41f05dd658a34d31e75b944bc06 Mon Sep 17 00:00:00 2001 From: Srikrishna Veturi Date: Fri, 4 Sep 2026 14:32:29 -0600 Subject: [PATCH 3/6] Simplify eBPF/GPA-service status checks to poll every loop tick - Revert the decoupled ~2-minute polling cadence and the force-recompute-on-transition logic for the eBPF/GuestProxyAgent-service substatus checks, per manual VM testing feedback: a 2-minute-stale cache was deemed acceptable, and the added complexity to make it 'immediate' was not worth it. compute_ebpf_substatus()/compute_gpa_service_substatus() are now called fresh on every regular monitor_thread loop tick (currently 15s), same as the rest of the loop, so the status file always reflects current machine state at the cost of querying the SCM/systemctl more frequently. - Removed should_poll, should_force_recompute, and the SERVICE_STATUS_POLL_INTERVAL_SECS constant, along with their now-orphaned unit tests. - Refactored the override call site for readability: replaced the boolean-juggling + duplicated per-platform declarations with a single new apply_service_health_overrides helper (Windows-only) that encapsulates the eBPF-wins-priority ordering; non-Windows now calls apply_gpa_service_status_override directly. Added test_apply_service_health_overrides_priority covering all three priority outcomes. - Kept the Running/Transitioning/Down state classification (is_transitioning, classify_service_state, TRANSITIONING_STATUS branches) from the prior code-review fix, since it addresses a separate, still-valid concern (avoiding false-positive Error while a service is merely mid-boot/mid-restart) unrelated to polling cadence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- proxy_agent_extension/src/constants.rs | 4 - proxy_agent_extension/src/service_main.rs | 272 +++++++++++----------- 2 files changed, 139 insertions(+), 137 deletions(-) diff --git a/proxy_agent_extension/src/constants.rs b/proxy_agent_extension/src/constants.rs index 99edeaaa..a273d4ad 100644 --- a/proxy_agent_extension/src/constants.rs +++ b/proxy_agent_extension/src/constants.rs @@ -77,10 +77,6 @@ pub const EBPF_SUBSTATUS_NAME: &str = "EbpfStatus"; pub const PROXY_AGENT_SERVICE_SUBSTATUS_NAME: &str = "ProxyAgentServiceStatus"; -// Cadence for polling the eBPF (Windows) and GuestProxyAgent (cross-platform) service -// runtime status, decoupled from the main monitor_thread loop_interval. -pub const SERVICE_STATUS_POLL_INTERVAL_SECS: u64 = 2 * 60; - pub const MAX_CONNECTION_SUMMARY_LEN: usize = 100; pub const MAX_FAILED_AUTH_SUMMARY_LEN: usize = 50; // Max KB of substatus string for connection summary and failed authentication summary diff --git a/proxy_agent_extension/src/service_main.rs b/proxy_agent_extension/src/service_main.rs index 15394e55..8baaa7c1 100644 --- a/proxy_agent_extension/src/service_main.rs +++ b/proxy_agent_extension/src/service_main.rs @@ -212,25 +212,10 @@ async fn monitor_thread() { let mut restored_in_error = false; let mut proxy_agent_update_reported: Option = None; let loop_interval = Duration::from_secs(15); - // Decoupled cache/cadence for the eBPF (Windows only) and GuestProxyAgent service - // (cross-platform) runtime status checks - these are (re)queried only every - // SERVICE_STATUS_POLL_INTERVAL_SECS (~2 minutes), independent of loop_interval, while the - // cached substatus is still re-appended to the status object (and written to the status - // file) on every loop_interval tick. - #[cfg(windows)] - let mut last_ebpf_substatus: Option = None; - let mut last_gpa_service_substatus: Option = None; - let mut last_service_status_poll: Option = None; // Last known timestamp (as reported by the GPA aggregate status itself) used to annotate // the immediate eBPF/GPA-service overrides below; kept from the previous iteration whenever // the current iteration's fetch fails outright. let mut last_known_status_timestamp = String::new(); - // Tracks whether the aggregate-status check (Step 3) was successful on the previous - // iteration, so a fresh transition (success<->failure) can force an immediate recompute of - // the cached eBPF/GPA service substatus below, instead of waiting out the full poll - // interval - this avoids a stale cached Error substatus continuing to override a - // just-recovered aggregate status (or vice versa) for up to the poll interval. - let mut prev_aggregate_status_ok: Option = None; loop { let current_seq_no: String = common::get_current_seq_no(&exe_path); @@ -317,17 +302,6 @@ async fn monitor_thread() { last_known_status_timestamp = status_timestamp; } - // Detect an aggregate-status success/failure transition since the previous iteration, - // so Step 6 can force an immediate recompute of the cached eBPF/GPA service substatus - // instead of waiting out the full poll interval. Without this, a stale cached Error - // substatus could keep overriding a just-recovered aggregate status (or a stale cached - // healthy substatus could keep masking a newly-broken service) for up to the poll - // interval. - let aggregate_status_ok = status.status == *constants::SUCCESS_STATUS; - let force_service_status_recompute = - should_force_recompute(prev_aggregate_status_ok, aggregate_status_ok); - prev_aggregate_status_ok = Some(aggregate_status_ok); - // Step 4: Restore (on error) or purge (on success) the backed-up proxy agent, once if !restored_in_error { restored_in_error = restore_purge_proxy_agent(&mut status); @@ -346,60 +320,39 @@ async fn monitor_thread() { proxy_agent_update_reported = None; } - // Step 6: Poll eBPF (Windows only) and GuestProxyAgent service (cross-platform) runtime - // status on a decoupled ~2-minute cadence, independent of loop_interval (or immediately, - // regardless of cadence, when the aggregate-status result just transitioned - see - // `force_service_status_recompute` above). The cached substatus values are re-appended - // every iteration so the status file (written every loop_interval) always reflects the - // latest known state. - if should_poll( - last_service_status_poll, - std::time::Instant::now(), - Duration::from_secs(constants::SERVICE_STATUS_POLL_INTERVAL_SECS), - ) || force_service_status_recompute - { - #[cfg(windows)] - { - last_ebpf_substatus = Some(compute_ebpf_substatus()); - } - last_gpa_service_substatus = Some(compute_gpa_service_substatus()); - last_service_status_poll = Some(std::time::Instant::now()); - } - #[cfg(windows)] - if let Some(ebpf_substatus) = &last_ebpf_substatus { - status.substatus.push(ebpf_substatus.clone()); - } - if let Some(gpa_service_substatus) = &last_gpa_service_substatus { - status.substatus.push(gpa_service_substatus.clone()); - } - - // Step 7: Apply immediate overrides when eBPF (Windows only, highest priority - an - // unhealthy eBPF is frequently the root cause of the GuestProxyAgent service failing to - // start) or the GuestProxyAgent service itself (both platforms) is reporting Error. - // These bypass the debounce state machine and take priority over whatever status/message - // Steps 3-5 produced (stale/version-mismatch/connectivity-error/success), because a - // definitively-known local service failure is a complete, actionable, immediate answer - // on its own - there is no reason to wait for slower generic detection to catch up. + // Step 6: Report eBPF (Windows only) and GuestProxyAgent service (cross-platform) + // runtime status, computed fresh every loop_interval tick (same cadence as everything + // else in this loop, currently 15s) so the status file always reflects the current + // machine state. + // + // Step 7: Apply an immediate top-level status/message override when eBPF (Windows, + // highest priority) or the GuestProxyAgent service itself (both platforms) is + // unhealthy. This is still necessary even though the substatus data above is always + // fresh: the top-level status/message is derived from a *different* source (the GPA + // aggregate status file/wire-server response via Step 3), which has its own 5-minute + // staleness threshold and 20-iteration debounce designed to avoid flapping on transient + // network blips - it has no visibility into eBPF or the GuestProxyAgent service at all. + // Without this override, a locally-confirmed eBPF/service failure would still take + // several minutes to surface at the top level; see `apply_service_health_overrides`. + let gpa_service_substatus = compute_gpa_service_substatus(); #[cfg(windows)] - let overridden = match &last_ebpf_substatus { - Some(ebpf_substatus) => apply_ebpf_status_override( + { + let ebpf_substatus = compute_ebpf_substatus(); + apply_service_health_overrides( &mut status, - ebpf_substatus, + &ebpf_substatus, + &gpa_service_substatus, &last_known_status_timestamp, - ), - None => false, - }; - #[cfg(not(windows))] - let overridden = false; - if !overridden { - if let Some(gpa_service_substatus) = &last_gpa_service_substatus { - apply_gpa_service_status_override( - &mut status, - gpa_service_substatus, - &last_known_status_timestamp, - ); - } + ); + status.substatus.push(ebpf_substatus); } + #[cfg(not(windows))] + apply_gpa_service_status_override( + &mut status, + &gpa_service_substatus, + &last_known_status_timestamp, + ); + status.substatus.push(gpa_service_substatus); // Step 8: Write the final status file and sleep common::report_status( @@ -412,29 +365,6 @@ async fn monitor_thread() { } } -/// Returns true when a poll is due: either no poll has happened yet, or at least `interval` -/// has elapsed since the last one. Pure/testable helper for the decoupled service-status cadence. -fn should_poll( - last: Option, - now: std::time::Instant, - interval: Duration, -) -> bool { - match last { - None => true, - Some(last) => now.duration_since(last) >= interval, - } -} - -/// Returns true when `current` differs from the previously observed value, indicating the -/// aggregate-status success/failure state has just changed since the prior iteration. Returns -/// false on the very first call (when `prev` is `None`), since there is nothing yet to compare -/// against. Pure/testable helper used to force an immediate eBPF/GPA-service-status recompute -/// at meaningful transitions, without abandoning the steady-state decoupled polling cadence -/// the rest of the time. -fn should_force_recompute(prev: Option, current: bool) -> bool { - prev.is_some_and(|previous| previous != current) -} - fn write_state_event( state_key: &str, state_value: &str, @@ -622,6 +552,26 @@ fn apply_gpa_service_status_override( true } +/// Applies the Windows-only priority ordering between the two immediate overrides: eBPF errors +/// win over GuestProxyAgent-service errors, since an unhealthy eBPF is frequently the underlying +/// reason the GuestProxyAgent service itself cannot start, making it the more specific/actionable +/// signal. Only falls through to the GPA-service override when eBPF itself did not report Error. +#[cfg(windows)] +fn apply_service_health_overrides( + status: &mut StatusObj, + ebpf_substatus: &SubStatus, + gpa_service_substatus: &SubStatus, + last_known_status_timestamp: &str, +) { + if !apply_ebpf_status_override(status, ebpf_substatus, last_known_status_timestamp) { + apply_gpa_service_status_override( + status, + gpa_service_substatus, + last_known_status_timestamp, + ); + } +} + fn backup_proxy_agent(setup_tool: &String) { match Command::new(setup_tool).arg("backup").output() { Ok(output) => { @@ -2037,44 +1987,100 @@ mod tests { } #[test] - fn test_should_poll() { - use std::time::{Duration, Instant}; - - let interval = Duration::from_secs(120); - let now = Instant::now(); - - // Never polled before -> should poll - assert!(super::should_poll(None, now, interval)); + #[cfg(windows)] + fn test_apply_service_health_overrides_priority() { + let make_sub = |name: &str, status: &str, message: &str| SubStatus { + name: name.to_string(), + status: status.to_string(), + code: if status == constants::ERROR_STATUS { + constants::STATUS_CODE_NOT_OK + } else { + constants::STATUS_CODE_OK + }, + formattedMessage: FormattedMessage { + lang: constants::LANG_EN_US.to_string(), + message: message.to_string(), + }, + }; - // Polled recently -> should not poll yet - assert!(!super::should_poll(Some(now), now, interval)); + let error_ebpf_sub = make_sub( + constants::EBPF_SUBSTATUS_NAME, + constants::ERROR_STATUS, + "EbpfCore: Stopped, AutoStart, NetEbpfExt: Running, AutoStart, eBPFSvc: Running, AutoStart", + ); + let healthy_ebpf_sub = make_sub( + constants::EBPF_SUBSTATUS_NAME, + constants::SUCCESS_STATUS, + "EbpfCore: Running, AutoStart, NetEbpfExt: Running, AutoStart, eBPFSvc: Running, AutoStart", + ); + let error_gpa_sub = make_sub( + constants::PROXY_AGENT_SERVICE_SUBSTATUS_NAME, + constants::ERROR_STATUS, + &format!( + "{}: Stopped, AutoStart", + constants::PROXY_AGENT_SERVICE_NAME + ), + ); + let healthy_gpa_sub = make_sub( + constants::PROXY_AGENT_SERVICE_SUBSTATUS_NAME, + constants::SUCCESS_STATUS, + &format!( + "{}: Running, AutoStart", + constants::PROXY_AGENT_SERVICE_NAME + ), + ); - // Polled long enough ago -> should poll again - let long_ago = now - Duration::from_secs(121); - assert!(super::should_poll(Some(long_ago), now, interval)); + // Both unhealthy -> eBPF wins (message shows eBPF detail, not GPA-service detail) + let mut status = make_test_status_obj( + constants::SUCCESS_STATUS, + constants::STATUS_CODE_OK, + "ProxyAgent extension is reporting successful status.", + ); + super::apply_service_health_overrides(&mut status, &error_ebpf_sub, &error_gpa_sub, "ts"); + assert_eq!(status.status, constants::ERROR_STATUS); + assert!(status + .formattedMessage + .message + .contains("EbpfCore: Stopped")); + assert!( + !status + .formattedMessage + .message + .contains(constants::PROXY_AGENT_SERVICE_NAME), + "GPA-service detail should not appear when eBPF already overrode the message, got: {}", + status.formattedMessage.message + ); - // Exactly at the interval boundary -> should poll (>=) - let exactly_at_interval = now - interval; - assert!(super::should_poll(Some(exactly_at_interval), now, interval)); - } + // eBPF healthy, GPA-service unhealthy -> falls through to the GPA-service override + let mut status = make_test_status_obj( + constants::SUCCESS_STATUS, + constants::STATUS_CODE_OK, + "ProxyAgent extension is reporting successful status.", + ); + super::apply_service_health_overrides(&mut status, &healthy_ebpf_sub, &error_gpa_sub, "ts"); + assert_eq!(status.status, constants::ERROR_STATUS); + assert!(status + .formattedMessage + .message + .contains("GuestProxyAgent: Stopped")); - #[test] - fn test_should_force_recompute() { - // First observation ever (no previous value) -> never force, nothing to compare against - assert!(!super::should_force_recompute(None, true)); - assert!(!super::should_force_recompute(None, false)); - - // No change since previous iteration -> don't force - assert!(!super::should_force_recompute(Some(true), true)); - assert!(!super::should_force_recompute(Some(false), false)); - - // Aggregate status just recovered (was failing, now succeeding) -> force recompute so - // a stale cached Error substatus doesn't keep overriding the fresh success. - assert!(super::should_force_recompute(Some(false), true)); - - // Aggregate status just broke (was succeeding, now failing) -> force recompute so a - // stale cached healthy substatus doesn't keep masking the new failure. - assert!(super::should_force_recompute(Some(true), false)); + // Both healthy -> no override at all, message left untouched + let mut status = make_test_status_obj( + constants::SUCCESS_STATUS, + constants::STATUS_CODE_OK, + "ProxyAgent extension is reporting successful status.", + ); + super::apply_service_health_overrides( + &mut status, + &healthy_ebpf_sub, + &healthy_gpa_sub, + "ts", + ); + assert_eq!(status.status, constants::SUCCESS_STATUS); + assert_eq!( + status.formattedMessage.message, + "ProxyAgent extension is reporting successful status." + ); } #[tokio::test] From 561ae4f7b4a18773be286c2c03c5cb60a35b8656 Mon Sep 17 00:00:00 2001 From: Srikrishna Veturi Date: Tue, 8 Sep 2026 14:34:54 -0600 Subject: [PATCH 4/6] Address maintainer review feedback and simplify service-health status code - Consolidate apply_ebpf_status_override and apply_gpa_service_status_override, which were identical apart from which SubStatus they read, into one apply_sub_status_override_in_error function used by both call sites (per review feedback: 'apply_gpa_service_status_override is the same as apply_ebpf_status_override... suggest keeping one'). - Replace map_is_active_output's (bool, bool, String) tuple return with a named ActiveState struct, so each field is self-documenting at call sites (per review feedback: 'you are going to kill me with this 3 (bool, bool, string), it is hard to maintain'). - Extract a shared combined_service_health helper encapsulating the Success/Error/Transitioning decision rule that build_ebpf_substatus (3 services) and build_proxy_agent_service_substatus (1 service) had each implemented separately, removing ~35 duplicated lines and centralizing the rule in one tested place. - Simplify query_service_run_status to delegate to the pre-existing check_service_status function instead of re-implementing the same SCM query_service_status/query_service_config lookup a second time, removing ~20 duplicated lines. - Add test_combined_service_health and update existing tests for the ActiveState struct. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- proxy_agent_extension/src/service_main.rs | 330 ++++++++---------- .../src/service/linux_service.rs | 89 +++-- .../src/service/windows_service.rs | 54 +-- 3 files changed, 236 insertions(+), 237 deletions(-) diff --git a/proxy_agent_extension/src/service_main.rs b/proxy_agent_extension/src/service_main.rs index 8baaa7c1..d9555d28 100644 --- a/proxy_agent_extension/src/service_main.rs +++ b/proxy_agent_extension/src/service_main.rs @@ -347,7 +347,7 @@ async fn monitor_thread() { status.substatus.push(ebpf_substatus); } #[cfg(not(windows))] - apply_gpa_service_status_override( + apply_sub_status_override_in_error( &mut status, &gpa_service_substatus, &last_known_status_timestamp, @@ -385,44 +385,51 @@ fn write_state_event( } } -#[cfg(windows)] -fn build_ebpf_substatus( - core: &proxy_agent_shared::service::ServiceStatusInfo, - ext: &proxy_agent_shared::service::ServiceStatusInfo, - svc: &proxy_agent_shared::service::ServiceStatusInfo, -) -> SubStatus { - use proxy_agent_shared::service::classify_service_state; - - let (core_running, core_transitioning) = classify_service_state(core.state.as_ref()); - let (ext_running, ext_transitioning) = classify_service_state(ext.state.as_ref()); - let (svc_running, svc_transitioning) = classify_service_state(svc.state.as_ref()); - - let all_running = core_running && ext_running && svc_running; - // "Down" means confirmed not-running and not actively transitioning toward Running. - let any_down = (!core_running && !core_transitioning) - || (!ext_running && !ext_transitioning) - || (!svc_running && !svc_transitioning); - - let (status, code) = if all_running { - ( +/// Classifies the combined health of one or more services into an overall (status, code) pair, +/// using the same rule everywhere a substatus reports on N services (eBPF's 3 services, or the +/// GuestProxyAgent service's 1): Success only if every service is running; Error if any service +/// is confirmed down (not running and not transitioning toward running); otherwise Transitioning +/// (nothing is confirmed down, but at least one service is still starting up). Reporting +/// Transitioning instead of Error for that last case is what keeps the immediate top-level +/// override (`apply_sub_status_override_in_error`, which only fires on Error) from firing on a +/// service that is simply mid-boot or mid-restart. +fn combined_service_health(services: &[(bool, bool)]) -> (String, i32) { + let all_running = services.iter().all(|(is_running, _)| *is_running); + if all_running { + return ( constants::SUCCESS_STATUS.to_string(), constants::STATUS_CODE_OK, - ) - } else if any_down { + ); + } + let any_down = services + .iter() + .any(|(is_running, is_transitioning)| !is_running && !is_transitioning); + if any_down { ( constants::ERROR_STATUS.to_string(), constants::STATUS_CODE_NOT_OK, ) } else { - // None are confirmed down, but at least one is still starting up (StartPending / - // ContinuePending) - a normal, usually brief condition during boot or a restart. - // Report Transitioning instead of Error so the immediate top-level override - // (`apply_ebpf_status_override`) does not fire on this benign condition. ( constants::TRANSITIONING_STATUS.to_string(), constants::STATUS_CODE_OK, ) - }; + } +} + +#[cfg(windows)] +fn build_ebpf_substatus( + core: &proxy_agent_shared::service::ServiceStatusInfo, + ext: &proxy_agent_shared::service::ServiceStatusInfo, + svc: &proxy_agent_shared::service::ServiceStatusInfo, +) -> SubStatus { + use proxy_agent_shared::service::classify_service_state; + + let (status, code) = combined_service_health(&[ + classify_service_state(core.state.as_ref()), + classify_service_state(ext.state.as_ref()), + classify_service_state(svc.state.as_ref()), + ]); let message = format!( "EbpfCore: {}, NetEbpfExt: {}, eBPFSvc: {}", @@ -461,26 +468,7 @@ fn compute_ebpf_substatus() -> SubStatus { fn build_proxy_agent_service_substatus( info: &proxy_agent_shared::service::ServiceRuntimeStatus, ) -> SubStatus { - let (status, code) = if info.is_running { - ( - constants::SUCCESS_STATUS.to_string(), - constants::STATUS_CODE_OK, - ) - } else if info.is_transitioning { - // Actively starting up (Windows StartPending/ContinuePending, or systemd - // "activating") - a normal, usually brief condition during boot or a restart. - // Report Transitioning instead of Error so the immediate top-level override - // (`apply_gpa_service_status_override`) does not fire on this benign condition. - ( - constants::TRANSITIONING_STATUS.to_string(), - constants::STATUS_CODE_OK, - ) - } else { - ( - constants::ERROR_STATUS.to_string(), - constants::STATUS_CODE_NOT_OK, - ) - }; + let (status, code) = combined_service_health(&[(info.is_running, info.is_transitioning)]); SubStatus { name: constants::PROXY_AGENT_SERVICE_SUBSTATUS_NAME.to_string(), @@ -504,48 +492,25 @@ fn compute_gpa_service_substatus() -> SubStatus { build_proxy_agent_service_substatus(&info) } -/// If `ebpf_substatus` reports Error, unconditionally overrides `status`'s top-level -/// status/code/message to surface the eBPF detail plus the last known status timestamp and the -/// current time. Bypasses the debounce state machine intentionally. Returns true if it applied -/// the override (used by the caller to give this priority over the GPA-service override). -#[cfg(windows)] -fn apply_ebpf_status_override( +/// If `substatus` reports Error, unconditionally overrides `status`'s top-level +/// status/code/message to surface the substatus detail plus the last known status timestamp and +/// the current time. Bypasses the debounce state machine intentionally. Returns true if it +/// applied the override. Shared by both the (Windows-only) eBPF substatus and the +/// (cross-platform) GuestProxyAgent-service substatus - the override behavior itself does not +/// depend on which substatus is being checked. +fn apply_sub_status_override_in_error( status: &mut StatusObj, - ebpf_substatus: &SubStatus, + substatus: &SubStatus, last_known_status_timestamp: &str, ) -> bool { - if ebpf_substatus.status != constants::ERROR_STATUS { + if substatus.status != constants::ERROR_STATUS { return false; } status.status = constants::ERROR_STATUS.to_string(); status.code = constants::STATUS_CODE_NOT_OK; status.formattedMessage.message = format!( "{}. Last status timestamp: {}, Current time: {}", - ebpf_substatus.formattedMessage.message, - last_known_status_timestamp, - misc_helpers::get_current_utc_time() - ); - true -} - -/// If `gpa_service_substatus` reports Error, unconditionally overrides `status`'s top-level -/// status/code/message to surface the GuestProxyAgent service detail plus the last known status -/// timestamp and the current time. Bypasses the debounce state machine intentionally, mirroring -/// `apply_ebpf_status_override`. Cross-platform (Windows and Linux). Returns true if it applied -/// the override. -fn apply_gpa_service_status_override( - status: &mut StatusObj, - gpa_service_substatus: &SubStatus, - last_known_status_timestamp: &str, -) -> bool { - if gpa_service_substatus.status != constants::ERROR_STATUS { - return false; - } - status.status = constants::ERROR_STATUS.to_string(); - status.code = constants::STATUS_CODE_NOT_OK; - status.formattedMessage.message = format!( - "{}. Last status timestamp: {}, Current time: {}", - gpa_service_substatus.formattedMessage.message, + substatus.formattedMessage.message, last_known_status_timestamp, misc_helpers::get_current_utc_time() ); @@ -563,8 +528,8 @@ fn apply_service_health_overrides( gpa_service_substatus: &SubStatus, last_known_status_timestamp: &str, ) { - if !apply_ebpf_status_override(status, ebpf_substatus, last_known_status_timestamp) { - apply_gpa_service_status_override( + if !apply_sub_status_override_in_error(status, ebpf_substatus, last_known_status_timestamp) { + apply_sub_status_override_in_error( status, gpa_service_substatus, last_known_status_timestamp, @@ -1488,6 +1453,72 @@ mod tests { } } + #[test] + fn test_combined_service_health() { + let running = (true, false); + let starting_up = (false, true); + let down = (false, false); + + // All running -> Success + assert_eq!( + super::combined_service_health(&[running, running, running]), + ( + constants::SUCCESS_STATUS.to_string(), + constants::STATUS_CODE_OK + ) + ); + + // Single service, running -> Success (covers the GuestProxyAgent-service call shape) + assert_eq!( + super::combined_service_health(&[running]), + ( + constants::SUCCESS_STATUS.to_string(), + constants::STATUS_CODE_OK + ) + ); + + // Any confirmed down -> Error, regardless of how many other services are running + assert_eq!( + super::combined_service_health(&[running, down, running]), + ( + constants::ERROR_STATUS.to_string(), + constants::STATUS_CODE_NOT_OK + ) + ); + assert_eq!( + super::combined_service_health(&[down]), + ( + constants::ERROR_STATUS.to_string(), + constants::STATUS_CODE_NOT_OK + ) + ); + + // Nothing confirmed down, but something is still starting up -> Transitioning + assert_eq!( + super::combined_service_health(&[running, starting_up, running]), + ( + constants::TRANSITIONING_STATUS.to_string(), + constants::STATUS_CODE_OK + ) + ); + assert_eq!( + super::combined_service_health(&[starting_up]), + ( + constants::TRANSITIONING_STATUS.to_string(), + constants::STATUS_CODE_OK + ) + ); + + // A confirmed-down service takes priority over a transitioning one + assert_eq!( + super::combined_service_health(&[starting_up, down]), + ( + constants::ERROR_STATUS.to_string(), + constants::STATUS_CODE_NOT_OK + ) + ); + } + #[test] #[cfg(windows)] fn test_build_ebpf_substatus() { @@ -1634,8 +1665,8 @@ mod tests { // 10. Core starting up (StartPending), Ext+Svc running → Transitioning, not Error. // Regression test: a service mid-boot/mid-restart must not immediately flip the - // top-level extension status to Error (see apply_ebpf_status_override, which only - // fires on ERROR_STATUS). + // top-level extension status to Error (see apply_sub_status_override_in_error, which + // only fires on ERROR_STATUS). let sub = super::build_ebpf_substatus( &make_info(constants::EBPF_CORE, Some(ServiceState::StartPending)), &make_info(constants::EBPF_EXT, running()), @@ -1753,7 +1784,7 @@ mod tests { // Starting up (StartPending on Windows / "activating" on Linux) → Transitioning, not // Error. Regression test: a service mid-boot/mid-restart must not immediately flip the - // top-level extension status to Error (see apply_gpa_service_status_override, which + // top-level extension status to Error (see apply_sub_status_override_in_error, which // only fires on ERROR_STATUS). let info = ServiceRuntimeStatus { service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), @@ -1780,10 +1811,12 @@ mod tests { } #[test] - #[cfg(windows)] - fn test_apply_ebpf_status_override() { - let make_ebpf_sub = |status: &str, message: &str| SubStatus { - name: constants::EBPF_SUBSTATUS_NAME.to_string(), + fn test_apply_sub_status_override_in_error() { + // The override behavior is identical regardless of which substatus (eBPF or + // GuestProxyAgent-service) is passed in, so a single test exercises the shared + // function with representative substatus shapes from both call sites. + let make_sub = |name: &str, status: &str, message: &str| SubStatus { + name: name.to_string(), status: status.to_string(), code: if status == constants::ERROR_STATUS { constants::STATUS_CODE_NOT_OK @@ -1796,17 +1829,19 @@ mod tests { }, }; - // eBPF Error overrides an otherwise-Success status + let ebpf_sub = make_sub( + constants::EBPF_SUBSTATUS_NAME, + constants::ERROR_STATUS, + "EbpfCore: Running, AutoStart, NetEbpfExt: Stopped, AutoStart, eBPFSvc: Running, AutoStart", + ); + + // Error substatus overrides an otherwise-Success status let mut status = make_test_status_obj( constants::SUCCESS_STATUS, constants::STATUS_CODE_OK, "ProxyAgent extension is reporting successful status.", ); - let ebpf_sub = make_ebpf_sub( - constants::ERROR_STATUS, - "EbpfCore: Running, AutoStart, NetEbpfExt: Stopped, AutoStart, eBPFSvc: Running, AutoStart", - ); - let overridden = super::apply_ebpf_status_override( + let overridden = super::apply_sub_status_override_in_error( &mut status, &ebpf_sub, "2026-08-21 8:13:38.104 +00:00:00", @@ -1824,13 +1859,13 @@ mod tests { .contains("Last status timestamp: 2026-08-21 8:13:38.104 +00:00:00")); assert!(status.formattedMessage.message.contains("Current time:")); - // eBPF Error overrides an already-Error stale message too + // Error substatus overrides an already-Error stale message too let mut status = make_test_status_obj( constants::ERROR_STATUS, constants::STATUS_CODE_NOT_OK, "Proxy agent aggregate status file is stale. Status timestamp: ..., Current time: ...", ); - let overridden = super::apply_ebpf_status_override( + let overridden = super::apply_sub_status_override_in_error( &mut status, &ebpf_sub, "2026-08-21 8:13:38.104 +00:00:00", @@ -1842,18 +1877,19 @@ mod tests { .message .contains("NetEbpfExt: Stopped")); - // eBPF healthy leaves the existing message untouched + // Healthy substatus (eBPF-shaped) leaves the existing message untouched let mut status = make_test_status_obj( constants::SUCCESS_STATUS, constants::STATUS_CODE_OK, "ProxyAgent extension is reporting successful status.", ); - let healthy_ebpf_sub = make_ebpf_sub( + let healthy_ebpf_sub = make_sub( + constants::EBPF_SUBSTATUS_NAME, constants::SUCCESS_STATUS, "EbpfCore: Running, AutoStart, NetEbpfExt: Running, AutoStart, eBPFSvc: Running, AutoStart", ); let overridden = - super::apply_ebpf_status_override(&mut status, &healthy_ebpf_sub, "irrelevant"); + super::apply_sub_status_override_in_error(&mut status, &healthy_ebpf_sub, "irrelevant"); assert!(!overridden); assert_eq!(status.status, constants::SUCCESS_STATUS); assert_eq!( @@ -1861,7 +1897,7 @@ mod tests { "ProxyAgent extension is reporting successful status." ); - // eBPF Transitioning (e.g. a service mid-boot/mid-restart) must NOT trigger the + // Transitioning substatus (e.g. a service mid-boot/mid-restart) must NOT trigger the // override - regression test for the reviewer finding that this override previously // fired immediately on any non-Running state, including benign transitional ones. let mut status = make_test_status_obj( @@ -1869,54 +1905,42 @@ mod tests { constants::STATUS_CODE_OK, "ProxyAgent extension is reporting successful status.", ); - let transitioning_ebpf_sub = make_ebpf_sub( + let transitioning_ebpf_sub = make_sub( + constants::EBPF_SUBSTATUS_NAME, constants::TRANSITIONING_STATUS, "EbpfCore: Running, AutoStart, NetEbpfExt: StartPending, AutoStart, eBPFSvc: Running, AutoStart", ); - let overridden = - super::apply_ebpf_status_override(&mut status, &transitioning_ebpf_sub, "irrelevant"); + let overridden = super::apply_sub_status_override_in_error( + &mut status, + &transitioning_ebpf_sub, + "irrelevant", + ); assert!( !overridden, - "Transitioning eBPF substatus must not trigger the immediate override" + "Transitioning substatus must not trigger the immediate override" ); assert_eq!(status.status, constants::SUCCESS_STATUS); assert_eq!( status.formattedMessage.message, "ProxyAgent extension is reporting successful status." ); - } - #[test] - fn test_apply_gpa_service_status_override() { - let make_gpa_sub = |status: &str, message: &str| SubStatus { - name: constants::PROXY_AGENT_SERVICE_SUBSTATUS_NAME.to_string(), - status: status.to_string(), - code: if status == constants::ERROR_STATUS { - constants::STATUS_CODE_NOT_OK - } else { - constants::STATUS_CODE_OK - }, - formattedMessage: FormattedMessage { - lang: constants::LANG_EN_US.to_string(), - message: message.to_string(), - }, - }; - - // GPA-service Error overrides an otherwise-Success status immediately (no gating on - // top-level already being Error) + // GuestProxyAgent-service-shaped substatus works identically (Error overrides, with + // its own message content and no gating on the top-level status already being Error) let mut status = make_test_status_obj( constants::SUCCESS_STATUS, constants::STATUS_CODE_OK, "ProxyAgent extension is reporting successful status.", ); - let gpa_sub = make_gpa_sub( + let gpa_sub = make_sub( + constants::PROXY_AGENT_SERVICE_SUBSTATUS_NAME, constants::ERROR_STATUS, &format!( "{}: Stopped, AutoStart", constants::PROXY_AGENT_SERVICE_NAME ), ); - let overridden = super::apply_gpa_service_status_override( + let overridden = super::apply_sub_status_override_in_error( &mut status, &gpa_sub, "2026-08-21 8:13:38.104 +00:00:00", @@ -1932,58 +1956,6 @@ mod tests { .formattedMessage .message .contains("Last status timestamp: 2026-08-21 8:13:38.104 +00:00:00")); - assert!(status.formattedMessage.message.contains("Current time:")); - - // GPA-service healthy leaves the existing message untouched - let mut status = make_test_status_obj( - constants::SUCCESS_STATUS, - constants::STATUS_CODE_OK, - "ProxyAgent extension is reporting successful status.", - ); - let healthy_gpa_sub = make_gpa_sub( - constants::SUCCESS_STATUS, - &format!( - "{}: Running, AutoStart", - constants::PROXY_AGENT_SERVICE_NAME - ), - ); - let overridden = - super::apply_gpa_service_status_override(&mut status, &healthy_gpa_sub, "irrelevant"); - assert!(!overridden); - assert_eq!( - status.formattedMessage.message, - "ProxyAgent extension is reporting successful status." - ); - - // GPA-service Transitioning (e.g. mid-boot/mid-restart) must NOT trigger the override - - // regression test for the reviewer finding that this override previously fired - // immediately on any non-Running state, including benign transitional ones. - let mut status = make_test_status_obj( - constants::SUCCESS_STATUS, - constants::STATUS_CODE_OK, - "ProxyAgent extension is reporting successful status.", - ); - let transitioning_gpa_sub = make_gpa_sub( - constants::TRANSITIONING_STATUS, - &format!( - "{}: StartPending, AutoStart", - constants::PROXY_AGENT_SERVICE_NAME - ), - ); - let overridden = super::apply_gpa_service_status_override( - &mut status, - &transitioning_gpa_sub, - "irrelevant", - ); - assert!( - !overridden, - "Transitioning GPA-service substatus must not trigger the immediate override" - ); - assert_eq!(status.status, constants::SUCCESS_STATUS); - assert_eq!( - status.formattedMessage.message, - "ProxyAgent extension is reporting successful status." - ); } #[test] diff --git a/proxy_agent_shared/src/service/linux_service.rs b/proxy_agent_shared/src/service/linux_service.rs index 3f0e37f4..d6771907 100644 --- a/proxy_agent_shared/src/service/linux_service.rs +++ b/proxy_agent_shared/src/service/linux_service.rs @@ -188,22 +188,41 @@ pub fn check_service_installed(service_name: &str) -> (bool, String) { } } -/// Maps the trimmed stdout of `systemctl is-active ` to -/// (is_running, is_transitioning, state_display). +/// The subset of a service's runtime state derived from `systemctl is-active` output. A named +/// struct (rather than a bare tuple) so each field is self-documenting at every call site. +#[derive(Debug, PartialEq)] +struct ActiveState { + is_running: bool, + is_transitioning: bool, + state_display: String, +} + +/// Maps the trimmed stdout of `systemctl is-active ` to an `ActiveState`. /// `activating` mirrors Windows `StartPending`/`ContinuePending`: the unit is heading *toward* /// active and this is a normal, usually brief condition during boot or a restart, so it is /// intentionally distinguished from a confirmed failure. `deactivating` mirrors Windows /// `StopPending`: the unit is heading *away* from active, which is treated as a confirmed down /// state (not transitioning), since it's actionable information worth surfacing immediately. /// Pure function so it is unit-testable without shelling out to `systemctl`. -fn map_is_active_output(output: &str) -> (bool, bool, String) { - match output.trim() { - "active" => (true, false, "Running".to_string()), - "inactive" => (false, false, "Stopped".to_string()), - "failed" => (false, false, "Failed".to_string()), - "activating" => (false, true, "Activating".to_string()), - "deactivating" => (false, false, "Deactivating".to_string()), - other => (false, false, capitalize_first(other)), +fn map_is_active_output(output: &str) -> ActiveState { + let (is_running, is_transitioning, state_display) = match output.trim() { + "active" => (true, false, "Running"), + "inactive" => (false, false, "Stopped"), + "failed" => (false, false, "Failed"), + "activating" => (false, true, "Activating"), + "deactivating" => (false, false, "Deactivating"), + other => { + return ActiveState { + is_running: false, + is_transitioning: false, + state_display: capitalize_first(other), + } + } + }; + ActiveState { + is_running, + is_transitioning, + state_display: state_display.to_string(), } } @@ -246,14 +265,18 @@ pub fn check_service_run_status(service_name: &str) -> crate::service::ServiceRu }; } - let (is_running, is_transitioning, state_display) = + let active_state = match misc_helpers::execute_command("systemctl", vec!["is-active", service_name], -1) { Ok(output) => map_is_active_output(&output.stdout()), Err(e) => { logger_manager::write_info(format!( "check_service_run_status: failed to query is-active for {service_name}: {e}" )); - (false, false, "Unknown".to_string()) + ActiveState { + is_running: false, + is_transitioning: false, + state_display: "Unknown".to_string(), + } } }; @@ -271,9 +294,9 @@ pub fn check_service_run_status(service_name: &str) -> crate::service::ServiceRu crate::service::ServiceRuntimeStatus { service_name: service_name.to_string(), is_installed: true, - is_running, - is_transitioning, - state_display, + is_running: active_state.is_running, + is_transitioning: active_state.is_transitioning, + state_display: active_state.state_display, start_type_display, } } @@ -286,30 +309,54 @@ mod tests { fn map_is_active_output_test() { assert_eq!( map_is_active_output("active\n"), - (true, false, "Running".to_string()) + ActiveState { + is_running: true, + is_transitioning: false, + state_display: "Running".to_string() + } ); assert_eq!( map_is_active_output("inactive\n"), - (false, false, "Stopped".to_string()) + ActiveState { + is_running: false, + is_transitioning: false, + state_display: "Stopped".to_string() + } ); assert_eq!( map_is_active_output("failed\n"), - (false, false, "Failed".to_string()) + ActiveState { + is_running: false, + is_transitioning: false, + state_display: "Failed".to_string() + } ); // "activating" is transitioning toward Running - not a confirmed failure. assert_eq!( map_is_active_output("activating\n"), - (false, true, "Activating".to_string()) + ActiveState { + is_running: false, + is_transitioning: true, + state_display: "Activating".to_string() + } ); // "deactivating" is heading away from Running - treated as a confirmed down state, // consistent with Windows StopPending, since it's actionable to know immediately. assert_eq!( map_is_active_output("deactivating\n"), - (false, false, "Deactivating".to_string()) + ActiveState { + is_running: false, + is_transitioning: false, + state_display: "Deactivating".to_string() + } ); assert_eq!( map_is_active_output("unknown\n"), - (false, false, "Unknown".to_string()) + ActiveState { + is_running: false, + is_transitioning: false, + state_display: "Unknown".to_string() + } ); } diff --git a/proxy_agent_shared/src/service/windows_service.rs b/proxy_agent_shared/src/service/windows_service.rs index faff2905..793fc03a 100644 --- a/proxy_agent_shared/src/service/windows_service.rs +++ b/proxy_agent_shared/src/service/windows_service.rs @@ -238,44 +238,24 @@ pub fn classify_service_state(state: Option<&ServiceState>) -> (bool, bool) { } } -/// Queries a service's runtime status in the cross-platform `ServiceRuntimeStatus` shape, -/// re-mapping the same data already fetched by `check_service_status`/`query_service_config`. +/// Queries a service's runtime status in the cross-platform `ServiceRuntimeStatus` shape. +/// Delegates the actual SCM query to `check_service_status` (the same function used for the +/// Windows-only eBPF substatus) instead of re-implementing the query + config lookup a second +/// time, so there is a single place that knows how to ask the SCM about a service. pub fn query_service_run_status(service_name: &str) -> crate::service::ServiceRuntimeStatus { - match query_service_status(service_name) { - Ok(status) => { - let start_type_display = match query_service_config(service_name) { - Ok(config) => format!("{:?}", config.start_type), - Err(e) => { - logger_manager::write_info(format!( - "Failed to query config for service '{service_name}': {e}", - )); - "Unknown".to_string() - } - }; - let (is_running, is_transitioning) = - classify_service_state(Some(&status.current_state)); - crate::service::ServiceRuntimeStatus { - service_name: service_name.to_string(), - is_installed: true, - is_running, - is_transitioning, - state_display: format!("{:?}", status.current_state), - start_type_display, - } - } - Err(e) => { - logger_manager::write_info(format!( - "Failed to query status for service '{service_name}': {e}. Treating as not installed.", - )); - crate::service::ServiceRuntimeStatus { - service_name: service_name.to_string(), - is_installed: false, - is_running: false, - is_transitioning: false, - state_display: "NotInstalled".to_string(), - start_type_display: "NotInstalled".to_string(), - } - } + let info = crate::service::check_service_status(service_name); + let (is_running, is_transitioning) = classify_service_state(info.state.as_ref()); + let state_display = match &info.state { + Some(state) => format!("{state:?}"), + None => "NotInstalled".to_string(), + }; + crate::service::ServiceRuntimeStatus { + is_installed: info.state.is_some(), + is_running, + is_transitioning, + state_display, + start_type_display: info.start_type, + service_name: info.service_name, } } From 303033b09905049493cfb1eaf89d55fe8bda3622 Mon Sep 17 00:00:00 2001 From: Srikrishna Veturi Date: Tue, 8 Sep 2026 15:54:22 -0600 Subject: [PATCH 5/6] Shorten overly long comments to 2-3 lines Comment-only change: trimmed 12 doc/inline comment blocks (ranging 4-14 lines) that were added in the preceding commits down to 2-3 lines each, keeping only the essential what/why. No code or logic was changed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- proxy_agent_extension/src/service_main.rs | 65 +++++-------------- proxy_agent_shared/src/service.rs | 16 ++--- .../src/service/linux_service.rs | 10 +-- .../src/service/windows_service.rs | 18 ++--- 4 files changed, 32 insertions(+), 77 deletions(-) diff --git a/proxy_agent_extension/src/service_main.rs b/proxy_agent_extension/src/service_main.rs index d9555d28..1b760605 100644 --- a/proxy_agent_extension/src/service_main.rs +++ b/proxy_agent_extension/src/service_main.rs @@ -320,20 +320,9 @@ async fn monitor_thread() { proxy_agent_update_reported = None; } - // Step 6: Report eBPF (Windows only) and GuestProxyAgent service (cross-platform) - // runtime status, computed fresh every loop_interval tick (same cadence as everything - // else in this loop, currently 15s) so the status file always reflects the current - // machine state. - // - // Step 7: Apply an immediate top-level status/message override when eBPF (Windows, - // highest priority) or the GuestProxyAgent service itself (both platforms) is - // unhealthy. This is still necessary even though the substatus data above is always - // fresh: the top-level status/message is derived from a *different* source (the GPA - // aggregate status file/wire-server response via Step 3), which has its own 5-minute - // staleness threshold and 20-iteration debounce designed to avoid flapping on transient - // network blips - it has no visibility into eBPF or the GuestProxyAgent service at all. - // Without this override, a locally-confirmed eBPF/service failure would still take - // several minutes to surface at the top level; see `apply_service_health_overrides`. + // Step 6/7: report eBPF (Windows) + GuestProxyAgent service (cross-platform) status + // fresh every tick, then immediately override the top-level status/message on Error - + // the aggregate-status source (Step 3) has its own staleness/debounce and can't see these. let gpa_service_substatus = compute_gpa_service_substatus(); #[cfg(windows)] { @@ -385,14 +374,9 @@ fn write_state_event( } } -/// Classifies the combined health of one or more services into an overall (status, code) pair, -/// using the same rule everywhere a substatus reports on N services (eBPF's 3 services, or the -/// GuestProxyAgent service's 1): Success only if every service is running; Error if any service -/// is confirmed down (not running and not transitioning toward running); otherwise Transitioning -/// (nothing is confirmed down, but at least one service is still starting up). Reporting -/// Transitioning instead of Error for that last case is what keeps the immediate top-level -/// override (`apply_sub_status_override_in_error`, which only fires on Error) from firing on a -/// service that is simply mid-boot or mid-restart. +/// Classifies combined health of N services: Success if all running, Error if any is +/// confirmed down, else Transitioning (something is merely starting up). Transitioning +/// prevents `apply_sub_status_override_in_error` from firing on a benign restart. fn combined_service_health(services: &[(bool, bool)]) -> (String, i32) { let all_running = services.iter().all(|(is_running, _)| *is_running); if all_running { @@ -492,12 +476,9 @@ fn compute_gpa_service_substatus() -> SubStatus { build_proxy_agent_service_substatus(&info) } -/// If `substatus` reports Error, unconditionally overrides `status`'s top-level -/// status/code/message to surface the substatus detail plus the last known status timestamp and -/// the current time. Bypasses the debounce state machine intentionally. Returns true if it -/// applied the override. Shared by both the (Windows-only) eBPF substatus and the -/// (cross-platform) GuestProxyAgent-service substatus - the override behavior itself does not -/// depend on which substatus is being checked. +/// If `substatus` is Error, overrides `status`'s top-level status/code/message with the +/// substatus detail plus the last known timestamp and current time, bypassing the debounce +/// state machine. Shared by the eBPF and GuestProxyAgent-service override call sites. fn apply_sub_status_override_in_error( status: &mut StatusObj, substatus: &SubStatus, @@ -517,10 +498,8 @@ fn apply_sub_status_override_in_error( true } -/// Applies the Windows-only priority ordering between the two immediate overrides: eBPF errors -/// win over GuestProxyAgent-service errors, since an unhealthy eBPF is frequently the underlying -/// reason the GuestProxyAgent service itself cannot start, making it the more specific/actionable -/// signal. Only falls through to the GPA-service override when eBPF itself did not report Error. +/// Windows-only priority: eBPF errors win over GuestProxyAgent-service errors, since an +/// unhealthy eBPF is often the underlying cause. Falls through only if eBPF is healthy. #[cfg(windows)] fn apply_service_health_overrides( status: &mut StatusObj, @@ -1422,13 +1401,9 @@ mod tests { #[test] fn test_compute_gpa_service_substatus() { - // Cross-platform (unlike compute_ebpf_substatus, not gated to Windows): exercises the - // real check_service_run_status call (SCM on Windows, systemctl on Linux) against - // whatever GuestProxyAgent service state the test runner happens to have, and verifies - // the result is well-formed and internally consistent regardless of that state. This - // backfills test coverage for a function introduced in the prior commit that previously - // had no dedicated test (only its pure `build_proxy_agent_service_substatus` helper was - // tested). + // Cross-platform (unlike compute_ebpf_substatus): exercises the real + // check_service_run_status call against whatever GuestProxyAgent service state the + // test runner has, and just checks the result is internally consistent. let substatus = super::compute_gpa_service_substatus(); assert_eq!( substatus.name, @@ -1663,10 +1638,8 @@ mod tests { assert_eq!(sub.status, constants::ERROR_STATUS, "All three stopped"); assert_eq!(sub.code, constants::STATUS_CODE_NOT_OK); - // 10. Core starting up (StartPending), Ext+Svc running → Transitioning, not Error. - // Regression test: a service mid-boot/mid-restart must not immediately flip the - // top-level extension status to Error (see apply_sub_status_override_in_error, which - // only fires on ERROR_STATUS). + // 10. Core starting up (StartPending), Ext+Svc running → Transitioning, not Error + // (a mid-restart service must not immediately flip the top-level status to Error). let sub = super::build_ebpf_substatus( &make_info(constants::EBPF_CORE, Some(ServiceState::StartPending)), &make_info(constants::EBPF_EXT, running()), @@ -1782,10 +1755,8 @@ mod tests { format!("{}: NotInstalled", constants::PROXY_AGENT_SERVICE_NAME) ); - // Starting up (StartPending on Windows / "activating" on Linux) → Transitioning, not - // Error. Regression test: a service mid-boot/mid-restart must not immediately flip the - // top-level extension status to Error (see apply_sub_status_override_in_error, which - // only fires on ERROR_STATUS). + // Starting up (StartPending/"activating") → Transitioning, not Error (a mid-restart + // service must not immediately flip the top-level status to Error). let info = ServiceRuntimeStatus { service_name: constants::PROXY_AGENT_SERVICE_NAME.to_string(), is_installed: true, diff --git a/proxy_agent_shared/src/service.rs b/proxy_agent_shared/src/service.rs index c3367da4..96c2086e 100644 --- a/proxy_agent_shared/src/service.rs +++ b/proxy_agent_shared/src/service.rs @@ -184,21 +184,17 @@ pub use windows_service::ServiceState; #[cfg(windows)] pub use windows_service::ServiceStatusInfo; -/// Cross-platform runtime status of a system service (Windows SCM or Linux systemd), -/// used for reporting service health that is meaningful on both platforms (e.g. the -/// GuestProxyAgent service itself). Unlike `ServiceStatusInfo` (Windows-only, used for -/// the Windows-specific eBPF driver/service substatus), this type has an implementation -/// on every platform. +/// Cross-platform runtime status of a system service (Windows SCM or Linux systemd), used for +/// service health that's meaningful on both platforms. Unlike the Windows-only +/// `ServiceStatusInfo`, this type has an implementation on every platform. #[derive(Debug, Clone, PartialEq, Eq)] pub struct ServiceRuntimeStatus { pub service_name: String, pub is_installed: bool, pub is_running: bool, - /// True when the service is actively transitioning *toward* a running state (e.g. - /// Windows `StartPending`/`ContinuePending`, or systemd `activating`). This is a normal, - /// usually brief condition during boot or a service restart and is intentionally treated - /// as distinct from a confirmed failure (`is_running == false && is_transitioning == - /// false`), so callers don't have to treat "still starting up" the same as "actually down". + /// True when the service is actively starting (e.g. Windows `StartPending`/ + /// `ContinuePending`, or systemd `activating`) - a normal transient state, distinct from a + /// confirmed failure (`is_running == false && is_transitioning == false`). pub is_transitioning: bool, /// Human-readable running state, e.g. "Running", "Stopped", "Failed". pub state_display: String, diff --git a/proxy_agent_shared/src/service/linux_service.rs b/proxy_agent_shared/src/service/linux_service.rs index d6771907..acec2610 100644 --- a/proxy_agent_shared/src/service/linux_service.rs +++ b/proxy_agent_shared/src/service/linux_service.rs @@ -197,13 +197,9 @@ struct ActiveState { state_display: String, } -/// Maps the trimmed stdout of `systemctl is-active ` to an `ActiveState`. -/// `activating` mirrors Windows `StartPending`/`ContinuePending`: the unit is heading *toward* -/// active and this is a normal, usually brief condition during boot or a restart, so it is -/// intentionally distinguished from a confirmed failure. `deactivating` mirrors Windows -/// `StopPending`: the unit is heading *away* from active, which is treated as a confirmed down -/// state (not transitioning), since it's actionable information worth surfacing immediately. -/// Pure function so it is unit-testable without shelling out to `systemctl`. +/// Maps `systemctl is-active` output to an `ActiveState`. `activating` mirrors Windows +/// `StartPending`/`ContinuePending` (transitioning, not a failure); `deactivating` mirrors +/// `StopPending` (a confirmed down state). Pure function, unit-testable without `systemctl`. fn map_is_active_output(output: &str) -> ActiveState { let (is_running, is_transitioning, state_display) = match output.trim() { "active" => (true, false, "Running"), diff --git a/proxy_agent_shared/src/service/windows_service.rs b/proxy_agent_shared/src/service/windows_service.rs index 793fc03a..b2f5760c 100644 --- a/proxy_agent_shared/src/service/windows_service.rs +++ b/proxy_agent_shared/src/service/windows_service.rs @@ -221,15 +221,9 @@ pub fn query_service_config(service_name: &str) -> Result { .map_err(|e| Error::WindowsService(e, std::io::Error::last_os_error())) } -/// Classifies a Windows service state into (is_running, is_transitioning). -/// `StartPending`/`ContinuePending` are transitioning *toward* Running - a normal, usually -/// brief condition during boot or a service restart, distinct from a confirmed failure. -/// `StopPending`/`PausePending`/`Paused`/`Stopped` (and no state at all) are treated as a -/// confirmed down state, since they are heading away from - or already away from - Running. -/// Pure function so it is unit-testable without a real SCM service. Takes `Option<&ServiceState>` -/// (rather than owning it) so callers don't need `ServiceState` to implement `Copy`/`Clone`, and -/// so it can be reused as-is by `proxy_agent_extension`'s eBPF substatus classification (see -/// `pub use` re-export below). +/// Classifies a Windows service state into (is_running, is_transitioning). `StartPending`/ +/// `ContinuePending` are transitioning toward Running; every other state (including no state) +/// is a confirmed down state. Takes `Option<&ServiceState>` so it's reusable without `Copy`. pub fn classify_service_state(state: Option<&ServiceState>) -> (bool, bool) { match state { Some(ServiceState::Running) => (true, false), @@ -238,10 +232,8 @@ pub fn classify_service_state(state: Option<&ServiceState>) -> (bool, bool) { } } -/// Queries a service's runtime status in the cross-platform `ServiceRuntimeStatus` shape. -/// Delegates the actual SCM query to `check_service_status` (the same function used for the -/// Windows-only eBPF substatus) instead of re-implementing the query + config lookup a second -/// time, so there is a single place that knows how to ask the SCM about a service. +/// Queries a service's runtime status in the cross-platform `ServiceRuntimeStatus` shape, +/// delegating to `check_service_status` instead of re-querying the SCM a second time. pub fn query_service_run_status(service_name: &str) -> crate::service::ServiceRuntimeStatus { let info = crate::service::check_service_status(service_name); let (is_running, is_transitioning) = classify_service_state(info.state.as_ref()); From f83f47861836a678fe09eac0bdba3c4bbbb58a0b Mon Sep 17 00:00:00 2001 From: Srikrishna Veturi Date: Wed, 9 Sep 2026 10:17:09 -0600 Subject: [PATCH 6/6] Address code review: use pub-use re-export pattern for check_service_run_status Renamed windows_service::query_service_run_status to check_service_run_status so both platforms expose the same name, then replaced the hand-written cross-platform dispatcher in service.rs with two cfg-gated pub-use lines, matching the existing re-export pattern used for classify_service_state/set_default_failure_actions/ServiceState/ServiceStatusInfo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d728311e-027c-4d6e-9ec2-331ba40b3c88 --- proxy_agent_shared/src/service.rs | 17 ++++------------- .../src/service/windows_service.rs | 4 ++-- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/proxy_agent_shared/src/service.rs b/proxy_agent_shared/src/service.rs index 96c2086e..f24e395a 100644 --- a/proxy_agent_shared/src/service.rs +++ b/proxy_agent_shared/src/service.rs @@ -175,6 +175,10 @@ pub fn check_service_status(service_name: &str) -> windows_service::ServiceStatu } } +#[cfg(not(windows))] +pub use linux_service::check_service_run_status; +#[cfg(windows)] +pub use windows_service::check_service_run_status; #[cfg(windows)] pub use windows_service::classify_service_state; #[cfg(windows)] @@ -218,19 +222,6 @@ impl ServiceRuntimeStatus { } } -/// Checks the runtime status (running state + start type) of a service in a cross-platform -/// way. Uses the Windows SCM on Windows and `systemctl` on Linux. -pub fn check_service_run_status(service_name: &str) -> ServiceRuntimeStatus { - #[cfg(windows)] - { - windows_service::query_service_run_status(service_name) - } - #[cfg(not(windows))] - { - linux_service::check_service_run_status(service_name) - } -} - #[cfg(test)] mod tests { #[test] diff --git a/proxy_agent_shared/src/service/windows_service.rs b/proxy_agent_shared/src/service/windows_service.rs index b2f5760c..081e4349 100644 --- a/proxy_agent_shared/src/service/windows_service.rs +++ b/proxy_agent_shared/src/service/windows_service.rs @@ -232,9 +232,9 @@ pub fn classify_service_state(state: Option<&ServiceState>) -> (bool, bool) { } } -/// Queries a service's runtime status in the cross-platform `ServiceRuntimeStatus` shape, +/// Checks a service's runtime status in the cross-platform `ServiceRuntimeStatus` shape, /// delegating to `check_service_status` instead of re-querying the SCM a second time. -pub fn query_service_run_status(service_name: &str) -> crate::service::ServiceRuntimeStatus { +pub fn check_service_run_status(service_name: &str) -> crate::service::ServiceRuntimeStatus { let info = crate::service::check_service_status(service_name); let (is_running, is_transitioning) = classify_service_state(info.state.as_ref()); let state_display = match &info.state {