fix(fs): detect an unwritable directory from the write, not from mkdir - #510
Conversation
🦋 Changeset detectedLatest commit: 0ac99e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
4 Skipped Deployments
|
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe filesystem drain now detects unwritable directories during append operations. It caches permission-related failures, warns once per directory, skips future writes, and continues reporting other failures. ChangesFilesystem drain behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Thank you for following the naming conventions! 🙏 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/fs-drain-probe-the-write.md:
- Line 7: Update the release-note wording to replace “baked in” with an explicit
statement that the log directory already existed, while preserving the
surrounding explanation of the probe and warning behavior.
In @.changeset/fs-drain-readonly-directory.md:
- Line 14: Update the changeset wording to avoid promising that only one append
is attempted: state that concurrent batches may each call appendFile until the
drain observes a matching EROFS, EACCES, or EPERM error, after which it disables
itself and warns once.
In `@packages/evlog/src/adapters/fs.ts`:
- Around line 178-196: Update the JSDoc for createFsDrain with a short `@remarks`
sentence stating that EROFS, EACCES, and EPERM errors mark the configured
directory unavailable and disable the filesystem drain for the current process.
In `@packages/evlog/test/adapters/fs.test.ts`:
- Around line 404-414: Rename the test case around createFsDrainFresh to state
that a non-permission write failure is logged rather than propagated, matching
the await drain(createDrainContext()) behavior and console.error assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ac37b29-ba69-458a-854a-aefebf945cac
📒 Files selected for processing (4)
.changeset/fs-drain-probe-the-write.md.changeset/fs-drain-readonly-directory.mdpackages/evlog/src/adapters/fs.tspackages/evlog/test/adapters/fs.test.ts
1 participant
Follow-up to #509, which shipped a check that does not hold. CodeRabbit caught both problems after the merge and it was right on both.
mkdirdoes not prove write access. With{ recursive: true }it is a no-op on a directory that already exists, so it succeeds on a read-only one. Verified directly:A deployment whose log directory is baked into the image therefore passed the probe and kept throwing on every batch, which is the exact failure #509 set out to remove.
The probe was not concurrency-safe. It cached only after awaiting, so simultaneous batches each ran it and each warned.
Both go away by making the write the check. The first append that fails with
EROFS,EACCESorEPERMmarks the directory and warns once;resolvereturnsnullfrom then on.markUnwritableis synchronous, so its check-then-add cannot interleave no matter how many batches arrive together. Anything else still surfaces through the drain's own error path.Also applies the two wording fixes from that review: "an HTTP adapter", and "disables itself in the Edge runtime" in the changeset.
Testing
packages/evlog/test/adapters/fs.test.ts— 25 passing, three new cases: a directory that exists but rejects the write, three concurrent batches warning once, andENOSPCstill reported rather than silenced. Full package suite 1788 passing,pnpm api:snapshotunchanged.Summary by CodeRabbit