fix: js thread lock on scene lifecycle - #8702
Conversation
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
|
Claude finished @lorux0's task in 6m 1s —— View job Claude PR Review
SummaryProblem: Root-cause check: PR explicitly admits the underlying JS deadlock cause is unknown. The diff is a layered defensive fix:
So #3 is a true root-cause fix; #1 and #2 are defense-in-depth. Acceptable framing. Blocking
Non-blocking concerns
TestsTest updates correctly track the API rename and split. No coverage added for the watchdog itself (interrupt path, threshold trip, state transition guards) — would be nice but not blocking. REVIEW_RESULT: FAIL ❌ |
|
Claude finished @lorux0's task in 5m 28s —— View job Claude PR Review
SummaryProblem this PR solves: scenes (notably Root cause vs symptom: the PR is explicit that the underlying V8/JS hang has an obscure cause. The three changes are layered defenses:
Net assessment: not a "swallow the exception" fix. The lifecycle split addresses ordering directly; the watchdog is a defensive layer with a clear failure path; the dt cap is honestly labeled as mitigation. Blocking issues
Non-blocking observations
REVIEW_RESULT: FAIL ❌ |
|
🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
|
Claude finished @lorux0's task in 3m 24s —— View job Claude PR Review
SummaryProblem: scenes (notably Root cause vs symptom: the PR is honest that the underlying V8/JS hang has an obscure cause. The diff is layered:
Net assessment: not a swallow-the-exception fix. Lifecycle split is a true root-cause fix; watchdog and dt cap are defensive layers honestly labeled as such. Since the previous review the only commit ( Blocking issues (all carried over from prior review)
Non-blocking observations
TestsTest API was updated to track the rename and split. No coverage added for the watchdog itself (interrupt path, threshold trip, state-transition guard) — would be useful but not blocking. REVIEW_RESULT: FAIL ❌ |
|
Claude finished @lorux0's task in 3m 41s —— View job Claude PR Review
SummaryProblem: scenes (notably Root cause vs symptom: the PR is honest that the underlying V8/JS hang has an obscure cause. The diff is a layered defense:
Net assessment: not a swallow-the-exception fix. Lifecycle split is a true root-cause fix; watchdog and dt cap are defensive layers honestly labeled as such. Since the previous review (
Blocking issues (still unaddressed)
Non-blocking observations
TestsTest API was updated to track the rename and split. No coverage added for the watchdog itself (interrupt path, threshold trip, state-transition guard) — would be useful but not blocking. REVIEW_RESULT: FAIL ❌ |
# Conflicts: # Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs
|
Claude finished @lorux0's task in 4m 22s —— View job Claude PR Review
SummaryProblem: scenes (notably Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:
Since the previous review (last failing on the
Blocking issues1. If Since Fix: guard before the call: if (SceneStateProvider.State != SceneState.Starting)
return;
await UpdateLoopAsync(ct);(See inline comment on line 183.) Non-blocking observations
REVIEW_RESULT: FAIL ❌ • Branch: |
|
Claude finished @lorux0's task in 4m 57s —— View job Claude finished @lorux0's task — Claude PR Review
SummaryProblem: scenes (notably Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:
Since the previous review (commits
Blocking issuesNone. Non-blocking observations
REVIEW_RESULT: PASS ✅ • Branch: |
|
🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
|
Claude finished @lorux0's task in 5m 42s —— View job Claude PR Review
SummaryProblem: scenes (notably Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:
Since the previous PASS review (commits
All previously-resolved blocking issues remain fixed:
Blocking issuesNone. Non-blocking observations
REVIEW_RESULT: PASS ✅ • Branch: |
mikhail-dcl
left a comment
There was a problem hiding this comment.
Looks good to me, just fix that second disconnected task
|
🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
|
Claude finished @lorux0's task in 2m 39s —— View job Claude PR Review
SummaryProblem: scenes (notably Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:
Since the previous PASS review (commit
All previously-resolved blocking issues remain fixed:
Blocking issuesNone. Non-blocking observations
REVIEW_RESULT: PASS ✅ • Branch: |
|
🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [✔️ ] Backpack and wearables in world
- [✔️ ] Emotes in world and in backpack
- [ ✔️] Teleport with map/coordinates/Jump In
- [ ✔️] Chat and multiplayer
- [✔️ ] Profile card
- [ ✔️] Settings
Teleported from one world to bloomgarden and to genesis plaza several times without any problems and could not repro the issue , also reloaded the platforms several times (+20) without any success on reproing the issue per-se 🚀
Played bloomgarden and attempted both repro states with no success , thanks team!
Link to the video since it takes more than 10 minutes of video
|
Claude finished @lorux0's task in 1m 36s —— View job Claude finished @lorux0's task — Claude PR Review
SummaryProblem: scenes (notably Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:
Latest commit ( All blocking issues resolved in prior commits remain fixed:
Blocking issuesNone. Non-blocking observations
REVIEW_RESULT: PASS ✅ • Branch: |
|
🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
Fixes #8654 and #8493
Problem
The world
bloomgarden.dcl.ethsometimes gets stuck atawait runtimeInstance.UpdateScene(deltaTime);. The JS thread is locked. The explorer did not recover from that, keeping the scene state as "running code", being unable to dispose it on teleport.The REAL reason is obscure, but i manage to conclude that it breaks 100% when the delta time passed to the scene is large, ie:
3.9, when it normally is0.025. This happens especially after the loading screen hides and the scene renders, which sometimes provokes a hiccup.Most likely there is an infinite loop in the scene code. I've tried making a simple scene with an endless loop and it has the same symptom.
Solution
Test Steps
The reproduction rate of these issues is very low. Only achieved breaking it by forcing the code.
Try starting the explorer at
bloomgarden.dcl.eth. Play a bit.Perform teleport operations. Perform /reloads. Move around the world.
Check that scenes runs normally.
Check portable experiences too.
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.