Merge #567: pin LeadMailbox.inspect's probe-channel close
fleetd #567. LeadMailbox.inspect opens a probe channel and closes it in a finally. The production
code was already correct; nothing asserted it, so a future refactor could drop the close and leak an
AMQP channel per inspect() call with the suite green.
Test only. LeadMailbox.java is untouched — sha256 a2cd99be77345b7e... before and after.
The test asserts the CONSEQUENCE rather than the return value: it caps the connection at three
channels (LeadMailbox uses two, consume and publish), runs a successful inspect, then requires a
replacement channel. If the probe is left open, the broker has no channel number left and
createChannel() returns null. A test that only checked inspect()'s MailboxState would pass under the
mutation, which is the whole reason this hole existed.
Lead verification, re-running rather than accepting the worker's numbers, on the branch merged with
main at ed2fd66:
mvn -o clean install -Pcontract -> exit 0, 1792 tests from Maven and from an independent sum
over 137 surefire reports.
1792 = 1761 (main) + 30 (contract-only) + 1 (new), which also confirms the default-profile count
is untouched: the new test is in an @Tag("contract") class.
My own mutation, located fresh rather than assuming the reported line number: delete
LeadMailbox.java:338 `probe.close();`, anchor by grep -Fxc 1 -> 0. Result under -Pcontract: exactly
1 failure, inspectClosesItsSuccessfulProbeChannel:226, "the replacement channel was null". Restored
to a2cd99be77345b7e..., git status --short empty.
THE PROFILE TRAP THIS TICKET EXISTS BECAUSE OF. An earlier sweep worker instrumented this line, saw
zero hits under the DEFAULT profile, and concluded "no test executes this line". That was false:
fleetd/pom.xml:264 sets excludedGroups=contract, so the covering tests were excluded from the run,
not absent. A surviving mutation has THREE causes — never executes, executes with nothing asserted,
or the covering tests were excluded from the profile — and only naming the profile tells them apart.
The true finding was covered-but-unasserted.
Known fragility, recorded rather than fixed: the test's channel cap of three assumes LeadMailbox
holds exactly two channels. If it ever holds more, this test fails loudly, which is fine. If it ever
holds fewer, a leak would no longer exhaust the cap and the test would go vacuous silently. Worth
re-checking if LeadMailbox's channel usage changes.
Not covered, and stated rather than faked: the defensive catch (RuntimeException) around the passive
declare, which needs a connection dying between createChannel() and the declare landing. The
method's own javadoc already admits that branch is unproven; the worker did not invent a test for it.
This commit was merged in pull request #573.
This commit is contained in:
@@ -19,6 +19,7 @@ import java.util.concurrent.atomic.AtomicLong;
|
|||||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||||
import static org.junit.jupiter.api.Assertions.assertFalse;
|
import static org.junit.jupiter.api.Assertions.assertFalse;
|
||||||
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
|
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
|
||||||
|
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
||||||
import static org.junit.jupiter.api.Assertions.assertThrows;
|
import static org.junit.jupiter.api.Assertions.assertThrows;
|
||||||
import static org.junit.jupiter.api.Assertions.assertTrue;
|
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||||
|
|
||||||
@@ -207,6 +208,31 @@ class LeadMailboxTest {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* {@link LeadMailbox#inspect} opens a third channel after the mailbox's consume and publish
|
||||||
|
* channels. Limit this connection to three channels, then require a replacement channel after
|
||||||
|
* the successful inspect. If inspect leaves its probe open, the broker refuses that replacement.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
void inspectClosesItsSuccessfulProbeChannel() throws Exception {
|
||||||
|
var factory = LeadMailbox.connectionFactory(uri());
|
||||||
|
factory.setRequestedChannelMax(3);
|
||||||
|
Connection connection = factory.newConnection();
|
||||||
|
try (LeadMailbox mailbox = new LeadMailbox(connection, coordId("lead-inspect-probe-close"))) {
|
||||||
|
LeadChannel.MailboxState state = mailbox.inspect(mailbox.selfCoordId());
|
||||||
|
assertTrue(state.exists(), "the owned mailbox must be found before checking the probe channel");
|
||||||
|
|
||||||
|
Channel replacement = connection.createChannel();
|
||||||
|
assertNotNull(replacement,
|
||||||
|
"inspect must close its successful probe channel; the replacement channel was null");
|
||||||
|
try {
|
||||||
|
assertTrue(replacement.isOpen(), "the replacement channel must be open after inspect returns");
|
||||||
|
} finally {
|
||||||
|
replacement.close();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* fleetd #440: {@code heldDurable()} must be derived from what {@link LeadMailbox#own} actually
|
* fleetd #440: {@code heldDurable()} must be derived from what {@link LeadMailbox#own} actually
|
||||||
* did against the real broker — a durable queue declare plus a manual-ack consumer — not a
|
* did against the real broker — a durable queue declare plus a manual-ack consumer — not a
|
||||||
|
|||||||
Reference in New Issue
Block a user