From 2a14fab759235428db4da5f9fb657690e08cc864 Mon Sep 17 00:00:00 2001 From: My Name Date: Tue, 8 Sep 2026 11:48:59 +0000 Subject: [PATCH 1/5] Prevent run_cvd abort and keep monitor socket alive on graceful VM shutdown --- .../commands/run_cvd/server_loop_impl.cpp | 17 +++++++-- .../libs/process_monitor/process_monitor.cc | 38 ++++++++++++++++--- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp index e658ac176f9..4c6288031b7 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp +++ b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp @@ -116,16 +116,27 @@ Result ServerLoopImpl::Run() { CF_EXPECT(process_monitor.StartAndMonitorProcesses()); device_status_ = DeviceStatus::kActive; + bool process_monitor_active = true; while (true) { // TODO: use select to handle simultaneous connections. SharedFDSet read_set; read_set.Set(server_); - read_set.Set(process_monitor.status()); + if (process_monitor_active) { + read_set.Set(process_monitor.status()); + } Select(&read_set, nullptr, nullptr, nullptr); - if (read_set.IsSet(process_monitor.status())) { - return CF_ERR("process monitor has died"); + if (process_monitor_active && read_set.IsSet(process_monitor.status())) { + LOG(INFO) << "Process monitor has exited (guest VM shut down). Server " + "loop continuing to listen for status/restart."; + process_monitor_active = false; + auto stop_result = process_monitor.StopMonitoredProcesses(); + if (!stop_result.has_value()) { + LOG(WARNING) << "Failed to reap process monitor: " + << stop_result.error(); + } + continue; } CF_EXPECT(read_set.IsSet(server_)); diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc index 09cdb92ff4f..7523fc6e17c 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc +++ b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc @@ -357,18 +357,44 @@ ProcessMonitor::ProcessMonitor(ProcessMonitor::Properties&& properties, monitor_(-1) {} Result ProcessMonitor::StopMonitoredProcesses() { - CF_EXPECT(monitor_ != -1, "The monitor process has already exited."); - CF_EXPECT(parent_channel_.has_value(), - "The monitor socket is already closed"); - CF_EXPECT( - SendEmptyRequest(*parent_channel_, ParentToChildMessageType::kStop)); + if (monitor_ == -1) { + return {}; + } + int wstatus = 0; + pid_t wait_res = waitpid(monitor_, &wstatus, WNOHANG); + if (wait_res == monitor_ || (wait_res == -1 && errno == ECHILD)) { + monitor_ = -1; + parent_channel_.reset(); + return {}; + } + bool send_success = false; + if (parent_channel_.has_value()) { + auto send_result = + SendEmptyRequest(*parent_channel_, ParentToChildMessageType::kStop); + if (!send_result.has_value()) { + VLOG(0) << "SendEmptyRequest failed during StopMonitoredProcesses: " + << send_result.error(); + } else { + send_success = true; + } + } pid_t last_monitor = monitor_; monitor_ = -1; parent_channel_.reset(); - int wstatus; CF_EXPECT(waitpid(last_monitor, &wstatus, 0) == last_monitor, "Failed to wait for monitor process"); + if (!send_success) { + // Monitor process was already exiting when stop was requested. + if (WIFSIGNALED(wstatus)) { + LOG(WARNING) << "Monitor process exited due to a signal: " + << WTERMSIG(wstatus); + } else if (WIFEXITED(wstatus) && WEXITSTATUS(wstatus) != 0) { + LOG(WARNING) << "Monitor process exited with code " + << WEXITSTATUS(wstatus); + } + return {}; + } CF_EXPECT(!WIFSIGNALED(wstatus), "Monitor process exited due to a signal"); CF_EXPECT(WIFEXITED(wstatus), "Monitor process exited for unknown reasons"); CF_EXPECT(WEXITSTATUS(wstatus) == 0, From 347003d6e35941066b46df67a3f6fd049b819d91 Mon Sep 17 00:00:00 2001 From: My Name Date: Mon, 14 Sep 2026 16:33:17 +0000 Subject: [PATCH 2/5] Differentiate graceful VM shutdown from unexpected critical process exits --- .../cuttlefish/host/commands/run_cvd/main.cc | 2 + .../commands/run_cvd/server_loop_impl.cpp | 9 +- .../libs/process_monitor/process_monitor.cc | 103 +++++++++++------- 3 files changed, 72 insertions(+), 42 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/main.cc b/base/cvd/cuttlefish/host/commands/run_cvd/main.cc index 9e0a1d1cbde..0f5b9fb2a0a 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/main.cc +++ b/base/cvd/cuttlefish/host/commands/run_cvd/main.cc @@ -15,6 +15,7 @@ */ #include +#include #include #include @@ -245,6 +246,7 @@ void ConfigureLogs(const CuttlefishConfig& config, } // namespace Result RunCvdMain(int argc, char** argv) { + signal(SIGPIPE, SIG_IGN); google::ParseCommandLineFlags(&argc, &argv, false); CF_EXPECT(StdinValid(), "Invalid stdin"); diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp index 4c6288031b7..028139f7407 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp +++ b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp @@ -128,14 +128,15 @@ Result ServerLoopImpl::Run() { Select(&read_set, nullptr, nullptr, nullptr); if (process_monitor_active && read_set.IsSet(process_monitor.status())) { - LOG(INFO) << "Process monitor has exited (guest VM shut down). Server " - "loop continuing to listen for status/restart."; process_monitor_active = false; auto stop_result = process_monitor.StopMonitoredProcesses(); if (!stop_result.has_value()) { - LOG(WARNING) << "Failed to reap process monitor: " - << stop_result.error(); + return CF_ERR( + "process monitor exited unexpectedly: " << stop_result.error()); } + LOG(INFO) + << "Process monitor has exited gracefully (guest VM shut down). " + "Server loop continuing to listen for status/restart."; continue; } diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc index 7523fc6e17c..5770f743fe8 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc +++ b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc @@ -138,12 +138,38 @@ Result MonitorLoop(std::atomic_bool& running, it->proc.reset(new Subprocess(it->cmd->Start(std::move(options)))); } else { bool is_critical = it->is_critical; + std::string name = it->cmd->GetShortName(); monitored.erase(it); if (running.load() && is_critical) { - LOG(ERROR) << "Stopping all monitored processes due to unexpected " - "exit of critical process"; running.store(false); - break; + const bool is_vmm = + (name.find("crosvm") != std::string::npos || + name.find("qemu") != std::string::npos || + name.find("gem5") != std::string::npos || + name.find("process_restarter") != std::string::npos); + if (is_vmm && WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) { + LOG(INFO) + << "Stopping all monitored processes due to graceful exit " + "of critical process " + << name; + break; + } else { + LOG(ERROR) << "Stopping all monitored processes due to unexpected " + "exit of critical process " + << name; + if (WIFSIGNALED(wstatus)) { + return CF_ERRF("Critical process {} was killed by signal {}", + name, WTERMSIG(wstatus)); + } else if (WEXITSTATUS(wstatus) != 0) { + return CF_ERRF( + "Critical process {} exited with non-zero exit code {}", name, + WEXITSTATUS(wstatus)); + } else { + return CF_ERRF( + "Critical process {} exited unexpectedly with exit code 0", + name); + } + } } } } @@ -153,6 +179,12 @@ Result MonitorLoop(std::atomic_bool& running, Result StopSubprocesses(std::vector& monitored) { VLOG(0) << "Stopping monitored subprocesses"; + for (const auto& it : monitored) { + if (it.proc) { + (void)it.proc->SendSignal(SIGCONT); + (void)it.proc->SendSignalToGroup(SIGCONT); + } + } auto stop = [](const auto& it) { auto stop_result = it.proc->Stop(); if (stop_result == StopperResult::kFailure) { @@ -360,41 +392,26 @@ Result ProcessMonitor::StopMonitoredProcesses() { if (monitor_ == -1) { return {}; } - int wstatus = 0; - pid_t wait_res = waitpid(monitor_, &wstatus, WNOHANG); - if (wait_res == monitor_ || (wait_res == -1 && errno == ECHILD)) { - monitor_ = -1; - parent_channel_.reset(); - return {}; - } - bool send_success = false; - if (parent_channel_.has_value()) { - auto send_result = - SendEmptyRequest(*parent_channel_, ParentToChildMessageType::kStop); - if (!send_result.has_value()) { - VLOG(0) << "SendEmptyRequest failed during StopMonitoredProcesses: " - << send_result.error(); - } else { - send_success = true; - } - } pid_t last_monitor = monitor_; monitor_ = -1; - parent_channel_.reset(); - CF_EXPECT(waitpid(last_monitor, &wstatus, 0) == last_monitor, - "Failed to wait for monitor process"); - if (!send_success) { - // Monitor process was already exiting when stop was requested. - if (WIFSIGNALED(wstatus)) { - LOG(WARNING) << "Monitor process exited due to a signal: " - << WTERMSIG(wstatus); - } else if (WIFEXITED(wstatus) && WEXITSTATUS(wstatus) != 0) { - LOG(WARNING) << "Monitor process exited with code " - << WEXITSTATUS(wstatus); + + int wstatus = 0; + pid_t wait_res = waitpid(last_monitor, &wstatus, WNOHANG); + if (wait_res == 0) { + if (parent_channel_.has_value()) { + auto send_result = + SendEmptyRequest(*parent_channel_, ParentToChildMessageType::kStop); + if (!send_result.has_value()) { + VLOG(0) << "SendEmptyRequest failed during StopMonitoredProcesses: " + << send_result.error(); + } } - return {}; + wait_res = waitpid(last_monitor, &wstatus, 0); } + + parent_channel_.reset(); + CF_EXPECT(wait_res == last_monitor, "Failed to wait for monitor process"); CF_EXPECT(!WIFSIGNALED(wstatus), "Monitor process exited due to a signal"); CF_EXPECT(WIFEXITED(wstatus), "Monitor process exited for unknown reasons"); CF_EXPECT(WEXITSTATUS(wstatus) == 0, @@ -476,16 +493,26 @@ Result ProcessMonitor::MonitorRoutine() { auto parent_comms = std::async(std::launch::async, read_monitor_socket_loop, std::ref(running)); - CF_EXPECT(MonitorLoop(running, properties_mutex_, - properties_.restart_subprocesses_, - properties_.entries_)); + auto monitor_loop_result = + MonitorLoop(running, properties_mutex_, properties_.restart_subprocesses_, + properties_.entries_); running.store(false); if (child_sock_->IsOpen()) { child_sock_->Shutdown(SHUT_RDWR); } - CF_EXPECT(parent_comms.get(), "Should have exited if monitoring stopped"); + auto parent_comms_result = parent_comms.get(); + if (!parent_comms_result.has_value()) { + LOG(WARNING) << "Parent comms thread failed: " + << parent_comms_result.error(); + } - CF_EXPECT(StopSubprocesses(properties_.entries_)); + auto stop_result = StopSubprocesses(properties_.entries_); + if (!stop_result.has_value()) { + LOG(WARNING) << "Failed to stop subprocesses: " << stop_result.error(); + } + CF_EXPECT(std::move(monitor_loop_result)); + CF_EXPECT(std::move(parent_comms_result)); + CF_EXPECT(std::move(stop_result)); VLOG(0) << "Done monitoring subprocesses"; return {}; } From 0e899a25ab39e6d526cbb115c7eb7c1b97a99834 Mon Sep 17 00:00:00 2001 From: My Name Date: Thu, 17 Sep 2026 10:33:59 +0000 Subject: [PATCH 3/5] Improve process monitoring, signal safety, and shutdown status handling --- .../commands/run_cvd/server_loop_impl.cpp | 9 +++++ .../host/commands/run_cvd/server_loop_impl.h | 1 + .../host/libs/process_monitor/BUILD.bazel | 1 + .../libs/process_monitor/process_monitor.cc | 33 ++++++++++++++----- base/cvd/cuttlefish/process/command.cc | 1 + 5 files changed, 36 insertions(+), 9 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp index 028139f7407..42d1d879d10 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp +++ b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp @@ -134,6 +134,7 @@ Result ServerLoopImpl::Run() { return CF_ERR( "process monitor exited unexpectedly: " << stop_result.error()); } + device_status_ = DeviceStatus::kGuestOff; LOG(INFO) << "Process monitor has exited gracefully (guest VM shut down). " "Server loop continuing to listen for status/restart."; @@ -195,6 +196,8 @@ Result ServerLoopImpl::HandleExtended( switch (action_info.extended_action.actions_case()) { case ActionsCase::kSuspend: { VLOG(0) << "Run_cvd received suspend request."; + CF_EXPECT(device_status_.load() != DeviceStatus::kGuestOff, + "Device is powered off, cannot suspend"); if (device_status_.load() == DeviceStatus::kActive) { CF_EXPECT(HandleSuspend(process_monitor)); } @@ -203,6 +206,8 @@ Result ServerLoopImpl::HandleExtended( } case ActionsCase::kResume: { VLOG(0) << "Run_cvd received resume request."; + CF_EXPECT(device_status_.load() != DeviceStatus::kGuestOff, + "Device is powered off, cannot resume"); if (device_status_.load() == DeviceStatus::kSuspended) { CF_EXPECT(HandleResume(process_monitor)); } @@ -219,6 +224,8 @@ Result ServerLoopImpl::HandleExtended( } case ActionsCase::kStartScreenRecording: { VLOG(0) << "Run_cvd received start screen recording request."; + CF_EXPECT(device_status_.load() == DeviceStatus::kActive, + "Device is not active, cannot start screen recording"); CF_EXPECT(HandleStartScreenRecording()); return {}; } @@ -229,6 +236,8 @@ Result ServerLoopImpl::HandleExtended( } case ActionsCase::kScreenshotDisplay: { VLOG(0) << "Run_cvd received screenshot display request."; + CF_EXPECT(device_status_.load() == DeviceStatus::kActive, + "Device is not active, cannot take screenshot"); const auto& request = action_info.extended_action.screenshot_display(); CF_EXPECT(HandleScreenshotDisplay(request)); return {}; diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h index 1ec2ad1d9ca..a3e75df93d5 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h +++ b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h @@ -62,6 +62,7 @@ class ServerLoopImpl : public ServerLoop, kUnknown = 0, kActive = 1, kSuspended = 2, + kGuestOff = 3, }; private: diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/BUILD.bazel b/base/cvd/cuttlefish/host/libs/process_monitor/BUILD.bazel index 4cf55fdc2cc..97fa5209f3f 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/BUILD.bazel +++ b/base/cvd/cuttlefish/host/libs/process_monitor/BUILD.bazel @@ -20,6 +20,7 @@ cf_cc_library( "//cuttlefish/host/libs/config:known_paths", "//cuttlefish/host/libs/feature", "//cuttlefish/posix:strerror", + "//cuttlefish/posix:temp_failure_retry", "//cuttlefish/process:command", "//cuttlefish/process:subprocess", "//cuttlefish/result", diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc index 5770f743fe8..362fd37a998 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc +++ b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc @@ -43,6 +43,8 @@ #include "cuttlefish/host/libs/command_util/util.h" #include "cuttlefish/host/libs/config/known_paths.h" #include "cuttlefish/posix/strerror.h" +#include "cuttlefish/posix/temp_failure_retry.h" +#include "cuttlefish/process/command.h" #include "cuttlefish/process/subprocess.h" #include "cuttlefish/result/result.h" @@ -77,6 +79,22 @@ Result SendEmptyResponse(Channel& channel, uint32_t type) { return {}; } +bool IsVmmCommand(const Command& cmd) { + const std::string name = cmd.GetShortName(); + if (name.find("crosvm") != std::string::npos || + name.find("qemu") != std::string::npos || + name.find("gem5") != std::string::npos) { + return true; + } + if (name.find("process_restarter") != std::string::npos) { + const std::string full_cmd = cmd.ToString(); + return full_cmd.find("crosvm") != std::string::npos || + full_cmd.find("qemu") != std::string::npos || + full_cmd.find("gem5") != std::string::npos; + } + return false; +} + void LogSubprocessExit(const std::string& name, pid_t pid, int wstatus) { LOG(INFO) << "Detected unexpected exit of monitored subprocess " << name; if (WIFEXITED(wstatus)) { @@ -139,14 +157,10 @@ Result MonitorLoop(std::atomic_bool& running, } else { bool is_critical = it->is_critical; std::string name = it->cmd->GetShortName(); + const bool is_vmm = IsVmmCommand(*it->cmd); monitored.erase(it); if (running.load() && is_critical) { running.store(false); - const bool is_vmm = - (name.find("crosvm") != std::string::npos || - name.find("qemu") != std::string::npos || - name.find("gem5") != std::string::npos || - name.find("process_restarter") != std::string::npos); if (is_vmm && WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) { LOG(INFO) << "Stopping all monitored processes due to graceful exit " @@ -181,11 +195,13 @@ Result StopSubprocesses(std::vector& monitored) { VLOG(0) << "Stopping monitored subprocesses"; for (const auto& it : monitored) { if (it.proc) { - (void)it.proc->SendSignal(SIGCONT); (void)it.proc->SendSignalToGroup(SIGCONT); } } auto stop = [](const auto& it) { + if (!it.proc) { + return true; + } auto stop_result = it.proc->Stop(); if (stop_result == StopperResult::kFailure) { LOG(WARNING) << "Error in stopping \"" << it.cmd->GetShortName() << "\""; @@ -397,7 +413,7 @@ Result ProcessMonitor::StopMonitoredProcesses() { monitor_ = -1; int wstatus = 0; - pid_t wait_res = waitpid(last_monitor, &wstatus, WNOHANG); + pid_t wait_res = TEMP_FAILURE_RETRY(waitpid(last_monitor, &wstatus, WNOHANG)); if (wait_res == 0) { if (parent_channel_.has_value()) { auto send_result = @@ -407,7 +423,7 @@ Result ProcessMonitor::StopMonitoredProcesses() { << send_result.error(); } } - wait_res = waitpid(last_monitor, &wstatus, 0); + wait_res = TEMP_FAILURE_RETRY(waitpid(last_monitor, &wstatus, 0)); } parent_channel_.reset(); @@ -511,7 +527,6 @@ Result ProcessMonitor::MonitorRoutine() { LOG(WARNING) << "Failed to stop subprocesses: " << stop_result.error(); } CF_EXPECT(std::move(monitor_loop_result)); - CF_EXPECT(std::move(parent_comms_result)); CF_EXPECT(std::move(stop_result)); VLOG(0) << "Done monitoring subprocesses"; return {}; diff --git a/base/cvd/cuttlefish/process/command.cc b/base/cvd/cuttlefish/process/command.cc index bdbe625d3a8..9a3ff26d903 100644 --- a/base/cvd/cuttlefish/process/command.cc +++ b/base/cvd/cuttlefish/process/command.cc @@ -283,6 +283,7 @@ Subprocess Command::Start(SubprocessOptions options) const { prctl(PR_SET_PDEATHSIG, SIGHUP); // Die when parent dies } #endif + signal(SIGPIPE, SIG_DFL); do_redirects(redirects_); From 597ece45db084e9cdee5d539d7d5f16bb7ea7ace Mon Sep 17 00:00:00 2001 From: My Name Date: Thu, 17 Sep 2026 11:17:53 +0000 Subject: [PATCH 4/5] Exclude auxiliary VMs from shutdown handling and guard screen recording stop --- .../host/commands/run_cvd/server_loop_impl.cpp | 10 +++++++++- .../host/libs/process_monitor/process_monitor.cc | 7 ++++++- base/cvd/cuttlefish/process/command.cc | 1 - 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp index 42d1d879d10..c5cb11bf4ee 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp +++ b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp @@ -231,6 +231,8 @@ Result ServerLoopImpl::HandleExtended( } case ActionsCase::kStopScreenRecording: { VLOG(0) << "Run_cvd received stop screen recording request."; + CF_EXPECT(device_status_.load() == DeviceStatus::kActive, + "Device is not active, cannot stop screen recording"); CF_EXPECT(HandleStopScreenRecording()); return {}; } @@ -282,7 +284,13 @@ void ServerLoopImpl::HandleActionWithNoData(const LauncherAction action, break; } case LauncherAction::kStatus: { - // TODO(schuffelen): Return more information on a side channel + // TODO(schuffelen): Return more information on a side channel. + // Note: When device_status_ == DeviceStatus::kGuestOff, returning + // kSuccess keeps the socket responsive so that `cvd restart` can power + // the VM back on. Because `cvd status` hardcodes "Running" upon + // receiving LauncherResponse::kSuccess, reporting a distinct + // "Powered Off" status requires extending the + // LauncherAction/LauncherResponse protocol. auto response = LauncherResponse::kSuccess; // TODO(schuffelen): Handle unused result (void)client->Write(&response, sizeof(response)); diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc index 362fd37a998..e7ec4076893 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc +++ b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc @@ -81,13 +81,18 @@ Result SendEmptyResponse(Channel& channel, uint32_t type) { bool IsVmmCommand(const Command& cmd) { const std::string name = cmd.GetShortName(); + const std::string full_cmd = cmd.ToString(); + // Auxiliary VMs like OpenWRT should not be treated as the main guest VMM. + if (full_cmd.find("openwrt") != std::string::npos || + full_cmd.find("crosvm_openwrt") != std::string::npos) { + return false; + } if (name.find("crosvm") != std::string::npos || name.find("qemu") != std::string::npos || name.find("gem5") != std::string::npos) { return true; } if (name.find("process_restarter") != std::string::npos) { - const std::string full_cmd = cmd.ToString(); return full_cmd.find("crosvm") != std::string::npos || full_cmd.find("qemu") != std::string::npos || full_cmd.find("gem5") != std::string::npos; diff --git a/base/cvd/cuttlefish/process/command.cc b/base/cvd/cuttlefish/process/command.cc index 9a3ff26d903..bdbe625d3a8 100644 --- a/base/cvd/cuttlefish/process/command.cc +++ b/base/cvd/cuttlefish/process/command.cc @@ -283,7 +283,6 @@ Subprocess Command::Start(SubprocessOptions options) const { prctl(PR_SET_PDEATHSIG, SIGHUP); // Die when parent dies } #endif - signal(SIGPIPE, SIG_DFL); do_redirects(redirects_); From 94c2103eee8e98032eeb1512a4bba6bec2e17d7b Mon Sep 17 00:00:00 2001 From: Aleksandr Shirvinskii Date: Fri, 18 Sep 2026 07:06:19 +0000 Subject: [PATCH 5/5] Categorize monitored process types and improve device status comparisons --- .../commands/run_cvd/server_loop_impl.cpp | 50 ++++++++++++------- .../host/commands/run_cvd/server_loop_impl.h | 11 ++++ .../host/libs/feature/command_source.h | 34 ++++++++++++- .../libs/process_monitor/process_monitor.cc | 29 ++--------- .../libs/process_monitor/process_monitor.h | 11 +++- .../host/libs/vm_manager/crosvm_manager.cpp | 5 +- .../host/libs/vm_manager/gem5_manager.cpp | 2 +- .../host/libs/vm_manager/qemu_manager.cpp | 2 +- 8 files changed, 96 insertions(+), 48 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp index c5cb11bf4ee..6cb0bc16552 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp +++ b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp @@ -24,7 +24,9 @@ #include #include #include +#include #include +#include #include #include @@ -58,6 +60,24 @@ namespace cuttlefish { namespace run_cvd_impl { +std::string_view format_as(ServerLoopImpl::DeviceStatus status) { + switch (status) { + case ServerLoopImpl::DeviceStatus::kUnknown: + return "Unknown"; + case ServerLoopImpl::DeviceStatus::kActive: + return "Active"; + case ServerLoopImpl::DeviceStatus::kSuspended: + return "Suspended"; + case ServerLoopImpl::DeviceStatus::kGuestOff: + return "GuestOff"; + } +} + +std::ostream& operator<<(std::ostream& out, + ServerLoopImpl::DeviceStatus status) { + return out << format_as(status); +} + bool ServerLoopImpl::CreateQcowOverlay(const std::string& crosvm_path, const std::string& backing_file, const std::string& output_overlay_path) { @@ -129,11 +149,7 @@ Result ServerLoopImpl::Run() { if (process_monitor_active && read_set.IsSet(process_monitor.status())) { process_monitor_active = false; - auto stop_result = process_monitor.StopMonitoredProcesses(); - if (!stop_result.has_value()) { - return CF_ERR( - "process monitor exited unexpectedly: " << stop_result.error()); - } + CF_EXPECT(process_monitor.StopMonitoredProcesses()); device_status_ = DeviceStatus::kGuestOff; LOG(INFO) << "Process monitor has exited gracefully (guest VM shut down). " @@ -196,8 +212,8 @@ Result ServerLoopImpl::HandleExtended( switch (action_info.extended_action.actions_case()) { case ActionsCase::kSuspend: { VLOG(0) << "Run_cvd received suspend request."; - CF_EXPECT(device_status_.load() != DeviceStatus::kGuestOff, - "Device is powered off, cannot suspend"); + CF_EXPECT_NE(device_status_.load(), DeviceStatus::kGuestOff, + "Device is powered off, cannot suspend"); if (device_status_.load() == DeviceStatus::kActive) { CF_EXPECT(HandleSuspend(process_monitor)); } @@ -206,8 +222,8 @@ Result ServerLoopImpl::HandleExtended( } case ActionsCase::kResume: { VLOG(0) << "Run_cvd received resume request."; - CF_EXPECT(device_status_.load() != DeviceStatus::kGuestOff, - "Device is powered off, cannot resume"); + CF_EXPECT_NE(device_status_.load(), DeviceStatus::kGuestOff, + "Device is powered off, cannot resume"); if (device_status_.load() == DeviceStatus::kSuspended) { CF_EXPECT(HandleResume(process_monitor)); } @@ -216,30 +232,30 @@ Result ServerLoopImpl::HandleExtended( } case ActionsCase::kSnapshotTake: { VLOG(0) << "Run_cvd received snapshot request."; - CF_EXPECT(device_status_.load() == DeviceStatus::kSuspended, - "The device is not suspended, and snapshot cannot be taken"); + CF_EXPECT_EQ(device_status_.load(), DeviceStatus::kSuspended, + "The device is not suspended, and snapshot cannot be taken"); CF_EXPECT( HandleSnapshotTake(action_info.extended_action.snapshot_take())); return {}; } case ActionsCase::kStartScreenRecording: { VLOG(0) << "Run_cvd received start screen recording request."; - CF_EXPECT(device_status_.load() == DeviceStatus::kActive, - "Device is not active, cannot start screen recording"); + CF_EXPECT_EQ(device_status_.load(), DeviceStatus::kActive, + "Device is not active, cannot start screen recording"); CF_EXPECT(HandleStartScreenRecording()); return {}; } case ActionsCase::kStopScreenRecording: { VLOG(0) << "Run_cvd received stop screen recording request."; - CF_EXPECT(device_status_.load() == DeviceStatus::kActive, - "Device is not active, cannot stop screen recording"); + CF_EXPECT_EQ(device_status_.load(), DeviceStatus::kActive, + "Device is not active, cannot stop screen recording"); CF_EXPECT(HandleStopScreenRecording()); return {}; } case ActionsCase::kScreenshotDisplay: { VLOG(0) << "Run_cvd received screenshot display request."; - CF_EXPECT(device_status_.load() == DeviceStatus::kActive, - "Device is not active, cannot take screenshot"); + CF_EXPECT_EQ(device_status_.load(), DeviceStatus::kActive, + "Device is not active, cannot take screenshot"); const auto& request = action_info.extended_action.screenshot_display(); CF_EXPECT(HandleScreenshotDisplay(request)); return {}; diff --git a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h index a3e75df93d5..a042b6ba61d 100644 --- a/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h +++ b/base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.h @@ -17,7 +17,9 @@ #pragma once #include +#include #include +#include #include #include #include @@ -115,5 +117,14 @@ class ServerLoopImpl : public ServerLoop, std::atomic device_status_; }; +std::string_view format_as(ServerLoopImpl::DeviceStatus status); +std::ostream& operator<<(std::ostream& out, + ServerLoopImpl::DeviceStatus status); + +template +void AbslStringify(Sink& sink, ServerLoopImpl::DeviceStatus status) { + sink.Append(format_as(status)); +} + } // namespace run_cvd_impl } // namespace cuttlefish diff --git a/base/cvd/cuttlefish/host/libs/feature/command_source.h b/base/cvd/cuttlefish/host/libs/feature/command_source.h index f47b496782b..d4c5fcf2406 100644 --- a/base/cvd/cuttlefish/host/libs/feature/command_source.h +++ b/base/cvd/cuttlefish/host/libs/feature/command_source.h @@ -15,6 +15,8 @@ #pragma once +#include +#include #include #include @@ -26,12 +28,42 @@ namespace cuttlefish { +enum class ProcessCategory { + kNonCriticalSupport, + kCriticalSupport, + kVmm, +}; + +inline constexpr std::string_view format_as(ProcessCategory category) { + switch (category) { + case ProcessCategory::kVmm: + return "vmm"; + case ProcessCategory::kCriticalSupport: + return "critical support"; + case ProcessCategory::kNonCriticalSupport: + return "non-critical support"; + } +} + +inline std::ostream& operator<<(std::ostream& out, ProcessCategory category) { + return out << format_as(category); +} + struct MonitorCommand { Command command; bool is_critical; + ProcessCategory category; MonitorCommand(Command command, bool is_critical = true) - : command(std::move(command)), is_critical(is_critical) {} + : command(std::move(command)), + is_critical(is_critical), + category(is_critical ? ProcessCategory::kCriticalSupport + : ProcessCategory::kNonCriticalSupport) {} + + MonitorCommand(Command command, ProcessCategory category) + : command(std::move(command)), + is_critical(category != ProcessCategory::kNonCriticalSupport), + category(category) {} }; class CommandSource : public virtual SetupFeature { diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc index e7ec4076893..66a4bcde4f0 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc +++ b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.cc @@ -79,27 +79,6 @@ Result SendEmptyResponse(Channel& channel, uint32_t type) { return {}; } -bool IsVmmCommand(const Command& cmd) { - const std::string name = cmd.GetShortName(); - const std::string full_cmd = cmd.ToString(); - // Auxiliary VMs like OpenWRT should not be treated as the main guest VMM. - if (full_cmd.find("openwrt") != std::string::npos || - full_cmd.find("crosvm_openwrt") != std::string::npos) { - return false; - } - if (name.find("crosvm") != std::string::npos || - name.find("qemu") != std::string::npos || - name.find("gem5") != std::string::npos) { - return true; - } - if (name.find("process_restarter") != std::string::npos) { - return full_cmd.find("crosvm") != std::string::npos || - full_cmd.find("qemu") != std::string::npos || - full_cmd.find("gem5") != std::string::npos; - } - return false; -} - void LogSubprocessExit(const std::string& name, pid_t pid, int wstatus) { LOG(INFO) << "Detected unexpected exit of monitored subprocess " << name; if (WIFEXITED(wstatus)) { @@ -160,16 +139,16 @@ Result MonitorLoop(std::atomic_bool& running, // in the future, cmd->Start might not run exec() it->proc.reset(new Subprocess(it->cmd->Start(std::move(options)))); } else { - bool is_critical = it->is_critical; + const bool is_critical = it->is_critical; + const bool is_vmm = it->category == ProcessCategory::kVmm; std::string name = it->cmd->GetShortName(); - const bool is_vmm = IsVmmCommand(*it->cmd); monitored.erase(it); if (running.load() && is_critical) { running.store(false); if (is_vmm && WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) { LOG(INFO) << "Stopping all monitored processes due to graceful exit " - "of critical process " + "of VMM process " << name; break; } else { @@ -387,7 +366,7 @@ ProcessMonitor::Properties& ProcessMonitor::Properties::RestartSubprocesses( ProcessMonitor::Properties& ProcessMonitor::Properties::AddCommand( MonitorCommand cmd) & { - entries_.emplace_back(std::move(cmd.command), cmd.is_critical); + entries_.emplace_back(std::move(cmd.command), cmd.category); return *this; } diff --git a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.h b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.h index 2a6a889b9f9..da43d0ddb28 100644 --- a/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.h +++ b/base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.h @@ -34,9 +34,18 @@ struct MonitorEntry { std::unique_ptr cmd; std::unique_ptr proc; bool is_critical; + ProcessCategory category; + + MonitorEntry(Command command, ProcessCategory category) + : cmd(new Command(std::move(command))), + is_critical(category != ProcessCategory::kNonCriticalSupport), + category(category) {} MonitorEntry(Command command, bool is_critical) - : cmd(new Command(std::move(command))), is_critical(is_critical) {} + : cmd(new Command(std::move(command))), + is_critical(is_critical), + category(is_critical ? ProcessCategory::kCriticalSupport + : ProcessCategory::kNonCriticalSupport) {} }; // Launches and keeps track of subprocesses, decides response if they diff --git a/base/cvd/cuttlefish/host/libs/vm_manager/crosvm_manager.cpp b/base/cvd/cuttlefish/host/libs/vm_manager/crosvm_manager.cpp index 05b04198bc0..c659da8b3b3 100644 --- a/base/cvd/cuttlefish/host/libs/vm_manager/crosvm_manager.cpp +++ b/base/cvd/cuttlefish/host/libs/vm_manager/crosvm_manager.cpp @@ -1102,11 +1102,12 @@ Result> CrosvmManager::StartCommands( gpu_capture_logs); commands.emplace_back(std::move(gpu_capture_log_tee_cmd)); - commands.emplace_back(std::move(gpu_capture_command)); + commands.emplace_back(std::move(gpu_capture_command), + ProcessCategory::kVmm); } else { crosvm_cmd.Cmd().RedirectStdIO(Command::StdIoChannel::kStdOut, crosvm_logs); crosvm_cmd.Cmd().RedirectStdIO(Command::StdIoChannel::kStdErr, crosvm_logs); - commands.emplace_back(std::move(crosvm_cmd.Cmd()), true); + commands.emplace_back(std::move(crosvm_cmd.Cmd()), ProcessCategory::kVmm); } return commands; diff --git a/base/cvd/cuttlefish/host/libs/vm_manager/gem5_manager.cpp b/base/cvd/cuttlefish/host/libs/vm_manager/gem5_manager.cpp index f363c4a420c..cc90016bd65 100644 --- a/base/cvd/cuttlefish/host/libs/vm_manager/gem5_manager.cpp +++ b/base/cvd/cuttlefish/host/libs/vm_manager/gem5_manager.cpp @@ -368,7 +368,7 @@ Result> Gem5Manager::StartCommands( gem5_cmd.AddEnvironmentVariable("M5_PATH", config.assembly_dir()); std::vector commands; - commands.emplace_back(std::move(gem5_cmd), true); + commands.emplace_back(std::move(gem5_cmd), ProcessCategory::kVmm); return commands; } diff --git a/base/cvd/cuttlefish/host/libs/vm_manager/qemu_manager.cpp b/base/cvd/cuttlefish/host/libs/vm_manager/qemu_manager.cpp index 869556097f4..89703d07af0 100644 --- a/base/cvd/cuttlefish/host/libs/vm_manager/qemu_manager.cpp +++ b/base/cvd/cuttlefish/host/libs/vm_manager/qemu_manager.cpp @@ -917,7 +917,7 @@ Result> QemuManager::StartCommands( add_hvc_sink(); } - commands.emplace_back(std::move(qemu_cmd), true); + commands.emplace_back(std::move(qemu_cmd), ProcessCategory::kVmm); return commands; }