Skip to content

Commit a65f023

Browse files
committed
fix: heap corruption in remove(), clone() capacity, and stability index bugs (v1.0.3)
- Fix remove() in PriorityQueue, TypedPriorityQueue, and StableTypedPriorityQueue: up/down heap restoration directions were swapped, causing silent heap corruption when removing internal elements in queues with >4 elements. - Fix clone() in TypedPriorityQueue and StableTypedPriorityQueue: used _defaultSize instead of actual backing array length, causing RangeError after array growth. - Fix StablePriorityQueue copy constructor: _index (FIFO stability counter) was not copied, breaking insertion order stability in cloned queues. - Fix StableTypedPriorityQueue.heap getter: missing sindex property on returned nodes. - Remove duplicate _indices.set() call in StableTypedPriorityQueue.from(). - Fix incorrect JSDoc on peek() in IPriorityQueueLike (said 'Removes all elements'). - Add regression tests for all fixes. - Add package-lock.json to .gitignore (project uses bun.lockb). - Bump version to 1.0.3.
1 parent f652c6d commit a65f023

11 files changed

Lines changed: 184 additions & 20 deletions

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@ build/Release
4242
node_modules/
4343
jspm_packages/
4444

45+
# Lock files (project uses bun.lockb)
46+
package-lock.json
47+
4548
# Snowpack dependency directory (https://snowpack.dev/)
4649
web_modules/
4750

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "pq-ts",
3-
"version": "1.0.2",
3+
"version": "1.0.3",
44
"description": "A priority queue written in TypeScript.",
55
"main": "dist/main.js",
66
"module": "dist/main.mjs",

src/pq.test.ts

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,4 +241,56 @@ describe("PriorityQueue", () => {
241241
const priorities = dequeuedItems.map((_, index) => pq2.priorityAt(index, true));
242242
expect(priorities).toEqual([3, 4, 5]);
243243
});
244-
});
244+
245+
// Regression: remove() with many elements should preserve heap order
246+
// (Bug: up/down directions were swapped in remove(), causing heap corruption
247+
// when the last element needed to bubble in the opposite direction)
248+
it("should maintain heap order after removing an internal element (regression)", () => {
249+
const pq = new PriorityQueue<number>();
250+
// This specific priority pattern triggers the bug: after removing value 0
251+
// (pri=50), the last node (pri=25) must bubble UP past its new parent (pri=30),
252+
// but the buggy code called _down instead, leaving it in the wrong position.
253+
const priorities = [50, 10, 90, 20, 80, 30, 70, 40, 60, 5, 95, 15, 85, 25];
254+
for (let i = 0; i < priorities.length; i++) {
255+
pq.enqueue(i, priorities[i]);
256+
}
257+
258+
expect(pq.remove(0)).toBe(true);
259+
260+
// Verify the heap invariant: parent priority <= child priority
261+
const heap = pq.heap;
262+
const size = pq.count;
263+
for (let i = 1; i < size; i++) {
264+
const parentIdx = (i - 1) >> 2;
265+
expect(heap[i].priority).toBeGreaterThanOrEqual(heap[parentIdx].priority);
266+
}
267+
268+
// Also verify dequeue order
269+
let prev = pq.pop()!;
270+
while (!pq.isEmpty()) {
271+
const curr = pq.pop()!;
272+
expect(curr.priority).toBeGreaterThanOrEqual(prev.priority);
273+
prev = curr;
274+
}
275+
});
276+
277+
// Regression: remove() should preserve heap for all removals (stress test)
278+
it("should maintain heap order after removing any element (regression)", () => {
279+
const priorities = [50, 10, 90, 20, 80, 30, 70, 40, 60, 5, 95, 15, 85, 25];
280+
for (let removeIdx = 0; removeIdx < priorities.length; removeIdx++) {
281+
const pq = new PriorityQueue<number>();
282+
for (let i = 0; i < priorities.length; i++) {
283+
pq.enqueue(i, priorities[i]);
284+
}
285+
286+
pq.remove(removeIdx);
287+
288+
let prev = pq.pop()!;
289+
while (!pq.isEmpty()) {
290+
const curr = pq.pop()!;
291+
expect(curr.priority).toBeGreaterThanOrEqual(prev.priority);
292+
prev = curr;
293+
}
294+
}
295+
});
296+
});

src/pq.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -257,9 +257,9 @@ export class PriorityQueue<
257257
if (index < newSize) {
258258
const lastNode = this._elements[newSize];
259259
if (this.compare(lastNode, removedElement) < 0) {
260-
this._down(lastNode, index);
261-
} else {
262260
this._up(lastNode, index);
261+
} else {
262+
this._down(lastNode, index);
263263
}
264264
}
265265

src/stable.pq.test.ts

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,4 +286,37 @@ describe("StablePriorityQueue", () => {
286286
const elements = dequeuedItems.map((item) => item?.value);
287287
expect(elements).toEqual([2, 4, 3, 1]);
288288
});
289-
});
289+
290+
// Regression: clone should copy _index so new inserts into the clone
291+
// preserve FIFO stability relative to existing items
292+
it("should preserve stability after cloning and enqueueing (regression)", () => {
293+
const pq = new StablePriorityQueue<string>();
294+
pq.enqueue("a", 1);
295+
pq.enqueue("b", 1);
296+
pq.enqueue("c", 1);
297+
298+
const clone = pq.clone();
299+
// After clone, inserting "d" with same priority should come after a, b, c
300+
clone.enqueue("d", 1);
301+
302+
expect(clone.toArray()).toEqual(["a", "b", "c", "d"]);
303+
});
304+
305+
// Regression: remove() with many elements should preserve heap + stability
306+
it("should maintain heap order after removing internal element (regression)", () => {
307+
const pq = new StablePriorityQueue<number>();
308+
for (let i = 0; i < 15; i++) {
309+
pq.enqueue(i, i % 5); // Groups of 3 items at each priority 0..4
310+
}
311+
312+
// Remove an element from the middle
313+
expect(pq.remove(7)).toBe(true);
314+
315+
let prev = pq.pop()!;
316+
while (!pq.isEmpty()) {
317+
const curr = pq.pop()!;
318+
expect(curr.priority).toBeGreaterThanOrEqual(prev.priority);
319+
prev = curr;
320+
}
321+
});
322+
});

src/stable.pq.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ export class StablePriorityQueue<
4141
this._elements = [...elements._elements];
4242
this._size = elements._size;
4343
this.compare = elements.compare;
44+
this._index = elements._index;
4445
} else if (Array.isArray(elements)) {
4546
this._elements = new Array(elements.length);
4647
this.compare = comparer ?? min as Comparer;

src/stable.typed.pq.test.ts

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,4 +282,45 @@ describe("StableTypedPriorityQueue", () => {
282282
const elements = dequeuedItems.map((item) => item?.value);
283283
expect(elements).toEqual([2, 4, 3, 1]);
284284
});
285-
});
285+
286+
// Regression: remove() with many elements should preserve heap + stability
287+
it("should maintain heap order after removing an internal element (regression)", () => {
288+
const pq = new StableTypedPriorityQueue(Uint32Array, 4);
289+
for (let i = 1; i <= 20; i++) {
290+
pq.enqueue(i * 10, i);
291+
}
292+
293+
expect(pq.remove(150)).toBe(true);
294+
expect(pq.count).toBe(19);
295+
296+
let prev = pq.pop()!;
297+
while (!pq.isEmpty()) {
298+
const curr = pq.pop()!;
299+
expect(curr.priority).toBeGreaterThanOrEqual(prev.priority);
300+
prev = curr;
301+
}
302+
});
303+
304+
// Regression: clone() should work after internal array growth
305+
it("should clone correctly after the queue has grown beyond initial size (regression)", () => {
306+
const pq = new StableTypedPriorityQueue(Uint32Array, 2);
307+
for (let i = 0; i < 10; i++) {
308+
pq.enqueue(i, i);
309+
}
310+
311+
const clone = pq.clone();
312+
expect(clone.count).toBe(10);
313+
expect(clone.toArray()).toEqual(pq.toArray());
314+
});
315+
316+
// Regression: heap getter should include sindex for stable nodes
317+
it("should include sindex in heap nodes (regression)", () => {
318+
const pq = new StableTypedPriorityQueue(Uint32Array, 10);
319+
pq.enqueue(1, 5);
320+
pq.enqueue(2, 3);
321+
322+
const nodes = pq.heap;
323+
expect(nodes[0]).toHaveProperty("sindex");
324+
expect(nodes[1]).toHaveProperty("sindex");
325+
});
326+
});

src/stable.typed.pq.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ export class StableTypedPriorityQueue<
4949
return Array
5050
.from(this._elements)
5151
.map((value, index) => ({
52-
value, priority: this._priorities[index], nindex: index
52+
value, priority: this._priorities[index], nindex: index, sindex: this._indices[index]
5353
}) as Node);
5454
}
5555

@@ -72,7 +72,8 @@ export class StableTypedPriorityQueue<
7272
}
7373

7474
clone(): this {
75-
const clone = new StableTypedPriorityQueue<Node, Comparer>(this._backend, this._defaultSize, this.compare);
75+
const size = Math.max(this._elements.length, this._defaultSize);
76+
const clone = new StableTypedPriorityQueue<Node, Comparer>(this._backend, size, this.compare);
7677
clone._elements.set(this._elements);
7778
clone._priorities.set(this._priorities);
7879
clone._indices.set(this._indices);
@@ -102,12 +103,12 @@ export class StableTypedPriorityQueue<
102103
sindex: this._indices[newSize]
103104
} as const as Node;
104105

105-
// If the last element should be "bubbled up" (preserve heap property)
106+
// If lastNode has higher or equal priority than removed, it may need to bubble up;
107+
// otherwise it has lower priority and must bubble down.
106108
if (this.compare(removedNode, lastNode) < 0) {
107-
this._up(lastNode, index);
108-
} else {
109-
// Otherwise, it should "bubble down"
110109
this._down(lastNode, index);
110+
} else {
111+
this._up(lastNode, index);
111112
}
112113
}
113114

@@ -301,7 +302,6 @@ export class StableTypedPriorityQueue<
301302
newQueue._indices.set(queue._indices);
302303
newQueue._size = queue._size;
303304
newQueue._sindex = queue._sindex;
304-
newQueue._indices.set(queue._indices);
305305
return newQueue;
306306
};
307307

src/typed.pq.test.ts

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -257,4 +257,37 @@ describe("TypedPriorityQueue", () => {
257257
const priorities = dequeuedItems.map((_, index) => pq2.priorityAt(index, true));
258258
expect(priorities).toEqual([3, 4, 5]);
259259
});
260-
});
260+
261+
// Regression: remove() with many elements should preserve heap order
262+
// (Bug: up/down directions were swapped in remove())
263+
it("should maintain heap order after removing an internal element (regression)", () => {
264+
const pq = new TypedPriorityQueue(Uint32Array, 4);
265+
for (let i = 1; i <= 20; i++) {
266+
pq.enqueue(i * 10, i);
267+
}
268+
269+
// Remove an element with high priority value so lastNode must bubble up
270+
expect(pq.remove(150)).toBe(true);
271+
expect(pq.count).toBe(19);
272+
273+
let prev = pq.pop()!;
274+
while (!pq.isEmpty()) {
275+
const curr = pq.pop()!;
276+
expect(curr.priority).toBeGreaterThanOrEqual(prev.priority);
277+
prev = curr;
278+
}
279+
});
280+
281+
// Regression: clone() should work even after internal array growth
282+
// (Bug: clone used _defaultSize which is too small after growth)
283+
it("should clone correctly after the queue has grown beyond initial size (regression)", () => {
284+
const pq = new TypedPriorityQueue(Uint32Array, 2); // tiny initial size
285+
for (let i = 0; i < 10; i++) {
286+
pq.enqueue(i, i);
287+
}
288+
289+
const clone = pq.clone();
290+
expect(clone.count).toBe(10);
291+
expect(clone.toArray()).toEqual(pq.toArray());
292+
});
293+
});

src/typed.pq.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,8 @@ export class TypedPriorityQueue<
7777
}
7878

7979
clone(): this {
80-
const clone = new TypedPriorityQueue<Node, Comparer>(this._backend, this._defaultSize, this.compare);
80+
const size = Math.max(this._elements.length, this._defaultSize);
81+
const clone = new TypedPriorityQueue<Node, Comparer>(this._backend, size, this.compare);
8182
clone._elements.set(this._elements);
8283
clone._priorities.set(this._priorities);
8384
clone._size = this._size;
@@ -103,12 +104,12 @@ export class TypedPriorityQueue<
103104
nindex: newSize
104105
} as const as Node;
105106

106-
// If the last element should be "bubbled up" (preserve heap property)
107+
// If lastNode has higher or equal priority than removed, it may need to bubble up;
108+
// otherwise it has lower priority and must bubble down.
107109
if (this.compare(removedNode, lastNode) < 0) {
108-
this._up(lastNode, index);
109-
} else {
110-
// Otherwise, it should "bubble down"
111110
this._down(lastNode, index);
111+
} else {
112+
this._up(lastNode, index);
112113
}
113114
}
114115

0 commit comments

Comments
 (0)