Tests: fix flaky sync client recreate tests by joining the recreate thread

[berndverst]

Fixes #212

Root cause

TaskHubGrpcClient._run_recreate signals completion from its finally block —
durabletask/client.py L536-L542:

finally:
    self._recreate_done_event.set()

Event.set() runs inside the recreate thread, so the event is signalled while
that thread is still alive.

_schedule_recreate single-flights on exactly that liveness check, and the
early return sits before the clear() —
L518-L532:

existing = self._recreate_thread
if existing is not None and existing.is_alive():
    return                       # <-- trigger dropped, clear() never reached
self._recreate_done_event.clear()

The interleaving

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, leaving the event set from step 1.
4. The test's next wait() returns immediately off that stale signal.
5. Assertions about the second recreate fail, because no second recreate ran.

Because the failure surfaces as a different assertion each time (a missing
threading.Timer, an unclosed channel, a non-empty _retired_channels, an
unexpected get_grpc_channel count) it does not look like one bug, which is why
it has been misdiagnosed as infra noise. The window widens under CPU contention,
so it gets worse on busy runners and parallel runs.

Why this fixes the tests, not production

The production single-flight behaviour is correct and intentional — dropping
a trigger while a recreate is genuinely in flight is the documented design. This
is purely a test-synchronization defect: waiting on the event establishes that
the recreate finished its work, but the guard keys on thread liveness, so
the tests must wait on the same condition the guard actually reads.

Alternatives considered and rejected (see #212):

• Moving set() out of the thread — a thread cannot signal its own death
  from within itself, so this needs a supervisor thread or a blocking
  Thread.join() on the RPC path. Real complexity and a blocking call on a hot
  path, to fix a test-only problem.
• Clearing the event before the early return — 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 and reintroduces timing
  sensitivity.

The change

One file, tests/durabletask/test_client.py (+31 / −8). A module-level helper
waits on the event and joins the thread, so is_alive() is deterministically
False before the next trigger:

def await_sync_recreate(client: TaskHubGrpcClient, timeout: float = 5.0) -> None:
    assert client._recreate_done_event.wait(timeout=timeout)
    recreate_thread = client._recreate_thread
    if recreate_thread is not None:
        recreate_thread.join(timeout=timeout)
        assert not recreate_thread.is_alive()

Applied at all five wait sites across the two affected tests:

• test_sync_client_close_closes_all_retired_sdk_channels_immediately (2 sites)
• test_sync_client_recreate_cooldown_prevents_immediate_repeated_recreation (3 sites)

The install_resilient_test_stubs docstring, which previously pointed
contributors at the insufficient _recreate_done_event-only pattern, is
corrected to point at the helper so the bug is not reintroduced.

Note

The async 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 at
durabletask/client.py L1058-L1074
calls this out explicitly. Its wait site is left unchanged.

No production code is touched, and no changelog entry is added — per the repo's
contributor instructions, test-only changes with no user-visible behaviour
change are explicitly excluded from changelogs.

RED / GREEN evidence

The race was forced deterministically rather than waited for. A throwaway
harness (never committed, no repo file mutated) replaced
TaskHubGrpcClient._run_recreate at runtime on the class object with an
otherwise-identical body that lingers after set(), reproducing exactly the
step-1 state — event signalled, thread still alive:

finally:
    self._recreate_done_event.set()
    time.sleep(DELAY_SECONDS)   # thread still alive after signalling

RED — unmodified tests at 58f99b2, forced interleaving: both tests fail.

FAILED tests/durabletask/test_client.py::test_sync_client_close_closes_all_retired_sdk_channels_immediately
FAILED tests/durabletask/test_client.py::test_sync_client_recreate_cooldown_prevents_immediate_repeated_recreation
2 failed in 0.51s

with, respectively:

E  AssertionError: Expected 'cancel' to be called once. Called 0 times.

E  AssertionError: assert <MagicMock name='second-channel'> is <MagicMock name='third-channel'>
E   +  where <MagicMock name='second-channel'> = <TaskHubGrpcClient object>._channel

Two different assertion messages from one underlying bug — matching the
misdiagnosis pattern described in the issue.

GREEN — same forced interleaving, with this fix: both pass, and stay passing
as the forced thread lifetime is scaled up 7.5×, confirming the join makes it
delay-independent rather than merely faster:

forced post-set() thread lifetime	result
0.10 s	2 passed
0.75 s	2 passed

GREEN loop: 50 consecutive runs of the two tests under the forced
interleaving (0.05 s) — 0 failures.

Full core suite: pytest tests/durabletask -q --ignore-glob="*_e2e.py" —
684 passed, 7 skipped, byte-for-byte identical to the pre-change baseline on
main (*_e2e.py requires a sidecar and is excluded).

Validation

check	result
pytest tests/durabletask -q --ignore-glob="*_e2e.py"	684 passed, 7 skipped (matches baseline)
flake8 tests/durabletask/test_client.py	exit 0
pyright (strict) on the changed file	149 → 146 pre-existing errors; no new diagnostics introduced (the helper collapses five inlined private accesses into two). tests/ is outside the configured include scope.
forced-interleaving loop, 50 iterations	0 failures
files changed	tests/durabletask/test_client.py only

[Copilot]

Pull request overview

This PR fixes flakiness in the sync client resiliency tests by synchronizing on the condition the production single-flight logic actually uses (recreate thread liveness), not just the internal “done” event.

Changes:

• Introduces a shared await_sync_recreate() helper that waits for _recreate_done_event and then join()s the recreate thread to ensure it has fully exited.
• Updates five wait sites across the two affected sync-client tests to use the helper instead of waiting on _recreate_done_event alone.
• Updates install_resilient_test_stubs() documentation to steer contributors toward the correct synchronization pattern.