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

coordinatorView reports heldDurable as a hardcoded true, so it cannot go false when durability breaks #440

New Issue
Closed
opened 2026-09-10 09:21:36 +02:00 by ltms · 1 comment
ltms commented 2026-09-10 09:21:36 +02:00
Owner
Copy Link
Copy Source

Found by the fleet01 lead, reviewing PR #438 after I had merged it. I checked it against main and it reproduces.

What is wrong

FleetMcp.java:1418:

row.put("heldDurable", true);

The value is a literal. It is never derived from anything.

The test pins the literal, not the fact — FleetMcpTest.java:733:

assertTrue(out.contains("\"heldDurable\":true"), ...)

So a change that actually makes held mail non-durable leaves this assertion passing.

The claim is true today

I read the source the field claims to describe. LeadMailbox.java:

:195  channel.queueDeclare(queue, true, false, false, null);  // durable queue
:196  channel.basicConsume(queue, false, deliverCallback(), _ -> { });  // autoAck=false

So true is correct right now. This ticket is not "the field is lying". It is "the field cannot ever stop lying".

Why it matters

heldDurable is the conclusion of two independent facts, and it names neither:

  1. the queue is declared durable (queueDeclare's first boolean)
  2. the delivery is manual-ack, so a held message is still owned by the broker (basicConsume's autoAck=false)

Held mail is durable only while both hold. Change either one — a queue redeclared
non-durable, or a switch to autoAck=true to simplify the consume loop — and:

  • heldDurable still reports true
  • the full suite stays green
  • an operator reads a durability promise the daemon no longer keeps

The field was added in #421 to cure the "pending: 0" trap, where mailbox.pending was
the only number beside a non-empty held[] and invited the false reading "these are only
in memory". A field that cures a false reading by asserting a constant has moved the
false reading, not removed it.

The same method already knows better

About 12 lines below, mailboxView's javadoc states the rule this violates:

render a LeadChannel.MailboxState without ever presenting an unmeasured fact as a
measured one

and it enforces it — pending/consumers are omitted unless status == "exists",
specifically so a probe timeout cannot render as "pending: 0, consumers: 0". heldDurable
presents an unmeasured fact as a measured one, in the same method, under that doc.

Acceptance criteria

  1. heldDurable is derived from the queue's declared durability and the consume ack mode,
    not written as a constant. LeadChannel is the natural place to expose the fact, since
    it already owns selfCoordId() and peek(); LeadMailbox holds both inputs.
  2. The value is still true on the current configuration, and fleet_list's output for a
    configured coordinator is otherwise unchanged. This must not become a behaviour change
    for anyone.
  3. A test proves the field can go false. Set up a mailbox with autoAck=true (or a
    non-durable queue declaration) and assert the field reports false. A test that only
    asserts true repeats the existing defect.
  4. The mutation that must fail: flipping basicConsume's autoAck to true in
    LeadMailbox.java:196 must turn at least one test red. Today it turns none red. Paste
    the red run and the restored green run.
  5. Fix the javadoc at FleetMcp.java:1396-1404 too. It currently explains why the fact is
    true in prose. Once the field is derived, it should say where the fact comes from.

Out of scope

Do not change the ack mode or the queue declaration themselves. Both are correct. This
ticket only makes the report follow them.

Found by the fleet01 lead, reviewing PR #438 after I had merged it. I checked it against main and it reproduces. ## What is wrong `FleetMcp.java:1418`: ```java row.put("heldDurable", true); ``` The value is a literal. It is never derived from anything. The test pins the literal, not the fact — `FleetMcpTest.java:733`: ```java assertTrue(out.contains("\"heldDurable\":true"), ...) ``` So a change that actually makes held mail non-durable leaves this assertion passing. ## The claim is true today I read the source the field claims to describe. `LeadMailbox.java`: ``` :195 channel.queueDeclare(queue, true, false, false, null); // durable queue :196 channel.basicConsume(queue, false, deliverCallback(), _ -> { }); // autoAck=false ``` So `true` is correct right now. This ticket is not "the field is lying". It is "the field cannot ever stop lying". ## Why it matters `heldDurable` is the conclusion of **two** independent facts, and it names neither: 1. the queue is declared durable (`queueDeclare`'s first boolean) 2. the delivery is manual-ack, so a held message is still owned by the broker (`basicConsume`'s `autoAck=false`) Held mail is durable only while both hold. Change either one — a queue redeclared non-durable, or a switch to `autoAck=true` to simplify the consume loop — and: - `heldDurable` still reports `true` - the full suite stays green - an operator reads a durability promise the daemon no longer keeps The field was added in #421 to cure the "pending: 0" trap, where `mailbox.pending` was the only number beside a non-empty `held[]` and invited the false reading "these are only in memory". A field that cures a false reading by asserting a constant has moved the false reading, not removed it. ## The same method already knows better About 12 lines below, `mailboxView`'s javadoc states the rule this violates: > render a `LeadChannel.MailboxState` without ever presenting an unmeasured fact as a > measured one and it enforces it — `pending`/`consumers` are omitted unless `status == "exists"`, specifically so a probe timeout cannot render as "pending: 0, consumers: 0". `heldDurable` presents an unmeasured fact as a measured one, in the same method, under that doc. ## Acceptance criteria 1. `heldDurable` is derived from the queue's declared durability and the consume ack mode, not written as a constant. `LeadChannel` is the natural place to expose the fact, since it already owns `selfCoordId()` and `peek()`; `LeadMailbox` holds both inputs. 2. The value is still `true` on the current configuration, and `fleet_list`'s output for a configured coordinator is otherwise unchanged. This must not become a behaviour change for anyone. 3. A test proves the field can go **false**. Set up a mailbox with `autoAck=true` (or a non-durable queue declaration) and assert the field reports `false`. A test that only asserts `true` repeats the existing defect. 4. **The mutation that must fail:** flipping `basicConsume`'s `autoAck` to `true` in `LeadMailbox.java:196` must turn at least one test red. Today it turns none red. Paste the red run and the restored green run. 5. Fix the javadoc at `FleetMcp.java:1396-1404` too. It currently explains why the fact is true in prose. Once the field is derived, it should say where the fact comes from. ## Out of scope Do not change the ack mode or the queue declaration themselves. Both are correct. This ticket only makes the report follow them.
ltms closed this issue 2026-09-10 11:21:41 +02:00
ltms commented 2026-09-10 11:29:12 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged as PR #443, on main at 3f807d9.

What changed. coordinatorView reported heldDurable as a literal true (FleetMcp.java:1418), so the field could never say false and was not a measurement. It now reads channel.heldDurable(). LeadMailbox derives that fact from the two inputs that actually decide it, using the same named locals it passes to the real AMQP calls:

boolean durableQueue = true; // durable, non-exclusive, keep on idle
boolean autoAck = false;     // manual ack
channel.queueDeclare(queue, durableQueue, false, false, null);
channel.basicConsume(queue, autoAck, deliverCallback(), _ -> { });
this.heldDurable = durableQueue && !autoAck;

That shape is the load-bearing part: the fact cannot drift away from the behaviour, because one set of locals feeds both. LeadChannel.heldDurable() was added as abstract, not a default, so a future implementer gets a compile error instead of a silent true.

My own evidence, run on the merged tree (not the worker's report).

  • Full build: Tests run: 1573, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, 0 compile errors.
  • CONTROL (unmutated, 112 tests): green.
  • M1 — put the literal true back at :1418: KILLED by FleetMcpTest.listReportsHeldDurableFalseWhenTheChannelSaysMailIsNotDurable.
  • M3 — the half the worker did not touch, heldCount forced to 0 at :1417: KILLED by FleetMcpTest.listReportsAnHonestHeldCountAndDurabilityNotJustPendingZero.
  • M2 — break the derivation itself (this.heldDurable = true): SURVIVED, exactly as the worker honestly reported. LeadMailbox needs a real broker, so the only test that reaches those AMQP calls is tagged contract and does not run in a plain clean install. This is a known coverage limit of the derivation line, not of the reported field.

Follow-up worth its own ticket, from fleet01's review. One boolean over two inputs is lossy: false cannot tell you whether the queue was non-durable or the consumer was auto-ack. The mailboxView idiom right below in the same file already solves this the other way — it omits pending and consumers unless status == "exists", rather than presenting an unmeasured value. Either omitting the field when it cannot be measured, or reporting the two facts separately, would be better than one collapsed boolean. Not blocking this fix.

Fixed and merged as PR #443, on `main` at `3f807d9`. **What changed.** `coordinatorView` reported `heldDurable` as a literal `true` (`FleetMcp.java:1418`), so the field could never say `false` and was not a measurement. It now reads `channel.heldDurable()`. `LeadMailbox` derives that fact from the two inputs that actually decide it, using the same named locals it passes to the real AMQP calls: ```java boolean durableQueue = true; // durable, non-exclusive, keep on idle boolean autoAck = false; // manual ack channel.queueDeclare(queue, durableQueue, false, false, null); channel.basicConsume(queue, autoAck, deliverCallback(), _ -> { }); this.heldDurable = durableQueue && !autoAck; ``` That shape is the load-bearing part: the fact cannot drift away from the behaviour, because one set of locals feeds both. `LeadChannel.heldDurable()` was added as **abstract**, not a default, so a future implementer gets a compile error instead of a silent `true`. **My own evidence, run on the merged tree (not the worker's report).** - Full build: `Tests run: 1573, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, 0 compile errors. - CONTROL (unmutated, 112 tests): green. - **M1** — put the literal `true` back at `:1418`: **KILLED** by `FleetMcpTest.listReportsHeldDurableFalseWhenTheChannelSaysMailIsNotDurable`. - **M3** — the half the worker did not touch, `heldCount` forced to `0` at `:1417`: **KILLED** by `FleetMcpTest.listReportsAnHonestHeldCountAndDurabilityNotJustPendingZero`. - **M2** — break the derivation itself (`this.heldDurable = true`): **SURVIVED**, exactly as the worker honestly reported. `LeadMailbox` needs a real broker, so the only test that reaches those AMQP calls is tagged `contract` and does not run in a plain `clean install`. This is a known coverage limit of the derivation line, not of the reported field. **Follow-up worth its own ticket, from fleet01's review.** One boolean over two inputs is lossy: `false` cannot tell you whether the queue was non-durable or the consumer was auto-ack. The `mailboxView` idiom right below in the same file already solves this the other way — it omits `pending` and `consumers` unless `status == "exists"`, rather than presenting an unmeasured value. Either omitting the field when it cannot be measured, or reporting the two facts separately, would be better than one collapsed boolean. Not blocking this fix.
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#440
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?