From d7fab215383a2687711238cf2385ce1cea3bdfff Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 10:51:34 +0000 Subject: [PATCH 1/9] Add ResendMessagesBugDirectConnectionTest showcasing bug #344 with live acceptor/initiator connection Agent-Logs-Url: https://github.com/quickfix-j/quickfixj/sessions/51698dc5-437b-4294-9bc1-1f77b2c74796 Co-authored-by: chrjohn <6644028+chrjohn@users.noreply.github.com> --- ...ResendMessagesBugDirectConnectionTest.java | 426 ++++++++++++++++++ 1 file changed, 426 insertions(+) create mode 100644 quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java diff --git a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java new file mode 100644 index 000000000..cd0b42bd4 --- /dev/null +++ b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java @@ -0,0 +1,426 @@ +/******************************************************************************* + * Copyright (c) quickfixengine.org All rights reserved. + * + * This file is part of the QuickFIX FIX Engine + * + * This file may be distributed under the terms of the quickfixengine.org + * license as defined by quickfixengine.org and appearing in the file + * LICENSE included in the packaging of this file. + * + * This file is provided AS IS with NO WARRANTY OF ANY KIND, INCLUDING + * THE WARRANTY OF DESIGN, MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE. + * + * See http://www.quickfixengine.org/LICENSE for licensing information. + * + * Contact ask@quickfixengine.org if any conditions of this licensing + * are not clear to you. + ******************************************************************************/ + +package quickfix; + +import org.apache.mina.util.AvailablePortFinder; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import quickfix.field.BeginSeqNo; +import quickfix.field.BeginString; +import quickfix.field.EncryptMethod; +import quickfix.field.EndSeqNo; +import quickfix.field.GapFillFlag; +import quickfix.field.HeartBtInt; +import quickfix.field.MsgSeqNum; +import quickfix.field.MsgType; +import quickfix.field.NewSeqNo; +import quickfix.field.SenderCompID; +import quickfix.field.SendingTime; +import quickfix.field.TargetCompID; +import quickfix.mina.ProtocolFactory; + +import java.io.IOException; +import java.time.LocalDateTime; +import java.time.ZoneOffset; +import java.util.HashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +/** + * Integration test demonstrating bug #344 in {@link Session#resendMessages}. + * + *

The bug: when the last retrieved persisted message is an admin message, + * the {@code newBegin} variable is not updated before + * {@code generateSequenceResetIfNeeded} is called. As a result the method emits a + * SequenceReset-GapFill whose {@code MsgSeqNum} equals {@code beginSeqNo} (= 1) instead + * of the correct value (= first seqno of the trailing admin block = 6). The counterparty + * therefore receives a SequenceReset with a "too low" {@code MsgSeqNum} and silently drops + * it, leaving the session in a stuck state. + * + *

Scenario set up on the acceptor side (pre-populated message store): + * 

+ *   seqno 1  – Logon        (admin)
+ *   seqno 2  – ResendRequest (admin)
+ *   seqno 3  – ExecutionReport (app)   ← triggers SequenceReset(1→3) gap-fill
+ *   seqno 4  – ExecutionReport (app)   ← resent
+ *   seqno 5  – Heartbeat    (admin)    ← last retrieved = admin → triggers the bug
+ *   seqnos 6-10 – NOT in store
+ *   NextSenderMsgSeqNum = 11
+ *
+ * + *

When the initiator (fresh store, NextTargetSeqNum = 1) connects: + *

    + *
  1. Acceptor sends Logon(11).
    2. + *
  2. Initiator detects "too high" and sends ResendRequest(1, 0).
    3. + *
  3. Acceptor calls {@code resendMessages(1, 11)}: + * sends SequenceReset(1→3), ExecutionReport(3), ExecutionReport(4), + * SequenceReset(5→6), and — due to the bug — SequenceReset(MsgSeqNum=1, NewSeqNo=12).
    4. + *
  4. The correct trailing SequenceReset should have MsgSeqNum=6.
    5. + *
+ * + *

The test assertion {@code assertEquals(6, actualMsgSeqNum)} fails on current code + * (gets 1) and passes once the bug is fixed. + */ +public class ResendMessagesBugDirectConnectionTest { + + private final Logger log = LoggerFactory.getLogger(getClass()); + + private final SessionID acceptorSessionID = new SessionID( + FixVersions.BEGINSTRING_FIX42, "ACCEPTOR", "INITIATOR"); + private final SessionID initiatorSessionID = new SessionID( + FixVersions.BEGINSTRING_FIX42, "INITIATOR", "ACCEPTOR"); + + // ----------------------------------------------------------------------- + // Test + // ----------------------------------------------------------------------- + + @Test + public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() + throws Exception { + + AcceptorApplication acceptorApp = new AcceptorApplication(); + TestConnectorApplication initiatorApp = new TestConnectorApplication(); + + Acceptor acceptor = null; + Initiator initiator = null; + try { + final int port = AvailablePortFinder.getNextAvailable(); + + acceptor = createAcceptor(acceptorApp, port); + acceptor.start(); + + initiator = createInitiator(initiatorApp, port); + initiator.start(); + + // Both sessions reach onLogon quickly because the acceptor's Logon(11) + // is "too high" for the initiator, which still calls onLogon after + // sending the ResendRequest. + acceptorApp.waitForLogon(); + initiatorApp.waitForLogon(); + + // Now wait for the acceptor to finish the resend triggered by the + // initiator's ResendRequest. The last thing it sends is the + // trailing-gap SequenceReset that exhibits the bug. + assertTrue("Timed out waiting for the trailing-gap SequenceReset from acceptor", + acceptorApp.waitForTrailingGapSeqReset(10, TimeUnit.SECONDS)); + + Message trailingSeqReset = acceptorApp.getTrailingGapSeqReset(); + assertNotNull("Trailing-gap SequenceReset should have been captured", trailingSeqReset); + + final int actualMsgSeqNum = trailingSeqReset.getHeader().getInt(MsgSeqNum.FIELD); + final int newSeqNo = trailingSeqReset.getInt(NewSeqNo.FIELD); + + log.info("Trailing SequenceReset: MsgSeqNum={}, NewSeqNo={}", actualMsgSeqNum, newSeqNo); + + // Sanity-check: this IS the trailing-gap SequenceReset (NewSeqNo > 10) + assertTrue("Trailing SequenceReset should advance past the gap (NewSeqNo > 10), got " + + newSeqNo, newSeqNo > 10); + + // THE KEY ASSERTION: + // MsgSeqNum must equal 6 (the first seqno of the trailing admin block, + // i.e. msgSeqNum of last app message + 1 = 4 + 1 = 5, then next is 6 + // because seqno 5 is the Heartbeat gap-filled by the previous reset). + // + // Due to bug #344 the actual value is 1 (beginSeqNo), which is "too low" + // for the initiator (it has already advanced to seqno 6). + // This assertion FAILS on the current code and PASSES once the bug is fixed. + assertEquals( + "Bug #344: trailing SequenceReset has MsgSeqNum=" + actualMsgSeqNum + + " but should be 6 (first seqno after the last resent app message). " + + "MsgSeqNum=1 is 'too low' for the initiator (already at seqno 6) " + + "causing the SequenceReset to be silently dropped.", + 6, actualMsgSeqNum); + + } finally { + if (initiator != null) { + try { initiator.stop(); } catch (RuntimeException e) { log.error(e.getMessage(), e); } + } + if (acceptor != null) { + try { acceptor.stop(); } catch (RuntimeException e) { log.error(e.getMessage(), e); } + } + } + } + + // ----------------------------------------------------------------------- + // Connector factories + // ----------------------------------------------------------------------- + + private Acceptor createAcceptor(Application app, int port) throws ConfigError { + SessionSettings settings = new SessionSettings(); + HashMap defaults = new HashMap<>(); + defaults.put("ConnectionType", "acceptor"); + defaults.put("StartTime", "00:00:00"); + defaults.put("EndTime", "00:00:00"); + defaults.put("BeginString", FixVersions.BEGINSTRING_FIX42); + defaults.put("NonStopSession", "Y"); + // Disable data-dictionary validation so the pre-built stored messages + // do not need to satisfy required-field checks. + defaults.put(Session.SETTING_USE_DATA_DICTIONARY, "N"); + settings.setString(acceptorSessionID, "SocketAcceptProtocol", + ProtocolFactory.getTypeString(ProtocolFactory.SOCKET)); + settings.setString(acceptorSessionID, "SocketAcceptPort", String.valueOf(port)); + settings.set(defaults); + + MessageStoreFactory storeFactory = new BugScenarioStoreFactory(acceptorSessionID); + LogFactory logFactory = new SLF4JLogFactory(new SessionSettings()); + return new SocketAcceptor(app, storeFactory, settings, logFactory, + new DefaultMessageFactory()); + } + + private Initiator createInitiator(Application app, int port) throws ConfigError { + SessionSettings settings = new SessionSettings(); + HashMap defaults = new HashMap<>(); + defaults.put("ConnectionType", "initiator"); + defaults.put("StartTime", "00:00:00"); + defaults.put("EndTime", "00:00:00"); + defaults.put("HeartBtInt", "30"); + defaults.put("ReconnectInterval", "2"); + defaults.put("NonStopSession", "Y"); + // Disable data-dictionary so the minimally-populated ExecutionReports + // resent by the acceptor are accepted without required-field validation. + defaults.put(Session.SETTING_USE_DATA_DICTIONARY, "N"); + settings.setString(initiatorSessionID, "SocketConnectProtocol", + ProtocolFactory.getTypeString(ProtocolFactory.SOCKET)); + settings.setString(initiatorSessionID, "SocketConnectHost", "127.0.0.1"); + settings.setString(initiatorSessionID, "SocketConnectPort", String.valueOf(port)); + settings.set(defaults); + + MessageStoreFactory factory = new MemoryStoreFactory(); + LogFactory logFactory = new SLF4JLogFactory(new SessionSettings()); + return new SocketInitiator(app, factory, settings, logFactory, + new DefaultMessageFactory()); + } + + // ----------------------------------------------------------------------- + // Pre-populated message store + // ----------------------------------------------------------------------- + + /** + * {@link MessageStoreFactory} that returns a {@link BugScenarioStore} for the + * target acceptor session and a plain {@link MemoryStore} for all others. + */ + private static class BugScenarioStoreFactory implements MessageStoreFactory { + + private final SessionID targetSessionID; + + BugScenarioStoreFactory(SessionID sessionID) { + this.targetSessionID = sessionID; + } + + @Override + public MessageStore create(SessionID sessionID) { + try { + if (targetSessionID.equals(sessionID)) { + return new BugScenarioStore(); + } + return new MemoryStore(); + } catch (IOException e) { + throw new RuntimeException("Failed to create BugScenarioStore", e); + } + } + } + + /** + * A {@link MemoryStore} pre-populated with the sequence-number scenario that + * triggers the resendMessages bug: + *

+ * + *

Writes for sequence numbers greater than 5 are silently discarded so that + * the Logon sent during session start-up does not fill the artificial gap. + */ + private static class BugScenarioStore extends MemoryStore { + + private static final int LAST_PREPOPULATED_SEQNO = 5; + + BugScenarioStore() throws IOException { + super(); + populateBugScenario(); + } + + private void populateBugScenario() throws IOException { + final DefaultMessageFactory factory = new DefaultMessageFactory(); + final String beginString = FixVersions.BEGINSTRING_FIX42; + final String sender = "ACCEPTOR"; + final String target = "INITIATOR"; + // Use a fixed timestamp so stored messages have a valid SendingTime + // (required by initializeResendFields when the message is re-sent). + final LocalDateTime ts = LocalDateTime.of(2020, 1, 1, 12, 0, 0); + + // seqno 1: Logon (admin) + Message logon = factory.create(beginString, MsgType.LOGON); + logon.setInt(EncryptMethod.FIELD, 0); + logon.setInt(HeartBtInt.FIELD, 30); + setHeader(logon, beginString, sender, target, 1, ts); + super.set(1, logon.toString()); + + // seqno 2: ResendRequest (admin) + Message resendReq = factory.create(beginString, MsgType.RESEND_REQUEST); + resendReq.setInt(BeginSeqNo.FIELD, 1); + resendReq.setInt(EndSeqNo.FIELD, 0); + setHeader(resendReq, beginString, sender, target, 2, ts); + super.set(2, resendReq.toString()); + + // seqnos 3–4: ExecutionReport (app) — minimal fields, no data-dictionary + for (int seqno = 3; seqno <= 4; seqno++) { + Message execReport = factory.create(beginString, MsgType.EXECUTION_REPORT); + setHeader(execReport, beginString, sender, target, seqno, ts); + // Body can be minimal because UseDataDictionary=N is set on both sides + execReport.setString(37, "ORD-" + seqno); // OrderID + execReport.setString(17, "EXEC-" + seqno); // ExecID + super.set(seqno, execReport.toString()); + } + + // seqno 5: Heartbeat (admin) — the last stored message is admin: + // this is what triggers the bug when resendMessages processes the list. + Message heartbeat = factory.create(beginString, MsgType.HEARTBEAT); + setHeader(heartbeat, beginString, sender, target, 5, ts); + super.set(5, heartbeat.toString()); + + // seqnos 6–10 intentionally absent (simulating messages that were sent + // but are no longer available in the store). + + // NextSenderMsgSeqNum = 11 creates the gap that will be covered by the + // trailing SequenceReset in resendMessages. + setNextSenderMsgSeqNum(11); + setNextTargetMsgSeqNum(1); + } + + private static void setHeader(Message msg, String beginString, String sender, + String target, int seqno, LocalDateTime sendingTime) { + msg.getHeader().setString(BeginString.FIELD, beginString); + msg.getHeader().setString(SenderCompID.FIELD, sender); + msg.getHeader().setString(TargetCompID.FIELD, target); + msg.getHeader().setInt(MsgSeqNum.FIELD, seqno); + msg.getHeader().setUtcTimeStamp(SendingTime.FIELD, sendingTime); + } + + /** + * Discard any write for sequence numbers beyond the pre-populated range. + * This prevents the Logon sent at start-up (seqno 11) from appearing in + * the store and thereby hiding the gap that triggers the bug. + */ + @Override + public boolean set(int sequence, String message) throws IOException { + if (sequence > LAST_PREPOPULATED_SEQNO) { + return true; // discard – preserve the artificial gap + } + return super.set(sequence, message); + } + } + + // ----------------------------------------------------------------------- + // Applications + // ----------------------------------------------------------------------- + + /** + * Acceptor application that: + *

+ */ + private static class AcceptorApplication extends ApplicationAdapter { + + private final CountDownLatch logonLatch = new CountDownLatch(1); + private final CountDownLatch trailingSeqResetLatch = new CountDownLatch(1); + private final AtomicReference trailingGapSeqReset = new AtomicReference<>(); + + @Override + public void onLogon(SessionID sessionId) { + logonLatch.countDown(); + } + + @Override + public void toAdmin(Message message, SessionID sessionId) { + try { + if (!MsgType.SEQUENCE_RESET.equals(message.getHeader().getString(MsgType.FIELD))) { + return; + } + if (!message.isSetField(GapFillFlag.FIELD) || !message.getBoolean(GapFillFlag.FIELD)) { + return; + } + // The trailing-gap SequenceReset advances well past the original + // NextSenderMsgSeqNum (11); its NewSeqNo will be 12 in this scenario. + if (message.isSetField(NewSeqNo.FIELD) && message.getInt(NewSeqNo.FIELD) > 10) { + trailingGapSeqReset.set(message); + trailingSeqResetLatch.countDown(); + } + } catch (FieldNotFound e) { + // ignore + } + } + + void waitForLogon() { + try { + assertTrue("Acceptor logon timed out", logonLatch.await(10, TimeUnit.SECONDS)); + } catch (InterruptedException e) { + fail("Interrupted while waiting for acceptor logon"); + } + } + + boolean waitForTrailingGapSeqReset(long timeout, TimeUnit unit) { + try { + return trailingSeqResetLatch.await(timeout, unit); + } catch (InterruptedException e) { + return false; + } + } + + Message getTrailingGapSeqReset() { + return trailingGapSeqReset.get(); + } + } + + /** Plain application adapter used for the initiator side. */ + private static class TestConnectorApplication extends ApplicationAdapter { + + private final CountDownLatch logonLatch = new CountDownLatch(1); + + @Override + public void onLogon(SessionID sessionId) { + logonLatch.countDown(); + } + + void waitForLogon() { + try { + assertTrue("Initiator logon timed out", logonLatch.await(10, TimeUnit.SECONDS)); + } catch (InterruptedException e) { + fail("Interrupted while waiting for initiator logon"); + } + } + } +} From 2d3076883bbc3e6f6c9b0782ccb2cd258235c5ea Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 15:04:03 +0000 Subject: [PATCH 2/9] Add logging to toAdmin/fromAdmin/toApp/fromApp in AcceptorApplication and TestConnectorApplication Agent-Logs-Url: https://github.com/quickfix-j/quickfixj/sessions/f97854dd-c016-45b5-8b11-6d0c47ff8fe6 Co-authored-by: chrjohn <6644028+chrjohn@users.noreply.github.com> --- ...ResendMessagesBugDirectConnectionTest.java | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java index cd0b42bd4..fad3913cc 100644 --- a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java +++ b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java @@ -39,7 +39,6 @@ import java.io.IOException; import java.time.LocalDateTime; -import java.time.ZoneOffset; import java.util.HashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -355,6 +354,8 @@ public boolean set(int sequence, String message) throws IOException { */ private static class AcceptorApplication extends ApplicationAdapter { + private final Logger log = LoggerFactory.getLogger(AcceptorApplication.class); + private final CountDownLatch logonLatch = new CountDownLatch(1); private final CountDownLatch trailingSeqResetLatch = new CountDownLatch(1); private final AtomicReference trailingGapSeqReset = new AtomicReference<>(); @@ -366,6 +367,7 @@ public void onLogon(SessionID sessionId) { @Override public void toAdmin(Message message, SessionID sessionId) { + log.info("toAdmin: [{}] {}", sessionId, message); try { if (!MsgType.SEQUENCE_RESET.equals(message.getHeader().getString(MsgType.FIELD))) { return; @@ -384,6 +386,23 @@ public void toAdmin(Message message, SessionID sessionId) { } } + @Override + public void fromAdmin(Message message, SessionID sessionId) + throws FieldNotFound, IncorrectDataFormat, IncorrectTagValue, RejectLogon { + log.info("fromAdmin: [{}] {}", sessionId, message); + } + + @Override + public void toApp(Message message, SessionID sessionId) throws DoNotSend { + log.info("toApp: [{}] {}", sessionId, message); + } + + @Override + public void fromApp(Message message, SessionID sessionId) + throws FieldNotFound, IncorrectDataFormat, IncorrectTagValue, UnsupportedMessageType { + log.info("fromApp: [{}] {}", sessionId, message); + } + void waitForLogon() { try { assertTrue("Acceptor logon timed out", logonLatch.await(10, TimeUnit.SECONDS)); @@ -408,6 +427,8 @@ Message getTrailingGapSeqReset() { /** Plain application adapter used for the initiator side. */ private static class TestConnectorApplication extends ApplicationAdapter { + private final Logger log = LoggerFactory.getLogger(TestConnectorApplication.class); + private final CountDownLatch logonLatch = new CountDownLatch(1); @Override @@ -415,6 +436,28 @@ public void onLogon(SessionID sessionId) { logonLatch.countDown(); } + @Override + public void toAdmin(Message message, SessionID sessionId) { + log.info("toAdmin: [{}] {}", sessionId, message); + } + + @Override + public void fromAdmin(Message message, SessionID sessionId) + throws FieldNotFound, IncorrectDataFormat, IncorrectTagValue, RejectLogon { + log.info("fromAdmin: [{}] {}", sessionId, message); + } + + @Override + public void toApp(Message message, SessionID sessionId) throws DoNotSend { + log.info("toApp: [{}] {}", sessionId, message); + } + + @Override + public void fromApp(Message message, SessionID sessionId) + throws FieldNotFound, IncorrectDataFormat, IncorrectTagValue, UnsupportedMessageType { + log.info("fromApp: [{}] {}", sessionId, message); + } + void waitForLogon() { try { assertTrue("Initiator logon timed out", logonLatch.await(10, TimeUnit.SECONDS)); From 68bbb0f6de652fd995a363eb4a5d96c1b0aeb5c3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 16:57:05 +0000 Subject: [PATCH 3/9] Switch LogFactory to ScreenLogFactory so session events are visible in test output Agent-Logs-Url: https://github.com/quickfix-j/quickfixj/sessions/c3898893-28b5-44c2-b652-a134c6ab2747 Co-authored-by: chrjohn <6644028+chrjohn@users.noreply.github.com> --- .../java/quickfix/ResendMessagesBugDirectConnectionTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java index fad3913cc..7be4f811a 100644 --- a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java +++ b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java @@ -185,7 +185,7 @@ private Acceptor createAcceptor(Application app, int port) throws ConfigError { settings.set(defaults); MessageStoreFactory storeFactory = new BugScenarioStoreFactory(acceptorSessionID); - LogFactory logFactory = new SLF4JLogFactory(new SessionSettings()); + LogFactory logFactory = new ScreenLogFactory(settings); return new SocketAcceptor(app, storeFactory, settings, logFactory, new DefaultMessageFactory()); } @@ -209,7 +209,7 @@ private Initiator createInitiator(Application app, int port) throws ConfigError settings.set(defaults); MessageStoreFactory factory = new MemoryStoreFactory(); - LogFactory logFactory = new SLF4JLogFactory(new SessionSettings()); + LogFactory logFactory = new ScreenLogFactory(settings); return new SocketInitiator(app, factory, settings, logFactory, new DefaultMessageFactory()); } From fb0c0d51ca3e9de26b611e35021ad8f14e2fc68e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Apr 2026 09:44:57 +0000 Subject: [PATCH 4/9] Add HeartBtInt=5, wait for fresh heartbeat, and check logout has no too-low text Agent-Logs-Url: https://github.com/quickfix-j/quickfixj/sessions/a782b3cc-5406-494c-a244-300ae1081f6e Co-authored-by: chrjohn <6644028+chrjohn@users.noreply.github.com> --- ...ResendMessagesBugDirectConnectionTest.java | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java index 7be4f811a..32a80cb05 100644 --- a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java +++ b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java @@ -32,19 +32,24 @@ import quickfix.field.MsgSeqNum; import quickfix.field.MsgType; import quickfix.field.NewSeqNo; +import quickfix.field.PossDupFlag; import quickfix.field.SenderCompID; import quickfix.field.SendingTime; import quickfix.field.TargetCompID; +import quickfix.field.Text; import quickfix.mina.ProtocolFactory; import java.io.IOException; import java.time.LocalDateTime; import java.util.HashMap; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -154,6 +159,12 @@ public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() + "causing the SequenceReset to be silently dropped.", 6, actualMsgSeqNum); + // Wait for a fresh (not resent) heartbeat to confirm the session is fully + // synchronised after the resend. With HeartBtInt=5 this should arrive + // within 15 seconds once the bug is fixed. + assertTrue("Timed out waiting for heartbeat - session may be stuck due to bug #344", + initiatorApp.waitForHeartbeat(15, TimeUnit.SECONDS)); + } finally { if (initiator != null) { try { initiator.stop(); } catch (RuntimeException e) { log.error(e.getMessage(), e); } @@ -162,6 +173,19 @@ public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() try { acceptor.stop(); } catch (RuntimeException e) { log.error(e.getMessage(), e); } } } + + // After the graceful shutdown the only logout messages should be the normal + // session-close ones. A "too low" logout would indicate the bug caused the + // initiator to reject the trailing SequenceReset. + for (Message logout : initiatorApp.getLogoutMessages()) { + if (logout.isSetField(Text.FIELD)) { + final String text = logout.getString(Text.FIELD); + assertFalse( + "Logout should not contain a 'too low' sequence-number message, but got: " + + text, + text.contains("too low")); + } + } } // ----------------------------------------------------------------------- @@ -174,6 +198,7 @@ private Acceptor createAcceptor(Application app, int port) throws ConfigError { defaults.put("ConnectionType", "acceptor"); defaults.put("StartTime", "00:00:00"); defaults.put("EndTime", "00:00:00"); + defaults.put("HeartBtInt", "5"); defaults.put("BeginString", FixVersions.BEGINSTRING_FIX42); defaults.put("NonStopSession", "Y"); // Disable data-dictionary validation so the pre-built stored messages @@ -196,7 +221,7 @@ private Initiator createInitiator(Application app, int port) throws ConfigError defaults.put("ConnectionType", "initiator"); defaults.put("StartTime", "00:00:00"); defaults.put("EndTime", "00:00:00"); - defaults.put("HeartBtInt", "30"); + defaults.put("HeartBtInt", "5"); defaults.put("ReconnectInterval", "2"); defaults.put("NonStopSession", "Y"); // Disable data-dictionary so the minimally-populated ExecutionReports @@ -430,6 +455,8 @@ private static class TestConnectorApplication extends ApplicationAdapter { private final Logger log = LoggerFactory.getLogger(TestConnectorApplication.class); private final CountDownLatch logonLatch = new CountDownLatch(1); + private final CountDownLatch heartbeatLatch = new CountDownLatch(1); + private final List logoutMessages = new CopyOnWriteArrayList<>(); @Override public void onLogon(SessionID sessionId) { @@ -439,12 +466,31 @@ public void onLogon(SessionID sessionId) { @Override public void toAdmin(Message message, SessionID sessionId) { log.info("toAdmin: [{}] {}", sessionId, message); + try { + if (MsgType.LOGOUT.equals(message.getHeader().getString(MsgType.FIELD))) { + logoutMessages.add(message); + } + } catch (FieldNotFound e) { + // ignore + } } @Override public void fromAdmin(Message message, SessionID sessionId) throws FieldNotFound, IncorrectDataFormat, IncorrectTagValue, RejectLogon { log.info("fromAdmin: [{}] {}", sessionId, message); + try { + if (MsgType.HEARTBEAT.equals(message.getHeader().getString(MsgType.FIELD))) { + // Only count down for a fresh heartbeat, not a resent one (PossDupFlag=Y) + final boolean isPossDup = message.getHeader().isSetField(PossDupFlag.FIELD) + && message.getHeader().getBoolean(PossDupFlag.FIELD); + if (!isPossDup) { + heartbeatLatch.countDown(); + } + } + } catch (FieldNotFound e) { + // ignore + } } @Override @@ -465,5 +511,17 @@ void waitForLogon() { fail("Interrupted while waiting for initiator logon"); } } + + boolean waitForHeartbeat(long timeout, TimeUnit unit) { + try { + return heartbeatLatch.await(timeout, unit); + } catch (InterruptedException e) { + return false; + } + } + + List getLogoutMessages() { + return logoutMessages; + } } } From 367788275ff7e2d5e2f3bd4481beca58c538f9e6 Mon Sep 17 00:00:00 2001 From: Christoph John Date: Mon, 27 Apr 2026 11:54:11 +0200 Subject: [PATCH 5/9] reordered assertions --- .../ResendMessagesBugDirectConnectionTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java index 32a80cb05..63c9cefa2 100644 --- a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java +++ b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java @@ -144,6 +144,12 @@ public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() assertTrue("Trailing SequenceReset should advance past the gap (NewSeqNo > 10), got " + newSeqNo, newSeqNo > 10); + // Wait for a fresh (not resent) heartbeat to confirm the session is fully + // synchronised after the resend. With HeartBtInt=5 this should arrive + // within 15 seconds once the bug is fixed. + assertTrue("Timed out waiting for heartbeat - session may be stuck due to bug #344", + initiatorApp.waitForHeartbeat(15, TimeUnit.SECONDS)); + // THE KEY ASSERTION: // MsgSeqNum must equal 6 (the first seqno of the trailing admin block, // i.e. msgSeqNum of last app message + 1 = 4 + 1 = 5, then next is 6 @@ -159,12 +165,6 @@ public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() + "causing the SequenceReset to be silently dropped.", 6, actualMsgSeqNum); - // Wait for a fresh (not resent) heartbeat to confirm the session is fully - // synchronised after the resend. With HeartBtInt=5 this should arrive - // within 15 seconds once the bug is fixed. - assertTrue("Timed out waiting for heartbeat - session may be stuck due to bug #344", - initiatorApp.waitForHeartbeat(15, TimeUnit.SECONDS)); - } finally { if (initiator != null) { try { initiator.stop(); } catch (RuntimeException e) { log.error(e.getMessage(), e); } From f9c5bcbdf0054ebd372168e2b1774d6ec10ec797 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Apr 2026 11:40:33 +0000 Subject: [PATCH 6/9] Fix resendMessages bug: update newBegin past trailing admin block before generateSequenceResetIfNeeded Agent-Logs-Url: https://github.com/quickfix-j/quickfixj/sessions/be2e37cc-175b-4fcb-89ea-6f5c4b7b68b1 Co-authored-by: chrjohn <6644028+chrjohn@users.noreply.github.com> --- quickfixj-core/src/main/java/quickfix/Session.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quickfixj-core/src/main/java/quickfix/Session.java b/quickfixj-core/src/main/java/quickfix/Session.java index 19bede7b8..d26b26a8f 100644 --- a/quickfixj-core/src/main/java/quickfix/Session.java +++ b/quickfixj-core/src/main/java/quickfix/Session.java @@ -2420,7 +2420,7 @@ private void resendMessages(Message receivedMessage, int beginSeqNo, int endSeqN } int newBegin = beginSeqNo; - if (appMessageJustSent) { + if (appMessageJustSent || begin != 0) { newBegin = msgSeqNum + 1; } if (enableNextExpectedMsgSeqNum) { From 6c3b9872dc53cc137087b1bb83bcd3716a1195ea Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 28 Apr 2026 09:21:35 +0000 Subject: [PATCH 7/9] Make test scenario minimal and revert Session.java condition to show bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Simplify BugScenarioStore: NextSenderMsgSeqNum 11→6, remove absent seqnos 6–10 - Update capture threshold in AcceptorApplication.toAdmin from >10 to >6 - Update test sanity-check assertion from newSeqNo>10 to newSeqNo>6 - Revert Session.java condition to `if (appMessageJustSent)` to expose bug #344 Agent-Logs-Url: https://github.com/quickfix-j/quickfixj/sessions/5c8e8307-ada1-4ff4-ac56-49ef2abd8957 Co-authored-by: chrjohn <6644028+chrjohn@users.noreply.github.com> --- .../src/main/java/quickfix/Session.java | 2 +- ...ResendMessagesBugDirectConnectionTest.java | 38 +++++++++---------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/quickfixj-core/src/main/java/quickfix/Session.java b/quickfixj-core/src/main/java/quickfix/Session.java index d26b26a8f..19bede7b8 100644 --- a/quickfixj-core/src/main/java/quickfix/Session.java +++ b/quickfixj-core/src/main/java/quickfix/Session.java @@ -2420,7 +2420,7 @@ private void resendMessages(Message receivedMessage, int beginSeqNo, int endSeqN } int newBegin = beginSeqNo; - if (appMessageJustSent || begin != 0) { + if (appMessageJustSent) { newBegin = msgSeqNum + 1; } if (enableNextExpectedMsgSeqNum) { diff --git a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java index 63c9cefa2..a53beaa38 100644 --- a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java +++ b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java @@ -72,17 +72,16 @@ * seqno 3 – ExecutionReport (app) ← triggers SequenceReset(1→3) gap-fill * seqno 4 – ExecutionReport (app) ← resent * seqno 5 – Heartbeat (admin) ← last retrieved = admin → triggers the bug - * seqnos 6-10 – NOT in store - * NextSenderMsgSeqNum = 11 + * NextSenderMsgSeqNum = 6 * * *

When the initiator (fresh store, NextTargetSeqNum = 1) connects: *

    - *
  1. Acceptor sends Logon(11).
    2. + *
  2. Acceptor sends Logon(6).
    3. *
  3. Initiator detects "too high" and sends ResendRequest(1, 0).
    4. - *
  4. Acceptor calls {@code resendMessages(1, 11)}: + *
  5. Acceptor calls {@code resendMessages(1, 6)}: * sends SequenceReset(1→3), ExecutionReport(3), ExecutionReport(4), - * SequenceReset(5→6), and — due to the bug — SequenceReset(MsgSeqNum=1, NewSeqNo=12).
    6. + * SequenceReset(5→6), and — due to the bug — SequenceReset(MsgSeqNum=1, NewSeqNo=7). *
  6. The correct trailing SequenceReset should have MsgSeqNum=6.
    7. *
* @@ -140,9 +139,9 @@ public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() log.info("Trailing SequenceReset: MsgSeqNum={}, NewSeqNo={}", actualMsgSeqNum, newSeqNo); - // Sanity-check: this IS the trailing-gap SequenceReset (NewSeqNo > 10) - assertTrue("Trailing SequenceReset should advance past the gap (NewSeqNo > 10), got " - + newSeqNo, newSeqNo > 10); + // Sanity-check: this IS the trailing-gap SequenceReset (NewSeqNo > 6) + assertTrue("Trailing SequenceReset should advance past the gap (NewSeqNo > 6), got " + + newSeqNo, newSeqNo > 6); // Wait for a fresh (not resent) heartbeat to confirm the session is fully // synchronised after the resend. With HeartBtInt=5 this should arrive @@ -277,12 +276,11 @@ public MessageStore create(SessionID sessionID) { *
  • seqno 3 – ExecutionReport (app)
    • *
  • seqno 4 – ExecutionReport (app)
    • *
  • seqno 5 – Heartbeat (admin) ← last retrieved = admin → triggers bug
    • - *
  • seqnos 6–10 – absent from store
    • - *
  • NextSenderMsgSeqNum = 11
    • + *
  • NextSenderMsgSeqNum = 6
    • * * *

    Writes for sequence numbers greater than 5 are silently discarded so that - * the Logon sent during session start-up does not fill the artificial gap. + * the Logon sent during session start-up does not overwrite the scenario. */ private static class BugScenarioStore extends MemoryStore { @@ -332,12 +330,10 @@ private void populateBugScenario() throws IOException { setHeader(heartbeat, beginString, sender, target, 5, ts); super.set(5, heartbeat.toString()); - // seqnos 6–10 intentionally absent (simulating messages that were sent - // but are no longer available in the store). - - // NextSenderMsgSeqNum = 11 creates the gap that will be covered by the - // trailing SequenceReset in resendMessages. - setNextSenderMsgSeqNum(11); + // NextSenderMsgSeqNum = 6: the Logon sent at session start-up will use + // seqno 6, creating the gap that resendMessages must cover with a trailing + // SequenceReset. + setNextSenderMsgSeqNum(6); setNextTargetMsgSeqNum(1); } @@ -352,7 +348,7 @@ private static void setHeader(Message msg, String beginString, String sender, /** * Discard any write for sequence numbers beyond the pre-populated range. - * This prevents the Logon sent at start-up (seqno 11) from appearing in + * This prevents the Logon sent at start-up (seqno 6) from appearing in * the store and thereby hiding the gap that triggers the bug. */ @Override @@ -400,9 +396,9 @@ public void toAdmin(Message message, SessionID sessionId) { if (!message.isSetField(GapFillFlag.FIELD) || !message.getBoolean(GapFillFlag.FIELD)) { return; } - // The trailing-gap SequenceReset advances well past the original - // NextSenderMsgSeqNum (11); its NewSeqNo will be 12 in this scenario. - if (message.isSetField(NewSeqNo.FIELD) && message.getInt(NewSeqNo.FIELD) > 10) { + // The trailing-gap SequenceReset advances past the Logon gap; + // its NewSeqNo will be 7 in this scenario. + if (message.isSetField(NewSeqNo.FIELD) && message.getInt(NewSeqNo.FIELD) > 6) { trailingGapSeqReset.set(message); trailingSeqResetLatch.countDown(); } From aa9e7ceee2a087b2900538b81e1a4533926259db Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 28 Apr 2026 12:04:07 +0000 Subject: [PATCH 8/9] Add seqno 6 Heartbeat to store; Logon now at seqno 7 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add second Heartbeat (seqno 6) to BugScenarioStore; LAST_PREPOPULATED_SEQNO 5→6 - setNextSenderMsgSeqNum 6→7 so the startup Logon uses seqno 7 - Update toAdmin capture threshold from >6 to >7 (NewSeqNo=8 is the buggy reset) - Update sanity-check assertion from >6 to >7 - Update assertEquals(6) → assertEquals(7) and related comments - Update all Javadoc to reflect new seqno 6 Heartbeat and seqno 7 Logon Agent-Logs-Url: https://github.com/quickfix-j/quickfixj/sessions/6093151b-7a97-484c-a0b4-5528eaae35e7 Co-authored-by: chrjohn <6644028+chrjohn@users.noreply.github.com> --- ...ResendMessagesBugDirectConnectionTest.java | 72 ++++++++++--------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java index a53beaa38..7a6de5cb7 100644 --- a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java +++ b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java @@ -61,7 +61,7 @@ * the {@code newBegin} variable is not updated before * {@code generateSequenceResetIfNeeded} is called. As a result the method emits a * SequenceReset-GapFill whose {@code MsgSeqNum} equals {@code beginSeqNo} (= 1) instead - * of the correct value (= first seqno of the trailing admin block = 6). The counterparty + * of the correct value (= seqno of the Logon = 7). The counterparty * therefore receives a SequenceReset with a "too low" {@code MsgSeqNum} and silently drops * it, leaving the session in a stuck state. * @@ -71,21 +71,22 @@ * seqno 2 – ResendRequest (admin) * seqno 3 – ExecutionReport (app) ← triggers SequenceReset(1→3) gap-fill * seqno 4 – ExecutionReport (app) ← resent - * seqno 5 – Heartbeat (admin) ← last retrieved = admin → triggers the bug - * NextSenderMsgSeqNum = 6 + * seqno 5 – Heartbeat (admin) + * seqno 6 – Heartbeat (admin) ← last retrieved = admin → triggers the bug + * NextSenderMsgSeqNum = 7 * * *

    When the initiator (fresh store, NextTargetSeqNum = 1) connects: *

      - *
    1. Acceptor sends Logon(6).
      2. + *
    2. Acceptor sends Logon(7).
      3. *
    3. Initiator detects "too high" and sends ResendRequest(1, 0).
      4. - *
    4. Acceptor calls {@code resendMessages(1, 6)}: + *
    5. Acceptor calls {@code resendMessages(1, 7)}: * sends SequenceReset(1→3), ExecutionReport(3), ExecutionReport(4), - * SequenceReset(5→6), and — due to the bug — SequenceReset(MsgSeqNum=1, NewSeqNo=7).
      6. - *
    6. The correct trailing SequenceReset should have MsgSeqNum=6.
      7. + * SequenceReset(5→7), and — due to the bug — SequenceReset(MsgSeqNum=1, NewSeqNo=8). + *
    7. The correct trailing SequenceReset should have MsgSeqNum=7.
      8. *
    * - *

    The test assertion {@code assertEquals(6, actualMsgSeqNum)} fails on current code + *

    The test assertion {@code assertEquals(7, actualMsgSeqNum)} fails on current code * (gets 1) and passes once the bug is fixed. */ public class ResendMessagesBugDirectConnectionTest { @@ -139,9 +140,9 @@ public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() log.info("Trailing SequenceReset: MsgSeqNum={}, NewSeqNo={}", actualMsgSeqNum, newSeqNo); - // Sanity-check: this IS the trailing-gap SequenceReset (NewSeqNo > 6) - assertTrue("Trailing SequenceReset should advance past the gap (NewSeqNo > 6), got " - + newSeqNo, newSeqNo > 6); + // Sanity-check: this IS the trailing-gap SequenceReset (NewSeqNo > 7) + assertTrue("Trailing SequenceReset should advance past the gap (NewSeqNo > 7), got " + + newSeqNo, newSeqNo > 7); // Wait for a fresh (not resent) heartbeat to confirm the session is fully // synchronised after the resend. With HeartBtInt=5 this should arrive @@ -150,19 +151,18 @@ public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() initiatorApp.waitForHeartbeat(15, TimeUnit.SECONDS)); // THE KEY ASSERTION: - // MsgSeqNum must equal 6 (the first seqno of the trailing admin block, - // i.e. msgSeqNum of last app message + 1 = 4 + 1 = 5, then next is 6 - // because seqno 5 is the Heartbeat gap-filled by the previous reset). + // MsgSeqNum must equal 7 (the seqno of the Logon sent at startup, which is + // the trailing admin message that should be gap-filled). // // Due to bug #344 the actual value is 1 (beginSeqNo), which is "too low" - // for the initiator (it has already advanced to seqno 6). + // for the initiator (it has already advanced to seqno 7+). // This assertion FAILS on the current code and PASSES once the bug is fixed. assertEquals( "Bug #344: trailing SequenceReset has MsgSeqNum=" + actualMsgSeqNum - + " but should be 6 (first seqno after the last resent app message). " - + "MsgSeqNum=1 is 'too low' for the initiator (already at seqno 6) " + + " but should be 7 (seqno of the Logon sent at startup). " + + "MsgSeqNum=1 is 'too low' for the initiator " + "causing the SequenceReset to be silently dropped.", - 6, actualMsgSeqNum); + 7, actualMsgSeqNum); } finally { if (initiator != null) { @@ -275,16 +275,17 @@ public MessageStore create(SessionID sessionID) { *

  • seqno 2 – ResendRequest (admin)
    • *
  • seqno 3 – ExecutionReport (app)
    • *
  • seqno 4 – ExecutionReport (app)
    • - *
  • seqno 5 – Heartbeat (admin) ← last retrieved = admin → triggers bug
    • - *
  • NextSenderMsgSeqNum = 6
    • + *
  • seqno 5 – Heartbeat (admin)
    • + *
  • seqno 6 – Heartbeat (admin) ← last retrieved = admin → triggers bug
    • + *
  • NextSenderMsgSeqNum = 7
    • * * - *

    Writes for sequence numbers greater than 5 are silently discarded so that + *

    Writes for sequence numbers greater than 6 are silently discarded so that * the Logon sent during session start-up does not overwrite the scenario. */ private static class BugScenarioStore extends MemoryStore { - private static final int LAST_PREPOPULATED_SEQNO = 5; + private static final int LAST_PREPOPULATED_SEQNO = 6; BugScenarioStore() throws IOException { super(); @@ -324,16 +325,21 @@ private void populateBugScenario() throws IOException { super.set(seqno, execReport.toString()); } - // seqno 5: Heartbeat (admin) — the last stored message is admin: + // seqno 5: Heartbeat (admin) + Message heartbeat5 = factory.create(beginString, MsgType.HEARTBEAT); + setHeader(heartbeat5, beginString, sender, target, 5, ts); + super.set(5, heartbeat5.toString()); + + // seqno 6: Heartbeat (admin) — the last stored message is admin: // this is what triggers the bug when resendMessages processes the list. - Message heartbeat = factory.create(beginString, MsgType.HEARTBEAT); - setHeader(heartbeat, beginString, sender, target, 5, ts); - super.set(5, heartbeat.toString()); + Message heartbeat6 = factory.create(beginString, MsgType.HEARTBEAT); + setHeader(heartbeat6, beginString, sender, target, 6, ts); + super.set(6, heartbeat6.toString()); - // NextSenderMsgSeqNum = 6: the Logon sent at session start-up will use - // seqno 6, creating the gap that resendMessages must cover with a trailing + // NextSenderMsgSeqNum = 7: the Logon sent at session start-up will use + // seqno 7, creating the gap that resendMessages must cover with a trailing // SequenceReset. - setNextSenderMsgSeqNum(6); + setNextSenderMsgSeqNum(7); setNextTargetMsgSeqNum(1); } @@ -348,7 +354,7 @@ private static void setHeader(Message msg, String beginString, String sender, /** * Discard any write for sequence numbers beyond the pre-populated range. - * This prevents the Logon sent at start-up (seqno 6) from appearing in + * This prevents the Logon sent at start-up (seqno 7) from appearing in * the store and thereby hiding the gap that triggers the bug. */ @Override @@ -396,9 +402,9 @@ public void toAdmin(Message message, SessionID sessionId) { if (!message.isSetField(GapFillFlag.FIELD) || !message.getBoolean(GapFillFlag.FIELD)) { return; } - // The trailing-gap SequenceReset advances past the Logon gap; - // its NewSeqNo will be 7 in this scenario. - if (message.isSetField(NewSeqNo.FIELD) && message.getInt(NewSeqNo.FIELD) > 6) { + // The trailing-gap SequenceReset covers the Logon at seqno 7; + // its NewSeqNo will be 8 in this scenario. + if (message.isSetField(NewSeqNo.FIELD) && message.getInt(NewSeqNo.FIELD) > 7) { trailingGapSeqReset.set(message); trailingSeqResetLatch.countDown(); } From 9517aed6a527d3a4ff0a38e6d5d88593579aa466 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 28 Apr 2026 14:58:06 +0000 Subject: [PATCH 9/9] Revert to commit 367788275ff7e2d5e2f3bd4481beca58c538f9e6 (reordered assertions) Agent-Logs-Url: https://github.com/quickfix-j/quickfixj/sessions/c38eb35a-93d2-413b-bf85-4884ea7653fc Co-authored-by: chrjohn <6644028+chrjohn@users.noreply.github.com> --- ...ResendMessagesBugDirectConnectionTest.java | 82 +++++++++---------- 1 file changed, 40 insertions(+), 42 deletions(-) diff --git a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java index 7a6de5cb7..63c9cefa2 100644 --- a/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java +++ b/quickfixj-core/src/test/java/quickfix/ResendMessagesBugDirectConnectionTest.java @@ -61,7 +61,7 @@ * the {@code newBegin} variable is not updated before * {@code generateSequenceResetIfNeeded} is called. As a result the method emits a * SequenceReset-GapFill whose {@code MsgSeqNum} equals {@code beginSeqNo} (= 1) instead - * of the correct value (= seqno of the Logon = 7). The counterparty + * of the correct value (= first seqno of the trailing admin block = 6). The counterparty * therefore receives a SequenceReset with a "too low" {@code MsgSeqNum} and silently drops * it, leaving the session in a stuck state. * @@ -71,22 +71,22 @@ * seqno 2 – ResendRequest (admin) * seqno 3 – ExecutionReport (app) ← triggers SequenceReset(1→3) gap-fill * seqno 4 – ExecutionReport (app) ← resent - * seqno 5 – Heartbeat (admin) - * seqno 6 – Heartbeat (admin) ← last retrieved = admin → triggers the bug - * NextSenderMsgSeqNum = 7 + * seqno 5 – Heartbeat (admin) ← last retrieved = admin → triggers the bug + * seqnos 6-10 – NOT in store + * NextSenderMsgSeqNum = 11 * * *

    When the initiator (fresh store, NextTargetSeqNum = 1) connects: *

      - *
    1. Acceptor sends Logon(7).
      2. + *
    2. Acceptor sends Logon(11).
      3. *
    3. Initiator detects "too high" and sends ResendRequest(1, 0).
      4. - *
    4. Acceptor calls {@code resendMessages(1, 7)}: + *
    5. Acceptor calls {@code resendMessages(1, 11)}: * sends SequenceReset(1→3), ExecutionReport(3), ExecutionReport(4), - * SequenceReset(5→7), and — due to the bug — SequenceReset(MsgSeqNum=1, NewSeqNo=8).
      6. - *
    6. The correct trailing SequenceReset should have MsgSeqNum=7.
      7. + * SequenceReset(5→6), and — due to the bug — SequenceReset(MsgSeqNum=1, NewSeqNo=12). + *
    7. The correct trailing SequenceReset should have MsgSeqNum=6.
      8. *
    * - *

    The test assertion {@code assertEquals(7, actualMsgSeqNum)} fails on current code + *

    The test assertion {@code assertEquals(6, actualMsgSeqNum)} fails on current code * (gets 1) and passes once the bug is fixed. */ public class ResendMessagesBugDirectConnectionTest { @@ -140,9 +140,9 @@ public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() log.info("Trailing SequenceReset: MsgSeqNum={}, NewSeqNo={}", actualMsgSeqNum, newSeqNo); - // Sanity-check: this IS the trailing-gap SequenceReset (NewSeqNo > 7) - assertTrue("Trailing SequenceReset should advance past the gap (NewSeqNo > 7), got " - + newSeqNo, newSeqNo > 7); + // Sanity-check: this IS the trailing-gap SequenceReset (NewSeqNo > 10) + assertTrue("Trailing SequenceReset should advance past the gap (NewSeqNo > 10), got " + + newSeqNo, newSeqNo > 10); // Wait for a fresh (not resent) heartbeat to confirm the session is fully // synchronised after the resend. With HeartBtInt=5 this should arrive @@ -151,18 +151,19 @@ public void testResendMessagesBugCausesTooLowMsgSeqNumInTrailingSequenceReset() initiatorApp.waitForHeartbeat(15, TimeUnit.SECONDS)); // THE KEY ASSERTION: - // MsgSeqNum must equal 7 (the seqno of the Logon sent at startup, which is - // the trailing admin message that should be gap-filled). + // MsgSeqNum must equal 6 (the first seqno of the trailing admin block, + // i.e. msgSeqNum of last app message + 1 = 4 + 1 = 5, then next is 6 + // because seqno 5 is the Heartbeat gap-filled by the previous reset). // // Due to bug #344 the actual value is 1 (beginSeqNo), which is "too low" - // for the initiator (it has already advanced to seqno 7+). + // for the initiator (it has already advanced to seqno 6). // This assertion FAILS on the current code and PASSES once the bug is fixed. assertEquals( "Bug #344: trailing SequenceReset has MsgSeqNum=" + actualMsgSeqNum - + " but should be 7 (seqno of the Logon sent at startup). " - + "MsgSeqNum=1 is 'too low' for the initiator " + + " but should be 6 (first seqno after the last resent app message). " + + "MsgSeqNum=1 is 'too low' for the initiator (already at seqno 6) " + "causing the SequenceReset to be silently dropped.", - 7, actualMsgSeqNum); + 6, actualMsgSeqNum); } finally { if (initiator != null) { @@ -275,17 +276,17 @@ public MessageStore create(SessionID sessionID) { *

  • seqno 2 – ResendRequest (admin)
    • *
  • seqno 3 – ExecutionReport (app)
    • *
  • seqno 4 – ExecutionReport (app)
    • - *
  • seqno 5 – Heartbeat (admin)
    • - *
  • seqno 6 – Heartbeat (admin) ← last retrieved = admin → triggers bug
    • - *
  • NextSenderMsgSeqNum = 7
    • + *
  • seqno 5 – Heartbeat (admin) ← last retrieved = admin → triggers bug
    • + *
  • seqnos 6–10 – absent from store
    • + *
  • NextSenderMsgSeqNum = 11
    • * * - *

    Writes for sequence numbers greater than 6 are silently discarded so that - * the Logon sent during session start-up does not overwrite the scenario. + *

    Writes for sequence numbers greater than 5 are silently discarded so that + * the Logon sent during session start-up does not fill the artificial gap. */ private static class BugScenarioStore extends MemoryStore { - private static final int LAST_PREPOPULATED_SEQNO = 6; + private static final int LAST_PREPOPULATED_SEQNO = 5; BugScenarioStore() throws IOException { super(); @@ -325,21 +326,18 @@ private void populateBugScenario() throws IOException { super.set(seqno, execReport.toString()); } - // seqno 5: Heartbeat (admin) - Message heartbeat5 = factory.create(beginString, MsgType.HEARTBEAT); - setHeader(heartbeat5, beginString, sender, target, 5, ts); - super.set(5, heartbeat5.toString()); - - // seqno 6: Heartbeat (admin) — the last stored message is admin: + // seqno 5: Heartbeat (admin) — the last stored message is admin: // this is what triggers the bug when resendMessages processes the list. - Message heartbeat6 = factory.create(beginString, MsgType.HEARTBEAT); - setHeader(heartbeat6, beginString, sender, target, 6, ts); - super.set(6, heartbeat6.toString()); - - // NextSenderMsgSeqNum = 7: the Logon sent at session start-up will use - // seqno 7, creating the gap that resendMessages must cover with a trailing - // SequenceReset. - setNextSenderMsgSeqNum(7); + Message heartbeat = factory.create(beginString, MsgType.HEARTBEAT); + setHeader(heartbeat, beginString, sender, target, 5, ts); + super.set(5, heartbeat.toString()); + + // seqnos 6–10 intentionally absent (simulating messages that were sent + // but are no longer available in the store). + + // NextSenderMsgSeqNum = 11 creates the gap that will be covered by the + // trailing SequenceReset in resendMessages. + setNextSenderMsgSeqNum(11); setNextTargetMsgSeqNum(1); } @@ -354,7 +352,7 @@ private static void setHeader(Message msg, String beginString, String sender, /** * Discard any write for sequence numbers beyond the pre-populated range. - * This prevents the Logon sent at start-up (seqno 7) from appearing in + * This prevents the Logon sent at start-up (seqno 11) from appearing in * the store and thereby hiding the gap that triggers the bug. */ @Override @@ -402,9 +400,9 @@ public void toAdmin(Message message, SessionID sessionId) { if (!message.isSetField(GapFillFlag.FIELD) || !message.getBoolean(GapFillFlag.FIELD)) { return; } - // The trailing-gap SequenceReset covers the Logon at seqno 7; - // its NewSeqNo will be 8 in this scenario. - if (message.isSetField(NewSeqNo.FIELD) && message.getInt(NewSeqNo.FIELD) > 7) { + // The trailing-gap SequenceReset advances well past the original + // NextSenderMsgSeqNum (11); its NewSeqNo will be 12 in this scenario. + if (message.isSetField(NewSeqNo.FIELD) && message.getInt(NewSeqNo.FIELD) > 10) { trailingGapSeqReset.set(message); trailingSeqResetLatch.countDown(); }