temporalio / temporalio/s2s-proxy

Flaky TestMultiClientUpdateStateAsyncClient hangs to 5m timeout (require in goroutine deadlocks on close race)

Open
#247 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Go
Stars
16
Forks
9
Avg merge
6d 23h
Merged PRs (30d)
12

Description

Summary

TestMultiClientUpdateStateAsyncClient (in proxy/test/multi_client_conn_test.go) is flaky: on a lost race it hangs until the package-wide -timeout=5m fires and the whole proxy/test binary panics, instead of failing cleanly. A flaky failure is being reported as a 5-minute CI timeout.

panic: test timed out after 5m0s
	running tests:
		TestMultiClientUpdateStateAsyncClient (4m36s)
...
FAIL	github.com/temporalio/s2s-proxy/proxy/test	300.224s
make: *** [Makefile:96: test] Error 1

The Failed to get replication tasks log lines are a red herring

Failed to get replication tasks ... "xdc-source-cluster": "cluster-b-raaarmod" ... "error": "connection error: ..."

These come from leaked replication-task-processor goroutines of earlier integration tests in the same proxy/test binary (the failover/replication suites start real clusters; after teardown their background workers keep retrying against the torn-down cluster-b-…). TestMultiClientUpdateStateAsyncClient is a yamux/gRPC unit test with no cluster-b, so these lines only pollute the log — they are not the cause.

Root cause: deadlock on <-responsesCh

The worker goroutine only sends its result after all 1000 DescribeCluster calls complete:

go func() {
    responses := make(map[string]int, 10)
    for range 1000 {
        _ = wedge.Acquire(context.Background(), 1)
        wg.Done()
        resp, err := client.DescribeCluster(context.Background(), &adminservice.DescribeClusterRequest{})
        require.NoError(t, err)   // <-- called from a NON-test goroutine
        responses[resp.ClusterName]++
    }
    responsesCh <- responses      // <-- only reached if the loop finishes
}()
...
responses := <-responsesCh        // <-- test goroutine blocks here forever

require.NoError(t, err) is invoked from a spawned goroutine. On error it calls t.FailNow()runtime.Goexit(), which terminates only that goroutine — it never reaches responsesCh <- responses. The test goroutine then blocks on <-responsesCh indefinitely, so the package hits -timeout=5m and panics, pointing at whichever test is running (this one). A clean assertion failure is turned into a 5-minute hang (the classic "testify require in a non-test goroutine" pitfall).

Why it's flaky

After UpdateState({mux1,mux2,mux3}), then CloseMux(0) + CloseMux(2), the test asserts round_robin sends 0 requests to the dead mux 2 (responses["adminService on mux 2"] == 0). There is a race between CloseMux(2) and the round_robin LB marking that subconn down. If a request is dispatched to mux 2 in that window, DescribeCluster returns a connection error → require.NoErrorGoexit → deadlock → 5-minute timeout. When the LB wins the race, the test passes.

Suggested fixes

  1. Make failures fail cleanly (removes the hang): do not use require/FailNow inside the worker goroutine. Capture the error and surface it to the test goroutine (e.g. send err over a channel and select on result-or-error), then assert in the test body. A bad route then produces an immediate, legible failure instead of a 5-minute timeout.
  2. Address the underlying flake: if the LB should never dispatch to a just-closed mux, wait for the connection-state update to propagate before releasing the second batch (wedge.Release(500)), or tolerate/retry Unavailable for the closed subconn. If routing-to-closed is expected to error, the mux 2 == 0 assertion needs rethinking.
  3. Reduce cross-test log bleed: the leaked replication-task goroutines indicate earlier suites do not fully stop their workers on teardown; cleaner teardown (or isolating these unit tests in a separate package) would keep failure logs readable.

Environment

  • Package: github.com/temporalio/s2s-proxy/proxy/test
  • Test: TestMultiClientUpdateStateAsyncClient
  • grpc@v1.80.0, test timeout -timeout=5m (Makefile TEST_ARG)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in proxy/test/multi_client_conn_test.go at TestMultiClientUpdateStateAsyncClient, especially the worker goroutine's require.NoError call and responsesCh receive. Run the targeted test and inspect the close/state-update sequence around CloseMux and wedge.Release. Done means a routing error fails cleanly in the test goroutine instead of hanging until the five-minute timeout, with the flaky closed-mux behavior addressed or explicitly handled.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.