Repository navigation
[finding] service-cluster-redis's KV contract suite never exercises the WATCH/MULTI abort-retry path nor the versioned-delete MULTI branch — measured by instrumenting the shipped test #15983
Description
Recording + measurement only — no severity asserted, no fix chosen. Found while measuring the ioredis-mock major-version gap in #15467; it is a different defect class (coverage of a code path, not the version of the double), so it is filed rather than folded into that PR.
Method
The double handed to the driver in packages/services/service-cluster-redis/src/redis.contract.test.ts was temporarily wrapped in a recording Proxy (the client, and separately the client.duplicate() the pub/sub adapter makes), the shipped suite was run unmodified otherwise, and the set of methods actually invoked was dumped. All 28 tests passed under instrumentation, and the file was restored to a state proved byte-identical to HEAD (git diff HEAD empty, blob hash back at HEAD's).
Population of this census: exactly the methods the shipped contract test causes to be called on the injected client and its pub/sub duplicate. It says nothing about any other suite.
The reading
Exercised on the injected client:
del, duplicate, eval, get, incr, incrby, multi, multi().exec, multi().set, pttl, publish, set, unwatch, watch
Exercised on the pub/sub duplicate:
on, quit, subscribe, unsubscribe
multi().del never appears. That is the only call site in RedisKV.delete(key, opts)'s versioned branch — the opts.ifVersion !== undefined path at kv.ts. Every delete the contract suite performs goes through the unversioned fast path (client.del), because runKVContract only ever calls kv.delete('k') with no options.
The second gap, in the same code
RedisKV.set and the versioned delete both loop:
const result = await multi.exec();
// exec() returns null when WATCH detected a concurrent change.
if (result === null) continue;
Nothing in the suite ever produces result === null — that requires a competing writer to touch the key between WATCH and EXEC, and no contract test creates concurrent writers. So the retry limb, and with it the whole optimistic-concurrency claim the class header makes, is executed zero times.
The ifVersion mismatch cases that DO pass (ifVersion=0n requires absent key, cas succeeds on match, fails on mismatch) return before MULTI — they compare a version in JavaScript after a plain GET. They establish the driver's own comparison logic, not Redis's transactional abort semantics.
Why it is worth writing down
WATCH/MULTI/EXEC abort behaviour is the single surface on which an in-memory double is most likely to diverge from a real server, and it is exactly the surface with no assertion behind it. A green suite here is consistent with a double that implements watch() as a no-op.
Scope of the claim
- ⛔ No claim that
RedisKVis wrong. The unexercised limbs read correctly; they are simply unexecuted. - ⛔ No claim about how to close it. A concurrency test against a double whose WATCH fidelity is itself unmeasured may not be worth much — establishing what
ioredis-mockdoes with WATCH is arguably the prior step, and that may point at a real-Redis integration path, which is a cost decision rather than a lane call. - ⛔ Nothing about pub/sub, lock, or counter coverage; only these two KV limbs.
Dedup
MCP search_issues, one targeted query for this shape ⇒ 0 results. A control query in the same session (the tsconfig test-exclusion genre) returned 36, so the channel answers and the zero is a reading rather than a silent empty. A first attempt at the targeted query returned HTTP 422 on punctuation in the query string; that was a channel error, not a zero, and it was re-run.
Activity
os-warren commentedon Sep 5, 2026
Claiming this card.
claude/issue-15983-redis-kv-contract-gaps(pushed empty as the write-route probe before any edit)origin/main@2e357650306335f7fdb8939d16edce4c96b48831Read the card and its full timeline before claiming (12 timeline events, zero
IssueCommentnodes — no competing claim). Assignee is the PM's field and I am not touching it.Plan: reproduce the two coverage gaps with a named instrument first, then establish what
ioredis-mockactually does withWATCH/EXECbefore asserting any pin covers the abort-retry path — a pin whose condition can never occur is not a pin.Generated by Claude Code
Generated by Claude Code
os-warren commentedon Sep 5, 2026
os-dev-report
{ "issue": 15983, "status": "done", "branch": "claude/issue-15983-redis-kv-contract-gaps", "pr": "https://github.com/objectstack-ai/objectstack/pull/16054", "premise_still_valid": true, "summary": "Both limbs are now pinned in packages/services/service-cluster-redis/src/kv.transaction.test.ts (5 pins), with a pointer added to the shipped suite's header. ONE HALF OF THE CARD'S PREMISE IS FALSIFIED and the PR says so: gap 1 (multi().del called zero times) is confirmed by measurement, but gap 2 is not what the card said. The card reasoned the `result === null` retry limb runs zero times; instrumenting the shipped suite shows multi().exec() is called 10 times and returns null for 2 of them, so the limb was UNPINNED, not unreached. The card's exec-null claim was an inference from the code, not a reading. Cause: ioredis-mock@8.13.1 carries a per-connection stale watch flag, so a connection aborts its next WATCH+EXEC on a key it has itself EXECed on, with no competing writer present. Filed as #16053. Every pin therefore runs the driver's client on a key it has never EXECed on, seeding and competing from separate connections, so a competing write is the only available cause of an abort. The mock CAN produce exec() === null (measured three ways, with a control that fires), so the pins can fail — proved: all five were driven RED.", "tests": "Union of 45 gate families run at head c08761472 (git rev-parse --short HEAD), 45/45 exit 0. Family derived by `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack`; its Reconciliation line says 45 and `--commands` printed exactly 45. Two first returned exit 3 PREREQUISITE NOT MET (check:dual-build-cjs-loads, check:type-check-debt) — NOT MEASURED, not green; workspace closure built (turbo run build, 'Tasks: 71 successful, 71 total') and both re-run to real verdicts: dual-build-cjs-loads exit 0 'provenance — entries/packages/cjsFiles/probes: this run 103/66/619/1 · floors 90/58/520/1'; type-check-debt exit 0 'check-type-check-coverage --re-measure: OK — 12 ledger entr(ies) re-measured in 119.6s, 140 raw tsc error(s) total, none above its recorded number.'. Package tests: `pnpm --filter @objectstack/service-cluster-redis test` exit 0, 'Test Files 2 passed (2)' / 'Tests 33 passed (33)' (28 shipped + 5 new). Typecheck: `pnpm --filter @objectstack/service-cluster-redis typecheck` exit 0, echoed 'tsc --noEmit'; proved non-vacuous by `tsc --listFiles` — both edited files appear once each, control spelling returns 0; tsconfig is include:['src'] so tests are not excluded. All exit codes captured before any pipe. REPRODUCTION INSTRUMENTS: (1) recording Proxy over makeClient in a separate byte-identical scratch COPY of the shipped suite (shipped file never edited, so no restore was owed), 28/28 green under instrumentation, census: client methods 12, multi chain {set, exec} only, multi().del = 0, exec 10 of which 2 NULL, attributed by test name; (2) proxy-free replay of the WATCH/GET/MULTI-SET/EXEC sequence against a bare ioredis-mock, reproducing both nulls, ruling the Proxy out as the cause. MUTATION PROOF, five mutations, each anchor asserted unique IN THE FORM WRITTEN (control: the ambiguous bare anchor 'if (result === null) continue;' counts 2 and would have been refused), each mutation proved on disk by counting the injected marker AND the removed original (never the editor's exit code), fix COMMITTED BEFORE mutating, restore under `trap ... EXIT INT TERM` via `git checkout HEAD -- ABSOLUTE_PATH` (absolute, seeded from git rev-parse --show-toplevel), each restore proved by blob equality (d1139935a549dc9ffb914eaffd8ebaee0adad713 both sides, non-empty) AND an empty `git diff HEAD`: (1) set()'s retry limb removed -> pin 1 RED 'expected [ NULL ] to deeply equal [ NULL, ARRAY ]'; (2) delete()'s retry limb removed -> pin 2 RED 'expected false to be true'; (3) multi.del(physical) removed -> pins 2+3 RED 'expected false to be true'; (4) absent-key leg returns true -> pin 4 RED 'expected true to be false'; (5) plain Error instead of VersionMismatchError -> pin 5 RED 'expected Error: stale to be an instance of VersionMismatchError'. Final git status --porcelain after all five: empty. No build was needed for these mutations: the subject is reached by a relative source import (./kv.js resolves to src/kv.ts under vitest), not through package exports/dist — and a RED result is itself the proof the mutation reached the running code. NOT MEASURED: real Redis (no live path in this package); createRedisClient() (still driven by no test here); 6 CI families whose argv takes a workflow value ($RUNNER_TEMP, matrix.shard) and which dispatch-gates places outside the 45; 37 artifact-roster families whose 'silent' is a fact about a list, not a clearance; ioredis-mock versions other than the resolved 8.13.1 and commands other than WATCH/MULTI/EXEC/SET/DEL; full-repo `pnpm lint` (CI's, not run).", "mcp_calls": "6 — claim comment, one targeted dedup search_issues, issue_write create (#16053), create_pull_request (#16054), issue_write label union, this report comment. REST was 403 for this seat (session gate, gh absent), so all card and PR reads went through the zero-quota payload channel.", "open_questions": [], "out_of_scope_findings": [ "filed as #16053: ioredis-mock@8.13.1 aborts a WATCHed transaction with no competing writer — a per-connection stale watch flag. Measured proxy-free with a control that fires (fresh key on a fresh connection commits); the flag is per-connection, an explicit UNWATCH clears it, and the abort itself clears it. It already produces the 2 incidental exec() nulls in the shipped suite, and it is what corrected this card's premise. Labelled finding + domain:services, no assignee." ] }Generated by Claude Code
Generated by Claude Code
os-warren commentedon Sep 5, 2026
PM — PR #16054, and half this card's premise is measured FALSE
⭐ The better half of the outcome is the correction, not the pins.
Gap 1 confirmed:
multi().delis called zero times — measured, not inferred.Gap 2 is not what the card said. The card reasoned that the
result === nullretry limb runs zero times. Instrumenting the shipped suite showsmulti().exec()is called 10 times and returnsnullfor 2 of them. ⇒ The limb was UNPINNED, not unreached. ⛔ The card's exec-null claim was an inference from the code, not a reading — and this seat has spent the whole session on exactly that distinction.⭐ And the cause is a real find, not a detail
ioredis-mock@8.13.1carries a per-connection stale watch flag: a connection aborts its nextWATCH+EXECon a key it has itselfEXECed on, with no competing writer present. Measured proxy-free with a control that fires (a fresh key on a fresh connection commits), the flag is per-connection, an explicitUNWATCHclears it, and the abort itself clears it. Filed as #16053.⇒ So those 2 incidental nulls in the shipped suite were artifacts of a mock defect, not evidence the abort-retry path was exercised. A pin written naively against that behaviour would have been green for the wrong reason — the phantom class, arriving from a direction nobody was watching.
The question I put in the brief, answered
I required proof that the mock can produce
exec() === nullbefore any pin claimed to cover the abort-retry path — a pin that passes because its condition never occurs is worse than none. Measured three ways with a firing control, and then proved the harder way: all five pins were driven RED.⭐ One control worth keeping: the anchor
if (result === null) continue;occurs twice, so the mutation harness refused it as ambiguous rather than mutating the wrong site. Anchors asserted unique in the form written is a rule that only earns its keep when it actually refuses something — here it did.Five mutations, each proved on disk by counting both the injected marker and the removed original (⛔ never the editor's exit code), each restore proved by blob equality and an empty
git diff HEAD. ⭐ And the fix was committed before mutating — the rule a sibling round on this seat had to learn the hard way, when its restore leg silently deleted an uncommitted fix at exit 0.Gates
45 of 45, with the Reconciliation line read as the authority (45) rather than a harvest that stops at the local block. Two answered exit 3 PREREQUISITE NOT MET first — ⛔ recorded as NOT MEASURED, not green — then re-run to real verdicts after a full workspace build.
createRedisClient(), still driven by no test here — ⭐ which is the same hole card #15467's landing comment recorded, so it survives this PR · ioredis-mock versions other than the resolved 8.13.1, and commands beyondWATCH/MULTI/EXEC/SET/DEL· full-repopnpm lint.⛔ Contract review queued, not run — blocked by a rate limit at the declared tier (probed five times, all 429 at launch). Nothing lands until it passes.
Generated by Claude Code