(2.14) [IMPROVED] Rollup allowed if at DiscardNewPerSubject limit

[MauriceVanVeen]

When a stream is configured with DiscardNewPerSubject and the per-subject message limit is reached, Nats-Rollup messages were rejected before being stored. Even though the rollup would result in clearing up space below the DiscardNewPerSubject limit. This PR improves this by allowing rollups to go through even if at this limit.

Co-authored-by: Charles Duffy charles@cmdzero.io
Signed-off-by: Maurice van Veen github@mauricevanveen.com

[synadia-claude-reviewer]

Claude finished @MauriceVanVeen's task in 5m 26s —— View job

---

PR Review

• Gather PR diff and context
• Read relevant source files
• Analyze correctness, concurrency safety, and performance
• Post review findings

---

Summary

The overall approach is correct: allowing rollup messages through the DiscardNewPerSubject gate is sound because the rollup is stored first and then the purge immediately follows (in processJetStreamMsg at lines 6523–6530, under mset.mu.Lock()). The asl flag is still set to true for rollup messages reaching the per-subject limit, so the downstream MaxMsgs/MaxBytes replacement semantics are preserved correctly.

The AllowRollup guard is checked before the store-level DiscardNewPer bypass in all call paths:

• Non-clustered: processJetStreamMsg line 6137 checks AllowRollup before calling storeRawMsg.
• Clustered (regular + batch): checkMsgHeadersPreClusteredProposal checks allowRollup/denyPurge at line 856 before the DiscardNewPer check at line 938.

Test coverage is good — R1/R3 × file/memory, rollup-sub and rollup-all both verified.

One bug: == nil should be len(…) == 0 (all three sites)

sliceHeader returns a non-nil empty []byte{} when the header key is present but its value is empty (e.g. Nats-Rollup:\r\n). The == nil check does not catch that case.

getRollup (used everywhere else for rollup detection) normalises this correctly via len(r) == 0, returning _EMPTY_. But because the == nil check differs, a message with an empty Nats-Rollup: value:

1. Passes the AllowRollup guard (getRollup returns _EMPTY_, so the guard block is skipped entirely — no AllowRollup check fires).
2. Passes the new DiscardNewPer bypass (sliceHeader returns []byte{}, non-nil → == nil is false → enforcement is skipped).
3. Is stored without any subsequent purge (rollupSub/rollupAll stay false).

Net effect: a client can exceed the per-subject limit by one message on a stream that doesn't have AllowRollup=true, with no purge. The fix — using len(sliceHeader(…)) == 0 — is suggested inline on all three changed lines.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bf6aea71a1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[MauriceVanVeen]

@claude, brief re-review please.

[synadia-claude-reviewer]

Claude finished @MauriceVanVeen's task in 54s —— View job

---

Re-review

• Read changed files
• Assess whether prior feedback was addressed
• Post findings

---

Summary

The prior feedback has been fully addressed. All three sites now use len(sliceHeader(JSMsgRollup, hdr)) == 0 instead of the incorrect == nil:

• server/filestore.go: if fs.cfg.DiscardNewPer && len(sliceHeader(JSMsgRollup, hdr)) == 0
• server/memstore.go: if asl && ms.cfg.DiscardNewPer && len(sliceHeader(JSMsgRollup, hdr)) == 0
• server/jetstream_batching.go: if discardNewPer && maxMsgsPer > 0 && len(sliceHeader(JSMsgRollup, hdr)) == 0

This is consistent with getRollup in stream.go:4964 which also uses len(r) == 0. The fix is correct: sliceHeader returns hdr[start:index:index] (a non-nil empty slice) when the key exists with an empty value, so len(…) == 0 is the right guard to handle that edge case.

No further issues found. The overall approach (allowing rollup messages through the DiscardNewPerSubject gate), the AllowRollup guard ordering, and the test coverage (R1/R3 × File/Memory, both rollup-sub and rollup-all) all look good.

LGTM.