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

A throwable from any listener callback in onStatus skips the delivered-future completion, so a caller waits forever on a message that WAS delivered #553

New Issue
Closed
opened 2026-09-12 09:41:59 +02:00 by ltms · 6 comments
ltms commented 2026-09-12 09:41:59 +02:00
Owner
Copy Link
Copy Source

Found by following up a shape the #546 worker reported out of scope. Their instinct was right; the
consequence is larger than either of us scoped it, and unlike #546 this one needs no Error.

Measured on main at 93a9ed3 (after #549 merged).

The structure

Injector.onStatus has exactly two try blocks in its whole body:

$ grep -n 'try {' fleetd/src/main/java/dev/ltms/fleet/inject/Injector.java | awk -F: '$1>303 && $1<546'
382:                            try {      <- the delivery send (widened to Throwable by #549)
498:            try {                       <- the resubmit nudge, catch (RuntimeException)

There is no try/finally around the method. After the monitor is released, the rest of the
method runs as a straight sequence of unguarded calls, and the block that completes the
caller's future is last:

if (resubmit) {
    try { agentsFor(target).submit(target); }
    catch (RuntimeException e) { log.debug("resubmit to {} failed ...", target, e.getMessage()); }
}
if (notReady != null) { forget.accept(target); ... turnListener.onTurnFailed(target); }
if (turnCompleted) { ... turnListener.onTurnCompleteWithPostAction(target) / onTurnComplete(target); }
if (turnFailed)    { turnListener.onTurnFailed(target); }
if (sent != null) {                                        // <- :534, the last block
    if (sendError != null) { ... sent.delivered().completeExceptionally(sendError); }
    else { turnListener.onDelivered(target, sent.token()); sent.delivered().complete(null); }
}

Anything that throws in any earlier block unwinds out of onStatus and skips the sent block
entirely.

What that costs

By the time control reaches the listener blocks, the delivery has already happened inside the
monitor:

  • t.queue.poll() — the message is off the queue,
  • p.state = Pending.State.DELIVERED — recorded as delivered,
  • the text has been pasted and submitted into the member's pane.

Skipping the sent block means sent.delivered() is never completed, in either direction. The
CompletableFuture stays pending forever. There is no retry that can fix it: the message is gone
from the queue, so no later round revisits it.

Caller-visible: a blocking fleet_send rides out its full timeout and reports a failure for a brief
the member actually received and is already working on. An async wait:false ticket never resolves.

Why it is reachable without an Error

The production TurnListener is not a stub. Fleetd.java:494-528 delegates every callback straight
into two real subsystems:

public void onTurnComplete(String target) {
    completion.onTurnComplete(target);
    sessions.onTurnComplete(target);
}

forget.accept(target) is presence::forget. None of these is wrapped. An ordinary
RuntimeException from any of them is enough — no NoClassDefFoundError, no #413, none of
#546's reachability argument required.

The interaction with #543 and #549, which is the part I want on the record

Before #543, a throwable escaping onStatus killed the status-poller thread. That was a worse
outage but a loud one, and the daemon was visibly broken.

#543 made the loop survive. #549 made the delivery path survive. Both are right and I am not
proposing to revert either. But together they turn this from "the daemon dies" into "one caller's
future is silently orphaned and everything else carries on looking healthy". The blast radius
shrank and the detectability went to zero.

That is the third time this week the same pattern has shown up here, and it is worth stating as a
rule rather than a coincidence: removing a crash does not remove the half-finished state the
crash used to discard — it makes that state permanent and quiet. #546 was the same sentence about
a queue entry; this is it about a future.

The fix

Wrap the post-monitor section so the sent block always runs. The likely shape is a try/finally
where the finally carries only the sent completion — but note the finally needs care, because
completing the future is exactly the kind of "always do this" that must not also swallow the
original throwable. Whatever escaped still has to reach StatusPoller's catch (Throwable) so the
log.error fires.

Guarding each listener call individually is the alternative and is probably better: a listener that
throws is a defect in that listener, and swallowing it wholesale trades one silent failure for
another. At minimum each callback should be attempted independently so one bad listener cannot
prevent the others, and the sent completion should not be reachable-past.

I am not prescribing which. The ticket's job is the enumeration, per the fleet01 lead's checklist:
the mutable state and external side effects this region can leave half-done are the queue entry
(already removed), p.state (already DELIVERED), the pasted text (already in the pane), and the
caller's future (never completed) — and right now nobody restores the last one.

Also in scope: the narrow catch at :500

The resubmit nudge's own catch (RuntimeException e) is the same one-class-too-narrow shape #546
fixed at :391. On its own it is minor — an Error there skips one log.debug and one Enter nudge,
and the next round retries the nudge. What makes it worth fixing is its position: it is the
first block after the monitor, so an Error escaping it skips every block below, including the
sent completion. Fixing the wrapper above makes this one much less interesting; fixing this one
alone does not fix the wrapper, because every other block in the region is still unguarded.

Acceptance

  • A test that a RuntimeException from turnListener.onTurnComplete still leaves the delivered
    future completed. Failing before the fix.
  • The same for onTurnFailed, for onTurnCompleteWithPostAction, and for forget.accept.
  • A test that the original throwable still escapes onStatus (so StatusPoller's log.error still
    fires) — the fix must not convert a loud listener bug into a silent one. This is the control, and
    it is the one that can fail for the same reason the fix would falsely pass.
  • A test that an Error from the resubmit nudge at :500 does not prevent the sent completion.
  • For each: mutation red with that test's own message, restore, byte-identical shasum -a 256,
    green control, and the proof cell run against the un-mutated tree first to confirm it reports
    not-applied. Anchor the proof cell on the pristine text, by line number where the file has
    more than one instance — NOT_DELIVERED appears at :400, :419 and :582, and a file-wide grep
    conflates them.

Credit

The #546 worker reported the narrow catch at :500 as an out-of-scope shape, exactly as briefed, and
declined to fix it. Their assessment of its consequence — "just skips one debug log and one Enter
nudge, the next round retries" — is right about the nudge itself and stops one line short: it does
not account for what the escape skips below it. Reporting the shape is what made the rest of
this visible.

Related: #546 / PR #549, #538 / PR #543, #551 (the other half of the delivery record), #413.

Found by following up a shape the #546 worker reported out of scope. Their instinct was right; the consequence is larger than either of us scoped it, and unlike #546 **this one needs no `Error`.** Measured on `main` at `93a9ed3` (after #549 merged). ## The structure `Injector.onStatus` has exactly two `try` blocks in its whole body: ``` $ grep -n 'try {' fleetd/src/main/java/dev/ltms/fleet/inject/Injector.java | awk -F: '$1>303 && $1<546' 382: try { <- the delivery send (widened to Throwable by #549) 498: try { <- the resubmit nudge, catch (RuntimeException) ``` There is no `try`/`finally` around the method. After the monitor is released, the rest of the method runs as a straight sequence of unguarded calls, and **the block that completes the caller's future is last**: ```java if (resubmit) { try { agentsFor(target).submit(target); } catch (RuntimeException e) { log.debug("resubmit to {} failed ...", target, e.getMessage()); } } if (notReady != null) { forget.accept(target); ... turnListener.onTurnFailed(target); } if (turnCompleted) { ... turnListener.onTurnCompleteWithPostAction(target) / onTurnComplete(target); } if (turnFailed) { turnListener.onTurnFailed(target); } if (sent != null) { // <- :534, the last block if (sendError != null) { ... sent.delivered().completeExceptionally(sendError); } else { turnListener.onDelivered(target, sent.token()); sent.delivered().complete(null); } } ``` Anything that throws in any earlier block unwinds out of `onStatus` and **skips the `sent` block entirely**. ## What that costs By the time control reaches the listener blocks, the delivery has already happened inside the monitor: - `t.queue.poll()` — the message is off the queue, - `p.state = Pending.State.DELIVERED` — recorded as delivered, - the text has been pasted and submitted into the member's pane. Skipping the `sent` block means `sent.delivered()` is **never completed, in either direction**. The `CompletableFuture` stays pending forever. There is no retry that can fix it: the message is gone from the queue, so no later round revisits it. Caller-visible: a blocking `fleet_send` rides out its full timeout and reports a failure for a brief the member actually received and is already working on. An async `wait:false` ticket never resolves. ## Why it is reachable without an `Error` The production `TurnListener` is not a stub. `Fleetd.java:494-528` delegates every callback straight into two real subsystems: ```java public void onTurnComplete(String target) { completion.onTurnComplete(target); sessions.onTurnComplete(target); } ``` `forget.accept(target)` is `presence::forget`. None of these is wrapped. **An ordinary `RuntimeException` from any of them is enough** — no `NoClassDefFoundError`, no `#413`, none of #546's reachability argument required. ## The interaction with #543 and #549, which is the part I want on the record Before #543, a throwable escaping `onStatus` killed the `status-poller` thread. That was a worse outage but a **loud** one, and the daemon was visibly broken. #543 made the loop survive. #549 made the delivery path survive. Both are right and I am not proposing to revert either. But together they turn this from "the daemon dies" into "one caller's future is silently orphaned and everything else carries on looking healthy". The blast radius shrank and the **detectability went to zero**. That is the third time this week the same pattern has shown up here, and it is worth stating as a rule rather than a coincidence: **removing a crash does not remove the half-finished state the crash used to discard — it makes that state permanent and quiet.** #546 was the same sentence about a queue entry; this is it about a future. ## The fix Wrap the post-monitor section so the `sent` block always runs. The likely shape is a `try`/`finally` where the `finally` carries only the `sent` completion — but note the `finally` needs care, because completing the future is exactly the kind of "always do this" that must not also swallow the original throwable. Whatever escaped still has to reach `StatusPoller`'s `catch (Throwable)` so the `log.error` fires. Guarding each listener call individually is the alternative and is probably better: a listener that throws is a defect in that listener, and swallowing it wholesale trades one silent failure for another. At minimum each callback should be attempted independently so one bad listener cannot prevent the others, and the `sent` completion should not be reachable-past. I am not prescribing which. The ticket's job is the enumeration, per the fleet01 lead's checklist: **the mutable state and external side effects this region can leave half-done are** the queue entry (already removed), `p.state` (already `DELIVERED`), the pasted text (already in the pane), and the caller's future (never completed) — and right now nobody restores the last one. ## Also in scope: the narrow catch at :500 The resubmit nudge's own `catch (RuntimeException e)` is the same one-class-too-narrow shape #546 fixed at :391. On its own it is minor — an `Error` there skips one `log.debug` and one Enter nudge, and the next round retries the nudge. What makes it worth fixing is its **position**: it is the first block after the monitor, so an `Error` escaping it skips every block below, including the `sent` completion. Fixing the wrapper above makes this one much less interesting; fixing this one alone does **not** fix the wrapper, because every other block in the region is still unguarded. ## Acceptance - A test that a `RuntimeException` from `turnListener.onTurnComplete` still leaves the delivered future completed. Failing before the fix. - The same for `onTurnFailed`, for `onTurnCompleteWithPostAction`, and for `forget.accept`. - A test that the original throwable still escapes `onStatus` (so `StatusPoller`'s `log.error` still fires) — the fix must not convert a loud listener bug into a silent one. This is the control, and it is the one that can fail for the same reason the fix would falsely pass. - A test that an `Error` from the resubmit nudge at :500 does not prevent the `sent` completion. - For each: mutation red with that test's own message, restore, byte-identical `shasum -a 256`, green control, and the proof cell run against the un-mutated tree first to confirm it reports not-applied. Anchor the proof cell on the **pristine** text, by line number where the file has more than one instance — `NOT_DELIVERED` appears at :400, :419 and :582, and a file-wide grep conflates them. ## Credit The #546 worker reported the narrow catch at :500 as an out-of-scope shape, exactly as briefed, and declined to fix it. Their assessment of its consequence — "just skips one debug log and one Enter nudge, the next round retries" — is right about the nudge itself and stops one line short: it does not account for what the escape skips **below** it. Reporting the shape is what made the rest of this visible. Related: #546 / PR #549, #538 / PR #543, #551 (the other half of the delivery record), #413.
ltms commented 2026-09-12 10:05:17 +02:00
Author
Owner
Copy Link
Copy Source

Scoping correction before anyone writes the fix. There are two futures in this region, not one, and the obvious fix only covers one of them.

Measured on main at 93a9ed3.

The obvious fix, and why it is not enough

The natural patch is to wrap the post-monitor region in try/finally and complete sent.delivered() from the finally, so a throwing listener cannot orphan it. That is correct as far as it goes. It is not enough.

sent.delivered()        the DELIVERY future. The finally covers this one.
sent.token().waiter()   the RENDEZVOUS waiter — what a blocking fleet_send actually
                        waits on for the worker's ANSWER.

The rendezvous waiter is registered only by turnListener.onDelivered(...), and that call lives inside the very if (sent != null) block the finally is backstopping. On the throwing path it never runs:

  • CompletionResolver.java:247-265 captureBaseline is the only thing that does
    inFlight.put(target, new InFlight(waiter, ...)), and onDelivered (:239) is its only caller.
  • CompletionResolver.java:301-308 resolve: 
    CompletableFuture<Rendezvous.Resolution> waiter = turn == null ? null : turn.waiter();
if (waiter == null || waiter.isDone()) { inFlight.remove(target, turn); return; }
    With no inFlight entry, resolve returns silently and resolves nobody.
  • SessionManager.java:895 onDelivered is skipped too, so the session never moves to BUSY and its
    turn count never bumps.

So a finally that only completes the delivery future converts a hang into a hang with a success receipt. That is the worse of the two failures: the caller is told the send landed, and then waits out the full timeout for an answer that can never be resolved.

It is reachable on the ordinary path, not a corner

Injector.java:361 if (!t.awaitingPickup) contains both assignments, in order:

:365   t.awaitingCompletion = false;
:367   turnCompleted = true;
:376   if (!t.awaitingCompletion && !t.postTurnPending && !t.awaitingPostTurnPickup && !t.postTurnObserved) {
:378       Pending p = t.queue.peek();
:391       sent = p;

The delivery guard at :376 passes because of the assignment at :365. So turnCompleted == true and sent != null in the same round is the normal case — complete the previous turn, then deliver the next queued message. turnListener.onTurnComplete(target) then runs before if (sent != null), and Fleetd.java:494-528 delegates every callback straight into completion and sessions unguarded, so an ordinary RuntimeException from either is enough. No Error required.

What the fix should do

Preferred: move the if (sent != null) block to run first, before resubmit / notReady / turnCompleted / turnFailed. Register the turn, then run the side effects. That is the same "record first, then act" ordering #546 settled one layer down, and it makes the whole class of "an earlier listener threw" harmless rather than repairing one future out of two.

If some ordering constraint forbids that — for example if onTurnComplete for the previous turn must be observed before onDelivered for the new one — then the line that proves it goes on this ticket, the finally stays, and the finally must also call turnListener.onDelivered(target, sent.token()), guarded so a throw from it cannot mask the original throwable.

Either way the original throwable must still unwind to StatusPoller's catch (Throwable). A listener bug has to stay loud; swallowing it here would be a worse defect than the one being fixed.

The general rule, because this will recur

A backstop that completes the future the failing block was going to complete is not the same as a backstop that does what the block was going to do. The block's job was to register the turn and then complete it. A finally that only completes has done half of it. The trap is that the enumeration of half-finished state looks complete from inside the function — the registration lived in a different class.

Scoping correction before anyone writes the fix. **There are two futures in this region, not one, and the obvious fix only covers one of them.** Measured on `main` at `93a9ed3`. ## The obvious fix, and why it is not enough The natural patch is to wrap the post-monitor region in `try`/`finally` and complete `sent.delivered()` from the `finally`, so a throwing listener cannot orphan it. That is correct as far as it goes. It is not enough. ``` sent.delivered() the DELIVERY future. The finally covers this one. sent.token().waiter() the RENDEZVOUS waiter — what a blocking fleet_send actually waits on for the worker's ANSWER. ``` The rendezvous waiter is registered **only** by `turnListener.onDelivered(...)`, and that call lives inside the very `if (sent != null)` block the `finally` is backstopping. On the throwing path it never runs: - `CompletionResolver.java:247-265 captureBaseline` is the only thing that does `inFlight.put(target, new InFlight(waiter, ...))`, and `onDelivered` (`:239`) is its only caller. - `CompletionResolver.java:301-308 resolve`: ```java CompletableFuture<Rendezvous.Resolution> waiter = turn == null ? null : turn.waiter(); if (waiter == null || waiter.isDone()) { inFlight.remove(target, turn); return; } ``` With no `inFlight` entry, `resolve` returns silently and resolves nobody. - `SessionManager.java:895 onDelivered` is skipped too, so the session never moves to `BUSY` and its turn count never bumps. So a `finally` that only completes the delivery future converts a hang into **a hang with a success receipt**. That is the worse of the two failures: the caller is told the send landed, and then waits out the full timeout for an answer that can never be resolved. ## It is reachable on the ordinary path, not a corner `Injector.java:361` `if (!t.awaitingPickup)` contains both assignments, in order: ``` :365 t.awaitingCompletion = false; :367 turnCompleted = true; :376 if (!t.awaitingCompletion && !t.postTurnPending && !t.awaitingPostTurnPickup && !t.postTurnObserved) { :378 Pending p = t.queue.peek(); :391 sent = p; ``` The delivery guard at `:376` passes **because of** the assignment at `:365`. So `turnCompleted == true` and `sent != null` in the same round is the normal case — complete the previous turn, then deliver the next queued message. `turnListener.onTurnComplete(target)` then runs before `if (sent != null)`, and `Fleetd.java:494-528` delegates every callback straight into `completion` and `sessions` unguarded, so an ordinary `RuntimeException` from either is enough. No `Error` required. ## What the fix should do Preferred: **move the `if (sent != null)` block to run first**, before `resubmit` / `notReady` / `turnCompleted` / `turnFailed`. Register the turn, then run the side effects. That is the same "record first, then act" ordering #546 settled one layer down, and it makes the whole class of "an earlier listener threw" harmless rather than repairing one future out of two. If some ordering constraint forbids that — for example if `onTurnComplete` for the *previous* turn must be observed before `onDelivered` for the *new* one — then the line that proves it goes on this ticket, the `finally` stays, and the `finally` must also call `turnListener.onDelivered(target, sent.token())`, guarded so a throw from it cannot mask the original throwable. Either way the original throwable must still unwind to `StatusPoller`'s `catch (Throwable)`. A listener bug has to stay loud; swallowing it here would be a worse defect than the one being fixed. ## The general rule, because this will recur **A backstop that completes the future the failing block was going to complete is not the same as a backstop that does what the block was going to do.** The block's job was to *register* the turn and *then* complete it. A `finally` that only completes has done half of it. The trap is that the enumeration of half-finished state looks complete from inside the function — the registration lived in a different class.
ltms commented 2026-09-12 10:09:12 +02:00
Author
Owner
Copy Link
Copy Source

Correction to my own previous comment. Do not follow its "What the fix should do" section — the preferred option in it is wrong and would introduce a CB-116 regression. The measurement of the defect stands; only the recommended fix changes.

What I got wrong

I wrote: "Preferred: move the if (sent != null) block to run first, before resubmit / notReady / turnCompleted / turnFailed."

I also wrote the escape hatch — "if some ordering constraint forbids that ... the line that proves it goes on this ticket" — and then found the line. Here it is.

onDelivered writes inFlight; onTurnComplete reads it, for the previous turn:

// CompletionResolver.java:265 — inside captureBaseline, reached only via onDelivered
inFlight.put(target, new InFlight(waiter, baseline, nowNanos.getAsLong(), token.injectedText()));

// CompletionResolver.java:274-277
// Read the in-flight turn on the poller thread — before any next-turn delivery can overwrite
// it — then off-load the scrape (a herdr round-trip we must not block polling on) to a vthread.
InFlight turn = inFlight.get(target);

That comment states the constraint outright. Move onDelivered for the new turn ahead of onTurnComplete for the previous one, and onTurnComplete reads the new turn's InFlight and resolves the new turn's waiter with the previous turn's scraped output. That is the cross-turn stale reply CB-116 exists to prevent, and resolve's own comment at :303-305 names it:

// Nobody is blocked on THIS turn (it had no send, or its fleet_reply already won). Skip
// the scrape; resolving the current waiter here would be the CB-116 cross-turn stale reply.

So "register before side effects" is the right instinct in general and the wrong instruction here. Registration must come after the previous turn is resolved.

The fix, corrected

Keep if (sent != null) last, inside the try, and make the finally do the block's whole job rather than only its completion:

  1. boolean sentHandled = false; set to true at the end of the if (sent != null) block.
  2. In the finally, when sent != null && !sentHandled:
    • sendError == null → turnListener.onDelivered(target, sent.token()) first, then sent.delivered().complete(null).
    • sendError != null → sent.delivered().completeExceptionally(sendError) only. Nothing was delivered, so no onDelivered on this path.
  3. Wrap that recovery in its own try/catch (Throwable) that logs at WARN and swallows only its own throwable, then still completes the future.
  4. The original throwable must still unwind to StatusPoller's catch (Throwable). Do not return from the finally, and do not catch the original. A listener bug stays loud.

Acceptance, sharpened

With a TurnListener whose onTurnComplete throws, in a round that also delivers a message, assert all three:

  • onDelivered was called exactly once — this is what catches a recovery that double-registers on the normal path,
  • sent.delivered() completed,
  • the original throwable still propagated out of the method.

Why the correction was worth its own comment

The previous comment is instruction surface. A stale doc's severity is not what it says, it is what someone would do next if they believed it — and what they would do here is trade a hang for a stale-answer regression, which is the worse of the two. #513 is the same family.

**Correction to my own previous comment. Do not follow its "What the fix should do" section — the preferred option in it is wrong and would introduce a CB-116 regression.** The measurement of the defect stands; only the recommended fix changes. ## What I got wrong I wrote: *"Preferred: move the `if (sent != null)` block to run first, before `resubmit` / `notReady` / `turnCompleted` / `turnFailed`."* I also wrote the escape hatch — *"if some ordering constraint forbids that ... the line that proves it goes on this ticket"* — and then found the line. Here it is. `onDelivered` **writes** `inFlight`; `onTurnComplete` **reads** it, for the *previous* turn: ```java // CompletionResolver.java:265 — inside captureBaseline, reached only via onDelivered inFlight.put(target, new InFlight(waiter, baseline, nowNanos.getAsLong(), token.injectedText())); // CompletionResolver.java:274-277 // Read the in-flight turn on the poller thread — before any next-turn delivery can overwrite // it — then off-load the scrape (a herdr round-trip we must not block polling on) to a vthread. InFlight turn = inFlight.get(target); ``` That comment states the constraint outright. Move `onDelivered` for the new turn ahead of `onTurnComplete` for the previous one, and `onTurnComplete` reads the **new** turn's `InFlight` and resolves the **new** turn's waiter with the **previous** turn's scraped output. That is the cross-turn stale reply CB-116 exists to prevent, and `resolve`'s own comment at `:303-305` names it: ```java // Nobody is blocked on THIS turn (it had no send, or its fleet_reply already won). Skip // the scrape; resolving the current waiter here would be the CB-116 cross-turn stale reply. ``` So "register before side effects" is the right instinct in general and the wrong instruction *here*. Registration must come **after** the previous turn is resolved. ## The fix, corrected Keep `if (sent != null)` last, inside the `try`, and make the `finally` do the block's **whole job** rather than only its completion: 1. `boolean sentHandled = false;` set to `true` at the end of the `if (sent != null)` block. 2. In the `finally`, when `sent != null && !sentHandled`: - `sendError == null` → `turnListener.onDelivered(target, sent.token())` **first**, then `sent.delivered().complete(null)`. - `sendError != null` → `sent.delivered().completeExceptionally(sendError)` only. Nothing was delivered, so no `onDelivered` on this path. 3. Wrap that recovery in its own `try`/`catch (Throwable)` that logs at WARN and swallows only **its own** throwable, then still completes the future. 4. The **original** throwable must still unwind to `StatusPoller`'s `catch (Throwable)`. Do not return from the `finally`, and do not catch the original. A listener bug stays loud. ## Acceptance, sharpened With a `TurnListener` whose `onTurnComplete` throws, in a round that also delivers a message, assert all three: - `onDelivered` was called **exactly once** — this is what catches a recovery that double-registers on the normal path, - `sent.delivered()` completed, - the original throwable still propagated out of the method. ## Why the correction was worth its own comment The previous comment is instruction surface. A stale doc's severity is not what it says, it is what someone would do next if they believed it — and what they would do here is trade a hang for a stale-answer regression, which is the worse of the two. #513 is the same family.
ltms commented 2026-09-12 10:18:26 +02:00
Author
Owner
Copy Link
Copy Source

Two additions from a peer lead's question, both measured by me on main at 93a9ed3. Neither changes the fix in the previous comment; one strengthens the record, one gives a better acceptance test.

1. Every inFlight remove on the completion path is the conditional two-arg form

The question raised was whether the turnCompleted path could delete a registration the reorder had just installed — turning "waiter never registered" into "waiter registered and immediately removed", the same silent hang with a more convincing log trail. It reaches a remove, and it cannot do that. All eleven mutation sites in CompletionResolver, with their enclosing method:

captureBaseline  :251  inFlight.remove(target)                      <- ONE-ARG, and the only one
captureBaseline  :265  inFlight.put(target, new InFlight(...))
resolve          :308 :378 :405 :418   inFlight.remove(target, turn)
noReportMessage  :478 :498             inFlight.remove(target, turn)
fail             :522 :539 :582        inFlight.remove(target, turn)

onTurnComplete reaches resolve; onTurnFailed reaches fail. But every remove on those paths is ConcurrentMap.remove(key, value) — it deletes only if the mapped value is still turn. The previous turn's completion holds the old InFlight while the map would hold the new one, so the remove is a no-op and the registration survives. The single unconditional remove(target) is inside captureBaseline itself, on its own no-waiter path, unreachable from the previous turn's completion.

That distinction — conditional at nine sites, unconditional at the one place it is safe — is clearly deliberate. Worth not undoing.

The reorder is still unsafe, for the reason in the previous comment: the hazard is the get at :276, not the remove. The two-arg remove protects the map; nothing protects the read.

2. A better acceptance test than the one I specified

I asked for "assert onDelivered was called exactly once". Assert the invariant instead of the call that happens to maintain it:

With a TurnListener whose onTurnComplete throws, in a round that also delivers, assert the waiter is registered — that CompletionResolver.inFlight(target) returns an entry carrying sent.token().waiter().

That assertion goes red on the current try/finally fix, red on a bad reorder, and green only on a correct one. Asserting onDelivered was called is the same property one level shallower: it tests the mechanism rather than the guarantee, and a future fix that satisfies the invariant another way would fail it for no reason. CompletionResolver.inFlight(String) at :269 already exists as a package-private test hook, so this needs no new seam.

Keep the exactly-once check as well — it is what catches a recovery that double-registers on the normal path — but the registration assertion is the one that defines "fixed".

3. The real defect is in the contract, and it is a separate ticket

Stated by the peer lead, and it survives both ordering arguments above:

An interface whose implementations are responsible for maintaining the caller's invariant is a defect in the contract, not in the implementation.

The Injector owns "every delivered turn has a registered waiter". The only thing that can satisfy it is CompletionResolver.captureBaseline, reached through a TurnListener callback that Fleetd.java:494-528 wires up unguarded. Any listener added later can break the Injector's invariant by throwing, and the Injector has no way to notice. Reordering moves which listener has to behave; it does not remove the dependency.

Filed separately — that is a contract change with a blast radius, and this ticket's job is to stop the live hang. Not in scope here.

Two additions from a peer lead's question, both measured by me on `main` at `93a9ed3`. Neither changes the fix in the previous comment; one strengthens the record, one gives a better acceptance test. ## 1. Every `inFlight` remove on the completion path is the *conditional* two-arg form The question raised was whether the `turnCompleted` path could **delete** a registration the reorder had just installed — turning "waiter never registered" into "waiter registered and immediately removed", the same silent hang with a more convincing log trail. It reaches a remove, and it cannot do that. All eleven mutation sites in `CompletionResolver`, with their enclosing method: ``` captureBaseline :251 inFlight.remove(target) <- ONE-ARG, and the only one captureBaseline :265 inFlight.put(target, new InFlight(...)) resolve :308 :378 :405 :418 inFlight.remove(target, turn) noReportMessage :478 :498 inFlight.remove(target, turn) fail :522 :539 :582 inFlight.remove(target, turn) ``` `onTurnComplete` reaches `resolve`; `onTurnFailed` reaches `fail`. But every remove on those paths is `ConcurrentMap.remove(key, value)` — it deletes only if the mapped value is still `turn`. The previous turn's completion holds the **old** `InFlight` while the map would hold the **new** one, so the remove is a no-op and the registration survives. The single unconditional `remove(target)` is inside `captureBaseline` itself, on its own no-waiter path, unreachable from the previous turn's completion. That distinction — conditional at nine sites, unconditional at the one place it is safe — is clearly deliberate. Worth not undoing. **The reorder is still unsafe**, for the reason in the previous comment: the hazard is the `get` at `:276`, not the remove. The two-arg remove protects the map; nothing protects the read. ## 2. A better acceptance test than the one I specified I asked for "assert `onDelivered` was called exactly once". Assert the **invariant** instead of the call that happens to maintain it: > With a `TurnListener` whose `onTurnComplete` throws, in a round that also delivers, assert **the waiter is registered** — that `CompletionResolver.inFlight(target)` returns an entry carrying `sent.token().waiter()`. That assertion goes **red** on the current `try`/`finally` fix, **red** on a bad reorder, and **green** only on a correct one. Asserting `onDelivered` was called is the same property one level shallower: it tests the mechanism rather than the guarantee, and a future fix that satisfies the invariant another way would fail it for no reason. `CompletionResolver.inFlight(String)` at `:269` already exists as a package-private test hook, so this needs no new seam. Keep the exactly-once check as well — it is what catches a recovery that double-registers on the normal path — but the registration assertion is the one that defines "fixed". ## 3. The real defect is in the contract, and it is a separate ticket Stated by the peer lead, and it survives both ordering arguments above: > **An interface whose implementations are responsible for maintaining the caller's invariant is a defect in the contract, not in the implementation.** The `Injector` owns "every delivered turn has a registered waiter". The only thing that can satisfy it is `CompletionResolver.captureBaseline`, reached through a `TurnListener` callback that `Fleetd.java:494-528` wires up unguarded. Any listener added later can break the `Injector`'s invariant by throwing, and the `Injector` has no way to notice. Reordering moves *which* listener has to behave; it does not remove the dependency. Filed separately — that is a contract change with a blast radius, and this ticket's job is to stop the live hang. Not in scope here.
ltms commented 2026-09-12 10:24:35 +02:00
Author
Owner
Copy Link
Copy Source

One more constraint on the fix, and one observation about CompletionResolver that is not this ticket's job but should not be lost. Both came from a peer lead's review; both measured by me on main at 26f380a.

The sentHandled flag must be set BEFORE the calls, not after

The fix in the previous comment guards the finally's recovery with a sentHandled flag so the normal path cannot double-register. Where that flag is set is itself the bug the ticket is about, one level in:

if (sent != null) {
    sentHandled = true;                 // record the INTENT to handle, before any side effect
    if (sendError != null) {
        log.warn(...);
        sent.delivered().completeExceptionally(sendError);
    } else {
        turnListener.onDelivered(target, sent.token());
        sent.delivered().complete(null);
    }
}

Set it after the calls and a throw from onDelivered part way through — after captureBaseline's inFlight.put at CompletionResolver.java:265, before whatever follows — leaves the flag false. The finally reads "not handled" and calls onDelivered again, running a second captureBaseline over a registration that had already succeeded.

And the second capture is not merely redundant — it can make the hang permanent:

// CompletionResolver.java:364-369
String baseline = turn.baseline();
if (baseline != null && baseline.equals(tail)) {
    log.debug("suppressing misattributed completion for {} (no output change since delivery)", target);
    return;   // keep the in-flight record: a later genuine completion still needs it
}

The baseline is a snapshot of the pane taken at delivery, and a completion is suppressed whenever the tail still equals it. The recovery's re-capture runs later — after onTurnComplete threw and after any scraping it did — so it can snapshot a pane that has already absorbed the new turn's output. From then on tail never differs from baseline, and every subsequent completion for that turn is suppressed. The return deliberately keeps the in-flight record, which is exactly what makes this permanent rather than transient: the waiter stays registered, stays eligible, and can never fire.

So the two compose. The late flag is the trigger; the late baseline is the damage. Setting the flag first removes both — no second call, therefore no second capture — which is why no extra guard on the re-capture is needed.

The general form, and it is #546's rule one layer in: the flag must record the intent to call, not the fact of having completed the call. A flag written after a side effect cannot distinguish "never ran" from "ran and failed halfway" — the same one-value-for-several-states problem as NOT_DELIVERED.

Extra acceptance test

A TurnListener whose onDelivered throws after the registration happened, asserting onDelivered was called exactly once, not twice. That assertion goes red on the wrong flag placement and on nothing else.

Not this ticket: the conditional removes are load-bearing and untested

Recorded here so it is not lost, and carried onto #556 as a non-goal-to-break.

Nine of the ten inFlight removes are the conditional remove(key, value); the one unconditional remove(target) is inside captureBaseline's own no-waiter path, where a previous turn's completion cannot reach it. That is someone having thought carefully about exactly the overwrite hazard — and having left the thinking in the code rather than in a comment or a test.

Nothing exercises the distinction. The next person to "tidy" a two-arg remove into a one-arg one removes the protection without a single test going red.

This is the same family as #512's unknown arm, from the other direction: there, a branch that is now unreachable in production has its test as the entire remaining coverage; here, a branch that is reachable has no coverage at all. Both are guarantees maintained only by a detail nothing exercises.

A test that fails when a two-arg remove becomes a one-arg one is the shape wanted — the mktemp -t lesson from #545 pointed at a Java idiom instead of a shell one. Whether that is expressible without being brittle is an open question, and saying so is better than quietly dropping it.

One more constraint on the fix, and one observation about `CompletionResolver` that is not this ticket's job but should not be lost. Both came from a peer lead's review; both measured by me on `main` at `26f380a`. ## The `sentHandled` flag must be set BEFORE the calls, not after The fix in the previous comment guards the `finally`'s recovery with a `sentHandled` flag so the normal path cannot double-register. **Where that flag is set is itself the bug the ticket is about**, one level in: ```java if (sent != null) { sentHandled = true; // record the INTENT to handle, before any side effect if (sendError != null) { log.warn(...); sent.delivered().completeExceptionally(sendError); } else { turnListener.onDelivered(target, sent.token()); sent.delivered().complete(null); } } ``` Set it *after* the calls and a throw from `onDelivered` **part way through** — after `captureBaseline`'s `inFlight.put` at `CompletionResolver.java:265`, before whatever follows — leaves the flag `false`. The `finally` reads "not handled" and calls `onDelivered` again, running a second `captureBaseline` over a registration that had already succeeded. **And the second capture is not merely redundant — it can make the hang permanent:** ```java // CompletionResolver.java:364-369 String baseline = turn.baseline(); if (baseline != null && baseline.equals(tail)) { log.debug("suppressing misattributed completion for {} (no output change since delivery)", target); return; // keep the in-flight record: a later genuine completion still needs it } ``` The baseline is a snapshot of the pane taken **at delivery**, and a completion is suppressed whenever the tail still equals it. The recovery's re-capture runs *later* — after `onTurnComplete` threw and after any scraping it did — so it can snapshot a pane that has already absorbed the new turn's output. From then on `tail` never differs from `baseline`, and every subsequent completion for that turn is suppressed. The `return` deliberately keeps the in-flight record, which is exactly what makes this permanent rather than transient: the waiter stays registered, stays eligible, and can never fire. So the two compose. The late flag is the trigger; the late baseline is the damage. Setting the flag first removes both — no second call, therefore no second capture — which is why no extra guard on the re-capture is needed. **The general form, and it is #546's rule one layer in:** the flag must record the **intent** to call, not the fact of having **completed** the call. A flag written after a side effect cannot distinguish "never ran" from "ran and failed halfway" — the same one-value-for-several-states problem as `NOT_DELIVERED`. ### Extra acceptance test A `TurnListener` whose `onDelivered` throws **after** the registration happened, asserting `onDelivered` was called **exactly once**, not twice. That assertion goes red on the wrong flag placement and on nothing else. ## Not this ticket: the conditional removes are load-bearing and untested Recorded here so it is not lost, and carried onto #556 as a non-goal-to-break. Nine of the ten `inFlight` removes are the conditional `remove(key, value)`; the one unconditional `remove(target)` is inside `captureBaseline`'s own no-waiter path, where a previous turn's completion cannot reach it. That is someone having thought carefully about exactly the overwrite hazard — and having left the thinking **in the code rather than in a comment or a test**. Nothing exercises the distinction. The next person to "tidy" a two-arg remove into a one-arg one removes the protection without a single test going red. This is the same family as #512's `unknown` arm, from the other direction: there, a branch that is now unreachable in production has its test as the entire remaining coverage; here, a branch that *is* reachable has no coverage at all. Both are guarantees maintained only by a detail nothing exercises. A test that fails when a two-arg remove becomes a one-arg one is the shape wanted — the `mktemp -t` lesson from #545 pointed at a Java idiom instead of a shell one. Whether that is expressible without being brittle is an open question, and saying so is better than quietly dropping it.
ltms commented 2026-09-12 10:40:12 +02:00
Author
Owner
Copy Link
Copy Source

Lead verification of PR #557 at d4a51c6. This comment is newer than any brief — where it disagrees, this wins.

The PR is good and I am not asking for a redesign. The flag placement is right, the CB-116 reasoning is right, forget.accept matches main's count exactly (2 sites, no leftover marker — I checked), and both mutations reproduce. One gap, found by measurement, and it is on the one path the flag placement itself created.

The gap

onStatus states its own invariant in the new comment:

sent's future MUST be completed one way or another, in every path out of this region

That is not yet true. Trace a throw from onDelivered on the normal path:

:597   sentHandled = true;                       // set FIRST — correct, do not change this
:605   turnListener.onDelivered(target, sent.token());   // throws
       -> propagates out of the try
:622   if (sent != null && !sentHandled) {       // sentHandled is TRUE -> backstop SKIPPED
       -> sent.delivered() is never completed

The message was typed into the pane and taken off the queue, and its delivery future stays pending forever.

This is not a regression — main does the same thing. But it is the ticket's own stated goal left unclosed, and the flag is what closes the backstop against it.

Measured, not argued

I added one assertion the PR does not make, and ran it against d4a51c6:

InjectorTest.leadProbe_onDeliveredThrowStillCompletesTheDeliveryFuture
  assertTrue(delivered.isDone())

MVN_EXIT=1
AssertionFailedError: LEAD PROBE: the message WAS delivered (typed into the pane, off the
queue), so its delivery future must be completed on every path out of onStatus
  ==> expected: <true> but was: <false>

The fix — split the two concerns the one flag is currently doing

sentHandled has to guard the onDelivered re-call, because a second captureBaseline is the permanent-suppression bug. It must not guard the future completion, because CompletableFuture.complete and completeExceptionally are idempotent — on the normal path the completion already happened and a second call is a no-op returning false.

So move the flag off the block and onto the call it actually protects:

-            if (sent != null && !sentHandled) {
+            if (sent != null) {
                 try {
-                    if (sendError == null) {
+                    if (!sentHandled && sendError == null) {
                         turnListener.onDelivered(target, sent.token());
                     }
                 } catch (Throwable recoveryError) {
                     log.warn(...);
                 } finally {
+                    // complete/completeExceptionally are idempotent: on the normal path the block
+                    // above already completed this and the call here is a no-op returning false.
                     if (sendError != null) {
                         sent.delivered().completeExceptionally(sendError);
                     } else {
                         sent.delivered().complete(null);
                     }
                 }
             }

I ran this before asking for it

Applied to d4a51c6, then reverted (the worktree is back to a clean d4a51c6):

mvn -Dtest=InjectorTest,CompletionResolverTest test
MVN_EXIT=0
Tests run: 47, Failures: 0, Errors: 0, Skipped: 0  -- InjectorTest      (46 + my probe)
Tests run: 59, Failures: 0, Errors: 0, Skipped: 0  -- CompletionResolverTest
BUILD SUCCESS

The 47 includes anOnDeliveredThrowAfterItsOwnRegistrationDoesNotRunASecondTime, still green — the split does not reintroduce the double-call, which was the thing worth checking. !sentHandled still gates the call; only the completion escaped the gate.

The general shape, for the next person

One boolean was carrying two different meanings: "the onDelivered call has been made" and "the future has been dealt with". Those come apart exactly when the call throws between them — which is the case this ticket is about. A flag that records an intent covers everything after the intent; it cannot also stand in for the completion of a step that happens later and can fail on its own. When a guard has to be moved earlier to fix one hazard, check every other thing that guard was also holding up: here, moving it earlier was correct and it silently took the completion with it.

Lead verification of PR #557 at `d4a51c6`. **This comment is newer than any brief — where it disagrees, this wins.** The PR is good and I am not asking for a redesign. The flag placement is right, the CB-116 reasoning is right, `forget.accept` matches `main`'s count exactly (2 sites, no leftover marker — I checked), and both mutations reproduce. **One gap, found by measurement, and it is on the one path the flag placement itself created.** ## The gap `onStatus` states its own invariant in the new comment: > `sent`'s future MUST be completed one way or another, in every path out of this region That is not yet true. Trace a throw from `onDelivered` on the **normal** path: ``` :597 sentHandled = true; // set FIRST — correct, do not change this :605 turnListener.onDelivered(target, sent.token()); // throws -> propagates out of the try :622 if (sent != null && !sentHandled) { // sentHandled is TRUE -> backstop SKIPPED -> sent.delivered() is never completed ``` The message was typed into the pane and taken off the queue, and its delivery future stays pending forever. This is **not a regression** — `main` does the same thing. But it is the ticket's own stated goal left unclosed, and the flag is what closes the backstop against it. ## Measured, not argued I added one assertion the PR does not make, and ran it against `d4a51c6`: ``` InjectorTest.leadProbe_onDeliveredThrowStillCompletesTheDeliveryFuture assertTrue(delivered.isDone()) ``` ``` MVN_EXIT=1 AssertionFailedError: LEAD PROBE: the message WAS delivered (typed into the pane, off the queue), so its delivery future must be completed on every path out of onStatus ==> expected: <true> but was: <false> ``` ## The fix — split the two concerns the one flag is currently doing `sentHandled` has to guard the `onDelivered` **re-call**, because a second `captureBaseline` is the permanent-suppression bug. It must **not** guard the **future completion**, because `CompletableFuture.complete` and `completeExceptionally` are idempotent — on the normal path the completion already happened and a second call is a no-op returning `false`. So move the flag off the block and onto the call it actually protects: ```java - if (sent != null && !sentHandled) { + if (sent != null) { try { - if (sendError == null) { + if (!sentHandled && sendError == null) { turnListener.onDelivered(target, sent.token()); } } catch (Throwable recoveryError) { log.warn(...); } finally { + // complete/completeExceptionally are idempotent: on the normal path the block + // above already completed this and the call here is a no-op returning false. if (sendError != null) { sent.delivered().completeExceptionally(sendError); } else { sent.delivered().complete(null); } } } ``` ## I ran this before asking for it Applied to `d4a51c6`, then reverted (the worktree is back to a clean `d4a51c6`): ``` mvn -Dtest=InjectorTest,CompletionResolverTest test MVN_EXIT=0 Tests run: 47, Failures: 0, Errors: 0, Skipped: 0 -- InjectorTest (46 + my probe) Tests run: 59, Failures: 0, Errors: 0, Skipped: 0 -- CompletionResolverTest BUILD SUCCESS ``` The 47 includes `anOnDeliveredThrowAfterItsOwnRegistrationDoesNotRunASecondTime`, still green — **the split does not reintroduce the double-call**, which was the thing worth checking. `!sentHandled` still gates the call; only the completion escaped the gate. ## The general shape, for the next person **One boolean was carrying two different meanings: "the onDelivered call has been made" and "the future has been dealt with".** Those come apart exactly when the call throws between them — which is the case this ticket is about. A flag that records an intent covers everything after the intent; it cannot also stand in for the completion of a step that happens later and can fail on its own. When a guard has to be moved earlier to fix one hazard, check every other thing that guard was also holding up: here, moving it earlier was correct and it silently took the completion with it.
ltms commented 2026-09-12 10:51:04 +02:00
Author
Owner
Copy Link
Copy Source

Merged as f606fcc (PR #557). Closing.

Verified on the merged tree, not on the branch — main had moved, so the branch passing proves nothing about what actually lands.

mvn clean install   MVN_EXIT=0
Tests run: 1729, Failures: 0, Errors: 0, Skipped: 0   (127 surefire report files)

Lead mutation — one the worker did not run

The worker's own mutation reverted my fix (put !sentHandled back on the outer if). That proves the outer guard is wrong. It does not prove the inner guard is load-bearing — so I ran the other half.

Dropped !sentHandled from the inner guard at :637, letting onDelivered run twice. Line-anchored sed, pristine anchor counted with awk string-equality (not a regex, not perl): 1 → 0, so the mutation genuinely applied.

Killed by two tests, each naming the observed value:

ordinarySuccessStillCompletesExactlyOnceAfterOnDelivered
  expected: <[onDelivered, completed]> but was: <[onDelivered, completed, onDelivered]>

anOnDeliveredThrowAfterItsOwnRegistrationDoesNotRunASecondTime
  expected: <1> but was: <2>

That second line is the positive-direction evidence: the mutant did not merely fail, it produced the double call the guard exists to prevent. Restored, shasum -a 256 -c OK (byte-identical), green control 47/59.

So both halves of the split are now pinned by a killed mutation, in opposite directions. That was the thing worth checking, because the whole defect was one boolean doing two jobs — proving one job is guarded says nothing about the other.

What the defect turned out to be

sentHandled carried two meanings: "onDelivered was called" and "the future has been dealt with". Those are the same fact only while nothing between them can fail — and they come apart exactly when onDelivered throws part way through, which is the case this ticket is about.

Setting the flag before the call is correct, and that was the fleet01 lead's catch: a flag written after a side effect cannot tell never ran from ran and failed halfway, and a second captureBaseline makes the hang permanent via CompletionResolver's own :364-369 suppression. But the same flag was also the outer guard on the backstop, so moving it earlier — right, for that hazard — silently took the future completion with it.

CompletableFuture.complete is idempotent, so the completion never needed a guard at all.

The general shape, which is the part worth keeping

When a guard has to move earlier to fix one hazard, enumerate everything else that guard was holding up. Anything idempotent almost certainly did not need it, and that is where the bug hides.

And the method that found it: a comment claiming an invariant is a free test case. The PR's own comment said "sent's future MUST be completed one way or another, in every path out of this region". Turning that sentence into the one assertion the PR did not make produced a value — expected: <true> but was: <false> — instead of an opinion. That is cheaper than reading the code to decide whether the comment is true.

Unblocked by this

#551 and #556 were both held on this merging. Both are now free.

A daemon redeploy is owed — this changes Java — but #544 is also Java and still in flight, so I am doing one redeploy after it lands rather than restarting the daemon under a live worker.

Merged as `f606fcc` (PR #557). Closing. **Verified on the merged tree**, not on the branch — `main` had moved, so the branch passing proves nothing about what actually lands. ``` mvn clean install MVN_EXIT=0 Tests run: 1729, Failures: 0, Errors: 0, Skipped: 0 (127 surefire report files) ``` ## Lead mutation — one the worker did not run The worker's own mutation reverted my fix (put `!sentHandled` back on the **outer** `if`). That proves the outer guard is wrong. It does not prove the **inner** guard is load-bearing — so I ran the other half. Dropped `!sentHandled` from the inner guard at `:637`, letting `onDelivered` run twice. Line-anchored `sed`, pristine anchor counted with `awk` string-equality (not a regex, not `perl`): **1 → 0**, so the mutation genuinely applied. Killed by **two** tests, each naming the observed value: ``` ordinarySuccessStillCompletesExactlyOnceAfterOnDelivered expected: <[onDelivered, completed]> but was: <[onDelivered, completed, onDelivered]> anOnDeliveredThrowAfterItsOwnRegistrationDoesNotRunASecondTime expected: <1> but was: <2> ``` That second line is the positive-direction evidence: the mutant did not merely fail, it produced the **double call** the guard exists to prevent. Restored, `shasum -a 256 -c` **OK** (byte-identical), green control 47/59. So both halves of the split are now pinned by a killed mutation, in opposite directions. That was the thing worth checking, because the whole defect was one boolean doing two jobs — proving one job is guarded says nothing about the other. ## What the defect turned out to be `sentHandled` carried two meanings: *"`onDelivered` was called"* and *"the future has been dealt with"*. Those are the same fact **only while nothing between them can fail** — and they come apart exactly when `onDelivered` throws part way through, which is the case this ticket is about. Setting the flag **before** the call is correct, and that was the fleet01 lead's catch: a flag written after a side effect cannot tell *never ran* from *ran and failed halfway*, and a second `captureBaseline` makes the hang permanent via `CompletionResolver`'s own `:364-369` suppression. But the same flag was also the outer guard on the backstop, so moving it earlier — right, for that hazard — silently took the **future completion** with it. `CompletableFuture.complete` is idempotent, so the completion never needed a guard at all. ## The general shape, which is the part worth keeping **When a guard has to move earlier to fix one hazard, enumerate everything else that guard was holding up.** Anything idempotent almost certainly did not need it, and that is where the bug hides. And the method that found it: **a comment claiming an invariant is a free test case.** The PR's own comment said *"`sent`'s future MUST be completed one way or another, in every path out of this region"*. Turning that sentence into the one assertion the PR did not make produced a value — `expected: <true> but was: <false>` — instead of an opinion. That is cheaper than reading the code to decide whether the comment is true. ## Unblocked by this #551 and #556 were both held on this merging. Both are now free. A daemon redeploy is owed — this changes Java — but #544 is also Java and still in flight, so I am doing one redeploy after it lands rather than restarting the daemon under a live worker.
ltms closed this issue 2026-09-12 10:51:10 +02:00
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#553
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?