Uh oh!
There was an error while loading. Please reload this page.
fs: fixes recursive fs.watch crash on Linux when deleting files - #52349
Conversation
Signed-off-by: Matteo Collina <hello@matteocollina.com> Fixes: nodejs#52018
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot
commented
Apr 3, 2024
anonrig
commented
Apr 10, 2024
Tests seems to fail on macOS @mcollina |
mcollina
commented
Apr 11, 2024
@MoLow ptal |
nodejs-github-bot
commented
Apr 11, 2024
nodejs-github-bot
commented
Apr 11, 2024
nodejs-github-bot
commented
Apr 18, 2024
nodejs-github-bot
commented
Apr 18, 2024
nodejs-github-bot
commented
Apr 19, 2024
Commit Queue failed- Loading data for nodejs/node/pull/52349 ✔ Done loading data for nodejs/node/pull/52349 ----------------------------------- PR info ------------------------------------ Title fs: fixes recursive fs.watch crash on Linux when deleting files (#52349) Author Matteo Collina (@mcollina) Branch mcollina:fix-52018 -> nodejs:main Labels fs, author ready, needs-ci, lts-watch-v20.x Commits 10 - fs: fixes recursive fs.watch crash on Linux when deleting files - fixup - fixup - Update test/parallel/test-fs-watch-recursive-delete.js - fixup - fixup - fixup - fixup - fixup - fixup Committers 2 - Matteo Collina - GitHub PR-URL: https://github.com/nodejs/node/pull/52349 Fixes: https://github.com/nodejs/node/issues/52018 Reviewed-By: Yagiz Nizipli Reviewed-By: Moshe Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52349 Fixes: https://github.com/nodejs/node/issues/52018 Reviewed-By: Yagiz Nizipli Reviewed-By: Moshe Atlow -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 03 Apr 2024 14:42:00 GMT ✔ Approvals: 2 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/52349#pullrequestreview-2009056028 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/52349#pullrequestreview-2009091163 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-04-18T17:34:54Z: https://ci.nodejs.org/job/node-test-pull-request/58502/ - Querying data for job/node-test-pull-request/58502/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 52349 From https://github.com/nodejs/node * branch refs/pull/52349/merge -> FETCH_HEAD ✔ Fetched commits as 580aae58f62c..3a358b291a3f -------------------------------------------------------------------------------- [main 0211a26868] fs: fixes recursive fs.watch crash on Linux when deleting files Author: Matteo Collina Date: Wed Apr 3 16:40:46 2024 +0200 2 files changed, 32 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-fs-watch-recursive-delete.js [main ddb4e31ca1] fixup Author: Matteo Collina Date: Wed Apr 3 16:52:27 2024 +0200 2 files changed, 13 insertions(+), 17 deletions(-) [main e4e2bebb2d] fixup Author: Matteo Collina Date: Thu Apr 11 11:45:50 2024 +0200 1 file changed, 2 insertions(+), 1 deletion(-) [main d23c7c8330] Update test/parallel/test-fs-watch-recursive-delete.js Author: Matteo Collina Date: Thu Apr 11 11:46:31 2024 +0200 1 file changed, 3 insertions(+), 2 deletions(-) [main 6669dac5e8] fixup Author: Matteo Collina Date: Thu Apr 11 11:47:23 2024 +0200 1 file changed, 2 insertions(+), 3 deletions(-) [main 1c8c1ef7f9] fixup Author: Matteo Collina Date: Thu Apr 11 13:37:38 2024 +0200 1 file changed, 1 insertion(+), 1 deletion(-) [main e08d7b4c34] fixup Author: Matteo Collina Date: Thu Apr 11 15:43:11 2024 +0200 1 file changed, 6 insertions(+), 4 deletions(-) [main 61410fe592] fixup Author: Matteo Collina Date: Thu Apr 11 16:54:50 2024 +0200 1 file changed, 6 insertions(+), 2 deletions(-) [main 14cf96222c] fixup Author: Matteo Collina Date: Thu Apr 18 16:18:58 2024 +0200 1 file changed, 3 insertions(+) [main 0cb4e9b123] fixup Author: Matteo Collina Date: Thu Apr 18 16:26:10 2024 +0200 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 10 commits in the PR. Attempting autorebase. Rebasing (2/20)https://github.com/nodejs/node/actions/runs/8753676430 |
nodejs-github-bot
commented
Apr 19, 2024
Landed in d3eb1cb |
`fs.watch(dirname(configPath), { recursive: true })` is one kernel watch on
macOS and Windows and a userland per-entry walk everywhere else. On Linux that
walk opened one inotify watch descriptor per file and per directory under the
user's project root, `node_modules` included: 3610 descriptors for a 3000-file
tree, measured against a real `orangerail studio` process on ubuntu-latest.
Three defects fell out of that. A failed watch was fatal, because the walker
reports one by emitting `'error'` and nothing ever listened. Exhausting the
inotify budget truncated the watch set silently. And deleting a watched file
killed the server on Node 20.0-20.12 and 22.0, where the walker's per-file
callback still called `statSync` unguarded (nodejs/node#52349).
Studio now walks the config directory itself -- bounded, skipping
`node_modules`, dot-directories and symlinked directories -- and opens one
non-recursive watch per directory, which is the base capability on every
platform. Each watcher gets the `'error'` handler it never had, the cap reports
itself through a new `onWarn` channel, and the debounced tick re-syncs the set
so a directory created after startup is picked up. Same project on the same
runner after the change: 3 descriptors, and the delete no longer kills it.
`ERR_FEATURE_UNAVAILABLE_ON_PLATFORM` is not part of this: that throw left
`lib/fs.js` in Node 19.1.0 and is unreachable on `engines.node >= 20`, verified
on Linux at 20.0.0, 20.12.2, 20.13.0, 22.0.0 and 22.20.0. No engines floor moves.`fs.watch(dirname(configPath), { recursive: true })` is one kernel watch on
macOS and Windows and a userland per-entry walk everywhere else. On Linux that
walk opened one inotify watch descriptor per file and per directory under the
user's project root, `node_modules` included: 3610 descriptors for a 3000-file
tree, measured against a real `orangerail studio` process on ubuntu-latest.
Three defects fell out of that. A failed watch was fatal, because the walker
reports one by emitting `'error'` and nothing ever listened. Exhausting the
inotify budget truncated the watch set silently. And deleting a watched file
killed the server on Node 20.0-20.12 and 22.0, where the walker's per-file
callback still called `statSync` unguarded (nodejs/node#52349).
Studio now walks the config directory itself -- bounded, skipping
`node_modules`, dot-directories and symlinked directories -- and opens one
non-recursive watch per directory, which is the base capability on every
platform. Each watcher gets the `'error'` handler it never had, the cap reports
itself through a new `onWarn` channel, and the debounced tick re-syncs the set
so a directory created after startup is picked up. Same project on the same
runner after the change: 3 descriptors, and the delete no longer kills it.
`ERR_FEATURE_UNAVAILABLE_ON_PLATFORM` is not part of this: that throw left
`lib/fs.js` in Node 19.1.0 and is unreachable on `engines.node >= 20`, verified
on Linux at 20.0.0, 20.12.2, 20.13.0, 22.0.0 and 22.20.0. No engines floor moves.
Fixes: #52018