feat(server): Add integration test for message deduplicatpr by tomighita · Pull Request #2879 · apache/iggy

tomighita · 2026-03-05T16:14:11Z

Which issue does this PR close?

Rationale

As stated in the issue description, the message deduplicator was lacking integration tests. This change adds one based on existing test harness.

What changed?

The message deduplicator is only tested through unit tests, which means it lacks end to end verification. This PR adds one integration test, covering common scenarios like:

Add messages with new ids
Add messages with existing ids
Add messages where some of the ids exist
Add messages after the TTL expired and ensure we can insert duplicate ids

This change also fixed two minor bugs where sending a batch with all ids duplicated panicked and one where the deduplicator was not instantiated and None was always set.

Local Execution

Passed
Pre-commit hooks ran

AI Usage

Which tools: copilot cli
Scope of usage: repo onboarding, explaining concepts and architecture and generating test scaffold
How did you verify the generated code works correctly: ran the test
Can you explain every line of the code if asked: yes

tomighita · 2026-03-05T16:15:22Z

core/server/src/shard/system/messages.rs

+        if batch.count() == 0 {
+            return Ok(());
+        }
+


[note] This was causing a bug when all the messages were with duplicate IDs because the batch was empty and we unwraped on first

tomighita · 2026-03-05T16:15:45Z

core/server/src/shard/system/partitions.rs

+        let message_deduplicator = create_message_deduplicator(&self.config.system).map(Arc::new);
+
        let partition = LocalPartition::with_log(
            loaded_log,
            stats,
            std::sync::Arc::new(std::sync::atomic::AtomicU64::new(current_offset)),
            consumer_offsets,
            consumer_group_offsets,
-            None,
+            message_deduplicator,


[note] I've added this here since the message_deduplicator was always None

codecov · 2026-03-05T18:03:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.42%. Comparing base (6735eea) to head (1111a99).
⚠️ Report is 15 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2879      +/-   ##
============================================
+ Coverage     68.33%   68.42%   +0.09%     
  Complexity      739      739              
============================================
  Files          1052     1052              
  Lines         84745    84749       +4     
  Branches      61281    61293      +12     
============================================
+ Hits          57910    57992      +82     
+ Misses        24463    24376      -87     
- Partials       2372     2381       +9

Flag	Coverage Δ
csharp	`67.43% <ø> (-0.19%)`	⬇️
go	`6.27% <ø> (ø)`
java	`54.83% <ø> (ø)`
node	`92.26% <ø> (-0.03%)`	⬇️
python	`81.57% <ø> (ø)`
rust	`70.18% <100.00%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
core/server/src/shard/system/messages.rs	`89.25% <100.00%> (+0.07%)`	⬆️
core/server/src/shard/system/partitions.rs	`84.61% <100.00%> (+0.06%)`	⬆️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

atharvalade

I found no critical issues in this PR

atharvalade

lgtm

hubcio

overall, good direction - please just test more edge-cases, not happy paths :) also please fix typo in the PR title

hubcio · 2026-03-09T19:18:16Z

core/integration/tests/server/scenarios/message_deduplication_scenario.rs

you never test id 0 (auto-generated ids). the server replaces 0 with a uuid before the dedup check (messages_batch_mut.rs:164-166), so sending multiple messages with id 0 should all pass through. without this, you could silently break the default send path and never know. after that, please also poll the messages to check whether they have unique message id.

hubcio · 2026-03-09T19:20:55Z

core/server/src/shard/system/messages.rs

+        if batch.count() == 0 {
+            return Ok(());
+        }


i get that it's a fix, but you didn't test it. nothing in this test actually exercises it. you'd need a step that sends a batch where every message is a duplicate (e.g. resend ids 1-10 a third time without any new ids mixed in) and verify the server doesn't blow up and the message count stays the same.

Isn’t this exactly what the step does where all ids are overlapping?

hubcio · 2026-03-09T19:21:27Z

core/integration/tests/server/scenarios/message_deduplication_scenario.rs

+    let polled = poll_all(&client, MESSAGES_PER_BATCH * 4).await;
+    assert_eq!(
+        polled.messages.len() as u32,
+        total_after_step3,
+        "Only new IDs from the mixed batch should have been accepted"
+    );


steps 3 and 4 only check message counts but never verify payloads. step 1 does this correctly (checks for "original" prefix), but step 3 doesn't verify the 5 surviving messages are actually the new ones (ids 21-25 with "mixed" payload) andnot some other combination. same for step 4 -- should check "after-ttl" payloads. count-only assertions can pass even if the wrong messages are kept.

hubcio · 2026-03-09T19:21:59Z

core/integration/tests/server/scenarios/message_deduplication_scenario.rs

+    let polled = poll_all(&client, MESSAGES_PER_BATCH * 2).await;
+    assert_eq!(
+        polled.messages.len() as u32,
+        MESSAGES_PER_BATCH,
+        "Duplicate messages should have been dropped"
+    );


after dedup removes messages mid-batch, offsets and indexes get rebuilt via remove_messages(). you never verify that the polled messages actually have correct, contiguous offsets. a bug in the offset recalculation would be a silent data corruption issue and this test wouldn't catch it.

hubcio · 2026-03-09T19:23:12Z

core/integration/tests/server/scenarios/message_deduplication_scenario.rs

+    let overlap_id_start = 16;
+    let total_after_step3 = MESSAGES_PER_BATCH + overlap_id_start - 1;
+    // Ensure the `overlap_id_start` is between two batches.
+    assert!(overlap_id_start > MESSAGES_PER_BATCH && overlap_id_start < 2 * MESSAGES_PER_BATCH);


overlap_id_start = 16 is a magic number and the formula total_after_step3 = MESSAGES_PER_BATCH + overlap_id_start - 1 only works because ids happen to be contiguous from 1. consider deriving it from the actual unique id range, e.g. let new_ids_in_mixed = (overlap_id_start + MESSAGES_PER_BATCH) - (2 * MESSAGES_PER_BATCH + 1) or just computing the set difference explicitly. also build_messages uses 0-based i in the payload instead of the actual message id, which makes correlating payloads to ids harder than it needs to be.

tomighita added 2 commits March 5, 2026 16:06

Add integration test and fix small edge cases

1a1cddd

Update test and linted files

e8ea671

tomighita commented Mar 5, 2026

View reviewed changes

tomighita changed the title ~~Add integration test for message deduplicatpr~~ feat(server): Add integration test for message deduplicatpr Mar 5, 2026

Trigger Build

1111a99

atharvalade reviewed Mar 7, 2026

View reviewed changes

atharvalade approved these changes Mar 7, 2026

View reviewed changes

hubcio requested changes Mar 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): Add integration test for message deduplicatpr#2879

feat(server): Add integration test for message deduplicatpr#2879
tomighita wants to merge 3 commits intoapache:masterfrom
tomighita:tomighita/feat/message-dedup-int-test

tomighita commented Mar 5, 2026 •

edited

Loading

Uh oh!

tomighita Mar 5, 2026

Uh oh!

tomighita Mar 5, 2026

Uh oh!

codecov bot commented Mar 5, 2026 •

edited

Loading

Uh oh!

atharvalade left a comment

Uh oh!

atharvalade left a comment

Uh oh!

hubcio left a comment •

edited

Loading

Uh oh!

hubcio Mar 9, 2026

Uh oh!

hubcio Mar 9, 2026

Uh oh!

tomighita Mar 9, 2026

Uh oh!

hubcio Mar 9, 2026

Uh oh!

hubcio Mar 9, 2026

Uh oh!

hubcio Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

tomighita commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale

What changed?

Local Execution

AI Usage

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

atharvalade left a comment

Choose a reason for hiding this comment

Uh oh!

atharvalade left a comment

Choose a reason for hiding this comment

Uh oh!

hubcio left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomighita commented Mar 5, 2026 •

edited

Loading

codecov bot commented Mar 5, 2026 •

edited

Loading

hubcio left a comment •

edited

Loading