Skip to content

Add TermWithReason support to AckTerminateAsync#1048

Merged
mtmk merged 2 commits into
mainfrom
term-with-reason
Feb 26, 2026
Merged

Add TermWithReason support to AckTerminateAsync#1048
mtmk merged 2 commits into
mainfrom
term-with-reason

Conversation

@mtmk

@mtmk mtmk commented Jan 22, 2026

Copy link
Copy Markdown
Member

Add optional reason parameter to AckTerminateAsync that sends +TERM <reason> to the server, matching the Go client's TermWithReason functionality.

The reason appears in JetStream advisory events (requires NATS Server 2.10.4+).

Low-allocation implementation using ArrayPool<byte> with platform-specific optimizations for netstandard2.0 vs newer targets.

⚠️ This is a breaking change for applications implementing INatsJSMsg<T>

I only expect tests would break and can be detected and fairly easily fixed during build. However, if applications are using their own implementations of INatsJSMsg<T> they will break during build too but more severely if they are transitive dependencies using an earlier version of NATS.Client.JetStream they would fail at runtime.

Interface INatsJSMsg<T> Changes

Change Binary Compatible Source Compatible
defaultnull on ReplyAsync params ✅ Yes ✅ Yes
defaultnull on AckAsync param ✅ Yes ✅ Yes
defaultnull on NakAsync param ✅ Yes ✅ Yes
defaultnull on AckProgressAsync param ✅ Yes ✅ Yes
defaultnull on AckTerminateAsync param ✅ Yes ✅ Yes
New method AckTerminateAsync(string reason, ...) No No

Struct NatsJSMsg<T> Changes

Change Binary Compatible Source Compatible
defaultnull on all method params ✅ Yes ✅ Yes
New overload AckTerminateAsync(string reason, ...) ✅ Yes ✅ Yes
delay == defaultdelay == TimeSpan.Zero ✅ Yes ✅ Yes
Connection == defaultConnection == null ✅ Yes ✅ Yes
Doc comments → <inheritdoc /> ✅ Yes ✅ Yes
Internal NatsJSMsgConstants class ✅ Yes ✅ Yes

Final Verdict

Usage Pattern Safe to Ship?
Applications using NatsJSMsg<T> directly Yes
Applications calling methods via INatsJSMsg<T> Yes
Applications/libraries implementing INatsJSMsg<T> No

Breaking Change Details

What breaks:

// Any type implementing INatsJSMsg<T> - e.g., user mocks for testing
public class MockJSMsg<T> : INatsJSMsg<T>
{
    // Must now implement:
    // ValueTask AckTerminateAsync(string reason, AckOpts? opts, CancellationToken ct)
}

How it breaks:

Scenario: Source break
Failure Mode: Compilation error when updating package and recompiling

Scenario: Binary break
Failure Mode: TypeLoadException at runtime if old compiled mock is loaded with new library version (transitive dependency scenario)

Who is affected:

  • Users who created test doubles/mocks of INatsJSMsg
  • Libraries that expose their own implementations of INatsJSMsg

Who is NOT affected:

  • Users who only consume messages (vast majority)
  • Users who only use the concrete NatsJSMsg struct

Risk Assessment

Factor: Likelihood of user interface implementations
Assessment: Medium - Interface docs explicitly mention "creating test doubles in unit testing"

Factor: Severity of break
Assessment: High - TypeLoadException crashes app at startup

Factor: Detectability
Assessment: Low for transitive - User may not know they have a dependency with an implementation

Factor: SemVer classification
Assessment: Breaking change - requires major version bump per SemVer


Conclusion

The current implementation is a breaking change.

It will cause TypeLoadException at runtime for any code that implements INatsJSMsg, including through transitive dependency updates where the user never modified their own code.

Add optional reason parameter that sends "+TERM <reason>" to the server,
matching the Go client's TermWithReason functionality. The reason appears
in JetStream advisory events and requires NATS Server 2.10.4+.

Implementation uses ArrayPool to avoid allocations. Platform-specific
code paths handle netstandard2.0 vs newer targets for optimal performance.
@mtmk
mtmk requested a review from scottf January 22, 2026 13:33

@scottf scottf left a comment

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.

LGTM

@mtmk mtmk added the breaking label Jan 23, 2026
@mtmk

mtmk commented Jan 23, 2026

Copy link
Copy Markdown
Member Author

Alternative non breaking change using extension method:

  public static class NatsJSMsgExtensions
  {
      public static ValueTask AckTerminateAsync<T>(
          this NatsJSMsg<T> msg,
          string reason,
          AckOpts? opts = null,
          CancellationToken cancellationToken = default)
      {
          return msg.AckTerminateInternalAsync(reason, opts, cancellationToken);
      }
  }

Down side not useful for testing

@mtmk
mtmk marked this pull request as draft January 23, 2026 12:41
@mtmk mtmk changed the title Add TermWithReason support to AckTerminateAsync Proposal: Add TermWithReason support to AckTerminateAsync Jan 23, 2026
@mtmk mtmk changed the title Proposal: Add TermWithReason support to AckTerminateAsync [PROPOSAL] Add TermWithReason support to AckTerminateAsync Jan 23, 2026
@mtmk

mtmk commented Feb 24, 2026

Copy link
Copy Markdown
Member Author

we can probably take the risk of breaking INatsJSMsg<T> in this occasion.

@mtmk
mtmk marked this pull request as ready for review February 24, 2026 12:57
@mtmk mtmk changed the title [PROPOSAL] Add TermWithReason support to AckTerminateAsync Add TermWithReason support to AckTerminateAsync Feb 24, 2026
@mtmk mtmk self-assigned this Feb 24, 2026
@mtmk
mtmk merged commit 9a7ac08 into main Feb 26, 2026
22 checks passed
@mtmk
mtmk deleted the term-with-reason branch February 26, 2026 06:33
mtmk added a commit that referenced this pull request Mar 13, 2026
* Add optional window_size parameter to StreamSnapshotRequest (#1088)
* Bump Microsoft.Bcl.Memory from 9.0.0 to 9.0.14 (#1089)
* Fix `PingCommand` cancellation (#1086)
* rework terminate reason (#1081)
* Fix OTel network telemetry tags (#1078)
* Add consumer info usage warnings (#1079)
* Add TermWithReason support to AckTerminateAsync (#1048)
* Fix code analyser warning (#1076)
* Add cancelled token handling for consumers (#1068)
* Add validation for unsupported PinnedClient calls (#1063)
* Add see-also references to Orbit packages (#1077)
* Add Synadia.Orbit.Testing.NatsServerProcessManager (#1065)
* Add slow consumer docs (#1073)
* Fix error logs URI rewritten by OnConnectingAsync (#1067)
* Add more tests for JetStream consumer behavior (#1055)
@mtmk mtmk mentioned this pull request Mar 13, 2026
mtmk added a commit that referenced this pull request Mar 13, 2026
* Add optional window_size parameter to StreamSnapshotRequest (#1088)
* Bump Microsoft.Bcl.Memory from 9.0.0 to 9.0.14 (#1089)
* Fix `PingCommand` cancellation (#1086)
* rework terminate reason (#1081)
* Fix OTel network telemetry tags (#1078)
* Add consumer info usage warnings (#1079)
* Add TermWithReason support to AckTerminateAsync (#1048)
* Fix code analyser warning (#1076)
* Add cancelled token handling for consumers (#1068)
* Add validation for unsupported PinnedClient calls (#1063)
* Add see-also references to Orbit packages (#1077)
* Add Synadia.Orbit.Testing.NatsServerProcessManager (#1065)
* Add slow consumer docs (#1073)
* Fix error logs URI rewritten by OnConnectingAsync (#1067)
* Add more tests for JetStream consumer behavior (#1055)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants