feat(client): align default keepAliveInitialDelay and commandTimeout#3292
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
PavelPashov
left a comment
PavelPashov
left a comment
There was a problem hiding this comment.
We need to update the docs as well and does it make sense to create a const file for all defaults?
nkaradzhov
force-pushed
the
default-values
branch
from
7ba2251 to
cdec8b4
Compare
Bump TCP keep-alive idle to 30s and default commandOptions.timeout to 5s across client, cluster, and sentinel constructors, per the default client library values proposal. User-supplied commandOptions are merged so partial overrides still inherit the timeout default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…add regression tests
RedisClient.withCommandOptions(opts) set `proxy._commandOptions = opts`
as an own property, shadowing the prototype's merged options and dropping
the 5s default timeout introduced for v6. Merge with the current
effective options so partial overrides (e.g. `{ asap: true }`) still
inherit the default — matching the documented v5→v6 contract.
Lock the v6 default-commandOptions contract in with unit tests across
createClient, createCluster, and createSentinel (default applied,
partial-override merge, explicit opt-out via `timeout: undefined`), plus
a withCommandOptions regression case. Add socket-level coverage that
stubs `net.createConnection` to assert `keepAliveInitialDelay: 30000`
flows through and that a user override is honored verbatim.
Fix two preexisting sentinel spec assertions that compared
`sentinel.commandOptions` by reference against the user-supplied
options object — the constructor now merges defaults in, so the
reference equality no longer holds. Switched to property-level checks
that prove the same propagation/no-mutation intent.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nkaradzhov
force-pushed
the
default-values
branch
from
cdec8b4 to
7949851
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 7949851. Configure here.
|
|
||
| if (options?.commandOptions) { | ||
| this._commandOptions = options.commandOptions; | ||
| } |
There was a problem hiding this comment.
Cluster withCommandOptions drops default timeout by not merging
High Severity
The cluster's withCommandOptions sets proxy._commandOptions = options without merging the existing _commandOptions, unlike the client which was correctly updated to { ...this._commandOptions, ...options }. This means calling cluster.withCommandOptions({ asap: true }) silently drops the new default timeout: 5000 because the proxy's own _commandOptions property ({ asap: true }) shadows the constructor-set value on the prototype. The cluster has no commandOptions getter to merge at access time (unlike sentinel, which is fine), so the default timeout is permanently lost for all commands dispatched through the proxy.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7949851. Configure here.
2 participants
Bump TCP keep-alive idle to 30s and default commandOptions.timeout to 5s across client, cluster, and sentinel constructors, per the default client library values proposal. User-supplied commandOptions are merged so partial overrides still inherit the timeout default.
Description
Checklist
npm testpass with this change (including linting)?Note
Medium Risk
Breaking default behavior for upgrades: queued commands can now fail after 5s unless opted out, and idle keep-alive probe timing changes; impact is broad but mitigated by documented explicit v5 settings.
Overview
v6 default connection and command behavior shifts in two places: TCP/TLS
keepAliveInitialDelaygoes from 5s to 30s, andcommandOptions.timeoutdefaults to 5000 ms (previously no default). Shared constants live inpackages/client/lib/defaults.tsand are applied inRedisSocket(TCP/TLS only) and at construction for standalone client, cluster, and sentinel.Partial
commandOptions(e.g.{ asap: true }) still pick up the 5s timeout via merge.timeout: undefined(or0per docs) opts out. The standalone client’swithCommandOptionsnow merges into existing options so chained proxies keep the default timeout (regression fix covered by new tests). Cluster_executebuilds command options with{ ...options, slotNumber }instead of mutating a shared object.Docs update
client-configuration.md,command-options.md, andv5-to-v6.mdwith the migration table and explicit v5-preservation snippet. Tests assert defaults, merge/opt-out, keepalive passed tonet.createConnection, and proxy behavior.Reviewed by Cursor Bugbot for commit 7949851. Bugbot is set up for automated code reviews on this repo. Configure here.