fix(core): load fresh runtime scene on same-url reload - #3100
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. WalkthroughScene loading now bypasses the scene loader cache and uses loop-safe iteration to destroy old scenes. ChangesLoading lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Same-URL uncached loads now produce independent runtime results while cached loads retain sharing and cancellation behavior. The potential callback-ownership issue applies only to an extension-loader pattern not used by the repository’s uncached loaders, so no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Caller
participant ResourceManager
participant Loader
Caller->>ResourceManager: load same uncached URL
ResourceManager->>Loader: start independent load
Caller->>ResourceManager: load same uncached URL
ResourceManager->>Loader: start second independent load
ResourceManager->>ResourceManager: track both promises
Caller->>ResourceManager: cancelNotLoaded URL
ResourceManager->>ResourceManager: cancel every pending promise
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue ✨ 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 #3100 +/- ##
===========================================
- Coverage 85.88% 85.74% -0.14%
===========================================
Files 811 811
Lines 94808 94746 -62
Branches 11592 11607 +15
===========================================
- Hits 81425 81243 -182
- Misses 13293 13411 +118
- Partials 90 92 +2
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:
|
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
本轮按首次完整增量审查了 dev/2.0@490d2bc8ca1f1c187a995da3c913e59c56959a30...8cc72e47c4eea04e52fed990f3293d9de98fd6f2,覆盖 3 个改动文件,并沿 SceneManager → ResourceManager/SceneLoader → Scene.destroy()/SceneManager 向上下游各追一层。顺序同 URL 重载和销毁时遍历跳项的修复方向正确,CI 的 lint、三平台 build、e2e 与 patch coverage 均已通过;但仍有 1 个与本 PR 根因相同的 P1 阻塞问题。实际 review 动作为 REQUEST_CHANGES,目标 HEAD 为 8cc72e47c4eea04e52fed990f3293d9de98fd6f2。自动 CR 不替代人工 Reviewer 的合入门禁,修复后仍需人工审核确认。
问题
- [P1] 非缓存 Scene 仍被在途 Promise 复用,并发同 URL 重载会再次挂回已销毁 Scene —
packages/loader/src/SceneLoader.ts:170只关闭了完成后的_assetUrlPool缓存,但直接上游packages/core/src/asset/ResourceManager.ts:361-375仍会让第二个同 URL 请求复用第一个_loadingPromises结果。若第一次loadScene("main.scene", true)尚未完成时再调用一次,两次回调会在packages/core/src/SceneManager.ts:95-102收到同一个运行时 Scene:第一个回调销毁旧场景并挂载该 Scene,第二个回调随即把它销毁,再把这个destroyed === true、实体树已清空且原生 PhysicsScene 已释放的对象挂回,确定性重现本 PR 要修的 zombie 状态。PR 正文虽把重叠调用排除在 scope 外,但公开 API 没有禁止或拒绝这种调用,而且这是本次“资源身份复用与终止型运行时生命周期冲突”的同一根因,不能留作另一条并行状态机。应保留SceneManager._scenes作为运行时 Scene 身份与生命周期的权威 owner,删除运行时 Scene 在_assetUrlPool以及_loadingPromises中的身份复用;资源层如需合并下载,只共享不可变的 SceneFile/SceneAsset/字节,再为每次loadScene机械构造独立 Scene。不要增加 identity/zombie guard、generation 镜像状态或 compatibility fallback。补一条通过公开loadScene发起两个延迟、重叠同 URL 调用的链路测试,断言两个结果身份独立、旧实例终止且最终活动实例可用,并反向移除修复确认测试失败。[Cocos Creator/Unity] 都只缓存 SceneAsset/场景数据、每次加载物化独立运行时 Scene,这正好避免共享终止型对象。
架构、熵增与测试治理
改动前,运行时 Scene 同时受 URL 完成态缓存、在途加载 Promise 和 SceneManager._scenes 三处身份复用/生命周期影响;本 PR 删除完成态 URL 缓存,并复用既有 SafeLoopArray 快照修正销毁期间的遍历,没有新增持久字段、wrapper、兼容分支或重复校验,顺序路径的熵确实下降。问题在于在途 Promise 仍保存第二份运行时身份真相,所以重叠窗口内 owner 数量没有收敛,复杂度只是从稳定态缩到了并发态。应让 ResourceManager 只拥有不可变加载数据与请求进度,让 SceneManager 独占运行时 Scene 的挂载/销毁;删除共享 Promise 对运行时 Scene 的复用后,数据流应为“共享不可变数据 → 每次调用构造新 Scene → SceneManager 原子替换旧 Scene”。
新增两条测试都从公开 loadScene 入口验证公开生命周期结果,且覆盖了顺序同 URL 新身份和 live-array 删除跳项;没有发现为旧测试保留的生产兼容逻辑,也没有需要继续维护的旧 fixture/mock/snapshot。当前缺口正是上述在途复用分支,patch coverage 100% 不能替代该负向并发契约测试。
luzhuang
left a comment
There was a problem hiding this comment.
当前 HEAD 7c344b173 已完成独立复核,旧 HEAD 8cc72e47c 的并发同 URL P1 已收口:ResourceManager 现在只对 useCache=true loader 复用在途 Promise,uncached Scene 每次创建独立请求;数组化 pending owner 按 Promise 身份清理,cancelNotLoaded 覆盖 URL/列表/全部请求。公开 SceneManager.loadScene 测试覆盖两种重叠完成顺序并验证最终 active Scene 未销毁,三平台 build、lint、4 个 E2E shard 与 patch coverage 通过。当前未发现新的 P0/P1/P2。仍需原 reviewer 对新 HEAD 复审;codecov/project 当前为项目覆盖率 -0.14% 的外部门禁,不能等同于源码 finding。未合并,也未启用 auto-merge。
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
本轮完整复核了 dev/2.0@490d2bc8ca1f1c187a995da3c913e59c56959a30...7c344b1735ed48223266db64d7cf8b22ede6774f 的 5 个改动文件,并重点审查上一轮 8cc72e47c4eea04e52fed990f3293d9de98fd6f2...7c344b1735ed48223266db64d7cf8b22ede6774f 增量,沿 SceneManager.loadScene → ResourceManager → SceneLoader → Scene.destroy()/SceneManager 向上下游各追一层。上一轮并发同 URL 的 P1 已修复,本轮未发现新的 P0/P1/P2,源码阻塞级别为无;lint、三平台 build、4 个 e2e shard、codecov 与 codecov/patch 均通过,codecov/project 因项目覆盖率下降 0.14% 仍是待处理的外部门禁。实际 review 动作为 COMMENTED,目标 HEAD 为 7c344b1735ed48223266db64d7cf8b22ede6774f。自动 CR 不替代人工 Reviewer 的合入门禁,APPROVE 仍需人工审核确认。
已关闭问题清单
- [已修复] 非缓存 Scene 的在途 Promise 复用会让并发同 URL 重载挂回已销毁 Scene — commit
7c344b1735ed48223266db64d7cf8b22ede6774f将ResourceManager._loadingPromises从单 Promise 改为按 URL 保存请求集合,并在packages/core/src/asset/ResourceManager.ts:358-390以既有Loader.useCache同时控制完成态缓存读取和在途复用;useCache=false的SceneLoader因而每次都创建独立 Scene。清理按 Promise 身份删除,cancelNotLoaded也覆盖集合中的全部请求。对应tests/src/core/SceneManagerLoadScene.test.ts:30-84从公开loadScene入口覆盖两种相反完成顺序,断言结果身份独立、被替换实例终止且最终活动实例可用;tests/src/core/resource/ResourceManager.test.ts:121-183进一步守住完成态 cache bypass、在途独立与批量取消。该测试在旧实现上会因只有一个请求 resolver / 被覆盖的在途 Promise 而失败,闭环成立。对应上一轮 review:pullrequestreview-5061036857。
架构、熵增与测试治理
改动前,运行时 Scene 身份同时受完成态 URL cache、单值在途 Promise 与 SceneManager._scenes 三处影响;改动后,SceneManager 保留运行时 Scene 挂载与终止生命周期的唯一权威 ownership,SceneLoader 每次机械物化新实例,ResourceManager 只拥有加载请求集合、进度与取消,完成态 cache / 在途共享均由已有的 Loader.useCache 契约统一决定。数组化和 _removeLoadingPromise 是对原单槽请求台账的基数扩展,没有新增 generation、identity guard、wrapper、镜像状态、compatibility branch 或第二条转换/校验路径;缓存型 loader 仍只共享首个请求,三个内建非缓存 loader 的行为也统一收口。
测试按新的公开契约重写了原顺序场景,并补齐并发链路和取消链路;没有为旧断言保留生产 fallback,也没有残留需要删除的 legacy fixture、mock 或 snapshot。沿子资产下游核对后,现有 eager sub-asset 通知仍由缓存型 loader 使用,当前三个非缓存 loader 不引入第二套回调 ownership。覆盖率项目门禁仍需按仓库策略处理,但不构成为旧测试保留兼容生产路径的理由。
Fixes #2979
Root cause
SceneLoaderwas changed to disable completed-result URL caching, butResourceManagerstill reused one in-flight_loadingPromisesentry for every loader. Two overlapping same-URLSceneManager.loadScenecalls therefore received the same runtimeScene; the later lifecycle callback could destroy it and reattach the destroyed object.Solution
useCache=falseloaders neither read completed URL cache nor reuse an in-flight Promise, so each load materializes its own result.cancelNotLoaded(url)andcancelNotLoaded()cancel every request in each collection.ProjectLoader and PrimitiveMeshLoader use the same generic
useCache=falsepath; the ResourceManager test exercises the real PrimitiveMesh loader registration and verifies both completed-cache bypass and independent concurrent results.Validation
pnpm exec vitest run --config vitest.config.ts src/core/SceneManagerLoadScene.test.ts src/core/resource/ResourceManager.test.ts src/loader/SceneFormatV2.test.ts src/core/mesh/PrimitiveMesh.test.tsfromtests: 95/95 passed in Chromium.pnpm b:module: passed; existing circular-dependency and sourcemap warnings only.pnpm b:types: passed, including Engine knowledge validation.pnpm lint: passed with 0 errors and existing warnings only.git diff --check: passed.Summary by CodeRabbit
Bug Fixes
Tests