Repository navigation
Pace replication sender yields with a worker time budget - #959
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a shared yield budget mechanism (yieldSendLoop) with a 2 ms interval to optimize event loop yielding during replication send loops, preventing event loop starvation and ensuring sequence-update timers can fire. It updates related documentation and integration tests, and adds a new unit test suite. Feedback on the changes highlights that the new unit test suite relies on fragile string slicing of compiled JavaScript to extract nested closures for testing, and recommends refactoring the target code to make these helpers directly testable.
| const source = readFileSync(new URL('../../dist/replication/replicationConnection.js', import.meta.url), 'utf8'); | ||
| const skipStart = source.indexOf('function skipAuditRecord() {'); | ||
| const skipEnd = source.indexOf('if (!sentNodeIds.has(', skipStart); | ||
| const waitStart = source.indexOf('// wait if there is back-pressure', skipEnd); | ||
| const waitEnd = source.indexOf('const sendQueuedData =', waitStart); | ||
| assert(skipStart >= 0 && skipEnd > skipStart && waitStart > skipEnd && waitEnd > waitStart); | ||
| const skip = source.slice(skipStart, skipEnd); | ||
| const wait = source.slice(waitStart, waitEnd).replace(/};\s*$/, ''); |
There was a problem hiding this comment.
The test relies on reading the compiled JavaScript file (dist/replication/replicationConnection.js) and slicing it using hardcoded string indices (indexOf) to extract nested functions (skipAuditRecord, etc.) for execution in a VM context.
While this is a creative way to test nested closures without running the entire replicateOverWS function, it is extremely fragile. Any minor change to comments, formatting, or compiler/bundler configurations (such as minification or variable mangling) will silently break the test suite.
Recommendation: Consider refactoring replicateOverWS to extract these nested helper functions into top-level, unexported (or package-private) functions within replicationConnection.ts. This would allow them to be imported and tested directly (or via a clean test-only export) without relying on fragile string slicing of compiled assets.
|
Sequencing note: the 2 ms sender budget is only safe on a dedicated replication thread. On shared HTTP workers, a catch-up sender can hold the event loop long enough to starve application requests. This PR should land with or after the replication worker pool (#975, core half HarperFast/harper#3028; design in #435), with the budget gated to pool workers and HTTP workers keeping the per-record yield. 🤖 Claude Opus 5.5 on behalf of Kris. |
c5c6a0c to
7b00a46
Compare
6fc1ba3 to
f4079fb
Compare
f4079fb to
8759baf
Compare
Replace normal and skipped per-record event-loop turns with a 2 ms monotonic budget and one shared pending turn. Preserve drain/blob waits and the copy-flush pacer. Pin under-budget sends, skipped timer progress, peer sharing, and wait precedence. Dispatch-Task: harper-pro-send-loop-per-record-yield Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Flush pending fake turns and restore the real clock anchor after each regression case. Correct the sequence-update fixture code and narrow the design note to audit-send pacing. Preserve the existing integration assertions and mark historical per-record-yield evidence as historical. Dispatch-Task: harper-pro-send-loop-per-record-yield Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Use the real monotonic clock as a lower bound when selecting each fake-clock anchor. Simulated 2,000,000 ms process uptime reproduced two failures before the fix and passes all eight cases after it. Dispatch-Task: harper-pro-send-loop-per-record-yield Co-Authored-By: GPT-5 Codex <noreply@openai.com>
- afterEach now optional-chains clock/performanceNow so a beforeEach failure does not mask itself with a teardown TypeError. - Trim jargon/narration from two comments (replicationConnection.ts:467, sendLoopYield.test.mjs preamble). - Point DESIGN.md note 2 at the shared send budget (note 25) instead of the old per-record yield description, since skipAuditRecord no longer yields unconditionally. Dispatch-Task: pr-maint-51a908966a4cc0b4fc288ea34637531c Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Line 5714's comment repeated the general skip/yield-budget rule already stated once at the skipAuditRecord() definition (line 5838). Dispatch-Task: pr-maint-51a908966a4cc0b4fc288ea34637531c Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- DESIGN.md note 2 pointed at "note 25" for the shared send budget; this rebase's renumbering (24->27) left it stale. - Drop two comment-narration nits flagged across multiple review rounds (replicationConnection.ts, sendLoopYield.test.mjs preamble). - Soften an overclaiming test-header phrase: the oracle proves an update lands within the probe window, not that yields specifically are what let the timer fire mid-walk. Dispatch-Task: pr-maint-51a908966a4cc0b4fc288ea34637531c Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- DESIGN.md note 27 said the copy-flush pacer's yield "remains unconditional"; it's gated on copyFlushPacer.due() (verified at replicationConnection.ts:6681-6682). - Soften the integration test's "during the run" phrasing to match what the >=2-distinct-values oracle actually proves (progress within the probe window, not necessarily mid-walk) -- same fix already applied to the file header, now applied to the inline comment and assertion message too. Dispatch-Task: pr-maint-51a908966a4cc0b4fc288ea34637531c Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Origin-closed floor certification (#1011, now in base) independently claimed item 27 for the same list position this PR's note occupied. Renumber to 28 and repoint the one cross-reference (item 2). Dispatch-Task: pr-maint-51a908966a4cc0b4fc288ea34637531c Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Keep a 2 ms budget on dedicated replication workers and use 0.5 ms on shared HTTP workers and the main-thread fallback. Select once from workerData.name and cover both real worker contexts, pending-yield sharing, and delayed resets. Dispatch-Task: chat-pr-harper-pro-959-kriszyp Co-Authored-By: Codex <noreply@openai.com>
7f4de4c to
2e70121
Compare
Replication catch-up currently pays an event-loop turn for every sent or skipped audit record. Normal sends and audit skips now share a worker-local budget and one pending yield promise, allowing larger socket batches while retaining socket-drain and blob-admission waits. The budget is 2 ms on a dedicated replication thread and 0.5 ms on a shared HTTP worker or the main-thread fallback (item 5 below).
For the human reviewer
recordConcurrencystay unchanged. Reverting the two exits restores the old behavior without a migration.Framing-Verdict: better-alternative-exists. Adopted one shared pending immediate and a reset-on-resume clock instead of independent peer turns, which multiply the worker slice. Look hardest at the shared promise and callback reset. A policy change requires a release and renewed peer-fairness verification.--mode planframing recheck returnedFraming-Verdict: chosen-approach-sound, confirming the anchor-at-own-yield choice — the worst case is one extra macrotask per wait episode, still cheaper than the pre-fix(replication): yield the audit replay loop on not-subscribed table skips #536 baseline of one per record, and stamping the clock from each per-connection wait's resume point would reintroduce the per-connection couplingshared-worker-budgetabove already rejects.workerData.namerather than from whether the dedicated pool (Run replication on a dedicated worker pool (replication.threads) #983) is active, so an HTTP worker still gets the shorter slice even whenreplication.threadsis enabled. This responds to kriszyp's PR sequencing note: the original flat 2 ms budget assumed every sender runs on a dedicated replication thread, but a cluster that leaves the pool disabled sends from shared HTTP workers, where a 2 ms hold competes directly with request handling. Both constants stay fixed, with no runtime setting, same rationale as the original flat value; changing either requires a code release. These measurements are not a benchmark of this exact implementation and predate the split — the 0.5 ms shared-worker path is covered bysendLoopYield.test.mjs's regression cases, not by a new Linux measurement.copyFlushPacer.due, not this PR's budget) for its seconds-scale watchdog contract; copy-onlycontinuepaths stay under that pacer (Bulk-copy send loop wedges with event-loop starvation on macOS; progresses only via watchdog reconnect cycles #656). Budgeting every copy iteration needs separate work. The pre-existing skip timer can publish a cursor past a buffered transaction; publication/framing logic is unchanged, with separate investigation recorded in dispatch Findings.The replication design note records the worker budget and separate copy-flush contract. The sender skip rationale and copy-pacer contract comments now describe budgeted pacing.
Measurements
Provided macOS loopback experiment: one source, N peers,
threads.count=4, 10-record transactions, approximately 500-byte records; profiles cover catch-up after writes stop. All incoming connections landed on one source worker. These are the dispatcher's measurements, not a new Linux benchmark; the measured build SHA was not supplied.Baseline
writevaccounted for 18% and async-context/immediate/microtask work approximately 10%; these mostly disappeared in the patch, leaving audit reads and GC dominant. The four-peer patch became receiver-apply bound. Concurrent writes fell approximately 5% as replication kept up during the write phase, with lower total sender work. This PR additionally coalesces pending yields across subscriptions.Verification
npm run typecheck, CI'snpm run lint:required, and changed-file Prettier check: passed.c5c6a0c7: build/integration matrix passed builds on Node 22/24/26.5 and all six cluster plus three non-cluster shards on Node 24; unit matrix passed on Node 22/24/26. Typecheck, lint, review-coverage and companion checks are green.npm run test:unit: 1,529 passed, 2 pending after the clock-anchor repair (only fixture-header wording changed afterward).origin/main(0317e247) with a pending promise instead ofundefined, then passed with the fix. The eight regression cases cover pacing/shared promises, skipped timer progress and drain/blob/close behavior. The sender exits are structural fixtures of compiled closures using the real helper; integration tests cover actual transport/framing. Simulated 2,000,000 ms process uptime reproduced two test failures in the clock-anchor setup before the clock-anchor repair and passed all eight afterward.npm run test:integration:all: 214 passed, 13 skipped, 0 failures as reported by the runner (29.5 minutes, concurrency 2). Includes skipped-audit replay, copy-mode blob deadlock, backpressure copy watchdog, worker restart and subscription recovery; opt-in stress coverage was disabled.npm run lint: fails on unmodifiedorigin/maintoo, including the unchangedsubscriptionRequestwarning. The new test passes strict lint; the CI quiet lint gate passes.Rebase maintenance (2026-10-06)
c5c6a0c7ontomain(28 commits ahead, incl.hdb_nodesalias-key work, the record-lock/isolated-worker change, and the shared status-buffer rework). One conflict, in DESIGN.md's numbered-note list (see ❓ item 8 above);replicationConnection.tsauto-merged clean. Post-rebaserange-diffand--remerge-diffconfirmed no commit's content silently changed or dropped.lint:required, and Prettier on the touched files: passed, on the rebased head and again after every follow-up commit below.main).sendLoopYield.test.mjs: 8/8 passed.auditReplayYieldExcludedTables.test.mjs(QA-690, dropped-table skip run): 3/3 passed — B's resume cursor showed 2 distinct values, monotonic, no wedge.afterEachcould throw past a failedbeforeEachand mask the real failure), two comment-narration nits, and the stale DESIGN.md note-2 cross-reference. Declined: the pre-existingSEQUENCE_ID_UPDATE-ordering major and the test-oracle-margin concern, both addressed in ❓ item 8 above with evidence rather than a code change.6fc1ba30(current head): build matrix green on Node 22/24/26.5, all six cluster shards and all three non-cluster integration shards; unit matrix green on Node 22/24/26; typecheck/lint/validategreen;review-coverageandcompanion-checkgreen. No new PR comments or reviews after a full quiet watch cycle.Rebase maintenance (2026-10-07)
6fc1ba30ontomain(98698201,coresubmodule bumped along with it — not this PR's own gitlink, main's). One conflict, DESIGN.md's numbered-note list again (❓ item 8);replicationConnection.tsauto-merged clean.range-diffand--remerge-diffagainst the pre-rebase tip confirmed no commit's content silently changed or dropped.lint:required: passed on the rebased head and again after every commit below. Replication units: 1,212 passed, 1 pending.sendLoopYield.test.mjs: 8/8.auditReplayYieldExcludedTables.test.mjs: 3/3, 2 distinct cursor values, monotonic, no wedge.yieldSendLoopanchor (item 4 above) and triggered the CLI'sframing-recheck: REQUIREDgate. Ran--mode planwith the four-axis spanning set;Framing-Verdict: chosen-approach-sound— see item 4 for the resolution. No code change.sendLoopYield.test.mjs:8-15, sametest-via-dist-slice/do-less-alternativetrade-off accepted at original authoring — item 6 above); the test'safterEachreset being coupled to the literal value ofSEND_YIELD_INTERVAL(test-only, hypothetical); DESIGN.md note 27 sitting aftermain's own pre-existing doubled---divider (verified pre-existing onmain, not introduced by this PR); and a disputed comment-narration nit atreplicationConnection.ts:470(domain's own adjudication kept it as a nit but disputed the characterization — the comments state a rationale and a rule, not a restatement of the code).f4079fbf: build/integration matrix green on Node 22/24/26.5, all six cluster shards and all three non-cluster integration shards; unit matrix green on Node 22/24/26; typecheck/lint/validategreen;review-coverageandcompanion-checkgreen. No new PR comments or reviews after a full quiet watch cycle (three ~4-minute REST polls).Rebase maintenance (2026-10-09, post-#1011)
f4079fbf) before touching anything, then rebased ontoorigin/main(158a619c). One real conflict this time (not just DESIGN.md):#1011's origin-closed floor certification independently restructured the copy loop's withholding/anchoring block around the exact lines this PR's LOCAL_ONLY-skip comment touches. Resolved by keepingmain's restructured code verbatim and carrying forward only this PR's own wording edit to that one comment (confirmed via a side-by-side diff of both conflict sides that the only textual difference was the wording, not logic). DESIGN.md's numbered-note list collided again too (❓ item 8):#1011independently claimed item 27 for this PR's note's position; renumbered 27→28, repointed the cross-reference.range-diff(three-ref form against the pre-rebase tip) andgit log --merges --remerge-diffconfirmed no commit's content silently changed, dropped, or reappeared;git diff --name-onlyconfirmed the branch still touches only its original 4 files.lint:required: passed on the rebased head and again after the DESIGN.md renumbering commit. Replication units: 1,232 passed, 1 pending.auditReplayYieldExcludedTables.test.mjs: 3/3, 2 distinct cursor values, monotonic, no wedge.performance.now()framing, same underlying tradeoff); the pre-existing skip-timer cursor-ordering gap (item 7/❓(a) above); the test-oracle margin concern (❓(b) above, re-verified: still 2 distinct values, monotonic, 3/3 green); the dist-slice test fixture's fragility and itsafterEach+2hardcode (item 6 above, samedo-less-alternativetrade-off); DESIGN.md's doubled---divider and the note's placement after it (pre-existing onmain, not introduced here); and the test file's "during the run"/"during a skip-run" wording in its header, a JSDoc comment, and its own title (❓(d) above — newly spotted this round, but outside this PR's diff, same disposition as the other pre-existing items).7f4de4c0(current head): build/typecheck/lint/unit/integration CI triggered by this push; see the live PR checks for current status (not yet quiet-watched as of this writing).Rebase maintenance (2026-10-09, post-#1017)
harper-pro#1017merged intomainrestoring the certified core origin-floor pin (harpermain910a0dbb7, containingharper#3109) and fixingmain's deterministic Cluster 6/6idleOriginFloorResumefailure — the only thing that was red on the prior generation's head. Verifiedorigin/fix/replication-sender-time-budgetstill matched the recorded7f4de4c0before touching anything. The worktree also carried two additional local commits from an unpushed chat-turn authorized by this rebase task: a no-op merge commit (confirmed tree-identical to7f4de4c0viagit diff --quiet) and39a4498c, the worker-type budget split now in item 5 above.origin/main(252bbf32) withgit rebase --onto origin/main 158a619c 7f4de4c0rather than replaying the no-op merge —158a619cis the exact merge-base of both the old branch tip and the neworigin/main, confirmed before rebasing.#1017's own diff (.github/workflows/sync-core.yaml, root/build-toolsDESIGN.md,build-tools/core-sync-guard.sh,core, two newbuild-tools/replicationunit test files) touches none of this PR's files, so the rebase and the follow-up cherry-pick of39a4498cboth applied clean with no manual resolution — no DESIGN.md numbering collision this generation (origin-closed floor certificates stayed at item 27; this PR's note stayed at 28).range-diffagainst the pre-rebase 8-commit chain: all 8=(identical patch). The cherry-picked commit's diff against its new parent matched its original diff exactly (same 3 files, same hunks).git diff --name-only origin/main...HEADconfirmed the branch still touches only its original 4 files.coresubmodule checkout had drifted to a newer commit than the branch's own gitlink (bc61f1f8vs. the recorded910a0dbb) — same bootstrap artifact noted in the prior generation;git submodule update corerestored it (harmless either way: build/test use npm's installednode_modules/harper, not the submodule checkout).lint:required(fails only on the pre-existing, unmodifiedsubscriptionRequestwarning — same as every prior generation): passed. Replication units: 1,236 passed, 1 pending.auditReplayYieldExcludedTables.test.mjs: 3/3, 2 distinct cursor values, monotonic, no wedge.--force-with-leasepinned to the recorded head (lease held, no race). Independent re-review: one full round (codex graded, Gemini, Cursor Composer, Harper-domain adjudication), required because the force-push invalidated delta coverage. Surviving findings: 2 major and 1 minor, all classifiedpre-existing/previously-adjudicatedby the CLI's own convergence trace — the unmeasured sender-hot-path tradeoff (now also covering the 0.5 ms shared-worker leg, item 5 above), the pre-existingSEQUENCE_ID_UPDATE-ordering gap (item 7/❓(a) above), and a pre-existing minor on a retired/closed sender still running its copy or audit tail before any closed check (same family as ❓(a), not introduced by this PR). Declined, consistent with every prior generation's rulings: nothing here is new production-logic risk from this rebase or from the worker-type budget split. Dropped as factually wrong: Gemini's claim that the shared budget starves later senders — refuted by code trace, both send loopsawaittheir yield so senders take turns record-by-record within the budget. Fixed: none needed.2e701211(current head): build (Node 22/24/26.5), units (Node 22/24/26), typecheck, lint,validate,review-coverage,copy-gap runtime regression(×3), YCSB, Socket Security, both non-cluster integration shards that aren't skipped, and all six cluster shards (including 6/6, the shard that was deterministically red before Warn, never refuse, when a core sync drops the committed pointer's changes; restore certified origin floors by pinning core at merged harper#3109 #1017) — all green.companion-check: pass, no companion dependencies (this PR doesn't movecore's pointer). No new PR comments or reviews since the push.Refs #958
Co-Authored-By: GPT-5 Codex noreply@openai.com
Dispatch: task
harper-pro-send-loop-per-record-yield· queued by unknown · ran by codex/gpt-6.1-sol/xhigh · worker kzyp-xps-1Related PRs: #554 independent (copy-range checksum validation, no shared code path), #717 independent (blob-setup fault handling, blob-capacity waits unchanged), #815 independent (stall recovery/detection, not sender pacing), #940 overlaps (adds a new audit-skip branch — must return
skipAuditRecord()to stay under budget), #956 overlaps (same — new skip branch for a dropped table generation), #982 independent (status-buffer backport, different mechanism and release line), #983 independent (dedicated worker-pool placement, composes with a per-worker budget), #987 independent (round-robin subscription placement, same reasoning as #983), #988 independent (isolated-worker backport of already-merged #979, different release line), #984 independent (already merged into this PR's rebase base), #986 independent (already merged into this PR's rebase base), #979 independent (already merged into this PR's rebase base), #942 independent (already merged into this PR's rebase base), #955 independent (already merged into this PR's rebase base), #943 independent (already merged into this PR's rebase base; added DESIGN.md item 25 "replicate: false", one source of the 2026-10-07 renumbering), #998 independent (already merged into this PR's rebase base; added DESIGN.md item 26 "per-origin resume cursors", the other source), #848 independent (already merged into this PR's rebase base), #1011 independent (already merged into this PR's rebase base; added DESIGN.md item 27 "origin-closed floor certificates", the source of the 2026-10-09 renumbering, and independently restructured the copy loop's withholding/anchoring code this rebase's real conflict was resolved against), #1013 independent (open, draft; touches the copy loop'sgetSharedStatus()[SENDING_TIME_POSITION]assignment and a receive-side debug log, both near but not overlapping this PR's own two sender-yield exits), #1017 independent (already merged into this PR's rebase base; restored the certified core origin-floor pin and fixed main's Cluster 6/6idleOriginFloorResumefailure — the trigger for this generation's rebase, no file overlap with this PR), #1019 independent (open, draft; renames restart-site log dirs across 15integrationTests/cluster/*.test.mjsfiles, not includingauditReplayYieldExcludedTables.test.mjs— no file overlap with this PR, though both call the same shared restart helper pattern)Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse; rounds=17; full=10 @ 2e70121
Review-Attention: study ~15m (critical: replicationConnection.ts; decisions: shared-worker-turn, fixed-budget-constants, do-less-alternative) @ 2e70121