ci(viewer): run each shard serially, so a timing test does not race three others - #3061
Conversation
…hree others `useSandbox.runSupersession.test.tsx` failed on THREE unrelated PRs within an hour (#3025, #3027, #3044), always on `Viewer tests (shard 1)`, always with a gate-timing assertion counting 0 or 2 where it wants 1. None of those diffs can reach that test: two touch only packages/extensions and packages/create. It passes locally 3 of 3 at both concurrency settings, and #3038's shard 1 on the same base ran the identical suite green. This is my change's consequence, so I am undoing the part that caused it rather than asking three authors to re-run. --test-concurrency=4 came from the FIRST attempt at this lane, which I measured and showed did not help CI at all. I carried it forward once sharding worked, on the grounds that a real 2x locally was harmless. It is not harmless: it makes a timing-sensitive test race three neighbours under CI load, and its speedup is now redundant, because the parallelism comes from four concurrent shard JOBS. Measured cost of going back to 1, rather than assumed: 70s versus 19s per shard locally, so roughly 8 minutes versus 2 on CI, against a 25 minute cap. That budget absorbs it without argument. Not claiming proof. I have a mechanism and a correlation, not a reproduction -- it passes locally either way, and I have no clean pre-sharding CI baseline because the lane was broken. What I do have is a variable I introduced whose benefit is now redundant, so removing it costs a well-affordable six minutes and eliminates a candidate. If the flake survives this, it was never concurrency and #3060 is where it goes.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_759b659c-82ee-4a21-b46b-4c9f85e9b45e) |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 minutes Limit details: You’ve used the included review currently available. Your 102 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/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: 190e2f25c8
ℹ️ 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".
| "typecheck": "tsc --noEmit", | ||
| "preview": "vite preview", | ||
| "test": "tsx --import ./src/test/vite-module-hooks.mjs --test --test-timeout=120000 --test-concurrency=4 $(find src -type f \\( -name '*.test.ts' -o -name '*.test.tsx' \\) | sort | awk -v s=\"${TEST_SHARD:-0}\" -v n=\"${TEST_SHARDS:-1}\" 'NR % n == s')", | ||
| "test": "tsx --import ./src/test/vite-module-hooks.mjs --test --test-timeout=120000 --test-concurrency=1 $(find src -type f \\( -name '*.test.ts' -o -name '*.test.tsx' \\) | sort | awk -v s=\"${TEST_SHARD:-0}\" -v n=\"${TEST_SHARDS:-1}\" 'NR % n == s')", |
There was a problem hiding this comment.
When the canonical root pnpm test runs with TEST_SHARDS unset, this hardcoded value serializes all 521 viewer test files, not merely each CI shard. The four-way parallelism described in test.yml exists only in CI, while the preceding change measured the unsharded suite at 142s with concurrency 4 versus 295s with concurrency 1, so every developer/full Turbo run now incurs this regression. Keep the package default parallel and pass concurrency 1 specifically from the viewer-tests workflow.
AGENTS.md reference: AGENTS.md:L12-L13
Useful? React with 👍 / 👎.
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
|
Result: all four shards green, including shard 1. And my cost estimate in the description above was wrong, in the good direction. I predicted roughly 8 minutes per shard, extrapolating from 70s locally against 19s at concurrency 4. Actual is 2:43 to 4:03, against 1:50 to 2:29 before. The CI cost is dominated by fixed setup rather than test execution, so the local ratio did not transfer. That is the same mistake as my first attempt at this lane, where a real 2x locally bought exactly nothing on CI. A local ratio is a hypothesis about CI, not a measurement of it. It happened to land in my favour this time, which does not make the reasoning better. What this does and does not establish. One green run cannot prove a flake is gone; a flaky test passes by luck sometimes. What it adds to the five failures under concurrency 4 is that the mechanism now has a matching observation on both sides, and the fix costs about a minute a shard rather than the six I was prepared to pay. If it recurs after this lands, concurrency was not the cause and #3060 is where it continues — specifically the more interesting hypothesis there, that a QuickJS runtime aborting during teardown in the preceding file perturbs the next one, in which case the fix belongs in the teardown rather than the assertion. |
…#3066) `useSandbox.runSupersession.test.tsx` failed on three unrelated PRs with `0 !== 1, then 2 !== 1` on "the reproducer must be parked on its host gate", always right after a `QuickJS aborted while freeing a runtime (#1922)` cascade. #3061 dropped the shard's `--test-concurrency` from 4 to 1, which mitigated it. This is the cause. The fixture slept a flat 50ms and then asserted that a run had reached its host gate. That is a guess about how long `execute()` needs to import `@ifc-lite/sandbox` and stand up a QuickJS runtime, and it is wrong exactly where this file makes it hardest: the #1922 tests at the bottom abort a real WASM module, an aborted module is RETIRED, and the next `execute()` therefore pays a full fresh `newQuickJSWASMModule()` rather than reusing the cached one. So the maintainer's reading of the log was right in substance and off by one boundary — a teardown cascade does perturb what follows it, but the boundary is between TESTS in this file, not between files. Instrumenting the six wait sites on a contended machine: the one site that runs after two module aborts takes 46-72ms, every other site stays under ~20ms. The 50ms sleep straddled the cost it was supposed to cover. When it lost, the damage did not stop at one test. The assertion threw with the run still un-awaited, that orphaned run parked a moment later into the NEXT test's freshly emptied `gates`, and that test counted two — which is precisely the reported `0 !== 1, then 2 !== 1` pair, and why it reads as a flake: the trigger is machine load, not the diff under test. Five further sites had the same guess in a different disguise. They started a second run, awaited it, and treated the newer run settling as proof that the older one had already REACHED its gate. Under load it has not. Every site now waits for the gate to be registered rather than predicting when it will be. That removes the guess instead of enlarging it: no assertion is weakened, every `gates.length` count is unchanged, and the wait's 10s bound is a hang bound, not a timing assumption — it is never approached on the passing path. At the five settle-order sites the wait deliberately comes AFTER the settle-order assertion, which is the load-bearing one; it cannot mask a violation of that, because a run parked on a gate cannot settle until this file opens the gate. Reproduced and measured, `--test-concurrency=1` throughout, contention supplied by background CPU load rather than by raising concurrency: Node 22.13.1, unloaded, before: 30/30 pass — it does NOT fail alone Node 22.23.2, unloaded, after: 30/30 pass Node 22.13.1, loaded, before: 19/30 pass, then 17/20 on a re-run Node 22.13.1, loaded, after: 30/30 pass, and 30/30 at 2x the load Node 22.23.2, loaded, before: 17/20 pass, identical signature Node 22.23.2, loaded, after: 30/30 pass Both Node versions fail before and pass after, so this is not the 22.15.0 `module.registerHooks` loader change: it is load-dependent, not version-dependent. Nothing here adds a delay to make a race less likely; the sleep that WAS such a delay is what gets removed. The `--test-concurrency=1` comment in test.yml is corrected to say that the setting mitigated #3060 rather than fixing it, since a confident wrong comment is worse than none. The setting itself is left alone.
1 participant
Undoing the part of my own CI change that is causing flakes on other people's PRs.
The evidence
useSandbox.runSupersession.test.tsxfailed on three unrelated PRs within an hour — #3025, #3027, #3044 — always onViewer tests (shard 1), always on a gate-timing assertion counting 0 or 2 where it wants 1.None of those diffs can reach that test. Two touch only
packages/extensionsandpackages/create, which never import it. On the same base, #3038's shard 1 ran the identical suite green. Locally shard 1 passes 3 of 3, at both concurrency settings.So: same base, same suite, some red some green, no path from any of the diffs.
Why this is mine to undo
--test-concurrency=4came from the first attempt at this lane — the one I measured and showed did not help CI at all. When sharding finally worked I carried it forward on the grounds that a real 2x locally was harmless.It is not harmless. It makes a timing-sensitive test race three neighbours under CI load, and its speedup is now redundant: the parallelism comes from four concurrent shard jobs, not from concurrency inside one.
The cost, measured rather than assumed
Both against a 25 minute cap. 1386 tests pass either way.
Eight minutes per shard, four shards in parallel, is a budget that absorbs this without argument.
What I am not claiming
Not proof. I have a mechanism and a correlation, not a reproduction: it passes locally either way, and I have no clean pre-sharding CI baseline because the lane was broken all day.
What I do have is a variable I introduced whose benefit is now redundant. Removing it costs six well-affordable minutes and eliminates a candidate. If the flake survives this, it was never concurrency, and #3060 is where that investigation continues — including the more interesting possibility there, that a QuickJS runtime aborting during teardown in the preceding file perturbs the next one, in which case the fix is in the teardown and not in the assertion.