Skip to content

Pace replication sender yields with a worker time budget - #959

Merged
kriszyp merged 9 commits into
mainfrom
fix/replication-sender-time-budget
Oct 9, 2026
Merged

kriszyp merged 9 commits into
mainfrom
fix/replication-sender-time-budget

Conversation

@kriszyp

@kriszyp kriszyp commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

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

  1. Requirement and scope: the supplied measurements warrant removing per-record scheduler overhead. Accept the reported approximately 5% concurrent-write throughput trade-off for lower total sender CPU. Keeping per-record yields retains that measured overhead. This belongs in Pro replication; no superseding sender-yield change was found in the current open PR list or freshly fetched main. Receive pacing and recordConcurrency stay unchanged. Reverting the two exits restores the old behavior without a migration.
  2. shared-worker-budget: planning returned 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.
  3. per-record-time-accounting: retain a monotonic clock read per eligible record instead of periodic count-based sampling. This accepts clock cost for tighter elapsed-time accounting; consequential clock overhead was not measured here. Changing cadence is localized but weakens the time bound.
  4. preserve-budget-through-io: a peer's drain/blob wait does not reset the shared anchor for other active peers. Accept a possible extra immediate after an I/O wait. Resetting globally would extend other peers' slices and change the explicitly preserved wait paths; this is locally reversible. Re-examined 2026-10-07 after an independent review raised a major on exactly this (a sender resuming from any wait pays one extra yield because the clock only advances on an actual yield, not on each wait's resolution): a --mode plan framing recheck returned Framing-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 coupling shared-worker-budget above already rejects.
  5. worker-type-budget: the shared budget now depends on the executing thread's identity instead of one flat constant — 2 ms on a dedicated replication worker, 0.5 ms on a shared HTTP worker or the main-thread fallback, selected once from workerData.name rather 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 when replication.threads is 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 by sendLoopYield.test.mjs's regression cases, not by a new Linux measurement.
  6. closure-fixture-boundary: use compiled subscription-exit fixtures wired to the real helper instead of extracting the large production closure. Asserted anchors make refactor drift fail loudly; tests prove returns, shared turns and timer progress, not a hard scheduler-latency bound. Replacing them needs broader scaffolding. GitHub Gemini repeats the helper-extraction suggestion; missing anchors assert at module load, and a production extraction would broaden this task. Retained the integration's historical negative-control evidence and DESIGN test reference despite the repeated narration nit because they identify the guard's scope and provenance.
  7. audit-send-scope: all six audit skip branches retain the existing exit. The separate copy-flush pacer stays on its own cadence (copyFlushPacer.due, not this PR's budget) for its seconds-scale watchdog contract; copy-only continue paths 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.
  8. ❓ Your call: three rebases onto main so far have each hit one DESIGN.md numbering conflict, because the note list keeps growing underneath this PR's own sender-fairness note. 2026-10-06 (28 commits ahead): main independently added its own item 24 at this note's position; kept both, renumbered this PR's note 24→25, repointed its one cross-reference (note 2). 2026-10-07 (main advanced further, items 25/26 "replicate: false" and "per-origin resume cursors" landed at the same position): kept all three, renumbered this PR's note 25→27, repointed the same cross-reference again. 2026-10-09 (#1011's origin-closed floor certificates merged, independently claiming item 27 for the same position): kept both, renumbered this PR's note 27→28, repointed the same cross-reference again. This same rebase also hit a real (non-DESIGN.md) conflict for the first time: #1011 restructured the copy loop's withholding/anchoring logic around the exact lines this PR's LOCAL_ONLY-skip comment touches; resolved by keeping main's restructured code and carrying forward only this PR's own wording edit (skips normal record sending and its budgeted yield), verified duplicate-free via range-diff. Each rebase's re-review surfaced nothing new in the production logic beyond items already tracked: (a) a pre-existing major — the skip timer's SEQUENCE_ID_UPDATE can publish ahead of an unflushed non-skipped transaction's frame — predates this PR (same mechanism on main), is not widened by it (budgeted yields fire the timer less often than per-record yields did), and is the same gap item 7 above already discloses; tracked as a dispatch Finding for separate follow-up, not fixed here. (b) a test-oracle-margin concern that budgeted yields could shrink the 70k-row dropped-table skip walk below the 300 ms sequence-timer window, collapsing the cursor-progress assertion in auditReplayYieldExcludedTables.test.mjs to a single sample — refuted every time: the file's own header already records 3 cold reruns with this exact fix passing at >=2 distinct values, and a fresh rerun after each rebase reproduced that (2 distinct values, monotonic, 3/3 tests green). No code change made for either. (c) the anchor-at-own-yield design question — see item 4 above. (d) the test file's header (:22), sampleCursorProgress JSDoc (:136) and this test's own title still say the resume cursor advances "progressively during the run"/"during a skip-run," which is stronger than what the >=2-distinct-values assertion actually proves (progress within the 8s probe window, not necessarily mid-walk); this text is outside this PR's diff (unmodified from the file's original authoring, unlike the inline assertion comment/message this PR already corrected at :371-373) — out of scope for a rebase-maintenance task, same as (a)/(b). No code change made for (a), (b) or (d); item 4 covers (c)'s resolution.

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.

Measurement Per-record yields 2 ms budget patch
Sender CPU, 4 peers, 3 runs each 9.6–10.3 s (user ~6.9 s, sys ~3.1 s) ~3.45 s (user ~2.9 s, sys ~0.57 s)
CPU per entry sent ~8.1 µs ~3.2 µs
Replication rate per peer, 4 peers 19.1–19.3k/s 21.4–21.9k/s
Replication rate, 1 peer 27.5k/s 30.8k/s
Concurrent source write throughput 56–59k/s ~55k/s

Baseline writev accounted 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

  • Build, npm run typecheck, CI's npm run lint:required, and changed-file Prettier check: passed.
  • GitHub CI on 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.
  • Replication unit suite: 1,119 passed, 1 pending. Full npm run test:unit: 1,529 passed, 2 pending after the clock-anchor repair (only fixture-header wording changed afterward).
  • Regression guard: both below-budget assertions failed on freshly built origin/main (0317e247) with a pending promise instead of undefined, 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.
  • Full 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.
  • The existing skipped-audit integration passed without assertion/workload changes: 52 cursor samples (0 → final after 611 ms), 580 pings averaging 3.1 ms / maximum 14 ms. Its negative-control comment now labels the old evidence as historical, and the probe-window comment notes that receiver setup is included. This oracle proves an end-to-end update, not multiple updates during replay; the deterministic skip test proves a timer fires while the run continues. This timing-oracle limitation is recorded in dispatch Findings.
  • Strict npm run lint: fails on unmodified origin/main too, including the unchanged subscriptionRequest warning. The new test passes strict lint; the CI quiet lint gate passes.

Rebase maintenance (2026-10-06)

  • Rebased c5c6a0c7 onto main (28 commits ahead, incl. hdb_nodes alias-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.ts auto-merged clean. Post-rebase range-diff and --remerge-diff confirmed no commit's content silently changed or dropped.
  • Build, typecheck, lint:required, and Prettier on the touched files: passed, on the rebased head and again after every follow-up commit below.
  • Replication units: 1,168 passed, 1 pending (grew from 1,119/1 — unrelated tests landed on 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.
  • Independent re-review: 4 full + 3 delta rounds (codex, Gemini, Cursor, Harper-domain), required because a rebase force-push invalidates delta coverage. Fixed: an unguarded test-teardown dereference the CLI's own self-check flagged (afterEach could throw past a failed beforeEach and mask the real failure), two comment-narration nits, and the stale DESIGN.md note-2 cross-reference. Declined: the pre-existing SEQUENCE_ID_UPDATE-ordering major and the test-oracle-margin concern, both addressed in ❓ item 8 above with evidence rather than a code change.
  • GitHub CI on 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/validate green; review-coverage and companion-check green. No new PR comments or reviews after a full quiet watch cycle.

Rebase maintenance (2026-10-07)

  • Rebased 6fc1ba30 onto main (98698201, core submodule bumped along with it — not this PR's own gitlink, main's). One conflict, DESIGN.md's numbered-note list again (❓ item 8); replicationConnection.ts auto-merged clean. range-diff and --remerge-diff against the pre-rebase tip confirmed no commit's content silently changed or dropped.
  • Build, typecheck, 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.
  • Independent re-review: 9 rounds (6 full, counting a graded-timeout degraded round as non-passing per the CLI's own rule) across codex, Gemini, Cursor Composer and Harper-domain, required because the force-push invalidated delta coverage. Fixed: the stale DESIGN.md note-2 cross-reference (again, after the renumbering above), two comment-narration nits, and two review-verified wording inaccuracies (DESIGN.md's copy-pacer claim, and the integration test's "during the run" overclaim — the same fix already applied to the file header, now applied to the inline assertion comment/message too).
  • A fresh graded round raised a major against the existing, unchanged yieldSendLoop anchor (item 4 above) and triggered the CLI's framing-recheck: REQUIRED gate. Ran --mode plan with the four-axis spanning set; Framing-Verdict: chosen-approach-sound — see item 4 for the resolution. No code change.
  • Review-depth checkpoint at full pass 6: diminishing returns (repeated low-severity nits on pre-existing or already-decided items, no new production-logic finding since the framing recheck cleared) — deferred further iteration and pushed. Remaining declined nits (not fixed, judged out of proportion to a rebase-maintenance task): the dist-slice test fixture's fragility to compiler-output drift (sendLoopYield.test.mjs:8-15, same test-via-dist-slice/do-less-alternative trade-off accepted at original authoring — item 6 above); the test's afterEach reset being coupled to the literal value of SEND_YIELD_INTERVAL (test-only, hypothetical); DESIGN.md note 27 sitting after main's own pre-existing doubled --- divider (verified pre-existing on main, not introduced by this PR); and a disputed comment-narration nit at replicationConnection.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).
  • GitHub CI on 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/validate green; review-coverage and companion-check green. No new PR comments or reviews after a full quiet watch cycle (three ~4-minute REST polls).

Rebase maintenance (2026-10-09, post-#1011)

  • Verified the remote branch head matched the dispatch's observed SHA (f4079fbf) before touching anything, then rebased onto origin/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 keeping main'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): #1011 independently 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) and git log --merges --remerge-diff confirmed no commit's content silently changed, dropped, or reappeared; git diff --name-only confirmed the branch still touches only its original 4 files.
  • Build, typecheck, 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.
  • Independent re-review: 3 rounds (1 full + 1 delta + 1 closing full, codex/Gemini/Cursor/Harper-domain), required because the force-push invalidated delta coverage. Fixed: the DESIGN.md 27→28 renumbering (the only new issue this rebase introduced — everything else below was already open before this generation started). Declined, with evidence (not fixed, judged out of proportion to a rebase-maintenance task, consistent with every prior generation's rulings on the same items): the unmeasured sender-hot-path throughput/latency tradeoff (items 1/3/5 above — already disclosed and accepted at original authoring, re-raised again this round including via a per-record 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 its afterEach +2 hardcode (item 6 above, same do-less-alternative trade-off); DESIGN.md's doubled --- divider and the note's placement after it (pre-existing on main, 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).
  • GitHub CI on 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#1017 merged into main restoring the certified core origin-floor pin (harper main 910a0dbb7, containing harper#3109) and fixing main's deterministic Cluster 6/6 idleOriginFloorResume failure — the only thing that was red on the prior generation's head. Verified origin/fix/replication-sender-time-budget still matched the recorded 7f4de4c0 before 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 to 7f4de4c0 via git diff --quiet) and 39a4498c, the worker-type budget split now in item 5 above.
  • Rebased the reviewed 8-commit chain directly onto origin/main (252bbf32) with git rebase --onto origin/main 158a619c 7f4de4c0 rather than replaying the no-op merge — 158a619c is the exact merge-base of both the old branch tip and the new origin/main, confirmed before rebasing. #1017's own diff (.github/workflows/sync-core.yaml, root/build-tools DESIGN.md, build-tools/core-sync-guard.sh, core, two new build-tools/replication unit test files) touches none of this PR's files, so the rebase and the follow-up cherry-pick of 39a4498c both 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-diff against 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...HEAD confirmed the branch still touches only its original 4 files.
  • Hygiene: the worktree's core submodule checkout had drifted to a newer commit than the branch's own gitlink (bc61f1f8 vs. the recorded 910a0dbb) — same bootstrap artifact noted in the prior generation; git submodule update core restored it (harmless either way: build/test use npm's installed node_modules/harper, not the submodule checkout).
  • Build, typecheck, lint:required (fails only on the pre-existing, unmodified subscriptionRequest warning — 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-pushed with --force-with-lease pinned 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 classified pre-existing/previously-adjudicated by 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-existing SEQUENCE_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 loops await their yield so senders take turns record-by-record within the budget. Fixed: none needed.
  • GitHub CI on 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 move core'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-1

Related 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's getSharedStatus()[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/6 idleOriginFloorResume failure — the trigger for this generation's rebase, no file overlap with this PR), #1019 independent (open, draft; renames restart-site log dirs across 15 integrationTests/cluster/*.test.mjs files, not including auditReplayYieldExcludedTables.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

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

Comment on lines +10 to +17
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*$/, '');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

@kriszyp

kriszyp commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

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.

@kriszyp
kriszyp force-pushed the fix/replication-sender-time-budget branch from c5c6a0c to 7b00a46 Compare October 6, 2026 20:00
@kriszyp
kriszyp force-pushed the fix/replication-sender-time-budget branch from 6fc1ba3 to f4079fb Compare October 7, 2026 20:56
@kriszyp
kriszyp force-pushed the fix/replication-sender-time-budget branch from f4079fb to 8759baf Compare October 9, 2026 05:53
kriszyp and others added 6 commits October 9, 2026 11:59
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>
kriszyp and others added 3 commits October 9, 2026 11:59
- 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>
@kriszyp
kriszyp force-pushed the fix/replication-sender-time-budget branch from 7f4de4c to 2e70121 Compare October 9, 2026 18:05
@kriszyp
kriszyp marked this pull request as ready for review October 9, 2026 21:06
@kriszyp
kriszyp merged commit b9e4b5d into main Oct 9, 2026
45 checks passed
@kriszyp
kriszyp deleted the fix/replication-sender-time-budget branch October 9, 2026 21:06
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