Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected cache behavior (unexpected hits and unexpected outputs) after renaming file #23106

Closed
1 of 4 tasks
ChumpChief opened this issue Apr 30, 2024 · 5 comments
Closed
1 of 4 tasks
Assignees
Labels

Comments

@ChumpChief
Copy link

Current Behavior

In the repro repo, the "build" command has caching enabled. Running it (pnpm build at root) does the correct thing in response to modifying file contents, but renaming the file exhibits a couple strange behaviors. The repro steps walk through a series of actions to encounter several unexpected symptoms, annotated with ❌. I believe this is a regression, since I repro on nx 18.3.4 but not 17.0.6.

Expected Behavior

Renaming the file should have similar impact on determining cache hits as modifications to file contents.

GitHub Repo

https://github.com/ChumpChief/nx-repro

Steps to Reproduce

Prereq: pnpm globally installed

  1. Clone the repro repository
  2. pnpm i at repo root
  3. pnpm build at repo root
  4. Observe packages/testPackage/built contains the expected foo.js
  5. mv packages/testPackage/foo.ts packages/testPackage/bar.ts at repo root
  6. pnpm build at repo root
  7. Observe the build command DOES NOT get a cache hit, which is expected since we renamed the file ✅
  8. Observe packages/testPackage/built contains the expected bar.js (and still has foo.js which is expected since we didn't do anything to delete it). ✅
  9. mv packages/testPackage/bar.ts packages/testPackage/woz.ts
  10. pnpm build at repo root
  11. Observe the build command DOES get a cache hit, which is surprising since we renamed the file ❌
  12. Observe packages/testPackage/built is missing the expected woz.js (but still has the foo.js and bar.js files as expected since we didn't do anything to delete them). ❌
  13. pnpm clean at repo root (which will delete the built output)
  14. mv packages/testPackage/woz.ts packages/testPackage/foo.ts (returning the file name back to the original state)
  15. pnpm build at repo root
  16. Observe the build command DOES get a cache hit, which is expected since we built with foo.ts in step 3 ✅
  17. Observe packages/testPackage/built has the expected foo.js, but surprisingly also has bar.js (which was not part of the output from step 3). ❌

Nx Report

Node   : 20.12.2
OS     : darwin-arm64
pnpm   : 9.0.6

nx          : 18.3.4
@nrwl/tao   : 18.3.4
typescript  : 5.4.5

Failure Logs

There are no failure logs, all commands report as passing.

Package Manager Version

9.0.6

Operating System

  • macOS
  • Linux
  • Windows
  • Other (Please specify)

Additional Information

  • The top tsconfig.json is just the default one created from tsc --init.
  • Does NOT repro on nx 17.0.6 with everything else the same.
    • However, step 15 does not get a cache hit which seems surprising but relatively benign.
    • I did not try to narrow down the first-repro-version further.
@FrozenPandaz FrozenPandaz added the scope: core core nx functionality label May 8, 2024
@rezoled
Copy link

rezoled commented May 22, 2024

Same for me here, I have GQL files that we generate TS file from. the task to generate the files id dependant on **/*.gql files.
Editing or creating new files works great, but when moving or renaming files the cache misses the changes and loads the cached task output.

@jokeyrhyme
Copy link

We're experiencing this with nx 17.2.8, so I think the regression happened somewhere after 17.0.6 and either in or before 17.2.8

@iAmNathanJ
Copy link
Contributor

Same experience with version 19.6.5 - it looks like rust based hashing doesn't account for project file names? https://github.com/nrwl/nx/blob/master/packages/nx/src/native/tasks/hashers/hash_project_files.rs#L20

If I'm reading it right, I see from the commit where the rust version was introduced that the previous js-based version did factor in the file name.

const files: string[] = [];
for (const { file, hash } of filteredFiles) {
files.push(file, hash);
}
const value = hashArray(files);

@Cammisuli @AgentEnder am I following that correctly?

If so, would the fix be as simple as adding the filename to the hash here or would it require deeper changes?

This seems like an important fix. I'd be happy to open a PR if it's a reasonably straight forward change.

iAmNathanJ added a commit to iAmNathanJ/nx that referenced this issue Sep 10, 2024
iAmNathanJ added a commit to iAmNathanJ/nx that referenced this issue Sep 11, 2024
iAmNathanJ added a commit to iAmNathanJ/nx that referenced this issue Sep 11, 2024
iAmNathanJ added a commit to iAmNathanJ/nx that referenced this issue Sep 11, 2024
iAmNathanJ added a commit to iAmNathanJ/nx that referenced this issue Sep 11, 2024
iAmNathanJ added a commit to iAmNathanJ/nx that referenced this issue Sep 11, 2024
@ChumpChief
Copy link
Author

I see the fix made it into 19.8.0 (thank you @iAmNathanJ !!!) - I just verified on my repro repo using the steps mentioned above that the issue is indeed fixed using 19.8.2, so closing this issue out.

Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants