Skip to content

Conversation

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Oct 6, 2025

Motivation:

This test occasionally fails in our ongoing continuous CI solution. Flaky tests are bad, and we should aim not to have them, so let's fix it.

My diagnosis of the most likely race here (which leads to alreadyClosed) being thrown somewhere from this code is that the client channel got closed unexpectedly. This would happen if an unusual series of events occurs:

  1. The client connection succeeds and the channel starts up
  2. The server close then goes through and the server channel is closed before it actually accepts the connection.
  3. This causes the client connection to be reset.
  4. The client channel observes this and handles that close.
  5. We then close the client from the outside.

Modifications:

Add a ConditionLock we can use to wait for a connection to be established.
Make the use of this ConditionLock resilient to weird test behaviour by always using timeouts for blocking operations.

Result:

Less flaky tests.

Motivation:

This test occasionally fails in our ongoing continuous CI solution.
Flaky tests are bad, and we should aim not to have them, so let's
fix it.

My diagnosis of the most likely race here (which leads to `alreadyClosed`)
being thrown _somewhere_ from this code is that the client channel got
closed unexpectedly. This would happen if an unusual series of events
occurs:

1. The client connection succeeds and the channel starts up
2. The server close then goes through and the server channel is closed
    _before_ it actually accepts the connection.
3. This causes the client connection to be reset.
4. The client channel observes this and handles that close.
5. We then close the client from the outside.

Modifications:

Add a ConditionLock we can use to wait for a connection to be
established.
Make the use of this ConditionLock resilient to weird test behaviour
by always using timeouts for blocking operations.

Result:

Less flaky tests.
@Lukasa Lukasa added the semver/none No version bump required. label Oct 6, 2025
@Lukasa Lukasa merged commit 97c3f28 into apple:main Oct 6, 2025
58 of 62 checks passed
@Lukasa Lukasa deleted the cb-flaky-test-again-yo branch October 6, 2025 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver/none No version bump required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants