Skip to content

Make vary-3-order isolate header order#176

Open
mnot wants to merge 1 commit into
mainfrom
fix/vary-3-order
Open

Make vary-3-order isolate header order#176
mnot wants to merge 1 commit into
mainfrom
fix/vary-3-order

Conversation

@mnot

@mnot mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes #170: vary-3-order claimed to test matching "regardless of header order" but reordered the headers and changed Bar's value (abcde vs stored abc). The value mismatch alone forced not_cached, so ordering was never exercised — it was effectively a duplicate of vary-3-no-match.

This converts it into a real order-independence test: the request sends the stored values (Foo:1, Bar:abc, Baz:789) in a different order and expects reuse (cached). A cache that did order-sensitive secondary-key comparison would fail to match.

Design note / alternative: I chose to make this a positive order-independence test (expected cached, kind: optimal, renamed). The alternative would be to keep it a no-match test and simply drop the "regardless of header order" framing from the name — but that would leave it redundant with vary-3-no-match. Happy to switch to the conservative wording-only fix if preferred.

Closes #170


🤖 This PR was generated by an AI agent (Claude Code) under human supervision, as part of a maintainer-directed review of the test suite.

The test reordered headers but also changed Bar's value, so the value
mismatch (not the ordering) forced not_cached and order-independence
was never exercised. Send the stored values in a different order and
expect reuse, so an order-sensitive cache would fail to match.

Closes #170

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mnot

mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Review

I reviewed the diff against tests/vary.mjs, the surrounding vary-3-* tests, and the expected_type verification logic in test-engine/client/test.mjs. The diagnosis in #170 is correct, and the rewrite is a real improvement. A few points, including one I'd ask you to fix before merge and one genuine design call.

Correctness: expected_type: 'cached' is right

With Vary: Foo, Bar, Baz and the second request sending the identical values (Foo:1, Bar:abc, Baz:789) in a different order, the secondary cache key must match. Field order across distinct field names is not semantically significant (RFC 9110 §5.3), and §4.1's "selecting header fields... nominated by Vary, match" is a per-field-name value comparison with no ordering component. So a conformant cache reuses the response, and cached is the correct expectation. The engine verifies this as "the origin server did not see a second request" (resNum < reqNum in test.mjs:117), which is exactly the reuse semantics we want.

Does it actually isolate ordering? Yes

This is the key win. The old test changed Bar to abcde, so the value mismatch alone forced not_cached — ordering was never exercised and it duplicated vary-3-no-match. The new version holds all values constant and varies only order, so an order-sensitive secondary-key implementation is the only thing that fails it. And it's now meaningfully distinct from vary-3-match, which sends the same values in the stored order. Good.

One thing to fix: the dropped spec_anchors

The diff removes spec_anchors: ['caching.negotiated.responses'] from this test along with the rename. The other vary-3-* tests carry it (or inherit at suite level). Unless that's deliberate, please restore it so the test stays linked to the right spec section.

Design call: optimal vs required

I'd push back here, mildly. Order-insensitive matching isn't optional behavior — a cache that's order-sensitive is non-conformant, since the spec gives field order across distinct names no meaning. By that reasoning this is closer to required than optimal. The wrinkle: the positive sibling vary-3-match is itself optimal, and in this suite "reuse" tests are conventionally optimal because reuse is never mandatory — a cache is always free to revalidate or refetch. Given the engine treats required as a hard fail and optimal as a soft optional_fail (results.mjs:41-52), and that a cache declining to reuse here can't be called non-conformant, optimal is the defensible and consistent choice. I'd keep optimal to match vary-3-match, but it's worth a one-line comment noting that order-insensitivity is mandated even though reuse isn't — so a future reader doesn't mistake the soft scoring for "ordering doesn't matter."

Coverage loss: reorder-with-mismatch

The old test, despite its mislabeling, did exercise a no-match path (different Bar value, reordered). That's now gone. But it was never really testing ordering, and the no-match semantics are covered by vary-3-no-match (mismatched Foo, same order). The specific "mismatch and reordered" combination isn't covered anywhere now — but I don't think it earns its own test; ordering and value-matching are orthogonal and each is exercised independently. Not worth blocking on. If you want it, a separate small test is cleaner than overloading this one.

Scope

Touches only vary-3-order (plus the spec_anchors removal noted above). It does not disturb vary-3-match or vary-3-no-match. Good.

Recommendation

Take the positive-test rewrite (this PR), not the conservative wording-only alternative. The conservative fix would leave vary-3-order redundant with vary-3-no-match and would mean the suite still has zero coverage of order-independence — the rewrite fills a genuine gap. Two asks before merge: (1) restore spec_anchors; (2) keep kind: optimal but add a brief comment that order-insensitivity is mandated. Verdict: approve with those minor changes.


This is an AI-generated review (Claude Code), produced as part of a maintainer-directed review of the test suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vary-3-order does not isolate header order

1 participant