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

AmqpReplyInbox / LeadMailbox: the pendingByMsgId half of the dual-map cleanup is unasserted at every error-path site (from the #577 sweep) #582

New Issue
Closed
opened 2026-09-12 15:05:46 +02:00 by ltms · 1 comment
ltms commented 2026-09-12 15:05:46 +02:00
Owner
Copy Link
Copy Source

Found by the #577 per-site assertion sweep. Proven by mutation. Severity 2 of the two findings that sweep returned — real, but lower harm than the CompletionResolver finding filed alongside it.

The invariant

Every publish is tracked in two maps at once: pendingBySeq (keyed by the broker sequence number) and pendingByMsgId (keyed by message id). Whenever an entry leaves pendingBySeq, the matching pendingByMsgId entry must go too. Otherwise it stays forever — message ids are effectively never reused, so nothing ever cleans it up.

The sites — 5 in each of two classes

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

site lines
publish() catch 363-364
publish() finally 380-381
resolveConfirm() 499
failPendingPublishesOnRecovery() 531
failPendingPublishesOnClose() 549

fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java:

site lines
publish() catch 245-246
publish() finally 263-264
resolveConfirm() 488
failPendingPublishesOnRecovery() 514
failPendingPublishesOnClose() 531

The gap

AmqpReplyInboxRecoveryRaceTest.java does exercise failPendingPublishesOnRecovery() and close(), with a hermetic fake Channel/Connection and no Docker. But it only checks that a fresh concurrent publish is not wrongly failed, and that close() fails an in-flight publish promptly. It never asserts that pendingByMsgId was actually cleaned up.

LeadMailboxTest.java (401 lines) was grepped for pendingByMsgId, failPendingPublishesOn and resolveConfirm — no test touches this invariant there at all.

Mutation proof (both survived)

Mutation 3 — AmqpReplyInbox.java:531 (failPendingPublishesOnRecovery()). Deleted pendingByMsgId.remove(pending.msgId, pending); with a line-anchored sed. The anchor appears at both 531 and 549, so the count dropped 2 → 1, confirming the edit hit exactly one line.

BUILD SUCCESS — Tests run: 1766, Failures: 0, Errors: 0   (default profile)

Restored; sha256 matched e6279ef810f503ce8c5ce9404049b732c58ef377061a8defeff97b951bb952ed.

Mutation 4 — LeadMailbox.java:531 (failPendingPublishesOnClose()). Same deletion, count 2 → 1.

BUILD SUCCESS — Tests run: 1797, Failures: 0, Errors: 0   (-Pcontract)

The contract profile was used deliberately here: LeadMailboxTest needs Docker via Testcontainers RabbitMQ and is excluded by default (pom.xml:264), so this run includes the ~30 contract tests and nothing relevant was skipped. Restored; sha256 matched a2cd99be77345b7ef7f5818bb79139bcbeec7fe4663f174f4ccad393fff96dc1, git status --short clean, and a final default-profile build passed at 1766.

Reachability confirmed, not assumed: failPendingPublishesOnRecovery() runs on AMQP connection recovery while a publish is unconfirmed, and AmqpReplyInboxRecoveryRaceTest drives that path directly — it still passes after the mutation. failPendingPublishesOnClose() runs on mailbox shutdown with an unconfirmed publish, driven by LeadMailboxTest's close-path tests.

Why severity 2

The consequence is a stale Pending object left in pendingByMsgId under an id that is practically never reused. The only reader of pendingByMsgId is onReturn(), so the stale entry just sits there unread. That is a slow, bounded memory leak — not a wrong answer and not a lost message. Real, but well below the misdirected-completion risk in the sibling finding.

Prior art — this exact shape was already fixed once

The sweeping worker checked git history before counting these as new. LeadMailbox.inspect()'s probe.close() had the same defect, fixed in commit 3f8c38f (merged as #567, test-only). That makes four known instances of this shape alongside #561, #572 and #575. It is not reported again here; it is named so the history is on the record.

The work

Assert the pendingByMsgId half at every site that is currently unasserted, in both classes.

Acceptance:

  1. One assertion per site, in both files. Ten sites total; a single combined test is not acceptable, for the reason this whole sweep exists.
  2. Each test red under its own mutation. Delete that site's pendingByMsgId.remove(...) line, show the named failure, restore, paste the sha256.
  3. LeadMailboxTest needs Docker — run it with -Pcontract and say so in the PR, with the test count from that run. Also run the default profile and report both numbers.
  4. Test-only. Do not change production code.
Found by the #577 per-site assertion sweep. Proven by mutation. Severity 2 of the two findings that sweep returned — real, but lower harm than the `CompletionResolver` finding filed alongside it. ## The invariant Every publish is tracked in **two** maps at once: `pendingBySeq` (keyed by the broker sequence number) and `pendingByMsgId` (keyed by message id). Whenever an entry leaves `pendingBySeq`, the matching `pendingByMsgId` entry must go too. Otherwise it stays forever — message ids are effectively never reused, so nothing ever cleans it up. ## The sites — 5 in each of two classes `fleetd/src/main/java/dev/ltms/fleet/msg/AmqpReplyInbox.java`: | site | lines | |---|---| | `publish()` catch | 363-364 | | `publish()` finally | 380-381 | | `resolveConfirm()` | 499 | | `failPendingPublishesOnRecovery()` | 531 | | `failPendingPublishesOnClose()` | 549 | `fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java`: | site | lines | |---|---| | `publish()` catch | 245-246 | | `publish()` finally | 263-264 | | `resolveConfirm()` | 488 | | `failPendingPublishesOnRecovery()` | 514 | | `failPendingPublishesOnClose()` | 531 | ## The gap `AmqpReplyInboxRecoveryRaceTest.java` does exercise `failPendingPublishesOnRecovery()` and `close()`, with a hermetic fake `Channel`/`Connection` and no Docker. But it only checks that a fresh concurrent publish is not wrongly failed, and that `close()` fails an in-flight publish promptly. **It never asserts that `pendingByMsgId` was actually cleaned up.** `LeadMailboxTest.java` (401 lines) was grepped for `pendingByMsgId`, `failPendingPublishesOn` and `resolveConfirm` — **no test touches this invariant there at all.** ## Mutation proof (both survived) **Mutation 3 — `AmqpReplyInbox.java:531`** (`failPendingPublishesOnRecovery()`). Deleted `pendingByMsgId.remove(pending.msgId, pending);` with a line-anchored `sed`. The anchor appears at both 531 and 549, so the count dropped 2 → 1, confirming the edit hit exactly one line. ``` BUILD SUCCESS — Tests run: 1766, Failures: 0, Errors: 0 (default profile) ``` Restored; `sha256` matched `e6279ef810f503ce8c5ce9404049b732c58ef377061a8defeff97b951bb952ed`. **Mutation 4 — `LeadMailbox.java:531`** (`failPendingPublishesOnClose()`). Same deletion, count 2 → 1. ``` BUILD SUCCESS — Tests run: 1797, Failures: 0, Errors: 0 (-Pcontract) ``` The contract profile was used deliberately here: `LeadMailboxTest` needs Docker via Testcontainers RabbitMQ and is excluded by default (`pom.xml:264`), so this run **includes** the ~30 contract tests and nothing relevant was skipped. Restored; `sha256` matched `a2cd99be77345b7ef7f5818bb79139bcbeec7fe4663f174f4ccad393fff96dc1`, `git status --short` clean, and a final default-profile build passed at 1766. **Reachability confirmed, not assumed:** `failPendingPublishesOnRecovery()` runs on AMQP connection recovery while a publish is unconfirmed, and `AmqpReplyInboxRecoveryRaceTest` drives that path directly — it still passes after the mutation. `failPendingPublishesOnClose()` runs on mailbox shutdown with an unconfirmed publish, driven by `LeadMailboxTest`'s close-path tests. ## Why severity 2 The consequence is a stale `Pending` object left in `pendingByMsgId` under an id that is practically never reused. The only reader of `pendingByMsgId` is `onReturn()`, so the stale entry just sits there unread. That is a slow, bounded memory leak — not a wrong answer and not a lost message. Real, but well below the misdirected-completion risk in the sibling finding. ## Prior art — this exact shape was already fixed once The sweeping worker checked git history before counting these as new. `LeadMailbox.inspect()`'s `probe.close()` had the same defect, fixed in commit `3f8c38f` (merged as #567, test-only). That makes **four** known instances of this shape alongside #561, #572 and #575. It is not reported again here; it is named so the history is on the record. ## The work Assert the `pendingByMsgId` half at every site that is currently unasserted, in both classes. Acceptance: 1. **One assertion per site, in both files.** Ten sites total; a single combined test is not acceptable, for the reason this whole sweep exists. 2. **Each test red under its own mutation.** Delete that site's `pendingByMsgId.remove(...)` line, show the named failure, restore, paste the `sha256`. 3. `LeadMailboxTest` needs Docker — run it with `-Pcontract` and say so in the PR, with the test count from that run. Also run the default profile and report both numbers. 4. Test-only. Do not change production code.
ltms commented 2026-09-12 15:52:56 +02:00
Author
Owner
Copy Link
Copy Source

Done. PR #583 merged as 49a5875.

Ten assertions, one per pendingByMsgId cleanup site across AmqpReplyInbox (5) and LeadMailbox
(5). Every one is proven to fail when the line it guards is removed — six proofs by the
implementer, the last four run by me. Full numbers and the mutation table are in the PR.

Contract build on the merged tree: Tests run: 1825, Failures: 0. The four-mutation run gave
1825, Failures: 4 — exactly four named failures, one per site, no crowd.

What this ticket cost, and the process lesson

The implementer wrote all ten assertions in its first turn and proved three. It said so plainly
rather than reporting the PR as finished, which is the behaviour I want and the reason the gap was
visible at all. I sent it back; it proved three more and again reported honestly what it had not run.
I ran the last four.

AN IMPLEMENTED ASSERTION IS NOT A PROVEN ASSERTION, AND THE DIFFERENCE IS INVISIBLE IN THE DIFF.
Ten green assertions and three green assertions look identical in review — both are green. A test
that asserts nothing useful passes exactly as fast as one that pins a real invariant. So the proof
obligation has to be part of the brief and part of the merge gate, not a nice-to-have.

The brief defect on my side: my first brief asked for the assertions and asked for proofs, but
did not say how many builds that is. Ten sites means ten mutations, and each is a full build on a
contract profile. A worker that reads "prove them" and has a turn budget will prove what fits. The
fix for next time is to state the count, and to say that an unproven assertion must be listed by
name in the reply — which it did, and which is the only reason this closed correctly.

Done. PR #583 merged as `49a5875`. Ten assertions, one per `pendingByMsgId` cleanup site across `AmqpReplyInbox` (5) and `LeadMailbox` (5). **Every one is proven to fail when the line it guards is removed** — six proofs by the implementer, the last four run by me. Full numbers and the mutation table are in the PR. Contract build on the merged tree: `Tests run: 1825, Failures: 0`. The four-mutation run gave `1825, Failures: 4` — exactly four named failures, one per site, no crowd. ### What this ticket cost, and the process lesson The implementer wrote all ten assertions in its first turn and proved **three**. It said so plainly rather than reporting the PR as finished, which is the behaviour I want and the reason the gap was visible at all. I sent it back; it proved three more and again reported honestly what it had not run. I ran the last four. **AN IMPLEMENTED ASSERTION IS NOT A PROVEN ASSERTION, AND THE DIFFERENCE IS INVISIBLE IN THE DIFF.** Ten green assertions and three green assertions look identical in review — both are green. A test that asserts nothing useful passes exactly as fast as one that pins a real invariant. So the proof obligation has to be part of the brief and part of the merge gate, not a nice-to-have. **The brief defect on my side:** my first brief asked for the assertions and asked for proofs, but did not say *how many builds that is*. Ten sites means ten mutations, and each is a full build on a contract profile. A worker that reads "prove them" and has a turn budget will prove what fits. The fix for next time is to state the count, and to say that an unproven assertion must be listed by name in the reply — which it did, and which is the only reason this closed correctly.
ltms closed this issue 2026-09-12 15:53:02 +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#582
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?