Skip to content

test(runtime-host): give async wait helpers explicit time budgets - #4511

Open
qianzhu18 wants to merge 1 commit into
apache:mainfrom
qianzhu18:test/runtime-host-wait-budgets
Open

test(runtime-host): give async wait helpers explicit time budgets#4511
qianzhu18 wants to merge 1 commit into
apache:mainfrom
qianzhu18:test/runtime-host-wait-budgets

Conversation

@qianzhu18

Copy link
Copy Markdown

Summary

The same failure class as #4383, applied to the packages/runtime-host test 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 waitFor primitive from @maka/core/test-only/async-primitives with an explicit 5s wall-clock deadline and a 10ms poll interval:

  • execution-model-composition.test.ts — graph-wake loop, startTurn, waitForTerminal, waitForUsage, waitForCanonicalAttempts, waitForCaptureArtifacts, waitForAutomaticMemoryRequestsToSettle
  • plan-two-client-uds.test.tswaitForTerminal
  • runtime-policy-coordinator.test.ts — inline turn-settlement loop (now via the file's existing pollFor import)
  • peer-mesh.test.ts — roster propagation loop (previously a 200ms budget)
  • peer-native.test.tswaitForRequestCount (previously 10 immediates)

Each helper keeps its failure message; waitForCanonicalAttempts and waitForAutomaticMemoryRequestsToSettle keep their detailed diagnostics by building them from the final state when the wait times out. The in-memory setImmediate helpers (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 --test on all five converted files: 69/69 pass on macOS arm64 (28 in execution-model-composition, 11 plan-two-client-uds, 4 peer-native, 19 peer-mesh, 17 runtime-policy-coordinator — same totals as the pre-change baseline)
  • biome check on the five files — clean

Not 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

  • Generative tooling made a substantive contribution

@qianzhu18

Copy link
Copy Markdown
Author

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 packages/runtime), and it closes #4510 with the full inventory listed there. I noticed #4510 was assigned after this PR was opened — no duplicate work is in flight (as noted on the issue).

Happy to split it per-file or adjust scope if that makes review easier.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
@qianzhu18
qianzhu18 force-pushed the test/runtime-host-wait-budgets branch from a6f6c09 to 4200038 Compare September 6, 2026 19:15
@qianzhu18

Copy link
Copy Markdown
Author

Rebased onto current main (was 183 commits behind — apologies, that was the triage blocker). CI should run on this push now.

Notes on the rebase, since two helpers had moved upstream:

  • The graph-wake loop and waitForCanonicalRequests now poll upstream's newer semantics (invocation outcomes / modelCallSummary) under the same waitFor budget instead of tick counts; the capture-artifacts helper that upstream removed was not reintroduced.
  • On the queued observation about a loosened budget: yes — the largest change is the peer-mesh roster-propagation wait (20×10ms ticks → a 5s wall-clock deadline). That loosening is the point rather than a side effect: 200ms was the flake-prone guess, and the deadline only ever fires when mesh propagation genuinely never completes, at the cost of a slower failing test. It matches the 5s precedent from fix(runtime): give sandbox boundary wait a time budget #4387. That said, if you'd rather bound genuine-failure latency tighter (e.g. 2s for peer-mesh), happy to set per-helper budgets accordingly.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread package-lock.json
"libc": [
"musl"
],
"license": "MIT",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(runtime-host): wait helpers poll on fixed tick budgets and can flake under load

3 participants