fix: isolate automation pubsub Redis client and recover stats consumption - #900
Conversation
seafile-dev
left a comment
seafile-dev
left a comment
There was a problem hiding this comment.
Verdict: Comment
Warnings:
- Test-automation requests that complete after ten minutes now fail although the previous fifteen-minute contract allowed them; see inline comment.
Suggested fix: retain the 15-minute deadline unless a coordinated product/API change explicitly requires a shorter timeout. - The Redis reconnect race is not covered by automated regression testing.
Suggested fix: add a focused test that reconnects the subscriber client whilestats()uses the command client, and assert that stats consumption continues.
The Redis client split correctly isolates refresh_subscriber() from normal queue commands; the PR's existing build is green.
|
|
||
| self._redis_client = RedisClient(socket_connect_timeout=5, socket_timeout=10, | ||
| health_check_interval=30, retry_on_timeout=True) | ||
| # Keep pubsub reconnects isolated from regular Redis commands. |
There was a problem hiding this comment.
[Warning] Missing reconnect regression test
Why this matters:
This fix depends on refresh_subscriber() reconnecting only the subscriber client while stats() continues using the command client. The current CI runs SQL tests only, so a future client refactor could silently reintroduce the thread-exit race.
Suggested fix: add a focused unit test that forces a subscriber refresh during stats queue consumption and verifies the command client can still pop and process a result.
| if done: | ||
| break | ||
| if time.time() - start_at > 15 * 60: | ||
| if time.time() - start_at > 10 * 60: |
There was a problem hiding this comment.
[Warning] Shortened test-task deadline
Why this matters:
This unrelated change makes a test automation that completes between 10 and 15 minutes report a timeout, whereas the previous behavior waited 15 minutes. The result can therefore complete successfully but the caller still receives a failure.
Suggested fix: keep the existing 15-minute timeout for this bug fix, or document and coordinate the new 10-minute API behavior with its callers.
3 participants
No description provided.