Skip to content

Fix sourcedFrom cache-fill conflict convergence - #2065

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

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

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 4, 2026

Copy link
Copy Markdown
Member

Problem

Two nodes can independently resolve the same missing sourcedFrom key. The previous commit path pinned the entry captured before the source fetch, so its compare-and-swap compared that snapshot with itself. If a replicated fill arrived during the fetch, each node could skip its own fill and retain the other node's winner. Requests distributed across those nodes then appeared to combine metadata and blob values from different writes, even though each stored record remained internally paired.

This is not a general cross-thread commit visibility failure. The failure is specific to competing cache fills and their stale conflict snapshot.

Change

  • Mint the source-write ordering timestamp before the source fetch.
  • Reload the current entry inside the commit transaction.
  • Let positive first fills use deterministic table ordering when another write races them.
  • Keep exact-CAS behavior for revalidation and prevent a negative source result from deleting a raced record.
  • Diff indices and inherit createdAt from the record actually replaced.
  • Preserve version monotonicity when a revalidation inherits an older request timestamp.
  • Add deterministic unit coverage for both first-fill orderings, revalidation CAS, version monotonicity, index cleanup, created-time inheritance, and raced deletion.

The coordinated Pro PR adds the two-node/two-worker external-blob regression.

Verification

  • npm run build
  • npx mocha unitTests/resources/caching.test.js — 25 passing
  • Coordinated Pro regression — 10/10 post-fix source-fill races
  • Existing Pro RocksDB + LMDB cluster replication file — 10/10, including cached blobs
  • Independent full review at 5d4c123b6 plus final graded delta at 625f122e9 — Claude graded review + Harper-domain adjudication

The surgical pre-fix harness failed repeatedly after every worker had reached a stable state; the same 10-trial harness passes after this change.

Tradeoff

The fill version represents fetch start so a write that lands while the source is outstanding wins. That can place an audited source-fill entry behind unrelated commits made during a slow fetch. The current RocksDB audit iterator is commit-ordered, but timestamp-gated consumers deserve explicit review before this is marked ready.

Refs HarperFast/harper-pro#645

Authored by GPT-5 Codex.

🤖 Generated with Claude Code

kriszyp and others added 4 commits August 3, 2026 17:35
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>

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

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Comment thread resources/Table.ts
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.

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