Repository navigation
feat: own the dynamic ChildrenComponent lifecycle (no MAX_CHILDREN cap) - #797
Conversation
Pairs with labelle-core's dynamic `ChildrenComponent` (ArrayList-backed, no `MAX_CHILDREN`). Since the ECS backends store components by value and run no destructors, the engine — which exclusively manages the hierarchy — owns the child-list heap lifecycle at every choke point: - `setParent` / nested-prefab spawn thread `game.allocator` into `addChild`. - `destroyEntity` copies child ids onto an INDEPENDENT allocation before the cascade (the old shallow struct snapshot would now alias — and outlive the free of — the heap-backed list), then frees the entity's own list. - `destroyEntityOnly` frees the dying entity's list (children stay alive). - `resetEcsBackend` and `World.deinit` free every `ChildrenComponent` before the ECS wipe drops them by value. `World` now retains its allocator for the teardown free. Removes the silent-drop-past-16 orphaning of slot-heavy rooms/workstations (#657). Verified leak-free: the engine test suite runs under the DebugAllocator's leak detector — a first cut that missed the `World.deinit` free was caught by the prefab-with-children tests. `#701`'s repeated-destroy test is re-pinned to the dynamic model. Requires labelle-core with dynamic `ChildrenComponent`. Claude-Session: https://claude.ai/code/session_011szWvquoss1yNX7KWSKCaM
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change propagates the world allocator into child-list creation and explicitly deinitializes ChangesChildren allocation lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review |
![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/game/entity_mixin.zig`:
- Around line 242-245: Update the child iteration in destroyEntity to check each
snapshot entry is still live before recursively destroying it, so hooks that
already destroyed a sibling are skipped. Perform the liveness check immediately
before destroyEntity and preserve identity semantics when ECS IDs may be
recycled during hooks, avoiding destruction of a recycled unrelated entity.
🪄 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 Plus
Run ID: 5312c42c-f37a-4cb3-9f66-e2cf27ae09cd
📒 Files selected for processing (6)
src/game.zigsrc/game/components_mixin.zigsrc/game/entity_mixin.zigsrc/game/world_mixin.zigsrc/jsonc/scene_loader/nested_spawn.zigtest/destroy_unlink_test.zig
| const kids = self.allocator.dupe(Entity, children_comp.getChildren()) catch @panic("OOM: destroyEntity children snapshot"); | ||
| defer self.allocator.free(kids); | ||
| for (kids) |child| { | ||
| destroyEntity(self, child); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip children already destroyed during the cascade.
Line 244 can re-destroy a later snapshot entry when an earlier child’s entity_destroyed hook destroys that sibling. This can hit a dead or recycled ECS ID and corrupt an unrelated entity. Check liveness before recursion; preserve identity semantics if IDs can be recycled during hooks.
Proposed minimum guard
for (kids) |child| {
+ if (!self.ecs_backend.entityExists(child)) continue;
destroyEntity(self, child);
}📝 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.
| const kids = self.allocator.dupe(Entity, children_comp.getChildren()) catch @panic("OOM: destroyEntity children snapshot"); | |
| defer self.allocator.free(kids); | |
| for (kids) |child| { | |
| destroyEntity(self, child); | |
| const kids = self.allocator.dupe(Entity, children_comp.getChildren()) catch `@panic`("OOM: destroyEntity children snapshot"); | |
| defer self.allocator.free(kids); | |
| for (kids) |child| { | |
| if (!self.ecs_backend.entityExists(child)) continue; | |
| destroyEntity(self, child); |
🤖 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/game/entity_mixin.zig` around lines 242 - 245, Update the child iteration
in destroyEntity to check each snapshot entry is still live before recursively
destroying it, so hooks that already destroyed a sibling are skipped. Perform
the liveness check immediately before destroyEntity and preserve identity
semantics when ECS IDs may be recycled during hooks, avoiding destruction of a
recycled unrelated entity.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8efe7e45e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // backend drops the component by value (the ECS runs no | ||
| // destructor). Re-fetch: the cascade above may have relocated the | ||
| // Children pool under any pointer held before it. | ||
| if (self.ecs_backend.getComponent(entity, Children)) |cc| cc.deinit(self.allocator); |
There was a problem hiding this comment.
In preview runs, destroying an entity that still has children now frees the Children backing list before emitEntityDestroyed, while the next comment preserves the contract that preview consumers can still introspect the entity before the backend destroy. In that scenario the entity still has a Children component, but getChildren would return a slice into the freed ArrayList allocation, so editor-side inspection can see dangling memory or crash; move this deinit until after the preview notification but before ecs_backend.destroyEntity.
Useful? React with 👍 / 👎.
`destroyEntity` freed the entity's own `Children` list right after the child cascade — but that's BEFORE `emitEntityDestroyed`, and the preview contract lets an editor-side consumer still introspect the entity (via a `getComponent`/`getChildren`-style API) during that telemetry. Freeing first handed such a consumer a slice into a freed `ArrayList` (UAF / crash). Move the free to just before `ecs_backend.destroyEntity` — after the preview emit and the parent-unlink — so `getChildren` stays valid through the notification. (`destroyEntityOnly` already frees after its top-of-fn preview emit, so it was fine.) Claude-Session: https://claude.ai/code/session_011szWvquoss1yNX7KWSKCaM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bb1b72b52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (self.ecs_backend.getComponent(parent_entity, Children)) |children_comp| { | ||
| children_comp.addChild(child); | ||
| children_comp.addChild(self.allocator, child); |
There was a problem hiding this comment.
Because this line now allocates the Children backing list with self.allocator, the public generic component paths are still an unhandled drop path: removeComponent(parent, Game.ChildrenComp) and overwriting it through setComponent/addComponent still go straight to the ECS, which drops the old component by value and never reaches the new destroy/reset cc.deinit calls. Any game or tooling code that strips or replaces ChildrenComp directly will leak that entity's child-list allocation; special-case Children in those generic paths or make direct removal inaccessible.
Useful? React with 👍 / 👎.
`ChildrenComponent` owns a heap list; the engine frees it at its hierarchy choke points (setParent/destroy/reset), but the public GENERIC component API bypasses those — `removeComponent(e, ChildrenComp)`, or an `addComponent` / `setComponent` that overwrites an existing `ChildrenComponent`, dropped the old value by value and leaked its ArrayList. Special-case `Children` in `addComponent` / `setComponent` / `removeComponent` to `deinit` the existing list before the by-value drop. These guards only fire on direct external manipulation — the engine's own `setParent` goes through `ecs_backend.addComponent` on the no-existing branch, so no double-free. New test drives all three generic paths on a 20-child parent under `testing.allocator` (leak = fail). Claude-Session: https://claude.ai/code/session_011szWvquoss1yNX7KWSKCaM
|
@codex review |
|
Merging on green (CI CLEAN, CodeRabbit pass). codex's round-3 inline is a stale re-post of the generic-removal P2 anchored to the |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…797) Engine owns the child-list heap lifecycle for core's ArrayList-backed ChildrenComponent (no MAX_CHILDREN) at every choke point: setParent / destroy(+Only) / resetEcsBackend / World.deinit, plus the generic add/set/removeComponent paths. Requires labelle-core >= v1.27.0. Claude-Session: https://claude.ai/code/session_011szWvquoss1yNX7KWSKCaM
…797) Engine owns the child-list heap lifecycle for core's ArrayList-backed ChildrenComponent (no MAX_CHILDREN) at every choke point: setParent / destroy(+Only) / resetEcsBackend / World.deinit, plus the generic add/set/removeComponent paths. Requires labelle-core >= v1.27.0. (.version was stale at 2.4.0 while tags ran to v2.7.1; realigned to the tag.) Claude-Session: https://claude.ai/code/session_011szWvquoss1yNX7KWSKCaM
What
Pairs with labelle-core#65 (dynamic
ChildrenComponent— ArrayList-backed, noMAX_CHILDREN). The ECS backends store components by value and run no destructors, so the engine — which exclusively manages the hierarchy — owns the child-list heap lifecycle at every choke point:setParent/ nested-prefab spawn threadgame.allocatorintoaddChild.destroyEntitycopies child ids onto an independent allocation before the cascade — the old shallow struct snapshot (children_comp.*) would now alias, and outlive the free of, the heap-backed list — then frees the entity's own list (re-fetched, since the cascade can relocate the pool).destroyEntityOnlyfrees the dying entity's list (its children stay alive).resetEcsBackendandWorld.deinitfree everyChildrenComponentbefore the ECS wipe drops them by value.Worldnow retains its allocator for the teardown free.Removes the silent-drop-past-16 orphaning of slot-heavy rooms/workstations (consumer #657).
Verification
The engine test suite runs under the DebugAllocator's leak detector. A first cut that missed the
World.deinitfree was caught by the prefab-with-children tests; with all choke points covered the full suite passes leak-free.#701's repeated-destroy test is re-pinned to the dynamic model (no moreMAX_CHILDREN).Depends on
labelle-core#65 — merge that first. This engine build compiles against core's new
addChild(allocator, …)/deinit(allocator)API; CI here is red until core main carries the change.https://claude.ai/code/session_011szWvquoss1yNX7KWSKCaM
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.