Drain the sampling debounce long enough for the node CI actually runs - #9332
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.
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/.
![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: 90f34d8ab6
ℹ️ 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".
| timeout-minutes: 33 | ||
| env: | ||
| RETRY_ATTEMPTS: '2' | ||
| RETRY_ATTEMPT_TIMEOUT: '420' |
There was a problem hiding this comment.
When both guarded helper calls exhaust their retries, this 33-minute step can still terminate the final attempt prematurely. The wrapper runs each 420-second attempt with timeout --kill-after=30, and timeout --help confirms that the KILL is sent 30 seconds after the initial timeout signal; including that grace and the 125-second inter-attempt lock wait gives 2 * (2 * (420 + 30) + 125) = 2050s (34m10s), before probe/copy overhead. In the cache-miss + missing-libraries + stalled-command case this step therefore recreates the truncated, unnamed failure it is intended to prevent; the step and enclosing job budgets need to cover the wrapper's full bound.
Useful? React with 👍 / 👎.
| # whether invoked as `playwright`, `python -m playwright` or `python3 -m playwright`. | ||
| # Deliberately NOT anchored on `chromium`/`webkit`: the engine list is irrelevant | ||
| # to the defect, which is that apt runs at all. | ||
| _WITH_DEPS = re.compile(r"playwright\s+install\b[^\n]*--with-deps") |
There was a problem hiding this comment.
If a workflow formats the forbidden command naturally as python -m playwright install \ followed by --with-deps chromium on the next line, this regex does not match because [^ ]* stops at the line break. The new guard would therefore remain green while the apt-bypassing form returns; add a line-wrapped positive case and allow shell whitespace/continuations between install and --with-deps.
Useful? React with 👍 / 👎.
… 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7935798344
ℹ️ 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".
| id: pw | ||
| working-directory: ${{ github.workspace }} | ||
| run: | | ||
| python3 -m pip install 'playwright>=1.45,<2' pytest |
There was a problem hiding this comment.
If PyPI or the runner's network stalls during this install, the new standalone step has no timeout-minutes, so it can consume the enclosing 40-minute job budget and skip all browser and lifecycle tests. Before this change the same pip command ran inside the explicitly bounded Chromium-install step; keep a per-step timeout here (or include it in another bounded step) so moving it to compute the cache key does not reintroduce an unnamed job-level cancellation.
Useful? React with 👍 / 👎.
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.
|
Two follow-ups pushed, both surfaced by the fix rather than caused by it. The drain bound moved 200 -> 600200 turned out to be marginal, not safe: two runs at 200 on the same machine gave 0 and 1 failures, 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. That is the trade. I also tried an adaptive drain that stops once the rows stop changing, and it is wrong here in a way worth recording so nobody tries it again: the rows only change when the write lands, so "rows have stopped changing" is precisely the pending state being waited through. It stops early by construction and scored 4 failures where the fixed bound scored 0. The browser smokes ran for the first time since #9055, and one failedWith Nothing had taken the dialog down; The nav size is now measured before the panel is blocked and compared against itself, which is the invariant that was meant. The same drift had a quieter half: the smoke's Full |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1a58ca755
ℹ️ 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.
settings-dialog.tsx defines the rendered list as const TABS, not SETTINGS_TABS, so this match currently fails and region becomes the entire file—the exact bare scan the comment warns against. If any unrelated object in this component later gains an id: "..." field, the guard invents a settings tab and fails CI by claiming the smoke omitted it. Match the local TABS declaration explicitly rather than falling back to all source text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eba2327295
ℹ️ 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".
| for (let round = 0; round < SETTLE_ROUNDS; round += 1) { | ||
| tick(1000); | ||
| for (let i = 0; i < 6; i += 1) { | ||
| await new Promise((resolve) => setImmediate(resolve)); | ||
| } | ||
| if (until?.()) return; |
There was a problem hiding this comment.
Every settle call in this file passes only the timer callback, so until is always undefined and this loop can never take the advertised early return. On Node 22, the changed compat and simulation suites took 327 seconds versus 62 seconds for the parent versions, with A4 alone spending 196 seconds here; because npm test is a required workflow step, this adds several minutes to every frontend CI run. Pass observable completion predicates at the waiting call sites or otherwise avoid paying the full bound for every permutation.
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.
|
Third commit, and it replaces the round count rather than raising it. The Windows job failed at 600 rounds on a commit that had passed the same job an hour earlier, so 600 was never sufficient, only load-dependently lucky. The reason is a second kind of pending work nothing was waiting on: the store's write ends in Counting the mocked timers alone does not cover it either: with the counter installed and 25 quiet rounds per drain, v22.23.2 still lost 7 orderings. The drain now waits on both, and the backstop throws instead of returning a stale value. That last part is the important one. An under-drain used to look identical to the store losing an edit, which is what sent this investigation into the store twice. Side effect: it is 3.5x faster, since it exits when the work is done. Simulation 106.5s to 29s on the CI runtime. Verified on v22.23.2 directly, not just locally: both suites green twice on each runtime, full frontend suite 4080 passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e9b5ee6ab
ℹ️ 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".
| continue-on-error: true | ||
| with: | ||
| path: ${{ github.workspace }}/.apt-archives | ||
| key: apt-archives-${{ runner.os }}-${{ env.ImageOS }}-${{ env.ImageVersion }}-c-v1 |
There was a problem hiding this comment.
On GitHub-hosted runners, ImageOS and ImageVersion are runner environment variables rather than workflow-defined entries in the env expression context, so these interpolations resolve empty and the effective key remains apt-archives-Linux---c-v1. After ubuntu-latest advances to another image, the immutable cache therefore continues restoring the old image's .deb set instead of creating a fresh entry; when the launch probe needs apt, it must reject/refetch stale packages and loses the mirror-stall protection this cache is intended to provide. Export the runner values to step outputs or $GITHUB_ENV before constructing the key.
Useful? React with 👍 / 👎.
test_source_read_encoding caught four read_text() calls this PR added with no encoding. It is right and they are a real defect: the guard reads settings-dialog.tsx, playwright_settings_tabs.py and studio-frontend-ci.yml, and on a Windows runner Path.read_text() uses the ANSI code page, so any non-ASCII byte in any of them raises UnicodeDecodeError. The whole point of this guard is that a settings page can be added without anyone noticing; a guard that cannot be collected on Windows fails the same way. Repo tests (CPU) was otherwise clean: 1 failed, 8781 passed.
test_the_pwsh_filter_keeps_the_log_clean_and_the_exit_code_intact went red on a hosted ubuntu runner with completely empty stdout and pwsh's own banner: "An error has occurred that was not properly handled. Additional information is shown below. The PowerShell process will exit." The interpreter died; the script never ran. Workflow trigger lint is green on the last 8 main runs and this PR touches no PowerShell, so it is the runner, not the repo. Read as an ordinary failure it says "$LASTEXITCODE did not survive the added pipeline stages, so a failing install.ps1 would leave its step green" -- an accusation against the installer, raised by a run that produced no evidence either way. That is the same shape as the drain in this PR: an environment shortfall wearing the costume of a product regression. _run_pwsh retries only that case, and the distinction is what keeps it honest: a run that reaches the `RC=` line is returned on the first attempt whatever the value, so a genuine regression can never be retried into green. Only a run with no RC= AND the crash banner is retried, because it carries no verdict to preserve. If both attempts crash it fails with a message naming the interpreter, not install.ps1. 17 passed. Exercised _run_pwsh against a script that prints the banner and no RC=: it raises, so the branch is not vacuous.
|
Fully green.
|
…loader (unslothai#9367) unslothai#9332 fixed these suites by waiting on two observables: the mocked timers, which it counts exactly, and the module loader, which it raced by issuing an import of its own each round. The second one works and is still the wrong thing to assert. The loader probe rests on an assumption about node internals that I recorded in the comment and never verified: that the hooks thread serves requests in order over one port, so a reply to a request issued after the store's cannot arrive first. If that ever stops holding it fails as a stale read, which is precisely the failure mode unslothai#9332 existed to abolish -- a wait that comes up short reports "chat A minP: owed 0, shows 0.01" and sends the next reader into the store. The store already tracks the thing worth waiting on. threadSettingsWriteChains holds the live promise chain per thread, and awaitThreadScopedSettingsWrite already awaits one of them by id. What a drain needs is all of the started ones, without knowing which chats the store decided to write, so this adds awaitStartedThreadScopedSettingsWrites. It is deliberately not a flush. A debounce that has not fired yet is left alone, so a caller cannot use it to make a write happen earlier than the store would have; that would let a test pass against a store that never scheduled the write at all. It is bounded at 20 passes rather than looping until the map empties, so a write that keeps rescheduling itself surfaces as a failed assertion instead of a hang. It repeats rather than awaiting one snapshot because a chain that settles can leave a newer one behind it for the same chat. drainMockedTimers now takes that as a caller-supplied `barrier` instead of carrying its own loader probe. The timer counting is unchanged, and so is the exhaustion throw, which is the property that keeps an under-drain from ever again looking like a lost edit. The approach is the one from unslothai#9352, which I opened against the same bug in parallel and is closed in favour of this. unslothai#9332's mechanism landed first and turned Windows green; this replaces the part of it that was a guess about the runtime with a measurement of the subject. Verified ------------------------------------------------------------------------ Both suites twice on v24.14.0 and twice on v22.23.2, the version setup-node 22 resolves to: compat 16 passed, simulation 18 passed, every run. Full frontend suite 4080 passed, 0 failed. Mutation-tested on v22.23.2, which is the only runtime where this is observable: disabling the barrier and leaving the timer counting in place gives 8 failed, 10 passed. So the barrier is carrying the load, not decorating a wait that already worked.
1 participant
Frontend CI has been red on main since #9055, and not intermittently - eight consecutive main runs failed at
Unit tests, every one on nodev22.23.2.Why it looked flaky
I first measured it as 2/6 and 1/8 from a census window that spanned the merge, so it averaged pre-merge green runs together with post-merge red ones. Filtering to runs after #9055 landed, it is 8/8 red.
The cause
The three suites from #9055 wait for a debounced write with a fixed drain - three rounds of
tick(1000)plus sixsetImmediateturns - and then assert. Three rounds is enough on node 24, which is what my dev box happens to have. It is not enough on node 22, whichsetup-node: 22resolves to: the same chain drains far fewer continuations per round, so the write had not landed when the assertion ran.Reproduced by downloading node 22.23.2 and running the suites against both. On the compat suite:
The two suites reported the same defect very differently, which is what made it read as a feature bug:
expected 1.37, actual undefinedchat A temperature: owed 0.6, shows 1.37- because a scenario whose write has not landed looks exactly like one that wrote the wrong valueThe change
There were three copies of the drain: one per suite, plus the shared
thread-sampling-worldhelper 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.
settle()also takes an optionaluntilpredicate: it returns as soon as the caller can see the work it was waiting for, 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.Verification
Only the simulation suite imports the helper, and no fixed three-round drain remains anywhere in
tests/.Why this matters beyond the two suites
Unit testsis step 13 ofFrontend build + bundle sanity, and every later step carries an implicitsuccess(). While it fails, the Chromium install and all seven browser smokes are skipped - the job goes red without ever running the browser coverage it exists to provide. It is also what blocked #9299 from being validated three times.