Retain a blob file for the duration of its replication send - #687
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the core subproject and introduces a mechanism to retain blob files during replication using holdBlobFile to prevent files from being reclaimed prematurely. The reviewer identified a potential resource leak where the blob hold and blobsBeingSent entry could leak if blob.stream() throws an error. Restructuring the function with an outer try...finally block was suggested to ensure robust cleanup and simplify the early exit paths.
Contributor
|
Reviewed; no blockers found. |
kriszyp
force-pushed
the
kris/blob-retention-refaware
branch
9 times, most recently
from
August 12, 2026 14:34
58b6735 to
15ff27f
Compare
…e id (#537) The receive-apply path wrapped record decode in a bare catch that logged and skipped EVERY decode failure, advancing the resume cursor past it without any classification or metric — silently sealing the skipped range and, for the genuinely-unrecoverable missing-structure case (harper#1163), never surfacing it. Two receive-path failures are now handled by their true nature: 1. Decode failure = PERMANENT (skip + surface). Transient conditions are handled upstream; every known decode source is root-caused, so one reaching this catch is unrecoverable old-version/corrupt data no re-copy heals (verified: the replication decoder is a raw StructonPackr that THROWS "Could not find typed structure" on an absent structure, and the sender always sends TABLE_FIXED_STRUCTURE before the dependent record — so a throw means the structure is genuinely absent, not merely unsynced). classifyReplicationDecodeError routes the missing-structure subclass to the same `decode-missing-structure` metric core's local-read path fires (alertable, concise for the #537 flood), everything else to a lazy error log. Both skip + advance. 2. Unknown table id = TRANSIENT (hold, don't seal). tableDecoders is populated only by TABLE_FIXED_STRUCTURE, which precedes the first record over an ordered WS, so a missing decoder means a missed structure sync or un-propagated schema (#1497) — recoverable on reconnect. The old code logged, fell through, and double-threw on tableDecoder.decoder inside the catch, escaping it and wedging the leg. Now it holds: close + reconnect + resume from the durable cursor (which re-sends TABLE_FIXED_STRUCTURE), without advancing past the record. This also makes tableDecoder provably non-null in the decode catch. Adds classifyReplicationDecodeError + unit coverage. harper-pro-only. Cross-model reviewed (Codex + Gemini + Harper-domain adjudication); reproduced against the harper#1163 divergence cluster harness (4/4 pass, mode-A throw confirmed, gap permanent, fix surfaces it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… component tests (#537) Two tests for the decode-classify fix landed in the parent commit: Test 1 — Integration: poisonedCopyCursorDataLoss.test.mjs Characterizes the cursor-trust symptom the #537 decode-drop exploited. A receiver node presents a copyCursor with afterKey='row-0500' (injected via HARPER_TEST_INJECT_COPY_CURSOR_JSON) against a 1000-row source. The leader trusts the cursor, skips rows row-0001..row-0500, and delivers only the tail; B ends up with 500 rows and believes itself current — the gap is permanent. copyStartTime is set to 24h in the future to suppress audit replay (which would otherwise backfill the skipped range and hide the symptom). Passes 1/1 (~24s). Marked as a characterization/regression-guard: this still- present cursor-trust behavior is correct for a legitimate interrupted copy; it only becomes a data-loss vector when the cursor is wrong. Adds a minimal test hook (maybeInjectCopyCursorForTest) following the existing HARPER_TEST_COPY_STALL_ONCE_DB / HARPER_TEST_REPLICATION_WEDGE_DB pattern. The hook only fires when the env var is set and never overrides a real cursor. Test 2 — Unit (component fallback): decodeDropMissingStructure.test.mjs Drives the real StructonPackr encode/decode path (not synthetic errors) to confirm the full classification chain for a genuine missing-structure failure: StructonPackr encode auto-learns struct 0 → decode with empty typedStructs throws "Could not find typed structure 0" → isMissingStructureError → true → classifyReplicationDecodeError → 'skip-missing-structure'. Also pins DECODE_MISSING_STRUCTURE_METRIC = 'decode-missing-structure'. Passes 6/6 (< 5ms). Does NOT cover: full cluster end-to-end (no real replication frames), the readAuditEntry + getValue binary path (direct decoder call only), or actual recordAction metric firing (the branch condition is what's verified). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ed-cursor test (#537) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dawsontoth
approved these changes
Aug 12, 2026
…ture-hold close (#537) Addresses PR #545 review: - Count the generic decode-drop arm, not just missing-structure — the field failure (Unexpected end of MessagePack data) lands there, so drops were silent. - Set wsClosed before the unknown-table hold's close(), so queued/in-flight frames can't advance the resume cursor past the held record (markInboundClosed-first, #440). - Add a decode-hold metric so a held leg is observable, not just a log line. - Note the transient-blob-in-flight gap (needs #403 blob-gap taxonomy) as follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…de lazily (#537) The earlier note claimed an in-flight blob surfaces at this catch as a generic underrun and is dropped. Tracing the receive path shows that can't happen: a blob reference decodes to a lazy handle (core blob.ts unpack), so no read occurs here and an in-flight blob produces no decode error — its transient(503)/permanent split is the async receiveBlobs/#403 path. What reaches this catch is a truncated/mis-framed or structure-forked record buffer. Comment now reflects the actual mechanism. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A blob resolved from the record being replicated is not opened until `blob.stream()`, and between resolving it and getting there the send can park on the outstanding-sends cap for an unbounded time. A write that supersedes the record in that gap reclaims the file, so the send reads a missing file and reports it to the peer as unrecoverable at source; the peer advances its resume cursor past a record whose bytes it will never have (#403/#388), leaving a live record with a dangling blob reference until a manual repair_blob_data run. Take a core retention hold for the send and release it on every exit path. The hold is lock-backed in core, so it is visible to the thread that does the unlinking rather than only to the sending thread. Bumps core to the matching reclamation change (harper#2134). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/blob-retention-refaware
branch
from
August 13, 2026 03:07
15ff27f to
c02be90
Compare
Co-authored-by: kriszyp <34054+kriszyp@users.noreply.github.com>
…ify-537 fix(replication): classify undecodable records + hold on unknown table id (#537)
…ger condemns a link to endless base copies hasBlobGap is a per-connection one-way latch: a single transient blob-save fault pinned the durable resume cursor, stopped the copy cursor, and blocked maybeFinishCopy for the rest of the connection's life. Only a reconnect heals it (fresh state re-streams the gapped blob), but a latched connection keeps flowing frames and answering pings, so no watchdog ever noticed it — in the field a link sat latched for 3.7h until an unrelated reconnect resumed a whole-table base copy, and any copy that took even one fault never exited copy mode, re-copying 376GB on every reconnect (#683). Three complementary mitigations, none of which weakens the transient-fault- must-hold durability invariant: - Blob-gap reconnect timer (createBlobGapReconnectTimer): armed when the latch sets, forces the healing reconnect after blobTimeout (900s default) if none happens on its own — mirroring the #453 copy-progress watchdog pattern. A latched link now heals in bounded time instead of never. - Last-durable copy-cursor snapshot (lastDurableCopyCursor): once a gap holds, flushDurableCopyCursor used to persist nothing for the rest of the connection; it now persists the last durable-eligible staged cursor once (behind the same #480 store flush), so the forced reconnect resumes the copy from the last gap-free point instead of the last cadence flush. - Sender-side 503 retry (BLOB_SEND_RETRY_DELAYS_MS, escalating 250ms-2s): the dominant latching fault is the source failing to read its own PENDING placeholder from a concurrent receive (#481) — measured to self-heal at the source within seconds (5,701/5,701 healed on re-check). sendBlobs now retries the read in place, only while no chunk is on the wire, before forwarding the error. Under all-to-all copies every node is source and receiver at once, so without this no large copy could complete gap-free. 503 stays classified transient at the receiver — reclassifying it as permanent (the other proposed fix) would trade a seconds-long contention blip for permanent record divergence. Fixes #683 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSY7Eta7Xvsr7NTtcm1CRj
…g (pre-push review round 1) Cross-model review findings on the #683 fix, all addressed: - major: the copy-cursor flush retry timer and a late flush .then could outlive the connection and overwrite a replacement connection's newer cursor (or resurrect a removed one). persistCopyCursor now early-returns on wsClosed (entry and post-flush), and the close handler clears copyFlushRetryTimer. The .then guard also covers the pre-existing #480 exposure this change made reachable at the worst instant. - major: the gap-path snapshot persist runs inside the blob-save .finally whose promise onCommit awaits, so a synchronous store throw (likely the same ENOSPC that latched the gap) escaped as an uncaughtException. The gap branch now catches, logs, and leaves the snapshot unpersisted. - minor: the blob-gap watchdog now re-arms after each fire instead of latching one-shot: forceReconnect() can be a no-op (retry scheduled then abandoned), and a single wasted fire would leave the wedge unbounded again. Siblings (copy-progress, pause-stall) already repeat. - minor: blob-send retries no longer run while draining for worker shutdown, and a purely-retrying send no longer reports drain progress; the gate is extracted as shouldRetrySourceBlobRead and unit-tested. - accuracy: DESIGN.md no longer overstates the snapshot's value — the recovered distance is bounded by the flush cadence, not "the rest of the connection". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSY7Eta7Xvsr7NTtcm1CRj
…t close (pre-push review round 2) Round-2 findings, all addressed: - major: wsClosed is the wrong liveness test — forceReconnect's teardown of the old socket is best-effort and may never fire 'close' (#420), so a zombie closure could re-fire the repeating blob-gap watchdog against each healthy replacement every blobTimeout, and its cursor-flush retries could still overwrite the replacement's newer cursor. All such actions now gate on connectionSuperseded() (wsClosed OR options.connection.socket !== ws, the same identity idiom the close handler uses), and a superseded watchdog fire stops its own timer. - minor: the sync-throw containment moved from the gap-branch call site into persistCopyCursor, covering the staged path too (pre-existing exposure) — a shared onPersistFailure now restores state, applies the same escalating backoff, and schedules the same re-drive for sync and async failures alike. - minor: blob.stream() / iterator creation moved inside the try so a synchronous 503 (or any sync throw) still engages the retry and the error frame instead of bypassing both. - nit: watchdog warn logs the effective timeout rather than a possibly 0/undefined raw blobTimeout; the copy-progress sibling had the identical bug and now shares effectiveBlobTimeoutMs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSY7Eta7Xvsr7NTtcm1CRj
…-push review round 3) Round-3 findings, all addressed: - major: the ownership invariant gated every WRITE to the shared [copyCursor, nodeId] key but not its DELETE — a superseded closure's late flush .finally could reach maybeFinishCopy and remove the replacement connection's cursor, forcing a from-scratch re-copy. maybeFinishCopy now early-returns when superseded. - minor: a zero-chunk (legitimately empty) blob threw a TypeError whose code-less error frame latched the receiver's gap — with the repeating watchdog that would mean a reconnect cycle every blobTimeout, forever. The terminal frame now goes out with an empty body instead. - nit: onPersistFailure no longer clears copyFlushInFlight it may not own; flushRootStore() is invoked before the flag is taken, so a sync throw can never orphan (or falsely clear) another flush's flag. - test coverage: the ownership predicate is extracted as the pure isConnectionSuperseded and pinned by connectionSuperseded.test.mjs, including the replaced-socket-without-close (#420) shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSY7Eta7Xvsr7NTtcm1CRj
…kew (pre-push review round 4) Round 4 verdict: COMMENTS, all prior majors verified resolved. Two small items taken: - The flush-failure retry timer IS the backoff expiring, but Node timers can fire fractionally early relative to performance.now(); an early fire hitting the backoff guard no-oped with nothing left to re-drive a quiescent link, silently degrading the snapshot persist to pre-change behavior. The retry callback now clears copyFlushBackoffUntil first. - The watchdog callback runs under try/catch: re-armed, a throwing callback would otherwise repeat a worker crash every cycle. Declined (recorded in the PR): send-slot hold under minutes-long PENDING, production-path/e2e harness (follow-up), comment-volume nits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSY7Eta7Xvsr7NTtcm1CRj
…ack-pressure and from superseded sessions (#695) Fixes two replication watchdog defects that killed healthy base copies (Closes #697). 1. The copy-progress watchdog counted intentional back-pressure pauses as copy stalls: the commit-backlog pause stops the watchdog via addPauseReason(), but the same frame's noteCopyProgress() re-armed it unconditionally. noteCopyProgress now resets only when pauseReasons === 0, mirroring resetPingTimer; removePauseReason re-arms on resume. 2. Watchdog fires from superseded replicateOverWS instances acted on the shared NodeReplicationConnection, tearing down the current healthy leg. Supersession now runs the full, idempotent instance teardown (retireInstance): NodeReplicationConnection.retireSession() is the single enforcement point for "at most one live session per connection", called at the socket-replacement point in connect() and up front in forceReconnect(); a session created by an open() that raced a replacement is retired on the spot and not resolved into the replacement's session promise. A socket-identity guard in all four fire handlers remains as a backstop. Regression coverage: copyProgressBackpressureFalseFire.test.mjs and watchdogSupersededInstanceFire.test.mjs, driven by one-shot env-gated fault injections; copyProgressWedgeRecovery (genuine-stall recovery), replicationWedgeRecovery (#420) and replicationReconnect verified as non-regressions. Out of scope, tracked in #697: why silent-from-birth surplus legs exist for the same (db, peer) — that needs sender-side evidence.
kriszyp
marked this pull request as ready for review
August 15, 2026 00:57
…DB instead of marking the node Available mid-copy (#657) * Clone sync check: verify no-target databases by the copy-completion watermark checkSyncStatus skipped any database whose leader target timestamp was falsy. On RocksDB the describe operations report last_updated_record for no table (harper#2091, getKeys TODO stub), so every target was 0, every socket was skipped, and the first poll declared "All databases synchronized" seconds into a multi-GB base copy — the clone marked itself Available and cloned with a fraction of the leader's data (#655; Large-Data stress run 31006315068). A no-target database now requires a positive received-version watermark instead of being skipped. The watermark is held at 0 for the whole bulk copy and only becomes positive via the final end_txn the sender always emits at copyStartTime — the copy's own completion signal — so this is correct against any leader version and for empty databases (their copy still emits the final end_txn). A database with a target but no socket yet also holds completion, so a lone early socket (the system DB's small copy finishes in seconds) cannot complete the check vacuously either. Fixes #655 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Scope the missing-socket hold to subscribed databases so a v4 leader cannot wedge the clone A legacy (v4) leader never replicates the system database, but system is added to the sync targets unconditionally, so requiring every target to have a replication socket wedged the clone forever against a v4 leader (cloneFromLegacy CI failure: "data: Synchronized" + "system: no replication socket" repeating until the Available timeout). checkSyncStatus now takes the set of databases whose socket is required (cloneNode passes the user databases); a non-required database is still verified whenever its socket does exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Ratchet seen sockets into the required set so a dropped system socket cannot complete the clone From the cross-model review of the v4-leader fix: making the system socket optional for v4 leaders also removed its socket-presence gate on v5, where a small user database's copy could complete before system's socket registers. Once a target database's socket has been seen it now stays required — its later loss holds completion instead of completing around it — while a socket that never appears (v4 leader) still cannot wedge the clone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Filter sync targets by replication.databases and require the system socket by leader version From PR #657 review threads: - gemini-code-assist: with replication.databases configured, non-replicated leader databases became sync targets whose sockets never exist, wedging the clone. getLastUpdatedRecord now applies the same isReplicatedDatabase gating cloneSchemas uses (helper hoisted to share it). - KrAIs: whether the system socket is required is a leader capability, not an ordering heuristic — probe it via registration_info (present on every leader version) and require system up front on v5+, closing the reverse race where a small user database completes before the system subscription registers. Unknown version leaves system optional with the seen-socket ratchet as the fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fail the leader-version probe closed and exclude sharded entries from sync targets From the cross-model review of the previous commit: - The registration_info probe failed open: a transient error or a missing/unparseable version left the system socket optional on a real v5 leader, reopening the premature-Available race. Only a positively-read legacy major version now exempts system; the probe retries and persistent failure requires the socket (a wedge is visible; a premature Available is not). - isReplicatedDatabase accepted sharded entries by name, but a sharded entry replicates only from same-shard peers (shouldReplicateFromNode) — the leader's shard is unknown here, so a differently-sharded database would have become a required target with no socket, wedging every sharded clone. Sync targets now exclude sharded entries; when one does replicate, its socket exists and the watermark rule plus the seen-socket ratchet still verify it. Schema pre-create keeps its original name-based gating. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Evaluate the real shard predicate for sync targets instead of excluding sharded entries From the cross-model review: excluding every sharded entry from sync targets reopened the ordering race for a same-shard sharded database (replicating, copied, but never required — a small user database could complete the clone around it). The shard predicate is now evaluated exactly as shouldReplicateFromNode does — the leader's shard read from its configuration (get_configuration), compared to replication.shard — failing closed to keep sharded targets when the configuration is unreadable. The predicate logic moves to cloneNode/replicatedDatabases.ts (pure, unit-tested); schema pre-create keeps its original name-based gating via the fail-closed default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Tolerate prefixed leader version strings in the probe Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Persist clone copy completion across worker restarts Co-Authored-By: GPT-5 Codex <noreply@openai.com> * Harden clone completion restoration Co-Authored-By: GPT-5 Codex <noreply@openai.com> * Preserve selective clone targeting Co-Authored-By: GPT-5 Codex <noreply@openai.com> * fix(clone): don't require a system socket the node never subscribes to The sync monitor required the `system` replication socket on any v5+ leader. `shouldReplicateFromNode` runs every database through `replication.databases`, system included, so a node configured with e.g. `databases: ['data']` never opens a system socket — the clone then waited Unavailable until the stall window expired even though its data was fully copied. Gate the requirement on this node's own `replication.databases` first, and only probe the leader version when the local config actually subscribes to system. Also drops an unused callback parameter that failed oxlint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: GPT-5 Codex <noreply@openai.com>
Co-authored-by: kriszyp <34054+kriszyp@users.noreply.github.com>
dawsontoth
approved these changes
Aug 17, 2026
…690) (#691) * test(replication): e2e guard for the decode-drop wedge recovery (#537/#690) Adds a stress-gated two-node cluster test that reproduces the #521 wedge trigger — a replicated record whose value can't be decoded — via a new env-gated one-shot injection hook (maybeInjectDecodeFailureForTest, HARPER_TEST_DECODE_FAIL_RECORD_PREFIX), same pattern as the sibling copy-cursor / stall injections. Asserts the #545 disposition end to end: every clean row is delivered (leg not starved), poison records are skipped (absent, not applied), the connection never enters the close/resume loop (0 inbound-close lines), and a live post-poison write still replicates. The behavior is already proven on real containers in HarperFast/harper-fabric-lab#36 (stock catch: 452 close/resume cycles, 0/5 rows; fixed catch: 0 closes, leg alive); this lands the same coverage in the cluster suite, addressing the coverage gap noted on #545. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZ9NnEpv3BygcMJYvkyC9K * style(replication): prettier format on decode-drop test + hook Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZ9NnEpv3BygcMJYvkyC9K * perf(replication): cache decode-fail test prefix at load, not per record Cross-model review (Gemini, perf lens): reading process.env in the per-record injection hook is a native getter on the receive-apply hot path. The test env var is set before process start, so read it once at module load — the production (unset) path becomes a cached-undefined check with zero per-record env cost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZ9NnEpv3BygcMJYvkyC9K * ci(stress): run decodeDropRecovery in the receive-side regression matrix The stress matrix is an explicit file list, not a glob — a cluster test not named here is executed by no CI path (the PR shards skip stress-gated tests). Add the new decode-drop test so the stress suite actually runs it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZ9NnEpv3BygcMJYvkyC9K * test(replication): guard teardown against unset nodes; drop dead count assertion Addresses gemini-code-assist review on #691: teardownHarper(undefined) would throw synchronously if the before-hook didn't assign a node (.catch can't reach a sync throw) — guard on ctx.node*. And the post-waitForCount ok() was dead (waitForCount only returns on success, throws on timeout) — the throw is the assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZ9NnEpv3BygcMJYvkyC9K --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…arper companion lands (#704) * ci: companion-check gate for PRs dependent on companion PRs (e.g. harper core) Adds a workflow posting a companion-check commit status driven by Depends-on: markers in PR bodies, so a coordinated harper-pro PR can be approved and armed for auto-merge, then merge automatically once its harper companion lands. Merging such a PR fires the existing sync_core repository_dispatch so the core pointer is re-pointed at harper main promptly instead of at the nightly run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: harden companion-check per cross-model review Fail closed on unparseable markers, support repo#N shorthand, bound and dedupe refs, isolate per-dep/per-PR errors, restrict the cross-repo token to same-org refs, guard sweep/event races, and skip no-marker PRs in the cron sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: fail closed on partial markers, heal statuses in sweep, harden sync Round-2 review fixes matching the documentation-repo copy, plus harper-pro-specific ones: split the closed-event concurrency group so an edit to a merged PR cannot cancel a pending sync_core dispatch, drop the dead case variant in the normalize-core condition, and make Sync Core reset core's branch tracking to main so a merged core:set-branch override cannot wedge the nightly or dispatched sync. Adds the self-contained node test harness for the embedded script. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: run companion-check tests in runLinter; hedge not-found diagnostic Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: definitive 404s fail closed; align normalize grammar; guard sync reset to main Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: stateful script-block extraction and per-scenario fetch isolation (review feedback) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: GPT-5 Codex <noreply@openai.com>
…oveNodeBlastRadius nightly (#708) * test(cluster): retry the seed poll's unknown-attribute transient in removeNodeBlastRadius The 'B joins A and C' subtest polls B for the seeded record immediately after an upsert on A with replicatedConfirmation: 1. That confirmation can be satisfied by C, so the first poll can precede the record's arrival on B entirely — and search_by_id validates get_attributes against B's known attributes BEFORE reading, so while B has not yet learned the dynamically created 'value' attribute it answers 500 "unknown attribute 'value'" instead of an empty result. sendOperation asserts 200, and the waitUntil loop treated any error as fatal, so a legal, milliseconds-scale metadata transient failed the test outright (~half of nightly runs since 2026-08-13). Instrumented 3-node probe evidence: the transient reproduces at the very first attribute-bearing write (record not yet on B at the failure instant) and heals in p50 3ms / p95 7ms / max 74ms — well inside the poll's 45s budget. Retry exactly this transient in the seed poll; every other error still fails the test, and no later subtest is touched. The same probe surfaced a real, permanent attribute-propagation defect at the msgpackr 32-shared-structure cap, filed separately as #707 (not the cause of this flake — the test's table has 4 attributes). Refs #707 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LEA8N9FkRDU5nHfBp5jf6B * test(cluster): branch the seed-poll retry on status + error field per review Pre-push review round 1 (codex graded + cursor-composer + harper-domain adjudication) found the retry discrimination coupled to sendOperation's assert plumbing and the full stringified body, and noted a permanent metadata failure would degrade into an anonymous 45s timeout. Rework the poll to fetch directly, treat only a non-200 whose body.error starts with "unknown attribute" as retryable, fail every other non-200 through the same equal() shape as sendOperation, and append the last swallowed validation error to the timeout diagnostic. Also trims the comment to the load-bearing rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LEA8N9FkRDU5nHfBp5jf6B * test(cluster): clear the retained transient so the timeout diagnostic reflects the final poll state (delta review round 2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LEA8N9FkRDU5nHfBp5jf6B * test(cluster): reset the retained transient per poll iteration (delta review round 3) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LEA8N9FkRDU5nHfBp5jf6B --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… blob files on copy re-delivery (#701) * fix(replication): bank copy-cursor progress up to the first blob gap instead of discarding the walked prefix (#699) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: GPT-5 Codex <noreply@openai.com> * Address copy-gap repair review findings Co-Authored-By: GPT-5 Codex <noreply@openai.com> * Hold uncertain duplicate blob repairs for retry Co-Authored-By: GPT-5 Codex <noreply@openai.com> * Update core repair formatting Co-Authored-By: GPT-5 Codex <noreply@openai.com> * Use PID-aware restart in copy-gap integration test Co-Authored-By: GPT-5 Codex <noreply@openai.com> * fix(replication): validate leading duplicate blob health Co-Authored-By: GPT-5 Codex <noreply@openai.com> * perf(replication): bound duplicate blob probes Co-Authored-By: GPT-5 Codex <noreply@openai.com> * Preserve unknown blob probe results Co-Authored-By: GPT-5 Codex <noreply@openai.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: GPT-5 Codex <noreply@openai.com>
Preserve the replication sender's blob retention hold while carrying forward main's source-read retries, blob-gap repair, and current core pointer. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A blob resolved from the record being replicated is not opened until
blob.stream(), and between resolving it and getting there the send can park on the outstanding-sends cap for an unbounded time. A write that supersedes the record in that gap reclaims the file, so the send reads a missing file and reports it to the peer as unrecoverable at source — the peer then advances its resume cursor past a record whose bytes it will never have (#403/#388), leaving a live record with a dangling blob reference until someone runsrepair_blob_data.This takes a core retention hold for the duration of the send and releases it on every exit path.
Pairs with HarperFast/harper#2145, which adds the hold API and the reference-aware reclamation queue underneath it; the
corepointer is bumped to that commit. Reported context in HarperFast/harper#2134.Scope
This closes the sender-side window (resolve → park → open) only. It does not cover a peer that is far enough behind that the blob was already reclaimed before the send began — that case has no file to hold, and is covered instead by the core retention window (
storage.blobRetention), which an operator can raise to exceed their replication lag. Making retention properly lag-aware (retain until every peer's resume cursor has passed the superseding version) is a larger design and is not attempted here.Verification
Route: existing blob-replication integration suites, run locally with the stress gate enabled.
HARPER_RUN_STRESS_TESTS=1 npm run test:integration -- integrationTests/cluster/replicationBlobResyncOnFailure.test.mjs integrationTests/cluster/replicationBlobRepairAuthoritative.test.mjs integrationTests/cluster/blobSaveRejectionContainment.test.mjsReceive-side blob save rejection containment— passReceive-side blob resume-cursor clamp on transient save failure— passAuthoritative-table blob byte-integrity after receive-side save failure— fails, and fails identically on a cleanorigin/maintree with thecorepointer restored (ECONNREFUSED 127.0.0.15:9933during the restart-phase component deploy). Pre-existing in this local environment, not a regression from this change.an explicit retention hold defers reclamation until it is released).For the human reviewer
Three entries, all inherited from the core PR's design rather than introduced here.
holdBlobFilereports success anyway — so this changes nothing in exactly the lagging-peer case that produces the divergence in harper#2134. The lever for that case isstorage.blobRetention, or lag-aware retention, which is not attempted. Worth ruling on whether shipping the narrow fix is worth it on its own.blob.stream()throws before the try/finally, bounded by core's 20-minute age cap. The pre-existingblobsBeingSent/outstandingBlobsBeingSentstate leaks on that same path with no bound, so this is not a new class of problem — but it is a knowingly unfixed one.Human-Review-Need: 4 (decisions: hold-layer-pro-vs-core, hold-through-park-vs-at-slot, null-hold-proceeds-vs-declines, no-submodule-bump) @ fc9b493