diff --git a/.agents/skills/uloop-pause-point/references/fast-progressing-games.md b/.agents/skills/uloop-pause-point/references/fast-progressing-games.md index 281998603c..4539d3f8b7 100644 --- a/.agents/skills/uloop-pause-point/references/fast-progressing-games.md +++ b/.agents/skills/uloop-pause-point/references/fast-progressing-games.md @@ -28,6 +28,8 @@ Digit keys are `Digit0`-`Digit9` or `Numpad0`-`Numpad9` — bare `0`-`9` is reje `--resume-play` runs after the marker's arming is confirmed and before `--trigger` is dispatched: it resumes PlayMode only when PlayMode is actually paused, and reports what it did in `ResumePlayResult` (`WasPaused` / `Resumed` / `Error`; an abandoned wait adds `Repaused` / `RepauseError`). If the resume fails, the trigger is not dispatched and `TriggerResult.Error` says so. If the trigger itself is rejected before it runs, the wait is abandoned and the resume is undone: `Repaused: true` (or `RepauseError`) reports PlayMode being paused again, so gameplay cannot consume the preserved marker while the trigger value is being fixed. When the game reaches the line on its own after resuming (gravity, physics), omit `--trigger` and keep `--resume-play`. +One case skips the resume on purpose: if the marker had **already** hit by the time the wait confirmed its arming (a single-shot marker, or any marker whose mode does not wait for a new hit — including one just armed by the same `enable-pause-point --await` call), the wait settles on that recorded hit instantly, so resuming would restart the game and then report an old hit. The wait returns the recorded hit and reports `ResumePlayResult.Skipped` (with `Resumed: false` and no `Error`); a `--trigger` given alongside is not dispatched and says so in `TriggerResult.Error`. Play Mode is left as it is (still paused when that hit paused it) — clear or re-enable the marker and wait again to capture a new one. An already-hit `continuous`/`trace` marker that was armed earlier is unaffected: its wait needs a later hit, so it still resumes. + ## Do Not Use Time.timeScale = 0 Projects that read unscaled time keep advancing regardless, and the value silently persists into the next PlayMode session. Editor pause and `Step` freeze the entire player loop independent of `Time.timeScale`. diff --git a/.claude/skills/uloop-pause-point/references/fast-progressing-games.md b/.claude/skills/uloop-pause-point/references/fast-progressing-games.md index 281998603c..4539d3f8b7 100644 --- a/.claude/skills/uloop-pause-point/references/fast-progressing-games.md +++ b/.claude/skills/uloop-pause-point/references/fast-progressing-games.md @@ -28,6 +28,8 @@ Digit keys are `Digit0`-`Digit9` or `Numpad0`-`Numpad9` — bare `0`-`9` is reje `--resume-play` runs after the marker's arming is confirmed and before `--trigger` is dispatched: it resumes PlayMode only when PlayMode is actually paused, and reports what it did in `ResumePlayResult` (`WasPaused` / `Resumed` / `Error`; an abandoned wait adds `Repaused` / `RepauseError`). If the resume fails, the trigger is not dispatched and `TriggerResult.Error` says so. If the trigger itself is rejected before it runs, the wait is abandoned and the resume is undone: `Repaused: true` (or `RepauseError`) reports PlayMode being paused again, so gameplay cannot consume the preserved marker while the trigger value is being fixed. When the game reaches the line on its own after resuming (gravity, physics), omit `--trigger` and keep `--resume-play`. +One case skips the resume on purpose: if the marker had **already** hit by the time the wait confirmed its arming (a single-shot marker, or any marker whose mode does not wait for a new hit — including one just armed by the same `enable-pause-point --await` call), the wait settles on that recorded hit instantly, so resuming would restart the game and then report an old hit. The wait returns the recorded hit and reports `ResumePlayResult.Skipped` (with `Resumed: false` and no `Error`); a `--trigger` given alongside is not dispatched and says so in `TriggerResult.Error`. Play Mode is left as it is (still paused when that hit paused it) — clear or re-enable the marker and wait again to capture a new one. An already-hit `continuous`/`trace` marker that was armed earlier is unaffected: its wait needs a later hit, so it still resumes. + ## Do Not Use Time.timeScale = 0 Projects that read unscaled time keep advancing regardless, and the value silently persists into the next PlayMode session. Editor pause and `Step` freeze the entire player loop independent of `Time.timeScale`. diff --git a/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/fast-progressing-games.md b/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/fast-progressing-games.md index 281998603c..4539d3f8b7 100644 --- a/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/fast-progressing-games.md +++ b/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/fast-progressing-games.md @@ -28,6 +28,8 @@ Digit keys are `Digit0`-`Digit9` or `Numpad0`-`Numpad9` — bare `0`-`9` is reje `--resume-play` runs after the marker's arming is confirmed and before `--trigger` is dispatched: it resumes PlayMode only when PlayMode is actually paused, and reports what it did in `ResumePlayResult` (`WasPaused` / `Resumed` / `Error`; an abandoned wait adds `Repaused` / `RepauseError`). If the resume fails, the trigger is not dispatched and `TriggerResult.Error` says so. If the trigger itself is rejected before it runs, the wait is abandoned and the resume is undone: `Repaused: true` (or `RepauseError`) reports PlayMode being paused again, so gameplay cannot consume the preserved marker while the trigger value is being fixed. When the game reaches the line on its own after resuming (gravity, physics), omit `--trigger` and keep `--resume-play`. +One case skips the resume on purpose: if the marker had **already** hit by the time the wait confirmed its arming (a single-shot marker, or any marker whose mode does not wait for a new hit — including one just armed by the same `enable-pause-point --await` call), the wait settles on that recorded hit instantly, so resuming would restart the game and then report an old hit. The wait returns the recorded hit and reports `ResumePlayResult.Skipped` (with `Resumed: false` and no `Error`); a `--trigger` given alongside is not dispatched and says so in `TriggerResult.Error`. Play Mode is left as it is (still paused when that hit paused it) — clear or re-enable the marker and wait again to capture a new one. An already-hit `continuous`/`trace` marker that was armed earlier is unaffected: its wait needs a later hit, so it still resumes. + ## Do Not Use Time.timeScale = 0 Projects that read unscaled time keep advancing regardless, and the value silently persists into the next PlayMode session. Editor pause and `Step` freeze the entire player loop independent of `Time.timeScale`. diff --git a/cli/project-runner/internal/projectrunner/pause_point_resume_play.go b/cli/project-runner/internal/projectrunner/pause_point_resume_play.go index 00f1db771c..1731f59157 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_resume_play.go +++ b/cli/project-runner/internal/projectrunner/pause_point_resume_play.go @@ -22,6 +22,12 @@ type pausePointResumePlayResult struct { Resumed bool `json:"Resumed"` Error string `json:"Error,omitempty"` + // Skipped explains a deliberate no-resume that is not a failure, so the wait still reports the + // hit as a success. Why not Error: Error means the resume was attempted or refused and the wait + // treats it as broken (it skips --trigger), while this case is a correct outcome the caller + // still has to know about, because Unity stays paused by the hit that was already recorded. + Skipped string `json:"Skipped,omitempty"` + // Repaused reports that the wait put PlayMode back into pause after resuming it, which happens // when the wait is abandoned because the --trigger command was rejected before it ran. Reported // rather than kept internal: a caller that asked for a resume must be able to see that the diff --git a/cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go b/cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go index a63dcbcf6f..9498904d76 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go @@ -1,6 +1,7 @@ package projectrunner import ( + "bytes" "context" "fmt" "io" @@ -96,19 +97,7 @@ func TestWaitForPausePointResumesPlayBeforeTriggerWhenPaused(t *testing.T) { resumePlayModeForPausePoint = originalResume }() - queryPausePointStatus = func( - ctx context.Context, - connection unityipc.Connection, - id string, - ) (pausePointStatusResponse, error) { - return pausePointStatusResponse{ - Id: id, - Status: pausePointStatusHit, - IsHit: true, - HitCount: 1, - EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, - }, nil - } + queryPausePointStatus = pausePointEnabledThenHitStatusQuery() calls := make([]string, 0, 2) resumePlayModeForPausePoint = func( @@ -166,19 +155,7 @@ func TestWaitForPausePointSkipsPlayWhenAlreadyUnpaused(t *testing.T) { resumePlayModeForPausePoint = originalResume }() - queryPausePointStatus = func( - ctx context.Context, - connection unityipc.Connection, - id string, - ) (pausePointStatusResponse, error) { - return pausePointStatusResponse{ - Id: id, - Status: pausePointStatusHit, - IsHit: true, - HitCount: 1, - EditorState: pausePointEditorState{IsPlaying: true, IsPaused: false, CapturedAt: "PausePointHit"}, - }, nil - } + queryPausePointStatus = pausePointEnabledThenHitStatusQuery() triggerDispatched := false resumePlayModeForPausePoint = func( @@ -237,19 +214,7 @@ func TestWaitForPausePointSkipsTriggerWhenResumePlayFails(t *testing.T) { resumePlayModeForPausePoint = originalResume }() - queryPausePointStatus = func( - ctx context.Context, - connection unityipc.Connection, - id string, - ) (pausePointStatusResponse, error) { - return pausePointStatusResponse{ - Id: id, - Status: pausePointStatusHit, - IsHit: true, - HitCount: 1, - EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, - }, nil - } + queryPausePointStatus = pausePointEnabledThenHitStatusQuery() dispatchCalled := false resumePlayModeForPausePoint = func( @@ -441,19 +406,7 @@ func TestWaitForPausePointResumesWithoutTriggerWhenArmed(t *testing.T) { resumePlayModeForPausePoint = originalResume }() - queryPausePointStatus = func( - ctx context.Context, - connection unityipc.Connection, - id string, - ) (pausePointStatusResponse, error) { - return pausePointStatusResponse{ - Id: id, - Status: pausePointStatusHit, - IsHit: true, - HitCount: 1, - EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, - }, nil - } + queryPausePointStatus = pausePointEnabledThenHitStatusQuery() resumeCalled := false resumePlayModeForPausePoint = func( @@ -639,3 +592,397 @@ func TestWaitForPausePointUnarmedErrorIncludesClearedReasonWhenQuerySucceeded(t t.Fatalf("ResumePlayResult.Error mismatch: got %#v, want %q", resumeResult, wantError) } } + +// pausePointEnabledThenHitStatusQuery answers the arm-confirmation query with an armed-but-unhit +// marker and every later poll with a hit. Why not a fixed Hit response: a marker that was already +// hit when the wait started makes the wait settle on that old hit, which is exactly the case where +// no resume or trigger may run, so it cannot model a wait whose own side effects produce the hit. +func pausePointEnabledThenHitStatusQuery() func( + context.Context, + unityipc.Connection, + string, +) (pausePointStatusResponse, error) { + queryCount := 0 + return func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + queryCount++ + if queryCount == 1 { + return pausePointArmedStatusResponse(id), nil + } + return pausePointStatusResponse{ + Id: id, + Status: pausePointStatusHit, + IsHit: true, + HitCount: 1, + LastHitSequence: 1, + EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, + }, nil + } +} + +// pausePointAlreadyHitSingleShotStatusQuery answers the arm-confirmation query with a marker that +// was already hit before the wait started, and fails the test on any later query. Why fail: the +// recorded hit must be the one reported, so a wait that re-queries could return whatever the marker +// became afterwards (cleared, expired) instead — a single fixed Hit answer would hide that. +func pausePointAlreadyHitSingleShotStatusQuery(t *testing.T, mode string) func( + context.Context, + unityipc.Connection, + string, +) (pausePointStatusResponse, error) { + t.Helper() + + queryCount := 0 + return func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + queryCount++ + if queryCount > 1 { + t.Fatalf("expected the recorded hit to be returned without re-polling, got query %d", queryCount) + return pausePointStatusResponse{}, nil + } + return pausePointStatusResponse{ + Id: id, + Status: pausePointStatusHit, + IsHit: true, + HitCount: 1, + Mode: mode, + LastHitSequence: 1, + EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, + }, nil + } +} + +// Verifies --resume-play never resumes a single-shot marker that had already hit before the wait +// started: the wait settles on that old hit, so resuming would report a hit while Unity is running. +func TestWaitForPausePointSkipsResumeWhenMarkerAlreadyHitAtWaitStart(t *testing.T) { + originalQuery := queryPausePointStatus + originalResume := resumePlayModeForPausePoint + defer func() { + queryPausePointStatus = originalQuery + resumePlayModeForPausePoint = originalResume + }() + + queryPausePointStatus = pausePointAlreadyHitSingleShotStatusQuery(t, "single-shot") + resumePlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + ) pausePointResumePlayResult { + t.Fatal("expected no resume for a marker that had already hit at wait start") + return pausePointResumePlayResult{} + } + + response, state, triggerResult, resumeResult, hasNewHitBaseline, err := waitForPausePoint( + context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 1, + timeout: time.Second, + resumePlay: true, + }) + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateHit { + t.Fatalf("expected the recorded hit to still be returned, got %q", state) + } + if !response.IsHit || response.LastHitSequence != 1 { + t.Fatalf("expected the arm-confirmation hit to be the reported response, got %#v", response) + } + if hasNewHitBaseline { + t.Fatal("single-shot must not establish a new-hit baseline") + } + if triggerResult != nil { + t.Fatalf("expected nil TriggerResult when --trigger was not given, got %#v", triggerResult) + } + if resumeResult == nil { + t.Fatal("expected a ResumePlayResult explaining the skip, got nil") + } + if resumeResult.Resumed || resumeResult.WasPaused { + t.Fatalf("expected no resume to be reported, got %#v", resumeResult) + } + if resumeResult.Error != "" { + t.Fatalf("a deliberate skip must not be reported as a resume failure: %#v", resumeResult) + } + if resumeResult.Skipped != pausePointResumeSkippedForExistingHitMessage { + t.Fatalf("ResumePlayResult.Skipped mismatch: %#v", resumeResult) + } +} + +// Verifies a marker whose Mode is absent (older Unity package) is treated like single-shot, so its +// already-recorded hit also suppresses --resume-play and --trigger. +func TestWaitForPausePointSkipsSideEffectsWhenAlreadyHitMarkerHasNoMode(t *testing.T) { + originalQuery := queryPausePointStatus + originalResume := resumePlayModeForPausePoint + originalDispatch := dispatchPausePointTriggerCommand + defer func() { + queryPausePointStatus = originalQuery + resumePlayModeForPausePoint = originalResume + dispatchPausePointTriggerCommand = originalDispatch + }() + + queryPausePointStatus = pausePointAlreadyHitSingleShotStatusQuery(t, "") + resumePlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + ) pausePointResumePlayResult { + t.Fatal("expected no resume for a marker that had already hit at wait start") + return pausePointResumePlayResult{} + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + t.Fatal("expected no trigger dispatch for a marker that had already hit at wait start") + return 0 + } + + _, state, triggerResult, resumeResult, _, err := waitForPausePoint( + context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 1, + timeout: time.Second, + resumePlay: true, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Press"}, + }) + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateHit { + t.Fatalf("expected the recorded hit to still be returned, got %q", state) + } + if resumeResult == nil || resumeResult.Skipped != pausePointResumeSkippedForExistingHitMessage { + t.Fatalf("ResumePlayResult.Skipped mismatch: %#v", resumeResult) + } + if triggerResult == nil || triggerResult.Completed { + t.Fatalf("expected a skipped TriggerResult, got %#v", triggerResult) + } + if triggerResult.Error != pausePointTriggerSkippedForExistingHitError { + t.Fatalf("TriggerResult.Error mismatch: %#v", triggerResult) + } + if triggerResult.Command != "simulate-keyboard --action Press" { + t.Fatalf("TriggerResult.Command mismatch: %#v", triggerResult) + } +} + +// Verifies an already-hit continuous marker keeps resuming: its wait awaits a later hit, so the +// resume is what lets that next hit happen. +func TestWaitForPausePointStillResumesAlreadyHitContinuousMarker(t *testing.T) { + originalQuery := queryPausePointStatus + originalResume := resumePlayModeForPausePoint + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Millisecond + defer func() { + queryPausePointStatus = originalQuery + resumePlayModeForPausePoint = originalResume + pausePointStatusPoll = originalPoll + }() + + queryCount := 0 + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + queryCount++ + sequence := 5 + if queryCount >= 2 { + sequence = 6 + } + return pausePointStatusResponse{ + Id: id, + Status: pausePointStatusHit, + IsHit: true, + HitCount: sequence, + Mode: pausePointModeContinuous, + LastHitSequence: sequence, + EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, + }, nil + } + resumeCalled := false + resumePlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + ) pausePointResumePlayResult { + resumeCalled = true + return pausePointResumePlayResult{WasPaused: true, Resumed: true} + } + + _, state, _, resumeResult, hasNewHitBaseline, err := waitForPausePoint( + context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 1, + timeout: time.Second, + resumePlay: true, + }) + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateHit { + t.Fatalf("state mismatch: %q", state) + } + if !resumeCalled { + t.Fatal("expected an already-hit continuous marker to still be resumed") + } + if !hasNewHitBaseline { + t.Fatal("expected a new-hit baseline for an already-hit continuous marker") + } + if resumeResult == nil || !resumeResult.Resumed || resumeResult.Skipped != "" { + t.Fatalf("ResumePlayResult mismatch: %#v", resumeResult) + } +} + +// Verifies enable-pause-point --await --resume-play still resumes an armed marker that has not hit +// yet, which is the case the flag exists for: the enable froze nothing, so Play must be resumed for +// the line to be reached at all. +func TestWaitForPausePointStillResumesWhenMarkerJustEnabled(t *testing.T) { + originalQuery := queryPausePointStatus + originalResume := resumePlayModeForPausePoint + defer func() { + queryPausePointStatus = originalQuery + resumePlayModeForPausePoint = originalResume + }() + + queryPausePointStatus = pausePointEnabledThenHitStatusQuery() + resumeCalled := false + resumePlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + ) pausePointResumePlayResult { + resumeCalled = true + return pausePointResumePlayResult{WasPaused: true, Resumed: true} + } + + _, state, _, resumeResult, _, err := waitForPausePoint( + context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 1, + timeout: time.Second, + resumePlay: true, + markerJustEnabled: true, + }) + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateHit { + t.Fatalf("state mismatch: %q", state) + } + if !resumeCalled { + t.Fatal("expected enable --await --resume-play to resume an armed, unhit marker") + } + if resumeResult == nil || resumeResult.Skipped != "" { + t.Fatalf("ResumePlayResult mismatch: %#v", resumeResult) + } +} + +// Verifies enable-pause-point --await --resume-play skips the resume when the line was reached +// between the enable and the arm confirmation (a per-frame method): that hit already settles the +// wait, so resuming would restart the game and report it as a hit just observed. +func TestWaitForPausePointSkipsResumeWhenJustEnabledMarkerAlreadyHit(t *testing.T) { + originalQuery := queryPausePointStatus + originalResume := resumePlayModeForPausePoint + defer func() { + queryPausePointStatus = originalQuery + resumePlayModeForPausePoint = originalResume + }() + + queryPausePointStatus = pausePointAlreadyHitSingleShotStatusQuery(t, "single-shot") + resumePlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + ) pausePointResumePlayResult { + t.Fatal("expected no resume for a marker already hit at the arm confirmation") + return pausePointResumePlayResult{} + } + + response, state, _, resumeResult, _, err := waitForPausePoint( + context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 1, + timeout: time.Second, + resumePlay: true, + markerJustEnabled: true, + }) + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateHit { + t.Fatalf("state mismatch: %q", state) + } + if response.LastHitSequence != 1 || !response.IsHit { + t.Fatalf("expected the recorded hit to be returned, got %#v", response) + } + if resumeResult == nil || resumeResult.Skipped != pausePointResumeSkippedForExistingHitMessage { + t.Fatalf("ResumePlayResult.Skipped mismatch: %#v", resumeResult) + } +} + +// Verifies the public await-pause-point success payload carries the skip on +// ResumePlayResult.Skipped (and no Error), so a caller reading only the JSON learns that Unity is +// still paused by the hit it is looking at rather than running. +func TestRunWaitForPausePointCommandReportsResumeSkipForAlreadyHitMarker(t *testing.T) { + originalQuery := queryPausePointStatus + originalResume := resumePlayModeForPausePoint + originalFetch := fetchMatchingLogs + t.Cleanup(func() { + queryPausePointStatus = originalQuery + resumePlayModeForPausePoint = originalResume + fetchMatchingLogs = originalFetch + }) + + queryPausePointStatus = pausePointAlreadyHitSingleShotStatusQuery(t, "single-shot") + resumePlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + ) pausePointResumePlayResult { + t.Fatal("expected no resume for a marker that had already hit at wait start") + return pausePointResumePlayResult{} + } + fetchMatchingLogs = func( + ctx context.Context, + connection unityipc.Connection, + searchText string, + maxCount int, + ) (pausePointMatchingLogsResult, error) { + return pausePointMatchingLogsResult{SearchText: searchText}, nil + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := runWaitForPausePointCommand( + context.Background(), + unityipc.Connection{}, + []string{"--id", "jump", "--timeout-seconds", "1", "--resume-play"}, + "", + &stdout, + &stderr, + ) + if code != 0 { + t.Fatalf("expected hit success, got %d stdout=%s stderr=%s", code, stdout.String(), stderr.String()) + } + + payload := decodeJSONObject(t, stdout.Bytes()) + resumeResult, ok := payload["ResumePlayResult"].(map[string]any) + if !ok { + t.Fatalf("ResumePlayResult missing or wrong shape: %#v", payload) + } + if resumeResult["Resumed"] != false { + t.Fatalf("expected Resumed=false, got %#v", resumeResult) + } + if _, hasError := resumeResult["Error"]; hasError { + t.Fatalf("a deliberate skip must not be reported as an Error: %#v", resumeResult) + } + skipped, _ := resumeResult["Skipped"].(string) + if skipped != pausePointResumeSkippedForExistingHitMessage { + t.Fatalf("Skipped mismatch: %#v", resumeResult) + } +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_status_response_key_order_test.go b/cli/project-runner/internal/projectrunner/pause_point_status_response_key_order_test.go index ae33945957..8fad292429 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_status_response_key_order_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_status_response_key_order_test.go @@ -158,6 +158,7 @@ func fullyPopulatedPausePointStatusResponse() pausePointStatusResponse { WasPaused: true, Resumed: true, Error: "e", + Skipped: "s", Repaused: true, RepauseError: "e", }, diff --git a/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis_test.go b/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis_test.go index 178eb8a24a..2f066fa85a 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis_test.go @@ -31,19 +31,35 @@ func stubPausePointHit(t *testing.T, unityWarning string) { queryPausePointStatus = originalQuery }) + // The first query answers the wait's arm confirmation with an armed-but-unhit marker: a marker + // already hit at wait start suppresses --resume-play and --trigger, which is the opposite of the + // dispatched-trigger scenarios these stubs serve. Every later poll reports the hit. + queryCount := 0 queryPausePointStatus = func( ctx context.Context, connection unityipc.Connection, id string, ) (pausePointStatusResponse, error) { + queryCount++ + if queryCount == 1 { + return pausePointStatusResponse{ + Success: true, + Id: id, + Status: pausePointStatusEnabled, + IsEnabled: true, + Warning: unityWarning, + EditorState: pausePointEditorState{IsPlaying: true, CapturedAt: "Current"}, + }, nil + } return pausePointStatusResponse{ - Success: true, - Id: id, - Status: pausePointStatusHit, - IsHit: true, - HitCount: 1, - Warning: unityWarning, - EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, + Success: true, + Id: id, + Status: pausePointStatusHit, + IsHit: true, + HitCount: 1, + LastHitSequence: 1, + Warning: unityWarning, + EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, }, nil } } diff --git a/cli/project-runner/internal/projectrunner/pause_point_trigger_test.go b/cli/project-runner/internal/projectrunner/pause_point_trigger_test.go index 8d9c2b4dbb..758c01a7dd 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_trigger_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_trigger_test.go @@ -342,21 +342,10 @@ func TestWaitForPausePointJoinsTriggerResult(t *testing.T) { dispatchPausePointTriggerCommand = originalDispatch }() - queryPausePointStatus = func( - ctx context.Context, - connection unityipc.Connection, - id string, - ) (pausePointStatusResponse, error) { - return pausePointStatusResponse{ - Id: id, - Status: pausePointStatusHit, - IsHit: true, - HitCount: 1, - EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, - }, nil - } - t.Run("trigger completes before the wait settles", func(t *testing.T) { + // Re-stubbed per subtest: the stub's first answer is the wait's arm confirmation, so a shared + // instance would hand the second subtest an already-hit marker and suppress its trigger. + queryPausePointStatus = pausePointEnabledThenHitStatusQuery() dispatchPausePointTriggerCommand = func( ctx context.Context, connection unityipc.Connection, @@ -390,6 +379,7 @@ func TestWaitForPausePointJoinsTriggerResult(t *testing.T) { }) t.Run("trigger still running past the join grace window reports Completed=false", func(t *testing.T) { + queryPausePointStatus = pausePointEnabledThenHitStatusQuery() release := make(chan struct{}) t.Cleanup(func() { close(release) }) diff --git a/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go b/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go index a1ff63a31a..7159c6ac16 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go +++ b/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go @@ -47,18 +47,28 @@ func waitForPausePoint( connection unityipc.Connection, options waitForPausePointOptions, ) (pausePointStatusResponse, pausePointWaitState, *pausePointTriggerResult, *pausePointResumePlayResult, bool, error) { - triggerHandle, skippedTriggerResult, resumeResult, baselineSequence, hasBaseline, baselineDecided := startPausePointWaitSideEffects(ctx, connection, options) + sideEffects := startPausePointWaitSideEffects(ctx, connection, options) + + if sideEffects.immediateHit != nil { + // Returned without polling at all: the arm confirmation is the hit this wait reports, and a + // re-query could observe the marker cleared or expired in the meantime (or fail transiently + // and poll on to a timeout), turning "the recorded hit, as-is" into some other outcome. + return *sideEffects.immediateHit, pausePointWaitStateHit, sideEffects.skippedTriggerResult, + sideEffects.resumeResult, sideEffects.hasBaseline, nil + } + resumeResult := sideEffects.resumeResult response, state, polledTriggerResult, hasBaseline, err := waitForPausePointStatus( - ctx, connection, options, triggerHandle, baselineSequence, hasBaseline, baselineDecided) + ctx, connection, options, sideEffects.triggerHandle, + sideEffects.baselineSequence, sideEffects.hasBaseline, sideEffects.baselineDecided) if state == pausePointWaitStateTriggerFailed && resumeResult != nil && resumeResult.Resumed { repaused := repausePlayModeAfterAbandonedWait(ctx, connection, *resumeResult) resumeResult = &repaused } - if skippedTriggerResult != nil { - return response, state, skippedTriggerResult, resumeResult, hasBaseline, err + if sideEffects.skippedTriggerResult != nil { + return response, state, sideEffects.skippedTriggerResult, resumeResult, hasBaseline, err } // The trigger goroutine's buffered channel yields exactly one value, so a result already // received by the poll loop must be reused here instead of joining again — a second receive @@ -66,12 +76,29 @@ func waitForPausePoint( if polledTriggerResult != nil { return response, state, polledTriggerResult, resumeResult, hasBaseline, err } - if triggerHandle != nil { - return response, state, triggerHandle.join(), resumeResult, hasBaseline, err + if sideEffects.triggerHandle != nil { + return response, state, sideEffects.triggerHandle.join(), resumeResult, hasBaseline, err } return response, state, nil, resumeResult, hasBaseline, err } +// pausePointWaitSideEffects is what the pre-wait --resume-play / --trigger stage produced. Why a +// type: the stage has seven outputs and one of them (immediateHit) changes what the caller does next, +// which a positional tuple made impossible to read at the call site. +type pausePointWaitSideEffects struct { + triggerHandle *pausePointTriggerHandle + skippedTriggerResult *pausePointTriggerResult + resumeResult *pausePointResumePlayResult + baselineSequence int + hasBaseline bool + baselineDecided bool + + // immediateHit is the arm-confirmation response when it already settles the wait: an + // already-hit marker that awaits no later hit. It is the wait's answer, so the poll loop must + // not run at all — see waitForPausePoint. + immediateHit *pausePointStatusResponse +} + // startPausePointWaitSideEffects performs the pre-wait --resume-play / --trigger work, returning // either a live trigger handle or the fixed TriggerResult explaining why the trigger was skipped. // When it queries status to confirm arming, that response is the wait-start snapshot for the @@ -81,9 +108,9 @@ func startPausePointWaitSideEffects( ctx context.Context, connection unityipc.Connection, options waitForPausePointOptions, -) (*pausePointTriggerHandle, *pausePointTriggerResult, *pausePointResumePlayResult, int, bool, bool) { +) pausePointWaitSideEffects { if options.triggerCommand == "" && !options.resumePlay { - return nil, nil, nil, -1, false, false + return pausePointWaitSideEffects{baselineSequence: -1} } armResponse, armed, armQueryErr := queryPausePointArmStatus(ctx, connection, options.id) @@ -112,32 +139,79 @@ func startPausePointWaitSideEffects( // Why baselineDecided=false: a transient arm-query failure is also reported as not armed. // Leaving baseline undecided lets the first successful poll establish it, so a stale // continuous Hit cannot slip through as an immediate wait success. - return nil, skippedTriggerResult, resumeResult, -1, false, false + return pausePointWaitSideEffects{ + skippedTriggerResult: skippedTriggerResult, + resumeResult: resumeResult, + baselineSequence: -1, + } } baselineSequence, hasBaseline, baselineDecided := decidePausePointNewHitBaseline(armResponse, options.markerJustEnabled) + sideEffects := pausePointWaitSideEffects{ + baselineSequence: baselineSequence, + hasBaseline: hasBaseline, + baselineDecided: baselineDecided, + } + + if pausePointWaitSucceedsOnExistingHit(armResponse, hasBaseline) { + // This response already settles the wait, so resuming Play (or firing the trigger into the + // running game) would restart the game and then report the old hit as if the wait had just + // observed it — the response would claim a pause the Editor is no longer holding. + sideEffects.resumeResult, sideEffects.skippedTriggerResult = pausePointSideEffectsSkippedForExistingHit(options) + sideEffects.immediateHit = &armResponse + return sideEffects + } - var resumeResult *pausePointResumePlayResult if options.resumePlay { result := resumePlayModeForPausePoint(ctx, connection) - resumeResult = &result + sideEffects.resumeResult = &result if result.Error != "" { - if options.triggerCommand == "" { - return nil, nil, resumeResult, baselineSequence, hasBaseline, baselineDecided + if options.triggerCommand != "" { + sideEffects.skippedTriggerResult = &pausePointTriggerResult{ + Command: pausePointTriggerCommandString(options.triggerCommand, options.triggerArgs), + Error: "trigger was not dispatched: --resume-play failed to resume play mode", + } } - return nil, &pausePointTriggerResult{ - Command: pausePointTriggerCommandString(options.triggerCommand, options.triggerArgs), - Error: "trigger was not dispatched: --resume-play failed to resume play mode", - }, resumeResult, baselineSequence, hasBaseline, baselineDecided + return sideEffects } } if options.triggerCommand == "" { - return nil, nil, resumeResult, baselineSequence, hasBaseline, baselineDecided + return sideEffects } - handle := startPausePointTrigger(ctx, connection, options.startPath, options.triggerCommand, options.triggerArgs) - return handle, nil, resumeResult, baselineSequence, hasBaseline, baselineDecided + sideEffects.triggerHandle = startPausePointTrigger( + ctx, connection, options.startPath, options.triggerCommand, options.triggerArgs) + return sideEffects +} + +// pausePointWaitSucceedsOnExistingHit reports that the arm-confirmation response is already the +// wait's answer, so no pre-wait side effect may run: the marker is hit and no new-hit baseline makes +// the wait look past that hit. hasBaseline is the value decidePausePointNewHitBaseline just +// returned, which covers enable --await too — a marker enabled by this very command gets no +// baseline, so a hit its arm query already observes is likewise returned without polling. +func pausePointWaitSucceedsOnExistingHit(response pausePointStatusResponse, hasBaseline bool) bool { + return response.Status == pausePointStatusHit && !hasBaseline +} + +// pausePointSideEffectsSkippedForExistingHit builds the reported outcomes for the flags that were +// passed but deliberately not performed. Both are reported rather than omitted for the same reason as +// on the not-armed path: a silently missing result is indistinguishable from one that never reported. +func pausePointSideEffectsSkippedForExistingHit( + options waitForPausePointOptions, +) (*pausePointResumePlayResult, *pausePointTriggerResult) { + var resumeResult *pausePointResumePlayResult + if options.resumePlay { + resumeResult = &pausePointResumePlayResult{Skipped: pausePointResumeSkippedForExistingHitMessage} + } + var triggerResult *pausePointTriggerResult + if options.triggerCommand != "" { + triggerResult = &pausePointTriggerResult{ + Command: pausePointTriggerCommandString(options.triggerCommand, options.triggerArgs), + Error: pausePointTriggerSkippedForExistingHitError, + } + } + return resumeResult, triggerResult } // queryPausePointArmStatus reports whether the marker is enabled or already hit. A query failure is @@ -159,6 +233,13 @@ func queryPausePointArmStatus( const ( pausePointResumeNotArmedAtWaitStartError = "resume was not dispatched: the marker could not be confirmed armed at wait start" pausePointTriggerNotArmedAtWaitStartError = "trigger was not dispatched: the marker could not be confirmed armed at wait start" + + pausePointResumeSkippedForExistingHitMessage = "resume was skipped: the marker had already hit " + + "before this wait started, so the recorded hit is returned as-is and Play Mode is left as it is; " + + "clear or re-enable the marker and wait again to capture a new hit" + pausePointTriggerSkippedForExistingHitError = "trigger was not dispatched: the marker had already " + + "hit before this wait started, so the recorded hit is returned as-is; clear or re-enable the " + + "marker and wait again to capture a new hit" ) func pausePointUnarmedAtWaitStartSuffix(response pausePointStatusResponse, queryErr error) string {