skyframe: report top-level aspect failures - #29850
Closed
sluongng wants to merge 1 commit into
Closed
Conversation
sluongng
marked this pull request as ready for review
June 16, 2026 17:00
github-actions
Bot
added
team-Core
sluongng
force-pushed
the
sluongng/fix-output-conflict-events
branch
4 times, most recently
from
June 17, 2026 09:01
4e4bfd1 to
a445d7b
Compare
joeleba
approved these changes
Jun 19, 2026
joeleba
left a comment
joeleba
left a comment
Member
There was a problem hiding this comment.
Thanks for the fix. This entire error processing area is plagued by legacy code and needs some reworking. It's on my TODO list but I just need some time to work on it...
joeleba
added
awaiting-PR-merge
Contributor
|
Please look into the failing checks. |
OutputArtifactConflictTest flakes under Skymeld + nokeep_going because
evaluation can abort with a TopLevelAspectsKey as the only processed
analysis error. A local stress run on origin/master reproduced it as:
FAIL: ...OutputArtifactConflictTest (shard 3 of 3, run 32 of 50)
testConflictErrorAndUnfinishedAspectAnalysis_mergedAnalysisExecution
[skymeld=true,minimizeMemory=true,keepGoing=false]
expected to contain any of:
[//x:y, //x/y:y, //x:fail_analysis]
but was : []
The existing error processor skipped non-configured-target keys there.
BEP consumers could miss the AnalysisFailureEvent even though the
analysis error was reported to the event handler.
Report failures against the top-level aspect's base configured target so
the failure is emitted on the same path used for configured target keys.
Update the BEP integration test expectation for the additional aborted
event that is now reported.
sluongng
force-pushed
the
sluongng/fix-output-conflict-events
branch
from
June 19, 2026 11:18
a445d7b to
8c88070
Compare
Contributor
Author
|
I don’t think the failure was related so i just rebased |
Contributor
Author
|
@joeleba @deepalak56 would appreciate if somebody could help me import this PR |
Member
|
It's in progress. Thanks for your patience. |
github-actions
Bot
removed
the
awaiting-PR-merge
Collaborator
|
@bazel-io fork 9.2.0 |
Collaborator
|
@bazel-io fork 8.8.0 |
This was referenced Jun 23, 2026
bazel-io
added a commit
to bazel-io/bazel
that referenced
this pull request
Jul 3, 2026
…bazelbuild#29951) OutputArtifactConflictTest flakes under Skymeld + nokeep_going because evaluation can abort with a TopLevelAspectsKey as the only processed analysis error. A local stress run on origin/master reproduced the flake with: ```text bazel test --nocache_test_results --runs_per_test=50 \ --test_filter=testConflictErrorAndUnfinishedAspectAnalysis_mergedAnalysisExecution \ //src/test/java/com/google/devtools/build/lib/buildtool:OutputArtifactConflictTest ``` The failing run looked like this: ```text FAIL: ...OutputArtifactConflictTest (shard 3 of 3, run 32 of 50) testConflictErrorAndUnfinishedAspectAnalysis_mergedAnalysisExecution [skymeld=true,minimizeMemory=true,keepGoing=false] expected to contain any of: [//x:y, //x/y:y, //x:fail_analysis] but was : [] ``` The existing error processor skipped non-configured-target keys in this path, so BEP consumers could miss the AnalysisFailureEvent even though the analysis error was reported to the event handler. Report failures against the top-level aspect's base configured target so the failure is emitted on the same path used for configured target keys. Update the BEP integration test expectation for the additional aborted event that is now reported. Local validation: - Reproduced the baseline flake on origin/master (a300dd5): failed on run 32/50 with empty `analysisFailures`. - Verified the CopyOnWriteArrayList-only test-listener change still failed on run 10/50 with the same empty-list assertion. - Verified this patch without the listener-list change passed 50/50 and 100/100 local stress runs of the same filtered test. Closes bazelbuild#29850. PiperOrigin-RevId: 936417007 Change-Id: If2ea9a4b157fb6d646608a8adfb2c5f4d12de5c4 Commit bazelbuild@8d8826c Co-authored-by: Son Luong Ngoc <sluongng@gmail.com>
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
4 participants
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.
OutputArtifactConflictTest flakes under Skymeld + nokeep_going because
evaluation can abort with a TopLevelAspectsKey as the only processed
analysis error.
A local stress run on origin/master reproduced the flake with:
The failing run looked like this:
The existing error processor skipped non-configured-target keys in this
path, so BEP consumers could miss the AnalysisFailureEvent even though the
analysis error was reported to the event handler.
Report failures against the top-level aspect's base configured target so
the failure is emitted on the same path used for configured target keys.
Update the BEP integration test expectation for the additional aborted
event that is now reported.
Local validation:
(a300dd5): failed on run 32/50
with empty
analysisFailures.on run 10/50 with the same empty-list assertion.
100/100 local stress runs of the same filtered test.