Fix async ZipArchive calling non-async Stream methods

[ZingBallyhoo]

Fixes #121624
Supersedes #121633

Note: this could be classed as a breaking change, as an exception that was thrown on accessing .Entries is now thrown from CreateAsync (although this is the approved API shape #1541 (comment)). The ZipArchive_InvalidEntryTable and ZipFile_Open.InvalidFilesAsync tests have been updated to reflect this.

[stephentoub]

What's wrong with #121633 ? If it's just about the comments already left there, where an existing test should be fixed, we should fix that in the existing PR rather than creating a new one.

[ZingBallyhoo]

There are two related issues in #121624

1. The author's issue (post): DisposeAsync calls Dispose on the underlying stream.
2. My issue (commented): There is no way to access .Entries without a sync read, as a note in the API review was missed.

Both issues shipped because of one buggy test, which I located after some analysis.

The Copilot PR only fixes 1, and it's approach to fixing it is not correct. It's not going to figure out that the only purpose of IsRestrictionEnabled is to hide bugs without someone explicitly saying that.

As I said in my reply there, I was happy to contribute this as a replacement or let somebody convince Copilot to do it correctly, but I cannot do that myself (It can't see my review comments).

---

There is another test failing because of the "possible breaking change" I mentioned, so I would like some guidance on what to do about it. I checked the API review video and there didn't seem to be any discussion about CreateAsync possibly throwing (due to invalid central directory) where new ZipArchive doesn't, due to the eager reading...

Edit: I'll just modify the test for now to see if CI catches anything else. I'm struggling to run full libraries tests locally

[ericstj]

I see why you suggest this is breaking - since any exception thrown from Entries will now be thrown from CreateAsync. I reread the API notes a few times and agree with your interpretation - that we didn't need GetEntriesAsync, GetEntryAsync, or EnsureLoadedAsync - all of that work can happen in CreateAsync. The only case where we might want separate methods would be someone who opens but doesn't fetch the entries - can you think of any use case for this? Maybe someone who only wants to read the Comment. So it's probably good to mark this breaking so that anyone who tries to catch the exception from Entries/GetEntry can include the call to CreateAsync inside, but I doubt this will break any end-to-end usage.

[dotnet-policy-service]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[rzikm]

Changes LGTM, should be good to go when ericstj's question above gets answered.

[ZingBallyhoo]

can you think of any use case for this? Maybe someone who only wants to read the Comment

At least in the current ZipArchive API, no, unless .NET is simply passing a zip to another service (but needs the Comment). In the design issue for Comment there are links relating to exposing other data, but that has never happened. #1547 (comment). I don't know enough about the format (and its vendor specific extensions) to say more here.

but I doubt this will break any end-to-end usage.

Yes, especially as the method is already documented as throwing InvalidDataException: "The contents of the stream could not be interpreted as a ZIP file". It's just a slightly stricter definition of "valid".

[rzikm]

/ba-g Test failure is unrelated

[rzikm]

Thanks for the contribution!

[rzikm]

@ericstj Can we run your breaking change tool on this PR to generate the docs issue?

[ericstj]

Seems like it hit an issue https://github.com/dotnet/runtime/actions/runs/19819155489/job/56777239257. This might be the first PR that it's running on a trigger. Let me try it out locally to diagnose what went wrong.

Update: yes, I broke this while refactoring. I'll add a comment with a local run of the script and submit a PR to the workflow.

[ericstj]

📋 Breaking Change Documentation Required

Create a breaking change issue with AI-generated content

Generated by Breaking Change Documentation Tool - 2025-12-01 11:12:36

[rzikm]

Created dotnet/docs#50254