Fix case where messages can be written multiple times in Iceberg by ballard26 · Pull Request #29251 · redpanda-data/redpanda

ballard26 · 2026-01-14T06:05:31Z

Callers of serde_parquet_writer::add_data_struct assume that the data struct wasn't successfully written to the file if writer_error::ok isn't returned. However, it's possible for _writer.write_row(...) to be successful then have some disk/memory reservation error. Prior to this PR these errors would result in the message being written twice as the callers would assume that the first write had failed.

Backports Required

Release Notes

Bug Fixes

Fixes duplicate message writes in Iceberg when disk or memory reservation errors occur.

Copilot

Pull request overview

This PR fixes a bug where Iceberg messages could be written multiple times when disk or memory reservation errors occur after a successful row write. The issue arose because serde_parquet_writer::add_data_struct would return an error after successfully writing a row, causing callers to retry the write.

Changes:

Modified error handling to defer reporting of post-write reservation errors to the next call
Added an _error field to track deferred errors
Added test infrastructure to inject OOM conditions for validation

Reviewed changes

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

File	Description
`src/v/datalake/serde_parquet_writer.h`	Added `_error` field to store deferred errors
`src/v/datalake/serde_parquet_writer.cc`	Updated error handling to defer post-write errors and return success when row write succeeds
`src/v/datalake/tests/test_data_writer.h`	Added OOM injection capability to `noop_mem_tracker` for testing
`src/v/datalake/tests/serde_parquet_writer_test.cc`	Added test case validating correct behavior when OOM occurs after row write

Copilot · 2026-01-14T20:51:01Z

+    if (_error != writer_error::ok) {
+        co_return std::exchange(_error, writer_error::ok);
+    }


There's a potential race condition if add_data_struct can be called concurrently. The check-and-exchange pattern on _error is not atomic, which could lead to lost errors or multiple coroutines seeing the same error. Consider adding synchronization or documenting that this method must not be called concurrently.

It can never be called concurrently from different threads and therefore there is no need for std::exchange to be atomic.

do we need to std::exchange()? Just retain the _error state once it is set?

The local_parquet_file_writer has a similar mechanism that prevents further writes once the serde_parquet_writer returns an error once. So I think the behavior will be the same whether or not we clear the error in serde_parquet_writer, at least for any user of local_parquet_file_writer. Will remove the exchange.

Copilot · 2026-01-14T20:51:01Z

+    // Hence, the current solution is to return those errors on the subsequent
+    // call to `add_data_struct`.


The comment should clarify what happens if the error is never consumed (e.g., if add_data_struct is not called again before finish()). Document whether finish() checks _error or if the caller needs to ensure one final call to add_data_struct to retrieve deferred errors.

Suggested change

// Hence, the current solution is to return those errors on the subsequent

// call to `add_data_struct`.

// Hence, the current solution is to defer those errors by storing them in

// `_error` and returning them on the subsequent call to `add_data_struct`.

// Callers must ensure that any deferred error is observed (either via a

// follow-up call to `add_data_struct` or by the finalization path, e.g.

// `finish()` inspecting `_error`) before treating the writer as

// successfully completed.

vbotbuildovich · 2026-01-15T00:18:43Z

Retry command for Build#79055

please wait until all jobs are finished before running the slash command

/ci-repeat 1
skip-redpanda-build
skip-units
skip-rebase
tests/rptest/tests/cluster_linking_e2e_test.py::ShadowLinkingMetricsTests.test_link_metrics

vbotbuildovich · 2026-01-15T00:25:36Z

CI test results

test results on build#79055

test_class	test_method	test_arguments	test_kind	job_url	test_status	passed	reason	test_history
ShadowLinkingMetricsTests	test_link_metrics	null	integration	https://buildkite.com/redpanda/redpanda/builds/79055#019bbebc-b592-475a-b597-715a62dbcf88	FLAKY	15/21	Test FAILS after retries.Significant increase in flaky rate(baseline=0.0307, p0=0.0003, reject_threshold=0.0100)	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=ShadowLinkingMetricsTests&test_method=test_link_metrics

test results on build#79607

test_class	test_method	test_arguments	test_kind	job_url	test_status	passed	reason	test_history
TestReadReplicaService	test_identical_lwms_after_delete_records	{"cloud_storage_type": 1, "partition_count": 5}	integration	https://buildkite.com/redpanda/redpanda/builds/79607#019bf864-3212-42ed-8fa7-c7aceede37bd	FLAKY	10/11	Test PASSES after retries.No significant increase in flaky rate(baseline=0.0057, p0=1.0000, reject_threshold=0.0100. adj_baseline=0.1000, p1=0.3487, trust_threshold=0.5000)	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=TestReadReplicaService&test_method=test_identical_lwms_after_delete_records

ballard26 · 2026-01-15T18:41:16Z

/ci-repeat 1
skip-redpanda-build
skip-units
skip-rebase
tests/rptest/tests/cluster_linking_e2e_test.py::ShadowLinkingMetricsTests.test_link_metrics

bharathv

lgtm just one minor question.

bharathv · 2026-01-15T22:36:10Z

+    if (_error != writer_error::ok) {
+        co_return std::exchange(_error, writer_error::ok);
+    }


do we need to std::exchange()? Just retain the _error state once it is set?

Callers of `serde_parquet_writer::add_data_struct` assume that the data struct wasn't successfully written to the file if `writer_error::ok` isn't returned. However, it's possible for `_writer.write_row(...)` to be successful then have some disk/memory reservation error. Prior to this commit these errors would result in the message being written twice as the callers would assume that the first write had failed.

In a previous commit `serde_parquet_writer` was changed to always return an `ok` result when the record is written. This resulted in the flush at the translation_task level to no longer return a no data error. The unit test is therefore modified in this commit to reflect the new behavior.

vbotbuildovich · 2026-01-26T15:57:14Z

/backport v25.3.x

vbotbuildovich · 2026-01-26T15:57:15Z

/backport v25.2.x

vbotbuildovich · 2026-01-26T15:57:16Z

/backport v25.1.x

vbotbuildovich · 2026-01-26T15:58:30Z

Failed to create a backport PR to v25.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-29251-v25.1.x-724 remotes/upstream/v25.1.x
git cherry-pick -x c7c8c5c2e2 efc3a22e26 7f506e3c94

Workflow run logs.

vbotbuildovich · 2026-01-26T15:58:32Z

Failed to create a backport PR to v25.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-29251-v25.2.x-899 remotes/upstream/v25.2.x
git cherry-pick -x c7c8c5c2e2 efc3a22e26 7f506e3c94

Workflow run logs.

ballard26 requested review from andrwng and bharathv January 14, 2026 06:05

github-actions Bot added the area/redpanda label Jan 14, 2026

ballard26 changed the title ~~Prevent messages from being written multiple times in Iceberg when reservation errors occur~~ Fix case where messages can be written multiple times in Iceberg Jan 14, 2026

ballard26 force-pushed the datalake-dup-msg-writes branch from f3491fb to 2e688f2 Compare January 14, 2026 20:49

ballard26 marked this pull request as ready for review January 14, 2026 20:50

Copilot AI review requested due to automatic review settings January 14, 2026 20:50

Copilot AI reviewed Jan 14, 2026

View reviewed changes

ballard26 force-pushed the datalake-dup-msg-writes branch from 2e688f2 to 59fd83b Compare January 14, 2026 21:01

tyson-redpanda added this to the v25.3.5 milestone Jan 14, 2026

datalake: allow oom injection in noop_mem_tracker

c7c8c5c

ballard26 force-pushed the datalake-dup-msg-writes branch from 59fd83b to 2e51457 Compare January 14, 2026 22:41

bharathv reviewed Jan 15, 2026

View reviewed changes

tyson-redpanda modified the milestones: v25.3.5, v25.3.x-next Jan 16, 2026

ballard26 added 2 commits January 25, 2026 22:21

ballard26 force-pushed the datalake-dup-msg-writes branch from 2e51457 to 7f506e3 Compare January 26, 2026 03:21

ballard26 requested a review from bharathv January 26, 2026 03:21

bharathv approved these changes Jan 26, 2026

View reviewed changes

ballard26 merged commit 8c654cc into redpanda-data:dev Jan 26, 2026
19 checks passed

		// Hence, the current solution is to return those errors on the subsequent
		// call to `add_data_struct`.

-    // Hence, the current solution is to return those errors on the subsequent
-    // call to `add_data_struct`.
+    // Hence, the current solution is to defer those errors by storing them in
+    // `_error` and returning them on the subsequent call to `add_data_struct`.
+    // Callers must ensure that any deferred error is observed (either via a
+    // follow-up call to `add_data_struct` or by the finalization path, e.g.
+    // `finish()` inspecting `_error`) before treating the writer as
+    // successfully completed.

Conversation

ballard26 commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backports Required

Release Notes

Bug Fixes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

ballard26 Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

bharathv Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

ballard26 Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vbotbuildovich commented Jan 15, 2026

Retry command for Build#79055

Uh oh!

vbotbuildovich commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI test results

Uh oh!

ballard26 commented Jan 15, 2026

Uh oh!

bharathv left a comment

Choose a reason for hiding this comment

Uh oh!

bharathv Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vbotbuildovich commented Jan 26, 2026

Uh oh!

vbotbuildovich commented Jan 26, 2026

Uh oh!

vbotbuildovich commented Jan 26, 2026

Uh oh!

vbotbuildovich commented Jan 26, 2026

Uh oh!

vbotbuildovich commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ballard26 commented Jan 14, 2026 •

edited

Loading

vbotbuildovich commented Jan 15, 2026 •

edited

Loading