#45 Write TryGetAsync payload directly into the caller buffer

[matthewdevenny]

Part of #45 (zero-copy TryGetAsync). One of three independent PRs splitting that issue.

What

The IBufferWriter<byte> overload of TryGetAsync copied the payload twice: transport buffer → CacheEntry.Data → caller buffer. This adds a per-read, destination-bound deserializer (BufferWritingCacheEntryDeserializer) that writes the payload straight into the caller's buffer on a hit, eliminating the intermediate array on the common path.

• Factor the shared header parse into CacheEntryBinarySerializer.TryReadHeader and extract UpdateEntryExpirationAsync, so the array and buffer read paths agree byte-for-byte and refresh sliding TTLs through one implementation.

Design

• Not zero-copy but single-copy: the payload must land in the caller's buffer, so one copy is irreducible; this removes the redundant intermediate array.
• The write happens only for a genuine hit, preserving the "nothing written on a miss" contract — undeserializable framing, absolutely-expired, and not-found all leave the buffer untouched (absolute expiry is checked inside the deserializer, the only point holding the payload).
• Sliding-expiration entries keep the materialized two-copy path, because refreshing their TTL re-writes the value and therefore needs the bytes. A sliding entry past its absolute expiration still evicts + misses.

Testing

• dotnet build -p TreatWarningsAsErrors=true: clean, net8.0 + net10.0.
• Unit tests: 109/109. Integration tests: 84/84, including 7 new buffer-path tests (exact-payload hit, empty-payload hit, and nothing-written on miss / undeserializable / absolutely-expired / sliding-past-absolute).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Implements the “single-copy” TryGetAsync(string, IBufferWriter<byte>, …) fast path by deserializing directly into the caller-provided buffer (eliminating the intermediate byte[] on the common hit path), while keeping behavior aligned with the existing array-based read path for expiration, eviction, and telemetry.

Changes:

• Added a destination-bound deserializer (BufferWritingCacheEntryDeserializer) that streams the payload into the caller’s IBufferWriter<byte> on genuine hits.
• Factored shared binary framing parsing into CacheEntryBinarySerializer.TryReadHeader so array and buffer read paths validate/locate payload identically.
• Added integration tests covering exact-payload hits, empty payload hits, and “nothing written on miss/invalid/expired” behavior for the buffer overload.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
test/IntegrationTests/Cache/TryGetAsyncBufferTests.cs	Adds integration coverage for buffer-path hit/miss/expired/undeserializable semantics.
src/NatsDistributedCache/NatsCache.cs	Replaces the buffer overload implementation with a single-copy read core and shares sliding-TTL refresh logic.
src/NatsDistributedCache/CacheEntryBinarySerializer.cs	Extracts header parsing/validation into TryReadHeader to share framing rules across read paths.
src/NatsDistributedCache/BufferWritingCacheEntryDeserializer.cs	Introduces a per-read deserializer that conditionally writes payload directly into the caller buffer.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mtmk]

Good win on the common path, and keeping sliding entries on the materialized path is the right call. One thing I would like fixed before merge; everything below that is optional.

Please fix: a failing destination write is reported as an undeserializable entry

Moving destination.Write into Deserialize puts caller-supplied code inside NATS's deserialization callback, where exceptions are swallowed:

1. NatsMsg<T>.Build (NatsMsg.cs:398-414, v3.0.1) catches anything the deserializer throws, sets headers.Error = new NatsDeserializeException(...) and returns data = default.
2. NatsKVStore.TryGetEntryAsync (NatsKVStore.cs:365-376) treats that as success and returns Value = null, Error = <exception>.
3. TryGetAndRefreshBufferAsync sees result == null, logs LogUndeserializableEntry at Debug, tags the miss undeserializable, and drops kvEntry.Error.

The concrete trigger is HybridCache: it hands this overload a RecyclableArrayBufferWriter<byte>.Create(MaximumPayloadBytes) (1 MiB default), and that writer's Advance throws InvalidOperationException("Max length exceeded") on quota. An oversized L2 entry, written either by a node with a higher limit or through plain IDistributedCache.Set which is not quota checked, now looks like data corruption at Debug level. That pollutes the signal UndeserializableEntryTagsMissReasonUndeserializable exists to protect: a burst of oversized entries becomes indistinguishable from a stalled JSON to binary migration. Before this PR the same throw came out of destination.Write(result) in TryGetAsync and hit LogSwallowedException at Warning with the exception attached, so this is a net loss.

Suggested shape, catching at the write site:

try
{
    WritePayload(remaining);
}
catch (Exception ex)
{
    // The destination writer failed (e.g. HybridCache's payload quota), which is not corrupt data.
    // Carry it out so the read core surfaces an error instead of an undeserializable miss.
    return new CacheEntryBufferReadResult(
        new CacheEntry { AbsoluteExpiration = absoluteExpiration },
        payloadWritten: false,
        absolutelyExpired: false,
        destinationFailure: ex);
}

and in the core, before the PayloadWritten check:

if (result.DestinationFailure is { } failure)
{
    // Reaches TryGetAsync's catch: scope.SetError, Warning with the exception, and false to the caller.
    ExceptionDispatchInfo.Capture(failure).Throw();
}

Catching here rather than inspecting kvEntry.Error in the core avoids depending on NATS's error-wrapping behaviour, which is the layer that hides this in the first place, and keeps corrupt framing on the miss path. A blanket throw kvEntry.Error would also throw on NatsHeaderParseException, making GetAsync fail the caller's operation on a corrupt entry rather than degrading to a miss.

This is also a telemetry fix relative to main, where the scope completes as hit inside GetAndRefreshAsync before destination.Write runs, so a quota failure records a hit while the caller gets false.

One doc point to fold in: for a multi-segment payload, segments written before the throw stay committed, so the buffer holds a partial write on a false return. Harmless for HybridCache, which discards the writer, but the "nothing written on a miss" contract should say the buffer state is unspecified when the destination write fails.

Optional

1. The deserializer re-implements IsAbsolutelyExpired instead of calling it, and the XML doc asserts the two agree. TimeProviderExpirationUnitTests pins that predicate, including the exact-instant boundary, for the NatsCache copy only. A shared static over CacheEntry would keep them from drifting.
2. WritesNothingOnAbsolutelyExpiredEntry and SlidingEntryPastAbsoluteExpirationWritesNothing use ~1-2s expirations with Task.Delay polling, so NATS is likely to reap the key first and the read lands on NotFound. Both assertions pass either way, so the intended branch may never run. ExpiredEntryReadTagsMissReasonExpiredAndRecordsNoRemoveOperation has the deterministic recipe: long real TTL plus FakeTimeProvider.Advance, and asserting the miss reason pins the branch. Also drops ~8s of sleeping.
3. No unit tests for BufferWritingCacheEntryDeserializer. The multi-segment branch of WritePayload is almost certainly never executed, since integration payloads are single-segment; CacheEntryBinarySerializerTests.Deserialize_MultiSegmentSequence_RoundTrips has the mirror worth copying, along with bad-header-returns-null and the expiry boundary. No server needed.
4. No telemetry coverage for the new core. This adds a second instrumented read path and the existing suite drives miss reasons entirely through GetAsync.
5. Both cores ignore kvEntry.Error for logging. A corrupt-headers entry and a legacy JSON envelope produce the identical Debug line; including the error separates them.
6. Roughly 50 lines of scope, expiry, eviction and revision-conflict handling now exist twice, kept in sync by comments. Worth a look at whether the array path can run over the buffer path with a pooled writer.
7. CacheEntryBufferReadResult's two bools make payloadWritten && absolutelyExpired representable, and it allocates a CacheEntry that is discarded on both the fast and expired paths. An outcome enum plus a nullable entry removes both.
8. The IsSingleSegment branch in WritePayload is redundant; the foreach covers it and the enumerator is a struct.
9. Six -> arrows are written as U+2192 in the new file; src/ currently has none.
10. The "nothing written on a miss" contract is stated in the file header, again in <remarks>, again per branch, and again in NatsCache. One canonical statement plus references would read better.
11. TryGetAsync still calls this "a zero-copy detail" while the PR text carefully argues single-copy.
12. LegacyJsonEntry and WriteRawEntryAsync are now in a third test file; candidate for TestBase.

[matthewdevenny]

Thanks for the thorough review. Addressed in b902d12.

Required fix — destination-write failures no longer read as undeserializable misses. The writer exception is now caught at the write site in the deserializer and carried out as a DestinationFailure outcome; the read core rethrows it (via ExceptionDispatchInfo) so TryGetAsync records an error and logs a Warning with the exception, as on main — never a Debug undeserializable miss. Also fixes the telemetry gap you noted (main completed the scope as hit before the write). Covered by a telemetry test (error, not miss; misses empty; Warning logged), a behavioral buffer test, and a deserializer unit test. The <remarks> now states the buffer contents are unspecified when a multi-segment write fails.

Optional items — all addressed:

1. Shared IsAbsolutelyExpired static so the two paths can't drift.
2. Absolute-expiry tests are now deterministic (FakeTimeProvider + advance), which also drops ~8s of sleeping.
3. Added CacheEntryReadDeserializerTests (multi-segment, bad-header→null, the exact-instant boundary, sliding, destination-failure).
4. Added buffer-path telemetry coverage (destination-failure → error; expired → miss reason).
5. Undeserializable log now includes kvEntry.Error, separating corrupt framing from a legacy envelope.
6. Unified the array and buffer paths into one GetAndRefreshAsync — removes the duplicated ~50 lines (net −92 across the PR). The array path materializes via the canonical deserializer; the buffer fast path streams.
7. Replaced the two bools with a CacheEntryReadOutcome enum; hit/expired paths now allocate nothing beyond the payload.
8. Dropped the redundant IsSingleSegment branch.
9. ASCII arrows.
   10/11. Consolidated the "nothing written on a miss" doc; fixed the "zero-copy" → "single-copy" wording.
10. Moved LegacyJsonEntry/WriteRawEntryAsync to TestBase.

118 unit + 88 integration tests pass; build clean on net8.0/net10.0 under TreatWarningsAsErrors.

[mtmk]

LGTM