Reproducer for #335 fix #2: parse-asymmetry should fall back to simple hash diff#352
Open
tinder-maxwellelliott wants to merge 1 commit into
Open
Reproducer for #335 fix #2: parse-asymmetry should fall back to simple hash diff#352tinder-maxwellelliott wants to merge 1 commit into
tinder-maxwellelliott wants to merge 1 commit into
Conversation
When the two module-graph JSON payloads passed to get-impacted-targets are not byte-equal but one of them fails to parse, findChangedModules(emptyMap, fullMap) reports every module in the populated graph as "added". The impacted set then explodes -- either fanning out into one rdeps subprocess per matched repo (the multi-hour fan-out in #335) or, when no BazelQueryService is bound, falling through to allTargets.keys so every hashed label is reported as impacted. Both outcomes are far worse than a per-target hash diff. PR #336 made the parser tolerant of stderr-polluted JSON, which covers the historical 18.0.x base graph case, but the asymmetry remains a real failure mode for genuinely unparseable input (corrupted base graphs, truncation, future serialization format changes). The reproducer adds two @ignore'd tests in CalculateImpactedTargetsInteractorTest covering both code paths that branch on changedModules.isNotEmpty(): execute() and executeWithDistances(). Both feed a non-JSON fromGraph and a parseable toGraph and assert that only the actually-hash-changed target appears in the impacted set. Today both tests fail (every target is reported); the @ignore'd state keeps CI green until fix #2 lands. Generated with Claude Code (https://claude.com/claude-code)
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
Adds two
@Ignored reproducer tests for #335 fix #2 (the "fall back tocomputeSimpleImpactedTargetswhen parse asymmetry is detected" proposal in the issue body).Today, when the two module-graph JSON payloads passed to
get-impacted-targetsdiffer but one of them fails to parse,CalculateImpactedTargetsInteractor.detectChangedModulesreturns the full set of modules from the parseable side viafindChangedModules(emptyMap, fullMap). Every module is then treated as "added". The impacted set explodes in one of two ways:BazelQueryServicebound, every "added" module fans out into anrdepsquery against its canonical repo(s). On the workspace in Upgrading 18.0.x → 18.1.0 with a reused base graph triggers multi-hourqueryTargetsDependingOnModulesfan-out #335 this produced ~5,000 serial subprocesses and the run took multiple hours.BazelQueryServicebound (or one that errors), the failure-tolerant fallback atCalculateImpactedTargetsInteractor.kt:321-323returnsallTargets.keys— every hashed label is reported as impacted.Both outcomes are far worse than the per-target hash diff that would have run if no module graph had been supplied. PR #336 made the parser tolerant of stderr-polluted JSON, which covers the historical 18.0.x base graph case, but the asymmetry remains a real failure mode for any genuinely unparseable input (corrupted base graphs pulled from object storage, truncation, a future
bazel mod graphserialisation change).The reproducer
Two
@Ignored tests incli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.ktcover both code paths that branch onchangedModules.isNotEmpty():execute_parseAsymmetryFallsBackToSimpleHashDiff_reproducerForIssue335Fix2— exercisesexecute()(the defaultget-impacted-targetspath).executeWithDistances_parseAsymmetryFallsBackToSimpleHashDiff_reproducerForIssue335Fix2— exercisesexecuteWithDistances()(the--depsFilepath).Both feed:
fromModuleGraphJsonwith no{at all ("garbage-non-json-payload") soModuleGraphParser.parseModuleGraphfalls through toif (start < 0) return emptyMap().toModuleGraphJsonthat parses cleanly to one or two modules.startHashes/endHashespair where only//:changedhas actually changed.Each test asserts that the impacted set is exactly
{//:changed}. Today both fail with:The
@Ignored state keeps CI green; the annotations document the issue. Drop@Ignoreonce an asymmetry-detection fallback lands (e.g. "ifparseModuleGraphreturns an empty map for one side and a non-empty map for the other, skipqueryTargetsDependingOnModulesand fall through tocomputeSimpleImpactedTargets").Verification
bazel test //cli:CalculateImpactedTargetsInteractorTest— PASSED (new tests are@Ignored and skipped; existing 19 tests still pass).@Ignoreannotations and re-ran — FAILED with the assertion above, confirming the reproducer catches the bug.Test plan
bazel test //cli:CalculateImpactedTargetsInteractorTest— passes with@Ignored.@Ignore'd and re-ran — both tests fail with the expected "every target reported" assertion error.@Ignorefrom both tests and re-run — both should pass.Generated with Claude Code (https://claude.com/claude-code)