fleetd #437: fleet_ack errors instead of claiming success on a miss #448

2026-09-10T11:44:41+02:00

agent commented

2026-09-10 11:44:41 +02:00

Ticket

fleetd #437 — fleet_ack said "acknowledged " for a message it never touched, because nothing in the ack chain reported hit vs. miss.

What changed

ReplyInbox.ack(target, msgId) now returns boolean (true if it removed an entry, false if there was nothing to remove) instead of void. Updated the interface javadoc and both implementations: InMemoryReplyInbox and AmqpReplyInbox. An ack for a target this daemon does not own still returns false, not an error — that part of the contract is unchanged.
MessageService.ackReply now returns that boolean, unchanged otherwise.
MessageService.drainReplies still ignores the boolean, as instructed — it peeked those ids itself, so a false there is a harmless race, and its javadoc already documents a deliberate loss window that is out of scope here.
FleetMcp.ack now returns error(...) when the boolean is false, naming the target/msgId and pointing at fleet_poll{coordId} for held lead-to-lead (peer) mail, which has no route through fleet_ack. The success string ("acknowledged " + msgId) is unchanged.
Updated the fleet_ack tool schema's target parameter description to say what the code now actually does.

Did not route fleet_ack to LeadChannel.ack — per the ticket, held peer mail the lead has never been shown must stay refusable, not ackable, so fleet_ack against a coord-id simply errors.

Tests

FleetMcpTest: rewrote bridgeAckReturnsConfirmationForValidArgs to publish a real message before asserting success. Added bridgeAckOfAnIdInNoInboxIsAnError and bridgeAckOfACoordIdTargetIsAnErrorNamingFleetPoll. Rewrote bridgeAckRemovesSpecificReply's assertDoesNotThrow line (the one the ticket named as pinning the defect) to assertFalse(messages.ackReply(...)). Added bridgeAckRemovingARealQueuedReplyReportsSuccessAndRemovesIt.
InMemoryReplyInboxTest: boolean assertions on ackRemovesTheMessage, ackForUnknownMsgIdIsNoOp, ackForUnknownTargetIsNoOp, peekAndAckAreNoOpsForUnownedTarget. Kept per review feedback: mutating InMemoryReplyInbox.ack's unowned-target branch to return true is killed by exactly these two of the four (ackForUnknownTargetIsNoOp, peekAndAckAreNoOpsForUnownedTarget) plus FleetMcpTest.bridgeAckOfACoordIdTargetIsAnErrorNamingFleetPoll — dropping them would leave that contract resting on one test.
AmqpReplyInboxContractTest (new, added on review feedback): ackReportsHitVsMissAgainstARealBroker — a msgId never held for an owned target returns false without throwing; a real held reply returns true and is removed; acking the same msgId again returns false. This is the one contract-group class CI actually runs, and it previously made zero assertions on ack()'s return value, so the exact defect this ticket is about was unpinned in the adapter fleetd runs live. Verified against both broker modes the class supports: Testcontainers (AMQP_URI unset) and an external broker via AMQP_URI (the CI shape — I used a disposable rabbitmq:3.13-management container on a throwaway port, not the shared local LavinMQ instance backing the live fleet, then stopped and removed it).

Evidence

Full build, repo root (fleetd/): mvn clean install → Tests run: 1577, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. (The log has scattered ERROR/WARN lines from unrelated test fixtures simulating backend failures — e.g. OpenCodeLauncherTest, SessionManagerTest — not real failures; the Tests run: line is the source of truth.)

Control, default suite (unmutated): mvn test -Dtest=FleetMcpTest,InMemoryReplyInboxTest → Tests run: 101, Failures: 0 — BUILD SUCCESS.

Control, contract suite (unmutated), both broker modes:

Testcontainers (AMQP_URI unset): mvn -Pcontract test -Dtest=AmqpReplyInboxContractTest → Tests run: 9, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.
External broker (AMQP_URI set to a disposable container): same command → Tests run: 9, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Mutation 1 (FleetMcp.ack ignoring the boolean, always return text("acknowledged " + msgId); printed the mutated line to confirm it landed): mvn test -Dtest=FleetMcpTest → Tests run: 85, Failures: 2, failing methods:

dev.ltms.fleet.mcp.FleetMcpTest.bridgeAckOfAnIdInNoInboxIsAnError
dev.ltms.fleet.mcp.FleetMcpTest.bridgeAckOfACoordIdTargetIsAnErrorNamingFleetPoll

Restored, re-ran mvn test -Dtest=FleetMcpTest,InMemoryReplyInboxTest → Tests run: 101, Failures: 0 — BUILD SUCCESS.

Mutation 2 (AmqpReplyInbox.ack's h == null branch changed to return true; — was return false;; printed the mutated line to confirm it landed): mvn -Pcontract test -Dtest=AmqpReplyInboxContractTest (Testcontainers path) → Tests run: 9, Failures: 1, failing method:

dev.ltms.fleet.msg.AmqpReplyInboxContractTest.ackReportsHitVsMissAgainstARealBroker — acking the same msgId twice must report false the second time ==> expected: <false> but was: <true>

Restored, re-ran mvn -Pcontract test -Dtest=AmqpReplyInboxContractTest → Tests run: 9, Failures: 0 — BUILD SUCCESS. Then the full mvn clean install above (1577/0/0, BUILD SUCCESS) as the final state.

Scope notes

Did not add boolean-return coverage to any other contract-group test class — only AmqpReplyInboxContractTest was named as the one CI actually runs.
Did not touch LeadChannel/LeadMailbox/LeadCoordLoop — confirmed (again) that fleet_ack has no route there and must not gain one.
Ran the CI-path check against a disposable rabbitmq:3.13-management container I started and stopped myself, never against the shared local LavinMQ instance the live fleetd daemon uses, to avoid leaving orphaned durable queues on production infrastructure.

## Ticket fleetd #437 — `fleet_ack` said "acknowledged <msgId>" for a message it never touched, because nothing in the ack chain reported hit vs. miss. ## What changed - `ReplyInbox.ack(target, msgId)` now returns `boolean` (`true` if it removed an entry, `false` if there was nothing to remove) instead of `void`. Updated the interface javadoc and both implementations: `InMemoryReplyInbox` and `AmqpReplyInbox`. An ack for a target this daemon does not own still returns `false`, not an error — that part of the contract is unchanged. - `MessageService.ackReply` now returns that boolean, unchanged otherwise. - `MessageService.drainReplies` still ignores the boolean, as instructed — it peeked those ids itself, so a `false` there is a harmless race, and its javadoc already documents a deliberate loss window that is out of scope here. - `FleetMcp.ack` now returns `error(...)` when the boolean is `false`, naming the target/msgId and pointing at `fleet_poll{coordId}` for held lead-to-lead (peer) mail, which has no route through `fleet_ack`. The success string (`"acknowledged " + msgId`) is unchanged. - Updated the `fleet_ack` tool schema's `target` parameter description to say what the code now actually does. **Did not** route `fleet_ack` to `LeadChannel.ack` — per the ticket, held peer mail the lead has never been shown must stay refusable, not ackable, so `fleet_ack` against a coord-id simply errors. ## Tests - `FleetMcpTest`: rewrote `bridgeAckReturnsConfirmationForValidArgs` to publish a real message before asserting success. Added `bridgeAckOfAnIdInNoInboxIsAnError` and `bridgeAckOfACoordIdTargetIsAnErrorNamingFleetPoll`. Rewrote `bridgeAckRemovesSpecificReply`'s `assertDoesNotThrow` line (the one the ticket named as pinning the defect) to `assertFalse(messages.ackReply(...))`. Added `bridgeAckRemovingARealQueuedReplyReportsSuccessAndRemovesIt`. - `InMemoryReplyInboxTest`: boolean assertions on `ackRemovesTheMessage`, `ackForUnknownMsgIdIsNoOp`, `ackForUnknownTargetIsNoOp`, `peekAndAckAreNoOpsForUnownedTarget`. **Kept per review feedback**: mutating `InMemoryReplyInbox.ack`'s unowned-target branch to `return true` is killed by exactly these two of the four (`ackForUnknownTargetIsNoOp`, `peekAndAckAreNoOpsForUnownedTarget`) plus `FleetMcpTest.bridgeAckOfACoordIdTargetIsAnErrorNamingFleetPoll` — dropping them would leave that contract resting on one test. - **`AmqpReplyInboxContractTest`** (new, added on review feedback): `ackReportsHitVsMissAgainstARealBroker` — a `msgId` never held for an owned target returns `false` without throwing; a real held reply returns `true` and is removed; acking the same `msgId` again returns `false`. This is the one contract-group class CI actually runs, and it previously made zero assertions on `ack()`'s return value, so the exact defect this ticket is about was unpinned in the adapter fleetd runs live. Verified against both broker modes the class supports: Testcontainers (`AMQP_URI` unset) and an external broker via `AMQP_URI` (the CI shape — I used a disposable `rabbitmq:3.13-management` container on a throwaway port, not the shared local LavinMQ instance backing the live fleet, then stopped and removed it). ## Evidence **Full build**, repo root (`fleetd/`): `mvn clean install` → **Tests run: 1577, Failures: 0, Errors: 0, Skipped: 0** — **BUILD SUCCESS**. (The log has scattered `ERROR`/`WARN` lines from unrelated test fixtures simulating backend failures — e.g. `OpenCodeLauncherTest`, `SessionManagerTest` — not real failures; the `Tests run:` line is the source of truth.) **Control, default suite** (unmutated): `mvn test -Dtest=FleetMcpTest,InMemoryReplyInboxTest` → **Tests run: 101, Failures: 0** — **BUILD SUCCESS**. **Control, contract suite** (unmutated), both broker modes: - Testcontainers (`AMQP_URI` unset): `mvn -Pcontract test -Dtest=AmqpReplyInboxContractTest` → **Tests run: 9, Failures: 0, Errors: 0, Skipped: 0** — **BUILD SUCCESS**. - External broker (`AMQP_URI` set to a disposable container): same command → **Tests run: 9, Failures: 0, Errors: 0, Skipped: 0** — **BUILD SUCCESS**. **Mutation 1** (`FleetMcp.ack` ignoring the boolean, always `return text("acknowledged " + msgId)`; printed the mutated line to confirm it landed): `mvn test -Dtest=FleetMcpTest` → **Tests run: 85, Failures: 2**, failing methods: - `dev.ltms.fleet.mcp.FleetMcpTest.bridgeAckOfAnIdInNoInboxIsAnError` - `dev.ltms.fleet.mcp.FleetMcpTest.bridgeAckOfACoordIdTargetIsAnErrorNamingFleetPoll` Restored, re-ran `mvn test -Dtest=FleetMcpTest,InMemoryReplyInboxTest` → **Tests run: 101, Failures: 0** — **BUILD SUCCESS**. **Mutation 2** (`AmqpReplyInbox.ack`'s `h == null` branch changed to `return true;` — was `return false;`; printed the mutated line to confirm it landed): `mvn -Pcontract test -Dtest=AmqpReplyInboxContractTest` (Testcontainers path) → **Tests run: 9, Failures: 1**, failing method: - `dev.ltms.fleet.msg.AmqpReplyInboxContractTest.ackReportsHitVsMissAgainstARealBroker` — `acking the same msgId twice must report false the second time ==> expected: <false> but was: <true>` Restored, re-ran `mvn -Pcontract test -Dtest=AmqpReplyInboxContractTest` → **Tests run: 9, Failures: 0** — **BUILD SUCCESS**. Then the full `mvn clean install` above (1577/0/0, BUILD SUCCESS) as the final state. ## Scope notes - Did not add boolean-return coverage to any other contract-group test class — only `AmqpReplyInboxContractTest` was named as the one CI actually runs. - Did not touch `LeadChannel`/`LeadMailbox`/`LeadCoordLoop` — confirmed (again) that `fleet_ack` has no route there and must not gain one. - Ran the CI-path check against a disposable `rabbitmq:3.13-management` container I started and stopped myself, never against the shared local LavinMQ instance the live fleetd daemon uses, to avoid leaving orphaned durable queues on production infrastructure.

agent added 1 commit 2026-09-10 11:44:41 +02:00

fleetd #437 : fleet_ack errors instead of claiming success on a miss

CI / contract (pull_request) Successful in 1m25s

Details

CI / build (pull_request) Successful in 1m37s

Details

703a05db41

ReplyInbox.ack now returns boolean (true = removed, false = nothing to
remove) instead of void, so FleetMcp.ack can finally tell a hit from a
miss. FleetMcp.ack returns an error when the boolean is false, naming
fleet_poll{coordId} for held peer mail, which has no route through this
call. MessageService.ackReply propagates the boolean; drainReplies keeps
ignoring it (its own javadoc already documents that loss window as
deliberate). Updated the tool schema's target description to match.

Rewrote FleetMcpTest's ack tests to publish a real message before
asserting success, and added tests for a never-queued id and a coord-id
target, both now erroring. Added boolean assertions to
InMemoryReplyInboxTest's existing ack cases.

agent added 1 commit 2026-09-10 11:59:03 +02:00

fleetd #437 : pin the ack hit/miss contract in the AMQP contract test

CI / contract (pull_request) Successful in 1m19s

Details

CI / build (pull_request) Successful in 1m34s

Details

5289eb509f

AmqpReplyInboxContractTest is the one contract-group class CI actually
runs, and it never asserted on ack()'s return value at all — so the
exact defect this ticket fixes (reporting success for an ack that
removed nothing) was unpinned in the adapter fleetd runs live.

Add ackReportsHitVsMissAgainstARealBroker: a msgId never held for an
owned target returns false without throwing, a real held reply returns
true and is removed, and acking the same msgId again returns false.
Ran against both broker modes the class supports: Testcontainers
(AMQP_URI unset) and an external broker via AMQP_URI (the CI shape,
using a disposable container — not the shared local LavinMQ instance).

ltms referenced this pull request

2026-09-10 11:59:28 +02:00

fleet_ack says "acknowledged <msgId>" for a held peer message it never touches #437

ltms merged commit c11ad71ed0 into main

2026-09-10 12:20:49 +02:00

ltms referenced this pull request

2026-09-10 12:21:07 +02:00

fleet_ack says "acknowledged <msgId>" for a held peer message it never touches #437

ltms referenced this issue from a commit

2026-09-10 12:35:12 +02:00

Merge #452: run the contract tag in CI, fix the stale protocol 14 assertion (fleetd #449)

ltms referenced this pull request