Skip to content

Fix broken critical section in static World create/destroy - #309

Merged
genaray merged 1 commit into
genaray:masterfrom
penspanic:pr/static-worlds-lock
Jul 15, 2026
Merged

Fix broken critical section in static World create/destroy#309
genaray merged 1 commit into
genaray:masterfrom
penspanic:pr/static-worlds-lock

Conversation

@penspanic

@penspanic penspanic commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Problem

World.Create and World.Dispose lock the static Worlds array itself, but
Create replaces that array on resize:

lock (Worlds)
{
    ...
    if (recycledId >= Worlds.Length)
    {
        var worlds = Worlds;
        Array.Resize(ref worlds, newCapacity);
        Worlds = worlds;          // <- the lock target just changed
    }
    Worlds[recycledId] = world;
}

After a resize, a creator blocked on the old array's monitor and a newly
arriving creator locking the new array enter the critical section
concurrently. Two failure modes follow:

  • both dequeue/compute the same world id → two live worlds share an id →
    EntityInfo lookups cross-wire → fatal AccessViolationException deep in
    Chunk.GetArray
  • a slot write lands in the stale array → the live Worlds array holds
    null for a freshly created world → NullReferenceException in
    EntityExtensions.Get / World.Get immediately after Create

Readers (EntityExtensions, the generated accessors) also index Worlds with
no publication barrier, so on weakly-ordered CPUs (ARM64) a resized array can
become visible before its copied contents.

We hit this constantly in CI (12 of 15 recent failed runs on our project) with
NUnit fixtures creating worlds in parallel; the victim test differed every run,
and about a third of the failures escalated to a fatal AV that killed the test
host.

Fix

  • Guard create/destroy with a dedicated WorldsLock object whose identity
    never changes.
  • On resize, fill the new world's slot in the copy before publishing the
    array with Volatile.Write; non-resize slot writes are volatile element
    writes.
  • Readers stay lock-free and unchanged: element loads carry an address
    dependency on the array reference, so release-publication is sufficient.
    No hot-path cost.

Tests

  • ConcurrentWorldCreateProducesUniqueUsableWorlds — 8 threads × 64 worlds
    created concurrently (forcing several resizes), each world used immediately
    through the static lookup, then id-uniqueness and slot identity asserted.
  • ConcurrentWorldCreateDestroyChurnDoesNotCorruptStaticStorage — 8 threads ×
    200 create/use/destroy rounds so id recycling, slot writes and resizes
    interleave.
  • WorldRecycle now drains the recycled-id queue before asserting reuse, so it
    no longer depends on the queue being empty when the fixture runs.

Both new tests fail against the current locking in 3/3 runs (NRE / duplicate
ids) and pass with the fix. The test file is #if !PURE_ECS — the static
registry it exercises does not exist under PURE_ECS. Full suite green
(net8.0, Debug + Release × Default / Events / PureECS).

@penspanic
penspanic force-pushed the pr/static-worlds-lock branch from d10c677 to de84389 Compare July 14, 2026 04:40
World.Create and World.Dispose locked the static Worlds array itself, but
Create replaces that array on resize. After a resize, a creator blocked on the
old array's monitor and a newly arriving creator locking the new array enter
the "critical section" concurrently: two worlds can be handed the same id
(cross-wiring EntityInfo lookups into AccessViolation deep in Chunk), and a
slot write can land in the stale array so the live Worlds array holds null for
a created world (NullReferenceException in EntityExtensions/World.Get right
after Create). Readers also indexed Worlds with no publication barrier, so on
weakly-ordered CPUs (ARM64) a resized array could become visible before its
copied contents.

Guard create/destroy with a dedicated lock object, fill the resized copy
before publishing it with a volatile write, and write slots volatile. Readers
stay lock-free: element loads carry an address dependency on the array
reference, so release-publication is sufficient.

Adds WorldConcurrencyTest covering concurrent create (forcing resizes) and
create/use/destroy churn — both fail against the previous locking in 3/3 runs.
WorldRecycle now drains the recycled-id queue first so it no longer depends on
the queue being empty when the fixture runs.

Co-Authored-By: Claude Fable 5 <[email protected]>
@penspanic
penspanic force-pushed the pr/static-worlds-lock branch from de84389 to 583177b Compare July 14, 2026 04:45
@genaray
genaray merged commit cfbb343 into genaray:master Jul 15, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants