Repository navigation
fix: avoid sleeping while holding lifecycle locks - #19810
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses CodeQL’s java/sleep-with-lock-held findings by moving retry-loop sleeps out of synchronized/monitor-held regions in the executor lifecycle and GCE autoscaler paths, while preserving existing retry/timeout behavior and synchronization for critical state publication.
Changes:
- Refactors
ExecutorLifecyclefile-lock acquisition into a helper that only synchronizes aroundtryLock()/publication and sleeps outside the monitor. - Updates
GceAutoScalercompute-service initialization to avoid sleeping while holding the autoscaler monitor, usingvolatile+ synchronized double-check for single initialization. - Adds unit tests covering lock retry success and immediate-timeout behavior for
ExecutorLifecycle.acquireTaskFileLock(...).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| indexing-service/src/test/java/org/apache/druid/indexing/worker/executor/ExecutorLifecycleTest.java | Adds coverage for file-lock retry and timeout paths. |
| indexing-service/src/main/java/org/apache/druid/indexing/worker/executor/ExecutorLifecycle.java | Moves lock retry sleep out of the lifecycle monitor via a helper method. |
| extensions-contrib/gce-extensions/src/main/java/org/apache/druid/indexing/overlord/autoscaling/gce/GceAutoScaler.java | Avoids sleeping while holding the autoscaler monitor; makes cached compute service cross-thread visible. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
extensions-contrib/gce-extensions/src/main/java/org/apache/druid/indexing/overlord/autoscaling/gce/GceAutoScaler.java:162
- If
createComputeServiceImpl()returnsnullon all attempts,createComputeService()will returnnullafter the retry loop. All callers immediately dereference the result (e.g.,computeService.instanceGroupManagers()), so this leads to a lateNullPointerExceptionrather than a clear initialization failure.
Consider throwing a GceServiceException after the retry loop if cachedComputeService is still null, to make the failure explicit and easier to diagnose.
synchronized (this) {
if (cachedComputeService == null) {
cachedComputeService = createComputeServiceImpl();
}
}
FrankChen021
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 changed files. No actionable correctness, concurrency, security, compatibility, or test-gap findings.
This is an automated review by Codex GPT-5.6-Sol
What changed
Why
Both retry loops called
Thread.sleepwhile holding an object monitor. A slow file-lock acquisition or delayed GCE service creation could therefore block unrelated lifecycle or autoscaler operations and triggered CodeQL'sjava/sleep-with-lock-heldrule.Impact
Retry timing and timeout behavior remain unchanged, but other threads no longer wait behind sleeping retry loops. Concurrent executor starts are still rejected, and the GCE compute service is still initialized at most once.
Validation
ExecutorLifecycleConfigTest(1 test)GceAutoScalerTest(12 tests)validate/ Checkstyle forindexing-serviceandextensions-contrib/gce-extensionsgit diff --checkFixes CodeQL alerts 2886 and 2887.