Skip to content

Fix world: Skip dead targets in the world:targets iterator#333

Closed
PRMS-magnet wants to merge 1 commit into
Ukendio:mainfrom
PRMS-magnet:fix/world-targets-dead-id
Closed

Fix world: Skip dead targets in the world:targets iterator#333
PRMS-magnet wants to merge 1 commit into
Ukendio:mainfrom
PRMS-magnet:fix/world-targets-dead-id

Conversation

@PRMS-magnet

Copy link
Copy Markdown

Brief Description of your Changes

world:targets yields a non-alive entity id for a relationship target that is deleted after the iterator is created but before that target is reached. world:target and world:contains both report the target as gone, so only the targets iterator surfaces the bad id. Reading each target and deleting it in the same loop is unaffected, since every target is still alive when it is read.

No existing issue for this, so here is a deterministic repro:

local world = jecs.world()
local pair = jecs.pair

local ROOT = world:entity()
local e = world:entity()
local a, b, c = world:entity(), world:entity(), world:entity()
world:add(e, pair(ROOT, a))
world:add(e, pair(ROOT, b))
world:add(e, pair(ROOT, c))

local iter = world:targets(e, ROOT) -- snapshot taken here, includes b
world:delete(b)                     -- b dies after the iterator was created

for t in iter do
	print(t, world:contains(t))     -- yields b's ghost id with contains == false
end
print(world:target(e, ROOT, 0), world:target(e, ROOT, 1)) -- a, c, the correct answer

world:targets snapshots the archetype types, the start offset and the count when the iterator is created, then resolves each target with a raw dense_array[sparse_array[...].dense]. Unlike world:target, which resolves through entity_index_get_alive, the iterator never checks dense > alive_count and never guards a missing sparse record. When a target dies after the snapshot, its dense slot moves past alive_count, so the raw read returns the generation-bumped ghost of the dead target, or a live entity that later reused the slot.

The fix resolves each target through entity_index_get_alive, the same helper world:target uses, and skips targets that are no longer alive instead of yielding them. The now unused captured sparse_array and dense_array locals are dropped.

Impact of your Changes

Corrects the bad target id. world:targets now yields only live targets, matching world:target and the existing "should ignore deleted targets" intent. The common all live path is unchanged in behavior. No data model or API change.

Tests Performed

Added a regression test beside the existing world:targets cases that deletes a target after obtaining the iterator and checks every yielded id is contained. The existing "should ignore deleted targets" case deletes before the iterator is created, so cleanup removes the pair before the snapshot and never exercised this path.

The full suite stays at 140/140. The new test fails on stock (139/140) and passes with the fix. Also confirmed against a standalone repro on current main.

Additional Comments

world:target already guards this through entity_index_get_alive. The plural iterator just missed it, so I kept the fix minimal by reusing the same helper rather than inlining.

PR #317 rewrites world_targets and keeps the same raw lookup, so it would need this guard too.

world:targets snapshotted the archetype types and resolved each target with
a raw dense lookup, dropping the dense > alive_count guard that world:target
uses via entity_index_get_alive. A target deleted after the iterator was
created came back as a generation-bumped ghost id, or a live entity that
reused the freed dense slot. Resolve through entity_index_get_alive and skip
dead targets instead of yielding them.

Added a regression test that deletes a target after obtaining the iterator;
fails on stock (139/140), passes with the fix (140/140).
Copilot AI review requested due to automatic review settings June 29, 2026 20:11
@PRMS-magnet
PRMS-magnet marked this pull request as draft June 29, 2026 20:12
@PRMS-magnet PRMS-magnet changed the title fix(world): Skip dead targets in the world:targets iterator Fix world: Skip dead targets in the world:targets iterator Jun 29, 2026
@PRMS-magnet
PRMS-magnet marked this pull request as ready for review June 29, 2026 20:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a correctness gap in world:targets() where the iterator could yield non-alive (generation-bumped “ghost”) targets if a relationship target is deleted after the iterator is created but before that target is reached. The iterator now resolves targets via the same liveness-aware helper used by world:target(), ensuring only live targets are yielded.

Changes:

  • Update world_targets to resolve each candidate target with entity_index_get_alive(...) and skip targets that are no longer alive.
  • Remove now-unused captured sparse_array / dense_array locals from the iterator closure.
  • Add a regression test covering deletion of a target after iterator creation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/jecs.luau Makes world:targets() skip non-alive targets by resolving through entity_index_get_alive, aligning behavior with world:target().
test/tests.luau Adds a deterministic regression test ensuring world:targets() never yields deleted targets created in the iterator snapshot.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Ukendio

Ukendio commented Jun 29, 2026

Copy link
Copy Markdown
Owner

I dont like this solution, targets is meant to be a drained iterator, i.e. use right after being defined. unfortunately we cant enforce this via the type system so undefined behaviour such as this will pop up. We might have to consider just deleting world:targets as an utility if it is big enough of a footgun.

@Ukendio Ukendio closed this Jun 29, 2026
@kurokuukyo

Copy link
Copy Markdown
Contributor

This kind of bug makes sense with the current design of world:targets(...) only considering immediate usage in a loop:

for target in world:targets(e, component) do
    ...
end

It could be redesigned to compute more on first iteration to avoid this specific issue. @Ukendio thoughts?

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.

4 participants