Fix #268 InvalidDependencyEdgesException now warns + reports distance 0#341
Merged
Merged
Conversation
…argetType
Users who pass `--targetType=Rule` to `generate-hashes` end up with hash
JSONs that only contain Rule entries -- SourceFile and GeneratedFile rows
are stripped at write time. When that JSON is fed back into
`get-impacted-targets --depsFile=...` (which routes through
`executeWithDistances` -> `computeAllDistances`), an indirectly impacted
Rule whose only changed dependency is a filtered-out GeneratedFile
triggers:
InvalidDependencyEdgesException("<label> was indirectly impacted, but has no impacted dependencies.")
The whole job crashes. The user in #268 narrowed the cause to exactly this
combination: `--targetType=Rule` plus a Rule that depends on a
GeneratedFile in their autogenerated graph. The asked-for fix is to
downgrade this to a warning (with a conservative distance like 0) so the
indirectly impacted Rule still appears in the impacted-targets output
instead of taking the whole CI down.
This adds a @ignore'd unit reproducer that asserts computeAllDistances
does not throw and the Rule still appears in the result. Drop the @ignore
once the fix lands.
Refs #268
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Users who pass `--targetType=Rule` to `generate-hashes` end up with
hash JSONs that only contain Rule entries -- SourceFile and
GeneratedFile rows are stripped at write time. When that JSON was fed
back into `get-impacted-targets --depsFile=...` (which routes through
executeWithDistances -> computeAllDistances), an indirectly impacted
Rule whose only changed dep was a filtered-out GeneratedFile triggered:
InvalidDependencyEdgesException("<label> was indirectly impacted, but has no impacted dependencies.")
and crashed the whole job. @agustinmista narrowed the cause to exactly
this combination in the issue thread.
Fix: replace both `throw InvalidDependencyEdgesException(...)` sites in
CalculateImpactedTargetsInteractor.calculateDistance with a logger.w()
that points at --targetType as the most likely cause, and return
TargetDistanceMetrics(0, 0). The indirectly impacted target still
appears in the impacted-targets output rather than crashing the run.
Two code paths now warn-and-fallback instead of throw:
- The target has no entry in the dep-edges file at all.
- The target has dep-edges entries, but none of those deps are
themselves in the impacted-labels map.
The previously-thrown @VisibleForTesting nested class
`InvalidDependencyEdgesException` is no longer referenced; removed
along with its (now unused) `com.google.common.annotations.VisibleForTesting`
import.
testInvalidEdgesRaises updated and renamed to
testInvalidEdgesFallsBackToDistanceZero -- it now asserts both fallback
paths return distance 0. The reproducer (added in the previous commit)
loses its @ignore and is renamed to a regression test.
Refs #268
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1975f8c to
49b9663
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #268.
Root cause. Users who pass
--targetType=Ruletogenerate-hashesend up with hash JSONs that only containRuleentries —SourceFileandGeneratedFilerows are stripped at write time. When that JSON was fed back intoget-impacted-targets --depsFile=...(which routes throughexecuteWithDistances→computeAllDistances), an indirectly impactedRulewhose only changed dep was a filtered-outGeneratedFiletriggered:and crashed the whole job. @agustinmista narrowed the cause to exactly this combination in the issue thread.
Fix. Both
throw InvalidDependencyEdgesException(...)sites inCalculateImpactedTargetsInteractor.calculateDistanceare replaced with alogger.w()that points at--targetTypeas the most likely cause, plus a return ofTargetDistanceMetrics(0, 0). The indirectly impacted target still appears in the impacted-targets output rather than crashing the run.Two code paths now warn-and-fallback instead of throw:
The previously-thrown
@VisibleForTestingnested classInvalidDependencyEdgesExceptionis no longer referenced, so it's removed along with its (now unused)com.google.common.annotations.VisibleForTestingimport.Tests.
testInvalidEdgesRaisesbecomestestInvalidEdgesFallsBackToDistanceZeroand now asserts both fallback paths return distance 0. The reproducer added in the first commit on this branch loses its@Ignoreand is renamed to a regression test — it sets up the exact--targetType=Rule+ GeneratedFile-in-depEdges-but-not-in-hashes scenario from the issue and asserts the call succeeds with//pkg:rulereported at distance 0.Test plan
bazel build //cli:cli-test-lib— clean.bazel test //cli:CalculateImpactedTargetsInteractorTest --cache_test_results=no→ PASSED in 1.3s (all tests in the class, including the un-@Ignored reproducer and the rewrittentestInvalidEdgesFallsBackToDistanceZero).kt_jvm_testtargets → all 12 PASSED.🤖 Generated with Claude Code