From 65c9ee15ad9af5fba43b86999da477f0e9843245 Mon Sep 17 00:00:00 2001
From: Rain <rain@oxide.computer>
Date: Wed, 8 Nov 2023 16:35:49 -0800
Subject: [PATCH] [wicket] before starting a new update, require that update
 state be cleared (#4452)

This is already how the TUI behaves, but we'd like to align the CLI with
it.
---
 wicketd/src/update_tracker.rs              | 21 +++++++++--------
 wicketd/tests/integration_tests/updates.rs | 26 +++++++++++++++++++---
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/wicketd/src/update_tracker.rs b/wicketd/src/update_tracker.rs
index 368579fd55..1ce71d183b 100644
--- a/wicketd/src/update_tracker.rs
+++ b/wicketd/src/update_tracker.rs
@@ -227,24 +227,23 @@ impl UpdateTracker {
 
         let mut errors = Vec::new();
 
-        // Check that we're not already updating any of these SPs.
-        let update_in_progress: Vec<_> = sps
+        // Check that we don't already have any update state for these SPs.
+        let existing_updates: Vec<_> = sps
             .iter()
             .filter(|sp| {
                 // If we don't have any update data for this SP, it's not in
                 // progress.
                 //
-                // If we do, it's in progress if the task is not finished.
-                update_data
-                    .sp_update_data
-                    .get(sp)
-                    .map_or(false, |data| !data.task.is_finished())
+                // This used to check that the task was finished, but we changed
+                // that in favor of forcing users to clear update state before
+                // starting a new one.
+                update_data.sp_update_data.get(sp).is_some()
             })
             .copied()
             .collect();
 
-        if !update_in_progress.is_empty() {
-            errors.push(StartUpdateError::UpdateInProgress(update_in_progress));
+        if !existing_updates.is_empty() {
+            errors.push(StartUpdateError::ExistingUpdates(existing_updates));
         }
 
         let plan = update_data.artifact_store.current_plan();
@@ -709,8 +708,8 @@ impl UpdateTrackerData {
 pub enum StartUpdateError {
     #[error("no TUF repository available")]
     TufRepositoryUnavailable,
-    #[error("targets are already being updated: {}", sps_to_string(.0))]
-    UpdateInProgress(Vec<SpIdentifier>),
+    #[error("existing update data found (must clear state before starting): {}", sps_to_string(.0))]
+    ExistingUpdates(Vec<SpIdentifier>),
 }
 
 #[derive(Debug, Clone, Error, Eq, PartialEq)]
diff --git a/wicketd/tests/integration_tests/updates.rs b/wicketd/tests/integration_tests/updates.rs
index 43367b8bc5..fb1637f44e 100644
--- a/wicketd/tests/integration_tests/updates.rs
+++ b/wicketd/tests/integration_tests/updates.rs
@@ -182,6 +182,26 @@ async fn test_updates() {
         }
     }
 
+    // Try starting the update again -- this should fail because we require that update state is
+    // cleared before starting a new one.
+    {
+        let error = wicketd_testctx
+            .wicketd_client
+            .post_start_update(&params)
+            .await
+            .expect_err(
+                "post_start_update should fail \
+                 since update data is already present",
+            );
+        let error_str = error.to_string();
+        assert!(
+            // Errors lose type information across the OpenAPI boundary, so sadly we have to match on
+            // the error string.
+            error_str.contains("existing update data found"),
+            "unexpected error: {error_str}"
+        );
+    }
+
     // Try clearing the update via the wicket CLI.
     {
         let args = vec![
@@ -426,8 +446,8 @@ async fn test_update_races() {
         .await
         .expect_err("failed because update is currently running");
 
-    // Also try starting another fake update, which should fail -- we don't let
-    // updates be started in the middle of other updates.
+    // Also try starting another fake update, which should fail -- we don't let updates be started
+    // if there's current update state.
     {
         let (_, receiver) = watch::channel(());
         let err = wicketd_testctx
@@ -439,7 +459,7 @@ async fn test_update_races() {
         assert_eq!(err.len(), 1, "one error returned: {err:?}");
         assert_eq!(
             err.first().unwrap(),
-            &StartUpdateError::UpdateInProgress(vec![sp])
+            &StartUpdateError::ExistingUpdates(vec![sp])
         );
     }