fix(flowcontrol): call up() after down() in maxMinHeap.Remove#1689
Closed
RishabhSaini wants to merge 1 commit into
Closed
fix(flowcontrol): call up() after down() in maxMinHeap.Remove#1689RishabhSaini wants to merge 1 commit into
RishabhSaini wants to merge 1 commit into
Conversation
RishabhSaini
requested review from
a team,
LukeAVanDrie and
shmuelk
as code owners
June 18, 2026 16:24
github-actions
Bot
added
the
size/M
RishabhSaini
force-pushed
the
heapCorrupt
branch
from
June 18, 2026 16:28
2d46666 to
f2c71cd
Compare
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.
Add the missing up(i) call after down(i). If down moved the element,
up is a no-op at the new position (and vice versa).
Signed-off-by: RishabhSaini <rishabhsaini01@gmail.com>
RishabhSaini
force-pushed
the
heapCorrupt
branch
from
June 18, 2026 16:30
f2c71cd to
fe41c1b
Compare
Contributor
|
@RishabhSaini and I discussed this and are going ahead with #1690 which resolves the issue and moves the code to a more maintainable state. |
Contributor
Can this PR be closed? |
Contributor
Author
|
in favor of #1690 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
maxMinHeap.Removeonly callsdown()after swapping the removed elementwith the last array element. When the replacement comes from a different
subtree, ancestor-level violations go unfixed. Add the missing
up()call.Details
In a max-min heap, levels alternate between max and min. The subtree property
(each node is the max or min of its entire subtree) is NOT transitive across
alternating levels, so
down()alone cannot fix violations with ancestors.Current production impact: none. The only
Removecaller (dispatchItem)always removes the root (index 0) via
PeekHead, wheredown()alone iscorrect regardless of heap layout. However,
Removeaccepts an arbitraryhandle and its contract promises O(log n) arbitrary removal. Any future caller
(request cancellation, queue eviction) would hit silent heap corruption.
Test plan
TestMaxMinHeap_Remove_CrossSubtreeSwapfails without the fix,passes with it