AlexanderAshitkin commented on PR #534: URL: https://github.com/apache/maven-build-cache-extension/pull/534#issuecomment-6136539969
I agree that the reproduced invalidation issue requires a fix. However, I have several concerns with the proposed implementation that I'd like resolved before merging. ### 1. Maven already defines and controls acceptable SNAPSHOT staleness Maven does not guarantee that a build uses the latest available SNAPSHOT. It deliberately trades freshness for resolution cost and makes that trade-off configurable through settings `<snapshots><updatePolicy>`: - `always` — check for updates on every resolution - `daily` — check once per day (the default) - `interval:X` — check at a configured interval - `never` — do not check for updates Additionally, `-U` forces update checks, while `-o` disables remote resolution. Maven persists update-check state in `resolver-status.properties`, avoiding unnecessary repository checks within the configured window. Consequently, building against a day-old `leaf-1.0-SNAPSHOT` is valid Maven behavior, not a defect. **The cache should preserve Maven's resolution semantics, not introduce a stricter freshness guarantees.** The correct invariant is not "outputs must match the latest artifact available in the repository," which Maven itself does not promise, but rather "outputs must match the inputs Maven actually resolved for this invocation." For example: - With `updatePolicy=never`, Maven continues using the previously resolved SNAPSHOT. The effective inputs remain unchanged, so a cache hit is correct. - With `-U`, Maven checks for updated SNAPSHOTs. If a dependency changes, its checksum must reflect that change. The cache therefore does not need a separate freshness policy. The acceptable staleness window is already configured per repository by the build's owner. This also addresses the resolution-cost concern: `resolveArtifact` respects Maven's update policy and does not repeatedly download artifacts within the configured window. Forcing offline collection offers no meaningful advantage here. ### 2. Suggested implementation: track resolved mutable dependencies Rather than introducing a separate fingerprint of the mediated dependency graph, I suggest expanding the existing mutable-dependency input set with the SNAPSHOT artifacts produced by Maven's dependency resolution, and store it within the effective pom. If the cached buildinfo contains the leaf snapshot dependency, the App will be properly invalidated. Specifically: - **Reactor dependencies:** Continue using their project checksums. - **External SNAPSHOT dependencies:** Use the existing `resolveArtifact` mechanism and hash the resolved artifact files. - **SNAPSHOTs:** Include them in the tracked inputs or the effective pom entry. This is the missing input in the current implementation. This approach naturally accounts for dependency management, exclusions, scope mediation, and `<type>pom</type>` expansion because it tracks the results of Maven's resolution rather than attempting to reconstruct or independently fingerprint dependency graph semantics. Using the original `session.getRepositorySession()` instead of the forced-offline wrapper also preserves Maven's existing update behavior, including `-U`, without additional policy logic. It also eliminates the need for the proposed "incomplete graph" state and its random-UUID checksum marker. If dependency collection fails, the build will fail shortly afterward during `ensureDependenciesAreResolved` anyway. Introducing a separate fail-closed mechanism into cache key generation provides no useful recovery path. ### 3. Blocking concern: phase-dependent checksums I cannot support the following change: ```java String key = BuilderCommon.getKey(project) + "|testDependencies=" + includeTestDependencies; ``` **Identical resolved inputs must produce identical checksums, independently of the requested lifecycle phase.** Beyond violating that invariant, this change breaks partial cache restoration. `CacheControllerImpl` maintains one entry per checksum and progressively extends it using `getHighestCompletedGoal()` and `isLaterPhaseThanBuild`. Consequently, an entry created by `mvn compile` must remain discoverable when a subsequent invocation runs `mvn test-compile`. Including `includeTestDependencies` in the key makes those entries incompatible. Partial restoration across these invocations becomes impossible because the checksum changes before the existing cache entry can be found. There are additional consequences: - `shouldIncludeTestDependencies` can change the key based on whether a `MojoDescriptor` was successfully resolved. - CI builds running `verify` can no longer share remote cache entries with developers running `compile`, even when the relevant inputs are identical. - The existing progressive cache restoration model becomes fragmented by lifecycle phase. This is a regression in a fundamental cache invariant and **must be reverted before merging**. ### 4. Keep the PR focused on SNAPSHOT input tracking I suggest limiting this PR to correcting SNAPSHOT dependency tracking. The proposed graph-shape fingerprint, fail-closed behavior, and test-scope selectivity introduce separate design decisions with broader implications for cache semantics. These should be tracked as independent issues and evaluated on their own merits rather than bundled into this fix. I would also remove two test cases: **`releaseDependency`** This scenario relies on a release artifact depending on a SNAPSHOT. Such dependencies are incompatible with standard release practices: `maven-release-plugin` rejects them during release preparation, and `enforcer:requireReleaseDeps` can explicitly prohibit them. The cache intentionally treats release artifacts as immutable. Changing that assumption to accommodate this scenario is outside the scope of SNAPSHOT invalidation and should not be presented as a defect addressed by this PR. **`testScopedTransitiveSnapshotOnlyInvalidatesTestLifecycle`** This test passes on `master` for an unrelated reason. The cached entry never progresses beyond `compile`, so `testCompile` executes again and picks up the updated transitive dependency. As a result, the test does not actually establish that test-scoped transitive SNAPSHOT changes selectively invalidate the test lifecycle. Its passing result is incidental rather than evidence of the behavior its name claims to verify. ### Summary I agree that the reproduced invalidation issue requires a fix. My concern is with the additional cache semantics introduced by this implementation, not with the underlying bug. The preferred solution is to track the actual mutable inputs produced by Maven's dependency resolution, including transitive SNAPSHOT dependencies, while preserving Maven's existing update policies and the cache's phase-independent checksum model. That would address the demonstrated defect without introducing a parallel dependency fingerprint, a separate freshness policy, or regressions in partial cache restoration. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
