c/scheduling: fix allocation underflow on no-op reallocate by joe-redpanda · Pull Request #31071 · redpanda-data/redpanda

joe-redpanda · 2026-07-10T16:46:43Z

Attempts to deflake the nodewise recovery tests led us to more agressively change partition replication factors. This surfaced a bug on reconfigurations where if the reconfiguration was a no-op, we would actually accidentally delete all of that partitions allocations.

This pr does a few things.

adds tests on allocated partition because there were previously none
adds a regression test for the above double free
refactors allocated partition via a helper type which makes the error immediately obvious
fixes that issue
a little cleanup on the side because its nice to have

The actual bug:

allocated_partition is meant to yield the newly added replicas on a given partition
those go in an raii guard which should make sure that should anything go wrong, those allocations are re-decremented
When there were no actually allocated replicas, the logic to yield the added replicas accidentally yielded all of the replicas.
No actually replicas get added so nothing gets incremented.
The guard though, destructs decrementing allocations on all nodes where the partition had replicas.
Whenever that node gets drained, the replica count goes negative

Backports Required

Release Notes

Bug Fixes

fix allocation double free on no-op rf changes

Adds unit tests for allocated partition. Why? We found a bug in it that could reasonably have been caught with unit test coverage.

Adds a convenience wrapper on allocated partition's _original_node2shard map. There is a nuance in the logic where nullopt is not the same as empty which is not terribly apparent to a code reader. This change allows for the mistake to be self evident.

Regression test which simulates what happens when a reallocate is called with the same replication factor that the partition had prior. The long and short: allocated partition would mistakenly consider all replicas on the partition to be new replicas, and delete all of the allocations. This would eventually bubble up as a node crash on double freed allocations.

Copilot

Pull request overview

Fixes an allocation-state underflow/double-free triggered by “no-op” reallocations (requested RF equals current replica count), where existing replicas were incorrectly treated as newly allocated and then released.

Changes:

Add a regression test covering allocator behavior when allocating with RF equal to the existing replica count.
Refactor allocated_partition to track an “original placement snapshot” via a helper type, and adjust release/destructor logic to only release truly new replicas.
Add a focused allocated_partition unit test suite and Bazel target to exercise allocation counter behavior (no-op, new partition, move, revert, destructor reconciliation).

Reviewed changes

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

Show a summary per file

File	Description
src/v/cluster/tests/topic_updates_dispatcher_test.cc	Adds an end-to-end regression test ensuring no-op reallocate does not deallocate existing replicas.
src/v/cluster/scheduling/types.h	Introduces `allocated_partition::original_node2shard` helper and test friendship to make original-placement handling explicit.
src/v/cluster/scheduling/types.cc	Implements snapshot helper, fixes “added replicas” reporting for no-op reallocations, and adjusts destructor reconciliation behavior.
src/v/cluster/scheduling/tests/BUILD	Adds Bazel `redpanda_cc_gtest` target for the new `allocated_partition` unit tests.
src/v/cluster/scheduling/tests/allocated_partition_test.cc	Adds unit tests that validate allocation/final-count accounting across key `allocated_partition` scenarios.

joe-redpanda · 2026-07-14T18:37:58Z

 #include "base/format_to.h"
+#include "base/vassert.h"
 #include "cluster/scheduling/allocation_state.h"
 #include "utils/exceptions.h"


will fix if this gets another pass

Fixes the logic for yielding new replicas, if there were no changes, there are very clearly no new replicas.

Use is_original() + one pass instead of find/erase scratch; get() is now only used for full iteration.

vbotbuildovich · 2026-07-14T20:20:45Z

CI test results

test results on build#87085

test_status	test_class	test_method	test_arguments	test_kind	job_url	passed	reason	test_history
FLAKY(PASS)	ConsumerGroupBalancingTest	test_coordinator_nodes_balance	null	integration	https://buildkite.com/redpanda/redpanda/builds/87085#019f620b-925c-453f-b505-623ab2c45ccb	10/11	Test PASSES after retries.No significant increase in flaky rate(baseline=0.0058, 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=ConsumerGroupBalancingTest&test_method=test_coordinator_nodes_balance
FLAKY(PASS)	NodeWiseRecoveryTest	test_recovery_local_data_missing	{"wait_for_final_manifest_uploads": false}	integration	https://buildkite.com/redpanda/redpanda/builds/87085#019f6210-1955-49a2-bec7-5baf8a974df7	9/11	Test PASSES after retries.No significant increase in flaky rate(baseline=0.0836, p0=0.5823, reject_threshold=0.0100. adj_baseline=0.2304, p1=0.2910, trust_threshold=0.5000)	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=NodeWiseRecoveryTest&test_method=test_recovery_local_data_missing
FLAKY(PASS)	ShadowLinkingRandomOpsTest	test_node_operations	{"failures": false, "workload_set": "cloud_combos"}	integration	https://buildkite.com/redpanda/redpanda/builds/87085#019f6210-1956-4c5b-96bb-234930314b03	10/11	Test PASSES after retries.No significant increase in flaky rate(baseline=0.0263, 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=ShadowLinkingRandomOpsTest&test_method=test_node_operations

bharathv · 2026-07-15T03:00:53Z

+        auto find(this Self&& self, model::node_id node) {
+            vassert(self._snapshot, "original placement snapshot not captured");
+            return std::forward<Self>(self)._snapshot->find(node);
+        }
+
+        template<typename Self>
+        auto end(this Self&& self) {
+            vassert(self._snapshot, "original placement snapshot not captured");
+            return std::forward<Self>(self)._snapshot->end();
+        }


can we club these both into contains(node).. end() API is a little odd.

are you thinking like find -> optional where nullopt implies not found?

the deducing this nonsense is for const correctness on the returned iterator (cend & normal end for mutable)

all the callers of find() are essentially doing foo.find() != foo.end() .. which is effectively equivalent to contains(), so I was wondering if we even need find() and and end() as separate methods.

theres a few
find
found == end
erase

actually, no I think you're right, the find end erase gets folded away in the destructor commit. I'll add another cleanup round and remove the unused apis

add has one legitimate usage of find with the it

vbotbuildovich · 2026-07-15T17:12:28Z

/backport v26.1.x

vbotbuildovich · 2026-07-15T17:12:29Z

/backport v25.3.x

vbotbuildovich · 2026-07-15T17:12:30Z

/backport v25.2.x

vbotbuildovich · 2026-07-15T17:15:46Z

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

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-31071-v25.3.x-178 remotes/upstream/v25.3.x
git cherry-pick -x f2694e9e12 939fb1bfd1 4024162ca5 3b8d9c0f19 7d32db4beb

Workflow run logs.

vbotbuildovich · 2026-07-15T17:15:53Z

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-31071-v25.2.x-73 remotes/upstream/v25.2.x
git cherry-pick -x f2694e9e12 939fb1bfd1 4024162ca5 3b8d9c0f19 7d32db4beb

Workflow run logs.

github-actions Bot added the area/redpanda label Jul 10, 2026

joe-redpanda force-pushed the joe/fix-alloc-reserve-underflow branch from 9b68942 to 8e3449c Compare July 14, 2026 16:46

github-actions Bot added the area/build label Jul 14, 2026