fix(core): harden render pipeline resource lifecycle - #3065
Conversation
WalkthroughRender pipeline resource lifetime handling is expanded: instance buffers are protected from ordinary GC, pipeline passes now release and destroy owned resources idempotently, and render-target pools reject duplicate frees. Tests cover buffer lifetime, camera teardown, and pool reuse. ChangesRender pipeline resource lifetime
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3065 +/- ##
===========================================
+ Coverage 79.52% 79.83% +0.30%
===========================================
Files 904 906 +2
Lines 101168 101342 +174
Branches 11377 11621 +244
===========================================
+ Hits 80456 80906 +450
+ Misses 20528 20251 -277
- Partials 184 185 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
二轮审查 @ 882db26c — 唯一 P3 已闭环，LGTM
自首轮 207d03584c 起线性 +1 commit（compare ahead_by:1/behind_by:0 无 force-push）：882db26c chore(core): remove instance buffer comments，仅 InstanceBuffer.ts 删 2 行注释、isGCIgnored=true 逻辑逐字不变。
首轮唯一 [P3]（新增两行 // 注释末尾带句号、不合本目录 // 惯例）作者选择整段删除这两行 // 注释——闭环，本目录 // 无句号惯例恢复。核心修复（InstanceBuffer 裸 buffer 打 isGCIgnored=true 防 ResourceManager.gc() 回收 active GPU-instancing buffer）与首轮逐字一致，本轮零逻辑改动无需复核。CI 全绿。
无 P0/P1/P2/P3。可合。
sun-nocat
changed the title
![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 `@tests/src/core/Camera.test.ts`:
- Around line 138-152: Add spies for saoPass.release and finalPass.release to
the releaseSpies collection in the lifecycleCamera.destroy test, alongside the
existing pass release spies, so the existing toHaveBeenCalledOnce assertions
verify all delegated release calls.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 95a109c6-5f4d-4e6f-862f-1516f9ed3472
📒 Files selected for processing (8)
packages/core/src/RenderPipeline/BasicRenderPipeline.tspackages/core/src/RenderPipeline/OpaqueTexturePass.tspackages/core/src/RenderPipeline/RenderTargetPool.tspackages/core/src/lighting/ambientOcclusion/ScalableAmbientObscurancePass.tspackages/core/src/postProcess/FinalPass.tspackages/core/src/shadow/CascadedShadowCasterPass.tstests/src/core/Camera.test.tstests/src/core/RenderPipeline/RenderTargetPool.test.ts
| const releaseSpies = [ | ||
| vi.spyOn(cascadedShadowPass, "release"), | ||
| vi.spyOn(depthOnlyPass, "release"), | ||
| vi.spyOn(opaqueTexturePass, "release") | ||
| ]; | ||
| const saoMaterial = saoPass["_material"]; | ||
| const finalSrgbMaterial = finalPass["_sRGBmaterial"]; | ||
| const finalAntiAliasingMaterial = finalPass["_antiAliasingMaterial"]; | ||
|
|
||
| lifecycleCamera.destroy(); | ||
|
|
||
| for (const releaseSpy of releaseSpies) expect(releaseSpy).toHaveBeenCalledOnce(); | ||
| expect(saoMaterial.destroyed).to.eq(true); | ||
| expect(finalSrgbMaterial.destroyed).to.eq(true); | ||
| expect(finalAntiAliasingMaterial.destroyed).to.eq(true); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert release for the SAO and final passes too.
BasicRenderPipeline.destroy() now calls saoPass.destroy() and finalPass.destroy(), both of which delegate to release(). Add spies for saoPass.release and finalPass.release; otherwise a regression that removes either release call could still pass while the material assertions remain green.
🤖 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 `@tests/src/core/Camera.test.ts` around lines 138 - 152, Add spies for
saoPass.release and finalPass.release to the releaseSpies collection in the
lifecycleCamera.destroy test, alongside the existing pass release spies, so the
existing toHaveBeenCalledOnce assertions verify all delegated release calls.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
三轮增量审查覆盖 882db26c963b220636c4cbbba6a422b1df19d943 → 39a191d62ccb95e5767d62cab23f8670326523ba 的线性单 commit，以及 Camera → BasicRenderPipeline → 各 pass → RenderTargetPool 的直接上下游。目标 HEAD 为 39a191d62ccb95e5767d62cab23f8670326523ba。本轮发现 1 个 P1（阻塞）和 1 个 P2，实际 review 动作为 REQUEST_CHANGES；自动 CR 不替代人工 Reviewer 的合入门禁。
已关闭问题清单
- 首轮 [P3]：
InstanceBuffer.ts两行单行注释末尾句号已由882db26c chore(core): remove instance buffer comments删除；isGCIgnored = true的生命周期修复逐字保留，本轮不重复提出。
问题
-
[P1]
packages/core/src/RenderPipeline/RenderTargetPool.ts:122-131—includes不是 lease ownership 校验，无法兑现“不会把同一资源租给两个消费者”的契约。 当前检查只挡连续两次free(A)；执行free(A) → allocate()（B 得到 A）→旧 owner 再free(A)时，A 已不在 free-list，includes返回 false，A 会在 B 仍使用期间重新入池，随后又可分配给 C。沿所有直接调用点核对后，真正的权威 owner 是各 pass/manager 的持有字段：release()在归还后置null，PipelineUtils.recreate*负责把旧 lease 交还并让调用方覆盖 slot；pool 只应拥有已归还的 free-list。请删除这两个不完整的includes校验及RenderTargetPool.test.ts:100-109/124-133两个锁定无效兼容契约的测试，保留并验证各 owner 的 exact-once 归还与清空。若未来确实要在 pool 强制验证 stale owner，需要带 generation/token 的 lease 协议，而不是从 free-list 反推 ownership；这不必扩进本 PR。 -
[P2]
tests/src/core/Camera.test.ts:129-154— 新测试没有创建任何待释放的 render target，无法回归本次资源泄漏修复。 该 Camera 从未渲染，shadow/depth/SAO/opaque/final pass 的 target 均为空；因此测试只证明私有方法被调用和三个 Material 被销毁，即使删除OpaqueTexturePass.release()、shadow target 归还或 shadow binding 清理，仍可通过。当前codecov/patch也对应失败：shadow 新逻辑缺 9 行、opaque 缺 3 行。请通过真实配置启用相关 pass 并至少渲染一帧，再销毁 Camera，断言实际 target 已归还/在 pool GC 时销毁、scene shadow texture binding 已按 owner 清除；另用 shadow resolution/cascade 变化覆盖旧 target 经PipelineUtils.recreateRenderTargetIfNeeded()归还的路径。现有私有 call-spy fixture 应按这些行为契约重写；仅再补 SAO/Final 的releasespy 仍锁实现且不能守住泄漏。
架构、熵增与测试治理
- 上游唯一生命周期入口是
Camera._onDestroy()；EngineObject.destroy()已保证 Camera teardown 只执行一次。下游资源流应保持Camera → BasicRenderPipeline → pass 持有字段 → freeRenderTarget/freeTexture → pool free-list → pool.gc()，各 pass 字段是 lease 的权威 owner，pool 从归还时才接管资源。 - shadow 设置变化链路中，删除
_updateShadowSettings()里提前把_renderTarget置空是正确的：旧 target 现在能继续传给PipelineUtils.recreateRenderTargetIfNeeded()，由该唯一转换/归还 owner 判断复用或入池。 - 增量净变化新增了 pass teardown 入口和 material 的显式销毁，方向正确；但同时新增的两条 free-list
includes校验把 caller ownership 下沉成不完整的第二份真相，且两个测试把该兼容分支固化。删除它们后，owner/状态/校验数量回到单一路径，不需要 legacy fallback、镜像 lease 状态或第二条校验协议。 InstanceBuffer的 GC owner、显式 BatcherManager teardown 和原回归测试均未变化，既有结论继续成立。
2 participants
Summary
BasicRenderPipelineis destroyedRoot causes
Instance buffer lifetime
InstanceBufferretained its GPUBufferthrough a plain engine-owned field, so it had no component reference count. Because the buffer was not excluded from ordinary resource GC,ResourceManager.gc()could destroy it while the render pipeline still held and reused the wrapper. The next upload then reached a destroyedGLBuffer, causing repeatedbindBufferfailures and WebGL context loss.Camera pipeline teardown
BasicRenderPipeline.destroy()previously released only its culling results and two top-level pooled resources. Camera-owned shadow, depth, SAO, opaque-texture, and final passes could therefore retain render targets or internal materials after camera destruction. In addition, the shadow pass discarded its current render-target reference beforePipelineUtils.recreateRenderTargetIfNeeded()could return an incompatible target to the pool, and the pool accepted duplicate returns of the same object.Ownership boundaries
ResourceManager.gc()must not reclaim engine-owned buffers that remain active in the render pipeline.Cameraowns itsBasicRenderPipeline; destroying the camera releases every resource owned by that pipeline exactly once.RenderTargetPoolowns reuse after a resource is returned and ignores duplicate returns; it remains responsible for destroying pooled resources during pool GC or engine teardown.BatcherManager.destroy()and engine teardown still destroy the instance buffer.Validation
pnpm run b:moduleHEADLESS=true pnpm exec vitest run tests/src/core/RenderPipeline/InstanceBuffer.test.ts tests/src/core/RenderPipeline/RenderTargetPool.test.ts tests/src/core/Camera.test.ts(3 files, 32 tests)pnpm run b:typesgit diff --check origin/dev/2.0...HEADgit merge-tree --write-tree origin/dev/2.0 HEAD(clean virtual merge)Fixes #3064
Summary by CodeRabbit