Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR removes the workerdExperimental gate from the cf.cacheControl feature, making cacheControl synthesis, mutual-exclusion validation, and cf-blob dual-writes unconditional for all workers.
Issues (ranked by severity)
-
[High] Ungated backward-incompatible change -- new
TypeErrorfor previously-accepted inputs. TheRequestInitializerDict::validatechanges introduceTypeErrorthrows for workers that passcf.cacheControltogether withcf.cacheTtlor thecacherequest option. Previously (without the--experimentalflag), these combinations were silently accepted. Any existing worker that happens to setcf.cacheControlalongsidecf.cacheTtlwill now break atRequestconstruction time. Per the project's ownworkerdExperimentaldocumentation incompatibility-date.capnp(lines 266-269): "Don't use this for backwards-incompatible changes; give them their own flag." And perAGENTS.md: "Strong backwards compatibility commitment -- features cannot be removed or changed once deployed." This validation should be gated behind a dedicated compat flag (e.g.,cf_cache_control_validation) with a future enable date, rather than being unconditionally enabled. -
[Medium] Ungated wire-format change -- new
cacheControlfield in serialized cf blob for all workers. Removing the gate means every worker usingcacheTtlor thecacherequest option now gets an additionalcacheControlfield in the serialized cf JSON blob (the dual-write path). While the old fields are preserved, this changes the downstream wire format for existing workers that never opted into this behavior. If downstream infrastructure is already consumingcacheControl(as suggested by the superseded PR requiring a specific pingora-origin version), unconditionally enabling this for all workers without a staged rollout via compat flag could cause issues for workers on older infrastructure versions.
Both of these changes warrant their own compat flag per the project's conventions. The workerdExperimental flag was the correct development-time gate, but the path from experimental to GA should go through a dedicated compat flag, not straight to unconditional.
|
Review posted successfully on PR #6630. Here's a summary of my findings: Two issues flagged:
Both changes should go through a dedicated compat flag rather than being unconditionally enabled. |
27f8751 to
e726fe2
Compare
Supersedes #6623