Filter spawn strategies by execution platform #13
Conversation
…-for-exec-platform
…-for-exec-platform
WalkthroughThis update introduces execution platform-based filtering for spawn strategies within the build system. A new command-line option allows users to specify allowed strategies per execution platform. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BazelStrategyModule
participant ExecutionOptions
participant SpawnStrategyRegistry.Builder
participant SpawnStrategyRegistry
participant Spawn
User->>ExecutionOptions: Set --allowed_strategies_by_exec_platform
ExecutionOptions->>BazelStrategyModule: Provide platform-strategy map
BazelStrategyModule->>SpawnStrategyRegistry.Builder: addExecPlatformFilter(Label, List<String>)
SpawnStrategyRegistry.Builder->>SpawnStrategyRegistry: Build with platform filters
User->>Spawn: Request execution
Spawn->>SpawnStrategyRegistry: getStrategies(spawn, handler)
SpawnStrategyRegistry->>SpawnStrategyRegistry: Apply mnemonic/regex filters
SpawnStrategyRegistry->>SpawnStrategyRegistry: Apply platform filter (by execution platform label)
SpawnStrategyRegistry-->>Spawn: Return filtered strategies
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces a new flag, --allowed_strategies_by_exec_platform, to filter spawn strategies based on the execution platform. This is part of the implementation for the Execution Platform Scoped Spawn Strategies proposal. The changes modify how spawn strategies are resolved, incorporating the new flag to allow or disallow certain strategies for specific execution platforms. The goal is to provide more fine-grained control over where actions are executed, for example, preventing remote execution for host platform actions.
Highlights
- New Flag: Introduces the
--allowed_strategies_by_exec_platformflag to filter spawn strategies based on the execution platform. - Strategy Filtering: Implements filtering logic within the
SpawnStrategyRegistryto consider the allowed strategies for a given execution platform. - Error Reporting: Updates the error message when no valid spawn strategies are available after filtering, indicating the potential impact of the new flag.
- Remote Fallback: Modifies the
RemoteLocalFallbackRegistryto consider the execution platform when selecting a fallback strategy.
Changelog
Click here to see the changelog
- src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java
- Modifies the condition for checking empty execution platform properties to include checks for both
execPropertiesandremoteExecutionProperties.
- Modifies the condition for checking empty execution platform properties to include checks for both
- src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java
- Adds a new dependency on
com.google.devtools.build.lib.cmdline.Label. - Adds logic to register execution platform filters from the
--allowed_strategies_by_exec_platformflag.
- Adds a new dependency on
- src/main/java/com/google/devtools/build/lib/exec/BUILD
- Adds a dependency on
//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options. - Adds a dependency on
//src/main/java/com/google/devtools/build/lib/cmdline.
- Adds a dependency on
- src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java
- Introduces the
--allowed_strategies_by_exec_platformoption, allowing users to specify allowed strategies for different execution platforms.
- Introduces the
- src/main/java/com/google/devtools/build/lib/exec/RemoteLocalFallbackRegistry.java
- Modifies the
getRemoteLocalFallbackStrategymethod to accept aSpawnparameter. - Updates the
RemoteLocalFallbackRegistryinterface to pass theSpawntogetRemoteLocalFallbackStrategy.
- Modifies the
- src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
- Adds a
StrategyPlatformFilterto filter strategies based on the execution platform. - Modifies the
getStrategiesmethods to incorporate platform-based filtering. - Updates the
getRemoteLocalFallbackStrategymethod to use the platform filter. - Adds logging for platform-specific strategy filters.
- Adds
addExecPlatformFilterto theBuilderclass to allow adding platform filters. - Adds a new inner class
StrategyPlatformFilterto handle filtering strategies based on execution platform.
- Adds a
- src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java
- Updates the error message when no valid spawn strategies are available to include the
--allowed_strategies_by_exec_platformflag.
- Updates the error message when no valid spawn strategies are available to include the
- src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
- Updates the call to
getRemoteLocalFallbackStrategyto pass theSpawnobject.
- Updates the call to
- src/test/java/com/google/devtools/build/lib/exec/BUILD
- Adds a dependency on
//src/main/java/com/google/devtools/build/lib/analysis/platform.
- Adds a dependency on
- src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java
- Adds a test case for platform-based strategy filtering.
- Updates tests to pass a
Spawnobject togetRemoteLocalFallbackStrategy.
- src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java
- Modifies the
FakeOwnerconstructor to includePlatformInfo.EMPTY_PLATFORM_INFOby default.
- Modifies the
- src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
- Updates the
ImmutableClassToInstanceMapto accept a function that takes aSpawnobject.
- Updates the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A flag to filter,
Strategies align by platform,
Control takes its hold.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new flag --allowed_strategies_by_exec_platform to filter spawn strategies based on the execution platform. The implementation involves adding a new filter StrategyPlatformFilter to the SpawnStrategyRegistry and updating the logic to retrieve spawn strategies. The changes seem well-structured and the provided examples are helpful. However, there are a few areas that could be improved, such as adding a null check for platformLabel and improving the error message in the Preconditions.checkNotNull call.
Summary of Findings
- Missing null check: The code directly dereferences
platformLabelwithout checking if it's null, which could lead to aNullPointerException. A null check should be added before accessingplatformLabel. - Preconditions check message: The message in the
Preconditions.checkNotNullcall inStrategyPlatformFilteris not very informative. It should be updated to provide more context about why the execution platform is required. - Inconsistent use of var: The code uses
varin some places and explicit types in others. It would be more consistent to use eithervaror explicit types throughout the code. - Logging level: The log messages in
SpawnStrategyRegistryare at the INFO level, which might be too verbose. Consider using a more appropriate logging level, such as DEBUG.
Merge Readiness
The pull request is not ready for merging. There are a few issues that need to be addressed before merging, such as adding a null check for platformLabel and improving the error message in the Preconditions.checkNotNull call. I am unable to directly approve this pull request, and users should have others review and approve this code before merging.
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java (1)
125-141: Help text and converter mismatch can confuse users
allowed_strategies_by_exec_platformis declared withStringToStringListConverter, which expects key=value1,value2 syntax, but the help text shows multi-line examples and back-ticks.
Two small issues:
- Consistency – every other flag uses a regular string, not a text-block. Sticking to one style avoids noise in
--help.- Ambiguity – mention explicitly that the option can be repeated and that the right-hand side is comma-separated.
No code breakage, just documentation polish.
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java (1)
630-668:StrategyPlatformFilter– performance & correctness tweaks
allowedStrategies.contains(strategy)is O(n) over anImmutableList.
Convert to anImmutableSetonce in the constructor for O(1) lookups.- The overload that accepts an
ImmutableCollectionconverts vianewCopyOnWriteArrayList, incurring unnecessary concurrency overhead. A plainArrayListsuffices.Not blocking, but an easy micro-optimisation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java(1 hunks)src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java(2 hunks)src/main/java/com/google/devtools/build/lib/exec/BUILD(2 hunks)src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java(1 hunks)src/main/java/com/google/devtools/build/lib/exec/RemoteLocalFallbackRegistry.java(2 hunks)src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java(14 hunks)src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java(1 hunks)src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java(1 hunks)src/test/java/com/google/devtools/build/lib/exec/BUILD(1 hunks)src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java(4 hunks)src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java(1 hunks)src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java (1)
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java (1)
SpawnStrategyRegistry(65-743)
🔇 Additional comments (12)
src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java (1)
82-88: Improved platform safety by replacing null with EMPTY_PLATFORM_INFOThis change ensures consistency with the broader execution platform-aware filtering being implemented. By explicitly providing
PlatformInfo.EMPTY_PLATFORM_INFOinstead of implicitly passingnull, the code prevents potential NullPointerExceptions and aligns with the execution platform-based spawn strategy filtering introduced in this PR.src/main/java/com/google/devtools/build/lib/exec/RemoteLocalFallbackRegistry.java (1)
17-18: Updated interface to support platform-specific strategy selectionThe method signature change for
getRemoteLocalFallbackStrategynow includes aSpawnparameter, allowing fallback strategies to be selected based on execution platform information associated with each spawn. This change properly aligns with the new execution platform-based strategy filtering functionality.Also applies to: 32-33
src/main/java/com/google/devtools/build/lib/exec/BUILD (2)
107-110: Added dependency for new options functionalityAdding the
config/fragment_optionsdependency to theexecution_optionstarget supports the new command-line option--allowed_strategies_by_exec_platformthat filters spawn strategies by execution platform.
367-369: Added dependency for command-line handlingAdding the
cmdlinedependency to thespawn_strategy_registrytarget enables parsing and handling of new command-line options for execution platform-specific strategy filtering.src/test/java/com/google/devtools/build/lib/exec/BUILD (1)
47-49: Added platform dependency for testing execution platform filteringAdding the
analysis/platformdependency enables tests to properly validate the new execution platform-based strategy filtering functionality.src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java (1)
89-94: Clean error message update with new flag information.The error message was updated to include the new
--allowed_strategies_by_exec_platformflag, which is consistent with the PR objective of implementing platform-scoped spawn strategies. Using a text block (triple quotes) also improves readability of the code.src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java (1)
21-21: LGTM - Necessary import added.Added import for Label class which is needed for the new platform filtering functionality.
src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java (1)
558-558: Updated method call to pass spawn parameter.The method call is correctly updated to pass the spawn instance to the
getRemoteLocalFallbackStrategymethod, aligning with the new API that enables platform-aware fallback strategy selection.src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java (1)
267-267: Test correctly updated to match the new API.The lambda expression now accepts a Spawn parameter (
_spawn), properly matching the updated method signature inRemoteLocalFallbackRegistry. The underscore prefix is a common convention indicating the parameter is not used in the lambda body.src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java (1)
79-84: Early-return guard looks good, just double-check the cost ofremoteExecutionProperties()The additional check correctly treats execution platforms that have no
execPropertiesand noremoteExecutionPropertiesthe same as “no execution platform”.
BecauseremoteExecutionProperties()returns a (possibly large) string, consider calling it only once and caching the result in a local variable if this method ever appears on a hot path.
Otherwise the change is ✔️.src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java (1)
174-178:⚠️ Potential issue
getRemoteLocalFallbackStrategycan NPE / require JDK-21
List#getFirst()is only available since JDK 21.
On older LTS releases (JDK 11/17) this method does not exist, causing compilation failure.It is also unsafe when
remoteLocalFallbackStrategy == nulland is filtered-out.- return strategyPlatformFilter.getStrategies( - spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) - .getFirst(); + var filtered = + strategyPlatformFilter.getStrategies( + spawn, Lists.newArrayList(remoteLocalFallbackStrategy)); + return filtered.isEmpty() ? null : filtered.get(0);Likely an incorrect or invalid review comment.
src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java (1)
305-321: Test may hide real-world null-platform issues
createSpawnWithMnemonicAndDescriptionproduces aSimpleSpawnwithout an execution platform, yet the test passes becausePlatformInfo.EMPTY_PLATFORM_INFOhappens to be returned.
Consider adding a second assertion with a trulynullplatform (or adapting the production code per earlier comment) to avoid a false sense of security.
src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Solid Implementation - Platform-Based Filtering Needs Reliability Improvements!
|
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Platform-specific Spawn Strategy Implementation👍 Well Done
📌 Files Processed
|
| public <T extends SpawnStrategy> List<T> getStrategies( | ||
| Spawn spawn, List<T> candidateStrategies) { | ||
| var platformLabel = spawn.getExecutionPlatformLabel(); | ||
| Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); |
There was a problem hiding this comment.
Null pointer risk in platform label validation
The null check for platformLabel will throw NPE when a spawn has no execution platform. This contradicts the existing code in getPlatformProto that handles null execution platforms gracefully.
| Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); | |
| if (platformLabel == null) { | |
| return candidateStrategies; | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- SRE-Graceful-Degradation
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | ||
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | ||
| .getFirst(); |
There was a problem hiding this comment.
NoSuchElementException risk in remote fallback strategy
If remoteLocalFallbackStrategy is null, Lists.newArrayList will create an empty list, and calling getFirst() on an empty list will throw NoSuchElementException, causing runtime failures.
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | |
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | |
| .getFirst(); | |
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | |
| if (remoteLocalFallbackStrategy == null) { | |
| return null; | |
| } | |
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | |
| .getFirst(); |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- SRE-Defensive-Programming
|
/refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: SpawnStrategyRegistry👍 Well Done
📁 Selected files for review (12)
🎯 Custom Instructions
|
| public <T extends SpawnStrategy> List<T> getStrategies( | ||
| Spawn spawn, List<T> candidateStrategies) { | ||
| var platformLabel = spawn.getExecutionPlatformLabel(); | ||
| Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); |
There was a problem hiding this comment.
Missing Null Check
Code directly calls checkNotNull without first checking if platformLabel is null. This violates the repository guideline to check for null before using Preconditions.checkNotNull. This could lead to unnecessary exceptions when a simple null check could handle the case more gracefully.
if (platformLabel == null) {
return candidateStrategies;
}
Commitable Suggestion
| Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); | |
| if (platformLabel == null) { | |
| return candidateStrategies; | |
| } |
Standards
- Repo-Guideline-check weather null check condition applied for platformlabel before Preconditions.checkNotNull
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | ||
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | ||
| .getFirst(); |
There was a problem hiding this comment.
Potential NPE Risk
The method returns the first element from filtered strategies without checking if the list is empty. If all strategies are filtered out, getFirst() will throw a NoSuchElementException. This creates a reliability risk when no strategies match the platform filter criteria.
public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) {
if (remoteLocalFallbackStrategy == null) {
return null;
}
List<AbstractSpawnStrategy> filteredStrategies = strategyPlatformFilter.getStrategies(
spawn, Lists.newArrayList(remoteLocalFallbackStrategy));
return filteredStrategies.isEmpty() ? null : filteredStrategies.get(0);
Commitable Suggestion
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | |
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | |
| .getFirst(); | |
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | |
| if (remoteLocalFallbackStrategy == null) { | |
| return null; | |
| } | |
| List<AbstractSpawnStrategy> filteredStrategies = strategyPlatformFilter.getStrategies( | |
| spawn, Lists.newArrayList(remoteLocalFallbackStrategy)); | |
| return filteredStrategies.isEmpty() ? null : filteredStrategies.get(0); |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- SRE-Error-Handling
| "FilterDescriptionToStrategyImplementations: \"%s\" = [%s]", | ||
| entry.getKey(), toImplementationNames(value)); |
There was a problem hiding this comment.
Inappropriate Logging Level
The code uses logger.atInfo() for logging filter strategy implementations. According to repository guidelines, a more appropriate logging level should be considered for this type of diagnostic information, likely debug level would be more suitable for detailed implementation mappings.
Standards
- Repo-Guideline-Consider using a more appropriate logging level
|
/refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: SpawnStrategyRegistry Platform Filtering👍 Well Done
📁 Selected files for review (12)
🎯 Custom Instructions
📝 Additional Comments
|
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | ||
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | ||
| .getFirst(); |
There was a problem hiding this comment.
Potential NullPointerException
The method returns the first element from filtered strategies without checking if remoteLocalFallbackStrategy is null or if the filtered list is empty. This could lead to NullPointerException when remoteLocalFallbackStrategy is null or when all strategies are filtered out.
public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) {
if (remoteLocalFallbackStrategy == null) {
return null;
}
List<AbstractSpawnStrategy> filteredStrategies = strategyPlatformFilter.getStrategies(
spawn, Lists.newArrayList(remoteLocalFallbackStrategy));
return filteredStrategies.isEmpty() ? null : filteredStrategies.get(0);
Commitable Suggestion
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | |
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | |
| .getFirst(); | |
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | |
| if (remoteLocalFallbackStrategy == null) { | |
| return null; | |
| } | |
| List<AbstractSpawnStrategy> filteredStrategies = strategyPlatformFilter.getStrategies( | |
| spawn, Lists.newArrayList(remoteLocalFallbackStrategy)); | |
| return filteredStrategies.isEmpty() ? null : filteredStrategies.get(0); |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| public <T extends SpawnStrategy> List<T> getStrategies( | ||
| Spawn spawn, List<T> candidateStrategies) { | ||
| var platformLabel = spawn.getExecutionPlatformLabel(); | ||
| Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); |
There was a problem hiding this comment.
Missing Null Check
Null check is performed using Preconditions.checkNotNull without first checking if platformLabel is null. This violates the repository guideline requiring a null check before using Preconditions.checkNotNull. This could lead to unnecessary exceptions when a simple null check could handle the case more gracefully.
if (platformLabel == null) {
throw new NullPointerException("Attempting to spawn action without an execution platform.");
}
Commitable Suggestion
| Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); | |
| if (platformLabel == null) { | |
| throw new NullPointerException("Attempting to spawn action without an execution platform."); | |
| } |
Standards
- Repo-Guideline-check weather null check condition applied for platformlabel before Preconditions.checkNotNull
| @@ -203,9 +212,16 @@ void logSpawnStrategies() { | |||
| strategyRegexFilter.getFilterToStrategies().asMap().entrySet()) { | |||
| Collection<SpawnStrategy> value = entry.getValue(); | |||
| logger.atInfo().log( | |||
There was a problem hiding this comment.
Inappropriate Logging Level
The code uses INFO level for logging filter platform to strategy implementations. According to repository guidelines, a more appropriate logging level should be considered. This may lead to log pollution in production environments where this level of detail isn't necessary.
Standards
- Repo-Guideline-Consider using a more appropriate logging level
|
/refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Platform Strategy Registry👍 Well Done
📁 Selected files for review (12)
🎯 Custom Instructions
📝 Additional Comments
|
| var platformLabel = spawn.getExecutionPlatformLabel(); | ||
| Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); |
There was a problem hiding this comment.
Missing Null Check
Repository guideline violation: null check condition should be applied before Preconditions.checkNotNull. The current implementation retrieves platformLabel and immediately passes it to checkNotNull without validation, which could cause unclear error messages if the value is null.
if (platformLabel == null) {
throw new NullPointerException("Attempting to spawn action without an execution platform.");
}
Commitable Suggestion
| var platformLabel = spawn.getExecutionPlatformLabel(); | |
| Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); | |
| if (platformLabel == null) { | |
| throw new NullPointerException("Attempting to spawn action without an execution platform."); | |
| } |
Standards
- Repo-Guideline-check weather null check condition applied for platformlabel before Preconditions.checkNotNull
- CWE-476
- OWASP-A06
| @@ -90,5 +91,9 @@ public void registerSpawnStrategies( | |||
| for (Map.Entry<RegexFilter, List<String>> entry : options.strategyByRegexp) { | |||
| registryBuilder.addDescriptionFilter(entry.getKey(), entry.getValue()); | |||
| } | |||
|
|
|||
| for (Map.Entry<String, List<String>> strategy : options.allowedStrategiesByExecPlatform) { | |||
| registryBuilder.addExecPlatformFilter(Label.parseCanonicalUnchecked(strategy.getKey()), strategy.getValue()); | |||
There was a problem hiding this comment.
Unchecked Label Parsing
Using parseCanonicalUnchecked without validation allows malformed label strings to cause runtime exceptions or unexpected behavior. Malicious configuration could exploit this to cause denial of service or system instability through invalid label injection.
Standards
- CWE-20
- OWASP-A03
- CWE-754
| logger.atInfo().log( | ||
| "FilterPlatformToStrategyImplementations: \"%s\" = [%s]", | ||
| entry.getKey().getCanonicalName(), toImplementationNames(value)); |
There was a problem hiding this comment.
Inappropriate Logging Level
Repository guideline violation: using INFO level for platform filtering logging may not be appropriate. This implementation logs platform-to-strategy mappings at INFO level which could create excessive log noise in production environments. Consider using DEBUG or TRACE level for detailed implementation mappings to improve system observability without overwhelming logs.
Standards
- Repo-Guideline-Consider using a more appropriate logging level
- ISO-IEC-25010-Reliability-Maturity
- SRE-Observability
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | ||
| .getFirst(); |
There was a problem hiding this comment.
Resource Leak Risk
Potential null pointer exception when remoteLocalFallbackStrategy is null. The method creates a new ArrayList containing potentially null strategy without validation, then calls getFirst() which could fail if filtering returns empty list. This creates unreliable fallback behavior that could cause service degradation during remote execution failures.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Postcondition-Validation
| if ((spawn.getExecutionPlatform() == null | ||
| || ( | ||
| spawn.getExecutionPlatform().execProperties().isEmpty() | ||
| && spawn.getExecutionPlatform().remoteExecutionProperties().isEmpty() | ||
| )) |
There was a problem hiding this comment.
Platform Condition Logic
Complex conditional logic checks platform nullability then accesses platform methods without null safety. If spawn.getExecutionPlatform() returns null, subsequent calls to execProperties() and remoteExecutionProperties() will cause NullPointerException. Logic should use null-safe evaluation pattern.
Standards
- Algorithm-Correctness-Null-Safety
- Logic-Verification-Conditional-Flow
- Business-Rule-Error-Prevention
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | ||
| .getFirst(); |
There was a problem hiding this comment.
Strategy Collection Logic
Method calls getFirst() on filtered strategy list without null or empty validation. If strategyPlatformFilter returns empty list or remoteLocalFallbackStrategy is null, getFirst() will throw NoSuchElementException. Business logic requires fallback strategy availability guarantee.
Standards
- Algorithm-Correctness-Collection-Safety
- Business-Rule-Fallback-Logic
- Logic-Verification-Error-Handling
| logger.atInfo().log( | ||
| "FilterPlatformToStrategyImplementations: \"%s\" = [%s]", | ||
| entry.getKey().getCanonicalName(), toImplementationNames(value)); |
There was a problem hiding this comment.
Inappropriate Logging Level
Repository guideline violation: using INFO level for platform filter logging may not be appropriate. Platform filtering configuration details might be better suited for DEBUG level to reduce noise in production logs while maintaining debugging capability.
Standards
- Repo-Guideline-Consider using a more appropriate logging level
- Clean-Code-Comments
- Maintainability-Quality-Observability
3 participants
This PR implements part of the Execution Platform Scoped Spawn Strategies proposal.
It adds a new flag
--allowed_strategies_by_exec_platformwhich permits filtering spawn strategies for spawns execution platform.Example
For an action with mnemonic
FOOconfigured for the host platform (@platforms//host:host), it will resolveworker,sandboxed,localas it's spawn strategy candidates.remotewas eliminated as a candidate (not in allow list for platform).--spawn_strategywas preserved.For an action with mnemonic
BARconfigured for the host platform (@platforms//host:host), it will resolvesandboxedas it's spawn strategy candidate.remotewas eliminated as a candidate (not in allow list for platform).sandboxedas the final candidate.For an action with mnemonic
BARconfigured for the remote platform (//:remote_platform), it will resolveremoteas it's spawn strategy candidate.sandboxedwas eliminated as a candidate (not in allow list for platform).remoteas the final candidate.If no spawn strategy candidate remains after filtering, the standard error will be logged.
TODO
CoverageReportActionBuilder.CoverageReportActionaction uses spawn machinery but lacks an execution platform, leading to a precondition check failure. Being resolved in Give builtin actions a platform bazelbuild/bazel#23231//src/test/java/com/google/devtools/build/lib/remote:RemoteTeststest failureSummary by CodeRabbit