Skip to content

test(codec_pipeline): pin the pool size in the fork regression test - #4504

Open
d-v-b wants to merge 5 commits into
zarr-developers:mainfrom
d-v-b:test/fork-codec-pool-followup
Open

d-v-b wants to merge 5 commits into
zarr-developers:mainfrom
d-v-b:test/fork-codec-pool-followup

Conversation

@d-v-b

@d-v-b d-v-b commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Little follow-up to #4478 that hardens the tests

🤖 AI text below 🤖

This hardens the regression test for #4478 added in #4483. It pins codec_pipeline.max_workers so the test exercises the bug on any host, and moves the test next to the other FusedCodecPipeline pool tests.

  • Pinned pool size. The test now uses the shared _FUSED_POOL_CONFIG (max_workers=4). With the default, max_workers resolves to os.cpu_count(); on a 1-CPU host it is 1, the parent never creates the pool, and the test passes whether or not the fix is present. The test also asserts that the parent created the pool before forking.
  • Runs on macOS. The darwin skip is gone, so only Windows is skipped. The fork start method is available on macOS.
  • Location. Moved from tests/test_array.py to tests/test_fused_pipeline.py, after the other pool tests.

The agent checked that the test passes with the #4483 fix and fails with a 30 s TimeoutError when the src/ change is reverted. It did this on macOS (Python 3.12; 20 of 20 passing runs with the fix) and on Linux (Python 3.12 and 3.14). This is a test-only change, so there is no changelog fragment.

Stack notes

This branch is built on the head of #4483 (8d225dac2), so until #4483 merges the diff here includes its changes. After #4483 is squash-merged, the agent will rebase this branch onto main so that only the commit test(codec_pipeline): pin pool size in the fork regression test remains.

🤖 Generated with Claude Code

barlowa124 and others added 4 commits October 6, 2026 18:11
A forked child inherits codec_pipeline._pool pointing at threads that
do not exist in the child, plus the parent's idle-semaphore permits.
Submissions up to the permit count are consumed without spawning a
thread, so small FusedCodecPipeline reads hang forever.

Add an after_in_child hook that drops the inherited pool, size, and
lock. The lock is recreated because it may have been held by a thread
that no longer exists in the child.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Move the zarr-developers#4478 regression test next to the other FusedCodecPipeline
pool tests and pin `codec_pipeline.max_workers` to 4 through the shared
`_FUSED_POOL_CONFIG`. With the default, a 1-CPU host resolves
`max_workers` to 1, never creates the pool, and passes the test without
exercising the bug. The test now asserts that the parent created the
pool before forking, and it runs on macOS, where the fork start method
is available.

Assisted-by: ClaudeCode:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.71%. Comparing base (6599477) to head (0755f07).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4504   +/-   ##
=======================================
  Coverage   94.71%   94.71%           
=======================================
  Files          94       94           
  Lines       13663    13663           
=======================================
  Hits        12941    12941           
  Misses        722      722           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label Oct 9, 2026
@d-v-b
d-v-b marked this pull request as ready for review October 9, 2026 17:41

This branch has not been deployed

No deployments
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.

2 participants