Skip to content

fix(eventhubs): reject a checkpoint from an event with no position - #5107

Open
Johnathan W (j7nw4r) wants to merge 2 commits into
Azure:mainfrom
j7nw4r:j7nw4r/fix-eventhubs-checkpoint-no-annotations
Open

fix(eventhubs): reject a checkpoint from an event with no position#5107
Johnathan W (j7nw4r) wants to merge 2 commits into
Azure:mainfrom
j7nw4r:j7nw4r/fix-eventhubs-checkpoint-no-annotations

Conversation

@j7nw4r

Copy link
Copy Markdown
Member

Summary

PartitionClient::update_checkpoint now returns an error when the event carries no offset and no sequence number. Such a call returned Ok(()) and wrote nothing before, so a caller that checked every result still ended with no stored position.

Motivation

The offset and the sequence number come from the message annotations, and two paths reached the same silent success. On the first path the annotations map was absent, and the method returned Ok(()) before it reached the store. On the second path the map was present but neither x-opt-offset nor x-opt-sequence-number resolved, because the key was missing or its AMQP value had the wrong type, and the method wrote a checkpoint with both fields empty.

The second path is the more damaging one, so the guard keys on the resolved values instead of on message_annotations.is_none(). In EventProcessor::get_start_position the else if that reads start_positions.per_partition hangs off the outer checkpoints.contains_key(partition_id) test. A checkpoint with both fields empty therefore enters the outer branch, matches neither value arm, and never reaches the per-partition lookup, so it silently overrides a per-partition start position the caller configured. BlobCheckpointStore::update_checkpoint builds the blob metadata from the checkpoint fields, so the same record calls set_metadata with an empty map, and Azure Blob Storage replaces all metadata on such a call. That erases a good checkpoint.

Commit adff670762 (#3148) replaced two error returns in this method with continue and added the early Ok(()) return.

.NET reports the same input as an error. EventProcessorClient raises InvalidOperationException with the message "A checkpoint cannot be created or updated using an empty event." Go and Java do not error on this path, so this change claims parity with .NET only.

Changes

  • Added the ErrorKind::MissingCheckpointMetadata { partition_id } variant to the Event Hubs crate's own ErrorKind, so a caller can match on the kind instead of parsing the message text and can tell this failure apart from a store failure.
  • Kept azure_core::error::ErrorKind unchanged. That enum is not #[non_exhaustive], so a new variant there would be a major semver break, and it is not on this method's return path. The Event Hubs ErrorKind is already #[non_exhaustive], so this addition is not a breaking type change.
  • Changed update_checkpoint to resolve the two values through ReceivedEventData::offset and ReceivedEventData::sequence_number, and to return the new error when both are absent. One check covers both causes.
  • Removed the hand-rolled annotation loop, which duplicated the scan those two accessors already perform, including the same AMQP value type guards.
  • Recorded the change in CHANGELOG.md under Features Added, Breaking Changes, and Bugs Fixed.

The behavior change is deliberate and observable. A call that asks to record a checkpoint for an event with no position now fails where it used to report success, which is why the changelog carries a Breaking Changes entry.

Test plan

Eight unit tests were added to partition_client.rs, and all of them run offline with no broker and no credential.

  • Three tests pin the error paths: an event with no message annotations, an event whose annotations carry neither key, and an event whose annotations carry both keys with the wrong AMQP value types. Each asserts that the store holds no checkpoint afterward.
  • One test asserts that the caller can match ErrorKind::MissingCheckpointMetadata and read the partition id from it.
  • Four tests pin the unchanged behavior: an offset-only event, a sequence-number-only event, an event with both values and all four identity fields, and a store failure that keeps its Failed to update checkpoint for partition context.

The four error tests were proved red before the fix. The three behavioral tests failed against the unchanged source, and the two that cover the second cause printed the offending record, Checkpoint { ..., offset: None, sequence_number: None }. The fourth failed to compile, because it names the new variant.

Commands run in the worktree, each exiting 0.

  • CARGO_BUILD_JOBS=1 RUSTFLAGS=-Dwarnings cargo test --no-run --package azure_messaging_eventhubs
  • CARGO_BUILD_JOBS=1 cargo test -p azure_messaging_eventhubs --all-features --lib -- --test-threads=1, giving 152 passed, 0 failed, 14 ignored, against a pre-change baseline of 144 passed
  • cargo fmt -p azure_messaging_eventhubs -- --check
  • CARGO_BUILD_JOBS=1 cargo clippy -p azure_messaging_eventhubs --all-features --all-targets -- -Dwarnings
  • CARGO_BUILD_JOBS=1 RUSTDOCFLAGS=-Dwarnings cargo doc -p azure_messaging_eventhubs --no-deps --all-features

No live test was run for this change. The only in-repo caller, tests/eventhubs_processor.rs, records checkpoints from real broker events, which always carry both annotations, so the guard does not change that path.

The blob metadata erasure itself is not covered by a new test, because proving it end to end needs a live storage account. The fix stops such a record from reaching any store, and the second error test pins that cause.

Closes #5097

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
3 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@j7nw4r Johnathan W (j7nw4r) self-assigned this Aug 20, 2026
@j7nw4r
Johnathan W (j7nw4r) force-pushed the j7nw4r/fix-eventhubs-checkpoint-no-annotations branch from bc38ce7 to e1f9b12 Compare August 25, 2026 18:13
The partition client returns Ok when an event carries no message
annotations, and it writes a checkpoint with no offset and no sequence
number when the annotations hold neither key. Both cases lose the
caller's progress without a signal.

Three tests fail against the unchanged source. A fourth does not
compile, because it matches on an ErrorKind variant that the fix adds.
Four more tests pass green. They pin the write path, the identity
fields, and the store failure context.
PartitionClient::update_checkpoint returned Ok(()) and wrote nothing
when the event had no message annotations. It also wrote a checkpoint
with no offset and no sequence number when the annotations held
neither value. Such a checkpoint names no position in the partition.
It suppressed the per-partition start position the caller configured,
because EventProcessor prefers any stored checkpoint over that
position. It also erased a good checkpoint in BlobCheckpointStore,
because the store builds the blob metadata from the checkpoint
fields, and Azure Blob Storage replaces all metadata on a
set-metadata call.

The method now reads the offset and the sequence number through the
ReceivedEventData accessors and returns the new error variant
ErrorKind::MissingCheckpointMetadata when both are absent. An event
that carries only one of the two still writes a checkpoint. This
matches the InvalidOperationException that .NET raises for the same
input.

Refs Azure#5097
@j7nw4r
Johnathan W (j7nw4r) force-pushed the j7nw4r/fix-eventhubs-checkpoint-no-annotations branch from e1f9b12 to ae44e6e Compare September 1, 2026 19:59
@j7nw4r
Johnathan W (j7nw4r) marked this pull request as ready for review September 1, 2026 20:26
Copilot AI balanced review requested due to automatic review settings September 1, 2026 20:26
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
3 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The checkpoint validation is correct and thoroughly tested; only a minor changelog concision issue remains.

Pull request overview

Prevents invalid Event Hubs checkpoints from silently discarding or erasing stored positions.

Changes:

  • Adds ErrorKind::MissingCheckpointMetadata.
  • Rejects events lacking both offset and sequence number.
  • Adds comprehensive unit tests and release notes.
File summaries
File Description
partition_client.rs Validates checkpoint position and adds tests.
error.rs Defines and formats the typed error.
CHANGELOG.md Documents the public behavior change.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

- Fixed a deadlock when a CBS failure during management-client creation started connection recovery. ([#4728](https://github.com/Azure/azure-sdk-for-rust/issues/4728))
- Closed a stale-resource window in connection recovery. A `ReconnectConnection` recovery that fired while a slow-path attach (authorize, session begin, or sender/receiver link attach) was in flight could cache a resource bound to the just-dropped connection; the next operation on that resource failed (unauthorized / detached / closed) and triggered a second, redundant recovery cycle. A recovery generation counter now tags each cached resource, and a slow path that completes across a recovery discards its result and re-attaches against the new connection instead of caching the stale one. The authorizer's token cache is mutable (a background task refreshes tokens) so it cannot use the same one-shot cell as the connection caches; both of its writers, `authorize_path` and the refresh task, instead re-check the generation under the same lock that recovery's clear takes, and a recovery brackets its invalidation with a generation bump on each side, which leaves the counter odd for as long as the recovery runs, so a slow path that overlaps a recovery at either end also discards rather than caching a resource bound to the connection that recovery is dropping. A token refresh pass that a recovery discards now applies the same backoff floor as a failed pass, so a recovery storm cannot turn the refresh loop into an uncapped stream of credential and CBS calls. The per-path / per-partition concurrency is preserved. ([#4454](https://github.com/Azure/azure-sdk-for-rust/issues/4454))
- `InMemoryCheckpointStore` now rotates the ETag and refreshes `last_modified_time` when an existing ownership is renewed, matching the create path and the production `BlobCheckpointStore`. Previously the renewal path reinserted the caller's record verbatim, leaving a stale ETag and timestamp; that divergence from the real store could mask bugs in code that relies on ETag rotation for optimistic concurrency. ([#4594](https://github.com/Azure/azure-sdk-for-rust/issues/4594))
- `PartitionClient::update_checkpoint` no longer reports success without writing a checkpoint. It returned `Ok(())` and wrote nothing when the event had no message annotations. It also wrote a checkpoint with no offset and no sequence number when the annotations held neither value. Such a checkpoint suppressed the per-partition start position the caller configured, because `EventProcessor` prefers any stored checkpoint over that position. It also erased a good checkpoint in `BlobCheckpointStore`, because the store builds the blob metadata from the checkpoint fields, and Azure Blob Storage replaces all metadata on a set-metadata call. ([#5097](https://github.com/Azure/azure-sdk-for-rust/issues/5097))
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.

[Event Hubs] PartitionClient::update_checkpoint silently succeeds when the event has no annotations

2 participants