fix: Don't dispose source stream by default in ToMemoryStreamAsync

[thomhurst]

Summary

Fixes #1534

The ToMemoryStreamAsync extension method was unexpectedly disposing the input stream after copying, which is surprising behavior for an extension method.

Problems Fixed

1. Unexpected disposal: Extension methods shouldn't typically dispose their inputs
2. Inconsistent behavior: MemoryStream inputs were returned as-is (not disposed), while other streams were disposed
3. No opt-out: Callers couldn't prevent disposal

Changes

• Add disposeSource parameter (defaults to false)
• Reset output MemoryStream position to 0 for immediate reading
• Also reset MemoryStream input position to 0 for consistency
• Updated tests to pass disposeSource: true to prevent resource leaks

Migration

Existing callers who relied on the disposal behavior should update their calls:

// Before (implicit disposal)
var memoryStream = await stream.ToMemoryStreamAsync();

// After (explicit disposal)
var memoryStream = await stream.ToMemoryStreamAsync(disposeSource: true);

Test plan

• Build succeeds
• Tests updated to pass disposeSource: true
• Verify stream copying works correctly
• Verify disposeSource parameter works as expected

🤖 Generated with Claude Code

[thomhurst]

Summary

This PR improves the ToMemoryStreamAsync extension method by making stream disposal explicit via a disposeSource parameter (defaults to false) and ensuring the returned MemoryStream is positioned at 0 for immediate reading.

Critical Issues

1. Inconsistent disposal behavior when input is already a MemoryStream

The disposeSource parameter is not respected in the early-return path for MemoryStream inputs:

if (stream is MemoryStream memoryStream)
{
    if (memoryStream.CanSeek)
    {
        memoryStream.Position = 0;
    }

    return memoryStream;  // Bug: doesn't respect disposeSource parameter
}

Impact: If a caller passes disposeSource: true with a MemoryStream, they expect it to be disposed, but it won't be. This creates inconsistent behavior between MemoryStream inputs and other stream types.

Fix: Add disposal logic before returning:

if (stream is MemoryStream memoryStream)
{
    if (memoryStream.CanSeek)
    {
        memoryStream.Position = 0;
    }

    if (disposeSource)
    {
        await memoryStream.DisposeAsync().ConfigureAwait(false);
    }

    return memoryStream;
}

Wait, that won't work because we're returning the same instance we just disposed. The semantics here need clarification:

• Should ToMemoryStreamAsync on an already-MemoryStream input just return the same instance?
• If so, disposing it and then returning it is wrong
• Perhaps when disposeSource: true on a MemoryStream input, we should NOT return it directly but instead copy to a new MemoryStream?

Alternatively, document that disposeSource only applies to non-MemoryStream inputs, though that's surprising.

Suggestions

Consider the MemoryStream input semantics more carefully

The current behavior returns the same MemoryStream instance that was passed in (just with position reset). This has implications:

1. If the caller mutates the returned stream, they're mutating the original
2. If disposeSource: true, what should happen? The caller can't use the returned stream if we dispose the input

One option: Always create a new MemoryStream copy when disposeSource: true:

if (stream is MemoryStream sourceMs)
{
    if (!disposeSource)
    {
        if (sourceMs.CanSeek)
        {
            sourceMs.Position = 0;
        }
        return sourceMs;
    }
    
    // disposeSource is true, so copy to new MemoryStream
    // (fall through to regular copy logic below)
}

Previous Review Status

No previous comments.

Verdict

⚠️ REQUEST CHANGES - Critical issue with inconsistent disposal behavior for MemoryStream inputs

[thomhurst]

Summary

Changes ToMemoryStreamAsync to no longer dispose the source stream by default, adding an opt-in disposeSource parameter.

Critical Issues

Resource leak in tests - The test updates only add disposeSource: true to two test methods (ReadEmptyFile and ReadWriteFile), but file.GetStream() returns a stream that needs disposal. These tests will leak file handles.

The pattern in FileTests.cs:110,131 should be:

// Current (leaks the FileStream from GetStream())
var stream = await file.GetStream().ToMemoryStreamAsync(disposeSource: true);

// Should be one of:
// Option 1: Dispose source stream (what the test intends)
await using var sourceStream = file.GetStream();
var stream = await sourceStream.ToMemoryStreamAsync(disposeSource: false);

// Option 2: Use using declaration with the result
await using var stream = await file.GetStream().ToMemoryStreamAsync(disposeSource: true);

The current code passes disposeSource: true but the source is file.GetStream() (a FileStream), not the MemoryStream result. The FileStream is never disposed.

Suggestions

1. Position reset logic: Setting Position = 0 on the input MemoryStream (line 16-17 in the new code) happens only when returning early, but not in the copy path. For consistency, consider whether non-seekable MemoryStreams should be handled differently or if this is intentional.

2. Documentation clarity: The XML doc states "callers are responsible for disposing the source stream" when disposeSource: false, but doesn't mention that callers are responsible for disposing the returned MemoryStream. Consider adding that for completeness.

3. Breaking change impact: While changing the default from "always dispose" to "never dispose" is the right call (principle of least surprise), this is technically a breaking change. Consider if a major version bump is warranted or if the fix justifies the break.

Previous Review Status

No previous review comments found.

Verdict

⚠️ REQUEST CHANGES - Critical resource leak in tests must be fixed

[Copilot]

Pull request overview

This PR fixes unexpected stream disposal behavior in the ToMemoryStreamAsync extension method by making disposal opt-in rather than automatic. The change addresses issue #1534 where the method was disposing input streams without caller control, which is surprising for an extension method.

Key changes:

• Added disposeSource parameter (defaults to false) to give callers explicit control over source stream disposal
• Added position reset logic to ensure returned MemoryStream is ready for immediate reading
• Updated existing tests to pass disposeSource: true to maintain previous behavior and prevent resource leaks

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/ModularPipelines/Extensions/StreamExtensions.cs	Added disposeSource parameter with default value false, conditional disposal logic, position reset for output stream, and improved optimization logic for MemoryStream inputs
test/ModularPipelines.UnitTests/FileTests.cs	Updated two test methods to explicitly pass disposeSource: true to maintain previous disposal behavior

[Copilot]

The new default behavior (disposeSource: false) lacks test coverage. Consider adding tests that verify:

1. The source stream is NOT disposed when disposeSource is false (or omitted)
2. The returned MemoryStream can be read (Position is 0)
3. Behavior with MemoryStream inputs vs other Stream types
4. The source stream IS disposed when disposeSource is true

[thomhurst]

Summary

Fixes resource disposal issues in StreamExtensions.ToMemoryStreamAsync() by adding a disposeSource parameter and resetting stream position.

Critical Issues

1. Breaking Change - Default Behavior Changed ⚠️

Old behavior: Always disposed the source stream (line 29 in old code)
New behavior: Only disposes when disposeSource: true (defaults to false)

This is a breaking change that will cause resource leaks for existing callers who relied on automatic disposal.

Impact: Any existing code calling ToMemoryStreamAsync() without the new parameter will leak the source stream.

Files affected:

• src/ModularPipelines/Extensions/StreamExtensions.cs:39-42

Recommendation: Consider one of these approaches:

• Change default to disposeSource: true to maintain backward compatibility
• Add [Obsolete] to the old signature and create a new method name to force callers to make explicit choice
• Search entire codebase for all usages and update them

---

2. Inconsistent Behavior When Source is MemoryStream

When input is already a MemoryStream:

• disposeSource: false → Returns original stream directly
• disposeSource: true → Creates new MemoryStream, copies data, disposes original

Issue: Why copy a MemoryStream just to dispose the source? If the caller wants to dispose it, they could call .Dispose() themselves. The copy is wasteful.

Files affected:

• src/ModularPipelines/Extensions/StreamExtensions.cs:13-18

Recommendation: Consider returning the source MemoryStream directly even when disposeSource: true, and let the caller handle disposal (since they're getting the stream back anyway).

---

3. Missing Position Reset in Early Return Path

The optimization path for non-disposable MemoryStreams (lines 13-20) only resets position if sourceMs.CanSeek is true. But the method documentation promises: "with Position set to 0 for reading."

Files affected:

• src/ModularPipelines/Extensions/StreamExtensions.cs:14-18

Issue: If sourceMs.CanSeek is false, position is not reset, violating the documented contract.

Recommendation: Either handle non-seekable streams consistently or document this edge case.

---

Suggestions

1. Consider Simplifying the API

The current API has subtle behavior differences. Consider these alternatives:

Option A: Always copy, never dispose source

public static async Task<MemoryStream> ToMemoryStreamAsync(this Stream stream)
{
    var ms = new MemoryStream();
    if (stream.CanSeek) stream.Position = 0;
    await stream.CopyToAsync(ms).ConfigureAwait(false);
    ms.Position = 0;
    return ms;
}

Caller is always responsible for disposing the source.

Option B: Separate methods for clarity

ToMemoryStreamAsync() // Never disposes
ToMemoryStreamAndDisposeAsync() // Always disposes

---

Verdict

⚠️ REQUEST CHANGES - Critical breaking change in default disposal behavior will cause resource leaks in existing code.

[thomhurst]

Summary

Changes ToMemoryStreamAsync to default to NOT disposing the source stream (breaking change), adds disposeSource parameter, and optimizes when input is already a MemoryStream.

Critical Issues

None found ✅

Suggestions

1. Resource Leak Risk in Tests (Lines 110, 131 in FileTests.cs)

The current implementation has a potential resource leak. In the tests, you're calling:

await using var stream = await file.GetStream().ToMemoryStreamAsync(disposeSource: true);

However, file.GetStream() returns a FileStream that is immediately passed to ToMemoryStreamAsync. If ToMemoryStreamAsync throws an exception BEFORE the disposal logic runs, the FileStream will leak because it was never assigned to a variable that has disposal tracking.

Recommended fix:

// Option 1: Use intermediate variable
await using var sourceStream = file.GetStream();
await using var stream = await sourceStream.ToMemoryStreamAsync(disposeSource: true);

// Option 2: Since we're disposing anyway, just use the file stream directly for testing
await using var stream = file.GetStream();
// Read from stream for assertions...

Given the tests are just reading content, you might not even need ToMemoryStreamAsync at all - you could test the stream directly.

2. Consider Position Reset Edge Case

In StreamExtensions.cs:38-42, when the input is already a MemoryStream and disposeSource is false, you check CanSeek before resetting position. However, you unconditionally set Position = 0 at line 61 for the copied stream without checking CanSeek.

While MemoryStream is always seekable, for consistency and safety with the public API contract (accepting any Stream), consider:

if (memoryStream.CanSeek)
{
    memoryStream.Position = 0;
}
return memoryStream;

3. Documentation Note

The breaking change is well-documented in XML docs. Consider also:

• Adding an obsolete overload with the old behavior to help migrations: [Obsolete("Use ToMemoryStreamAsync(disposeSource: true) instead")]
• Or mentioning this in CHANGELOG.md / release notes if not already done

The explicit documentation about the breaking change is excellent - users will understand the migration path clearly.

Verdict

✅ APPROVE - No critical issues

The core implementation is solid. The suggestions are minor improvements around resource management best practices and edge case handling. The breaking change is well-justified (principle of least surprise) and properly documented.