Conversation
ATS 9.2 split "/path;params" into separate path and params components and hashed them as path + ";" + params. That parsing was removed, so ";params" now stays inside the path, yet the 9.2 hasher still appended a separator. A path such as "/a;b=1" hashed as "/a;b=1;", a key 9.2 never produced, so the compatibility lookup could not find objects a 9.2 cache stored for such paths. Only add the separator when the path does not carry one, and pin the equivalence with unit tests. The fast hash path had the same separator baked in, which made it a 9.2 implementation rather than a canonical one: enabling it through url_hash_method would have made canonical keys collide with 9.2 keys and tripped the debug parity assert on the first request. Rename it to url_CryptoHash_get_fast_92 and leave it reachable only from url_CryptoHash_get_92, so the invariant holds by construction instead of by the option staying off.
traeak
force-pushed
the
92x_compat_key_semicolon
branch
from
September 17, 2026 21:01
408fa6e to
cf3d8e0
Compare
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The compatibility logic and literal versus escaped semicolon cases are covered consistently without unresolved correctness issues.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
traeak
marked this pull request as ready for review
September 17, 2026 21:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow on to #12271 and #12283
ATS 9.2 split
/path;paramsinto separate path and params parts and built the cache key as path +;+ params. That parsing was removed, so;paramsnow stays in the path. The 9.2 key function still added its own;, so/a;b=1was hashed as/a;b=1;. 9.2 never produced that key, so lookups for these URLs could never find objects that 9.2 had cached.This PR adds the separator only when the raw path has no literal
;. An escaped%3Bstill counts as no params, just as in 9.2.The fast hash path emitted the same separator, so itg only ever produced 9.2 keys: it is renamed `url_Crypto_get_fast_92() and confined to url_CryptoHash_get_92(), leaving the canonical has on the general algorithm.