Skip to content

Commit 28b36f1

Browse files
committed
test(flowcontrol): add failing test for maxMinHeap.Remove heap corruption
Remove swaps position i with the last element then calls down() but not up(). When the last element comes from a different subtree, the ancestor relationship can break: Before: After Remove(index=3, time=1): 0 0 / \ / \ 5 50 5 50 / \ \ / \ 1 4 40 40 4 Last element (index 5, time=40) swaps into index 3. down(3): no children, no-op. Node 1 (min-level, time=5) must be lowest-priority in {5, 40, 4}. time=40 has lower priority than time=5 -- heap property violated. Signed-off-by: RishabhSaini <rishabhsaini01@gmail.com>
1 parent bdbf7cb commit 28b36f1

1 file changed

Lines changed: 70 additions & 0 deletions

File tree

pkg/epp/flowcontrol/framework/plugins/queue/maxminheap_test.go

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package queue
1818

1919
import (
20+
"fmt"
2021
"math"
2122
"testing"
2223
"time"
@@ -61,6 +62,75 @@ func TestMaxMinHeap_InternalProperty(t *testing.T) {
6162
}
6263
}
6364

65+
// TestMaxMinHeap_Remove_CrossSubtreeSwap demonstrates that Remove corrupts the heap when the replacement element
66+
// (swapped from the end of the array) belongs to a different subtree than the removed element.
67+
// The root cause is that Remove calls down() but not up(), so ancestor-level violations go unfixed.
68+
func TestMaxMinHeap_Remove_CrossSubtreeSwap(t *testing.T) {
69+
t.Parallel()
70+
71+
// 6-element max-min heap (FCFS: earlier time = higher priority):
72+
//
73+
// 0 L0 (max) -- highest priority
74+
// / \
75+
// 5 50 L1 (min) -- lowest priority in subtrees
76+
// / \ \
77+
// 1 4 40 L2 (max) -- leaves
78+
//
79+
// Left subtree of root: {5, 1, 4} (index 1, bounded by min-ancestor time=5)
80+
// Right subtree of root: {50, 40} (index 2, bounded by min-ancestor time=50)
81+
times := []int{0, 5, 50, 1, 4, 40}
82+
83+
q := newMaxMinHeap(enqueueTimePolicy)
84+
flowKey := flowcontrol.FlowKey{ID: "f", Priority: 0}
85+
base := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)
86+
for i, sec := range times {
87+
item := mocks.NewMockQueueItemAccessor(10, fmt.Sprintf("t%d", sec), flowKey)
88+
item.EnqueueTimeV = base.Add(time.Duration(sec) * time.Second)
89+
hi := &heapItem{item: item, index: i}
90+
item.SetHandle(hi)
91+
q.items = append(q.items, item)
92+
q.handles[item.Handle()] = hi
93+
}
94+
q.byteSize.Store(60)
95+
96+
// Remove index 3 (time=1, left subtree leaf). Last element (index 5, time=40, right subtree) swaps in.
97+
// down(3) has no children -> no-op. Heap becomes [0, 5, 50, 40, 4].
98+
// Node 1 (min, time=5) must be lowest priority in its subtree {5, 40, 4}.
99+
// But time=40 has lower priority than time=5 -> VIOLATION.
100+
_, err := q.Remove(q.items[3].Handle())
101+
require.NoError(t, err)
102+
103+
// Verify every node is the max (or min) of its entire subtree -- not just its direct children.
104+
for i := range q.items {
105+
isMin := isMinLevel(i)
106+
for d := range q.items {
107+
if !isAncestor(i, d) {
108+
continue
109+
}
110+
if isMin {
111+
require.Falsef(t, q.policy.Less(q.items[i], q.items[d]),
112+
"min-level node %d (t=%d) has higher priority than descendant %d (t=%d)",
113+
i, times[i], d, q.items[d].EnqueueTime().Sub(base)/time.Second)
114+
} else {
115+
require.Falsef(t, q.policy.Less(q.items[d], q.items[i]),
116+
"max-level node %d has lower priority than descendant %d", i, d)
117+
}
118+
}
119+
}
120+
}
121+
122+
// isAncestor reports whether a is a strict ancestor of d in a binary heap.
123+
func isAncestor(a, d int) bool {
124+
if d <= a {
125+
return false
126+
}
127+
for d > a {
128+
d = (d - 1) / 2
129+
}
130+
return d == a
131+
}
132+
133+
64134
// assertHeapProperty checks if the slice of items satisfies the max-min heap property.
65135
func assertHeapProperty(t *testing.T, h *maxMinHeap, msgAndArgs ...any) {
66136
t.Helper()

0 commit comments

Comments
 (0)