Fixes #3366 #3368
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,270 @@ | ||
| /** | ||
| * Test case for Issue #3256: MOVED errors with corrupted shard connections | ||
| * | ||
| * This test demonstrates the deadlock scenario where: | ||
| * 1. A shard connection gets corrupted | ||
| * 2. MOVED errors are returned consistently | ||
| * 3. Rediscover doesn't fix it because connections are reused | ||
| * 4. The fix should force connection recreation on MOVED | ||
| */ | ||
|
|
||
| import { strict as assert } from 'node:assert'; | ||
|
|
||
| describe('Cluster MOVED Error Recovery', () => { | ||
| describe('Issue #3256: Corrupted Shard Connection Recovery', () => { | ||
| it('should recreate shard connections when MOVED errors persist', async () => { | ||
| /** | ||
| * SCENARIO: | ||
| * 1. Create a cluster client | ||
| * 2. Mock a shard connection to return MOVED errors | ||
| * 3. Call a command that triggers rediscover | ||
| * 4. Verify the corrupted connection is destroyed and recreated | ||
| * 5. Verify subsequent commands work | ||
| */ | ||
|
|
||
| // This test would require: | ||
| // - Mocking RedisClusterSlots | ||
| // - Simulating a node that returns MOVED errors | ||
| // - Tracking connection creation/destruction | ||
| // - Verifying forceRefresh parameter is passed to #discover | ||
|
|
||
| assert.ok(true); // Placeholder | ||
| }); | ||
|
|
||
| it('should force refresh connections on MOVED error from non-readonly command', async () => { | ||
| /** | ||
| * Tests the specific path at index.ts:505-529 | ||
| * When a regular command gets MOVED error: | ||
| * 1. Extract node address from error | ||
| * 2. Try to find node in topology | ||
| * 3. If topology unchanged, force refresh flag should be true | ||
| * 4. Connection should be recreated, not reused | ||
| */ | ||
| }); | ||
|
|
||
| it('should force refresh connections on MOVED error from sharded pub/sub', async () => { | ||
| /** | ||
| * Tests the specific path at index.ts:649-650 | ||
| * When a sharded pub/sub command gets MOVED error: | ||
| * 1. Should trigger rediscover with forceRefresh=true | ||
| * 2. Existing pub/sub client should be destroyed | ||
| * 3. New pub/sub client should be created | ||
| */ | ||
| }); | ||
|
|
||
| it('should not recreate healthy connections unnecessarily', async () => { | ||
| /** | ||
| * Verify that: | ||
| * 1. Normal rediscover (not from MOVED) doesn't force refresh | ||
| * 2. If topology changes, connections are recreated only for new nodes | ||
| * 3. forceRefresh=false by default maintains current behavior | ||
| */ | ||
| }); | ||
|
|
||
| it('should handle multiple MOVED errors from different shards', async () => { | ||
| /** | ||
| * SCENARIO: | ||
| * 1. Two shards both return MOVED errors | ||
| * 2. Each triggers rediscover | ||
| * 3. Both should have connections recreated | ||
| * 4. Concurrent operations should succeed after recovery | ||
| */ | ||
| }); | ||
|
|
||
| describe('Connection Reuse Behavior', () => { | ||
| it('should reuse existing node if address matches and forceRefresh=false', async () => { | ||
| /** | ||
| * BASELINE: Current behavior that causes the deadlock | ||
| * Verify that when: | ||
| * 1. Topology is unchanged | ||
| * 2. forceRefresh is false (default) | ||
| * 3. Existing node is returned from nodeByAddress | ||
| */ | ||
| }); | ||
|
|
||
| it('should destroy and recreate node if forceRefresh=true', async () => { | ||
| /** | ||
| * PROPOSED FIX: Force refresh behavior | ||
| * Verify that when: | ||
| * 1. forceRefresh is true | ||
| * 2. Existing node exists for same address | ||
| * 3. Old connection is destroyed | ||
| * 4. New connection is created | ||
| */ | ||
| }); | ||
|
|
||
| it('should preserve node metadata while recreating connection', async () => { | ||
| /** | ||
| * When recreating a connection: | ||
| * 1. Keep node.address | ||
| * 2. Keep node.host, node.port | ||
| * 3. Keep node.id | ||
| * 4. Only recreate client and connectPromise | ||
| */ | ||
| }); | ||
| }); | ||
|
|
||
| describe('Rediscover Integration', () => { | ||
| it('should pass forceRefresh through discovery chain', async () => { | ||
| /** | ||
| * Verify that forceRefresh parameter: | ||
| * 1. Is accepted by #discover() | ||
| * 2. Is passed to #initiateSlotNode() | ||
| * 3. Does not affect #discoverWithRootNodes() | ||
| * 4. Does not affect #discoverWithKnownNodes() | ||
| */ | ||
| }); | ||
|
|
||
| it('should trigger forceRefresh only from MOVED error handlers', async () => { | ||
| /** | ||
| * forceRefresh should be set to true only when: | ||
| * 1. Handling MOVED error at index.ts:505-529 | ||
| * 2. Handling MOVED error at index.ts:649-650 | ||
| * | ||
| * Should remain false for: | ||
| * 1. Normal rediscover calls | ||
| * 2. Topology refresh triggered by reconnections | ||
| * 3. Node reconnection attempts | ||
| */ | ||
| }); | ||
|
|
||
| it('should handle concurrent rediscover with and without forceRefresh', async () => { | ||
| /** | ||
| * SCENARIO: | ||
| * 1. MOVED error triggers rediscover(forceRefresh=true) | ||
| * 2. Meanwhile, node reconnection triggers rediscover(forceRefresh=false) | ||
| * 3. Both should complete without conflicts | ||
| * 4. Connections should be properly cleaned up | ||
| */ | ||
| }); | ||
| }); | ||
|
|
||
| describe('Pub/Sub Specific', () => { | ||
| it('should handle pubSub connection in force refresh', async () => { | ||
| /** | ||
| * When forcing refresh on a master node: | ||
| * 1. Regular client should be destroyed | ||
| * 2. pubSub client should be destroyed | ||
| * 3. Both should be recreated if needed | ||
| * 4. Pub/Sub listeners should be resubscribed | ||
| */ | ||
| }); | ||
|
|
||
| it('should handle pubSubNode separately', async () => { | ||
| /** | ||
| * If pubSubNode exists: | ||
| * 1. Should be destroyed if not in addressesInUse | ||
| * 2. Should preserve listeners | ||
| * 3. Should trigger resubscription | ||
| */ | ||
| }); | ||
| }); | ||
|
|
||
| describe('Memory Cleanup', () => { | ||
| it('should remove destroyed clients from reconnection tracker', async () => { | ||
| /** | ||
| * When destroying a connection due to forceRefresh: | ||
| * 1. Call this.#reconnectionTracker.removeClient() | ||
| * 2. Prevent memory leaks from accumulated clients | ||
| * 3. Clean up connection pools | ||
| */ | ||
| }); | ||
|
|
||
| it('should not leak destroyed connections', async () => { | ||
| /** | ||
| * Verify that destroyed connections: | ||
| * 1. Are properly garbage collected | ||
| * 2. Release socket resources | ||
| * 3. Remove event listeners | ||
| */ | ||
| }); | ||
| }); | ||
|
|
||
| describe('Edge Cases', () => { | ||
| it('should handle rediscover during connection setup', async () => { | ||
| /** | ||
| * If forceRefresh=true during eagerConnect: | ||
| * 1. Should destroy pending connections | ||
| * 2. Should create new ones | ||
| * 3. Should handle promise race conditions | ||
| */ | ||
| }); | ||
|
|
||
| it('should handle missing nodes during force refresh', async () => { | ||
| /** | ||
| * If a node referenced in forceRefresh: | ||
| * 1. Doesn't exist in nodeByAddress (unexpected) | ||
| * 2. Should be created normally | ||
| * 3. Should not crash | ||
| */ | ||
| }); | ||
|
|
||
| it('should handle readonly replicas during force refresh', async () => { | ||
| /** | ||
| * When forcing refresh: | ||
| * 1. Master connections should be recreated | ||
| * 2. Replica connections should be recreated if useReplicas=true | ||
| * 3. readonly flag should be preserved | ||
| */ | ||
| }); | ||
|
|
||
| it('should handle minimizeConnections during force refresh', async () => { | ||
| /** | ||
| * When minimizeConnections=true: | ||
| * 1. Should not eager connect during eagerConnect phase | ||
| * 2. Should still recreate existing connections if forceRefresh=true | ||
| */ | ||
| }); | ||
| }); | ||
|
|
||
| describe('Command Execution After Recovery', () => { | ||
| it('should successfully execute commands after MOVED recovery', async () => { | ||
| /** | ||
| * INTEGRATION TEST: | ||
| * 1. Command fails with MOVED | ||
| * 2. Rediscover runs with forceRefresh=true | ||
| * 3. Connection is recreated | ||
| * 4. Command retries and succeeds | ||
| */ | ||
| }); | ||
|
|
||
| it('should handle max redirections with corrupted connections', async () => { | ||
| /** | ||
| * If maxCommandRedirections exceeded: | ||
| * 1. Should throw error (don't loop forever) | ||
| * 2. Should not attempt rediscover on final retry | ||
| * 3. Should provide helpful error message | ||
| */ | ||
| }); | ||
|
|
||
| it('should maintain command ordering after recovery', async () => { | ||
| /** | ||
| * After MOVED recovery: | ||
| * 1. Subsequent commands should execute in order | ||
| * 2. No command reordering | ||
| * 3. Transaction semantics preserved | ||
| */ | ||
| }); | ||
| }); | ||
|
|
||
| describe('Metrics and Observability', () => { | ||
| it('should emit debug events on force refresh', async () => { | ||
| /** | ||
| * When forceRefresh occurs: | ||
| * 1. Should emit a specific event or log | ||
| * 2. Should include node address | ||
| * 3. Should be useful for debugging | ||
| */ | ||
| }); | ||
|
|
||
| it('should track force refresh frequency', async () => { | ||
| /** | ||
| * To detect persistent issues: | ||
| * 1. Count force refresh occurrences | ||
| * 2. Alert if frequency is high | ||
| * 3. Help users detect infrastructure issues | ||
| */ | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Node's WHATWG URL parser,
redis://default:p@ssword@localhost:6379is valid: the@is treated as part of the userinfo and percent-encoded, leavinghostnameaslocalhost. As a resultRedisClient.parseURLreturns successfully here and thisassert.throwsfails whenever the client specs run; use a truly malformed unescaped credential character such as#/?, or assert successful parsing for@instead.Useful? React with 👍 / 👎.