fleetd #302: require content in MessageService.reply so a REST reply with no content cannot silently resolve a waiter
This commit is contained in:
@@ -408,10 +408,28 @@ public final class MessageService {
|
||||
* as the zero-candidate case does, and let {@link #abandon} apply the eventual recovery
|
||||
* deterministically instead.
|
||||
*
|
||||
* <p><strong>{@code content} is required (fleetd #302).</strong> 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
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
|
||||
@@ -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<String> 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<String> goodReply = postJson(port, "/sessions/term_a/reply", "{\"content\":\"LGTM ship it\"}");
|
||||
assertEquals(200, goodReply.statusCode());
|
||||
|
||||
HttpResponse<String> 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<String> empty = postJson(port, "/sessions/term_a/reply", "{\"content\":\"\"}");
|
||||
assertEquals(400, empty.statusCode());
|
||||
assertEquals("bad_request", mapper.readTree(empty.body()).get("error").asText());
|
||||
|
||||
HttpResponse<String> 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();
|
||||
|
||||
Reference in New Issue
Block a user