fix (Bazel): add command execution timeout to prevent indefinite hangs - #1878
bd-samratmuk wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds configurable timeouts for Bazel command execution to prevent indefinite Detect hangs.
Changes:
- Adds
detect.bazel.command.timeoutwith an 1800-second default. - Propagates the timeout through Bazel options and execution.
- Adds timeout behavior tests.
File summaries
| File | Description |
|---|---|
| src/main/java/com/blackduck/integration/detect/configuration/DetectProperties.java | Updated as part of this pull request. |
| src/main/java/com/blackduck/integration/detect/configuration/DetectableOptionFactory.java | Updated as part of this pull request. |
| detectable/src/test/java/com/blackduck/integration/detectable/detectables/bazel/v2/unit/BazelDetectableOptionsTestBuilder.java | Updated as part of this pull request. |
| detectable/src/test/java/com/blackduck/integration/detectable/detectables/bazel/v2/unit/BazelCommandExecutorTest.java | Updated as part of this pull request. |
| detectable/src/main/java/com/blackduck/integration/detectable/detectables/bazel/v2/BazelV2Detectable.java | Updated as part of this pull request. |
| detectable/src/main/java/com/blackduck/integration/detectable/detectables/bazel/pipeline/step/BazelCommandExecutor.java | Updated as part of this pull request. |
| detectable/src/main/java/com/blackduck/integration/detectable/detectables/bazel/BazelDetectableOptions.java | Updated as part of this pull request. |
Review details
Suppressed comments (3)
detectable/src/main/java/com/blackduck/integration/detectable/detectables/bazel/pipeline/step/BazelCommandExecutor.java:57
newCachedThreadPoolis unbounded, so concurrent calls (or repeated calls after a timed-out task that ignores interruption) can create one daemon thread per Bazel invocation. That contradicts the bounded-executor goal and can exhaust resources while the underlying Bazel processes remain active; use a single/fixed worker (or otherwise enforce a maximum number of in-flight commands) for this executor.
private final ExecutorService commandExecutor = Executors.newCachedThreadPool(DAEMON_THREAD_FACTORY);
detectable/src/main/java/com/blackduck/integration/detectable/detectables/bazel/pipeline/step/BazelCommandExecutor.java:269
- Cancelling a Java
Futureonly interrupts the worker; it does not terminate the Bazel client process started byDetectableExecutableRunner. IfwaitFor()or the runner's process cleanup does not honor interruption, the timed-out Bazel process/server fetch continues in the background, consuming resources and potentially interfering with later scans. The timeout path needs process-level cancellation/cleanup (or a runner API that guarantees it), rather than relying onfuture.cancel(true)alone.
} catch (TimeoutException e) {
future.cancel(true); // interrupt the worker thread, unblocking any local Process.waitFor()
detectable/src/main/java/com/blackduck/integration/detectable/detectables/bazel/pipeline/step/BazelCommandExecutor.java:77
- The legacy
BazelExtractorstill calls this 3-argument constructor, sodetect.bazel.command.timeoutis silently ignored for the legacy Bazel detectable and it always uses 1800 seconds. Since the property is exposed as the Bazel command timeout, pass the configured value throughBazelExtractoras well (or remove the legacy path) rather than relying on this fallback.
public BazelCommandExecutor(DetectableExecutableRunner executableRunner, File workspaceDir, ExecutableTarget bazelExe) {
this(executableRunner, workspaceDir, bazelExe, DEFAULT_COMMAND_TIMEOUT_SECONDS);
}
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // Set up Bazel command executor and determine environment mode | ||
| BazelCommandExecutor bazelCmd = new BazelCommandExecutor(executableRunner, environment.getDirectory(), bazelExe); | ||
| BazelCommandExecutor bazelCmd = new BazelCommandExecutor(executableRunner, environment.getDirectory(), bazelExe, options.getCommandTimeoutSeconds()); |
There was a problem hiding this comment.
extreme edge case not worth considering.
| String modeOverride = detectConfiguration.getNullableValue(DetectProperties.DETECT_BAZEL_MODE); | ||
| return new BazelDetectableOptions(targetName, dependencySourcesFromProperty, bazelCqueryAdditionalOptions, bazelQueryAdditionalOptions, modeOverride); | ||
| int commandTimeoutSeconds = detectConfiguration.getValue(DetectProperties.DETECT_BAZEL_COMMAND_TIMEOUT); | ||
| return new BazelDetectableOptions(targetName, dependencySourcesFromProperty, bazelCqueryAdditionalOptions, bazelQueryAdditionalOptions, modeOverride, commandTimeoutSeconds); |
There was a problem hiding this comment.
The legacy BazelDetectable is old code - pending deletion.
| IntegerProperty.newBuilder("detect.bazel.command.timeout", 1800) | ||
| .setInfo("Bazel Command Timeout", DetectPropertyFromVersion.VERSION_12_0_0) | ||
| .setHelp( | ||
| "The amount of time, in seconds, Detect will wait for any single Bazel command (query, cquery, mod graph, show_repo, etc.) to complete before aborting it.", |
There was a problem hiding this comment.
I might be inclined to use some of this entry as a release note and simplify the messaging a little bit here?
Something like:
The maximum time, in seconds, that Detect waits for an individual Bazel command to complete before aborting it.
This timeout prevents Detect from waiting indefinitely if Bazel becomes unresponsive. The default value is intentionally high as commands against large workspaces might take a long time to complete.
If this timeout is reached, Detect logs an error and aborts the command. Bazel uses a persistent background server, which might require running Bazel shutdown in the project directory to clear a stalled server process.
There was a problem hiding this comment.
Addressed in the latest commit. can you check again? @cpottsbd
There was a problem hiding this comment.
Reminder to add a release note. : ) (Or I can add it, just let me know.)
Problem
Bazel subprocess calls (query/cquery/mod graph/show_repo) had no timeout, so a
stalled Bazel fetch (e.g. broken local_repository) could hang Detect forever.
Fix
BazelCommandExecutor now runs Bazel commands on a bounded executor and aborts
with a clear error after a configurable timeout (new property
detect.bazel.command.timeout, default 1800s). Scoped to Bazel only.