Skip to content

rebase rivian fork 2.10.3 with main. - #1

Open
Codecheetos2023 wants to merge 6007 commits into
rivian:mainfrom
nats-io:main
Open

Codecheetos2023 wants to merge 6007 commits into
rivian:mainfrom
nats-io:main

Conversation

@Codecheetos2023

Copy link
Copy Markdown
  • Link to issue, e.g. Resolves #NNN
  • Documentation added (if applicable)
  • Tests added
  • Branch rebased on top of current main (git pull --rebase origin main)
  • Changes squashed to a single commit (described here)
  • Build is green in Travis CI
  • You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Resolves #

Changes proposed in this pull request:

@codecheetos

Copy link
Copy Markdown

approved

MauriceVanVeen and others added 29 commits August 21, 2026 12:42
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
If replicated SyncAlways stream is restarted before its first Raft
snapshot,
WAL replay would reset the stream store to sequence zero, ignoring
FirstSeq.
)

This PR fixes a number of issues relating to errors on consumer create:
- `setConsumer` no longer overwrites an existing consumer and
`removeConsumer` only unregisters when the entry is the same instance
that is passed, so a stale consumer with the same name can't evict the
live consumer from `mset.consumers`, `cList` and `csl`.
- The name-exists check now runs before
`mset.store.ConsumerStore(o.name, ...)`, which opened the existing
consumer's on-disk directory and then deleted it when the rejected
create tore itself down.
- Error paths release the temporary consumer with `o.stop()` instead of
`o.deleteWithoutAdvisory()`, which on the early name checks could
`os.RemoveAll` an unrelated stream named `obs` (unlikely but fixed
anyhow).
- The `ackMsgs`/`nextMsgReqs` ipQueues are created after the name-exists
check, since they key `s.ipQueues` by consumer name and the rejected
create overwrote and then unregistered the live consumer's entries.
- `mset.numFilter` is removed, as it was maintained in three places,
never read, and miscounted consumers using `FilterSubjects`.

In practice the name-exists paths above only apply to single/standalone
servers, since clustered paths perform this check elsewhere and would
prevent it.
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
When WAL recovery truncated a filestore to a deleted and compacted
snapshot sequence, Truncate failed with errDeletedMsg.
The fix is to truncate the selected block at the preceding stored
message, if any.

Signed-off-by: Daniele Sciascia <daniele@nats.io>
Prevent it from failing on container based environments.

Signed-off-by: Waldemar Quevedo <wally@nats.io>
Signed-off-by: Waldemar Quevedo <wally@nats.io>
Unbounded concurrent JS API requests an queue at the meta leader
longer than the 10s JS API timeout from the client, causing
context deadline exceeded on slower CI machines.

Bound in-flight creates to 50 consumers and 25 streams.

Signed-off-by: Waldemar Quevedo <wally@nats.io>
…#8494)

Prevent it from failing on container based environments.

Signed-off-by: Waldemar Quevedo <wally@nats.io>
Fixes
```
=== RUN   TestJetStreamClusterParallelStreamCreation
    jetstream_cluster_3_test.go:1552: require uint64 equal, but got: 0 != 100
--- FAIL: TestJetStreamClusterParallelStreamCreation (1.23s)
```

Signed-off-by: Waldemar Quevedo <wally@nats.io>
Unbounded concurrent JS API requests an queue at the meta leader longer
than the 10s JS API timeout from the client, causing context deadline
exceeded on slower CI machines.

Signed-off-by: Waldemar Quevedo <wally@nats.io>
#8493)

When WAL recovery truncated a filestore to a deleted and compacted
snapshot sequence, Truncate failed with errDeletedMsg. The fix is to
truncate the selected block at the preceding stored message, if any.
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
…ts (#8502)

Internal message deletes, such as those for replicated Interest/WQ as a
result of message acknowledgement, were accounted in the API stats. This
PR fixes that by preventing the accounting as well as advisories that
would be sent for all these acked and removed messages.
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
…austing MaxDeliver

decDeliveryCount decremented o.rdc[sseq] without checking that an entry
existed, so undoing a delivery attempt for a message that had never been
redelivered wrapped the count to 2^64-1. The nil branch also created the
map before underflowing into it, giving a consumer redelivery state for a
message it had never redelivered.

deliveryCount's `dc >= 1` guard does not catch the wrapped value, so it was
returned as-is and hasMaxDeliveries saw a count >= any configured
MaxDeliver. A single failed delivery attempt on a message's first delivery
therefore dropped it from pending, advanced the ack floor past it and
emitted a max-deliveries advisory carrying the wrapped count, and the
message was never redelivered. The same value reached the BackOff index
computations as int(-1).

Make decDeliveryCount the inverse of incDeliveryCount: no-op when there is
no entry, and remove the entry rather than leaving a zero when going back
to the initial delivery, since needAck and decStreamPending test for key
presence rather than value.

Resolves #8511

Signed-off-by: Ishan Shaurya Jaiswal <shauryajaiswal.dev@gmail.com>
Signed-off-by: Waldemar Quevedo <wally@nats.io>
The final checkFor dereferenced ci.Cluster unconditionally, which panicked
with a nil pointer dereference when the consumer info response came back
without cluster info.

```
--- FAIL: TestJetStreamClusterMemoryConsumerCompactVsSnapshot (4.40s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered, repanicked]
[signal SIGSEGV: segmentation violation code=0x1 addr=0x50 pc=0x119f772]

goroutine 61033 [running]:
testing.tRunner.func1.2({0x23093b8, 0x2461f70})
	/opt/hostedtoolcache/go/1.27.0/x64/src/testing/testing.go:2123 +0x232
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.27.0/x64/src/testing/testing.go:2126 +0x329
panic({0x23093b8?, 0x2461f70?})
	/opt/hostedtoolcache/go/1.27.0/x64/src/runtime/panic.go:859 +0x125
github.com/nats-io/nats-server/v2/server.TestJetStreamClusterMemoryConsumerCompactVsSnapshot.func1()
	/home/runner/work/nats-server/nats-server/server/jetstream_cluster_2_test.go:5600 +0x72
github.com/nats-io/nats-server/v2/server.checkForErr(0x12a05f200, 0x5f5e100, 0x213a5c8fdf28)
	/home/runner/work/nats-server/nats-server/server/server_test.go:48 +0x94
github.com/nats-io/nats-server/v2/server.checkFor({0x2405ea0, 0x213a5b438908}, 0x12a05f200, 0x5f5e100, 0x213a5c8fdf28)
	/home/runner/work/nats-server/nats-server/server/server_test.go:59 +0x48
github.com/nats-io/nats-server/v2/server.TestJetStreamClusterMemoryConsumerCompactVsSnapshot(0x213a5b438908)
	/home/runner/work/nats-server/nats-server/server/jetstream_cluster_2_test.go:5597 +0x568
```

Signed-off-by: Waldemar Quevedo <wally@nats.io>
…te size

Change to compare against nbMaxVectorSize.

Signed-off-by: Waldemar Quevedo <wally@nats.io>
MauriceVanVeen and others added 30 commits September 17, 2026 10:18
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Concurrent `updateAccountClaimsWithRefresh` calls on the same account
could race, since one call empties the account's exports while
rebuilding them and another re-checks other accounts' imports against
those exports, latching them as invalid so importing accounts silently
stopped receiving messages.

This PR fixes that by adding `a.cmu`, serializing claim updates for a
single account. Also contains various test de-flakes, one of which
shortens `TestJWTImportsOnServerRestartAndClientsReconnect` from ~90s to
~1.5s, since it was mostly sleeping.
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
…8416)

QoS2 ingest was stop-and-wait on the server side. Each PUBLISH blocked
the publisher's readLoop on the JetStream store of its held copy into
`$MQTT_qos2in`, and each PUBREL on three more round trips: load the
copy back, delete it waiting for the ack, and store it into `$MQTT_msgs`
for delivery. A QoS2 publisher was therefore capped at a few hundred
messages per second whatever its in-flight window.

This PR extends the QoS1 ack pipeline of #8415 to QoS2. The JetStream
operations are submitted asynchronously, the PUBREL delivers from a copy
the connection kept so there is no load, the delete no longer waits, and
the PUBREC and PUBCOMP are sent from the pipeline as the acks arrive, in
packet order. Single-connection QoS2 throughput, flat at 600-800/s on
main whatever the window, reaches ~18K/s at a 256-message window (22x)
and rises with the client's window from 1.4x at a window of 1; aggregate
QoS2 over 100-200 connections improves 1.4-1.5x; QoS1 and QoS0 are
within the run's control spread. Where asynchrony costs the readLoop
its read-your-own-writes view of JetStream, the connection keeps the
state it needs in memory, and a `Nats-Msg-Id` on the delivery store
covers the one case a connection cannot see, a PUBREL retransmitted on
another connection.

## Benchmark

GCP, 6 brokers in two 3-node R3 clusters, generators on separate VMs,
two crossover passes (the builds swap clusters between passes), ratios
pooled. `mqtt-test` sweep, QoS2, main (3c80d8f) vs this branch
(2eb9772):

| in flight | main | this PR | ratio |
|---|---|---|---|
| 1 | 579/s (p99 2.2ms) | 801/s (p99 1.6ms) | 1.4x |
| 10 | 807/s (p99 17.3ms) | 5,827/s (p99 3.0ms) | 7.2x |
| 20 | 814/s (p99 30.6ms) | 8,800/s (p99 3.9ms) | 10.8x |
| 256 | 816/s (p99 346.9ms) | 18,051/s (p99 24.1ms) | 22.1x |

Aggregate, 100 and 200 connections publishing QoS2 at once: 1.5x and
1.4x (about 12K/s to 17.3K/s). QoS1 aggregate and mqtt.js QoS1 1.0x;
QoS1 sweep 0.9-1.0x and the QoS0 control 1.1x, both within this fleet's
placement spread (the unchanged-code control read 0.8x and 1.3x in the
two passes). An earlier run of the same comparison on a quieter fleet
(control 1.00 in both passes) gave 26x at 256, 1.7-1.8x aggregate, and
1.0x for QoS1 at every window. The window-1 gain is the two round trips
removed per exchange (the load and the waited delete); the rest of the
curve is the pipelining. The remaining gap to QoS1's aggregate is
structural: two appends and a delete per message on the account's
streams.

## What runs where

Everything below is per connection unless noted.

- The readLoop (`mqttProcessPub`, `mqttProcessPubRel`) parses the
  packet, pushes the JetStream request(s) to the account's `jsa.sendq`,
  and admits one entry per packet into the connection's ack pipeline, a
  buffered channel of `mqttMaxPipelined` (1024) entries. A full window
  blocks the readLoop, which backpressures the client; no ack for the
  oldest entry within the JS API timeout (5s) fails the connection. The
  readLoop-owned state (`c.mqtt.pipe`, `qos2Exchanges`, `qos2Released`,
  `qos2CachedBytes`) is accessed without a lock, from the readLoop only.
- The ack loop (`mqttAckLoop`) is one go routine per connection with
  inbound QoS1/2 traffic, started with `s.startGoRoutine` on the first
  pipelined packet. It takes entries in order, waits for each entry's
  JetStream ack(s) with one re-armed timer, and enqueues the PUBACK,
  PUBREC, or PUBCOMP under the client lock (`mqttEnqueuePubResponse`).
  The single FIFO preserves the per-type ordering of [MQTT-4.6.0-2] and
  [MQTT-4.6.0-3]. A failed or timed-out operation closes the connection,
  as the synchronous code did; the client re-sends on reconnect
  [MQTT-4.4.0-1]. `mqttHandleClosedClient` stops the loop and drains the
  queue.
- The JS API reply callback (`processJSAPIReplies`, which can run from
  various go routines) claims the entry's reply registration with
  `LoadAndDelete` on the account's `jsa.replies` and completes the
  entry's buffered(1) channel; it never blocks. Every give-up path in
  the pipeline (error, timeout, pipeline stopped, server shutdown, and
  the close drain) removes its registrations, so nothing leaks in the
  account-scoped map. Note that one PUBCOMP entry holds two
  registrations, for the store and for the delete.
- The sendq consumer (`sendJSAPIrequests`) adds the `Nats-Msg-Id`
  header to a delivery store when the request carries one, in the
  buffer rebuild it already does for messages with headers: one
  allocation, the same count as the trailing CRLF append it replaces.

## The QoS2 flow

PUBLISH (Method A of [MQTT-4.3.3-2], as before): the readLoop stores the
message into `$MQTT_qos2in` as `$MQTT.qos2.in.<client id>.<PI>`, with
the packet encoded in the NATS header, and keeps a copy on the
connection in `qos2Exchanges` so the PUBREL needs no load. The copies
are capped per connection at 1024 messages and 4 MB
(`mqttMaxCachedQoS2Msgs`, `mqttMaxCachedQoS2Bytes`); past either cap
the PUBREL loads the copy from the stream, as it does for a resumed
session, so the caps bound memory and cost one load per message beyond
them, never correctness. 1024 is the pipeline window, so a client inside
its window never hits the count; 4 MB is four default `max_payload`s.
Both are constants, like the pipeline window in #8415; making them
options is a small follow-up if wanted. A PUBLISH
retransmitted while the connection holds the publication gets its PUBREC
from the pipeline, in order, and no second store [MQTT-4.3.3-2]: a
second
store could be applied after the PUBREL's delete of the first copy
(the delete is a JS API request, the store is not, and nothing orders
the two) and leave a stray copy under the PI. A retransmit the
connection does not hold (a resumed session) is stored and deduped by
the stream's max-msgs-per-subject of 1, as before. The reply callback
records the outcome of the hold store on the cached exchange (an atomic
state plus the held copy's sequence, written before the entry's channel
is completed, so both are set before the PUBREC goes out):
`mqttQoS2HoldStored` with the sequence, or `mqttQoS2HoldDeduped` when
max-msgs-per-subject rejected the store, in which case no sequence came
back and the PUBREL loads the copy from the stream.

PUBREL: the readLoop takes the cached copy, marks the PI released in
`qos2Released` (an `avl.SequenceSet`, 65535 PIs at most), broadcasts the
message as a QoS0 delivery would (`processInboundClientMsg`, the same
call as QoS1), and submits two JetStream requests at once: the delivery
store into `$MQTT_msgs` and the delete of the held copy by its exact
sequence. The PUBCOMP is sent when both are acked. A PUBREL for a PI in
`qos2Released` (a retransmission) is answered with a PUBCOMP without
consulting JetStream, where a load could still see the not-yet-deleted
copy [MQTT-4.3.3-1]. A PUBREL for a PI unknown to the connection (a
session resumed on a new connection, a copy dropped past the cache
caps, or a deduped hold store) loads the copy from `$MQTT_qos2in` as
the synchronous code always did; the load returns the sequence, so the
delete is still by sequence. A new PUBLISH on a released PI is a new
publication [MQTT-4.3.3-2] and clears the released mark.

Note that the delete is by sequence in every case, so it cannot hit a
successor copy on a reused PI. The PUBCOMP waits for the delete ack, and
in a cluster that ack is sent when the delete is applied, so a compliant
client's next PUBLISH on the PI reaches the stream after the old copy is
gone.

## Why the message id

With the delete asynchronous, a PUBCOMP's prerequisites can be in
flight when the client loses its connection. The client retransmits the
PUBREL on reconnect [MQTT-4.4.0-1], possibly to another server, which
has no memory of the exchange, loads the copy if the delete has not
applied yet, and delivers the message a second time. The synchronous
code deleted the copy before delivering, so a retransmitted PUBREL found
nothing.

Every QoS2 delivery store into `$MQTT_msgs` therefore carries a
`Nats-Msg-Id` of `q2-<held copy sequence>`. The held copy's sequence is
the same across retransmits of one publication and distinct across
publications (a client id plus PI would drop a new publication on a
reused PI), so JetStream's duplicate detection drops the second store
wherever it comes from. QoS1 stores carry no id.

The dedupe holds for the stream's duplicates window. `$MQTT_msgs` is
created without an explicit window, as before, so JetStream's two minute
default applies, clamped to the server's `duplicate_window` limit and to
the stream's max age. It is also per leader: the map is rebuilt from the
stream on a restart, and `$MQTT_msgs` is interest based, so a PUBREL
retransmitted later than the window, or across a restart of the stream
leader after the first delivery was consumed, is delivered again. A
store that conflicts with an in-flight store of the same id (JetStream's
"duplicate message id is in process", meaning the first store is
proposed but not yet committed) is an error like any other: the
connection closes, and the retransmitted PUBREL then finds the first
store committed, or stores.

A durable released mark in `$MQTT_qos2in` (overwrite the held subject
with a marker) was considered instead. It changes what a server finds
under the subject, and a server without this PR deletes the message
before parsing it, so a mixed-version cluster would destroy the marker
and then fail the connection or deliver an empty payload. That needs a
two-phase rollout and a marker format kept forever; the message id gives
the same cross-server dedupe with no format change, so the exchange
state stays connection scoped.

## Behavior changes

- PUBREC and PUBCOMP are no longer sent inline; up to 1024 packets per
  connection can be in flight, and PUBCOMP is sent only after both the
  delivery store and the delete are acked. A failure or timeout on
  either closes the connection instead of acknowledging.
- A load failure on PUBREL closes the connection. Before, every failure
  was treated as "not found" and answered with a PUBCOMP, which ended
  the exchange with the copy still held and never delivered.
- A PUBREL that arrives before its PUBREC was sent closes the
  connection. An invalid held message is deleted and the connection
  closed without the PUBCOMP the old code sent via `defer`, so the
  retried PUBREL converges to a clean PUBCOMP.
- QoS0 subscribers and core NATS subscribers can receive a QoS2 message
  twice: the broadcast precedes the delivery store, and the message id
  covers the store only. The case is a PUBREL retransmitted on a new
  connection while the previous connection's delete is still pending.
  They already can for a QoS1 publish the publisher retransmits; QoS1/2
  subscribers, whose delivery comes from `$MQTT_msgs`, are covered by
  the id. This is a loosening of the QoS2 receiver contract for those
  subscribers and should go in the release notes.
- A client that reuses a PI before receiving its PUBCOMP (a violation
  of [MQTT-2.3.1-2]) can have the new publication dropped: its hold
  store can be rejected by max-msgs-per-subject if the previous copy's
  delete has not applied, and the PUBREL then finds either nothing or
  the old copy. The synchronous code ran a PUBREL to completion before
  parsing the next packet and did not have this window.
- A connection holds up to 4 MB of QoS2 copies between PUBREC and
  PUBREL (see the flow section); this memory is outside `max_pending`.

## Compatibility

No stream configuration, subject, or persisted format changes. The
held copies in `$MQTT_qos2in` are written and read as before, so a
mixed-version cluster during a rolling upgrade is fine: a server without
this PR processes its own connections' PUBRELs synchronously as it does
today, and a server with it dedupes on the `Nats-Msg-Id` regardless of
which server leads `$MQTT_msgs`, since the header is core JetStream.
Messages in `$MQTT_msgs` gain the header; the MQTT delivery path only
reads `Nmqtt-Pub` and ignores it. A clean-session connect still does not
purge the client's held copies, same as before.

## Code changes

- One pipelined type per acknowledgment, `mqttPipelinedPubAck`,
  `mqttPipelinedPubRec`, and `mqttPipelinedPubComp`, behind the small
  `mqttPipelined` interface (response type, PI, reply channels to wait
  on, and `abandon` to drop the reply registrations); the reply
  dispatcher translates per type.
- `deleteMsg` lost its `wait` parameter; `deleteMsgAsync` is the fire
  and forget variant, with an optional reply subject for the pipelined
  delete. The "already deleted" error check moved into
  `isMsgAlreadyDeletedErr`.
- PUBREL deliveries route through `mqttInitiateMsgDelivery` via a
  `heldSeq` parameter, so the broadcast code stays in one place, and a
  permission drop still gates its PUBCOMP on the delete.
- The cached exchange copies the four packet slices out of one
  allocation.

## Tests

Added: PUBREC/PUBCOMP burst ordering with exactly-once delivery
(`TestMQTTQoS2AckPipelineOrder`, which also checks the id is on QoS2
stores and not on QoS1 stores); an abrupt close with exchanges in flight
leaks no reply registrations, delivers the released messages and not
the others, and a successor connection works
(`TestMQTTQoS2AckPipelineConnClose`); back-to-back PI reuse across
releases (`TestMQTTQoS2PIReuseAfterRelease`); a PUBLISH retransmitted
without DUP on a resumed session resolving from the stream
(`TestMQTTQoS2RetransmitWithoutDupResolvesFromStream`); PUBCOMP
implying the held copy is gone, checked without polling after each one,
plus a PUBREL retransmitted on a resumed session finding nothing
(`TestMQTTQoS2PubCompImpliesDelete`); and a PUBLISH retransmitted while
held producing its PUBRECs and one hold store, counted on the stream's
subject (`TestMQTTQoS2RetransmitWhileHeldStoresOnce`). The existing
`TestMQTTQoS2RejectPublishDuplicates` fails without the released marks.
All of `TestMQTT` passes under `-race`.
…nt filters

Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Use the mapped delivery subject when evaluating subscription
deny permissions for queue subscribers. This makes queue
delivery checks consistent with regular subscriptions.

Signed-off-by: Daniele Sciascia <daniele@nats.io>
…refix

Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
…dy gone (#8595)

A message with a per-message TTL that is removed before the TTL fires
(subject purge, rollup) leaves its entry in the timed hash wheel. When
the TTL comes due, `expireMsgs` finds the message already gone and
leaves the entry in place, so it is collected again on every pass,
persisted in `thw.db`, and recovered on restart. With every such entry
in the past the expiry timer re-arms at its 250ms floor and each pass
walks the whole wheel. Details and production numbers in #8594.

- `fileStore.expireMsgs` / `memStore.expireMsgs`: drop the entry when
`removeMsg` reports the message is already gone, mirroring the
subject-delete-marker branch. A genuine removal failure still keeps the
entry so it is retried.
- Regression tests for both stores: store TTL messages, `PurgeEx` them
by subject, run the expiry pass, assert the wheel is empty. Both fail on
`main` without the change.
- Servers already carrying stale entries heal on the first expiry pass
after upgrade.

Resolves #8594

Signed-off-by: Lubomír Cvrk <digitalstraw@gmail.com>
[skip ci]

Signed-off-by: Neil Twigg <neil@nats.io>
[skip ci]

Signed-off-by: Neil Twigg <neil@nats.io>
The test `TestRouteImplicitUsesAdvertisedURLForTLSName` verifies
that the cluster's advertise hostname is used when creating
implicit routes.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
[skip ci]

Signed-off-by: Neil Twigg <neil@nats.io>
A message could already have been redelivered more times than a
newly lowered MaxDeliver (for example when MaxDeliver is changed from
-1/unlimited while redeliveries are in flight). Both paths that expire
a message once it hits MaxDeliver only emitted the delivery exceeded
advisory when the delivery count was exactly at the threshold (MaxDeliver+1),
so a count that jumped past it was silently dropped and no advisory
was sent at all.

Notify whenever the count is at or above the limit instead. In both paths
the sequence is removed from pending immediately after notifying,
which already guarantees the advisory is emitted at most once per sequence.

Resolves #7148

Signed-off-by: Lin Zijie <92932557+zeke-lin@users.noreply.github.com>
…r lowered (#8605)

### Summary

Fixes the delivery exceeded advisory not being emitted when a consumer's
`MaxDeliver` is lowered below a message's current delivery count (most
easily
reproduced by starting with `MaxDeliver: -1` and redelivering, then
lowering it).

Resolves #7148

### Root cause

There are two places where a message is expired on reaching
`MaxDeliver`:

- `getNextMsg`, for messages picked off the redelivery queue (`rdq`)
- `hasMaxDeliveries`, reached from `checkPending` on AckWait expiry

Both used an exact equality check to "only send the advisory once":

- `getNextMsg`: `if dc == o.maxdc+1`
- `hasMaxDeliveries`: `if dc == o.maxdc`

This assumed the delivery count advances by exactly one towards the
threshold.
When `MaxDeliver` is lowered at runtime, the recorded delivery count can
already
be well above the new threshold, so the equality never matches and no
advisory
is produced (the message is still expired, just silently).

### Fix

Notify whenever the delivery count is at or above the limit:

- `getNextMsg`: notify unconditionally inside the `dc > o.maxdc` branch
- `hasMaxDeliveries`: notify unconditionally inside the `dc >= o.maxdc`
branch

The "once" guarantee does not come from the count comparison. Both paths
remove
the sequence from `pending` immediately after notifying, and every code
path that
could re-queue a sequence is guarded on it still being in `pending`
(e.g.
`processNak` returns early when the sequence is no longer pending). So
the
advisory is still emitted at most once per sequence, and an expired
sequence can
never be redelivered.

### Testing

Added two cluster regression tests covering both expiry paths, including
state
consistency after a consumer leader step-down:

- `TestJetStreamClusterConsumerMaxDeliveryLoweredRedeliveredAdvisory`
- `TestJetStreamClusterConsumerMaxDeliveryLoweredAckWaitAdvisory`
Use the mapped delivery subject when evaluating subscription deny
permissions for queue subscribers. This makes queue delivery checks
consistent with regular subscriptions.
…8617)

The test `TestRouteImplicitUsesAdvertisedURLForTLSName` verifies that
the cluster's advertise hostname is used when creating implicit routes.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.