Skip to content

Release 2.8.0-preview.1#1112

Merged
mtmk merged 1 commit intomainfrom
release-2.8.0-preview.1
Apr 16, 2026
Merged

Release 2.8.0-preview.1#1112
mtmk merged 1 commit intomainfrom
release-2.8.0-preview.1

Conversation

@mtmk
Copy link
Copy Markdown
Member

@mtmk mtmk commented Apr 16, 2026

Preview release bumping version to 2.8.0-preview.1. Ships two breaking changes on the preview channel before they land in a stable minor: subject validation is now on by default (subjects containing whitespace throw; set SkipSubjectValidation=true to opt out), and the public NATS.Client.Core.NKeys and NKeyPair types are removed in favour of the NATS.NKeys package. Also includes protocol size checks, KV watcher cancellation fixes, and assorted test and CI fixes.

@synadia-claude-reviewer
Copy link
Copy Markdown
Contributor

synadia-claude-reviewer Bot commented Apr 16, 2026

Claude finished @mtmk's task in 6m 6s —— View job


PR Review: Release 2.8.0-preview.1

  • Gather context (CLAUDE.md, PR diff)
  • Analyze changed files and commits
  • Review for correctness, security, and performance
  • Post inline comments on specific issues
  • Summarize findings

This PR is a version bump only (version.txt: 2.7.32.8.0-preview.1). All substantive changes were already merged to main individually. I reviewed the code behind each major change in this release.

Overall Assessment

The implementation is sound. The two breaking changes are well-handled: subject validation defaults to enabled with a clear opt-out (SkipSubjectValidation = true), and the NATS.NKeys package migration is clean. Below are specific findings.


Issues Found

1. NatsException.Throw missing [DoesNotReturn]src/NATS.Client.Core/NatsException.cs:19

Every other throw-helper in the file (NatsNoReplyException.Throw, NatsProtocolViolationException.Throw, NatsTimeoutException.Throw) is annotated with [DoesNotReturn], but the base NatsException.Throw is not. Callers in Internal/HeaderWriter.cs (lines 71, 86) use it as a non-returning throw, so the compiler can't perform accurate control-flow analysis there. Fix this →

2. NatsKVWatcher.DisposeAsync — potential subscription leaksrc/NATS.Client.KeyValueStore/Internal/NatsKVWatcher.cs:148

The dispose sequence is:

1. _nats.ConnectionDisconnected -= OnDisconnected   // stops disconnect-triggered CreateSub
2. if (_sub != null) await _sub.DisposeAsync()       // disposes current sub
3. TryComplete() on all channels
4. await _consumerCreateTask

ConsumerCreateLoop runs concurrently. If a "recreate" message was already dequeued from _consumerCreateChannel before step 3 completes, CreatePushConsumer will create and register a new _sub (step 4 only waits for the task to finish — after the new sub is already created). That new sub never gets DisposeAsync called. The connection's subscription manager will eventually clean it up, but it's an avoidable leak. A fix would be to cancel an internal CancellationTokenSource in DisposeAsync before awaiting tasks, rather than relying solely on channel completion.

3. ClearingMemoryPool is pipe-wide, not credential-scopedsrc/NATS.Client.Core/Internal/ClearingMemoryPool.cs:5-8

The class doc says "preventing credential data from leaking through pooled memory." The pool is actually used for the entire CommandWriter pipe (Commands/CommandWriter.cs:81), clearing all outbound protocol data (subjects, headers, payloads) on return. The security property is correct — credential bytes that flow through the pipe will be zeroed — but the comment implies narrower scope than the actual behavior.

4. maxPayloadHardCap not validated against server-advertised max_payloadsrc/NATS.Client.Core/Internal/NatsReadProtocolProcessor.cs:181

The read-side hard cap (default 64 MB) is independent of the server's advertised MaxPayload. A server could advertise max_payload = 1 MB but the client won't reject incoming messages up to 64 MB. The write-side correctly enforces info.MaxPayload. Incoming messages exceeding the server's own limit shouldn't normally occur (the server enforces its own cap), but the inconsistency is worth noting for defense-in-depth. Not a regression — pre-existing design.


Positive Notes

  • Auth nonce signing (Fix nonce signing when auth_required is omitted #1109): the fix opts.Sig = info is { Nonce: { } } ? Sign(info.Nonce, seed) : null correctly decouples nonce signing from auth_required, handling servers with no_auth_user properly.
  • Protocol size checks (Add protocol size checks #1095): negative payload length and oversized message detection in NatsReadProtocolProcessor will disconnect (not crash or OOM) the client, and the NatsProtocolViolationException path correctly calls SignalDisconnected to allow reconnect.
  • Subject validation (Enable subject validation by default #1093): the dual-path implementation (manual loop for short subjects, SearchValues/SIMD for long ones) and consistent application across Publish, Subscribe, and Request APIs is well done.
  • Credential state reset on failure (Reset state on credential loading failure #1107): credential load failures now set ConnectionState = Closed before rethrowing, allowing retry connect without getting stuck.

Comment thread version.txt
@@ -1 +1 @@
2.7.3
2.8.0-preview.1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version bump looks correct for a SemVer preview release. The 2.8.0-preview.1 label is appropriate given the two breaking changes shipped: subject validation enabled by default and the NATS.NKeys type removal.

Copy link
Copy Markdown
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mtmk mtmk merged commit 4d72b48 into main Apr 16, 2026
29 of 30 checks passed
@mtmk mtmk deleted the release-2.8.0-preview.1 branch April 16, 2026 09:57
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.

2 participants