fix: Imposter.delete() of an absent imposter succeeds over HTTP but throws embedded (rift#1371)

docs/design/sdk-api.md

@@ -255,7 +255,7 @@ public interface RiftTransport extends AutoCloseable {
   JsonValue createImposter(JsonValue definition);          // returns created definition (with port)
   JsonValue getImposter(int port);                         // ImposterNotFound on 404
   JsonValue getImposter(int port, boolean replayable, boolean removeProxies);
-  void deleteImposter(int port);
+  void deleteImposter(int port);                           // idempotent: an absent port is a no-op (rift#1371)
   void deleteAll();
   JsonValue listImposters(boolean replayable, boolean removeProxies);
   void replaceAllImposters(JsonValue impostersDoc);
@@ -384,7 +384,7 @@ public interface Imposter {
   // -- lifecycle --
   void enable(); void disable();
   boolean isEnabled();                        // definition().enabled(): the engine's current view, one GET
-  void delete();                              // DELETE /imposters/:port; handle unusable after
+  void delete();                              // DELETE /imposters/:port; handle unusable after; idempotent (an absent imposter is a no-op, rift#1371)
 }
 
 public interface Recording extends AutoCloseable {

rift-java-conformance/src/test/java/io/github/achirdlabs/rift/conformance/ImposterDeleteIT.java

@@ -0,0 +1,36 @@
+package io.github.achirdlabs.rift.conformance;
+
+import io.github.achirdlabs.rift.Imposter;
+import io.github.achirdlabs.rift.Rift;
+import org.junit.jupiter.api.DynamicTest;
+import org.junit.jupiter.api.TestFactory;
+
+import java.util.Optional;
+import java.util.stream.Stream;
+
+import static io.github.achirdlabs.rift.conformance.LiveEngine.engine;
+import static io.github.achirdlabs.rift.conformance.LiveEngine.gated;
+import static io.github.achirdlabs.rift.dsl.RiftDsl.imposter;
+import static io.github.achirdlabs.rift.dsl.RiftDsl.ok;
+import static io.github.achirdlabs.rift.dsl.RiftDsl.onGet;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/** {@code Imposter.delete()} is idempotent on every transport (#308, rift#1371). Needs {@code RIFT_IT=1}. */
+class ImposterDeleteIT {
+
+    @TestFactory
+    Stream<DynamicTest> deleteIsIdempotent() {
+        return gated("deleting an imposter twice succeeds, and it is gone", () -> {
+            try (Rift rift = engine()) {
+                Imposter created = rift.create(imposter("gone").stub(onGet("/").willReturn(ok())));
+                int port = created.port();
+                assertTrue(rift.imposter(port).isPresent());
+
+                created.delete();
+                created.delete();
+                assertEquals(Optional.empty(), rift.imposter(port));
+            }
+        });
+    }
+}

rift-java-core/src/main/java/io/github/achirdlabs/rift/Imposter.java

@@ -232,5 +232,14 @@ public interface Imposter {
      */
     boolean isEnabled();
 
+    /**
+     * Deletes this imposter, freeing its port. Idempotent, as in Mountebank (rift#1371): deleting
+     * an imposter that is already gone succeeds, on every transport and engine version. This handle
+     * is unusable afterwards. To tell whether an imposter still exists, use {@link Rift#imposter(int)}.
+     *
+     * @throws io.github.achirdlabs.rift.error.EngineError if the engine fails to delete it and it is
+     *         still registered
+     * @throws io.github.achirdlabs.rift.error.EngineUnavailable if the engine cannot be reached
+     */
     void delete();
 }

rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RemoteTransport.java

@@ -40,8 +40,9 @@
  * EngineUnavailable}; HTTP 400 becomes {@link InvalidDefinition}; HTTP 404 on a
  * {@code /imposters/{port}} path becomes {@link ImposterNotFound}; a 2xx response whose body
  * fails to parse becomes {@link CommunicationError}; any other non-2xx status becomes {@link
- * EngineError}. {@link #flowStateGet} is the one exception: a 404 there means "no such key" and
- * is reported as {@link Optional#empty()}, not an exception.
+ * EngineError}. Two 404s are not errors: {@link #flowStateGet}'s means "no such key" and is
+ * reported as {@link Optional#empty()}, and {@link #deleteImposter}'s means the imposter is already
+ * gone, which a delete does not refuse.
  */
 public final class RemoteTransport implements RiftTransport {
 
@@ -100,7 +101,11 @@ public JsonValue getImposter(int port, boolean replayable, boolean removeProxies
 
     @Override
     public void deleteImposter(int port) {
-        executeVoid("DELETE", "/imposters/" + port, null, OptionalInt.of(port));
+        HttpResponse<String> response = send("DELETE", "/imposters/" + port, null);
+        // An engine before 0.22.0 answers a missing port 404; from 0.22.0 it is 200 {} (rift#1371).
+        if (!isSuccess(response.statusCode()) && response.statusCode() != 404) {
+            throw mapError(response, OptionalInt.of(port));
+        }
     }
 
     @Override

rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RiftTransport.java

@@ -37,6 +37,10 @@ public interface RiftTransport extends AutoCloseable {
      */
     JsonValue getImposter(int port, boolean replayable, boolean removeProxies);
 
+    /**
+     * Deletes the imposter on {@code port}. Idempotent: a port with no imposter is not an error
+     * (rift#1371); only a delete that leaves the imposter in place, or an unreachable engine, throws.
+     */
     void deleteImposter(int port);
 
     void deleteAll();

rift-java-core/src/test/java/io/github/achirdlabs/rift/transport/RemoteTransportGateTest.java

@@ -7,7 +7,6 @@
 import io.github.achirdlabs.rift.error.CommunicationError;
 import io.github.achirdlabs.rift.error.EngineError;
 import io.github.achirdlabs.rift.error.EngineUnavailable;
-import io.github.achirdlabs.rift.error.ImposterNotFound;
 import io.github.achirdlabs.rift.error.InvalidDefinition;
 import org.junit.jupiter.api.Test;
 
@@ -77,14 +76,42 @@ void missingImposterOnGetReturnsEmpty() {
     }
 
     @Test
-    void deleteMissingImposterThrowsImposterNotFound() {
+    void deletingAnImposterAnOlderEngine404sIsANoOp() {
+        // engines before 0.22.0 answered a missing port with 404; delete is idempotent on every engine (#308)
         try (FakeAdminServer server = new FakeAdminServer()) {
             server.respond("POST /imposters", 201, IMPOSTER_JSON);
             server.respond("DELETE /imposters/4545", 404, "{\"errors\":[{\"code\":\"no such resource\",\"message\":\"missing\"}]}");
             try (Rift rift = connectNoPreflight(server)) {
                 Imposter imp = rift.create(imposter("x").port(4545));
-                ImposterNotFound ex = assertThrows(ImposterNotFound.class, imp::delete);
-                assertEquals(4545, ex.port());
+                imp.delete();
+                assertEquals(1, server.received().stream().filter(r -> r.method().equals("DELETE") && r.path().equals("/imposters/4545")).count());
+            }
+        }
+    }
+
+    @Test
+    void deletingAnAbsentImposterOnA022EngineIsANoOp() {
+        // rift 0.22.0 answers it 200 {} (rift#1371), Mountebank's idempotent teardown
+        try (FakeAdminServer server = new FakeAdminServer()) {
+            server.respond("POST /imposters", 201, IMPOSTER_JSON);
+            server.respond("DELETE /imposters/4545", 200, "{}");
+            try (Rift rift = connectNoPreflight(server)) {
+                rift.create(imposter("x").port(4545)).delete();
+                assertEquals(1, server.received().stream().filter(r -> r.method().equals("DELETE") && r.path().equals("/imposters/4545")).count());
+            }
+        }
+    }
+
+    @Test
+    void aFailedDeleteStillThrows() {
+        try (FakeAdminServer server = new FakeAdminServer()) {
+            server.respond("POST /imposters", 201, IMPOSTER_JSON);
+            server.respond("DELETE /imposters/4545", 500, "{\"errors\":[{\"code\":\"internal\",\"message\":\"datadir unlink failed\"}]}");
+            try (Rift rift = connectNoPreflight(server)) {
+                Imposter imp = rift.create(imposter("x").port(4545));
+                EngineError ex = assertThrows(EngineError.class, imp::delete);
+                assertEquals(500, ex.code());
+                assertEquals("datadir unlink failed", ex.getMessage());
             }
         }
     }

rift-java-embedded/src/main/java/io/github/achirdlabs/rift/embedded/FfiCalls.java

@@ -66,8 +66,27 @@ void replaceStubs(int port, JsonValue stubs) {
 
     void deleteImposter(int port) {
         ensureLive();
-        if (ffi.deleteImposter(handle, port) != 0) {
-            throw engineError();
+        if (ffi.deleteImposter(handle, port) == 0) {
+            return;
+        }
+        // rift_delete_imposter answers -1 for a missing port too, but delete is idempotent over HTTP
+        // (rift#1371). Read the error first: the probe clears it. A failed delete leaves the imposter
+        // registered, so its presence tells a real failure from an absent one.
+        RuntimeException failure = engineError();
+        if (imposterExists(port)) {
+            throw failure;
+        }
+    }
+
+    private boolean imposterExists(int port) {
+        try (Arena args = Arena.ofConfined()) {
+            MemorySegment probe = ffi.getImposter(handle, port,
+                    FfmCompat.allocateCString(args, listOptionsJson(false, false)));
+            if (RiftFfi.isNull(probe)) {
+                return false;
+            }
+            ffi.free(probe);
+            return true;
         }
     }
 

rift-java-embedded/src/test/java/io/github/achirdlabs/rift/embedded/EmbeddedTransportIT.java

@@ -138,6 +138,17 @@ void flowStateClearEmptiesOneFlowAndIsIdempotent() {
         }
     }
 
+    @Test
+    void deletingAnImposterTwiceIsIdempotent() {
+        // rift_delete_imposter answers -1 for a missing port; the SDK's delete is idempotent like HTTP's (rift#1371)
+        try (EmbeddedTransport t = open()) {
+            int port = portOf(t.createImposter(flowStateImposter()));
+            t.deleteImposter(port);
+            t.deleteImposter(port);
+            t.deleteImposter(45999);
+        }
+    }
+
     @Test
     void flowStateRoundTrips() {
         try (EmbeddedTransport t = open()) {