fix: prevent iterator invalidation in handleDeadDefender (BUG-02)#3943
fix: prevent iterator invalidation in handleDeadDefender (BUG-02)#3943berkelmali wants to merge 2 commits into
Conversation
Snapshot target.tiles() into an array before iterating to prevent modifying the collection during iteration when conquer() is called. Also increase loop limit from 10 to 100 with early break on empty to properly handle large empires.
WalkthroughThe change refines the post-defender-conquest sweep in ChangesPost-defender conquest sweep
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/execution/AttackExecution.ts`:
- Around line 370-373: Add deterministic regression tests for the new sweep
iteration logic in AttackExecution (the loop that calls target.tiles() and does
up to 100 passes). Create tests that (1) mutate the live collection during
iteration to verify safe snapshot behavior by asserting no runtime errors and
that mutated tiles are processed as expected, (2) simulate a larger
dead-defender tile set to confirm full absorption (all defender tiles removed)
after the sweep completes, and (3) verify stable termination when no tiles
remain by asserting the loop exits early (fewer than 100 passes) and no infinite
loop occurs; use a fixed board/state or seeded RNG to ensure determinism and
reference the AttackExecution class and its sweep behavior (the for-loop using
target.tiles()) in test names and assertions.
🪄 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: dadb6691-141d-4eea-93f3-a13671286c01
📒 Files selected for processing (1)
src/core/execution/AttackExecution.ts
| for (let i = 0; i < 100; i++) { | ||
| const remainingTiles = Array.from(target.tiles()); | ||
| if (remainingTiles.length === 0) break; | ||
| for (const tile of remainingTiles) { |
There was a problem hiding this comment.
Add regression tests for this core simulation change
This fixes a real runtime edge case, but the PR includes no tests for the new sweep behavior (snapshot iteration + 100-pass/early-break flow). Please add deterministic tests that cover at least: (1) live-collection mutation safety, (2) full absorption for larger dead-defender tile sets, and (3) stable termination when no tiles remain.
As per coding guidelines: “All changes to src/core/ must include tests”.
🤖 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/execution/AttackExecution.ts` around lines 370 - 373, Add
deterministic regression tests for the new sweep iteration logic in
AttackExecution (the loop that calls target.tiles() and does up to 100 passes).
Create tests that (1) mutate the live collection during iteration to verify safe
snapshot behavior by asserting no runtime errors and that mutated tiles are
processed as expected, (2) simulate a larger dead-defender tile set to confirm
full absorption (all defender tiles removed) after the sweep completes, and (3)
verify stable termination when no tiles remain by asserting the loop exits early
(fewer than 100 passes) and no infinite loop occurs; use a fixed board/state or
seeded RNG to ensure determinism and reference the AttackExecution class and its
sweep behavior (the for-loop using target.tiles()) in test names and assertions.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
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 `@tests/HandleDeadDefender.test.ts`:
- Line 141: Capture the attacker's tile count before performing the sweep/attack
by storing attacker.numTilesOwned() in a variable (e.g., preTiles), then run the
existing sweep/attack call in the test, then get a new count (postTiles =
attacker.numTilesOwned()) and replace the current loose assertion
expect(attacker.numTilesOwned()).toBeGreaterThan(0) with
expect(postTiles).toBeGreaterThan(preTiles) to assert tiles were transferred
from the defender; use variable names like preTiles/postTiles to make intent
clear.
🪄 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: f1dcb017-7bd9-43d3-aab8-e5e6a2973fd0
📒 Files selected for processing (1)
tests/HandleDeadDefender.test.ts
| expect(defender.numTilesOwned()).toBe(0); | ||
|
|
||
| // Attacker should have gained tiles | ||
| expect(attacker.numTilesOwned()).toBeGreaterThan(0); |
There was a problem hiding this comment.
Strengthen the assertion to verify tile transfer.
The assertion checks that the attacker has more than zero tiles, but the attacker already owns tiles from spawning (line 48-50). This doesn't prove that tiles were transferred from the defender during the sweep.
Consider capturing the attacker's tile count before the attack and asserting that it increased:
💪 Suggested improvement
+ const attackerTilesBefore = attacker.numTilesOwned();
+
// Attack and kill the defender
game.addExecution(new AttackExecution(null, attacker, defender.id(), null));
for (let i = 0; i < 500; i++) {
game.executeNextTick();
if (!defender.isAlive()) break;
}
// All defender tiles should now belong to someone other than defender
expect(defender.numTilesOwned()).toBe(0);
// Attacker should have gained tiles
- expect(attacker.numTilesOwned()).toBeGreaterThan(0);
+ expect(attacker.numTilesOwned()).toBeGreaterThan(attackerTilesBefore);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(attacker.numTilesOwned()).toBeGreaterThan(0); | |
| const attackerTilesBefore = attacker.numTilesOwned(); | |
| // Attack and kill the defender | |
| game.addExecution(new AttackExecution(null, attacker, defender.id(), null)); | |
| for (let i = 0; i < 500; i++) { | |
| game.executeNextTick(); | |
| if (!defender.isAlive()) break; | |
| } | |
| // All defender tiles should now belong to someone other than defender | |
| expect(defender.numTilesOwned()).toBe(0); | |
| // Attacker should have gained tiles | |
| expect(attacker.numTilesOwned()).toBeGreaterThan(attackerTilesBefore); |
🤖 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 `@tests/HandleDeadDefender.test.ts` at line 141, Capture the attacker's tile
count before performing the sweep/attack by storing attacker.numTilesOwned() in
a variable (e.g., preTiles), then run the existing sweep/attack call in the
test, then get a new count (postTiles = attacker.numTilesOwned()) and replace
the current loose assertion expect(attacker.numTilesOwned()).toBeGreaterThan(0)
with expect(postTiles).toBeGreaterThan(preTiles) to assert tiles were
transferred from the defender; use variable names like preTiles/postTiles to
make intent clear.
1 participant
Description: In
AttackExecution.handleDeadDefender(),target.tiles()returns a live collection that is modified during iteration whenconquer(tile)is called. This can cause iterator invalidation and unpredictable behavior infor...ofloops over Sets. Fix: Snapshottarget.tiles()into an array viaArray.from()before iterating, and increase the loop limit from 10 to 100 with an earlybreakwhen no tiles remain -- ensuring large empires are fully absorbed.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires