Skip to content

fix: serialize concurrent runtime cache writes#373

Closed
ndycode wants to merge 1 commit intomainfrom
fix/concurrent-file-writers
Closed

fix: serialize concurrent runtime cache writes#373
ndycode wants to merge 1 commit intomainfrom
fix/concurrent-file-writers

Conversation

@ndycode
Copy link
Copy Markdown
Owner

@ndycode ndycode commented Apr 6, 2026

Problem

Multiple runtime cache writers could overlap within the same process.

  • quota-cache already used temp-file renames, but concurrent saves still raced logically.
  • auto-update-checker wrote directly to the final cache file, so concurrent writes had a higher corruption risk.

Fix

Serialize in-process cache writes and make update-check cache writes atomic.

Changes

  • lib/quota-cache.ts
    • added an in-process write queue for saveQuotaCache()
    • retained temp-file + rename behavior, but now writes are serialized per process
  • lib/auto-update-checker.ts
    • made cache saves queued and atomic via temp-file rename
    • kept retry behavior for transient EBUSY / EPERM failures
    • added best-effort temp cleanup on failed writes
  • test/auto-update-checker.test.ts
    • updated expectations for temp-file + rename persistence
    • added concurrency coverage for concurrent cache writes

Validation

npx vitest run test/quota-cache.test.ts test/auto-update-checker.test.ts
Test Files  2 passed (2)
Tests      47 passed (47)

npx vitest run
Test Files  222 passed (222)
Tests      3314 passed (3314)

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

serializes in-process cache writes for both quota-cache and auto-update-checker using a module-level promise queue with temp-file + rename for atomicity. the quota-cache refactor is clean. auto-update-checker has two minor gaps: clearUpdateCache bypasses the new queue (a queued rename can silently undo a clear), and the new code uses tabs while the rest of the file uses spaces.

Confidence Score: 4/5

safe to merge after addressing the clearUpdateCache queue bypass and fixing mixed indentation

the core serialization logic is correct and well-tested; the P2 gap in clearUpdateCache is a real behavioral issue (a stale rename can silently undo a clear) but unlikely to be triggered in normal use — still worth fixing before merge per the PR's explicit serialization goal

lib/auto-update-checker.ts — clearUpdateCache queue bypass and mixed tabs/spaces indentation

Important Files Changed

Filename Overview
lib/auto-update-checker.ts makes saveCache async with temp-file rename and write queue; clearUpdateCache still bypasses the queue and mixed tabs/spaces indentation introduced
lib/quota-cache.ts cleanly wraps writeTask in a module-level write queue; payload captured correctly per closure; async retries preserved
test/auto-update-checker.test.ts mocks updated for renameSync/unlinkSync; new concurrency test verifies two sequential renames through separate temp paths

Sequence Diagram

sequenceDiagram
    participant C1 as Caller 1
    participant C2 as Caller 2
    participant Q as writeQueue
    participant FS as Filesystem

    C1->>Q: saveCache() → enqueue writeTask1
    C2->>Q: saveCache() → enqueue writeTask2
    Q->>FS: writeFileSync(tempPath1)
    Q->>FS: renameSync(tempPath1 → CACHE_FILE)
    Q->>FS: writeFileSync(tempPath2)
    Q->>FS: renameSync(tempPath2 → CACHE_FILE)
    note over FS: writes serialized,\neach atomic via rename

    note over C1,FS: clearUpdateCache() bypasses Q
    C1->>FS: writeFileSync(CACHE_FILE, '{}')
    Q-->>FS: renameSync(tempPath → CACHE_FILE) races ↑
Loading

Comments Outside Diff (1)

  1. lib/auto-update-checker.ts, line 276-284 (link)

    P2 clearUpdateCache bypasses the write queue

    clearUpdateCache writes directly to CACHE_FILE without going through updateCacheWriteQueue. if a queued saveCache task has already written its temp file but hasn't yet called renameSync, clearUpdateCache writes {} to the final path — then the pending renameSync immediately overwrites it with stale data, silently undoing the clear. routing through the queue would fix this:

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: lib/auto-update-checker.ts
    Line: 276-284
    
    Comment:
    **`clearUpdateCache` bypasses the write queue**
    
    `clearUpdateCache` writes directly to `CACHE_FILE` without going through `updateCacheWriteQueue`. if a queued `saveCache` task has already written its temp file but hasn't yet called `renameSync`, `clearUpdateCache` writes `{}` to the final path — then the pending `renameSync` immediately overwrites it with stale data, silently undoing the clear. routing through the queue would fix this:
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Codex

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/auto-update-checker.ts
Line: 276-284

Comment:
**`clearUpdateCache` bypasses the write queue**

`clearUpdateCache` writes directly to `CACHE_FILE` without going through `updateCacheWriteQueue`. if a queued `saveCache` task has already written its temp file but hasn't yet called `renameSync`, `clearUpdateCache` writes `{}` to the final path — then the pending `renameSync` immediately overwrites it with stale data, silently undoing the clear. routing through the queue would fix this:

```suggestion
export function clearUpdateCache(): void {
  const current = updateCacheWriteQueue.catch(() => undefined);
  updateCacheWriteQueue = current.then(() => {
    try {
      if (existsSync(CACHE_FILE)) {
        writeFileSync(CACHE_FILE, "{}", "utf8");
      }
    } catch {
      // Ignore errors
    }
  });
}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/auto-update-checker.ts
Line: 60-107

Comment:
**mixed indentation (tabs vs spaces)**

the new `saveCache` body and the `await saveCache(...)` block at lines 238–242 use tabs, while the rest of the file uses 2-space indentation. this breaks the file's consistent style and will show up as noisy diffs in editors configured for spaces.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/auto-update-checker.ts
Line: 84

Comment:
**`sleepSync` blocks the event loop on windows**

`sleepSync` uses `Atomics.wait` on the main thread — on windows where `EBUSY`/`EPERM` is common, retries can block for up to `15 + 30 + 60 = 105 ms` per queued save. all subsequent queued saves stall during that window. `quota-cache.ts` already uses async `sleep()` for its retries; aligning `writeTask` to be fully async would remove this risk and match the pattern used next door.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix: serialize concurrent runtime cache ..." | Re-trigger Greptile

Quota cache saves and update-check cache writes could overlap within the
same process. That risks write races, and the update checker also wrote
directly to the final cache file instead of staging through a temp file.

Serialize in-process writes for both caches and make update-check cache
writes atomic via temp-file rename so concurrent runtime writers do not
corrupt the on-disk state.
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 6, 2026

Warning

Rate limit exceeded

@ndycode has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 56 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 56 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4930a5ab-43b7-4787-ab44-8eb019b9e892

📥 Commits

Reviewing files that changed from the base of the PR and between 478f44c and 77637f4.

📒 Files selected for processing (3)
  • lib/auto-update-checker.ts
  • lib/quota-cache.ts
  • test/auto-update-checker.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/concurrent-file-writers
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/concurrent-file-writers

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode
Copy link
Copy Markdown
Owner Author

ndycode commented Apr 6, 2026

Superseded by #387, which rebuilds the full open PR stack onto one reviewed integration branch.

@ndycode
Copy link
Copy Markdown
Owner Author

ndycode commented Apr 6, 2026

Closing in favor of #387.

@ndycode ndycode closed this Apr 6, 2026
@ndycode ndycode deleted the fix/concurrent-file-writers branch April 12, 2026 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant