improvement(redis): pair lock acquire failures with connection state - #7669
Conversation
A lock acquire is often a process's first Redis call, so an unusable connection surfaces there as `Error: Command timed out` — a rejection carrying only ioredis timer frames, no app frame, and nothing to separate a handshake still in flight from a socket that died silently. `status` is what separates them, so log it alongside the failure. Read before the reclaim, which awaits and would otherwise report the state it left behind rather than the one that failed.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR improves Redis lock-acquisition failure diagnostics by logging the state of the exact client that executed the failed command.
Confidence Score: 5/5The PR appears safe to merge, with no outstanding correctness, security, or repository-rule issues identified. The lock failure path preserves its existing rethrow and reclaim behavior while recording diagnostics from the failing client before asynchronous cleanup can change connection state. The previous replacement-client finding is fully addressed by passing the captured client and withholding globally tracked ages after client identity diverges.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/core/config/redis.ts | Adds failure-path lock diagnostics and correctly associates status with the Redis client that executed the command. |
| apps/sim/lib/core/config/redis.test.ts | Adds focused regression tests for diagnostic state, ordering, client replacement, and non-failure logging behavior. |
Reviews (2): Last reviewed commit: "fix(redis): describe the client that ran..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A command can outlive the client that issued it: the PING health check drops `state.client` after consecutive failures, which is the same unhealthy stretch in which that command is timing out. Reading the global in the failure path then described the replacement — reporting `no-client` or a fresh `connecting` for a failure belonging to the connection before it, misclassifying the very timeout the diagnostic exists to explain. Take the client as an argument, and withhold the ages when `state` no longer holds it rather than dating a connection its timestamps never measured.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
1 participant
Summary
Error: Command timed out— a rejection carrying only ioredis timer frames, no app frame, and nothing to say which of several very different causes applieddescribeRedisConnection()already exists for exactly this and is wired into the usage-reservation path; this extends it toacquireLock, which covers every lock callerstatusis what separates the cases:connecting/reconnectingmeans the command was parked waiting on connection setup,readymeans it went to a live socket that never answeredLogging only — no behavior change. The failure path is unchanged: same error, same rethrow, same reclaim. Nothing is logged on the taken or contended paths, which the poll routes run constantly.
Type of Change
Testing
Three tests added: the failure is paired with connection state, the state is read before the reclaim, and the success/contended paths stay quiet. Each verified to fail when its behavior is reverted — the ordering test goes red when the diagnostic read is moved after the reclaim.
bun run type-checkclean. 3,893 tests pass acrosslib/core/config,lib/table,lib/billing,background,lib/concurrency,lib/mcp,lib/workspace-files.bun run lint, block-registry check,docs-manifest:check, and all 46 audits incheck:auditspass.Checklist