Caches not invalidated as expected

[PieterVanEde]

Hi, first off thanks for your great plugin, I really find it very nice to work with. However, I have discovered a situation that is not desirable when we started using remote caches.

Brought down to the essence, suppose we have a multimodule project consisting of 3 modules: A, B and C. C depends on both A and B. C is an aggregate multi (like a WAR of a Fat JAR).

Then when we change the sources of A, all 3 modules are affected. This is as we expect.

When we then run a target (test or build) for affected, it processes all 3 modules. There is where we start seeing funny stuff:

• module A is not pulled from cache. This is expected.
• module B is pulled from cache. This is expected as nothing has changed.
• module C is pulled from cache. This is not expected and wrong, because now it contains an incorrect JAR for module A.

With the same setup, we see another strange situation. Suppose module C is not an aggregate pom, but a parent pom which defines dependencies versions in its dependencyManagement section. Then if a dependency version changes:

• modules A and B are also affected. This is expected, as now they depend on module C as a parent.
• modules A and B are pulled from cache. This is not expected, as the changed dependencies could have produced a different outcome (especially for tests)

Might we be configuring the plugin wrong, or is this actually a bug?

[khalilou88]

Hi @PieterVanEde,

Thanks for using the plugin. I need a reproduction so other users an me can provide feedback.

I think maybe you use -SNAPSHOT and in this case we don't need cache ? If yes this is not handled by the plugin yet.

[PieterVanEde]

We are indeed using SNAPSHOT, only during a build on our main branch we replace these by actual version numbers. However we never commit those version numbers to GIT. So from a cache perspective we always keep using SNAPSHOT versions.

Do you have plans to support SNAPSHOT versions in the future? Or would we have to move to a Gradle build (your version or the official plugin that launched with NX19) to keep this working?

[khalilou88]

I will fix the issue, just I should be sure about it.

If SNAPSHOT is the issue I can add during graph calculation an input with date to invalidate the cache for build target if {options.outputDirLocalRepo} is present and if project version contains SNAPSHOT:

    "build": {
      "executor": "@jnxplus/nx-maven:run-task",
      "inputs": [{ "runtime": "date +%s" }],
      "outputs": [
        "{options.outputDirLocalRepo}",
        "{projectRoot}/.flattened-pom.xml"
      ],
      "options": {
        "task": "install -N"
      }
    }

[khalilou88]

Here the changes I want to make: https://github.com/khalilou88/jnxplus/pull/1287

[PieterVanEde]

Thanks for your quick replies!

As for your proposed change, if I get it right you include the datetime if you encounter a SNAPSHOT version. That would always invalidate the cache, right? If I understood that correctly, that would indeed produce always correct builds.

However, it does kill all the performance gains that caching would offer. Would it be possible to solve it like this:

• Pick the rule that for each module A that depend on depencies B-Z, that if any of B-Z could not come from cache, then A itself also cannot come from the cache.
• To facilitate this, if possible, you could solve it like this:
  • For each module store the cache hash as input for the module
  • Then for the example above, if any of B-Z's hashes would change, that would mean that the input for module A also changes, which means that it could not come from the cache
  • But it also means that for all modules B-Z (all having SNAPSHOT versions) that did NOT change, that it would still resolve via cache. So we keep the performance gains from cacheing, and at the same time get always correct outputs.

I don't know if something like this is achievable, but I do see that for TS projects it seems to work roughly this way.

[khalilou88]

Unfortunately the cache is managed by nx. My fix is to provide the correct result. For performance, maybe you need to change how you manage the versions in your workspace. Maybe you can use the version 0.0.0 for all projects and use snapshot version for the project you are working on.

[PieterVanEde]

I think your fix is indeed going to always produce correct results, so it's already an improvement.

However as for keeping cacheing activated where it is valid: I dont understand how you mean your suggestion. Do I get your suggestion correct like this:

• have fixed versions on the main branch
• on a branch, change the version to snapshot for the artifacts you are working on
• change those versions back to a newer fixed version upon merging to main

This doesnt work, as then on a branch a developer might forget to change the version and end up with strange results.

But while I understand NX controls the cache, I do see this working with Typescript projects. There we also don't modify the package.json versions. But for some reason, if for instance you would replace the moduls aboven with Angular libs, then it would work. So there must be (although also not known to me) a way to achieve this.

[PieterVanEde]

Perhaps to clarify my suggestion in pseudocode. For any module A, do:

• where you currently insert a datetime to invalidate the cache, instead do these steps:
  1. fetch all modules that A depends upon (so B-Z).
  2. fetch from the NX cache the current hash for those modules (dont know if this is possible)
  3. feed the combination of those hashes as extra input (just like you inserted the datetime). For instance concatenate them as a comma-separated list.

Perhaps what I mean is a bit like this example from the NX documentation: https://github.com/nrwl/nx.git In that example they make one project dependent on changes to another project (in this case that other projects package.json). I would propose to see if that project is changed, and do that recursively until you either come to the leaves (thus nothing has changed), or you find a project that has really changed.

[PieterVanEde]

Sorry that was the wrong link, I meant https://nx.dev/extending-nx/recipes/project-graph-plugins#example.

[khalilou88]

This already done except using filesToProcess for optimization and I just opened an issue for it.

[PieterVanEde]

How do you mean that? I'm currently fiddling around wit a proprosal myself, let me create a reference to the idea I was having so far (still in pseudocode, doing this alongside other stuff atm).

[PieterVanEde]

I sorry, now I see what you mean, having a look at your PR in a moment. I have also just opened a PR (really still the rough idea), because I think you should not invalidate cache when a project as SNAPSHOT version, but instead only if it relies on a SNAPSHOT dependency.

Then even better would it be, to only invalidate the cache if such a SNAPSHOT dependency itself has changed (and do that recursively)

[khalilou88]

I got your idea, but we can make it just for a testing version. In a stable version we will calculate this variable in an intermediate step so we can calculate version just once per project. Version calculation is too expensive. But we need a reproduction of the Issue because until now it just suppositions.

[PieterVanEde]

You're right, let start indeed with a reproduction path. I've compiled the two problems in this repo: reproduction-repo. The readme in that repo contains the steps to reproduce.

I have also found a maven hash plugin that we could in deed call once per execution, although it is unfortunately quite costly. Maybe something to opt-in to?

[PieterVanEde]

While I was trying to implement a fix, I ran across this piece of documentation: nx. Might this not be the thing we need?

[PieterVanEde]

That was indeed the solution. For others that have the same problem, I fixed it by adding this section in nx.json's targetDefaults:

"@jnxplus/nx-maven:run-task": {
      "inputs": [
        "default",
        { "dependentTasksOutputFiles": "**/*", "transitive": true },
        {
          "externalDependencies": []
        }
      ],
      "cache": true
    },

The important part here is dependentTasksOutputFiles and it's transitive option.

I you agree @khalilou88, then I think your change is not needed, but instead this should be added to the documentation for those that have snapshot versioning in their repo.

[PieterVanEde]

I've just raised a PR to update the documentation

[khalilou88]

I don't know about dependentTasksOutputFiles but I think maybe you need just to verify one type of files:

{ "dependentTasksOutputFiles": "**/maven-metadata*.xml", "transitive": true }

https://maven.apache.org/ref/3.8.8/maven-repository-metadata/

[PieterVanEde]

If I read the docs on maven-metadata correctly, that still only applies to more or less the effective pom. But suppose I have a module that contains a class that tells me if it rains outside, and I modify the implementation, so that it tells me if it is cold outside.

Then the metadata wont change (because the effective pom does not change), but downstream test and build results will incorrectly fetch the previous implementation from cache.

[khalilou88]

Just checked your reproduction repo, you use old versions. At least you need to use last version of nx 18 and last version of nx-maven before version 1.

[PieterVanEde]

I do use old versions, as I am using this in a corporate setting where I cannot upgrade further at the moment due to peer dependency requirements. I think that the issue however is not different with newer versions.

[khalilou88]

I made a test with your reproduction repo and I don't see the issue you mentioned about lib-d. If you see the log nx will build lib-d and put it in the cache, and will use the cache for future build. Seeing the log is not enough to tell that the app contains an old version of lib-d. What you are doing with dependentTasksOutputFiles is building lib-d over and over....

To build lib-d over and over, you don't need dependentTasksOutputFiles, just remove localRepoRelativePath from nx.json

[khalilou88]

I made changes in your repo to show you that there is no issue with nx cache: https://github.com/PieterVanEde/jnxplus-reproduction-snapshot-cache/pull/1 I removed build from serve app target to not build app again.

[PieterVanEde]

I am afraid that I have to disagree, the logs really do show what is going wrong. So I rewrote my reproduction a little to the toplevel app now produces a WAR, so we can reason about the contents of the WAR instead of the logs. This is now pushed to the main branch of my reproduction-repo. Could you verify if you agree that there still are issues, that can be fixed by the dependentTaskOutputFiles fix?

[khalilou88]

@PieterVanEde yes you are, nx build the app project but don't update the cache because "existing outputs match the cache". I don't think this is just for snapshot but all versions!

[khalilou88]

@PieterVanEde I made a test with repo https://github.com/khalilou88/jnxplus-examples, I didn't reproduce the issue

[PieterVanEde]

I think I've found the culprit, and it's due to a wrong change while setting up my repo a long time ago. I have overwritten the inputs in the targetDefaults section for some reason. That is also present in my reproduction repo. If I remove that, then the issue does not appear.

So sorry for bothering here, appearantly I have introduced this problem myself 😞