fix: Detect pause-point interruption during Press/KeyDown observation wait#1919
Conversation
Round-7 feedback traps kept getting rediscovered because each round's repro lived only in a throwaway memo. Establish a permanent, re-runnable scene+script convention under Assets/RegressionHarness/<TrapName>/ (docs in docs/regression-harness.md) and add the first scene for the pause-point key-state divergence investigation (KeyStateAfterPauseInterruption).
WaitForPressLifetime and WaitForRuntimeFrames could silently absorb a genuine Unity pause into a false "Completed" result: real time keeps advancing while paused, so a duration/frame-count condition could become satisfied inside the same window a pause started, with no re-check of pause state at that exact return point. Add EditorPauseAwaiter (event-based, races EditorApplication.pauseStateChanged against the existing frame/timeout wait) and extract the post-wait decision into PressLifetimeIterationResolver so the pause-priority rule is unit testable independent of the Editor player loop. Add a regression harness scene/script that arms a pause-point on a key-consuming line before Press starts, verifying the interruption is reported before the requested duration elapses.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds pause-aware frame and press-lifetime handling, centralizes outcome resolution, adds tests, and introduces a manual Unity regression harness for Space-key interruption after a pause-point. ChangesPause interruption handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant simulate-keyboard
participant InputSystemRuntimeFrameWaiter
participant EditorPauseAwaiter
participant UnityEditor
simulate-keyboard->>InputSystemRuntimeFrameWaiter: start press-lifetime wait
InputSystemRuntimeFrameWaiter->>EditorPauseAwaiter: race frame/timeout against pause
EditorPauseAwaiter->>UnityEditor: subscribe to pauseStateChanged
UnityEditor-->>EditorPauseAwaiter: report PauseState.Paused
EditorPauseAwaiter-->>InputSystemRuntimeFrameWaiter: return Paused
InputSystemRuntimeFrameWaiter-->>simulate-keyboard: return InterruptedByPausePoint
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
EditorPauseAwaiter deliberately never reads EditorApplication.isPaused (only subscribes to pauseStateChanged), so the comment claiming otherwise was corrected. The harness script's wait under set -e would exit before printing the RESULT_FILE diagnostics on a non-zero Press exit; tolerate that and let the jq/FAIL check report it instead.
There was a problem hiding this comment.
Actionable comments posted: 1
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/InputSystem/InputSystemRuntimeFrameWaiter.cs (1)
42-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
WaitForRuntimeFrames'sTimedOutbranch is missing the pause fallback recheck thatWaitForPressLifetimenow has.Right below, a genuine
frameOutcome == Completedresult gets anIsPaused()recheck (lines 56-60) before continuing, but aTimedOutresult returns immediately without ever checkingIsPaused(). SinceWaitOneRuntimeFrameOrTimeout's wall-clock timeout keeps advancing during a real pause, this reintroduces the exact "pause absorbed into a wrong outcome" bug this PR is fixing elsewhere — just asTimedOuthere instead ofCompletedinWaitForPressLifetime.WaitForPressLifetimenow guards against this viaPressLifetimeIterationResolver'sisPausedFallback; this sibling method has no equivalent guard.🐛 Proposed fix
if (frameOutcome == InputSimulationWaitOutcome.TimedOut) { - return InputSimulationWaitOutcome.TimedOut; + await InputSystemUpdateHelper.SwitchToMainThreadIfNeeded(ct); + if (InputSystemUpdateHelper.IsPaused()) + { + return InputSimulationWaitOutcome.Paused; + } + + return InputSimulationWaitOutcome.TimedOut; }🤖 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/InputSystem/InputSystemRuntimeFrameWaiter.cs` around lines 42 - 54, Update the TimedOut handling in WaitForRuntimeFrames to recheck IsPaused() before returning. If the timeout occurred while paused, return InputSimulationWaitOutcome.Paused; otherwise preserve the existing TimedOut result, matching the pause-fallback behavior used by WaitForPressLifetime.
🧹 Nitpick comments (1)
scripts/regression-harness-key-state-after-pause-interruption.sh (1)
66-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
set -ecan swallow diagnostics on a failedsimulate-keyboardrun.If the backgrounded
run_uloop simulate-keyboardcall (or thejqparse at lines 72-73) exits non-zero,set -e(line 2) terminates the script right there, before the FAIL log /cat "$RESULT_FILE"diagnostics at lines 76-79 run — only thecleanuptrap fires. That's the exact scenario where the diagnostic output is most needed.♻️ Proposed fix to preserve diagnostics on failure
-wait "$PRESS_PID" +set +e +wait "$PRESS_PID" +PRESS_EXIT_CODE=$? +set -e +if [ "$PRESS_EXIT_CODE" -ne 0 ]; then + log "FAIL: simulate-keyboard exited with code $PRESS_EXIT_CODE" + cat "$RESULT_FILE" + exit 1 +fi ELAPSED_SECONDS=$(( $(date +%s) - START_SECONDS ))🤖 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 `@scripts/regression-harness-key-state-after-pause-interruption.sh` around lines 66 - 79, Update the regression flow around the backgrounded run_uloop command and the jq extractions so failures do not trigger set -e before diagnostics are emitted. Capture each command’s exit status without aborting, then route non-zero results through the existing FAIL log and cat "$RESULT_FILE" path while preserving the Success and InterruptedByPausePoint validation.
🤖 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/InputSystem/InputSystemRuntimeFrameWaiter.cs`:
- Around line 207-213: Update the wait flow around Task.WhenAny in
InputSystemRuntimeFrameWaiter so execution switches back to the main editor
thread before calling raceCancellation.Cancel(). Preserve the existing winner
check and paused outcome handling, ensuring EditorPauseAwaiter’s cancellation
callback unregisters pauseStateChanged on the editor thread.
---
Outside diff comments:
In
`@Packages/src/Editor/FirstPartyTools/Common/InputSystem/InputSystemRuntimeFrameWaiter.cs`:
- Around line 42-54: Update the TimedOut handling in WaitForRuntimeFrames to
recheck IsPaused() before returning. If the timeout occurred while paused,
return InputSimulationWaitOutcome.Paused; otherwise preserve the existing
TimedOut result, matching the pause-fallback behavior used by
WaitForPressLifetime.
---
Nitpick comments:
In `@scripts/regression-harness-key-state-after-pause-interruption.sh`:
- Around line 66-79: Update the regression flow around the backgrounded
run_uloop command and the jq extractions so failures do not trigger set -e
before diagnostics are emitted. Capture each command’s exit status without
aborting, then route non-zero results through the existing FAIL log and cat
"$RESULT_FILE" path while preserving the Success and InterruptedByPausePoint
validation.
🪄 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: 530162f6-51c2-4269-9200-1256483b32a6
⛔ Files ignored due to path filters (8)
Assets/RegressionHarness.metais excluded by none and included by noneAssets/RegressionHarness/KeyStateAfterPauseInterruption.metais excluded by none and included by noneAssets/RegressionHarness/KeyStateAfterPauseInterruption/KeyStateAfterPauseInterruption.unityis excluded by!**/*.unityand included by noneAssets/RegressionHarness/KeyStateAfterPauseInterruption/KeyStateAfterPauseInterruption.unity.metais excluded by none and included by noneAssets/RegressionHarness/KeyStateAfterPauseInterruption/SpaceHoldPoller.cs.metais excluded by none and included by noneAssets/Tests/Editor/PressLifetimeIterationResolverTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Common/InputSystem/EditorPauseAwaiter.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Common/InputSystem/PressLifetimeIterationResolver.cs.metais excluded by none and included by none
📒 Files selected for processing (8)
AGENTS.mdAssets/RegressionHarness/KeyStateAfterPauseInterruption/SpaceHoldPoller.csAssets/Tests/Editor/PressLifetimeIterationResolverTests.csPackages/src/Editor/FirstPartyTools/Common/InputSystem/EditorPauseAwaiter.csPackages/src/Editor/FirstPartyTools/Common/InputSystem/InputSystemRuntimeFrameWaiter.csPackages/src/Editor/FirstPartyTools/Common/InputSystem/PressLifetimeIterationResolver.csdocs/regression-harness.mdscripts/regression-harness-key-state-after-pause-interruption.sh
…ead safety WaitForRuntimeFrames's TimedOut branch had no pause recheck, unlike the Completed branch right below it and WaitForPressLifetime's TimedOut branch — reintroducing the same class of pause-absorbed-into-wrong-outcome bug this PR fixes elsewhere. Also switch back to the main thread before raceCancellation.Cancel(), since EditorPauseAwaiter's cancellation callback unsubscribes EditorApplication.pauseStateChanged (main-thread-only) and the preceding ConfigureAwait(false) may have left the continuation off-thread.
df3c171
into
feature/round7-pause-point-improvements
Summary
WaitForPressLifetime/WaitForRuntimeFramescould silently absorb a genuine Unity pause into a falseCompletedresult: real time keeps advancing while paused, so a duration/frame-count condition could become satisfied inside the same real-time window a pause started, and the original code re-checked pause state at some return points but not theTimedOut-with-duration-already-satisfied one.EditorPauseAwaiter(event-based, racesEditorApplication.pauseStateChangedagainst the existing frame/timeout wait) so a pause is observed the instant it happens instead of only between completed waits.PressLifetimeIterationResolverso the pause-priority rule is unit-testable independent of the Editor player loop.Assets/RegressionHarness/KeyStateAfterPauseInterruption/,scripts/regression-harness-key-state-after-pause-interruption.sh) that arms a pause-point on a key-consuming line before Press starts, so the hit fires naturally from game code, and verifies the interruption is reported before the requested duration elapses.Verification
EditorApplication.updateticks stall for several seconds while paused (main-thread continuation starvation) — an environment condition, not something a CLI command sequence can reliably orchestrate. Attempting to synthesize the stall (e.g. racing a second CLI command against Press) was tried and rejected: any concurrent CLI command is itself subject to the same starvation, making that approach structurally unstable and non-deterministic.isPausedFallbackcheck inPressLifetimeIterationResolver.ResolvePostWaitOutcometemporarily reverted to pre-fix behavior,PressLifetimeIterationResolverTests.ResolvePostWaitOutcome_WhenTimedOutWithDurationSatisfiedAndPausedFallback_ReturnsPausedfails:Expected: Paused / But was: Completed. With the fix restored, the same test (and the other 5 in the file) pass: 6/6.feature/round7-pause-point-improvementsHEAD, which fail identically (2 dispatcher-version-pin drift assertions inCliSetupApplicationServiceTests, 1DescriptionAttributeexposure assertion inFirstPartyToolSchemaMetadataTests— none touch files this PR changes).simulate-keyboard Press --key Space --duration 5returnsInterruptedByPausePoint: trueimmediately (0s elapsed, well under the 5s requested).uloop compile: 0 errors, 0 warnings.Known related observation (out of scope for this PR)
The ~5s+ delay in
enable-pause-point's reportedEnabledAtUtcsometimes observed during heavy concurrent CLI usage is a symptom of main-thread continuation-queue starvation, not command serialization — there is no locking in the IPC command dispatch path. This is the same root cause that makes the swallow bug in this PR environment-dependent. Fixing the starvation itself is out of scope here.Test plan
uloop compile— 0 errors, 0 warningsuloop run-tests --test-mode EditMode --filter-value PressLifetimeIterationResolverTests— 6/6 passed (and confirmed failing pre-fix)uloop run-tests --test-mode PlayMode— 93/93 passeduloop run-tests --test-mode EditMode— 2268/2271 passed, 3 pre-existing unrelated failures confirmed via baseline stash comparisonscripts/regression-harness-key-state-after-pause-interruption.sh— PASS (interrupted after 0s, under the 5s requested duration)