From 21ff63b11d52f5d2d9690b7cb7164459c703f275 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 12:48:21 +0700 Subject: [PATCH] #304: the member routes report a herdr failure instead of a bare 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /members and DELETE /members/{paneId} were the two routes in FleetApp with no catch (HerdrException). FleetApp has no Javalin exception mapper, so the exception escaped as the default 500 with the body "Server Error" — no herdr code, no herdr message. fleet_spawn and fleet_stop catch the same exception and report a named error, so this was the same one-door-guarded shape as #297. Both now go through the existing herdrError mapper: 404 when herdr says the target is gone, 502 otherwise. That is an answer the caller can act on. stopMember matters more than spawnMember. SessionManager.release deregisters the session, notifies the release listener and preserves a dirty worktree before it calls launcher.stop, so a throw from that stop arrives after the teardown the caller asked for has already happened. A bare 500 told the caller to retry and carried nothing to explain what went wrong. The two existing tests that asserted 500 now assert 502 and check the error body. Neither was about the status code: one guards that a failed teardown is not reported as a successful 204, the other that a failed spawn still closes its tab. Both properties are unchanged. Fixes #304 --- .../java/dev/ltms/fleet/rest/FleetApp.java | 26 +++++++++++++++++-- .../dev/ltms/fleet/rest/FleetAppTest.java | 14 ++++++++-- 2 files changed, 36 insertions(+), 4 deletions(-) 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 2541660..2ebf041 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java @@ -510,6 +510,12 @@ public final class FleetApp { ctx.status(400).json(Map.of("error", "unknown_profile", "detail", e.getMessage())); } catch (PeerUnreachableException e) { ctx.status(502).json(Map.of("error", "spawn_timeout", "detail", e.getMessage())); + } catch (HerdrException e) { + // fleetd #304: not every herdr failure on the spawn path is a readiness timeout, so + // PeerUnreachableException above does not cover this. Without this catch the exception + // escapes to Javalin's default 500, while fleet_spawn reports the same failure as a + // clean named error (FleetMcp.spawn) — the #297 one-door-guarded shape. + herdrError(ctx, e); } } @@ -530,13 +536,29 @@ public final class FleetApp { return (s == null || s.isBlank()) ? null : s; } - /** Tear a worker down by pane id. */ + /** + * Tear a worker down by pane id. + * + *

fleetd #304: the {@code HerdrException} catch is not cosmetic. {@code release} deregisters + * the session, notifies the release listener and preserves a dirty worktree before it + * calls {@code launcher.stop}, so a throw from that stop arrives after the teardown the caller + * asked for has already happened. Letting it escape gave Javalin's default 500, which tells the + * caller to retry — and the retry finds nothing in the registry, reaches the same stop, and + * throws again, so it can never succeed. {@code herdrError} instead answers 404 ("the pane is + * gone, stop retrying") or 502 ("herdr is upstream and broken, a retry may help"), matching what + * {@code fleet_stop} reports for the same failure. + */ private void stopMember(Context ctx) { String paneId = ctx.pathParam("paneId"); if (!allow(ctx, routeAction("DELETE /members/{paneId}"), paneId)) { return; } - sessions.release(paneId); + try { + sessions.release(paneId); + } catch (HerdrException e) { + herdrError(ctx, e); + return; + } ctx.status(204); } 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 a3e7a67..be273a1 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/rest/FleetAppTest.java @@ -415,7 +415,11 @@ class FleetAppTest { FakeHerdr herdr = new FakeHerdr().agentNameTakenTimes(99); int port = start(herdr, "http://gx00.gw:8000", Set.of("gx00.gw")); - assertEquals(500, req(port, "POST", "/members").statusCode()); + // fleetd #304: 502, not Javalin's default 500 — the herdr failure is named, and the body + // carries herdr's own message, matching what fleet_spawn reports for the same failure. + HttpResponse res = req(port, "POST", "/members"); + assertEquals(502, res.statusCode()); + assertEquals("herdr_error", mapper.readTree(res.body()).get("error").asText()); assertTrue(herdr.called("tab.create"), "a tab was created before the failed start"); assertEquals("w9:t2", params(herdr, "tab.close").get("tab_id"), "orphaned tab must be closed"); } @@ -733,7 +737,12 @@ class FleetAppTest { int port = start(herdr, "http://gx00.gw:8000", Set.of("gx00.gw")); // A genuine teardown failure must surface, not be reported as a successful 204. - assertEquals(500, req(port, "DELETE", "/members/w9:pW").statusCode()); + // fleetd #304: it surfaces as a named 502 rather than Javalin's default 500. The property + // this test guards is "not 204" and the herdr detail reaching the caller — a bare 500 gave + // the body "Server Error" and said nothing about herdr. + HttpResponse res = req(port, "DELETE", "/members/w9:pW"); + assertEquals(502, res.statusCode()); + assertEquals("herdr_error", mapper.readTree(res.body()).get("error").asText()); assertFalse(herdr.called("tab.close"), "tab is not removed when the pane close failed"); } @@ -746,4 +755,5 @@ class FleetAppTest { assertEquals(204, req(port, "DELETE", "/members/w9:pW").statusCode()); assertTrue(herdr.called("tab.close")); } + }