Uh oh!
There was an error while loading. Please reload this page.
test(runtime-host): bound test wait helpers by wall-clock deadlines - #4566
test(runtime-host): bound test wait helpers by wall-clock deadlines#4566ggbdpq wants to merge 1 commit into
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
The predicates are intact, which is the thing that would have sunk this. I compared all eleven converted sites against their pre-images and every waited condition is the same logic, including the memory-settle loop's stablePolls >= 5 and the peer-native exact-count check that follows its wait. No "wait for three, now wait for at least one" softening anywhere. Attaching to the shared waitFor from @maka/core/test-only rather than inventing a helper is the right seam, and #4387 is direct precedent for the same class in a sibling package.
Why this PR?
Worth asking, since the change is preventive. #4510 argues the mechanism soundly, a tick count is not a time budget, so a loaded runner makes a helper give up while the condition is still becoming true, and that failure is indistinguishable from a real regression. That reasoning holds up on reading. What is missing is one witnessed instance: no CI link, no flake log, no reproduction. The one behavioral observation offered, that execution-model-composition previously stopped producing output mid-run on a Windows machine and now completes, is suggestive but undiagnosed.
I would not close this over that. The mechanism is real and the precedent is direct. But if anyone has a CI run showing the failure, it belongs in the PR, because right now the case rests entirely on argument.
The sharper problem is that in its current form the change makes its own headline case worse at the one site it most needs to help.
[P2] The largest loop's budget is cut, in exactly the direction this PR exists to fix
execution-model-composition.test.ts, graph-wake. Before: for (let attempt = 0; attempt < 400; …) with a trailing setTimeout(resolve, 10) and two store reads per iteration. That is a 4s sleep floor plus 800 SQLite round-trips, so well over 5s of real budget on an idle machine and more under load. After: { timeoutMs: 5_000, pollMs: 10 }, a hard 5s wall clock that those 800 reads now come out of.
And the two budgets do not combine the way one might hope. waitFor's guard is attempt < limit && Date.now() < deadline, so passing both stops at whichever expires first.
This is the only one of eleven where the effective budget shrinks, and it is the loop #4510 lists first. { timeoutMs: 30_000, pollMs: 10 } here would dominate the old floor.
[P2] The richest diagnostic in the suite became unreachable
That same waitFor is called with no message, so expiry throws waitFor's default 'condition was not met'. The assert.ok(finish, JSON.stringify({ updateCount, lastUpdate, runs: …, requests: providerRequestTrace(…) })) that followed the old loop now only runs on success.
The PR body says "two helpers with rich diagnostic payloads re-throw those payloads from a catch so the failure text stays identical to before." There are three such sites. waitForCanonicalAttempts and waitForAutomaticMemoryRequestsToSettle got the try/catch; this one, which carries schedule-update counts, per-run status, wake-attempt ids and the full provider trace, did not.
Four other sites also drop to the default message, so 2 !== 3 and 'Pending' !== 'Online' become condition was not met. Smaller losses, but the same shape, and passing the trailing assertion's subject as message fixes each in one line.
Two smaller notes
peer-native's waitForRequestCount goes from ten setImmediate turns to a 5s wall clock. That is the one site where the loosening could hide a genuinely slow path. The trailing exact-count assert keeps the correctness obligation, and #4510 reasons explicitly that this helper crosses a real native transport, so I think it is right; just flagging it as the one deliberate latency relaxation in the set.
startTurn ends with return turn as TurnSnapshot while waitForUsage, for the identical shape, adds a post-waitFor undefined check whose error string duplicates the message already passed to waitFor. One is unsafely optimistic, its twin is dead code. Either is fine, both is odd.
Next step
The two P2s are in one file and both cheap, and I would want them before merge: the first because the PR currently regresses its own strongest case, the second because the body claims a property that does not hold at the most valuable site.
No production surface, so nothing to accept manually. CI is green on 61db5092; a fix push moves the head and this review is pinned to that one.
Evidence boundary: all read at the pinned head, no suite run, so every timing claim is arithmetic on the source (sleep floors and iteration counts) rather than measurement. The P2 budget conclusion depends on loaded per-iteration cost exceeding about 12.5ms, which is inferred from two SQLite round-trips plus a drifting 10ms timer, not measured. I could not reproduce the PR's Windows verification table on macOS.
AI-assisted review: drafted with Maka.
8283b21 to
524e19cCompareSeveral runtime-host test helpers polled for async conditions with a fixed count of ticks (or fixed ticks x ms) instead of a wall-clock budget. Under a loaded runner the underlying work can span more ticks than the loop allows, so the helper gave up early and the test failed in a way indistinguishable from a real regression (apache#4510, same class as Convert the listed loops to the shared waitFor primitive from `@maka/core/test-only/async-primitives` with an explicit 5s wall-clock deadline and a small poll interval, keeping each helper's failure message and assertion shape: - execution-model-composition.test.ts: the graph-wake loop (400 ticks), startTurn (200), waitForTerminal (200), waitForUsage (100), waitForCanonicalAttempts (100), waitForCaptureArtifacts (100), and the memory-settlement stability loop (100 x 10ms) - plan-two-client-uds.test.ts: waitForTerminal over a real UDS connection - runtime-policy-coordinator.test.ts: the inline turn-settlement loop - peer-mesh.test.ts: the roster propagation loop (20 x 10ms) - peer-native.test.ts: waitForRequestCount (10 immediates) Production behavior is unchanged; this is test infrastructure only. Side observation: the pre-existing EBUSY unlink in the DeepSeek auxiliary-calls cleanup and the peer-native ESM dynamic-import scheme error both reproduce identically on unpatched main on Windows and are left untouched. Fixesapache#4510 Generated-by: GLM-5.3-Flash (ZCode)
524e19c to
f9b8b19Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head f9b8b19e26fafb86ce364c23f43ccdfd2a27e4db (OPEN). Technical GO — no P0–P2. Test-infrastructure-only change: test wait helpers get wall-clock deadlines.
What was checked
All 11 conversions before/after plus the waitFor implementation, the PR's validation table, and the prior review thread. The deadline bounds real waiting instead of masking regressions — bounded waits fail loudly on timeout rather than passing vacuously.
What I could not judge
Real timing behavior under a loaded runner (no CI failure recordings available, and the author's evidence is Windows-machine behavior observation only).
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @DSv4F-Qronos-AstroHan 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移。当前 head 是 f9b8b19,未关闭。测试基建改动,限时有界,技术上无阻断问题。
Summary
packages/runtime-hosttest helpers polled for asynchronous conditions with a fixed count of ticks (or fixed ticks x ms) instead of a wall-clock budget. Under a loaded CI runner the underlying work (turn settlement over UDS, graph-wake persistence, peer roster propagation, provider request capture) can span more ticks than the loop allows, so the helper gave up early and the failure was indistinguishable from a real regression (test(runtime-host): wait helpers poll on fixed tick budgets and can flake under load #4510, same class as test(runtime): sandbox boundary request wait has no time budget and flakes on CI #4383/fix(runtime): give sandbox boundary wait a time budget #4387).waitForprimitive from@maka/core/test-only/async-primitiveswith an explicit 5s wall-clock deadline and a small poll interval, keeping each helper's failure message and assertion shape. Two helpers with rich diagnostic payloads re-throw those payloads from a catch so the failure text stays identical to before. Production code is untouched - test infrastructure only.Verification
node --test dist/__tests__/runtime-policy-coordinator.test.jsnode --test dist/__tests__/plan-two-client-uds.test.jsnode --test dist/__tests__/peer-mesh.test.jsnpm run format:checknpm run check:asf-headersexecution-model-composition: 1 fail both ways (EBUSY unlink ofruntime.sqlitein the DeepSeek auxiliary-calls cleanup, reproduces on unpatched main);peer-native: 2 fails both ways (ERR_UNSUPPORTED_ESM_URL_SCHEMEon Windows dynamic import). Two converted suites that used to rely on the old budgets now complete normally on this machineexecution-model-composition.test.jspreviously stopped producing output mid-run on this Windows machine; after conversion the suite runs to completion and reports a full summary. The residual post-summary non-exit of the node process is present with and without the change and is out of scope hereAI use
Analysis, conversion, and verification were produced with GLM-5.3-Flash (ZCode) under the contributor's direction; the contributor reviewed and is the human contributor of record.
Checklist