Skip to content

correctly track begin seqnum when resend range ends with admin messages #1270

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 3 commits into
base: master
Choose a base branch
from
+194 −23
Draft

correctly track begin seqnum when resend range ends with admin messages #1270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7462d8d
fix: track newBegin inside resend loop to handle trailing admin messages
Copilot Jun 29, 2026
45627b5
test: add regression test for issue #344 and fix newBegin after trail…
Copilot Jun 30, 2026
9bbfce4
fix+test: unify post-loop branch and add enableNextExpectedMsgSeqNum …
Copilot Jun 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 5 additions & 23 deletions quickfixj-core/src/main/java/quickfix/Session.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2377,10 +2377,9 @@ private void resendMessages(Message receivedMessage, int beginSeqNo, int endSeqN
int msgSeqNum = 0;
int begin = 0;
int current = beginSeqNo;
boolean appMessageJustSent = false;
int newBegin = beginSeqNo;

for (final String message : messages) {
appMessageJustSent = false;
final Message msg;
try {
// QFJ-626
Expand Down Expand Up @@ -2418,7 +2417,7 @@ private void resendMessages(Message receivedMessage, int beginSeqNo, int endSeqN
return;
}
begin = 0;
appMessageJustSent = true;
newBegin = msgSeqNum + 1;
} else {
if (begin == 0) {
begin = msgSeqNum;
Expand All @@ -2427,28 +2426,11 @@ private void resendMessages(Message receivedMessage, int beginSeqNo, int endSeqN
}
current = msgSeqNum + 1;
}

int newBegin = beginSeqNo;
if (appMessageJustSent) {
if (begin != 0) {
generateSequenceReset(receivedMessage, begin, msgSeqNum + 1);
newBegin = msgSeqNum + 1;
}
if (enableNextExpectedMsgSeqNum) {
if (begin != 0) {
generateSequenceReset(receivedMessage, begin, msgSeqNum + 1);
} else {
/*
* I've added an else here as I managed to fail this without it in a unit test, however the unit test data
* may not have been realistic to production on the other hand.
* Apart from the else
*/
generateSequenceResetIfNeeded(receivedMessage, newBegin, endSeqNo, msgSeqNum);
}
} else {
if (begin != 0) {
generateSequenceReset(receivedMessage, begin, msgSeqNum + 1);
}
generateSequenceResetIfNeeded(receivedMessage, newBegin, endSeqNo, msgSeqNum);
}
generateSequenceResetIfNeeded(receivedMessage, newBegin, endSeqNo, msgSeqNum);
}

private void generateSequenceResetIfNeeded(Message receivedMessage, int beginSeqNo, int endSeqNo, int msgSeqNum)
Expand Down
189 changes: 189 additions & 0 deletions quickfixj-core/src/test/java/quickfix/SessionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2338,6 +2338,195 @@ public void correct_sequence_number_for_last_gap_fill_if_next_sender_sequence_nu
assertEquals(lastGapFill.getHeader().getString(MsgSeqNum.FIELD), "6");
}

/**
* Regression test for GitHub issue #344 ({@code enableNextExpectedMsgSeqNum=false}).
*
* <p>Bug: When {@code resendMessages} processes a range where the last retrieved
* persisted message is an <em>admin</em> message (e.g. a Heartbeat at seqno 5),
* {@code appMessageJustSent} is {@code false} at the end of the loop.
* Therefore {@code newBegin} is never updated from its initial value of
* {@code beginSeqNo} (= 1 in this scenario) and the subsequent call to
* {@code generateSequenceResetIfNeeded} emits a SequenceReset-GapFill with
* {@code MsgSeqNum=1} instead of the correct {@code MsgSeqNum=6}.
*
* <p>The test asserts the <em>correct</em> value (6) and therefore
* <strong>fails on the current buggy code</strong>, serving as a
* regression test once the bug is fixed.
*
* <p>Scenario:
* <ul>
* <li>Messages 1 (Logon) and 2 (ResendRequest) are admin – present in store</li>
* <li>Messages 3 and 4 are application (ExecutionReport) – present in store</li>
* <li>Message 5 is admin (Heartbeat) – present in store</li>
* <li>Messages 6-10 are admin – NOT in store (seqnum incremented only)</li>
* <li>{@code NextSenderMsgSeqNum = 11}</li>
* </ul>
*
* <p>Expected outgoing messages when the counterparty's ResendRequest arrives:
* <ol>
* <li>SequenceReset(MsgSeqNum=1, NewSeqNo=3) – gap-fill for admin 1-2</li>
* <li>ExecutionReport(3) resent (PossDupFlag)</li>
* <li>ExecutionReport(4) resent (PossDupFlag)</li>
* <li>SequenceReset(MsgSeqNum=5, NewSeqNo=6) – gap-fill for admin Heartbeat at 5</li>
* <li>SequenceReset(MsgSeqNum=<b>6</b>, NewSeqNo=11) – gap-fill for admin 6-10 (BUG gives 1)</li>
* </ol>
*/
@Test
public void resendMessages_whenLastRetrievedMessageIsAdmin_finalSequenceResetShouldNotOverlapResentAppMessages()
throws IOException, InvalidMessage, FieldNotFound, RejectLogon, UnsupportedMessageType,
IncorrectTagValue, IncorrectDataFormat {
doResendMessages_whenLastRetrievedMessageIsAdmin_scenario(false);
}

/**
* Regression test for GitHub issue #344 ({@code enableNextExpectedMsgSeqNum=true}).
*
* <p>Same bug as the non-{@code enableNextExpectedMsgSeqNum} variant: when
* {@code resendMessages} ends on an <em>admin</em> message, {@code newBegin} is
* not advanced. With {@code enableNextExpectedMsgSeqNum=true} the post-loop
* code takes a different branch, so the trailing gap-fill SequenceReset is
* omitted entirely rather than being emitted with the wrong sequence number.
*
* <p>The test asserts the <em>correct</em> output (7 messages total, last one
* being SequenceReset(MsgSeqNum=6, NewSeqNo=11)) and therefore
* <strong>fails on the current buggy code</strong>.
*
* <p>Scenario (with {@code enableNextExpectedMsgSeqNum=true}, no explicit
* ResendRequest is sent by the session at logon time, so app messages start
* one seqno earlier than in the non-{@code enableNextExpectedMsgSeqNum} variant):
* <ul>
* <li>Message 1 (Logon) is admin – present in store</li>
* <li>Messages 2, 3 and 4 are application (ExecutionReport) – present in store</li>
* <li>Message 5 is admin (Heartbeat) – present in store</li>
* <li>Messages 6-10 are admin – NOT in store (seqnum incremented only)</li>
* <li>{@code NextSenderMsgSeqNum = 11}</li>
* </ul>
*
* <p>Expected outgoing messages when the counterparty's ResendRequest arrives:
* <ol>
* <li>SequenceReset(MsgSeqNum=1, NewSeqNo=2) – gap-fill for admin 1</li>
* <li>ExecutionReport(2) resent (PossDupFlag)</li>
* <li>ExecutionReport(3) resent (PossDupFlag)</li>
* <li>ExecutionReport(4) resent (PossDupFlag)</li>
* <li>SequenceReset(MsgSeqNum=5, NewSeqNo=6) – gap-fill for admin Heartbeat at 5</li>
* <li>SequenceReset(MsgSeqNum=<b>6</b>, NewSeqNo=11) – gap-fill for admin 6-10 (BUG: omitted)</li>
* </ol>
*/
@Test
public void resendMessages_whenLastRetrievedMessageIsAdmin_finalSequenceResetShouldNotOverlapResentAppMessages_withEnableNextExpectedMsgSeqNum()
throws IOException, InvalidMessage, FieldNotFound, RejectLogon, UnsupportedMessageType,
IncorrectTagValue, IncorrectDataFormat {
doResendMessages_whenLastRetrievedMessageIsAdmin_scenario(true);
}

/**
* Shared scenario driver for the two issue #344 regression tests.
*
* <p>With {@code enableNextExpectedMsgSeqNum=false} the session sends an explicit
* ResendRequest (seqno 2) after receiving the counterparty's too-high Logon, so
* the application messages are stored at seqnos 3 and 4.
* With {@code enableNextExpectedMsgSeqNum=true} the resend is implicit via tag 789
* in the outgoing Logon and no ResendRequest is transmitted, so the application
* messages start one seqno earlier (seqnos 2, 3 and 4).
* In both cases the Heartbeat ends up at seqno 5 and
* {@code NextSenderMsgSeqNum} ends at 11, giving 7 total outgoing messages.
*/
private void doResendMessages_whenLastRetrievedMessageIsAdmin_scenario(boolean enableNextExpectedMsgSeqNum)
throws IOException, InvalidMessage, FieldNotFound, RejectLogon, UnsupportedMessageType,
IncorrectTagValue, IncorrectDataFormat {
final SessionID sessionID = new SessionID(FixVersions.BEGINSTRING_FIX44, "SENDER", "TARGET");
final boolean resetOnLogon = false;
final boolean validateSequenceNumbers = true;

Session session = new Session(new UnitTestApplication(), new MemoryStoreFactory(), new InMemoryMessageQueueFactory(),
sessionID, null, null, null, null,
new DefaultMessageFactory(), 30, false, 30, UtcTimestampPrecision.MILLIS, resetOnLogon,
false, false, false, false, false, true, false, 1.5, null, validateSequenceNumbers,
new int[]{5}, false, false, false, false, true, false, true, false, null, true, 0,
enableNextExpectedMsgSeqNum, false, true, new ArrayList<>(), Session.DEFAULT_HEARTBEAT_TIMEOUT_MULTIPLIER, false);

Responder mockResponder = mock(Responder.class);
when(mockResponder.send(anyString())).thenReturn(true);
session.setResponder(mockResponder);

// Send Logon (seqno 1); NextSenderMsgSeqNum becomes 2.
session.logon();
session.next();

ArgumentCaptor<String> messageCaptor = ArgumentCaptor.forClass(String.class);
verify(mockResponder).send(messageCaptor.capture());

// Receive Logon from counterparty with seqno 101 (too high).
// With enableNextExpectedMsgSeqNum=false: session sends an explicit ResendRequest
// (seqno 2); NextSenderMsgSeqNum becomes 3.
// With enableNextExpectedMsgSeqNum=true: resend is implicit via tag 789; no
// ResendRequest is transmitted; NextSenderMsgSeqNum stays at 2.
session.next(createLogonResponse(sessionID, new Message(messageCaptor.getValue()), 101));
MessageStore messageStore = session.getStore();

// Store application messages (ExecutionReports).
// The start seqno is read from the store so it adapts to whether a ResendRequest
// was sent (false → starts at 3; true → starts at 2). The loop always stops at 4.
for (int i = messageStore.getNextSenderMsgSeqNum(); i <= 4; i++) {
String executionReportString = "8=FIX.4.2\0019=0246\00135=8\001115=THEM\00134=" + i + "\00143=Y\001122=20100908-17:52:37.920\00149=THEM\00156=US\001369=178\00152=20100908-17:59:30.642\00137=10118506\00111=a00000052.1\00117=17537743\00120=0\001150=4\00139=4\00155=ETFC\00154=1\00138=500000\00144=0.998\00132=0\00131=0\001151=0\00114=0\0016=0\00160=20100908-17:52:37.920\00110=80\001";
messageStore.set(i, executionReportString);
messageStore.incrNextSenderMsgSeqNum();
}

// Store an admin (Heartbeat) message at seqno 5 – the last retrieved message.
// This is the key ingredient for bug #344: when the resendMessages loop ends on an
// admin message, appMessageJustSent is false, so newBegin is never updated from
// its initial value of beginSeqNo, causing the final SequenceReset to be wrong.
Heartbeat heartbeat = new Heartbeat();
heartbeat.getHeader().setString(SenderCompID.FIELD, "THEM");
heartbeat.getHeader().setString(TargetCompID.FIELD, "US");
heartbeat.getHeader().setInt(MsgSeqNum.FIELD, 5);
heartbeat.getHeader().setUtcTimeStamp(SendingTime.FIELD, LocalDateTime.now(ZoneOffset.UTC));
messageStore.set(5, heartbeat.toString());
messageStore.incrNextSenderMsgSeqNum(); // NextSenderMsgSeqNum = 6

// Simulate admin messages 6-10 that were sent but not persisted.
for (int i = 0; i < 5; i++) {
messageStore.incrNextSenderMsgSeqNum(); // ends at 11
}

// Counterparty sends a ResendRequest for all our messages.
// The session will call resendMessages(1, 10).
final Message resendRequest = createResendRequest(1, 1);
session.next(resendRequest);

// Seven messages should have been sent in total.
// With enableNextExpectedMsgSeqNum=false:
// 1. Logon (seqno 1)
// 2. ResendRequest (seqno 2, our request to counterparty)
// 3. SequenceReset(MsgSeqNum=1, NewSeqNo=3) – gap-fill for admin 1-2
// 4. ExecutionReport(3) resent
// 5. ExecutionReport(4) resent
// 6. SequenceReset(MsgSeqNum=5, NewSeqNo=6) – gap-fill for Heartbeat at 5
// 7. SequenceReset(MsgSeqNum=?, NewSeqNo=11) – trailing gap 6-10 (BUG: MsgSeqNum=1)
// With enableNextExpectedMsgSeqNum=true:
// 1. Logon (seqno 1)
// 2. SequenceReset(MsgSeqNum=1, NewSeqNo=2) – gap-fill for admin 1
// 3. ExecutionReport(2) resent
// 4. ExecutionReport(3) resent
// 5. ExecutionReport(4) resent
// 6. SequenceReset(MsgSeqNum=5, NewSeqNo=6) – gap-fill for Heartbeat at 5
// 7. SequenceReset(MsgSeqNum=6, NewSeqNo=11) – trailing gap 6-10 (BUG: omitted)
verify(mockResponder, times(7)).send(messageCaptor.capture());

Message lastSent = new Message(messageCaptor.getAllValues().get(messageCaptor.getAllValues().size() - 1));
assertEquals("Last message should be a SequenceReset-GapFill",
MsgType.SEQUENCE_RESET, lastSent.getHeader().getString(MsgType.FIELD));
// Bug #344: current code gives MsgSeqNum=1 (beginSeqNo) instead of 6 (last admin seqno + 1)
// for the enableNextExpectedMsgSeqNum=false case, and omits the trailing reset entirely
// for the enableNextExpectedMsgSeqNum=true case (causing times(7) above to fail first).
assertEquals("SequenceReset covering trailing gap should begin at seqno 6 (just after the admin "
+ "Heartbeat at 5), not at beginSeqNo=1",
"6", lastSent.getHeader().getString(MsgSeqNum.FIELD));
assertEquals("SequenceReset NewSeqNo should advance to NextSenderMsgSeqNum=11",
"11", lastSent.getString(NewSeqNo.FIELD));
}

@Test
// QFJ-795
public void testMsgSeqNumTooHighWithDisconnectOnError() throws Exception {
Expand Down
Loading