Stop Frontend CI installing Chromium's system libraries through apt every run - #9299
Stop Frontend CI installing Chromium's system libraries through apt every run#9299danielhanchen wants to merge 14 commits into
Conversation
…very run `playwright install --with-deps chromium` runs its own `apt-get update` inside itself, so it bypassed everything CI has learned about apt: the shared retry helper's 20s transfer cap, APT_ACQUIRE_RETRIES: '0', and the archive cache. The job failed 3 of 8 runs on main. Job 96072994354 (main, 2026-08-19): 9 packages, 21.1 MB, and `fonts-wqy-zenhei [7472 kB]` alone took 5m50s off azure.archive.ubuntu.com. Both 420s attempts died mid-download. That is the same mirror and the same package that took the webkit shards down in #9289. Attempt 2 logged "Need to get 8833 kB/21.1 MB", so apt does resume partials across attempts and still could not finish. Split the way studio-ui-smoke.yml splits it: download the engine, launch it to find out whether the system libraries are actually missing, and run `install-deps` only if they are. ubuntu-latest is a browser-testing image and usually ships them, so the common path now runs no apt at all. The browser and apt-archive cache keys are deliberately identical to the chromium-only shards in studio-ui-smoke.yml (engine token `c`): same image, same Playwright version, same single engine, so the entry is shared rather than duplicated against a budget measured at 99.3% full. The step's authorised worst case doubles with the second helper call, to 2 x (2 x 420s + 125s) = 1930s, so its timeout goes 17m -> 33m and stays under the job's 40m. Both guarded calls are skipped on the common path. Guard: tests/studio/test_playwright_install_avoids_with_deps.py fails the build if `--with-deps` returns to any workflow, and is wired into workflow-trigger-lint, the only job with no paths filter.
![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: e1a3b129da
ℹ️ 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".
| # when the probe says the libraries are already there. This budget is only | ||
| # reachable when the cache misses AND the image is missing libraries AND the | ||
| # mirror is degraded -- which is the run that used to fail outright. | ||
| timeout-minutes: 33 |
There was a problem hiding this comment.
When the browser cache misses, the launch probe requests dependencies, and both helper calls exhaust their retries, this step may consume 33 minutes, but the job still has a 40-minute total timeout. Because npm installation, audits, typechecking, unit tests, and the build all run before this step, any run where those preceding gates take more than seven minutes will be cancelled by the job timeout before this step can report its bounded failure—the exact cancellation mode this change is intended to eliminate. Increase the job timeout or reduce the retry/step budget so the full step allowance fits after the preceding work.
Useful? React with 👍 / 👎.
for more information, see https://pre-commit.ci
Frontend CI has been red on main since #9055, not intermittently: eight consecutive main runs failed at `Unit tests`, every one on node v22.23.2. The three suites from #9055 wait for a debounced write with a fixed drain -- three rounds of tick(1000) plus six setImmediate turns -- and then assert. Three rounds is enough on node 24, which is what a dev box happens to have, and is not enough on node 22, which `setup-node: 22` resolves to. The same chain drains far fewer continuations per round there, so the write had not landed when the assertion ran. Reproduced by downloading node 22.23.2 and running the suites against both. Measured on the compat suite: rounds 3 10 30 60 failing 7 5 1 0 The compat suite reported it as a missing value (expected 1.37, actual undefined). The simulation suite reported it as an ORDERING violation -- "chat A temperature: owed 0.6, shows 1.37" -- because a scenario whose write has not landed looks exactly like one that wrote the wrong value, which is why this read as a feature bug rather than a slow test. There were three copies of the drain: one per suite plus the shared thread-sampling-world helper the simulations run through. Fixing only the two suites left the simulations red, since their scenarios drain through the helper. The bound is 200, generous rather than tuned to the observed 60, and settle() now takes an optional `until` predicate: it returns as soon as the caller can see the work, and throws naming itself if the condition never holds, so the next slow runtime reports "settle gave up" instead of an assertion on a missing value. Verified: node 22.23.2 compat 16/16 and simulation 18/18 (both were failing); node 24 full frontend suite 4060/4060. Only the simulation suite imports the helper, and no fixed three-round drain remains in tests/.
… more At 200 the simulations were 0 failing on one run and 1 on the next on the same machine, and the Windows runner -- slower again -- still had 2 of 120 orderings short. 600 is 0 failing across three consecutive runs, at 107s against 52s. Also records the adaptive version that was tried and is wrong here, so it is not tried again: the rows only change WHEN the write lands, so 'rows have stopped changing' is precisely the pending state being waited through. Quiescence on that observable stops early by construction and scored 4 failures where the fixed bound scored 0.
Unskipping the browser smokes surfaced this immediately: the blocked-panel run
failed with 'blocking the data panel took the dialog down' while its own report
said dialog: True. Nothing had taken the dialog down. The check was
if not state["dialog"] or state["nav"] != 12:
and the keyboard-shortcuts page had made the nav 13. A stale constant, reading
as an error-handling regression.
The nav size is now read before the panel is blocked and compared against
itself, which is the invariant that was meant: blocking a panel must not
collapse the dialog, whatever size the dialog is.
The same drift had a quieter half. The smoke's TABS list still had twelve
entries, so keyboard-shortcuts had no browser coverage at all and the smoke
stayed green without it. It is added here, and
tests/studio/test_settings_smoke_covers_every_tab.py pins both directions
against settings-dialog.tsx so the next page cannot go uncovered silently. It
also checks the workflow's PW_CHUNK_FAIL names a tab that exists -- that value
lives in studio-frontend-ci.yml, not in the smoke, and a rename would leave the
run blocking nothing while still reporting PASS.
Wired into workflow-trigger-lint, the only job with no paths filter, because it
reads a workflow. Mutation-tested both ways: dropping the tab from TABS and
restoring the literal count each turn it red.
|
Merged #9332 into this branch. Not a dependency of the change, but without it the frontend job dies at Will drop back to a plain rebase on main once #9332 lands. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9b5fb6a8d
ℹ️ 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".
| block = re.search(r"SETTINGS_TABS[^=]*=\s*\[(.*?)^\]", text, re.S | re.M) | ||
| region = block.group(1) if block else text |
There was a problem hiding this comment.
The dialog declares const TABS, not SETTINGS_TABS, so this regex never matches and _dialog_tabs() always falls back to scanning the entire file. Consequently, adding any unrelated object with an id: "..." property to settings-dialog.tsx will invent a settings tab and fail the unfiltered workflow guard even though the smoke and dialog still agree; match the actual TABS declaration and fail if that block cannot be found.
Useful? React with 👍 / 👎.
| const DRAIN_ROUNDS = 600; | ||
|
|
||
| async function drain(tick: (ms: number) => void): Promise<void> { | ||
| for (let round = 0; round < 3; round += 1) { | ||
| for (let round = 0; round < DRAIN_ROUNDS; round += 1) { |
There was a problem hiding this comment.
On the Node 22 runtime used by this workflow, running only the two affected test files took 297 seconds, with A4 alone taking 177 seconds, because each of its 840 orderings now executes 3,600 setImmediate continuations regardless of when the pending write completes. This turns a unit-test synchronization fix into roughly five minutes of CPU-bound CI work and materially reduces the frontend job's remaining timeout budget; wait for the specific pending write or otherwise stop draining once it completes rather than applying the full 600-round bound to every scenario.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31e16e7be8
ℹ️ 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".
|
|
||
| # `playwright install --with-deps`, however the flag is spelled or ordered, and | ||
| # whether invoked as `playwright`, `python -m playwright` or `python3 -m playwright`. | ||
| # Deliberately NOT anchored on `chromium`/`webkit`: the engine list is irrelevant |
There was a problem hiding this comment.
This guard misses the common shell form python3 -m playwright install chromium \ --with-deps because [^ ]* cannot cross the line continuation. A workflow can therefore reintroduce the apt-triggering flag while this newly added unfiltered guard remains green; normalize backslash-newline continuations before matching or allow them in the expression, and add that form to the self-test.
Useful? React with 👍 / 👎.
…gives up
The fixed round count in the previous commit was still a guess, and the Windows
job proved it: the SAME commit at 600 rounds passed one run and failed the next
with "A1: 2 violation(s) across 120 orderings", reporting stale reads as lost
edits. Raising the number again was not the answer.
The pending work has a second half nothing was waiting on. The store's
thread-scoped write ends in `await import("../utils/chat-history-storage")`
(chat-runtime-store.ts:1326 and :1754), and these suites register() a resolver
hook, which routes that import through the hooks thread. Three repeat imports of
an already-loaded module:
v24.14.0 no hook 1, 1, 1 turns hook registered 1, 1, 1
v22.23.2 no hook 1, 1, 1 turns hook registered 6, 3, 35
That is the whole green-locally / red-on-CI split, and it is why a loaded Windows
runner fails what the same commit passed an hour earlier: the pending work is a
message to another thread, so its cost is scheduling latency, not instructions.
No round count is correct for that.
Counting the mocked timers alone does not cover it either, which is worth
recording since it is the obvious next idea. With the counter installed and 25
consecutive quiet rounds per drain, 150 macrotask turns of nothing, v22.23.2
still lost 7 orderings across A1 and A3, every one a write that had not landed.
So drain on both observables. tests/helpers/mock-timer-drain.ts wraps the MOCKED
setTimeout with a counter, giving an exact count of timers scheduled and not yet
fired or cleared, and each round also issues its own import and waits for it, so
the wait scales with the loader instead of guessing at it. The drain returns when
no timer is outstanding and three consecutive rounds neither scheduled nor fired
one. With the probe, three quiet rounds is green on v22 and v24 alike.
The generous bound stays as a BACKSTOP that THROWS and names what was still
outstanding, which is the change that matters most here. An under-drain used to
be indistinguishable from the store losing an edit, so it sent the investigation
into the store for two rounds. Now it says so:
settle: drain exhausted after 2 rounds, with no timer pending but work still
scheduling or firing within the last 3 rounds. Nothing read after this point is
trustworthy: a queued write has not landed, so the store still shows the
PREVIOUS value, which reads as a wrong value rather than a missing one. Fix the
work or raise the backstop; do not read this as the store losing an edit.
It is also much faster, because it stops when the work is done rather than after
600 rounds regardless. A typical drain now takes 4 rounds; instrumented over 840
drains in A1 the maximum was 4.
simulation v24 101.7s -> 30s v22.23.2 106.5s -> 29s
compat v24 0.54s -> 0.46s v22.23.2 0.61s -> 0.52s
The three copies of the drain shape, one in the world helper and one in each test
file, are now one helper. The dead end already recorded is kept next to the new
measurements: quiescence on the store ROWS is still wrong, because the rows only
change WHEN the write lands.
Verified: both suites green twice each on v24.14.0 and on v22.23.2, the version
setup-node 22 resolves to. Full frontend suite 4080 passed, 0 failed. The
exhaustion throw was confirmed by forcing the backstop to 2, which produces the
message above and no wrong-value violation. A deliberately broken
sanitizeThreadScopedSettings still produces a real ordering violation on both
runtimes, so the drain is not exiting early. Test files only.
|
Closing: this landed on main already, inside #9332, and I should have noticed sooner.
It was not merged unverified. #9299's own run exercised the whole path: The browser cache hit on the key deliberately shared with studio-ui-smoke.yml's chromium shards, so the engine download was skipped; the probe found the libraries present, so the apt transaction never ran. Two seconds, against a 17 minute cap that was failing 38% of the time, and all seven browser smokes passed afterwards. #9332's own The cost of the mistake is that the commit message on main describes only the drain fix. Noting it here so the history is searchable. One defect left for a follow-up, deliberately not fixed here. The apt archive cache key resolves to |
1 participant
playwright install --with-deps chromiumruns its ownapt-get updateinside itself, sostudio-frontend-ci.ymlbypassed everything the rest of CI has learned about apt: the shared retry helper's 20s transfer cap,APT_ACQUIRE_RETRIES: '0', and the archive cache from #9289.Frontend build + bundle sanityfailed 3 of 8 runs on main, 2 of them at this step.What the log says
Job 96072994354, main, 2026-08-19:
Same mirror and the same package that took the webkit shards down in #9289. Worth noting attempt 2 resumed rather than restarting, so apt does carry partials across attempts and still could not finish.
The change
Split it the way
studio-ui-smoke.ymlalready splits it: download the engine, launch it to find out whether the system libraries are actually missing, and runinstall-depsonly if they are.ubuntu-latestis a browser-testing image and usually ships them, so the common path runs no apt at all.Cache keys are deliberately identical to the chromium-only shards in
studio-ui-smoke.yml(engine tokenc) - same image, same Playwright version, same single engine - so the entry is shared rather than duplicated against a budget measured at 99.3% full.The step's authorised worst case doubles with the second helper call, to
2 x (2 x 420s + 125s) = 1930s, so its timeout goes 17m -> 33m and stays under the job's 40m. Both calls are guarded, so the common path spends none of it.Guard
tests/studio/test_playwright_install_avoids_with_deps.pyfails the build if--with-depsreturns to any workflow. Mutation-tested: reintroducing the flag turns it red. It is wired intoworkflow-trigger-lint, the only job with no paths filter, because a PR that edits only workflow files is exactly the change it exists to reject.Two existing guards caught real defects in this change while it was being written, and both are fixed here rather than relaxed:
test_the_retry_budget_fits_inside_the_step_timeoutrejected the 17m step cap once there were two helper calls (1930s > 1020s).test_a_cache_save_of_downloaded_artifacts_waits_for_the_download_to_succeedrejected both new saves for running underalways(), which could have stored a half-downloaded engine under an immutable key and served it to every later run.Not addressed here
Frontend unit tests (Windows)fails 2/6 on main at itsUnit testsstep. That is a genuine test failure, unrelated to apt, and is left for a separate change.