fix(accounts): accounts.json is safe across gclo processes; an unreadable file is not a corrupt one #156

2026-11-10T21:24:58Z

james.maes commented

2026-11-10 21:24:58 +00:00

What

AccountsStore is safe across processes. Every change (Save, Delete, RecordSyncResult) takes a lock that every gclo process takes (accounts.json.lock, opened for one process alone), reads the file again, applies its one change by account id to what the file holds at that moment, and writes that.
An unreadable file is not a corrupt file. Only content that is not a valid list of accounts is moved aside. A file that is in use is tried again for a short while; one that stays unreadable is left exactly where it is: at construction the store starts empty and logs why, and a change fails without writing anything.
The swap into place is retried when something outside gclo holds the live file for a moment.
AppSettings: written beside the file and moved into place; a failed load or save is logged; Save returns whether it succeeded; the misleading "0 means absent" branch is gone.

Why

Closes #155

With the desktop app open, an account added from the CLI was erased by the app's next write, and its token was left in the vault with nothing pointing at it. The reverse happened during a long gclo sync --account.

How it was tested

New AccountsStoreSharedFileTests: two stores over one folder stand for two processes. An account added, edited, or deleted through one survives (or stays deleted after) the next change through the other; tokens stay with their accounts; a name taken meanwhile is refused; 50 concurrent saves through two stores are all kept; a held lock is waited for, and a lock or file that stays held fails the change without writing and without moving the file aside; a damaged file found by a change is still moved aside; the swap is retried.
AppSettingsTests: success result and no leftover temporary file; a failed save reports, logs, and leaves the previous settings; a failed load logs; a duration of 0 is brought into range.
The three test classes that set GCLO_DATA_DIR for the process now share one xunit collection (a latent source of flaky runs).
Local gates: locked zero-warning x64 build, 817 engine and 164 CLI tests, 100% line coverage on the three gated assemblies (branch: Engine 94.44%, ViewModels 93.99%, CLI 98.20%), dotnet format --verify-no-changes.

Notes for the reviewer

Reads (GetAll, FindByName) still answer from what the store last read (construction or its latest change). The app therefore lists an account added from the CLI only after a restart; it no longer destroys it. docs/CLI.md says so.
The lock file is left in place, empty, beside accounts.json; it is not deleted on close on purpose (a file in delete-pending state answers the next opener with "access denied").
Default patience for a busy lock or file is 5 seconds; these calls are synchronous and can be made from the UI thread.
The desktop project change is two call sites passing the activity log to AppSettings.Load / Save.

Checklist

Commits are signed and signed off (git commit -S -s)
Zero build warnings; format and lint are clean
Tests added or updated; the 100% coverage gate still passes
Docs updated for user-visible changes; threat model updated if the attack surface changed (no change to the attack surface)
No secrets, credentials, or personal data
New dependencies are justified below, or none were added (none)
Substantial AI assistance, if any, is noted here: written with Claude Code, reviewed by an independent read-only agent before merge

🤖 Generated with Claude Code

## What - **`AccountsStore` is safe across processes.** Every change (`Save`, `Delete`, `RecordSyncResult`) takes a lock that every gclo process takes (`accounts.json.lock`, opened for one process alone), reads the file again, applies its one change by account id to what the file holds at that moment, and writes that. - **An unreadable file is not a corrupt file.** Only content that is not a valid list of accounts is moved aside. A file that is in use is tried again for a short while; one that stays unreadable is left exactly where it is: at construction the store starts empty and logs why, and a change fails without writing anything. - **The swap into place is retried** when something outside gclo holds the live file for a moment. - **`AppSettings`**: written beside the file and moved into place; a failed load or save is logged; `Save` returns whether it succeeded; the misleading "0 means absent" branch is gone. ## Why Closes #155 With the desktop app open, an account added from the CLI was erased by the app's next write, and its token was left in the vault with nothing pointing at it. The reverse happened during a long `gclo sync --account`. ## How it was tested - New `AccountsStoreSharedFileTests`: two stores over one folder stand for two processes. An account added, edited, or deleted through one survives (or stays deleted after) the next change through the other; tokens stay with their accounts; a name taken meanwhile is refused; 50 concurrent saves through two stores are all kept; a held lock is waited for, and a lock or file that stays held fails the change without writing and without moving the file aside; a damaged file found by a change is still moved aside; the swap is retried. - `AppSettingsTests`: success result and no leftover temporary file; a failed save reports, logs, and leaves the previous settings; a failed load logs; a duration of 0 is brought into range. - The three test classes that set `GCLO_DATA_DIR` for the process now share one xunit collection (a latent source of flaky runs). - Local gates: locked zero-warning x64 build, 817 engine and 164 CLI tests, 100% line coverage on the three gated assemblies (branch: Engine 94.44%, ViewModels 93.99%, CLI 98.20%), `dotnet format --verify-no-changes`. ## Notes for the reviewer - Reads (`GetAll`, `FindByName`) still answer from what the store last read (construction or its latest change). The app therefore lists an account added from the CLI only after a restart; it no longer destroys it. `docs/CLI.md` says so. - The lock file is left in place, empty, beside `accounts.json`; it is not deleted on close on purpose (a file in delete-pending state answers the next opener with "access denied"). - Default patience for a busy lock or file is 5 seconds; these calls are synchronous and can be made from the UI thread. - The desktop project change is two call sites passing the activity log to `AppSettings.Load` / `Save`. ## Checklist - [x] Commits are signed and signed off (`git commit -S -s`) - [x] Zero build warnings; format and lint are clean - [x] Tests added or updated; the 100% coverage gate still passes - [x] Docs updated for user-visible changes; threat model updated if the attack surface changed (no change to the attack surface) - [x] No secrets, credentials, or personal data - [x] New dependencies are justified below, or none were added (none) - [x] Substantial AI assistance, if any, is noted here: written with Claude Code, reviewed by an independent read-only agent before merge 🤖 Generated with [Claude Code](https://claude.com/claude-code)

james.maes added 2 commits 2026-11-10 22:06:06 +00:00

fix(accounts): accounts.json is safe across gclo processes; an unreadable file is not a corrupt one c7880167ab

The store read accounts.json once and wrote that snapshot back on every
change. With the desktop app open, an account added from the CLI was
erased by the app's next write (recording a sync result was enough), and
its token stayed in the vault with nothing pointing at it. A long
`gclo sync --account` did the same to the app, and two app instances to
each other.

- Every change now takes a lock that every gclo process takes
  (accounts.json.lock, opened for one process alone), reads the file
  again, applies its one change by account id to what the file holds at
  that moment, and writes that. A sync result recorded for an account
  another process deleted is ignored, not written back.
- A file that cannot be read is no longer taken for a corrupt one. Only
  content that is not a valid list of accounts is moved aside. A file in
  use is tried again for a short while; one that stays unreadable is left
  where it is: the store starts empty and logs why, and a change fails
  without writing anything.
- The swap of the new file into place is tried again when something
  outside gclo holds the live file for a moment.

settings.json is written beside the file and moved into place, so a crash
mid-write leaves the previous settings. A failed load or save is logged,
and Save says whether it succeeded. The "0 means absent" special case for
the splash duration described something that does not happen (an absent
field keeps the default); the value is simply brought into range.

Tests: the three classes that set GCLO_DATA_DIR for the process now share
one collection, so they never run beside each other.

docs/CLI.md named the wrong folder for accounts.json; corrected.

Closes #155

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

fix(accounts): address the independent review of the shared accounts file 17955a3c7b

One rename puts the new file in place, so it is never absent; start-up
reads without the lock. An edit of an account another process removed is
refused; a newer recorded sync result is kept; a change that finds the
file gone or damaged is applied to what the store last read. Busy means a
sharing violation; a read-only file fails at once; a lock held for the
whole wait is an AccountsFileBusyException (CLI exit 4). Settings saves
retry briefly, use a temporary name of their own and leave nothing behind.

Refs #155

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

james.maes force-pushed fix/155-accounts-store from 493ef39d49 to 17955a3c7b

2026-11-10 22:06:06 +00:00

Compare

james.maes commented

2026-11-10 22:06:06 +00:00

Independent review (read-only agent): merge after fixes

The reviewer ran real processes against one folder (up to six at once): no account was lost and the JSON was never invalid. It found six medium and three low issues around the edges. What was done, in the second commit (the branch was also rebased onto main):

#	Finding	Resolution
M1	A store could still start silently empty while another process saved: `File.Replace` leaves the name absent for a moment (13 of 4,800 start-ups).	The new file is put in place with one rename; the backup is written by copy beforehand. Start-up reads without the lock.
M2	Editing an account that another process had removed brought it back without its token.	Such an edit is refused with a message; adding the account again is a separate, explicit step.
M3	An edit from an older copy overwrote the sync result the other process had recorded; the docs claimed more than the code kept.	The newer sync result is kept. Docs now say: added/removed accounts and sync results are kept, the later edit of the same account wins.
M4	A change that found the file damaged or missing started over from nothing, orphaning tokens.	The change is applied to what the store last read, which writes the accounts back (as `main` did); logged.
M5	`AppSettings.Save` failed while anything had the file open (114 of 1,000), collided between processes, and left a temporary file.	Short retry, a temporary name of its own, nothing left behind; the dialog's log line says when settings were not written.
M6	Waits of 5 to 15 s on the UI thread for conditions that never pass (read-only file), and every I/O error relabelled "in use by another gclo process".	"Busy" is a sharing or lock violation by its code; a read-only file fails at once; the file-level patience is a fifth of the lock's.
L1	The CLI reported a busy lock as an internal bug (exit 70).	`AccountsFileBusyException` → exit 4, documented.
L2	A lock file that could not be opened hid readable accounts.	Start-up does not need the lock.
L3	Threat model and class remarks.	Updated.

Re-run of the reviewer's own stress harness against the fixed build (real processes, one folder):

Four processes × 200 saves, plus a process constructing 1,500 stores and one polling for the file: 800 of 800 saves on disk, 0 failures, the file absent in 0 of 269,759 polls, no start-up saw fewer accounts than existed, slowest save 1.8 s (was 4.1 s).
Two processes × 500 settings saves: 1,000 of 1,000 succeeded, no leftover files (about half failed before).

Gates after the fixes: 911 engine and 164 CLI tests, 100% line coverage on the three gated assemblies, zero warnings, format clean.

Left for the shell work: store changes still run on the UI thread, the app lists a CLI-added account only after a restart, and a failed settings save is logged but not shown.

## Independent review (read-only agent): merge after fixes The reviewer ran real processes against one folder (up to six at once): no account was lost and the JSON was never invalid. It found six medium and three low issues around the edges. What was done, in the second commit (the branch was also rebased onto `main`): | # | Finding | Resolution | | --- | --- | --- | | M1 | A store could still start silently empty while another process saved: `File.Replace` leaves the name absent for a moment (13 of 4,800 start-ups). | The new file is put in place with one rename; the backup is written by copy beforehand. Start-up reads without the lock. | | M2 | Editing an account that another process had removed brought it back without its token. | Such an edit is refused with a message; adding the account again is a separate, explicit step. | | M3 | An edit from an older copy overwrote the sync result the other process had recorded; the docs claimed more than the code kept. | The newer sync result is kept. Docs now say: added/removed accounts and sync results are kept, the later edit of the same account wins. | | M4 | A change that found the file damaged or missing started over from nothing, orphaning tokens. | The change is applied to what the store last read, which writes the accounts back (as `main` did); logged. | | M5 | `AppSettings.Save` failed while anything had the file open (114 of 1,000), collided between processes, and left a temporary file. | Short retry, a temporary name of its own, nothing left behind; the dialog's log line says when settings were not written. | | M6 | Waits of 5 to 15 s on the UI thread for conditions that never pass (read-only file), and every I/O error relabelled "in use by another gclo process". | "Busy" is a sharing or lock violation by its code; a read-only file fails at once; the file-level patience is a fifth of the lock's. | | L1 | The CLI reported a busy lock as an internal bug (exit 70). | `AccountsFileBusyException` → exit 4, documented. | | L2 | A lock file that could not be opened hid readable accounts. | Start-up does not need the lock. | | L3 | Threat model and class remarks. | Updated. | Re-run of the reviewer's own stress harness against the fixed build (real processes, one folder): - Four processes × 200 saves, plus a process constructing 1,500 stores and one polling for the file: 800 of 800 saves on disk, 0 failures, the file absent in 0 of 269,759 polls, no start-up saw fewer accounts than existed, slowest save 1.8 s (was 4.1 s). - Two processes × 500 settings saves: 1,000 of 1,000 succeeded, no leftover files (about half failed before). Gates after the fixes: 911 engine and 164 CLI tests, 100% line coverage on the three gated assemblies, zero warnings, format clean. Left for the shell work: store changes still run on the UI thread, the app lists a CLI-added account only after a restart, and a failed settings save is logged but not shown.

james.maes merged commit 8c1587c0e4 into main

2026-11-10 22:06:08 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/gclo#156