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
Draft
Conversation
… 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
Contributor
📓 Docs Drift CheckNothing 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
Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #15983
Pins the two
RedisKVlimbs 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
Proxycensus. An otherwise byte-identical copy ofredis.contract.test.tswithmakeClient()wrapped in a recording Proxy, the suite run unmodified (28/28 green under instrumentation), dumping every method invoked on the injected client plus everymulti().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:
multi().delcallsmulti().exec()callsnullmulti()chainsetonlymulti().delis called zero times. Its only call site is theopts.ifVersion !== undefinedbranch ofRedisKV.delete, andrunKVContractonly ever callskv.delete('k')with no options, so the versioned branch is entirely unexecuted.result === nullretry limb runs zero times. It runs twice — inset then get round-trips and increments versionand incas 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.setissues (WATCH, GET, MULTI/SET, EXEC) was replayed directly against a bareioredis-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:exec()nullnullnullThe control fires, so the
nullreadings 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.1leaves 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 attemptnull, thirdarray(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.set()abort-retry. Covers: theresult === nulllimb converging, and re-reading rather than merely re-EXECing — asserted via the returned version, which can only be2nif the retry went back through WATCH+GET and saw the rival's1n. Does not cover: any real-server interleaving.delete(key, {ifVersion})abort-retry into MULTI/DEL. Covers: the same limb in the versioned delete, retrying into a successfulmulti().del. The rival rewrites the row's current bytes verbatim so the version is unchanged and the retry'sifVersionstill matches. The physical key is read back from what the driver itself WATCHed, so the test does not encodeRedisKV's private key layout.delete(key, {ifVersion})clean path. Covers: the versioned branch reachingmulti().delat 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 unversionedclient.delfast path.delete(key, {ifVersion})on an absent key. Covers: the early return, and that no MULTI is opened.delete(key, {ifVersion})on a stale version. Covers: the rejection, asserted on the error's structured fields (key,expected,actual) rather than a baretoThrow(), 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.tsrestored undertrap ... EXIT INT TERMviagit checkout HEAD -- ABSOLUTE_PATH. Every anchor was asserted unique in the form written before mutating (a deliberately ambiguous control anchor, the bareif (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 emptygit diff HEAD.set()'sif (result === null) continueexpected [ 'NULL' ] to deeply equal [ 'NULL', 'ARRAY' ]delete()'sif (result === null) continueexpected false to be truemulti.del(physical)removedexpected false to be truetrueexpected true to be falseErrorinstead ofVersionMismatchErrorexpected Error: stale to be an instance of VersionMismatchErrorMutation 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 baretoThrow()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 --porcelainafter all five: empty.Verification
pnpm --filter @objectstack/service-cluster-redis testTest Files 2 passed (2)/Tests 33 passed (33)(28 shipped + 5 new)pnpm --filter @objectstack/service-cluster-redis typechecktsc --noEmittypecheckis a real reading, not a vacuous one:tsc --listFilesputs both edited files in the program (1 occurrence each), against a control spelling that returns 0. The package'stsconfig.jsonisinclude: ["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--commandsprinted 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-loadsexit 0 —provenance — entries/packages/cjsFiles/probes: this run 103/66/619/1 · floors 90/58/520/1check:type-check-debtexit 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
createRedisClient(). Still called by no test in this package. Unchanged by this PR.$RUNNER_TEMP,${{ matrix.shard }}) have no local invocation;dispatch-gatesplaces them outside the 45 by design.silentfor every card in the tree; their silence is a fact about a list, not a clearance.ioredis-mockversions other than the resolved 8.13.1, and commands other than WATCH/MULTI/EXEC/SET/DEL.pnpm lintwas 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-mockWATCH 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.jsonpublishesfiles: ["dist", ...]and tsup bundles fromsrc/index.ts, so no test file reaches the tarball and no published surface or behaviour moves.skip-changesetapplies and is applied.Generated by Claude Code