Detect concurrent config edits instead of silently losing one #75

Merged

claude merged 1 commit from issue30_config_edit_race_condition into main

2026-07-08 06:51:22 +01:00

claude commented

2026-07-08 06:40:46 +01:00

(Migrated from gitlab.keyop.co.uk)

Closes #30

Background

Confirmed the concern: two browser sessions with /config/edit open at once could both load the same on-disk config, edit independently, and save - handleAPIConfigSave had no check that the file hadn't changed since the session loaded it, so the second save would silently overwrite the first session's change with no warning (a classic "lost update").

Fix

Optimistic concurrency control, similar in spirit to an HTTP ETag/If-Match:

configEditResponse.Hash - a SHA-256 hex digest of the returned YAML - is now included in GET /api/config/raw and every POST /api/config/save response.
The client must echo whatever Hash it last received back as configEditRequest.BaseHash on its next save.
handleAPIConfigSave re-reads the file immediately before writing, computes its current hash, and rejects the save with 409 Conflict (Conflict: true, plus the file's actual current YAML/Config/Hash) if BaseHash doesn't match - including when BaseHash is empty/omitted, so a client that never called GET /api/config/raw first can't save at all.
Client-side, a 409 deliberately does not auto-resolve: state.hash is left stale (so a bare retry keeps conflicting rather than quietly winning the race on a second attempt), and an explicit "Reload latest version" button appears that discards the current session's in-progress edit and re-fetches the true on-disk state. Silently pulling in the other session's version instead would just move the "whose edit wins" problem somewhere less visible.
POST /api/config/validate has no hash check at all - it never writes, so there's nothing to conflict with; the raw/structured mode toggle and the explicit "Validate" button both go through it freely.

Testing

TestAPIConfigSaveConflictRejectsStaleBaseHash - the core regression test: a real successful save, then a second save using the now-stale original base_hash, asserting the second gets 409/Conflict: true and never touches what the first save wrote.
TestAPIConfigSaveMissingBaseHashConflicts - same rejection for an omitted base_hash.
Updated the two pre-existing save tests that predate this field to supply a correct base_hash (they were previously passing on save attempts that hadn't been asserted to actually reach the write step).
make lint/make test both clean.

Docs

Added a CLAUDE.md Design notes bullet and a short README.md mention of the new conflict-detection/reload behaviour.

Closes #30 ## Background Confirmed the concern: two browser sessions with `/config/edit` open at once could both load the same on-disk config, edit independently, and save - `handleAPIConfigSave` had no check that the file hadn't changed since the session loaded it, so the second save would silently overwrite the first session's change with no warning (a classic "lost update"). ## Fix Optimistic concurrency control, similar in spirit to an HTTP ETag/If-Match: - `configEditResponse.Hash` - a SHA-256 hex digest of the returned YAML - is now included in `GET /api/config/raw` and every `POST /api/config/save` response. - The client must echo whatever `Hash` it last received back as `configEditRequest.BaseHash` on its next save. - `handleAPIConfigSave` re-reads the file immediately before writing, computes its current hash, and rejects the save with `409 Conflict` (`Conflict: true`, plus the file's *actual* current `YAML`/`Config`/`Hash`) if `BaseHash` doesn't match - including when `BaseHash` is empty/omitted, so a client that never called `GET /api/config/raw` first can't save at all. - Client-side, a 409 deliberately does **not** auto-resolve: `state.hash` is left stale (so a bare retry keeps conflicting rather than quietly winning the race on a second attempt), and an explicit "Reload latest version" button appears that discards the current session's in-progress edit and re-fetches the true on-disk state. Silently pulling in the other session's version instead would just move the "whose edit wins" problem somewhere less visible. - `POST /api/config/validate` has no hash check at all - it never writes, so there's nothing to conflict with; the raw/structured mode toggle and the explicit "Validate" button both go through it freely. ## Testing - `TestAPIConfigSaveConflictRejectsStaleBaseHash` - the core regression test: a real successful save, then a second save using the now-stale original `base_hash`, asserting the second gets `409`/`Conflict: true` and never touches what the first save wrote. - `TestAPIConfigSaveMissingBaseHashConflicts` - same rejection for an omitted `base_hash`. - Updated the two pre-existing save tests that predate this field to supply a correct `base_hash` (they were previously passing on save attempts that hadn't been asserted to actually reach the write step). - `make lint`/`make test` both clean. ## Docs Added a `CLAUDE.md` Design notes bullet and a short `README.md` mention of the new conflict-detection/reload behaviour.

claude commented

2026-07-08 06:40:46 +01:00

(Migrated from gitlab.keyop.co.uk)

requested review from @danny