From bbbb4c1eb3812a7a7a4391506b6bf91132f8195a Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 12:31:09 +0700 Subject: [PATCH] fleetd #302: require content in MessageService.reply so a REST reply with no content cannot silently resolve a waiter --- .../dev/ltms/fleet/msg/MessageService.java | 18 +++++++ .../java/dev/ltms/fleet/rest/FleetApp.java | 14 +++++- .../dev/ltms/fleet/rest/FleetAppTest.java | 47 +++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/msg/MessageService.java b/fleetd/src/main/java/dev/ltms/fleet/msg/MessageService.java index 2f9869a..902f9a4 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/msg/MessageService.java +++ b/fleetd/src/main/java/dev/ltms/fleet/msg/MessageService.java @@ -408,10 +408,28 @@ public final class MessageService { * as the zero-candidate case does, and let {@link #abandon} apply the eventual recovery * deterministically instead. * + *

{@code content} is required (fleetd #302). Both doors that reach this + * method must reject a missing/blank reply the same way, so the check lives here rather than in + * either caller: {@code FleetMcp.reply} already refuses a {@code null} content before it ever + * calls this method (its own required-arg guard), and no test or production call site anywhere + * in the codebase relies on replying with empty content — confirmed by searching every call site + * of this method before adding the check, not assumed. Without this guard, a REST {@code + * POST /sessions/{id}/reply} whose body omits {@code content} (or a client library that maps a + * missing field to {@code ""}) used to reach {@link Rendezvous#resolve} with an empty string, + * silently completing the lead's blocking wait with nothing — indistinguishable from a worker + * that genuinely replied with nothing, which is worse than a loud failure because it destroys the + * information that the reply never arrived. + * + * @throws IllegalArgumentException if {@code content} is {@code null} or blank — the caller must + * report this as a client error (REST: 400 {@code bad_request}) rather than resolve + * anything * @return always {@code true} — the reply resolved a live send, completed a parked ticket, or * was queued */ public boolean reply(String session, String content) { + if (content == null || content.isBlank()) { + throw new IllegalArgumentException("content is required"); + } if (rendezvous.resolve(session, content)) { count(FleetMetrics.REPLIES, "path", "rendezvous"); return true; // a live send took it — unchanged fast path diff --git a/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java b/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java index 56a36fc..2541660 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java @@ -684,7 +684,19 @@ public final class FleetApp { ctx.status(400).json(Map.of("error", "bad_request", "detail", "body must be JSON")); return; } - messages.reply(id, content); + // fleetd #302: content is required. `.path("content").asText("")` above turns a missing key + // into "" rather than throwing, so without this check an empty/blank reply used to reach + // messages.reply(...) and silently resolve the lead's waiter — the same class of bug as the + // sibling "content is required" guards on sendMessage/askMessage below, except this one wrote + // a WRONG value instead of failing loudly. The check lives in MessageService.reply so both + // this door and FleetMcp.reply inherit the same rule; this catch only translates it into the + // {error, detail} envelope this file uses everywhere else. + try { + messages.reply(id, content); + } catch (IllegalArgumentException e) { + ctx.status(400).json(Map.of("error", "bad_request", "detail", e.getMessage())); + return; + } ctx.status(200).json(Map.of("sessionId", id, "delivered", true)); } diff --git a/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java b/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java index dc9166b..a3e7a67 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java @@ -532,6 +532,53 @@ class FleetAppTest { assertEquals("orphan", body.get("replies").get(0).get("content").asText()); } + @Test + void replyWithMissingContentIsRejectedAndDoesNotResolveTheWaiter() throws Exception { + // fleetd #302: `.path("content").asText("")` used to turn a missing "content" key into an + // empty string that reached rendezvous.resolve, silently completing the lead's blocking wait + // with nothing. Prove the fix two ways: the bad call is rejected with 400, AND the real send + // it would have wrongly resolved is still open afterwards — a real reply completes it. + FakeHerdr herdr = new FakeHerdr().agentStatus("idle"); + int port = start(herdr, "http://gx00.gw:8000", Set.of("gx00.gw")); + + var send = java.util.concurrent.CompletableFuture.supplyAsync(() -> { + try { return postMessage(port, "{\"content\":\"review this\",\"timeoutMs\":4000}"); } + catch (Exception e) { throw new RuntimeException(e); } + }); + + Thread.sleep(200); // let the background send open its rendezvous waiter + + HttpResponse badReply = postJson(port, "/sessions/term_a/reply", "{}"); + assertEquals(400, badReply.statusCode()); + JsonNode err = mapper.readTree(badReply.body()); + assertEquals("bad_request", err.get("error").asText()); + assertTrue(err.has("detail")); + + // The waiter must still be open — a real reply now completes the ORIGINAL send. + HttpResponse goodReply = postJson(port, "/sessions/term_a/reply", "{\"content\":\"LGTM ship it\"}"); + assertEquals(200, goodReply.statusCode()); + + HttpResponse res = send.get(6, java.util.concurrent.TimeUnit.SECONDS); + assertEquals(200, res.statusCode()); + assertEquals("LGTM ship it", mapper.readTree(res.body()).get("reply").asText()); + } + + @Test + void replyWithEmptyOrWhitespaceContentIsRejectedSameAsMissing() throws Exception { + // fleetd #302 sibling case: present-but-blank content is treated the same as a missing key — + // FleetMcp's own required-content guard (fleet_reply's "content is required") makes no + // distinction between the two either, so diverging here would be a new asymmetry. + int port = startHealthy(); + + HttpResponse empty = postJson(port, "/sessions/term_a/reply", "{\"content\":\"\"}"); + assertEquals(400, empty.statusCode()); + assertEquals("bad_request", mapper.readTree(empty.body()).get("error").asText()); + + HttpResponse whitespace = postJson(port, "/sessions/term_a/reply", "{\"content\":\" \"}"); + assertEquals(400, whitespace.statusCode()); + assertEquals("bad_request", mapper.readTree(whitespace.body()).get("error").asText()); + } + @Test void drainRepliesReturnsEmptyForNoReplies() throws Exception { int port = startHealthy();