fleetd #440: derive coordinator.heldDurable from queue durability + ack mode #443

Merged

ltms merged 1 commits from worker/440-helddurable-derived-d462d7-13 into main 2026-09-10 11:21:41 +02:00

Conversation 0 Commits 1 Files Changed 6

+92 -8

agent commented 2026-09-10 09:33:22 +02:00

Member

Copy Link

Copy Source

Fixes #440.

What was wrong

FleetMcp.coordinatorView wrote heldDurable as the literal true (FleetMcp.java:1418, confirmed in this tree). The value it describes -- held peer mail survives a crash because the queue is declared durable AND the consumer uses manual ack -- was true, but nothing in the code checked it. Breaking either fact (a non-durable queue, or autoAck=true) would leave the field, and the whole test suite, green.

Lines named in the ticket (FleetMcp.java:1418, FleetMcpTest.java:733, LeadMailbox.java:195/:196) all matched what I found in my own tree before I started.

What changed

• LeadChannel gets a new heldDurable() method. Javadoc states it as the conjunction of "queue declared durable" and "consumer is manual-ack", and that an implementation must derive it from what it actually did, never assert it.
• LeadMailbox.own() now captures the exact booleans it passes to queueDeclare/basicConsume into locals (durableQueue, autoAck), stores durableQueue && !autoAck in a new field, and heldDurable() returns that field. The queue declaration and the ack mode themselves are unchanged -- only how the fact is captured.
• FleetMcp.coordinatorView now reads channel.heldDurable() instead of the literal. Javadoc at FleetMcp.java:1396-1404 rewritten to say where the fact comes from instead of arguing it's true in prose.
• FakeLeadChannel gets a heldDurable field (defaults to true, matching production) and a withHeldDurable(boolean) setter, so hermetic tests can force the false case.
• FleetMcpTest: new test listReportsHeldDurableFalseWhenTheChannelSaysMailIsNotDurable -- proves the field follows the channel, not a constant (acceptance criterion 3).
• LeadMailboxTest (the @Tag("contract") real-broker test, unchanged in scope): new test heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck. This is the one that actually exercises own()'s real queueDeclare/basicConsume calls -- FleetMcpTest only exercises the fake, so it can't catch a mutation to LeadMailbox itself.
• A third anonymous LeadChannel in FleetMcpTest (the timeout/interrupt test) got a trivial heldDurable() { return true; } to keep compiling -- it's an unrelated fake with no bearing on this fact.

Mechanism note (departure from the ticket's suggestion)

The ticket suggested exposing this "through LeadChannel... since it already owns selfCoordId()/peek()" -- I did exactly that, heldDurable() lives on LeadChannel. No departure there.

Acceptance criterion 2 -- no behaviour change

heldDurable is still true on the current configuration (proved by the existing listReportsAnHonestHeldCountAndDurabilityNotJustPendingZero test, unchanged, still green) and no other fleet_list field moved.

Acceptance criterion 4 -- the mutation must fail (evidence)

Ran everything against a real broker via -Pcontract (Docker was available in this worktree).

1. Baseline green, before any mutation:

[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.870 s -- in dev.ltms.fleet.msg.LeadMailboxTest
[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

Mutation applied -- flipped own()'s autoAck local to true (this is LeadMailbox.java:201 in the refactored code; confirmed applied before running):

$ grep -n "boolean autoAck" src/main/java/dev/ltms/fleet/msg/LeadMailbox.java
201:        boolean autoAck = true; // manual ack

2. RED run, same test class, same command (mvn -Pcontract test -Dtest=LeadMailboxTest):

[ERROR] Tests run: 15, Failures: 3, Errors: 0, Skipped: 0, Time elapsed: 16.59 s <<< FAILURE! -- in dev.ltms.fleet.msg.LeadMailboxTest
[ERROR] dev.ltms.fleet.msg.LeadMailboxTest.heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck -- Time elapsed: 0.013 s <<< FAILURE!
[ERROR] dev.ltms.fleet.msg.LeadMailboxTest.unackedMessageSurvivesRestartAndIsRedelivered -- Time elapsed: 10.14 s <<< FAILURE!
[ERROR] dev.ltms.fleet.msg.LeadMailboxTest.inspectReportsPendingMessagesAndZeroConsumersWhenNobodyIsReadingAnymore -- Time elapsed: 0.100 s <<< FAILURE!
[ERROR] Failures: 
[ERROR]   LeadMailboxTest.heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck:220 own() declares a durable queue and consumes with autoAck=false, so held mail is durable ==> expected: <true> but was: <false>
[ERROR]   LeadMailboxTest.inspectReportsPendingMessagesAndZeroConsumersWhenNobodyIsReadingAnymore:276 the unacked message must be requeued, never dropped ==> expected: <1> but was: <0>
[ERROR]   LeadMailboxTest.unackedMessageSurvivesRestartAndIsRedelivered:151 an unacked persistent message is redelivered after restart ==> expected: <1> but was: <0>
[INFO] BUILD FAILURE

My new test caught it directly; two pre-existing redelivery tests also went red as a side effect of the same mutation (autoAck=true means the broker never redelivers on a fresh connection).

3. Restored, then GREEN again -- reverted the local to false, confirmed the revert applied, reran:

$ grep -n "boolean autoAck" src/main/java/dev/ltms/fleet/msg/LeadMailbox.java
201:        boolean autoAck = false; // manual ack

[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.944 s -- in dev.ltms.fleet.msg.LeadMailboxTest
[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

Full-suite run (hermetic, no Docker profile)

$ mvn -f /Users/dai.ha/LTMS/.bridged-worktrees/8df711-13/fleetd/pom.xml clean install
[INFO] Tests run: 1563, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

Run twice (once before the mutation experiment, once after restoring it) -- same count both times, 1563/0/0, BUILD SUCCESS. LeadMailboxTest stays @Tag("contract")-excluded from this run, as before; it's exercised separately above.

Extra: other constant-looking fields in coordinatorView (report only, not fixed)

• row.put("configured", true) at FleetMcp.java in coordinatorView -- this one is currently safe: the method returns null two lines above whenever channel == null, so by the time this line runs, configured really is a fact, not a guess. It's still a literal describing a condition already established elsewhere in the method rather than reading it directly, so worth a second look if that guard ever gets refactored.
• No other literal booleans/numbers found inside coordinatorView, mailboxView, heldView, or peerView -- everything else there reads off LeadMessage/MailboxState/held.size()/coordination.peers().

Caveats for review

• The mutation-sensitive test (heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck) lives in the @Tag("contract") class, so it needs Docker (or AMQP_URI) to run -- it does not run under plain mvn clean install/mvn test. I don't see a way around this: there is no existing hermetic seam (no Mockito, no hand-rolled fake Channel/Connection) for exercising LeadMailbox.own() without a real broker, and adding one felt out of scope for this ticket. FleetMcpTest's new test covers the wiring (criterion 3) hermetically via FakeLeadChannel; the contract test covers the actual derivation (criterion 4).
• I did not touch the queue declaration or the ack mode themselves, per the ticket's "out of scope" note -- only how the fact about them is captured and reported.

Fixes #440. ## What was wrong `FleetMcp.coordinatorView` wrote `heldDurable` as the literal `true` (`FleetMcp.java:1418`, confirmed in this tree). The value it describes -- held peer mail survives a crash because the queue is declared durable AND the consumer uses manual ack -- was true, but nothing in the code checked it. Breaking either fact (a non-durable queue, or `autoAck=true`) would leave the field, and the whole test suite, green. Lines named in the ticket (`FleetMcp.java:1418`, `FleetMcpTest.java:733`, `LeadMailbox.java:195`/`:196`) all matched what I found in my own tree before I started. ## What changed - `LeadChannel` gets a new `heldDurable()` method. Javadoc states it as the conjunction of "queue declared durable" and "consumer is manual-ack", and that an implementation must derive it from what it actually did, never assert it. - `LeadMailbox.own()` now captures the exact booleans it passes to `queueDeclare`/`basicConsume` into locals (`durableQueue`, `autoAck`), stores `durableQueue && !autoAck` in a new field, and `heldDurable()` returns that field. The queue declaration and the ack mode themselves are unchanged -- only how the fact is captured. - `FleetMcp.coordinatorView` now reads `channel.heldDurable()` instead of the literal. Javadoc at `FleetMcp.java:1396-1404` rewritten to say where the fact comes from instead of arguing it's true in prose. - `FakeLeadChannel` gets a `heldDurable` field (defaults to `true`, matching production) and a `withHeldDurable(boolean)` setter, so hermetic tests can force the false case. - `FleetMcpTest`: new test `listReportsHeldDurableFalseWhenTheChannelSaysMailIsNotDurable` -- proves the field follows the channel, not a constant (acceptance criterion 3). - `LeadMailboxTest` (the `@Tag("contract")` real-broker test, unchanged in scope): new test `heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck`. This is the one that actually exercises `own()`'s real `queueDeclare`/`basicConsume` calls -- `FleetMcpTest` only exercises the fake, so it can't catch a mutation to `LeadMailbox` itself. - A third anonymous `LeadChannel` in `FleetMcpTest` (the timeout/interrupt test) got a trivial `heldDurable() { return true; }` to keep compiling -- it's an unrelated fake with no bearing on this fact. ## Mechanism note (departure from the ticket's suggestion) The ticket suggested exposing this "through `LeadChannel`... since it already owns `selfCoordId()`/`peek()`" -- I did exactly that, `heldDurable()` lives on `LeadChannel`. No departure there. ## Acceptance criterion 2 -- no behaviour change `heldDurable` is still `true` on the current configuration (proved by the existing `listReportsAnHonestHeldCountAndDurabilityNotJustPendingZero` test, unchanged, still green) and no other `fleet_list` field moved. ## Acceptance criterion 4 -- the mutation must fail (evidence) Ran everything against a real broker via `-Pcontract` (Docker was available in this worktree). **1. Baseline green**, before any mutation: ``` [INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.870 s -- in dev.ltms.fleet.msg.LeadMailboxTest [INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` **Mutation applied** -- flipped `own()`'s `autoAck` local to `true` (this is `LeadMailbox.java:201` in the refactored code; confirmed applied before running): ``` $ grep -n "boolean autoAck" src/main/java/dev/ltms/fleet/msg/LeadMailbox.java 201: boolean autoAck = true; // manual ack ``` **2. RED run**, same test class, same command (`mvn -Pcontract test -Dtest=LeadMailboxTest`): ``` [ERROR] Tests run: 15, Failures: 3, Errors: 0, Skipped: 0, Time elapsed: 16.59 s <<< FAILURE! -- in dev.ltms.fleet.msg.LeadMailboxTest [ERROR] dev.ltms.fleet.msg.LeadMailboxTest.heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck -- Time elapsed: 0.013 s <<< FAILURE! [ERROR] dev.ltms.fleet.msg.LeadMailboxTest.unackedMessageSurvivesRestartAndIsRedelivered -- Time elapsed: 10.14 s <<< FAILURE! [ERROR] dev.ltms.fleet.msg.LeadMailboxTest.inspectReportsPendingMessagesAndZeroConsumersWhenNobodyIsReadingAnymore -- Time elapsed: 0.100 s <<< FAILURE! [ERROR] Failures: [ERROR] LeadMailboxTest.heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck:220 own() declares a durable queue and consumes with autoAck=false, so held mail is durable ==> expected: <true> but was: <false> [ERROR] LeadMailboxTest.inspectReportsPendingMessagesAndZeroConsumersWhenNobodyIsReadingAnymore:276 the unacked message must be requeued, never dropped ==> expected: <1> but was: <0> [ERROR] LeadMailboxTest.unackedMessageSurvivesRestartAndIsRedelivered:151 an unacked persistent message is redelivered after restart ==> expected: <1> but was: <0> [INFO] BUILD FAILURE ``` My new test caught it directly; two pre-existing redelivery tests also went red as a side effect of the same mutation (autoAck=true means the broker never redelivers on a fresh connection). **3. Restored, then GREEN again** -- reverted the local to `false`, confirmed the revert applied, reran: ``` $ grep -n "boolean autoAck" src/main/java/dev/ltms/fleet/msg/LeadMailbox.java 201: boolean autoAck = false; // manual ack [INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.944 s -- in dev.ltms.fleet.msg.LeadMailboxTest [INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` ## Full-suite run (hermetic, no Docker profile) ``` $ mvn -f /Users/dai.ha/LTMS/.bridged-worktrees/8df711-13/fleetd/pom.xml clean install [INFO] Tests run: 1563, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` Run twice (once before the mutation experiment, once after restoring it) -- same count both times, 1563/0/0, BUILD SUCCESS. `LeadMailboxTest` stays `@Tag("contract")`-excluded from this run, as before; it's exercised separately above. ## Extra: other constant-looking fields in coordinatorView (report only, not fixed) - `row.put("configured", true)` at `FleetMcp.java` in `coordinatorView` -- this one is currently safe: the method returns `null` two lines above whenever `channel == null`, so by the time this line runs, `configured` really is a fact, not a guess. It's still a literal describing a condition already established elsewhere in the method rather than reading it directly, so worth a second look if that guard ever gets refactored. - No other literal booleans/numbers found inside `coordinatorView`, `mailboxView`, `heldView`, or `peerView` -- everything else there reads off `LeadMessage`/`MailboxState`/`held.size()`/`coordination.peers()`. ## Caveats for review - The mutation-sensitive test (`heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck`) lives in the `@Tag("contract")` class, so it needs Docker (or `AMQP_URI`) to run -- it does **not** run under plain `mvn clean install`/`mvn test`. I don't see a way around this: there is no existing hermetic seam (no Mockito, no hand-rolled fake `Channel`/`Connection`) for exercising `LeadMailbox.own()` without a real broker, and adding one felt out of scope for this ticket. `FleetMcpTest`'s new test covers the wiring (criterion 3) hermetically via `FakeLeadChannel`; the contract test covers the actual derivation (criterion 4). - I did not touch the queue declaration or the ack mode themselves, per the ticket's "out of scope" note -- only how the fact about them is captured and reported.

agent added 1 commit 2026-09-10 09:33:22 +02:00

fleetd #440: derive coordinator.heldDurable from queue durability + ack mode ... c16d118f09

FleetMcp.coordinatorView wrote heldDurable as a literal true, so a change
that broke either the durable queue declare or the manual-ack consume in
LeadMailbox would leave the field, and the full suite, green.

- LeadChannel gets a new heldDurable() method: the conclusion of a durable
  queue declare AND a manual-ack consumer, derived by the implementation
  from what it actually did, never asserted.
- LeadMailbox.own() captures the exact booleans it passes to
  queueDeclare/basicConsume and stores their conjunction; heldDurable()
  returns it.
- FleetMcp.coordinatorView now reads channel.heldDurable() instead of a
  literal; updated the javadoc to say where the fact comes from.
- FakeLeadChannel gets a heldDurable field (default true) + withHeldDurable
  setter so FleetMcpTest can prove the field goes false.
- FleetMcpTest: new test asserts heldDurable:false when the channel says so.
- LeadMailboxTest (contract, real broker): new test asserts heldDurable()
  true against a real LeadMailbox. Verified by hand that flipping own()'s
  autoAck local to true turns this test (and two pre-existing redelivery
  tests) red, and restoring it turns them green again.

ltms merged commit 3f807d9f1b into main 2026-09-10 11:21:41 +02:00

ltms referenced this pull request 2026-09-10 11:29:12 +02:00

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

Sign in to join this conversation.

Reviewers

No Reviewers

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

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fleet/fleetd#443