Repository navigation
Select the copy-gap banking test's blob faults by record so the copy's post-walk tail cannot shift them - #903
Merged
Merged
Conversation
…y save ordinal The fault injector counted every data blob save and failed fixed ordinals (15, 31, 42), which assumed the base copy's first pass writes exactly one blob per record. Since harper-pro#878 the leader tails its log in append order after the max committed key, so entries appended after that key (transactions created earlier, committed later) are re-delivered after the walk. At ae7f001 the CI run's first pass had 42 saves — 40 records plus two re-deliveries — so the 42nd fault hit a re-delivery instead of the resumed walk's second repair, the resumed pass had no fault, and the test saw a single cursor-based resume. Faults are now `<record>:<n>` (that record's n-th fresh save) or `<record>:repair` (its first in-place repair), decided when the payload's first byte identifies the record, so re-deliveries of other records cannot move a fault between passes. A is seeded sequentially so its log has no inversions to re-deliver, and the wait loop breaks on the referenced-payload oracle: the on-disk hash oracle was satisfied by an orphaned fresh save while the record still referenced a PENDING stub, so the loop exited before the copy had converged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CcbZqR3uQZPkEc6521jjTT Dispatch-Task: hp-copygap-cursor-banking-878
The injector returned the write stream's own accepted value on the first payload chunk, so Readable.pipe never paused and the manual 'drain' released nothing: saves finished as fast as the filesystem allowed, and the sustained-in-flight condition the banking test states was not produced (the pre-rewrite injector had the same gap, since core ignores the header write's return value). The first payload chunk now reports backpressure and the delayed 'drain' releases it. A failed save also settles the held header callback and the chunk callback, and an empty body (no second write) writes the held header on end instead of leaving a header-less file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CcbZqR3uQZPkEc6521jjTT Dispatch-Task: hp-copygap-cursor-banking-878
There was a problem hiding this comment.
Code Review
This pull request refactors the copy-cursor banking integration test and its corresponding blob fault injector to use record-specific failure schedules instead of global save ordinals, ensuring more deterministic test execution. It also updates the seeding process to run sequentially to maintain key order. A critical deadlock issue was identified in the fault injector where returning false on the first write (the size header) can permanently pause the stream without triggering a drain event; returning true is suggested to resolve this.
Contributor
|
Reviewed; no blockers found. |
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.
The copy-gap banking test (
integrationTests/cluster/copyGapCursorBanking.test.mjs) selected its injected blob faults by a global save ordinal, which assumed the base copy's first pass writes exactly one blob per record. Since harper-pro#878 the leader tails its transaction log in append order after the max committed key, so every committed entry appended after that key (a transaction created earlier but committed later) is re-delivered after the walk. At mainae7f0017theCluster Integration Tests 3/6 (Node.js v26.5.0)run's first pass had 42 saves — the 40 records plus re-deliveries of records 3 and 11 — so the 42nd fault hit a re-delivery instead of the resumed walk's second repair, the resumed pass had no fault, and the test failed withfewer than 2 cursor-based copy resumes (1). Faults are now selected by record:<record>:<n>fails that record's n-th fresh save and<record>:repairits first in-place repair, decided when the payload's first byte identifies the record, so re-deliveries of other records cannot move a fault between passes. The leader is seeded sequentially so its log carries no inversions to re-deliver, and the wait loop breaks on the referenced-payload oracle rather than on on-disk hashes, which the CI run satisfied with an orphaned fresh save while the record still referenced a PENDING stub.The investigation also found a convergence hole this PR deliberately does not fix: a copy-mode tail re-delivery replaces a record's good blob file with a fresh save, and if that save fails transiently the record can stay dangling permanently (no identity-tie skip or in-place repair on the copy-mode tail; after finalization the reconnect resumes by numeric key and never replays a lower-keyed entry). It is recorded for a separate issue and its own design round.
For the human reviewer
Framing-Verdict: better-alternative-existsagainst a receiver-window fix — it only covers a failure before the copy finalizes — and named a durable append-order resume position as the real fix, which is a protocol/persistence change. A "no" here means re-opening that scope in this PR.blob-save-startlines after the walk in B's log).BLOB_SLOW_MSnever paused anything and saves finished as fast as the filesystem allowed — the "every save held in flight" premise in the test header was not produced (found by the pre-push review). The first payload chunk now reports backpressure and a delayeddrainreleases it; saves take ≥ 400 ms in the retained logs. The test's cycle shape is unchanged (3/3 runsresumeKeys=[13,29]), so this is a strengthening of the oracle rather than a behavior the test depended on. Open review finding, left unchanged on evidence: the second-round adjudicator argued (75% confidence, code-read only) that a ~51 KB single chunk would exceed the write stream's 16 KB high-water mark and let the stream's owndrainresume the pipe early; the retained logs of three runs show every one of the 40 saves held 399–402 ms fromblob-save-starttoblob-save-done, so the hold is real for this workload. Deferring the payload write itself would make the fixture independent of chunk size; not done here to keep the receipt on the pushed head. A "no" means keeping a knob that does nothing.fd = null), and settles the held header and chunk callbacks with the error; a non-failing save writes the buffered header with the first payload chunk, and an empty body writes it onend. The repair path (<path>.repair) goes through the same patch, which is what lets30:repairtarget the resumed walk. Where to look hardest:real.writein the fixture — that no byte reaches the file before the failure decision, and that the non-failing path writes header then payload in order.Verification
Route: the existing integration test, extended. Local, sequential, each run with a private TMPDIR and retained node logs:
copyGapCursorBanking.test.mjson this branch (the header now states the referenced-bytes oracle; the fault schedule is passed to B's fixture, andmissingReferencedPayloadIdsis the only integrity oracle): 3/3 pass, every run loggingresumeKeys=[13,29] … inPlaceRepairs=2 missingReferencedPayloadIds=[]— the intended three cycles (fault 14 → bank 13 → resumed walk repairs 14, fault on 30's repair → bank 29 → third pass repairs 30).resumeKeys=[9]/[13], one resume, and the re-delivered record left on a PENDING stub), and with the new record-based schedule runs the intended three cycles and converges (2/2). Base itself passes 5/5 on this box because its seed commits in key order, which is why the CI failure was intermittent.copyGapCursorBanking.test.mjsafter the review-round fixes (real backpressure): 3/3 pass, same shape, and the retained logs show each save held ~400 ms (blob-save-start→blob-save-done).npm run test:unit: 1264 passing, 1 pending (30 s).oxlint --deny-warningsandprettier --checkon both changed files: clean. No TypeScript changed.Refs #878
🤖 Generated with Claude Code
https://claude.ai/code/session_01CcbZqR3uQZPkEc6521jjTT
Complexity: easy
Origin — the dispatch brief this PR was written from
harper-pro's
Cluster Integration Tests 3/6 (Node.js v26.5.0)job failed at main HEADae7f00171(2026-09-24T17:31:37Z), the very push that merged harper-pro#878 ("Anchor the replication base copy on a log-order barrier instead of a timestamp"), immediately following a green run at the prior head76fe06a38. The failing test isintegrationTests/cluster/copyGapCursorBanking.test.mjs:185"each gap cycle banks the prefix walked before its first fault, and the copy converges", assertion at :248:AssertionError [ERR_ASSERTION]: fewer than 2 cursor-based copy resumes (1) — cycles are not banking progress. Determine whether harper-pro#878's replacement of the copy resume boundary (timestamp ->findLastCommittedLogKey-anchored log-order barrier) broke this test's cursor-based-resume banking invariant, or whether the test itself needs updating for the new anchor mechanism, and fix whichever is true.Acceptance
Either: (a) a fix restoring cursor-based copy-resume progress banking across gap cycles under the new log-order-anchored resume mechanism, with
copyGapCursorBanking.test.mjsgreen under repeated runs (not just one, given the file's own flake history); or (b) if investigation shows the test's assumptions are stale for the new anchor mechanism rather than a real regression, a corrected test plus a one-line note on why the old assertion no longer applies. Do not weaken the resume anchor's log-order semantics without confirming the invariant harper-pro#878 fixed (a replicated transaction silently never delivered to a joining peer, described in harper-pro#876) stays intact.Dispatch: task
hp-copygap-cursor-banking-878· queued by cron-kriszyp-ci-health · ran by claude/fable/high · worker kzyp-xps-1Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=2; full=1 @ 647f71d
Human-Review-Need: 3 (decisions: select-faults-by-record, sequential-seeding, referenced-payload-oracle) @ 647f71d