Fix sourcedFrom cache-fill conflict convergence - #2065
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces timestamp-based conflict resolution and deterministic ordering for source fills and revalidations in resources/Table.ts to prevent overwriting later writes and preserve exact-CAS semantics. Comprehensive unit tests are also added to cover various race conditions and caching conflict scenarios. The feedback suggests hoisting the monotonicTimestamp helper function out of the hot cache-resolution path to optimize performance by avoiding unnecessary closure allocations.
| const monotonicTimestamp = () => | ||
| isRocksDB ? (primaryStore as RocksDatabase).getMonotonicTimestamp() : getNextMonotonicTime(); |
There was a problem hiding this comment.
The monotonicTimestamp helper is currently defined inside getFromSource, which is on the hot cache-resolution path. Since it does not close over any variables local to getFromSource (only isRocksDB and primaryStore from the outer makeTable scope), we should avoid defining it inside this hot method to prevent unnecessary closure allocations on every execution. Hoist it to the outer makeTable scope (or module scope if possible) to optimize performance.
References
- Avoid defining helper functions inside methods on hot paths if they do not close over any variables from the outer scope. Hoist them to the module scope (or a wider cold-path scope) to avoid unnecessary allocations of function closures on every execution.
|
Reviewed; no blockers found. |
| ? (primaryStore as RocksDatabase).getMonotonicTimestamp() | ||
| : getNextMonotonicTime(); | ||
| const nextExistingVersion = |
There was a problem hiding this comment.
Suggestion (non-blocking): This makes monotonicTimestamp an eagerly-evaluated value instead of a lazily-invoked function, so getMonotonicTimestamp() / getNextMonotonicTime() now runs on every getFromSource call — even on the (likely common) revalidation path where inheritedTimestamp wins the ternary below and the computed value is discarded. Both are stateful/monotonic-clock calls (the RocksDB variant crosses the native binding boundary), so this trades a cheap closure allocation for an unconditional clock read on the hot cache-resolution path gemini flagged — likely costing more than it saves on the path where the value goes unused. Consider hoisting the arrow function itself to makeTable's outer scope (created once, not per getFromSource call) so evaluation stays lazy per the original ternary.
625f122 to
ab1d1f4
Compare
Let initial fills that raced a replicated winner resolve through the table's deterministic write ordering. Keep exact-CAS semantics for source revalidation, and update indices against the actual record being replaced. 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>
Honor a valid source-reported lastModified before the source transaction fallback timestamp. When a RocksDB replacement cannot advance the current version, retain that version and mark the encoded record as non-unique for rocksdb-js#766 instead of fabricating an epsilon timestamp.
ab1d1f4 to
43bba3b
Compare
Human-Review-Need: 4 @ 43bba3b
Important
Depends on HarperFast/rocksdb-js#766 — fix(vt): never vouch for a version the producer marked non-unique. Keep this PR in draft and do not merge it until #766 is merged, released, and Harper's dependency floor and lockfile are updated to that release.
Problem
Two nodes can independently resolve the same missing
sourcedFromkey. The old commit path compared against the entry captured before the source fetch, so a write that landed during the fetch was not resolved against the actual record being replaced. The first version of this PR fixed convergence by minting an incremented cache-record timestamp, but that conflated three different things:lastModifiedversion;source.get()runs.In particular, installing a fetch-start token on
sourceContext.timestampbackdated any write the source performed during resolution.Change
The updated sequence is:
sourceContext.timestamp. The source-resolution transaction, including source-side writes, retains its normal transaction timestamp.sourceContext.lastModifiedas the cache record's candidate version. Use the reserved token only when the source did not report one.VERSION_NOT_UNIQUE_FLAGrather than fabricating an epsilon timestamp. Test #766 makes that safe by refusing to publish or confirm the reused version through the VerificationTable.The producer flag is wired centrally in
recordUpdater, so it also covers the ordinary resequenced partial-write case that motivated #766. LMDB continues to store the source candidate directly because it retains separate source-version and local-time semantics.Dependency state
The current lock still resolves
@harperfast/rocksdb-js2.7.1, which predates #766. The end-to-end VerificationTable regression is therefore probe-gated and pending locally. It becomes active as soon as Harper installs a build exporting the matchingconstants.VERSION_NOT_UNIQUE_FLAG; at that point the dependency floor and lockfile must be updated before this PR leaves draft.Verification
npm run buildnpm run format:checkoxlinton changed source/testsschemaMigrationFragility.test.jsnative crash: 1,555 passing, 16 pending (the additional pending test is the Test #766-gated VT regression)mainDeliberate tradeoffs for human review
Refs HarperFast/harper-pro#645
Authored by KrAIs (Codex).