fix: Cancelled test runs no longer hang, and log retrieval responds faster#1321
Merged
Merged
Conversation
ExecuteTestWithEventNotification awaited a TaskCompletionSource that was only resolved by the test-completion callback. When the CancellationToken fired while tests were still running (e.g. dropped CLI connection), the await never completed, keeping the TestRunnerApi callback subscription alive indefinitely. Register the token so cancellation resolves the TCS, letting the using-scoped callback dispose and unsubscribe.
…lookups GetLogEntryAt resolved the LogEntry type, the GetEntryInternal method, and three FieldInfo handles from scratch for every console entry, and the mask helpers re-resolved the consoleFlags property on every call. With thousands of console entries these repeated reflection lookups dominated get-logs latency. All handles are now resolved once in the constructor, which also states the Unity-internal members this class depends on in one place.
Every tool invocation allocated a fresh JsonSerializerSettings plus CamelCasePropertyNamesContractResolver, throwing away the resolver's per-type contract cache each time, and every ParameterSchema access regenerated the schema through full reflection. JsonRpcProcessor likewise allocated identical serializer settings on all five response paths. Hold one shared camelCase serializer for parameter conversion, cache the generated schema per closed TSchema type, and unify the response settings into a single static readonly instance.
FilterAndLimitLogs copied the entries into three intermediate arrays via Skip/Reverse/Select; it now fills a single pre-sized array in one reverse pass. SearchConsoleLogs rebuilt an interpreted Regex from the user pattern on every call even though AI clients poll with the same pattern; the last pattern is now kept compiled and reused. Behavior is pinned by new direct unit tests for LogFilteringService (ordering, limiting, stack-trace handling, argument validation).
…e-subscription MonitorClientDisconnectAsync delayed without the request token, so every request teardown and server shutdown waited out a full poll interval before the monitor noticed cancellation; the delay is now cancelled directly. UnityCliLoopPackageRemovalSettingsResetter subscribed to Events.registeringPackages without the unsubscribe-first guard used by the other editor-lifetime subscriptions, allowing duplicate handlers if registration ever ran twice.
Replace per-iteration time.After timers with a single time.NewTicker per wait loop (pause point, play mode, connection retry, tool readiness, compile status), matching the existing waitForUnityLockfile idiom and avoiding a timer allocation per tick. Read the tool catalog once per readiness probe sequence instead of three times (each read walks the project for the catalog file), and compile the completion-block regex at package level like every other pattern in the package.
Contributor
|
Warning Review limit reached
More reviews will be available in 21 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCaches expensive runtime/serialization resources, replaces per-iteration timers with reusable tickers in CLI polling loops, and fixes cancellation and duplicate-event-handler behavior; adds tests for LogFilteringService. ChangesPerformance Optimization through Resource Caching and Cancellation Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Packages/src/Editor/FirstPartyTools/Common/Console/ConsoleLogRetriever.cs`:
- Around line 44-51: ConsoleLogRetriever currently only validates
_logEntriesType/_logEntryType but lets reflection member lookups
(_consoleFlagsProperty, _getCountMethod, _getEntryInternalMethod, _messageField,
_modeField, _callstackTextStartField) be null and silently degrades
GetLogCount()/GetLogEntryAt(); update the constructor/initialization in
ConsoleLogRetriever so that after performing the reflection lookups you verify
each of those members is non-null and, if any are missing, throw an informative
InvalidOperationException (or similar) that names the missing members and the
expected Unity internal member names; also remove or avoid null-coalescing
fallbacks in GetLogCount()/GetLogEntryAt() so they assume members exist (the
constructor check guarantees this). This ensures a fast, clear failure when
Unity internal reflection contracts change.
In `@Packages/src/Editor/FirstPartyTools/Common/Console/LogGetter.cs`:
- Around line 288-292: GetOrCreateSearchRegex currently constructs user-supplied
Regex without a timeout which can hang SearchConsoleLogs when IsMatch is called;
change the Regex creation in GetOrCreateSearchRegex (and any cache assignment of
_cachedSearchRegex) to use the overload that accepts a TimeSpan (e.g., new
Regex(searchText, RegexOptions.Compiled, TimeSpan.FromMilliseconds(...))) so a
finite timeout is enforced, choose a sensible small timeout value, and leave the
caching logic ( _cachedSearchRegex ) and consumers like SearchConsoleLogs
unchanged except they will now receive a Regex with a timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e61bd73-f420-4d68-b9ab-1bc1120754a2
⛔ Files ignored due to path filters (1)
Assets/Tests/Editor/LogFilteringServiceTests.cs.metais excluded by none and included by none
📒 Files selected for processing (15)
Assets/Tests/Editor/LogFilteringServiceTests.csPackages/src/Editor/FirstPartyTools/Common/Console/ConsoleLogRetriever.csPackages/src/Editor/FirstPartyTools/Common/Console/LogGetter.csPackages/src/Editor/FirstPartyTools/GetLogs/LogFilteringService.csPackages/src/Editor/FirstPartyTools/RunTests/TestFramework/PlayModeTestExecuter.csPackages/src/Editor/Infrastructure/Api/JsonRpcProcessor.csPackages/src/Editor/Infrastructure/Settings/UnityCliLoopPackageRemovalSettingsResetter.csPackages/src/Editor/Infrastructure/UnityCliLoopBridgeServer.csPackages/src/Editor/ToolContracts/UnityCliLoopTool.cscli/internal/cli/compile_wait.gocli/internal/cli/completion.gocli/internal/cli/connection_retry.gocli/internal/cli/control_play_mode_wait.gocli/internal/cli/pause_point_wait.gocli/internal/cli/tool_readiness.go
There was a problem hiding this comment.
3 issues found across 16 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review feedback: the cached reflection members could be null after an internal Unity API change, and the per-method null guards silently degraded into zero counts and empty log data. Validate all members at construction like the existing type checks, and remove the now unreachable null guards and ConsoleWindow fallback.
Review feedback: a pathologically backtracking user pattern could stall the Editor main thread while get-logs matches every console entry. The cached compiled regex now carries a two-second match timeout.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/src/Editor/FirstPartyTools/Common/Console/ConsoleLogRetriever.cs (1)
31-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
ConsoleWindowtype lookup.This line retrieves the
ConsoleWindowtype but discards the result. Based on the summary, this appears to be leftover from the removedGetMaskFromConsoleWindowfallback.🧹 Suggested fix
_logEntriesType = editorAssembly.GetType("UnityEditor.LogEntries"); - editorAssembly.GetType("UnityEditor.ConsoleWindow"); if (_logEntriesType == null)🤖 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 `@Packages/src/Editor/FirstPartyTools/Common/Console/ConsoleLogRetriever.cs` at line 31, Remove the stray unused type lookup call editorAssembly.GetType("UnityEditor.ConsoleWindow") from ConsoleLogRetriever.cs since its return value is discarded and the GetMaskFromConsoleWindow fallback was removed; locate the call in the body of the ConsoleLogRetriever class (where editorAssembly is used) and delete that single statement to clean up dead code.
🤖 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.
Outside diff comments:
In `@Packages/src/Editor/FirstPartyTools/Common/Console/ConsoleLogRetriever.cs`:
- Line 31: Remove the stray unused type lookup call
editorAssembly.GetType("UnityEditor.ConsoleWindow") from ConsoleLogRetriever.cs
since its return value is discarded and the GetMaskFromConsoleWindow fallback
was removed; locate the call in the body of the ConsoleLogRetriever class (where
editorAssembly is used) and delete that single statement to clean up dead code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7fd73692-764b-40e5-acf7-96aff30e9ddd
📒 Files selected for processing (2)
Packages/src/Editor/FirstPartyTools/Common/Console/ConsoleLogRetriever.csPackages/src/Editor/FirstPartyTools/Common/Console/LogGetter.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Packages/src/Editor/FirstPartyTools/Common/Console/LogGetter.cs
The repository convention advances cli/contract.json and MINIMUM_REQUIRED_CLI_VERSION in lockstep whenever Go CLI sources change; the build-cli gate enforces it. This PR's CLI changes are internal optimizations with no new contract dependency, so this is the mechanical one-step bump. Version gate tests reference the constant symbolically and pass unchanged.
Merged
This was referenced Jun 14, 2026
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
1 participant
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
get-logsresponds faster on large consoles, and every tool request does less redundant work per call.User Impact
get-logson consoles with thousands of entries is noticeably cheaper: per-entry reflection lookups, triple array copies, and per-call regex rebuilds are gone.Changes
Notes for reviewers
time.Afterrestarted the timer after each loop body, while a ticker fires on a fixed schedule. If a loop body (e.g. a slow connection-retry send) runs longer than the poll interval, the next iteration starts immediately from the buffered tick instead of waiting one more interval. Timeout termination conditions are unchanged.Verification
uloop compileafter each change: 0 errors, 0 warnings.origin/v3-beta).scripts/check-go-cli.sh: formatting, vet, lint, tests, and dist rebuild all pass.uloop get-logsagainst a running Editor with the rebuilt dev binary.