feat: _ANR detector and Sentry reporting Minidump files#8811
feat: _ANR detector and Sentry reporting Minidump files#8811NickKhalow wants to merge 44 commits into
Conversation
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
NickKhalow
added
force-build
NickKhalow
added
force-build
…feat/anr-detection
…nd-macos-only-flags # Conflicts: # .github/workflows/build-unitycloud.yml
|
PR #8811, run #26234952932 Builds: Windows change, Windows baseline, macOS change, macOS baseline Framework 13 i7
|
github-actions
Bot
requested review from
anicalbano,
lorenzo-ranciaffi and
mikhail-dcl
This comment has been minimized.
This comment has been minimized.
NickKhalow
changed the title
|
PR #8811, run #26238775740 Builds: Windows change, Windows baseline, macOS change, macOS baseline Framework 13 i7
|
…plorer into feat/anr-detection
Claude PR Review — security re-review after
|
NickKhalow
changed the title
Claude PR Review — security re-review after
|
|
Claude finished @NickKhalow's task in 4m 13s —— View job Claude PR Review — security re-review after
|
| Previous MEDIUM concern | Status |
|---|---|
| Microsoft Sysinternals redistribution license | Resolved — nothing redistributed |
Authenticode signature verification of bundled procdump*.exe |
Resolved — binaries removed |
| Disproportionate capability shipped to every Windows user | Resolved — in-process API, no external .exe |
The new path is also tighter: opens the current process with PROCESS_QUERY_INFORMATION | PROCESS_VM_READ (the minimum that MiniDumpWriteDump requires for self-dump — no SeDebugPrivilege, no full-memory dump), and the chosen MINIDUMP_TYPE is MiniDumpNormal | MiniDumpWithThreadInfo | MiniDumpWithHandleData | MiniDumpWithUnloadedModules — deliberately not MiniDumpWithFullMemory, which keeps the attachment small and limits data exfiltration surface.
Workflow review — .github/workflows/build-unitycloud.yml
Diff is unchanged from the previous review:
- New
script_debugginginput +script-debuggingPR label → addsDevelopment+AllowDebuggingto build options. - New
enable-sentryPR label → alternative path to the existingsentry_enabledinput. DESTINATION_PATHrefactored into its own step + new "Print S3 upload URL" step writing toGITHUB_STEP_SUMMARY.
W.1–W.10: N/A — workflow is pull_request (not pull_request_target), no LLM step, no untrusted text routed into a prompt, no new allowedTools/wildcards. The label-driven branches all read from github.event.pull_request.labels, which on pull_request requires repo write to set, so external contributors cannot flip sentry_enabled / AllowDebugging for themselves. Fork PRs do not have access to SENTRY_DSN, SENTRY_CLI_AUTH_TOKEN, EXPLORER_TEAM_AWS_*, or the SSL.com code-signing secrets.
Pre-existing, still worth a follow-up: sslcom/esigner-codesign@develop is pinned to a branch on a step that consumes the Windows code-signing TOTP secrets (build-unitycloud.yml:599). Recommend SHA-pinning (W.7a). Not introduced by this PR.
Dependency / binary surface review
| File | Change | Type | Scope | Risk | Confidence |
|---|---|---|---|---|---|
procdump.exe, procdump64.exe, procdump64a.exe |
Removed | Sysinternals PE | runtime, Win | n/a | HIGH |
Explorer/Assets/Plugins/DclNativeProcesses/DCLProcesses.dll (+.exp/.lib) |
Rebuilt | native, built from in-repo dcl_processes.c |
runtime, Win | LOW | HIGH |
Explorer/Assets/Plugins/DclNativeProcesses/libDCLProcesses.dylib |
Not rebuilt (last touched 27b999ed9, 2026-05-05) |
native | runtime, macOS | LOW | HIGH |
Utility.asmdef references 1087662aaf… + 75edf6fa50ff… (DCL.Native.Processes) |
Added | asmdef ref | — | LOW | HIGH |
Net delta vs my previous review: the supply-chain surface has shrunk. The only "binary added by this PR" is a rebuild of the in-repo DCLProcesses.dll, whose source is right next to it.
Native-source review (dcl_processes.c)
- New export
dcl_start_process_blocking(Windows:_spawnvp(_P_WAIT, …); macOS:posix_spawnp+waitpidwithEINTRretry). Implementation looks correct. - Pre-existing memory-leak fix on the Windows path of
get_process_name(now closeshProcesson the success branch) is still good. dcl_start_process_blockinghas zero C# callers anywhere in the repo — the move toMiniDumpWriteDumpmade it dead code in this PR. Worth removing in a follow-up to keep the native surface minimal.libDCLProcesses.dylibdrift persists. It wasn't rebuilt with the newdcl_start_process_blockingexport. Today this is harmless because nothing callsExecuteBlockingand the ANR path is#if UNITY_STANDALONE_WIN. Still worth rebuilding (or, since the function is unused, deleting the export fromdcl_processes.c/DclProcessesNativeMethods.cs).
DEPENDENCY_REVIEW: PASS
Non-security code review — items from previous review still standing
These are minor and were called out last round; would be nice to clean up before merge:
-
Explorer/Assets/DCL/Chat/Commands/AnrDumpChatCommand.cs:2,6—using Cysharp.Threading.Tasks;is still duplicated. Fix this → -
Explorer/Assets/DCL/Chat/Commands/AnrSimulateChatCommand.cs:23-25—int.TryParsereturn value is still ignored andThread.Sleep(freezeMs)runs unconditionally./anr-simulate -1→ArgumentOutOfRangeException;/anr-simulate 2147483647→ ~25-day freeze. Clamp to e.g.[100, 60000]ms inValidateParameters. Fix this → -
Explorer/Assets/DCL/PerformanceAndDiagnostics/Diagnostics/ReportsHandling/Sentry/DclAnrIntegration.cs:35-73—Watchdog.OnApplicationNotResponding +=is still outside theif (Watchdog is null)guard. IfRegisterruns twice (editor reload, or two integrations registered), the same ANR is reported once perRegistercall. Move the+=inside theif. Fix this →
New non-security observations on the native-minidump path
DclAnrIntegration.cs:104-117(NewDumpAttachment) — on dump failure, the original.dmpis not cleaned up; the only cleanup happens inCollectDumpInfoBase64. ANR-path attachments live inApplication.persistentDataPathand will accumulate over time for users that experience repeated ANRs. Consider deleting on failure or on a TTL sweep at startup.DclAnrIntegration.cs:546-589(MiniDumpWriteDump) — Microsoft's guidance is that a hung-process dump is best collected out-of-process, because the dumping thread shares state with the hung one. In practice it usually succeeds for a stuck main thread because the watchdog runs from a background thread, but worth knowing it's a known constraint of the in-process approach.- Privacy note (informational): even without
MiniDumpWithFullMemory, thread-info + handle-data captures thread stack memory and handle tables. Auth tokens, web3 auth chains, encrypted credential material that the main thread has recently touched can end up in the dump that gets uploaded to Sentry. Worth confirming with whoever signed off on Sentry data handling that minidump attachments are acceptable for production rollout — the existing// TODO: validate correctness before production rolloutcomment from the PR description should probably land in code (SentryBuildTimeConfiguration.cs:32) alongsideoptions.DisableAnrIntegration().
Verdict
The PR is substantially cleaner than the previous revision. Procdump removal + in-process MiniDumpWriteDump resolves the dependency-supply-chain concerns I raised earlier, and the workflow / label changes remain safe (no pull_request_target, no LLM secret exposure).
DEPENDENCY_REVIEW: PASS
The three carry-over nits (duplicate using, unbounded Thread.Sleep, double-subscribed ANR handler) and the suggested macOS dylib rebuild / dead-code cleanup are non-blockers.
· branch feat/anr-detection
|
PR #8811, run #26248729154 Builds: Windows change, Windows baseline, macOS change, macOS baseline Framework 13 i7
|
|
Build: Passed: ✅
Everything I can validate from the client side looks good on both platforms. Sentry was verified here Windows with dump MacOS without dump Logs: |
3 participants
Pull Request Description
What does this PR change?
Features
MiniDumpWriteDumpAPI/anr-simulate [ms]— Freeze the main thread to trigger ANR detection (default 10s)/anr-dump— Manually collect and archive a process dump (Windows only)Key components
DclAnrIntegration— SentryISdkIntegrationthat creates and registers the watchdogDclAnrWatchDog(abstract) — Base watchdog with pause/resume awareness, configurable timeout, and minidump collection on reportDclAnrWatchDogMultiThreaded— Desktop implementation: a background thread monitors main-thread heartbeats via a coroutine tick counterDclAnrWatchDogSingleThreaded— WebGL fallback: coroutine-based elapsed-time check (single-threaded environment)DclApplicationNotRespondingException— Custom exception type with Sentry mechanism metadata; on Windows, carries the dump file path for attachmentThreadsDumpUtility— Windows-only utility that:MiniDumpWriteDumpWin32 API (viaMiniDumpNative) to generate a.dmpfile — no external binaries requiredPROCESS_QUERY_INFORMATION | PROCESS_VM_READviaProcessInfoNative, with properIDisposablehandle cleanupApplication.persistentDataPathandApplication.streamingAssetsPathon the main thread ([RuntimeInitializeOnLoadMethod]) since Unity APIs are unavailable from background threadsTools/ProcDump/Dump Current) for quick testing during developmentMiniDumpNative— Static class wrapping theDbghelp.dllMiniDumpWriteDumpP/Invoke with thread-info, handle-data, and unloaded-modules dump flagsProcessInfoNative/ProcessHandle— Win32 process handle management withIDisposablefor safe cleanupDclProcesses.ExecuteBlocking/dcl_start_process_blocking— New native C function for blocking process execution (waits for child exit and returns exit code), available on both Windows (_spawnvp _P_WAIT) and macOS (posix_spawnp+waitpid)Debug chat commands (registered in
DynamicWorldContainer)/anr-simulate [ms]— Freezes the main thread for the specified duration (default 10s) to trigger ANR detection/anr-dump— Manually collects and archives a process dump to the app directory (Windows only); runs on a background thread viaExecuteOnThreadPoolScopeSentry diagnostics & CI
DiagnosticInfoUtilsnow logs whether Sentry is enabled, plus its environment, release, and DSN status at startupenable-sentryPR label — Thebuild-unitycloud.ymlworkflow now checks for anenable-sentrylabel on PR builds, allowing Sentry to be activated for specific PRs without requiring a manual workflow inputscript-debuggingPR label /script_debuggingworkflow input — EnablesAllowDebuggingbuild option (forces aDevelopmentbuild). Can be toggled via thescript-debugginglabel on PR builds or thescript_debugginginput forworkflow_dispatch/workflow_calltriggersReportHub.LogErrorin addition to the user-facing error messageNative code changes
dcl_processes.c/.h— Addeddcl_start_process_blocking()for synchronous child process execution; also fixed aCloseHandleleak inget_process_nameon Windows (handle was closed after the buffer was freed)DCLProcesses.dll/.exp/.lib— Rebuilt native binaries with the new blocking exportDclProcessesNativeMethods.cs— Added managed P/Invoke binding fordcl_start_process_blockingThe built-in Sentry ANR integration is explicitly disabled (
DisableAnrIntegration()) to avoid duplicate detection.Test Instructions
Steps (standard run):
Expected result:
Steps (fresh account):
Expected result:
Prerequisites
Test Steps
/anr-simulatechat command (or/anr-simulate 8000for 8s) to freeze the main thread and trigger ANR detectionDclApplicationNotRespondingExceptionevent with a.dmpfile attachment (ZIP-compressed)/anr-dumpchat command to manually collect a dump and confirm the file paths are printedTools > ProcDump > Dump Currentto verify the editor integration worksenable-sentrylabel to a test PR and verify Sentry is enabled in the resulting buildscript-debugginglabel to a test PR and verify the build includesAllowDebuggingandDevelopmentflagsAdditional Testing Notes
MiniDumpWriteDumptakes longer, the dump will fail gracefullyMiniDumpNormal | WithThreadInfo | WithHandleData | WithUnloadedModulesflags (equivalent to procdump-mt)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.