Repository navigation
Conversation
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sderrow
changed the title
sderrow
changed the base branch from
sean/redis-characterization-tests
to
graphite-base/30
October 5, 2026 03:52
sderrow
force-pushed
the
graphite-base/30
branch
from
October 5, 2026 03:53
490dc27 to
f0d9071
Compare
sderrow
force-pushed
the
sean/remove-redis-option
branch
from
October 5, 2026 03:53
f8c0f7d to
3a9d43f
Compare
sderrow
force-pushed
the
sean/remove-redis-option
branch
from
October 5, 2026 03:53
3a9d43f to
dbb0586
Compare
RedisConnection and IORedisConnection now take { client, subscriber? }
instead of building clients from a Redis library, and are generic over the
client type: connection.client / .subscriber are typed from what's passed.
The client belongs to the consumer. The connection only sends commands on
it: no listeners, no setMaxListeners, no connect(), no ioredis
defineCommand script methods, and disconnect() never closes it. node-redis
clients must be connect()ed first (CLIENT_NOT_OPEN otherwise). The
subscriber the connection duplicates is its own: it connects, listens to,
and closes it, detached from the caller's async context. A consumer-supplied
subscriber is left open.
Both connections run scripts the same way: SHA1 computed locally, EVALSHA,
and EVAL on NOSCRIPT. No SCRIPT LOAD at startup, recovery after SCRIPT FLUSH,
and per-node caching on ioredis Cluster. ioredis keyPrefix still applies.
Transitional: limiters and Groups still accept Redis / clientOptions /
clusterNodes. ownedConnection() builds a client from them and hands it to
the connection as owned, so their behavior is unchanged until the next
commit removes those options.
NodeRedisClient / IORedisClient are exported as the client constraints.
Verified against ioredis 5/6 and redis 4/5/6 (tsc and tests).
Remove the Redis / client / clientOptions / clusterNodes / datastore options
from limiters and Groups: a Redis-backed limiter or Group takes a
connection built from the consumer's own client, and datastore is derived
from it. Passing a removed option throws LEGACY_REDIS_OPTIONS with a
migration hint; datastore: "local" is still accepted.
Nothing in Bottleneck owns a connection now. limiter.disconnect() only
leaves its connection; group.disconnect() disconnects its limiters. Neither
closes the connection. The connection's subscriber errors reach the
connection and every limiter using it, so limiter.on("error") still sees
pub/sub failures. Client errors belong to the client's owner.
Remove clients() and ClientsList (use connection.client / .subscriber).
ready() resolves to void. Drop the transitional ownedConnection shim.
A limiter now only unsubscribes channels it holds, and tolerates its
connection closing mid-UNSUBSCRIBE. Before, disconnecting a limiter whose
setup had failed hung on its dead subscriber.
Tests: the harness (test/bottleneck.ts) turns its datastore flag and test
clientOptions into a test-owned client and connection, so the step-0
characterization bodies are unchanged. Tests of the removed options are
replaced by LEGACY_REDIS_OPTIONS guards; the ownership pins now assert the
new contract. Harness node-redis clients close gracefully: destroy() surfaced
unhandled rejections from node-redis's own queue with none of Bottleneck's
commands in flight.
Verified on ioredis 5/6 and redis 4/5/6 (tsc and tests).
Connections are generic over the consumer's client and constrain it with structural types, so each supported client major (ioredis 5, redis 4/5) has to type-check, not just pass tests. Add tsc to the CI client-matrix job and to the matching local legs in pnpm test:ci.
Rewrite the Clustering docs around connections built from your own client: - Enabling Clustering, with ioredis and node-redis examples. - Managing Redis Connections: sharing, what Bottleneck does with your client, errors, and who closes what. - Updated Cluster/Sentinel and script-loading notes. Remove clients(), add the CLIENT_NOT_OPEN and LEGACY_REDIS_OPTIONS error codes, and add a v5 breaking-changes summary and an Upgrading to v5 guide.
sderrow
force-pushed
the
sean/remove-redis-option
branch
from
October 5, 2026 05:39
dbb0586 to
36e66e6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bottleneck used to build Redis clients itself from a
Redislibrary reference plusclientOptions/clusterNodes. That left the clients untyped, made cluster mode impossible to infer, and gave three different construction paths, each with its own rules about who closes what. It also changed clients that consumers passed in: it added error listeners (which hid unhandled errors), defined ioredis script methods on them, calledconnect(), and closed them on disconnect.This PR makes the connection explicit for v5. You build the client, wrap it in a connection, and pass that to limiters and Groups:
Key changes
RedisConnection/IORedisConnectiontake{ client, subscriber? }and are generic over the client type, soconnection.clientis typed from what you pass in. Limiters and Groups accept Redis only asconnection. The old options throwLEGACY_REDIS_OPTIONSwith a migration hint.connect()ed first (CLIENT_NOT_OPEN).connection.disconnect()only closes the subscriber it created.EVALfallback on both clients. There's noSCRIPT LOADat startup, limiters recover fromSCRIPT FLUSHand restarts, and it works per node on a Redis Cluster.clients()/ClientsList(use the connection).ready()now resolves toundefined.Testing
The characterization tests from the PR below this one pass unchanged, except the ones pinning behavior this PR changes on purpose; those are updated and flipped to assert the new contract. The client-matrix CI job now also runs
tscagainst each pinned client version, because the client types are now a public constraint. Verified on ioredis 5/6 and redis 4/5/6.The README has a v5 breaking-changes summary and an "Upgrading to v5" guide.