test(cluster): backport waitForCondition to v5.2 so the #432 cluster suites can load - #817
test(cluster): backport waitForCondition to v5.2 so the #432 cluster suites can load#817kriszyp wants to merge 1 commit into
Conversation
…suites can load Both cluster tests from the harper-pro#432 series came to v5.2 without the helper they import: waitForCondition was added to clusterShared.mjs on main by #787, which was never backported. Both files therefore die at import with SyntaxError: The requested module './clusterShared.mjs' does not provide an export named 'waitForCondition' integrationTests/cluster/relayedOriginResumeGap.test.mjs:52 integrationTests/cluster/blobGapEscalationBudget.test.mjs:32 That kills the Cluster Integration 1/6 shard process and cancels everything queued behind it. It reproduced on all three Node versions in the v5.2.9 release-commit run. The second file is the escalation budget's own regression test, so the feature 5.2.9 ships has no executing cluster coverage on this branch at all. Backports both halves of #787 the suites depend on, byte-identical to main: waitForCondition, and sendOperation's options.signal, which they pass (relayedOriginResumeGap.test.mjs:153, blobGapEscalationBudget.test.mjs:103) and which v5.2's two-argument sendOperation silently dropped — without it the deadline cannot abort an in-flight request, the exact case the signal exists for. Existing two-argument callers pass undefined, which fetch treats as no signal. waitForCatchUp is left in place, unlike on main where #787 removed it. It has no live caller on v5.2 — failoverWriteVisibility.test.mjs:312-323 only documents why it deliberately does not call it — but deleting a helper is a separate decision from unbreaking the suites, and not one to fold into a release branch. Refs #432 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NMujzPGQQcYvRf54aobS3Z
There was a problem hiding this comment.
Code Review
This pull request introduces an options.signal parameter to sendOperation to allow aborting fetch requests, and adds a new helper function waitForCondition to poll a probe function with timeout and abort support. The review feedback highlights a potential issue in waitForCondition where unconditionally aborting the controller in the finally block on the success path could prematurely abort signal-bound resources returned by a successful probe, suggesting a fix to only abort on failure or timeout.
| let lastError; | ||
| try { | ||
| while (!signal.aborted) { | ||
| try { | ||
| const result = await probe(signal); | ||
| if (result) return result; | ||
| } catch (error) { | ||
| if (!signal.aborted) throw error; | ||
| lastError = error; | ||
| } | ||
| if (signal.aborted) break; | ||
| await delay(pollMs, undefined, { signal }).catch((error) => { | ||
| if (!signal.aborted) throw error; | ||
| }); | ||
| } | ||
| } finally { | ||
| // aborts requests the probe left in flight, not just the deadline timer | ||
| controller.abort(); | ||
| clearTimeout(deadline); | ||
| } |
There was a problem hiding this comment.
Aborting the controller unconditionally in the finally block on the success path will abort the signal for the successfully returned resource if it is signal-bound (such as a Response or stream returned by the probe). Since there is at most one probe execution in flight at any time (as they are executed sequentially in the while loop), we only need to abort the controller if the condition was not met (i.e., on timeout or failure).
We can track whether the probe succeeded using a boolean flag and only call controller.abort() if success is false.
let lastError;
let success = false;
try {
while (!signal.aborted) {
try {
const result = await probe(signal);
if (result) {
success = true;
return result;
}
} catch (error) {
if (!signal.aborted) throw error;
lastError = error;
}
if (signal.aborted) break;
await delay(pollMs, undefined, { signal }).catch((error) => {
if (!signal.aborted) throw error;
});
}
} finally {
clearTimeout(deadline);
if (!success) {
controller.abort();
}
}
Both cluster suites from the harper-pro#432 series arrived on
v5.2without the helper they import, so both die at import rather than running:waitForConditionwas added toclusterShared.mjsonmainby #787 ("replace the broken waitForCatchUp oracle with a bounded waitForCondition"), which was never backported; the test files were cherry-picked without it. A failure at module link kills the Cluster Integration 1/6 shard process, so everything queued behind it reportstest did not finish before its parent and was cancelled— that is the acl-connect cascade in the same run. It reproduced on all three Node versions in the v5.2.9 release-commit run (33834420072).The second file matters more than the first:
integrationTests/cluster/blobGapEscalationBudget.test.mjsis the escalation budget's own regression test, so the feature v5.2.9 ships has had no executing cluster coverage on this branch at all.This backports the two halves of #787 the suites depend on, byte-identical to
main:waitForCondition— the bounded polling helper. TheAbortSignalit hands the probe fires at the deadline, sotimeoutMsbounds the whole wait rather than only the gaps between polls.sendOperation'soptions.signal— the suites pass it (relayedOriginResumeGap.test.mjs:153,blobGapEscalationBudget.test.mjs:103) and v5.2's two-argumentsendOperationsilently dropped it. Without it the deadline cannot abort an in-flight request, which is the exact case the signal exists for. Existing two-argument callers passundefined, whichfetchtreats as no signal.For the human reviewer
waitForCatchUpis kept, unlike onmain. #787 deleted it there. It has no live caller on v5.2 —failoverWriteVisibility.test.mjs:312-323only documents why it deliberately does not call it — so it is dead code, and the review flagged that this is the natural moment to decide. I left it: deleting a helper is a separate decision from unbreaking the suites, and not one to fold into a release branch. Say the word and it goes.Nits I declined, all inherited from
main's implementation. The pre-push review (codex + gemini + cursor-composer + harper-domain, adjudicated nit) raised four, each real but each present inmain's copy verbatim: thefinallyaborts on the success path too (so a probe returning a live signal-boundResponsewould hand the caller a dead body — no current probe does);timeoutMsabove2**31-1clamps to 1 ms and fires immediately; adescriptioncallback that throws a nullish value raises aTypeErrorover the timeout it was meant to explain; and the added comments run against the zero-new-comments default. Fixing any of them here would fork the backport frommain. They belong onmainfirst, then ride down — same reasoning as the #2465 resolution.Cancellation is still unverified on this branch.
main'sunitTests/integrationTests/clusterShared.test.mjswas not backported, and the two consuming suites only exercise the happy path. A node that accepts the TCP connection and never answers — the case the helper was written for — is exactly what nothing on v5.2 covers.Verification
node --checkon the helper and both affected suites.relayedOriginResumeGap.test.mjsnow links and reaches a runtime assertion instead of theSyntaxError.prettier --checkclean.distand multiple live nodes.v5.2for the first time (run 33869594203):✔ Relayed-origin resume gap (harper-pro#432)— 91.6s, Cluster Integration 1/6, the shard that was dying at import✔ Blob-gap escalation budget bounds a 503-forever source (#432)— 59.8s, Cluster Integration 3/6Two jobs are still red and neither is reachable from this diff, which only adds an export and an optional parameter:
Integration Tests 2/3(cloneNode.test.mjs— "should clone three more nodes successfully" timing out) andCluster Integration Tests 2/6(aclConnectCrossNode.test.mjs— 317s suite timeout). Both match the pre-existing failures on the v5.2.9 release commit (run 33834420072), where the acl-connect entries were cancellations cascading from this shard crash and are now root failures in their own right. Not addressed here.Refs #432
🤖 Generated with Claude Code
https://claude.ai/code/session_01NMujzPGQQcYvRf54aobS3Z
Review-Coverage: authored=claude; ran=gemini,cursor-composer,codex; adjudicated=domain; declined=cursor-grok; rounds=2 @ 86558cc
Human-Review-Need: 3 (decisions: backport-fidelity-vs-nit-fixes, probe-error-policy, deadline-enforcement-mechanism, backport-scope) @ 86558cc