fix(redis): detect an existing connection by shape instead of instanceof, and support ioredis 6 - #71
Open
DavideCarvalho wants to merge 3 commits into
Conversation
`RedisTransport` decided whether it received a live client or a plain options object with `instanceof Redis || instanceof Cluster`. That check is evaluated against the copy of `ioredis` this package resolved, so a perfectly valid client coming from another copy - a different major, or simply a duplicate in the dependency tree - fails it, and we fall through to `new Redis(<a live client treated as an options bag>)`. ioredis ignores the unrecognised properties and connects to 127.0.0.1:6379, silently. `useMessageBuffer` is a second casualty: it only reaches the transport through the third constructor argument, which the options-object branch ignores, so it degrades to `false` and binary payloads are decoded from a lossy string. Both call sites now go through a duck-type check on `duplicate` and `sendCommand`. Both are on the prototype of `Redis` and `Cluster` in every major, and neither name exists in `RedisOptions` or `ClusterOptions`, so an options object can never be misclassified. Values that carry the markers of a client (`status`, `options`, `emit`) without being recognised as one now throw instead of quietly dialing localhost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The whole `RedisTransport` surface was validated against ioredis 6.0.0 with RESP3 actually negotiated (`options.protocol === 3`, server-side `HELLO` agrees): publish/subscribe/unsubscribe, self-message filtering, the `messageBuffer` binary path (byte-identical round trip), the `duplicate()` path for an existing client, reconnection, and a real three-node Redis cluster. `Redis#duplicate` and `Cluster#duplicate` became generic over the reply mapping in ioredis 6, which made the call against a `Redis | Cluster` union uncompilable (TS2349); it now goes through a narrow local signature so the source typechecks and builds against both majors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The foreign-major client was simulated with a `Proxy` over a null-prototype target, which is faithful in shape but leaves open the fair question of whether it behaves like a real client of another major. A second, genuine copy of `ioredis` is now installed side by side through the npm alias `"ioredis-v6": "npm:ioredis@^6.0.0"`, so the three cross-major tests hand `RedisTransport` an actual ioredis 6 client - the exact shape of an application on `@adonisjs/redis@11` passing its connection to a bus resolved against ioredis 5. Each one asserts the premise at runtime (`instanceof ForeignRedis`, not `instanceof Redis`, and the two constructors are not the same object) before asserting the behaviour, so the tests cannot silently stop covering the bug. Delivery is still proven against the configured server through a witness connection bound to the testcontainers port, and the `useMessageBuffer` case now compares the bytes seen on the subscriber with the bytes the publisher wrote, on top of the `messageBuffer`/`message` listener counts. The `Proxy` helper is dropped: a real foreign client is a strictly harder case than a duplicated copy of the same major, and the "client-shaped object that is no ioredis at all" tripwire is already covered by its own test with a plain object literal. Reverting `src/transports/redis.ts` to the `instanceof` version turns the suite red on exactly these tests: 13 passed / 4 failed against 17 passed on the branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
The bug
RedisTransportdecides whether it was handed a live client or a plain options object withinstanceof:instanceofis evaluated against the copy ofioredisthis package resolved. When the host application resolves a different copy — a different major, or simply a duplicate in the dependency tree — the check isfalsefor a perfectly valid client, and we fall through tonew Redis(<a live client treated as an options bag>). ARedisinstance has none ofhost/port/pathas own properties, so ioredis keeps its defaults and connects to127.0.0.1:6379, silently, with no error.There is a second symptom, specific to this package.
useMessageBufferonly reaches the transport through the third constructor argument, which the options-object branch ignores — it readsoptions.useMessageBufferinstead, which does not exist on a client. So the subscriber quietly listens onmessageinstead ofmessageBuffer, and binary payloads come back through a lossy string. That is a data-corruption bug, not just a connectivity one.Reproduced end to end, with two genuinely different majors installed
Built package, an app on
ioredis@6.0.0,@boringnode/busresolving its ownioredis@5.11.1, a Redis on port 6390 (deliberately not the default),useMessageBuffer: trueand a binary encoder:mainduplicate()calls:6379:6390useMessageBufferhonouredstring)Buffer)undefined)Isolating just the connection, on
main, the client built out of a live foreign-major instance reports:It is
ready. It is connected. It is connected to the wrong server, and nothing anywhere says so.The chain this sits at the bottom of
This is worth spelling out, because the same one-line check has now caused an incident and blocked a release train across three packages.
@adonisjs/redis@10.0.1shipped ioredis 6 in a patch.@adonisjs/cachepasses itsioConnectionstraight down, so theinstanceofcheck one layer below started failing, a second connection was opened to127.0.0.1:6379, and production logs filled withconnect ECONNREFUSED 127.0.0.1:6379roughly every 2 seconds. It was resolved by reverting to ioredis 5 and re-shipping ioredis 6 as the11.0.0major.instanceof.@adonisjs/cachestill cannot move to@adonisjs/redis@11today, at all.ioredispeer, and the stated reason is this package:@boringnode/busis a hard dependency of bentocache and peersioredis ^5.0.0, so widening upstream would only produce unmet-peer warnings while the bus still could not accept a version-6 client.#123applied, a foreign-major client is now correctly forwarded toRedisTransport, which then fails itsinstanceofand re-breaks everything one layer down. I confirmed that live while working on#123; it is what sent me here.So:
@boringnode/bus→bentocache→@adonisjs/cache. Fixing it here is what lets bentocache widen its peer, which is what unblocks@adonisjs/cache. The other two cannot proceed without this one.What changed
1. Duck-typing instead of
instanceofWhy those two properties, verified against
5.11.1and6.0.0side by side:RedisandCluster, in both majors — all four combinations checked (v5.Redis,v5.Cluster,v6.Redis,v6.Cluster).RedisOptionsorClusterOptionsin either major (grepped the shipped.d.ts), so a legitimate options object can never be misclassified as a client. That direction matters most: a false positive fails loudly on the first command, a false negative is the silent-localhost bug.constructor.nameis not usable here, in case it comes up in review — ioredis builds its clients through a mixin, so instances of bothRedisandClusterreportEventEmitterin both majors.Because the live-client branch is now taken,
useMessageBufferfrom the third argument survives, which fixes the second symptom.2. A tripwire instead of the silent fallback
If a value is not recognised as a client but still carries the markers of one — a
statusstring, anoptionsobject andemit— we throwInvalidArgumentsExceptionrather than dialing localhost. None of those three names are valid ioredis options either, so no legitimate config trips it. The genuine options-object path is untouched: that is the documented API and changing it would be breaking.This branch is unreachable for every ioredis major that exists today; it only exists so that a future shape change degrades into a boot-time error instead of another production incident. Happy to drop that hunk if you would rather keep the diff to just the discriminator. (Same offer is on
#123, so the two stay coherent either way.)Peer range: widened to
^5.0.0 || ^6.0.0Unlike bentocache, this package is in a position to widen — nothing sits below it. So I validated ioredis 6 properly first rather than assuming.
RESP3 is genuinely negotiated, not just tolerated:
client.options.protocol === 3under ioredis 6 (it isundefinedunder 5), and aHELLOround trip has the server reportingproto=3. Everything below ran under that.Full existing suite, unmodified, against
ioredis@6.0.0: 72 passed, 0 failed — byte-for-byte the same result as under5.11.1, including the real-cluster test (I brought up a three-node cluster on 7000-7002 so it would not be skipped).Targeted validation of the whole
RedisTransportsurface — a standalone harness against Redis 7.2, run under both majors:options.protocol === 3HELLOreportsproto=3unsubscribestops deliverymessageBufferdelivers aBufferduplicate()path deliversduplicate()path keepsmessageBufferonReconnectfires after a server restartThe binary payload was deliberately nasty: a 300-character key, embedded control bytes, multi-byte UTF-8 (
👍,日本語) and a base64 blob of0x00 0x01 0x02 0xFA 0xFB 0xFC 0xFF— 416 bytes total, compared withBuffer.compareagainst exactly what was published. Under RESP3 it comes back identical. (For contrast, the non-buffermessagestring path is lossy in both majors — no regression there, just a reminder of whyuseMessageBufferexists.)Cluster on RESP3 was checked separately too, against the real three-node cluster:
Cluster+useMessageBuffer: true+ a >320-byte multi-byte payload round-trips exactly, and the member nodes reportprotocol === 3.One real code change was needed to make the widen honest.
Redis#duplicateandCluster#duplicatebecame generic over the reply mapping in ioredis 6, and the two signatures no longer unify, sooptions.duplicate()on aRedis | Clusterunion does not compile:This is pre-existing —
mainproduces the identical error on the identical two lines when typechecked against ioredis 6; my change did not introduce it. The call now goes through a narrow local signature (duplicateClient), after whichyarn typecheckandyarn buildare clean under both majors. Without that, widening the peer would have advertised support this package's own source could not compile against.engines.nodeis already>=20.6, comfortably above ioredis 6's>=20.0.0, so no engine change is needed.The
ioredisdevDependency stays at^5.11.1, so CI keeps testing against 5 by default. Alongside it there is now a second, real copy installed under an npm alias, used only by the tests:Yarn 4 with the
node-moduleslinker resolves thenpm:protocol natively — the lockfile records it as"ioredis-v6@npm:ioredis@^6.0.0"resolving toioredis@npm:6.0.0, and it lands innode_modules/ioredis-v6next tonode_modules/ioredis@5.11.1, so the two are genuinely distinct module instances with distinct classes. Noresolutions, no patch, no install script.If you would like a CI matrix that runs the suite against both majors as the primary copy, say the word and I will add it — I kept
.github/out of this PR on purpose.Tests
Added to
tests/drivers/redis_transport.spec.ts. The cross-major cases use a real ioredis 6 client, not a simulation. Thanks to the alias above, the test file imports a second, genuine copy of the package:That is exactly the real-world scenario: an application on
@adonisjs/redis@11(ioredis 6) handing its connection to a bus resolved against ioredis 5. Each cross-major test asserts its own premise at runtime before asserting behaviour, so it cannot silently stop covering the bug:To tell "reused the connection" from "built a new one", the tests record what
duplicate()returns through an own-property spy that shadows the prototype method — the client stays an instance of its own copy, which is the whole point.Redisclient is reused — assertsduplicate()was called twice and that the duplicates are real ioredis 6 clients, then proves the message really lands on the configured server by reading it off an independent witness connection bound to the testcontainers portClusterclient is reused — asserts the duplicates are real foreignClusterinstances (and notinstanceofourCluster), and that the transport wiredonReconnectonto the duplicated subscriberuseMessageBuffersurvives on the live-client path — asserts the subscriber duplicate listens onmessageBufferand not onmessage, then round-trips a binary payload through Redis and compares the bytes seen on the subscriber with the exact bytes the publisher wrote (Buffer#equals)Redis is provisioned the way the file already does it, through the existing
@testcontainers/rediscontainer started ingroup.setup— no second mechanism. The cluster case useslazyConnect, so it needs no server and still runs on CI.The first four fail on
mainand pass on this branch. Verified by reverting onlysrc/transports/redis.tsand re-running the file: 13 passed / 4 failed, versus 17 passed here, and the four failures are precisely those tests. The fifth passes on both by design — it exists so a future tweak to the discriminator cannot start swallowing options objects.Why the
Proxystand-in is goneThe previous revision simulated the foreign client with a
Proxyover a null-prototype target. It was shape-faithful and it did fail onmain, but a real ioredis 6 client is a strictly harder case: it covers everything theProxycovered (different classes,instanceoffalse, workingduplicate()) plus the parts a facade cannot vouch for — a real prototype chain, a real RESP3 connection, a realduplicate()implementation from another major.The one thing a real client genuinely cannot exercise is the tripwire path — "client-shaped object that is no
ioredisat all". That case keeps its own test, which uses a plain object literal ({ status, options, emit }) and never needed theProxyin the first place. So the helper had no remaining job and was removed rather than kept as dead weight next to the real thing.What I ran
main, same machine, cluster up: 67 passed, 0 failed.ioredis@5.11.1as the resolved copy and withioredis@6.0.0as the resolved copy (in that second run the alias is a second copy of the same major, which is the duplicated-in-the-tree half of the bug — the tests hold there too).tests/drivers/redis_transport.spec.tsalone: 17 passed on the branch, 13 passed / 4 failed withsrc/transports/redis.tsreverted to theinstanceofversion.yarn lint,yarn typecheckandyarn build: clean under both majors. TheduplicateClientnarrow signature is still required with the alias installed — ioredis 6's genericduplicateis now in the dependency tree either way.What I did not validate