Eliaaazzz opened a new issue, #39364:
URL: https://github.com/apache/beam/issues/39364
### What happened?
On Windows, every Java portable ValidatesRunner test hangs for its full
JUnit timeout (1200s) during pipeline submission, inside artifact staging. With
#39332 fixed locally (so `beamTestPipelineOptions` parses), running
```
gradlew.bat :runners:spark:3:job-server:validatesPortableRunnerBatch --tests
"*SplittableDoFnTest*"
```
gives 9 tests, 9 failures, all identical:
```
org.junit.runners.model.TestTimedOutException: test timed out after 1200
seconds
at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2004)
at
org.apache.beam.runners.fnexecution.artifact.ArtifactStagingService.offer(ArtifactStagingService.java:555)
at
org.apache.beam.runners.portability.PortableRunner.run(PortableRunner.java:197)
at
org.apache.beam.runners.portability.testing.TestPortableRunner.run(TestPortableRunner.java:80)
```
Nothing is ever written to the artifact staging directory
(`$TMP/beam-artifact-staging` is never created). This is runner-agnostic: it is
the job server's `ArtifactStagingService`, so any portable Java VR suite on
Windows is affected.
### Root cause
The staged file name embeds the environment id verbatim.
`ArtifactStagingService.createFilename` does
```java
return clip(String.format("%s-%s-%s", idGenerator.getId(), clip(environment,
25), base), 100);
```
and Java SDK environment ids are environment URNs:
`SdkComponents.registerEnvironment` uses `uniqify(env.getUrn(),
environmentIds.values())`, producing ids like `beam:env:embedded:v1`. Colons
are legal in Linux file names (CI actually creates files named
`1-beam:env:embedded:v1-<staged-name>.jar`) but illegal on Windows:
```java
Paths.get("C:\\...\\beam-artifact-staging\\abc123")
.resolve("1-beam:env:embedded:v1-beam-runners-spark-3-job-server-2.68.0-SNAPSHOT-AbCdEf.jar")
// java.nio.file.InvalidPathException: Illegal char <:> at index 6:
1-beam:env:embedded:v1-...
```
So on Windows `StoreArtifact.call()` throws `InvalidPathException` in its
first statement, `destinationProvider.getDestination(...)` (via
`LocalResourceId.resolve`).
### Why it hangs instead of failing
`InvalidPathException` is a `RuntimeException`, and `StoreArtifact.call()`
only handles `IOException | InterruptedException`:
```java
} catch (IOException | InterruptedException exn) {
totalPendingBytes.setException(exn);
LOG.error("Exception staging artifacts", exn);
...
}
```
The runtime exception escapes: `totalPendingBytes.setException` is never
called, nothing is logged, and the exception sits in a `Future` that is only
inspected in `finishStaging`, which is never reached. The queue consumer is
dead, so the gRPC `onNext` thread blocks forever in
`OverflowingSemaphore.aquire` (GETCHUNK), the reverse-retrieval stream is never
completed or errored, and the client waits forever in
`ArtifactStagingService.offer`.
Thread dump of a hung test worker (jstack while the test was inside the
1200s wait):
```
"Time-limited test" WAITING
at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2004)
at
org.apache.beam.runners.fnexecution.artifact.ArtifactStagingService.offer(ArtifactStagingService.java:555)
"grpc-default-executor-2" WAITING (on object monitor)
at java.lang.Object.wait(Native Method)
at
org.apache.beam.runners.fnexecution.artifact.ArtifactStagingService$OverflowingSemaphore.aquire(ArtifactStagingService.java:230)
at
org.apache.beam.runners.fnexecution.artifact.ArtifactStagingService$2.onNext(ArtifactStagingService.java:411)
"pool-3-thread-1" .. "pool-3-thread-10" WAITING (parking) <- staging
executor, all idle, consumers dead
```
`OverflowingSemaphore.aquire` is waiting with no exception set, which
confirms the consumer died on a path that bypassed `setException`.
### Related weaknesses in the same code
- The `GETCHUNK` catch block calls the inbound observer's `onError`, which
logs and sets `state = ERROR` but never calls `responseObserver.onError(...)`,
so even handled staging failures leave the client waiting rather than failing
fast.
- `createFilename` splits with `Splitter.onPattern("[^A-Za-z-_.]]")`. The
doubled `]` looks like a typo (and `0-9` is missing), so the pattern only
matches a non-alphanumeric character followed by a literal `]` and `base` ends
up being the whole input rather than its last alphanumeric run. For artifacts
without a `STAGING_TO` role this embeds the full source path (including `C:` on
Windows) in the staged file name.
Fixing this probably means (a) sanitizing the staged file name (environment
id included) down to file-name-safe characters, and (b) hardening the failure
path so a dead `StoreArtifact` reliably errors the stream to the client instead
of deadlocking, e.g. catching `Throwable` in `StoreArtifact.call` and
propagating staging errors to `responseObserver.onError`.
This does not affect CI, which runs on Linux. Found while validating the fix
for #39332 on Windows; with both issues present, #39332 masks this one (tests
fail at `TestPipeline.create()` before ever reaching artifact staging).
### Issue Priority
Priority: 3 (minor)
### Issue Components
- [x] Component: Java SDK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
