Skip to content

Make stale-sie-503 actually return a 503#173

Merged
mnot merged 1 commit into
mainfrom
fix/stale-sie-503
Jun 9, 2026
Merged

Make stale-sie-503 actually return a 503#173
mnot merged 1 commit into
mainfrom
fix/stale-sie-503

Conversation

@mnot

@mnot mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes #167: stale-sie-503 was a byte-for-byte behavioral duplicate of stale-sie-close — its second request used disconnect: true rather than returning a 503, so the stale-if-error-on-error-status case was never tested. Its second request now returns 503 Service Unavailable (mirroring the existing stale-503 test) and expects a 200 served from the stale cache.

Closes #167


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

stale-sie-503 was identical to stale-sie-close: its second request
disconnected instead of returning a 503, so the stale-if-error-on-
error-status path was never exercised. Return 503 like stale-503 does.

Closes #167

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

mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Review

Verified against the engine and the sibling tests. This is correct and does what #167 asks.

Scope — The diff is limited to the stale-sie-503 second request (lines 144-147). The adjacent stale-sie-close test is untouched, as intended.

The change is the right shape:

  • response_status: [503, 'Service Unavailable'] makes the origin actually return a 503 on the revalidation attempt (handle-test.mjs:58). That is the error condition that exercises stale-if-error — the whole point that disconnect: true was failing to test.
  • expected_status: 200 + expected_type: 'cached' is the correct combination. In test.mjs the status check is an if ('expected_status') ... else if ('response_status') chain (lines 126-135), so the presence of expected_status: 200 asserts the cache served a 200 and suppresses the would-be 503 assertion on response_status. expected_type: 'cached' then asserts the response came from the store (resNum < reqNum) and skips the server-state expectation. So the test now requires: origin says 503 → cache serves the stale stored 200. Exactly the stale-if-error-on-error-status behavior.

It genuinely differs from stale-sie-close now. stale-sie-close keeps disconnect: true (transport failure path); this test exercises the HTTP-error-status path. They are no longer behavioral duplicates.

pause_after / staleness — Already present on the setup request (line 142), and unchanged. max-age=2 with a 3000ms pause (utils.mjs:9) means the stored response is genuinely stale before the second request, so the served-stale path is actually traversed rather than a still-fresh hit. No concern here.

Consistency — The new shape mirrors the existing stale-503 test (lines 53-60) verbatim aside from the stale-if-error=60 on the setup, which is the correct distinguishing factor.

One minor nit, non-blocking: stale-503 builds its setup with templates.becomeStale({}) while stale-sie-503 spells out the headers inline (because it needs the extra stale-if-error directive). That asymmetry is unavoidable given the template doesn't take a CC override, so inlining is fine.

Verdict: LGTM — approve. The fix is minimal, correct, and restores the test's intended coverage.


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

@mnot mnot merged commit 930ba1d into main Jun 9, 2026
2 checks passed
@mnot mnot deleted the fix/stale-sie-503 branch June 9, 2026 04:13
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.

stale-sie-503 is a duplicate of stale-sie-close and never sends a 503

1 participant