Discarding `PossDup` messages with too low seqnum

[morvael]

Describe the bug
I have an issue with resent messages being dropped by session due to "too low" sequence numbers. I know I can use the global flag to turn off validation of sequence numbers, but not sure if this is safe.

Expected behavior
Messages recieved with PossDupFlag=Y not discarded.

system information:

• OS: Linux
• Java version: JDK21
• QFJ Version: 3.0.1

Additional context

            if(!state.isResetReceived()){
                if (checkTooHigh && isTargetTooHigh(msgSeqNum)) {
                    doTargetTooHigh(msg);
                    return false;
                } else if (checkTooLow && isTargetTooLow(msgSeqNum)) {
                    doTargetTooLow(msg);
                    return false;
                }
            }

Seing how doTargetTooLow returns boolean from validatePossDup if it is one, I believe the code above should be changed to:

            if(!state.isResetReceived()){
                if (checkTooHigh && isTargetTooHigh(msgSeqNum)) {
                    doTargetTooHigh(msg);
                    return false;
                } else if (checkTooLow && isTargetTooLow(msgSeqNum)) {
                    return doTargetTooLow(msg);
                }
            }

alternatively

            if(!state.isResetReceived()){
                if (checkTooHigh && isTargetTooHigh(msgSeqNum)) {
                    doTargetTooHigh(msg);
                    return false;
                } else if (checkTooLow && isTargetTooLow(msgSeqNum) && doTargetTooLow(msg) == false) {
                    return false;
                }
            }

I know it would help in my case, not sure if this is generally a good fix.

[morvael]

This is of course the code from quickfix.Session. And this is the referenced method doTargetTooLow:

    private boolean doTargetTooLow(Message msg) throws FieldNotFound, IOException {
        if (!isPossibleDuplicate(msg)) {
            final int msgSeqNum = msg.getHeader().getInt(MsgSeqNum.FIELD);
            final String text = "MsgSeqNum too low, expecting " + getExpectedTargetNum()
                    + " but received " + msgSeqNum;
            generateLogout(text);
            throw new SessionException(text);
        }
        return validatePossDup(msg);
    }

So the duplicartes I get do not get a log message, and are silently discarded by ignoring the above return value (if true).

[chrjohn]

Hmm, can you elaborate on why you need PossDup messages with too low a seqnum? Normally PossDup messages are sent during resend request processing where the target is too high...

[morvael]

That's how https://gpwwats.pl/en-home rolls 😄 I think the relevant documentation is chapter 6 of https://gpwwats.pl/pub/WATS/Dokumenty%20i%20pliki/v_1_7_10/GPW_WATS_2.02_FIX_Order_Gateway_Specification_v1.7.10.pdf (though I'm doing this for Drop Copy gateway, not Order Gateway, but their low level mechanics are the same and shared).

I'm testing a scenario where as a client I lose local fix database. My client starts with seq nums at 1. My logon is rejected, I need to adjust my message seq numbers (both local and remote) to what they want. Then on second logon I'm accepted but only get new messages. To get old messages I have to send ResendRequest with proper seq nums. And those messages arrive with original seq nums and PossDupFlag=Y. Then the code in Session drops them silently due to the above code. If it would care about result of doTargetTooLow then my problem would not exist.

[morvael]

I am for a long time perplexed by the need to hack around the library to get it work with the above system. Reported this to them many times, but it seems that's how it will ever be. So I'm down to Session.lookupSession, Session.setNextSenderMsgSeqNum, Session.setNextTargetMsgSeqNum, manual creation/sending of ResendRequest and SequenceReset (for different type of recovery), which includes reflection access to Session.initializeHeader, and adjusting Logon message on the fly, adding ResetSeqNumFlag only in very special case.

[chrjohn]

Sounds interesting. I will think about if there is a better alternative and come back to you.

[morvael]

I still think the original intent of boolean doTargetTooLow was to let duplicates pass the "too low" check (and this covers my use case), only its return value got ignored, perhaps someone wanted nice symmetry with void doTargetTooHigh.

[chrjohn]

I still think the original intent of boolean doTargetTooLow was to let duplicates pass the "too low" check

No, definitely not. See the FIX spec https://fixtrading.org/packages/fix-session-layer-test-cases/# (search for "PossDup") in section 4.5.1. condition "e" :

Check to see if MsgSeqNum(34) has already been received. If already received then ignore the message, otherwise accept and process the message.

I thought about this:

To get old messages I have to send ResendRequest with proper seq nums. And those messages arrive with original seq nums and PossDupFlag=Y. Then the code in Session drops them silently due to the above code

Can't you set the expected sequence number on your side to a higher value, so that the received messages do not appear as "too low" but as "too high" (just as it would be on a normal ResendRequest)?
Apart from that I cannot think of any other way.
I know it is hard to deal with vendors which treat FIX in a abnormal way... ;)

[morvael]

IMHO the trick is to differently interpret "Check to see if MsgSeqNum(34) has already been received. If already received then ignore the message, otherwise accept and process the message." - that my sequence numbers are lower than stored value for "current" message stream is not a proof I have received messages from sequence number 1. So if that code would actually bother to check message store for this specific seq num it would find out I don't have it. But currently it only focuses on "highwatermark" number which is wrong. My interpretation doesn't really violate that contract (though it's different than "let all duplicates pass", but the end result is the same).

I guess I'll be forced to fork the library and do my own extra switch that will let the duplicates pass, as deadlines approach 😄

[morvael]

Checking can be achieved by using void get(int startSequence, int endSequence, Collection<String> messages) from MessageStore, though of course a simpler version not bothering to return messages in collection would do as well. So I think the implementation of the rule took a shortcut here and didn't do proper check for PossDup message with seqNum that isn't present in store. And obviously it doing that check would be welcome, since I wouldn't have to check for duplicates at later stage (but it's not a big problem, so I was all for letting duplicates pass).

Of course an extra config switch to enable that behavior would be most welcome and backwards compatible (without changing default behavior).

[morvael]

I'll try to provide my own session factory to be able to use customized session class.

[morvael]

Ugh, it isn't easy that way either, as Session is not really made to be extendable (for SessionFactory had to basically copy all code to be able to replace the created Session class). Everything private.

[morvael]

Cloned repo, made my own branch, made tiny changes to DefaultSessionFactory (protected factory method to be able to change class of new Session that is created), small changes to Session (changed signature of isTargetTooLow to boolean isTargetTooLow(int msgSeqNum, Message msg) throws IOException, FieldNotFound, adjusted the call to it, and made isPossibleDuplicate protected), and built this as my own version (so that I'm 100% sure that my own version is used). That way I can make my own session factory and session class with necessary adjustment:

    @Override
    protected boolean isTargetTooLow(int msgSeqNum, Message msg) throws IOException, FieldNotFound {
        if (super.isTargetTooLow(msgSeqNum, msg)) {
            if (letDuplicatesPass && isPossibleDuplicate(msg)) {
                return false;
            } else {
                return true;
            }
        } else {
            return false;
        }
    }

[chrjohn]

So if that code would actually bother to check message store for this specific seq num it would find out I don't have it. But currently it only focuses on "highwatermark" number which is wrong.

What do you mean by "check message store"? You don't have any incoming messages in the store. The store is for outgoing messages.

The only thing I could think of to make life easier for you would be some kind of handler or callback that is called on reception of a PossDup message.

[morvael]

What do you mean by "check message store"? You don't have any incoming messages in the store. The store is for outgoing messages.

Ah right, sorry, then IMHO the library can't reliably know what was received based only on next expected sequence number 😀 so it can't really fulfill the PossDup check. Isn't it logical that duplicates may arrive with duplicate (previously seen) sequence numbers?

In any case my class hack has been deployed. I only wish it was easier to override parts of DefaultSessionFactory and Session, because currently it's not really possible (apart from copying whole implementation of both, and that may depend on some more private stuff and is harder to maintain).