Fix socket listener connection error callback_idx assertion failure - #6527
Fix socket listener connection error callback_idx assertion failure#6527xShinnRyuu wants to merge 1 commit into
Conversation
When client creation fails in listen_on_client_stream, the error response
was sent with callback_idx = u32::MAX (4294967295) instead of the expected
callback_idx = 0. This causes an assertion failure in tests when the
connection to the standalone server fails transiently:
assertion `left == right` failed
left: 4294967295
right: 0
The connection request does not carry a callback_idx field; the protocol
convention is that the response always uses index 0. The success path in
create_client correctly used 0, but the error paths in
listen_on_client_stream used u32::MAX.
This commit:
1. Introduces CONNECTION_RESPONSE_CALLBACK_IDX constant (= 0) and uses it
consistently in both the success and error paths for connection responses.
2. Adds a connection retry strategy (3 retries with exponential backoff) to
the socket listener test connect_to_redis helper, so transient server
startup delays in CI do not cause immediate connection failures.
Fixes valkey-io#6434
|
The single commit in this PR is missing the required |
There was a problem hiding this comment.
I found one remaining test-side issue in the socket-listener fix: the new retry budget is still much smaller than the server-startup race this helper is trying to absorb, so the flake can still reappear on slower CI. I also left a separate issue comment for the missing DCO/conventional-commit commit message requirements.
| use_tls: use_tls.to_bool(), | ||
| cluster_mode, | ||
| request_timeout: Some(REQUEST_TIMEOUT_MS), | ||
| connection_retry_strategy: Some( |
There was a problem hiding this comment.
These three retries only shrink the startup race instead of making the fixture deterministic. In the failure mode from #6434, the client creation path feeds this config straight into RetryStrategy::new() (glide-core/src/client/standalone_client.rs:162-168), and with { exponent_base: 2, factor: 2, number_of_retries: 3 } that bounded iterator is only 4/8/16ms (glide-core/redis-rs/redis/src/retry_strategies.rs:57-68,124-136). The rest of the test suite already treats RedisServer::new_*() as asynchronous startup and waits for readiness (glide-core/tests/test_client.rs:84-97, glide-core/tests/utilities/mod.rs:535-568), so if CI takes longer than ~28ms to accept connections this helper still fails before the server is ready. Using the existing readiness helper here would remove the flake without changing the client configuration under test.
Summary
Fixes #6434
When client creation fails in
listen_on_client_stream, the error response was sent withcallback_idx = u32::MAX(4294967295) instead of the expectedcallback_idx = 0. This caused an assertion failure in 14 socket_listener standalone cluster tests when the connection to the standalone server failed transiently in CI:Root Cause
The connection request proto (
ConnectionRequest) does not carry acallback_idxfield. The protocol convention is that connection responses always use index 0. The success path increate_clientcorrectly used0, but the error paths inlisten_on_client_streamusedu32::MAX.Changes
CONNECTION_RESPONSE_CALLBACK_IDXconstant (= 0) insocket_listener.rsand uses it consistently in both the success and error paths for connection responses.connect_to_redishelper, so transient server startup delays in CI do not cause immediate connection failures.Testing
cargo check --features socket-layerpasses (library and tests compile)4294967295 != 0cannot recur.