test: document and pin that a body-less non-idempotent request is not retried (#87)

* test: pin and document that a body-less non-idempotent request is not retried

The retry-safety gate keys body-less requests off method idempotency rather
than off the absence of a body: with no body the request is retried only when
its method is idempotent, and with a body only when the body is replayable. A
consequence that was easy to miss is that a bare POST (a body-less,
non-idempotent request, e.g. a trigger/activate-style endpoint) is NOT retried
even though it carries no payload to re-send — replaying it could duplicate a
side effect the server may already have applied.

This behavior was intended but undocumented and untested. Make it explicit:

- Spell out the body-less branch in the KDoc of DefaultRetryStep (isRetrySafe
  and the "Body replayability" section) and the recovery-aware RetryStep
  (canRetry), stating that body-less retry safety keys off method idempotency.
- Tighten the retry idempotency note in docs/pipelines.md to match the actual
  per-axis gate and call out the bare-POST case.
- Add regression tests: a body-less POST against a retryable 503 results in
  exactly one attempt (no retry), with a body-less PUT control proving the
  same branch retries an idempotent method.

KDoc, docs, and tests only; no public API or behavior change.

* test: mirror body-less retry-eligibility cases in the recovery-pipeline RetryStep suite

The http.pipeline DefaultRetryStep suite pins that a body-less request's
retry eligibility keys off method idempotency (bare POST not retried,
body-less PUT retried). The recovery-oriented pipeline.step.retry.RetryStep
gate has the same rule but only covered body-bearing requests
(non-replayable POST/PUT). Add the two body-less cases there so both gates
carry the regression, and cross-reference the mirrored cases in both suites.

Author: OmarAlJarrah

AUDIT.md

@@ -362,9 +362,14 @@ It retries only when the outcome is a `Failure` whose throwable is classified re
   `RetrySettings.retryableStatuses`.
 - A `NetworkException` (a transport failure with no response on the wire — always retryable).
 
-Idempotency is enforced independently of classification: a request is eligible only when its
-method is in `RetrySettings.retryableMethods` **or** its body is replayable. Non-idempotent
-methods (`POST`/`PATCH`) with a non-replayable body are never re-sent.
+Idempotency is enforced independently of classification, keyed off whether the request carries
+a body. A request **with a body** is eligible only when its body is replayable (a non-replayable
+body cannot be re-sent — the second `writeTo` would trip its consume-once guard). A request
+**with no body** is eligible only when its method is in `RetrySettings.retryableMethods`.
+Body-less retry safety keys off method idempotency, not off the absence of a body: a body-less
+non-idempotent request — a bare `POST`/`PATCH` to a trigger / activate-style endpoint — is
+therefore never re-sent, even though there is no payload to replay, because the server may
+already have applied the side effect.
 
 Waits between attempts use a `ScheduledExecutorService` plus `CompletableFuture.get`, never
 `Thread.sleep`, so virtual-thread carriers can unmount during the delay. An interrupt restores

docs/openai-java-deep-dive.md

@@ -46,15 +46,20 @@ import java.util.concurrent.ThreadLocalRandom
  *
  * ## Body replayability
  *
- * Eligibility is gated on re-sendability. A body-less request is retried only when its method
- * is idempotent; a body-bearing request is retried only when its body is replayable — a
- * non-replayable body physically cannot be re-sent (the second `writeTo` trips the body's
- * consume-once guard and surfaces as a confusing wrapped [IllegalStateException] that masks the
- * real failure). A replayable body is re-sendable regardless of method; making the re-sent
- * request idempotent (for a non-idempotent method, e.g. via an idempotency key) is the caller's
- * responsibility. When the request is not re-sendable the loop runs exactly one attempt and
- * returns the response (or rethrows the exception) as-is. This mirrors
- * `pipeline.step.retry.RetryStep.canRetry`.
+ * Eligibility is gated on re-sendability, keyed off whether the request carries a body:
+ *  - **No body** — retried only when the method is idempotent ([IDEMPOTENT_METHODS]). Body-less
+ *    retry safety keys off method idempotency, NOT off the absence of a body, so a body-less
+ *    non-idempotent request — e.g. a bare `POST` to a trigger / activate-style endpoint — is NOT
+ *    retried even though there is no payload to re-send: replaying it could duplicate the side
+ *    effect the server may already have applied.
+ *  - **Has a body** — retried only when the body is replayable. A non-replayable body physically
+ *    cannot be re-sent (the second `writeTo` trips the body's consume-once guard and surfaces as
+ *    a confusing wrapped [IllegalStateException] that masks the real failure). A replayable body
+ *    is re-sendable regardless of method; making the re-sent request idempotent (for a
+ *    non-idempotent method, e.g. via an idempotency key) is the caller's responsibility.
+ *
+ * When the request is not re-sendable the loop runs exactly one attempt and returns the response
+ * (or rethrows the exception) as-is. This mirrors `pipeline.step.retry.RetryStep.canRetry`.
  *
  * ## Delay precedence (highest to lowest)
  *
@@ -246,11 +251,14 @@ public open class DefaultRetryStep
 
         /**
          * Returns `true` when [request] may be re-sent. A body-less request is retry-safe only
-         * when its method is idempotent; a body-bearing request is retry-safe only when its body
-         * is replayable — a non-replayable body cannot be re-sent (the second
-         * `RequestBody.writeTo` trips the body's consume-once guard and surfaces as a confusing
-         * wrapped [IllegalStateException]). Making a re-sent body-bearing request idempotent is
-         * the caller's responsibility. Mirrors `pipeline.step.retry.RetryStep.canRetry`.
+         * when its method is idempotent ([IDEMPOTENT_METHODS]) — the gate keys off method
+         * idempotency, not off the absence of a body, so a body-less non-idempotent request (a
+         * bare `POST`) is NOT retry-safe even though there is no payload to re-send. A body-bearing
+         * request is retry-safe only when its body is replayable — a non-replayable body cannot be
+         * re-sent (the second `RequestBody.writeTo` trips the body's consume-once guard and
+         * surfaces as a confusing wrapped [IllegalStateException]). Making a re-sent body-bearing
+         * request idempotent is the caller's responsibility. Mirrors
+         * `pipeline.step.retry.RetryStep.canRetry`.
          */
         private fun isRetrySafe(request: Request): Boolean {
             val body = request.body ?: return request.method in IDEMPOTENT_METHODS

docs/pipelines.md

@@ -344,9 +344,11 @@ public class RetryStep
 
         /**
          * Returns true when [request] is safe to retry. A body-less request is retry-safe only
-         * when its method is in [RetrySettings.retryableMethods] (idempotent); a body-bearing
-         * request is retry-safe only when its body is replayable — a non-replayable body cannot
-         * be re-sent (the second `writeTo` trips the consume-once guard). Ensuring a re-sent
+         * when its method is in [RetrySettings.retryableMethods] (idempotent) — the gate keys off
+         * method idempotency, not off the absence of a body, so a body-less non-idempotent request
+         * (a bare `POST`) is NOT retried even though there is no payload to re-send. A body-bearing
+         * request is retry-safe only when its body is replayable — a non-replayable body cannot be
+         * re-sent (the second `writeTo` trips the consume-once guard). Ensuring a re-sent
          * body-bearing request is idempotent is the caller's responsibility.
          */
         private fun canRetry(request: Request): Boolean {

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt

@@ -685,6 +685,61 @@ class RetryStepTest {
         assertEquals(2, fake.callCount, "replayable POST must retry")
     }
 
+    @Test
+    fun `body-less POST is NOT retried on a retryable response`() {
+        // Body-less retry safety keys off METHOD idempotency, not off the absence of a body. A
+        // bare POST (no payload to re-send) is non-idempotent, so it must NOT be retried even on a
+        // retryable status — a second POST could duplicate a side effect the server already
+        // applied. The 503 is returned as-is after exactly one attempt. This exercises the
+        // body == null branch of isRetrySafe on a non-idempotent method. Mirrored by the
+        // `body-less POST is not retried` case in the pipeline.step.retry RetryStep suite.
+        val fake =
+            FakeHttpClient()
+                .enqueue { status(503) }
+                .enqueue { status(200) } // must never be reached
+
+        val pipeline =
+            HttpPipelineBuilder(fake)
+                .append(DefaultRetryStep(HttpRetryOptions(maxRetries = 3), zeroDelayClock()))
+                .build()
+
+        val request =
+            Request.builder()
+                .method(Method.POST)
+                .url("https://api.example.com/x")
+                .build()
+
+        val response = pipeline.send(request)
+        assertEquals(503, response.status.code)
+        assertEquals(1, fake.callCount, "body-less POST must not be retried — POST is non-idempotent")
+    }
+
+    @Test
+    fun `body-less PUT IS retried because PUT is idempotent`() {
+        // Control for the body == null branch: with no body the gate falls through to method
+        // idempotency. PUT is idempotent, so a body-less PUT is retry-safe and retries normally.
+        // Mirrored by the `body-less PUT is retried` case in the pipeline.step.retry RetryStep suite.
+        val fake =
+            FakeHttpClient()
+                .enqueue { status(503) }
+                .enqueue { status(200) }
+
+        val pipeline =
+            HttpPipelineBuilder(fake)
+                .append(DefaultRetryStep(HttpRetryOptions(maxRetries = 3), zeroDelayClock()))
+                .build()
+
+        val request =
+            Request.builder()
+                .method(Method.PUT)
+                .url("https://api.example.com/x")
+                .build()
+
+        val response = pipeline.send(request)
+        assertEquals(200, response.status.code)
+        assertEquals(2, fake.callCount, "body-less PUT must retry — PUT is idempotent")
+    }
+
     @Test
     fun `non-replayable PUT body is NOT retried even though PUT is idempotent`() {
         // Both gates are required: PUT is idempotent, but a non-replayable body physically

sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt

@@ -167,6 +167,18 @@ class RetryStepTest {
             .build()
     }
 
+    private fun requestPostBodyless(): Request =
+        Request.builder()
+            .url("https://api.example.com/resource")
+            .method(Method.POST)
+            .build()
+
+    private fun requestPutBodyless(): Request =
+        Request.builder()
+            .url("https://api.example.com/resource")
+            .method(Method.PUT)
+            .build()
+
     private fun response(
         status: Int,
         headers: Headers = Headers.builder().build(),
@@ -248,6 +260,39 @@ class RetryStepTest {
         assertTrue(client.calls.isEmpty())
     }
 
+    @Test
+    fun `503 with body-less POST is not retried because POST is non-idempotent`() {
+        // Body-less retry safety keys off METHOD idempotency, not off the absence of a body. A
+        // bare POST has no payload to re-send, but it is non-idempotent, so it must NOT be retried
+        // — a second POST could duplicate a side effect the server already applied. Exercises the
+        // body == null branch of canRetry on a non-idempotent method. Mirrors the
+        // `body-less POST is NOT retried` case in the http.pipeline DefaultRetryStep suite.
+        val client = FakeClient()
+        val request = requestPostBodyless()
+        val step = RetryStep(client, zeroDelaySettings(InstantScheduler()), request)
+        val outcome = ResponseOutcome.Failure(httpException(SC_SERVICE_UNAVAILABLE))
+        val out = step.invoke(outcome)
+        assertSame(outcome, out, "Outcome must be unchanged — a body-less POST is non-idempotent")
+        assertTrue(client.calls.isEmpty(), "body-less POST must not be re-dispatched")
+    }
+
+    @Test
+    fun `503 with body-less PUT is retried because PUT is idempotent`() {
+        // Control for the body == null branch: with no body the gate falls through to method
+        // idempotency. PUT is idempotent, so a body-less PUT is retry-safe and retries normally —
+        // here the single retry succeeds. Mirrors the `body-less PUT IS retried` control in the
+        // http.pipeline DefaultRetryStep suite.
+        val ok = response(SC_OK)
+        val client = FakeClient(listOf(Canned.Ok(ok)))
+        val request = requestPutBodyless()
+        val step = RetryStep(client, zeroDelaySettings(InstantScheduler()), request)
+        val outcome = ResponseOutcome.Failure(httpException(SC_SERVICE_UNAVAILABLE))
+        val out = step.invoke(outcome)
+        assertTrue(out is ResponseOutcome.Success, "body-less PUT must retry — PUT is idempotent")
+        assertSame(ok, (out as ResponseOutcome.Success).response)
+        assertEquals(1, client.calls.size)
+    }
+
     @Test
     fun `404 is not retried`() {
         val client = FakeClient()