redis: gate the socket keep-alive adopt and defer destruction to finalize - #34874
Closed
robobun wants to merge 7 commits into
Closed
redis: gate the socket keep-alive adopt and defer destruction to finalize#34874robobun wants to merge 7 commits into
robobun wants to merge 7 commits into
Claude / Claude Code Review
completed
Jul 21, 2026 in 17m 40s
Code review found 1 important issue
Found 1 candidates, confirmed 1. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 0 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | test/js/valkey/valkey-gc.test.ts:228-238 |
New GC test retains all clients, so finalize() never runs and the test likely passes on the parent commit |
Annotations
Check failure on line 238 in test/js/valkey/valkey-gc.test.ts
claude / Claude Code Review
New GC test retains all clients, so finalize() never runs and the test likely passes on the parent commit
The `clients` array is read after both `Bun.gc(true)` calls, so every wrapper stays reachable and `JSValkeyClient::finalize()` never runs — the comment "Drive GC so finalize() runs" describes something that cannot happen here. Combined with the PR's own note that every close/reconnect dispatch in this test has a matching `socket_refs` increment (i.e. the parent's unconditional adopt was also balanced on this path), the test very likely passes on the parent commit. Either drop the `clients` array
Loading