Skip to content

[Bug / Stability] Dead defender cleanup crashes server via iterator invalidation and ghost territory state corruption #4091

@berkelmali

Description

@berkelmali

Summary

AttackExecution.handleDeadDefender() iterates directly over the live target.tiles() collection while simultaneously calling this._owner.conquer(tile) inside that same loop. conquer() mutates the underlying Set<TileRef> mid-iteration, triggering JavaScript's iterator invalidation behavior — causing silent data corruption followed by a delayed, hard-to-trace server crash.


Root Cause

In src/core/execution/AttackExecution.ts, the upstream cleanup loop looks like this:

// ⚠️ Upstream (unpatched) — iterates live collection while mutating it
for (const tile of target.tiles()) {
  this._owner.conquer(tile); // ← mutates the same Set mid-iteration
}

JavaScript's Set iterator specification (ECMA-262 §23.2.5.2.1) guarantees that:

  • Entries deleted before being visited by the iterator will be skipped silently — no exception is thrown.
  • Entries added after iteration begins will be visited — potentially extending the loop.

This means conquer() calls that transfer or remove tiles from the Set cause the iterator to silently skip remaining defender tiles, leaving them in a corrupt intermediate state.


Failure Mode: Two-Stage Corruption + Crash

Stage 1 — Ghost Territory (Silent Corruption)

The iterator skips a subset of the defender's tiles unpredictably. Those tiles remain registered under the dead/eliminated Player object in the game state, but that player has zero tiles from the game's perspective. The result is orphaned phantom territories — tiles with an owner that no longer exists as an active participant.

Stage 2 — Downstream Null-Dereference Crash (Delayed)

On the next game tick, downstream systems process every tile in the map — including the orphaned ones. Systems such as win condition checks, score calculation, and border updates call this.mg.owner(tile) on a ghost tile and receive a stale Player reference — or null. Attempting to call methods on this reference (e.g., .numTilesOwned(), .id()) throws an uncaught TypeError, crashing the entire server process.

Because the crash originates several ticks after the corrupting event in AttackExecution.ts, stack traces point at the wrong system — making this extremely difficult to diagnose without understanding the iterator invalidation root cause.


Why This Is a Verified Structural Vulnerability

This is not a speculative bug — it is structurally guaranteed by the ECMA-262 specification. The behavior is deterministic: any game where a defender's tile count drops below 100 and triggers handleDeadDefender() is at risk, regardless of map or player count.


Proposed Fix

Snapshot the tile collection into a static array before beginning iteration:

// ✅ Fixed — snapshot prevents iterator invalidation
for (let i = 0; i < 100; i++) {
  const remainingTiles = Array.from(target.tiles()); // frozen copy
  if (remainingTiles.length === 0) break;
  for (const tile of remainingTiles) {
    this._owner.conquer(tile); // safe — not touching the iterator
  }
}

Array.from(target.tiles()) creates a complete, isolated snapshot of the Set at that moment. All subsequent conquer() mutations operate on the live Set but the iterator is unaffected, eliminating both the ghost territory and the downstream crash.


Affected File

src/core/execution/AttackExecution.tshandleDeadDefender() method

Linked PR

#3943

Metadata

Metadata

Assignees

No one assigned

    Labels

    not-approvedThis issue has NOT been approved by the maintainers.

    Type

    No type
    No fields configured for issues without a type.

    Projects

    Status
    Triage

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions