#298: AmqpReplyInbox.release() requeues held deliveries instead of dropping them

release(target) used to cancel the target's consumer and clear held's local
record for it. Cancelling a consumer does not requeue the broker's in-flight
deliveries — they stay unacked on the still-open channel until a real
connection drop. So a held-but-undrained reply became permanently
unreachable: never acked, never nacked, never requeued, invisible to peek.

Fix: cancel the consumer first (so it can no longer receive redeliveries),
then nack-with-requeue every held delivery for that target before dropping
the local record. Nacking before the cancel was tried first but a real
broker demonstrated a race: the still-active consumer immediately received
the requeued message back, racing held.remove and leaving peek non-empty.
Cancelling first avoids that. A failed requeue is logged at WARN and does
not abort release(), matching the best-effort teardown style #293 settled
for HerdrPeerLauncher.stop().

Extends AmqpReplyInboxContractTest.releaseCancelsConsumer... to assert the
held delivery is recoverable via a later own(), not just absent from peek.

fleetd/src/main/java/dev/ltms/fleet/health/FleetHealthMonitor.java

@@ -14,6 +14,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 import java.util.function.BiConsumer;
@@ -28,6 +29,14 @@ public final class FleetHealthMonitor {
     static final int MAX_FAIL_TARGET_ATTEMPTS = 3;
     // CB-641: Match the injector's 60s readiness gate so health allows a full first boot.
     static final long READINESS_GRACE_NANOS = TimeUnit.SECONDS.toNanos(60);
+    /**
+     * fleetd #280: how long after a terminal transition to wait before the one bounded re-check
+     * fires. Must exceed the worst-case reverse-rendezvous {@code fleet_ask} window (55-115s, see
+     * {@code FleetMcp.ASK_DEFAULT_TIMEOUT_MS} / {@code FleetApp.MAX_ASK_TIMEOUT_MS}) so that, if the
+     * target was genuinely {@code ASKING} when {@code state} was first observed, its own ask has had
+     * time to lapse (clearing {@code Task#question} back to {@code null}) before this fires.
+     */
+    static final long ASK_LAPSE_RECHECK_DELAY_SECONDS = 120;
 
     private final AgentControl agents;
     private final Supplier<List<MemberSession>> roster;
@@ -38,7 +47,16 @@ public final class FleetHealthMonitor {
     private final long workingSuspectAfterNanos;
     private final BiConsumer<String, String> failTarget;
     private final Map<String, HealthPrior> priors = new HashMap<>();
-    private final Map<String, HealthState> states = new HashMap<>();
+    /**
+     * The live classification per member, and the only one of this class's three maps that more
+     * than one scheduler task touches. {@code tick} writes it (and prunes it to the roster);
+     * fleetd #280's delayed {@link #recheckTerminalTarget} reads it from its own separate scheduled
+     * task. Both run on the single-threaded scheduler {@code Fleetd} passes in today, so they are
+     * serialised — but nothing in this class enforces that, and an unsynchronised {@link HashMap}
+     * read racing a resize can spin a CPU forever rather than fail visibly. {@code priors} and
+     * {@code orphanStreaks} stay plain maps because {@code tick} is still their only toucher.
+     */
+    private final Map<String, HealthState> states = new ConcurrentHashMap<>();
     /**
      * CB-643: consecutive ticks on which a target looked like an orphaned delegation. The fact
      * {@link MessageService#hasOrphanedDelegation} reports is a true snapshot, but it can read true
@@ -171,6 +189,7 @@ public final class FleetHealthMonitor {
         // member stayed terminal.
         if (terminal(next)) {
             failTerminalTarget(target, next);
+            scheduleTerminalRecheck(target, next);
         }
     }
 
@@ -191,6 +210,48 @@ public final class FleetHealthMonitor {
                 target, state, MAX_FAIL_TARGET_ATTEMPTS, last);
     }
 
+    /**
+     * fleetd #280: schedule the one bounded, delayed follow-up for a terminal transition — never a
+     * per-tick retry (CB-580 rejected that shape; {@link #reportTransition} still fires
+     * {@link #failTerminalTarget} exactly once per transition, unconditionally on the tick loop).
+     * This is a single one-shot task, scheduled once per transition into GONE/NEVER_READY, so a
+     * member stuck terminal for the rest of its life gets exactly one extra attempt, not one per
+     * tick. See {@link #recheckTerminalTarget} for why the extra attempt is safe.
+     */
+    private void scheduleTerminalRecheck(String target, HealthState state) {
+        if (scheduler.isShutdown()) return;
+        try {
+            scheduler.schedule(() -> recheckTerminalTarget(target, state),
+                    ASK_LAPSE_RECHECK_DELAY_SECONDS, TimeUnit.SECONDS);
+        } catch (RuntimeException e) {
+            log.warn("fleet health: could not schedule terminal re-check for member={} state={}",
+                    target, state, e);
+        }
+    }
+
+    /**
+     * fleetd #280: the delayed re-check {@link #scheduleTerminalRecheck} scheduled for one terminal
+     * transition. By now, a {@code fleet_ask} that was still open when {@code state} was first
+     * observed has had time to lapse on its own (see {@link #ASK_LAPSE_RECHECK_DELAY_SECONDS}),
+     * clearing {@code Task#question} back to {@code null} — which is exactly what
+     * {@link MessageService#abandon(String, String, boolean)}'s {@code sweepAsking=false} filter
+     * needs to finally match it. Calling {@link #failTerminalTarget} again is safe only because
+     * {@code sweepAsking} stays {@code false}: a task genuinely still {@code ASKING} is skipped
+     * exactly as it was on the very first attempt — this never fails a ticket whose ask has not yet
+     * lapsed.
+     *
+     * <p><strong>Guarded on "target is still classified {@code state}."</strong> Without this guard,
+     * a member that recovered (or was released and dropped from the roster) between the transition
+     * and this re-check would still take a blind {@code failTarget} call — reaching into whatever
+     * brand-new, unrelated turn it has since picked up and failing it too. {@link #states} already
+     * carries the live classification (updated every tick, pruned to the current roster on release),
+     * so a stale or recovered target simply reads as a mismatch here and this is a no-op.
+     */
+    void recheckTerminalTarget(String target, HealthState state) {
+        if (states.get(target) != state) return;
+        failTerminalTarget(target, state);
+    }
+
     private static boolean terminal(HealthState state) {
         return state == HealthState.GONE || state == HealthState.NEVER_READY;
     }

fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java

@@ -947,7 +947,14 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
             // person can go close by hand.
             try {
                 spaces.closeTab(loc.tabId());
-            } catch (HerdrException e) {
+            } catch (RuntimeException e) {
+                // Caught as RuntimeException, not HerdrException, to match releaseZdotdir's own
+                // guard five lines below. Today the two are the same set — HerdrCodec wraps every
+                // encode/decode failure and UnixSocketHerdrClient wraps every IOException, so
+                // HerdrException is all closeTab can actually throw. Narrowing to it anyway would
+                // leave this step guarded against the expected failure and bare against any other,
+                // which is the exact asymmetry fleetd #293 exists to remove. No behaviour change
+                // today; it stops a later change inside WorkspaceControl.closeTab reopening it.
                 log.warn("tab.close({}) failed — the pane is already torn down, so continuing; the "
                         + "tab may need manual cleanup: {}", loc.tabId(), e.getMessage());
             }

fleetd/src/main/java/dev/ltms/fleet/msg/AmqpReplyInbox.java

@@ -208,18 +208,72 @@ public final class AmqpReplyInbox implements ReplyInbox, AutoCloseable {
         }
     }
 
+    /**
+     * Release ownership of {@code target}: cancel its consumer, then nack-with-requeue every
+     * delivery still held for it instead of just dropping the local record.
+     *
+     * <p><strong>Cancelling a consumer does not requeue its in-flight deliveries.</strong> In AMQP,
+     * a delivery that was pushed to a consumer stays unacked, attached to the still-open
+     * {@link #channel}, until that channel or the connection closes — {@code basicCancel} alone does
+     * neither. So before this method existed with a requeue step, it dropped {@link #held}'s entries
+     * for {@code target} while the broker still considered them outstanding: never acked, never
+     * nacked, never requeued, and no longer reachable by {@link #peek} — permanently invisible. This
+     * is unlike {@link #handleRecovery} and {@link #close()}, whose bare {@code held.clear()} is
+     * correct because each has already made the broker requeue (a real connection drop, or
+     * {@code channel.close()} respectively) before clearing local state.
+     *
+     * <p><strong>Order: cancel first, then nack.</strong> A delivery tag stays valid for
+     * {@code basicNack} on this channel regardless of whether its consumer is still attached — only
+     * a channel/connection close invalidates it — so cancelling {@code target}'s consumer first does
+     * not risk the tags. Doing it the other way round does: nacking a delivery with {@code requeue}
+     * while its consumer is still active hands the message straight back to that <em>same</em>
+     * consumer the instant a prefetch slot frees up (confirmed against a real broker — see
+     * {@code AmqpReplyInboxContractTest.releaseCancelsConsumerAndRequeuesHeldDeliveryForRecovery}),
+     * which races this method's own {@code held.remove(target)}: the redelivery can land after the
+     * clear and leave a stale entry behind, so {@link #peek} is no longer reliably empty right after
+     * {@link #release}. Cancelling first closes that consumer, so the requeued message goes back to
+     * the queue for whichever consumer picks it up next (a later {@link #own}), not this one.
+     *
+     * <p><strong>Failure of the requeue is best-effort, not fatal.</strong> {@link #release} runs
+     * during teardown ({@code Fleetd} calls it right after {@code MessageService.abandon}), and a
+     * throw here would abort cleanups the caller depends on — the same argument fleetd #293 settled
+     * for {@code HerdrPeerLauncher.stop()}'s tab-close step. So a failed {@code basicNack} is logged
+     * at WARN, naming the target and delivery tag that leaked, and release proceeds; the delivery
+     * stays unacked on the broker rather than being silently dropped, so it is still recoverable by a
+     * later connection drop even though this release did not manage to requeue it immediately. A
+     * failed {@code basicCancel} still throws, unchanged from before this fix — that failure means
+     * the consumer may still be attached, so best-effort requeue is not attempted underneath it.
+     */
     @Override
     public void release(String target) {
         synchronized (channelLock) {
             String tag = consumerTags.remove(target);
-            held.remove(target);                   // stale delivery tags must not survive release
-            if (tag == null) {
-                return;
+            if (tag != null) {
+                try {
+                    channel.basicCancel(tag);
+                } catch (IOException e) {
+                    throw new IllegalStateException("cannot cancel consumer for " + target, e);
+                }
             }
-            try {
-                channel.basicCancel(tag);
-            } catch (IOException e) {
-                throw new IllegalStateException("cannot cancel consumer for " + target, e);
+            var perTarget = held.remove(target);
+            if (perTarget != null) {
+                synchronized (perTarget) {
+                    for (Held h : perTarget.values()) {
+                        try {
+                            channel.basicNack(h.deliveryTag(), false, true); // requeue, don't drop
+                        } catch (IOException | RuntimeException e) {
+                            // Caught broadly (not just IOException) for the same reason #293 catches
+                            // RuntimeException in HerdrPeerLauncher.stop(): best-effort teardown must
+                            // not be guarded only against the expected failure and bare against any
+                            // other. The message stays unacked on the broker either way — not lost,
+                            // just not proactively requeued — until a connection drop frees it.
+                            log.warn("release({}): could not requeue held delivery (msgId={}, tag={})"
+                                            + " back to the broker — it stays unacked until a connection"
+                                            + " drop frees it: {}",
+                                    target, h.message().msgId(), h.deliveryTag(), e.getMessage());
+                        }
+                    }
+                }
             }
         }
     }

fleetd/src/test/java/dev/ltms/fleet/health/FleetHealthMonitorTest.java

@@ -24,7 +24,9 @@ import org.slf4j.LoggerFactory;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.ScheduledThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
@@ -434,4 +436,120 @@ class FleetHealthMonitorTest {
             logger.detachAppender(appender);
         }
     }
+
+    // --- fleetd #280: a GONE/NEVER_READY guess whose fleet_ask lapses AFTER the first sweep must
+    // still be swept, without ever reaching into a target that has since recovered or left the
+    // roster. See FleetHealthMonitor.recheckTerminalTarget's javadoc for the full reachability chain.
+
+    @Test void terminalTransitionSchedulesExactlyOneDelayedRecheck() {
+        ScheduledThreadPoolExecutor scheduler = new ScheduledThreadPoolExecutor(1);
+        FleetHealthMonitor monitor = monitor(new FakeHerdr(),
+                List.of(member("term_a", MemberSession.State.BUSY, 0, 0)), scheduler, () -> 1, 600,
+                (_, _) -> { });
+
+        monitor.reportTransition("term_a", HealthState.GONE);
+
+        // Driven via reportTransition directly (not tick()), so the queue holds only the recheck.
+        assertEquals(1, scheduler.getQueue().size());
+        ScheduledFuture<?> scheduled = (ScheduledFuture<?>) scheduler.getQueue().peek();
+        assertTrue(scheduled.getDelay(TimeUnit.SECONDS) > 100,
+                "the delay must clear the worst-case fleet_ask lapse window (up to 115s)");
+
+        // An unchanged tick must not queue a second one (CB-580's fire-once rule extends to this).
+        monitor.reportTransition("term_a", HealthState.GONE);
+        assertEquals(1, scheduler.getQueue().size());
+        monitor.stop();
+    }
+
+    @Test void recheckIsANoOpOnceTheTargetHasRecovered() {
+        RecordingFailTarget failTarget = new RecordingFailTarget();
+        FleetHealthMonitor monitor = monitorWith(failTarget);
+        monitor.reportTransition("term_a", HealthState.GONE);
+        assertEquals(1, failTarget.calls.size());
+
+        monitor.reportTransition("term_a", HealthState.IDLE); // recovered before the recheck fired
+        monitor.recheckTerminalTarget("term_a", HealthState.GONE);
+
+        assertEquals(1, failTarget.calls.size(), "a recovered target must not be reached into again");
+        monitor.stop();
+    }
+
+    @Test void recheckIsANoOpForATargetItNeverObserved() {
+        // Mirrors "left the roster": tick() prunes states.keySet() to the current roster on release
+        // (see FleetHealthMonitor.tick), so a target this monitor never recorded is the same case.
+        RecordingFailTarget failTarget = new RecordingFailTarget();
+        FleetHealthMonitor monitor = monitorWith(failTarget);
+
+        monitor.recheckTerminalTarget("term_never_seen", HealthState.GONE);
+
+        assertEquals(0, failTarget.calls.size(), "an untracked/released target must not be reached into");
+        monitor.stop();
+    }
+
+    /**
+     * The scenario from the ticket, end to end, driven through the real {@link MessageService}: a
+     * target's ask is still genuinely open when health first observes GONE (sweep must skip it,
+     * exactly as {@code abandonDoesNotFailAnAsyncTicketWaitingForAnAnswer} pins), the ask then lapses
+     * on its own, an unchanged tick still must not refire, and only the delayed recheck sweeps the
+     * now-lapsed ticket to FAILED.
+     */
+    @Test void delayedRecheckSweepsATicketWhoseAskLapsedAfterGoneWasFirstObserved() throws Exception {
+        FakeHerdr herdr = new FakeHerdr().withAgent("worker", "term_a", "pane-term_a", "tab_a")
+                .readText("$ prompt");
+        AgentControl agents = new AgentControl(herdr);
+        Rendezvous rendezvous = new Rendezvous();
+        Injector injector = new Injector(agents);
+        InMemoryReplyInbox inbox = new InMemoryReplyInbox();
+        inbox.own("term_a");
+        MessageService messages = new MessageService(agents, injector, rendezvous, inbox);
+        FleetHealthMonitor monitor = new FleetHealthMonitor(agents,
+                () -> List.of(member("term_a", MemberSession.State.READY, 0, 0)), messages,
+                new ScheduledThreadPoolExecutor(1), () -> 1, 60, 600, messages::abandon);
+
+        String ticket = messages.sendAsync("term_a", "task that asks");
+        long deadline = System.currentTimeMillis() + 2000;
+        while (!rendezvous.isWaiting("term_a") && System.currentTimeMillis() < deadline) {
+            Thread.sleep(5);
+        }
+        assertTrue(rendezvous.isWaiting("term_a"), "the async send should have opened its waiter");
+        injector.onStatus("term_a", AgentStatus.IDLE);    // deliver the task
+        injector.onStatus("term_a", AgentStatus.WORKING); // the worker picks it up
+
+        // The worker asks, with a short timeout so its own fleet_ask lapses quickly in test time.
+        CompletableFuture<MessageService.AskResult> ask = CompletableFuture.supplyAsync(
+                () -> messages.ask("term_a", "which config?", 200));
+        awaitPhase(messages, ticket, MessageService.Phase.ASKING);
+
+        monitor.reportTransition("term_a", HealthState.GONE);
+        assertEquals(MessageService.Phase.ASKING, messages.poll(ticket).phase(),
+                "the first sweep must not fail a ticket that is still genuinely being asked");
+
+        // The worker's own fleet_ask now lapses on its own — task.question clears to null.
+        assertEquals(MessageService.AskOutcome.TIMED_OUT, ask.get(5, TimeUnit.SECONDS).outcome());
+
+        // An unchanged tick still must not refire (CB-580).
+        monitor.reportTransition("term_a", HealthState.GONE);
+        assertEquals(MessageService.Phase.PENDING, messages.poll(ticket).phase());
+
+        // The delayed recheck scheduled for the original transition finally sweeps it.
+        monitor.recheckTerminalTarget("term_a", HealthState.GONE);
+        assertEquals(MessageService.Phase.FAILED, messages.poll(ticket).phase());
+
+        monitor.stop();
+    }
+
+    private static MessageService.TaskView awaitPhase(MessageService messages, String ticket,
+            MessageService.Phase phase) throws InterruptedException {
+        long deadline = System.currentTimeMillis() + 2000;
+        MessageService.TaskView view;
+        do {
+            view = messages.poll(ticket);
+            if (view.phase() == phase) {
+                return view;
+            }
+            Thread.sleep(5);
+        } while (System.currentTimeMillis() < deadline);
+        assertEquals(phase, view.phase());
+        return view;
+    }
 }

fleetd/src/test/java/dev/ltms/fleet/msg/AmqpReplyInboxContractTest.java

@@ -154,7 +154,7 @@ class AmqpReplyInboxContractTest {
     }
 
     @Test
-    void releaseCancelsConsumerAndClearsHeld() throws Exception {
+    void releaseCancelsConsumerAndRequeuesHeldDeliveryForRecovery() throws Exception {
         String target = "worker-release-" + System.nanoTime();
         try (AmqpReplyInbox inbox = AmqpReplyInbox.open(uri())) {
             inbox.own(target);
@@ -164,6 +164,22 @@ class AmqpReplyInboxContractTest {
             inbox.release(target);
             assertTrue(inbox.peek(target).isEmpty(),
                     "release clears the local held snapshot");
+
+            // fleetd #298: release() must not just drop the local record — the broker delivery was
+            // never acked, so cancelling the consumer alone leaves it unacked-but-orphaned on the
+            // still-open channel unless release() nacks it back with requeue=true. Prove the message
+            // is genuinely recoverable, not merely absent from peek: re-own the same target and
+            // confirm the broker redelivers it to the fresh consumer.
+            inbox.own(target);
+            List<ReplyInbox.InboxMessage> recovered = awaitPeek(inbox, target);
+            assertEquals(1, recovered.size(),
+                    "a reply held (but undrained) at release() time must still be recoverable — "
+                            + "release() must requeue it, not silently drop it while the broker still "
+                            + "considers it outstanding");
+            assertEquals("m1", recovered.getFirst().msgId());
+            assertEquals("release me", recovered.getFirst().content());
+
+            inbox.ack(target, "m1");
         }
     }