Let CRAMContainerStreamWriter callers keep ownership of their stream

[tfenne]

Closes #1092.

CRAMContainerStreamWriter takes an OutputStream created by the caller but unconditionally closed it in finish(), so code that manages the stream lifecycle itself had no way to finish writing without having its stream closed underneath it — and a subsequent close() by the owner would then fail.

Adds finish(writeEOFContainer, closeStream) so callers can opt out of the close. The existing finish(writeEOFContainer) overload delegates with closeStream=true, so behaviour is unchanged for every current caller, including CRAMFileWriter, which relies on it to close the file stream it owns.

The stream is flushed either way, so opting out never loses data.

Tests cover both overloads, that the single-argument form still closes (backwards compatibility), and that a caller can keep using its own stream afterwards.

Summary by CodeRabbit

• New Features

  • Added an overload to CRAM writer finalization that lets callers choose whether the underlying output stream is closed.
  • The original single-argument finalization continues to close the stream by default.

• Bug Fixes

  • Finalization now conditionally closes the stream only when requested, allowing callers to keep using the same stream after finalization.
  • Flushing, optional EOF container emission, indexing, and existing error-handling behavior are preserved.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7f6a6fd5-1b44-4941-8761-fa48f9b5cb9e

📥 Commits

Reviewing files that changed from the base of the PR and between c15932c and ab81611.

📒 Files selected for processing (1)

• src/test/java/htsjdk/samtools/cram/CRAMContainerStreamWriterClosureTest.java

🚧 Files skipped from review as they are similar to previous changes (1)

• src/test/java/htsjdk/samtools/cram/CRAMContainerStreamWriterClosureTest.java

---

📝 Walkthrough

Walkthrough

CRAMContainerStreamWriter adds a finish overload that controls whether the supplied OutputStream is closed. The existing overload continues closing the stream by default, with tests covering both behaviors and continued stream use.

Changes

CRAM stream closure

Layer / File(s)	Summary
Configurable finish API src/main/java/htsjdk/samtools/CRAMContainerStreamWriter.java	Adds finish(boolean, boolean) and makes stream closure conditional while preserving the existing single-argument behavior.
Stream closure validation src/test/java/htsjdk/samtools/cram/CRAMContainerStreamWriterClosureTest.java	Tests optional closure, default closure, CRAM output flushing, and writing additional data after finishing without closure.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: allowing callers to retain ownership of the output stream.
Linked Issues check	✅ Passed	The changes satisfy issue #1092 by adding a finish mode that flushes without closing the caller-owned OutputStream.
Out of Scope Changes check	✅ Passed	The diff stays focused on CRAMContainerStreamWriter stream-closure behavior and targeted tests, with no obvious unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch tf_fix_cram_1092

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/test/java/htsjdk/samtools/cram/CRAMContainerStreamWriterClosureTest.java`:
- Around line 57-68: Update CloseRecordingStream and
finish_does_not_close_stream_when_close_stream_is_false so the test records
whether flush() was invoked and asserts that flag after writer.finish(true,
false). Remove the size-based assertion, since ByteArrayOutputStream visibility
does not verify flushing, while preserving the assertion that the
caller-provided stream remains open.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d442414a-82ea-4241-9f64-55b0fd2d1584

📥 Commits

Reviewing files that changed from the base of the PR and between 2ae4aaa and c15932c.

📒 Files selected for processing (2)

• src/main/java/htsjdk/samtools/CRAMContainerStreamWriter.java
• src/test/java/htsjdk/samtools/cram/CRAMContainerStreamWriterClosureTest.java

[tfenne]

Good catch, thanks — fixed in the follow-up commit.

You were right that the assertion proved nothing about flushing: ByteArrayOutputStream makes writes visible immediately and inherits a no-op flush(), so size() > 0 only showed that bytes had been written. It would have passed just as happily if finish(..., false) had stopped flushing altogether.

CloseRecordingStream now tracks flush() alongside close(), and the test asserts the flag instead of the stream size.

I confirmed the new assertion actually bites by temporarily removing the outputStream.flush() call from finish():

AssertionError: declining the close must still flush the stream, or data can be lost
  expected [true] but found [false]

The old size() > 0 assertion passed under that same regression.