Skip to content

Drain sink on stream destroy + destroy run-out test#158

Merged
nclack merged 8 commits into
acquire-project:mainfrom
nclack:fix-147-152-teardown
Jun 18, 2026
Merged

Drain sink on stream destroy + destroy run-out test#158
nclack merged 8 commits into
acquire-project:mainfrom
nclack:fix-147-152-teardown

Conversation

@nclack

@nclack nclack commented Jun 18, 2026

Copy link
Copy Markdown
Member

Closes #147.
Closes #152.

Finishing a stream writes out the last partial shards, which queues their footers to be written in the background. If that step fails partway through, the queued writes still point at buffers the stream is about to free — so a background writer could read freed memory and corrupt an otherwise-good shard.

Fix: drain the queued writes before freeing the buffers they reference.

  • Drain on the flush error paths (GPU and CPU), where the destination is still live.
  • On destroy, drain as a safety net — but only when destroy ran the flush itself. A caller that flushed successfully may tear its destination down first, so destroy must not touch it in that case.
  • Apply the same drain to the multi-array writers, which had the same gap (draining only the arrays destroy flushed).

Tests:

  • A CPU test forces a flush failure with writes still queued and checks, under AddressSanitizer, that destroy cleans up without touching freed memory.
  • test_destroy_midstream does not exercise the teardown run-out path (needs flush fault injection) #152: the teardown test never actually left work queued at teardown, so it wasn't exercising that path. It now holds the delivery worker so a job is still pending when the stream is destroyed, and checks teardown runs it out without hanging or reading freed memory.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.28571% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/multiarray/stream.c 53.84% 2 Missing and 4 partials ⚠️
src/zarr/shard_pool_fs.c 66.66% 2 Missing and 1 partial ⚠️
src/cpu/stream.c 50.00% 0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (64.28%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Files with missing lines Coverage Δ
src/cpu/stream.body.c 78.66% <100.00%> (+1.98%) ⬆️
src/cpu/stream.c 81.03% <50.00%> (+0.29%) ⬆️
src/zarr/shard_pool_fs.c 61.13% <66.66%> (+0.22%) ⬆️
src/multiarray/stream.c 73.77% <53.84%> (-0.69%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nclack nclack merged commit 639a6f3 into acquire-project:main Jun 18, 2026
8 checks passed
@nclack nclack deleted the fix-147-152-teardown branch June 18, 2026 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant