Clear ArrayPool buffers before returning to pool

[mtmk]

Pooled byte arrays carrying credentials (passwords, tokens, JWT, nkey signatures) were returned to ArrayPool without clearing, allowing sensitive data to leak into subsequent buffer rentals. This adds clearArray: true to all ArrayPool.Return() calls and introduces a ClearingMemoryPool for the Pipe in CommandWriter so CONNECT message buffers are also zeroed.

• Verify no performance regression in publish/subscribe hot paths

[synadia-claude-reviewer]

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

---

PR Review

• Gather context
• Read changed files
• Analyze security correctness and performance
• Post inline comments
• Post review summary

---

The fix is directionally correct — clearing pooled buffers prevents credential data from leaking into future rentals. The ClearingMemoryPool on the CommandWriter pipe is the most targeted and well-implemented piece.

Issues found:

1. Hot-path clearing in NatsPooledBufferWriter.Reset() (performance)
Reset() is called in the finally block of every publish operation (payloadBuffer, headersBuffer). Adding clearArray: true means every PUB now zeroes the buffer regardless of whether it held credentials. The CONNECT message credentials are already covered by ClearingMemoryPool on the pipe. → inline comment

2. Resize default parameter change (correctness/scope)
Changing clearArray = false → clearArray = true on NatsBufferWriterExtensions.Resize<T> affects all buffer growths in publish/subscribe paths, not just credential-adjacent ones. This is broader than what the PR description claims. → inline comment

3. ClearingMemoryOwner.Dispose() thread safety
The null-check + set pattern is not atomic — two concurrent callers could both capture inner and double-dispose the underlying IMemoryOwner<byte>. An Interlocked.Exchange would make this safe and idiomatic. Unlikely to trigger in practice given pipe's single-writer/reader model, but worth fixing. → inline comment

Minor observations (not blocking):

• ClearingMemoryPool.Dispose() is a no-op (correct, since MemoryPool<byte>.Shared is a shared singleton), but a comment would clarify intent.
• NatsMemoryOwner and NatsBufferWriter are used for general deserialization/serialization across all message types, not just credentials. Clearing on every Dispose() adds overhead across all receive paths. If this is an accepted defence-in-depth trade-off, the commit message / PR body should say so.

[github-actions]

🚀 Deployed on https://69ca5736b90fa839671bc8b2--teal-klepon-7e92d0.netlify.app

[mtmk]

dotnet run -c Release --framework net8.0 --project sandbox/MicroBenchmark -- --filter '*PublishSerialBench*'

this is this branch:

Method	Iter	Mean	Error	StdDev	Allocated
PublishAsync	64	124.9 us	41.62 us	2.28 us	473 B
PublishAsync	512	291.2 us	35.28 us	1.93 us	664 B
PublishAsync	1024	488.6 us	30.27 us	1.66 us	857 B
PublishAsync	1000000	381,460.7 us	70,041.02 us	3,839.18 us	432312 B

...and this is main:

Method	Iter	Mean	Error	StdDev	Allocated
PublishAsync	64	124.7 us	39.52 us	2.17 us	425 B
PublishAsync	512	289.6 us	140.29 us	7.69 us	428 B
PublishAsync	1024	478.5 us	183.77 us	10.07 us	429 B
PublishAsync	1000000	372,067.7 us	72,403.04 us	3,968.65 us	4352 B

Latency is within noise across all sizes, no measurable throughput regression.
The allocation increase is real though. It scales with iteration count.
That's the ClearingMemoryOwner wrapper allocation.

[mtmk]

this is after pooling:

Method	Iter	Mean	Error	StdDev	Allocated
PublishAsync	64	123.3 us	17.87 us	0.98 us	428 B
PublishAsync	512	289.3 us	7.83 us	0.43 us	424 B
PublishAsync	1024	481.5 us	24.35 us	1.33 us	424 B
PublishAsync	1000000	379,333.8 us	38,087.31 us	2,087.69 us	1880 B

on 1 million iteration it performs better than main. we might need to check our pipe usage later, but this means there is no noticeable performance impact.

[scottf]

LGTM