Skip to content

fs: handle recursive watch setup races - #65699

Open
panva wants to merge 2 commits into
nodejs:mainfrom
panva:fix-recursive-watch-aix-enodev
Open

fs: handle recursive watch setup races#65699
panva wants to merge 2 commits into
nodejs:mainfrom
panva:fix-recursive-watch-aix-enodev

Conversation

@panva

@panva panva commented Sep 1, 2026

Copy link
Copy Markdown
Member

Arm directory watchers before reading entries to close the setup window. Discard stale watcher state when paths disappear so recreated paths can be watched again.

On AIX, treat ENODEV as missing only when stat confirms the path is gone. Preserve root throwIfNoEntry behavior and other watch errors.

Fixes: #65697

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Sep 1, 2026
@panva
panva requested a review from codebytere September 1, 2026 08:33
@panva
panva force-pushed the fix-recursive-watch-aix-enodev branch from 668c4de to bdb25e9 Compare September 1, 2026 08:39
@panva
panva marked this pull request as draft September 1, 2026 08:56
@aduh95

aduh95 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Arm directory watchers before reading entries to close the setup
window. Discard stale watcher state when paths disappear so recreated
paths can be watched again.

On AIX, treat ENODEV as missing only when stat confirms the path is
gone. Preserve root throwIfNoEntry behavior and other watch errors.

Fixes: nodejs#65697

Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Assisted-by: Codex
@panva
panva force-pushed the fix-recursive-watch-aix-enodev branch from bdb25e9 to 67b5c41 Compare September 1, 2026 10:05
@panva
panva marked this pull request as ready for review September 1, 2026 10:10
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 65.21739% with 24 lines in your changes missing coverage. Please review.
βœ… Project coverage is 90.06%. Comparing base (f9ab994) to head (a075c13).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/fs/recursive_watch.js 65.21% 21 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65699      +/-   ##
==========================================
- Coverage   90.07%   90.06%   -0.02%     
==========================================
  Files         754      754              
  Lines      256395   256444      +49     
  Branches    48494    48516      +22     
==========================================
+ Hits       230947   230963      +16     
- Misses      16563    16582      +19     
- Partials     8885     8899      +14     
Files with missing lines Coverage Ξ”
lib/internal/fs/recursive_watch.js 85.12% <65.21%> (-3.73%) ⬇️

... and 29 files with indirect coverage changes

πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • πŸ“¦ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 1, 2026
Comment thread test/parallel/test-fs-watch-recursive-setup-races.js Outdated
@panva panva removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 1, 2026
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Assisted-by: Codex

@lpinca lpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@panva panva added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 2, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 2, 2026
@sxa

sxa commented Sep 2, 2026

Copy link
Copy Markdown
Member

@panva This is presumably blocked because it still has needs-ci on it but the github checks are complete. I do note that the last node-test-pull-request run didn't run to completion but I guess an earlier one did to make the github checks complete ok ... Should we just remove needs-ci?

@panva

panva commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

It is not blocked. It'll land with the first CQ after it ages.

The `needs-ci` label identifies pull requests that require a full Jenkins CI
run. It is a classification, not an indication that CI is still pending. Leave
it in place after CI completes. Removing it does not waive the underlying CI
requirement or make a pull request eligible to land without the required
checks. Removing it also makes it harder for releasers to identify the scope of
a change when working on a release proposal.

I do note that the last node-test-pull-request run didn't run to completion

It did. With known flakes. It's not red and it's not aborted. So it's good. Orange is acceptable.

git node land 65699                              
βœ”  Done loading data for nodejs/node/pull/65699
----------------------------------- PR info ------------------------------------
Title      fs: handle recursive watch setup races (#65699)
Author     Filip Skokan <panva.ip@gmail.com> (@panva)
Branch     panva:fix-recursive-watch-aix-enodev -> nodejs:main
Labels     fs, author ready, needs-ci, commit-queue
Commits    2
 - fs: handle recursive watch setup races
 - fixup! fs: handle recursive watch setup races
Committers 1
 - Filip Skokan <panva.ip@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/65699
Fixes: https://github.com/nodejs/node/issues/65697
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
--------------------------------------------------------------------------------
   β„Ή  This PR was created on Tue, 01 Sep 2026 08:33:33 GMT
   βœ”  Approvals: 2
   βœ”  - Shelley Vohr (@codebytere): https://github.com/nodejs/node/pull/65699#pullrequestreview-5080561571
   βœ”  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/65699#pullrequestreview-5082737185
   ✘  This PR needs to wait 15 more hours to land
   βœ”  Last GitHub CI successful
   β„Ή  Last Full PR CI on 2026-09-02T03:52:29Z: https://ci.nodejs.org/job/node-test-pull-request/76868/
βœ”  Build data downloaded
   βœ”  Last Jenkins CI successful
--------------------------------------------------------------------------------
? This PR is not ready to land, do you want to continue? (Y/n)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs-watch-recursive-delete-race consistently failing on AIX

6 participants