Implement RFC 9659 compliance for "zstd" HttpContent compression

[rzikm]

Follow-up on #130082 (I didn't get a notification about that one so I didn't comment in time).

cc @iremyux

Open Question: What do we for user-provided ZstandardCompressionOptions? should we prevent users from creating non-compliant content?

Opportunistically fixes #130948

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Adjusts ZstandardCompressedContent to avoid producing non-RFC-compliant "zstd" request bodies (RFC 9659 window requirement) and updates functional tests to validate round-tripping across compression levels under an 8 MiB (2^23) max window decoder.

Changes:

• Clamp CompressionLevel.SmallestSize for zstd request compression by switching to explicit ZstandardCompressionOptions with WindowLog2 = 23.
• Expand Brotli/Zstandard functional tests to cover all CompressionLevel values and validate zstd output is decodable with MaxWindowLog2 = 23.
• Update test payload to better exercise compression behavior.

Reviewed changes

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

File	Description
src/libraries/System.Net.Http/src/System/Net/Http/ZstandardCompressedContent.cs	Uses RFC 9659-compatible window sizing for CompressionLevel.SmallestSize by selecting explicit zstd options.
src/libraries/System.Net.Http/tests/FunctionalTests/CompressedContentTest.NonBrowser.cs	Expands compression-level coverage and enforces the RFC window constraint in zstd decompression during tests.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "f8bf24f8cb9c2bc3a26073784f12facd1f742a66",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "883277ee4090d28d5e1d996cab7be81bf4f96c50",
  "last_reviewed_commit": "f8bf24f8cb9c2bc3a26073784f12facd1f742a66",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "883277ee4090d28d5e1d996cab7be81bf4f96c50",
  "last_recorded_worker_run_id": "29751396766",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "70e4987bc8e6d6ebd4ed506ca6f1909733ba4b7b",
      "review_id": 4730926033
    },
    {
      "commit": "f8bf24f8cb9c2bc3a26073784f12facd1f742a66",
      "review_id": 4736054723
    }
  ]
}

[github-actions]

Holistic Review

Motivation: RFC 9659 requires that the zstd HTTP content coding be decodable with a window size of at least 8 MB (2^23) and recommends encoders not exceed it. Previously ZstandardCompressedContent mapped CompressionLevel.SmallestSize to zstd's maximum quality, which can select a window larger than 8 MB, producing frames a conformant server may reject. This PR is a follow-up to #130082 to close that gap.

Approach: For the CompressionLevel constructor, SmallestSize is special-cased to use an explicit ZstandardCompressionOptions with Quality = MaxQuality and WindowLog2 = 23, routing it through the options-based stream path instead of the level-based path. Other levels are unchanged. The round-trip test is expanded to a [Theory] covering all four compression levels for both br and zstd, uses a larger, more compressible payload to exercise larger windows, and configures the test decoder with MaxWindowLog2 = 23 to mirror an RFC-conformant server.

Summary: The change is small, targeted, and correct. Capping the window at 2^23 for SmallestSize while preserving max quality is the right way to stay RFC-compliant without regressing other levels, and the decoder-side test change validates decodability within the RFC window bound. Test coverage is meaningfully improved by parameterizing over all levels. I found one minor naming-convention issue (inline). The PR author's open question about whether to also validate/reject non-compliant user-provided ZstandardCompressionOptions is worth resolving, but it is a deliberate scoping decision rather than a defect in this patch. Verdict: LGTM aside from the minor style nit.

Detailed Findings

• Naming convention (minor, inline): The new static field s_SmallestSizeRfcOptions uses PascalCase after the s_ prefix; repo convention is s_camelCase (e.g. s_smallestSizeRfcOptions).

Non-blocking observations

• The open question raised in the PR description (whether to prevent users from constructing non-RFC-compliant content via the ZstandardCompressionOptions constructor overload) is out of scope for this patch. This PR only constrains the CompressionLevel-based path; the options-based constructor still lets callers specify any WindowLog2 up to MaxWindowLog2 (2^31). That is a reasonable choice (explicit options imply the caller knows what they want), but consider documenting the RFC recommendation on the options constructor.
• The test's larger, repetitive payload (Enumerable.Repeat(...)) is a good way to encourage the encoder toward a larger window, strengthening the regression value of the SmallestSize case.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 61.8 AIC · ⌖ 10 AIC · ⊞ 10K

[MihaZupan]

Open Question: What do we for user-provided ZstandardCompressionOptions? should we prevent users from creating non-compliant content?

It wouldn't hurt to mention in remarks for the ZstandardCompressionOptions ctor overload that they may want to limit the window.

Given that options like smallest size are likely unpractically slow in practice anyway, I think it's fine to allow the user to set non-compliant options.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/libraries/System.Net.Http/src/System/Net/Http/ZstandardCompressedContent.cs:51

• In the SmallestSize case, _compressionLevel is left at the enum default (0) while _compressionOptions is set. Even though CreateCompressionStream currently uses options when non-null, leaving a readonly field uninitialized makes the instance state inconsistent and fragile if future logic (or debugging) relies on _compressionLevel. Assign _compressionLevel unconditionally and set _compressionOptions conditionally (similar to the Brotli/GZip implementations).

            if (compressionLevel == CompressionLevel.SmallestSize)
            {
                // use RFC-compliant options for SmallestSize to avoid producing frames that a conformant server would reject
                _compressionOptions = s_smallestSizeRfcOptions;
            }

src/libraries/System.Net.Http/src/System/Net/Http/ZstandardCompressedContent.cs:25

• The new RFC 9659 comment suggests this type is ensuring "zstd" HTTP content-coding compliance, but the ZstandardCompressedContent(HttpContent, ZstandardCompressionOptions) overload can still accept WindowLog2 = 0 (implementation-defined) or an explicit value > 23, which could produce non-compliant frames. Please clarify the intended policy: either (a) validate/clamp/throw for non-compliant compressionOptions, or (b) explicitly document that the options overload is an escape hatch and may produce non-RFC payloads.

        // RFC 9659 requires zstd decoders for the "zstd" content coding to support a window size of at
        // least 8 MB (2^23) and recommends that encoders not produce frames requiring a larger window.
        // Some compression levels (notably CompressionLevel.SmallestSize) would otherwise select a larger
        // window, producing payloads that a conformant server would reject. See RFC 9659, Section 3.
        private const int RfcMaxWindowLog2 = 23;

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Net.Http/src/System/Net/Http/ZstandardCompressedContent.cs:55

• In the CompressionLevel.SmallestSize branch, _compressionLevel is never assigned and ends up left at its default value (typically CompressionLevel.Optimal). It doesn’t affect behavior today because _compressionOptions is used, but it makes the object state misleading and risks future bugs if additional logic later relies on _compressionLevel (e.g., diagnostics, branching, or new properties). Consider always storing the user-provided level and then overriding _compressionOptions when you need RFC-compliant behavior.

            if (compressionLevel == CompressionLevel.SmallestSize)
            {
                // use RFC-compliant options for SmallestSize to avoid producing frames that a conformant server would reject
                _compressionOptions = s_smallestSizeRfcOptions;
            }
            else
            {
                _compressionLevel = compressionLevel;
            }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/libraries/System.Net.Http/src/System/Net/Http/ZstandardCompressedContent.cs:61

• In the CompressionLevel constructor, when compressionLevel == CompressionLevel.SmallestSize you assign _compressionOptions but leave _compressionLevel at its default value. This works today because CreateCompressionStream checks _compressionOptions, but it’s an inconsistency vs the other *CompressedContent types and makes future maintenance/error diagnosis harder (the instance no longer reliably reflects the requested CompressionLevel). Consider always capturing _compressionLevel and then setting _compressionOptions only for the SmallestSize special-case.

            if (compressionLevel == CompressionLevel.SmallestSize)
            {
                // use RFC-compliant options for SmallestSize to avoid producing frames that a conformant server would reject
                _compressionOptions = s_smallestSizeRfcOptions;
            }
            else
            {
                _compressionLevel = compressionLevel;
            }

src/libraries/System.Net.Http/src/System/Net/Http/ZstandardCompressedContent.cs:79

• The ZstandardCompressedContent(HttpContent, ZstandardCompressionOptions) overload doesn’t enforce the RFC 9659 8MB window guidance, so callers can (accidentally) produce Content-Encoding: zstd payloads that a minimally-conformant decoder may reject (e.g., by specifying WindowLog2 > 23, or potentially relying on an implementation-defined default). The remarks currently only say “consider limiting…”, but the type name/header imply this is the standardized zstd content-coding. It would be good to make an explicit decision here (validate/throw, clamp/copy options, or intentionally allow non-RFC output and document it clearly).

        /// <remarks>
        /// RFC 9659 requires that the "zstd" content coding be decodable with a window size of 8 MB (2^23) and
        /// recommends that encoders not produce frames requiring a larger window. When supplying custom options,
        /// consider limiting <see cref="ZstandardCompressionOptions.WindowLog2"/> to 23 or less so that the
        /// produced content is accepted by servers that enforce this limit.
        /// </remarks>
        public ZstandardCompressedContent(HttpContent content, ZstandardCompressionOptions compressionOptions)

[github-actions]

Holistic Review

Motivation: RFC 9659 requires that the zstd HTTP content coding be decodable with an 8 MB (2^23) window and recommends encoders not exceed it. ZstandardCompressedContent mapped CompressionLevel.SmallestSize to zstd's maximum quality, which can select a larger window, producing frames a conformant server may reject. This PR (follow-up to #130082) closes that gap.

Approach: The CompressionLevel constructor special-cases SmallestSize to explicit ZstandardCompressionOptions with Quality = MaxQuality and WindowLog2 = 23, routing it through the options path; other levels are unchanged. The round-trip test is parameterized across all four levels for br and zstd, uses a larger, more compressible payload, configures the decoder with MaxWindowLog2 = 23 to mirror an RFC-conformant server, and skips zstd+SmallestSize on 32-bit processes to avoid intermittent OOM. The single incremental commit since the prior review adds a <remarks> block to the CompressionLevel constructor documenting the RFC window behavior for SmallestSize — mirroring the <remarks> already present on the options constructor.

Summary: The incremental change is documentation-only and correct: the new <remarks> accurately describes the SmallestSize window-limiting behavior implemented in the constructor and is consistent with the class-level comment and the options-constructor remarks. It introduces no behavioral change and no risk. The cumulative change remains small, targeted, and RFC-compliant without regressing other levels. No new actionable findings. Verdict: LGTM.

Assessment History

• review 4730926033 (commit 70e4987): verdict LGTM with a minor style nit. Current verdict: LGTM. Assessment is unchanged in motivation, approach, and risk; the flagged naming nit and documentation suggestion were subsequently resolved.
• review 4733542998 (commit 9db9336): verdict LGTM. Current verdict: LGTM. Assessment is unchanged; the only new commit (f8bf24f) adds an XML-doc <remarks> block to the CompressionLevel constructor, a purely documentary refinement with no effect on motivation, approach, or risk.

Detailed Findings

None. The only incremental change adds accurate XML documentation to the CompressionLevel constructor.

Non-blocking observations

• The new <remarks> on the CompressionLevel constructor now parallels the existing <remarks> on the ZstandardCompressionOptions constructor, giving consistent RFC guidance across both entry points.
• The previously noted open question about validating user-supplied non-RFC ZstandardCompressionOptions remains out of scope; the documentation approach continues to be a reasonable middle ground.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 58 AIC · ⌖ 16.1 AIC · ⊞ 10K