Skip to content

Select the copy-gap banking test's blob faults by record so the copy's post-walk tail cannot shift them - #903

Merged
kriszyp merged 2 commits into
mainfrom
fix/copy-tail-redelivery-repair
Sep 25, 2026
Merged

kriszyp merged 2 commits into
mainfrom
fix/copy-tail-redelivery-repair

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

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 main ae7f0017 the Cluster 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 with fewer 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>: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. 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

  1. Requirement. The task asked whether Anchor the replication base copy on a log-order barrier instead of a timestamp #878 broke the banking invariant or the test's assumptions were stale. The cursor banking itself works (bank at 13, resume at 13, then 29); the stale assumption was the injector's ordinal schedule. Fixing the test is warranted on its own: it is the only CI gate for 5.2.2 blob-gap wedge is bounded but banks zero copy progress — the copy cursor never persists during a copy body, and the watchdog's reconnect mints the next cycle's faults #699's banking and it was red on main. Not superseded by in-flight work. The convergence hole the same run exposed is a real data-integrity defect (a node reports Available with a permanently dangling blob reference), and the choice to split it out was made by the task owner: the planning review returned Framing-Verdict: better-alternative-exists against 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.
  2. Sequential seeding removes the inversions instead of tolerating them. The alternative was to keep concurrent seeding and make every oracle tolerant of a variable number of tail re-deliveries; rejected because a re-delivery of a faulted record (14 or 30) would replace its damaged file with a fresh good one and break the in-place-repair oracle, which is the point of the test. Sequential seeding of 40 × 50 KB costs about a second (the seed loop). The cost is coverage: this test no longer exercises a tail re-delivery colliding with a blob fault, and that path has no test until the follow-up issue's fix lands. If a future leader change re-introduces re-deliveries here, the failure signature is the one diagnosed above (extra blob-save-start lines after the walk in B's log).
  3. The injector now holds saves in flight for real. The pre-existing injector returned the header write's backpressure, which core ignores, so BLOB_SLOW_MS never 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 delayed drain releases it; saves take ≥ 400 ms in the retained logs. The test's cycle shape is unchanged (3/3 runs resumeKeys=[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 own drain resume the pipe early; the retained logs of three runs show every one of the 40 saves held 399–402 ms from blob-save-start to blob-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.
  4. The injector holds the 8-byte header until the record is known. A failed save leaves an empty file that the receiver stamps PENDING, the same state as before (the previous injector returned a fake stream with 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 on end. The repair path (<path>.repair) goes through the same patch, which is what lets 30:repair target the resumed walk. Where to look hardest: real.write in 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.mjs on this branch (the header now states the referenced-bytes oracle; the fault schedule is passed to B's fixture, and missingReferencedPayloadIds is the only integrity oracle): 3/3 pass, every run logging resumeKeys=[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).
  • Fails-on-base shape: a scratch variant that forces one log inversion on the leader (a seed whose transaction is created first and commits 4 s later) reproduces the CI failure on base with the old ordinal schedule (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.mjs after 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).
  • Per-record attempt counters reset when the injector is armed. npm run test:unit: 1264 passing, 1 pending (30 s). oxlint --deny-warnings and prettier --check on 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 HEAD ae7f00171 (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 head 76fe06a38. The failing test is integrationTests/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.mjs green 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-1

Review-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

kriszyp and others added 2 commits September 24, 2026 13:07
…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
@kriszyp kriszyp added this to the v5.3 milestone Sep 24, 2026

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

Copy link
Copy Markdown

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

@kriszyp
kriszyp marked this pull request as ready for review September 25, 2026 18:21
@kriszyp
kriszyp requested a review from a team as a code owner September 25, 2026 18:21
@kriszyp
kriszyp merged commit bb484d5 into main Sep 25, 2026
49 checks passed
@kriszyp
kriszyp deleted the fix/copy-tail-redelivery-repair branch September 25, 2026 18:21
@claude

claude Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

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