diff --git a/packages/services/service-cluster-redis/src/kv.transaction.test.ts b/packages/services/service-cluster-redis/src/kv.transaction.test.ts new file mode 100644 index 0000000000..b88dc76e0b --- /dev/null +++ b/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, +): { 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('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('k'))?.value).toBe('v0'); + + await kv.close(); + }); +}); diff --git a/packages/services/service-cluster-redis/src/redis.contract.test.ts b/packages/services/service-cluster-redis/src/redis.contract.test.ts index 481614adc8..8d805a85e1 100644 --- a/packages/services/service-cluster-redis/src/redis.contract.test.ts +++ b/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`