fleetd #361 review round 2: pin isMissingQueue's false branch
CI / contract (pull_request) Successful in 1m5s
CI / build (pull_request) Successful in 1m34s

The reviewer's mutation (isMissingQueue always returns true) restored
the exact overstatement fleetd #361 exists to fix -- every declare
failure reading as a confirmed absence -- and still left mvn clean
install green (1389/1389), because no test drove a non-404 shape
through inspect(). The false branch was the whole discriminator
between MailboxState.absent() and MailboxState.unknown(), unpinned.

Widened LeadMailbox.isMissingQueue from private to package-private and
added LeadMailboxIsMissingQueueTest: five hermetic tests (no broker)
covering the true case and all three false shapes isMissingQueue's own
javadoc lists -- a different reply code, a ShutdownSignalException
whose reason isn't a Channel.Close, and an IOException with no such
cause at all (plus an IOException wrapping an unrelated exception
type). Re-ran the reviewer's exact mutation locally: 4 of 5 new tests
went red with the expected assertion messages; reverted, and mvn clean
install is green again at 1394/1394 (1389 + 5 new).

Also added a one-line javadoc note on LeadMailbox.inspect being honest
about which of its two RuntimeException catches is proven by a test
(the createChannel() one, end-to-end against a real broker) and which
stays purely defensive (the declare-site one, for a connection-drops-
mid-call race no test drives on purpose).
This commit is contained in:
Dai Ha
2026-09-05 13:12:24 +07:00
parent c4d40fbc2b
commit 01492059d4
2 changed files with 93 additions and 3 deletions
@@ -280,6 +280,16 @@ public final class LeadMailbox implements LeadChannel, AutoCloseable {
* not an {@link IOException}) — an {@code inspect} that only caught {@code IOException}
* would let that escape, breaking the "never throws" contract this method promises.
* </ul>
*
* <p><strong>Honesty about which catch is measured and which is defensive:</strong> the
* {@code createChannel()} catch above is exercised end-to-end against a real broker by
* {@code LeadMailboxTest.inspectReportsUnknownRatherThanThrowingWhenTheConnectionIsAlreadyClosed}.
* The second {@code catch (RuntimeException e)}, around the passive declare itself — for the
* narrower race where the connection drops <em>between</em> {@code createChannel()} succeeding
* and the declare landing — has no such test; reaching it needs a connection that dies at that
* exact instant, which is not a scenario this suite drives on purpose. It stays purely
* defensive: correct by the same reasoning as the first catch, but unproven the way the first
* one is proven.
*/
@Override
public MailboxState inspect(String coordId) {
@@ -320,10 +330,13 @@ public final class LeadMailbox implements LeadChannel, AutoCloseable {
* LeadMailboxTest.passiveDeclareOfAMissingQueueThrowsAnIOExceptionWrappingA404ShutdownSignal}):
* an {@link IOException} whose cause is a {@link ShutdownSignalException} carrying an
* {@link AMQP.Channel.Close} reason with {@code replyCode == 404}. Any other shape (a different
* reply code, no {@code ShutdownSignalException} cause, or none at all) is a declare failure of
* some other kind and must not be read as "confirmed absent".
* reply code, a {@code ShutdownSignalException} cause whose reason is not a
* {@code Channel.Close}, or no cause at all) is a declare failure of some other kind and must
* not be read as "confirmed absent" — pinned hermetically, with no broker needed, by
* {@code LeadMailboxIsMissingQueueTest} for exactly those three false shapes. Package-private
* (not {@code private}) so that test can call it directly.
*/
private static boolean isMissingQueue(IOException e) {
static boolean isMissingQueue(IOException e) {
if (!(e.getCause() instanceof ShutdownSignalException sse)) {
return false;
}
@@ -0,0 +1,77 @@
package dev.ltms.fleet.msg;
import com.rabbitmq.client.ShutdownSignalException;
import com.rabbitmq.client.impl.AMQImpl;
import org.junit.jupiter.api.Test;
import java.io.IOException;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
/**
* fleetd #361 review round 2: a mutation that made {@link LeadMailbox#isMissingQueue} return
* {@code true} unconditionally still left {@code mvn clean install} green — 1389 tests, 0
* failures — because nothing exercised its false branch. That branch is the whole discriminator
* between {@link LeadChannel.MailboxState#absent} and {@link LeadChannel.MailboxState#unknown};
* without a test pinning it, a future refactor that widens it back to "always true" (restoring the
* exact overstatement fleetd #361 exists to fix) would pass this suite.
*
* <p>Hermetic — no broker needed, per the review's own suggestion. {@code isMissingQueue} takes a
* plain {@link IOException}, so every input here is constructed directly rather than provoked from
* a live connection. The real 404 shape itself is still pinned against a real broker, in
* {@code LeadMailboxTest.passiveDeclareOfAMissingQueueThrowsAnIOExceptionWrappingA404ShutdownSignal}
* — this class covers the three false shapes {@link LeadMailbox#isMissingQueue}'s own javadoc
* lists, so both directions of the discriminator are proven somewhere.
*/
class LeadMailboxIsMissingQueueTest {
@Test
void aConfirmedMissingQueueIsRecognized() {
ShutdownSignalException sse = new ShutdownSignalException(true, false,
new AMQImpl.Channel.Close(404, "NOT_FOUND - no queue 'lead.x.inbox' in vhost '/'", 50, 10), null);
IOException e = new IOException("channel error", sse);
assertTrue(LeadMailbox.isMissingQueue(e), "a genuine 404 Channel.Close must be recognized as a missing queue");
}
@Test
void aDifferentReplyCodeIsNotAMissingQueue() {
// E.g. 403 ACCESS_REFUSED — the queue may well exist; this call was simply refused.
ShutdownSignalException sse = new ShutdownSignalException(true, false,
new AMQImpl.Channel.Close(403, "ACCESS_REFUSED", 50, 10), null);
IOException e = new IOException("channel error", sse);
assertFalse(LeadMailbox.isMissingQueue(e),
"a non-404 reply code must never be read as a confirmed absence — the mailbox's real state is unknown");
}
@Test
void aShutdownSignalWhoseReasonIsNotAChannelCloseIsNotAMissingQueue() {
// A Connection.Close (a whole different broker-level shutdown) is still a ShutdownSignalException,
// but its reason is not a Channel.Close at all — must not be misread as "no such queue".
ShutdownSignalException sse = new ShutdownSignalException(true, false,
new AMQImpl.Connection.Close(404, "coincidentally 404, but this is a CONNECTION close", 10, 50), null);
IOException e = new IOException("connection error", sse);
assertFalse(LeadMailbox.isMissingQueue(e),
"a ShutdownSignalException whose reason is not a Channel.Close must never be read as a missing queue,"
+ " even if its reply code happens to be 404");
}
@Test
void anIOExceptionWithNoCauseAtAllIsNotAMissingQueue() {
IOException e = new IOException("some other declare failure, no cause attached");
assertFalse(LeadMailbox.isMissingQueue(e),
"an IOException with no ShutdownSignalException cause must never be read as a confirmed absence");
}
@Test
void anIOExceptionWithAnUnrelatedCauseIsNotAMissingQueue() {
IOException e = new IOException("wrapped something else entirely", new RuntimeException("boom"));
assertFalse(LeadMailbox.isMissingQueue(e),
"a cause that isn't even a ShutdownSignalException must never be read as a confirmed absence");
}
}