Skip to content

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

Draft
os-warren wants to merge 1 commit into
mainfrom
claude/issue-15983-redis-kv-contract-gaps
Draft

test(service-cluster-redis): pin the WATCH/MULTI abort-retry loop and the versioned-delete MULTI/DEL branch#16054
os-warren wants to merge 1 commit into
mainfrom
claude/issue-15983-redis-kv-contract-gaps

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes #15983

Pins the two RedisKV limbs the shipped contract suite drives to no assertion: the WATCH/MULTI abort-retry loop, and the versioned-delete MULTI/DEL branch.

All readings below are from head c08761472.

The gap, reproduced before it was closed

Instrument 1 — recording Proxy census. An otherwise byte-identical copy of redis.contract.test.ts with makeClient() wrapped in a recording Proxy, the suite run unmodified (28/28 green under instrumentation), dumping every method invoked on the injected client plus every multi().exec() return value. The copy was a separate scratch file, so the shipped test was never edited for the measurement and needed no restore.

Reading:

observation count
multi().del calls 0
multi().exec() calls 10
... of which returned null 2
commands ever queued on a multi() chain set only
  • Gap 1 confirmed. multi().del is called zero times. Its 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.
  • Gap 2 corrected. The card reasoned that the result === null retry limb runs zero times. It runs twice — in set then get round-trips and increments version and in cas succeeds on match, fails on mismatch. It was unpinned, not unreached. That half of the card's premise was an inference from the code rather than a reading, and the measurement contradicts it.

Instrument 2 — proxy-free replay. To rule the Proxy itself out as the cause, the client-level sequence RedisKV.set issues (WATCH, GET, MULTI/SET, EXEC) was replayed directly against a bare ioredis-mock, no Proxy anywhere. It reproduces both nulls, so the behaviour is the double's.

Can the pin fail? Yes — measured, with a control

The card warns that a pin for the abort path is worthless if the double cannot produce exec() === null. It can:

scenario exec()
competing write from another connection between WATCH and EXEC null
competing write from the same connection null
rival rewrites byte-identical content null
CONTROL — fresh key, fresh connection, no competing writer array, not null

The control fires, so the null readings are a discrimination and not a constant. watch() is not a no-op on this double.

The divergence those two incidental nulls come from

ioredis-mock@8.13.1 leaves a per-connection stale watch flag: a connection that has itself EXECed a write to a key aborts its next WATCH+EXEC on that key with no competing writer present. Measured: second attempt null, third array (the abort clears it), an explicit UNWATCH clears it, and another connection is unaffected. A real server would commit there.

So every pin here runs the driver's client on a key that client has never EXECed on, seeding and competing from separate connections over the mock's shared store — which makes a competing write the only available cause of an abort. Filed separately as #16053 (a fidelity finding about the double, not about our code); that card is a recording and stays open.

The pins and what each one's population is

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

  1. set() abort-retry. Covers: the result === null limb converging, and re-reading rather than merely re-EXECing — asserted via the returned version, which can only be 2n if the retry went back through WATCH+GET and saw the rival's 1n. Does not cover: any real-server interleaving.
  2. delete(key, {ifVersion}) abort-retry into MULTI/DEL. Covers: the same limb in the versioned delete, retrying into a successful multi().del. The rival rewrites the row's current bytes verbatim so the version is unchanged and the retry's ifVersion still matches. The physical key is read back from what the driver itself WATCHed, so the test does not encode RedisKV's private key layout.
  3. delete(key, {ifVersion}) clean path. Covers: the versioned branch reaching multi().del at all, and reading the removal out of the EXEC reply. Asserts the queued command list is exactly ['del'], which is what distinguishes it from the unversioned client.del fast path.
  4. delete(key, {ifVersion}) on an absent key. Covers: the early return, and that no MULTI is opened.
  5. delete(key, {ifVersion}) on a stale version. Covers: the rejection, asserted on the error's structured fields (key, expected, actual) rather than a bare toThrow(), and that no MULTI is opened.

Across all five: they establish that the driver's retry limb converges when its client reports an abort, and that this double implements WATCH abort on a genuine competing write. They do not establish that a real Redis server aborts under the same interleaving, nor that the double's WATCH fidelity matches a real server in general — the divergence above proves it does not. Closing that needs a live-Redis path, which this package has none of.

Mutation proof — every pin was driven RED

Each production branch was broken in turn, the pin observed RED, and src/kv.ts restored under trap ... EXIT INT TERM via git checkout HEAD -- ABSOLUTE_PATH. Every anchor was asserted unique in the form written before mutating (a deliberately ambiguous control anchor, the bare if (result === null) continue;, counts 2 and would have been refused). Each mutation was proved to have landed on disk by counting the injected marker and the removed original — never by the editor's exit code. Restore proved by blob equality and an empty git diff HEAD.

# branch broken pin RED assertion text
1 set()'s if (result === null) continue pin 1 expected [ 'NULL' ] to deeply equal [ 'NULL', 'ARRAY' ]
2 delete()'s if (result === null) continue pin 2 expected false to be true
3 multi.del(physical) removed pins 2, 3 expected false to be true
4 absent-key leg returns true pin 4 expected true to be false
5 throws a plain Error instead of VersionMismatchError pin 5 expected Error: stale to be an instance of VersionMismatchError

Mutation 1's text is worth reading twice: [ 'NULL' ] shows the abort still happened, so the pin's competing writer really does trip the double's WATCH — the retry is the only thing the mutation removed. Mutation 5 is why the rejection asserts the envelope: a bare toThrow() would have stayed green.

The implementation was committed before any mutation ran, so the restore leg had a real reference to restore to. Final git status --porcelain after all five: empty.

Verification

command exit verdict line
pnpm --filter @objectstack/service-cluster-redis test 0 Test Files 2 passed (2) / Tests 33 passed (33) (28 shipped + 5 new)
pnpm --filter @objectstack/service-cluster-redis typecheck 0 echoed tsc --noEmit
gate union, 45 families 0 45/45

typecheck is a real reading, not a vacuous one: tsc --listFiles puts both edited files in the program (1 occurrence each), against a control spelling that returns 0. The package's tsconfig.json is include: ["src"], so it does not exclude *.test.ts.

The gate family was derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack — 45 families per its Reconciliation line, and --commands printed exactly 45. Two initially returned exit 3, PREREQUISITE NOT MET (check:dual-build-cjs-loads, check:type-check-debt) — not a pass and not a failure, nothing measured. The workspace closure was built (turbo run build, 71/71 successful) and both re-run to real verdicts:

  • check:dual-build-cjs-loads exit 0 — provenance — entries/packages/cjsFiles/probes: this run 103/66/619/1 · floors 90/58/520/1
  • check: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.

Every gate exit code was captured before any pipe (cmd > log 2>&1; code=$?), never read through | head.

NOT MEASURED

  • Real Redis. No live-Redis path exists in this package; nothing here is evidence about a real server's WATCH/EXEC behaviour.
  • createRedisClient(). Still called by no test in this package. Unchanged by this PR.
  • 6 CI families whose argv takes a value from the workflow ($RUNNER_TEMP, ${{ matrix.shard }}) have no local invocation; dispatch-gates places them outside the 45 by design.
  • 37 artifact-roster families score silent for every card in the tree; their silence 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 was not run; it is CI's. The gate union above is what this card owes locally.

Not in scope

#15986 carries the ioredis 5-vs-6 version-pin question and is untouched here; it stays open. This PR does not make that pin easier or harder — it adds no dependency on either version's behaviour, and its own version-sensitive claim (the ioredis-mock WATCH readings) is dated and scoped to the resolved 8.13.1 in the file header, so it expires the same way the existing declaration does.

Changeset

None. Tests-only: one new test file plus a comment block in an existing test file. package.json publishes files: ["dist", ...] and tsup bundles from src/index.ts, so no test file reaches the tarball and no published surface or behaviour moves. skip-changeset applies and is applied.


Generated by Claude Code

… the versioned-delete MULTI/DEL branch

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.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions github-actions Bot added the size/m label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 53cbad9f75572600ef43bb2a18071633fd6c0f68packageMentionDocs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m skip-changeset PR has no user-facing published change; bypasses the changeset gate tests

Projects

None yet

2 participants