feat(redis): use native commands for cache reads

[lan17]

Summary

Replace read-side Lua with native Redis commands and decode DialCache's frame in TypeScript:

• untracked reads use GET
• tracked reads use one atomic, primary-routed MGET for the value and watermark
• write and invalidation remain Lua-backed; a watermark-fenced tracked write now atomically unlinks the stale value it rejects
• node-redis registers only the three mutation scripts, and GLIDE owns only the three mutation script handles
• custom adapters can reuse the public decodeRedisFrame and decodeTrackedRedisFrame helpers

This removes the Redis-to-Lua payload materialization and string.sub copy on every hit while preserving the semantic DialCacheRedisClient.read() boundary.

Read architecture

Adapter / mode	Untracked	Tracked	Primary guarantee
node-redis standalone	GET	MGET	standalone connection
node-redis Cluster	GET	raw MGET	sendCommand(..., false, ...) routes to the slot primary
GLIDE standalone	GET	one-command Batch(false).mget(...)	standalone batches execute on the primary even with replica reads configured; MGET itself is atomic
GLIDE Cluster	GET	custom-command MGET	explicit primarySlotKey route

The shared decoder:

• validates the frame version and minimum length
• preserves missing/short/unsupported frames as clean misses
• parses integer and fractional legacy watermarks with the same accepted grammar as Lua
• rejects values whose Redis-created timestamp is at or before the watermark
• preserves unsupported payload encodings as DialCacheRedisPayloadEncodingError
• returns binary payloads through a zero-copy Buffer.subarray() view

Tracked value and watermark reads retain one atomic snapshot, with both values returned by a single MGET. Their existing shared Cluster hash tag remains required; mismatched tags still fail with CROSSSLOT.

Breaking change

• READ_CACHE_SCRIPT and READ_TRACKED_CACHE_SCRIPT are removed from dialcache/redis-protocol.
• dialcacheRedisScripts.dialcacheRead and dialcacheRedisScripts.dialcacheReadTracked are removed from dialcache/node-redis.
• Custom node-redis wrappers must expose native get / sendCommand; legacyMode clients are unsupported because neither their callback surface nor .v4 view exposes the complete native-command-plus-custom-script contract.
• The GLIDE helper requires GLIDE 2.x, a direct official GlideClient or GlideClusterClient, and the same module namespace that created it. Forwarding wrappers should implement DialCacheRedisClient directly because their topology cannot be inferred safely.
• Official node-redis clients and direct GLIDE 2.x clients passed through the documented helpers keep the same application-facing call shape, so those consumers can bump the package without code changes.
• Redis keys, frame format, and invalidation behavior are unchanged. A tracked write rejected by an active future watermark still returns false, but now also unlinks the stale value key. No data migration or cache flush is required.
• The fenced-write cleanup requires UNLINK (Redis 4.0+ or compatible Valkey) and permission for scripts to invoke it. With a command-restricted ACL that denies UNLINK, the write fails open as cache_write and leaves the stale value for a later cleanup or expiry.

BREAKING CHANGE: the four deprecated read-Lua exports and registrations above are removed; node-redis adapters require the promise-mode native-command surface; the GLIDE helper requires a direct GLIDE 2.x client from the supplied runtime; and the fenced-write cleanup requires Redis UNLINK support plus ACL permission. Under the repository's release configuration, this change should release as v1.0.0.

Adapter behavior changes

• The node-redis factory now requires native get and sendCommand methods in addition to the three registered mutation methods.
• The GLIDE factory declares an optional @valkey/valkey-glide ^2.0.0 peer, validates Batch support eagerly, and classifies standalone versus cluster behavior from the supplied runtime's client identities before allocating scripts. Its standalone non-atomic primary batch avoids consuming caller-owned WATCH state.
• Redis MGET returns null for wrong-type members. A tracked wrong-type value is therefore a clean miss and may be repaired with a valid DialCache frame after fallback succeeds, while a wrong-type watermark prevents the tracked write from succeeding. An untracked GET still surfaces WRONGTYPE. Real-engine tests cover both repair and repeated fail-open behavior, including metrics.
• The public read contract now specifies frame decoding, miss and watermark rules, atomic authoritative snapshots, and returned-buffer ownership. Shared decoders validate leaf reply types; adapters retain only client-specific envelope validation.

Benchmark

The benchmark harness and JSON results were intentionally kept outside the repository. Methodology:

• Redis 6.2.22 and Valkey 8.1.8
• Node 22.22.0, node-redis 4.7.1, GLIDE 2.4.2
• binary payloads of 100 B, 1 KiB, 10 KiB, 100 KiB, and 1 MiB
• fresh untracked hit, fresh tracked hit, and invalidated tracked miss
• three alternating rounds, one command in flight, loopback Docker
• median throughput, latency, Redis INFO commandstats execution time, and network bytes

At 1 MiB, native fresh-hit throughput improved 15-45% across the two engines and adapters. Server-reported command execution time per logical read fell 95-98%. Small 100 B / 1 KiB end-to-end results were mostly flat/noisy while reported command time still fell about 80-90%; the notable small-case regression was Redis/node-redis's 100 B tracked hit at about -10% throughput. These loopback, one-in-flight results are directional rather than production-capacity measurements.

Representative Redis 6.2 + node-redis medians:

1 MiB scenario	Lua ops/s	Native ops/s	Lua server us/read	Native server us/read	Lua -> native p50
untracked hit	230	269	719.8	32.6	3.718 ms -> 2.955 ms
tracked hit	217	259	713.7	31.2	3.630 ms -> 3.016 ms
invalidated tracked miss	1,762	284	361.6	31.0	0.566 ms -> 2.949 ms

The invalidated-miss row is the main tradeoff: Lua returns only a null reply, while native MGET transfers the stale frame before TypeScript rejects it. At 1 MiB this changes roughly 3-5 response bytes into about 1.05 MB. Across both engines and adapters, invalidated-miss throughput fell 77-84% at 1 MiB (46-58% at 100 KiB), even though server-reported command time still fell 91-94%.

The benchmark intentionally measured the read itself and therefore includes that full transfer. In the application path, the first completed fallback that reaches a still-fenced tracked write now atomically unlinks the stale value, bounding subsequent transfers for that entry. This is only a partial mitigation: a read failure or timeout never reaches the write-side cleanup, so the stale payload can continue to transfer or time out until another completed read cleans it up or its TTL expires.

Scope

This branch is updated onto the current v0.15.0 read contract, including the untracked-cache shadowing changes from #122. It deliberately does not include the server-time / maximum-age behavior proposed in #121. That work can be evaluated separately against this read path and its benchmark tradeoffs.

Validation

• corepack pnpm typecheck
• corepack pnpm test - 424 tests, coverage thresholds passed
• corepack pnpm build
• corepack pnpm test:package - including real node-redis and GLIDE standalone and Cluster consumer types, plus packed ESM/CommonJS absence checks for all four removed APIs
• corepack pnpm test:integration - 113 tests across Redis 6.2, Valkey 8, and Redis Cluster
• tracked wrong-type value repair and repeated wrong-type watermark fail-open behavior exercised end to end across both adapters and both standalone engines
• stale tracked frames exercise the real decoder and record a remote miss, request/get/fallback timing, and no read error across both adapters and both standalone engines
• fenced tracked writes prove stale-value unlinking while preserving the exact watermark and its TTL trajectory
• cluster SCRIPT FLUSH recovery proves mutation scripts repopulate every master and a subsequent identical read is a cache hit
• GLIDE package tests compile against the supported 2.0.0 floor and exercise separate module instances plus packed ESM/CommonJS error identity
• focused GLIDE primary/replica probe and three-node Cluster probe
• git diff --check

[lan17]

should we just remove altogether?

[lan17]

same, probably best to just remove at this point.

[lan17]

will return null count as a miss logically and in instrumentation?

[lan17]

Verification

• tsc --noEmit on the branch: clean.
• Full unit suite: 418/418 pass, coverage thresholds hold.
• Real node-redis RedisClientType / RedisClusterType structurally satisfy the new NodeRedisStandaloneClient / NodeRedisClusterClient interfaces (compiled a standalone assignability probe against the redis@1.6.1 typings).
• Signatures check out: RedisCluster.sendCommand(firstKey, isReadonly, args, options) and RedisClient.sendCommand(args, options) match the declared shapes.
• invokeScriptWithRoute exists only on GlideClusterClient, and customCommand exists on both, so the cluster type guard's conjunction is sound.
• The TS decoder is a faithful port of the Lua. I diffed parseRedisWatermark against PARSE_WATERMARK_LUA grammar-for-grammar (^%d+$ / ^%d+%.%d+$, >= math.huge rejection, 1\n rejection via Lua's strict $), and the frame checks against READ_FRAME_LUA (len < 10, version byte, sub(value,10) = subarray(9), struct.unpack(">I8", sub(value,2,9)) = readBigUInt64BE(1), created_at <= watermark). Missing watermark is a miss in both. No semantic drift found.
• The GLIDE standalone-batch primary claim is documented, not just empirical — Batch.d.ts:3086: @remarks Standalone Batches are executed on the primary node.

The core change is well-executed. Issues below are ordered by what I'd want addressed.

---

1. Delete the deprecated read API instead of retaining it

DialCache is new; unused API should be deleted, not deprecated.

The PR keeps READ_CACHE_SCRIPT and READ_TRACKED_CACHE_SCRIPT (src/internal/redis-scripts.ts:57) plus dialcacheRead and dialcacheReadTracked (src/node-redis.ts:56) as inert compatibility entries. The cost shows up immediately in the tests: test/node-redis.test.ts:248 now exercises transformReply on dead code purely to hold coverage, and line 78 asserts Object.keys(dialcacheRedisScripts) in exact order — a brittle assertion whose only job is to pin the dead entries in place. The README grows a sentence explaining that two registrations exist but do nothing.

The deletion is mechanical and fully enumerable:

• READ_CACHE_SCRIPT / READ_TRACKED_CACHE_SCRIPT from redis-scripts.ts and redis-protocol.ts
• READ_FRAME_LUA and RETURN_PAYLOAD_LUA become unreferenced (PARSE_WATERMARK_LUA stays — write-tracked and invalidate still use it)
• dialcacheRead / dialcacheReadTracked from dialcacheRedisScripts, plus the now-unused readReply helper
• scripts/test-package.mjs:52 imports READ_CACHE_SCRIPT → switch it to a write script
• The two test assertions above, and the README sentences

2. The invalidated-miss regression has a worse case than the benchmark shows

The PR body honestly reports 77-84% throughput loss on invalidated tracked misses at 1 MiB, framed as a transient cost until the fallback repopulates. But WRITE_TRACKED_CACHE_SCRIPT returns 0 without writing when watermark >= now_ms, and INVALIDATE_CACHE_SCRIPT never touches the value key. So when a caller passes a non-zero futureBufferMs to invalidateRemote — the documented safety window for source replication lag — no write can repair the entry for the whole window, and every tracked read in that window ships the full stale payload only to discard it. Under Lua those reads returned ~3 bytes. Default futureBufferMs = 0 keeps this off the common path, but the feature exists precisely to be used.

Second-order effect worth stating: readTimeoutMs defaults to 50 ms. A 1 MiB stale transfer that used to be a null reply can plausibly blow that deadline over a real network, so invalidations of large tracked entries can spike cache_read timeout metrics — a failure mode that didn't previously exist.

I'd extend the tradeoff section to cover the future-buffer window explicitly. If you want to actually mitigate it, the cheapest option is for the tracked read path to fire-and-forget an UNLINK on the value key when it decodes a stale frame; that trades one extra command on a cold path for bounding the amplification.

3. The cluster test lost its strongest assertion

test/redis-cluster.integration.test.ts:110 changed the recovery namespace to "cluster-cache-recovery", so calls goes 30 → 60 and expect(second).toEqual(first) weakened to second.map(({ id }) => id). I understand why: the second round now has to miss in order to exercise mutation-script reload after SCRIPT FLUSH, since reads no longer reload anything.

But the assertion that disappeared — "after SCRIPT FLUSH, reads still serve the previously cached values with no extra source calls" — is exactly the one that catches a broken native cluster read path. Add a third round on the original "cluster-cache" namespace asserting calls stays at 60 and the values deep-equal first. That restores the coverage without undoing the rewrite.

4. The GLIDE cluster-detection fallback is unsound

src/valkey-glide.ts:76 requires both customCommand and invokeScriptWithRoute; if either is missing, a client falls through to new glide.Batch(false) at line 163. For a real GlideClusterClient that is the wrong class — GlideClusterClient.exec takes ClusterBatch, not Batch. Today the guard cannot miss a genuine cluster client, but the failure mode if GLIDE ever renames a method is silent misrouting, and the new test locks in the fallback as intended behavior.

node-redis got the "positive marker" treatment ("masters" in client); do the same here. Detect cluster-ness on one stable marker and throw a clear error if the routing capability is absent, rather than degrading into a code path that cannot be correct for a cluster client.

---

Nits

• Error-message asymmetry. node-redis distinguishes "…expected a bulk string or null" from "…expected two bulk strings or nulls"; GLIDE throws the same generic "Invalid DialCache Redis payload reply" from three distinct sites (src/valkey-glide.ts:152-176). The README sells these errors as a way to distinguish failure sites in logs — worth matching node-redis's specificity.
• Unbounded watermark stringification. parseRedisWatermark does raw.toString("utf8") on the whole buffer before validating. A watermark key holding a large string allocates it fully on every tracked read. A raw.length > 32 → null guard costs one line.
• Empty-value behavior change. The GLIDE test dropped read({ valueKey: "empty" }) → DialCacheRedisPayloadError; a zero-length GET reply is now a clean miss. Sensible fail-open, but it is an intentional loosening that is not called out anywhere.
• Cite the GLIDE doc. The comment at src/valkey-glide.ts:96 asserts the standalone-batch invariant as if it were an observation. It is in GLIDE's own docs — quoting that makes the invariant auditable instead of dependent on a probe someone ran once.
• Optional. BatchOptions supports timeout, so the standalone tracked path could forward context.timeoutMs and get a real per-request budget where the other GLIDE paths cannot. Asymmetric, so only worth it if partial coverage beats none.

Bottom line

The read-path rewrite is correct and the Lua→TS port is faithful — I checked it closely and found no semantic drift. §1 is the one I would block on; §2 and §3 are documentation and test-coverage debt that are cheap to pay now.

[lan17]

Re-reviewed at e64577e (one new commit since the last pass: refactor(redis)!: remove legacy read Lua).

Re-verified on the new head

• tsc --noEmit: clean.
• Unit suite: 418/418 pass, coverage thresholds hold.
• Grepped every removed name across src, test, scripts, README.md, AGENTS.md — the only remaining hits are the intentional negative assertions in test-package.mjs. Clean deletion.
• Verified the @ts-expect-error-over-a-multi-specifier-import pattern actually compiles as intended (standalone probe with --strict --noUnusedLocals; the directive absorbs both TS2305s and is not flagged unused).

§1 — Delete the deprecated read API: done, and past what I asked for

Everything on the enumeration came out: READ_CACHE_SCRIPT, READ_TRACKED_CACHE_SCRIPT, READ_FRAME_LUA, RETURN_PAYLOAD_LUA, dialcacheRead, dialcacheReadTracked, the orphaned readReply, the redis-protocol.ts re-exports, both README sentences, and the two coverage-driven test assertions. Object.keys(dialcacheRedisScripts) is down to the three real scripts.

The addition I did not ask for and like: test-package.mjs now pins the removal at two levels — @ts-expect-error on the consumer side, plus runtime in checks against the packed ESM and CommonJS entries for all four names. That is the right way to make a deletion stay deleted.

PARSE_WATERMARK_LUA correctly survived (write-tracked and invalidate still use it), and REDIS_FRAME_VERSION is still live in both WRITE_FRAME_LUA and the TS decoder.

§2 — Invalidated-miss regression: partially addressed

The new records a stale tracked frame as a remote miss without a read error integration test is a good addition — it drives the real decoder end to end, asserts request / miss / observeGet / observeFallback fire with the right labels and error does not, and it sits exactly on the createdAt == watermark boundary. That closes the "does a stale frame look like a failure to the metrics layer" question.

It does not cover what I raised, though. The PR body's tradeoff paragraph is unchanged, so it still reads as if the invalidated-miss cost is one-shot until the fallback repopulates. It is not: WRITE_TRACKED_CACHE_SCRIPT returns 0 without writing while watermark >= now_ms, and INVALIDATE_CACHE_SCRIPT never touches the value key — so a non-zero futureBufferMs on invalidateRemote blocks the repairing write for the entire window, and every tracked read in it ships the full stale payload. Still worth a sentence, along with the readTimeoutMs (50 ms default) interaction: a 1 MiB stale transfer that used to be a null reply can now blow the read deadline over a real network.

§3 — Cluster test assertion: not addressed

test/redis-cluster.integration.test.ts is byte-identical to 7c76bac. The expect(second).toEqual(first) / calls === 30 coverage is still gone.

§4 — GLIDE cluster-detection fallback: not addressed

src/valkey-glide.ts is byte-identical to 7c76bac. A cluster client that fails the two-method guard still falls through to new glide.Batch(false), which GlideClusterClient.exec does not take.

Nits

The five from the previous review still stand (GLIDE error-message asymmetry, unbounded watermark toString, the undocumented empty-value loosening, citing GLIDE's own Batch.d.ts:3086 remark, optional BatchOptions.timeout).

One new, minor: the single @ts-expect-error at scripts/test-package.mjs:53 covers both specifiers on that line, so it only asserts at least one of the two is missing — if one export came back, the directive would still be satisfied by the other. The runtime in checks at lines 656-663 / 906-913 do pin them individually, so nothing is actually unguarded; splitting into two directives would just make the compile-time half as strong as it looks.

---

Net: the blocking item is resolved cleanly. What is left is §3 and §4 plus documentation on §2 — none of which touch the read path's correctness.

[lan17]

review-loop — report mode

Run: report mode, lanes brutal + reliability + performance. Snapshot target=fad2736 base=fad2736 head=e64577e.

Incomplete, deliberately posted early: all 7 leaf lanes finished and I adjudicated them, but the holistic stage-one pass was still running and stage two never ran. So this is an adjudicated leaf-lane result, not a completed review-loop run — no cross-lane audit challenged my dispositions below.

16 raw findings deduped to 8 accepted. correctness returned clean.

---

Verified, not just relayed

Three lanes independently re-derived Lua→TS decode parity and all three agree it holds exactly: REDIS_FRAME_MIN_BYTES = 10 ≡ string.len(value) < 10; raw[0] === REDIS_FRAME_VERSION ≡ string.byte(value, 1) ~= 1; raw.subarray(9) ≡ string.sub(value, 10); readBigUInt64BE(1) ≡ struct.unpack(">I8", string.sub(value, 2, 9)); /^[0-9]+(?:\.[0-9]+)?/ plus full-length match ≡ ^%d+$/^%d+%.%d+$ including trailing-newline and embedded-NUL rejection; Number.isFinite ≡ value >= math.huge for digit-only input. Frames written by the unchanged write Lua decode identically under old and new readers, so rolling deploy and rollback are safe.

Primary routing was verified in the compiled internals rather than assumed: node-redis sendCommand(firstKey, false, …) resolves through #execute → slots.getClient(firstKey, false) to slots[slot].master; returnBuffers applies to nested array members, so MGET really does yield [Buffer|null, Buffer|null]; GLIDE Decoder.Bytes applies to the whole response tree; ClusterResponse<T> returns bare T for single-node routes.

A latent pre-existing hazard is removed by this change: node-redis always emits plain EVALSHA (client/index.js:207) regardless of IS_READ_ONLY — that flag only picks the cluster node. So the old untracked read script could be routed to a replica under useReplicas: true and rejected. Native GET eliminates that.

---

Accepted findings

1. Stale tracked frames re-transfer on every read for the whole invalidation window

defect · medium · src/internal/redis-scripts.ts:68 (mitigation site)

Three code facts make the invalidated-miss cost sustained rather than one-shot:

1. INVALIDATE_CACHE_SCRIPT only bumps the watermark and cannot delete the value key — one watermark covers every args variant of every tracked use case sharing keyType+id.
2. WRITE_TRACKED_CACHE_SCRIPT returns 0 without writing while watermark >= now_ms, so during a nonzero futureBufferMs window no fill ever replaces the stale frame.
3. src/dialcache.ts:783 sets suppressCacheWrite = wroteRemote === false, which then skips putLocalFailOpen at line 789. There is no local backstop and no negative caching, so every request in the window reaches Redis.

I corrected the reporting lane's magnitude. It sized this at ~200 MB per invalidation from 1000 rps × 2 s × 100 KiB. That ignores DialCache's own process-scope coalescing: one Redis read per key is in flight per process, and each cycle also pays fallback latency. Real amplification is roughly processes × window / (read + fallback time) full-payload transfers — one to two orders of magnitude smaller. I reduced severity from high accordingly. The mechanism stands.

The sharpest consequence is the deadline interaction: readTimeoutMs defaults to 50 ms, which must now cover a full stale-payload transfer. Invalidating a large tracked value can convert cheap misses into cache_read_timeout errors on a real network, and GLIDE has no per-invocation cancellation, so the discarded payload keeps streaming after DialCache has already fallen back.

Recommendation: add redis.call("UNLINK", KEYS[1]) immediately before the return 0. I checked this is read-neutral: at that point watermark >= now_ms, so any frame at KEYS[1] was written with created_at <= now_ms <= watermark and the TS gate already rejects it; Lua atomicity closes the race; KEYS[1]/KEYS[2] share the {namespace:keyType:id} tag so it is cluster-safe. One caveat I'd want acknowledged: under a failover with clock skew a frame could carry created_at > now_ms and would then be deleted where the gate would have accepted it — a miss, not incorrectness.

Also worth a sentence in the README future-buffer sizing guidance, which currently says overestimating only "increases fallback load."

2. GLIDE topology is chosen by probing a method the adapter never calls

defect · medium · src/valkey-glide.ts:76-83

isValkeyGlideClusterClient requires both customCommand and invokeScriptWithRoute. The adapter never calls invokeScriptWithRoute — it is a nominal brand implemented as a capability probe, needed only because standalone GlideClient also has customCommand.

Both mis-detections fail silently, and the reachable one is not hypothetical:

• Cluster read as standalone — a caller wrapping a cluster client in exactly the documented ValkeyGlideScriptingClient surface (which the README invites) typechecks fine, fails the sniff, and takes the new glide.Batch(false) path. Batch and ClusterBatch both extend BaseBatch, so it likely executes and lets GLIDE route MGET under the client's readFrom.
• Standalone read as cluster — GlideClient.customCommand(args, options?: DecoderOption) takes no RouteOption, so the excess route is silently ignored and MGET again follows readFrom.

Either path reads the invalidation watermark from a replica with no error, no metric, no log — defeating the invariant AGENTS.md calls critical, and turning an invalidated payload into a hit.

Nothing can catch it. There is no runtime GLIDE cluster test in the repository: GlideClusterClient appears exactly once, at scripts/test-package.mjs:531, as a never-executed declare const — and it is passed only to the factory, which checks the public parameter type, while ValkeyGlideClusterReadClient is unexported and reachable only through the predicate. The unit test "requires the complete cluster command capability" exercises a client with invokeScriptWithRoute but no customCommand, a shape no real client presents, so no test proves the guard needs both.

Contrast src/node-redis.ts:127-147, where the same distinction is a declared union with masters on the cluster arm: topology is carried by the type and a mis-sniff fails loudly on the wrong sendCommand overload.

Recommendation: discriminate on identity, not shape. The caller already must pass the same GLIDE namespace, so add GlideClusterClient to ValkeyGlideRuntime and use instanceof. Failing that, probe a capability the adapter actually consumes and throw on ambiguity rather than defaulting to the replica-eligible path.

(I raised a weaker version of this in an earlier review and then withdrew it as speculative. The withdrawal was wrong — I had not found the narrow-wrapper path or the second mis-detection direction.)

3. glide.Batch is resolved lazily, on one code path

defect · medium · src/valkey-glide.ts:109-111 vs :163

The factory eagerly constructs all three new glide.Script(...) handles at wiring time, but never touches glide.Batch. The first dereference is new glide.Batch(false).mget(...) inside read(), reached only on the tracked standalone path.

@valkey/valkey-glide is a devDependency only — it is absent from peerDependencies, and the README install line carries no version floor. Batch/ClusterBatch are the v2 names (the 2.4.2 typings still carry Transaction extends Batch marked deprecated, which is the v1→v2 rename), and the pre-change adapter needed only Script, Decoder, and invokeScript — all present in v1.

So on an older runtime, or a JS consumer, or a runtime/typings mismatch: construction succeeds, dispose() succeeds, untracked reads, writes, invalidation and every cluster read succeed — then tracked standalone reads throw an opaque TypeError, not a DialCache error type. Reads fail open, so the deployment silently degrades to 100% source load on tracked use cases with only a cache_read metric. Because the cluster branch never touches Batch, this is invisible in cluster-backed staging and appears first in standalone production.

This is precisely the mixed-install hazard the eager-Script design already guards against.

Recommendation: validate typeof glide.Batch === "function" in the factory alongside the eager Script construction, and state the minimum GLIDE version in the README and the breaking-change note — ideally as an optional peer dependency.

4. The cluster SCRIPT FLUSH test can no longer fail for its stated reason

defect · medium · test/redis-cluster.integration.test.ts:110-157

Re-namespacing the recovery round to cluster-cache-recovery makes its 30 keys disjoint from round one, so every read is a guaranteed miss and the surviving assertions — expect(calls).toBe(60) and an id-only comparison — are derived purely from the source function.

DialCache fails open on both sides: read errors are recorded and rethrown but absorbed upstream, and write errors are swallowed at src/dialcache.ts:784-785 (logger.warn, then suppressCacheWrite = key.trackForInvalidation). So if the write script failed to reload on every shard after SCRIPT FLUSH — or if Redis were entirely unreachable — calls would still be 60 and the ids would still match. sizesBeforeFlush is captured before the flush and cannot compensate.

The pre-change assertions (calls === 30, second deep-equals first) did prove post-flush recovery, because 30 remote hits were required. The standalone suite keeps the correct pattern at test/redis-real.integration.test.ts:1098-1126.

Recommendation: add a third round against cluster-cache-recovery asserting calls stays 60 and the values deep-equal second. That proves the post-flush writes landed on every shard and that native GET returns hits across all 30 slots.

(I flagged this in an earlier review, then downgraded it in a self-audit as "thin — the lost coverage is marginal." That downgrade was wrong. I was measuring lost hit-coverage; the real problem is that the test is now vacuous with respect to its own name.)

5. The read half of the Redis protocol lost its published source of truth

improvement · medium · src/redis-protocol.ts:1-9

dialcache/redis-protocol previously exported READ_CACHE_SCRIPT and READ_TRACKED_CACHE_SCRIPT. The rules that replaced them — header length, version equality, watermark grammar, and the createdAt <= watermark fence — now live only in src/internal/redis-payload.ts, which no entry point re-exports. DialCacheRedisClient.read documents neither the primary-routing requirement nor any miss rule nor the comparison direction, and the README still invites custom adapters in the same paragraph that now offers only write and invalidation sources.

The write side ships as an executable spec any adapter can EVAL and get right by construction. The read side ships as prose plus a private module. A third-party adapter that inverts <=, or treats a missing watermark as fresh, silently serves invalidated data — failing at exactly the guarantee the library exists to provide. Both bundled adapters are safe only because they share the internal module.

Recommendation: re-export the two frame decoders under public names from src/redis-protocol.ts, and move the read invariants into the DialCacheRedisClient.read doc comment. Both changes are additive.

6. Read-reply shape validation is written three times

improvement · medium · src/node-redis.ts:155-190, src/valkey-glide.ts:136-173, src/internal/redis-payload.ts:54-75

decodeRedisFrame and decodeTrackedRedisFrame accept only pre-narrowed Buffers, so each adapter re-derives "a DialCache read reply is a bulk string or null" independently: node-redis grows validateRedisBulkStringReply and validateRedisMGetReply plus a tuple threaded through readTracked; GLIDE grows asRedisFrame, an inline length check, and three copies of the same message string. Five throw sites, three distinct messages, two wire shapes.

The narrow parameter type is load-bearing in the wrong direction: decodeRedisFrame applied to a 10+ character string returns null rather than throwing, because raw.length >= 10 holds and raw[0] is a character. Any future adapter that forgets the pre-validator converts a wrong-typed reply into a silent cache miss. The safe path is opt-in rather than structural.

A shared home already exists as precedent: src/internal/redis-script-reply.ts owns the cross-adapter write/invalidate reply-domain checks.

Recommendation: widen the decoders to accept unknown and let them own the shape check and its single error message. Adapters collapse to return decodeRedisFrame(await client.get(options, valueKey)), deleting both node-redis validators, asRedisFrame, the tuple type, and two of three message strings. This also fixes the diagnostic asymmetry: GLIDE currently collapses three materially different wire failures — a non-Buffer MGET member, a malformed exec envelope, and a malformed MGET pair — into one message, which is the pair that most needs distinguishing in production, on the path that has no cancellation signal.

7. Wrong-type handling now diverges by tracked-ness

improvement · low · src/node-redis.ts:200-216

Pre-change both read paths ran GET inside Lua, so a wrong-type key raised WRONGTYPE uniformly. Now the untracked path still surfaces WRONGTYPE while the tracked path uses MGET, which reports a wrong-type member as nil and yields a clean miss.

The consequence is asymmetric and untested at one end. A wrong-type tracked value key self-heals — the integration test proves it. A wrong-type untracked key errors on every read forever and is never repaired, because a read failure never triggers a post-fallback write. And a wrong-type tracked watermark is the one case that never self-heals at all: WRITE_TRACKED_CACHE_SCRIPT does GET KEYS[2] on the hash, which aborts the script, so every request misses and every write errors. That case also lost half its observability in this PR — it previously recorded a cache_read error, and now only cache_write remains.

Recommendation: pick one semantic and state it in the read contract, and extend the wrong-type test to drive a tracked read twice against a hash-typed watermark, asserting the source is called both times and that cache_write records the error.

8. decodeRedisPayload's empty-payload guard is unreachable

improvement · low · src/internal/redis-payload.ts:38-41

Two lanes reached this independently and I confirmed it: decodeRedisPayload has exactly two callers, both in the same file, both gated by isSupportedRedisFrame requiring length >= 10. raw.subarray(9) therefore always has at least the encoding byte, so raw.length === 0 cannot hold. The function is not re-exported from any entry point, so no custom adapter can reach it either. Its only exercise is the direct call at test/redis-payload.test.ts:48, which keeps a dead branch green.

Moving the length invariant from Lua into TypeScript is what made this provable. Given the delete-don't-deprecate stance this PR otherwise applies cleanly, it's the same cleanup one level down.

---

Raised but not filed, with rationale

• Untracked GLIDE reads became replica-eligible. Three lanes raised this independently: invokeScript issues EVALSHA, which readFrom cannot route to a replica; client.get can. Correct and undocumented, but you explicitly set it aside earlier, so I am surfacing it for you to re-decide rather than silently including or dropping it.
• release.config.mjs maps breaking: true → major, so the ! marker cuts 0.15.0 → 1.0.0, not 0.16.0. Same treatment — you set the version question aside; a lane found it independently.
• node-redis legacyMode: true is newly broken. Legacy mode rewrites sendCommand and redefines get but leaves defineScript registrations alone, so the old script-only read path survived it and the native path returns undefined. Not filed: legacy mode is a v3 compat shim DialCache never claims to support. A one-line README statement would close it.
• Two nits from my earlier review, withdrawn. No lane raised the unbounded watermark toString or the shared @ts-expect-error directive. Both defended the wrong boundary; dropping them.

Open, needs a live cluster

• Whether GLIDE's explicitly-routed customCommand still refreshes the slot map and retries on MOVED during resharding. If it does not, tracked cluster reads could surface raw MOVED errors where the pre-change invokeScript path did not.
• GLIDE standalone batch primary-routing rests on GLIDE's own doc comment (Batch.d.ts:3086, "Standalone Batches are executed on the primary node"), not on the Rust core. Load-bearing for finding 3's alternative: if standalone customCommand(["MGET", ...]) already reaches the primary, the entire Batch concept — the interface, the required runtime member, and the exec/unwrap branch — collapses into the same call the cluster path uses.