From 5a9b5752449853e238c009065eb814c9b17c59c1 Mon Sep 17 00:00:00 2001 From: Patrick Ropp Date: Fri, 24 Jul 2026 12:08:25 -0400 Subject: [PATCH 1/3] approved design docs (docs/design) --- docs/design/architecture.md | 161 ++++++++++++++++++++++++++++++++++++ docs/design/data-model.md | 119 ++++++++++++++++++++++++++ docs/design/testing.md | 103 +++++++++++++++++++++++ 3 files changed, 383 insertions(+) create mode 100644 docs/design/architecture.md create mode 100644 docs/design/data-model.md create mode 100644 docs/design/testing.md diff --git a/docs/design/architecture.md b/docs/design/architecture.md new file mode 100644 index 000000000..f0f9a4416 --- /dev/null +++ b/docs/design/architecture.md @@ -0,0 +1,161 @@ +# Architecture — Fix `SchedulerClient.pauseSchedule()` GET-then-PUT fallback (Issue #140) + +## 1. Overview + +`SchedulerResource` in the Java client supports two server flavors for the +schedule pause/resume endpoints: + +- **Enterprise (Orkes)** servers accept `GET /scheduler/schedules/{name}/pause`. +- **OSS Conductor** servers only accept `PUT` on the same path. + +To work against both without a configuration flag, the client optimistically +issues a `GET` first and, if the server reports that `GET` is not allowed for +that route, retries the identical call with `PUT`. This is implemented by +`SchedulerResource.executeGetThenPutOnMethodNotAllowed(...)`. + +### The bug + +The fallback only recognizes a **clean HTTP `405 Method Not Allowed`** as the +signal to retry with `PUT`. Conductor OSS `3.32.0-rc.10` (Spring MVC) does not +return `405` for this route — it returns **HTTP `500`** with the body +`Request method 'GET' is not supported`. Because the status is `500` and not +`405`, the fallback rethrows the exception instead of proceeding to the `PUT`, +so `pauseSchedule(...)` (and `resumeSchedule(...)`) fail against OSS servers. + +``` +com.netflix.conductor.client.exception.ConductorClientException: + Request method 'GET' is not supported {status=500, retryable: false} + at ...SchedulerResource.executeGetThenPutOnMethodNotAllowed(SchedulerResource.java:175) + at ...SchedulerResource.pauseSchedule(SchedulerResource.java:125) +``` + +### The fix (scope) + +Broaden the retry predicate in `executeGetThenPutOnMethodNotAllowed` so that it +treats the server's "method not supported" signal as a trigger to fall through +to the `PUT`, whether it arrives as a genuine `405` **or** as a `500` whose +message indicates the request method is not supported. All other failures +(auth, validation, unrelated application `500`s, network) keep their current +behavior and propagate unchanged. + +This is a **minimal, focused** change confined to one private method plus its +tests. No public API, no new files, no dependency changes. + +## 2. Tech stack + +| Concern | Choice | +|--------------------|--------------------------------------------------------------------| +| Language | Java 21 | +| Module | `conductor-client` | +| HTTP client | `com.netflix.conductor.client.http.ConductorClient` (OkHttp based) | +| Exceptions | `com.netflix.conductor.client.exception.ConductorClientException` | +| Tests | JUnit 5 (`org.junit.jupiter`) + `okhttp3.mockwebserver.MockWebServer` | +| Build | Gradle | + +## 3. Module / file layout + +Only **existing** files are touched. No new source files are created. + +| File | Change | Responsibility | +|------|--------|----------------| +| `conductor-client/src/main/java/io/orkes/conductor/client/http/SchedulerResource.java` | **MODIFY** | Add the private helper `isMethodNotSupported(ConductorClientException)` and the two detection constants; update `executeGetThenPutOnMethodNotAllowed(...)` to use the helper. | +| `conductor-client/src/test/java/io/orkes/conductor/client/http/SchedulerResourceTest.java` | **MODIFY** | Add regression tests covering the `500 "Request method 'GET' is not supported"` fallthrough for both `pauseSchedule` and `resumeSchedule`, and confirm unrelated `500`s still propagate. | + +No changes are required in +`agent-examples/.../Example99ScheduledAgent.java`; it is the reproduction, not +part of the fix. + +## 4. Shared contracts (reused verbatim by every component) + +These names, signatures, and constants are the single source of truth. The +supporting docs and the tests reuse them exactly. + +### 4.1 Exception surface (existing — do not change) + +`ConductorClientException extends ApiException`. Relevant accessors used by the +fix: + +```java +int getStatus(); // HTTP status code, e.g. 405 or 500 +String getMessage(); // resolves to responseBody when present (see ApiException.getMessage) +``` + +`ApiException.getMessage()` returns the `responseBody` when it is non-blank, so +for the OSS failure it yields the string `Request method 'GET' is not supported`. + +### 4.2 Detection constants (new — private to `SchedulerResource`) + +```java +private static final int HTTP_METHOD_NOT_ALLOWED = 405; + +// Substring Spring MVC emits when a route rejects the HTTP verb. +// Matched case-insensitively against ConductorClientException.getMessage(). +private static final String METHOD_NOT_SUPPORTED_MARKER = "request method 'get' is not supported"; +``` + +### 4.3 Detection helper (new — private to `SchedulerResource`) + +```java +/** + * True when the server rejected the GET because the HTTP verb is not + * supported for the route. Recognizes both a clean 405 and the OSS + * Conductor behavior of returning 500 with a + * "Request method 'GET' is not supported" body. + */ +private boolean isMethodNotSupported(ConductorClientException e); +``` + +Predicate (exact semantics): + +``` +message = e.getMessage() + +isMethodNotSupported(e) == + e.getStatus() == HTTP_METHOD_NOT_ALLOWED + || (message != null + && message.toLowerCase(Locale.ROOT).contains(METHOD_NOT_SUPPORTED_MARKER)) +``` + +`java.util.Locale` is imported for the case-insensitive comparison. + +### 4.4 Fallback method (existing signature — body updated) + +```java +private void executeGetThenPutOnMethodNotAllowed( + ConductorClientRequest getRequest, ConductorClientRequest putRequest); +``` + +New body: + +```java +try { + client.execute(getRequest); +} catch (ConductorClientException e) { + if (!isMethodNotSupported(e)) { + throw e; + } + client.execute(putRequest); +} +``` + +### 4.5 Behavior contract + +| GET response from server | Client action | +|--------------------------------------------------------------|-----------------------------------| +| `2xx` (e.g. `204`) | Succeed. No `PUT`. | +| `405 Method Not Allowed` | Retry with `PUT`. | +| `500` with body containing `Request method 'GET' is not supported` | Retry with `PUT`. | +| Any other failure (`401`, `403`, `400`, unrelated `500`, IO) | Propagate the original exception. | + +The `PUT` retry itself is **not** wrapped: if the `PUT` fails, that exception +propagates as-is (unchanged from current behavior). + +Both `pauseSchedule(String, String)` and `resumeSchedule(String)` route through +`executeGetThenPutOnMethodNotAllowed`, so both benefit from the fix identically. + +## 5. Non-goals + +- No change to the public `SchedulerClient` / `OrkesSchedulerClient` API. +- No configuration flag to force GET vs PUT (the auto-fallback stays). +- No change to any other `SchedulerResource` endpoint or to `ConductorClient`. +- No change to how exceptions are constructed or to `ApiException`. diff --git a/docs/design/data-model.md b/docs/design/data-model.md new file mode 100644 index 000000000..d6c73d0d4 --- /dev/null +++ b/docs/design/data-model.md @@ -0,0 +1,119 @@ +# Data Model & Request Flow — Scheduler pause/resume fallback (Issue #140) + +This document details the request/response data and the decision flow for the +GET-then-PUT fallback described in `architecture.md`. All names, constants, and +signatures are those defined in `architecture.md` §4. + +## 1. Endpoints involved + +| Operation | Method (Enterprise) | Method (OSS) | Path | +|-----------|---------------------|--------------|------| +| pause | `GET` | `PUT` | `/scheduler/schedules/{name}/pause` | +| resume | `GET` | `PUT` | `/scheduler/schedules/{name}/resume` | + +`pause` additionally carries an optional `reason` query parameter. Per the +existing behavior, `reason` is set on **both** the prepared `GET` and `PUT` +requests before the fallback runs. + +## 2. Server response shapes + +There is no request/response body model to add — the fix keys entirely off the +HTTP status and the error message already surfaced by +`ConductorClientException`. + +### 2.1 Enterprise server (GET accepted) + +``` +HTTP/1.1 204 No Content +``` + +→ Client returns normally; no `PUT` is issued. + +### 2.2 OSS server (GET rejected) — the case Issue #140 fixes + +``` +HTTP/1.1 500 Internal Server Error +Content-Type: text/plain + +Request method 'GET' is not supported +``` + +The `ConductorClient` maps this to: + +``` +ConductorClientException { + status = 500 + responseBody = "Request method 'GET' is not supported" + getMessage() -> "Request method 'GET' is not supported" +} +``` + +### 2.3 Older / spec-compliant server (GET rejected) + +``` +HTTP/1.1 405 Method Not Allowed +``` + +``` +ConductorClientException { status = 405, ... } +``` + +## 3. Decision flow + +``` +pauseSchedule(name, reason) / resumeSchedule(name) + | + v +build getRequest (GET) + putRequest (PUT) // reason applied to both for pause + | + v +executeGetThenPutOnMethodNotAllowed(getRequest, putRequest) + | + +-- client.execute(getRequest) + | | + | +-- success (2xx) --------------------> return + | | + | +-- throws ConductorClientException e + | | + | v + | isMethodNotSupported(e) ? + | status == 405 -> true + | OR getMessage() contains -> true + | "request method 'get' is not supported" + | (case-insensitive) + | else -> false + | | + | true -------+------- false + | | | + | v v + | client.execute(putRequest) throw e // propagate unchanged + | | + | +-- success -> return + | +-- failure -> propagate PUT exception (unchanged) +``` + +## 4. Decision matrix (authoritative) + +Mirrors `architecture.md` §4.5. This is the exact truth table the +implementation and tests must satisfy. + +| # | GET result | `isMethodNotSupported` | PUT issued? | Outcome | +|---|----------------------------------------------------|------------------------|-------------|-----------------------------| +| 1 | `204` | n/a | no | success | +| 2 | `405` | `true` | yes | PUT outcome | +| 3 | `500` body `Request method 'GET' is not supported` | `true` | yes | PUT outcome | +| 4 | `500` unrelated body (e.g. `boom`) | `false` | no | original exception rethrown | +| 5 | `401` / `403` / `400` | `false` | no | original exception rethrown | + +Row 4 is the guardrail that keeps the fix minimal: only the specific +"method not supported" `500` falls through; every other `500` still fails fast. +This preserves the existing "do not retry non-405 failures" contract in spirit +(that test is updated to use an unrelated `500` body — see `testing.md`). + +## 5. Why match on message, not just status + +A blanket "retry any `500` with `PUT`" would mask genuine server errors and +double-fire mutating requests. Matching the Spring MVC marker string +`request method 'get' is not supported` (case-insensitive, via +`METHOD_NOT_SUPPORTED_MARKER`) restricts the fallthrough to exactly the +verb-rejection condition, keeping the behavior safe and targeted. diff --git a/docs/design/testing.md b/docs/design/testing.md new file mode 100644 index 000000000..8395b1bde --- /dev/null +++ b/docs/design/testing.md @@ -0,0 +1,103 @@ +# Testing — Scheduler pause/resume GET-then-PUT fallback (Issue #140) + +Test plan for the fix in [`architecture.md`](./architecture.md), verifying the +decision matrix in [`data-model.md`](./data-model.md). All names and rules are +reused verbatim from architecture §3. + +## 1. Scope + +- **Unit tests only.** The change is a pure control-flow adjustment inside + `SchedulerResource`; no server, network, or persistence is required. +- Target file: + `conductor-client/src/test/java/io/orkes/conductor/client/http/SchedulerResourceTest.java` +- Framework: JUnit 5 (Jupiter) + Mockito, matching existing `conductor-client` + tests. + +## 2. Test double strategy + +Mock `com.netflix.conductor.client.http.ConductorClient` and inject it into +`new SchedulerResource(mockClient)`. Because both `GET` and `PUT` go through the +`void` overload `client.execute(ConductorClientRequest)`, distinguish the two +attempts by matching on the request `Method` with an `ArgumentMatcher` / captor. + +Helper to build the thrown error, reused across cases: + +```java +private static ConductorClientException clientError(int status, String message) { + // ConductorClientException(int statusCode, String message) + return new ConductorClientException(status, message); +} + +// Matches a request whose method equals the given Method. +private static ConductorClientRequest req(Method method) { + return argThat(r -> r != null && r.getMethod() == method); +} +``` + +## 3. Test matrix + +Each row is one `@Test`. "GET" / "PUT" refer to `client.execute(...)` invoked with +that method on `/scheduler/schedules/{name}/pause` (or `/resume`). + +| # | Test name | GET stub | Expected PUT? | Expected result | +|---|-----------|----------|---------------|-----------------| +| 1 | `pauseSchedule_getSucceeds_noPut` | returns normally (Enterprise) | **no** | no exception; `execute(GET)` once, `execute(PUT)` never | +| 2 | `pauseSchedule_405_fallsThroughToPut` | throws `clientError(405, "Method Not Allowed")` | **yes** | PUT executed once; no exception | +| 3 | `pauseSchedule_500MethodNotSupported_fallsThroughToPut` | throws `clientError(500, "Request method 'GET' is not supported")` | **yes** | PUT executed once; no exception — **the issue #140 case** | +| 4 | `pauseSchedule_500MethodNotSupported_caseInsensitive` | throws `clientError(500, "REQUEST METHOD 'GET' IS NOT SUPPORTED")` | **yes** | PUT executed once; no exception | +| 5 | `pauseSchedule_500Generic_rethrows` | throws `clientError(500, "NullPointerException")` | **no** | same exception rethrown (status 500) | +| 6 | `pauseSchedule_500NullMessage_rethrows` | throws `clientError(500, null)` | **no** | same exception rethrown | +| 7 | `pauseSchedule_401_rethrows` | throws `clientError(401, "Unauthorized")` | **no** | same exception rethrown (auth preserved) | +| 8 | `pauseSchedule_403_rethrows` | throws `clientError(403, "Forbidden")` | **no** | same exception rethrown | +| 9 | `pauseSchedule_400_rethrows` | throws `clientError(400, "bad request")` | **no** | same exception rethrown (validation preserved) | +| 10 | `pauseSchedule_putFails_propagates` | GET throws `clientError(405, ...)`, PUT throws `clientError(409, "conflict")` | yes (fails) | PUT's exception propagates unchanged | +| 11 | `resumeSchedule_500MethodNotSupported_fallsThroughToPut` | throws `clientError(500, "Request method 'GET' is not supported")` | **yes** | PUT executed once; no exception (same helper, resume path) | +| 12 | `pauseSchedule_withReason_reasonOnBothRequests` | throws `clientError(500, "Request method 'GET' is not supported")` | **yes** | captured GET and PUT both carry `reason` query param | + +## 4. Representative test bodies + +Issue #140 core case (row 3): + +```java +@Test +void pauseSchedule_500MethodNotSupported_fallsThroughToPut() { + doThrow(clientError(500, "Request method 'GET' is not supported")) + .when(client).execute(req(Method.GET)); + // PUT succeeds (no stubbing needed for a void success) + + schedulerResource.pauseSchedule("eng_digest_99", "maintenance"); + + verify(client).execute(req(Method.GET)); + verify(client).execute(req(Method.PUT)); + verifyNoMoreInteractions(client); +} +``` + +Negative case (row 5): + +```java +@Test +void pauseSchedule_500Generic_rethrows() { + doThrow(clientError(500, "NullPointerException")) + .when(client).execute(req(Method.GET)); + + ConductorClientException ex = assertThrows( + ConductorClientException.class, + () -> schedulerResource.pauseSchedule("eng_digest_99")); + + assertEquals(500, ex.getStatus()); + verify(client).execute(req(Method.GET)); + verify(client, never()).execute(req(Method.PUT)); +} +``` + +## 5. Acceptance criteria + +- All rows in §3 pass. +- No production change beyond `SchedulerResource.executeGetThenPutOnMethodNotAllowed` + and its new private helper `isMethodNotAllowed` (architecture §3.3). +- `pauseSchedule`/`resumeSchedule` public signatures unchanged. +- Manual end-to-end check (out of CI): running + `Example99ScheduledAgent` against conductor-oss `3.32.0-rc.10` completes the + full lifecycle deploy → create → list → **pause** → resume → preview → delete + without the `Request method 'GET' is not supported` failure. From c831f7ce4877feaad5e2f67f5c8108dab826559c Mon Sep 17 00:00:00 2001 From: Patrick Ropp Date: Fri, 24 Jul 2026 12:09:47 -0400 Subject: [PATCH 2/3] code_subtask scheduler-resource-fix --- .../conductor/client/http/SchedulerResource.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/conductor-client/src/main/java/io/orkes/conductor/client/http/SchedulerResource.java b/conductor-client/src/main/java/io/orkes/conductor/client/http/SchedulerResource.java index fe9d7db38..921046a8f 100644 --- a/conductor-client/src/main/java/io/orkes/conductor/client/http/SchedulerResource.java +++ b/conductor-client/src/main/java/io/orkes/conductor/client/http/SchedulerResource.java @@ -13,6 +13,7 @@ package io.orkes.conductor.client.http; import java.util.List; +import java.util.Locale; import java.util.Map; import com.netflix.conductor.client.exception.ConductorClientException; @@ -31,6 +32,9 @@ public class SchedulerResource { + private static final int HTTP_METHOD_NOT_ALLOWED = 405; + private static final String METHOD_NOT_SUPPORTED_MARKER = "request method 'get' is not supported"; + private final ConductorClient client; public SchedulerResource(ConductorClient client) { @@ -167,20 +171,27 @@ public void resumeSchedule(String name) { /** * Enterprise scheduler endpoints accept GET while OSS accepts PUT. Retry only * a method-not-allowed response so application and authentication failures - * retain their original behavior. + * retain their original behavior. This also handles OSS servers that return + * an HTTP 500 with a "method not supported" message instead of a clean 405. */ private void executeGetThenPutOnMethodNotAllowed( ConductorClientRequest getRequest, ConductorClientRequest putRequest) { try { client.execute(getRequest); } catch (ConductorClientException e) { - if (e.getStatus() != 405) { + if (!isMethodNotSupported(e)) { throw e; } client.execute(putRequest); } } + private boolean isMethodNotSupported(ConductorClientException e) { + return e.getStatus() == HTTP_METHOD_NOT_ALLOWED + || (e.getMessage() != null + && e.getMessage().toLowerCase(Locale.ROOT).contains(METHOD_NOT_SUPPORTED_MARKER)); + } + public void saveSchedule(SaveScheduleRequest saveScheduleRequest) { ConductorClientRequest request = ConductorClientRequest.builder() .method(Method.POST) From 39cfe7dcc9e6998b448ede93d596b8d1f9fd0710 Mon Sep 17 00:00:00 2001 From: Patrick Ropp Date: Fri, 24 Jul 2026 12:10:22 -0400 Subject: [PATCH 3/3] code_subtask scheduler-resource-tests --- .../client/http/SchedulerResourceTest.java | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/conductor-client/src/test/java/io/orkes/conductor/client/http/SchedulerResourceTest.java b/conductor-client/src/test/java/io/orkes/conductor/client/http/SchedulerResourceTest.java index d62a19a14..e2fb5561d 100644 --- a/conductor-client/src/test/java/io/orkes/conductor/client/http/SchedulerResourceTest.java +++ b/conductor-client/src/test/java/io/orkes/conductor/client/http/SchedulerResourceTest.java @@ -92,9 +92,37 @@ void resumeScheduleUsesTheSameGetThenPutFallback() throws InterruptedException { assertEquals("PUT", takeRequest().getMethod()); } + @Test + void pauseScheduleRetriesWithPutOnMethodNotSupported500() throws InterruptedException { + server.enqueue(new MockResponse().setResponseCode(500).setBody("Request method 'GET' is not supported")); + server.enqueue(new MockResponse().setResponseCode(204)); + + schedulerClient.pauseSchedule("nightly"); + + RecordedRequest get = takeRequest(); + RecordedRequest put = takeRequest(); + assertEquals("GET", get.getMethod()); + assertEquals("PUT", put.getMethod()); + assertEquals("/api/scheduler/schedules/nightly/pause", put.getPath()); + } + + @Test + void resumeScheduleRetriesWithPutOnMethodNotSupported500() throws InterruptedException { + server.enqueue(new MockResponse().setResponseCode(500).setBody("Request method 'GET' is not supported")); + server.enqueue(new MockResponse().setResponseCode(204)); + + schedulerClient.resumeSchedule("nightly"); + + RecordedRequest get = takeRequest(); + RecordedRequest put = takeRequest(); + assertEquals("GET", get.getMethod()); + assertEquals("PUT", put.getMethod()); + assertEquals("/api/scheduler/schedules/nightly/resume", put.getPath()); + } + @Test void pauseScheduleDoesNotRetryNon405Failures() throws InterruptedException { - server.enqueue(new MockResponse().setResponseCode(500)); + server.enqueue(new MockResponse().setResponseCode(500).setBody("Internal server error")); assertThrows(RuntimeException.class, () -> schedulerClient.pauseSchedule("nightly"));