From 01492059d44621ecddaaab6c3dff881faa015133 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 5 Sep 2026 13:12:24 +0700 Subject: [PATCH] fleetd #361 review round 2: pin isMissingQueue's false branch 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). --- .../java/dev/ltms/fleet/msg/LeadMailbox.java | 19 ++++- .../msg/LeadMailboxIsMissingQueueTest.java | 77 +++++++++++++++++++ 2 files changed, 93 insertions(+), 3 deletions(-) create mode 100644 fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxIsMissingQueueTest.java diff --git a/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java b/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java index f9da75e..a2f3b64 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java +++ b/fleetd/src/main/java/dev/ltms/fleet/msg/LeadMailbox.java @@ -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. * + * + *

Honesty about which catch is measured and which is defensive: 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 between {@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; } diff --git a/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxIsMissingQueueTest.java b/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxIsMissingQueueTest.java new file mode 100644 index 0000000..aa2f50e --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/msg/LeadMailboxIsMissingQueueTest.java @@ -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. + * + *

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"); + } +}