Make account store edits transactional

[btsouth]

Why

Every account edit read the store, changed it in memory, and wrote it back, with nothing holding those three steps together. Two overlapping edits could each write a copy of the file that predated the other, so one of them disappeared with no error and no sign anything had gone wrong. A Copilot device login finishing while the user edited accounts in Settings could do the same.

What

• DirectoryAccountStore::try_update and TokenAccountStore::try_update_provider hold the shared state write lock across the load, the mutation, and the atomic save.
• Route every command and CLI path through them: add/remove/switch token accounts, the directory account commands, ensure_ambient_registered, and the Copilot device login.
• DirectoryAccountStore::save now takes the lock too, matching what save_provider already did on the token store.

Notes for review

• The lock is not reentrant (it's a 10s cross-process file lock), so a closure must not reach another store. I checked every call site: all closures are pure local mutations, and try_update* calls save_unlocked rather than the locking save, so there's no nesting.
• Provider and network I/O stays outside the transaction. run_copilot_device_login completes the device flow first and only enters the closure to record the result, so the lock is never held across a request. Documented on both methods since it's the easy thing to get wrong later.
• Returning (data, T) lets callers keep the post-update snapshot without a second read.

Testing

cargo test -- account_dirs token_accounts cli::account — 21 pass, including a test per store where two threads add concurrently and both survive. cargo check on the tauri app is clean.

cargo clippy --all-targets -- -D warnings clean.

Note

Make account store edits transactional to prevent races between concurrent changes

• Adds try_update to DirectoryAccountStore and try_update_provider to TokenAccountStore, each performing a read-modify-write under a cross-process state lock and skipping writes when data is unchanged.
• All account mutations in the desktop Tauri commands and CLI (add, remove, switch) are replaced with these transactional APIs, preventing lost updates when concurrent edits occur.
• save and save_provider now also acquire the state lock internally, so direct saves are also serialized.
• PartialEq is derived on account data structs to enable no-op write detection.
• Behavioral Change: concurrent account operations that previously could silently overwrite each other now serialize under a lock; no-op transactions no longer write to disk.

^{Macroscope summarized eae7642.}

Summary by CodeRabbit

• Bug Fixes
  • Account changes now execute atomically against the latest saved state, preventing overlapping edits from overwriting one another.
  • Account additions, removals, switching, and device-login updates now preserve concurrent changes reliably.
  • Unchanged updates no longer create unnecessary storage files or writes.
• Documentation
  • Added a changelog entry describing the improved account update reliability.

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	ceiling	eae7642	Commit Preview URL Branch Preview URL	Aug 14 2026, 02:50 AM

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 53086ded-654b-4729-befe-ac5af4e3b64f

📥 Commits

Reviewing files that changed from the base of the PR and between 27acb82 and adbde49.

📒 Files selected for processing (7)

• CHANGELOG.md
• apps/desktop-tauri/src-tauri/src/commands/accounts.rs
• apps/desktop-tauri/src-tauri/src/commands/system.rs
• apps/desktop-tauri/src-tauri/src/commands/tokens.rs
• rust/src/cli/account.rs
• rust/src/core/account_dirs.rs
• rust/src/core/token_accounts.rs

---

📝 Walkthrough

Walkthrough

Account persistence now uses locked transactional updates. Directory and token account commands mutate the latest on-disk state, save only changed data, preserve storage errors, and test concurrent updates and no-op behavior.

Changes

Account persistence

Layer / File(s)	Summary
Directory account transactions rust/src/core/account_dirs.rs	Adds locked try_update operations, snapshot comparison, preserved storage errors, and tests for concurrency, no-op updates, and serialization failures.
Token account transactions rust/src/core/token_accounts.rs	Adds locked try_update_provider operations, provider snapshot comparison, preserved storage errors, and concurrency tests.
Account command integration apps/desktop-tauri/src-tauri/src/commands/accounts.rs, apps/desktop-tauri/src-tauri/src/commands/system.rs, apps/desktop-tauri/src-tauri/src/commands/tokens.rs, rust/src/cli/account.rs, CHANGELOG.md	Updates desktop and CLI account mutations and Copilot login persistence to use transactional store operations. Documents the fixed behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Mergeability Score: ⚪ Minimal · up to adbde

The account-edit changes are merge-ready after normal checks and review; no actionable merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant AccountCommand
  participant AccountStore
  participant StoreLock
  participant DiskState
  AccountCommand->>AccountStore: request transactional account update
  AccountStore->>StoreLock: acquire write lock
  StoreLock->>DiskState: load latest state
  AccountStore->>AccountStore: apply account mutation
  AccountStore->>DiskState: save changed state
  AccountStore-->>AccountCommand: return updated snapshot and result

Loading

Possibly related PRs

• tsouth89/ceiling#103: Introduces the account-store infrastructure extended by this PR.
• tsouth89/ceiling#116: Modifies the same ambient-account registration path.
• tsouth89/ceiling#287: Addresses concurrent credential persistence in another storage implementation.

Suggested reviewers: finesssee

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: account store edits now use transactional updates.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/sbs-736-account-transactions

---

_{Comment @coderabbitai help to get the list of available commands.}

[btsouth]

@macroscope-app review

[macroscopeapp]

Manual reviews triggered for commit 330a24b:

• Approvability Check
• Correctness Check

^{All prior checks · these links stay valid even if you push more commits.}

[macroscopeapp]

Review started. Results will be posted as check runs when complete.

[macroscopeapp]

Approvability

Verdict: Would Approve

This PR fixes a race condition by wrapping account store operations in a transactional lock. The changes are mechanical refactoring of existing logic, tests are included, and the author owns all modified files.

^{Macroscope would have approved this PR. Enable approvability here.}

[github-actions]

Automated review

New in this pass: 1 issue.

1. try_update collapses AccountStoreError::Json into Io

   rust/src/core/account_dirs.rs:398, rust/src/core/token_accounts.rs:645 · disposition: fix-if-quick · confidence: high · severity: medium · quick win

   try_update does self.load().map_err(io::Error::other) and self.save_unlocked(&data).map_err(io::Error::other) inside with_state_write_lock, then map_err(Into::into) to anyhow. A Json/serde failure from load or save_unlocked becomes an io::Error with kind Other, losing the AccountStoreError::Json variant. save() via with_store_lock preserves the variant by capturing op_err, but try_update does not, so the same failure is classified differently depending on entry point.

   Prompt for AI agents

   In rust/src/core/account_dirs.rs around line 398: Preserve the store error kind in try_update like with_store_lock does: capture the AccountStoreError in op_err and map the lock error to op_err.unwrap_or(Io), or make try_update return Result<_,AccountStoreError> and let callers convert. Add a test that try_update on an UnserializableIdentity returns AccountStoreError::Json not Io. Verify against the current code first; if no longer valid, skip with a brief reason. Keep the change minimal.

Still open from earlier passes:

• Directory add holds the global lock during identity file I/O — apps/desktop-tauri/src-tauri/src/commands/accounts.rs:317 · disposition: fix-if-quick · confidence: high · severity: medium
• First directory add is still two transactions and ignores ensure failure — apps/desktop-tauri/src-tauri/src/commands/accounts.rs:315 · disposition: fix-if-quick · confidence: medium · severity: medium

Resolved since the previous pass: 2.

_{For coding agents: fix BLOCK and FIX IF QUICK findings now; everything else is tracked or informational; never exceed one CodeRev fix round per PR.}

_{Advisory. Findings generated by grok-subscription and muse-spark-1.2-contributor, each filtered through a 3-vote refutation panel with the changed code in evidence.}