test: regression test 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 a single failing regression test to SessionTest that isolates the scenario:

• 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]>), serving as a regression guard for a future fix.

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: The bug of generateSequenceResetIfNeeded in resendMessages method #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.