From 964b8ce2d598a3fe68ac5e561afd4f2831a9d9d6 Mon Sep 17 00:00:00 2001 From: "A. Cody Schuffelen" Date: Fri, 10 Jul 2026 17:18:07 -0700 Subject: [PATCH 1/2] Add a default value to the timeout in InstanceManager::StopInstanceGroup Every caller was passing the same values. Bug: b/533440580 --- .../host/commands/cvd/cli/commands/remove.cpp | 3 +-- .../host/commands/cvd/cli/commands/start.cpp | 3 +-- .../host/commands/cvd/cli/commands/stop.cpp | 8 ++------ .../commands/cvd/instances/instance_manager.cpp | 15 +++++---------- .../commands/cvd/instances/instance_manager.h | 5 ++--- 5 files changed, 11 insertions(+), 23 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/remove.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/remove.cpp index 5d688e72cf4..6cca623ec57 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/remove.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/remove.cpp @@ -16,7 +16,6 @@ #include "cuttlefish/host/commands/cvd/cli/commands/remove.h" -#include #include #include @@ -94,7 +93,7 @@ Result RemoveCvdCommandHandler::StopGroup( return {}; } CF_EXPECT(instance_manager_.StopInstanceGroup( - group, std::chrono::seconds(5), InstanceDirActionOnStop::Clear)); + group, InstanceDirActionOnStop::Clear)); return {}; } diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/start.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/start.cpp index 1a67fdfa4f6..5aec366d497 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/start.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/start.cpp @@ -24,7 +24,6 @@ #include #include -#include #include #include #include @@ -504,7 +503,7 @@ Result CvdStartCommandHandler::Handle(const CommandRequest& request) { LOG(INFO) << "Stopping device..."; CF_EXPECT(instance_manager_.StopInstanceGroup( - group, std::chrono::seconds(5), InstanceDirActionOnStop::Keep, {})); + group, InstanceDirActionOnStop::Keep)); LOG(INFO) << "Device stopped."; return monitor_res; } diff --git a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/stop.cpp b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/stop.cpp index 0bdcc73328c..5d6acdde006 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/cli/commands/stop.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/cli/commands/stop.cpp @@ -41,10 +41,6 @@ namespace { constexpr char kSummaryHelpText[] = "Stop Cuttlefish instances"; -struct StopFlags { - size_t wait_for_launcher_secs = 5; - bool clear_instance_dirs = false; -}; } // namespace CvdStopCommandHandler::CvdStopCommandHandler(InstanceManager& instance_manager) @@ -80,10 +76,10 @@ Result CvdStopCommandHandler::Handle(const CommandRequest& request) { } Result stop_outcome = instance_manager_.StopInstanceGroup( - group, launcher_timeout, + group, flags_.clear_instance_dirs ? InstanceDirActionOnStop::Clear : InstanceDirActionOnStop::Keep, - instance_nums); + launcher_timeout.value_or(std::chrono::seconds(0)), instance_nums); GatherVmStopMetrics(group); diff --git a/base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.cpp b/base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.cpp index eb0a17bfbeb..27ef244e4ef 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.cpp @@ -212,9 +212,8 @@ Result InstanceManager::UpdateInstanceGroup( } Result InstanceManager::StopInstanceGroup( - LocalInstanceGroup& group, - std::optional launcher_timeout, - InstanceDirActionOnStop instance_dir_action, + LocalInstanceGroup& group, InstanceDirActionOnStop instance_dir_action, + std::chrono::seconds launcher_timeout, const std::vector& instance_nums) { // Validate that the requested instances actually belong to this group std::set valid_ids; @@ -231,14 +230,10 @@ Result InstanceManager::StopInstanceGroup( const auto stop_bin = CF_EXPECT(StopBin(group.HostArtifactsPath())); const auto stop_bin_path = group.HostArtifactsPath() + "/bin/" + stop_bin; - int wait_for_launcher_secs = 0; - if (launcher_timeout.has_value()) { - wait_for_launcher_secs = launcher_timeout->count(); - } Result cmd_result = RunStopCvd(StopCvdParams{ .bin_path = stop_bin_path, .home_dir = group.HomeDir(), - .wait_for_launcher_secs = wait_for_launcher_secs, + .wait_for_launcher_secs = static_cast(launcher_timeout.count()), .clear_runtime_dirs = instance_dir_action == InstanceDirActionOnStop::Clear, .instance_nums = instance_nums, @@ -271,8 +266,8 @@ Result InstanceManager::Clear() { for (auto& group : instance_groups) { // Only stop running instances. if (group.HasActiveInstances()) { - auto stop_result = StopInstanceGroup(group, std::chrono::seconds(5), - InstanceDirActionOnStop::Clear); + Result stop_result = + StopInstanceGroup(group, InstanceDirActionOnStop::Clear); if (!stop_result.ok()) { LOG(ERROR) << stop_result.error(); } diff --git a/base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.h b/base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.h index 458ee3a2afa..8e68c678bc4 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.h +++ b/base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.h @@ -83,9 +83,8 @@ class InstanceManager { // Stops the device by asking it over the control socket. If launcher_timeout // has a value, it will wait for at most that time before returning an error. Result StopInstanceGroup( - LocalInstanceGroup& group, - std::optional launcher_timeout, - InstanceDirActionOnStop instance_dir_action, + LocalInstanceGroup& group, InstanceDirActionOnStop instance_dir_action, + std::chrono::seconds launcher_timeout = std::chrono::seconds(5), const std::vector& instance_nums = {}); private: From 334c4d7d7aaab788cff173481bb90aef86003bed Mon Sep 17 00:00:00 2001 From: "A. Cody Schuffelen" Date: Fri, 10 Jul 2026 17:47:01 -0700 Subject: [PATCH 2/2] Merge RunStopCvdCmd into RunStopCvd Removed some print statements and simplified code. Bug: b/533440580 --- .../host/commands/cvd/instances/stop.cpp | 64 +++++++------------ 1 file changed, 24 insertions(+), 40 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/cvd/instances/stop.cpp b/base/cvd/cuttlefish/host/commands/cvd/instances/stop.cpp index dc4c2e803a7..c3e44220388 100644 --- a/base/cvd/cuttlefish/host/commands/cvd/instances/stop.cpp +++ b/base/cvd/cuttlefish/host/commands/cvd/instances/stop.cpp @@ -27,7 +27,6 @@ #include #include "absl/log/log.h" -#include "absl/strings/str_join.h" #include "android-base/file.h" #include "fmt/core.h" #include "fmt/ranges.h" @@ -63,22 +62,6 @@ static Command CreateStopCvdCommand( return command; } -Result RunStopCvdCmd( - const std::string& stopper_path, - const std::unordered_map& env, - const std::vector& args) { - Command stop_cmd = CreateStopCvdCommand(stopper_path, env, args); - - LOG(INFO) << "Running " << stop_cmd.ToString(); - Result cmd_res = RunAndCaptureStdout(std::move(stop_cmd)); - if (!cmd_res.ok()) { - LOG(ERROR) << "Failed to run " << stopper_path; - CF_EXPECT(std::move(cmd_res)); - } - VLOG(1) << "\"" << stopper_path << " successfully "; - return {}; -} - Result RunStopCvdAll(bool clear_runtime_dirs) { std::vector group_infos = CF_EXPECT(CollectRunCvdGroups()); LOG(INFO) << "Found " << group_infos.size() @@ -250,45 +233,46 @@ Result ForcefullyStopGroup(const uid_t any_id_in_group) { } Result RunStopCvd(StopCvdParams params) { - const auto& stopper_path = params.bin_path; - std::unordered_map stop_cvd_envs; - stop_cvd_envs["HOME"] = params.home_dir; // stop_cvd is located at $ANDROID_HOST_OUT/bin/stop_cvd - std::string android_host_out = - android::base::Dirname(android::base::Dirname(stopper_path)); - stop_cvd_envs[kAndroidHostOut] = android_host_out; - stop_cvd_envs[kAndroidSoongHostOut] = android_host_out; - auto config_file_path = CF_EXPECT(GetCuttlefishConfigPath(params.home_dir)); - stop_cvd_envs[kCuttlefishConfigEnvVarName] = config_file_path; - std::vector args; + const std::string android_host_out = + android::base::Dirname(android::base::Dirname(params.bin_path)); + const std::unordered_map stop_cvd_envs = { + std::make_pair("HOME", params.home_dir), + std::make_pair(kAndroidHostOut, android_host_out), + std::make_pair(kAndroidSoongHostOut, android_host_out), + std::make_pair(kCuttlefishConfigEnvVarName, + CF_EXPECT(GetCuttlefishConfigPath(params.home_dir))), + }; + std::string wait_flag = fmt::format("--wait_for_launcher={}", params.wait_for_launcher_secs); - args.push_back(wait_flag); + std::vector args = {wait_flag}; if (params.clear_runtime_dirs) { - args.push_back("--clear_instance_dirs=true"); + args.emplace_back("--clear_instance_dirs=true"); } if (!params.instance_nums.empty()) { - args.push_back(fmt::format("--instance_nums={}", - absl::StrJoin(params.instance_nums, ","))); + args.emplace_back(fmt::format("--instance_nums={}", + fmt::join(params.instance_nums, ","))); } - Result cmd_res = RunStopCvdCmd(stopper_path, stop_cvd_envs, args); + + Result cmd_res = RunAndCaptureStdout( + CreateStopCvdCommand(params.bin_path, stop_cvd_envs, args)); if (cmd_res.ok()) { return {}; } /** - * --clear_instance_dirs may not be available in old branches. This causes - * stop_cvd to terminate with a non-zero exit code due to a parsing error. Try - * again without that flag. + * --clear_instance_dirs or --instance_nums may not be available in old + * branches. This causes stop_cvd to terminate with a non-zero exit code due + * to a parsing error. Try again without that flag. */ - if (!params.clear_runtime_dirs) { + if (!params.clear_runtime_dirs && !params.instance_nums.empty()) { CF_EXPECT(std::move(cmd_res)); } - // TODO(kwstephenkim): deletes manually if `stop_cvd --clear_instance_dirs` - // failed. - LOG(ERROR) << "Perhaps --clear_instance_dirs is not supported."; + LOG(ERROR) << "--clear_instance_dirs or --instance_nums is not supported."; LOG(ERROR) << "Trying again without it"; - CF_EXPECT(RunStopCvdCmd(stopper_path, stop_cvd_envs, {wait_flag})); + CF_EXPECT(RunAndCaptureStdout( + CreateStopCvdCommand(params.bin_path, stop_cvd_envs, {wait_flag}))); return {}; } } // namespace cuttlefish