fix(io): publish an output only if its teardown succeeded (#434) - #436

Thanks — this is a careful patch, and the parts I could check hold up. CI hasn't run yet (fork PRs need approval here), so I ran it locally against dev @ 63f5d44: fmt and clippy clean, 710 tests pass on macOS, and output from both the serial and --cores 2 paths is valid gzip with identical decompressed md5s. Your reading of ZWriter::finish is right too — it take()s all three fields ParCompress::drop checks, so there's no second finish on the success path.

One thing your analysis couldn't see from inside this repo, and it changes the calculus on the framing change. nf-core/modules pins the compressed bytes in its nf-test snapshot:

"test_1_val_1.fq.gz:md5,75413e85910bbc2e1556e12f6479f935",
"test_2_val_2.fq.gz:md5,3c023e8e890b897821df3dc98f48c2b3"

Our CI is immune because it compares through gzip -dc, but those md5s are raw-byte, so a deflate-framing change is snapshot-breaking there. It won't break them today — that module still documents 0.6.x behaviour — but v2.x adoption is in flight, so the exposure is real at adoption time.

Could you make the fix byte-identical? Calling encoder.flush() before encoder.try_finish() in Sink::Gz::finish restores the sync marker while keeping everything the patch is actually for: the teardown is still explicit, its error is still propagated, and the output is still only published if it succeeded. The framing difference is incidental to the correctness fix rather than part of it, and dropping it means this needs no coordination with anyone downstream. Costs 11 bytes per file.

Otherwise I'm happy with the shape — deleting impl Drop for FastqWriter so finish can destructure is better than the Option-take dance I left there, and the read-only-File trick is a neater way to test the failure than the size-limited filesystem I assumed in #434.

Done — pushed as c277208. Byte-identity holds, but one flush was not enough, and that is worth a paragraph rather than a footnote.

encoder.flush() before try_finish() got the output to within 5 bytes of dev, not to zero. The pre-#434 teardown flushed the sink twice: FastqWriter::finish did self.writer.flush()?, then drop(self) ran impl Drop for FastqWriter's let _ = self.writer.flush() on the same writer immediately afterwards. Two Z_SYNC_FLUSHes, two empty stored blocks on the wire. The redundant one was invisible from inside the repo for exactly the reason you gave — everything here compares through gzip -dc — so it has been part of the shipped format without anyone choosing it.

Last 24 bytes of BS-seq_10K_R1_val_1.fq.gz, serial path:

dev @63f5d44      ... b4 49 fe 1f | 00 00 00 ff ff | 00 00 00 ff ff | 03 00 | ad 82 3a 53 | f8 37 1d 00
one flush         ... b4 49 fe 1f | 00 00 00 ff ff |                  03 00 | ad 82 3a 53 | f8 37 1d 00
this PR, before   ... b4 49 fe 1f |                                          ad 82 3a 53 | f8 37 1d 00

341,686 / 341,681 / 341,674 bytes. So the cost is 7 + 5 here, not 11 — the second marker is a flat 5, the first also has to terminate the open block and pad to a byte boundary, which is why the CHANGELOG's 1M-read measurement said 11 and this 10K one says 12. All three decompress to the same md5.

Sink::finish now emits both. The count is a named constant, PRE_434_GZ_SYNC_FLUSHES, because "call flush twice in a row" reads as a typo otherwise — and because it makes dropping the framing later a one-character edit rather than an archaeology exercise. Set it to 1 for the redundant marker only, 0 for both; either is a defensible cleanup and either wants a heads-up to whoever pins compressed bytes. It just isn't #434's business.

One thing your local run couldn't have caught, since it compared decompressed md5s: --cores N was already byte-identical at the byte level. I cmp'd it rather than assuming, so the ParGz arm needed no change — the re-framing was only ever the serial Sink::Gz path.

Verified against dev @63f5d44, both binaries built here, same inputs, cmp not md5:

Pinned by gz_sink_finish_is_byte_identical_to_the_pre_434_teardown, which rebuilds the reference from flate2 in the old call order rather than storing an md5 — so it keeps meaning something if flate2's deflate output ever changes, since what it pins is the call sequence — and counts the markers, because one flush is the plausible way to get this wrong and it fails silently everywhere our CI can see.

The CHANGELOG paragraph announcing the framing change is now the opposite paragraph: output bytes unchanged, with the reason recorded so the next person to look at that double flush knows it is load-bearing.

Here: 709 tests pass, 0 failed; cargo fmt --check and clippy --all-targets --release -- -D warnings clean. This box is Linux (musl, zig cc as the C driver), so your 710 on macOS is presumably a platform-gated test or two, and I can't run nf-core's nf-test suite here — the byte-identity above is the evidence I have that their snapshot is safe.

The double-flush finding is right and I'd have missed it — but the loop landed in Sink::Gz only, and ParGz needs it too. ParCompress::flush is flush_last(false) → FlushCompress::Sync, and the old teardown reached it twice by the same route (finish's explicit flush, then Drop's), so that arm went from two markers to none.

Measured against dev @ 63f5d44, both built here: at c277208, --hardtrim5 30 --cores 2 is 209,305 → 209,294 and --hardtrim3 30 --cores 4 is 209,190 → 209,178. Adding the same loop before w.finish()? in the ParGz arm makes all four arms cmp-identical, with 711 tests still passing.

Worth knowing why the 12/12 matrix missed it: --cores N trimming never builds a Sink — parallel.rs compresses per batch into a Vec and writes through a raw File. ParGz is only reachable from specialty.rs and demux.rs, so --hardtrim5 --cores 2 is the row that distinguishes the paths. Since that claim is in the commit message rather than just the PR body, it needs amending before this squashes onto dev.

Also from review, independent of the above: close(2) is unchecked in all three arms, so "every byte it owes was written" overstates what is delivered; ParCompress::finish fails before it disarms its own Drop, so a failed ParGz teardown panics rather than returning Err; and gz_sink_finish_reports_a_failing_teardown's comment names the trailer when the write that actually fails is the header. And ParGz still has no test of its own — it's the arm this thread has now caught twice.

Done — pushed as 6d727ba, and the branch is force-updated because the commit message needed amending as you said.

You were right about ParGz, and I reproduced your numbers exactly. --hardtrim5 30 --cores 2 209,305 → 209,294 and --hardtrim3 30 --cores 4 209,190 → 209,178, against dev @ 63f5d44, all three binaries built here. The loop is now in the ParGz arm and all four arms are cmp-identical: 32 outputs across 15 invocations — the 12 rows from before, plus --hardtrim5/--hardtrim3 at 1, 2 and 4 cores and --demux at 1 and 2. 709 → 712 tests, fmt and clippy -D warnings clean.

Your explanation of why the matrix missed it is in the code now, at PRE_434_GZ_SYNC_FLUSHES, because it is the thing that makes the arm easy to lose again: --cores N trimming never builds a Sink, so a --cores row in a trimming matrix cannot reach ParGz, and --hardtrim5 30 --cores 2 is the shortest invocation that separates the paths.

The panic is real, and the obvious test does not catch it

I wrote the test I expected to fail, and it passed both with and without a fix. That was worth chasing rather than shipping.

ParCompress::write, on a send failure, takes the join handle itself to report the writer thread's real error — which leaves handle as None and disarms Drop. So a test that writes bulk data to an unwritable sink never panics; it gets Bad file descriptor (os error 9) from write_all and a quiet drop. Measured.

The panic needs a run where no write ever failed: the payload still sits in the compressor's own buffer, nothing has been sent, and the writer thread has died on its own — failing the gzip header write. The teardown's first flush is then the first send, it fails, and Drop re-enters finish():

thread panicked at gzp-2.0.2/src/par/compress.rs:398:27:
called `Result::unwrap()` on an `Err` value: Io(Custom { kind: Other, error: "SendError(..)" })

pargz_sink_finish_reports_a_failing_teardown_without_panicking reproduces that and passes only with the disarm — verified in both directions. It is written so that losing the race makes it weaker, never flaky: if the send still succeeds, finish joins the thread and returns the same failure by the other route and the test still passes.

One thing I had to fix twice. My first disarm only covered finish(), and the flushes reach the same panic — ? out of that arm drops w on the way. Every fallible step is now inside one closure so a single error path covers all of them.

What disarming costs, stated rather than buried: mem::forget leaks a Sender pair and a JoinHandle, and the compressor threads then park on a channel that is never closed. It is bounded by the process — every FastqWriter::finish caller propagates with ? — and a panic in place of an error message seemed the worse trade on a path #434 exists to make reportable. The real fix is upstream: ParCompress::finish should take its fields before the ?. I'm happy to file that against gzp and drop this local workaround when it lands, if you'd rather not carry it.

close(2)

You're right and I have not delivered it — I corrected the claim instead. FastqWriter::finish no longer says every byte "has been written successfully"; it says handed to a write(2) that returned success, and then says plainly what that is not: close(2) is unchecked in all three arms, nothing calls fsync, and on NFS a run can still publish an output whose last bytes never reached the server.

Doing it properly means an explicit checked close, and File::close is not stable on the toolchain this builds with (1.97), so it would mean taking a libc or rustix dependency this crate does not have. That felt like your call rather than mine, and larger than #434 — which is the local-disk ENOSPC case the checked write(2) does cover. Say the word and I'll do it.

The test comment, and the case it was not testing

Fixed, and it was worse than a wrong comment. Nothing is written to that sink, so the first bytes it emits are the 10-byte header — the easy case. The comment now says so.

Added gz_sink_finish_reports_a_failure_after_a_successful_write for the case #434 is actually about: header and payload reach the destination, and only the teardown's bytes fail. A pipe gives that without a size-limited filesystem — writes succeed while the read end is open and fail with EPIPE once it is closed, and Rust sets SIGPIPE to SIG_IGN so it surfaces as an io::Error.

ParGz also has the byte-identity twin of the serial test now, single-threaded on both sides on purpose: with more than one thread neither the block boundaries nor the output bytes are pinned, and the marker count is what the test is about. Its marker assertion is a floor rather than an equality, since ParCompress emits a marker at its own block boundaries too — this payload is one block and measures exactly 2.

The commit message, and one more copy of the same claim

Amended: b9089ed now says the 12/12 matrix does not reach Sink::ParGz and that the earlier wording read it as though it did, and points at the following commit.

The same false claim was also in CHANGELOG.md — "--cores N output was never affected" — which you didn't mention and which is the copy a user reads. Fixed in the same commit, and it now names which modes reach the parallel sink and lists the full verified matrix.

Everything measured here is re-runnable; happy to hand over the exact invocations if useful. And thank you for the ParGz catch — that arm has now been the answer twice on this thread.

Confirmed at 6d727ba — the ParGz arm is right now. I rebuilt both sides and cmp'd 44 outputs across 24 invocations against dev @ 63f5d44, including --hardtrim5 30 --cores 2 and --hardtrim3 30 --cores 4, and every one is identical; 714 tests here, which is your 712 plus the two macOS-gated ones, with fmt and clippy -D warnings clean. I checked the panic test in both directions rather than taking it on trust: removing mem::forget(w) gives exactly the compress.rs:398 unwrap you quoted, and restoring it goes green.

On close(2): leave it. #434 is the local-disk ENOSPC case and the checked write(2) covers that, so a libc or rustix dependency for the NFS case is a decision I'd rather take on its own — the corrected wording is the right outcome here. Do please file the gzp issue, since ParCompress::finish taking its fields before the ? is the real fix and I'm happy to carry the disarm until it lands. One thing worth a line in that comment: the leak is bounded because every current FastqWriter::finish caller propagates with ?, which is a property of the callers rather than of the type, so a future caller that recovers and continues would quietly accumulate parked threads.

CI is running on this now — it had not run since b8aa7d8, as every push after it needed approval. You will need a rebase: #438 landed on dev an hour ago and collides with you in CHANGELOG.md only, where we both added an entry at the top of #### Changes. Nothing in fastq.rs or parallel.rs is touched.

Rebased and pushed as b22a9e5; mergeable is back to true. Both of your asks are done, and the rebase turned out to need one measurement rather than none.

The rebase

Three commits had landed on dev, not one — #423, #424 and #429 — and dev is now at 115d84f. Your read of the collision was right: the only textual conflict is the one entry at the top of #### Changes, and at file level your side touches CHANGELOG.md, src/bam.rs, src/io.rs, src/main.rs and two integration tests while this branch touches CHANGELOG.md, src/fastq.rs and src/parallel.rs. CHANGELOG.md is the whole intersection. Resolved by keeping both entries, #434 above #429.

But src/io.rs gained 361 lines in those three commits, so the byte-identity claim in the PR body was measured against a tree that is no longer the one being merged. A textual clean apply says nothing about output bytes, so I re-ran the matrix against the new tip rather than carrying the old numbers forward.

cmp, not md5, dev @ 115d84f vs b22a9e5, both built here: 32 outputs across 15 invocations, all 32 identical — the same matrix as last round (SE default, --compression 6, --cores 4, --rrbs, --polyA, --nextera; --paired serial, --cores 2, --compression 6; --hardtrim5 at 1 and 2 cores, --hardtrim3 at 4, --hardtrim5 --cores 2 on clock_10K, and --demux at 1 and 2 cores). Nothing in the three new commits moves the gzip framing.

Tests, both sides on this box: dev @ 115d84f is 720, this branch is 727 — so the PR still contributes the same 7, and the 15 between last round's 712 and today's 727 are yours. cargo fmt --check and clippy --all-targets --release -- -D warnings clean.

close(2)

Left, as you said, with the corrected wording standing. Nothing further on it here.

The gzp issue

Filed: sstadick/gzp#68. Your caller-property caveat is in it, in those terms — the leak is bounded only because every current FastqWriter::finish caller propagates with ?, which is a property of the callers rather than of the type, so a future caller that recovers and continues would quietly accumulate parked threads.

Two things I added while writing it up, since a bug report that needs a bioinformatics pipeline to reproduce tends not to get fixed.

It reproduces in 40 lines against gzp alone. A sink whose every write fails, and a caller that writes nothing: the writer thread dies on gzip's own header, so finish()'s flush_last(true)? is the first and only send in the program, and there is no race left to lose.

finish() returned Err as it should: sending on a closed channel
returned from main body without panicking

thread 'main' panicked at gzp-2.0.2/src/par/compress.rs:398:27:
called `Result::unwrap()` on an `Err` value: Io(Custom { kind: Other, error: "SendError(..)" })

The error is delivered. The panic is purely the second, redundant teardown — which I think is the clearest statement of why this is worth fixing upstream rather than working around.

And the reason it hides: Write::write already handles this correctly in the other direction. On a send failure it does self.handle.take().unwrap().join() to recover the writer thread's real error, and taking the handle disarms Drop as a side effect. That is the same thing I ran into locally when the obvious test passed with and without the fix, and it is worth naming in the issue because it is why nobody has hit this before: the paths that fail loudly are the ones that happen to disarm, and the quiet path is the one that panics.

I've offered to send the fix as a PR there. If it lands, dropping the local mem::forget is a one-commit follow-up here and I'll open it.