test(cluster): poll for seed materialization in copyModeBlobDeadlock before-hook#549
Merged
Conversation
…w assumption copyModeBlobDeadlock's before() hook asserted describe_table.record_count immediately after Promise.all(seedPromises) resolved. A sourcedFrom GET resolves the HTTP response as soon as source.get() returns; the cache-populating write commits in the background afterward, so the fetches completing never guaranteed the records had landed. harper#1641 (blob commit-gating: local writes now wait on blob durability/fsync before committing) widened that pre-existing race enough to make it flake under concurrent 512KB blob writes, most recently on harper-pro#546/#547's core bumps ("source did not materialize blobs: holds 73/80"). Poll describe_table for the actual condition instead, matching the pattern already used in this file's second test and in receiveBacklogMemory.test.mjs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the copyModeBlobDeadlock.test.mjs integration test to poll for the expected record count instead of asserting immediately, ensuring background cache-populating writes and fsyncs have completed. The reviewer suggested enhancing the polling loop with a time-based deadline and transient error handling to prevent test flakiness under heavy concurrent disk I/O.
Contributor
|
Reviewed; no blockers found. |
… seed poll Per Gemini review on #549: align the new seed-materialization poll with the time-based deadline + .catch(() => null) pattern already used a few lines down in this file, instead of an iteration-count loop that would abort the whole hook on a single transient sendOperation failure under the heavy concurrent disk I/O this test drives (80x 512KB blob writes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Patch cherry-pick: mergedCherry-picked onto |
kriszyp
added a commit
that referenced
this pull request
Jul 9, 2026
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
1 participant
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.
Summary
copyModeBlobDeadlock.test.mjs'sbefore()hook seeds 80 file-backed blobs on the source node via concurrent GETs, then assertsdescribe_table.record_count >= 80immediately afterPromise.all(seedPromises)resolves. That assertion races: asourcedFromGET resolves the HTTP response as soon assource.get()returns, while the cache-populating write (and record commit) happens in the background afterward — the fetches completing never actually guaranteed the records had landed.harper#1641(blob commit-gating: local writes now wait on the blob's durability/fsync before the record commits — a correct, intentional fix for an orphan-blob-reference bug) added real fsync-bound latency to that background commit path, widening this pre-existing race enough to flake under 80 concurrent 512KB blob writes on a loaded CI runner. This surfaced asAssertionError: source did not materialize blobs: holds 73/80on #546 and #547 (both core-submodule-bump PRs whose new core commits include #1641), and per the coordinator, also on #512 and #504 after their main-merge conflict resolution — i.e. any PR touching current main/core now hits it.This PR replaces the immediate post-
Promise.allassertion with a poll loop that waits for the actual condition (record_count >= RECORDS, bounded by a 20s timeout), matching the pattern already used later in this same file and inreceiveBacklogMemory.test.mjs.Verification
db8b89a2f(the exact commit chore: bump core for the blob commit-gating and drain-wakeup fixes #546 bumps to, which includes #1641) — reproduced the original race with the old code, confirmed the poll fixes it.before()hook; no other test behavior changed.Where to look
Test-only change, low risk. No core code touched (this is harper-pro's own test-timing assumption, not a core regression) — see diagnosis above for the full trace.
Kris, via Claude Sonnet 5
🤖 Generated with Claude Code