Repository navigation
Conversation
|
No actionable comments were generated in the recent review. Summary
Change: Other Merge Risk:
|
|
Claude finished @krassx's task in 42s —— View job Review of PR #270: docs on lost ThreadPool exceptions on Mono and late Task exceptionsThis is a docs-only change ( Checked against the code
Findings (both P3, inline)
Minor, not posted inline
Overall risk: Low. There are no code changes, and the only risk is documentation accuracy. Merge recommendation: Approve once the two wording points are addressed. Neither blocks the merge. Most important to fix
Positives
|
|
Claude finished @krassx's task in 20s —— View job Re-review of PR #270 after commit 54955b5Both findings from my earlier review are fixed. I found nothing new. This is a docs-only change, and I did not run tests or device scenarios. Earlier findings
Checked again against the code
No inline comments this round. Optional nit: the CHANGELOG bullet is long. It repeats most of the doc section, and could be shortened to a pointer.
|
There was a problem hiding this comment.
Stale comment
Docs-only review of e16ce2e (
Documentation~/index.md,CHANGELOG.md). No production change. Checked the new claims againstRuntime/Internal/ExceptionPipeline.cs,ManagedExceptionPayload.FromException, AndroidStartLaunch/Settlevs iOSLaunchNow, the existing unobserved-task notes, and the sampleunobservedharness. Did not run tests; CI was still pending at review time.The headline is right. On Mono,
ThreadPool.QueueUserWorkItem(_ => throw ...)is invisible to every hook this pipeline registers (Unity logs nothing,AppDomain.UnhandledExceptionis not raised, process keeps running). That matchesExceptionPipeline(those four subscriptions only) and Unity's own split (staff repro: Mono/Editor ThreadPool does not raise UnhandledException; IL2CPP does; IN-49334, will not fix). IL2CPP reporting twice as handled + crash-type also follows the code: Unity logs viaILogHandler(Report(..., source: LogException), non-fatal unlessTreatUnityLogExceptionsAsFatal) and the pipeline also filesOnUnhandledException(source: AppDomain, fatal).LogExceptionEcho.DuplicateKeyincludessource, so those two are not collapsed.UnobservedTaskExceptionreporting as a handled error with the throw stack matchesOnUnobservedTaskExceptionplus the single-innerAggregateExceptionunwrap inFromException.Two P2s in the new section would send integrators to the wrong workaround or the wrong conclusion about
UnobservedTaskException.
P2:
async voidis not reported normally on the Mono pool path this section is about
- Location:
Documentation~/index.md:213- Problem: The sentence treats
async voidlike a coroutine: Unity logs it and the SDK reports it. That is true forasync voidthat runs underUnitySynchronizationContext(a typical Start / UI / main-thread callback). It is not true forasync voidinvoked from aThreadPoolthread on Mono. ThereSynchronizationContext.Currentis null,AsyncVoidMethodBuilder.SetExceptiondoes not post back to Unity, and the throw lands on the same native-invoke + legacy unhandled-exception path this section documents as silent.- Impact: This paragraph is the contrast table for background work. Readers who take
async voidas the safe alternative toQueueUserWorkItem/ a droppedTaskwill still lose the exception on Android Mono, with no SDK involvement.- Scenario:
ThreadPool.QueueUserWorkItem(_ => Fire()); async void Fire() { throw ...; }or anasync voidhandler subscribed to an event raised from a pool thread. On the measured Mono ARMv7 player this is the pool-loss case, not the coroutine case.- Fix: Qualify it: an
async voidmethod that runs on the Unity main thread (or otherwise under Unity's synchronization context), and coroutines, are logged and reported.async voidcalled from a ThreadPool work item on Mono is lost the same way as a synchronous throw in that work item; catch andBugsee.LogException(or do not useasync voidthere).P2: 4 of 18 did not raise is judged at
WaitForPendingFinalizers, which this package already says is not when Unity raises the event
- Location:
Documentation~/index.md:215(CHANGELOG bullet compresses the same 4 of 18 / never)- Problem: The protocol given is
GC.Collect()+GC.WaitForPendingFinalizers()twice. This repo already records thatUnobservedTaskExceptionfollows that by an unpredictable interval: CHANGELOG forCaptureUnobservedTaskExceptions(seen: 14 to 50 s after a forced GC); sampleAttachmentTaskE2E.UnobservedRoutinecomments the same and waits up to 90 s after Collect/Wait/Collect.- Impact: As written, 4/18 cannot be distinguished from still in the well-known delay window. That over-states or not at all beyond process exit / a still-rooted
Task, and contradicts the earlier arrives-late contract on line 50.- Scenario: Reproducing with only the two GC+finalizer cycles will look like a miss for cases that would have fired at 14-50 s, which is exactly why the sample waits.
- Fix: If the 4/18 already included a post-finalizer wait, say so (duration, same 90 s cap as the sample). If it did not, either re-judge after that wait or keep late and reserve never for process exit / the
Taskremaining reachable.
Overall risk: Low
Docs only. The Mono pool-loss claim, the IL2CPP UnhandledException path, the hook list, and the Task finalizer dependency are accurate against the code and against Unity's documented Mono/IL2CPP split.
Merge recommendation
Request changes. The two P2s are wording in a new public limitation section; they will cause people to pick the wrong background pattern or to treat
UnobservedTaskExceptionas more hopeless than this package has already measured.Most important to fix
- Qualify
async voidas main-thread / Unity sync-context only.- Put the post-finalizer wait into the 4/18 protocol, or drop never except process exit / still-rooted
Task.Positives
- Hook list matches
ExceptionPipeline.Install(ILogHandler,logMessageReceivedThreaded+ echo drop,UnhandledException,UnobservedTaskExceptionbehind!NET_LEGACYandCaptureUnobservedTaskExceptions). NoFirstChanceExceptionis correct.- Mono pool silence is framed as a runtime fact (reproduced without the package), not an SDK miss, and the why is marked as Mono-source knowledge not verified on this build's symbols.
- IL2CPP twice, handled + crash-type is the real AppDomain + Unity-log pair (
DuplicateKeyincludessource); worth documenting. Milestone C1's appears-once only covers the ILogHandler/echo pair.- Measurement matrix (Unity 2021.3.45f1, Honor WOD-LX1 Mono ARMv7 / IL2CPP ARM64, iPhone XS IL2CPP) and the not-measured list are explicit.
Residual, not blocking: after the native launch and again when it succeeds is not how the bridges call
Install. Android installs hooks beforeLaunchand the secondInstallon success only re-syncs the UnobservedTask subscription; iOS installs once immediately after_bugsee_launchandOnNativeStarteddoes not install again. TheLogUnhandledExceptionalternative in the snippet files a crash-type report for a caught exception (the table above already distinguishes it fromLogException).Sent by Cursor Automation: Bugsee code review
There was a problem hiding this comment.
Stale comment
Deep review of docs-only PR #270 at 54955b5 (
Documentation~/index.md,CHANGELOG.md). No production code. Intent: document that MonoThreadPool.QueueUserWorkItemthrows never reach any SDK hook, and that droppedTask.Run/async Taskfaults are reported only viaUnobservedTaskException(late, and claimed sometimes never).Checked against
Runtime/Internal/ExceptionPipeline.cs(ILogHandler wrapper,logMessageReceivedThreadedecho drop,AppDomain.UnhandledException→ fatal,TaskScheduler.UnobservedTaskException→ handled, noFirstChanceException),LogExceptionEcho.DuplicateKey(source is part of the key, so Unity-log + AppDomain is two reports), AndroidInstallHooksbefore launch and again on success, iOSInstallonce after_bugsee_launch, sampleunobserved(AttachmentTaskE2E) and the existing CaptureUnobservedTaskExceptions note.Prior review threads (e16ce2e) are addressed in 54955b5:
new Thread/async void/ coroutine are now in the measured paragraph; the Mono lead sentence is scoped to Unity 2021.3.45f1 Android ARMv7; changelog “did not finalize” is now “did not raise UnobservedTaskException”.New P2: the 4/18 “never” rate is measured with a GC pulse this package already knows is not the observation window for
UnobservedTaskException.Overall risk: Low (docs only)
Merge recommendation: Request changes
Most important to fix: State the post-finalizer wait used for the 4 misses, or drop 4/18 as evidence of “never” (keep “late, or missing if the process exits first”).Positive: Mono pool loss is reproduced without the package, so it is not an SDK hole. IL2CPP dual-report matches
DuplicateKey(..., source)treatingLogExceptionandAppDomainas distinct. Workaround snippet is usable. Unverified Mono-runtime “why” is labelled as such.Sent by Cursor Automation: Bugsee code review
|
Claude finished @krassx's task in 24s —— View job Re-review of PR #270 at 0301df3Docs-only change ( Earlier findings, now fixed by the latest commits
New finding (inline, P3)
Minor, not inline
|
There was a problem hiding this comment.
Stale comment
Deep review of docs-only PR #270 at 0301df3 (
Documentation~/index.md,CHANGELOG.md). No production code. Intent: MonoThreadPool.QueueUserWorkItemthrows never reach any SDK hook; droppedTask.Run/async Taskfaults are reported only viaUnobservedTaskException.Checked against
ExceptionPipeline.Install(ILogHandler wrapper,logMessageReceivedThreadedecho drop,AppDomain.UnhandledException→ fatal,TaskScheduler.UnobservedTaskException→ handled, noFirstChanceException),LogExceptionEcho.DuplicateKey(source is part of the key), AndroidInstallHooksbefore launch / iOSInstallafter_bugsee_launch, sampleAttachmentTaskE2E.UnobservedRoutine(90 s player-loop wait), and the existing CaptureUnobservedTaskExceptions note (CHANGELOG.md:69: 14–50 s after a forced GC). Did not run tests locally. CI at review: Unity C# compile, wrapper-policy, Android wrapper, Windows Editor, and CodeQL passed; iOS bridge tests still pending.Prior findings addressed in 74d9366 / 0301df3:
async voidis limited to the Unity main thread, with Mono poolasync voidmarked unmeasured / expect-lost. The docs now state the GC protocol (8 s and 11 s after the throw, observed until 16 s) and that the event can arrive tens of seconds after a collection. Hook timing is “during launch”.LogUnhandledExceptionwas dropped from the snippet.Remaining P2:
0301df3still presents 4/18 as “late or never” after “about 8 s”, which is not this package’s UnobservedTaskException window.
Overall risk: Low
Docs only. Headline Mono pool-loss, IL2CPP dual-report, hook list, and Task finalizer dependency remain accurate against the pipeline and against Unity IN-49334.
Merge recommendation
Request changes. One P2 left, in the changelog bullet this commit was supposed to fix.
Most important to fix
- Stop using 4/18-in-~8 s as evidence of “never”. Match
Documentation~/index.md:215(16 s of the throw, GC at 8 s and 11 s) and this file’s own 14–50 s delay; reserve missing for process exit / a task that is never collected.Positives
- Mono pool silence is still framed as a runtime fact (reproduced without the package), with the “why” labelled as Mono-source knowledge not verified on this build’s symbols.
- IL2CPP twice (handled + crash-type) matches
DuplicateKey(..., source)treatingLogExceptionandAppDomainas distinct.- Measurement matrix and the not-measured list (Windows, other Unity versions, pool
async void) are explicit.- Workaround snippet now files a handled error (
LogException), matching the table above it.Sent by Cursor Automation: Bugsee code review
|
Claude finished @krassx's task in 15s —— View job Re-review of PR #270 at 027deeeDocs-only change ( Earlier findings, now fixed
New findings: none, so no inline comments this round. Optional nits, not blocking
|
There was a problem hiding this comment.
Stale comment
Deep review of docs-only PR #270 at 027deee (
Documentation~/index.md,CHANGELOG.md). No production code. Intent: MonoThreadPool.QueueUserWorkItemthrows never reach any SDK hook; droppedTask.Run/async Taskfaults are reported only viaUnobservedTaskException.Checked against
ExceptionPipeline.Install(ILogHandler wrapper,logMessageReceivedThreadedecho drop,AppDomain.UnhandledException→ fatal,TaskScheduler.UnobservedTaskException→ handled, noFirstChanceException),LogExceptionEcho.DuplicateKey(source is part of the key, so Unity-log + AppDomain is two reports), AndroidInstallHooksbefore launch / again on success vs iOSInstallafter_bugsee_launch,ManagedExceptionPayload.FromException(single-innerAggregateExceptionunwrap), sampleAttachmentTaskE2E.UnobservedRoutine(90 s player-loop wait), and the existing CaptureUnobservedTaskExceptions note (CHANGELOG.md:69: 14–50 s after a forced GC). NullRefE2E’s “thread-pool” case isTask.Runwith theTaskkept, so it does not contradict theQueueUserWorkItemsilence claim. Did not run tests locally (docs-only). CI at review: Unity C# compile, wrapper-policy, Android wrapper, Windows Editor, and CodeQL passed; iOS bridge tests still pending.Prior findings addressed in 027deee: the remaining changelog P2 is gone.
CHANGELOG.md:12now matchesDocumentation~/index.md:215: “late, or missing if the process exits first (4 of 18 did not raiseUnobservedTaskExceptionwithin 16 s of the throw, with forced GC at 8 s and 11 s; the event can arrive tens of seconds after a collection)”. Earlierasync void/ GC-window / hook-timing P2s stay addressed.No remaining P0–P3.
Overall risk: Low
Docs only. Headline Mono pool-loss, IL2CPP dual-report, hook list, and Task finalizer dependency remain accurate against the pipeline and against Unity IN-49334.
Merge recommendation
Approve.
Most important issues to fix
None remaining.
Positives
- Mono pool silence is framed as a runtime fact (reproduced without the package), with the “why” labelled as Mono-source knowledge not verified on this build’s symbols.
- IL2CPP twice (handled + crash-type) matches
DuplicateKey(..., source)treatingLogExceptionandAppDomainas distinct.- Measurement matrix and the not-measured list (Windows, other Unity versions, pool
async void) are explicit.- Workaround snippet files a handled error (
LogException), matching the table above it and the fact that the Mono pool throw does not kill the process.- Changelog 4/18 is now a 16 s observation, not a “never” rate, which matches this package’s own 14–50 s / 90 s UnobservedTaskException window.
Sent by Cursor Automation: Bugsee code review
…ceptions Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfQe6YfjwEeceEeuf3jS6x
…ements, fix changelog wording Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfQe6YfjwEeceEeuf3jS6x
…x hook timing wording Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfQe6YfjwEeceEeuf3jS6x
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfQe6YfjwEeceEeuf3jS6x
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfQe6YfjwEeceEeuf3jS6x
027deee to
95801f1
Compare
|
Claude finished @krassx's task in 15s —— View job Re-review of PR #270 at 95801f1Docs-only change ( Earlier findings: all addressed by the commits on this branch
New findings: none, so no inline comments this round. Optional nits, not blocking
|
There was a problem hiding this comment.
Deep review of docs-only PR #270 at 95801f1 (Documentation~/index.md, CHANGELOG.md). No production code. Intent: Mono ThreadPool.QueueUserWorkItem throws never reach any SDK hook; dropped Task.Run / async Task faults are reported only via UnobservedTaskException.
Checked against ExceptionPipeline.Install (ILogHandler wrapper, logMessageReceivedThreaded echo drop, AppDomain.UnhandledException → fatal, TaskScheduler.UnobservedTaskException → handled, no FirstChanceException), LogExceptionEcho.DuplicateKey (source is part of the key, so Unity-log + AppDomain is two reports), AndroidBridge.SendManagedPayload (JNI with no AndroidJniScope), ExceptionPipeline.DispatchPayload (handled reports hop to the main thread), sample SignatureThreadsE2E (attaches JNI on a worker before Bugsee.LogException), and the existing CaptureUnobservedTaskExceptions note (CHANGELOG.md:71: 14–50 s after a forced GC). NullRefE2E’s “thread-pool” case is Task.Run with the Task kept, so it does not contradict the QueueUserWorkItem silence claim. Did not run tests locally (docs-only). CI at review: Unity C# compile, Android wrapper, wrapper-build-id, and CodeQL (actions) passed; wrapper-policy, iOS bridge, Windows Editor, and CodeQL csharp/java still in progress.
Prior findings addressed (rebased as 7ef86a4 / 0110060 / a097978 / 95801f1): async void is limited to the Unity main thread, with Mono pool async void marked unmeasured / expect-lost. Docs and changelog both state the GC protocol (8 s and 11 s after the throw, observed until 16 s) and that the event can arrive tens of seconds after a collection. Changelog says “late, or missing if the process exits first”, not “never”. Hook timing is “during launch”. Workaround snippet files a handled error (LogException).
New P2: the documented Android Mono recovery calls Bugsee.LogException on the pool thread itself. That path does JNI without an attach; this same file already says that aborts the process.
Overall risk: Low
Docs only. Headline Mono pool-loss, IL2CPP dual-report, hook list, and Task finalizer dependency remain accurate against the pipeline and against Unity IN-49334. The new issue is the workaround snippet on the measured Android player, not the limitation itself.
Merge recommendation
Request changes. One P2: do not tell people to call Bugsee.LogException from QueueUserWorkItem on Android without an attach or a hop to the Unity main thread.
Most important issues to fix
- Make the pool-thread workaround JNI-safe on Android (
Debug.LogException, main-thread hop, orAndroidJNI.AttachCurrentThread/ wrapSendManagedPayloadinAndroidJniScope).
Positives
- Mono pool silence is framed as a runtime fact (reproduced without the package), with the “why” labelled as Mono-source knowledge not verified on this build’s symbols.
- IL2CPP twice (handled + crash-type) matches
DuplicateKey(..., source)treatingLogExceptionandAppDomainas distinct. - Measurement matrix and the not-measured list (Windows, other Unity versions, pool
async void) are explicit. - Changelog 4/18 is a 16 s observation, not a “never” rate, which matches this package’s own 14–50 s / 90 s UnobservedTaskException window.
Sent by Cursor Automation: Bugsee code review
| ThreadPool.QueueUserWorkItem(_ => | ||
| { | ||
| try { DoWork(); } | ||
| catch (Exception e) { Bugsee.LogException(e); } |
There was a problem hiding this comment.
Severity: P2 Medium
Location: Documentation~/index.md:209 (same call in the ContinueWith(..., OnlyOnFaulted) sentence at :213)
Problem: The workaround this section exists to teach is catch + Bugsee.LogException inside ThreadPool.QueueUserWorkItem. On Android that is a public JNI call with no attach. AndroidBridge.SendManagedPayload builds a UnityManagedException and calls Bugsee.logException on the calling thread and does not enter AndroidJniScope. Span APIs and GetLaunched do, because this same file already states the JVM “aborts the process on a JNI call from an unattached thread” (Documentation~/index.md:683; JniAttachPolicy / AndroidJniScope comments: CheckJNI “use of deleted local reference”, Galaxy Tab A 8.0). ExceptionPipeline.DispatchPayload hops handled auto-capture onto the Unity main thread before the same JNI; the public Bugsee.LogException path does not. Sample SignatureThreadsE2E only calls Bugsee.LogException from a worker after AndroidJNI.AttachCurrentThread().
Impact: On the configuration this paragraph measured (Android Mono ARMv7), following the snippet can replace a silently lost exception (process keeps running) with a hard abort. ContinueWith(..., OnlyOnFaulted) plus Bugsee.LogException is the same pool-thread JNI call.
Scenario: Copy the snippet onto the Honor / Unity 2021.3.45f1 Mono player. DoWork() throws, catch runs on a CLR thread-pool thread (IsThreadPoolThread true, the case JniAttachPolicy treats as AttachDetach). Bugsee.LogException → SendManagedPayload → new AndroidJavaObject(...) with no JNIEnv. Auto-capture of UnobservedTaskException would have been safe because it is handled and MainThreadDispatcher.Runs the send; this snippet bypasses that hop.
Fix: Point the snippet at a path that is already safe from a background thread: Debug.LogException(e) (CHANGELOG: background Debug.LogException now reports; the pipeline hops handled sends to main). Or keep Bugsee.LogException and say it must run on the Unity main thread, or after AndroidJNI.AttachCurrentThread() (and detach a pool thread), matching sigthreads. The durable product fix is using (AndroidJniScope.Enter()) around SendManagedPayload, which this docs-only PR does not have to include.


What
Docs only (
Documentation~/index.md,CHANGELOG.md). No code changes.logMessageReceivedThreadedfallback with dedup,AppDomain.UnhandledException,TaskScheduler.UnobservedTaskException), checked againstRuntime/Internal/ExceptionPipeline.cs.ThreadPoolwork item on Mono is lost with no SDK involvement; IL2CPP raisesAppDomain.UnhandledExceptionand is reported. Workaround: catch in the work item and callBugsee.LogException.Task.Run/ a droppedasync Taskis reported only when the faulted task is finalized, so it can be late or missing.Evidence
Unity 2021.3.45f1; physical Honor WOD-LX1 (Android 15, Mono ARMv7 and IL2CPP ARM64) and iPhone XS (iOS 18.7.9, IL2CPP). The pool loss was reproduced without the Bugsee package on Mono Release and Development. With forced GC, 4 of 18 task cases still did not finalize. Not measured: Windows, other Unity versions. The Mono "why" is from Mono runtime source knowledge, not verified on symbols of this build, and is marked as such in the docs.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SfQe6YfjwEeceEeuf3jS6x