Skip to content

[Reader] fix: deliver null-value tombstones instead of discarding them#1482

Merged
nodece merged 3 commits intoapache:masterfrom
bckground:reader-null-value-tombstone
Apr 28, 2026
Merged

[Reader] fix: deliver null-value tombstones instead of discarding them#1482
nodece merged 3 commits intoapache:masterfrom
bckground:reader-null-value-tombstone

Conversation

@jcmfernandes
Copy link
Copy Markdown
Contributor

Motivation

A message published with MessageMetadata.null_value = true (the Pulsar compaction tombstone convention) was conflated with a deserialization failure in partitionConsumer.MessageReceived and silently discarded. Because lastDequeuedMsg was never advanced past the tombstone, hasMoreMessages kept returning true and Reader.Next blocked forever when a tombstone was the last message on a topic.

Modifications

Consumer: when the reader yields an empty payload and message metadata or the single-message metadata has null_value set, build a normal message with payLoad == nil and take the usual dispatch path so lastDequeuedMsg advances. Real corruption still routes through discardCorruptedMessage.

Producer: set MessageMetadata.null_value/SingleMessageMetadata.null_value when both Value and Payload are nil, matching the Java client so Go-produced tombstones carry the flag consumers need.

Message gains an IsNullValue() bool accessor so applications can tell tombstones apart from empty payloads.

Note: This PR was written with the assistance of AI Anthropic’s Opus 4.7.

Verifying this change

This change added tests and can be verified as follows:

  • Added reader test

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no) (added IsNullValue() to the Message interface)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented): N/A.
  • If a feature is not applicable for documentation, explain why? N/A.
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation. N/A.

A message published with MessageMetadata.null_value = true (the Pulsar
compaction tombstone convention) was conflated with a deserialization
failure in partitionConsumer.MessageReceived and silently discarded.
Because lastDequeuedMsg was never advanced past the tombstone,
hasMoreMessages kept returning true and Reader.Next blocked forever
when a tombstone was the last message on a topic.

Consumer: when the reader yields an empty payload and msgMeta or the
single-message metadata has null_value set, build a normal message with
payLoad == nil and take the usual dispatch path so lastDequeuedMsg
advances. Real corruption still routes through discardCorruptedMessage.

Producer: set MessageMetadata.null_value / SingleMessageMetadata.null_value
when both Value and Payload are nil, matching the Java client so
Go-produced tombstones carry the flag consumers need.

Message gains an IsNullValue() bool accessor so applications can tell
tombstones apart from empty payloads.
@jcmfernandes jcmfernandes changed the title [pulsar-client-go/Reader] fix: deliver null-value tombstones instead of discarding them [Reader] fix: deliver null-value tombstones instead of discarding them Apr 20, 2026
@lhotari lhotari requested review from Copilot and crossoverJie April 21, 2026 21:21
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

Fixes reader/consumer handling of Pulsar compaction tombstones by delivering null-value messages (instead of discarding them) and ensuring producers correctly mark tombstones on the wire, with a new Message.IsNullValue() accessor for applications.

Changes:

  • Consumer: treat null_value messages as valid even when ReadMessage() returns internal.ErrEOM, ensuring dispatch progresses past tombstones.
  • Producer: set MessageMetadata.null_value / SingleMessageMetadata.null_value when both Value and Payload are nil.
  • Public API: add IsNullValue() to the Message interface and add a reader test covering tombstone delivery.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pulsar/consumer_partition.go Accept and dispatch tombstone messages (null-value) instead of discarding them as corrupted.
pulsar/producer_partition.go Emit protocol null_value flags for nil Value + nil Payload messages (single and batched).
pulsar/message.go Adds IsNullValue() to the Message interface with explanatory doc comment.
pulsar/impl_message.go Implements IsNullValue() on the concrete message type and stores a new isNullValue field.
pulsar/reader_test.go Adds a reader test ensuring tombstones are delivered and HasNext() terminates.
pulsar/negative_acks_tracker_test.go Updates test mocks to satisfy the extended Message interface.
pulsar/internal/pulsartracing/message_carrier_util_test.go Updates tracing test mock to satisfy the extended Message interface.

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

Comment thread pulsar/consumer_partition.go
Copy link
Copy Markdown
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

Quoting Copilot's feedback:

When accepting a tombstone with err == internal.ErrEOM, the code keeps err
non-nil and continues. It works today because err is not used later, but it’s
easy to misread and could become a latent bug if future logic inspects/returns
err after this block. Consider explicitly setting err = nil in the
tombstone/ErrEOM acceptance path to make the intent unambiguous.
@nodece
Copy link
Copy Markdown
Member

nodece commented Apr 27, 2026

@jcmfernandes Please fix lint.

@jcmfernandes
Copy link
Copy Markdown
Contributor Author

@jcmfernandes Please fix lint.

Done.

@nodece nodece merged commit fe8aaaf into apache:master Apr 28, 2026
7 checks passed
@nodece
Copy link
Copy Markdown
Member

nodece commented Apr 28, 2026

Good work @jcmfernandes

@jcmfernandes
Copy link
Copy Markdown
Contributor Author

Thank you very much, @nodece! And thanks for helping me get this merged.

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