test: pin Snappy codec reuse contract ahead of #2352; document upstream allocation blocker

[thomhurst]

Summary

Investigated #2214 ("last zero-alloc leftovers") to implementation. Findings changed the shape of the work:

#2214 item	Status
Pool DetachableBufferWriter in PreCompress	Already shipped on main — PR #2416 (2026-07-27) added the Rent + [ThreadStatic] single-slot wrapper cache; producer pre-serialization measured 40 B → 0 B there. Nothing left to do.
Reuse Snappier compressor/decompressor instances	Blocked upstream. Snappier 1.3.1 keeps SnappyCompressor/SnappyDecompressor internal; every public static entry point allocates a fresh driver per call (~48 B compress / ~80 B decompress). #2352 rules out reflection and vendoring (Native AOT + trimming). Upstream brantburnett/Snappier#148 is still open — the swap needs it merged and released, then a package bump.

So no product perf change is possible right now. This PR lands what is useful while blocked:

• Source comment in SnappyCompressionCodec classifying the residual allocations as per-block (per-batch) costs with the upstream blocker, so future zero-alloc audits don't misdiagnose them or "fix" them the forbidden way.
• Contract-pinning tests the eventual deps(snappy): expose reusable block codec lifecycle upstream #2352 swap must keep green:
  • Compress_ReusedCodec_ProducesIdenticalOutputToFreshInstance — thread-cached scratch never leaks across compress/decompress cycles.
  • CompressDecompress_ParallelWorkers_DoNotCorruptEachOther — 8 workers × 50 batches through one shared codec, single- and multi-segment inputs; catches ownership bugs if thread-local caches are ever converted to a shared pool incorrectly.
  • Rent_WhileOutstanding_ReturnsIndependentWriter — pins DetachableBufferWriter's documented single-slot cache invariant from Cache detachable record-batch writers #2416.

Reviewed via /simplify (4 agents): segment-building now delegates to the shared SequenceTestHelpers, writers pre-sized, comment trimmed to durable facts. Efficiency review specifically verified the parallel test cannot perturb the pool-warmth-sensitive TransactionalProduceAllocationTests gate (NotInParallel + disjoint pool buckets + its windowed design).

Test plan

• dotnet build Dekaf.sln -c Release -p:TreatWarningsAsErrors=true — 0 warnings/errors
• Full unit suite green; new tests pass (Snappy class 20/20, DetachableBufferWriter class 3/3)

No behavioral src/ change (comment only) — no benchmark numbers required. Suggest keeping #2214 open (or re-scoping it onto #2352) until upstream ships.

[greptile-apps]

Greptile Summary

The PR documents the upstream blocker preventing reusable Snappy drivers and adds contract tests for codec reuse, parallel isolation, and nested buffer-writer rentals.

• Exercises reused Snappy compression with a segment boundary deliberately placed inside a compression block.
• Verifies parallel compression and decompression through one codec instance.
• Verifies that simultaneous buffer-writer rentals remain independent.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported aligned-split coverage gap is fixed by placing the segment boundary inside a compression block.

Important Files Changed

Filename	Overview
src/Dekaf.Compression.Snappy/SnappyCompressionCodec.cs	Adds a documentation-only note explaining the remaining per-block allocations and upstream reuse blocker.
tests/Dekaf.Tests.Unit/Compression/SnappyCompressionCodecTests.cs	Adds codec-reuse and parallel-isolation tests; the revised off-boundary split now exercises the intended multi-segment scratch-buffer path.
tests/Dekaf.Tests.Unit/Protocol/DetachableBufferWriterTests.cs	Adds coverage confirming that a nested rental receives an independent writer while another rental is outstanding.

_{Reviews (2): Last reviewed commit: "test: straddle block boundary in Snappy ..." | Re-trigger Greptile}

[claude]

Review Summary

Reviewed against CLAUDE.md's rules, in particular the zero-allocation hot-path gate (Rule 1) and the no-benchmarks-needed-for-non-behavioral-changes carve-out. This PR is test/comment-only — verified the src/ diff is a 6-line comment addition to SnappyCompressionCodec with no logic change, so the "no behavioral change, no benchmark numbers required" claim in the PR description checks out.

Verification performed:

• Built tests/Dekaf.Tests.Unit in Release from the PR branch — 0 warnings, 0 errors.
• Ran the affected test classes (SnappyCompressionCodecTests, DetachableBufferWriterTests) — all 23 tests pass, including the 3 new ones, in 608ms (no flakiness observed).
• Read DetachableBufferWriter (src/Dekaf/Protocol/Records/RecordBatch.cs) to confirm the new Rent_WhileOutstanding_ReturnsIndependentWriter test actually exercises the documented single-slot [ThreadStatic] cache invariant it claims to pin — it does: two back-to-back Rent calls without an intervening Dispose are guaranteed to return distinct instances regardless of what was left in the thread-local cache slot by a prior test, so the assertion holds independent of test execution order.
• Read the SnappyCompressionCodec thread-local scratch fields (t_compressedBuffer, t_decompressionDestination) to confirm the new reuse/parallel tests target real state that could leak across calls — they do, and the tests correctly avoid throwing inside Parallel.For workers by aggregating failures via Interlocked.Increment and asserting once after the loop.
• No prior review comments/reviews existed on this PR to reconcile against.

Findings: None blocking. One trivial nit, not worth blocking on:

• SnappyCompressionCodecTests.cs: in Compress_ReusedCodec_ProducesIdenticalOutputToFreshInstance, reusedOutput is initialized once before the for loop and then unconditionally reassigned as the first statement inside the loop — the pre-loop initialization is dead. Harmless (test-only, not a hot path Rule 1 concerns), but ArrayBufferWriter<byte> reusedOutput = null!; before the loop would avoid the redundant allocation-then-discard.

Design/architecture: The comment correctly classifies the Snappier allocation as per-block (≈per-batch given default 1 MiB block size aligning with the project's ~1 MiB default batch size), consistent with the project's per-message-vs-per-batch cost framing in CLAUDE.md. The new tests are well-targeted contract-pinning tests for the eventual #2352 swap rather than incidental coverage, and correctly avoid asserting anything about the (currently non-zero, upstream-blocked) allocation counts themselves.

Clean, well-scoped PR. No correctness, security, or coverage issues found.

[claude]

Review Summary

Test/comment-only PR (per CLAUDE.md Rule 1's no-behavioral-change carve-out — verified the src/ diff is a 6-line comment addition to SnappyCompressionCodec with zero logic change, so the "no benchmark numbers required" claim holds).

Verification performed:

• Built tests/Dekaf.Tests.Unit in Release — 0 warnings, 0 errors.
• Ran SnappyCompressionCodecTests + DetachableBufferWriterTests — 23/23 pass in 698ms.
• Hand-traced the Compress_ReusedCodec_ProducesIdenticalOutputToFreshInstance block/segment math: with blockSize=1024 and the new straddlingSplit = payload.Length/2 + 1 = 4097, segment 1 covers bytes [0,4097) and segment 2 covers [4097,8192). Block 4 ([4096,5120)) starts inside segment 1 and ends inside segment 2, so blockSequence.IsSingleSegment is false for that block and Compress takes the t_compressedBuffer multi-segment path the test exists to pin. All other blocks are fully within one segment. This confirms the fix is correct, not just plausible.
• Confirmed against prior review history: this PR already incorporates fixes for both issues raised in earlier review rounds on this same PR —
  • Greptile's finding that the original 4096-byte aligned split let every block sit inside one segment, so the reuse test never touched the multi-segment scratch buffer it claimed to cover — fixed by the d697d5f commit's 4097 split (verified above).
  • My own prior nit about the dead pre-loop ArrayBufferWriter initialization in the same test — the same commit removed it (ArrayBufferWriter<byte> reusedOutput = null!; declared once, assigned only inside the loop).
• Read DetachableBufferWriter (src/Dekaf/Protocol/Records/RecordBatch.cs:189-273) to confirm Rent_WhileOutstanding_ReturnsIndependentWriter actually exercises the single-slot [ThreadStatic] cache: t_cachedWriter is only repopulated in Dispose(), so a nested Rent before the outer writer is disposed is guaranteed to allocate a distinct instance — the test's assertion holds regardless of execution order or prior test state.
• CompressDecompress_ParallelWorkers_DoNotCorruptEachOther correctly aggregates failures via Interlocked.Increment instead of asserting inside Parallel.For workers (avoids the anti-pattern of assertions on non-main threads being silently swallowed).

Findings: None blocking, no new issues found beyond what earlier review rounds already caught and this PR already fixed.

Design/architecture: The allocation classification in the new source comment (per-block ≈ per-batch given the 1 MiB default block size aligning with the project's ~1 MiB default batch size) is accurate per CLAUDE.md's per-message-vs-per-batch cost framing, and correctly avoids the trap of "fixing" a bounded, upstream-blocked allocation the forbidden way. The new tests are targeted contract-pinning tests for the eventual #2352 swap, not incidental coverage, and none of them assert on the (currently non-zero) allocation counts themselves, so they won't spuriously fail once #2352 lands.

Clean, well-scoped PR that has fully addressed prior review feedback. No correctness, security, or coverage issues found.

[thomhurst]

Review feedback addressed in ed17b4e.

greptile P2 — aligned split skips the scratch path: real, fixed. With an 8192-byte payload in 1024-byte blocks and the segment boundary at 4096, every block sat inside one segment, so Compress always took the blockSequence.IsSingleSegment branch and the t_compressedBuffer scratch this test claims to pin was never touched. Split moved to 4097, so the fifth block (4096–5120) straddles both segments and the multi-segment branch runs; the fresh-instance comparison uses the same split so the output comparison stays like-for-like. The local CreateMultiSegmentSequence helper now takes an optional splitAt and the reason is recorded in a comment so a future edit doesn't silently re-align it.

Claude review nit — dropped the dead pre-loop ArrayBufferWriter initialisation (unconditionally reassigned on the first iteration).

SnappyCompressionCodecTests 20/20 green; tests/Dekaf.Tests.Unit builds 0 warnings with TreatWarningsAsErrors.