[apotema]

The defect

tick runs renderer.sync in its always-run block (loop_mixin.zig:138), well before active_scene_update_fn (:314). A shader material replaced or cleared during that update had its backend material destroyed on the spot — while the imminent render() was still holding the id the sync had cached. That frame was submitted with a dead id and drawn through the plain-sprite fallback: a one-frame flash that reads as "the shader stuttered" and never reproduces on demand. A fixed-time golden cannot catch it either, because goldens sample after a sync.

The same ordering hits the curated setMaterial path, which clears the shader slot through clearShaderMaterial — which is why this was kept off the merge-gated #882.

The fix, and why this one

Retire list (the issue's option 1). releaseRecord queues the backend id on the world (World.shader_retire) instead of destroying it; tick flushes the queue immediately after the next renderer.sync — the first moment no cached draw can still name it. So a submitted id is always live for the frame it was cached in.

Chosen over "resync the affected entity inline on change" because it puts no sync work on the update path, needs no care with the world/renderer split, and covers the curated setMaterial path for free. It is also the conventional answer in renderers with cached draw lists.

Bounded, and not a leak by deferral:

• Extra lifetime is exactly one frame — the next sync's flush frees it.
• World.deinit flushes what is left while the renderer is still alive, so a material retired after the last sync is still destroyed.
• The whole-context retirements (clearShaderMaterials / invalidateShaderMaterials) drop the queue instead of destroying: those already dropped every backend material, and .invalidate means the context is gone, where running the destructor on a stale handle is UB.
• OOM on the one-pointer append falls back to destroying immediately — a one-frame fallback flash beats a leak.

Behavioural proof

test/shader_material_retire_test.zig asserts the mechanism, with a mock renderer shaped like a real one: sync snapshots each sprite's material id, render walks that snapshot and books every draw as a material_draw (live id), a fallback_draw (.none), or a dead_submit (an id the backend already destroyed — the defect). Each test drives real tick + render frames with a real active_scene_update_fn that swaps the material mid-update, and asserts the material path ran that frame — not merely that nothing crashed.

Restoring the immediate destroy (one line) turns all five red:

Build Summary: 257/260 steps succeeded (2 failed); 1399/1405 tests passed (6 failed)
  expected 0, found 1     <- dead_submits

Coverage: replace mid-update, clear mid-update, curated setMaterial mid-update (with the curated effect surviving the shader's retirement), destroyed-on-teardown, and destroyed-exactly-once (a second flush destroys nothing).

The four existing shader_material_test.zig assertions that read destroys right after a replace/clear now flush first, which pins the new contract explicitly (0 before the flush, N after).

Verification

• zig build clean, zig fmt --check clean on touched files.
• zig build test --summary all → 260/260 steps succeeded; 1405/1405 tests passed, exit 0 (baseline on main: 258/258, 1400/1400).

Closes #883
Refs #882

https://claude.ai/code/session_01XBGRLojiysEr5zy1U5mbP6

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: b4bf4b46-340c-4106-ac4e-75aa1b73294e

📥 Commits

Reviewing files that changed from the base of the PR and between 6b98478 and 0a084b7.

📒 Files selected for processing (6)

• build.zig
• src/game.zig
• src/game/loop_mixin.zig
• src/game/shader_material_mixin.zig
• test/shader_material_retire_test.zig
• test/shader_material_test.zig

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.

---

📝 Walkthrough

Walkthrough

The change defers shader-material destruction until after renderer synchronization. It adds per-world retirement queues, cleanup handling, and tests for cached rendering, texture-pin lifetime, context invalidation, teardown, and world isolation.

Changes

Shader Material Retirement

Layer / File(s)	Summary
Retirement lifecycle src/game.zig, src/game/shader_material_mixin.zig	Worlds retain complete retired material records. The queue preserves catalog pins until destruction, supports immediate fallback on allocation failure, and handles teardown and context invalidation.
Renderer integration and validation src/game/loop_mixin.zig, test/shader_material_retire_test.zig, test/shader_material_test.zig, build.zig	tick flushes retired materials after renderer synchronization. Tests cover cached submissions, destruction timing, texture ownership, multiple worlds, and teardown. The new regression suite is included in the standard tests.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~30 minutes

Change: Bug fix · Severity of issue fixed: Medium

sequenceDiagram
  participant active_scene_update_fn
  participant Game
  participant Renderer
  participant ShaderRetirementQueue
  active_scene_update_fn->>ShaderRetirementQueue: queue replaced material
  Game->>Renderer: synchronize cached draw state
  Game->>ShaderRetirementQueue: flush retired materials
  ShaderRetirementQueue->>Renderer: destroy backend material

Loading

Merge Risk: ⚪ Minimal · up to 0a084

The deferred material lifetime, cleanup, and per-world synchronization paths are covered without a remaining merge-blocking issue.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: deferring shader material destruction until after the next renderer sync.
Description check	✅ Passed	The description directly explains the defect, the retire-list fix, lifetime behavior, test coverage, and verification results. It is fully related to the changeset.
Linked Issues check	✅ Passed	Issue #883 coding requirements are met. shader_material_mixin.zig queues retired backend IDs with their texture pins, tick flushes only the active world's queue immediately after renderer.sync, …
Out of Scope Changes check	✅ Passed	The changed files stay within Issue #883. The World retire queue, flush API, loop integration, shader-material lifetime handling, build test registration, and additional texture-pin and world-lifeti…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-18T19:42:51.595122Z	0a084b7	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2044990b95

ℹ️ 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".

[chatgpt-codex-connector]

Keep texture pins alive until retired materials are flushed

When a material using a catalog texture is cleared or replaced mid-update and owns the last reference, this now defers destruction of the material ID but still immediately calls assets.release for its textures. AssetCatalog.release unloads a ready GPU texture as soon as its refcount reaches zero, so the renderer's already-cached material remains live for the imminent render while referencing an unloaded or recycled texture handle. Retain the texture pins with the retired material and release them only when the material is destroyed after the next sync.

Useful? React with 👍 / 👎.

[apotema]

Fixed in 0a084b7 — this was the real one, and it was a use-after-free created by the deferral itself.

The queue now carries the whole sm.Record, so the catalog pins travel with the material and are released in freeRecordPins at the moment it is actually destroyed. dropWorldRetired drops only the BACKEND destroy (a whole-context clear already killed every material) and still releases the pins — the asset catalog is an engine-side refcount that outlives the GPU context.

Teardown ordering, since this is the trap: Game.deinit tears the catalog down right after clearAllShaderMaterials, so the release has to happen inside that call. The .destroy path on a renderer without clearShaderMaterials now flushes the queue inline, which means clearWorldShaderMaterials always leaves it empty — and every World.deinit call site runs that first. World.deinit keeps a backstop that frees the backend material and the owned copies but does not touch refcounts (it cannot reach the catalog from a World, and by then it may be gone).

Mechanism test rather than a value-only one: destroyShaderMaterial snapshots the image backend's unload tally, so the test fails on the ORDERING (unloads_at_destroy: expected 0) and not on a final count a wrong order also reaches. Against a faithful revert (queue the id only) both new tests go red at "the pin is still held after the retire". A third test asserts the pins are released inside clearAllShaderMaterials — red when that path stops flushing, which is the deinit-ordering trap above.

The five existing assertions that read a refcount immediately after a clear now flush first, mirroring what the original commit did for the destroys counters.

[chatgpt-codex-connector]

Flush only the world whose renderer was synchronized

This flushes retired IDs for every inactive world even though tick has synchronized only active_world. For example, if world A retires a material after its last sync and is then shelved, the next tick can sync B and destroy A's retired ID here; switching back to A later in that tick makes render() consume A's unchanged cached draw list containing the now-dead ID. Flush only the active world after its sync, or otherwise invalidate/synchronize each inactive renderer before destroying its retired resources.

Useful? React with 👍 / 👎.

[apotema]

Fixed in 0a084b7. Confirmed: tick synchronizes self.renderer — the active world's — and nothing else, so flushing every world destroyed ids a shelved world's cached draw list can still name. The #883 defect, reintroduced for inactive worlds.

flushRetiredShaderMaterials is now active-world-only. A shelved queue waits for its own sync (the world becoming active again), and is released wholesale if the world is torn down (clearWorldShaderMaterials, which destroyWorld and the unnamed-shelve path both run) or its context dies (invalidateAllShaderMaterials).

Test: retire a material in world A after a frame so A's renderer has cached it, shelve A, run two frames on B, then assert A's sync count is unchanged, its queue still holds the entry, its renderer issued no destroy — and, as the mechanism rather than the count, that rendering A's stale list books zero dead submits. Reactivating A destroys it on the next frame. Restoring the flush-every-world loop turns it red at expected 1, found 0. A second test tears a shelved world down with a queued material and asserts the backend create/destroy tallies balance, so skipping inactive worlds cannot become a leak.

[apotema]

Review findings on the current head. Reproduction details and suggested fixes are attached inline.

[apotema]

[P1] Retain catalog texture pins until the deferred material is destroyed

releaseRecord now queues the material ID for later destruction, but immediately releases all of its catalog texture pins below. If this material owns the last reference to a ready texture, AssetCatalog.release synchronously calls the loader's free, so the cached draw still references a live material whose texture has already been unloaded. This leaves the late-update clear/replacement case broken for textured materials.

Reproduced on this head with an added local regression: register and load a catalog texture, create a material holding its only pin, sync, then clear the material. Before the retirement flush, renderer.destroys == 0 but ImageBackend.unloads == 1; the expected-zero unload assertion fails. The existing 17 shader lifecycle tests pass.

Retire the owned texture pins along with the material and release them only after the renderer has stopped referencing that material. Add a last-owner textured-material case to the deferred-lifetime tests.

[apotema]

Fixed in 0a084b7 — see the reply on the sibling thread for the full write-up. Short version: the retire queue now holds the whole sm.Record, so the pins are released at flush time alongside the destroy; dropWorldRetired still releases them (dropping only the backend destroy); and clearWorldShaderMaterials now always empties the queue so the release lands inside Game.deinit's clearAllShaderMaterials, while the catalog is still alive.

[apotema]

[P2] Flush a world's retired materials only after that world's renderer syncs

The tick site synchronizes only self.renderer, but this loop also destroys retired materials in every inactive world. Those worlds still retain their previous cached draw lists. Switching back after the active world's sync can therefore render a cached ID that this loop has already destroyed.

Reproduced with the PR's CachingRenderer: create a shader in named world A, tick A to cache it, clear the shader, switch to B, tick B, switch back to A, and render. The assertion dead_submits == 0 fails with 1. The five existing retirement tests pass alongside this failing probe.

Flush only the world whose renderer was synchronized, keeping inactive queues until their own synchronization or teardown (or explicitly synchronize an activated world before flushing/drawing it).

[apotema]

Fixed in 0a084b7 — see the sibling thread for the detail. The flush is now active-world-only; a shelved world's queue waits for its own sync, and is released wholesale by clearWorldShaderMaterials / invalidateAllShaderMaterials if the world is torn down or its context dies first. Covered by a two-world test that goes red against the flush-every-world loop.

[apotema]

Both findings addressed in 0a084b7. Both were real, and the P1 was a use-after-free introduced by this PR's own fix.

[P1] Catalog pins released while the material is still queued. releaseRecord deferred the backend destroy but released the record's catalog texture pins on the spot, so a material owning the last reference to a ready texture freed that texture while it was still queued — and the flush then destroyed a material whose texture was already gone. The queue now carries the whole sm.Record; the pins are released in freeRecordPins at the moment the material is actually destroyed. dropWorldRetired drops only the BACKEND destroy and still releases the pins (the asset catalog is an engine-side refcount that outlives the GPU context). The .destroy path on a renderer without clearShaderMaterials now flushes inline, so clearWorldShaderMaterials always leaves the queue empty — which is what keeps the release inside Game.deinit's clearAllShaderMaterials, while the catalog is still alive, rather than in World.deinit, which runs after it.

[P2] Flush only the synced world. tick synchronizes self.renderer alone, so flushing every world destroyed ids a shelved world's cached draw list could still name. The flush is now active-world-only; a shelved queue waits for its own sync, and is released wholesale if the world is torn down or its context dies.

Mechanism, not values. destroyShaderMaterial snapshots the image backend's unload tally, so the P1 test fails on the ORDERING (unloads_at_destroy: expected 0) rather than on a count a wrong order also reaches. Every new test was force-failed against a faithful revert and confirmed red with the totals including it:

• queue the id only → both P1 tests red at "the pin is still held after the retire"
• flush every world again → the two-world test red at expected 1, found 0
• stop flushing in the .destroy path → the whole-context test red at expected 0, found 1 (the deinit-ordering trap)

The five existing assertions that read a refcount immediately after a clear now flush first, mirroring what the original #883 commit did for the destroys counters — the pin lives until the flush by design.

Totals (zig build and zig build test --summary all, exit 0 each, checked separately from any pipe): 1400/1400 on main, 1405/1405 before this change, 1410/1410 after, 260/260 steps. zig fmt --check clean on the four touched files.

https://claude.ai/code/session_01XBGRLojiysEr5zy1U5mbP6