M5: Roll back orphaned keyring secret when AddAccount fails to persist #197

New Issue

2026-07-28T14:04:40Z

KofTwentyTwo commented

2026-07-28 14:04:40 +00:00

(Migrated from github.com)

Context

Story for milestone M5. Read docs/STORY-WORKFLOW.md first — it defines the process, gates, and patterns; this issue defines only the specific work.

AccountsGrpcService::add_account (crates/nunciod/src/grpc.rs:308-340) writes the account's password to the OS keyring vault before persisting the account row to SQLite:

self.secrets
    .set_secret(&config.keyring_secret_key, &req.password)
    .map_err(|e| Status::internal(format!("failed to store credential in vault: {e}")))?;

self.db
    .save_account(&config)
    .await
    .map_err(|e| Status::internal(format!("failed to persist account: {e}")))?;

The comment directly above this (line 325-329) claims the ordering makes it safe: "Store the password credential in the OS keyring vault FIRST... If this fails, the account row below is deliberately never saved, so a failed credential write can never leave behind an account config with no retrievable password." That reasoning only covers the case where set_secret itself fails. It does not cover the actual bug: if set_secret succeeds and save_account then fails (disk full, DB locked, constraint violation, etc.), the ? on the save_account call returns early and the secret stays in the keyring forever, orphaned — there is no account row that references it, no way to discover it via ListAccounts, and no way to clean it up short of directly inspecting the OS keyring. nuncio_store::vault::SecretManager already exposes delete_secret(&self, key: &str) -> Result<(), VaultError> (crates/nuncio-store/src/vault.rs:201), which is not called anywhere in this rollback path today. Fixes #169.

Goal

Make AddAccount transactionally honest with respect to the keyring: if save_account fails after set_secret succeeded, roll back by deleting the just-written secret before returning the error, so a failed AddAccount call never leaves an orphaned credential behind. This must not fake success or swallow the original error — the caller must still see the real save_account failure; the rollback is a side effect on the failure path, not a substitute for surfacing it.

Files to touch / patterns to mirror

crates/nunciod/src/grpc.rs — AccountsGrpcService::add_account (line 308-340). After self.db.save_account(&config).await fails, call self.secrets.delete_secret(&config.keyring_secret_key) before returning the original error. Decide and document behavior if the rollback delete itself fails (e.g. keyring unreachable at that moment): the RPC must still surface the original save_account failure (never mask it with the rollback's error), but log or otherwise surface that a manual cleanup may be needed — do not silently drop the rollback failure without a trace, and do not let it change the returned Status from what save_account's failure would have produced.
crates/nuncio-store/src/vault.rs — SecretManager::delete_secret (line 201, delegates to the SecretVault trait's delete_secret, line 58-64) already exists and is what to call; no new vault API should be needed for the happy-rollback-path test. SecretManager::mock() wraps MockKeyring (line 68-112), a thread-safe in-memory HashMap-backed SecretVault impl with no fault-injection hooks today. The acceptance criteria only require proving the rollback succeeds (secret gone after a failed add_account); a rollback-failure scenario is optional stretch scope — only add a set_should_fail-style hook to MockKeyring (mirroring nuncio_mail::MockMessageSender::set_should_fail) if you choose to also test that path.

Implementation steps

Read crates/nunciod/src/grpc.rs:308-340 and crates/nuncio-store/src/vault.rs:1-170 (the SecretVault trait and its MockKeyring/OsKeyring implementations, plus the SecretManager facade) to confirm the exact current call shape and mock capabilities before changing anything.
Wrap the save_account call's failure path: on Err(e), attempt self.secrets.delete_secret(&config.keyring_secret_key), then return the original Status::internal(format!("failed to persist account: {e}")) regardless of whether the rollback delete succeeded. If the rollback delete fails, include that fact in the error message or a log line (e.g. append " (and failed to roll back the credential: {rollback_err})" to the same Status) so an operator has a chance to notice — but the RPC's error code/primary message must remain about the save_account failure, not the rollback failure.
Update the existing doc comment (line 325-329) to accurately describe the two-step commit-with-rollback behavior instead of the current claim that only covers half the failure space.
Add a test proving the fix: save_account is INSERT OR REPLACE (crates/nuncio-store/src/db.rs:622), so a duplicate id will NOT trigger a constraint failure — instead, force a genuine, deterministic save_account failure by calling DatabaseEngine::close() (crates/nuncio-store/src/db.rs:201-206, closes the connection pool) on the DatabaseEngine right before constructing the AccountsGrpcService/making the add_account call in the test, so the subsequent query genuinely fails (pool closed) rather than simulating a fake error. Assert that a subsequent self.secrets.get_secret(&config.keyring_secret_key) returns Err (the secret is gone) rather than Ok (orphaned). Place this alongside the existing add_account_* tests in crates/nunciod/src/grpc.rs (e.g. near add_account_persists_across_a_simulated_daemon_restart, line 1742, and add_account_password_never_touches_sqlite_or_the_list_response, line 1825).
Add a second test proving the original failure is still what the caller sees (i.e. the rollback does not swallow or replace the save_account error).

Acceptance criteria

When set_secret succeeds but the subsequent save_account fails, the just-written keyring secret is deleted before add_account returns.
The RPC still returns an error reflecting the original save_account failure (never a fabricated success, never an error that only mentions the rollback and hides the real cause).
A new test proves the secret is unrecoverable (get_secret returns Err) after a failed add_account that got past set_secret.
A new test proves the surfaced error is about the persistence failure, not the rollback.
No change to the happy-path behavior of add_account (existing tests, e.g. add_account_persists_across_a_simulated_daemon_restart, add_account_password_never_touches_sqlite_or_the_list_response, continue to pass unmodified).
cargo verify green (fmt + clippy -D warnings all-targets + test-all), 0 ignored.

Definition of done & process

Follow docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in .rs/.proto); mock-only tests. Branch story/m5-s4-orphaned-keyring-cleanup off dev; PR to dev with the gate output pasted as evidence; the PO reviews before the issue is closed.

Dependencies / notes

Not a [PROTO] story — no nuncio.proto changes. Can run in parallel with any proto-touching M5 story. If M5-S1 (UpdateAccount) lands first and also writes to the keyring on an update path, apply the same rollback discipline there too as a quick follow-up in that story rather than duplicating this issue — check add_account's post-M5-S1 sibling before starting, in case it already absorbed this fix.

No relevant prior-art branch for this specific bug. Rough size: M.

Fixes #169.

## Context Story for milestone M5. **Read [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md) first** — it defines the process, gates, and patterns; this issue defines only the specific work. `AccountsGrpcService::add_account` (`crates/nunciod/src/grpc.rs:308-340`) writes the account's password to the OS keyring vault **before** persisting the account row to SQLite: ```rust self.secrets .set_secret(&config.keyring_secret_key, &req.password) .map_err(|e| Status::internal(format!("failed to store credential in vault: {e}")))?; self.db .save_account(&config) .await .map_err(|e| Status::internal(format!("failed to persist account: {e}")))?; ``` The comment directly above this (line 325-329) claims the ordering makes it safe: *"Store the password credential in the OS keyring vault FIRST... If this fails, the account row below is deliberately never saved, so a failed credential write can never leave behind an account config with no retrievable password."* That reasoning only covers the case where `set_secret` itself fails. It does not cover the actual bug: if `set_secret` **succeeds** and `save_account` then fails (disk full, DB locked, constraint violation, etc.), the `?` on the `save_account` call returns early and the secret **stays in the keyring forever**, orphaned — there is no account row that references it, no way to discover it via `ListAccounts`, and no way to clean it up short of directly inspecting the OS keyring. `nuncio_store::vault::SecretManager` already exposes `delete_secret(&self, key: &str) -> Result<(), VaultError>` (`crates/nuncio-store/src/vault.rs:201`), which is not called anywhere in this rollback path today. Fixes #169. ## Goal Make `AddAccount` transactionally honest with respect to the keyring: if `save_account` fails after `set_secret` succeeded, roll back by deleting the just-written secret before returning the error, so a failed `AddAccount` call never leaves an orphaned credential behind. This must not fake success or swallow the original error — the caller must still see the real `save_account` failure; the rollback is a side effect on the failure path, not a substitute for surfacing it. ## Files to touch / patterns to mirror - `crates/nunciod/src/grpc.rs` — `AccountsGrpcService::add_account` (line 308-340). After `self.db.save_account(&config).await` fails, call `self.secrets.delete_secret(&config.keyring_secret_key)` before returning the original error. Decide and document behavior if the rollback delete *itself* fails (e.g. keyring unreachable at that moment): the RPC must still surface the original `save_account` failure (never mask it with the rollback's error), but log or otherwise surface that a manual cleanup may be needed — do not silently drop the rollback failure without a trace, and do not let it change the returned `Status` from what `save_account`'s failure would have produced. - `crates/nuncio-store/src/vault.rs` — `SecretManager::delete_secret` (line 201, delegates to the `SecretVault` trait's `delete_secret`, line 58-64) already exists and is what to call; no new vault API should be needed for the happy-rollback-path test. `SecretManager::mock()` wraps `MockKeyring` (line 68-112), a thread-safe in-memory `HashMap`-backed `SecretVault` impl with no fault-injection hooks today. The acceptance criteria only require proving the rollback *succeeds* (secret gone after a failed `add_account`); a rollback-*failure* scenario is optional stretch scope — only add a `set_should_fail`-style hook to `MockKeyring` (mirroring `nuncio_mail::MockMessageSender::set_should_fail`) if you choose to also test that path. ## Implementation steps 1. Read `crates/nunciod/src/grpc.rs:308-340` and `crates/nuncio-store/src/vault.rs:1-170` (the `SecretVault` trait and its `MockKeyring`/`OsKeyring` implementations, plus the `SecretManager` facade) to confirm the exact current call shape and mock capabilities before changing anything. 2. Wrap the `save_account` call's failure path: on `Err(e)`, attempt `self.secrets.delete_secret(&config.keyring_secret_key)`, then return the original `Status::internal(format!("failed to persist account: {e}"))` regardless of whether the rollback delete succeeded. If the rollback delete fails, include that fact in the error message or a log line (e.g. append `" (and failed to roll back the credential: {rollback_err})"` to the same `Status`) so an operator has a chance to notice — but the RPC's error code/primary message must remain about the `save_account` failure, not the rollback failure. 3. Update the existing doc comment (line 325-329) to accurately describe the two-step commit-with-rollback behavior instead of the current claim that only covers half the failure space. 4. Add a test proving the fix: `save_account` is `INSERT OR REPLACE` (`crates/nuncio-store/src/db.rs:622`), so a duplicate id will NOT trigger a constraint failure — instead, force a genuine, deterministic `save_account` failure by calling `DatabaseEngine::close()` (`crates/nuncio-store/src/db.rs:201-206`, closes the connection pool) on the `DatabaseEngine` right before constructing the `AccountsGrpcService`/making the `add_account` call in the test, so the subsequent query genuinely fails (pool closed) rather than simulating a fake error. Assert that a subsequent `self.secrets.get_secret(&config.keyring_secret_key)` returns `Err` (the secret is gone) rather than `Ok` (orphaned). Place this alongside the existing `add_account_*` tests in `crates/nunciod/src/grpc.rs` (e.g. near `add_account_persists_across_a_simulated_daemon_restart`, line 1742, and `add_account_password_never_touches_sqlite_or_the_list_response`, line 1825). 5. Add a second test proving the original failure is still what the caller sees (i.e. the rollback does not swallow or replace the `save_account` error). ## Acceptance criteria - [ ] When `set_secret` succeeds but the subsequent `save_account` fails, the just-written keyring secret is deleted before `add_account` returns. - [ ] The RPC still returns an error reflecting the original `save_account` failure (never a fabricated success, never an error that only mentions the rollback and hides the real cause). - [ ] A new test proves the secret is unrecoverable (`get_secret` returns `Err`) after a failed `add_account` that got past `set_secret`. - [ ] A new test proves the surfaced error is about the persistence failure, not the rollback. - [ ] No change to the happy-path behavior of `add_account` (existing tests, e.g. `add_account_persists_across_a_simulated_daemon_restart`, `add_account_password_never_touches_sqlite_or_the_list_response`, continue to pass unmodified). - [ ] `cargo verify` green (fmt + clippy `-D warnings` all-targets + test-all), 0 ignored. ## Definition of done & process Follow [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md): no fabricated data; comment convention (intent-only, no story/GH/phase refs in `.rs`/`.proto`); mock-only tests. Branch `story/m5-s4-orphaned-keyring-cleanup` off `dev`; PR to `dev` with the gate output pasted as evidence; the PO reviews before the issue is closed. ## Dependencies / notes Not a `[PROTO]` story — no `nuncio.proto` changes. Can run in parallel with any proto-touching M5 story. If M5-S1 (`UpdateAccount`) lands first and also writes to the keyring on an update path, apply the same rollback discipline there too as a quick follow-up in that story rather than duplicating this issue — check `add_account`'s post-M5-S1 sibling before starting, in case it already absorbed this fix. No relevant prior-art branch for this specific bug. Rough size: M. Fixes #169.

KofTwentyTwo commented

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

(Migrated from github.com)

Delivered via PR #229 (squash-merged to dev). add_account now rolls back the just-written keyring secret if save_account fails after set_secret succeeded, restoring the 'no credential without a matching account' invariant. Original persistence error stays the primary/visible cause; a rollback-delete failure is appended + warn-logged, never masks the real error. Two tests (genuine pool-closed failure; error-preservation). Fixes #169. Establishes the rollback discipline M5-S1 update_account will mirror.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#197