Skip to content

Fix sourcedFrom cache-fill conflict convergence - #2065

Draft
kriszyp wants to merge 7 commits into
mainfrom
fix/sourced-from-cache-fill-conflict
Draft

Fix sourcedFrom cache-fill conflict convergence#2065
kriszyp wants to merge 7 commits into
mainfrom
fix/sourced-from-cache-fill-conflict

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 4, 2026

Copy link
Copy Markdown
Member

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 sourcedFrom key. 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:

  • the source's authoritative lastModified version;
  • the local token used to order competing first fills;
  • the timestamp of the transaction in which source.get() runs.

In particular, installing a fetch-start token on sourceContext.timestamp backdated any write the source performed during resolution.

Change

The updated sequence is:

  1. Reserve a monotonic ordering token before the fetch. An inherited request/transaction timestamp is used when present; otherwise the storage engine supplies one.
  2. Do not install that token on sourceContext.timestamp. The source-resolution transaction, including source-side writes, retains its normal transaction timestamp.
  3. After the source responds, use a valid sourceContext.lastModified as the cache record's candidate version. Use the reserved token only when the source did not report one.
  4. Reload the current entry inside the commit transaction. Revalidations keep exact-CAS behavior; first fills use the candidate version to order against a raced write; a source miss never deletes a raced record.
  5. On RocksDB, if the source candidate cannot advance the current record version, store at the current version and set VERSION_NOT_UNIQUE_FLAG rather 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-js 2.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 matching constants.VERSION_NOT_UNIQUE_FLAG; at that point the dependency floor and lockfile must be updated before this PR leaves draft.

Verification

  • npm run build
  • npm run format:check
  • targeted oxlint on changed source/tests
  • focused cache suites: 39 passing, 1 pending on the Test #766 dependency
  • resource suite excluding the pre-existing schemaMigrationFragility.test.js native crash: 1,555 passing, 16 pending (the additional pending test is the Test #766-gated VT regression)
  • the excluded migration crash reproduces on untouched main
  • independent delta review: Claude + Gemini

Deliberate tradeoffs for human review

  • A source-reported timestamp is authoritative and is not capped to local fetch time. A badly future-dated source can therefore establish a high version floor; silently replacing it with a local timestamp would violate the source-version contract.
  • A same-version RocksDB replacement is intentionally not VerificationTable-vouchable until a later write advances its version. This favors correctness and avoids fabricated versions, at the cost of the VT fast path for repeatedly clamped cache rows.
  • The reserved monotonic token is still necessary for deterministic first-fill ordering when the source reports no version. It is not used as the source transaction timestamp.

Refs HarperFast/harper-pro#645

Authored by KrAIs (Codex).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread resources/Table.ts Outdated
Comment on lines +5655 to +5656
const monotonicTimestamp = () =>
isRocksDB ? (primaryStore as RocksDatabase).getMonotonicTimestamp() : getNextMonotonicTime();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. 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.

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Comment thread resources/Table.ts Outdated
Comment on lines +5656 to +5658
? (primaryStore as RocksDatabase).getMonotonicTimestamp()
: getNextMonotonicTime();
const nextExistingVersion =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kriszyp
kriszyp force-pushed the fix/sourced-from-cache-fill-conflict branch from 625f122 to ab1d1f4 Compare August 7, 2026 04:20
kriszyp and others added 7 commits August 14, 2026 08:06
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.
@kriszyp
kriszyp force-pushed the fix/sourced-from-cache-fill-conflict branch from ab1d1f4 to 43bba3b Compare August 14, 2026 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant