Fix single-instance start after stop in multi-instance groups

base/cvd/cuttlefish/host/commands/cvd/cli/commands/start.cpp

@@ -24,6 +24,7 @@
 #include <unistd.h>
 
 #include <algorithm>
+#include <chrono>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
@@ -68,6 +69,7 @@
 #include "cuttlefish/host/commands/cvd/instances/cvd_persistent_data.pb.h"
 #include "cuttlefish/host/commands/cvd/instances/instance_database_types.h"
 #include "cuttlefish/host/commands/cvd/instances/instance_manager.h"
+#include "cuttlefish/host/commands/cvd/instances/local_instance.h"
 #include "cuttlefish/host/commands/cvd/instances/local_instance_group.h"
 #include "cuttlefish/host/commands/cvd/instances/operator_client.h"
 #include "cuttlefish/host/commands/cvd/instances/stop.h"
@@ -248,11 +250,12 @@ Result<void> SymlinkPreviousConfig(const std::string& group_home_dir) {
   return {};
 }
 
-Result<std::unique_ptr<OperatorControlConn>> PreregisterGroup(
-    const LocalInstanceGroup& group) {
+Result<std::unique_ptr<OperatorControlConn>> PreregisterInstances(
+    const LocalInstanceGroup& group,
+    const std::vector<LocalInstance>& instances) {
   std::unique_ptr<OperatorControlConn> operator_conn =
       CF_EXPECT(OperatorControlConn::Create());
-  CF_EXPECT(operator_conn->Preregister(group));
+  CF_EXPECT(operator_conn->Preregister(group, instances));
   return operator_conn;
 }
 
@@ -334,7 +337,8 @@ Result<std::vector<Flag>> GetCvdInternalStartFlags(
 bool CanBypassToSingleInstance(const LocalInstance& instance,
                                const LocalInstanceGroup& group,
                                const std::vector<std::string>& subcmd_args) {
-  if (instance.State() != cvd::INSTANCE_STATE_STOPPED) {
+  if (instance.State() != cvd::INSTANCE_STATE_STOPPED &&
+      instance.State() != cvd::INSTANCE_STATE_BOOT_FAILED) {
     return false;
   }
   if (group.StartTime() == TimeStamp{}) {
@@ -343,8 +347,11 @@ bool CanBypassToSingleInstance(const LocalInstance& instance,
 
   std::vector<std::string> args_copy = subcmd_args;
   bool daemon = true;
+  std::string report_anonymous_usage_stats;
   std::vector<Flag> safe_flags = {
       GflagsCompatFlag("daemon", daemon),
+      GflagsCompatFlag("report_anonymous_usage_stats",
+                       report_anonymous_usage_stats),
   };
   const Result<void> res = ConsumeFlags(safe_flags, args_copy);
   if (!res.has_value() || !daemon || !args_copy.empty()) {
@@ -552,14 +559,14 @@ std::vector<std::string> CvdStartCommandHandler::CmdList() const {
 
 Result<void> CvdStartCommandHandler::LaunchDevice(
     Command launch_command, LocalInstanceGroup& group,
-    const std::unordered_map<std::string, std::string>& envs,
-    const CommandRequest& request) {
+    const std::vector<LocalInstance>& instances) {
   // Don't destroy the returned object until after the devices have started, it
   // holds a connection to the orchestrator that ensures the devices remain
   // pre-registered there. If the connection is lost before the devices register
   // themselves the pre-registration is lost and group information won't be
   // shown in the UI.
-  auto conn_res = PreregisterGroup(group);
+  const Result<std::unique_ptr<OperatorControlConn>> conn_res =
+      PreregisterInstances(group, instances);
   if (!conn_res.has_value()) {
     LOG(ERROR) << "Failed to pre-register devices with operator, group "
                   "information won't show in the UI: "
@@ -572,14 +579,27 @@ Result<void> CvdStartCommandHandler::LaunchDevice(
   GatherVmStartMetrics(group);
 
   // NOLINTNEXTLINE(misc-include-cleaner)
-  siginfo_t infop = CF_EXPECT(subprocess_waiter_.Wait());
+  const siginfo_t infop = CF_EXPECT(subprocess_waiter_.Wait());
   // NOLINTNEXTLINE(misc-include-cleaner)
   if (infop.si_code != CLD_EXITED || infop.si_status != EXIT_SUCCESS) {
     LOG(INFO) << "Device launch failed, cleaning up";
     GatherVmBootFailedMetrics(group);
-    // run_cvd processes may be still running in background
-    // the order of the following operations should be kept
-    CF_EXPECT(CvdResetGroup(group));
+    if (instances.size() < group.Instances().size()) {
+      std::vector<unsigned> ids;
+      ids.reserve(instances.size());
+      for (const LocalInstance& instance : instances) {
+        ids.push_back(instance.Id());
+      }
+      const Result<void> stop_res = instance_manager_.StopInstanceGroup(
+          group, InstanceDirActionOnStop::Keep, std::chrono::seconds(5), ids);
+      if (!stop_res.has_value()) {
+        LOG(ERROR) << "Failed to stop failed instance(s): " << stop_res.error();
+      }
+    } else {
+      // run_cvd processes may be still running in background
+      // the order of the following operations should be kept
+      CF_EXPECT(CvdResetGroup(group));
+    }
   }
   CF_EXPECT(CheckProcessExitedNormally(infop));
   GatherVmBootCompleteMetrics(group);
@@ -601,8 +621,8 @@ Result<void> CvdStartCommandHandler::LaunchDeviceInterruptible(
     LOG(ERROR) << "Failed to symlink the config file at system wide home: "
                << symlink_config_res.error();
   }
-  Result<void> start_res =
-      LaunchDevice(std::move(command), group, envs, request);
+  const Result<void> start_res =
+      LaunchDevice(std::move(command), group, group.Instances());
   if (!start_res.has_value()) {
     group.SetAllStates(cvd::INSTANCE_STATE_BOOT_FAILED);
     CF_EXPECT(instance_manager_.UpdateInstanceGroup(group));
@@ -624,12 +644,13 @@ Result<void> CvdStartCommandHandler::LaunchSingleInstance(
   run_cvd_envs[kAndroidSoongHostOut] = group.HostArtifactsPath();
   run_cvd_envs[kCvdMarkEnv] = "true";
 
-  ConstructCommandParam construct_cmd_param{.bin_path = bin_path,
-                                            .home = group.HomeDir(),
-                                            .args = std::vector<std::string>{},
-                                            .envs = run_cvd_envs,
-                                            .working_dir = CurrentDirectory(),
-                                            .command_name = "run_cvd"};
+  const ConstructCommandParam construct_cmd_param{
+      .bin_path = bin_path,
+      .home = group.HomeDir(),
+      .args = std::vector<std::string>{},
+      .envs = run_cvd_envs,
+      .working_dir = instance.InstanceDirectory(),
+      .command_name = "run_cvd"};
 
   Command command = CF_EXPECT(ConstructCommand(construct_cmd_param));
   command.RedirectStdIO(Command::StdIoChannel::kStdOut,
@@ -649,7 +670,7 @@ Result<void> CvdStartCommandHandler::LaunchSingleInstance(
   }
 
   auto set_instance_state = [&group, &instance](cvd::InstanceState state) {
-    for (auto& inst : group.Instances()) {
+    for (LocalInstance& inst : group.Instances()) {
       if (inst.Id() == instance.Id()) {
         inst.SetState(state);
         break;
@@ -661,9 +682,8 @@ Result<void> CvdStartCommandHandler::LaunchSingleInstance(
   group.SetStartTime(CvdServerClock::now());
   CF_EXPECT(instance_manager_.UpdateInstanceGroup(group));
 
-  Result<void> start_res =
-      LaunchDevice(std::move(command), group, run_cvd_envs, request);
-
+  const Result<void> start_res =
+      LaunchDevice(std::move(command), group, {instance});
   if (!start_res.has_value()) {
     set_instance_state(cvd::INSTANCE_STATE_BOOT_FAILED);
     CF_EXPECT(instance_manager_.UpdateInstanceGroup(group));

base/cvd/cuttlefish/host/commands/cvd/cli/commands/start.h

@@ -18,10 +18,12 @@
 
 #include <optional>
 #include <string>
+#include <vector>
 
 #include "cuttlefish/flag_parser/flag.h"
 #include "cuttlefish/host/commands/cvd/cli/commands/command_handler.h"
 #include "cuttlefish/host/commands/cvd/instances/instance_manager.h"
+#include "cuttlefish/host/commands/cvd/instances/local_instance.h"
 #include "cuttlefish/host/commands/cvd/instances/local_instance_group.h"
 #include "cuttlefish/host/commands/cvd/utils/subprocess_waiter.h"
 #include "cuttlefish/process/command.h"
@@ -48,10 +50,8 @@ class CvdStartCommandHandler : public CvdCommandHandler {
                                     LocalInstanceGroup& group,
                                     const CommandRequest& request);
 
-  Result<void> LaunchDevice(
-      Command command, LocalInstanceGroup& group,
-      const std::unordered_map<std::string, std::string>& envs,
-      const CommandRequest& request);
+  Result<void> LaunchDevice(Command command, LocalInstanceGroup& group,
+                            const std::vector<LocalInstance>& instances);
 
   Result<void> LaunchDeviceInterruptible(
       Command command, LocalInstanceGroup& group,

base/cvd/cuttlefish/host/commands/cvd/instances/operator_client.cpp

@@ -32,20 +32,23 @@
 #include "cuttlefish/common/libs/fs/shared_fd.h"
 #include "cuttlefish/common/libs/utils/json.h"
 #include "cuttlefish/common/libs/utils/users.h"
+#include "cuttlefish/host/commands/cvd/instances/local_instance.h"
 #include "cuttlefish/host/commands/cvd/instances/local_instance_group.h"
 #include "cuttlefish/io/write_exact.h"
 #include "cuttlefish/result/result.h"
 
 namespace cuttlefish {
 
 namespace {
-Result<Json::Value> BuildPreRegistrationMsg(const LocalInstanceGroup& group) {
+Result<Json::Value> BuildPreRegistrationMsg(
+    const LocalInstanceGroup& group,
+    const std::vector<LocalInstance>& instances) {
   Json::Value msg;
   msg["message_type"] = "pre-register";
   msg["group_name"] = group.GroupName();
   msg["owner"] = CF_EXPECT(CurrentUserName());
   Json::Value devices(Json::arrayValue);
-  for (const auto& instance : group.Instances()) {
+  for (const LocalInstance& instance : instances) {
     Json::Value dev;
     dev["id"] = instance.WebRtcDeviceId();
     dev["name"] = instance.Name();
@@ -78,18 +81,28 @@ Result<std::unique_ptr<OperatorControlConn>> OperatorControlConn::Create(
  * Pre-registers an instance group with the operator
  */
 Result<void> OperatorControlConn::Preregister(const LocalInstanceGroup& group) {
-  Json::Value pre_registration_msg = CF_EXPECT(BuildPreRegistrationMsg(group));
+  return Preregister(group, group.Instances());
+}
+
+Result<void> OperatorControlConn::Preregister(
+    const LocalInstanceGroup& group,
+    const std::vector<LocalInstance>& instances) {
+  const Json::Value pre_registration_msg =
+      CF_EXPECT(BuildPreRegistrationMsg(group, instances));
   CF_EXPECT(WriteExact(*conn_, pre_registration_msg.toStyledString()),
             "Failed to send pre-registration message to operator");
-  Json::Value response =
+  const Json::Value response =
       CF_EXPECT(RecvMsg(conn_), "Error receiving pre-registration response");
 
   std::stringstream error_messages;
   bool errors_found = false;
-  for (const auto& res : CF_EXPECT(GetArrayValues<Json::Value>(response, {}))) {
-    auto id = CF_EXPECT(GetValue<std::string>(res, {"id"}));
-    auto status = CF_EXPECT(GetValue<std::string>(res, {"status"}));
-    auto message = CF_EXPECT(GetValue<std::string>(res, {"message"}));
+  for (const Json::Value& res :
+       CF_EXPECT(GetArrayValues<Json::Value>(response, {}))) {
+    const std::string id = CF_EXPECT(GetValue<std::string>(res, {"id"}));
+    const std::string status =
+        CF_EXPECT(GetValue<std::string>(res, {"status"}));
+    const std::string message =
+        CF_EXPECT(GetValue<std::string>(res, {"message"}));
     if (status != "accepted") {
       fmt::print(error_messages, "\nid: {}, status: {}, message: {}", id,
                  status, message);

base/cvd/cuttlefish/host/commands/cvd/instances/operator_client.h

@@ -18,8 +18,10 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "cuttlefish/common/libs/fs/shared_fd.h"
+#include "cuttlefish/host/commands/cvd/instances/local_instance.h"
 #include "cuttlefish/host/commands/cvd/instances/local_instance_group.h"
 #include "cuttlefish/result/result.h"
 
@@ -41,6 +43,12 @@ class OperatorControlConn {
    */
   Result<void> Preregister(const LocalInstanceGroup& group);
 
+  /**
+   * Pre-registers specific instances from a group with the operator
+   */
+  Result<void> Preregister(const LocalInstanceGroup& group,
+                           const std::vector<LocalInstance>& instances);
+
  private:
   OperatorControlConn(SharedFD conn) : conn_(conn) {}
 

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

@@ -81,12 +81,16 @@ std::set<std::string> FallbackDirs() {
 
 std::set<std::string> DirsForInstance(
     const CuttlefishConfig& config,
-    const CuttlefishConfig::InstanceSpecific instance) {
-  return {
-      config.assembly_dir(),
+    const CuttlefishConfig::InstanceSpecific& instance,
+    const bool include_assembly_dir) {
+  std::set<std::string> dirs = {
       instance.instance_dir(),
       instance.instance_uds_dir(),
   };
+  if (include_assembly_dir) {
+    dirs.insert(config.assembly_dir());
+  }
+  return dirs;
 }
 
 // Gets a set of the possible process groups of a previous launch
@@ -152,11 +156,13 @@ Result<void> CleanStopInstance(
 
 int StopInstance(const CuttlefishConfig& config,
                  const CuttlefishConfig::InstanceSpecific& instance,
-                 const int32_t wait_for_launcher) {
-  auto result = CleanStopInstance(instance, wait_for_launcher);
+                 const int32_t wait_for_launcher,
+                 const bool include_assembly_dir) {
+  const Result<void> result = CleanStopInstance(instance, wait_for_launcher);
   if (!result.has_value()) {
     LOG(ERROR) << "Clean stop failed: " << result.error();
-    return FallBackStop(DirsForInstance(config, instance));
+    return FallBackStop(DirsForInstance(
+        config, instance, /*include_assembly_dir=*/include_assembly_dir));
   }
 
   return 0;
@@ -205,10 +211,12 @@ int StopCvdMain(const int32_t wait_for_launcher, const bool clear_instance_dirs,
     return FallBackStop(FallbackDirs());
   }
 
-  std::set<unsigned> instance_ids(instance_nums.begin(), instance_nums.end());
+  const std::set<unsigned> instance_ids(instance_nums.begin(),
+                                        instance_nums.end());
 
   int exit_code = 0;
-  auto instances = config->Instances();
+  const std::vector<CuttlefishConfig::InstanceSpecific> instances =
+      config->Instances();
 
   if (!instance_ids.empty() && !instances.empty()) {
     unsigned first_instance_id;
@@ -223,9 +231,22 @@ int StopCvdMain(const int32_t wait_for_launcher, const bool clear_instance_dirs,
     }
   }
 
+  bool stopping_all_instances = instance_ids.empty();
+  if (!stopping_all_instances && !instances.empty()) {
+    stopping_all_instances = true;
+    for (const CuttlefishConfig::InstanceSpecific& instance : instances) {
+      unsigned id = 0;
+      if (!absl::SimpleAtoi(instance.id(), &id) ||
+          instance_ids.count(id) == 0) {
+        stopping_all_instances = false;
+        break;
+      }
+    }
+  }
+
   std::vector<std::future<int>> exit_state_futures;
   exit_state_futures.reserve(instances.size());
-  for (const auto& instance : instances) {
+  for (const CuttlefishConfig::InstanceSpecific& instance : instances) {
     unsigned id;
     if (!absl::SimpleAtoi(instance.id(), &id)) {
       LOG(ERROR) << "Failed to parse instance ID \"" << instance.id()
@@ -238,9 +259,11 @@ int StopCvdMain(const int32_t wait_for_launcher, const bool clear_instance_dirs,
     }
     std::future<int> exit_code_from_thread = std::async(
         std::launch::async,
-        [&instance, &config, &wait_for_launcher,
-         &clear_instance_dirs]() -> int {
-          int exit_status = StopInstance(*config, instance, wait_for_launcher);
+        [&instance, &config, &wait_for_launcher, &clear_instance_dirs,
+         stopping_all_instances]() -> int {
+          const int exit_status =
+              StopInstance(*config, instance, wait_for_launcher,
+                           /*include_assembly_dir=*/stopping_all_instances);
           if (clear_instance_dirs && DirectoryExists(instance.instance_dir())) {
             LOG(INFO) << "Deleting instance dir " << instance.instance_dir();
             if (!RecursivelyRemoveDirectory(instance.instance_dir())

e2etests/orchestration/stop_start_test/BUILD.bazel

@@ -16,6 +16,7 @@ load("@rules_go//go:def.bzl", "go_test")
 
 go_test(
     name = "stop_start_test_test",
+    size = "large",
     srcs = ["main_test.go"],
     deps = [
         "//orchestration/common",