test: regression tests for newBegin bug in Session.resendMessages (issue #344)

[Copilot]

In Session.resendMessages, when the last retrieved persisted message is an admin message, appMessageJustSent is false at loop end, so newBegin is never updated from beginSeqNo. The subsequent generateSequenceResetIfNeeded call emits a SequenceReset-GapFill with a MsgSeqNum that is too low — overlapping application messages already resent to the counterparty.

Change

Adds two failing regression tests to SessionTest (sharing a common private helper) that isolate the scenario with and without enableNextExpectedMsgSeqNum:

Scenario (enableNextExpectedMsgSeqNum=false):

• Seqno 1–2: admin (Logon, ResendRequest) — persisted
• Seqno 3–4: app (ExecutionReport) — persisted, will be resent
• Seqno 5: admin (Heartbeat) — persisted, last retrieved message
• Seqno 6–10: admin — not persisted, NextSenderMsgSeqNum = 11

When the counterparty sends ResendRequest(BeginSeqNo=1), the expected final SequenceReset is:

SequenceReset(MsgSeqNum=6, NewSeqNo=11)   ← correct: covers gap 6–10

Current code produces:

SequenceReset(MsgSeqNum=1, NewSeqNo=11)   ← bug: beginSeqNo used instead of lastAppSeqNo+1

The test asserts MsgSeqNum=6 and intentionally fails on the current code (expected:<[6]> but was:<[1]>).

Scenario (enableNextExpectedMsgSeqNum=true):

With this mode, the session does not send an explicit ResendRequest at logon time (the resend is implicit via tag 789), so app messages occupy seqnos 2–4 instead of 3–4. The bug manifests differently: the post-loop branch in resendMessages skips generateSequenceResetIfNeeded entirely, omitting the trailing gap-fill SequenceReset. The test expects 7 outgoing messages (the last being SequenceReset(MsgSeqNum=6, NewSeqNo=11)) and intentionally fails on the current code with only 6 messages sent.

The root cause — newBegin should be updated inside the loop each time an app message is successfully resent, not only after the loop based on the last message type — is left for a separate fix PR.

Original prompt

Create a PR against master in repository quickfix-j/quickfixj that adds a unit test which reproduces issue #344 (bug in Session.resendMessages / generateSequenceResetIfNeeded involving newBegin).

Requirements:

• Do not fix production code behavior. Only add a test that demonstrates the problematic behavior.
• The test should set up a resend range like beginSeqNo=2, endSeqNo=10 (or similar) where the message store returns only a subset (e.g., 2..6) and within those:
  • message 5 is an application message that is successfully resent (so the counterparty would advance expected sender seqnum),
  • message 6 is an admin message, which causes appMessageJustSent to be false at loop end and newBegin to remain beginSeqNo.
• Assert that the final SequenceReset generated by generateSequenceResetIfNeeded uses an incorrect begin sequence number (too low, e.g. begins at 2 instead of 6). The test should fail on current behavior to highlight the bug.
• Prefer asserting on the actual outgoing raw messages captured by a test responder / in-memory session transport (e.g., capture sendRaw / outgoing queue) rather than calling private methods directly.
• Keep the test deterministic and isolated (in-memory message store or stubbed SessionState/store).
• Add the test under the appropriate module (likely quickfixj-core) using the existing test framework already used in the repo.

Context:

• Suspected buggy area in quickfixj-core/src/main/java/quickfix/Session.java around the computation of newBegin after the resend loop and call to generateSequenceResetIfNeeded.
• Related code: resendMessages(...) and generateSequenceResetIfNeeded(...).
• Issue reference: SequenceReset might use wrong MsgSeqNum, message will be ignored by counterparty #344

Deliverables:

• One new unit test (and any minimal test fixtures) that reproduces the behavior.
• PR description should explain the scenario and that the test currently fails, serving as a regression test for a future fix.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Could you please help me understand this issue? Check the position of the possible bug in the screenshot

Assistant: Now I have a clear picture of the full resendMessages method. Let me put it all together to explain the bug.

Understanding the Bug in resendMessages / generateSequenceResetIfNeeded

Here's the relevant section of the code at the end of resendMessages, which is where the bug lives:

int newBegin = beginSeqNo;          // ← initialized to the original start of the range
if (appMessageJustSent) {
    newBegin = msgSeqNum + 1;       // ← updated ONLY if the LAST message was an app message
}
if (enableNextExpectedMsgSeqNum) {
    if (begin != 0) {
        generateSequenceReset(receivedMessage, begin, msgSeqNum + 1);
    } else {
        generateSequenceResetIfNeeded(receivedMessage, newBegin, endSeqNo, msgSeqNum);
    }
} else {
    if (begin != 0) {
        generateSequenceReset(receivedMessage, begin, msgSeqNum + 1);
    }
    generateSequenceResetIfNeeded(receivedMessage, newBegin, endSeqNo, msgSeqNum);
}

---

The Bug — Step by Step

Consider resending messages 2 to 10, but persistence only has messages 2 to 6:

SeqNo	Type	Action
2	App	Resent ✅ → appMessageJustSent = true, begin = 0
3	App	Resent ✅ → appMessageJustSent = true, begin = 0
4	App	Resent ✅ → appMessageJustSent = true, begin = 0
5	App	Resent ✅ → appMessageJustSent = true, begin = 0
6	Admin	Skipped → begin = 6, appMessageJustSent = false

After the loop:

• appMessageJustSent = false (because the last message processed was an admin message at seqno 6)
• msgSeqNum = 6 (last message in persistence)
• begin = 6 (set when admin message was encountered)

Because appMessageJustSent is false, newBegin stays at its initial value: beginSeqNo = 2.

Then generateSequenceResetIfNeeded is called with beginSeqNo = 2, sending a SequenceReset (gap-fill) FROM 2 TO 11.

But the counterparty already processed app messages 2–5, and is now expecting seqno 6. A SequenceReset from seqno 2 will be rejected as "sequence number too low"!

---

The Root Cause

The newBegin variable is only updated after the loop based on whether the very last message was an app message. It doesn't track the "beginning of the trailing gap" correctly when the sequence ends with one or more admin messages after the last app message.

// Current (buggy): newBegin only updates if LAST msg was app
int newBegin = beginSeqNo;
if (appMessageJustSent) {        // false because last msg was admin!
    newBegin = msgSeqNum + 1;
}

---

###...

This pull request was created from Copilot chat.

[chrjohn]

@copilot please add another unit test that basically does the same as your newly added test, but make the session use "enableNextExpectedMsgSeqNum=true".
Re-use as much code as possible in the two tests but make distinctions where necessary.
Only add the test, do not fix the problem.

[Copilot]

@copilot please add another unit test that basically does the same as your newly added test, but make the session use "enableNextExpectedMsgSeqNum=true".
Re-use as much code as poss...

Done. The existing test has been refactored to delegate to a shared private helper (doResendMessages_whenLastRetrievedMessageIsAdmin_scenario), and the new enableNextExpectedMsgSeqNum=true variant calls the same helper with true.

Both tests intentionally fail on the current code, but for different reasons:

• enableNextExpectedMsgSeqNum=false: 7 messages are sent but the final SequenceReset has MsgSeqNum=1 instead of 6 → fails on the content assertion.
• enableNextExpectedMsgSeqNum=true: the post-loop branch (if (begin != 0)) is taken but its else (which calls generateSequenceResetIfNeeded) is skipped, so the trailing gap SequenceReset is omitted entirely — only 6 messages are sent → fails on verify(mockResponder, times(7)).