Fail fast in artifact staging when storing an artifact fails#39367
Fail fast in artifact staging when storing an artifact fails#39367Eliaaazzz wants to merge 1 commit into
Conversation
…hanging A StoreArtifact task that failed with an unchecked exception, such as InvalidPathException on Windows, was never reported: the chunk producer blocked forever and the reverse-retrieval stream was never terminated, so clients hung in ArtifactStagingService.offer until their timeout. Report every failure to the pending-bytes semaphore, make the producer observe it promptly, and error the stream with the root cause.
Summary of ChangesHello, 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! This pull request addresses a critical issue where artifact staging on Windows would hang indefinitely due to unhandled exceptions during path resolution. By ensuring that all exceptions are properly propagated and that communication channels are resilient to consumer failures, the changes guarantee that pipeline submission fails fast with a clear error message when staging issues occur. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prevents the artifact staging service from hanging indefinitely when an exception (such as an unchecked exception during destination resolution) occurs. It introduces a polling mechanism for queue writes, catches general exceptions to set the failure status, and adds a unit test to verify this behavior. The review feedback highlights two important issues: first, calling stagingExecutor.shutdownNow() will permanently disable the shared executor service for future requests; second, catching a general Exception can swallow InterruptedException without restoring the thread's interrupted status.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| state = State.ERROR; | ||
| stagingExecutor.shutdownNow(); |
There was a problem hiding this comment.
Calling stagingExecutor.shutdownNow() will shut down the shared executor service of the ArtifactStagingService instance. Since ArtifactStagingService is typically a long-running gRPC service (e.g., in a JobServer), shutting down this executor will permanently prevent any future artifact staging requests from succeeding.
Since the StoreArtifact task has already failed and terminated (which is what set the exception on totalPendingBytes and triggered this catch block), there is no need to shut down or cancel anything in the executor. We can safely remove this call.
| state = State.ERROR; | |
| stagingExecutor.shutdownNow(); | |
| state = State.ERROR; |
| if (exn instanceof IOException) { | ||
| throw (IOException) exn; | ||
| } else if (exn instanceof RuntimeException) { | ||
| throw (RuntimeException) exn; | ||
| } else { | ||
| throw new RuntimeException(exn); |
There was a problem hiding this comment.
When catching Exception (which includes InterruptedException), the interrupted status of the thread is cleared. If the exception is an InterruptedException, we should restore the interrupted status by calling Thread.currentThread().interrupt() before rethrowing or wrapping it, so that calling code or the thread pool can handle the interruption properly.
if (exn instanceof IOException) {
throw (IOException) exn;
} else if (exn instanceof InterruptedException) {
Thread.currentThread().interrupt();
throw new RuntimeException(exn);
} else if (exn instanceof RuntimeException) {
throw (RuntimeException) exn;
} else {
throw new RuntimeException(exn);
}
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
11 similar comments
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Assigning reviewers: R: @chamikaramj added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
1 participant
Fixes #39364.
On Windows,
StoreArtifact.call()fails in its first statement with an uncheckedInvalidPathException, because the staged filename embeds the environment id and Java environment ids contain colons (beam:env:embedded:v1). The catch block only handledIOException | InterruptedException, so the failure was never reported anywhere: the queue consumer died silently, the gRPConNextthread eventually blocked forever inOverflowingSemaphore.aquire(or incurrentOutput.putonce the 100-slot chunk queue filled), the reverse-retrieval stream was neither completed nor errored, and the submitting client waited forever inArtifactStagingService.offer. This is what makes every portable Java ValidatesRunner test on Windows hang for its full 1200s JUnit timeout during pipeline submission.This PR makes any artifact storage failure fail fast and reach the client:
StoreArtifact.call()reports every exception tototalPendingBytes, including unchecked ones, before rethrowing.OverflowingSemaphore.aquireobserves a set exception even when permits are still available, instead of only after the 100 MB budget is exhausted.BlockingQueue.put; it polls with a timeout and rechecks the semaphore's exception, so a dead consumer cannot strand it.GETCHUNKerror handler terminates the stream withStatus.INTERNALcarrying the root cause description, instead of only flipping internal state, sooffercompletes exceptionally with the actual error.The regression test injects a destination provider that throws
InvalidPathException(asLocalResourceId.resolvedoes on Windows) and streams an artifact in more chunks than the service buffers, so staging cannot run to completion before the failure is observed. Without the main change the test hangs and fails by timeout, reproducing the reported bug; with it,offerfails within seconds and the client sees the root cause.#39363 complements this fix by sanitizing the Windows-illegal characters out of the generated filenames. With both, portable Java pipelines can stage artifacts on Windows, and any future staging failure will surface immediately instead of hanging the submission.
Tested with
gradlew :runners:java-fn-execution:test --tests "*ArtifactStagingServiceTest*": both tests pass, and with the main change reverted the new test fails by timeout as described in the issue.