Ensure site storage cache rolls back on persistence failures#94
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async function persist(nextLevels) { | ||
| const normalized = normalizeLevels(nextLevels); | ||
| await storage.setSiteLevels(normalized); | ||
| cache = normalized; | ||
| return getCache(); | ||
| const previousCache = cache; | ||
| try { | ||
| await storage.setSiteLevels(normalized); | ||
| cache = normalized; | ||
| return getCache(); | ||
| } catch (error) { | ||
| cache = previousCache; | ||
| throw error; |
There was a problem hiding this comment.
Deferring cache update drops concurrent site changes
The new persist postpones cache assignment until setSiteLevels resolves and restores the previous object on failure. Because upsert/remove copy from cache to build the next payload, any second call invoked while the first write is still pending will read the stale pre‑update cache. If both promises resolve successfully, whichever finishes last will overwrite the stored levels and the in‑memory cache with only its own changes, silently discarding the earlier update (e.g. quickly editing two different hosts in the manager). The previous implementation updated cache optimistically so subsequent calls merged pending changes; this change reintroduces lost updates unless writes are serialized or merged explicitly.
Useful? React with 👍 / 👎.
Summary
Testing
Codex Task