Prevent run_cvd abort and keep monitor socket alive on graceful VM sh…

base/cvd/cuttlefish/host/commands/run_cvd/main.cc

@@ -15,6 +15,7 @@
  */
 
 #include <errno.h>
+#include <signal.h>
 #include <stdlib.h>
 #include <unistd.h>
 
@@ -245,6 +246,7 @@ void ConfigureLogs(const CuttlefishConfig& config,
 }  // namespace
 
 Result<void> RunCvdMain(int argc, char** argv) {
+  signal(SIGPIPE, SIG_IGN);
   google::ParseCommandLineFlags(&argc, &argv, false);
 
   CF_EXPECT(StdinValid(), "Invalid stdin");

base/cvd/cuttlefish/host/commands/run_cvd/server_loop_impl.cpp

@@ -116,16 +116,29 @@ Result<void> 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())) {
+      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());
+      }
+      device_status_ = DeviceStatus::kGuestOff;
+      LOG(INFO)
+          << "Process monitor has exited gracefully (guest VM shut down). "
+             "Server loop continuing to listen for status/restart.";
+      continue;
     }
 
     CF_EXPECT(read_set.IsSet(server_));
@@ -183,6 +196,8 @@ Result<void> 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));
       }
@@ -191,6 +206,8 @@ Result<void> 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));
       }
@@ -207,16 +224,22 @@ Result<void> 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 {};
     }
     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 {};
     }
     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 {};
@@ -261,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));

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:

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",

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,27 @@ Result<void> 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)) {
@@ -138,12 +161,34 @@ Result<void> 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();
+        const bool is_vmm = IsVmmCommand(*it->cmd);
         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;
+          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,7 +198,15 @@ Result<void> MonitorLoop(std::atomic_bool& running,
 
 Result<void> StopSubprocesses(std::vector<MonitorEntry>& monitored) {
   VLOG(0) << "Stopping monitored subprocesses";
+  for (const auto& it : monitored) {
+    if (it.proc) {
+      (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() << "\"";
@@ -357,18 +410,29 @@ ProcessMonitor::ProcessMonitor(ProcessMonitor::Properties&& properties,
       monitor_(-1) {}
 
 Result<void> 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 {};
+  }
 
   pid_t last_monitor = monitor_;
   monitor_ = -1;
+
+  int wstatus = 0;
+  pid_t wait_res = TEMP_FAILURE_RETRY(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();
+      }
+    }
+    wait_res = TEMP_FAILURE_RETRY(waitpid(last_monitor, &wstatus, 0));
+  }
+
   parent_channel_.reset();
-  int wstatus;
-  CF_EXPECT(waitpid(last_monitor, &wstatus, 0) == last_monitor,
-            "Failed to wait for monitor process");
+  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,
@@ -450,16 +514,25 @@ Result<void> 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(stop_result));
   VLOG(0) << "Done monitoring subprocesses";
   return {};
 }