fix(fs): disable the drain when its directory is not writable - #509
Conversation
🦋 Changeset detectedLatest commit: aaa9cf5 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.
|
|
Thank you for following the naming conventions! 🙏 |
commit: |
📝 WalkthroughWalkthroughThe filesystem drain now checks directory writability before activation, caches permission-related failures, warns once, and skips later writes. Non-permission errors still propagate. Tests, documentation, and a changeset describe the behavior. ChangesFilesystem drain writability handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/evlog/src/adapters/fs.ts`:
- Line 64: Use factual, plain wording in both affected texts: in
packages/evlog/src/adapters/fs.ts lines 64-64, change “a HTTP adapter” to “an
HTTP adapter”; in .changeset/fs-drain-readonly-directory.md lines 14-14, replace
“bows out of the Edge runtime” with “disables itself in the Edge runtime.”
- Around line 55-60: The isDirWritable probe currently treats a successful
recursive mkdir as proof of write access; update it to perform an actual
temporary file write in the target directory and clean it up, or classify the
first write failure as non-writable and cache false. Preserve writableDirs
caching and ensure EACCES/EROFS results return false so later batches return
null. Add a regression case covering mockedMkdir resolving while the write
operation fails with EACCES or EROFS.
- Line 29: Update writableDirs and the filesystem drain flow to cache the
in-flight Promise<boolean> for each directory, so concurrent writability checks
share one isDirWritable() probe and warning. Replace the entry with the settled
boolean on success, and remove it when a non-permission error rejects so later
calls can retry. Add a regression test that invokes the same drain concurrently
and Promise.all-waits for all resolved drains when concurrent hook resolution is
supported.
🪄 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: cf3e90db-88fb-46a3-ba11-504ceeaed811
📒 Files selected for processing (4)
.changeset/fs-drain-readonly-directory.mdapps/docs/content/4.integrate/adapters/self-hosted/01.fs.mdpackages/evlog/src/adapters/fs.tspackages/evlog/test/adapters/fs.test.ts
| ] | ||
|
|
||
| const gitignoreWritten = new Set<string>() | ||
| const writableDirs = new Map<string, boolean>() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 '\bdefineDrain\s*\(' packages/evlog/src
rg -n -C 8 '\b(createFsDrain|isDirWritable)\b' packages/evlog/src packages/evlog/testRepository: HugoRCD/evlog
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked files matching fs.ts:\n'
git ls-files | rg '(^|/)packages/evlog/src/adapters/fs\.ts$|(^|/)evlog/src/adapters/fs\.ts$|/(fs)\.ts$' || true
printf '\nAll defineDrain usages:\n'
rg -n '\bdefineDrain\s*\(' packages/evlog/src packages/evlog/test packages/evlog 2>/dev/null || true
printf '\nAll createFsDrain / isDirWritable usages:\n'
rg -n '\b(createFsDrain|isDirWritable)\b' packages/evlog/src packages/evlog/test packages/evlog 2>/dev/null || true
printf '\nFile list with likely drain definitions:\n'
git ls-files 'packages/evlog/src/**/*.ts' 'packages/evlog/src/**/*.tsx' 'packages/evlog/test/**/*.ts' 'packages/evlog/test/**/*.tsx' | sed -n '1,200p'Repository: HugoRCD/evlog
Length of output: 12864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'packages/evlog/src/adapters/fs.ts relevant lines:\n'
sed -n '1,240p' packages/evlog/src/adapters/fs.ts | nl -ba
printf '\npackages/evlog/src/shared/define.ts outline:\n'
ast-grep outline packages/evlog/src/shared/define.ts --view compact || true
printf '\npackages/evlog/src/shared/define.ts resolve/defineDrain lines:\n'
rg -n -C 15 'resolve|defineDrain|define.*Drain|Drain' packages/evlog/src/shared/define.ts || true
printf '\npackages/evlog src shared files mentioning defineDrain or resolve:\n'
rg -n '\b(resolve|defineDrain)\s*\(|\b(drain|Drain)\b' packages/evlog/src/shared packages/evlog/src/*.ts packages/evlog/src/runtime packages/evlog/src/audit.ts packages/evlog/src/adapters 2>/dev/null || trueRepository: HugoRCD/evlog
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'packages/evlog/src/adapters/fs.ts relevant lines:\n'
awk '{printf "%6\t%s\n", NR, $0}' packages/evlog/src/adapters/fs.ts | sed -n '1,240p'
printf '\npackages/evlog/src/shared/define.ts outline:\n'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline packages/evlog/src/shared/define.ts --view compact || true
else
printf 'ast-grep not available\n'
fi
printf '\npackages/evlog/src/shared/define.ts resolve/defineDrain lines:\n'
rg -n -C 15 'resolve|defineDrain|define.*Drain|Drain' packages/evlog/src/shared/define.ts || true
printf '\npackages/evlog src shared files mentioning defineDrain or resolve:\n'
rg -n '\b(resolve|defineDrain)\s*\(|\b(drain|Drain)\b' packages/evlog/src/shared packages/evlog/src/*.ts packages/evlog/src/runtime packages/evlog/src/audit.ts packages/evlog/src/adapters 2>/dev/null || trueRepository: HugoRCD/evlog
Length of output: 28811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'packages/evlog/src/adapters/fs.ts isDirWritable / createFsDrain:\n'
awk '{printf "%6\t%s\n", NR, $0}' packages/evlog/src/adapters/fs.ts | sed -n '1,220p'
printf '\npackages/evlog/src/shared/drain.ts:\n'
awk '{printf "%6\t%s\n", NR, $0}' packages/evlog/src/shared/drain.ts | sed -n '1,80p'
printf '\npackages/evlog/test/adapters/fs.test.ts createFsDrain section:\n'
awk '{printf "%6\t%s\n", NR, $0}' packages/evlog/test/adapters/fs.test.ts | sed -n '290,390p'
printf '\nPackages/lock references:\n'
rg -n '"defineDrain"|defineDrain|fs\.mkdir|mkdir' package.json pnpm-lock.yaml packages -g '!node_modules' 2>/dev/null || trueRepository: HugoRCD/evlog
Length of output: 8301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'packages/evlog/src/adapters/fs.ts isDirWritable / createFsDrain (detailed):\n'
python3 - <<'PY'
from pathlib import Path
p = Path('packages/evlog/src/adapters/fs.ts')
lines = p.read_text().splitlines()
for i in range(26, 210):
print(f'{i+1:6}\t{lines[i]}')
PY
printf '\npackages/evlog/src/shared/drain.ts (detailed):\n'
python3 - <<'PY'
from pathlib import Path
p = Path('packages/evlog/src/shared/drain.ts')
lines = p.read_text().splitlines()
for i in range(0, min(len(lines), 90)):
print(f'{i+1:6}\t{lines[i]}')
PY
printf '\npackages/evlog/test/adapters/fs.test.ts relevant sections (detailed):\n'
python3 - <<'PY'
from pathlib import Path
p = Path('packages/evlog/test/adapters/fs.test.ts')
lines = p.read_text().splitlines()
for i in list(range(1,25)) + list(range(290,382)):
print(f'{i+1:6}\t{lines[i]}')
PY
printf '\nProgrammatic race model for writableDirs:\n'
python3 - <<'PY'
class Model:
def __init__(self):
self.writable_dirs = {}
self.probe_calls = 0
self.resolved = False
self.resolve_value = True
async def mkdir(self):
self.probe_calls += 1
await None # simulate async
def isDirWritable(self):
if self.writable_dirs.get('x') is not None:
return self.writable_dirs['x']
self.writable_dirs['x'] = True
return True
async def resolve_all(self):
for _ in range(3):
await self.mkdir()
self.resolve_value = True
for _ in range(3):
self.isDirWritable()
m = Model()
m.resolve_all()
print({
'concurrent_probe_calls': m.probe_calls,
'pending_false_lookups_sees_succeeded_entry': True,
})
PYRepository: HugoRCD/evlog
Length of output: 16479
Coalesce concurrent writability probes.
writableDirs stores only settled booleans. Concurrent dispatches can all miss the cache, run separate isDirWritable() probes, and call console.warn(...) on the same non-permission probe failure. Cache the in-flight Promise<boolean> per directory and clean it up if a non-permission error rejects. Add a regression test with concurrent calls to the same drain, and Promise.all-wait for the multiple resolved drains if the hook permits concurrent resolution.
🤖 Prompt for 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.
In `@packages/evlog/src/adapters/fs.ts` at line 29, Update writableDirs and the
filesystem drain flow to cache the in-flight Promise<boolean> for each
directory, so concurrent writability checks share one isDirWritable() probe and
warning. Replace the entry with the settled boolean on success, and remove it
when a non-permission error rejects so later calls can retry. Add a regression
test that invokes the same drain concurrently and Promise.all-waits for all
resolved drains when concurrent hook resolution is supported.
| async function isDirWritable(dir: string): Promise<boolean> { | ||
| const cached = writableDirs.get(dir) | ||
| if (cached !== undefined) return cached | ||
|
|
||
| try { | ||
| await mkdir(dir, { recursive: true }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -fsSL 'https://nodejs.org/download/release/v18.20.8/docs/api/fs.html' |
rg -n 'Calling .*fsPromises\.mkdir.*recursive.*false'Repository: HugoRCD/evlog
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked fs adapter candidates:\n'
fd -a 'fs\.ts$' . | sed 's#^\./##' | head -50
printf '\nInspect target file size and relevant sections:\n'
wc -l packages/evlog/src/adapters/fs.ts
sed -n '1,140p' packages/evlog/src/adapters/fs.ts
printf '\nSearch writableDirs and isDirWritable usages:\n'
rg -n "writableDirs|isDirWritable|mkdir\(dir|recursive" packages/evlog/src/adapters/fs.ts packages -g '*.ts' -g '*.tsx' | head -120
printf '\nFind writableDirs usages across repo:\n'
rg -n "writableDirs" . -g '*.ts' -g '*.tsx' -g '*.test.ts' -g '*.test.tsx' -g '*.spec.ts' -g '*.spec.tsx' | head -200Repository: HugoRCD/evlog
Length of output: 11451
🌐 Web query:
Node.js 18 fs.mkdir recursive existing directory promise resolves does not reject
💡 Result:
In Node.js 18 (and other versions), calling fsPromises.mkdir with the { recursive: true } option will not reject if the target directory already exists [1][2]. The promise will resolve successfully because the recursive option is specifically designed to prevent errors when the directory path already exists [1]. If the directory does not already exist, the promise resolves with the first directory path that was created [1]. If the directory already exists, it simply fulfills without creating any new directories [1]. Rejection only occurs when { recursive: false } is used and the target directory already exists [1].
Citations:
- 1: https://nodejs.org/docs/latest-v18.x/api/fs.html
- 2: https://nodejs.org/download/release/v18.20.8/docs/api/fs.html
Probe actual file write access instead of directory existence.
mkdir(dir, { recursive: true }) fulfills when dir already exists, so a pre-existing read-only directory can be cached as writable. That lets every subsequent batch fail instead of returning null once. Probe an actual write with cleanup, or classify the first permission failure from the file write and cache false. Add a regression case where mockedMkdir resolves but the write operation returns EACCES or EROFS.
🤖 Prompt for 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.
In `@packages/evlog/src/adapters/fs.ts` around lines 55 - 60, The isDirWritable
probe currently treats a successful recursive mkdir as proof of write access;
update it to perform an actual temporary file write in the target directory and
clean it up, or classify the first write failure as non-writable and cache
false. Preserve writableDirs caching and ensure EACCES/EROFS results return
false so later batches return null. Add a regression case covering mockedMkdir
resolving while the write operation fails with EACCES or EROFS.
| } catch (error) { | ||
| if (!isUnwritableError(error)) throw error | ||
| writableDirs.set(dir, false) | ||
| console.warn(`[evlog/fs] "${dir}" is not writable, so the file system drain is disabled. This is expected on a serverless host, where only the temp directory is writable and does not outlive the instance — send events to a HTTP adapter instead, or set the drain's \`dir\` to a writable path.`) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use factual, plain wording in the new user-facing text.
packages/evlog/src/adapters/fs.ts#L64-L64: change “a HTTP adapter” to “an HTTP adapter.”.changeset/fs-drain-readonly-directory.md#L14-L14: replace “bows out of the Edge runtime” with “disables itself in the Edge runtime.”
As per coding guidelines, keep prose factual and plain, including log messages and changeset descriptions, and omit filler.
📍 Affects 2 files
packages/evlog/src/adapters/fs.ts#L64-L64(this comment).changeset/fs-drain-readonly-directory.md#L14-L14
🤖 Prompt for 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.
In `@packages/evlog/src/adapters/fs.ts` at line 64, Use factual, plain wording in
both affected texts: in packages/evlog/src/adapters/fs.ts lines 64-64, change “a
HTTP adapter” to “an HTTP adapter”; in .changeset/fs-drain-readonly-directory.md
lines 14-14, replace “bows out of the Edge runtime” with “disables itself in the
Edge runtime.”
Source: Coding guidelines
1 participant
createFsDrain()guarded neither itsmkdirnor itsappendFile. On a serverless host, where everything outside the temp directory is read-only, attaching it threw once per batch for the lifetime of the deployment — and the events had nowhere to land regardless. The only way out was for the caller to guess at the environment:An env sniff is the wrong test anyway: the question is whether the directory is writable, which also covers read-only containers and restricted CI, and the drain can simply ask.
It now probes the directory once and caches the answer. On
EROFS,EACCESorEPERMit warns a single time and returnsnullfromresolve, which is exactly how it already bows out of the Edge runtime. Attach it unconditionally.Anything else — a full disk, a genuine bug — still propagates.
resolve()runs outsidedefineDrain's try block, so those surface rather than being swallowed.Testing
packages/evlog/test/adapters/fs.test.ts— 23 passing, two new cases: read-only warns once, writes nothing and probes once;ENOSPCpropagates. Full package suite 1786 passing.pnpm api:snapshotunchanged, no new export.The docs callout on
/integrate/adapters/self-hosted/fsnow covers the unwritable case alongside Edge.Summary by CodeRabbit
Bug Fixes
Documentation