Skip to content

Refactor TransactionManager key tracking to use SessionParseState and ScratchBufferAllocator#1638

Merged
vazois merged 20 commits intodevfrom
talzacc/key_mgmt
Mar 23, 2026
Merged

Refactor TransactionManager key tracking to use SessionParseState and ScratchBufferAllocator#1638
vazois merged 20 commits intodevfrom
talzacc/key_mgmt

Conversation

@TalZaccai
Copy link
Copy Markdown
Contributor

@TalZaccai TalZaccai commented Mar 18, 2026

This PR replaces raw PinnedSpanByte arrays and manual pointer management in transaction cluster slot verification and WATCH key storage with SessionParseState and ScratchBufferAllocator.

Problem:

  • Cluster key tracking: TransactionManager stored keys for slot verification as raw PinnedSpanByte pointers into the network receive buffer. Because the receive buffer can be reallocated between MULTI and EXEC, a fragile UpdateRecvBufferPtr method was needed to walk all stored pointers and rebase them — prone to dangling pointer bugs.
  • Inconsistent slot verification: EXEC used NetworkKeyArraySlotVerify (iterating Span<PinnedSpanByte>), while all other multi-key slot verification paths already use the SessionParseState-based NetworkMultiKeySlotVerify overload.
  • Watch key storage: WatchedKeysContainer managed its own raw byte[] buffer with manual pointer arithmetic and pointer fixup on resize — duplicating logic that ScratchBufferAllocator already provides.
  • Missing watchContainer.Reset() in DISCARD and aborted EXEC paths left watched key slices pointing into invalidated scratch buffer memory (use-after-reset).

Changes introduced:

  • Cluster key tracking:
    • Replaced PinnedSpanByte[] keys + keyCount with a SessionParseState clusterKeyParseState on TransactionManager
    • Keys are stored as raw pointers into the receive buffer (zero-copy fast path); only copied into a dedicated ScratchBufferAllocator (txnScratchBuffer) lazily when a receive buffer reallocation is detected
    • Eliminated UpdateRecvBufferPtr; replaced with CopyExistingKeysToScratchBuffer triggered by saveKeyRecvBufferPtr change detection
    • GetSlotVerificationInput builds ClusterSlotVerificationInput directly; EXEC uses the standard NetworkMultiKeySlotVerify(ref SessionParseState, ...) path
    • Added Capacity property to SessionParseState to support doubling growth
    • Added guard to skip slot verification when clusterKeyParseState.Count is 0 (e.g., MULTI/EXEC with only non-key commands)
  • Watch key storage:
    • Replaced manual byte[] buffer + raw pointer arithmetic + pointer fixup on resize with a shared ScratchBufferAllocator (txnScratchBuffer)
    • Removed 5 fields: watchBuffer, watchBufferPtr, watchBufferHeadAddress, watchBufferSize, initialWatchBufferSize
    • Added watchContainer.Reset() to DISCARD and aborted EXEC paths to prevent use-after-reset of watched key slices
  • Cleanup:
    • Removed unused NetworkKeyArraySlotVerify from RespServerSession, IClusterSession, and ClusterSession, along with its Span<PinnedSpanByte> MultiKeySlotVerify overload

@TalZaccai TalZaccai marked this pull request as ready for review March 19, 2026 05:39
Copilot AI review requested due to automatic review settings March 19, 2026 05:39
@TalZaccai TalZaccai changed the title [WIP] Refactor TransactionManager key tracking to use SessionParseState and ScratchBufferAllocator Refactor TransactionManager key tracking to use SessionParseState and ScratchBufferAllocator Mar 19, 2026
@TalZaccai TalZaccai requested review from badrishc and vazois March 19, 2026 05:40
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors transaction cluster-slot key tracking and WATCH key storage to avoid keeping raw pointers into the network receive buffer by copying key bytes into scratch buffers and reusing the SessionParseState key-processing path.

Changes:

  • Replaced transaction cluster key tracking from PinnedSpanByte[] + pointer rebasing to SessionParseState backed by a dedicated ScratchBufferAllocator.
  • Refactored WatchedKeysContainer to store watched key bytes via ScratchBufferAllocator instead of a manually-managed byte[] buffer.
  • Added SessionParseState.Capacity to support dynamic growth of the argument buffer.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
libs/server/Transaction/TxnWatchedKeysContainer.cs Switch WATCH key byte storage to ScratchBufferAllocator and simplify reset/growth logic.
libs/server/Transaction/TxnRespCommands.cs Update EXEC slot verification to use NetworkMultiKeySlotVerify(ref SessionParseState, ...) with ClusterSlotVerificationInput.
libs/server/Transaction/TxnClusterSlotCheck.cs Replace raw key array tracking with SessionParseState + scratch-buffer-copied key slices.
libs/server/Transaction/TransactionManager.cs Introduce txnScratchBuffer and clusterKeyParseState; update reset and slot verification input construction.
libs/server/Resp/Parser/SessionParseState.cs Add Capacity accessor for argument buffer capacity checks.

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

You can also share your feedback on Copilot code review. Take the survey.

@vazois vazois merged commit 00ceeb5 into dev Mar 23, 2026
44 of 45 checks passed
@vazois vazois deleted the talzacc/key_mgmt branch March 23, 2026 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants