Skip to content

Make conditional-etag-precedence actually test precedence#175

Merged
mnot merged 1 commit into
mainfrom
fix/conditional-etag-precedence
Jun 9, 2026
Merged

Make conditional-etag-precedence actually test precedence#175
mnot merged 1 commit into
mainfrom
fix/conditional-etag-precedence

Conversation

@mnot

@mnot mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes #169: in conditional-etag-precedence, both If-None-Match (matches the stored ETag) and If-Modified-Since (≈now, after the stored Last-Modified) independently evaluated to 304, so any evaluation order passed expected_status: 304 and a cache that gave If-Modified-Since precedence was not caught.

This moves If-Modified-Since to before Last-Modified (-10000 vs the response's -5000), so it alone would yield 200 while If-None-Match still yields 304. Only a cache that correctly gives If-None-Match precedence (RFC 9110 §13.2.2) returns the expected 304.

Closes #169


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

Both conditionals evaluated to 304, so any evaluation order passed and
a cache that ignored If-None-Match precedence was not caught. Move
If-Modified-Since before Last-Modified so it alone would yield 200,
while If-None-Match still yields 304; only a cache that honours the
If-None-Match precedence (RFC 9110 13.2.2) returns the expected 304.

Closes #169

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

mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Review: gives If-None-Match precedence over If-Modified-Since

I verified the conditional logic against the engine. The fix is correct and minimal.

Delta math checks out

  • Stored Last-Modified: -5000 is emitted by the origin as httpDate(server-now, -5000) = now − 5000s (test-engine/lib/utils.mjs httpDate, test-engine/server/handle-test.mjs).
  • The request's If-Modified-Since: -10000 with magic_ims: true is rewritten relative to the previous response's server-now via fixupHeader (tests/lib/templates.mjs inittest-engine/lib/header-fixup.mjs), giving now − 10000s. Both are anchored to the same setup response's server-now, so the 5000s gap is preserved.
  • I-M-S date (now−10000) is earlier than Last-Modified (now−5000) → the resource was modified after the I-M-S date → I-M-S alone evaluates to 200.
  • If-None-Match: "abcdef" matches the stored strong ETag → 304.

So the two conditionals now genuinely disagree (200 vs 304), which the old -1 value did not achieve. The test only passes if the cache gives I-N-M precedence and returns 304, exactly per RFC 9110 §13.2.2. Good.

expected_type: 'cached' still holds

Worth being explicit, since it's the crux: the server-side 304-vs-200 evaluation in handle-test.mjs (lines 59–71) only runs for expected_type ending in validated. This test is cached, so the origin is never consulted — the stored response is fresh (max-age=100000 from templates.fresh), and the cache must answer the conditional itself from the stored response. The change doesn't touch freshness, so cached remains correct and the origin is not (and should not be) forwarded to.

On false-failure risk — one honest caveat

The test asserts a single expected_status: 304 and is marked kind absent (i.e. a normative test, not check/optimal). Consider what a spec-compliant cache could legitimately do for a conditional GET on a fresh stored response:

  • A cache that evaluates conditionals and gives I-N-M precedence → 304. (Intended pass.)
  • A cache that gives I-M-S precedence → 200. (Intended, correct fail — this is the bug the test now catches.)
  • A cache that ignores request conditionals entirely on a fresh hit and serves the full stored 200 → 200. This is arguably permissible: RFC 9110 §13 frames conditional evaluation as a recipient behavior, and a cache MAY serve a fresh stored response without evaluating the client's conditionals (handling I-N-M precedence is a SHOULD-ish optimization, not a hard MUST for cached fresh responses). Such a cache would now fail this test even though it isn't violating the precedence rule per se — it simply never reaches a precedence decision.

This isn't a regression introduced by the PR: the pre-fix test had the same expected_type/expected_status shape and the same exposure (a conditional-ignoring cache would have returned 200 and failed before, too). The PR strictly improves discrimination without widening the false-failure surface. So I don't consider it a blocker. But the test name and the suite's framing should be read as "if the cache honors conditionals on a fresh hit, it must prefer I-N-M" — the result for a conditional-ignoring cache (a 200, scored as fail here) is a known limitation of expressing this as a single-status assertion, not something this PR makes worse.

If the maintainers want to be pedantically fair to conditional-ignoring caches, this could be kind: 'check' rather than a normative test, but that's a separate judgment call and out of scope for fixing #169.

Minimality

Changing the single I-M-S delta plus the explanatory comment is exactly sufficient. No other knob (freshness, ETag, the setup response) needed to move. The comment is accurate and useful.

Verdict

Approve. The fix makes the test actually exercise I-N-M-over-I-M-S precedence as #169 intended, the delta arithmetic is right, and expected_type: 'cached' is preserved. The one caveat (conditional-ignoring fresh caches score as a fail) is pre-existing and not worsened here.


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

@mnot mnot merged commit 7cb4b15 into main Jun 9, 2026
2 checks passed
@mnot mnot deleted the fix/conditional-etag-precedence branch June 9, 2026 04:15
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.

conditional-etag-precedence cannot detect reversed precedence

1 participant