Repository navigation
Read HELLO "server" before trusting "version" as the Redis API version - #3279
Merged
Merged
Conversation
#3278) Valkey reports its own version as HELLO "version" ("server" => "valkey", "version" => "9.1.2") while INFO still says redis_version:7.2.4; Garnet keeps "version" as the Redis API version ("server" => "redis") and adds "garnet_version". Both the interactive and subscription connections issue HELLO, so the detected version depended on whether the last reply processed was HELLO or INFO, and version-gated commands (DELEX for LockRelease) were sent to servers that don't have them. Now "version" only sets the Redis API version when "server" is redis; for a recognised non-Redis server it feeds the product variant instead, and for an unrecognised server it is ignored. A "<vendor>_version" HELLO field sets the product variant in the Garnet shape. The in-process test server can emulate both measured shapes, and a HELLO-only test (INFO disabled) pins the rule down on RESP2 and RESP3.
Parse it as a Version (no string) once we know "server" is redis; only the vendor path, which stores the string anyway, materializes it.
Adds Format.TryParseVersion(ReadOnlySpan<byte>): net10.0 has a built-in UTF-8 Version.TryParse; elsewhere a Utf8Parser loop over the dotted components, so no char transcoding and, downlevel, no string. The HELLO handler uses it via the function-pointer TryParseScalar overload; a parity test keeps it in step with the char/string overloads.
…d the gate normalisation System.Version treats "7.4" as (7, 4, -1, -1) and "7.4.0" as (7, 4, 0, -1); the UTF-8 overload must produce the same raw parts as the char/string overloads, and IsAtLeast/IsEqual must then treat the unspecified parts as zero.
The goal is identical results to the pre-existing char path for identical text, whatever System.Version does with it (-1 for unspecified parts, whitespace and sign tolerance per part), not a tidier parser. net10.0 fast-paths the built-in UTF-8 Version.TryParse; anything it rejects, and every other TFM, transcodes and calls the char overload. On .NET Framework that still allocates a string, which is acceptable. The parity test now covers the whitespace/sign cases.
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
1 participant
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.
Fixes #3278.
Problem
Since 3.4.0 the
HELLOreply has been a source of the server version, and both the interactive and subscription connections issueHELLO. The handler assignedversionstraight to the Redis API version, but the two conventions in the wild differ (measured against real servers):serverversionredis7.4.3(Redis API version)garnet_version => 2.2.2valkey9.1.2(Valkey's own version)redis_version:7.2.4On Valkey the detected version therefore depended on whether the last reply processed was
HELLO(9.1.2) orINFO(7.2.4), so version-gated commands (e.g.DELEXinLockRelease) were sometimes sent to a server that does not have them.Rule
Decided once the whole
HELLOmap has been read:server => redis(or absent):versionis the Redis API version; a<vendor>_versionkey, if present, sets the product variant (Garnet shape).serveris a recognised non-Redis name (valkey,redict, ...):versionis the vendor version and only feeds the product variant; the API version is left toINFO'sredis_version.serveris unrecognised:versionis ignored (logged). Conservative by design: ifINFOis unavailable too, the version falls back toDefaultVersionrather than guessing.HELLOnever touches the product variant, so it cannot clobberINFO's KeyDB detection (which comes fromexecutable, not a*_versionkey).Verification
HelloServerName,HelloVersionString,HelloExtraFields).ProductVariantUnitTestsandValkeyUnitTestsrun per protocol and assert the API version as well as the variant.DetectProductVariantFromHelloAlonedisablesINFOsoHELLOis the only source; it fails deterministically on RESP2 and RESP3 with the fix reverted. The test vendor version is now9.1.2rather than1.2.3-preview4, because an unparseable version was hiding the bug.Build.csprojRelease build withCI=trueis green.