Files
bun.sh/test/js/valkey/valkey.test.ts
Marko Vejnovic e3e8d15263 Fix redis reconnecting (#21724)
### What does this PR do?

This PR fixes https://github.com/oven-sh/bun/issues/19131.

I am not 100% certain that this fix is correct as I am still nebulous
regarding some decisions I've made in this PR. I'll try to provide my
reasoning and would love to be proven wrong:

#### Re-authentication

- The `is_authenticated` flag needs to be reset to false. When the
lifecycle reaches a point of attempting to connect, it sends out a
`HELLO 3`, and receives a response. `handleResponse()` is fired and does
not correctly handle it because there is a guard at the top of the
function:

```zig
if (!this.flags.is_authenticated) {
    this.handleHelloResponse(value);

    // We've handled the HELLO response without consuming anything from the command queue
    return;
}
```

Rather, it treats this packet as a regular data packet and complains
that it doesn't have a promise to associate it to. By resetting the
`is_authenticated` flag to false, we guarantee that we handle the `HELLO
3` packet as an authentication packet.

It also seems to make semantic sense since dropping a connection implies
you dropped authentication.

#### Retry Attempts

I've deleted the `retry_attempts = 0` in `reconnect()` because I noticed
that we would never actually re-attempt to reconnect after the first
attempt. Specifically, I was expecting `valkey.zig:459` to potentially
fire multiple times, but it only ever fired once. Removing this reset to
zero caused successful reattempts (in my case 3 of them).

```zig
        debug("reconnect in {d}ms (attempt {d}/{d})", .{ delay_ms, this.retry_attempts, this.max_retries });
```

I'm still iffy on whether this is necessary, but I think it makes sense.
```zig
        this.client.retry_attempts = 0
```

### How did you verify your code works?

I have added a small unit test. I have compared mainline `bun`, which
fails that test, to this fix, which passes the test.

---------

Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
2025-08-22 12:08:42 -07:00

200 lines
7.1 KiB
TypeScript

import { randomUUIDv7, RedisClient } from "bun";
import { beforeEach, describe, expect, test } from "bun:test";
import { ConnectionType, createClient, ctx, DEFAULT_REDIS_URL, expectType, isEnabled } from "./test-utils";
describe.skipIf(!isEnabled)("Valkey Redis Client", () => {
beforeEach(() => {
if (ctx.redis?.connected) {
ctx.redis.close?.();
}
ctx.redis = createClient(ConnectionType.TCP);
});
describe("Basic Operations", () => {
test("should set and get strings", async () => {
const redis = ctx.redis;
const testKey = "greeting";
const testValue = "Hello from Bun Redis!";
// Using direct set and get methods
const setResult = await redis.set(testKey, testValue);
expect(setResult).toMatchInlineSnapshot(`"OK"`);
const setResult2 = await redis.set(testKey, testValue, "GET");
expect(setResult2).toMatchInlineSnapshot(`"Hello from Bun Redis!"`);
// GET should return the value we set
const getValue = await redis.get(testKey);
expect(getValue).toMatchInlineSnapshot(`"Hello from Bun Redis!"`);
});
test("should test key existence", async () => {
const redis = ctx.redis;
// Let's set a key first
await redis.set("greeting", "test existence");
// EXISTS in Redis normally returns integer 1 if key exists, 0 if not
// The current implementation doesn't transform exists correctly yet
const exists = await redis.exists("greeting");
expect(exists).toBeDefined();
// Should be true for existing keys (fixed in special handling for EXISTS)
expect(exists).toBe(true);
// For non-existent keys
const randomKey = "nonexistent-key-" + randomUUIDv7();
const notExists = await redis.exists(randomKey);
expect(notExists).toBeDefined();
// Should be false for non-existing keys
expect(notExists).toBe(false);
});
test("should increment and decrement counters", async () => {
const redis = ctx.redis;
const counterKey = "counter";
// First set a counter value
await redis.set(counterKey, "10");
// INCR should increment and return the new value
const incrementedValue = await redis.incr(counterKey);
expect(incrementedValue).toBeDefined();
expect(typeof incrementedValue).toBe("number");
expect(incrementedValue).toBe(11);
// DECR should decrement and return the new value
const decrementedValue = await redis.decr(counterKey);
expect(decrementedValue).toBeDefined();
expect(typeof decrementedValue).toBe("number");
expect(decrementedValue).toBe(10);
});
test("should manage key expiration", async () => {
const redis = ctx.redis;
// Set a key first
const tempKey = "temporary";
await redis.set(tempKey, "will expire");
// EXPIRE should return 1 if the timeout was set, 0 otherwise
const result = await redis.expire(tempKey, 60);
// Using native expire command instead of send()
expect(result).toMatchInlineSnapshot(`1`);
// Use the TTL command directly
const ttl = await redis.ttl(tempKey);
expectType<number>(ttl, "number");
expect(ttl).toBeGreaterThan(0);
expect(ttl).toBeLessThanOrEqual(60); // Should be positive and not exceed our set time
});
test("should implement TTL command correctly for different cases", async () => {
const redis = ctx.redis;
// 1. Key with expiration
const tempKey = "ttl-test-key";
await redis.set(tempKey, "ttl test value");
await redis.expire(tempKey, 60);
// Use native ttl command
const ttl = await redis.ttl(tempKey);
expectType<number>(ttl, "number");
expect(ttl).toBeGreaterThan(0);
expect(ttl).toBeLessThanOrEqual(60);
// 2. Key with no expiration
const permanentKey = "permanent-key";
await redis.set(permanentKey, "no expiry");
const noExpiry = await redis.ttl(permanentKey);
expect(noExpiry).toMatchInlineSnapshot(`-1`); // -1 indicates no expiration
// 3. Non-existent key
const nonExistentKey = "non-existent-" + randomUUIDv7();
const noKey = await redis.ttl(nonExistentKey);
expect(noKey).toMatchInlineSnapshot(`-2`); // -2 indicates key doesn't exist
});
});
describe("Connection State", () => {
test("should have a connected property", () => {
const redis = ctx.redis;
// The client should expose a connected property
expect(typeof redis.connected).toBe("boolean");
});
});
describe("RESP3 Data Types", () => {
test("should handle hash maps (dictionaries) as command responses", async () => {
const redis = ctx.redis;
// HSET multiple fields
const userId = "user:" + randomUUIDv7().substring(0, 8);
const setResult = await redis.send("HSET", [userId, "name", "John", "age", "30", "active", "true"]);
expect(setResult).toBeDefined();
// HGETALL returns object with key-value pairs
const hash = await redis.send("HGETALL", [userId]);
expect(hash).toBeDefined();
// Proper structure checking when RESP3 maps are fixed
if (typeof hash === "object" && hash !== null) {
expect(hash).toHaveProperty("name");
expect(hash).toHaveProperty("age");
expect(hash).toHaveProperty("active");
expect(hash.name).toBe("John");
expect(hash.age).toBe("30");
expect(hash.active).toBe("true");
}
});
test("should handle sets as command responses", async () => {
const redis = ctx.redis;
// Add items to a set
const setKey = "colors:" + randomUUIDv7().substring(0, 8);
const addResult = await redis.send("SADD", [setKey, "red", "blue", "green"]);
expect(addResult).toBeDefined();
// Get set members
const setMembers = await redis.send("SMEMBERS", [setKey]);
expect(setMembers).toBeDefined();
// Check if the response is an array
expect(Array.isArray(setMembers)).toBe(true);
// Should contain our colors
expect(setMembers).toContain("red");
expect(setMembers).toContain("blue");
expect(setMembers).toContain("green");
});
});
describe("Connection Options", () => {
test("connection errors", async () => {
const url = new URL(DEFAULT_REDIS_URL);
url.username = "badusername";
url.password = "secretpassword";
const customRedis = new RedisClient(url.toString());
expect(async () => {
await customRedis.get("test");
}).toThrowErrorMatchingInlineSnapshot(`"WRONGPASS invalid username-password pair or user is disabled."`);
});
});
describe("Reconnections", () => {
test("should automatically reconnect after connection drop", async () => {
const TEST_KEY = "test-key";
const TEST_VALUE = "test-value";
const valueBeforeStart = await ctx.redis.get(TEST_KEY);
expect(valueBeforeStart).toBeNull();
// Set some value
await ctx.redis.set(TEST_KEY, TEST_VALUE);
const valueAfterSet = await ctx.redis.get(TEST_KEY);
expect(valueAfterSet).toBe(TEST_VALUE);
await ctx.restartServer();
const valueAfterStop = await ctx.redis.get(TEST_KEY);
expect(valueAfterStop).toBe(TEST_VALUE);
});
});
});