fix: avoid partial cache entries when file loading fails - #109
vibhor-aggr wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 10 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
Reviewer's GuideRefactors file loading to build full metadata objects before inserting them into the cache and adds regression coverage to ensure failed dynamic loads do not leave partial cache entries. Sequence diagram for updated loadFile cache insertionsequenceDiagram
participant Caller
participant loadFile
participant fs
participant files
Caller->>loadFile: loadFile(name, dir, options, files)
loadFile->>fs: statSync(filename)
fs-->>loadFile: stats
loadFile->>fs: readFileSync(filename)
fs-->>loadFile: buffer
loadFile->>loadFile: [build obj metadata]
loadFile->>files: set(pathname, obj)
loadFile-->>Caller: obj
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the new test, you're monkeypatching
mzfs.readFileSync, butloadFileusesfs.readFileSync, so the failure path you're trying to exercise may never be hit; consider patching the actualfs.readFileSync(or whatever the production code uses) to ensure the test is meaningful. - To make the new test more robust, you could guard
cleanup()with try/catch or usefs.rmSync(dir, { recursive: true, force: true })so failures during unlink/rmdir don't mask assertions or leak resources when the test fails midway.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new test, you're monkeypatching `mzfs.readFileSync`, but `loadFile` uses `fs.readFileSync`, so the failure path you're trying to exercise may never be hit; consider patching the actual `fs.readFileSync` (or whatever the production code uses) to ensure the test is meaningful.
- To make the new test more robust, you could guard `cleanup()` with try/catch or use `fs.rmSync(dir, { recursive: true, force: true })` so failures during unlink/rmdir don't mask assertions or leak resources when the test fails midway.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thanks for the review. I rechecked the current head Verified locally with Node
|
Closes #99.
loadFile()previously inserted an empty cache entry beforestatSync()andreadFileSync()completed. If the read failed, the cache could retain a partial object withpathbut nomtime, letting a later request crash atfile.mtime.getTime().This builds the metadata first and only publishes the cache entry after the file has been read and hashed successfully, while preserving existing per-file metadata such as
maxAge.Verification:
./node_modules/.bin/mocha --grep 'partial file data'npm testgit diff --checkSummary by Sourcery
Ensure static file cache entries are only created after files are successfully loaded to avoid partial metadata in the cache.
Bug Fixes:
Tests: