Flaky tests: sync client recreate tests race the fire-and-forget recreate thread

[berndverst]

1. What is the issue?

Two tests in tests/durabletask/test_client.py are intermittently flaky:

• test_sync_client_close_closes_all_retired_sdk_channels_immediately
• test_sync_client_recreate_cooldown_prevents_immediate_repeated_recreation

Both synchronize with the synchronous client's fire-and-forget channel-recreate thread by waiting on TaskHubGrpcClient._recreate_done_event. That event is set from inside the recreate thread, so it becomes signalled while the thread is still alive. A subsequent trigger therefore sees is_alive() as True, silently drops the recreate and skips the clear(), leaving the event set from the previous run. The next wait() then returns immediately without a recreate having happened, and the assertions that follow fail.

This is a test-synchronization defect only. The production single-flight behaviour is correct and intentional — dropping a trigger while a recreate is genuinely in flight is the documented design.

2. What is the impact?

Intermittent, hard-to-attribute CI and local test failures that are easy to dismiss as infrastructure noise.

• Both tests pass in isolation and fail only under load, so they waste triage time and erode trust in the suite.
• Because the failure surfaces as a different assertion each time — a missing threading.Timer, an unclosed channel, a non-empty _retired_channels, or an unexpected get_grpc_channel call count — it does not look like one bug, which makes it easy to misdiagnose repeatedly.
• The window widens under CPU contention, so this gets worse on busy CI runners and on parallel test runs.
• It creates a real risk of masking a genuine regression: a future change that legitimately breaks channel recreation would produce a failure indistinguishable from this flake.

3. Details

The recreate thread signals completion in its finally block, as its final statement:

durabletask-python/durabletask/client.py

Lines 536 to 542 in 7223d24

	def _run_recreate(self) -> None:
	try:
	self._maybe_recreate_channel()
	except Exception:
	self._logger.exception("Channel recreate failed")
	finally:
	self._recreate_done_event.set()

threading.Event.set() runs inside the thread, so at the moment the event is signalled the thread has not yet finished teardown and is_alive() still returns True.

The scheduler gates on exactly that liveness check, and — critically — the clear() sits after the early return:

durabletask-python/durabletask/client.py

Lines 518 to 532 in 7223d24

	try:
	if self._closing:
	return
	with self._recreate_thread_lock:
	existing = self._recreate_thread
	if existing is not None and existing.is_alive():
	return
	self._recreate_done_event.clear()
	thread = threading.Thread(
	target=self._run_recreate,
	name="durabletask-client-recreate",
	daemon=True,
	)
	self._recreate_thread = thread
	thread.start()

The failing interleaving is:

1. Thread T1 reaches its finally and calls set(). T1 is still alive.
2. The test's _recreate_done_event.wait(timeout=5.0) returns True.
3. The next RPC calls _schedule_recreate(). existing.is_alive() is still True, so the trigger is dropped at the early return — and clear() is never reached.
4. The test's next _recreate_done_event.wait(timeout=5.0) returns True immediately, off the stale set from step 1.
5. Assertions about the second recreate fail, because no second recreate ever ran.

The two affected tests chain multiple recreates through this pattern — two in the first, three in the second:

durabletask-python/tests/durabletask/test_client.py

Lines 667 to 675 in 7223d24

	with pytest.raises(FakeRpcError):
	client.get_orchestration_state("abc")
	# Wait for the first fire-and-forget recreate to complete so the
	# single-flight guard in _schedule_recreate does not drop the second
	# trigger.
	assert client._recreate_done_event.wait(timeout=5.0)
	with pytest.raises(FakeRpcError):
	client.get_orchestration_state("abc")
	assert client._recreate_done_event.wait(timeout=5.0)

durabletask-python/tests/durabletask/test_client.py

Lines 911 to 930 in 7223d24

	with pytest.raises(FakeRpcError):
	client.get_orchestration_state("abc")
	# Wait for the fire-and-forget recreate to complete before asserting
	# the channel was swapped.
	assert client._recreate_done_event.wait(timeout=5.0)
	assert client._channel is second_channel
	assert mock_get_channel.call_count == 2
	with pytest.raises(FakeRpcError):
	client.get_orchestration_state("abc")
	# Cooldown should fire-and-forget but exit without recreating; wait
	# for the no-op recreate to complete so the assertion is deterministic.
	assert client._recreate_done_event.wait(timeout=5.0)
	assert client._channel is second_channel
	assert mock_get_channel.call_count == 2
	with pytest.raises(FakeRpcError):
	client.get_orchestration_state("abc")
	assert client._recreate_done_event.wait(timeout=5.0)
	assert client._channel is third_channel

The first test's own comment states precisely the hazard it is trying to avoid — "so the single-flight guard in _schedule_recreate does not drop the second trigger" — but Event.wait() does not actually provide that guarantee.

Note

The asynchronous client is not affected. It gates on asyncio.Task.done() rather than thread liveness, and under a single-threaded event loop the task is already complete by the time a waiter resumes. Its docstring calls this out explicitly:

durabletask-python/durabletask/client.py

Lines 1058 to 1074 in 7223d24

	def _schedule_recreate(self) -> None:
	"""Schedule a fire-and-forget channel recreate on the event loop.
	Called from the resiliency interceptor when a unary RPC fails with a
	transport error. Single-flight: if ``_recreate_task`` is still
	pending, the trigger is dropped — the in-flight recreate will pick up
	the latest channel state on completion. asyncio is single-threaded
	so ``done()`` is race-free; no extra lock is required.
	"""
	try:
	if self._closing:
	return
	existing = self._recreate_task
	if existing is not None and not existing.done():
	return
	self._recreate_done_event.clear()
	self._recreate_task = asyncio.create_task(self._run_recreate())

4. Proposed solution

Fix the tests, not the production code. Waiting on the event establishes that the recreate finished its work, but the single-flight guard keys on thread liveness, so the tests must wait on the same condition the guard reads. Joining the thread after the event wait closes the gap:

assert client._recreate_done_event.wait(timeout=5.0)
recreate_thread = client._recreate_thread
if recreate_thread is not None:
    recreate_thread.join(timeout=5.0)

This makes is_alive() deterministically False before the next trigger, so the following _schedule_recreate() reaches clear() and the subsequent wait() reflects the new recreate rather than a stale signal.

A small test helper — something like _await_recreate(client) — applied at each of the five wait sites would keep this consistent and prevent the pattern being reintroduced.

Alternatives considered and rejected:

• Moving set() out of the thread. A thread cannot signal its own death from within itself, so this would require a supervisor thread or a Thread.join() in the scheduler — real complexity and a blocking call on the RPC path, to fix a test-only problem.
• Clearing the event before the early return in _schedule_recreate. This would make a dropped trigger indistinguishable from a pending one for any waiter, converting a test flake into a potential production hang.
• Retry/sleep in the tests. Masks the race rather than removing it, and reintroduces timing sensitivity.