Repository navigation
ci: restore EMFILE test on Node.js 26 - #2
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: eslint#21297 Source head: 0fd7ce0
⛔ Shipwright · BlockedRecommendation: do not merge PR #2 · Tier
Findings (3)
Fireworks usage: 5,958 input · 454 output · 6,412 total tokens · $0.0016 · 9s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| const fileName = `file_${i}.js`; | ||
|
|
||
| return readFile(`${OUTPUT_DIRECTORY}/${fileName}`); | ||
| return writeFile( |
There was a problem hiding this comment.
Shipwright · CRITICAL
The EMFILE test now overwrites every generated file with '// Overwritten' via concurrent writeFile calls.
Impact: The EMFILE test now overwrites every generated file with '// Overwritten' via concurrent writeFile calls. If the test fails partway or is interrupted, the output directory is left with corrupted files. More importantly, writeFile with a string payload opens the file with 'w' flag, truncating it. On filesystems or platforms where write open does not consume a file descriptor in the same way as read open (e.g., delaye…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| const fileName = `file_${i}.js`; | ||
|
|
||
| return readFile(`${OUTPUT_DIRECTORY}/${fileName}`); | ||
| return writeFile( |
There was a problem hiding this comment.
Shipwright · HIGH
The comment says 'writing is also what --fix does above', but the test now writes a hardcoded string '// Overwritten' to every file.
Impact: The comment says 'writing is also what --fix does above', but the test now writes a hardcoded string '// Overwritten' to every file. A future maintainer reading this will not understand why the test writes a comment string instead of simulating the actual fix output, and whether the content matters for triggering EMFILE. The magic string is unexplained and the relationship to --fix is asserted but not demonstrated.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
Prerequisites checklist
AI acknowledgment
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain: Restore the EMFILE check on Node.js 26 by switching
generateEmFileError()fromreadFile()towriteFile().Fixes eslint#21274.
What changes did you make? (Give an overview)
This PR restores the EMFILE check on Node.js 26 by changing its probe from
readFile()towriteFile().Since Node.js 26.8.0, small path-based
readFile()calls perform open, fstat, read, and close in a single thread pool task (nodejs/node#65327). Concurrent reads therefore no longer accumulate file descriptors. As a result,generateEmFileError()stopped raising EMFILE and failed with the errorEMFILE error not encountered.eslint#21265 temporarily skipped the check on Node.js 26.Specifically, this PR:
readFile()probe withwriteFile(). Path-based writes still open and close files in separate thread pool tasks in current Node.js releases, allowing concurrent writes to exhaust file descriptors. This also reflects the--fixworkload exercised earlier in the script.Test EMFILE Handlingstep in.github/workflows/ci.yml.I ran
node tools/check-emfile-handling.js, the command behindnpm run test:emfile, on macOS with both soft and hardRLIMIT_NOFILEset to 512. I tested Node.js 24.18.0, 26.7.0, and 26.8.1. In all three cases, the ESLint run passed and the probe raised EMFILE.Is there anything you'd like reviewers to focus on?
Windows behavior
Does
writeFile()reliably raise EMFILE onwindows-latest, where this CI step also runs? The script notes that Windows has no hard file-descriptor limit, and I could not test it locally.Probe contents
The probe overwrites each generated file with
// Overwritten. The content is arbitrary because the probe runs after ESLint and the directory is removed on exit. I kept it as a valid line comment in case the files are inspected.Future Node.js behavior
As noted on the issue, nodejs/node#65489 would batch small path-based
writeFile()calls into a single thread pool task. If that change ships in a Node.js version covered by ESLint CI, this probe will stop raising EMFILE. The check will then fail with the errorEMFILE error not encountered.This PR restores the check for current Node.js releases, but it is not a permanent way to trigger EMFILE.A raw
open()probe would keep triggering EMFILE, but it would no longer demonstrate that ESLint's actual read or write workload can exhaust file descriptors. If maintainers prefer a durable check now, deterministic EMFILE/ENFILE injection is the better direction.The optimization proposed in nodejs/node#65489 does not cover every promise-based write. Data larger than 512 KiB stays on the existing
FileHandlepath. Other activity in the process can also exhaust file descriptors. The optimization alone therefore would not establish that the retry handling inESLint.outputFixes()can be removed.Disclosure: I'm a participant of open source contribution program OSSCA.
Summary by CodeRabbit
Source merge-base:
87e0a082438264ad90b87fd74165ab4fd90f63efSource head:
0fd7ce0732003894d0bb26bc9fb8c35d5d748cb6