test(runtime-host): give async wait helpers explicit time budgets - #4511
test(runtime-host): give async wait helpers explicit time budgets#4511qianzhu18 wants to merge 1 commit into
Conversation
|
Friendly ping — this has been open ~5 days while the queue moves quickly, so it may have slipped past triage. Two pointers that may help: it's the runtime-host counterpart of #4383 (the same fixed-tick wait-budget class that #4387 fixed for Happy to split it per-file or adjust scope if that makes review easier. |
Astro-Han
left a comment
There was a problem hiding this comment.
A note on current state, not a technical verdict: this PR is currently CONFLICTING and its merge-base sits 183 commits behind origin/main, and it has never had test checks run on it. Any line-level review now would not survive the rebase.
Could the author please rebase onto current main and let CI run? Happy to review once it is rebased and green — there is already one substantive observation queued for then (a time-budget change that also substantially loosens one budget, plus a suggestion to carry with the rebase).
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.
Several runtime-host test helpers poll for asynchronous conditions (turn settlement over UDS, agent-graph wake-run persistence, peer mesh roster propagation, usage and artifact persistence) with a fixed count of macrotask ticks. A tick count is not a time budget: on a loaded CI runner the underlying work can span more ticks than the loop allows, so the helper gives up before the condition is genuinely false. Convert the fixed-tick loops to the shared waitFor primitive from @maka/core/test-only/async-primitives with a 5s wall-clock deadline and a 10ms poll interval, keeping each helper's failure message and diagnostics. In-memory setImmediate helpers are intentionally left alone. Production behavior is unchanged. Fixes apache#4510
a6f6c09 to
4200038
Compare
|
Rebased onto current Notes on the rebase, since two helpers had moved upstream:
|
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the complete six-file diff at 4200038. The wait-helper conversion retains the relevant success/failure assertions; no P0–P2 production defect was reproduced. One P3 lockfile installation-footprint regression is detailed inline.
The helpers use 5-second elapsed-time budgets between polls with 10 ms polling, rather than fixed attempt counts. startTurn still retries only session_busy; peer-count waiting still rejects extra requests; the graph, terminal and usage conditions remain checked. This does not forcibly interrupt an unresolved asynchronous predicate, and it is not evidence of a measured CI flake-rate improvement.
Clean install, repository dependency patches and full build:test passed. The five changed compiled suites ran 80 tests: 79 passed and one existing Bash sandbox result-content assertion failed. Running the actual base version of that test against the same unchanged production implementation reproduced the same failure. I do not attribute it to this PR, but the local run is not all green. Three controlled-time probes passed, covering delayed delivery beyond the old ten yields, timeout/missing/extra delivery, and busy versus non-busy admission behavior.
This head is mergeable, but no hosted checks are reported for it at publication preparation. No native macOS/Windows or full-repository acceptance run is claimed. The prior untested/conflicting-head review is not used as current evidence.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| "libc": [ | ||
| "musl" | ||
| ], | ||
| "license": "MIT", |
There was a problem hiding this comment.
[P3] Preserve the lockfile's libc platform filters
This hunk and 35 similar entries remove the glibc/musl restrictions even though dependency versions are unchanged. It changes installation behavior outside the wait-helper scope: with Node 24.18.1/npm 11.19.0 on the same glibc 2.39 host, a clean install of this lockfile installs six musl native packages that a clean install retaining the base lockfile skips (Astro compiler, Biome, Satteri, Sharp/libvips and Oxc). Those extra packages occupy about 96 MiB locally. Please retain the existing libc metadata rather than bringing this platform-filter churn into a test timing change. This is an unnecessary install-footprint regression, not a claim that the incompatible binaries are executed.
Summary
The same failure class as #4383, applied to the
packages/runtime-hosttest suite. Several helpers poll for an asynchronous condition with a fixed count of macrotask ticks (or immediates). A tick count is not a time budget: on a loaded CI runner the underlying work — turn settlement over real UDS connections, agent-graph wake-run persistence, peer mesh roster propagation over real serve/join networking, usage and artifact persistence — can span more ticks than the loop allows, so the helper gives up before the condition is genuinely false and the failure cannot be distinguished from a real regression.This converts the fixed-tick loops to the shared
waitForprimitive from@maka/core/test-only/async-primitiveswith an explicit 5s wall-clock deadline and a 10ms poll interval:execution-model-composition.test.ts— graph-wake loop,startTurn,waitForTerminal,waitForUsage,waitForCanonicalAttempts,waitForCaptureArtifacts,waitForAutomaticMemoryRequestsToSettleplan-two-client-uds.test.ts—waitForTerminalruntime-policy-coordinator.test.ts— inline turn-settlement loop (now via the file's existingpollForimport)peer-mesh.test.ts— roster propagation loop (previously a 200ms budget)peer-native.test.ts—waitForRequestCount(previously 10 immediates)Each helper keeps its failure message;
waitForCanonicalAttemptsandwaitForAutomaticMemoryRequestsToSettlekeep their detailed diagnostics by building them from the final state when the wait times out. The in-memorysetImmediatehelpers (waitForPending,waitForGoalRun) are intentionally left alone — they poll in-memory facades where a tick budget is effectively unbounded. Production behavior is unchanged; this is test-infrastructure only.Fixes #4510
Verification
npm run --workspace @maka/runtime-host build— clean (no TS errors)node --teston all five converted files: 69/69 pass on macOS arm64 (28 inexecution-model-composition, 11plan-two-client-uds, 4peer-native, 19peer-mesh, 17runtime-policy-coordinator— same totals as the pre-change baseline)biome checkon the five files — cleanNot run: the repository-wide CI matrix (Windows-only suites noted in #4390 do not affect these files; none of the converted files is Windows-gated except
execution-model-composition, which skips on win32 at the describe level).AI use