Skip to content

test: deflake test_encode_and_write_as_completed_cancels_stray_writes_on_failure - #4409

Merged
d-v-b merged 2 commits into
zarr-developers:mainfrom
d-v-b:test/deflake-stray-writes-cancel
Sep 25, 2026
Merged

d-v-b merged 2 commits into
zarr-developers:mainfrom
d-v-b:test/deflake-stray-writes-cancel

Conversation

@d-v-b

@d-v-b d-v-b commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

AI PR that makes a flaky test a bit more reliable.

🤖 AI text below 🤖

Summary

tests/test_fused_pipeline.py::test_encode_and_write_as_completed_cancels_stray_writes_on_failure (added in #4205) fails intermittently on assert write_started.is_set(). It has failed five times in CI since 2026-07-30:

Date Run Job
2026-07-31 30624113397 windows, py3.14, optional
2026-09-09 34382421806 (main) macos, py3.14, optional
2026-09-12 34707555000 macos, py3.14, optional
2026-09-20 35536950892 macos, py3.14, optional
2026-09-25 36104239476 (#4407) ubuntu, py3.13, minimal

Cause

The test assumes the slow write is always in flight when the failing write raises. It isn't when the failing chunk's encode lands first (encodes run in the thread pool, so order varies):

  1. The failing write task is created and raises while _encode_and_write_as_completed is still awaiting the other encode.
  2. The slow encode lands, and the slow write task is created, but it hasn't run a step yet.
  3. asyncio.gather runs the done-callbacks of already-finished children synchronously (the "finished eagerly" optimization). With return_exceptions=False, the failed child makes the outer future fail immediately.
  4. await gather(...) therefore doesn't suspend. The finally cancels the slow write before its first step, so write_started is never set.

Instrumented trace of a failing run:

task created 1          # failing write
task 1 first step
fail.set raise
task created 0          # slow write, not yet stepped
gather called: done=True, done=False
gather outer done=True  # already failed; await won't suspend
drain: Task(done=True), Task(done=False), ...

The pipeline behaves correctly here: no write is left running, since it's cancelled before it starts. The test is what's wrong: it only means to check cancellation of an in-flight write.

Fix

Test-only. The failing write now waits (bounded, 5 s) until the slow write has started before raising, so every run exercises the in-flight case. The docstring explains why.

Verification

  • Reproduced with pytest-repeat and 16 parallel workers: 8/400 failed on py3.13, 3/400 on py3.14.
  • With the fix: 2000/2000 pass on both py3.13 and py3.14.
  • Still catches the bug it guards: with the _cancel_and_drain call in _encode_and_write_as_completed replaced by pass, the test fails on assert not write_finished.
  • Full file: tests/test_fused_pipeline.py passes.

No changelog entry, since this is a test-only change.

🤖 Generated with Claude Code

…_on_failure

The test assumed the slow write is always in flight when the failing write
raises. When the failing chunk's encode lands first, its write raises
while the loop still awaits the other encode; `gather` then runs the done
callback of that already-failed child eagerly and returns an outer future
that has already failed, so `await gather(...)` does not suspend, and the
`finally` cancels the slow write before its first step. `write_started`
is never set.

That outcome is correct (no stray write), but it is not what the test
checks. Make the failing write wait until the slow write has started
before raising, so the in-flight case is exercised every time.

Reproduced at 8/400 (py3.13) and 3/400 (py3.14) with 16 parallel
workers; with the fix, 2000/2000 on both. It had failed in CI five times
since the test landed, on Linux, macOS and Windows.

Assisted-by: ClaudeCode:claude-opus-5-5
@github-actions github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label Sep 25, 2026
@d-v-b
d-v-b marked this pull request as ready for review September 25, 2026 07:39
@d-v-b
d-v-b merged commit bf14d68 into zarr-developers:main Sep 25, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs release notes Automatically applied to PRs which haven't added release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant