[hcs] Fix issue with powering off after suspension

src/platform/backends/hyperv_api/hcs_virtual_machine.cpp

@@ -121,16 +121,20 @@ HCSVirtualMachine::HCSVirtualMachine(const std::string& network_guid,
       monitor(monitor)
 {
     const auto created_from_scratch = maybe_create_compute_system();
-    const auto state = fetch_state_from_api();
+    const auto compute_state = fetch_state_from_api();
 
     mpl::debug(get_name(),
                "HCSVirtualMachine() > created_from_scratch: {}, state: {}",
                created_from_scratch,
-               state);
+               compute_state);
 
     // Reflect compute system's state
-    set_state(state);
-    HCSVirtualMachine::handle_state_update();
+    const auto prev_state = state;
+    set_state(compute_state);
+
+    // Persist initial state even if unchanged
+    if (prev_state == this->state)
+        HCSVirtualMachine::handle_state_update();
 }
 
 HCSVirtualMachine::~HCSVirtualMachine()
@@ -142,8 +146,7 @@ HCSVirtualMachine::~HCSVirtualMachine()
         // Auto-suspend if running
         suspend();
         // Persist previous VM state
-        state = VirtualMachine::State::running;
-        handle_state_update();
+        set_state(State::running);
     });
 }
 
@@ -162,8 +165,7 @@ void HCSVirtualMachine::compute_system_event_callback(HCS_EVENT* event, void* co
     case hcs::HcsEventType::SystemExited:
     {
         mpl::info(vm->get_name(), "compute_system_event_callback() > SystemExited event received");
-        vm->state = State::off;
-        vm->handle_state_update();
+        vm->set_state(State::off);
         vm->termination_signal.signal();
     }
     break;
@@ -364,31 +366,36 @@ void HCSVirtualMachine::set_state(hcs::ComputeSystemState compute_system_state)
         return;
     }
 
-    const auto prev_state = state;
     switch (compute_system_state)
     {
     case hcs::ComputeSystemState::created:
-        state = State::off;
+        set_state(State::off);
         break;
     case hcs::ComputeSystemState::paused:
-        state = State::suspended;
+        mpl::debug(vm_name, "VM is paused but not completely suspended");
+        set_state(State::suspended);
         break;
     case hcs::ComputeSystemState::running:
-        state = State::running;
+        set_state(State::running);
         break;
     case hcs::ComputeSystemState::saved_as_template:
     case hcs::ComputeSystemState::stopped:
-        state = has_saved_state_file() ? State::suspended : State::stopped;
+        set_state(has_saved_state_file() ? State::suspended : State::stopped);
         break;
     case hcs::ComputeSystemState::unknown:
-        state = State::unknown;
+        set_state(State::unknown);
         break;
     }
+}
 
-    if (state == prev_state)
+void HCSVirtualMachine::set_state(VirtualMachine::State new_state)
+{
+    if (state == new_state)
         return;
 
-    mpl::info(get_name(), "set_state() -> State changed from {} to {}", prev_state, state);
+    mpl::info(get_name(), "set_state() -> State changed from {} to {}", state, new_state);
+    state = new_state;
+    handle_state_update();
 }
 
 void HCSVirtualMachine::start()
@@ -400,8 +407,7 @@ void HCSVirtualMachine::start()
         mpl::debug(get_name(), "start() -> VM was not present, created from scratch");
 
     const auto prev_state = state;
-    state = VirtualMachine::State::starting;
-    handle_state_update();
+    set_state(VirtualMachine::State::starting);
     // Resume and start are the same thing in Multipass terms
     // Try to determine whether we need to resume or start here.
     const auto result = [&] {
@@ -426,8 +432,7 @@ void HCSVirtualMachine::start()
 
     if (!result)
     {
-        state = prev_state;
-        handle_state_update();
+        set_state(prev_state);
         throw StartComputeSystemException{"Could not start the VM: {}", result};
     }
     else if (has_saved_state_file())
@@ -444,6 +449,7 @@ void HCSVirtualMachine::start()
 
     mpl::debug(get_name(), "start() -> result `{}`", result);
 }
+
 void HCSVirtualMachine::shutdown(ShutdownPolicy shutdown_policy)
 {
     mpl::debug(get_name(), "shutdown() -> Shutting down, current state {}", state);
@@ -458,6 +464,10 @@ void HCSVirtualMachine::shutdown(ShutdownPolicy shutdown_policy)
         return;
     }
 
+    // Ensure that the ssh session is dropped at the end of this function, even if shutdown
+    // fails, since it means that the VM is in some sort of error state.
+    auto drop_ssh_session_sg = sg::make_scope_guard([this]() noexcept { drop_ssh_session(); });
+
     switch (shutdown_policy)
     {
     case ShutdownPolicy::Powerdown:
@@ -468,23 +478,36 @@ void HCSVirtualMachine::shutdown(ShutdownPolicy shutdown_policy)
         {
             // Fall back to SSH shutdown.
             ssh_exec("sudo shutdown -h now");
-            drop_ssh_session();
         }
         break;
     case ShutdownPolicy::Halt:
     case ShutdownPolicy::Poweroff:
         mpl::debug(get_name(),
                    "shutdown() -> Requested halt/poweroff, initiating forceful shutdown");
+
+        // FIXME: There is a rare case where suspend fails, and fails to terminate
+        // the VM as well. In this case the VM will be "paused". This is not handled
+        // for now.
+        if (state == State::suspended)
+        {
+            if (const auto ec = remove_saved_state_file_if_exists(); ec)
+                throw ShutdownComputeSystemException("Could not remove state file '{}': {}",
+                                                     get_saved_state_file_path(),
+                                                     ec);
+            update_current_state();
+            return;
+        }
         // These are non-graceful variants. Just terminate the system immediately.
         const auto r = HCS().terminate_compute_system(hcs_system);
+        if (!r)
+            throw ShutdownComputeSystemException("Could not terminate VM `{}`: {}", get_name(), r);
         mpl::debug(get_name(), "shutdown -> terminate_compute_system result: {}", r.code);
-        drop_ssh_session();
         break;
     }
 
     // We need to wait here.
     if (!termination_signal.wait_for(vm_shutdown_timeout))
-        throw std::runtime_error("timed out waiting for VM shutdown to complete");
+        throw ShutdownComputeSystemException("timed out waiting for VM shutdown to complete");
 
     switch (auto s = current_state())
     {
@@ -493,7 +516,7 @@ void HCSVirtualMachine::shutdown(ShutdownPolicy shutdown_policy)
     case VirtualMachine::State::suspended:
         break;
     default:
-        mpl::warn(get_name(), "shutdown -> VM is not in stopped state after termination: {}", s);
+        throw ShutdownComputeSystemException("VM is not in stopped state after termination: {}", s);
         break;
     }
 }
@@ -502,34 +525,32 @@ void HCSVirtualMachine::suspend()
 {
     mpl::debug(get_name(), "suspend() -> Suspending, current state {}", state);
 
-    if (const auto pause_result = HCS().pause_compute_system(hcs_system))
-    {
-        // Pause succeeded. We can suspend to disk now
-        if (const auto& r = HCS().save_compute_system(hcs_system, get_saved_state_file_path()); r)
-        {
-            // Save succeeded. Now, it's safe to terminate the system.
-            shutdown(ShutdownPolicy::Poweroff);
-        }
-        else
-            throw SaveComputeSystemException{"Could not save the virtual machine state for VM `{}` "
-                                             "to the disk for suspend. Error details: {}",
-                                             get_name(),
-                                             r};
-    }
-    else
-    {
+    if (const auto pause_result = HCS().pause_compute_system(hcs_system); !pause_result)
         throw SaveComputeSystemException{"Could not pause VM for suspend: {}", pause_result};
+
+    if (const auto save_result = HCS().save_compute_system(hcs_system, get_saved_state_file_path());
+        !save_result)
+    {
+        recover_from_failed_save();
+        throw SaveComputeSystemException{"Failed to save suspended VM state to disk: {}",
+                                         save_result};
     }
 
-    set_state(fetch_state_from_api());
-    handle_state_update();
+    // NOTE: We intentionally keep the old state here because updating it would map "paused" to
+    // "suspended", causing shutdown to remove the newly saved state file.
+    shutdown(ShutdownPolicy::Poweroff);
+    return;
 }
 
 HCSVirtualMachine::State HCSVirtualMachine::current_state()
 {
-    set_state(fetch_state_from_api());
+    update_current_state();
     return state;
 }
+void HCSVirtualMachine::update_current_state()
+{
+    set_state(fetch_state_from_api());
+}
 int HCSVirtualMachine::ssh_port()
 {
     return default_ssh_port;
@@ -687,4 +708,46 @@ std::shared_ptr<Snapshot> HCSVirtualMachine::make_specific_snapshot(const QStrin
                                                         description);
 }
 
+std::error_code HCSVirtualMachine::remove_saved_state_file_if_exists()
+{
+    if (has_saved_state_file())
+    {
+        mpl::trace(get_name(), "Saved state file exists, attempting to remove");
+        std::error_code ec{};
+        if (!MP_FILEOPS.remove(get_saved_state_file_path(), ec))
+        {
+            // FIXME: If the VM is stopped or terminated, it will still be reported as
+            // suspended because the saved-state file exists.
+            mpl::warn(get_name(), "Could not remove the saved state file, error: {}", ec);
+            return ec;
+        }
+    }
+
+    return {};
+}
+
+void HCSVirtualMachine::recover_from_failed_save()
+{
+    remove_saved_state_file_if_exists();
+
+    const auto resume_result = HCS().resume_compute_system(hcs_system);
+    if (resume_result)
+    {
+        update_current_state();
+        return;
+    }
+
+    mpl::error(get_name(),
+               "Could not resume after failed suspend ({}); powering off",
+               resume_result);
+    try
+    {
+        shutdown(ShutdownPolicy::Poweroff);
+    }
+    catch (const std::exception& e)
+    {
+        mpl::error(get_name(), "Power off after failed suspend also failed: {}", e.what());
+    }
+}
+
 } // namespace multipass::hyperv

src/platform/backends/hyperv_api/hcs_virtual_machine.h

@@ -28,6 +28,7 @@
 
 #include <memory>
 #include <optional>
+#include <system_error>
 
 struct HCS_EVENT;
 
@@ -105,6 +106,8 @@ struct HCSVirtualMachine : public BaseVirtualMachine
 
     [[nodiscard]] hcs::ComputeSystemState fetch_state_from_api() const;
     void set_state(hcs::ComputeSystemState state);
+    void set_state(State state);
+    void update_current_state();
 
     /**
      * Create the compute system if it's not already present.
@@ -123,6 +126,8 @@ struct HCSVirtualMachine : public BaseVirtualMachine
     [[nodiscard]] std::filesystem::path get_runtime_state_file_path() const;
     [[nodiscard]] std::filesystem::path get_saved_state_file_path() const;
     [[nodiscard]] bool has_saved_state_file() const;
+    std::error_code remove_saved_state_file_if_exists();
+    void recover_from_failed_save();
 
     void grant_access_to_scsi_device(const hcs::HcsScsiDevice& device) const;
     void grant_access_to_paths(std::list<std::filesystem::path> paths) const;

src/platform/backends/hyperv_api/hcs_virtual_machine_exceptions.h

@@ -92,4 +92,9 @@ struct SaveComputeSystemException : public FormattedExceptionBase<>
     using FormattedExceptionBase::FormattedExceptionBase;
 };
 
+struct ShutdownComputeSystemException : public FormattedExceptionBase<>
+{
+    using FormattedExceptionBase::FormattedExceptionBase;
+};
+
 } // namespace multipass::hyperv