From 16788af235b4ce28dcc88e896063ec223561c368 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Tue, 29 Sep 2026 20:24:16 +0200 Subject: [PATCH 1/2] Report streaming protocol failures as WinRMClientException Fixes #188. The streaming terminals (WqlRequest.stream(), CommandRequest.start(), RemoteFile.openStream()/openReader(), RemoteDirectoryListing.stream()) and the cursor close let RuntimeExceptions raised in the WSMan layer escape as-is, so an unexpected HTTP status, an exhausted authentication fallback or a malformed response surfaced as a raw IllegalStateException instead of the documented WinRMClientException. LightWinRMService.callStreaming() now wraps every non-typed failure in WinRMException, exactly like the blocking path's executeWithTimeout(), so the request classes translate it; the typed WinRMClientExceptions still pass through. The cursor's close() does the same. The stdin state checks ("input after the end mark") move out of the protocol step into RemoteCommand.checkStdinOpen(), called before it, so that caller bug stays an IllegalStateException. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 12 +++++ .../winrm/light/LightWinRMService.java | 9 ++-- .../metricshub/winrm/light/WsmanClient.java | 22 ++++++--- .../metricshub/winrm/StreamingApiTest.java | 48 +++++++++++++++++++ 4 files changed, 82 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ddaf37..4ba1b70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -155,3 +155,15 @@ Consequences: parentheses and modifiers removed, empty catch blocks named and commented, and two loops restructured. The only signature change is the removal of the unused `target` parameter from the `CipherGen` constructor (an internal NTLM helper). + +### Fixed + +- **Streaming terminals now report protocol failures as `WinRMClientException`** (#188). + `WqlRequest.stream()`, `CommandRequest.start()`, `RemoteFile.openStream()`/`openReader()`, + `RemoteDirectoryListing.stream()` and the closing of a `RemoteProcess` let raw + `IllegalStateException`s escape for an unexpected HTTP status (e.g. a 503 from a proxy), an + authentication fallback that exhausted every scheme, or a malformed response. They are now + wrapped like the blocking terminals' failures, with the raw exception in the cause chain; the + typed exceptions (`WinRMFaultException`, `WinRMAuthenticationException`, `WinRMTimeoutException`) + pass through unchanged, and genuine caller errors (invalid options, closed client, input after + the end of stdin) stay `IllegalArgumentException` / `IllegalStateException`. diff --git a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java index 3d8ce84..407eedb 100644 --- a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java +++ b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java @@ -517,6 +517,8 @@ private Chunk adapt(final WsmanClient.RemoteCommand.Chunk chunk) { @Override public void send(final byte[] data, final boolean end) throws TimeoutException, WindowsRemoteException { + // A caller bug (input after the end mark), reported as such before any protocol step. + remoteCommand.checkStdinOpen(); callStreaming(() -> { remoteCommand.send(data, end); return null; @@ -540,7 +542,7 @@ public int exitCode() { public void close() { try { remoteCommand.close(); - } catch (final RuntimeException e) { + } catch (final WinRMClientException e) { // Typed protocol failures (e.g. a fault answering the terminate Signal) pass through. throw e; } catch (final InterruptedException e) { @@ -565,12 +567,13 @@ public void close() { * @param the step's result type * @return the step's result * @throws TimeoutException when the step exceeds the inactivity timeout - * @throws WinRMException when the step fails with a checked failure + * @throws WinRMException when the step fails, with the raw failure as its cause — the typed + * {@link WinRMClientException}s pass through unchanged */ private static T callStreaming(final Callable step) throws TimeoutException, WinRMException { try { return step.call(); - } catch (final TimeoutException | RuntimeException e) { + } catch (final TimeoutException | WinRMClientException e) { throw e; } catch (final InterruptedException e) { Thread.currentThread().interrupt(); diff --git a/src/main/java/org/metricshub/winrm/light/WsmanClient.java b/src/main/java/org/metricshub/winrm/light/WsmanClient.java index 006be1b..290a313 100644 --- a/src/main/java/org/metricshub/winrm/light/WsmanClient.java +++ b/src/main/java/org/metricshub/winrm/light/WsmanClient.java @@ -736,6 +736,21 @@ Chunk pollChunk(final long askMs, final long maxWaitMs) throws Exception { } } + /** + * Reject input once the command's standard input is closed: a caller bug, reported before any + * protocol step. + * + * @throws IllegalStateException when the command has completed or its input was ended + */ + void checkStdinOpen() { + if (finished || exitCode != null) { + throw new IllegalStateException("The command has completed: its standard input is closed."); + } + if (stdinEnded) { + throw new IllegalStateException("The command's standard input has already been closed."); + } + } + /** * Feed standard input to the running command: one or more WSMan Send requests carrying the * bytes as base64 {@code stdin} streams, the last one flagged {@code End} when {@code end} is @@ -755,12 +770,7 @@ Chunk pollChunk(final long askMs, final long maxWaitMs) throws Exception { * @param end whether this is the last input the command will get */ void send(final byte[] data, final boolean end) throws Exception { - if (finished || exitCode != null) { - throw new IllegalStateException("The command has completed: its standard input is closed."); - } - if (stdinEnded) { - throw new IllegalStateException("The command's standard input has already been closed."); - } + checkStdinOpen(); if (data.length == 0 && !end) { // Nothing to say and no EOF to announce: an empty Send would be a pure round trip. return; diff --git a/src/test/java/org/metricshub/winrm/StreamingApiTest.java b/src/test/java/org/metricshub/winrm/StreamingApiTest.java index a3e366b..1eb23cc 100644 --- a/src/test/java/org/metricshub/winrm/StreamingApiTest.java +++ b/src/test/java/org/metricshub/winrm/StreamingApiTest.java @@ -47,6 +47,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.metricshub.winrm.exceptions.WinRMClientException; import org.metricshub.winrm.exceptions.WinRMTimeoutException; import org.metricshub.winrm.exceptions.WqlSyntaxException; import org.metricshub.winrm.light.FakeWsmanServer; @@ -300,6 +301,53 @@ private static String stderrChunk(final String text) { return stream("stderr", COMMAND_ID, text.getBytes(StandardCharsets.UTF_8)); } + private static Throwable rootCause(final Throwable e) { + Throwable cause = e; + while (cause.getCause() != null) { + cause = cause.getCause(); + } + return cause; + } + + @Test + void streamReportsAnUnexpectedHttpStatusAsAClientException() { + // A 503 from a proxy, or a non-WinRM service on the port: a protocol failure, not a caller bug. + server.enqueue(503, fault("999", "Service unavailable")); + + try (WinRMClient client = builder().build()) { + final WinRMClientException e = assertThrows( + WinRMClientException.class, + () -> client.wql("SELECT Name FROM Win32_Service").stream() + ); + assertTrue(e.getMessage().contains("HTTP 503"), e.getMessage()); + assertTrue(rootCause(e) instanceof IllegalStateException); + } + } + + @Test + void startReportsAnUnexpectedHttpStatusAsAClientException() { + server.enqueue(503, fault("999", "Service unavailable")); + + try (WinRMClient client = builder().build()) { + final WinRMClientException e = assertThrows(WinRMClientException.class, () -> client.command("dir").start()); + assertTrue(e.getMessage().contains("HTTP 503"), e.getMessage()); + assertTrue(rootCause(e) instanceof IllegalStateException); + } + } + + @Test + void closeReportsAnUnexpectedHttpStatusAsAClientException() throws Exception { + enqueueCommandStartup(); + // The terminate Signal of an early close is answered with a 503. + server.enqueue(503, fault("999", "Service unavailable")); + + try (WinRMClient client = builder().build()) { + final RemoteProcess process = client.command("dir").start(); + final WinRMClientException e = assertThrows(WinRMClientException.class, process::close); + assertTrue(e.getMessage().contains("HTTP 503"), e.getMessage()); + } + } + @Test void startStreamsOutputWhileTheCommandIsStillRunning() throws Exception { enqueueCommandStartup(); From 0ef6344d2e9090792d312631201e8691030b4bbb Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Tue, 29 Sep 2026 20:40:40 +0200 Subject: [PATCH 2/2] Address Codex review: keep the closed-client error on streaming steps A handle outliving its client (stream, close the client, advance past the buffered page) hit WsmanClient's closed guard inside the protocol step, which callStreaming now wrapped as a protocol failure. The executor's own checkNotClosed() runs before every streaming step, so the caller bug stays the documented IllegalStateException. Co-Authored-By: Claude Fable 5.1 --- .../metricshub/winrm/light/LightWinRMService.java | 5 ++++- .../org/metricshub/winrm/StreamingApiTest.java | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java index 407eedb..4e977b7 100644 --- a/src/main/java/org/metricshub/winrm/light/LightWinRMService.java +++ b/src/main/java/org/metricshub/winrm/light/LightWinRMService.java @@ -566,11 +566,14 @@ public void close() { * @param step the protocol step to run * @param the step's result type * @return the step's result + * @throws IllegalStateException when this executor was closed: a handle outliving its client is a + * caller bug, reported as such rather than as a protocol failure * @throws TimeoutException when the step exceeds the inactivity timeout * @throws WinRMException when the step fails, with the raw failure as its cause — the typed * {@link WinRMClientException}s pass through unchanged */ - private static T callStreaming(final Callable step) throws TimeoutException, WinRMException { + private T callStreaming(final Callable step) throws TimeoutException, WinRMException { + checkNotClosed(); try { return step.call(); } catch (final TimeoutException | WinRMClientException e) { diff --git a/src/test/java/org/metricshub/winrm/StreamingApiTest.java b/src/test/java/org/metricshub/winrm/StreamingApiTest.java index 1eb23cc..831a360 100644 --- a/src/test/java/org/metricshub/winrm/StreamingApiTest.java +++ b/src/test/java/org/metricshub/winrm/StreamingApiTest.java @@ -309,6 +309,20 @@ private static Throwable rootCause(final Throwable e) { return cause; } + @Test + void streamOutlivingItsClientFailsAsAClosedClient() throws Exception { + server.enqueue(200, envelope(enumeratePage("uuid:CTX-1", service("Spooler", "Running")))); + + final WinRMClient client = builder().build(); + final Iterator iterator = client.wql("SELECT Name FROM Win32_Service").stream().iterator(); + assertEquals("Spooler", iterator.next().string("Name")); + client.close(); + + // The next row needs a Pull on a closed client: a caller bug, not a protocol failure. + final IllegalStateException e = assertThrows(IllegalStateException.class, iterator::next); + assertEquals("This instance has been closed and a new one must be created.", e.getMessage()); + } + @Test void streamReportsAnUnexpectedHttpStatusAsAClientException() { // A 503 from a proxy, or a non-WinRM service on the port: a protocol failure, not a caller bug.