[TEST-FAILURE] reply-schemas-validator failing in daily test

[sarthakaggarwal97]

Summary

reply-schemas-validator is consistently failing in the daily test.
It looks to be happening after the change #2329

Error stack trace

multiprocessing.pool.RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/usr/lib/python3.12/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
                    ^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/multiprocessing/pool.py", line 48, in mapstar
    return list(map(*args))
           ^^^^^^^^^^^^^^^^
  File "/home/runner/work/valkey/valkey/./utils/req-res-log-validator.py", line 202, in process_file
    req = Request(fakefile, docs, line_counter)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/valkey/valkey/./utils/req-res-log-validator.py", line 85, in __init__
    length = int(line)
             ^^^^^^^^^
ValueError: invalid literal for int() with base 10: '-MOVED 5282 127.0.0.1:22115\r\n'
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/runner/work/valkey/valkey/./utils/req-res-log-validator.py", line 337, in <module>
    for result in pool.map(func, paths):
                  ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/multiprocessing/pool.py", line 367, in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/multiprocessing/pool.py", line 774, in get
    raise self._value
ValueError: invalid literal for int() with base 10: '-MOVED 5282 127.0.0.1:22115\r\n'

[hanxizh9910]

i am looking at this issue

[sarthakaggarwal97]

@zuiderkwast I am looking for some advice here. I think after #2329 we changed the semantics where blocking clients could return MOVED errors upon failovers / topology changes. I think due to this, reply-schemas-validator, which was expecting an integer, has started to fail on -MOVED responses due to parse errors.

Do you think we can handle this gracefully by skipping the MOVED responss?

[sarthakaggarwal97]

Discussed this issue with @madolson offline, I will be raising a change soon to skip the MOVED error with which are observing parsing issues.

[zuiderkwast]

Actually, I found that the client actually receives two MOVED reposponses for one request. This bug is worse than what we thought.

I verified by checking for another MOVED response and the test still passes:

diff --git a/tests/unit/cluster/replica-redirect.tcl b/tests/unit/cluster/replica-redirect.tcl
index ac9c8bd30..70fba8fe4 100644
--- a/tests/unit/cluster/replica-redirect.tcl
+++ b/tests/unit/cluster/replica-redirect.tcl
@@ -38,6 +38,7 @@ start_cluster 1 1 {tags {external:skip cluster}} {
 
         # Check that the client blocking on the old primary was MOVED to the new primary.
         assert_error "MOVED *" {$rd0 read}
+        assert_error "MOVED *" {$rd0 read}
 
         # Check that the readonly client blocking on the old primary is still blocked.
         assert_equal 1 [s 0 blocked_clients]

I have a fix in this PR:

• Fix double MOVED reply on unblock at failover #2734