fix(vt): never vouch for a version the producer marked non-unique - #766
fix(vt): never vouch for a version the producer marked non-unique#766kriszyp wants to merge 8 commits into
Conversation
The VerificationTable's premise is that a value's version identifies it, so version equality is evidence that a consumer's cached copy is still current. A producer that stores two distinct values under one version breaks that premise, and the consumer has no way to defend itself: the slot is only half the answer. On a slot miss the read compares the version it just read to the caller's expectedVersion and answers FRESH on equality alone, so the consumer holding the superseded copy is told it is current — precisely the consumer that is wrong. Read one flag from the value header to know when that has happened. VERSION_NOT_UNIQUE_FLAG extends the same header contract extractVersionFromValue already reads: the 8-byte big-endian version at offset 0, followed by a 4-byte big-endian metadata word whose top byte tags it and whose low 24 bits are producer flags. A value carrying the flag is never answered FRESH and never published to a slot, on every read path — sync, async, and both transactional ones — and vtPopulateIfSettled refuses it as well, since a snapshot read can return a value that is unique while the latest committed one is not. Because publication is refused, the pre-read slot fast path needs no change: a non-unique version cannot enter a slot, and the write that made it non-unique cleared whatever was there. A value with no header, or one whose word is not tagged, answers "unique" and keeps its existing behaviour; a false positive costs caching, never correctness. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3tmMwegDueYevPR7UBhkc
A transactional read answers FRESH from the value its snapshot returned, but FRESH speaks about the consumer's cached copy, not about the snapshot. A value that was unique when the snapshot was taken and was given a second value under the same version afterwards would still be confirmed, so a consumer that had cached the newer value from another reader was told its copy was current — the case the flag exists to prevent, reached the other way round. Check the latest value's flag before answering FRESH whenever the read may be behind it. Free when the read is provably the latest (no snapshot, or nothing committed since), and on the branch where it is not free, vtPopulateIfSettled was already paying for the same Get. The async paths carry the database handle for the check as a constructor parameter, so a new async read path cannot forget it. Also from the review: VERSION_NOT_UNIQUE_FLAG becomes constexpr alongside VERSION_HEADER_TAG rather than a #define leaking from a core header; the duplicate Slice constructions are hoisted; the flag's contract records that it is only read for column families that opted into the verification table, and that the explicit populateVersion() call never sees a value and so trusts its caller; and the four near-verbatim restatements of one rule are cut back to the one statement in verification_table.h. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3tmMwegDueYevPR7UBhkc
The previous commit did the check in the async completion, which runs after teardown may have released the database and the column family — a use-after-free on the read path, and on the transactional side it consulted the handle's column family rather than the one the caller's read was routed to. The worker is where both are known alive and the caller's descriptor is still pinned, so the check runs there and the completion reads a plain bool. Two more from the same round: when the guard fires, the async path fell through and published the ambiguous version anyway; and a failed latest Get was treated as evidence that FRESH is safe, when a key that cannot be read is not one whose cached copy should be confirmed — both now take the conservative branch. The check also returns the version it read, so the FRESH branch hands it to vtPopulateIfSettled instead of paying for the same Get twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3tmMwegDueYevPR7UBhkc
There was a problem hiding this comment.
Code Review
This pull request introduces the VERSION_NOT_UNIQUE_FLAG to handle cases where a producer stores multiple distinct values under the same version. It adds verification logic (valueVersionIsNotUnique and vtCheckLatest) to ensure that such non-unique versions are neither marked as FRESH nor published to the verification table slots, maintaining cache correctness even when read snapshots are behind. Unit tests have been added to verify this behavior across sync, async, and transactional paths. There are no review comments, so I have no feedback to provide.
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit e7a840c |
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
|
Reviewed The added Generated by Barber AI |
What
The VerificationTable answers
FRESH_VERSION_FLAGfrom version equality alone. That is only soundwhile a value's version identifies it — and a producer that stores two distinct values under one
version has no way to tell us, so the consumer holding the superseded copy is the one we confirm.
This adds one flag to the value-header contract the VT already reads, and stops answering FRESH or
publishing a slot for a value that carries it.
Why now
Harper hits this. A resequenced (out-of-order) write keeps
Math.max(txnTime, existingVersion), sothe merged record is stored under the version it merged onto: same version, different value. The
worker holding the pre-merge value is told it is current, and an
addTofolding onto that stalevalue drops the increment the merge had already applied — the lost counter increments
ttl-rate-limiter-concurrent.test.ts"(5) multi-worker stress" has been catching in Harper CI.Captured on Harper with a detector on the cache-hit path — same key, same version, two values:
Harper cannot defend itself against this from JS, which is why the fix belongs here. Parking a
sentinel in the slot only suppresses the pre-read fast path; the post-read equality check does not
consult the slot at all. Verified against the pinned build before writing any of this:
The contract
extractVersionFromValue()already documents the header this reads: an 8-byte big-endian version atoffset 0. A producer that has flags to declare writes a 4-byte big-endian metadata word after it —
top byte
VERSION_HEADER_TAG, low 24 bits producer flags.VERSION_NOT_UNIQUE_FLAG(exported asconstants.VERSION_NOT_UNIQUE_FLAG, so producers set the bit this reads) is the one flag thislibrary interprets.
A value carrying it is never answered FRESH and never published to a slot, on every read path —
sync, async, and both transactional ones. Because publication is refused, the pre-read fast path
needs no change of its own: such a version cannot enter a slot, and the write that made it non-unique
already cleared whatever was there.
The flag is only read for a column family that opted into the verification table, i.e. one whose
producer is already writing versions into these bytes. A value with no header, or one whose word is
not tagged, answers "unique" and keeps its existing behaviour — and a misread in the other direction
costs caching, never correctness.
Latest-value check
FRESH speaks about the consumer's cached copy, not about the value in hand, so a read that may be
behind the latest cannot answer it from what it read: the value that made the version ambiguous can
have been committed after the snapshot, and the consumer may have taken its copy from that newer
value. Every FRESH-answering path therefore consults the latest value when its read may be behind it
— free when the read is provably the latest (no snapshot, or nothing committed since), and on the
branch where it is not free,
vtPopulateIfSettledwas already paying for the sameGet, which thecheck now hands its result to instead of repeating.
On the async paths that check runs in the worker, where the database and the caller's column
family are still pinned; the completion only reads a bool. It has to: the completion runs after
teardown may have released both.
Test notes
test/verification-table.test.ts— aVERSION_NOT_UNIQUE_FLAGblock: no FRESH when the versionmatches, no publication (via either
POPULATE_VERSION_FLAGorexpectedVersion), caching resumesonce a later write gives the value a version of its own, the flag is read only from a tagged word,
and the sync / transactional / async / behind-a-snapshot paths each covered.
test/native/verification_table_test.cc— the predicate itself: tagged-and-marked, tagged-and-not,other flags in the same word, the same bit in an untagged word, and both too-short shapes.
pnpm test754 passed / 2 skipped, native GoogleTests 104 passed,tsc --noEmit/oxlint/oxfmt --checkclean.For the human reviewer
Start at
src/binding/core/verification_table.h— the contract and the predicate are the whole idea,and the predicate is deliberately permissive in one direction only. Then the four decision sites
(
database.cppsync,database.hasync,transaction.cpp,transaction_handle.cpp), which are thesame shape each time; the thing to check hardest is that no FRESH-answering path is left ungated, and
that the argument for leaving the pre-read fast path alone holds — it rests entirely on publication
being refused everywhere.
Decisions a reviewer might reasonably question, and why they went this way: the library now parses
one producer flag rather than staying agnostic about value bytes (it already parses the version from
those same bytes, and the alternative — keying the oracle on a per-write token such as the RocksDB
sequence number — is a much wider API change worth doing on its own terms, not as a bug fix); the
tag is 8 bits, so a header-less producer on a VT-enabled column family could in principle collide,
which costs caching and not correctness; and
populateVersion()is left alone, since it never sees avalue and by design trusts its caller.
Cross-model review ran three rounds (codex + gemini + harper-domain) and found real defects in the
first two — the snapshot-value hole and then a use-after-free in my first async attempt. Both are
fixed here; the review's remaining open items are the decisions above.
🤖 Generated with Claude Code
Human-Review-Need: 4 @ 6f66027