Skip to content

fix(spurctld): delete sacctmgr users atomically - #451

Open
hnotshe wants to merge 3 commits into
ROCm:mainfrom
hnotshe:fix/sacctmgr-delete-user
Open

fix(spurctld): delete sacctmgr users atomically#451
hnotshe wants to merge 3 commits into
ROCm:mainfrom
hnotshe:fix/sacctmgr-delete-user

Conversation

@hnotshe

@hnotshe hnotshe commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Fixes #435.

sacctmgr delete user could report Done. without deleting anything because the all-accounts form sent an empty account while the database required an exact account match. The RPC also ignored zero affected rows.

This change deletes matching users and associations rows in one transaction, treats an omitted account as all accounts, and returns NotFound when no row was removed instead of reporting false success.

Testing:

  • focused PostgreSQL regression covering one-account deletion, all-account deletion, association cleanup, and repeated deletion
  • cargo fmt --all --check
  • cargo clippy --workspace --exclude spur-ffi --all-targets --locked
  • cargo test --locked

@hnotshe
hnotshe marked this pull request as ready for review July 16, 2026 00:16
@yansun1996

Copy link
Copy Markdown
Member

The core fix looks correct: a single transaction with proper rollback on partial failure (? drops the tx before commit), ($2 = '' OR account = $2) correctly meaning all-accounts, the summed rows_affected() mapping zero → NotFound, and the test asserting row absence rather than just return codes. A couple of the notes inline are pre-existing rather than introduced here — flagged just for context.

One additional note: the new regression test is #[ignore]'d (it needs DATABASE_URL), so cargo test --locked won't run it in CI. That's consistent with the other DB tests, but it does mean the guard for this fix isn't exercised by default CI.

Comment thread crates/spurctld/src/accounting/grpc.rs
Comment thread crates/spurctld/src/accounting/db.rs
Comment thread crates/spurctld/src/accounting/grpc.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a correctness gap in the accounting RPC for sacctmgr delete user by making user/association deletion transactional, supporting “all accounts” deletion when the account field is omitted, and returning a gRPC NotFound instead of reporting success when nothing was actually deleted.

Changes:

  • Update the remove_user gRPC handler to detect zero deletions and return Status::not_found instead of false success.
  • Rework db::remove_user to delete matching associations and users rows within a single SQL transaction, treating an empty account as “all accounts”.
  • Add a focused PostgreSQL regression test covering one-account deletion, all-account deletion, association cleanup, and repeated deletion behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/spurctld/src/accounting/grpc.rs Returns NotFound when remove_user deletes zero rows, avoiding misleading “Done.” on no-op deletes.
crates/spurctld/src/accounting/db.rs Makes remove_user transactional across associations + users, supports empty-account-as-all, and adds a regression test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yansun1996
yansun1996 force-pushed the fix/sacctmgr-delete-user branch from 40017c7 to 9003f06 Compare July 30, 2026 00:36
yansun1996
yansun1996 previously approved these changes Jul 30, 2026
@codecov-commenter

codecov-commenter commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 100 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
- Coverage   74.25%   74.24%   -0.01%     
==========================================
  Files         165      165              
  Lines       59071    59425     +354     
==========================================
+ Hits        43862    44116     +254     
- Misses      15209    15309     +100     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yansun1996
yansun1996 force-pushed the fix/sacctmgr-delete-user branch from 056a7f7 to b8d21dd Compare July 30, 2026 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sacctmgr delete user reports success but does not remove the user/association

4 participants