npmInstall up-to-date check broken with fastNpmInstall

[SebastianS90]

I stumbled on a weird issue with the following steps:

1. Fresh git checkout, i.e. no node_modules directory present
2. Run task npmInstall with the fastNpmInstall option enabled
3. Run the clean task, i.e. remove the whole node_modules directory
4. Run npmInstall again, it reports as up-to-date
5. Other tasks fail because node_modules is missing

The problem is that in step 2 the file node_modules/.package-lock.json was not recorded as output because it did not yet exist at configuration time, only after task execution. So the task has no configured outputs and won't become out of date by removing the node_modules directory.

Proposed fix: When using fastNpmInstall, do not check for existance of the file at configuration time here, instead always return the file.

[deepy]

Weirdly enough I see the opposite behaviour: https://github.com/node-gradle/gradle-node-plugin/blob/06c0d5db77a1762929a56280eb6df91010fe835f/src/test/groovy/com/github/gradle/node/npm/task/NpmInstall_integTest.groovy#L84-L85

What npm version are you using?

[SebastianS90]

I use npm 10.5.2, installed from the Arch Linux repo.

The opposite behavior is also easy to explain:

1. Fresh git checkout, i.e. no node_modules directory present
2. Run task npmInstall. At configuration time the node_modules/.package-lock.json did not exist, so it is tracked as "null" in the task outputs, regardless of its contents after task execution.
3. Run npmInstall again. At configuration time the node_modules/.package-lock.json file exists, so the up to date check compares it's contents to the previously tracked output "null" and decides that the task it not up to date because the output has changed. After the task completes, the file content is tracked as output.
4. Run npmInstall again. At configuration time the node_modules/.package-lock.json file exists, so the up to date check compares it's contents to the contents tracked in step 3. It is the same, so the task is up to date (provided that no input has changed).

This scenario is not that severe, it just executes the task when it was not necessary. My scenario in the issue opening post however is rather annoying since it is not easy to convince Gradle to execute the task again when it thinks it is still up to date and it leads to a failing build.

The proposed fix (to not check for file existence at configuration time) should solve both scenarios.

[deepy]

The test itself turns out to be broken, which is a little easier to confirm outside a plane 😅 But this seems like it could lead to cache poisoning and that makes it a bit more severe, I'll need to fix the test before I can push a fix though. But I agree on the fix, it looks like I added that on accident

[deepy]

Do the tasks that use packages provided by npm install have a dependency on npmInstall?

I can only reproduce this with tasks that don't have the dependency and I can't really fix the output properly without dropping support for npm < 7 which I don't want to do just quite yet

[SebastianS90]

Thank you so much for looking into it and composing an integration test. In my actual project I also have npmInstallCommand = 'ci' and if you add that to your test then it will fail.

The reason for your test to pass is that with the default npmInstallCommand = 'install' (and adding --info) we have in the second run:

Task ':npmInstall' is not up-to-date because:
  Output property 'packageLockFileAsOutput' has been added for task ':npmInstall'

Again, the if-exists semantics is kind of weird here. At configuration time for the first execution the file does not exist, so "null" is tracked as output after task execution even though the file has just been created by that task. At configuration time for the second execution the file exists, so it is part of the outputs, and when the second run reaches the up-to-date check it sees that this output property has somehow changed from "null" to the existing file, so the task is not up-to-date.

With npmInstallCommand = 'ci' that output is not tracked (which is correct since the file is not written when using ci).

Another way to make your test fail even with npmInstallCommand = 'install' is install - delete - install - delete - taskWithDependency:

--- a/src/test/groovy/com/github/gradle/node/npm/task/NpmInstall_integTest.groovy
+++ b/src/test/groovy/com/github/gradle/node/npm/task/NpmInstall_integTest.groovy
@@ -148,6 +148,19 @@ class NpmInstall_integTest extends AbstractIntegTest {
         then:
         result.task(':deleteNodeModules').outcome == TaskOutcome.SUCCESS

+        when:
+        result = build('npmInstall')
+
+        then:
+        result.task(":npmInstall").outcome == TaskOutcome.SUCCESS
+
+        when:
+        // when the node_modules is removed
+        result = build('deleteNodeModules')
+
+        then:
+        result.task(':deleteNodeModules').outcome == TaskOutcome.SUCCESS
+
         when:
         result = build('taskWithDependency')