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

LeadMailbox.inspect leaks a probe channel if the close is removed, and 15 contract tests stay green while the line runs 3 times #567

New Issue
Closed
opened 2026-09-12 12:39:56 +02:00 by ltms · 0 comments
ltms commented 2026-09-12 12:39:56 +02:00
Owner
Copy Link
Copy Source

Measured by me on main at ba2f4d1. This is a covered-but-unasserted instance, the family #556 turned up.

The line

LeadMailbox.java:338, in inspect()'s finally:

} finally {
    try {
        if (probe.isOpen()) {
            probe.close();          // :338
        }
    } catch (Exception e) {
        log.debug("lead mailbox inspect: probe channel close for {}: {}", coordId, e.toString());
    }
}

inspect() opens a disposable AMQP channel to run a passive declare, and this finally is the only thing that gives it back. Every successful inspection depends on it.

What I measured

Removed the line, ran the tests that actually exercise it:

run result
mvn -o test -Pcontract -Dtest=LeadMailboxTest, pristine exit 0, 15 tests, all green
same, with :338 removed exit 0, 15 tests, all green — mutation survives
:338 instrumented, pristine, same profile 3 hits

Anchor probe.close();, pristine count 1 → mutated 0. LeadMailbox.java restored to a2cd99be77345b7ef7f5818bb79139bcbeec7fe4663f174f4ccad393fff96dc1 after each run, verified by shasum -a 256.

So the line executes three times against a real broker and nothing asserts its effect. That is the covered-but-unasserted shape exactly: a coverage number says yes and a green suite says yes, and neither is looking at the postcondition.

What is and is not asserted today

LeadMailboxTest does assert channel closure once, at :330:

assertFalse(probe.isOpen(), "the 404 must have closed the channel the declare ran on");

but that is about the broker closing a channel on a 404, on a channel the test itself opened. It says nothing about inspect()'s own finally. The success-path test, inspectReportsAnOwnedMailboxAsExistingWithItsOwnConsumer at :196-208, asserts coordId, exists, pending and consumers — every value in the returned MailboxState, and nothing about the channel.

That is the prediction the fleet01 lead made about this family, and it holds here: the test asserts the result the method returns, not everything the protected block owed.

Consequence

An AMQP connection has a bounded channel limit. inspect() is called per fleet_list and per mailbox-state check, so a leak here is once per inspection for the life of the daemon, and it ends in channel exhaustion on a long-running fleetd rather than a visible failure at the call site. Nothing in the suite would catch a regression that removed or broke this finally.

I have not observed a leak in production — the line is present and correct today. This ticket is that nothing pins it.

Suggested fix

One assertion on the success path: after inspect() returns, the probe channel is closed. The channel is local to the method, so the test needs a seam — the cheapest is probably asserting the connection's open-channel count is unchanged across an inspect() call, which needs no production change and pins the invariant rather than the idiom.

Acceptance

  • A test that fails when :338 is removed, with its own assertion message and the observed value.
  • Mutation proof: line-anchored sed, anchor counted with grep -Fxc (not awk -v — it escape-processes the value), count 1 → 0, red with the test's own message, restored byte-identical under shasum -a 256, green control.
  • The test must be run under -Pcontract. See the note below.

Note for whoever picks this up: the default build does not run these tests

LeadMailboxTest is @Tag("contract"), and the default Maven profile sets excludedGroups=contract (fleetd/pom.xml:264). So mvn -o clean install never runs it, and against the default build this line shows 0 instrumented hits and every mutation survives — which reads identically to "there is no test", when in fact there are 15.

You need -Pcontract, which needs Docker or AMQP_URI. Docker 29.4.0 works on this host.

Related: #556 (the same family, found the same way), #561.

Measured by me on `main` at `ba2f4d1`. This is a **covered-but-unasserted** instance, the family #556 turned up. ## The line `LeadMailbox.java:338`, in `inspect()`'s `finally`: ```java } finally { try { if (probe.isOpen()) { probe.close(); // :338 } } catch (Exception e) { log.debug("lead mailbox inspect: probe channel close for {}: {}", coordId, e.toString()); } } ``` `inspect()` opens a disposable AMQP channel to run a passive declare, and this `finally` is the only thing that gives it back. Every successful inspection depends on it. ## What I measured Removed the line, ran the tests that actually exercise it: | run | result | |---|---| | `mvn -o test -Pcontract -Dtest=LeadMailboxTest`, pristine | exit 0, **15 tests**, all green | | same, with `:338` removed | exit 0, **15 tests**, all green — **mutation survives** | | `:338` instrumented, pristine, same profile | **3 hits** | Anchor ` probe.close();`, pristine count 1 → mutated 0. `LeadMailbox.java` restored to `a2cd99be77345b7ef7f5818bb79139bcbeec7fe4663f174f4ccad393fff96dc1` after each run, verified by `shasum -a 256`. So the line executes three times against a real broker and nothing asserts its effect. That is the covered-but-unasserted shape exactly: a coverage number says yes and a green suite says yes, and neither is looking at the postcondition. ## What is and is not asserted today `LeadMailboxTest` does assert channel closure once, at `:330`: ```java assertFalse(probe.isOpen(), "the 404 must have closed the channel the declare ran on"); ``` but that is about the **broker** closing a channel on a 404, on a channel the test itself opened. It says nothing about `inspect()`'s own `finally`. The success-path test, `inspectReportsAnOwnedMailboxAsExistingWithItsOwnConsumer` at `:196-208`, asserts `coordId`, `exists`, `pending` and `consumers` — every value in the returned `MailboxState`, and nothing about the channel. That is the prediction the fleet01 lead made about this family, and it holds here: the test asserts **the result the method returns**, not **everything the protected block owed**. ## Consequence An AMQP connection has a bounded channel limit. `inspect()` is called per `fleet_list` and per mailbox-state check, so a leak here is once per inspection for the life of the daemon, and it ends in channel exhaustion on a long-running fleetd rather than a visible failure at the call site. Nothing in the suite would catch a regression that removed or broke this `finally`. I have **not** observed a leak in production — the line is present and correct today. This ticket is that nothing pins it. ## Suggested fix One assertion on the success path: after `inspect()` returns, the probe channel is closed. The channel is local to the method, so the test needs a seam — the cheapest is probably asserting the connection's open-channel count is unchanged across an `inspect()` call, which needs no production change and pins the invariant rather than the idiom. ## Acceptance - A test that fails when `:338` is removed, with its own assertion message and the observed value. - Mutation proof: line-anchored `sed`, anchor counted with `grep -Fxc` (not `awk -v` — it escape-processes the value), count 1 → 0, red with the test's own message, restored byte-identical under `shasum -a 256`, green control. - The test must be run under `-Pcontract`. See the note below. ## Note for whoever picks this up: the default build does not run these tests `LeadMailboxTest` is `@Tag("contract")`, and the default Maven profile sets `excludedGroups=contract` (`fleetd/pom.xml:264`). So `mvn -o clean install` never runs it, and against the default build this line shows **0 instrumented hits** and every mutation survives — which reads identically to "there is no test", when in fact there are 15. You need `-Pcontract`, which needs Docker or `AMQP_URI`. Docker 29.4.0 works on this host. Related: #556 (the same family, found the same way), #561.
ltms closed this issue 2026-09-12 13:26:36 +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#567
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?