feat: add cluster-aware batch invalidation

[lan17]

Closes #46

Summary

Add a class-level batch form of remote invalidation while preserving the scalar API and keeping Redis topology inside the bundled adapters.

• adds RemoteInvalidationTarget and DialCache.invalidateRemoteMany(targets, futureBufferMs?)
• adds optional DialCacheRedisClient.invalidateMany with an all-settled scalar compatibility fallback
• batches node-redis work in sequential chunks of at most 1,000 commands, concurrently across targeted Cluster primaries
• batches native and scalar GLIDE work in sequential windows of at most 1,000 commands
• documents and tests non-atomic partial or ambiguous completion without promising a general throughput win

Architecture

flowchart LR
  A["invalidateRemoteMany(targets, buffer)"] --> B["validate buffer; canonicalize String(id); deduplicate"]
  B --> C["derive every watermark request before dispatch"]
  C --> D{"client.invalidateMany?"}
  D -- "no" --> E["all-settled concurrent scalar calls"]
  D -- "node-redis standalone" --> F["sequential pipelines, max 1,000 commands"]
  D -- "node-redis Cluster" --> G["calculate slot; group by current master.id"]
  G --> H["owner groups concurrent; chunks per owner sequential"]
  G --> I["unmapped slots: bounded registered scalar windows"]
  H -- "final MOVED or ASK" --> J["scalar recovery for affected chunk"]
  D -- "native GLIDE" --> K["sequential Batch or ClusterBatch chunks, max 1,000 commands"]
  D -- "scalar GLIDE wrapper" --> M["sequential scalar windows, max 1,000 calls"]
  K -- "confirmed script miss" --> L["retry affected idempotent chunk once with EVAL"]

Loading

Each Lua invalidation remains independently atomic and monotonic. The public call is one semantic adapter-dispatch boundary, not a cross-key transaction or a throughput guarantee.

Public and core behavior

• The shared future buffer is validated first; an empty target list then returns without Redis work.
• IDs canonicalize through String(id). Duplicate (keyType, canonicalId) targets collapse in first-occurrence order.
• Every watermark request is derived before dispatch, so invalid later input cannot cause validation-time partial writes.
• One invalidation metric is emitted per attempted unique target. A failed batch logs once and records one error per distinct key type.
• When a custom adapter has no batch capability, core launches every scalar invalidation concurrently and settles every result. One failure preserves its identity; multiple failures produce an ordered AggregateError.
• invalidateRemote and the watermark/key protocol are unchanged. Invalidation remains remote-only.

Adapter behavior

node-redis

• A private 1,000-command ceiling bounds how much one semantic batch queues at a time; there is no new public configuration.
• Standalone Redis uses ceil(targets / 1,000) sequential non-transactional pipelines.
• Cluster handling snapshots the adapter-visible slot table, calculates each watermark slot with the private Redis-spec CRC16/XMODEM helper, and groups mapped requests by slots[slot].master.id.
• Primary-owner groups start concurrently. Each owner uses sequential first-key-routed pipeline chunks of at most 1,000 commands, while node-redis remains responsible for selecting the connection.
• The Cluster pipeline count is sum(ceil(ownerTargets / 1,000)); it is one pipeline per targeted primary when every owner has at most 1,000 targets.
• Unmapped slots use registered scalar scripts in sequential windows of at most 1,000 concurrent calls rather than one-command EVAL pipelines.
• If a mixed-slot owner chunk ultimately fails with a direct MOVED or ASK, that chunk is retried through registered scalar invalidations so every key routes independently. node-redis first exhausts its configured maxCommandRedirections budget, 16 retries by default.
• Non-redirection errors are rethrown unchanged. A failed chunk prevents later chunks in that execution group from being submitted, while already-started groups still settle.
• multi remains optional on the structural input. Narrow pre-batch wrappers use the same bounded scalar path.
• The ceiling bounds DialCache's queued contribution per chunk; it cannot guarantee admission with a lower or already-occupied caller commandsQueueMaxLength.
• Warm concurrent scalar invalidations are already auto-pipelined, so the standalone pipeline is not presented as a steady-state throughput win. It retains bounded dispatch, reply-count validation, and one source-bearing EVAL per pipeline instead of independent full-source recovery after SCRIPT FLUSH.

Valkey GLIDE

• A private 1,000-command ceiling applies to native Batch(false) / ClusterBatch(false) dispatch and scalar-only compatibility wrappers.
• Native chunks run sequentially. GLIDE remains responsible for routing each non-atomic Cluster chunk across its target nodes.
• Scalar-only wrappers run sequential windows of at most 1,000 concurrent invokeScript calls and settle every launched call in a window.
• When Script.getHash() is available, commands use EVALSHA. A confirmed Redis NOSCRIPT or GLIDE NoScriptError retries only the affected monotonic chunk once with EVAL; unrelated failures retain their identity.
• Script handles without getHash() retain one-pass EVAL compatibility.
• A failed native chunk or scalar window prevents later chunks from starting.
• Native Cluster chunks opt into retriable server and connection errors; standalone options are unchanged.
• Cluster batch detection uses client instanceof glide.GlideClusterClient from the supplied module namespace. Forwarding wrappers must preserve that identity, or expose only scalar scripting and use the compatibility path.
• The ceiling bounds DialCache's contribution but cannot guarantee scalar-path admission against a lower or already-occupied GLIDE in-flight request limit.
• GLIDE 2.4.2 exposes script-cache misses only through formatted Error.message; the matcher intentionally recognizes the observed GLIDE form and Redis's standard token, then fails closed.

Compatibility and failure model

• invalidateMany is optional, so existing custom DialCacheRedisClient implementations keep compiling.
• Core's custom-adapter scalar fallback is all-settled but intentionally does not impose a new dispatch policy; custom adapters own any additional limits.
• No public topology/configuration API, new dependency, key-format change, Lua-protocol change, metric label, or public batch-size setting is added.
• Pipeline, scalar-window, and Cluster work can completely, partially, or ambiguously apply before rejection. Retrying the canonical target list is safe because watermarks only advance.
• The scalar public path still calls invalidate; shared internal request construction removes duplication without silently routing scalar calls through an adapter's batch method.
• Public core retains the empty-batch no-op; direct adapter entry points retain defensive empty guards, while the unreachable internal duplicate guard was removed.

Review follow-up

• primary-owner grouping replaces the original exact-slot grouping
• stale-topology MOVED/ASK recovery is explicit rather than relying on whole-pipeline first-key retries
• node-redis and GLIDE native/scalar work is bounded to 1,000-command sequential chunks or windows
• the standalone node-redis pipeline remains for bounded dispatch, reply validation, and efficient cold-script recovery, not a claimed warm throughput advantage
• the unreachable synthetic MultiErrorReply path and tests were removed for pinned node-redis 4.7.1
• unmapped Cluster slots use bounded registered scalar routing
• GLIDE batches use per-chunk EVALSHA, cold-cache recovery, and Cluster retry flags
• node-redis narrow-wrapper compatibility, fixture behavior, attempted-target metrics, awaitAll(): Promise<void>, and Symbol.hasInstance rationale are covered
• the benchmark now measures node-redis and Valkey GLIDE with fresh alternating scalar/batch runs and median reporting
• documentation describes an adapter-dependent semantic operation rather than promising fewer round trips or higher throughput

Validation

Validated at c8869e4 on Node.js 22.22.0, rebased onto main@34bd022:

• corepack pnpm check
  • typecheck passed
  • 449 unit tests passed
  • 97.17% statement coverage
  • ESM/CJS builds, declarations, and packed-consumer tests passed
• corepack pnpm test:integration
  • 99 tests passed across Redis 6.2, Valkey 8, node-redis, GLIDE, and a three-primary Redis 7 Cluster
  • live node-redis coverage selects two distinct slots on one primary plus one on another and asserts exactly two pipelines
  • live node-redis and GLIDE coverage flushes scripts on every primary, advances every watermark, refreshes tracked values, and avoids CROSSSLOT
  • GLIDE Cluster proves two executions on the cold EVALSHA/EVAL path and one on the next warm batch
• repeated standalone Redis 7 benchmark, five alternating repetitions per adapter and size, with 10,000-command/request benchmark headroom
  • node-redis: 10 targets 0.75/0.60 ms (1.25x), 100 targets 1.85/1.74 ms (1.06x), 1,000 targets 6.27/6.26 ms (1.00x)
  • Valkey GLIDE: 10 targets 0.51/0.50 ms (1.02x), 100 targets 1.52/1.54 ms (0.99x), 1,000 targets 9.42/8.77 ms (1.07x)
  • each pair is scalar/batch with scalar-to-batch ratio; directional only, every watermark assertion passed, and no timing threshold is enforced
• git diff --check passed

[lan17]

Review: cluster-aware batch invalidation

Reviewed at d4d7c62, verified against the pinned peers in node_modules (redis@4.7.1 / @redis/client@1.6.1, @valkey/valkey-glide@2.4.2) rather than from the diff alone.

Up front: I found no correctness bug in either adapter. Nothing crashes, nothing writes a wrong watermark, no error is swallowed. Every finding below is performance, robustness, or API consistency. The most consequential one is that the node-redis Cluster path — the PR's headline capability — is correct but doesn't actually batch.

Verified as claimed

• The EVAL-first pipeline trick is real. @redis/client/dist/lib/multi-command.js:37 addScript emits EVAL <source> for a script's first use within a multi and EVALSHA thereafter, tracked per-multi via scriptsInUse. Redis executes a pipeline in order on one connection, so the leading EVAL populates the script cache before the trailing EVALSHAs run. SCRIPT FLUSH recovery works with no extra round trip.
• Cluster detection and routing are correct for the pinned peer. slots is a getter returning Shard[] on RedisCluster and absent on RedisClient. MULTI(routing) threads routing into #firstKey, which execAsPipeline uses for node selection. Peer is pinned ~4.7.1, so v5 divergence is out of scope.
• Per-command Redis errors are not masked. #addMultiCommands rejects on the first ErrorReply (client/commands-queue.js:63), so a real MOVED/NOSCRIPT/OOM propagates, and MOVED triggers rediscover() plus an idempotent whole-pipeline retry inside #execute (up to maxCommandRedirections, default 16). The reply-count and shape validation in executeInvalidationPipeline is defensive-only, which is fine.
• redisClusterSlot matches Redis exactly: first {, first } after it, non-empty span, else whole key; CRC16/XMODEM over UTF-8 bytes; % 16384 is equivalent to & 16383. The live CLUSTER KEYSLOT parity assertions (including foo{}{bar} and Unicode) are the right way to prove this.
• RedisClusterMultiCommand does no client-side slot validation. #firstKey is assigned once via ?? and later commands only append — no CROSSSLOT pre-check. This matters for the fix proposed below.
• Failure semantics. awaitAll settles every launched operation before rejecting, preserves a single error's identity, and produces an order-deterministic AggregateError otherwise. GLIDE batch work sits inside invokeTracked, so dispose() cannot release a live script handle. Tests pin all of this.

---

1. node-redis Cluster: correct, but it does not batch — and costs ~19x the bytes

Watermark keys are {namespace:keyType:id}#watermark, so the hash tag is unique per target and slots are effectively random. Grouping by exact slot yields roughly 16384 * (1 - (1 - 1/16384)^N) partitions: ~99.7 for 100 targets, ~970 for 1,000. So invalidateMany issues ~N separate multi().execAsPipeline() calls of one command each.

Worse, each partition's single command is that pipeline's first script use, so it goes out as EVAL <full source>. INVALIDATE_CACHE_SCRIPT is ~1.7 KB; the scalar path sends EVALSHA (~90 B). Compared against Promise.all(targets.map(invalidateRemote)):

• round trips: no reduction — #tick() corks the socket and drains the entire pending queue in one write, so concurrently issued scalar commands are already coalesced
• bytes on the wire: ~19x more
• behavior: identical

That socket-level coalescing is also why the standalone benchmark only shows 1.01x / 1.07x / 1.36x. The pipeline's real gain there is per-command bookkeeping, not round trips.

Fix: partition by owning primary, not by slot

The key insight is that CROSSSLOT is a per-command constraint, not a per-batch one. A pipeline is not a transaction; it is N independent commands sharing one socket write. Redis only rejects CROSSSLOT when a single command spans slots. Every command here is one EVAL with numkeys=1, so a partition only needs each key to live on the same primary, not in the same slot — and a primary owns ~5,461 slots in a 3-primary cluster.

Type the slot map instead of unknown:

interface NodeRedisClusterShard {
  readonly master: { readonly id: string };
}

interface NodeRedisScriptClient {
  // ...existing script members...
  multi(routing?: NodeRedisArgument): NodeRedisMultiCommand;
  readonly slots?: readonly NodeRedisClusterShard[];
}

RedisClusterType.slots is Shard[] whose master carries id: string, so it stays structurally assignable; standalone RedisClientType has no slots and the property is optional. This replaces isNodeRedisClusterClient — detection and lookup become the same read.

Partition by primary, with a pipeline size cap:

type NodeRedisInvalidationRequest = {
  readonly watermarkKey: string;
  readonly futureBufferMs: number;
};

// A pipeline is not a transaction, so a partition only needs every key to live on
// one primary rather than in one slot. Cap commands per pipeline so a large batch
// cannot monopolize a connection or overflow commandsQueueMaxLength.
const MAX_PIPELINE_COMMANDS = 1_000;

function invalidationPartitions(
  client: NodeRedisScriptClient,
  requests: readonly NodeRedisInvalidationRequest[],
): NodeRedisInvalidationRequest[][] {
  // Read the slot map once: rediscovery replaces the array wholesale
  // (cluster-slots.js:171 `this.slots = new Array(SLOTS)`), so one snapshot keeps
  // the whole grouping consistent with a single topology view.
  const shards = Array.isArray(client.slots) ? client.slots : undefined;
  if (shards === undefined) {
    return [[...requests]];
  }

  const byPrimary = new Map<string, NodeRedisInvalidationRequest[]>();
  for (const request of requests) {
    const slot = redisClusterSlot(request.watermarkKey);
    // An unmapped slot degrades to its own key-routed partition rather than
    // guessing an owner or failing the batch.
    const primary = shards[slot]?.master.id ?? `slot:${slot}`;
    const partition = byPrimary.get(primary);
    if (partition === undefined) {
      byPrimary.set(primary, [request]);
    } else {
      partition.push(request);
    }
  }

  return [...byPrimary.values()];
}

function* chunked<T>(items: readonly T[], size: number): Generator<readonly T[]> {
  for (let index = 0; index < items.length; index += size) {
    yield items.slice(index, index + size);
  }
}

Dispatch (executeInvalidationPipeline stays exactly as written):

async invalidateMany(requests) {
  if (requests.length === 0) {
    return;
  }
  await awaitAll(
    invalidationPartitions(client, requests).map(async (partition) => {
      // Concurrent across primaries, sequential within one, so a single
      // connection never queues an unbounded number of commands.
      for (const chunk of chunked(partition, MAX_PIPELINE_COMMANDS)) {
        await executeInvalidationPipeline(client, chunk);
      }
    }),
    "Multiple DialCache invalidation partitions failed",
  );
},

Why each edge case holds:

• Routing. executeInvalidationPipeline already passes client.multi(first.watermarkKey), so node-redis resolves the target via slots.getClient(firstKey) — the same primary the partition was grouped for. Since RedisClusterMultiCommand performs no slot validation, the mixed-slot pipeline is accepted as-is.
• Stale topology. A wrong snapshot means the affected command replies MOVED, which rejects the pipeline's Promise.all, which #execute catches → rediscover() → whole-pipeline retry. Re-running a monotonic watermark advance is a no-op, so the retry is free.
• Unassigned slots. Mid-resharding or pre-connect, shards[slot] is undefined and that request becomes its own key-routed partition — today's behavior, but only for the keys that need it.
• Standalone. No slots → one partition → chunked pipelines. Same as today, plus the size cap.
• Script payload. Each multi() gets a fresh scriptsInUse, so every chunk pays one EVAL and the rest are EVALSHA. At 1,000 commands per chunk that is ~0.2% overhead, versus 100% today on Cluster.

Net for 1,000 targets on 3 primaries: 3 pipelines instead of ~970.

Tests this needs

fakeBatchClient({ cluster: true }) currently sets slots: [], so every lookup misses and only the fallback path runs — the existing exact-slot test would still pass against the new code without proving anything. Give the fake a real map:

function fakeClusterSlots(assignments: ReadonlyMap<number, string>): unknown[] {
  const slots = new Array(16_384);
  for (const [slot, id] of assignments) {
    slots[slot] = { master: { id } };
  }
  return slots;
}

Then add:

• Groups by primary, not slot — three keys in three distinct slots, two of which map to primary-a and one to primary-b; assert client.multi is called twice and check partition contents. This is the test whose absence let the current behavior through.
• Unmapped slot isolates — a key whose slot has no assignment gets its own pipeline while mapped keys still group.
• Chunking is sequential per primary — MAX_PIPELINE_COMMANDS + 1 requests on one primary produce two pipelines; gate the first execAsPipeline on a deferred to assert the second multi() is not created until the first resolves.
• Integration — reuse selectCrossPrimaryBatchIds, spy on cluster.multi, and assert the call count is <= masters.length while every watermark still advances and tracked values refresh. That upgrades the existing live test from "it works" to "it batches."

Deletion

groupByRedisClusterSlot becomes unused — delete it from src/internal/redis-cluster-slot.ts and keep only redisClusterSlot. Move its test case ("groups different hash tags that collide on the same exact slot") into test/node-redis.test.ts as the by-primary grouping test; the CRC16 and hash-tag parity cases stay put.

That split also gets the layering right: CRC16 is client-agnostic and belongs in internal/, while slots[].master.id is node-redis-specific and belongs in the adapter.

---

2. GLIDE inlines the entire Lua script into every batch command

src/valkey-glide.ts:202-208 builds customCommand(["EVAL", INVALIDATE_CACHE_SCRIPT, "1", key, buffer]) per target — ~1.7 KB x N, so a 1,000-target batch ships ~1.7 MB of Lua text versus ~90 KB via EVALSHA.

BaseBatch genuinely has no script method (no invokeScript), so customCommand is forced. But GLIDE's native Script exposes getHash() (build-ts/native.d.ts:114). Emit EVALSHA <hash> and, if exec rejects with a NOSCRIPT error, retry the whole batch with EVAL. Retry is safe by the same monotonicity argument already documented. That needs an optional getHash?(): string on ValkeyGlideScriptHandle with an EVAL fallback when absent — consistent with how Batch/exec are already treated as optional capabilities.

Worth noting the GLIDE path is otherwise the right design: because each EVAL declares numkeys=1, glide-core routes per key, groups by node, pipelines per node, and reassembles replies in queue order. It does for free exactly what finding 1 asks the node-redis adapter to do by hand. Not passing route in the options is correct — route would pin the whole batch to one node and defeat that.

3. The GLIDE batch path is less resilient than the scalar path it replaces

Batch retries are opt-in via ClusterBatchRetryStrategy and default off, so invalidateMany currently gets no TRYAGAIN and no connection-error retry, where single invokeScript calls go through the core's normal redirect handling.

DialCache is the rare workload where GLIDE's documented cautions do not apply: reordering across slots is meaningless because each watermark key is independent, and duplicate execution is harmless because the script only advances a watermark monotonically. Passing retryStrategy: { retryServerError: true, retryConnectionError: true } buys transparent recovery from slot-migration TRYAGAIN and connection blips for free.

4. NodeRedisScriptClient.multi is required while GLIDE's exec is optional

multi was added as a required member of the parameter type for createNodeRedisDialCacheClient. Real RedisClientType/RedisClusterType both have it, so production callers are fine — but any hand-rolled narrow structural object or test double now fails to compile. The GLIDE side deliberately avoided this by making exec? optional with scalar fallback, and the README advertises that compatibility for GLIDE. The asymmetry is unexplained; make it multi? and fall back to the scalar invalidate loop when absent (awaitAll is already there).

scripts/test-package.mjs never constructs a narrow node-redis client, so nothing catches this — worth adding beside the existing scalar-only DialCacheRedisClient consumer check.

5. GLIDE cluster detection misses wrappers that forward exec

client instanceof GlideClusterClient (src/valkey-glide.ts:197) is false for a structural wrapper around a cluster client, so such a wrapper receives StandaloneBatch. Harmless in 2.4.2 — GlideClusterClient.exec reads only batch.commands, batch.isAtomic, and batch.setCommandsIndexes, all from BaseBatch — but that is a coincidence inside a ^2.4.2 range, and the types keep the classes separate precisely because ClusterBatch carries cluster routing options. One README sentence for wrapper authors would cover it.

6. The Cluster integration fixture is mutated suite-wide

configureAdvertisedClusterEndpoint sets cluster-announce-hostname, cluster-preferred-endpoint-type, and cluster-announce-port on every container in beforeAll, so every pre-existing node-redis Cluster test in the file now runs against hostname-advertised nodes with host-mapped ports in MOVED replies. Green today, but if it regresses it will look like an unrelated flake. A separate fixture would isolate the blast radius; at minimum extend the existing comment to say the change is suite-wide.

7. Benchmark methodology

One iteration per size, always scalar-then-batch on the same connection, no interleaving or repetition — so the batch leg always runs on warmer client state. The script is explicit that results are directional and asserts no threshold, which covers it, but 1.01x and 1.07x are inside plausible noise; a couple of alternating repetitions reporting a median would make the numbers mean something. It also only covers standalone node-redis, the one topology where the design is uncontroversial. A Cluster leg would have surfaced finding 1.

8. Minor

• awaitAll builds and returns values: T[], and all four call sites discard it. Dead accumulation on a path that handles 1,000-element batches — make it Promise<void>.
• Three layered empty-batch guards (invalidateRemoteMany, RedisCache.invalidateMany, each adapter). Keep the adapter guards (direct adapter use is legitimate) and drop the now-unreachable one in RedisCache.
• RedisCache.invalidateMany duplicates invalidate's body instead of the scalar delegating to it; they must now stay in sync as the request shape evolves.
• redisClusterSlot reimplements cluster-key-slot@1.1.2, which node-redis itself uses for routing (cluster-slots.js:22). Hand-rolling correctly keeps src/internal/ dependency-free since core and GLIDE share that directory, and the live parity test covers drift — noting it so the choice reads as deliberate.
• Docs say each unique target "records one invalidation metric." It records one per attempt — emitted before dispatch, retained on failure, and (per the test at test/dialcache-invalidation.test.ts) emitted even for targets whose key derivation later throws. Consistent with scalar invalidateRemote, so no change needed, but "attempted invalidation targets" is the accurate counter description.
• ValkeyGlideClusterClientConstructor typed as { [Symbol.hasInstance](value: unknown): boolean } is the right workaround for a class with no public constructor, but the doc comment does not say that. One line saves the next reader a detour.

---

Test coverage

Strong, and better than the feature strictly needed: canonical dedup across 1/"1"/1n, empty-batch no-op, pre-dispatch validation, single-error identity vs. AggregateError, settle-before-reject for both the scalar fallback and Cluster partitions, malformed/short/null batch replies for both adapters, dispose-during-batch, and live three-primary Cluster coverage for node-redis and GLIDE including all-node SCRIPT FLUSH recovery and CLUSTER KEYSLOT parity. expect(executeBatch).toHaveBeenCalledOnce() is the right way to prove GLIDE took the native path rather than silently falling back.

The gap is partition-count assertions — nothing checks how many pipelines a Cluster batch produces, which is exactly why finding 1 slipped through.

Security

Nothing new. No key-format, Lua-protocol, or metric-label changes. Error metrics stay bounded by distinct key type rather than id, so cardinality is safe under large batches. Adding EVALSHA per finding 2 would not change that.

Verdict

Sound design, unusually good docs, and the hard parts — script-cache recovery, slot math, settle-before-reject — are right. Nothing blocks on correctness. What blocks on value is finding 1: the PR is titled "cluster-aware batch invalidation," and on node-redis Cluster it currently produces roughly one pipeline per target with a full script body each time, which is strictly more expensive than the loop it replaces.

Findings 1 and 2 before merge; 3 and 4 are cheap enough to take in the same pass; the rest can follow.

[lan17]

Implementation plan after review

We aligned on the key requirement: node-redis Cluster batching should produce approximately M pipelines for M targeted primaries, not one pipeline per unique hash slot. DialCache will calculate slots and form the groups inside the adapter, while node-redis will continue choosing and routing to the actual node from each pipeline's first key.

1. Rework node-redis Cluster partitioning

• Keep the private CRC16/XMODEM slot calculator, but replace exact-slot grouping with adapter-local primary-owner grouping.
• Give the structural client only the minimum topology shape we need: an optional slot array whose entries expose master.id. Take one slot-table snapshot per batch.
• For each invalidation request:
  1. calculate its watermark key's slot;
  2. resolve the current owner from slots[slot]?.master.id;
  3. append it to that primary's partition while preserving input order within the partition.
• Treat an absent slots property as standalone Redis and retain one non-transactional pipeline.
• Treat an unmapped slot conservatively: isolate that request in its own key-routed partition instead of guessing an owner.
• Execute one multi(firstWatermarkKey).execAsPipeline() per partition, concurrently across partitions, and continue settling every launched partition before rejecting.
• Keep topology out of core and do not expose a public cluster abstraction. The first watermark key remains only a routing hint; the adapter never selects a node connection itself.
• Remove the now-unused exact-slot grouping helper, but retain and continue testing the slot calculator.

2. Handle topology changes explicitly

We should not rely on the review's claim that node-redis can always repair a mixed-slot pipeline through its native whole-pipeline MOVED retry. In node-redis 4.7.1, that retry keeps routing the complete pipeline from its first key, so an independently moved non-first slot can exhaust the redirect budget.

• If a primary-group pipeline ultimately rejects with MOVED or ASK, retry that partition through the registered scalar invalidation command for each key. That lets node-redis route and redirect each key independently.
• Wait for all scalar recovery attempts and preserve the existing deterministic single-error/AggregateError behavior.
• Re-throw non-redirection pipeline errors unchanged.
• Full-partition retry is safe because watermark advancement is monotonic and the public contract already permits retry after partial or ambiguous completion.

3. Preserve node-redis structural compatibility

• Make NodeRedisScriptClient.multi optional.
• When a narrow structural client does not expose multi, fall back to concurrent scalar invalidations and wait for every settlement.
• Add packed-consumer coverage proving a pre-batch, scalar-only node-redis wrapper still typechecks and works.
• Do not add the proposed hard-coded 1,000-command pipeline cap in this pass. It cannot guarantee compatibility with a caller's potentially lower or already-occupied commandsQueueMaxLength, and it would change the standalone one-pipeline contract. Pipeline-size policy can be designed separately if real workloads require it.

4. Reduce GLIDE batch script payloads

• Add optional getHash?(): string support to the structural script handle so older/minimal wrappers remain valid.
• When the hash is available, build the native batch with EVALSHA.
• On a confirmed script-cache miss, retry the complete batch once with EVAL; otherwise preserve the original failure.
• Recognize both Redis's normal NOSCRIPT form and GLIDE 2.4.2's observed NoScriptError: No matching script message. GLIDE does not expose a stable error code, so tests will pin these supported forms and prove unrelated errors are not retried.
• If getHash is absent, retain the current one-pass EVAL behavior.
• Keep the retry inside the existing in-flight tracking so dispose() cannot release script handles during either attempt.
• For a detected native GLIDE Cluster client, enable the documented non-atomic batch retry strategy for retriable server and connection errors. Duplicate/reordered execution is safe for independent monotonic watermarks; standalone behavior remains unchanged.

5. Expand tests around the actual batching guarantee

node-redis unit tests

• Distinct slots owned by the same primary share one pipeline.
• Requests spanning two owners create exactly two pipelines with the correct routing keys and preserved per-group order.
• Unmapped slots are isolated without disrupting mapped primary groups.
• A final MOVED/ASK pipeline error switches to scalar per-key recovery; unrelated errors remain unchanged.
• Missing multi uses scalar fallback, waits for every operation, validates replies, and preserves deterministic error aggregation.
• Existing empty-batch, malformed-reply, SCRIPT FLUSH, and settle-before-reject behavior remains covered.

live Redis Cluster integration

• Select at least two different slots on one primary plus a target on another primary.
• Spy on cluster.multi and assert the number of pipelines equals the number of distinct targeted primary owners—not the number of keys or slots.
• Continue verifying CLUSTER KEYSLOT parity, all-primary SCRIPT FLUSH recovery, every watermark advancing, tracked-value refresh, and no CROSSSLOT failure.

GLIDE tests

• Warm EVALSHA uses one native batch without embedding the Lua source per target.
• A cold script cache performs one failed EVALSHA batch followed by one successful EVAL batch.
• No-getHash runtimes retain the existing one-pass source behavior.
• Non-script errors are preserved and do not trigger source retry.
• The real GLIDE Cluster test should expect two executions immediately after all-primary SCRIPT FLUSH, then one execution for a subsequent warm batch.
• Assert cluster retry options only on the detected native Cluster path.

6. Documentation, benchmark, and small consistency fixes

• Update the README, PR architecture description, and Add batched remote invalidation with cluster-aware adapter routing #46 from “one pipeline per exact slot” to “one pipeline per targeted current primary, with conservative fallback for missing/stale topology.”
• Document that forwarding GLIDE Cluster wrappers must preserve native instanceof identity to receive ClusterBatch; scalar-only wrappers continue using the safe scalar path.
• Clarify that the advertised-endpoint configuration is a suite-wide choice for the shared Cluster fixture; do not duplicate the three-container fixture.
• Change invalidation metric wording to “attempted unique invalidation targets.”
• Make internal awaitAll return Promise<void> because every caller discards fulfilled values.
• Add the short rationale for the structural Symbol.hasInstance constructor type.
• Harden the maintainer benchmark with repeated alternating scalar/batch order and median reporting, while retaining semantic assertions and no timing threshold. Use the deterministic live pipeline-count assertion—not timing—as the Cluster batching proof.
• Update the PR validation results after rerunning the benchmark.

7. Explicitly out of scope for this pass

• No public topology/configuration API.
• No new Redis slot dependency; retain the verified private implementation.
• No separate Cluster fixture.
• No scalar-to-batch delegation refactor in RedisCache.
• No hard-coded batch-size limit.

8. Validation and handoff

Run on Node.js 22.22.0:

1. focused node-redis, GLIDE, slot, invalidation, and Cluster tests;
2. corepack pnpm typecheck;
3. corepack pnpm test;
4. corepack pnpm build;
5. corepack pnpm test:package;
6. corepack pnpm test:integration;
7. corepack pnpm benchmark:batch-invalidation;
8. git diff --check.

Then push the validated update, refresh the PR body/issue wording and benchmark numbers, and summarize which review findings were implemented versus deliberately deferred.

[lan17]

Review follow-up implemented

Implemented the agreed plan at b9d5d81, rebased onto main@34bd022 (v0.14.0).

What changed

• node-redis Cluster now batches by targeted current primary, not exact slot. The adapter calculates each watermark slot, snapshots slots, groups by master.id, and submits one first-key-routed pipeline per owner. The live three-primary test selects two distinct slots on one primary plus one on another and asserts exactly 2 pipelines for 2 targeted primaries.
• Topology changes recover conservatively. A final direct or pipeline-aggregate MOVED/ASK retries only that owner partition through independently routed scalar invalidations. Mixed or unrelated failures preserve the original error.
• Narrow node-redis wrappers remain compatible. multi is optional; the fallback launches scalar registered scripts, validates every reply, waits for every settlement, and preserves single-error/AggregateError behavior. Packed-consumer coverage pins this.
• GLIDE native batches use EVALSHA. A confirmed Redis NOSCRIPT or GLIDE NoScriptError retries the complete monotonic batch once with EVAL; runtimes without getHash() retain one-pass EVAL. Unrelated errors are not retried.
• GLIDE Cluster enables retriable server/connection behavior only on the native Cluster path. The live test proves 2 executions after all-primary SCRIPT FLUSH and 1 execution for the next warm batch.
• Follow-up cleanup landed. The shared fixture comment, forwarding-wrapper identity contract, attempted-unique metric wording, awaitAll(): Promise<void>, and Symbol.hasInstance rationale are updated.
• Benchmark methodology is stronger. It now uses fresh targets, alternates scalar/batch order, reports five-sample medians, asserts every watermark, and intentionally has no timing threshold.

Deliberately deferred

• No fixed 1,000-command cap. It cannot guarantee compatibility with a lower or already-occupied caller commandsQueueMaxLength, and it would change the standalone one-pipeline contract.
• No public topology/configuration API, new slot dependency, separate Cluster fixture, scalar-to-batch core refactor, or batch-size policy.

Validation

• corepack pnpm check: 441 unit tests, 96.99% statement coverage, typecheck, ESM/CJS builds, declarations, and packed consumers passed.
• corepack pnpm test:integration: all 99 Redis/Valkey integration tests passed, including live node-redis and GLIDE Cluster paths.
• Repeated standalone Redis 7 medians:
  • 10 targets: scalar 0.78 ms / batch 0.82 ms (0.96x)
  • 100 targets: scalar 1.41 ms / batch 1.33 ms (1.07x)
  • 1,000 targets: scalar 6.36 ms / batch 6.27 ms (1.01x)
• git diff --check passed.
• Exact-head GitHub checks are green: CI, CodeQL, and PR-title validation.

The PR body and #46 now describe primary-owner grouping and conservative topology recovery instead of exact-slot partitioning.

[lan17]

Re-review after perf: optimize batch invalidation adapters

Reviewed at b9d5d81 (rebased onto 34bd022 / 0.14.0). The delta since my last review is one commit, +639/−153 across 12 files. CI green. Verified against the pinned peers in node_modules (@redis/client@1.6.1, @valkey/valkey-glide@2.4.2) rather than from the diff alone.

What landed

Finding 1 (node-redis Cluster) — fixed, and better than what I proposed. partitionClusterInvalidations groups by slots[slot]?.master?.id, preserves first-encounter partition order, and isolates unmapped slots. The integration test now asserts client.multi is called exactly targetedPrimaryOwners.size times — precisely the assertion whose absence hid the original behavior.

The part I did not suggest and should have: the isRedisClusterRedirection → executeScalarInvalidations recovery. That closes a hole in my own recommendation. node-redis's redirect loop re-resolves only #firstKey (cluster/index.js:216), so if a non-first key in a by-owner partition has moved, retrying re-sends the same pipeline to the same node and receives the same MOVED — 16 times — then throws. Grouping by owner is what creates that possibility. The scalar fallback converts a guaranteed failure into success, and it is safe because registered scalar calls route each key independently and watermark advancement is idempotent. Good catch.

Finding 2 (GLIDE script payload) — fixed. EVALSHA via scripts.invalidate.getHash?.(), one EVAL retry on a script-cache miss, and direct EVAL when the runtime's handle exposes no getHash. The integration test is the right shape: two exec calls for the first batch after scriptFlush (miss, then retry), then one for the warm batch.

Finding 3 (GLIDE retry strategy) — fixed, with a detail I would have gotten wrong. Both flags on for cluster, and correctly not for standalone: BatchOptions carries no retryStrategy because GLIDE scopes retries to ClusterBatchOptions. The README states exactly that.

Findings 4, 5, 7, 8 — fixed. multi? optional with scalar fallback plus a narrow node-redis client in test-package.mjs; the instanceof wrapper limitation documented as explicitly unsupported, with a code comment; benchmark now reports medians over five alternating repetitions with DIALCACHE_BENCH_REPETITIONS; awaitAll returns void; groupByRedisClusterSlot deleted along with its test; Symbol.hasInstance comment added; metric docs and Prometheus help text say "attempted unique."

Finding 6 — the suite-wide fixture mutation is now documented rather than isolated. Reasonable call, and the comment says so plainly.

Still open

1. No batch size cap — now the top item

This was minor when Cluster partitions held ~1 command each. By-owner grouping changes that: 100k targets across 3 primaries becomes three ~33k-command pipelines. That trips commandsQueueMaxLength where configured, and otherwise blocks a primary for the duration. It also bounds the new recovery path's worst case — a 33k-command partition that hits MOVED currently degrades to 33k scalar calls, each with its own 16-redirect budget.

// A pipeline is not a transaction, so partition size is a resource decision, not
// a correctness one. Cap it so one batch cannot monopolize a connection.
const MAX_PIPELINE_COMMANDS = 1_000;

function* chunked<T>(items: readonly T[], size: number): Generator<readonly T[]> {
  for (let index = 0; index < items.length; index += size) {
    yield items.slice(index, index + size);
  }
}

await awaitAll(
  partitions.map(async (partition) => {
    // Concurrent across primaries, sequential within one.
    for (const chunk of chunked(partition, MAX_PIPELINE_COMMANDS)) {
      await executeInvalidationPipeline(client, chunk);
    }
  }),
  "Multiple DialCache invalidation partitions failed",
);

At 1,000 commands per chunk the extra EVAL per chunk is ~0.2% overhead. This also covers standalone, which is currently one unbounded pipeline.

2. The MultiErrorReply branch in isRedisClusterRedirection is unreachable

execAsPipeline routes through #addMultiCommands, whose Promise.all rejects with the first raw ErrorReply (client/commands-queue.js:63) before transformReplies can construct a MultiErrorReply. So with node-redis 4.7.1 only isDirectRedisClusterRedirection can fire from a pipeline; the replies/errorIndexes path is reachable only for exec() transactions, which this code never issues.

The two new tests build synthetic Object.assign(new Error(...), { replies, errorIndexes }) objects, so they pass without exercising anything reachable. Either drop the branch or add a comment marking it forward-defensive — given the project's no-dead-API stance, I would drop it.

3. The recovery burns the full redirect budget first

Because #execute retries MOVED/ASK up to maxCommandRedirections (default 16) before rethrowing, the scalar fallback only engages after 16 pipeline round trips for that partition. Correct, but operators sizing timeouts through a reshard will care. Worth one clause in the README paragraph that already describes the fallback.

4. Unmapped slots would be better served by the scalar path

An isolated one-command pipeline sends EVAL with the full ~1.7 KB source, whereas the registered scalar call sends EVALSHA (~90 B) with its own NOSCRIPT recovery. Since executeScalarInvalidations already exists, routing unmapped requests there is both smaller and cheaper than building a pipeline for them.

Low priority: I checked whether a rediscovery window could make this common, and it cannot. #resetSlots() and the repopulation loop run synchronously inside #discover with no await between them, so there is no observable empty-map window. This only affects genuinely unassigned slots.

5. Leftovers from the previous review

• Three layered empty-batch guards (invalidateRemoteMany, RedisCache.invalidateMany, each adapter); the RedisCache one is unreachable from the core path.
• RedisCache.invalidateMany still duplicates invalidate's body rather than delegating to it, so the two must stay in sync as the request shape evolves.

6. isScriptCacheMiss is the one message-format-gated behavior in either adapter

The two-pattern match (raw NOSCRIPT plus GLIDE's NoScriptError: No matching script) is the right hedge, a false positive costs only a wasted EVAL retry, and the integration test pins the real GLIDE string — so this is fine as built. Noting it only so the coupling is known if GLIDE reformats errors.

Assessment

The two findings that mattered are properly fixed rather than papered over, and the tests now assert the mechanism — pipeline count per primary, cold-versus-warm exec count — instead of just the outcome. The redirection recovery is a genuine improvement over what I suggested.

Item 1 is the only thing I would still want before merge. Items 2 through 6 are cleanup.

[lan17]

Addressed the re-review in 5e09ac1.

1. Bounded batch work: node-redis now uses a private 1,000-command ceiling. Standalone chunks run sequentially; Cluster primary-owner groups run concurrently with sequential chunks per owner. The same ceiling applies to no-multi, unmapped-slot, and redirection scalar routing, so incomplete topology cannot recreate an unbounded scalar fan-out. A failed chunk stops later chunks only in that execution group while already-started groups settle. The cap bounds DialCache's contribution but does not claim to override a lower or already-occupied caller commandsQueueMaxLength.
2. Reachable redirection shape only: removed the synthetic MultiErrorReply branch and its aggregate tests. Recovery now recognizes the direct MOVED / ASK error shape surfaced by pinned node-redis 4.7.1 execAsPipeline().
3. Redirect latency: README now states that scalar recovery begins after node-redis exhausts maxCommandRedirections, 16 retries by default.
4. Unmapped slots: these now use registered scalar scripts, grouped into bounded sequential windows, rather than one-command pipelines carrying the Lua source.
5. Cleanup: removed the unreachable internal empty guard and centralized RedisInvalidationRequest construction. I deliberately kept scalar RedisCache.invalidate() on client.invalidate() instead of delegating through invalidateMany(): delegation would change the established scalar adapter route and command behavior, not just remove duplication.
6. GLIDE coupling: added the code comment documenting that 2.4.2 exposes script misses through formatted Error.message and that the matcher intentionally fails closed outside the observed GLIDE and standard Redis forms.

The deterministic unit coverage now includes 1,001-command standalone and Cluster batches, concurrent owners with sequential owner chunks, stop-on-chunk-failure, and 1,001 unmapped scalar requests proving the second window is not dispatched early.

Validation on Node.js 22.22.0:

• corepack pnpm check: 443 unit tests, 97.16% statements, build/declarations/package consumer passed
• corepack pnpm test:integration: 99 live tests passed
• standalone Redis 7 benchmark rerun at 10/100/1,000 targets; all watermark assertions passed
• git diff --check passed

[lan17]

Re-review after 5e09ac1

Read the implementation plan, the follow-up summary, and the commit. CI green. The PR body and issue wording are fully updated — no stale exact-slot or 1.36x claims left anywhere.

Everything from the last review landed

The 1,000-command ceiling is applied across all five dispatch paths (standalone chunks, Cluster owner chunks, unmapped-scalar windows, no-multi fallback, and redirection recovery), the MultiErrorReply branch and its synthetic tests are gone, the 16-retry recovery latency is documented, unmapped slots route through registered scalar scripts, redisInvalidationRequest is centralized, and the GLIDE matcher has its coupling comment.

Two pushbacks were correct

• Declining the RedisCache.invalidate → invalidateMany delegation. Right call for the right reason. Delegating would route a single invalidation through the batch path — pipeline-embedded EVAL instead of the registered script's EVALSHA, plus a different error surface. That is a behavior change dressed up as deduplication. Centralizing only the request construction was the correct read of what was actually duplicated.
• The original objection to the cap. "It cannot guarantee compatibility with a caller's lower or already-occupied commandsQueueMaxLength" is true. Rather than dropping the objection when implementing the cap, the README now documents the limitation instead of overclaiming. That is the honest version.

The unmapped-slot handling also came out better than what I proposed: one scalar partition collecting every unmapped request, rather than my per-request isolation.

The new benchmark numbers change the story

targets	before (1 sample, batch always second)	after (median of 5, alternating)
10	1.01x	0.96x
100	1.07x	1.07x
1,000	1.36x	1.00x

The 1.36x was measurement artifact — batch always ran second on a warm connection. With alternating order it disappears. That is exactly what the mechanism predicts: #tick() corks the socket and drains the entire pending command queue in one write, so Promise.all(targets.map(invalidateRemote)) is already a pipeline. The standalone batch path's only remaining edge is fewer per-command bookkeeping objects, and that measures as zero.

So: does standalone node-redis batching earn its keep? It is now a chunk loop, a reply-count check, and a protocol error class for 1.00x. Routing non-cluster clients through executeScalarInvalidations instead would delete a path, delete the standalone chunking semantics from the README, and measure identically.

I lean toward keeping it, but for a reason worth stating explicitly rather than for throughput: it is the only path that validates reply count, and it keeps one code shape across topologies. If it stays, the docs should not imply a standalone throughput win — "optimized for fewer client round trips" currently reads as one when the measurement says otherwise. The real value of this feature is the Cluster path (M pipelines for M targeted primaries) and GLIDE, where glide-core genuinely collapses per-node round trips.

Two gaps the cap work exposed

1. The ceiling is node-redis-only

executeBatch in the GLIDE adapter still loops over every request with no chunking, so a 100k-target GLIDE batch is one exec carrying 100k customCommands — one very large protobuf request, and per-node pipelines of roughly 33k on a three-primary cluster. The README's "bounds DialCache's queued contribution per execution chunk" describes node-redis only.

This is defensible: GLIDE has no commandsQueueMaxLength analog and buffers in its Rust core. But "bounded batch work" is now an adapter-specific property while the docs read as a global one. Either bound the GLIDE batch too, or scope the claim.

2. There is no GLIDE benchmark at all

Every number in the PR is node-redis standalone — the one topology where batching demonstrably does nothing. The GLIDE path is where the mechanism should actually pay off: one exec fans out to per-node pipelines, and GLIDE's higher per-call protobuf overhead makes amortization more valuable than it is for node-redis. It is also the only path with zero measurement.

Given that the standalone numbers just deflated under better methodology, the same rigor should be pointed at the claim that is actually load-bearing.

The trade the cap introduces, stated plainly

I pushed for the cap, so I should name its cost: 10,000 standalone targets is now 10 sequential round trips where it used to be one socket write, while Promise.all(scalar) stays at roughly one. Above ~1,000 targets the batch path is slower than the loop it replaces on standalone.

I still think the cap is right — it converts an unbounded memory and queue-occupancy risk into graceful degradation, and realistic batch sizes are tens to low hundreds where chunking never triggers. But it does mean the crossover where standalone batching wins is narrow to nonexistent, which is the same conclusion the benchmark reaches independently.

Test quality

The new coverage is the right kind. The 1,001-command boundary cases use gates plus vi.waitFor to prove sequencing — the second chunk is not dispatched until the first resolves — rather than just asserting call counts. The Cluster test proves owner concurrency and per-owner sequencing simultaneously, and stop-on-chunk-failure is pinned.

I do not see a remaining hole: redirection recovery operates on a chunk of at most 1,000 by construction, so it is inherently a single scalar window.

Verdict

Mergeable as-is; nothing outstanding is a correctness issue.

The two things I would still want are a GLIDE leg in the benchmark and a decision on whether the standalone pipeline path stays, with the README adjusted either way. Both are about not carrying a claim the measurements do not support.

[lan17]

Addressed the re-review in c8869e4.

1. GLIDE is now bounded too. Native Batch / ClusterBatch work runs in sequential chunks of at most 1,000 commands, and scalar-only wrappers run sequential windows of at most 1,000 concurrent invocations. A failed chunk stops later chunks; every launched scalar call in the current window settles. One outer lifecycle scope covers the complete loop, so dispose() cannot release script handles between chunks.
2. Cold-script recovery is per chunk. Warm native commands use EVALSHA; only the affected chunk retries with idempotent EVAL after a confirmed script miss. Reply count and reply values are validated before the next chunk starts.
3. The standalone node-redis pipeline stays, with a narrower claim. Warm concurrent scalar calls are already auto-pipelined, so README no longer implies a general steady-state throughput or round-trip win. The explicit pipeline remains for bounded dispatch, reply-count validation, and cold-cache behavior: one source-bearing EVAL per pipeline instead of independent full-source recovery for every scalar call after SCRIPT FLUSH.
4. The benchmark now covers both bundled adapters. It retains fresh targets, alternating order, five-sample medians, semantic watermark assertions, and no timing threshold. Both clients receive 10,000-command/request benchmark headroom so the 1,000-way scalar leg is admissible.

Live standalone Redis 7 medians, shown as scalar / batch and scalar-to-batch ratio:

adapter	targets	scalar	batch	ratio
node-redis	10	0.75 ms	0.60 ms	1.25x
node-redis	100	1.85 ms	1.74 ms	1.06x
node-redis	1,000	6.27 ms	6.26 ms	1.00x
Valkey GLIDE	10	0.51 ms	0.50 ms	1.02x
Valkey GLIDE	100	1.52 ms	1.54 ms	0.99x
Valkey GLIDE	1,000	9.42 ms	8.77 ms	1.07x

The numbers support the revised wording: warm standalone behavior is near parity and adapter-dependent; the feature does not claim a universal throughput improvement.

Additional cleanup corrected the PR/issue wording that had accidentally described core's custom-adapter scalar fallback as bounded. Core keeps its existing all-settled concurrent fallback; the private 1,000 ceiling belongs to the bundled node-redis and GLIDE adapters.

Validation on Node.js 22.22.0:

• corepack pnpm check: 449 unit tests, 97.17% statement coverage, typecheck, builds/declarations, and packed consumers passed
• corepack pnpm test:integration: all 99 Redis 6.2, Valkey 8, and three-primary Redis 7 Cluster tests passed
• two-adapter benchmark: every watermark assertion passed
• git diff --check passed
• two fresh read-only reviews found no actionable implementation or benchmark/documentation issue