fix: reduce excessive file writes from account state updates#459
fix: reduce excessive file writes from account state updates#459mrm007 wants to merge 1 commit intoNoeFabris:mainfrom
Conversation
- Remove unconditional requestSaveToDisk() on every API request (line 1702) - Add snapshot deduplication to skip no-op writes - Increase debounce from 1s to 5s for rate-limit storm resilience - Move save trigger to after markAccountUsed() where lastUsed is persisted Fixes #436 Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c6dae-36aa-7524-849b-d8c515ba0cd1
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)src/plugin/accounts.test.ts (1)
src/plugin/accounts.ts (1)
🔇 Additional comments (4)
WalkthroughThe changes implement debouncing and optimistic saving to reduce excessive disk writes during account state mutations. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Key observationsCore optimization: The Timing change: The delay before triggering disk write increases from 1,000 ms to 5,500 ms, allowing more state mutations to coalesce into a single write operation within the debounce window. Storage refactoring: The new New public APIs: Eight new public methods are added to Test updates: Debounce window in tests increases from 1,500 ms to 5,500 ms. A new test validates that 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR reduces excessive Key changes and observations:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Plugin as plugin.ts
participant AM as AccountManager
participant FS as FileSystem
Note over Plugin, FS: Before this PR (excessive writes)
Plugin->>AM: markToastShown()
Plugin->>AM: requestSaveToDisk() ❌ (every request)
AM->>FS: saveAccounts() after 1s debounce
Note over Plugin, FS: After this PR (optimized writes)
Plugin->>AM: markToastShown()
Note over Plugin: No save triggered here
Plugin->>AM: markAccountUsed(index)
Plugin->>AM: requestSaveToDisk() ✅ (only on success)
AM->>AM: executeSave() after 5s debounce
AM->>AM: snapshot = JSON.stringify(buildStorageState())
alt snapshot !== lastSavedSnapshot
AM->>AM: saveToDisk() → buildStorageState() again
AM->>FS: saveAccounts(state)
AM->>AM: lastSavedSnapshot = snapshot
else snapshot == lastSavedSnapshot
AM-->>AM: skip write (deduplication)
end
Note over AM, FS: flushSaveToDisk() resolvers notified in finally block
Last reviewed commit: 4153865 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const snapshot = JSON.stringify(this.buildStorageState()); | ||
| if (snapshot !== this.lastSavedSnapshot) { | ||
| await this.saveToDisk(); | ||
| this.lastSavedSnapshot = snapshot; |
There was a problem hiding this comment.
Stale snapshot stored as last-saved state
lastSavedSnapshot is set to the snapshot taken before saveToDisk() is awaited, but saveToDisk() internally calls buildStorageState() again. If any state mutation happens during the await saveAccounts(...) call (e.g., a rate-limit update arriving while the lockfile/rename cycle is in progress), the snapshot stored in lastSavedSnapshot will diverge from what was actually written to disk. The next executeSave call will then compare against a snapshot that doesn't match the file on disk, potentially skipping a write that should have happened.
A safer approach is to capture the snapshot once, pass it to saveToDisk, and only update lastSavedSnapshot after a confirmed successful write of that exact snapshot:
const snapshot = JSON.stringify(this.buildStorageState());
if (snapshot !== this.lastSavedSnapshot) {
await saveAccounts(JSON.parse(snapshot) as AccountStorageV4);
this.lastSavedSnapshot = snapshot;
}
This ensures the snapshot stored is exactly what was written, not a concurrent re-read.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/accounts.ts
Line: 1040:1043
Comment:
**Stale snapshot stored as last-saved state**
`lastSavedSnapshot` is set to the snapshot taken *before* `saveToDisk()` is awaited, but `saveToDisk()` internally calls `buildStorageState()` again. If any state mutation happens during the `await saveAccounts(...)` call (e.g., a rate-limit update arriving while the lockfile/rename cycle is in progress), the snapshot stored in `lastSavedSnapshot` will diverge from what was actually written to disk. The next `executeSave` call will then compare against a snapshot that doesn't match the file on disk, potentially skipping a write that should have happened.
A safer approach is to capture the snapshot once, pass it to `saveToDisk`, and only update `lastSavedSnapshot` after a confirmed successful write of that exact snapshot:
```
const snapshot = JSON.stringify(this.buildStorageState());
if (snapshot !== this.lastSavedSnapshot) {
await saveAccounts(JSON.parse(snapshot) as AccountStorageV4);
this.lastSavedSnapshot = snapshot;
}
```
This ensures the snapshot stored is exactly what was written, not a concurrent re-read.
How can I resolve this? If you propose a fix, please make it concise.| getHealthTracker().recordSuccess(account.index); | ||
| accountManager.markAccountUsed(account.index); | ||
|
|
||
| accountManager.requestSaveToDisk() |
There was a problem hiding this comment.
Missing semicolon
The call to requestSaveToDisk() is missing a trailing semicolon, inconsistent with the rest of the codebase style.
| accountManager.requestSaveToDisk() | |
| accountManager.requestSaveToDisk(); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 2331:2331
Comment:
**Missing semicolon**
The call to `requestSaveToDisk()` is missing a trailing semicolon, inconsistent with the rest of the codebase style.
```suggestion
accountManager.requestSaveToDisk();
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| it("skips write when account state has not changed", async () => { | ||
| vi.useFakeTimers(); | ||
|
|
||
| const stored: AccountStorageV4 = { | ||
| version: 4, | ||
| accounts: [ | ||
| { refreshToken: "r1", projectId: "p1", addedAt: 1, lastUsed: 0 }, | ||
| ], | ||
| activeIndex: 0, | ||
| }; | ||
|
|
||
| const manager = new AccountManager(undefined, stored); | ||
| const saveSpy = vi.spyOn(manager, "saveToDisk").mockResolvedValue(); | ||
|
|
||
| // First save — should write | ||
| manager.requestSaveToDisk(); | ||
| await vi.advanceTimersByTimeAsync(5500); | ||
| expect(saveSpy).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Second save with no state change — should skip | ||
| manager.requestSaveToDisk(); | ||
| await vi.advanceTimersByTimeAsync(5500); | ||
| expect(saveSpy).toHaveBeenCalledTimes(1); | ||
|
|
||
| saveSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
Deduplication test mocks saveToDisk, bypassing the real snapshot-write path
The new test mocks saveToDisk via vi.spyOn(...).mockResolvedValue(). This means executeSave takes a snapshot, sees a diff from null, calls the mock (which does nothing), then stores the snapshot as lastSavedSnapshot. The second call correctly sees equality and skips.
However, this doesn't test the case where the real saveToDisk (which calls buildStorageState() again independently) could write a state that differs from the comparison snapshot. Consider adding a test that exercises state mutation during the debounce window to confirm the snapshot reflects what's actually on disk.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/accounts.test.ts
Line: 1149:1174
Comment:
**Deduplication test mocks saveToDisk, bypassing the real snapshot-write path**
The new test mocks `saveToDisk` via `vi.spyOn(...).mockResolvedValue()`. This means `executeSave` takes a snapshot, sees a diff from `null`, calls the mock (which does nothing), then stores the snapshot as `lastSavedSnapshot`. The second call correctly sees equality and skips.
However, this doesn't test the case where the *real* `saveToDisk` (which calls `buildStorageState()` again independently) could write a state that differs from the comparison snapshot. Consider adding a test that exercises state mutation *during* the debounce window to confirm the snapshot reflects what's actually on disk.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.1 participant
Fixes #436
Problem
saveAccounts()writesantigravity-accounts.jsonfar too frequently during normal operation. Each write acquires aproper-lockfilelock (mkdir/rmdir cycle), writes a temp file, and renames it — producing 4+ FS events per save. During active use with rate limit cycling, this fires dozens of times per minute, thrashing the config directory and overwhelming file watchers.Changes
Remove unconditional
requestSaveToDisk()on every API request — the call at line 1702 fired on every request iteration even when no persistent state changed (only in-memory toast state was updated). Moved the save trigger to aftermarkAccountUsed()on success, wherelastUsedis actually persisted.Add snapshot deduplication —
executeSave()now compares a JSON snapshot of the current storage state against the last saved snapshot, skipping the write entirely when nothing changed. Uses the samebuildStorageState()method assaveToDisk()to avoid duplication.Increase debounce from 1s → 5s — rate limit reset times and quota cache are ephemeral state; a 5s window dramatically reduces writes during rate-limit storms while still being timely enough for persistence.