Skip to content

fs: preserve directory timestamps in cp - #65540

Open
abhi128nandan wants to merge 2 commits into
nodejs:mainfrom
abhi128nandan:fix-cp-directory-timestamps
Open

fs: preserve directory timestamps in cp#65540
abhi128nandan wants to merge 2 commits into
nodejs:mainfrom
abhi128nandan:fix-cp-directory-timestamps

Conversation

@abhi128nandan

@abhi128nandan abhi128nandan commented Aug 25, 2026

Copy link
Copy Markdown

fs: preserve directory timestamps in cp

This fixes an issue where fs.cp and fs.cpSync (when a filter is
provided) correctly restore mtime and atime on copied files when
preserveTimestamps: true is passed, but skip restoring them for
directories.

The fix applies the existing setDestTimestamps helper to directories,
ensuring it is called after directory creation/copying but before any
modes are restored (since restoring a read-only mode would prevent
timestamp modification).

Note: This fix covers fs.cp() (async, all cases) and fs.cpSync()
when a filter is provided. The native fast path used by cpSync()
without a filter (CpSyncCopyDir in src/node_file.cc) has the
identical gap and is left for a separate follow-up contribution, since
it requires native code changes and a different review path.

@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 Aug 25, 2026
This fixes an issue where fs.cp and fs.cpSync (when a filter is
provided) correctly restore mtime and atime on copied files when
preserveTimestamps: true is passed, but skip restoring them for
directories.

The fix applies the existing setDestTimestamps helper to directories,
ensuring it is called after directory creation/copying but before any
modes are restored (since restoring a read-only mode would prevent
timestamp modification).

Note: This fix covers fs.cp() (async, all cases) and fs.cpSync()
when a filter is provided. The native fast path used by cpSync()
without a filter (CpSyncCopyDir in src/node_file.cc) has the
identical gap and is left for a separate follow-up contribution, since
it requires native code changes and a different review path.

Signed-off-by: Abhinandan Kumar <abhi128618@gmail.com>
@abhi128nandan
abhi128nandan force-pushed the fix-cp-directory-timestamps branch from 70b223c to 321146b Compare August 25, 2026 11:46
Comment thread lib/internal/fs/cp/cp-sync.js
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 27, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 27, 2026
@nodejs-github-bot

This comment was marked as outdated.

@abhi128nandan

Copy link
Copy Markdown
Author

Could someone please check the result of Jenkins CI job 76579? I’m unable to access the job because Jenkins shows “abhi128nandan is missing the Overall/Read permission”, so I can’t see whether there’s an actual test failure or an infrastructure issue. Thanks!

@sxa

sxa commented Aug 27, 2026

Copy link
Copy Markdown
Member

Could someone please check the result of Jenkins CI job 76579? I’m unable to access the job because Jenkins shows “abhi128nandan is missing the Overall/Read permission”, so I can’t see whether there’s an actual test failure or an infrastructure issue. Thanks!

I woldn't normally see a request like this but I happened to be looking at the job (randomly) for something else so I'll reply. The jobs is still running. The message with the link was only posted 40 minutes ago and it takes longer than that to run through the tests.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.08%. Comparing base (2743045) to head (6661a99).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65540      +/-   ##
==========================================
+ Coverage   90.06%   90.08%   +0.01%     
==========================================
  Files         751      751              
  Lines      254917   254926       +9     
  Branches    48133    48136       +3     
==========================================
+ Hits       229580   229638      +58     
+ Misses      16493    16463      -30     
+ Partials     8844     8825      -19     
Files with missing lines Coverage Δ
lib/internal/fs/cp/cp-sync.js 70.56% <100.00%> (+7.29%) ⬆️
lib/internal/fs/cp/cp.js 89.02% <100.00%> (+0.16%) ⬆️

... and 32 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.

@abhi128nandan

Copy link
Copy Markdown
Author

Could a collaborator please approve/re-run the GitHub Actions and start Jenkins CI for the updated HEAD 6661a995? The previous CI run was fully green; I synced the branch with main, which resulted in the new HEAD.

@LiviaMedeiros

Copy link
Copy Markdown
Member

Could a collaborator please approve/re-run the GitHub Actions and start Jenkins CI for the updated HEAD 6661a995? The previous CI run was fully green; I synced the branch with main, which resulted in the new HEAD.

Was there a reason to sync? AFAICT there were no conflicts with current main, it's completely fine to keep PR branch slightly 'outdated' as long as there's no recent changes on main that can't coexist with it.

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 28, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 28, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@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 Aug 29, 2026
@abhi128nandan

Copy link
Copy Markdown
Author

“I’m unable to inspect the Jenkins failure because Jenkins says I’m missing the Overall/Read permission. Could someone check the failing job logs?”

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. 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.

6 participants