fix: delay on exit and exit stopwatch

[NickKhalow]

What does this PR change?

Replaces the simple ExitUtils.BeforeApplicationQuitting event with a structured cleanup pipeline that provides per-callback timing diagnostics, thread-safe registration, and reflection-based instrumentation of remaining Application.quitting subscribers. Also updates the LiveKit SDK to a branch that avoids explicit dispose issues, and adds UTC timestamps to debug log output for better exit-time correlation.

Related PR: decentraland/client-sdk-unity#68

Problem

The previous exit flow used a plain Action event, making it difficult to diagnose which cleanup callbacks contribute to exit delays. There was no protection against re-entrant Exit() calls or late registrations during shutdown, and no visibility into third-party or framework-level Application.quitting subscribers.

Solution

Structured cleanup pipeline:

• Introduces OnQuittingCleanUpCandidate — a named cleanup callback that wraps execution with a Stopwatch and logs elapsed time via ReportHub.
• Refactors ExitUtils to maintain a thread-safe (Mutex<List<...>>) registry of cleanup candidates instead of a raw event.
• Adds an Atomic<bool> isExiting guard to prevent re-entrant exits and to reject registrations/unregistrations once shutdown has started.
• Hooks into Application.quitting automatically via [RuntimeInitializeOnLoadMethod], so callers no longer need to wire up the quit event themselves.
• Clears the candidate list and resets isExiting in the Editor to handle play-mode re-entry correctly.

Reflection-based Application.quitting instrumentation (Patch inner class):

• Uses reflection to read the backing field of Unity's Application.quitting event.
• Wraps each existing subscriber with a timer delegate that logs `[ExitUtils.Patch] '{Type.Method}' Application.quitting subscriber took Xms`.
• Inserts itself as the first subscriber to re-patch on each invocation (idempotent — tracks already-wrapped delegates via a HashSet).
• Captures timing for subscribers that cannot use RegisterCleanUpCandidate (e.g., third-party code, DCLPlayerPrefs due to assembly cycle).
• Re-invoked before Quit() in Exit() to ensure any late-registered subscribers are also instrumented.

Migrated subscribers

• UIAudioPlaybackController — switched from BeforeApplicationQuitting += to RegisterCleanUpCandidate/UnregisterCleanUpCandidate.
• WalkedDistanceAnalytics — switched from Application.quitting += to RegisterCleanUpCandidate.
• TimeSpentInWorldAnalyticsSystem — switched from Application.quitting += to RegisterCleanUpCandidate.
• UnityObjectUtils — switched from Application.quitting += to RegisterCleanUpCandidate.
• SentryTransactionManager — consolidated duplicate subscriptions (BeforeApplicationQuitting + Application.quitting) into a single RegisterCleanUpCandidate call.
• DCLPlayerPrefs — cannot use the new API due to an assembly definition cycle (Utility → DCL.Prefs via PersistentSetting), so it remains on direct Application.quitting with its own stopwatch logging. The reflection patch also instruments it automatically.

Additional changes

• DebugLogReportHandler — prepends a UTC timestamp (HH:mm:ss.fff) to every debug log line for better exit-time correlation.
• com.decentraland.livekit-sdk — updated to the fix/no_explicit_dispose branch of decentraland/client-sdk-unity to avoid explicit dispose issues during shutdown.

Safety net

• Adds a VerifyShouldNotUseApplicationQuitting code convention test that forbids direct Application.quitting usage, with explicit exceptions for ExitUtils.cs and DCLPlayerPrefs.cs.

Test Instructions

Steps (standard run):

metaforge explorer run 8770

Expected result:

• Application exits without additional delay.
• Production logs show per-candidate cleanup timing from RegisterCleanUpCandidate, e.g. `[ExitUtils] 'UIAudioPlaybackController' cleanup took Xms (total Yms)`.
• Production logs show per-subscriber timing from the reflection patch, e.g. `[ExitUtils.Patch] 'DCLPlayerPrefs.OnQuitting' Application.quitting subscriber took Xms`.
• Debug log lines include UTC timestamps.

Steps (fresh account):

metaforge account create --clear
metaforge explorer run 8770

Expected result:

• Same as above — clean exit with timing diagnostics in logs.

Automation (if applicable):

metaforge explorer test 8770

Test Steps

1. Launch the Explorer and enter a world.
2. Quit the application.
3. Verify in logs that each cleanup candidate is logged with its elapsed time.
4. Verify the reflection patch logs timing for any remaining Application.quitting subscribers.
5. Verify the total cleanup time is logged.
6. Re-enter play mode in the Editor and confirm exit works correctly on subsequent runs (validates isExiting reset and candidate list clearing).

Additional Testing Notes

• Verify that the new code convention test (VerifyShouldNotUseApplicationQuitting) passes and catches any new direct Application.quitting usage.
• Verify thread safety: no race conditions when registering/unregistering candidates from different threads during exit.
• Verify the reflection patch handles the case where Application.quitting backing field is inaccessible (graceful fallback with log message).

Quality Checklist

• Changes have been tested locally
• Documentation has been updated (if required)
• Performance impact has been considered
• For SDK features: Test scene is included

[github-actions]

Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below.

Name	Link
Commit	937e12e
Logs	https://github.com/decentraland/unity-explorer/actions/runs/25918668587
Download Windows	https://github.com/decentraland/unity-explorer/suites/69168411638/artifacts/7018168512
Download Windows S3	https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/fix/delay-on-exit/pr-22309-937e12e/Decentraland_windows64.zip
Download Mac	https://github.com/decentraland/unity-explorer/suites/69168411638/artifacts/7018025166
Download Mac S3	https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/fix/delay-on-exit/pr-22309-937e12e/Decentraland_macos.zip
Built on	2026-05-15T13:32:50Z

[claude]

Claude finished @NickKhalow's task in 1m 24s —— View job

---

Code Review

• Gather context
• Read changed file
• Analyze diff
• Post review

---

Root-cause Check

Problem: Delay on exit — the PR adds stopwatch instrumentation to ExitUtils.Exit() to measure how long BeforeApplicationQuitting handlers take before the actual quit call is dispatched.

Assessment: This PR adds diagnostic logging to identify where delay occurs, not to fix the delay itself. That is a valid approach for an investigation step, but the PR title ("fix: delay on exit") is misleading — there is no delay fix here, only observability tooling. This is acceptable as long as a follow-up PR addresses the actual slow handler once identified.

---

Blocking Issues

Line 17 — First log always prints 0ms

Stopwatch.StartNew() is called on line 16; by the time line 17 executes, elapsed is 0ms (or a few µs). The message format ("at {stopwatch.ElapsedMilliseconds}ms") implies a meaningful timestamp but will always read 0. See inline comment with suggested fix.

---

Non-blocking Notes

• ReportHub.LogProductionInfo API usage is correct (matches the (string message, ReportHandler reportToHandlers = ReportHandler.All) signature).
• The indentation change (tabs → spaces on BeforeApplicationQuitting and its invocation) is a valid formatting fix per .editorconfig.
• PR description is still "TODO" — please fill it in before merge.

---

REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Touches a single utility class with only logging/instrumentation additions; no ECS, async, plugin, or asset pipeline involvement.
QA_REQUIRED: YES

[github-actions]

Auto-approved by Claude — simple fix/chore with no blocking issues. QA approval is still required.

[claude]

Claude finished @NickKhalow's task in 2m 44s —— View job

---

Claude finished @NickKhalow's task —— View job

---

Code Review

• Gather context
• Read changed files and diff
• Analyze changes
• Post review

---

Root-cause Check

Problem: The exit path has an unknown delay caused by one or more Application.quitting handlers. The old code used a bare static event BeforeApplicationQuitting with no instrumentation or attribution.

Assessment: This PR replaces the static event with a named, timed cleanup pipeline (OnQuittingCleanUpCandidate + ExitUtils.RegisterCleanUpCandidate). It does not yet fix the slow handler — it instruments the exit path so the slow handler can be identified. That is the correct first step. The PR title ("fix: delay on exit") is still slightly misleading (this is observability, not a fix), but the approach is sound.

---

Blocking Issues

1. Execute method visibility — ExitUtils.cs line 26

Execute(Stopwatch stopwatch) is public on a public class, but it is only ever called from ExitUtils.Exit(). This leaks the internal Stopwatch-passing contract and allows callers to invoke it with arbitrary timing context. It should be internal. See inline suggestion.

2. First log always prints 0ms — ExitUtils.cs line 104 (previously flagged, not addressed)

Stopwatch.StartNew() is called on line 103. By line 104, elapsed is always 0ms (or a few µs). The "at {X}ms" phrasing implies a meaningful offset into the exit sequence, but it carries no information. The inline suggestion from the previous review was not applied — please address it.

---

Non-blocking Notes

• TOCTOU between isExiting check and candidates.Lock() (lines 64, 83): isExiting is read before acquiring the list lock. A concurrent Exit() call setting isExiting = true and then acquiring the list lock could race with a RegisterCleanUpCandidate that read isExiting = false but hasn't yet locked the list. In practice this is extremely unlikely (main-thread init vs. main-thread quit), but strictly speaking the check should be inside the lock. Low priority.

• Static state after domain reload is disabled: If "Domain Reload" is turned off in Project Settings, isExiting remains true across editor play sessions. All RegisterCleanUpCandidate calls in the second play session would be silently dropped. This is not a production concern but can be a friction point in development. Adding [RuntimeInitializeOnLoadMethod] to reset isExiting = false alongside the Application.quitting subscription would fix it.

• SentryTransactionManager previously double-subscribed (both ExitUtils.BeforeApplicationQuitting and Application.quitting) — this is now correctly collapsed into a single RegisterCleanUpCandidate. Good fix.

• Convention test is well-scoped: the VerifyShouldNotUseApplicationQuitting test correctly allows ExitUtils.cs itself and will catch future regressions.

• PR description is still "TODO" — please fill it in before merge.

---

REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Touches a single utility class with logging/instrumentation additions and migrates existing subscribers; no ECS, async, plugin, or asset pipeline involvement.
QA_REQUIRED: YES

[claude]

Claude finished @NickKhalow's task in 1m 46s —— View job

---

Code Review

• Gather context
• Read changed files and diff
• Analyze changes
• Post review

---

Root-cause Check

Problem: Exit has an unmeasured delay caused by unknown Application.quitting handlers. This PR replaces the old static BeforeApplicationQuitting event with a named, timed cleanup pipeline (OnQuittingCleanUpCandidate + ExitUtils.RegisterCleanUpCandidate), migrates all existing subscribers, and adds a convention test to prevent direct Application.quitting subscriptions going forward.

Assessment: The approach is sound — attributing each handler by name and measuring its duration is the correct first step to identify the slow handler. The architecture (named candidates, timed execution, convention enforcement) is well-designed.

---

Blocking Issues

1. Execute still public — ExitUtils.cs line 26 (carried over, not addressed)

Only ExitUtils.Exit() calls this. Making it public leaks the Stopwatch-passing contract to all callers. Should be internal. Fix this →

2. First log always prints 0ms — ExitUtils.cs line 104 (carried over, not addressed)

Stopwatch.StartNew() is called on line 103; by line 104 elapsed is always 0ms. Drop the timestamp. Fix this →

3. isExiting not reset in [RuntimeInitializeOnLoadMethod] — ExitUtils.cs line 49 (new)

The [RuntimeInitializeOnLoadMethod] re-subscribes to Application.quitting, which correctly handles no-domain-reload editor sessions. But isExiting is never reset to false here. In a second play session with Domain Reload disabled, isExiting is still true from the previous session, silently dropping every RegisterCleanUpCandidate call. Add isExiting.Set(false) before re-subscribing. Fix this →

4. Debug.Log in DCLPlayerPrefs.cs line 140 (new — CLAUDE.md violation)

CLAUDE.md: "Use ReportHub instead of Debug.Log for all logging." The timing log in OnQuitting uses UnityEngine.Debug.Log directly. Fix this →

---

Non-blocking Notes

• SentryTransactionManager previously double-subscribed (both ExitUtils.BeforeApplicationQuitting and Application.quitting) — now correctly a single RegisterCleanUpCandidate. Good fix.
• Convention test is well-scoped and will catch future regressions.
• PR description is still "TODO" — please fill it in before merge.

---

REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches 8 files including audio, analytics systems, preferences, and a shared infrastructure utility used across the entire application.
QA_REQUIRED: YES

[claude]

Claude finished @NickKhalow's task in 2m 6s —— View job

---

Code Review

• Gather context
• Read changed files and diff
• Analyze changes
• Post review

---

Root-cause Check

Problem: The exit path had an unknown delay caused by one or more Application.quitting / ExitUtils.BeforeApplicationQuitting subscribers. The old code had no attribution or timing — it was impossible to tell which handler was slow.

Assessment: This PR replaces the static event with a named, timed candidate pipeline (OnQuittingCleanUpCandidate + ExitUtils.RegisterCleanUpCandidate), migrates all existing subscribers, and adds a convention test to prevent future direct Application.quitting subscriptions. This is the correct instrumentation-first approach. The Execute method that was public in the previous revision is now correctly internal ✅.

---

Blocking Issues

1. ExitUtils.cs line 104 — first log always prints 0ms (flagged in rounds 1 and 2, still unaddressed)

Stopwatch.StartNew() is called on line 103. By the time line 104 runs, ElapsedMilliseconds is 0 (or a few µs). The "at {X}ms" phrasing implies a meaningful timeline position, but it is always zero and conveys nothing. See inline suggestion to drop the timestamp. Fix this →

2. DCLPlayerPrefs.cs line 134 — Debug.Log instead of ReportHub.LogProductionInfo

CLAUDE.md: "Use ReportHub instead of Debug.Log for all logging." The new timing log uses UnityEngine.Debug.Log, which bypasses the severity/category system and won't surface in production telemetry. ReportHub lives in DCL.Diagnostics, which does not depend on DCL.Prefs, so there is no cycle preventing this. See inline suggestion. Fix this →

---

Non-blocking Notes

• Static isExiting with Domain Reload disabled (editor only, raised in round 2, acknowledged): [RuntimeInitializeOnLoadMethod] re-subscribes Application.quitting each play session, but isExiting stays true from the previous session, silently dropping all RegisterCleanUpCandidate calls. Adding isExiting.Set(false) inside SubscribeToApplicationQuitting would be a complete fix with no production impact.
• TOCTOU on isExiting vs candidates.Lock() (raised in round 2, acknowledged): extremely unlikely in practice.
• SentryTransactionManager double-subscription was correctly collapsed into a single RegisterCleanUpCandidate ✅
• Convention test is well-scoped; the DCLPlayerPrefs.cs exclusion with its explanatory comment is appropriate ✅
• PR description is still "TODO" — please fill it in before merge.

---

REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Touches a single utility/infrastructure class with logging and instrumentation additions plus migration of existing subscribers; no ECS, async, plugin, or asset pipeline involvement.
QA_REQUIRED: YES

[github-actions]

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

[github-actions]

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

[claude]

Claude finished @NickKhalow's task in 3m 6s —— View job

---

Code Review

• Gather context
• Read changed files and diff
• Analyze changes
• Post review

---

Root-cause Check

Problem: Exit path had no per-handler attribution or timing, making it impossible to identify which cleanup callback caused exit delays.

Assessment: This PR instruments the exit sequence with a named, timed cleanup pipeline (OnQuittingCleanUpCandidate + ExitUtils.RegisterCleanUpCandidate) and a reflection-based Patch class that wraps direct Application.quitting subscribers with stopwatch timers. All previously blocking issues from prior rounds are resolved. A new commit in this sync introduces a blocking package dependency change unrelated to the exit fix.

---

Progress Since Last Review

All prior blocking issues resolved ✅

• Execute is internal ✅
• First log uses DateTime.UtcNow:O instead of always-zero elapsed ✅
• isExiting.Set(false) and candidates.Lock().Clear() both called under #if UNITY_EDITOR ✅
• Patch.ApplicationQuittingFirstSubscriberSelfPatchWithTimers() correctly qualified in Exit() ✅
• DCLPlayerPrefs.cs Debug.Log — assembly cycle to DCL.Utility (which owns ReportHub) verified as justified ✅
• PR description filled in ✅

New in this sync:

• TriggerAreaHandlerSystem.cs: removes ProcessOnStayInTriggerArea (no longer emits TaetStay per frame on wire — SDK runtime synthesizes stay events locally). Test renamed and extended to cover ENTER-only and ENTER+EXIT contracts. This was merged in from PR fix: trigger area onstay CRDT spamming #8733 and has proper test coverage.

---

Blocking Issues

manifest.json line 13 — package pinned to unstable development branch

com.decentraland.livekit-sdk was changed from the default branch reference to https://github.com/decentraland/client-sdk-unity.git#fix/no_explicit_dispose. A development branch is not a stable package reference: it can be force-pushed, rebased, or deleted, and any change silently alters what all contributors resolve on package-install. The commit message ("link the fix branch") confirms this was intentional for local testing, but it must be resolved before merging to dev.

See inline suggestion — revert to the unversioned reference (or pin to a specific commit SHA once fix/no_explicit_dispose is merged upstream and a release is cut).

---

Non-blocking Notes

• Patch.wrapped not cleared in editor reset (carried from rounds 9, 10): isExiting and candidates are reset under #if UNITY_EDITOR, but Patch.wrapped is not. With Domain Reload disabled, stale wrapper closures from session 1 persist in wrapped across editor sessions (minor memory leak and double-wrap risk).

• OnApplicationQuitting self-unregister is a no-op after patching (carried from rounds 9, 10): Patch rebuilds the invocation list with wrapped delegates; Application.quitting -= OnApplicationQuitting removes the original while the rebuilt list holds the wrapper — the unsubscribe is a no-op. Safe (re-entrant Exit() is guarded), but produces extra log entries. A comment would help future readers.

• [ExitUtils] prefix in DCLPlayerPrefs.cs line 139 (carried from multiple rounds): copy-paste artifact — this code runs inside DCLPlayerPrefs.OnQuitting, not ExitUtils. [DCLPlayerPrefs] cleanup took Xms would be clearer.

• Typo at ExitUtils.cs line 150: Idempotentency → Idempotency.

---

REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: The Patch inner class uses reflection to manipulate Application.quitting's private backing field; the diff also includes removal of STAY event emission from TriggerAreaHandlerSystem (an ECS SDK component system), and a package dependency pinned to an unstable branch.
QA_REQUIRED: YES

[claude]

Claude finished @NickKhalow's task in 3m 33s —— View job

---

Dependency Security Review

• Identify dependency changes in manifest.json / packages-lock.json
• Check for binary/plugin additions
• Look up registry metadata for changed packages
• Assess provenance, capability, and shipping impact
• Post review

---

Summary

This PR contains one dependency change and no binary/plugin/asmdef additions.

Dependency	Old	New	Source	Type	Scope	Risk	Confidence
com.decentraland.livekit-sdk	client-sdk-unity.git (lock hash bdb26fb)	client-sdk-unity.git#fix/no_explicit_dispose (lock hash e5b8af1)	Decentraland-owned GitHub fork	Source (managed C#) + bundled native FFI binaries (pre-existing in the package)	Runtime, ships in desktop client	LOW (with one provenance note → see below)	HIGH

No other registry changes. No new .dll / .so / .dylib / .bundle / .aar / .asmdef introduced anywhere in the PR.

---

Step 1 — Dependency changes visible in the PR

Explorer/Packages/manifest.json — single line changed:

- "com.decentraland.livekit-sdk": "https://github.com/decentraland/client-sdk-unity.git",
+ "com.decentraland.livekit-sdk": "https://github.com/decentraland/client-sdk-unity.git#fix/no_explicit_dispose",

Explorer/Packages/packages-lock.json — lockfile updated:

- "version": "https://github.com/decentraland/client-sdk-unity.git",
+ "version": "https://github.com/decentraland/client-sdk-unity.git#fix/no_explicit_dispose",
...
- "hash": "bdb26fbf60fe2ecf0aac83967be8f4c2fa7e7e94"
+ "hash": "e5b8af15a755baf19a2386ae3097bd157743c2f7"

Transitive dependencies declared by the package (com.nickkhalow.renum, com.nickkhalow.richtypes, io.livekit.unity) are unchanged in the lockfile — no new transitive surface.

---

Step 1.5 — Registry / repo metadata

[Registry metadata] decentraland/client-sdk-unity is a fork of the upstream livekit/client-sdk-unity (official LiveKit Unity SDK). Default branch main. Owned by the decentraland GitHub organization — same org that owns this repository.

[Registry metadata] Branch fix/no_explicit_dispose exists (head e5b8af1). Two commits ahead of main:

1. 10c1544 — "silent drop fix" (NickKhalow, 2026-05-14). Modifies one file: Runtime/Scripts/Internal/FFIClient.cs. Removes an explicit DisposeRequest call to the native FFI server during shutdown; replaces it with a debug log + a comment explaining that the upstream Rust FFI documents disposal as optional. This is consistent with the stated "fix delay on exit" goal of this PR.
2. e5b8af1 — merge of origin/main into the branch (NickKhalow, 2026-05-14). Brings in unrelated spatial-audio refactor of LivekitAudioSource.cs (ILD attenuation curves, click-prevention gain ramping). This is upstream-fork content, not a new contribution.

Both commits authored by the PR author (NickKhalow), pushed to the Decentraland-owned fork.

---

Step 2 — Assessment

A. Provenance / source trust

• [PR] Source is the Decentraland-owned fork of the official LiveKit SDK. Trust level is the same as before this PR.
• [PR] manifest.json references a branch (fix/no_explicit_dispose) rather than an immutable commit. The branch is not yet merged into the fork's main.
• [PR] packages-lock.json pins to commit e5b8af15a755baf19a2386ae3097bd157743c2f7. So current resolution is immutable, but a lockfile regeneration without the branch existing or after the branch is force-pushed could resolve to a different commit silently.
• [Inference] Lowest-friction hardening: either (a) merge fix/no_explicit_dispose into main of the fork and revert the manifest line, or (b) pin to the commit hash explicitly: client-sdk-unity.git#e5b8af15a755baf19a2386ae3097bd157743c2f7.

B. Runtime capability / attack surface

• The LiveKit SDK was already shipped in the client before this PR (voice/video chat / room signaling). The diff only reduces native-side activity at shutdown — no new capabilities are introduced.
• [PR] The C# diff in commit 10c1544 is a single deletion of SendRequest(DisposeRequest) from FFIClient.cs. No new network calls, no new reflection, no new dynamic code loading.
• [Inference] Skipping native dispose at exit could in theory leave native resources held briefly until OS reclamation. On a desktop shutdown this is the expected and intended outcome (the OS reaps the process). Not a security concern.

C. Shipping impact

• The LiveKit SDK is a runtime dependency that ships with the desktop client (used by voice/video). All desktop platforms affected.

D. Maintenance / known risk

• [UNKNOWN] No advisories visible from the public repo page; manual CVE review of LiveKit Rust FFI is out of scope here.
• [PR] No license change.

E. Transitive risk

• None — lockfile transitives are unchanged.

---

Notable concerns

1. Manifest pins to a branch, not a commit (LOW–MEDIUM)
The lockfile currently pins to e5b8af1, but manifest.json references the branch name fix/no_explicit_dispose. If anyone regenerates the lockfile, the resolution depends on whatever that branch points to at that moment. The branch is in a Decentraland-controlled repo, so this is a housekeeping concern, not an external supply-chain concern, but it weakens reviewability. Two clean options:

• Land the fix in the fork's main and point manifest.json back at client-sdk-unity.git.
• Pin to the commit hash directly: client-sdk-unity.git#e5b8af15a755baf19a2386ae3097bd157743c2f7.

2. LiveKit SDK change is not described in the PR body (LOW)
The PR body documents the ExitUtils instrumentation work in detail, but does not mention that this PR also bumps the LiveKit SDK to a custom branch that removes an explicit native-FFI disposal call. Reviewers will not see this unless they inspect manifest.json. Worth a sentence in the PR description, especially since the LiveKit change is functionally relevant to the stated "delay on exit" problem (skipping a native round-trip at shutdown is plausibly part of the fix, while the rest of the PR is instrumentation).

---

Step 3 — Risk classification

LOW RISK — official-equivalent (Decentraland-owned fork), minimal scoped change, lockfile pinned, no new binaries or transitive deps, no expanded capability.

---

Step 4 — Recommendation

No blocker. Optional hardening:

• Pin manifest.json to the commit hash, or merge the fix branch into the fork's main before merging this PR.
• Add a one-line note to the PR description acknowledging the LiveKit SDK bump and what it changes (FFIClient no longer sends explicit DisposeRequest at shutdown).

DEPENDENCY_REVIEW: PASS

---

· Branch

[github-actions]

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

[claude]

Claude finished @NickKhalow's task in 2m 57s —— View job

---

Code Review

• Gather context
• Read changed files and diff
• Analyze changes
• Post review

---

Root-cause Check

Problem: Exit path had no per-handler attribution or timing, making it impossible to identify which cleanup callback caused exit delays.

Assessment: This PR instruments the exit sequence with a named, timed cleanup pipeline (OnQuittingCleanUpCandidate + ExitUtils.RegisterCleanUpCandidate) and a reflection-based Patch class that wraps direct Application.quitting subscribers with stopwatch timers. All previously-resolved blocking issues remain fixed. Two new blocking issues were introduced in the latest sync (b7b48bb4 report timestamp and 330169b8 update hash).

---

Progress Since Last Review

All prior blocking issues resolved ✅

• Execute is internal ✅
• First log uses DateTime.UtcNow:O instead of always-zero elapsed ✅
• isExiting.Set(false) and candidates.Lock().Clear() both called under #if UNITY_EDITOR ✅
• Patch.ApplicationQuittingFirstSubscriberSelfPatchWithTimers() correctly qualified in Exit() ✅
• DCLPlayerPrefs.cs Debug.Log — assembly cycle to DCL.Utility confirmed as justified ✅
• PR description filled in ✅

New in this sync:

• DebugLogReportHandler.cs line 121 — adds UTC timestamp prefix to every log message (blocking, see below)
• packages-lock.json hash updated from e5b8af1 to 0254da1 — the branch reference mutated during PR review, proving the reproducibility concern raised in the previous round (blocking, see below)

---

Blocking Issues

1. DebugLogReportHandler.cs line 121 — out-of-scope global log format change

Commit b7b48bb4 prepends HH:mm:ss.fff - to every log message written through DebugLogReportHandler. CLAUDE.md: "Don't add features, refactor, or introduce abstractions beyond what the task requires." The PR already provides per-candidate and per-subscriber timing inside ExitUtils — a global timestamp on all messages is unrelated to the exit fix, changes the log format that tooling and grep patterns may depend on, and should be a separate PR if intentional. See inline suggestion.

2. manifest.json line 13 — branch reference mutated during this PR cycle (carried from previous round, now confirmed)

The lock hash changed from e5b8af1 to 0254da1 mid-review (commit 330169b8 update hash), demonstrating that the fix/no_explicit_dispose branch is a moving target. Any developer who ran package-install between those two states resolved a different version silently. Pin to the current commit hash before merging. See inline suggestion.

---

Non-blocking Notes

• Patch.wrapped not cleared in editor reset (carried from rounds 9–10): candidates is cleared but Patch.wrapped is not. With Domain Reload disabled, stale wrapper closures from session 1 persist in wrapped across editor sessions (minor memory leak).
• DCLPlayerPrefs.cs line 139 — [ExitUtils] prefix (carried from prior rounds): copy-paste artifact; code runs inside DCLPlayerPrefs.OnQuitting. See inline suggestion.
• Typo Idempotentency at ExitUtils.cs line 150 (carried from prior rounds): Idempotency. See inline suggestion.

---

REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: The Patch inner class uses reflection to manipulate Application.quitting's private backing field; this sync also modifies DebugLogReportHandler (a shared logging infrastructure component) and pins a LiveKit SDK dependency to an unstable branch.
QA_REQUIRED: YES

[claude]

Claude finished @NickKhalow's task in 2m 49s —— View job

---

Dependency Security Review

• Identify dependency changes in manifest.json / packages-lock.json
• Check for binary/plugin/asmdef additions
• Look up registry metadata for changed packages
• Assess provenance, capability, and shipping impact
• Post review

---

Summary

One dependency change. No new binaries, native plugins, or .asmdef modifications anywhere in the PR.

Dependency	Old	New	Source	Type	Scope	Risk	Confidence
com.decentraland.livekit-sdk	client-sdk-unity.git (lock bdb26fb)	client-sdk-unity.git#fix/no_explicit_dispose (lock 0254da1)	Decentraland-owned GitHub fork	Source (managed C#) + pre-existing native FFI binaries (unchanged)	Runtime, desktop client	LOW on content, MEDIUM on provenance	HIGH

---

Step 1 — Dependency changes visible in the PR

Explorer/Packages/manifest.json — single line changed:

- "com.decentraland.livekit-sdk": "https://github.com/decentraland/client-sdk-unity.git",
+ "com.decentraland.livekit-sdk": "https://github.com/decentraland/client-sdk-unity.git#fix/no_explicit_dispose",

Explorer/Packages/packages-lock.json — lockfile updated to commit 0254da1fb4491a8deee68e93a5285fca547d9386 (was bdb26fb). Note: this is a different commit than the previous review round (e5b8af1) — the branch was rewritten since round 11.

Transitive deps unchanged: com.nickkhalow.renum, com.nickkhalow.richtypes, io.livekit.unity lockfile entries are byte-identical.

No new binary files: pre-fetched binary list is None, and none of the 11 changed source files is a binary.

---

Step 1.5 — Registry/repo metadata

[Registry metadata] decentraland/client-sdk-unity is a Decentraland-owned fork of the official livekit/client-sdk-unity. Same org as this repo. Trust level unchanged from prior to this PR.

[Registry metadata] Branch fix/no_explicit_dispose (head 0254da1) is now 4 commits ahead of the fork's main:

1. 10c1544 — "silent drop fix" (removed explicit DisposeRequest at FFI shutdown)
2. e5b8af1 — merge of main (brought in unrelated spatial-audio refactor)
3. 06e0a3a — "Revert 'silent drop fix'" (undoes commit 1)
4. 0254da1 — "make callback background threads" (current head)

Key change since round 11: the original "skip explicit FFI dispose" commit that was supposed to be the actual delay fix has been reverted. The current head only contains:

• Setting the FFI callback thread to IsBackground = true so it does not block process shutdown (this IS now the actual exit-delay fix on the LiveKit side)
• Adding millisecond-precision UTC timestamps to LiveKit's internal Debug / Error log helpers
• One extra debug log line reporting the captureLogs flag at init

All commits authored by NickKhalow, who is also the PR author and a Decentraland team member.

---

Step 2 — Assessment

A. Provenance

• [PR] Source is the Decentraland-owned fork — provenance same as before this PR.
• [PR] manifest.json references a branch (fix/no_explicit_dispose) rather than an immutable commit. The branch has already been rewritten once during this PR's lifetime (head moved from e5b8af1 → 0254da1, including a revert). It is not yet merged into the fork's main.
• [PR] packages-lock.json pins the current resolution to 0254da1. A lockfile regeneration would re-resolve to whatever the branch head is at that moment.
• [Inference] The branch name (fix/no_explicit_dispose) no longer describes what is on the branch — that commit was reverted. Mildly confusing for future readers.

B. Runtime capability / attack surface

• [PR] No new network calls, no new reflection, no dynamic code loading, no new native binaries.
• [PR] Thread-flag change (IsBackground = true) is standard .NET — it does not grant new capabilities, only tells the runtime not to wait for the thread at process exit.
• [PR] The added logs route through the package's own Utils.Debug / Utils.Error helpers, which sit on top of UnityEngine.Debug.Log. No telemetry leak surface added.

C. Shipping impact

• Runtime dependency. Ships to all desktop platforms.

D. Maintenance / known risk

• [UNKNOWN] No public advisories on the Decentraland fork; upstream LiveKit FFI CVE review is out of scope.
• [PR] No license change.

E. Transitive risk

• None. Lockfile transitives unchanged.

---

Notable concerns

1. Manifest still pins to a branch, not a commit (LOW–MEDIUM) — carried from round 11, not addressed

manifest.json references #fix/no_explicit_dispose; only the lockfile pins the actual commit. The fact that the branch head has already moved during this PR's review (and a previous commit on it was reverted) demonstrates the issue: anyone who regenerates the lockfile gets whatever is on the branch at that moment, not what was reviewed. Two clean options:

• Pin to the commit: client-sdk-unity.git#0254da1fb4491a8deee68e93a5285fca547d9386
• Merge the change into the fork's main and revert to the unversioned reference

2. Branch name no longer reflects branch content (LOW) — new

The commit that gave the branch its name (silent drop fix removing explicit FFI dispose) was reverted. The branch now only contains a thread-background flag + log timestamps. Either rename the branch or, ideally, land into the fork's main so the manifest can reference an immutable ref. This is housekeeping, not a security concern.

3. PR description still does not mention the LiveKit SDK bump (LOW) — carried from round 11, not addressed

The PR body documents ExitUtils instrumentation in detail but never mentions the LiveKit bump. Worth a sentence so reviewers know to look at manifest.json. The actual change in the LiveKit fork (FFI callback thread → background) is plausibly the most impactful exit-delay fix in this PR — it deserves explicit callout.

---

Step 3 — Risk classification

LOW RISK on content (proportionate change, no new capability, no binaries, no transitive surface) + MEDIUM on provenance/reviewability (mutable branch ref, branch head already shifted mid-review, missing PR description note).

Step 4 — Recommendation

The change itself is fine. The same housekeeping action requested in round 11 remains outstanding and is now reinforced by evidence the branch ref is unstable (its head moved during review):

• Pin manifest.json to commit 0254da1fb4491a8deee68e93a5285fca547d9386, or
• Merge fix/no_explicit_dispose into the fork's main and revert the manifest reference.

Add one sentence to the PR description acknowledging the LiveKit bump and what changed (FFIClient callback thread now IsBackground = true; log timestamps).

DEPENDENCY_REVIEW: NEEDS_ATTENTION

---

· Branch

[github-actions]

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

[DafGreco]

✔️ PR reviewed and approved by QA on Boh 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
• [✔️ ] Settings

Evidence :
Login screen ✅
CTRL+ALT = Close application ✅
Exit modal in game ✅
X button on windowed mode ✅

20260515-0827-57.8075233.mp4

[sandrade-dcl]

Good job mate!

[claude]

Claude finished @NickKhalow's task in 3m 0s —— View job

---

Dependency Security Review

• Identify dependency changes in manifest.json / packages-lock.json
• Check for binary/plugin/asmdef additions
• Look up registry metadata for changed packages
• Assess provenance, capability, and shipping impact
• Post review

---

Summary

This sync (commit f83ede89 "reference updated branch") fully addresses the dependency concerns raised in rounds 11 and 12. One dependency change remains in the PR, but it is no longer pinned to a branch — the upstream fix has been merged and the lockfile now resolves to the default-branch HEAD.

Dependency	Old (round 12)	New (current)	Source	Type	Scope	Risk	Confidence
com.decentraland.livekit-sdk	client-sdk-unity.git#fix/no_explicit_dispose (lock 0254da1)	client-sdk-unity.git (lock dc6e479)	Decentraland-owned GitHub fork	Source (managed C#) + pre-existing native FFI binaries (unchanged)	Runtime, desktop client	LOW	HIGH

No new binary files, no new .asmdef files, no transitive lockfile changes anywhere in the PR.

---

Step 1 — Dependency changes visible in this sync

Explorer/Packages/manifest.json — reverted to the unversioned default-branch reference:

- "com.decentraland.livekit-sdk": "https://github.com/decentraland/client-sdk-unity.git#fix/no_explicit_dispose",
+ "com.decentraland.livekit-sdk": "https://github.com/decentraland/client-sdk-unity.git",

Explorer/Packages/packages-lock.json — hash updated:

- "version": "https://github.com/decentraland/client-sdk-unity.git#fix/no_explicit_dispose",
+ "version": "https://github.com/decentraland/client-sdk-unity.git",
...
- "hash": "0254da1fb4491a8deee68e93a5285fca547d9386"
+ "hash": "dc6e479148106b1f8a68f0fbdba475bf7b96693e"

Transitive deps unchanged (com.cysharp.unitask, com.nickkhalow.richtypes, io.livekit.unity).

---

Step 1.5 — Registry / repo metadata

[Registry metadata] Lock hash dc6e479148106b1f8a68f0fbdba475bf7b96693e is the GitHub-signed merge commit of upstream PR #68 ("Fix/no explicit dispose") on decentraland/client-sdk-unity, which is now the HEAD of the fork's default branch (main). PGP verification status: verified=true. Authored by Nick Khalow (same as PR author).

[Registry metadata] Files changed upstream between the previous lock hash (bdb26fb) and the new one (dc6e479) — confirmed via the GitHub API commit endpoint, only two files:

1. Runtime/Scripts/Internal/FFIClient.cs (+9 / −3): the actual exit-delay fix.
   • FFICallback now sets Thread.CurrentThread.IsBackground = true so the FFI callback thread does not block the process at shutdown. Comment links to a Unity support ticket explaining the IL2CPP-registered-thread freeze pattern.
   • Two Debug.Log → Utils.Debug substitutions (consistent with the package's own LK_DEBUG-gated logging helper).
   • One log message gains a captureLogs flag value.
2. Runtime/Scripts/Internal/Utils.cs (+2 / −2): prepends DateTime.UtcNow:HH:mm:ss.fff to the package's internal Utils.Debug / Utils.Error helpers.

The "skip explicit FFI dispose" approach from earlier commits on the branch was reverted upstream; the merged fix is exclusively the IsBackground = true change.

---

Step 2 — Assessment

A. Provenance ✅

• [PR] Source is the Decentraland-owned fork — same provenance as before this PR.
• [PR] Manifest is back to the unversioned default-branch reference; lockfile pins to dc6e479 which is the default-branch HEAD of decentraland/client-sdk-unity. Re-resolving the lockfile against the current default branch will produce the same hash.
• [Registry metadata] The merge commit is GitHub-signed (verified=true), authored by Nick Khalow, and is the merge product of upstream PR feat: unity transform system #68.

B. Runtime capability / attack surface ✅

• [PR] No new network calls, no new reflection, no dynamic code loading, no new native binaries.
• [PR] The thread-flag change (IsBackground = true) is standard .NET — it does not grant new capabilities; it tells the runtime not to wait for the thread at process exit. Proportionate to the stated purpose ("delay on exit").
• [PR] Added timestamp logging routes through the package's own LK_DEBUG-gated helpers — no telemetry surface added.

C. Shipping impact

• Runtime dependency. Ships to all desktop platforms.

D. Maintenance / known risk

• [UNKNOWN] No public advisories on the Decentraland fork.
• [PR] No license change.

E. Transitive risk

• None. Lockfile transitives unchanged.

---

Changes since round 12

• ✅ Manifest branch ref removed — back to unversioned default-branch reference (previously blocking).
• ✅ Lockfile pinned to default-branch HEAD — dc6e479148106b1f8a68f0fbdba475bf7b96693e is the merge commit of PR feat: unity transform system #68 and currently the HEAD of main.
• ✅ Upstream merge is GitHub-signed — verification confirmed via API.
• ✅ Diff between previous lockfile and new lockfile is exactly the intended fix — IsBackground = true on the FFI callback thread plus an internal timestamp prefix on the package's debug logs.

The round-12 concerns ("branch is a moving target", "branch name no longer describes content") are no longer applicable — the manifest doesn't reference the branch at all anymore. The PR description still accurately describes the bump as "to the fix/no_explicit_dispose branch", which is now slightly stale (the change has landed in main), but this is documentation only and not a security concern.

---

Step 3 — Risk classification

LOW RISK. Proportionate change, no new capability, no binaries, no transitive surface, immutable reference via lockfile, default-branch resolution, signed merge commit.

Step 4 — Recommendation

No action required for dependency review. Optional nit: update the PR description so it reads "merged upstream PR #68 into the default branch" instead of "switched to the fix/no_explicit_dispose branch" — that branch is no longer referenced.

DEPENDENCY_REVIEW: PASS

--- · Branch

[claude]

Claude finished @NickKhalow's task in 3m 13s —— View job

---

Dependency Security Review

• Gather context (manifest.json, lockfile, new binaries)
• Inspect registry metadata for changed packages
• Assess provenance, capability, and shipping impact
• Post review

---

Summary

Net dependency change in this PR vs origin/dev is exactly one line in packages-lock.json: the LiveKit SDK lockfile hash bumps from bdb26fb → dc6e479. manifest.json is identical to dev (the temporary #fix/no_explicit_dispose branch reference was reverted in commit f83ede89). No new binaries, no new .asmdef, no transitive lockfile changes.

Dependency	Old (dev)	New (PR)	Source	Type	Scope	Risk	Confidence
com.decentraland.livekit-sdk	client-sdk-unity.git @ bdb26fb	client-sdk-unity.git @ dc6e479	Decentraland-owned GitHub fork	Source (managed C#) + pre-existing native FFI binaries (unchanged)	Runtime, desktop client	LOW	HIGH

---

Step 1 — Dependency changes visible in the PR (vs origin/dev)

Explorer/Packages/manifest.json — no net change.

Explorer/Packages/packages-lock.json:

-      "hash": "bdb26fbf60fe2ecf0aac83967be8f4c2fa7e7e94"
+      "hash": "dc6e479148106b1f8a68f0fbdba475bf7b96693e"

Transitive deps in the lockfile (com.cysharp.unitask, com.nickkhalow.richtypes, io.livekit.unity) are byte-identical.

No binary / .dll / .so / .dylib / .bundle / .aar files added in any of the PR's own commits (git diff 3ba69be41..HEAD -- '*.dll' '*.so' '*.dylib' '*.bundle' '*.aar' '*.asmdef' '*.meta' returns empty).

---

Step 1.5 — Registry / repo metadata

[Registry metadata] Lock hash dc6e479148106b1f8a68f0fbdba475bf7b96693e is verified via GitHub API as:

• The current HEAD of main on decentraland/client-sdk-unity.
• The GitHub-signed merge commit of upstream PR feat: unity transform system #68 ("Fix/no explicit dispose") — verification verified=true, valid signature.
• Authored by NickKhalow (same as PR author), committed via web-flow (merged in the GitHub UI).
• Parent SHA is bdb26fb (the prior dev lock state) — so the diff between the two resolutions is exactly the merged content of PR feat: unity transform system #68.

[Registry metadata] Files changed upstream between bdb26fb and dc6e479 (per GitHub API):

1. Runtime/Scripts/Internal/FFIClient.cs (+9 / −3): the FFI callback thread now sets Thread.CurrentThread.IsBackground = true so it does not block the process at shutdown. Two Debug.Log calls migrated to the package's own Utils.Debug helper.
2. Runtime/Scripts/Internal/Utils.cs (+2 / −2): prepends DateTime.UtcNow:HH:mm:ss.fff to the package's internal Utils.Debug / Utils.Error helpers.

Total: 16 changes (11 additions, 5 deletions). The earlier "skip explicit DisposeRequest" approach that was on the temporary branch during rounds 11–12 was reverted upstream; the merged fix is exclusively the background-thread flag plus log timestamps.

---

Step 2 — Assessment

A. Provenance ✅

• [PR] Source is the Decentraland-owned fork of the official livekit/client-sdk-unity — same provenance as prior to this PR.
• [PR] Manifest is back to the unversioned default-branch reference; lockfile pins to an immutable commit.
• [Registry metadata] The pinned commit is the default-branch HEAD and is a GitHub-signed merge commit. The round-11/12 concern about a moving branch reference no longer applies.

B. Runtime capability / attack surface ✅

• [PR] No new network calls, no new reflection, no dynamic code loading, no new native binaries.
• [PR] IsBackground = true is standard .NET — it does not grant new capabilities; it tells the runtime not to wait for the thread at process exit. Proportionate to the stated "delay on exit" goal.
• [PR] Added timestamp prefixes flow through the package's existing LK_DEBUG-gated logging helpers — no new telemetry/network surface.

C. Shipping impact — Runtime dependency. Ships to all desktop platforms.

D. Maintenance / known risk

• [UNKNOWN] No public advisories on the Decentraland fork; upstream LiveKit Rust FFI CVE review out of scope here.
• [PR] No license change.

E. Transitive risk — None. Lockfile transitives unchanged.

---

Step 3 — Risk classification

LOW RISK. Proportionate change, no new capability, no new binaries, no transitive surface, immutable lockfile resolution, default-branch reference, signed merge commit. All round-11/12 housekeeping concerns are now resolved.

Step 4 — Recommendation

No action required for dependency review.

Optional nit: the PR description still describes the bump as "to the fix/no_explicit_dispose branch", which is slightly stale — the change has now been merged into the fork's main (manifest is back to the unversioned default-branch reference). Updating that sentence would aid future readers.

DEPENDENCY_REVIEW: PASS

---

Branch ·