Fix AStar overflowing the priority queue on twisted paths#4149
Merged
Conversation
Contributor
WalkthroughThe A* pathfinding algorithm removes the configurable ChangesA Queue Implementation Simplification*
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
small-fix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/pathfinding/algorithms/AStar.AbstractGraph.ts`:
- Around line 5-8: Write a regression test that constructs an AbstractGraph
instance and invokes AbstractGraphAStar.findPath with a config that forces many
lazy duplicate pushes (enough re-entries to exceed the previous numNodes-sized
heap) so the MinHeap must be sized correctly; mock/spy on console.error and
assert it is never called with the string "MinHeap capacity exceeded", and also
assert the returned path is correct/valid. Use the AbstractGraphAStar.findPath
entrypoint and supply a graph topology (e.g., a grid or crafted node re-entry
pattern) and A* config (heuristicWeight/maxIterations) to trigger duplicate
re-insertions, and verify no capacity error and that path length/contents match
expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 616753df-1d3d-42db-bd19-74b95f0aeb4b
📒 Files selected for processing (1)
src/core/pathfinding/algorithms/AStar.AbstractGraph.ts
Comment on lines
5
to
8
| export interface AbstractGraphAStarConfig { | ||
| heuristicWeight?: number; | ||
| maxIterations?: number; | ||
| useMinHeap?: boolean; // Use MinHeap instead of BucketQueue (better for variable costs) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any lingering callers passing the removed option, and existing tests for this file.
rg -nP '\buseMinHeap\b' -C2 --type=ts
rg -nP 'AbstractGraphAStar' -C2 --glob '**/*test*' --glob '**/*spec*'Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Re-check remaining callers for `useMinHeap`
echo "=== useMinHeap occurrences (TypeScript) ==="
rg -n --no-heading '\buseMinHeap\b' --type=ts -S .
# 2) Find tests/specs that mention AStar / pathfinding
echo
echo "=== AStar-related tests/specs ==="
rg -n --no-heading 'AStar|AbstractGraphAStar|pathfinding|PriorityQueue|MinHeap|useMinHeap' \
--glob '**/*test*.*' --glob '**/*spec*.*' .
# 3) Inspect the AStar file for queue/resize/numNodes overflow logic hints
echo
echo "=== Key queue/overflow keywords in AStar.AbstractGraph.ts ==="
rg -n --no-heading 'queue|PriorityQueue|numNodes|resize|overflow|twisty|height|width|maxIterations|heuristicWeight' \
src/core/pathfinding/algorithms/AStar.AbstractGraph.ts
# 4) Show a small window around interface and config usage
echo
echo "=== Interface + config usage around AbstractGraphAStarConfig ==="
rg -n --no-heading 'AbstractGraphAStarConfig|heuristicWeight|maxIterations' \
-n src/core/pathfinding/algorithms/AStar.AbstractGraph.tsRepository: openfrontio/OpenFrontIO
Length of output: 111
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== useMinHeap occurrences (TypeScript) ==="
rg -n --no-heading '\buseMinHeap\b' --type=ts -S . || true
echo
echo "=== AStar-related tests/specs ==="
rg -n --no-heading 'AStar|AbstractGraphAStar|pathfinding|PriorityQueue|MinHeap|useMinHeap' \
--glob '**/*test*.*' --glob '**/*spec*.*' . || true
echo
echo "=== Key queue/overflow keywords in AStar.AbstractGraph.ts ==="
rg -n --no-heading 'queue|PriorityQueue|numNodes|resize|overflow|twisty|height|width|maxIterations|heuristicWeight' \
src/core/pathfinding/algorithms/AStar.AbstractGraph.ts || true
echo
echo "=== Interface + config usage around AbstractGraphAStarConfig ==="
rg -n --no-heading 'AbstractGraphAStarConfig|heuristicWeight|maxIterations' \
src/core/pathfinding/algorithms/AStar.AbstractGraph.ts || trueRepository: openfrontio/OpenFrontIO
Length of output: 5383
🏁 Script executed:
#!/bin/bash
set +euo pipefail
# Inspect the relevant AStar file sections
echo "=== AStar.AbstractGraph.ts (relevant sections) ==="
sed -n '1,260p' src/core/pathfinding/algorithms/AStar.AbstractGraph.ts
# Look for direct test coverage references
echo
echo "=== Test files referencing AStar.AbstractGraph / MinHeap / PriorityQueue ==="
rg -n --no-heading 'AStar\.AbstractGraph|AbstractGraphAStarConfig|new (MinHeap|PriorityQueue)|MinHeap|PriorityQueue' \
--glob '**/*test*.*' --glob '**/*spec*.*' tests . || true
# Look for useMinHeap in the whole repo (non-type limited)
echo
echo "=== useMinHeap occurrences (whole repo) ==="
rg -n --no-heading '\buseMinHeap\b' . || trueRepository: openfrontio/OpenFrontIO
Length of output: 7563
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== AStar.AbstractGraph.ts (relevant sections) ==="
sed -n '1,260p' src/core/pathfinding/algorithms/AStar.AbstractGraph.ts
echo
echo "=== Test files referencing AStar.AbstractGraph / AbstractGraphAStarConfig / MinHeap / PriorityQueue ==="
rg -n --no-heading 'AStar\.AbstractGraph|AbstractGraphAStarConfig|new (MinHeap|PriorityQueue)|MinHeap|PriorityQueue' \
--glob '**/*test*.*' --glob '**/*spec*.*' tests . || true
echo
echo "=== useMinHeap occurrences (whole repo) ==="
rg -n --no-heading '\buseMinHeap\b' . || trueRepository: openfrontio/OpenFrontIO
Length of output: 7590
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== Where AbstractGraphAStar is used (src + tests) ==="
rg -n --no-heading '\bAbstractGraphAStar\b' src tests . || true
echo
echo "=== Where MinHeap/PriorityQueue are used ==="
rg -n --no-heading '\bMinHeap\b|\bPriorityQueue\b' src/core/pathfinding . || true
echo
echo "=== Check AStar wrapper/pathfinder implementations that might hit AbstractGraphAStar ==="
rg -n --no-heading 'UniversalPathFinding|PathFinderStepper|ConnectedComponents|PathFinding\.(Air|Rail|Water)|PathFinder\.' \
src/core/pathfinding tests/core/pathfinding || trueRepository: openfrontio/OpenFrontIO
Length of output: 12836
🏁 Script executed:
rg -n --no-heading 'WaterHierarchical|AbstractGraphAStar' src/core/pathfinding src/core/pathfinding/algorithms/ AStar.WaterHierarchical.tsRepository: openfrontio/OpenFrontIO
Length of output: 2433
🏁 Script executed:
#!/bin/bash
set +euo pipefail
# Show MinHeap overflow/resize logic
echo "=== PriorityQueue.ts (MinHeap capacity/resize checks) ==="
rg -n --no-heading 'capacity|exceeded|resize|grow|push\(' src/core/pathfinding/algorithms/PriorityQueue.ts
echo
sed -n '1,220p' src/core/pathfinding/algorithms/PriorityQueue.ts
# Show WaterHierarchical algorithm using AbstractGraphAStar
echo
echo "=== AStar.WaterHierarchical.ts ==="
sed -n '1,220p' src/core/pathfinding/algorithms/AStar.WaterHierarchical.ts
# Show PathFinding.Water test coverage context
echo
echo "=== tests/core/pathfinding/PathFinding.Water.test.ts ==="
sed -n '1,240p' tests/core/pathfinding/PathFinding.Water.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 19250
🏁 Script executed:
#!/bin/bash
set +euo pipefail
# Show MinHeap overflow/resize logic
echo "=== PriorityQueue.ts (MinHeap capacity/resize checks) ==="
rg -n --no-heading 'capacity|exceeded|resize|grow|push\(' src/core/pathfinding/algorithms/PriorityQueue.ts
echo
sed -n '1,220p' src/core/pathfinding/algorithms/PriorityQueue.ts
# Show WaterHierarchical algorithm using AbstractGraphAStar
echo
echo "=== AStar.WaterHierarchical.ts ==="
sed -n '1,220p' src/core/pathfinding/algorithms/AStar.WaterHierarchical.ts
# Show PathFinding.Water test coverage context
echo
echo "=== tests/core/pathfinding/PathFinding.Water.test.ts ==="
sed -n '1,240p' tests/core/pathfinding/PathFinding.Water.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 19250
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== Search tests for MinHeap overflow warning text ==="
rg -n --no-heading 'MinHeap capacity exceeded' tests src . || true
echo
echo "=== Search tests for PriorityQueue/MinHeap resize-related spies ==="
rg -n --no-heading 'capacity exceeded|Resizing, but this indicates a bug|MinHeap|PriorityQueue' tests . || true
echo
echo "=== Search tests that spy on console.error ==="
rg -n --no-heading 'spyOn\(console,\s*"error"\)|console\.error' tests . || trueRepository: openfrontio/OpenFrontIO
Length of output: 21191
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== Search tests for MinHeap overflow warning text ==="
rg -n --no-heading 'MinHeap capacity exceeded' tests src . || true
echo
echo "=== Search tests for PriorityQueue/MinHeap resize-related spies ==="
rg -n --no-heading 'capacity exceeded|Resizing, but this indicates a bug|MinHeap|PriorityQueue' tests . || true
echo
echo "=== Search tests that spy on console.error ==="
rg -n --no-heading 'spyOn\(console,\s*"error"\)|console\.error' tests . || trueRepository: openfrontio/OpenFrontIO
Length of output: 21191
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== PathFinder.Water wiring ==="
rg -n --no-heading 'Water\(game\)|PathFinding\.Water|new AStarWaterHierarchical|AStarWaterHierarchical|WaterHierarchical' src/core/pathfinding/PathFinder.ts src/core/pathfinding/PathFinderBuilder.ts src/core/pathfinding/PathFinder.*.ts src/core/pathfinding/*.ts src/core/pathfinding/**.ts || true
# Show the Water factory implementation if found in PathFinder.ts
rg -n --no-heading 'PathFinding\.Water|function Water|static Water|Water\(game\)' src/core/pathfinding/PathFinder.ts || true
sed -n '1,240p' src/core/pathfinding/PathFinder.tsRepository: openfrontio/OpenFrontIO
Length of output: 8147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== PathFinding.Water wiring (which A* gets used) ==="
rg -n --no-heading 'PathFinding\.Water|Water\(game\)|AStar\.WaterHierarchical|AStarWaterHierarchical|WaterHierarchical' src/core/pathfinding/PathFinder.ts src/core/pathfinding/PathFinder.*.ts src/core/pathfinding/**.ts || true
echo
echo "=== PathFinder.ts (where Water is built) ==="
sed -n '1,260p' src/core/pathfinding/PathFinder.tsRepository: openfrontio/OpenFrontIO
Length of output: 7857
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== Find miniWaterHPA/miniWaterGraph methods ==="
rg -n --no-heading 'miniWaterHPA\(|miniWaterGraph\(' src/core/game src/core -S || true
echo
echo "=== Inspect Setup util to see how test worlds are initialized ==="
rg -n --no-heading 'setup\("' tests/util/Setup.* tests -S || true
fd -t f Setup tests/util -x echo {} | head -n 20 || true
ls -la tests/util || trueRepository: openfrontio/OpenFrontIO
Length of output: 5681
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== Find miniWaterHPA / miniWaterGraph ==="
rg -n --no-heading 'miniWaterHPA\(|miniWaterGraph\(' src/core/game src/core -S || true
echo
echo "=== Locate tests/util/Setup ==="
ls -la tests/util || true
fd -t f 'Setup.*' tests/util -a || true
fd -t f 'Setup.*' tests -a || true
echo
echo "=== Find how `setup('ocean_and_land')` is configured ==="
rg -n --no-heading 'setup\("ocean_and_land"|setup\("world"' tests -S || true
echo
echo "=== Open tests/util/Setup.ts (or closest match) ==="
setup_file=$(fd -t f 'Setup.*\.(ts|js|tsx)$' tests/util tests -a | head -n 1)
echo "Using: ${setup_file:-<not found>}"
if [[ -n "${setup_file}" ]]; then
sed -n '1,240p' "$setup_file"
fiRepository: openfrontio/OpenFrontIO
Length of output: 5835
Add a regression test for AbstractGraphAStar MinHeap overflow behavior
AStar.AbstractGraph.ts now sizes the MinHeap to avoid the “capacity exceeded”/resize path caused by lazy duplicate pushes.
- Add a regression test that runs
AbstractGraphAStar.findPathon anAbstractGraphsetup that forces enough re-entries (lazy duplicates) to exceed the oldnumNodes-sized heap. - Assert
console.erroris not called withMinHeap capacity exceeded(and keep asserting the returned path is still valid).
There are no remaining useMinHeap usages in the repo, so caller updates are already handled by the type changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/pathfinding/algorithms/AStar.AbstractGraph.ts` around lines 5 - 8,
Write a regression test that constructs an AbstractGraph instance and invokes
AbstractGraphAStar.findPath with a config that forces many lazy duplicate pushes
(enough re-entries to exceed the previous numNodes-sized heap) so the MinHeap
must be sized correctly; mock/spy on console.error and assert it is never called
with the string "MinHeap capacity exceeded", and also assert the returned path
is correct/valid. Use the AbstractGraphAStar.findPath entrypoint and supply a
graph topology (e.g., a grid or crafted node re-entry pattern) and A* config
(heuristicWeight/maxIterations) to trigger duplicate re-insertions, and verify
no capacity error and that path length/contents match expected.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
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
2 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.
Resolves pathfinding issue: https://discord.com/channels/1359946986937258015/1458870041964445706
Description:
BucketQueue requires
maxFto be defined. Estimating it is much more complicated than the code assumed. This caused the bucket to overflow on certain paths, mostly (a) twisty paths - defined as one which must traverse both ways along the same axis, (b) maps where height > width, because we usedwidth ** 2to estimate size instead ofwidth * height(iirc height was not easily accessible).This PR replcaes BucketQueue with already existing MinHeap.
AStar.AbstractGraphis already specialized in traversingAbstractGraph, so I have dropped theuseMinHeapoption and instead made it the default path. Otherwise we'd be leaving dead code. The max priority was also fixed to account for duplicate connections, abstract graph is already very small so it should not affect (and in my testing does not) affect performance.Before
After
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
moleole