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

@@ -24,7 +24,9 @@
 #include <algorithm>
 #include <cstdlib>
 #include <memory>
+#include <ostream>
 #include <string>
+#include <string_view>
 #include <utility>
 #include <vector>
 
@@ -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) {
@@ -116,16 +136,25 @@ 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;
+      CF_EXPECT(process_monitor.StopMonitoredProcesses());
+      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 +212,8 @@ Result<void> ServerLoopImpl::HandleExtended(
   switch (action_info.extended_action.actions_case()) {
     case ActionsCase::kSuspend: {
       VLOG(0) << "Run_cvd received suspend request.";
+      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));
       }
@@ -191,6 +222,8 @@ Result<void> ServerLoopImpl::HandleExtended(
     }
     case ActionsCase::kResume: {
       VLOG(0) << "Run_cvd received resume request.";
+      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));
       }
@@ -199,24 +232,30 @@ Result<void> 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_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_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_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 {};
@@ -261,7 +300,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

@@ -17,7 +17,9 @@
 #pragma once
 
 #include <atomic>
+#include <ostream>
 #include <string>
+#include <string_view>
 #include <unordered_map>
 #include <unordered_set>
 #include <vector>
@@ -62,6 +64,7 @@ class ServerLoopImpl : public ServerLoop,
     kUnknown = 0,
     kActive = 1,
     kSuspended = 2,
+    kGuestOff = 3,
   };
 
  private:
@@ -114,5 +117,14 @@ class ServerLoopImpl : public ServerLoop,
   std::atomic<DeviceStatus> device_status_;
 };
 
+std::string_view format_as(ServerLoopImpl::DeviceStatus status);
+std::ostream& operator<<(std::ostream& out,
+                         ServerLoopImpl::DeviceStatus status);
+
+template <typename Sink>
+void AbslStringify(Sink& sink, ServerLoopImpl::DeviceStatus status) {
+  sink.Append(format_as(status));
+}
+
 }  // namespace run_cvd_impl
 }  // namespace cuttlefish

base/cvd/cuttlefish/host/libs/feature/command_source.h

@@ -15,6 +15,8 @@
 
 #pragma once
 
+#include <ostream>
+#include <string_view>
 #include <utility>
 #include <vector>
 
@@ -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 {

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"
 
@@ -137,13 +139,35 @@ Result<void> 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();
         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 VMM 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);
+            }
+          }
         }
       }
     }
@@ -154,6 +178,9 @@ Result<void> MonitorLoop(std::atomic_bool& running,
 Result<void> StopSubprocesses(std::vector<MonitorEntry>& monitored) {
   VLOG(0) << "Stopping monitored subprocesses";
   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() << "\"";
@@ -334,7 +361,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;
 }
 
@@ -357,18 +384,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 +488,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 {};
 }

base/cvd/cuttlefish/host/libs/process_monitor/process_monitor.h

@@ -34,9 +34,18 @@ struct MonitorEntry {
   std::unique_ptr<Command> cmd;
   std::unique_ptr<Subprocess> 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