fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

POST /members and DELETE /members/{paneId} turn a herdr failure into a bare 500, and a stop that already succeeded can never report success #304

New Issue
Closed
opened 2026-09-04 07:43:27 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 07:43:27 +02:00
Owner
Copy Link
Copy Source

The two siblings deferred from #297's shape check. I said I would decide on them; I have read the code and I am fixing them. This ticket records why, because my earlier judgement of them was wrong.

What I said when I deferred them

those produce a visible bare 500 and are already an accepted shape in that file with passing tests

The first half is true. The second half is the mistake: "visible" is not the same as "actionable", and for stopMember the 500 is not even honest.

The asymmetry

FleetMcp catches HerdrException on both calls:

// FleetMcp.stop :1323
} catch (HerdrException e) {
    return error("herdr error stopping " + paneId + ": " + e.getMessage());
}
// FleetMcp.spawn :973
} catch (HerdrException e) {
    return error("herdr error spawning worker: " + e.getMessage());
}

FleetApp catches it on agents(), listMembers(), sendMessage, sessionStatus and others — but not on these two. FleetApp has no Javalin exception mapper, so an escaped HerdrException is Javalin's default 500. That is the same one-door-guarded shape as #297.

Why stopMember is worse than I judged

SessionManager.release does its work in this order:

  1. registry.remove(paneId) — the session is deregistered
  2. the release listener is notified, so a blocked fleet_send fails fast
  3. a dirty worktree is snapshotted and preserved
  4. launcher.stop(paneId) — bare, no try/catch
  5. the worktree is removed

So by the time step 4 throws, steps 1–3 have already happened. The teardown succeeded from the caller's point of view: the session is gone from the roster and nothing can be sent to it. The REST caller is told 500.

Then the caller retries, because that is what a 500 means. The second call finds nothing in the registry, reaches step 4 again, and throws again. The retry can never succeed. The caller has no way to learn the session is already gone.

Reaching step 4's throw is ordinary, not exotic: stopping a pane that already exited on its own, a double stop, or herdr being briefly unreachable.

The fix

Route both through the existing herdrError mapper, which every other herdr-calling route in the file already uses:

private static void herdrError(Context ctx, HerdrException e) {
    if (e.code() != null && e.code().endsWith("_not_found")) {
        ctx.status(404).json(Map.of("error", "session_not_found", "detail", e.getMessage()));
    } else {
        ctx.status(502).json(Map.of("error", "herdr_error", "detail", e.getMessage()));
    }
}

That turns both cases into an answer the caller can act on: 404 means the pane is gone, stop retrying; 502 means herdr is upstream and broken, a retry may help. It also matches what the MCP door reports for the same failure, which is the point of #297.

I am not changing release's ordering. Steps 1–3 running before the pane stop is deliberate (CB-516/CB-581: a blocked caller must fail fast, and the pane must always be attempted). The defect is that the REST door drops the resulting exception on the floor, not that the exception happens.

The two siblings deferred from #297's shape check. I said I would decide on them; I have read the code and I am fixing them. This ticket records why, because my earlier judgement of them was wrong. ## What I said when I deferred them > those produce a visible bare 500 and are already an accepted shape in that file with passing tests The first half is true. The second half is the mistake: "visible" is not the same as "actionable", and for `stopMember` the 500 is not even honest. ## The asymmetry `FleetMcp` catches `HerdrException` on both calls: ```java // FleetMcp.stop :1323 } catch (HerdrException e) { return error("herdr error stopping " + paneId + ": " + e.getMessage()); } // FleetMcp.spawn :973 } catch (HerdrException e) { return error("herdr error spawning worker: " + e.getMessage()); } ``` `FleetApp` catches it on `agents()`, `listMembers()`, `sendMessage`, `sessionStatus` and others — but not on these two. `FleetApp` has no Javalin exception mapper, so an escaped `HerdrException` is Javalin's default 500. That is the same one-door-guarded shape as #297. ## Why `stopMember` is worse than I judged `SessionManager.release` does its work in this order: 1. `registry.remove(paneId)` — the session is deregistered 2. the release listener is notified, so a blocked `fleet_send` fails fast 3. a dirty worktree is snapshotted and preserved 4. **`launcher.stop(paneId)`** — bare, no try/catch 5. the worktree is removed So by the time step 4 throws, steps 1–3 have already happened. The teardown succeeded from the caller's point of view: the session is gone from the roster and nothing can be sent to it. The REST caller is told `500`. Then the caller retries, because that is what a 500 means. The second call finds nothing in the registry, reaches step 4 again, and throws again. **The retry can never succeed.** The caller has no way to learn the session is already gone. Reaching step 4's throw is ordinary, not exotic: stopping a pane that already exited on its own, a double stop, or herdr being briefly unreachable. ## The fix Route both through the existing `herdrError` mapper, which every other herdr-calling route in the file already uses: ```java private static void herdrError(Context ctx, HerdrException e) { if (e.code() != null && e.code().endsWith("_not_found")) { ctx.status(404).json(Map.of("error", "session_not_found", "detail", e.getMessage())); } else { ctx.status(502).json(Map.of("error", "herdr_error", "detail", e.getMessage())); } } ``` That turns both cases into an answer the caller can act on: 404 means the pane is gone, stop retrying; 502 means herdr is upstream and broken, a retry may help. It also matches what the MCP door reports for the same failure, which is the point of #297. I am not changing `release`'s ordering. Steps 1–3 running before the pane stop is deliberate (CB-516/CB-581: a blocked caller must fail fast, and the pane must always be attempted). The defect is that the REST door drops the resulting exception on the floor, not that the exception happens.
ltms closed this issue 2026-09-04 07:48:27 +02:00
ltms commented 2026-09-04 07:48:47 +02:00
Author
Owner
Copy Link
Copy Source

Fixed in 21ff63b, on main. One claim in the issue body above was wrong and I am correcting it here rather than editing it away.

The correction

I wrote that a double stop reaches launcher.stop's throw and so "the retry can never succeed". That is false. HerdrPeerLauncher.stop already tolerates an already-gone pane:

try {
    agents.close(paneId);
} catch (HerdrException e) {
    if (!isAlreadyGone(e)) throw e;   // *_not_found is treated as success
    log.debug("pane.close({}) ignored — already gone: {}", paneId, e.getMessage());
}

and FleetAppTest.stopToleratesAnAlreadyGonePane has pinned that since before this ticket. A double stop returns 204, correctly. I asserted a path into the bad state without checking that the path was open — the exact mistake I have a standing note about.

What is actually reachable, and what I measured:

  • pane.close failing for any reason that is not *_not_found — a herdr transport failure, or a protocol error like herdr_busy. That propagates.
  • agent.start failing with a herdr error on the spawn path. The daemon log line from my test run: spawn failed for profile=null role=DEV: herdr error [internal_error]: agent.start failed. PeerUnreachableException above it does not cover this, because it is not a readiness timeout.

So the defect is real and the fix is right, but it is narrower than I described: the harm is a bare 500 with the body Server Error carrying no herdr detail, not an unbreakable retry loop.

What changed

Both routes now go through the existing herdrError mapper. I did not add new tests. Two tests already covered these paths and asserted 500:

  • spawnClosesTheCreatedTabWhenTheWorkerNeverStarts
  • stopReportsFailureWhenPaneCloseFailsForARealReason

I wrote three new tests first, then deleted them — one duplicated stopToleratesAnAlreadyGonePane outright, and the other two duplicated these. Changing an assertion a previous change deliberately wrote needs a reason, so here it is: neither test is about the status code. The first guards that a failed spawn still closes its tab; the second guards that a failed teardown is not reported as a successful 204, and says so in its own comment. 502 preserves both properties and adds the herdr detail the caller could not see before. I updated the two expectations to 502 and added a body assertion to each.

Mutation proof

Reverted only FleetApp.java (git apply -R), kept the tests:

[ERROR] Tests run: 2, Failures: 2, Errors: 0, Skipped: 0 -- in dev.ltms.fleet.rest.FleetAppTest
org.opentest4j.AssertionFailedError: expected: <502> but was: <500>
org.opentest4j.AssertionFailedError: expected: <502> but was: <500>

Before the fix the spawn body was literally Server Error. Restored, then mvn clean install: Tests run: 1310, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Not changed

SessionManager.release's ordering. Steps 1–3 running before the pane stop is deliberate (CB-516/CB-581): a blocked caller must fail fast, and the pane must always be attempted. The defect was the REST door dropping the resulting exception, not the exception happening.

Fixed in `21ff63b`, on `main`. **One claim in the issue body above was wrong and I am correcting it here rather than editing it away.** ## The correction I wrote that a double stop reaches `launcher.stop`'s throw and so "the retry can never succeed". That is false. `HerdrPeerLauncher.stop` already tolerates an already-gone pane: ```java try { agents.close(paneId); } catch (HerdrException e) { if (!isAlreadyGone(e)) throw e; // *_not_found is treated as success log.debug("pane.close({}) ignored — already gone: {}", paneId, e.getMessage()); } ``` and `FleetAppTest.stopToleratesAnAlreadyGonePane` has pinned that since before this ticket. A double stop returns 204, correctly. I asserted a path into the bad state without checking that the path was open — the exact mistake I have a standing note about. **What is actually reachable**, and what I measured: - `pane.close` failing for any reason that is *not* `*_not_found` — a herdr transport failure, or a protocol error like `herdr_busy`. That propagates. - `agent.start` failing with a herdr error on the spawn path. The daemon log line from my test run: `spawn failed for profile=null role=DEV: herdr error [internal_error]: agent.start failed`. `PeerUnreachableException` above it does not cover this, because it is not a readiness timeout. So the defect is real and the fix is right, but it is narrower than I described: the harm is a bare 500 with the body `Server Error` carrying no herdr detail, not an unbreakable retry loop. ## What changed Both routes now go through the existing `herdrError` mapper. I did **not** add new tests. Two tests already covered these paths and asserted `500`: - `spawnClosesTheCreatedTabWhenTheWorkerNeverStarts` - `stopReportsFailureWhenPaneCloseFailsForARealReason` I wrote three new tests first, then deleted them — one duplicated `stopToleratesAnAlreadyGonePane` outright, and the other two duplicated these. **Changing an assertion a previous change deliberately wrote needs a reason, so here it is:** neither test is about the status code. The first guards that a failed spawn still closes its tab; the second guards that a failed teardown is not reported as a successful 204, and says so in its own comment. `502` preserves both properties and adds the herdr detail the caller could not see before. I updated the two expectations to `502` and added a body assertion to each. ## Mutation proof Reverted only `FleetApp.java` (`git apply -R`), kept the tests: ``` [ERROR] Tests run: 2, Failures: 2, Errors: 0, Skipped: 0 -- in dev.ltms.fleet.rest.FleetAppTest org.opentest4j.AssertionFailedError: expected: <502> but was: <500> org.opentest4j.AssertionFailedError: expected: <502> but was: <500> ``` Before the fix the spawn body was literally `Server Error`. Restored, then `mvn clean install`: `Tests run: 1310, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. ## Not changed `SessionManager.release`'s ordering. Steps 1–3 running before the pane stop is deliberate (CB-516/CB-581): a blocked caller must fail fast, and the pane must always be attempted. The defect was the REST door dropping the resulting exception, not the exception happening.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#304
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?