mirror of
https://github.com/oven-sh/bun
synced 2026-02-10 02:48:50 +00:00
e3e8d15263
### 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>