fix(backup): roll back a half-created config on upsert's create path (audit H6, L12)

crates/commons-servers/src/backup_secrets.rs

@@ -145,13 +145,21 @@ impl BackupSecrets {
 				Ok(())
 			}
 			Self::Memory(store) => {
-				store
-					.lock()
-					.unwrap()
-					.entry(secret_name.to_string())
-					.or_default()
-					.insert(key.to_string(), value.to_string());
-				Ok(())
+				use std::collections::btree_map::Entry;
+
+				match store.lock().unwrap().entry(secret_name.to_string()) {
+					// Kube answers 409 here. The double has to as well, or the
+					// double-create path passes in tests and 502s in production
+					// — and the callers' rollback of a failed create can't be
+					// exercised at all.
+					Entry::Occupied(_) => Err(AppError::Upstream(format!(
+						"secret create failed: {secret_name} already exists"
+					))),
+					Entry::Vacant(slot) => {
+						slot.insert(BTreeMap::from([(key.to_string(), value.to_string())]));
+						Ok(())
+					}
+				}
 			}
 		}
 	}
@@ -312,6 +320,33 @@ mod tests {
 		// Deleting an already-absent Secret is a no-op success.
 		secrets.delete_password("backup-repo-x").await.unwrap();
 	}
+
+	/// `create_password` is create-if-absent (Kube answers 409). The double
+	/// has to reject too — otherwise a double-create passes in tests and 502s
+	/// in production, and no test can reach a caller's rollback path.
+	#[tokio::test]
+	async fn memory_create_password_rejects_an_existing_secret() {
+		let secrets = BackupSecrets::memory();
+		secrets
+			.create_password("backup-repo-y", "password", "first")
+			.await
+			.unwrap();
+		assert!(
+			secrets
+				.create_password("backup-repo-y", "password", "second")
+				.await
+				.is_err(),
+			"creating over an existing secret must fail",
+		);
+		assert_eq!(
+			secrets
+				.read_password("backup-repo-y", "password")
+				.await
+				.unwrap(),
+			"first",
+			"the rejected create must not have overwritten anything",
+		);
+	}
 }
 
 /// Generate a strong repo passphrase: 8 words from the EFF large wordlist

crates/private-server/src/fns/backups.rs

@@ -1076,12 +1076,22 @@ pub async fn upsert(
 				},
 			)
 			.await?;
-			kube.create_password(
-				&repo_password_ref,
-				REPO_PASSWORD_SECRET_KEY,
-				&commons_servers::backup_secrets::generate_passphrase(),
-			)
-			.await?;
+			// Same all-or-nothing invariant as `create`: a failed Secret create
+			// must not leave a half-created config stuck in `provisioning` with
+			// a `repo_password_ref` pointing at nothing. Without the rollback
+			// every retry takes the *update* path — which never creates the
+			// Secret — so the group could never be provisioned again.
+			if let Err(e) = kube
+				.create_password(
+					&repo_password_ref,
+					REPO_PASSWORD_SECRET_KEY,
+					&commons_servers::backup_secrets::generate_passphrase(),
+				)
+				.await
+			{
+				let _ = ServerGroupBackupConfig::delete(&mut conn, args.server_group_id).await;
+				return Err(e);
+			}
 		}
 	}
 

crates/private-server/tests/it/backups.rs

@@ -856,6 +856,52 @@ async fn update_region_and_delete() {
 	.await;
 }
 
+/// Onboarding is all-or-nothing on both create paths. A config row left behind
+/// by a failed Secret create is unrecoverable: it points its
+/// `repo_password_ref` at nothing, and every retry now sees a config and takes
+/// the *update* path, which never creates the Secret.
+#[tokio::test(flavor = "multi_thread")]
+async fn upsert_rolls_back_the_config_when_the_secret_cannot_be_created() {
+	commons_tests::server::run(async |mut conn, _public, private| {
+		let group_id = seed_group(&mut conn).await;
+
+		// Onboard once so the group's passphrase Secret exists...
+		let resp = private
+			.post("/api/backups/create")
+			.json(&serde_json::json!({
+				"server_group_id": group_id,
+				"bucket": "bes-iac",
+				"target_role_arn": "arn:aws:iam::123:role/dev",
+				"maintenance_role_arn": "arn:aws:iam::123:role/maint",
+				"mode": "from_birth",
+			}))
+			.await;
+		resp.assert_status_ok();
+
+		// ...then drop only the config row, leaving the Secret behind. `upsert`
+		// now takes the create path, and its create-if-absent Secret write
+		// fails — the shape of any transient secret-store failure.
+		conn.batch_execute(&format!(
+			"DELETE FROM server_group_backup_config WHERE group_id = '{group_id}'"
+		))
+		.await
+		.expect("drop config row");
+
+		let resp = private
+			.post("/api/backups/upsert")
+			.json(&serde_json::json!({
+				"server_group_id": group_id,
+				"bucket": "bes-iac",
+				"target_role_arn": "arn:aws:iam::123:role/dev",
+				"maintenance_role_arn": "arn:aws:iam::123:role/maint",
+			}))
+			.await;
+		resp.assert_status(axum::http::StatusCode::BAD_GATEWAY);
+		assert_no_config!(private, group_id);
+	})
+	.await;
+}
+
 #[tokio::test(flavor = "multi_thread")]
 async fn upsert_creates_then_reapplies_idempotently() {
 	commons_tests::server::run(async |mut conn, _public, private| {