Repository navigation
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
@slukes Thanks for the PR. I’ll take a look and review it. In the meantime, please check why the new tests are failing. |
4bb325e to
0096700
Compare
|
Rebased onto latest
Converting back to draft until the new CI run is green. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 4 potential issues.
Reviewed by Cursor Bugbot for commit 0096700. Configure here.
| if (targetInstance !== this) { | ||
| debug("Routing command %s to read instance", command.name); | ||
| return targetInstance.sendCommand(command, stream); | ||
| } |
There was a problem hiding this comment.
Pipeline reads break reply routing
High Severity
When scaleReads routes a pipelined read to a replica sendCommand, the command is queued on that replica but pipeline bytes are still flushed to the primary socket via stream.destination.redis. Replies arrive on the primary while the replica’s queue waits, so pipeline and auto-pipelined reads can hang or resolve with wrong results.
Reviewed by Cursor Bugbot for commit 0096700. Configure here.
| }; | ||
|
|
||
| debug("Creating read instance for %s:%d", endpoint.host, readOptions.port); | ||
| return new Redis(readOptions); |
There was a problem hiding this comment.
Replica connections ignore lazyConnect
High Severity
Read replicas created from the scaleReads array always set lazyConnect: false, overriding the parent’s lazyConnect: true. Constructing the main client then opens every replica connection immediately, even when the app intended to defer all Redis I/O until connect().
Reviewed by Cursor Bugbot for commit 0096700. Configure here.
| }; | ||
|
|
||
| debug("Creating read instance for %s:%d", endpoint.host, readOptions.port); | ||
| return new Redis(readOptions); |
There was a problem hiding this comment.
Sentinel options leak to replicas
Medium Severity
Read clients copy the full parent RedisOptions via spread and only clear scaleReads. If the primary uses Sentinel (sentinels set), replicas still pick SentinelConnector instead of direct replica host/port, so explicit replica endpoints are ignored.
Reviewed by Cursor Bugbot for commit 0096700. Configure here.
| scaleReadsOption = this.options.scaleReads as string | ScaleReadsFunction; | ||
| } | ||
|
|
||
| this.readWriteRouter = new ReadWriteRouter(scaleReadsOption, readInstances); |
There was a problem hiding this comment.
String scaleReads has no replicas
Medium Severity
For non-array scaleReads values such as 'all', 'slave', or a custom function, initializeReadWriteRouter never populates readInstances, and there is no API to register replicas. ReadWriteRouter then always falls back to the primary for reads, so cluster-style string strategies do nothing on standalone.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0096700. Configure here.
This feature enables read/write splitting for non-cluster Redis setups,
particularly useful for ElastiCache read replicas. It uses the same
battle-tested routing logic as cluster mode for consistency.
Key features:
- Unified scaleReads API compatible with cluster mode
- Support for both endpoint arrays and cluster-style strategies
- Automatic read-only command detection and routing
- Graceful fallback to primary when read instances fail
- Round-robin load balancing across read replicas
Usage:
```js
const redis = new Redis({
host: 'primary.cache.amazonaws.com',
scaleReads: [
{ host: 'replica1.cache.amazonaws.com' },
{ host: 'replica2.cache.amazonaws.com' }
]
});
```
0096700 to
82f37a3
Compare
|
Rebased onto latest |
This branch has not been deployed
2 participants
Why
Enables read/write splitting for non-cluster Redis setups — primarily for ElastiCache read replicas but applicable to any topology with a primary + read replicas. Today,
scaleReadsis cluster-only; standalone users have no equivalent.How
Extracts the routing logic from cluster mode into a shared
ReadWriteRouterutility, then wires it into the standaloneRedisclass:lib/utils/ReadWriteRouter.ts— new shared class with the same read/write routing logic that cluster already uses (isReadOnly, round-robin,slave/all/masterstrategies, function-based routing)lib/Redis.ts— initializes aReadWriteRouterin the constructor; routes commands insendCommandbefore the existing offline-queue logic; disconnects read instances when the primary disconnectslib/redis/RedisOptions.ts— addsscaleReadsoption (array of endpoints, string strategy, or function) toCommonRedisOptionsArray format creates lazy-connecting read
Redisinstances; string/function format delegates to the router directly — same semantics as cluster.Changes
lib/utils/ReadWriteRouter.ts— new shared routing utility (extracted from cluster)lib/Redis.ts—initializeReadWriteRouter(), routing insendCommand, cleanup indisconnect()lib/redis/RedisOptions.ts—scaleReadsoption onCommonRedisOptionsexamples/read_write_splitting.js— usage examples (ElastiCache + local dev)test/functional/read_write_splitting.ts— functional tests (routing, round-robin, fallback, cleanup)test/unit/read_write_command_classification.ts— unit tests for read/write command detectionEvidence of Testing
Functional tests cover:
disconnect()scaleReads→ unchanged behavior)Review Focus
ReadWriteRouterextraction: does the abstraction feel right, or should the logic stay inline?sendCommandinsertion point (beforeblockingTimeout) — open to moving itscaleReads: 'slave'is the right alias for standalone (vs. something more neutral like'replica')This pull request was created with AI assistance.
Note
Medium Risk
Changes core
sendCommandrouting and opens extra connections; replica lag can surface stale reads, though writes and non-readonly commands still hit the primary.Overview
Standalone Redis now supports read/write splitting through the same
scaleReadsoption shape as cluster mode, aimed at primary + replica setups (e.g. ElastiCache).A new
ReadWriteRoutercentralizes routing: non–read-only commands stay on the primary; read-only commands (via@ioredis/commandsflags orcommand.isReadOnly) go to round-robin replicas whenscaleReadsis an endpoint array or'all'/'slave', with optional custom function selection. Replicas that are notreadyare skipped so reads fall back to the primary instead of hanging on dead connections.Rediswires this ininitializeReadWriteRouter()(arrayscaleReadsspawns childRedisconnections withreadOnly: trueand eager connect),sendCommandwhen status isready, anddisconnecttears down replica clients.CommonRedisOptionsdocuments the newscaleReadstypes. Addsexamples/read_write_splitting.jsplus functional and unit tests for routing, round-robin, fallback, and command classification.Reviewed by Cursor Bugbot for commit 0096700. Bugbot is set up for automated code reviews on this repo. Configure here.