commit repos to the database first, then hand off

oxidecomputer · Jan 6, 2025 · 6aa4af5 · 6aa4af5
1 parent a113221
commit 6aa4af5
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 46 deletions.
diff --git a/nexus/src/app/update/mod.rs b/nexus/src/app/update/mod.rs
@@ -11,7 +11,7 @@ use nexus_db_model::TufRepoDescription;
 use nexus_db_queries::authz;
 use nexus_db_queries::context::OpContext;
 use omicron_common::api::external::{
-    Error, SemverVersion, TufRepoInsertResponse,
+    Error, SemverVersion, TufRepoInsertResponse, TufRepoInsertStatus,
 };
 use omicron_common::update::ArtifactId;
 use update_common::artifacts::ArtifactsWithPlan;
@@ -56,29 +56,31 @@ impl super::Nexus {
             ArtifactsWithPlan::from_stream(body, Some(file_name), &self.log)
                 .await
                 .map_err(|error| error.to_http_error())?;
+        // Now store the artifacts in the database.
         let tuf_repo_description = TufRepoDescription::from_external(
             artifacts_with_plan.description().clone(),
         );
-        // Move the `ArtifactsWithPlan`, which carries with it the
-        // `Utf8TempDir`s storing the artifacts, into the artifact replication
-        // background task. This is done before the database insert because if
-        // this fails, we shouldn't record the artifacts in the database.
-        self.tuf_artifact_replication_tx
-            .send(artifacts_with_plan)
-            .await
-            .map_err(|err| {
-                Error::internal_error(&format!(
-                    "failed to send artifacts for replication: {err}"
-                ))
-            })?;
-        // Now store the artifacts in the database.
         let response = self
             .db_datastore
             .update_tuf_repo_insert(opctx, tuf_repo_description)
             .await
             .map_err(HttpError::from)?;
-        // Finally, immediately activate the artifact replication task.
-        self.background_tasks.task_tuf_artifact_replication.activate();
+
+        // If we inserted a new repository, move the `ArtifactsWithPlan` (which
+        // carries with it the `Utf8TempDir`s storing the artifacts) into the
+        // artifact replication background task, then immediately activate the
+        // task.
+        if response.status == TufRepoInsertStatus::Inserted {
+            self.tuf_artifact_replication_tx
+                .send(artifacts_with_plan)
+                .await
+                .map_err(|err| {
+                    Error::internal_error(&format!(
+                        "failed to send artifacts for replication: {err}"
+                    ))
+                })?;
+            self.background_tasks.task_tuf_artifact_replication.activate();
+        }
 
         Ok(response.into_external())
     }

diff --git a/nexus/tests/integration_tests/updates.rs b/nexus/tests/integration_tests/updates.rs
@@ -195,16 +195,6 @@ async fn test_repo_upload() -> Result<()> {
         "initial description matches fetched description"
     );
 
-    // Even though the repository already exists, the artifacts are sent to the
-    // replication task ahead of database insertion. The task should have run
-    // once, found nothing to do, and deleted the artifacts.
-    let status =
-        wait_tuf_artifact_replication_step(&cptestctx.internal_client).await;
-    eprintln!("{status:?}");
-    assert_eq!(status.requests_ok, 0);
-    assert_eq!(status.requests_outstanding, 0);
-    assert_eq!(status.local_repos, 0);
-
     // Upload a new repository with the same system version but a different
     // version for one of the components. This will produce a different hash,
     // which should return an error.
@@ -232,16 +222,6 @@ async fn test_repo_upload() -> Result<()> {
         )?;
     }
 
-    // Even though the repository was rejected, the artifacts are sent to the
-    // replication task ahead of database insertion. The task should have run
-    // once, found nothing to do, and deleted the artifacts.
-    let status =
-        wait_tuf_artifact_replication_step(&cptestctx.internal_client).await;
-    eprintln!("{status:?}");
-    assert_eq!(status.requests_ok, 0);
-    assert_eq!(status.requests_outstanding, 0);
-    assert_eq!(status.local_repos, 0);
-
     // Upload a new repository with a different system version and different
     // contents (but same version) for an artifact.
     {
@@ -268,16 +248,6 @@ async fn test_repo_upload() -> Result<()> {
         )?;
     }
 
-    // Even though the repository was rejected, the artifacts are sent to the
-    // replication task ahead of database insertion. The task should have run
-    // once, found nothing to do, and deleted the artifacts.
-    let status =
-        wait_tuf_artifact_replication_step(&cptestctx.internal_client).await;
-    eprintln!("{status:?}");
-    assert_eq!(status.requests_ok, 0);
-    assert_eq!(status.requests_outstanding, 0);
-    assert_eq!(status.local_repos, 0);
-
     // Upload a new repository with a different system version but no other
     // changes. This should be accepted.
     {