Add scene disposal checks to prevent errors during cleanup#648
Merged
Conversation
Loading characters in a tight loop and re-running could leave in-flight LoadAssetContainerAsync calls outliving disposeOldScene(). This caused an unhandled "aborted" rejection, "No scene available" warnings, and a null scene crash in setupMesh. - Attach a benign catch to the readiness promise so aborted loads don't surface as unhandled rejections when no whenModelReady consumer exists. - Bail out of createCharacter/createObject early when no live scene. - Skip the load .then handlers (dispose the container instead) when the scene was disposed or the load aborted mid-flight. https://claude.ai/code/session_01ENptDkxsw6UEFt5BDdydPt
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdded disposal-safe checks throughout the model loading pipeline. ChangesScene Disposal Resilience in Model Loading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Deploying flockxr with
|
| Latest commit: |
a008936
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3d6566fb.flockxr.pages.dev |
| Branch Preview URL: | https://claude-vibrant-bohr-xtadk.flockxr.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/models.js (1)
367-377:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese disposal exits leak
modelReadyPromisesand reserved names.Both new
returnpaths leave the per-call readiness promise pending forever and never releasemeshName. That can strandwhenModelReadycallers and block future reuse after scene teardown.createObjectneeds the same failure cleanup contract ascreateCharacterhere.Proposed fix
- let resolveReady; - const readyPromise = new Promise((res) => { - resolveReady = res; - }); + let resolveReady, rejectReady; + const readyPromise = new Promise((res, rej) => { + resolveReady = res; + rejectReady = rej; + }); flock.modelReadyPromises.set(meshName, readyPromise); + + const failReady = (error) => { + rejectReady?.(error); + flock.modelReadyPromises.delete(meshName); + flock._releaseName?.(meshName); + }; ... if (flock.modelsBeingLoaded[modelName]) { flock.modelsBeingLoaded[modelName].then(() => { - if (!flock.scene || flock.scene.isDisposed) return; + if (!flock.scene || flock.scene.isDisposed) { + failReady(new Error("scene disposed")); + return; + } flock._recycleOldestByKey(modelName); const mesh = flock.modelCache[modelName].clone( flock.modelCache[modelName].name, ); ... loadPromise.then((container) => { // Scene disposed while the container was loading: drop it. if (!flock.scene || flock.scene.isDisposed) { try { container?.dispose?.(); } catch (_) { console.warn("Suppressed non-critical error:", _); } + failReady(new Error("scene disposed")); delete flock.modelsBeingLoaded[modelName]; return; }Also applies to: 390-400
🤖 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 `@api/models.js` around lines 367 - 377, The then() callbacks inside flock.modelsBeingLoaded[modelName] currently bail early on scene/disposed checks and never call the per-call readiness resolver or release the reserved meshName, leaking modelReadyPromises and names; update those early-return paths (both the block around lines shown and the similar block at 390-400) to perform the same cleanup as createCharacter: call the per-invocation resolve/reject helper (resolveReady/rejectReady or the local promise resolver used to fulfill modelReadyPromises) so the waiting promise completes, remove the meshName reservation (the data structure that records reserved names), and ensure any modelReadyPromises entry for that call is cleared before returning; mirror the cleanup sequence used in createCharacter (finalizing the promise and releasing the name) so no promises or name reservations are left stranded when flock.scene is missing or disposed.
🤖 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 `@api/models.js`:
- Around line 145-157: The disposed-scene branch currently just disposes the
container and doesn't perform the same abort cleanup, leaving readyPromise
unresolved and meshName reserved; modify the branch handling signal?.aborted ||
!flock.scene || flock.scene.isDisposed to invoke the same abort cleanup used for
an explicit abort (i.e., call the routine that rejects readyPromise and releases
meshName — the same behavior as the abort path), ensuring you use the existing
cleanupAbort/onAbort helper (or explicitly reject readyPromise and free
meshName) so any whenModelReady waiters are settled and the reserved name is
released.
---
Outside diff comments:
In `@api/models.js`:
- Around line 367-377: The then() callbacks inside
flock.modelsBeingLoaded[modelName] currently bail early on scene/disposed checks
and never call the per-call readiness resolver or release the reserved meshName,
leaking modelReadyPromises and names; update those early-return paths (both the
block around lines shown and the similar block at 390-400) to perform the same
cleanup as createCharacter: call the per-invocation resolve/reject helper
(resolveReady/rejectReady or the local promise resolver used to fulfill
modelReadyPromises) so the waiting promise completes, remove the meshName
reservation (the data structure that records reserved names), and ensure any
modelReadyPromises entry for that call is cleared before returning; mirror the
cleanup sequence used in createCharacter (finalizing the promise and releasing
the name) so no promises or name reservations are left stranded when flock.scene
is missing or disposed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
CodeRabbit flagged that createCharacter's disposed-scene branch only disposed the container. When the scene is disposed without an abort, readyPromise never settles and the reserved name leaks, hanging any whenModelReady waiter. Mirror the abort cleanup in that branch. https://claude.ai/code/session_01ENptDkxsw6UEFt5BDdydPt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add defensive checks throughout the model loading and object creation pipeline to gracefully handle cases where the scene is disposed or becomes unavailable during asynchronous operations. This prevents errors and unhandled promise rejections when operations complete after scene cleanup.
Key Changes
createCharacterandcreateObjectto bail out if no active scene existsreadyPromiseto prevent unhandled rejection warnings when loads are aborted without consumers.then()blocks after container loading completes, ensuring we don't attempt to add meshes to a disposed scenemodelsBeingLoaded) is cleaned upImplementation Details
The changes follow a consistent pattern: check if
flock.sceneis null orflock.scene.isDisposedat critical points where the scene is accessed. This is particularly important in promise chains where the scene state can change between when the promise was created and when it resolves. The implementation ensures that even if a scene is disposed mid-operation, no errors propagate and resources are properly cleaned up.https://claude.ai/code/session_01ENptDkxsw6UEFt5BDdydPt
Summary by CodeRabbit