From df6c61156b4c509f79db563540d636cda8852e50 Mon Sep 17 00:00:00 2001 From: hatayama Date: Fri, 12 Jun 2026 10:17:34 +0900 Subject: [PATCH] Widen timing margins in flaky connection retry and IPC client tests Four tests relied on sub-100ms real-time windows that a loaded CI machine cannot honor; the busy-after-retry-window test already failed in CI with a dial i/o timeout surfacing instead of the busy RPC error. - Raise the busy test retry window from 30ms to 500ms so at least one busy response always lands inside the window, and stub the Unity process probe to block until the context is done: the dial deadline is a separate timer that can fire microseconds before retryContext reports expiry, so an instant probe would skip the busy-masking guard - Widen the accepted-ack tests from a 20ms window vs 60ms server delay to 200ms vs 600ms, keeping the 3x ratio that defines their semantics - Widen the IPC client accept timeout test from 50ms vs 150ms to 250ms vs 750ms for the same reason Verified with -count=30 -race stress runs on both packages and a full scripts/check-go-cli.sh pass. --- cli/internal/cli/connection_retry_test.go | 31 +++++++++++++++++++---- cli/internal/unityipc/client_test.go | 7 +++-- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/cli/internal/cli/connection_retry_test.go b/cli/internal/cli/connection_retry_test.go index 8aaa064042..a424a77f21 100644 --- a/cli/internal/cli/connection_retry_test.go +++ b/cli/internal/cli/connection_retry_test.go @@ -236,7 +236,10 @@ func TestSendWithTransientConnectionRetryDoesNotCancelAcceptedRequestAtRetryTime } originalTimeout := serverConnectionRetryTimeout - serverConnectionRetryTimeout = 20 * time.Millisecond + // The retry window must be wide enough that the dial plus accepted ack always + // completes inside it even on a loaded CI machine, while the server delay stays + // well past the window so the timeout reliably fires mid-request. + serverConnectionRetryTimeout = 200 * time.Millisecond t.Cleanup(func() { serverConnectionRetryTimeout = originalTimeout }) @@ -271,7 +274,7 @@ func TestSendWithTransientConnectionRetryDoesNotCancelAcceptedRequestAtRetryTime return } - time.Sleep(60 * time.Millisecond) + time.Sleep(600 * time.Millisecond) final := []byte(`{"jsonrpc":"2.0","result":{"ok":true},"id":1}`) if err := unityipc.Write(conn, final); err != nil { @@ -315,7 +318,10 @@ func TestSendWithTransientConnectionRetryKeepsRetryTimeoutBeforeAcceptedAck(t *t } originalTimeout := serverConnectionRetryTimeout - serverConnectionRetryTimeout = 20 * time.Millisecond + // The retry window must be wide enough that the dial and request write always + // complete inside it even on a loaded CI machine, while the server delay stays + // well past the window so the pre-accept timeout reliably fires first. + serverConnectionRetryTimeout = 200 * time.Millisecond t.Cleanup(func() { serverConnectionRetryTimeout = originalTimeout }) @@ -344,7 +350,7 @@ func TestSendWithTransientConnectionRetryKeepsRetryTimeoutBeforeAcceptedAck(t *t return } - time.Sleep(60 * time.Millisecond) + time.Sleep(600 * time.Millisecond) final := []byte(`{"jsonrpc":"2.0","result":{"ok":true},"id":1}`) if err := unityipc.Write(conn, final); err != nil { @@ -446,11 +452,26 @@ func TestSendWithTransientConnectionRetryReturnsBusyAfterRetryWindow(t *testing. t.Skip("TCP endpoint injection is only used by this non-Windows client test") } + originalFinder := findRunningUnityProcessForConnectionRetry originalTimeout := serverConnectionRetryTimeout originalPoll := serverConnectionRetryPoll - serverConnectionRetryTimeout = 30 * time.Millisecond + // The busy assertion only holds once at least one busy response lands inside the + // retry window. A narrow window can expire before the first dial completes on a + // loaded CI machine, surfacing a dial timeout instead of the busy RPC error. + serverConnectionRetryTimeout = 500 * time.Millisecond serverConnectionRetryPoll = 5 * time.Millisecond + // A dial cut short by the expiring window probes for a running Unity process. + // The dial deadline is a separate timer that can fire microseconds before + // retryContext reports expiry, so an instant probe would reach the busy-masking + // guard while retryContext.Err() is still nil and surface the dial error instead. + // Block until the context is done, like a real OS process scan that always + // outlasts those microseconds, so the busy guard sees the expired context. + findRunningUnityProcessForConnectionRetry = func(ctx context.Context, projectRoot string) (*unityProcess, error) { + <-ctx.Done() + return nil, ctx.Err() + } t.Cleanup(func() { + findRunningUnityProcessForConnectionRetry = originalFinder serverConnectionRetryTimeout = originalTimeout serverConnectionRetryPoll = originalPoll }) diff --git a/cli/internal/unityipc/client_test.go b/cli/internal/unityipc/client_test.go index db8d9cdb25..424a11bf62 100644 --- a/cli/internal/unityipc/client_test.go +++ b/cli/internal/unityipc/client_test.go @@ -219,7 +219,7 @@ func TestSendWithProgressOutcomeWaitsForFinalResponseAfterDispatchAckWithoutAcce return } - time.Sleep(150 * time.Millisecond) + time.Sleep(750 * time.Millisecond) final := []byte(`{"jsonrpc":"2.0","result":{"ok":true},"id":1}`) if err := Write(conn, final); err != nil { @@ -236,7 +236,10 @@ func TestSendWithProgressOutcomeWaitsForFinalResponseAfterDispatchAckWithoutAcce ProjectRoot: "/tmp/MyProject", } client := NewClient(connection, "3.0.0-beta.6") - client.acceptTimeout = 50 * time.Millisecond + // The accept timeout must be wide enough that the accepted ack always arrives + // inside it even on a loaded CI machine, while the server delays the final + // response well past it to prove accepted requests outlive the accept timeout. + client.acceptTimeout = 250 * time.Millisecond outcome, err := client.SendWithProgressOutcome(context.Background(), "execute-dynamic-code", map[string]any{}, nil) if err != nil {