fix(accounts): roll back orphaned keyring secret on add_account failure #229

2026-07-30T01:11:12Z

KofTwentyTwo commented

2026-07-30 01:11:12 +00:00

(Migrated from github.com)

Summary

Fixes #197 (M5-S4). AccountsGrpcService::add_account writes the account's password to the keyring vault (set_secret) before persisting the account row via save_account. If set_secret succeeds but save_account then fails (disk full, DB locked, pool closed, etc.), the credential was orphaned in the keyring forever -- unreferenced by any account row, invisible to ListAccounts.

On a save_account failure, add_account now calls SecretManager::delete_secret on the just-written key before returning.
The RPC still returns the original persistence-failure Status::internal, never a fabricated success and never a rollback-only error masking the real cause.
If the rollback delete itself fails, that failure is appended to the error message (... (and failed to roll back the credential: {rollback_err})) and logged via tracing::warn!, but the primary error code/meaning stays "persistence failed."
Updated the doc comment above add_account to describe the two-step commit-with-rollback behavior instead of the previous half-true "if this fails, nothing else happens" description.

Tests

Two new tests near the existing add_account_* tests in crates/nunciod/src/grpc.rs:

add_account_rolls_back_keyring_secret_when_save_account_fails: forces a genuine save_account failure by calling DatabaseEngine::close() (closing the connection pool) before making the add_account RPC call, then asserts secrets.get_secret(...) returns Err (secret rolled back, not orphaned).
add_account_rollback_preserves_the_original_persistence_error: same forced-failure setup, asserts the surfaced error message describes the persistence failure ("failed to persist account") and is not the unrelated set_secret failure message.

All existing add_account_* tests pass unmodified (happy-path behavior unchanged).

Gate output

cargo fmt --all -- --check   -> clean, no output
cargo check-all              -> Finished `dev` profile [unoptimized + debuginfo] target(s) in 4.29s (zero warnings)
cargo test-all                -> 400 passed; 0 failed; 0 ignored (across all workspace crates)

Notable subset (nunciod lib): 112 passed; 0 failed; 0 ignored, including the two new rollback tests.

Test plan

cargo fmt --all -- --check
cargo check-all (clippy --all-targets --workspace -D warnings)
cargo test-all (400 passed, 0 failed, 0 ignored)
New tests force a real save_account failure via DatabaseEngine::close(), not a simulated one
Existing add_account_* tests pass unmodified

## Summary Fixes #197 (M5-S4). `AccountsGrpcService::add_account` writes the account's password to the keyring vault (`set_secret`) before persisting the account row via `save_account`. If `set_secret` succeeds but `save_account` then fails (disk full, DB locked, pool closed, etc.), the credential was orphaned in the keyring forever -- unreferenced by any account row, invisible to `ListAccounts`. - On a `save_account` failure, `add_account` now calls `SecretManager::delete_secret` on the just-written key before returning. - The RPC still returns the original persistence-failure `Status::internal`, never a fabricated success and never a rollback-only error masking the real cause. - If the rollback delete itself fails, that failure is appended to the error message (`... (and failed to roll back the credential: {rollback_err})`) and logged via `tracing::warn!`, but the primary error code/meaning stays "persistence failed." - Updated the doc comment above `add_account` to describe the two-step commit-with-rollback behavior instead of the previous half-true "if this fails, nothing else happens" description. ## Tests Two new tests near the existing `add_account_*` tests in `crates/nunciod/src/grpc.rs`: - `add_account_rolls_back_keyring_secret_when_save_account_fails`: forces a genuine `save_account` failure by calling `DatabaseEngine::close()` (closing the connection pool) before making the `add_account` RPC call, then asserts `secrets.get_secret(...)` returns `Err` (secret rolled back, not orphaned). - `add_account_rollback_preserves_the_original_persistence_error`: same forced-failure setup, asserts the surfaced error message describes the persistence failure (`"failed to persist account"`) and is not the unrelated `set_secret` failure message. All existing `add_account_*` tests pass unmodified (happy-path behavior unchanged). ## Gate output ``` cargo fmt --all -- --check -> clean, no output cargo check-all -> Finished `dev` profile [unoptimized + debuginfo] target(s) in 4.29s (zero warnings) cargo test-all -> 400 passed; 0 failed; 0 ignored (across all workspace crates) ``` Notable subset (`nunciod` lib): `112 passed; 0 failed; 0 ignored`, including the two new rollback tests. ## Test plan - [x] `cargo fmt --all -- --check` - [x] `cargo check-all` (clippy `--all-targets --workspace -D warnings`) - [x] `cargo test-all` (400 passed, 0 failed, 0 ignored) - [x] New tests force a real `save_account` failure via `DatabaseEngine::close()`, not a simulated one - [x] Existing `add_account_*` tests pass unmodified

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#229