Skip to content

Release 3.0.0-preview.9#1184

Merged
mtmk merged 1 commit into
release/3.0from
release-3.0.0-preview.9
Jun 15, 2026
Merged

Release 3.0.0-preview.9#1184
mtmk merged 1 commit into
release/3.0from
release-3.0.0-preview.9

Conversation

@mtmk

@mtmk mtmk commented Jun 15, 2026

Copy link
Copy Markdown
Member

Ninth 3.0 preview. Request-reply now defaults to Direct mode and
subscription channel overflow defaults are unified, both behavior
changes worth shaking out before 3.0 final. Adds an explicit drain
API for subscriptions and consumers, and deprecates
SkipSubjectValidation.

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: Release 3.0.0-preview.9

Review posted as a PR comment. Key findings:

  1. PR Default request-reply to Direct mode #1182 (Direct mode default): _directSetIntentionally sentinel is correctly implemented. JetStream ThrowIfNoResponders=false coverage is complete. The NatsOpts equality including _directSetIntentionally is semantically correct but worth documenting.
  2. PR Unify subscription channel overflow defaults #1181 (Unified overflow defaults): Two breaking changes worth release-noting: (a) NatsClient(NatsOpts, BoundedChannelFullMode) constructor removed — callers get DropNewest instead of the old Wait default; (b) DI package silently flips from Wait to DropNewest. The 16× SubPendingChannelCapacity increase (1024 → 16384) is intentional but has memory implications.
  3. PR Add explicit subscription and consumer drain #1177 (Drain API): DrainAsync silently swallows OperationCanceledException — documented but unusual; consider a debug log or stronger interface doc. Core drain logic (volatile _draining, finally guarantees, heartbeat race handling) is correct.
  4. PR Fix server.port type and trace tag source #1175 (OTel fix): Clean fix — connect URI instead of ServerInfo.Host, boxed port reuse, and correct int-typed port tags.
  5. PR Fix flaky CI tests #1179 (CI flakiness): MaxParallelThreads=1 is more conservative than ParallelizeTestCollections=false; may be worth revisiting once the MeterTracker lock is in place.

@synadia-claude-reviewer

Copy link
Copy Markdown
Contributor

Code Review: 3.0.0-preview.9 (PRs #1177, #1180, #1181, #1182, #1175)

This is a version-bump PR; the actual code changes shipped in the referenced PRs. Notes below are on the substantive changes bundled in this preview.


PR #1182 — Default request-reply to Direct mode

The _directSetIntentionally sentinel in NatsOpts is a clean way to distinguish "I set Direct explicitly" from "I left the default". Implementation looks correct: the field initializer bypasses the init accessor so Default correctly has _directSetIntentionally = false, and with { RequestReplyMode = Direct } calls the accessor and flips it to true.

One subtlety worth documenting: since NatsOpts is a sealed record, _directSetIntentionally participates in synthesized equality. Two instances with the same RequestReplyMode value but different _directSetIntentionally are not equal:

NatsOpts.Default != new NatsOpts { RequestReplyMode = NatsRequestReplyMode.Direct }
// _directSetIntentionally differs → HasNoResponders behavior differs → not equal

That's actually the correct semantic, but callers comparing NatsOpts instances (e.g. caching by opts) may be surprised.

JetStream ThrowIfNoResponders = false coverage: TryPublishAsync and TryJSRequestAsync correctly opt out. NatsJSStream.GetDirectAsync does not, but existing tests already assert NatsNoRespondersException is thrown there (DirectGetTest.cs:36), so that's intentional pre-existing behavior, not a regression.


PR #1181 — Unify subscription channel overflow defaults

Breaking API change: NatsClient(NatsOpts opts, BoundedChannelFullMode pending = BoundedChannelFullMode.Wait) is removed. Any existing code that compiled against preview.7/preview.8 using the two-argument form—including the default pending: Wait—now gets DropNewest semantics (the new NatsClient(NatsOpts) inherits NatsOpts.Default). This is a behavior change, not just a compile error.

Channel capacity increase: SubPendingChannelCapacity went from 1,024 → 16,384. Per-subscription memory ceiling increases 16×. Systems with thousands of subscriptions holding mostly-empty channels won't notice, but workloads that actually fill channels could see significant heap pressure. Worth a release-notes callout even in preview.

DI overflow mode flip: NATS.Extensions.Microsoft.DependencyInjection previously forced BoundedChannelFullMode.Wait; it now falls through to DropNewest. Applications that relied on the DI package's Wait default without explicitly setting it will silently drop messages instead of applying backpressure after upgrading.


PR #1177 — Explicit subscription and consumer drain

The refactor of DrainAsync into DrainCoreAsync + DrainOnDisposeAsync is clean. A few notes:

Silent cancellation: DrainAsync(cancellationToken) never throws OperationCanceledException, even when the token is cancelled before or during the call. The docs say "Cancellation does not abort the drain", but callers who pass a real token (e.g. from CancellationTokenSource(timeout)) will see no exception if the PING fence silently times out; they have no way to know the fence was skipped. Consider at least logging at debug level when the fence is abandoned, or documenting the fire-and-forget nature of the call more prominently at the interface level.

Heartbeat / drain race: _draining is correctly marked volatile. StopDelivery() is called before the PING fence, and ResetHeartbeatTimer() guards on _draining, so the timer can't re-arm past the drain. The race window (timer fires between the lock and StopDelivery()) lands safely: the heartbeat callback checks _draining at the top and returns, or calls CompleteStop() which is a no-op once the finally has called TryComplete(). Looks sound.

finally guarantees: The finally block in DrainCoreAsync always calls TryComplete(), ensuring that even if PingAsync, UnsubscribeForDrainAsync, or StopDelivery() throw, no consumer is left blocked on the channel forever. Good defensive coding.


PR #1175 — Fix server.port type and trace tag source

The caching of BoxedServerPort, ServerHost, and ClientId (written with Volatile.Write, published before ServerInfo so readers observing the new ServerInfo are guaranteed to see the matching tags) is correct and meaningfully reduces per-message allocation. Using the connect URI instead of ServerInfo.Host (often 0.0.0.0) fixes the OTel server.address tag source correctly.

The port is now typed as int (boxed object) rather than string, matching the OTel semconv expectation for server.port and network.peer.port. New test assertions (AssertIntTag) verify this.


PR #1179 — Fix flaky CI tests

The xunit.runsettings change replaces <ParallelizeTestCollections>false</ParallelizeTestCollections> with <MaxParallelThreads>1</MaxParallelThreads>. These have different semantics: the former serialised collections but allowed intra-collection parallelism; the latter caps the entire runner to one thread. The intent (avoid concurrent writes to MeterListener/global state) is achieved either way, but MaxParallelThreads=1 may be more conservative than needed and will slow the full test suite. The lock added to MeterTracker is the right fix; it may be worth revisiting whether ParallelizeTestCollections=false suffices once that lock is in place.


Summary

The release is well-tested and the major correctness concerns (heartbeat/drain race, finally guarantees, JetStream no-responders handling) are handled correctly. The items most worth tracking before 3.0 final:

  1. The NatsClient(NatsOpts, BoundedChannelFullMode) removal + DI WaitDropNewest flip is a silent behaviour change for existing preview users.
  2. DrainAsync silent-swallow of cancellation warrants a clearer interface-level doc or debug log.
  3. The 16× channel capacity increase and its memory implications should appear in the migration guide.

@mtmk
mtmk merged commit 5129ee9 into release/3.0 Jun 15, 2026
33 of 35 checks passed
@mtmk
mtmk deleted the release-3.0.0-preview.9 branch June 15, 2026 13:37
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