Fix ordered push consumer subscription leak on teardown

[mtmk]

NatsJSOrderedPushConsumer.DisposeAsync did not dispose its current subscription, so the sub's ConnectionOpened handler stayed attached to the connection and the subscription was rooted for the connection's lifetime. Each object store Get/Watch call leaked one subscription on long-lived connections. Dispose the current sub during teardown, after the background loops stop.

Diagnosis and original fix by @haoguanjun in #1186; this rebases the same change onto main and adds a regression test.

Fixes #1187

[synadia-claude-reviewer]

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

---

PR Review

• Gather context
• Read PR diff and changed files
• Review fix correctness and async/disposal safety
• Review regression test
• Post findings

---

The fix is correct and the approach is sound. Here's a detailed breakdown:

Fix Analysis (NatsJSOrderedPushConsumer.cs)

The root cause is clear: NatsJSOrderedPushConsumerSub registers ConnectionOpened in its constructor and removes it in DisposeAsync(), but the final _sub instance was never disposed during teardown, leaving the handler dangling.

The ordering in DisposeAsync() is correct:

1. Timer disposed → heartbeat can't fire
2. _consumerCreateTask awaited → no new _sub can be assigned
3. _commandTask awaited → no reads of _sub in flight
4. [new] _sub disposed → ConnectionOpened handler removed, NATS sub removed from manager
5. DeleteConsumerAsync → server-side cleanup

The volatile read of _sub at step 4 is safe because both background tasks have already completed.

Minor: redundant UnsubscribeAsync()
NatsSubBase.DisposeAsync() already calls UnsubscribeAsync() internally (with an idempotency guard on _unsubscribed). The explicit await sub.UnsubscribeAsync() before await sub.DisposeAsync() is therefore redundant — but it matches the existing pattern in CreatePushConsumer and is harmless, so no change is necessary.

Minor: missing ConfigureAwait(false)
The two new awaits don't carry .ConfigureAwait(false), unlike _context.Connection.AddSubAsync(...) elsewhere in the same class. Not a correctness issue in a library context (and the surrounding code in DisposeAsync() itself is also inconsistent), but worth noting for future cleanup.

Test Analysis (ObjectStoreTest.cs)

The regression test correctly verifies the fix: it counts ConnectionOpened invocation-list entries before and after 10 GetBytesAsync calls and asserts they're equal.

Reflection fragility

var field = typeof(NatsConnection).GetField("ConnectionOpened", BindingFlags.Instance | BindingFlags.NonPublic);

This depends on the backing field name of an auto-implemented event, which is stable but could silently break if the event is ever converted to an explicit implementation (the Assert.NotNull guard ensures it fails loudly rather than silently). Acceptable for a leak test where no public API exposes handler count.

Baseline measurement
Measuring baseline after connection setup (not before any store operations) is correct — it accounts for any handlers the connection itself registers permanently.

---

Overall: the fix is minimal, correctly ordered, and the test definitively covers the leak scenario. No blocking issues.

[github-actions]

🚀 Deployed on https://6a310d1859d521654d9b0f94--teal-klepon-7e92d0.netlify.app

[MauriceVanVeen]

LGTM