Skip to content

[fix][client] Fix batch container buffer ownership on the failure-recovery path - #26455

Open
nodece wants to merge 1 commit into
apache:masterfrom
nodece:fix-batch-buffer-use-after-release
Open

[fix][client] Fix batch container buffer ownership on the failure-recovery path#26455
nodece wants to merge 1 commit into
apache:masterfrom
nodece:fix-batch-buffer-use-after-release

Conversation

@nodece

@nodece nodece commented Sep 3, 2026

Copy link
Copy Markdown
Member

Motivation

When compression or encryption releases the batch buffer before a flush that then fails, resetPayloadAfterFailedPublishing() could not tell whether the container still owned the buffer: retrying reused the released buffer (use-after-free), and the compressed/encrypted payload that replaced it was left orphaned (leak).

Modifications

  • BatchMessageContainerImpl: track batch-buffer ownership with an explicit batchPayloadOwned flag instead of inferring it from the buffer reference. A build only starts while the container owns its buffer; compression/encryption that releases it clears the flag, and resetPayloadAfterFailedPublishing() reallocates instead of reusing released memory. The compressed payload is released when encryption fails and the container no longer owns it (fixes the orphaned-payload leak). resetPayloadAfterFailedPublishing() also skips reallocation when the container has no messages left, since key-based batching forwards the reset to sub-batches that already succeeded and cleared.
  • RawBatchMessageContainerImpl: symmetrically release the compressed payload and any partially built encrypted buffer when encryption fails, and release the serialized payloads in toByteBuf() on failure instead of orphaning them.
  • StrategicTwoPhaseCompactor: always clear the batch container on a failed flush, including when the failure is an Error (discard(Exception) cannot take an Error).
  • Tests: cover failure recovery with/without compression, encryption-failure buffer release, and the fail-fast guard against re-entering a build without reset.

…overy path

When compression or encryption releases the batch buffer before a flush that
then fails, the old recovery reused the released buffer (use-after-free) or
orphaned the compressed payload (leak).

Track batch-buffer ownership explicitly instead of nulling the buffer inside
the build method:

- BatchMessageContainerImpl: add a batchPayloadOwned flag; a build only starts
  while the container owns its buffer. Compression/encryption that releases it
  clears the flag, and resetPayloadAfterFailedPublishing() reallocates instead
  of reusing released memory. Releasing the compressed payload when encryption
  fails fixes the orphaned-payload leak.
- resetPayloadAfterFailedPublishing(): skip reallocation when the container has
  no messages left, since key-based batching forwards the reset to sub-batches
  that already succeeded and cleared.
- RawBatchMessageContainerImpl: symmetrically release the compressed payload
  and any partially built encrypted buffer when encryption fails, and release
  the serialized payloads in toByteBuf() on failure instead of orphaning them.
- StrategicTwoPhaseCompactor: always clear the batch container on a failed
  flush, including when the failure is an Error (discard(Exception) cannot take
  an Error).
- Tests: cover failure recovery with/without compression, encryption-failure
  buffer release, and the fail-fast guard against re-entering a build without
  reset.

Assisted-by: Claude Code
Assisted-by: Codex
@void-ptr974

Copy link
Copy Markdown
Contributor

One additional case worth considering is partial success during multi-batch construction. For example, sub-batch A successfully creates an OpSendMsg, but sub-batch B fails while constructing its command. In this situation, A’s operation becomes unreachable and its ByteBufPair is not released.

Repeated retries may retain additional command buffers and cause direct-memory growth or eventually OOM. With CompressionType.NONE, the abandoned command may also reference the payload reused during retry, resulting in inconsistent buffer ownership.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants