Fix sourcedFrom blob metadata divergence - #647
Conversation
Pin connections to every worker on two replicated nodes and race independent sourcedFrom fills through an external barrier. Require the raw record, point reads, metadata, and blob payload to converge on one write. 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>
There was a problem hiding this comment.
Code Review
This pull request adds a new integration test suite and associated fixtures to verify that a sourced record's metadata and blob converge correctly during competing cache fills across multiple nodes. The feedback recommends replacing CommonJS-specific globals with ESM-safe fallbacks, ensuring parallel processes are tracked for cleanup even if one fails, wrapping test cleanup steps in try-catch blocks to prevent resource leaks, and adding safety checks for potentially null payload references.
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
|
Reviewed; no blockers found. The prior finding (node-orphan on partial |
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Point core at the current head of HarperFast/harper#2065 so the dependent regression stays aligned with the implementation it validates. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Clarify why the raw stores are scanned again after every worker has materialized the record. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
| @@ -1 +1 @@ | |||
| Subproject commit f3246b9982eec796599740931f1c236e94957cd5 | |||
| Subproject commit ac6007bf6280802172ee40f690ea1263fb04f996 | |||
There was a problem hiding this comment.
High: the pinned core writes VERSION_NOT_UNIQUE_FLAG, but no published @harperfast/rocksdb-js reads it
This bump moves core to ac6007b (head of harper#2065 — Fix sourcedFrom cache-fill conflict convergence), which adds VERSION_NOT_UNIQUE_FLAG = 0x10000 in resources/RecordEncoder.ts and sets it whenever a RocksDB cache write cannot advance the version (including an ordinary 304 revalidation, where recordVersion === existingVersion). Core's DESIGN.md states the safety property that depends on it:
rocksdb-js#766then refuses to publish or confirm that version through the VerificationTable.
That consumer is not in any release you can install:
- rocksdb-js#766 "fix(vt): never vouch for a version the producer marked non-unique" merged 2026-08-18.
v2.7.1was tagged 2026-08-06, before that merge;npm dist-tagsshowslatest: 2.7.1with nothing newer published.- Grepping the installed 2.7.1 for
VERSION_NOT_UNIQUE/notUnique/0x10000/65536returns zero matches.
So the producer sets the bit and nothing reads it: stale record-cache values stay eligible for VerificationTable fast-path confirmation, which is exactly what the flag exists to prevent.
Compounding it, this PR does not move the dependency: harper-pro declares ^2.6.1 and package-lock.json pins 2.6.1, while the core you are pinning declares ^2.7.1. Since harper-pro compiles core into its own dist and resolves from its own node_modules, the pinned core runs against 2.6.1 — below its own declared minimum.
Suggested fix: block this on a @harperfast/rocksdb-js release containing #766, then bump harper-pro's dependency and lockfile to it in this PR. If that release is not close, say explicitly in the PR body that the flag is inert until then, so the VerificationTable gap is a known, tracked residual rather than an assumed-closed one.
—
Generated by Barber AI
| @@ -1 +1 @@ | |||
| Subproject commit f3246b9982eec796599740931f1c236e94957cd5 | |||
| Subproject commit ac6007bf6280802172ee40f690ea1263fb04f996 | |||
There was a problem hiding this comment.
Medium: the new regression test has never run against the core pin it is meant to validate
The PR's Verification section reports 10/10 races and 585 unit passes, but those were produced against an older core. Workflow history for this branch:
- Integration Tests ran on
7c83f72,04e2aee,9f46d08(all 2026-08-04) —corewas625f122then. 767edd2("chore: update core companion", 2026-08-25) movedcoretoac6007b, the pin under review.- On
767edd2andfae303a, only Companion Check, Smoke Tests and Component Stress Tests ran. Unit Tests, Integration Tests and Lint did not run at all.
So no run of sourcedBlobPairing.test.mjs has ever exercised ac6007b.
That gap is not covered elsewhere, and it is measurable. I mutated the convergence guard this PR exists to validate — core/resources/Table.ts:6012, const replacesRacedRecord = racedVersion == null || sourceVersion > racedVersion replaced with = true, which reinstates precisely the divergence being fixed — rebuilt, and confirmed exactly one isolated occurrence in dist/core/resources/Table.js:5831. harper-pro's unit suite returned 584 passing / 1 failing, byte-identical to the unmutated build (the one failure is injectedKeyCustody, a /proc test that cannot pass on macOS). The mutation survived: the unit suite provides zero coverage of this fix, so the integration test is the only guard, and it has not run here.
Also worth noting the pin is not just the fix. f3246b9..ac6007b is 218 files / +21,507 lines; decomposed, harper main advanced 215 files / +21,150 between the two merge bases, and #2065 itself contributes only 5 files / +502. A commit titled "chore: update core companion" is carrying a large unrelated core advance. To its credit, I built and tested both sides and found no regression from it: base 24aca7f and head fae303a produce an identical 29 baseline type errors (9 analytics/profile.ts, 20 replication/replicationConnection.ts) and an identical 584/1 unit result — so the PR body's "unrelated baseline type errors" claim checks out.
Suggested fix: push an empty commit or re-run the workflows so Unit + Integration Tests execute against ac6007b before approval, and call out the core advance in the PR body.
—
Generated by Barber AI
| } | ||
|
|
||
| tables.PairRecord.sourcedFrom({ | ||
| async get(id) { |
There was a problem hiding this comment.
Medium: the fixture never reports lastModified, so the test only covers the easy half of the convergence claim
get(id) takes no context and never sets context.lastModified. In the pinned core that makes validReportedVersion false on every fill, so sourceVersion falls back to sourceTimestamp — a node-local monotonic timestamp. Two nodes therefore always mint distinct versions, and convergence follows from ordinary version-ordered LWW.
The branch that is left untested is the one core's own DESIGN.md singles out as the dangerous one:
two replicas resolving the same tie could keep different values at the same version — the one state anti-entropy cannot repair.
A source that does report lastModified — the canonical caching case, an origin echoing Last-Modified — gives both replicas the same candidate version. That is the equal-version tie. Nothing in this test reaches it, and nothing reaches the version-capping logic (versionCeiling = Math.max(sourceTimestamp, Date.now())) that the same core change added to stop a future-dated source version freezing a row.
To be clear about what I did and did not find: I traced the tie path and it does appear to converge. resources/replayLogs.ts:201 passes the originating nodeId into the write options, so precedesExistingVersion breaks a cross-node tie on the originating node name, symmetrically on both replicas — not on getThisNodeId(). So this reads as a genuine convergence fix rather than a rarity fix, and order-independence holds in both the distinct-version and equal-version cases. This is a coverage gap, not a known defect — but it is the gap that matters, because it is the half of the claim the test cannot support. (Static analysis only; I did not run the integration test.)
Suggested fix: add a trial variant where the origin returns a fixed Last-Modified while still minting distinct per-fill tokens, and assert the same convergence. That is the interleaving that distinguishes "converges" from "usually converges".
—
Generated by Barber AI
| logging: { colors: false, stdStreams: false, console: true }, | ||
| replication: { securePort: `${nodeCtx.harper.hostname}:9933` }, | ||
| threads: { count: WORKERS }, | ||
| }, |
There was a problem hiding this comment.
Medium: only the default storage engine is exercised, though the fix is explicitly engine-divergent
This config sets no HARPER_STORAGE_ENGINE, so every trial runs on RocksDB. The core change branches on isRocksDB in two places that decide the stored version:
resources/Table.ts:recordVersion = isRocksDB && racedVersion != null ? Math.max(sourceVersion, racedVersion) : sourceVersion— RocksDB clamps a non-advancing candidate, LMDB stores the source candidate directly.resources/RecordEncoder.ts:VERSION_NOT_UNIQUE_FLAGis only set whenisRocksDB.
DESIGN.md states the divergence deliberately: "LMDB stores the source candidate directly, preserving its separate source-version/local-time semantics; only RocksDB clamps a non-advancing candidate because it uses one version for both roles." The LMDB half of that behavior has no coverage in this regression.
The PR body notes fullyConnectedReplication.test.mjs passes "10/10 across RocksDB and LMDB", but that suite does not exercise competing sourcedFrom fills, so it does not close this.
Suggested fix: parametrize the suite over HARPER_STORAGE_ENGINE (rocksdb and lmdb), as fullyConnectedReplication.test.mjs already does.
—
Generated by Barber AI
| @@ -0,0 +1,362 @@ | |||
| /** | |||
| * Regression for harper-pro#645: a sourcedFrom record's metadata and blob must | |||
| * converge as one winning write when two nodes independently fill the same key. | |||
There was a problem hiding this comment.
Medium: no stated residual for records that already diverged
The fix is forward-only, which is fine, but the PR body states the original symptom had "no self-healing" and then never says what happens to rows that already diverged before this lands. Two replicas sitting on different values at the same version will not be repaired by this change — that is the state DESIGN.md calls "the one state anti-entropy cannot repair" — and a fill only re-runs when the record is missing, invalidated, or expired, so an already-filled divergent row is not naturally revisited.
Suggested fix: state the residual explicitly in the PR body — whether operators should invalidate or evict affected caching tables after upgrade, or whether normal expiry is considered sufficient. A one-paragraph upgrade note is enough; the gap is that a reader currently cannot tell whether existing divergence is expected to clear.
—
Generated by Barber AI
| const originTrial = ctx.origin.trial(id); | ||
| ok(originTrial, `${id} performed no source fills`); | ||
| equal(originTrial.calls.length, 2, `${id} must perform exactly two independent source fills`); | ||
| equal(originTrial.timedOut, false, `${id} barrier timed out, so this trial is inconclusive`); |
There was a problem hiding this comment.
Low: a genuinely inconclusive trial is asserted as a failure
The barrier releases early only when two calls arrive; otherwise it fires after 10s with timedOut = true, and this line then fails the run. But a trial where one node never issues an independent fill — because it learned the key through replication first — has not demonstrated a convergence bug, and the assertion message says as much ("so this trial is inconclusive"). Asserting on it converts a scheduling accident into a red build.
With TRIALS=10 running sequentially, each able to spend up to 10s in the barrier plus two 30s convergence waits, this also sits close to the suite's 300000ms budget when trials are slow.
Suggested fix: retry the trial on a timed-out barrier (bounded, say 3 attempts) and fail only if the race cannot be staged repeatedly — keeping genuine non-convergence a hard failure while not failing on an unstaged race.
—
Generated by Barber AI
There was a problem hiding this comment.
Reviewed fae303a and found no blocking issues. No new blocking findings were confirmed in the supplied diff. Material concerns at the changed paths are already covered by the supplied discussion and were not repeated.
—
Generated by Barber AI
heskew
left a comment
There was a problem hiding this comment.
Reviewed exact head fae303aa. I found no distinct new code defect, so I am not adding duplicate inline comments.
Requesting changes for the exact-head correctness and qualification gaps already identified by @cb1kenobi: this head advances core to ac6007b while Pro still resolves @harperfast/rocksdb-js 2.6.1, which predates the verification-table consumer required by that core change, and the added regression has never run in CI against this core/dependency combination. Current Pro main now carries RocksDB 2.8.0 and a newer core containing the merged companion, so rebasing and resolving the sole submodule conflict should address the dependency mismatch; the focused regression then needs to run on that rebased head before approval.
@cb1kenobi’s equal-version/lastModified, LMDB, pre-existing-row recovery note, and inconclusive-barrier threads remain useful coverage and documentation follow-ups. Under the scope rule for this review, I am not treating those as additional blockers absent a demonstrated regression introduced by this PR.
Verification here: npm ci and git diff --check pass. Build and full lint reproduce documented baseline failures only. The focused integration test could not start because this macOS environment lacks the required 127.0.0.2 loopback alias; that is environmental, not a product failure. The core companion, HarperFast/harper#2065, is merged.
🤖 Posted by Codex on behalf of @heskew
Problem
Concurrent independent
sourcedFromfills can settle on opposite winners across replicated nodes. The original four-node cached-blob test exposed this as metadata from one fill paired with a blob endpoint response from another, with no self-healing.The smaller harness isolates the mechanism:
The records were not torn internally. Each node could retain a different complete winner after both local fills encountered a peer fill. Routing metadata and blob reads across those divergent nodes produced the apparent split.
Change
This PR adds the surgical regression and points core at HarperFast/harper#2065 — Fix sourcedFrom cache-fill conflict convergence.
The test requires one stable
(version, token)across both nodes and every worker, then verifies both the raw record and the materialized blob bytes carry that token. It also makes origin barriers and convergence polling resilient to late calls and transient restart responses.The core PR reloads commit-time state, applies deterministic ordering to competing positive first fills, preserves strict revalidation/deletion safety, and updates indices/created-time metadata against the actual winner.
Evidence
Before the core fix, the stable all-worker assertion failed repeatedly and sometimes showed the nodes retaining swapped winners after 30 seconds. Surgical controls remained clean:
sourcedFromraces: 5/5This excludes a general blob atomicity or cross-thread visibility failure and isolates the source-fill conflict path.
Verification
HARPER_645_TRIALS=10 HARPER_645_WORKERS=2 node --test integrationTests/cluster/sourcedBlobPairing.test.mjs— 10/10 races passedfullyConnectedReplication.test.mjs— 10/10 across RocksDB and LMDB, including “Replicating cached blobs”npm run test:unit— 585 passingnpm run buildand caching suite — clean, 25 passing7c83f72bplus graded delta at04e2aeee— Claude graded review + Harper-domain adjudication9f46d08d— review gate failed because the generated artifact omitted its verdict; locally verified 10/10 in a fresh poolThe Pro build still reports unrelated baseline type errors in
analytics/profile.tsand replication WebSocket typings; this change does not touch those paths.Dependency
Depends-on: HarperFast/harper#2065
This PR may be reviewed and approved now. The required companion check keeps it unmergeable until the core PR lands; while it remains open, rebase automation must keep
corepinned to its current head.Fixes #645
Authored by GPT-5 Codex.
🤖 Generated with Claude Code
Human-Review-Need: 4 @ 9f46d08