[FIXED] JetStream: decDeliveryCount underflow treated messages as exhausting MaxDeliver

Project: nats-io/nats-server | Language: Go | Concepts: data-structures, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/nats-io/nats-server/pull/8512

The problem

A bug in the JetStream consumer's delivery count tracking allowed the delivery count to underflow and wrap to a huge value (2^64-1) when attempting to decrement the count for a message that had never been redelivered. This caused some messages to be immediately treated as having exhausted their maximum delivery attempts (MaxDeliver), resulting in them being dropped and generating incorrect advisories. Additionally, the code could leave behind unnecessary state for messages that should not have any redelivery tracking.

Why it's non-trivial

A naive fix might simply guard against negative values, but would miss that the presence of a key in the redelivery count map (rdc) is meaningful: some code checks for the existence of the key, not just its value. Furthermore, the bug wasn't just about preventing underflow—it was about ensuring the delivery count tracking is symmetric and doesn't fabricate or leak state for messages that were never redelivered.

The approach

Guard against missing map and entry, and clean up state properly:

func (o *consumer) decDeliveryCount(sseq uint64) {
-	if o.rdc == nil {
-		o.rdc = make(map[uint64]uint64)
-	}
-	o.rdc[sseq] -= 1
+	if o.rdc == nil {
+		return
+	}
+	dc, ok := o.rdc[sseq]
+	if !ok {
+		return
+	}
+	// Mirror incDeliveryCount, which goes from "no entry" to 1 on the first
+	// redelivery. Going back to the initial delivery therefore has to remove
+	// the entry rather than leave a zero behind, since a number of call sites
+	// (needAck, decStreamPending) test for key presence rather than value.
+	if dc > 1 {
+		o.rdc[sseq] = dc - 1
+	} else {
+		delete(o.rdc, sseq)
+	}
}

• What this does:
  • If the map is nil (no redelivery state at all), or the entry is missing (never redelivered), it returns early—no decrement, no new state created.
  • If the count is greater than 1, it decrements.
  • If the count is 1, it deletes the entry entirely (restoring the "never redelivered" state), rather than leaving a zero value behind.
• Why:
  • Prevents underflow (0-1 wrapping to 2^64-1).
  • Ensures that the presence/absence of a key accurately reflects whether a message has redelivery state, matching how other parts of the code check for redelivered messages.

Test covers both underflow and round-trip symmetry:

func TestJetStreamConsumerDeliveryCountUnderflow(t *testing.T) {
    // ...setup omitted...

    o.mu.Lock()
    defer o.mu.Unlock()

    // Underflow test: decDeliveryCount on a message never redelivered
    o.decDeliveryCount(1)
    require_True(t, o.rdc == nil)
    require_Equal(t, o.deliveryCount(1), 1)
    require_False(t, o.hasMaxDeliveries(1))

    // Round-trip test: increment and decrement delivery count
    require_Equal(t, o.incDeliveryCount(1), 2)
    require_Equal(t, o.deliveryCount(1), 1)
    require_Equal(t, o.incDeliveryCount(1), 3)
    require_Equal(t, o.deliveryCount(1), 2)
    o.decDeliveryCount(1)
    require_Equal(t, o.deliveryCount(1), 1)
    o.decDeliveryCount(1)
    require_Equal(t, o.deliveryCount(1), 1)
    _, present := o.rdc[1]
    require_False(t, present)
}

• What this does:
  • First block: Calls decDeliveryCount on a message that was never redelivered. Checks that no state is created, the delivery count is correct, and the message is not treated as having exhausted its deliveries.
  • Second block: Increments and decrements the delivery count, ensuring that the map entry is deleted when the count returns to the "never redelivered" state.

From the review thread

MauriceVanVeen: Although I agree that defense in depth for this would be good, I'm wondering if this can reproduce normally?
TestJetStreamConsumerDeliveryCountUnderflow currently bypasses the public interface. Is it possible for a normal consumer delivery path, high-level/as used by a client, to trigger this bug?

no-hup: Fair question, and you're right that the test goes under the public interface. So I went and checked whether a normal client path actually gets there.

It does, and this repo's own test hits it. Patch decDeliveryCount to panic instead of decrementing:

func (o *consumer) decDeliveryCount(sseq uint64) {
    if o.rdc == nil {
        panic(fmt.Sprintf("underflow: nil rdc sseq=%d maxdc=%d", sseq, o.maxdc))
    } else if _, ok := o.rdc[sseq]; !ok {
        panic(fmt.Sprintf("underflow: absent sseq=%d maxdc=%d", sseq, o.maxdc))
    }
    o.rdc[sseq] -= 1
}

then run go test ./server -run TestJetStreamConsumerOverflow -count=10:

panic: underflow: nil rdc sseq=96 maxdc=0
...

That's a pull consumer driven entirely over $JS.API.CONSUMER.MSG.NEXT. A message gets sent to a reply inbox that no longer has interest, internalLoop sees !didDeliver and calls didNotDeliver. It's the message's first delivery, so there is no rdc entry, and the count wraps.

Where I should be straight with you though: in that test maxdc == 0, so hasMaxDeliveries returns early and the wrapped count never turns into a dropped message. I have not managed to build an end-to-end case that combines that path with MaxDeliver set, including bolting MaxDeliver onto the overflow test itself, which then doesn't take the same route. So I'd call the message loss reachable on the code paths but not something I've demonstrated to you, and I don't want to claim more than that.

What is definitely happening on the current code is that a consumer ends up holding a fabricated redelivered entry of 2^64-1 for a message that was never redelivered. deliveryCount hands that value straight back, needAck presence-checks the key, and it reaches the BackOff index as int(-1), which I'm guessing is why :2281, :5944 and :6122 each carry a if dc < 0 { dc = 0 } guard.

Happy to go either way from here: keep digging for the end-to-end repro, or rework the test to assert through the client using the overflow test's shape instead of reaching into the consumer. Let me know which you'd rather have.

Takeaways

• Defensive programming in data structure management is critical: Always check for the existence of a map and key before mutating or decrementing values, especially when absence is semantically meaningful.
• When incrementing and decrementing counters that are tracked via optional map entries, ensure that the decrement operation is the true inverse of increment—not just in value, but also in key presence—if other code depends on key existence rather than value.
• Writing targeted unit tests for subtle state transitions (like underflow and round-trip symmetry) can catch bugs that are difficult to reproduce through public APIs or integration tests.