test(service-cluster-redis): pin the WATCH/MULTI abort-retry loop and the versioned-delete MULTI/DEL branch (#16054)

The shipped contract suite drives neither limb to an assertion. Measured
with a recording Proxy over an otherwise byte-identical copy of the suite
(28/28 green under instrumentation), then re-measured proxy-free by
replaying the client-level sequence against a bare ioredis-mock:

  - `multi().del` is called zero times — `runKVContract` only ever calls
    `kv.delete('k')` with no options, so the versioned branch of
    `RedisKV.delete` is entirely unexecuted. The card's reading, confirmed.
  - `multi().exec()` returns null 2 times out of 10 — so the retry limb is
    NOT unexecuted, as the card inferred. It runs, unasserted, and it runs
    for the wrong reason: ioredis-mock@8.13.1 leaves a per-connection stale
    watch flag after a connection EXECs a write, so that connection's next
    WATCH+EXEC on the same key aborts with no competing writer present.

The new pins therefore run the driver's client on keys it has never EXECed
on, seeding and competing from separate connections, so a competing write
is the only available cause of an abort. Controls measured and recorded in
the file header, including the one that must NOT abort.


Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-warren

packages/services/service-cluster-redis/src/kv.transaction.test.ts

@@ -0,0 +1,268 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * Pins for the two `RedisKV` limbs the shipped contract suite in
+ * `redis.contract.test.ts` does not assert: the WATCH/MULTI abort-retry
+ * loop, and the versioned-delete MULTI/DEL branch.
+ *
+ * ## What was measured before writing these, and how
+ *
+ * Instrument 1 — a recording `Proxy` around the `makeClient()` of an
+ * otherwise byte-identical copy of the shipped suite, run unmodified
+ * (28/28 green under instrumentation), dumping every method the driver
+ * invoked on the injected client plus every `multi().exec()` return
+ * value. Its reading:
+ *
+ *   - `multi().del` is called ZERO times. The only call site is the
+ *     `opts.ifVersion !== undefined` branch of `RedisKV.delete`, and
+ *     `runKVContract` only ever calls `kv.delete('k')` with no options,
+ *     so the versioned branch is entirely unexecuted. Confirmed.
+ *   - `multi().exec()` is called 10 times and returns `null` for 2 of
+ *     them. So the `result === null` retry limb is NOT unexecuted — it
+ *     runs twice, in `set then get round-trips` and in `cas succeeds on
+ *     match, fails on mismatch`. Nothing asserts that it ran, and both
+ *     tests pass either way, so it was unpinned rather than unreached.
+ *
+ * Instrument 2 — a proxy-free replay of the exact client-level sequence
+ * `RedisKV.set` issues (WATCH, GET, MULTI/SET, EXEC) against a bare
+ * `ioredis-mock`, to rule the Proxy itself out as the cause. It
+ * reproduces the two nulls, so the behaviour is the double's.
+ *
+ * ## The divergence those two nulls come from
+ *
+ * On `ioredis-mock@8.13.1` a connection that has itself EXECed a write to
+ * a key carries a stale dirty flag for it: the NEXT WATCH+EXEC on that
+ * same key from that same connection aborts once even though no competing
+ * writer exists. Measured, with controls that fire:
+ *
+ *   - fresh key on a fresh connection, no competing writer: EXEC returns
+ *     an array, NOT null (so the probe distinguishes the two outcomes)
+ *   - second WATCH+EXEC on the same key from the same connection: null
+ *   - third: array again (the abort clears the flag)
+ *   - the flag is per-connection: another connection WATCHing a key this
+ *     one EXECed on is unaffected
+ *   - an explicit UNWATCH between the two clears it
+ *
+ * A real server would not abort there. That is why every pin below runs
+ * the driver's client on a key that client has never EXECed on, seeding
+ * and competing from SEPARATE connections over ioredis-mock's shared
+ * store: it makes a competing write the only possible cause of an abort.
+ *
+ * ## What these pins do and do not establish
+ *
+ * They establish that the driver's retry limb converges, and re-reads
+ * rather than merely re-EXECing, when its client reports an aborted
+ * transaction; and that the versioned-delete branch reaches MULTI/DEL and
+ * reports removal from the reply. They also establish that this double
+ * does implement WATCH abort on a genuine competing write, rather than
+ * treating `watch()` as a no-op.
+ *
+ * They do NOT establish that a real Redis server aborts under the same
+ * interleaving, nor that this double's WATCH fidelity matches a real
+ * server in general — the divergence above is proof that it does not.
+ * That remains the open question the card records, and closing it needs a
+ * live-Redis path, which this package has none of.
+ */
+
+// `ioredis-mock` publishes no type declarations; the shipped suite's
+// import carries the full note on what that costs.
+// @ts-expect-error — ioredis-mock has no published types
+import RedisMock from 'ioredis-mock';
+import { describe, expect, it } from 'vitest';
+
+import { RedisKV, VersionMismatchError } from './kv.js';
+
+// ioredis-mock shares state across instances, so a per-test key prefix is
+// what isolates the tests — same device the shipped suite uses.
+let suffix = 0;
+const uniquePrefix = () => `tx${++suffix}:`;
+
+/** The mock is untyped (see the import note), so its instances are `any`. */
+type MockClient = any;
+
+interface Recorder {
+    /** `'NULL'` or `'ARRAY'` per `multi().exec()`, in call order. */
+    execOutcomes: string[];
+    /** Commands queued onto a `multi()` chain, e.g. `'set'`, `'del'`. */
+    multiCommands: string[];
+    /** Keys passed to `watch()`, in call order. */
+    watchedKeys: string[];
+}
+
+/**
+ * Wraps an ioredis-mock connection so a test can observe what the driver
+ * did inside its WATCH/MULTI loop, and can run a competing writer at the
+ * one instant that matters: after the driver's GET has resolved and
+ * before it calls EXEC.
+ *
+ * The wrapper adds no Redis semantics of its own — every reply, and the
+ * WATCH bookkeeping that decides whether EXEC aborts, still comes from
+ * ioredis-mock.
+ */
+function instrument(
+    raw: MockClient,
+    onFirstGet?: (rec: Recorder) => Promise<void>,
+): { client: MockClient; rec: Recorder } {
+    const rec: Recorder = { execOutcomes: [], multiCommands: [], watchedKeys: [] };
+    let hookFired = false;
+
+    const wrapMulti = (chain: MockClient): MockClient =>
+        new Proxy(chain, {
+            get(target, prop, receiver) {
+                const value = Reflect.get(target, prop, receiver);
+                if (typeof prop !== 'string' || typeof value !== 'function') return value;
+                return (...args: unknown[]) => {
+                    if (prop !== 'exec') rec.multiCommands.push(prop);
+                    const out = value.apply(target, args);
+                    if (prop === 'exec') {
+                        return Promise.resolve(out).then((reply: unknown) => {
+                            rec.execOutcomes.push(reply === null ? 'NULL' : 'ARRAY');
+                            return reply;
+                        });
+                    }
+                    // Keep the chain observable when a command returns `this`.
+                    return out === target ? receiver : out;
+                };
+            },
+        });
+
+    const client: MockClient = new Proxy(raw, {
+        get(target, prop, receiver) {
+            const value = Reflect.get(target, prop, receiver);
+            if (typeof prop !== 'string' || typeof value !== 'function') return value;
+            return (...args: unknown[]) => {
+                if (prop === 'watch') rec.watchedKeys.push(String(args[0]));
+                if (prop === 'multi') return wrapMulti(value.apply(target, args));
+                if (prop === 'get' && onFirstGet) {
+                    return (async () => {
+                        const reply = await value.apply(target, args);
+                        if (!hookFired) {
+                            hookFired = true;
+                            await onFirstGet(rec);
+                        }
+                        return reply;
+                    })();
+                }
+                return value.apply(target, args);
+            };
+        },
+    });
+
+    return { client, rec };
+}
+
+describe('RedisKV — WATCH/MULTI transaction limbs — redis(mock)', () => {
+    it('set(): a competing writer between WATCH and EXEC drives the abort-retry limb', async () => {
+        const keyPrefix = uniquePrefix();
+        // The rival is a separate connection driving the same production
+        // class, so the competing write is a real KV write rather than a
+        // hand-rolled storage envelope.
+        const rival = new RedisKV({ client: new RedisMock(), keyPrefix });
+        const { client, rec } = instrument(new RedisMock(), async () => {
+            await rival.set('k', 'intruder');
+        });
+        const kv = new RedisKV({ client, keyPrefix });
+
+        const entry = await kv.set('k', 'mine');
+
+        // Exactly one abort then one commit: the `result === null` limb ran
+        // once. Without the competing writer this key is fresh on this
+        // connection, so a null here has no other available cause.
+        expect(rec.execOutcomes).toEqual(['NULL', 'ARRAY']);
+        // v2, not v1. Only a retry that went back through WATCH and GET can
+        // have seen the rival's v1 and bumped past it; a retry that merely
+        // re-issued EXEC would still be writing v1.
+        expect(entry.version).toBe(2n);
+        expect(entry.value).toBe('mine');
+
+        const got = await kv.get<string>('k');
+        expect(got?.value).toBe('mine');
+        expect(got?.version).toBe(2n);
+
+        await kv.close();
+    });
+
+    it('delete(key, {ifVersion}): a competing writer drives the abort-retry limb into MULTI/DEL', async () => {
+        const keyPrefix = uniquePrefix();
+        // Seed from a separate connection so the driver's client has never
+        // EXECed on this key (see the header note on the per-connection
+        // stale flag) — the abort below can then only be the rival's doing.
+        const seeder = new RedisKV({ client: new RedisMock(), keyPrefix });
+        expect((await seeder.set('k', 'v0')).version).toBe(1n);
+
+        // The rival rewrites the row's current bytes verbatim. WATCH tracks
+        // writes, not value changes, so this aborts the driver's transaction
+        // while leaving the version at 1 — which is what lets the retry find
+        // a still-matching `ifVersion` and proceed into MULTI/DEL. The
+        // physical key is taken from what the driver itself WATCHed, so this
+        // test does not encode the key-layout private to RedisKV.
+        const rivalClient = new RedisMock();
+        const { client, rec } = instrument(new RedisMock(), async (r) => {
+            const physical = r.watchedKeys[r.watchedKeys.length - 1];
+            await rivalClient.set(physical, await rivalClient.get(physical));
+        });
+        const kv = new RedisKV({ client, keyPrefix });
+
+        expect(await kv.delete('k', { ifVersion: 1n })).toBe(true);
+        expect(rec.execOutcomes).toEqual(['NULL', 'ARRAY']);
+        expect(rec.multiCommands).toEqual(['del', 'del']);
+        expect(await kv.get('k')).toBeUndefined();
+
+        await kv.close();
+    });
+
+    it('delete(key, {ifVersion}) removes through the MULTI/DEL branch and reports it from the reply', async () => {
+        const keyPrefix = uniquePrefix();
+        const seeder = new RedisKV({ client: new RedisMock(), keyPrefix });
+        expect((await seeder.set('k', 'v0')).version).toBe(1n);
+
+        const { client, rec } = instrument(new RedisMock());
+        const kv = new RedisKV({ client, keyPrefix });
+
+        expect(await kv.delete('k', { ifVersion: 1n })).toBe(true);
+        // The unversioned fast path calls `client.del` and opens no
+        // transaction; this asserts the versioned branch was the one taken.
+        expect(rec.multiCommands).toEqual(['del']);
+        expect(rec.execOutcomes).toEqual(['ARRAY']);
+        expect(await kv.get('k')).toBeUndefined();
+
+        await kv.close();
+    });
+
+    it('delete(key, {ifVersion}) on an absent key returns false without opening a MULTI', async () => {
+        const keyPrefix = uniquePrefix();
+        const { client, rec } = instrument(new RedisMock());
+        const kv = new RedisKV({ client, keyPrefix });
+
+        expect(await kv.delete('absent', { ifVersion: 1n })).toBe(false);
+        expect(rec.multiCommands).toEqual([]);
+        expect(rec.execOutcomes).toEqual([]);
+
+        await kv.close();
+    });
+
+    it('delete(key, {ifVersion}) rejects a stale version, opens no MULTI, and leaves the row', async () => {
+        const keyPrefix = uniquePrefix();
+        const seeder = new RedisKV({ client: new RedisMock(), keyPrefix });
+        await seeder.set('k', 'v0');
+
+        const { client, rec } = instrument(new RedisMock());
+        const kv = new RedisKV({ client, keyPrefix });
+
+        // Assert the error's structured fields, not just that something was
+        // thrown: a bare `toThrow()` stays green when the driver throws a
+        // plain Error for an unrelated reason.
+        const err: unknown = await kv
+            .delete('k', { ifVersion: 99n })
+            .then(() => null, (e: unknown) => e);
+        expect(err).toBeInstanceOf(VersionMismatchError);
+        expect((err as VersionMismatchError).key).toBe('k');
+        expect((err as VersionMismatchError).expected).toBe(99n);
+        expect((err as VersionMismatchError).actual).toBe(1n);
+
+        expect(rec.multiCommands).toEqual([]);
+        expect((await kv.get<string>('k'))?.value).toBe('v0');
+
+        await kv.close();
+    });
+});

packages/services/service-cluster-redis/src/redis.contract.test.ts

@@ -70,6 +70,15 @@
  * why the version gap is inert here, and it is not a reason to trust the
  * double: the mock is simply never asked to stand in for the surface on
  * which the two majors differ.
+ *
+ * Two `RedisKV` limbs this file drives but does not assert are pinned in
+ * the sibling `kv.transaction.test.ts`: the WATCH/MULTI abort-retry loop,
+ * and the versioned-delete MULTI/DEL branch. Measured here rather than
+ * assumed: `multi().del` is called zero times by these suites, while
+ * `multi().exec()` DOES return null twice — not because a competing
+ * writer exists, but because of a per-connection stale-watch divergence
+ * in the double. The sibling file carries that measurement and its
+ * controls.
  */
 
 // `ioredis-mock` publishes no type declarations of its own — no `types`