Skip to content

CACHE-13371: more cache knobs under request.cf object#6365

Open
tewaro wants to merge 1 commit intomainfrom
yawar/CACHE-13371_add-cf-cache-control-field
Open

CACHE-13371: more cache knobs under request.cf object#6365
tewaro wants to merge 1 commit intomainfrom
yawar/CACHE-13371_add-cf-cache-control-field

Conversation

@tewaro
Copy link
Contributor

@tewaro tewaro commented Mar 19, 2026

Supersedes #6300.

@tewaro tewaro requested review from a team as code owners March 19, 2026 19:35
Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This PR adds cacheControl and several other cache-related fields to the request.cf object, gated behind the workerdExperimental flag, with mutual-exclusion validation and synthesis of cacheControl from cacheTtl/cacheMode.

Issues found (ranked by severity):

  1. [HIGH] Negative cacheTtl (other than -1) produces an RFC-invalid max-age value (e.g., max-age=-5).
  2. [MEDIUM] Non-integer cacheTtl (e.g., 3.5) silently skips cacheControl synthesis, breaking the dual-write contract.
  3. [MEDIUM] CacheMode::RELOAD does not synthesize any cacheControl value — downstream consumers migrating to cacheControl won't see the bypass intent.
  4. [LOW] Synthesis tests only verify request construction, not the actual serialized cacheControl output.
  5. [LOW] Copyright year says 2025, should be 2026.

This review was generated by an AI assistant and may contain inaccuracies. Please verify all suggestions before applying.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 19, 2026

Review posted on PR #6365 with 5 findings:

  1. [HIGH] Negative cacheTtl values other than -1 produce RFC-invalid max-age directives (e.g., max-age=-5) — includes a suggested fix to handle negative values and non-integer floats.
  2. [MEDIUM] Non-integer cacheTtl (e.g., 3.5) silently skips synthesis, breaking the dual-write contract.
  3. [MEDIUM] CacheMode::RELOAD doesn't synthesize any cacheControl value, leaving downstream consumers blind to the bypass intent.
  4. [LOW] Synthesis tests don't actually verify the serialized output — they only check request construction roundtrips.
  5. [LOW] Copyright year is 2025 instead of 2026.

The feature gating behind workerdExperimental is appropriate, and the TypeScript type definitions and mutual-exclusion validation logic look correct. The new properties (cacheReserveEligible, respectStrongEtag, stripEtags, stripLastModified, cacheDeceptionArmor, cacheReserveMinimumFileSize) are pass-through on the cf blob and don't require C++ logic changes, which is fine.

github run

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 19, 2026

Merging this PR will improve performance by 13%

⚡ 1 improved benchmark
✅ 69 untouched benchmarks
⏩ 129 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
Encode_ASCII_32[TextEncoder][0/0/32] 3.5 ms 3.1 ms +13%

Comparing yawar/CACHE-13371_add-cf-cache-control-field (dd099be) with main (2565b03)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@tewaro tewaro enabled auto-merge March 19, 2026 20:49
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.

3 participants