fix(spurctld): delete sacctmgr users atomically

crates/spur-cli/src/sacctmgr.rs

@@ -534,18 +534,26 @@ async fn delete(entity: &str, params: &[String], addr: &str) -> Result<()> {
             let account = p.get("account").cloned().unwrap_or_default();
 
             let mut client = connect(addr).await?;
-            client
+            let acct_display = if account.is_empty() { "all" } else { &account };
+            match client
                 .remove_user(RemoveUserRequest {
                     user: name.clone(),
                     account: account.clone(),
                 })
                 .await
-                .context("RemoveUser RPC failed")?;
-
-            let acct_display = if account.is_empty() { "all" } else { &account };
-            println!(" Deleting user {} from account {}", name, acct_display);
-            println!(" Done.");
-            Ok(())
+            {
+                Ok(_) => {
+                    println!(" Deleting user {} from account {}", name, acct_display);
+                    println!(" Done.");
+                    Ok(())
+                }
+                // Slurm prints "Nothing deleted" and exits 0 when the delete is a no-op.
+                Err(status) if status.code() == tonic::Code::NotFound => {
+                    println!(" Nothing deleted.");
+                    Ok(())
+                }
+                Err(status) => Err(anyhow::Error::new(status).context("RemoveUser RPC failed")),
+            }
         }
         "qos" => {
             let name = p

crates/spurctld/src/accounting/db.rs

@@ -855,19 +855,22 @@ pub async fn add_user<'a>(
     Ok(())
 }
 
-/// Remove a user from an account.
-pub async fn remove_user(pool: &PgPool, user: &str, account: &str) -> anyhow::Result<()> {
-    sqlx::query("DELETE FROM users WHERE name = $1 AND account = $2")
-        .bind(user)
-        .bind(account)
-        .execute(pool)
-        .await?;
-    sqlx::query("DELETE FROM associations WHERE user_name = $1 AND account = $2")
+/// Remove a user from one account, or every account when `account` is empty.
+pub async fn remove_user(pool: &PgPool, user: &str, account: &str) -> anyhow::Result<u64> {
+    let mut tx = pool.begin().await?;
+    let associations =
+        sqlx::query("DELETE FROM associations WHERE user_name = $1 AND ($2 = '' OR account = $2)")
+            .bind(user)
+            .bind(account)
+            .execute(&mut *tx)
+            .await?;
+    let users = sqlx::query("DELETE FROM users WHERE name = $1 AND ($2 = '' OR account = $2)")
         .bind(user)
         .bind(account)
-        .execute(pool)
+        .execute(&mut *tx)
         .await?;
-    Ok(())
+    tx.commit().await?;
+    Ok(associations.rows_affected() + users.rows_affected())
 }
 
 /// List users, joining each one's own association row for `default_qos`/
@@ -2319,6 +2322,66 @@ mod job_history_tests {
         Ok(())
     }
 
+    #[tokio::test]
+    #[ignore = "requires DATABASE_URL and PostgreSQL"]
+    async fn remove_user_deletes_one_or_all_account_associations() -> anyhow::Result<()> {
+        let pool = test_pool().await?;
+        let pid = std::process::id();
+        let user = format!("spur_remove_user_{pid}");
+        let account_one = format!("spur_remove_acct_one_{pid}");
+        let account_two = format!("spur_remove_acct_two_{pid}");
+
+        sqlx::query("DELETE FROM associations WHERE user_name = $1")
+            .bind(&user)
+            .execute(&pool)
+            .await?;
+        sqlx::query("DELETE FROM users WHERE name = $1")
+            .bind(&user)
+            .execute(&pool)
+            .await?;
+        sqlx::query("DELETE FROM accounts WHERE name IN ($1, $2)")
+            .bind(&account_one)
+            .bind(&account_two)
+            .execute(&pool)
+            .await?;
+
+        upsert_account(&pool, &account_one, "d", "o", None, 1, None).await?;
+        upsert_account(&pool, &account_two, "d", "o", None, 1, None).await?;
+        add_user(&pool, &user, &account_one, "none", true, "").await?;
+        add_user(&pool, &user, &account_two, "none", false, "").await?;
+
+        let deleted = remove_user(&pool, &user, &account_one).await?;
+        assert_eq!(deleted, 2);
+        let remaining = list_users(&pool, None).await?;
+        assert!(!remaining
+            .iter()
+            .any(|record| record.name == user && record.account == account_one));
+        assert!(remaining
+            .iter()
+            .any(|record| record.name == user && record.account == account_two));
+
+        let deleted = remove_user(&pool, &user, "").await?;
+        assert_eq!(deleted, 2);
+        let remaining = list_users(&pool, None).await?;
+        assert!(!remaining.iter().any(|record| record.name == user));
+        let association_count: i64 =
+            sqlx::query_scalar("SELECT COUNT(*) FROM associations WHERE user_name = $1")
+                .bind(&user)
+                .fetch_one(&pool)
+                .await?;
+        assert_eq!(association_count, 0);
+
+        let deleted = remove_user(&pool, &user, "").await?;
+        assert_eq!(deleted, 0);
+
+        sqlx::query("DELETE FROM accounts WHERE name IN ($1, $2)")
+            .bind(&account_one)
+            .bind(&account_two)
+            .execute(&pool)
+            .await?;
+        Ok(())
+    }
+
     #[tokio::test]
     #[ignore = "requires DATABASE_URL and PostgreSQL"]
     async fn list_users_reads_default_qos_from_a_legacy_empty_partition_row() -> anyhow::Result<()>

crates/spurctld/src/accounting/grpc.rs

@@ -460,9 +460,17 @@ impl SlurmAccounting for AccountingService {
         request: Request<RemoveUserRequest>,
     ) -> Result<Response<()>, Status> {
         let req = request.into_inner();
-        db::remove_user(&self.pool, &req.user, &req.account)
+        let deleted = db::remove_user(&self.pool, &req.user, &req.account)
             .await
             .map_err(|e| Status::internal(e.to_string()))?;
+        if deleted == 0 {
+            let target = if req.account.is_empty() {
+                format!("user '{}'", req.user)
+            } else {
+                format!("user '{}' in account '{}'", req.user, req.account)
+            };
+            return Err(Status::not_found(format!("{target} does not exist")));
+        }
         Ok(Response::new(()))
     }