[FIXED] Use correct sequence on duplicate message with no interest #5818

MauriceVanVeen · 2024-08-22T18:57:08Z

A duplicate message inserted into an Interest-based stream with no interest would return 0 as sequence instead of the sequence of the initial message.

$ nats req interest '' -H 'Nats-Msg-Id: dedupe'
20:31:43 Sending request on "interest"
20:31:43 Received with rtt 187.55µs
{"stream":"INTEREST", "seq":6}

$ nats req interest '' -H 'Nats-Msg-Id: dedupe'
20:31:43 Sending request on "interest"
20:31:43 Received with rtt 101.95µs
{"stream":"INTEREST", "seq":0,"duplicate": true}
# ---- seq should be 6 -----^

Signed-off-by: Maurice van Veen [email protected]

Signed-off-by: Maurice van Veen <[email protected]>

derekcollison

Actually seq 0 signifies from the server that the message was not stored. So this would be a behavioral departure from the initial design.

Also note the the err would be set in that pub ack as well.

MauriceVanVeen · 2024-08-23T08:22:40Z

Also note the the err would be set in that pub ack as well.

Not sure if I understand this, in the PubAck response there is no error message? Just {"stream":"INTEREST", "seq":0,"duplicate": true} with duplicate: true and seq: 0.

Actually seq 0 signifies from the server that the message was not stored. So this would be a behavioral departure from the initial design.

Ah, good to know. In that case there is already behavioral difference. For example a Limits-based stream does report the 'correct' sequence (depending on definitions):

$ nats req limits '' -H 'Nats-Msg-Id: dedupe'
09:37:32 Sending request on "limits"
09:37:32 Received with rtt 162.995µs
{"stream":"LIMITS", "seq":1}

$ nats req limits '' -H 'Nats-Msg-Id: dedupe'
09:37:35 Sending request on "limits"
09:37:35 Received with rtt 144.56µs
{"stream":"LIMITS", "seq":1,"duplicate": true}

There it does contain the duplicate: true, but also seq: 1 where the non-duplicate message was stored.

This PR is actually related to an issue reported by Marco and Antithesis. The second test fails: 8f12b9e

So it seems that for a duplicate message the seq always matches the message that did get persisted, except for these two cases:

interest-based stream with no interest and the message is a duplicate (which this PR would fix)
the issue reported by Marco/Antithesis and the failing test above, where two publishes with the same Msg-Id happen at the same time and it returns seq: 0 simply because the message is not propagated through the replicas yet at the time when the second message comes in.

What would be a good way forward?

Reserving seq: 0 for duplicate: true. In which case this PR could be closed, seq should not be saved in the duplicates map and the reported issue by Marco/Antithesis is actually intended behaviour.
Or, fix it

I'm leaning toward reserving seq: 0 for duplicate messages. Since in the replicated stream scenario, you simply can't be sure what the sequence will be before all is propagated and persisted without blocking on the second publish and waiting for those to go through to return the 'correct' sequence instead of just seq:0,duplicate:true.

ripienaar · 2024-08-23T09:06:36Z

Also note the the err would be set in that pub ack as well.

Not sure if I understand this, in the PubAck response there is no error message? Just {"stream":"INTEREST", "seq":0,"duplicate": true} with duplicate: true and seq: 0.

the response going back is JSPubAckResponse

nats-server/server/stream.go

Lines 118 to 121 in 2cae85d

    
           type JSPubAckResponse struct { 
        
           	Error *ApiError `json:"error,omitempty"` 
        
           	*PubAck 
        
           }

MauriceVanVeen · 2024-08-23T10:13:09Z

the response going back is JSPubAckResponse

Yeah I've seen that, but Error is not set in this case and not returned in the request, that's what confused me.

ripienaar · 2024-08-23T10:16:36Z

the response going back is JSPubAckResponse

Yeah I've seen that, but Error is not set in this case and not returned in the request, that's what confused me.

Ah, indeed if its 0 there should be an error

derekcollison · 2024-08-23T13:58:36Z

If we already return persisted seq and this would make that consistent I am supportive.

derekcollison · 2024-08-23T16:37:22Z

So @ripienaar you agree we should merge this and make the behavior consistent, so on dupe detection we return duplicate: true but seq > 0 and matches the original?

wallyqs · 2024-08-23T16:58:03Z

server/jetstream_test.go

@@ -23925,3 +23925,32 @@ func TestJetStreamConsumerStartSequenceNotInStream(t *testing.T) {
 		require_Equal(t, consumer.asflr, 10)
 	}()
 }
+
+func TestInterestStreamWithDuplicateMessages(t *testing.T) {


Is the test name ok? I think it has to be TestJetStreamInterestStreamWithDuplicateMessages for it to run

https://github.com/nats-io/nats-server/blob/main/scripts/runTestsOnTravis.sh#L41

Corrected this and another case.

Signed-off-by: Maurice van Veen <[email protected]>

derekcollison

LGTM

A duplicate message inserted into an Interest-based stream with no interest would return 0 as sequence instead of the sequence of the initial message. ```sh $ nats req interest '' -H 'Nats-Msg-Id: dedupe' 20:31:43 Sending request on "interest" 20:31:43 Received with rtt 187.55µs {"stream":"INTEREST", "seq":6} $ nats req interest '' -H 'Nats-Msg-Id: dedupe' 20:31:43 Sending request on "interest" 20:31:43 Received with rtt 101.95µs {"stream":"INTEREST", "seq":0,"duplicate": true} # ---- seq should be 6 -----^ ``` Signed-off-by: Maurice van Veen <[email protected]> --------- Signed-off-by: Maurice van Veen <[email protected]>

Includes: - #5782 - #5785 - #5757 - #5789 - #5790 - #5793 - #5794 - #5809 - #5812 - #5816 - #5819 - #5818

@wallyqs

Test should've been prefixed with `TestJetStream`. As noted by @wallyqs: #5818 (comment) Signed-off-by: Maurice van Veen <[email protected]>

A duplicate message inserted into an Interest-based stream with no interest would return 0 as sequence instead of the sequence of the initial message. ```sh $ nats req interest '' -H 'Nats-Msg-Id: dedupe' 20:31:43 Sending request on "interest" 20:31:43 Received with rtt 187.55µs {"stream":"INTEREST", "seq":6} $ nats req interest '' -H 'Nats-Msg-Id: dedupe' 20:31:43 Sending request on "interest" 20:31:43 Received with rtt 101.95µs {"stream":"INTEREST", "seq":0,"duplicate": true} # ---- seq should be 6 -----^ ``` Signed-off-by: Maurice van Veen <[email protected]> --------- Signed-off-by: Maurice van Veen <[email protected]>

Use correct sequence on duplicate message with no interest

716077d

Signed-off-by: Maurice van Veen <[email protected]>

MauriceVanVeen requested a review from a team as a code owner August 22, 2024 18:57

derekcollison reviewed Aug 23, 2024

View reviewed changes

wallyqs reviewed Aug 23, 2024

View reviewed changes

Correct test names

919401e

Signed-off-by: Maurice van Veen <[email protected]>

derekcollison self-requested a review August 23, 2024 17:54

derekcollison approved these changes Aug 23, 2024

View reviewed changes

derekcollison merged commit d62065c into main Aug 23, 2024
5 checks passed

derekcollison deleted the fix/correct-seq-on-no-interest-duplicate-message branch August 23, 2024 18:00

wallyqs mentioned this pull request Aug 23, 2024

Cherry picks for v2.10.19 RC5 #5823

Merged

wallyqs changed the title ~~Use correct sequence on duplicate message with no interest~~ [FIXED] Use correct sequence on duplicate message with no interest Aug 23, 2024

wallyqs added a commit that referenced this pull request Aug 23, 2024

Cherry picks for v2.10.19 RC5 (#5823)

26c8196

Includes: - #5782 - #5785 - #5757 - #5789 - #5790 - #5793 - #5794 - #5809 - #5812 - #5816 - #5819 - #5818

MauriceVanVeen mentioned this pull request Aug 23, 2024

[FIXED] Correct test name to TestJetStreamInterestStreamConsumerFilterEdit #5825

Merged

derekcollison added a commit that referenced this pull request Aug 23, 2024

Correct test name (#5825)

9afb8f4

Test should've been prefixed with `TestJetStream`. As noted by @wallyqs: #5818 (comment) Signed-off-by: Maurice van Veen <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXED] Use correct sequence on duplicate message with no interest #5818

[FIXED] Use correct sequence on duplicate message with no interest #5818

MauriceVanVeen commented Aug 22, 2024

derekcollison left a comment

MauriceVanVeen commented Aug 23, 2024

ripienaar commented Aug 23, 2024 •

edited

Loading

MauriceVanVeen commented Aug 23, 2024

ripienaar commented Aug 23, 2024

derekcollison commented Aug 23, 2024

derekcollison commented Aug 23, 2024

wallyqs Aug 23, 2024

wallyqs Aug 23, 2024

MauriceVanVeen Aug 23, 2024

derekcollison left a comment

[FIXED] Use correct sequence on duplicate message with no interest #5818

[FIXED] Use correct sequence on duplicate message with no interest #5818

Conversation

MauriceVanVeen commented Aug 22, 2024

derekcollison left a comment

Choose a reason for hiding this comment

MauriceVanVeen commented Aug 23, 2024

ripienaar commented Aug 23, 2024 • edited Loading

MauriceVanVeen commented Aug 23, 2024

ripienaar commented Aug 23, 2024

derekcollison commented Aug 23, 2024

derekcollison commented Aug 23, 2024

wallyqs Aug 23, 2024

Choose a reason for hiding this comment

wallyqs Aug 23, 2024

Choose a reason for hiding this comment

MauriceVanVeen Aug 23, 2024

Choose a reason for hiding this comment

derekcollison left a comment

Choose a reason for hiding this comment

ripienaar commented Aug 23, 2024 •

edited

Loading