443 commits

Author	SHA1	Message	Date
Daniel Han	2043c734c4	Take two ubuntu jobs off their own runners: absorb one, delete the other (#9360) ... Over 400 completed main push runs, 25 of 96 job types execute in under 120s: 1116s of work spread across 25 runners, each queuing for about three hours. Two of them are dealt with here. Security audit :: pytest tests/security 72s exec, 11096s queue Unsloth export capability :: capability (ubuntu-latest) 67s exec, 10642s queue Different problems, so different treatments. The security suite MOVED, onto the Workflow trigger lint runner ------------------------------------------------------------------------ Same argument that put the lockfile and load-orchestrator lanes into Lint CI in #9176: work with a narrow trigger, moved into a job that was going to occupy a runner on this commit anyway, can only reduce the slots a commit takes. Here the trigger widens too, since this host has no paths filter and security-audit.yml's pull_request does, so the suite now runs on every pull request rather than on the ones that touch its paths. This host and NOT Lint CI, where the other lanes went, and that is the whole decision. Lint CI installs shellcheck from apt, so its harden-runner has to permit escalation and an apt mirror; a security gate moved there would run under a policy weaker than the one it has today. Workflow trigger lint's harden-runner block is byte-for-byte identical to the one the job carried in security-audit.yml (block, disable-sudo, the same six endpoints), so nothing about its isolation changes. harden-runner binds per runner, not per step, which is what makes that the deciding constraint rather than a detail. Folded into the existing pytest invocation rather than added as a step of its own. I wrote it as a separate step first, so a security regression would not be reported as a workflow-guard failure, and test_the_guards_run_in_one_pytest_invocation rejected it: one step per module costs about 15s of interpreter and conftest startup, measured in this repo at 53.9s as one invocation against 300.8s as one each. The guard is right and the attribution preference is not worth 15s. pytest and PyYAML are now pinned here to the versions security-audit.yml pinned them to. That suite runs scripts/lint_workflow_triggers.py as a SUBPROCESS and asserts on its exit semantics, so a pytest or PyYAML that resolves differently changes what it is asserting against. The capability ubuntu leg was DELETED, because it was already duplicated ------------------------------------------------------------------------ That workflow's own comment already explains why it has no macOS leg: every test in tests/test_export_capability.py goes through _patch(), which monkeypatches _has_torch, get_device and is_apple_silicon, so a real Mac proves nothing a Linux runner does not -- and studio-backend-ci.yml runs the same file on ubuntu-latest as part of `pytest tests/`. That argument reaches one step further than it was taken. If Backend CI covers the file on Linux, the ubuntu leg HERE is the duplicate too. Checked: the file is not in that job's --ignore list. The import-safety test does not need a torch-free image either; it installs its own builtins.__import__ blocker and drops preloaded torch/unsloth from sys.modules, so it proves the same thing inside Backend CI's fully installed environment. Windows stays. Nothing else in CI runs that file there, and _has_torch's import probe is the per-OS behaviour the job exists for. The guard ------------------------------------------------------------------------ tests/studio/test_short_job_absorption.py, wired into the unfiltered job. Both changes fail silently rather than loudly if they regress, which is what it is for: - the suite still runs somewhere, and no longer runs twice - the absorbing job's harden-runner has not widened past the six endpoints the suite came with, since "the policy is identical" is the entire justification for this host - the absorbing job has not gained a paths filter - capability still has its Windows leg - studio-backend-ci.yml still runs the whole tests/ tree and does not name test_export_capability.py, because one line added to that --ignore list would remove the coverage the ubuntu leg was deleted for, and nothing would turn red Mutation-tested, each failing exactly one test: drop tests/security from the invocation; add one endpoint to the allowlist; add --ignore=tests/test_export_capability.py to Backend CI; remove the Windows leg. Also corrected a docstring in tests/security/test_scan_packages.py that named tests-security and what it installs. It was about to become false. Verification ------------------------------------------------------------------------ tests/security under the host's own -n 4: 409 passed, 6 skipped. scripts/lint_workflow_triggers.py: OK across 41 workflow files. All three workflows still parse; security-audit.yml keeps its other 4 jobs. Net: 7 short ubuntu slots per commit, down to 5. A note for whoever extends this. The census that found these 15 candidates was partly stale and I nearly acted on it: the lockfile and load-orchestrator rows were already absorbed by #9176 and their samples were pre-merge tails, and the two Local Agent Guides rows show ~0s because they are if-gated to schedule and dispatch, which is a skip and not a fast job. Read the trigger before ranking by duration. Of the remaining candidates, Scorecard is blocked by its job-level id-token: write, the Kaggle gate by a downstream needs:, npm-provenance by an audit egress policy plus registry.npmjs.org, and the notransport clean-install lane by a container that asserts several common tools are absent.	2026-08-20 00:05:28 -07:00
Daniel Han	1c3dde199b	Drain the sampling debounce long enough for the node CI actually runs (#9332) ... * Stop Frontend CI installing Chromium's system libraries through apt every 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. * Drain the sampling debounce long enough for the node CI actually runs 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/. * Raise the scenario drain to 600: 200 was marginal, and Windows needed 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. * Stop the settings smoke asserting a tab count a new page invalidates 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. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Drain the sampling suites on the loader too, and fail loudly when it 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. * Read the workflow and the dialog as UTF-8, not as the platform default 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. * Tell a pwsh crash apart from install.ps1 losing its exit code 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. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 23:55:01 -07:00
Daniel Han	3457a57a33	Stub the two helpers the sliced harnesses started importing (#9342) ... * Stub the two helpers the sliced harnesses started importing Backend CI's "Repo tests (CPU)" has been red on main since 588405dce, with 111 failures in two files. Both are the same defect, and neither is about what the tests claim to be testing. These harnesses replay real studio source with its import block stripped, so every name the sliced region calls has to be defined in the harness. Two landed without one: refresh-context-usage.ts findLatestUserVideoBase64 (#9056, decline to price a prompt carrying video) chat-runtime-store.ts resolvePreserveThinkingOnLoad Both product changes are right. The first is the more dangerous shape: with no stub the replayed body throws a ReferenceError, the effect bails, `counts` stays 0, and the assertion reads "the empty New Chat view must be priced exactly once" -- a pricing bug that does not exist. 41 tests failed that way, and bisecting main was the only way to see it, because the message points at the wrong thing. test_chat_autoload_failure_gate.py already guards this and said exactly what was missing, which is why its 70 failures named resolvePreserveThinkingOnLoad outright. test_new_chat_context_recount.py had no such guard; it does now, built the same way -- parse the real import list, assert the harness defines each name. Mutation-tested: dropping the video stub turns it red with the name in the message. The preserve-thinking stub is the real resolver's rule verbatim, since no scenario here sets a stored preference. tests/studio: 0 failures, from 111. * Count nesting when finding install.sh's GPU-detection fallback With the harness stubs in place, Repo tests (CPU) is down to one failure, and it is the last of the same kind: correct product code reported as a defect by a scan that reads the file too loosely. test_no_torch_backend_auto_outside_fallback allows --torch-backend=auto only inside install.sh's "GPU detection failed" branch, and located the end of that branch as the first line equal to `fi` after the comment. #8670 put a `case` into the branch to choose the desktop install spec, with an `if`/`fi` inside one of its arms. That `fi` is now the first one, so the block ended four lines early and the branch's own install call -- line 5280, the one the test exists to permit -- was reported as a primary path using the flag. The end is now found by counting nesting. `case` counts too: it closes with `esac`, so an `if`/`fi` inside a case arm would still throw off a counter that only tracked `if`. Added test_the_fallback_range_reaches_the_end_of_the_branch, because the bug here was in the range and not in what the range was used for, so nothing was checking the range itself. It pins both ends: the block contains the branch's first `if` and both of its --torch-backend=auto calls, and does not run past its closing `fi`. Mutation-tested both ways. Adding --torch-backend=auto outside the branch still fails the original assertion; reverting the nesting count to the first-`fi` behaviour fails the new one. tests/python/test_cross_platform_parity.py 78 passed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 22:06:51 -07:00
Pascal André	489fab4a71	Support OpenCode V2 in unsloth start (#9275) ... * Support OpenCode V2 in unsloth start * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep stable OpenCode guide coverage * Follow the OpenCode V2 stable release * Honor OpenCode V2 policy and server semantics * Fix OpenCode V2 launch command recipes --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-20 04:42:41 +02:00
Daniel Han	607e8a310d	Cache the .deb set webkit needs, so a dead mirror costs a miss not a shard (#9289) ... #9283 stopped two of the four Chat UI shards asking for webkit's system libraries at all, which is why extra and picker went from 14 minutes failing to under 6 passing. The shards that genuinely drive webkit -- chat, banner and the cross-browser indicator -- still pay the full price, and it is still the thing taking them down: 0 upgraded, 181 newly installed Need to get 102 MB/114 MB of archives Get:2 .../fonts-wqy-zenhei [7472 kB] -> 4m51s, then the attempt died Seen again today on a staging chat shard, after every other apt lever in this repo had already been pulled. Bounding the wait cannot help: the work is a real 102 MB and the mirror was delivering 7 MB in five minutes. So stop re-downloading it. apt keeps what it installed in /var/cache/apt/archives until something runs `apt-get clean`, so the .debs are already sitting there at the end of a good run. This copies them out, caches them on main, and copies them back in before install-deps on later runs. apt checks each file against its index and re-fetches only what does not match, so a stale entry costs a download rather than a wrong install -- the failure mode is slow, not incorrect. Keyed on the runner image as well as the engine set, because which .deb versions satisfy a dependency set is a property of the image. Saved on main only, the same rule as every other cache here: a PR-ref entry can only be restored by re-runs of that same PR while still counting against the shared budget. The engine guard from #9283 caught this change, which is the second time it has earned its place. Its detector read step NAMES as well as run bodies, and this step is named for the .deb set it holds -- webkit's -- so two chromium-only shards suddenly looked like webkit users. Fixed in the detector rather than by renaming the step: a `uses:` step cannot drive a browser, so only steps that run something should count.	2026-08-19 06:57:19 -07:00
Daniel Han	32ba55b578	Run the zoo suite in parallel, minus the two files that cannot share a worker (#9285) ... * Run the zoo suite in parallel, minus the two files that cannot share a worker unsloth runs unsloth_zoo's whole CPU suite in each of three matrix cells, at about nine minutes a cell. Measured on a staging runner, whole suite: cell serial -n 4 --dist loadfile HF=latest + TRL=latest 580s 253s HF=4.57.6 + TRL<1 517s 229s HF=default + TRL=default 568s 246s Two of the three cells agreed exactly, failure sets and skip sets both. The third produced 14 failures serial does not: 8 in test_mlx_generate.py and 6 in test_moe_bnb4bit_per_expert_conversions.py. Both files pass on their own, and pass under xdist on their own. They are self-contained, so what breaks them is another file running first in the same worker -- an ordering serial never produces, because serial is alphabetical. The speedup and the divergence are therefore separable, and the pair runs in a process of its own where they are reliable. Being clear about what this is: a mitigation. The suite has cross-file pollution that alphabetical ordering happens to hide, and the cure belongs in unsloth_zoo, on whichever file leaks. What these two files prove does not change here -- a fresh process is what they already get today. loadfile rather than the default `load`, because 34 of the 236 zoo test files touch sys.modules or importlib.reload, so tests within a file have to stay on one worker and in order. The guard exists because half of this pairing is silent. An ignore with no serial rerun deletes 51 tests from CI and the job stays green, which is strictly worse than the 14 failures, since those at least announce themselves. Same shape as test_backend_ci_parallel_isolation for studio-backend-ci. Mutation-tested four ways: dropping the serial rerun, dropping an ignore, dropping --dist loadfile, and losing a deselect in the split -- that last one matters because the three deselects lived on the single command this change split in two, and losing one turns a deliberate "deselected" into a failure on a GPU-less runner. Listed in workflow-trigger-lint because it reads a workflow, so a workflow-only PR would otherwise never collect it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Correct the evidence: the divergence moves between cells A second A/B run across all three cells changed what the first one meant. First run: HF=default diverged, the other two agreed. Second run: HF=latest diverged, HF=default agreed. Same 14 tests both times, same two files -- 8 in test_mlx_generate.py, 6 in test_moe_bnb4bit_per_expert_conversions.py. So it is not a dependency combination, which is what one run made it look like. It is worker scheduling, and roughly one cell per run draws the losing order. Four of six cell-runs were clean. That matters for anyone who re-runs this and sees green: a single clean run proves nothing here, and would otherwise read as grounds to drop the pin. The comment and the guard docstring now say so, because the previous wording pointed at a cell and would have sent the next person looking at transformers and trl versions. The files being pinned are unchanged; if anything the case for them is stronger, since the divergence lands on exactly the same 14 tests each time rather than wandering. * Give the xet stall detector a quiet process too Staging ran the split for real and produced a failure the A/B never did: FAILED tests/test_hf_xet_fallback.py::test_a_second_buffering_episode_after_a_drain_is_not_a_stall AssertionError: a child refilling after a drain was killed: ['Download appears stalled (xet transport) -- no progress for 0s'] Nothing was stalled. The file drives a stall detector against the real clock -- 27 sub-second sleeps, one of them commented "within the unmeasurable window" -- and under four workers on four cores the test was simply descheduled long enough for the detector to fire. "No progress for 0s" is the detector saying so. That is a different fault from the two files already isolated. Theirs is ordering, which is why a fresh process fixes it; this one is a wall-clock margin, and no ordering fix helps a margin. Same remedy, different reason, and the registry records the difference so nobody later "consolidates" the entries and loses why each is there. Checked for the rest of the class rather than waiting to meet it on the next staging round: exactly two files in the suite use sub-second sleeps, and the other one sleeps 0.01s to widen a race window without asserting on elapsed time, so it is not exposed to this. Found only because the split changes how work is distributed, which is worth noting: the A/B ran the whole suite under xdist and this file passed every time. Measuring the change is not the same as running it. * Record why each pinned file needs the quiet process Both causes are now known and they are not the same, which matters because the remedy differs. The bnb one is a sys.modules stub that was never taken back out, and it is fixed upstream in unsloth_zoo#1076 -- so that pin becomes belt and braces rather than the only thing holding it. The mlx one is an import-order contract stated in the shim's own docstring, and the clean fix would spoof platform.system() suite-wide, so the pin is doing real work there and will keep doing it. * Correct the mlx explanation: it is not established I wrote that test_mlx_generate.py fails because the MLX-on-torch shim has to be installed before any unsloth_zoo MLX module is imported, quoting the shim's own docstring. It fits the symptom -- eight isinstance failures are what two copies of a class look like -- but it is wrong. conftest imports unsloth_zoo, which pulls unsloth_zoo.mlx in before any test module loads. So the precondition is violated on every run, including every run that passes, and the file passes on its own. I found this by asserting the precondition and watching it fail a green run, which is the only reason this comment is not shipping as fact. The pin does not change: eight tests fail under xdist and pass serially, reproducibly, on the same commit. That observation is what justifies it. The explanation was mine and it did not survive being tested, so it now says so -- a wrong lead in a comment costs the next person more than no lead at all. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 06:25:09 -07:00
Michael Han	dc470257b6	studio: keep each chat's sampling params and system prompt with the chat (#9055) ... * studio: keep each chat's sampling params and system prompt with the chat The composer pills, permission level and retrieval controls already travel with the thread. The sampling params and the system prompt did not, so returning to a chat started under one prompt showed whichever prompt the last chat had, and every chat shared one set of sliders. Those eight join the snapshot: temperature, top P, top K, min P, the two penalties, the system prompt and its variables. They are the only thread-scoped settings that live under params rather than as store fields of their own, so the read and apply paths route them through there and the whole snapshot is still one object on the thread row. An edit made with a chat open lands on that chat; with no chat open it moves the installation defaults, as before. A model's own recommendation is not an edit: both paths that apply one now say so, or loading a model while a chat was open would pin that chat to the model's sampling. Context and checkpoint stay out. They describe the model that loaded rather than the conversation, and a chat restoring a budget the current model cannot hold is the failure that exclusion exists to prevent. * studio: keep a chat's sampling through a model load and a held edit Three follow-ups from review on the per-chat sampling change. A model's recommendation still landed in the live params: fromModelDefaults only moved where the value was persisted. A chat with stored sampling ran the model's instead, and the next unrelated edit snapshotted that over what the chat had. setParams now puts back the keys the chat holds; one it never set still follows the model. The Qwen mode table is the model's recommendation, not a value the user picked, so both paths that apply it are marked. Loading the model and toggling Think reach the same table, so marking one and not the other pinned the chat by the other route. The held-edit flush read the sampling keys as store fields. They sit under params, so it read undefined, the sanitizer dropped them, and an edit made while the chat's snapshot was in flight was lost when the user navigated away. * studio: persist a model's sampling defaults even when the chat pins them Restoring the chat's values before the diff made every pinned key equal on both sides, so it dropped out of changedParams and never reached the installation defaults. One chat pinning temperature left every later new chat on whatever model loaded before it. The restore is a live-store concern only. Persistence diffs the model's own object again, as it did before the restore existed, so the recommendation reaches the defaults while the open chat keeps what it stored. getChangedInferenceParams bumps the mutation versions, so it stays a single call rather than one diff per purpose. * studio: keep a chat's sampling across a model switch, both directions setCheckpoint replays the destination model's remembered params itself, without going through setParams, so the restore never ran there. An external switch has no load after it to correct the result, so the chat adopted that model's prompt and sampling and kept them. The restore now runs on that path too, and persistence still reads the unrestored object so the model's own values reach the installation defaults. The other direction leaked the same way. rememberOutgoingModel snapshots the live params, which inside a chat are that chat's, and on a model with no entry it persists that snapshot whole. Filtering the incoming edit cannot undo a write that already happened, so the outgoing snapshot is now built without the values the open chat owns. What the model already remembered wins over the installation copy, so stepping off a model inside a chat does not flatten a preference the model was given outside one. persistReplayedParams writes the installation defaults on the setCheckpoint path, so it keeps the in-memory copy level the same way setParams does. * Keep a chat's sampling edits through hydration, model loads and remembered-params writes * Keep a held chat edit out of the outgoing model, and keep a default published mid-pairing withoutActiveThreadParams returned early while a chat's read was still out, so switching model in that window snapshotted the open chat's temperature and system prompt into the outgoing model's shared memory. The sibling restore path already read the held value first; this one consulted only the override. Driving the real store put a chat's own sentinel straight into paramsByModel, which the simulation harness already treats as a leak. Second, a model default published inside the pairing window was sent to the settings route but then rolled back in memory from the pre-window sample, so the session and the server disagreed and the stale value was pinned onto the next chat with no snapshot. noteThreadScopedDefaults also returned early when nothing had been applied yet, which is the common case, so the value had nowhere to land. * Put the installation defaults back when a chat is left mid-read Leaving a chat while its settings read is still out writes the edits made in it to that chat's row, but the store kept showing them, so the next chat took them: a snapshot-less chat captured the store as the installation defaults and was pinned with the previous chat's temperature and system prompt. Restore the pre-window values over the committed fields, and only when the chat is actually being left, so a retry after a failed read and a fork both leave the edit on screen. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: tighten the per-chat inference comments Comments only. Collapses the multi-line explanations in the thread-scoped sampling path to their shortest clear form and drops the restatements, keeping the reasons behind the pairing window, the held-edit capture and the outgoing-model filter. --------- Co-authored-by: Unsloth <michaelhan@Michaels-MacBook-Pro.local> Co-authored-by: Daniel Han <danielhanchen@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 06:24:54 -07:00
Daniel Han	ba9e207ac7	Scroll into view before a forced click (#9291) ... * Scroll into view before a forced click `Compare tab: send to two panes` failed on a macOS runner with Locator.click: Element is outside of the viewport then a cascade of TargetClosedError as the browser came down behind it. The menu item existed and was found. It was simply below the fold: a Mac runner's window is shorter than a Linux one and Compare sits at the bottom of a long menu. `click(force = True)` turns off actionability checks, which is exactly what these call sites want against a menu whose overlay would otherwise intercept the click. It also turns off the scroll that brings the element into the viewport, and Playwright will not click a point it cannot reach. So the flag was doing two things and only one of them was wanted. The forced clicks are not new -- they arrived with the composer redesign in #5891 and have always worked on Linux. What changed is #9264, which made the Compare nav be found reliably instead of sampled for: before that the step often gave up earlier with "Compare nav not found" and never reached the click. That PR is correct; it surfaced this rather than causing it, the same way #9283 was going to surface the picker's context-pin assertion once the shard stopped dying in apt. click_forced keeps force and adds the scroll back. The scroll is best effort: an element that cannot be scrolled -- fixed position, zero size -- should still reach the click and fail there with Playwright's own message, rather than here with a scrolling one that names the wrong problem. Applied to all 17 forced clicks across the three drivers, not just the one that failed. They are the same hazard on the same runners, and the guard would be worth little if the file it was written for were the only one obeying it. The guard is a source scan for the bare shape, so a new one fails here rather than twenty minutes into a Mac job. Driven against a fake locator: these tests run on browserless lanes, and the ordering is the whole contract, so a stand-in that records call order tests it exactly -- scroll before click, force preserved, a failing scroll not swallowing the click, and a failing click still propagating. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 06:17:48 -07:00
Michael Han	334db4875f	Studio: add a keyboard shortcuts page and make the existing chords rebindable (#8948) ... * Studio: add a keyboard shortcuts page and make the existing chords rebindable Settings gains a Shortcuts tab listing every shortcut, with search, a recorder to change a chord, and controls to unassign or restore a default. The four chords that already existed were hardcoded across four files. They now resolve through one registry, so an edit applies without a reload. * Keep the shortcuts tab across reloads, and decide contested chords by registry order loadInitialTab checked a hand-written list that the new tab was missing from, so picking it and reloading fell back to General. The list is now the source of the SettingsTab union, so the two cannot drift. A chord claimed by two actions is consumed by whichever window listener runs first, which followed mount order and so varied by route. Registry order owns it instead, and the tab names the row that loses. * Clear rebound shortcuts on preference reset, and translate the shortcuts tab in every locale * Lock the auth-route settings guard to its behaviour rather than one spelling * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Record nothing for a Super chord off macOS There is nowhere to put Meta on Windows or Linux: matchesBinding rejects an event carrying it, and the recorder wrote mod from ctrlKey alone, so Super+Alt+K persisted and displayed as Alt+K. That assigns the action to a chord the user did not choose, fires it on Alt+K pressed by itself, and leaves the chord they actually pressed unable to match anything. Returning null is the answer the recorder already gives while only modifiers are held, so it simply keeps listening. * Studio: render the sidebar shortcut hints from the live binding The search tooltip and the Settings menu row hard-coded the shipped chords, so after a rebind or a clear in the shortcuts tab they kept telling the user to press a chord that no longer runs anything. Both now read the resolved binding through useShortcutLabel and drop the hint when the action is unassigned. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: Daniel Han <danielhanchen@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@users.noreply.github.com>	2026-08-19 06:11:21 -07:00
oobabooga	de102032ee	Desktop: ship a complete Linux AppImage (#9113) ... --------- Co-authored-by: Wasim Yousef Said <wasimysdev@gmail.com>	2026-08-19 10:09:29 -03:00
Daniel Han	6c41897833	Cache uv's downloads, which is now the largest single cost in CI (#9271) ... * Cache uv's downloads, which is now the largest single cost in CI Re-profiling after the apt work landed changed the ranking completely. The old top step, Install Playwright browsers at 557-671s, is now 91s. What sits at the top instead: 3588s total 92s x39 Install Unsloth (--local, --no-torch) 1925s total 175s x11 Update banner layout regression (Playwright) 1910s total 478s x 4 unsloth_zoo @ main - full pytest (CPU) 1683s total 99s x17 Perform CodeQL Analysis More total time than any test, in 39 job runs, and all of it uv re-resolving and re-downloading the same wheels: nothing carried its cache between runners. UV_CACHE_DIR appears in this repo only in the two clean-machine workflows, which set it in order to delete it. Caching this is safe because of WHAT is cached. uv's cache is content- addressed by URL and hash, so a stale entry cannot serve wrong content -- the worst it can do is miss. That is the difference between this and caching the venv, which would have to reason about an editable overlay (--local installs the repo with -e, so source changes need no reinstall), a moving `unsloth-zoo @ git+main`, and absolute paths baked into console scripts. I looked at the venv first and it is not worth the hazard. restore-keys is deliberate and is correct only here: a near-miss still supplies almost every wheel, which is most of the win, and it is safe precisely because entries are content-addressed. The same fallback on an install cache would be a bug. Saved on main only, like the HF and Playwright caches and for the reason they record: a PR-scoped entry is restorable only by re-runs of that same PR, while every PR can restore from the default branch, so saving on PRs spends a 50 GiB budget measured at 99.3% full once already and evicts main's copy. `uv cache prune --ci` first, so the key does not grow without bound across 39 jobs. Placed in the composite action, so all 39 call sites get it from one definition -- which is what that action exists for. The cold-install lanes are untouched and a test keeps them that way. They do not use this action today; if one ever did, the composite writes UV_CACHE_DIR to $GITHUB_ENV, which outranks a job-level env: for every later step, so a warm cache would silently replace the cold machine those workflows are named after and they would still go green. Nine tests. All five mutants reintroduced and confirmed red: the cache pointed at the venv, saved on every ref, restore-keys removed, UV_CACHE_DIR set after the install, and a cold lane adopting the action. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 05:47:32 -07:00
Daniel Han	b01b6ab82e	Re-type the native default, not whatever the input happens to show (#9282) ... * Tell the deliberate context pin apart from the phantom one "re-typing the shown context does not pin an override" reports a regression against behaviour the product implements on purpose. model-config-page.tsx: const pinFixedLayerContext = target.isGguf && loadableConfig.gpuMemoryMode === "manual" && loadableConfig.gpuLayers != null && loadableConfig.gpuLayers >= 0 && loadableConfig.customContextLength == null && activeLoadedContext != null; with the reason stated directly above it: "If the user fixes GPU Layers (Manual) and remembers, pin that shown context so a later fresh load keeps the fitted placement instead of sending native/0 and recreating the OOM." So storing the context is the feature. The step could not tell that apart from the phantom pin it was written to catch, and on a CPU-only runner -- placement manual, gpuLayers 0 -- the documented condition always holds, so the step failed deterministically every time it ran. Why it looked intermittent rather than always broken: as introduced in #7351 it inherited the popover from the Reset step above, Reset closes the popover, and the step skipped without saying so. #7760 made it own its state and it started running for real. Both of those PRs are right; the step was passing by not executing, and neither change could have known that. It has never failed on main, either, and that is not evidence it works: main's picker shard has been dying in "Install Playwright browsers" on the webkit package set, so the suite never reaches this step. Once that is fixed the shard gets far enough to run it, which is how this surfaced -- on a staging run, on a PR touching one backend test file. The distinction is now made from the stored entry: a pinned context is expected when that entry also carries the fixed-layer placement, and a regression when it does not. The phantom-pin guard survives, because a pin with no manual placement behind it still fails, and the failure now prints the fields it judged on rather than a bare number. Supersedes my first attempt on this branch, which re-typed the pre-load default instead of the shown value. That was wrong twice over: the pre-load 2048 is a UI default rather than the model's native context (32768 once loaded), and the step's invariant is about entering an UNCHANGED value, which only the shown one is. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 05:08:17 -07:00
Daniel Han	65bdc91a1a	Repair the tests main is currently red on (#9192) ... * Give the mmproj fallback import its file extension node --experimental-strip-types resolves relative specifiers literally, so the extensionless import added in #9173 fails at import time and takes queued-model-capabilities.test.ts down with it. Before that commit this module had only import type lines, which are erased, so it carried no runtime relative import at all and the test passed. 55 other modules already spell the extension. * Studio: repair the repo tests #9173 left red The mmproj fallback landing added a mmprojFallbackMessage import to chat-adapter.ts. Two repo tests read that file's source: - test_chat_autoload_failure_gate slices autoLoadSmallestModel out of the adapter and runs it, and its own guard fails the whole file when the slice references a name the harness preamble does not define. Add the stub. - test_model_picker_contracts pinned the success toast's description as the literal 'description: cpuFallbackReason'. The mmproj branch now sits ahead of the CPU one, so pin the description expression instead of which fallback is tested first. 71 failures on main, none of them a real regression. * Studio: repair the backend test #9173 left red test_tensor_split_abort_raises_early_to_layer_fallback pins the order of two branches in load_model by their source offsets. #9173 renamed the variable handed to _strip_mmproj_args, so the find() of the old spelling returned -1 and the ordering assert compared against it. Pin the call itself and fail loudly when it is no longer reachable. * Stop the Windows chat UI smoke racing the route Suspense fallback exercise_permission_mode_controls asserts the permission pill visible on the 5s expect default, immediately after the composer mounts. The composer can mount and then drop back behind the root Suspense fallback while the route chunk loads, so the assertion lands on 'Loading...' and gives up. Reproduced on main at aa32c1861: same step, same line, aria snapshot '- text: Loading...'. It is Windows-only in practice because the mac smoke wraps the script in a 3-attempt retry and the Windows one does not, so a slow 4-vCPU runner surfaces the race raw. Wait the pill out on the same budget the composer already uses. * Wait the permission pill out at every reload, not just the first The macOS smoke hit the same Suspense race at the second assertion, which the first commit did not cover. Four of the six assertions in this block follow page.reload(wait_until = domcontentloaded), which returns long before the lazy chat chunk resolves, so asserting on expect's 5s default races it by construction rather than only on a slow runner. Route all six through one helper that waits on the composer's budget. Seen on both the Windows and macOS smokes as 'Aria snapshot: - text: Loading...' at playwright_chat_ui.py:135 and :189. * Give the playwright stub every name the harnesses import The CPU test job has no playwright, so tests/studio/test_heavy_thread_measurement_integrity.py puts a stub `playwright.sync_api` into sys.modules at collection time rather than skipping the harness arithmetic along with the browser. That stub defined one name, `sync_playwright`, which is all its own harness imports. sys.modules is session-wide, so the stub is not read only by the file that installs it. Three of the four harnesses import `sync_playwright` alone and were satisfied by it; playwright_strip_ansi_smoke also imports `Page` and `expect`, and got ImportError: cannot import name 'Page' from 'playwright.sync_api' (unknown location) from a stub two files away. `(unknown location)` is the stub itself: a bare ModuleType has no __file__. The two tests that import that harness, test_the_ansi_dump_survives_a_vite_server_that_is_ still_talking and test_an_empty_smoke_base_url_means_unset[playwright_strip_ansi_smoke], failed on it. pytest.importorskip("playwright") does not save them, because by then the stub package is importable. This is pre-existing on main and predates this branch, which never touched either file. It was masked: the same job was failing with 71 other errors, which this PR fixes, so these two were never the reported cause. They belong here because this is the PR that makes the job green. The stub now answers any public name with a callable that raises, so a harness can satisfy its imports without quietly measuring a browser that is not there, and adding an import to a harness cannot break a different file again. Dunders still raise AttributeError, since pytest and inspect probe those and answering them makes the stub look like a package. The install probe also imports off the submodule now: a partial install leaves `playwright` importable while `playwright.sync_api` resolves to an empty namespace, which fails the same way. Verified by blocking playwright in a subprocess to reproduce the CPU job: 2 failed, 104 passed before, 106 passed after, with the two files alone skipping cleanly when the stub is not collected. * Stub the fork-count endpoint the heavy-thread smoke actually requests The heavy-thread browser smoke fails its own harness check on this branch: HARNESS-BROKEN chromium at 25000 chars let 2 /api/ requests reach the network during the measured actions; the timings include a round trip per request Both leaked requests are GET /api/chat/threads/__LOCALID_.../forks, one during seeding and two more inside the measured actions. The smoke page answers a small allowlist of endpoints in-page so no round trip lands in a timed region, and the fork-count entry in that allowlist still matches the per-message endpoint, /threads/{id}/messages/{id}/forks. #8992 replaced it with one per-thread request built by getThreadForkCounts, and the allowlist was not moved with it, so every fork-count GET went to the dev server. fork-count-store refreshes on CHAT_HISTORY_UPDATED_EVENT, which the delete action fires, which is why two of them land inside the measurement rather than only at load. Match the URL the client builds and answer the body the endpoint returns: getThreadForkCounts reads data.counts into a Map, so {"counts":{}} is "no message has forks" and renders no badge. The fixture is unchanged by this: the census at both sizes is identical to the failing run, 3676 and 14285 DOM nodes, 20 and 80 messages, 3216 and 12804 highlighted tokens. Only the round trips are gone. This has been broken repo-wide since #8992 and invisible. On every other open PR the earlier Unit tests step in this job fails on a broken import, so Install Chromium and every Browser smoke step is skipped; this branch is the first job in which they run at all. Compare step outcomes on #9081, Unit tests failure and all smokes skipped, with this branch, Unit tests success and the smokes running. Two things so the next drift costs less. The stray-request failure now names the first three distinct URLs, because it fires in CI where nobody can attach a listener afterwards, and finding this one meant re-running the smoke locally with a print statement patched in. And a unit test now checks the allowlist against the URL chat-api.ts builds, rather than against a string someone remembered to update, so the same drift fails in Dev-server lifecycle tests instead of waiting for a job where the browser smokes get to run. The self-check itself is untouched: still zero tolerated strays, still failing the run. Verified locally on chromium at 25000 and 100000 chars: before, 2 stray requests at each size and exit 1; after, 0 stray requests at each size, 9 stubbed rather than 6, and exit 0. The three smokes the aborted job skipped, settings tab panels, a settings panel that cannot load, and chat stream pacing, all pass locally as well, so nothing further is hiding behind this one.	2026-08-19 05:03:46 -07:00
alkinun	41b93a9169	Allow unlimited Deep Research time budgets (#9078) ... * Allow unlimited Deep Research time budgets * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Bound unlimited research request setup * Validate unlimited research timeout settings * Handle unavailable research timeout storage * Preserve research stream idle timeout * Update Deep Research dialog contract * Bound the unlimited path and restore the timeout floor for PR #9078 An unlimited budget removed the wall clock that used to backstop a queued request, so a backend that sent one queue notice and went silent held the run open until the user cancelled. Bound the gap between notices with the same value the unlimited read timeout already uses; each notice refreshes it, so genuine queueing is still not charged. - Clamp the model-load and model-switch waits: with the 3600s ceiling gone, a large finite budget could make the poll loop wait for years. - Give modelTimeoutSeconds a readable one-year ceiling instead of float max, so the 400 no longer quotes a 309-digit number, and keep the 10s floor for positive values with 0 as the only sentinel below it. - Bound the same field in the persisted chat settings. - Offer the default minutes when a dialog switches off No limit, rather than 1. * Report an unlimited stall by name instead of racing HTTPX for PR #9078 With no wall clock behind an unlimited request, HTTPX's read timeout and the first-output/idle guards were set to the same value, so which one fired was a coin flip. HTTPX won often enough in an end-to-end run against a real SSE server, and it raises ReadTimeout with an empty message, so the run failed with a blank error instead of naming the stall. Give the read timeout headroom so the named guards expire first, and convert a bare ReadTimeout that still gets through into the matching stall error. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Bound the research timeout where it is set for PR #9078 Two validators disagreed with the run route, so a value could be persisted that made every later Deep Research run fail to start with nothing pointing at the setting that caused it. - The composer's minutes field had no ceiling and the mirror bound was MAX_SAFE_INTEGER, while ChatSettingsPayload and the run route both cap at one year, so a large entry was accepted, dropped from the settings patch, and then rejected on every run. Bound it where it is set: the input, the store setter and loader, and the mirror now share the backend ceiling. - The settings payload took 1 through 9, which the run route rejects. Refuse those there too, so a persisted value is one the run route will accept. * Refuse a sub-floor research timeout in the frontend too for PR #9078 The settings payload now rejects 1 through 9, but the mirror bound and the store validator still accepted them, so a stale or hand-edited local value would hydrate, be sent unchanged, and 400 every run start while its own patch was also rejected. sanitizeBoundedNumber takes an optional minPositive so a sentinel below the floor stays legal while the gap above it does not, which is the shape this field needs and not a special case in the caller. * Clamp an over-cap typed budget instead of resetting it for PR #9078 The max attribute does not stop a typed value from reaching the save handler, which passed it on to a setter that substitutes the 900 second default for anything over the cap. So someone asking for a very long run saved what looked like their limit and got a fifteen minute one, which is the failure this PR exists to remove. Clamp to the documented maximum at save time. * Bound queue gaps by the configured admission heartbeat for PR #9078 * Tighten the comments for PR #9078 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Name the Deep Research budget by its per-request scope for PR #9078 * Reject a boolean budget instead of reading it as unlimited for PR #9078 * Reject a boolean settings timeout and keep an untouched budget exact for PR #9078 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>	2026-08-19 05:01:10 -07:00
Daniel Han	718fe69fb0	Install the browser engines each Chat UI shard actually drives (#9283) ... The four shards install all three engines each. Two of them never open anything but chromium: nothing in `extra` or `picker` names firefox or webkit at all. Installing webkit's system libraries for them is not a rounding error. On 2026-08-19, in the extra shard: 0 upgraded, 181 newly installed, 0 to remove Need to get 102 MB/114 MB of archives Get:2 .../noble/universe amd64 fonts-wqy-zenhei all 0.9.45-8 [7472 kB] -> 4m51s later, attempt 2/2 did not finish within 300s Fonts, X fonts and a soundfont, fetched so that a shard which never launches webkit could time out fetching them. Both attempts died there and the shard reported nothing about the UI it exists to test. That is the last apt exposure in this job, and unlike the previous two rounds it cannot be fixed by bounding anything. Making the index refresh cheap (the retries change) left the download, and no timeout survives a mirror moving 7 MB in five minutes. The fix is to stop asking for 102 MB that nothing uses. chat keeps all three for Cross-browser permission controls, banner keeps all three for its "other engines" step, and the browser cache key carries the engine set: without that the chromium-only cache would be restored by a three-engine shard, report a hit, skip the download, and fail to launch what it never got. The matrix keeps `shard:` as a list and augments it through `include:`, because test_chat_ui_shards_cover_everything reads that list to prove every Playwright step lands on some shard. An include-only matrix passes YAML and silently hides the shards from that guard, which I did first and it caught. test_ui_shard_engines enforces both directions against the steps, never against the comment above. Installing an unused engine is waste; driving an uninstalled one is worse, because Playwright reports it as a launch failure deep inside a suite, minutes after the install step went green, which reads as a flaky test rather than a missing package. Mutation-tested three ways: adding webkit to picker, removing webkit from chat, and collapsing two engine sets onto one cache key. It is listed in workflow-trigger-lint because it reads a workflow, so a workflow-only PR -- exactly the change it exists to reject -- would otherwise never collect it. That guard caught this too.	2026-08-19 04:26:37 -07:00
Daniel Han	4d0d327c92	Wait for the Compare nav instead of sampling for it (#9264) ... * Wait for the Compare nav instead of sampling for it #9251's Windows Chat UI run failed with [ui-extra] FAIL: Compare nav not found six milliseconds after the step began. Locator.count() does not wait. Playwright's auto-waiting covers actions and expectations, not counting, so the gate was a sample of one instant presented as a question about the app, and it sampled the instant a reload snapshot overlay was still up. #9251 paints a cloned overlay over the app and removes it on hydration (or after 5000ms), which opens a window where the composer is on screen but not yet in the accessibility tree. The report was true about that instant and false about the app, and in CI it is indistinguishable from the menu item having been deleted, which is the expensive part: it sends the next reader looking for a removed control that was never removed. wait_for_first() waits and returns None on timeout, so the callers keep their present-or-not branch, including the fallback that legitimately expects a miss and looks under the More submenu. Only a timeout is swallowed: a closed page still raises rather than being reported as a missing feature. The failure message now names which of the two controls never appeared. This is the first step after load, so it is the one that pays for anything slowing first paint. It will not be the last change to do that. Six tests, no browser needed, since a locator is a small protocol. All four mutants (shortened default, blanket except, raising, waiting on visible) were reintroduced and confirmed red. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 03:48:57 -07:00
Daniel Han	51b7d4a155	Give the Colab freeze a pip cache, like the job beside it already has (#9276) ... smoke-install downloads a 709-line pip-freeze on eight matrix legs at once and keeps none of it. api-introspect, two jobs up in the same file, already restores and saves through the action pair; this one was left on a bare setup-python. Keyed on the freeze plus the workflow. The freeze is the pin set, so it is what the downloads depend on, and the workflow comes too because the seed step rewrites those pins in place -- CPU index mapping, skips, spoofs -- so an edit there changes what gets installed without touching the freeze. api-introspect keys on the workflow alone because it pins its dependencies inline; keying this job the same way would rebuild 709 downloads whenever any other job in the file is edited. Saved with always(), because the failure path is when the cache is worth most: if the freeze does not resolve as a set the job falls back to installing 709 pins one at a time, and a partial download set is still a head start. Correcting something I have been assuming about this job: it is `schedule` and `workflow_dispatch` only, so it never runs on a pull request and none of this is on a PR's critical path. It is still worth doing, for a different reason than I had -- eight ubuntu runners holding a 25 minute cap contend for the same pool every PR job queues against, and the queue is repo-wide. The allowlist in test_cache_budget_discipline caught the new entry, which is what it is for: a cache is a claim on a shared 50 GiB budget and a job has to earn it. Listing it there also enrolls it in the checks that were already running for the other nine, so the key scoping, the restore-to-save wiring and the main-only save gate are all now enforced on this job rather than trusted.	2026-08-19 03:22:16 -07:00
Daniel Han	214e24c0ce	Move the two version-incidental macOS jobs onto the emptier pool (#9277) ... * Move the two version-incidental macOS jobs onto the emptier pool Rust unit tests (macos) is this repo's last finisher. The census behind that found minutes of work sitting behind hours of queue, and no execution-time work touches it, so the runner pool is the only lever left. studio-mac-install-matrix runs macos-15 and macos-26 from ONE matrix, which makes them comparable on identical commits and identical triggers rather than across different jobs at different times. Over 163 jobs per leg: exec med exec p90 queue p90 macos-15 160s 713s 20667s macos-26 176s 597s 3866s The medians say these are the same class of machine, which they are: both free Apple Silicon standard runners. The queue p90 says they are not the same pool. macos-15 is 5.3x worse in the tail, five and a half hours against one. Worth being precise about what the tail means here, because the median is zero on both images and hides all of it. macOS queueing is not slow on average; it is occasionally catastrophic, and the occasional case is what sets the wall clock for the commit it lands on. The likely cause is the macos-14 retirement. Everything migrated onto macos-15, this job included, and macos-26 was left comparatively empty. That makes this a fact about today's pool rather than a property of the image, so the comment records the query as much as the result: it is one read of the install matrix and can be re-run whenever the queue looks wrong again. Two jobs, not all of them. These two test our Rust crate and our MLX dispatch, where the OS version is incidental. The install matrices are deliberately testing across OS versions and are left alone, and studio-mac-ui-smoke stays too: it documents a dependency on the frameworks the image ships for Chromium, which is a claim about macos-15 specifically and wants checking before it moves. The guard is for the next retirement rather than this one. macos-14's is recorded in three comments in this repo, each explaining why some job moved off it, and comments do not fail -- so the next one gets discovered the way this one was, by a pool nobody is watching. LIVE_MACOS_IMAGES cannot know GitHub's roadmap, but it forces the retirement to be written down once and then names every job that has to move. Mutation-tested by putting macos-14 back. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 03:21:25 -07:00
Daniel Han	6e57736ad9	Stop blaming the recipe for a turn that never came back (#9266) ... * Stop blaming the recipe for a turn that never came back `connection (codex)` fails on main and on unrelated branches, and the two lines it prints contradict each other: ##[warning] ... judging the turn on its assertions instead of calling it guide drift. ##[error] [guide drift] agent=codex: the documented launch command exited non-zero (rc=124) ... so the documented flow in start.py drifted. Neither is right. run_timed printed the warning unconditionally, but three callers (connection, resume, attribution-ab) have no assertion that can rescue a partial turn and treat a cap as fatal on purpose. So it promised the opposite of what was about to happen, for exactly the callers most likely to hit it. It now states the fact and leaves the verdict to the caller. The error is worse, because it sends the reader to the one file that is not implicated. The transcript shows codex launching perfectly -- correct provider, correct model, correct profile -- and then sitting on ERROR: Reconnecting... 1/5 for the full 600s. Nothing about the documented flow had drifted; the model server never answered. A cap means the launch command was fine and the turn never came back, which is a different failure with a different owner. Still fatal. Waiving a cap here would report "connection OK" for a recipe that printed a banner and then blocked on a headless prompt, which is the failure this job exists to catch, and assert_reply cannot tell a finished reply from a startup banner. Only the attribution changes. A non-zero exit from the launch command is still reported as drift, which is the case guide_fail is right about. Four static guards, since the script needs five agent CLIs and a live model server to run. All four mutants (run_timed speaking for its callers again, the timeout routed back through guide_fail, the timeout waived instead of fatal, a non-zero exit no longer reporting drift) were reintroduced and confirmed red. Wired into workflow-trigger-lint, the only job with no paths filter. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Make the waiver guard enforce its own sentence, not a magic number The shell suite went red on this branch: 'exactly the expected TIMED_OUT sites (got 4, want 3)'. Worth deciding which of the two was wrong rather than moving the number. The heading is 'only file-edit turn 1 rescues a soft timeout', and that is the invariant worth having. The assertion under it counted every consultation of TIMED_OUT and pinned the total at 3 -- so it conflated the single waiver with the fatal checks beside it, and its own comment admitted as much ('turn 1's guard, PLUS the resume and attribution-ab fatal checks'). The fourth site this branch adds makes a cap MORE explicitly fatal at connection. Failing a guard named for waivers because a fatal check was added is the assertion being over-specified, not the change being wrong. The neighbouring semantic assertions agree: 'connection guard has no TIMED_OUT escape' passes. So the two shapes are counted separately. The waiver is the || form and is the only one that lets execution continue past a cap; everything else consults TIMED_OUT to stop. That enforces the sentence rather than a number, and keeps all the power: a waiver added anywhere still fails. Mutation-tested against the repaired guard -- a waiver added to the connection guard, resume's fatal check turned into a waiver, and the connection check deleted outright are all caught. 44 pass, 0 fail. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 03:21:20 -07:00
Daniel Han	dccc792374	Ask whether Playwright system libraries are missing before installing them (#9273) ... * Ask whether Playwright's system libraries are missing before installing them `--with-deps` makes playwright run its own `apt-get update` internally. That is the one apt call in this repo that could not be restructured: every other one now tries the image's package lists first and only refreshes them on a miss, but this update happens inside playwright and there is no flag for it. So it still pays full price for a dead mirror. On this branch's own base, 2026-08-19: 07:42:09 Installing dependencies... Switching to root user... 07:42:29 Ign:2 http://azure.archive.ubuntu.com/ubuntu noble InRelease 07:58:22 (step fails, 16m14s) three shards at once, on the critical path of each. Two changes, both of the same shape as the apt work: separate the failures, then ask whether the work is needed. The browser download is a CDN fetch and has nothing to do with apt, so it is its own call now. A stalled mirror can no longer stop browsers being downloaded, and a CDN failure no longer reads as an apt problem. The apt transaction runs only when the libraries are actually missing. ubuntu-latest is a browser-testing image and ships nearly all of them; when it does, `install-deps` is a full apt update and transaction to install nothing. The honest test of that is launching each engine, which is exactly what the suites are about to do -- so the probe is the same question the job asks two steps later, asked cheaply and first. When the probe fails, install-deps runs and the probe runs AGAIN. Without that second check a missing library surfaces later as an opaque per-test timeout in whichever suite happens to run first, which is a much more expensive way to learn the same fact. Rebudgeted for two helper calls rather than one: 2 x 300s plus a lock wait, twice over, is 24m of worst case against the job's 30, so the per-attempt cap comes down from 420 and the step timeout goes 18 to 25. test_apt_steps_are_bounded caught that arithmetic when I first got it wrong -- the two calls doubled the authorised retries past the step's own cut-off. The probe's logic is exercised against a stubbed playwright: exits 0 when all three engines launch, and 1 naming each engine that does not, for one broken engine and for two. The embedded Python is parsed with ast and the surrounding shell with bash -n, in both shards. * Stop apt retrying inside the install-deps attempt The probe landed and worked: banner and the cross-browser shard found the libraries already present and skipped apt entirely, 18m to 7m40s and 12m36s. The other three shards found them genuinely missing, so install-deps ran, and there the premise of the previous commit was wrong. The image does not ship everything all three engines need. install-deps runs its own apt-get update and that update is the one apt call here that cannot be restructured to try the image's lists first. The helper's 20s transfer cap was applied to it and said so in the log: 08:41:45 apt configured to fail fast: 20s transfer timeout, 3 internal retries 08:42:14 Get:5 https://archive.ubuntu.com/ubuntu noble-security InRelease 08:46:45 attempt 1/2 did not finish within 300s 4m31s with a 20s cap in force. The cap was not the problem; repeating it was. Acquire::Retries defaults to 3, meaning four passes over the index, and with four InRelease URIs stalling to the cap that is 4 x 4 x 20s. The update spent the entire attempt and the packages were never fetched. So the retry moves out of apt and into the loop that already exists for it. One pass is about 80s, leaving 220s of the attempt for the download it is for, and a stall that outlives that is now a failed attempt the outer loop retries under a fresh timeout, with a warning naming which way it went. apt's internal retries are invisible by comparison and are charged to the caller's budget. Guarded, because reverting it looks harmless: the value would go back to apt's default, every other assertion in test_apt_steps_are_bounded would still pass since the step remains bounded and retried, and the symptom would come back as a step that fails slowly against a mirror, which reads as bad luck rather than as a setting. Mutation-tested by restoring the 3.	2026-08-19 02:37:33 -07:00
Daniel Han	2e3cc032f6	Record the venv own requirement digests, not the installers (#9263) ... * Record the venv's own requirement digests, not the installer's Every fresh desktop install on Linux and macOS came up ManagedStale and repaired itself before it would run. From the app's own log: 05:44:22 Managed preflight: install probe result Stale { reason: "studio_install_requirements_changed" } 05:44:22 desktop_preflight completed disposition=ManagedStale 05:44:22 start_managed_repair command called 05:44:24 studio install incomplete -- forcing dependency pass to repair... 05:44:32 Managed preflight: install probe result Ready The manifest recorded digests from REQ_ROOT, which is the requirements directory next to whichever install_python_stack.py ran. A desktop bundle carries its own copy. verify_install reads the INSTALLED package's copy, because at verify time install_manifest.py is imported out of the venv. Two different trees, compared to each other. They agree until a tracked requirement file changes upstream, and then every install performed by that bundle is stale for ever. v0.1.800-beta was cut 2026-08-14 and installs unsloth 2026.8.18; #9148 pinned openai in extras.txt in between. Windows was unaffected only by luck of layout. The digests now describe the tree the verifier will read. A source or editable install has no copy under site-packages and still uses the root it was given, which is what keeps an edited studio.txt invalidating the manifest on the --local path. Also: the CI gate reported "saw: ManagedStale" and nothing else on all four platforms. The reason is logged on its own line, and tail -60 scrolls it away the moment the backend starts logging, so the one fact that explained this had to be recovered from an uploaded artifact by hand. All three log dumps now grep for it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-19 01:31:09 -07:00
Daniel Han	fdf83cbd6a	Make apt fail fast, so the retry has something to retry (#9260) ... * Make apt fail fast, so the retry has something to retry #9256 bounded the stalls. This says why they happened and stops them. Reading the four logs, every one ends the same way: 04:47:02 Get:5 https://archive.ubuntu.com/ubuntu noble-security InRelease [126 kB] 05:16:29 ##[error]The operation was canceled. Twenty-nine minutes of silence mid-fetch of a 126 kB index file, and two of the four hung on that same file. What came before it is the other half: azure.archive.ubuntu.com was Ign'd four times over thirty seconds, so apt had already failed over through /etc/apt/apt-mirrors.txt to the public archive, which is not provisioned for this fleet. apt did not treat any of that as an error. Acquire::http::Timeout defaults to 120s and is an idle timeout, so a socket that is open and trickling never trips it. Cutting it to 20s with internal retries turns a 29 minute hang into a ~1 minute failure, and that matters beyond speed: the wall-clock kill is what orphans the dpkg lock, so an apt that gives up on its own is one we never have to kill. Not pinning a mirror. The evidence does not support it: in one stall Azure was dead and the public archive hung, in another Azure was serving fine and the transfer stalled at a 13.6 MB package. Neither is reliably better, and the mirrorlist failover is already the right mechanism. It just needed to be allowed to give up. The written config is validated against apt itself (apt-config dump round-trips all four options), and the helper's retry, timeout, giving up, recovery and no-command paths are exercised end to end. * Wait on all four apt locks, and stop racing a slow mirror Two bugs, both mine, both caught by CI on this branch. 1. apt-get update takes /var/lib/apt/lists/lock and nothing else. The helper waited on /var/lib/dpkg/lock-frontend only, so after an attempt was killed mid-update the wait saw a free lock, retried immediately, and produced E: Could not get lock /var/lib/apt/lists/lock. It is held by process 2420 (apt-get) twice in under two seconds. Three attempts, one real one -- exactly the 'a retry that cannot succeed is worse than none' failure the header warns about, in the one case it was written for. It now waits on all four: dpkg's two, lists, and archives. 2. 150s per attempt was racing the mirror, not bounding it. The mirror was degraded rather than dead, so every attempt got killed at the same point and none finished. Two attempts of 360s instead of three of 150: the bound exists to stop an infinite hang, not to beat a slow server. studio-update-smoke's job budget goes 15 to 25 to fit the new worst case; the guard checks that arithmetic. * Ask the image's package lists first, and refresh only if they miss The mirror outage is still live. From this branch's own GGUF smoke: 06:55:33 Ign:4 http://azure.archive.ubuntu.com/ubuntu noble-backports InRelease 06:55:35 Get:5 https://archive.ubuntu.com/ubuntu noble-security InRelease [126 kB] 07:01:11 (nothing, for 5m36s, then the attempt's cap) Azure is unreachable and the public archive it fails over to is not provisioned for this fleet. Acquire::http::Timeout does not save us there: it is an idle timeout, and a server dribbling a byte every few seconds never trips it, so the wall-clock bound is what ends the attempt. The bound is working as intended -- the step now fails in 14 minutes with the reason printed, where before it spent 30 and was reported as "cancelled" with nothing said. But a bounded failure is still a failure, and the operation that fails is one we mostly do not need. `apt-get update` refreshes every index for every suite. The runner image already ships populated lists, and these steps install a handful of ordinary packages. So try the install first, and refresh only when it misses: apt-get install -y X || { apt-get update && apt-get install -y X; } In the common case the slowest and most failure-prone apt operation is never performed at all. When the image's lists really are too stale -- the "Unable to locate package" case the runner-images maintainers warn about -- the update still runs and the install is retried, so nothing is traded away. Not applied to clean-machine-install-ci: a fresh WSL image genuinely has no lists, so install-first would always miss, and a bare machine is that leg's whole premise. The three outcomes were checked as shell rather than reasoned about: resolvable (update never reached, rc=0), not resolvable (update runs, rc=0), and both failing (rc=1, so the failure still propagates under the `bash -e` GitHub runs steps with).	2026-08-19 00:19:50 -07:00
Daniel Han	cabed07f95	Bound every apt step in CI, and fix the retry that never ran (#9256) ... * Give apt one bounded, retrying entry point, and fix the retry that never ran Two problems, one of them mine. The retry I added to the Playwright install last round was dead on arrival. GitHub runs `run:` blocks as `bash -e`, and I wrote the loop as timeout --signal=TERM --kill-after=30 480 python -m playwright "$@" rc=$? A bare failing command under -e aborts the step then and there, so the second attempt never ran. The step exited 124 having printed none of its own warnings -- indistinguishable from the unbounded stall it was meant to replace, which is exactly how it read on #9251's indicator and banner shards, both dying at 8m05s. The previous version used `if timeout ...; then` and was exempt from -e; changing it to read the exit code is what broke it. Reproduced both forms under `bash -e`. The second is broader. `Linux deps` is a bare `apt-get update && apt-get install` with no bound at all, and on #9251 it sat on the Azure mirror for 28 minutes. Of 24 apt-invoking steps across 13 workflows, 2 were bounded. An unbounded apt step is not slow, it is silent: it spends the job's whole budget, GitHub scores the result as "cancelled" rather than a failure, prints no reason, and skips every step after it. .github/scripts/retry-with-apt-lock.sh is now the single definition: per-attempt timeout, retry, dpkg-lock wait between attempts, and exit codes reported honestly (124/137 as a stall, anything else as the status it actually returned). It carries no `set -e` and uses `|| rc=$?`, so it works whatever the caller set. Six paths exercised locally, including the regression above: called from a `bash -e` step, both attempts still run. Sizing respects what this file already learned. The note on ui-smoke's timeout-minutes records that cutting the JOB budget to 20 turned this same stall from a slow pass into a red build, because one attempt had nowhere to go. The retry is what changes that, not a shorter wait: 180s is ~14x the 13 seconds `Linux deps` takes healthy, so only a mirror that is genuinely gone burns all three attempts. Every step's worst case fits inside its own bound and every step bound sits under the job's 30, which stays the final backstop and is deliberately not tightened. Listed in the workflow's paths filter, since both apt steps now execute it. The other 20 unbounded apt steps are left alone: every stall observed so far has been in this workflow, and converting release workflows unreviewed is a worse trade than leaving them until one of them actually stalls. * Bound every apt step in CI, not just the two that were noticed Three jobs were lost to an unbounded apt step in one day, each in a different workflow and each reported as "cancelled" with no reason: Chat UI Tests (chat) at 30m18s, Frontend build at 16m38s inside playwright install --with-deps, and Source lint at 5m02s on a job whose whole budget was 5 minutes. None of the three said anything about its actual subject, and the last was noticed only because someone happened to open a job that said "cancelled". Routes the remaining twelve on-runner apt steps through the shared retry helper, gives each an explicit step timeout, and adds a guard so the next one cannot be added without one. update and install now go as a single unit: retrying the install after a stalled update just re-reads the same broken package list. playwright install --with-deps counts as an apt step, because that is what it is. Leaving it out by name is what cost the Frontend build job. Job budgets were raised where the step's bounded worst case no longer fit: source-lint 5 to 20, api-smoke 12 to 20, frontend build 20 to 40. A job timeout is a backstop for the unforeseen; using it as the bound on a known-flaky step is what turns a diagnosable failure into a silent one. Not converted, with reasons recorded in the guard: clean-machine-install-ci runs apt inside bare containers and WSL with no checkout and deliberately no sudo, and the two steps where apt is the assertion rather than the setup. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 23:18:35 -07:00
Daniel Han	8b4a5ee07d	Count a capped overlay stack by what it reads, not by how the cap is spelled (#9253) ... Three tests assert every bottom-right overlay stack caps itself, and counted the literal `maxHeight: stack.maxHeight` in provider.tsx to do it. #9246 keeps the cap but wraps it -- `maxHeight: railMaxHeight(stack.maxHeight)`, which is the same value plus a constant gutter so the rail's overflow clip stops slicing the shadow off its bottom card -- and all three went red against a PR that had not removed a single cap. The tests are right about what they care about and wrong about how they check it. This file already made the same mistake once and fixed it the same way; the comment two lines below one of these assertions says so: Counted by the layer they sit on, not by a literal z-index: the overlay rail reads its depth from Z_LAYER now. _capped_stacks matches the cap being DERIVED from stack.maxHeight, through an optional wrapping call. That still fails on everything the count was protecting, verified by mutation: the cap dropped entirely, the cap hardcoded to a number so it no longer tracks the measurement, and only one of the two stacks capped -- the exact bug the "== stacks" comparison exists to catch. #9246's wrapped form passes, and its branch goes from 3 failures to 187 passed with this test alone swapped in. Nothing about the assertions' subject changes. A cap that stops reading stack.maxHeight still fails.	2026-08-18 22:03:10 -07:00
Daniel Han	567b3b7f99	Allowlist huggingface-hub's http_backoff loop so Security audit stops failing (#9252) ... * Allowlist huggingface-hub's http_backoff loop so Security audit stops failing Security audit has been red on every main commit since fc325f431, on all three pip scan-packages legs, with a single un-baselined CRITICAL: C2 polling/beaconing loop detected huggingface-hub huggingface_hub/utils/_http.py L461: while True: sha256:b087631... No repo commit caused it. fc325f431 changed workflows and one test file, nothing that resolves a dependency. What moved is upstream: the resolved huggingface-hub came off the 0.x line, and 1.26.1, 1.27.0 and 1.28.0 all carry this loop while 0.36.2 does not. Reproduced locally against each of those versions, and the sha256 matches CI's byte for byte. The code is `http_backoff`: it counts nb_tries against max_retries, sleeps with exponential backoff between attempts, and raises once the budget is spent. A bounded retry, not a beacon. RE_C2_POLLING is `while True .* sleep .* requests\.` under re.DOTALL, which cannot tell those apart, so the file is allowlisted rather than the check weakened -- consistent with the 51 CRITICALs already reviewed this way, four of them this same check in this same package. Added with the scanner's own --write-baseline so the evidence_hash is computed by the code that will match it, then merged as a single entry rather than by regenerating: a full rewrite re-sorts the file and turns a one-entry review into a 731-line diff. Nothing else added or removed, asserted on both directions of the key set. Worth recording: this file now holds FOUR entries for this check, at L298, L461, L462 and L461 again, one per revision of the loop huggingface-hub has shipped. That is the mechanism working -- the key is digest-pinned, so every edit reopens the finding for review -- but it means a hub release touching those thirty lines turns Security audit red again. The new test says so, so the next one is read as upstream drift rather than a break. test_the_hf_backoff_suppression_is_narrow guards the part that actually matters about silencing a CRITICAL in a file that already speaks HTTP: every entry stays pinned to reviewed code, none are duplicated, and a beaconing loop appended to the same file under the same check produces a different key rather than inheriting the suppression. Three mutations checked red (drop the entries, strip the sha256 pin, duplicate an entry). Verified: scan of huggingface-hub 1.28.0 goes 1 CRITICAL to 0, exit 1 to 0. tests/security/test_scan_packages.py 123 passed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 22:02:31 -07:00
Daniel Han	d9e5c059c8	Print what the server said when an inference smoke request 4xx's (#9202) ... * Print what the server said when an inference smoke request 4xx's broken for three main runs, and the only thing CI printed was urllib.error.HTTPError: HTTP Error 400: Bad Request The response body carries llama-server's own explanation and it went out unread, so the cause had to be reconstructed by hand from the workflow source. #9155 fixed the regression; this stops the next one costing the same dig. The HTTPError branch of every request helper in the three inference smoke workflows (nine sites) now prints the status, the reason and the body before re-raising. The read itself is guarded, because a truncated or already-consumed body must not replace the real status with a confusing one, and the original error still propagates: reporting is not tolerating. The tenth site, the tool-probe seed loop in studio-inference-smoke.yml, is a caller rather than a helper. post_sse has already printed the body by the time it re-raises there. tests/studio/test_inference_smoke_http_diagnostics.py parses the Python actually embedded in the workflows with ast rather than matching text, so a rewrite that keeps the behaviour keeps passing. It also asserts every embedded probe parses at all, which nothing else did: the heredocs are shell text inside YAML, invisible to every linter in the repo. Seven mutations checked red (revert the change, drop the re-raise, drop the print, drop the status code, unguard the read, remove the handler, break the syntax). Listed in workflow-trigger-lint because it reads workflow files, so the edit that breaks it is workflow-only and no paths filter would collect it. Confirmed by removing the line and watching test_workflow_guards_run_unfiltered name it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Collapse the split f-string the formatter left behind --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 21:02:07 -07:00
Daniel Han	566272b721	Run the Mac GGUF inference phases in the Mac UI job, not on a second runner (#9213) ... * Run the Mac GGUF inference phases in the Mac UI job, not on a second runner studio-mac-inference-smoke.yml did the same checkout, setup-node, setup-python, `install.sh --local --no-torch` and assert-llama-loads.sh as studio-mac-ui-smoke, and primed hf-cache from the same key with the same GGUF. A studio/** PR paid two macOS slots to install Unsloth twice on two Macs and then test two surfaces of it. Slots are the point, not seconds. Over the last 8 green main runs of each, the UI job executed 1154s behind a 17438s queue and the inference job 402s behind a 13252s queue. Concurrent macOS jobs are capped at 5 account-wide and that pool is shared with unslothai/unsloth-zoo, so a slot returned shortens the queue for everything else on it. Sequential execution costs about 400s of wall clock, which is under 4% of what the second queue cost. This is the fourth workflow folded into that job, after studio-mac-api-smoke and studio-mac-update-smoke. Placement is load-bearing in both directions. The absorbed phases go after the UI and API phases because `Phase N environment` writes GGUF_REPO, GGUF_VARIANT, GGUF_FILE and STUDIO_PORT to $GITHUB_ENV and phase 3 points them at gemma-4-E2B; $GITHUB_ENV outranks the job-level `env:` for every later step, so running them first would hand the UI phases a different model than the one they declare. They go before the update phase because that phase uninstalls and asserts the machine is clean, which is the teardown for the whole job. Four things the move would otherwise have broken silently: - Phase 1 carried no `if:` at all. Under the install that implicit success() meant "the install worked"; under the UI phases it means "and every Playwright test passed", so one flaky browser run would drop all of phase 1 as a skip. Phase 1's steps are now gated and chained the way phases 2 and 3 already were. - Phase 1 booted with boot-studio-api-only.sh's defaults, logs/studio.log and STUDIO_PID. logs/studio.log is the chat UI phase's server log and the artifact upload publishes it, so an absorbed phase 1 would erase the evidence of a chat UI failure that had already happened. It now uses logs/studio_chat_api.log and STUDIO_CHAT_PID. - UNSLOTH_ALLOW_HOST_OFFLOAD lived on the absorbed job's env and does not travel with the steps. Without it the load returns HTTP 400 several layers from the cause. It is back at job level, with the paravirtual-Metal explanation, and test_mac_host_offload_optin.py is what caught the omission. - Phase 3 bound 18899, which the UI job's WebKit indicator run also binds. The phases are sequential and each kills its server, so they would not overlap today, but "would not overlap" is a property of step order. Phase 3 is now on 18892. Phase 1's own HF restore/prime/save block is not carried over: it used the identical cache key for the identical GGUF this job already primes before the install, so it would restore over a cache it is sitting on and race its own save. The trigger gains .github/scripts/studio_smoke/multi_turn_chat.py, the only path the absorbed workflow watched that this one did not already cover. Timeout goes 65 to 100, against a measured 1154s + 483s worst case plus three cold downloads. tests/studio/test_mac_bundled_job_phases.py guards the four phase-isolation properties the bundle now depends on: no port and no server log shared by two phases, no absorbed step without an explicit `if:`, and the uninstall phase last. The log scan resolves boot-studio-api-only.sh's default from the script, because the collision that actually occurred was two phases both omitting `--log`, which a text scan cannot see. Six mutations checked red, including reintroducing each of the four defects above. The check name changes from "GGUF inference smoke (API, tools, vision)" on Mac to "Chat UI, API, Update and GGUF inference". No branch protection rule names it: required_status_checks.contexts is empty on this repo. scripts/build_mac_absorb.py in the workspace reproduces the splice from origin/main; it is not checked in. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop the permission-level reloads racing the composer mount studio-mac-ui-smoke went red at 35672fc9b and again at bfcaea465, both times on waiting for locator("button[aria-label=\"Permission level for tool calls\"]:visible") with green runs on either side. The permission block reloads the page four times with `wait_until="domcontentloaded"` and then asserts the pill visible on the default 5s expect timeout. DOMContentLoaded fires long before React has mounted the composer, and on a 3-core macOS runner that gap is regularly wider than 5s. The composer-mount step at the top of the script already settles the network before waiting, and its comment says why: "on macos-14 racing straight into wait_for() either burns the timeout or crashes the renderer mid-mount". The permission block never got the same treatment. It does now, through one helper used at all four sites. This asserts exactly what it asserted before. It just stops asking before the answer can exist. It rides with this PR because this PR is what makes that job the sole carrier of Mac GGUF coverage as well as the UI coverage, so a flake there now costs three more phases than it did. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 21:00:06 -07:00
Daniel Han	df8fb6a03b	Say both load fallbacks when both fire, and repair the two suites that guard it (#9189) ... * Repair the two chat auto-load suites that #9173 left red on main #9173 added a vision-projector fallback to the auto-load success toast. Both of the suites that read that code broke on it, and both were green on the commit immediately before (54b6ca4c3). 72 tests, in two different shapes: 1. tests/studio/test_chat_autoload_failure_gate.py, 71 failures. The harness slices autoLoadSmallestModel verbatim out of chat-adapter.ts and runs it, so anything the slice references must exist in PREAMBLE. mmprojFallbackMessage did not, which is a bare ReferenceError inside the retry loop; the loop catches it and scores it as a failed load, so every scenario fails as a wrong-model assertion. The file's own guard caught this and named the symbol, which is what it was written for after #7699 did the same thing. Stubbed as a function of the reason rather than a copy of the real three-message record. The value only reaches `options.description`, and these scenarios assert on which model loaded, never on toast copy, so copying user-facing strings in here would give them a second home to drift from. 2. tests/studio/test_model_picker_contracts.py, 1 failure. It asserted the literal `description: cpuFallbackReason`, and the mmproj branch went in front of it. The property held; only the spelling moved. Its own comment records this happening once already, when the CPU-fallback branch first appeared, so it now pins the property: the description varies on both fallback reasons and still has an undefined arm for the ordinary path. Scoped to the description EXPRESSION, not the whole helper. `cpuFallbackReason` is also a parameter name in the signature above, so a substring test over the block stays green with the CPU branch deleted outright -- the first cut of this check did exactly that, and mutation caught it. Three mutations verified red: the description no longer driven by any fallback reason, the CPU branch dropped, and the mmproj branch dropped. 4223 passed, 4 skipped. * Say both fallbacks when both fire, not just the projector one Both load paths wrote the toast description as mmprojFallbackReason ? mmprojMessage : cpuFallbackReason ? cpuMessage : undefined so whenever both reasons are set the CPU-fallback sentence is dropped. The user is told "loaded without vision" and never told the model is running on the CPU, which reads as a deliberate, explained degradation rather than an unaccelerated session. The combination is reachable. On a CPU-fallback replay llama_cpp.py preserves _cpu_fallback_reason (it clears it only when not _replaying_cpu_fallback) and resets _mmproj_fallback_reason so the projector can fail again inside that same launch. A low VRAM machine whose Vulkan backend crashed is exactly where the projector then falls back too. loadFallbackNotice() in mmproj-fallback.ts is now the single composition of the title suffix, the description and the degraded flag, and both call sites delegate to it. CPU_FALLBACK_MESSAGE moves there as well, so the two paths cannot describe the same condition differently again. Tests: four combined-case cases in mmproj-fallback.test.ts, verified red against the shipped nested ternary. test_model_picker_contracts.py now asserts the call sites delegate and pass both reasons rather than matching the old inline ternary, and test_chat_autoload_failure_gate.py's stub mirrors the composition so a call site dropping a reason stays detectable. * Repin the CPU-fallback toast test to behaviour, and unbreak the queued-capabilities test Two frontend suites were red. auto-load-cpu-fallback-toast.test.ts matched the warn-vs-success choice and the message text as substrings of showAutoLoadSuccess. Both moved into loadFallbackNotice, which is the single definition the explicit-load path now shares, so the match went stale. Matching the inline form again would go red on a refactor that changes nothing a user can see, and would stay green if only one of the two load paths kept the behaviour. It now calls loadFallbackNotice and asserts the verdict, and separately asserts the call site delegates to it. queued-model-capabilities.test.ts was red on main before this branch. #9173 added `import { isTextOnlyMmprojFallback } from "./mmproj-fallback"` to image-input-support.ts, which the test imports statically. Extensionless is the right form -- 2314 of the 2367 relative imports under src/ are written that way, and vite and tsconfig's "bundler" mode resolve them -- but the bare node loader does not, and a static import resolves before any registration can run. The test now registers the bundler resolver and imports dynamically, which is what mmproj-fallback.test.ts already does for the same module. Both files reformatted by biome; regex literals hoisted out of the test bodies for useTopLevelRegex. Full frontend suite: 3734 pass, 0 fail. typecheck clean. The 4 biome errors in chat-adapter.ts and use-chat-model-runtime.ts are byte-identical on origin/main. * Order the split-axis abort against the mmproj strip, not against its argument test_tensor_split_abort_raises_early_to_layer_fallback has been red on main since #9173, which renamed the text-only strip's argument from _last_spawn_cmd to _vision_gpu_cmd. That rename is right: the strip should read the vision GPU command rather than whatever was spawned last, and #9173 refreshes _last_spawn_cmd from the result immediately after. The test was pinned to the old argument name, so a rename with no behavioural content took it down. The failure also misreported itself. `assert raise_idx < src.find(needle)` reads as an ordering check but is two claims at once, and when the landmark is gone it fails with "assert 249423 < -1" -- which says the ordering broke, when what happened is that the landmark moved. Each landmark is now required to exist before it is ordered, and says so. The strip is matched on the call rather than on what is passed to it. What this test is about is that the abort raises BEFORE the projector is discarded (#6659); which command the strip reads from is that code's own business. Checked both ways: removing the strip call from load_model goes red with a message naming the missing landmark, and renaming the argument again stays green. * Name the endpoints when the heavy-thread harness catches a stray request The harness records every /api/ URL issued during a measured action, then keeps only the count, so the failure reads let 2 /api/ requests reach the network during the measured actions and stops there. It says an interaction paid for a round trip without saying which one, and the reader has to bisect the frontend to learn what the harness already knew and discarded. It now reports the endpoints. Deduplicated and capped at eight, because the case this instrument exists to catch is a request issued once per message, which would otherwise print hundreds of copies of one line. This is why it surfaced now: the step has not run on main since #9173, whose unit test break fails earlier in the same job and short-circuits it. It was last green at 54b6ca4c3. With the unit tests repaired on this branch the job reaches the step again, and the first thing it needed to say was the one thing it did not. * Skip the playwright harness tests on the module they need, not the package Five tests guard themselves with `pytest.importorskip("playwright")` and then import a harness that does `from playwright.sync_api import Page`. On the Repo tests (CPU) runner the top-level name resolves as a namespace directory with no sync_api inside it, so the guard passes and the import dies with ImportError: cannot import name 'Page' from 'playwright.sync_api' (unknown location) A skip condition reported as a failure, on every branch, for as long as that runner stays that way. It is red on #9202 and #9213 too, neither of which touches any of this. One of the two files already said what the guard was really for: "importing a harness pulls in playwright.sync_api". It now checks that. * Stop the fork-count store asking the server about threads it has never seen Two fixes, both found by the heavy-thread smoke once it could name what it caught. The smoke reported "let 2 /api/ requests reach the network during the measured actions" and, with the endpoints now printed, they were POST /api/chat/threads/__LOCALID_lsQbsDZ/forks A `__LOCALID_` thread has no server record, so that request can only 404, and getThreadForkCounts already maps 404 to the empty map the entry starts as. It is a round trip whose answer is known before it is sent. Not a rounding error. A new chat is in exactly that state, and this store refreshes on CHAT_HISTORY_UPDATED_EVENT, which fires once per streaming chunk, so the first reply in a new chat paid one useless request per debounce window for as long as it streamed. #8992 added the store to stop the chat getting slower as a thread fills; excluding threads the server has never seen is the same intent. thread-ids.ts already had the predicate. Two tests: a local thread must not fetch on subscribe or on a burst of history events, and a saved thread on screen beside it must still refresh -- the guard has to be per thread, not a global off switch. They import the real isAssistantLocalThreadId rather than restating the prefix, so the rule under test cannot drift from the app's. Both go red with the guard removed. Second fix, same job: the playwright skip guard. The previous commit moved it from "playwright" to "playwright.sync_api" and it still failed, because sync_api resolves as a namespace package on that runner too. Only the symbol the harnesses import distinguishes a usable install, so the guard now checks for Page the way the harness does. Verified both ways against a Page-less sync_api: it skips, and it still proceeds when Page is there. Frontend suite 3789 pass, typecheck clean, tests/studio 4237 pass. * Bound and retry the Playwright browser install so a stall is not a silent 30 minutes This step stalls. Three times in one day it sat in apt's download loop until the job's 30-minute timeout killed it, while the sibling shards finished the whole job in 4 to 9 minutes. Twice on #9202 and once on #9189, always the same step. The cost is out of proportion to the cause. GitHub scores a job timeout as "cancelled" rather than a failure, prints no reason, and skips every step after it, so the chat shard reported nothing about the chat surface for what both times turned out to be an infrastructure hiccup that cleared on a plain re-run of the same commit. A per-attempt `timeout` turns the stall into a failure instead of a silent wait, and the retry is what actually recovers. The healthy time is about 2 minutes, so 8 per attempt is 4x headroom and a merely slow mirror will not trip it. timeout-minutes bounds the pair in case `timeout` is outlived by an unkillable child. Same reasoning, and the same wording, as the bounded prime-hf step in studio-mac-ui-smoke.yml. Both install steps in this file, since ui-smoke and ui-indicator run the identical command. Left alone on mac and windows: neither passes --with-deps, so neither has the apt phase this is about, and neither has been observed to stall. * Make the Playwright install retry able to actually recover The bound added in the previous commit worked: the stall became an 8m37s step FAILURE with a complete log instead of a silent 30-minute cancellation, and the log named the cause on the first try. It also showed the retry could not work. playwright shells out to apt-get as root, so terminating the python parent leaves that child alive holding the lock, and attempt 2 died two seconds later with E: Could not get lock /var/lib/dpkg/lock-frontend. It is held by process 4578 (apt-get) A retry that cannot succeed is worse than no retry: it buries the real reason under a second, different failure. Attempt 2 now waits for the lock to clear, up to two minutes, and only then takes it -- the holder is our own orphan and the runner is a throwaway. Two smaller things the same log exposed. --kill-after was missing, so a process that ignores SIGTERM would have been waited on forever inside the step bound. And the warning said "did not finish within 8 minutes" about a two-second exit, which sends the next reader looking for a stall that never happened; it now separates timeout's 124/137 from playwright refusing outright, and prints the status. timeout-minutes 18 to 22 to cover two 8-minute attempts plus the lock wait, still inside the job's 30.	2026-08-18 20:58:30 -07:00
Daniel Han	b0843382d2	Studio: make the chat thread stop getting slower as it fills (#8992) ... * Fix two module resolution failures in the frontend test suite * Fix the third Windows-only path failure in the frontend test suite * Measure how the chat thread's interaction cost grows with message count Studio's chat UI is reported as sluggish on Windows 11 and worsening as the thread fills, while token generation is unaffected. That shape says the cost is per-message renderer work, so the thing to establish first is the curve. New smoke page mounts the real Thread against a synthetic local runtime, seeded to N messages, each carrying prose plus one code fence plus one KaTeX block so Streamdown, Shiki and KaTeX all pay their per-message price. No backend, no auth, no router. New harness runs four scripted actions at N in {10, 50, 200, 500} under 6x CDP CPU throttling: one keystroke into the composer, one scroll gesture, one message action menu opened and closed, and one message delete. Each is bracketed by Performance.getMetrics, which separates the two families of cost: work that grows because layout is uncontained lands in LayoutDuration, work that grows because a listener or an export is O(messages) lands in TaskDuration alone. It measures, it does not gate. There are no timing budgets: it prints the table and exits 0 unless the harness itself broke. Budgets belong in a later change, set from numbers taken on real hardware. What it does fail on is measuring nothing, since that is the failure mode that reads as good news: a seed that did not render, a menu that never opened, a delete that deleted nothing, a scroll that did not move, or four columns that do not rise with N. Measured on this tree, N=10 to N=500: menu open+close 1309ms to 33199ms, delete 438ms to 8491ms, keystroke median 86ms to 268ms. Every metric recorded reaches the printed table, and the harness contract test now enforces that mechanically by parsing the recorded keys out of the source and requiring each one in the table. CDP CPU throttling and longtask are Chromium-only, so Firefox and WebKit runs of this file are correctness checks and not performance ones. * Stop the thread-weight harness charging its own cost to the app A review of the first commit found four ways the measurement could look clean while reporting something other than the app, and two of them were forging part of the curve. All were reproduced before being fixed. The local runtime does have a remote id. It synthesises `__LOCALID_...`, which is truthy, so the per-message fork-count GET fires after all: seeding 20 messages issued 10 requests, against a comment claiming nothing reached the network. They were being answered by a Playwright route handler, so each one paused the renderer for a round trip to another process, once per assistant message. The page now answers them itself, before anything mounts, and the harness fails if a single request escapes during a measured action. Closing the menu was timed from after the Escape dispatch. Radix dismisses synchronously inside it, so the layer teardown, focus restore and re-render -- the O(messages) fan-out this issue is about -- were excluded from the number meant to capture them. Every timing carries a ~33ms floor, since a double rAF cannot resolve faster than two vsync intervals, and CPU throttling does not move it. An action that never happened therefore reported ~33ms, which reads as a plausible measurement rather than as a failure. The floor is now measured per N, printed, subtracted before every growth ratio, and a keystroke at or under it fails the run. The keystroke check read the DOM value back, which is what the harness itself wrote. It now compares against the runtime's own composer state, so a keystroke that reaches the textarea but not React is caught. Also removed from the timed regions: a per-frame document-wide querySelector in both poll loops, replaced by a MutationObserver flag and an isConnected check; a counts() call per frame in the seed gate; an animated scrollIntoView still in flight when the menu window opened; and a console warning per action-bar render, by giving the page the router its useNavigate calls expect. Long tasks are now read after a yield, since the observer delivers on a later task and the tail entry was being dropped. Corrected curve, N=10 to N=500, floors removed where they apply: menu open+close 1021ms to 33591ms, delete 297ms to 8563ms, keystroke 48ms to 283ms, scroll worst frame 5ms to 126ms. Layout stays flat and tiny throughout; the growth is in style recalc and task time. * Stop mounting the assistant action bar for every message At rest the full assistant action bar was mounted under every assistant message. Each one carries around eight tooltips, and every tooltip holds a useSyncExternalStore subscription to the shared modal-layer store, which re-walks its ancestors reading style.pointerEvents whenever Radix puts the body on the modal layer. A 500-message thread therefore mounted 250 bars and 1503 tooltip triggers, and every menu open fanned out across all of them. autohide unmounts rather than hides (ActionBarRoot returns null on the hidden status), so passing it removes the nodes and the subscriptions together. The user bar has always done this. Not unconditionally "always", though: this bar carries the only Stop reading control, which is why it already passes hideWhenRunning={!speaking} and why DeleteMessageButton guards the same case. With "always", moving the pointer off a message being read aloud would take that control away. At most one message speaks at a time, so exempting it costs nothing. Measured with tests/studio/playwright_thread_weight.py at 6x CPU throttle, before -> after, at 500 messages: action bars 250 -> 0 tooltip triggers 1503 -> 3 DOM nodes 56332 -> 41082 delete ms 8595.8 -> 3642.7 scroll worst frame 159.5 -> 87.3 menu open+close ms 33624.5 -> 25279.3 keystroke median ms 316.2 -> 241.0 Note what did not move: menu style recalc, 23725.8 -> 22567.7 ms. A 27% cut in DOM buys 5% there, so the bar is not what makes that number grow. It is the document-wide invalidation from Radix writing pointer-events onto the body, and it is still the dominant cost at large N. The index.css comment is corrected in passing: it justified forcing content-visibility: visible on every code block with "thread length is bounded", which is the assumption this issue disproves. The rule is kept for the flicker it was really fixing. * Studio chat: one fork-count subscription per thread, not one per message The fork badge registered its own CHAT_HISTORY_UPDATED_EVENT listener and issued its own GET, and it is mounted once per message. A delete on a 200-message thread therefore fired 200 requests before anything could repaint, and streaming raises that event once per chunk. Badges now share one debounced subscription per thread and one request that returns every fork count of that thread, so the cost is flat in thread length. * Studio chat: derive research-message ownership once per thread revision useOwnsResearchMessage exported the whole thread from inside a per-message render body, so one render pass over N messages exported N times and inspected N*N items. Streaming re-renders the thread once per chunk, so that pass is hot. The answer is a property of the thread revision, so derive it once for the message list every message in the pass already shares. Measured on a synthetic thread: 200 exports and 0.80ms per pass becomes 1 export and 0.014ms; at 1000 messages 16.4ms becomes 0.02ms. * Studio chat: stop deep-cloning the thread on every delete and every save exportedItemToRecord ran JSON.parse(JSON.stringify(...)) over every message's content and attachments on its way to a PUT that serializes the same records again, and syncExportedRepositoryToBackend ensured the thread row that syncStoredChatMessages already ensures, so every save paid for GET /threads/{id} twice. The parts are replaced rather than mutated, so a copy of the list is snapshot enough and the bytes on the wire are identical (asserted in the new test). Measured on a 200-message thread of ~4KB messages: the record step drops from 1.17ms to 0.015ms, and a delete makes one thread-row read instead of two. * Studio chat: open the message action menu non-modally A modal Radix menu writes pointer-events:none on <body>. That is an inherited property, so every open and close invalidates style for the whole document, and on a long thread the recalc is the bulk of the cost. Non-modal never writes it. Also teaches the harness the difference between a cost that was removed and a page that never mounted one, so the after-tree does not read as broken. * Pin the message action menu to the non-modal layer * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Drop the #8980 content this branch no longer needs * Print the hovered trigger count and keep the modal layer load-bearing in the verdict The contract test caught both: a metric recorded but not printed, and the verdict no longer reading body_pointer_events_while_open after the non-modal fix removed the old check. The layer is now compared ACROSS N instead, since either mode is legitimate but mixing them means the columns measure different mechanisms. * Stop the rapid-submit settle wait measuring the action bar instead of the reply This branch autohides the assistant action bar, and that turned the last wait of the rapid-submit step into a 7-of-7 failure on Windows CI. The wait is not what the step proves, and the clause that broke was not measuring what it claimed. innerText of a [data-role=assistant] root spans the whole subtree, and the action bar sits inside it, so 'every reply has non-empty innerText' was satisfied by button labels regardless of what the model returned. Instrumented at that point on the CI runners, two runs on this branch's merge base read: content=[0, 0] innerText=[73, 73] clause held, BOTH replies empty content=[0, 19] innerText=[73, 89] clause held, first reply empty gemma-3-270m-it answers 'Reply with exactly: rapid-first' with an empty completion in 3 of 8 sampled runs, on the merge base as much as here, and the clause held every time. So the empty reply is the model, is pre-existing, and was simply masked. With the bar autohidden the subtree is content only, and the same empty completion now fails. Dropped rather than repointed at the content element: an empty completion is the model's behaviour, so a content assertion would be flakier than what it replaces. What is left is exactly the settle this wait is for, two bubbles, nothing streaming, nothing queued. The behaviour the step exists to prove, that a 100 ms follow-up queues behind a held first turn, is state.queueSeen above and is untouched. Test-only. No Studio code changes, so the menu open+close and nodes-at-rest wins are unaffected. Verified on Windows CI at this branch's head: 4 of 4 Chat UI Tests jobs green with this change, against 7 of 7 red without it. * Validate the delete measurement at every size, not just the last * Keep the thread's fork counts across the autohidden badges * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Exempt the thread-weight harness from the CI-coverage check * Keep the newest reply's action bar in the tab order * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Load the crypto polyfill on the thread-weight smoke page The page this branch adds was the only one in studio/frontend without <script src="/crypto-boot.js"></script> in its head, so crypto-uuid-boot.test.ts fails with "smoke-thread-weight.html must load /crypto-boot.js". The rule is not cosmetic: the polyfill has to run before the module entry or crypto.randomUUID is missing on the older WebViews Desktop embeds, and a smoke page without it measures a page that differs from production in the one respect the harness is meant to hold constant. Reproduced on this branch and verified: the named assertion fails before the change and the file's four tests pass after it. * Debounce the fork count refresh instead of throttling it onHistoryUpdated returned while a timer existed, which is a leading edge throttle rather than a debounce. Streaming raises the history event once per chunk, so the timer expired mid stream and the next chunk armed another one, costing a whole thread fork count fetch every 300ms for as long as the reply ran. Fork counts cannot change during generation, so all of those were waste. Clear and reschedule on every event, as the sidebar refresh already does. The existing burst test fired all 20 events inside one window, where a throttle and a debounce behave identically, which is why this survived. The new case spreads the events across the window like a real stream: 20 chunks cost 10 refetches before this change and none after, with one refresh in the quiet window that follows. * Reveal the action bar on focus, not only on hover Unmounting the bar on every message but the newest took its controls out of the tab order, and there was no non-pointer way to bring them back, so Copy, Edit, Refresh, Delete, Read aloud and More were unreachable by keyboard or screen reader on every older reply. Deferred rendering is fine while the user can still ask for what is deferred, and tabbing is asking. Measured on the thread weight smoke page at 20 messages, older reply: the accessibility tree exposed no bar controls before this change and still none after focus entered the message; it now gains Copy, Refresh, Delete, Edit response, Read aloud and More. Three tabs from an older reply used to walk its two code fence buttons and step to the next message without ever entering a bar; the same walk now lands on the bar's Copy. The library has no focus path, so this drives its own isHovering flag from focus within the message root. Two writers share that flag, so a pointer leaving while focus is inside re-asserts it from a listener registered in an effect, which runs after the primitive's own mouseleave in the same dispatch and so never renders the intermediate false that would unmount the element holding focus. The clear is a one frame watchdog reading activeElement rather than a relatedTarget test, because relatedTarget is null both for browser chrome and for a portal, and no focusout fires at all when the focused element is removed, which is how the menu closes. At rest this stays at one mounted bar of ten, and at one again after a focus round trip, so the weight this branch is here to remove is unchanged: 1950 DOM nodes and 9 tooltip triggers, the same as before. * Give a plain prose reply a way into the tab order The focus reveal only fires once focus is inside the message, and a reply whose body is plain prose contains nothing focusable after autohide unmounts its action bar. The earlier measurement of two focusable controls per message was an artefact of a fixture where every reply carried a code fence, and Streamdown ships one Copy button per fence. Seeded with a prose only reply it is zero, and Tab walks straight past the message into the next reply, so Copy, Edit, Refresh, Delete, Read aloud and More stay unreachable. tabIndex on the message root rather than a visually hidden button: it adds no DOM node, which matters for a branch that exists to cut per message weight, and it draws nothing at rest. The app's own focus-visible rule gives it the same 1px keyboard indicator every other focusable container already has, and focus-visible means a mouse click still draws nothing. Measured at rest, unchanged: one action bar of ten, 1950 DOM nodes, 9 tooltip triggers. Outline is none with neither focus nor hover, and none after a click. The cost is one extra tab stop per assistant message, which is the price of the controls being reachable at all. The fixture gains an opt in plain prose variant so the existing weight measurements keep the exact thread they had. * Bound how long a fork change can wait behind an unrelated stream CHAT_HISTORY_UPDATED_EVENT fires once per streaming chunk, and the fork-count refresh was a pure trailing-edge debounce, so a reply running in a background thread reset the timer on every chunk. Deleting a fork from the sidebar while looking at its parent changes the displayed count, and the refresh was postponed until the unrelated stream went quiet, which on a long or queued run is minutes. That is starvation, not slowness. The event is a bare Event with no detail and six other consumers, so telling fork changes apart from chunks means changing a contract well outside this store. FORK_COUNT_REFRESH_MAX_WAIT_MS bounds the wait inside it instead: a second timer, started by the first event of a burst and deliberately not restarted by the ones after it, races the debounce, and whichever fires first cancels the other. A second timer rather than a Date.now() deadline so it runs off the same clock as the debounce and is testable without a fake Date. 2000ms because the bound costs one whole-thread fetch per window while a stream runs. At the 300ms debounce that is the per-chunk traffic this store exists to remove; at 2000 it is under a sixth of it, and only while something is streaming. The existing continuous-stream test asserted ZERO mid-stream fetches, which is the behaviour the review flagged, so it now measures the price of the bound instead of claiming there is none: it pins the count against both what the ceiling allows and what a leading-edge throttle would have cost, so a regression in either direction is a failure. Four assertions were made to fail on their own broken tree before being kept: the ceiling removed, the ceiling restarted per chunk so it never expires, the losing timer left uncancelled when the other fires, and the ceiling left running past unsubscribe. That last one was vacuous at first, since the entries map is empty after unsubscribe and a leaked timer refreshes nothing; it now subscribes a second thread inside the ceiling window, which is both observable and the case that actually costs a user a request. * Scope the popup lookup to the action bar The watchdog treated any expanded descendant as this message's open menu. Reasoning cards and tool-fallback cards are Radix CollapsibleTriggers and render aria-expanded=true for as long as the reader leaves them open, which is the resting state of a message whose tool output has been expanded. decide() therefore found a popup every frame, rescheduled itself every frame, held focusWithinRef and the synthetic hover set, and left the bar mounted indefinitely, at the cost of a DOM query per frame per such message. Scoped to .aui-assistant-action-bar-root, which is where the trigger the hook has to hand focus back to actually lives. Proving this took three attempts and the first two were wrong, which is worth recording because the failure was in the test rather than in the fix. isHovering has two writers. This hook is one; assistant-ui's own MessagePrimitive.Root mouseleave handler is the other, and it writes false directly. So a phase that reveals the bar by HOVERING and then moves the pointer away sees the bar unmount on both trees, because the library unmounted it. On the broken tree the watchdog genuinely spins forever, and the bar still goes away. An assertion on the mounted bar count cannot attribute that outcome to this branch, and C2 passed on the fixed and broken trees alike. The phase now keeps the pointer off the message entirely and reveals the bar by focus, which the tabIndex on the message root makes possible. With no mouseleave to fire, focus is the only writer and the bar's fate is decided by the watchdog alone. C2 is green on the fixed tree and red under --break widepopup, 1 bar still mounted and held indefinitely. Two guards sit in front of it: one asserts the pointer really is off the message, the other that the bar really was mounted and focused, so the phase fails loudly rather than passing vacuously if either precondition breaks. Also fixes a pre-existing crash the sweep was hiding. Under --break eagerclear the bar is gone by the time A8 runs, and a bare more.focus() on an undefined element threw a Playwright TypeError that killed the process before phases P, B and C ever ran, so the break reported fewer reds than it earns and ended in a traceback rather than a red result. It is more?.focus() now and the trigger's absence is folded into A8's condition, so eagerclear completes and A8 goes red on its own merits. Full sweep: head 14/23, notabindex 19/23, restring 21/23, focusring 22/23, leakflag 21/23, noreassert 22/23, eagerclear 21/23, widepopup 22/23 with C2 the only red. thread.tsx checksummed before and after all eight runs, identical every time. * Reveal the action bar from the backward traversal too The tabIndex on the message root only worked going FORWARD. A container is reached before its own descendants, so Shift+Tab arriving from the message below landed on the last tabbable thing in the message, and with the bar unmounted that is the root, which sits BEFORE the bar in DOM order. Focusing it mounted the controls and the next Shift+Tab then stepped straight past them to the previous message. Copy, Edit, Delete and More were reachable going forward and unreachable going backward, which is worse than being unreachable outright, because the forward pass makes it look solved. A sentinel span after the bar is what makes the backward pass land inside the message: focus stops there, the bar mounts, and the next Shift+Tab goes into the last control rather than out. It is deliberately NOT a focus redirect to that control, which would trap the forward pass in a loop between the last button and the sentinel. It carries no onFocus of its own because React's onFocus is focusin and already bubbles to the root, and no role, because it performs no action; the aria-label is what stops it being an unannounced stop. It DOES cost one DOM node per assistant message, and this branch is about per-message weight, so that is asserted rather than absorbed: the at-rest guard now requires exactly 1950 baseline nodes plus one sentinel per reply and nothing else, measured 1960 for 10 replies, with the sentinel count checked separately so the extra nodes are attributed rather than tolerated. Three assertions, each proven red. nosentinel takes D2 and D3 red while D1 stays green, which is precisely the reported asymmetry: focus still enters the reply, it just skips the bar. D1 says something weaker, so it needs noentry, which removes the sentinel and the tabIndex together and leaves nothing in the message reachable at all; D1, D2 and D3 all go red there. Full sweep, 26 assertions: fixed 26/26, nosentinel 23/26, noentry 18/26, notabindex 24/26, head 15/26, widepopup 25/26 with C2 alone, eagerclear 21/26, noreassert 25/26, leakflag 24/26, restring 24/26, focusring 25/26. thread.tsx checksummed before and after every run, identical throughout. * Draw a focus indicator on the backward reveal sentinel The shared soft-outline rule is :where(div, main, section, aside, ul, ol):focus-visible, which never matched a span, so the sentinel was a real tab stop that drew nothing: Shift+Tab into a message made focus visibly disappear for one stop before the next press reached the action bar. That is a focus-visible failure, not a cosmetic one. The element stays 0x0 and the ring is drawn by outline-offset. Outlines take no part in layout, so the indicator appears without shifting the message, which giving the span dimensions on focus would have done. Still nothing at rest, and :focus-visible means a mouse click draws nothing either. Measured: with keyboard focus the ring spans 14px against the UA default's 2px on a zero-sized span, which is the difference between an indicator and no indicator. D4 asserts the SPAN of the drawn ring rather than merely that an outline style exists, because the broken tree still reports outline-style auto and would satisfy a presence check while showing nothing. Proven red by --break blindsentinel, which removes the indicator rule and leaves the tab stop itself intact, so only D4 fails. * Stop tracking the generated Studio test database .studio-test-root/studio.db is written at test time by tests/studio/install/test_selection_logic.py, which points storage_roots.studio_root at that path. It is a mutable SQLite runtime database, not a fixture: nothing reads it, any test run or Studio start rewrites it and dirties the checkout, a later accidental commit could capture real local chat or settings data, and it puts 221 KB into every clone while exercising nothing. It was not in the tree deliberately. It arrived in the merge commit here because that commit was staged with `git add -A` after running the suite, and no gitignore rule covered the path. Main does not track it. Untracked and ignored, and the file is left on disk since creating it is normal. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com> Co-authored-by: oobabooga <112222186+oobabooga@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@users.noreply.github.com>	2026-08-18 06:19:05 -07:00
Nilay	9d1dcfe58a	Studio: fail fast when the installed llama.cpp prebuilt has no kernels for this GPU (#8841) ... * Studio: fail fast when the installed llama.cpp prebuilt has no kernels for this GPU * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: skip the CUDA SM gate for deliberate CPU-only loads * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: apply the CUDA SM gate at launch placement and fix its remedy * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix CUDA SM gate device order handling * Studio: gate CUDA SM on the oldest compiled arch, not exact SM overlap * Add the OS x GPU matrix and two installer invariants for PR #8841 The ROCm arch gate has test_gpu_arch_gate_os_matrix_7624.py asserting it is inert off its one supported host shape; the CUDA gate had no equivalent, and inertness is exactly what output alone cannot show since a gate that ran and passed returns None like one that never ran. Spy the marker reader and the nvidia-smi probe across [Windows, Linux, WSL, macOS] x [NVIDIA, AMD, CPU-only]. Also pins two properties the design rests on but nothing checked: supported_sms must stay out of expected_install_fingerprint, or every pre-existing install compares stale and reinstalls on upgrade, and a non-CUDA bundle must record an empty list, which is the only reason the call site needs no Vulkan guard. Covers the floor decision (sm_121 PTX cannot JIT down to sm_120, but sm_120 to sm_121 can), unreadable and failing probes, and the visibility masks: a numeric mask without PCI_BUS_ID ordering fails open, an empty one hides everything, and a UUID or MIG mask is dropped so the whole host is weighed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the new code comments for PR #8841 * Shorten the comments added by the CUDA SM gate --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: oobabooga <112222186+oobabooga@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com> Co-authored-by: danielhanchen <unslothai@gmail.com>	2026-08-18 05:40:52 -07:00
Daniel Han	06220da4d2	Run two short jobs as background lanes of Lint CI, not on their own runners (#9176) ... * Run two short jobs as background lanes of Lint CI, not on their own runners Two workflows each held a runner slot on every commit for a few seconds of read-only checking: Unsloth load-orchestrator CI :: test ~33s, its own slot Lockfile supply-chain audit :: audit ~6s, its own slot Both now run as background lanes inside Lint CI, which has no path filter and was already going to occupy a runner on every commit. Three slots become one. Backgrounded rather than appended as steps, so the lanes overlap the ~65s of lint instead of adding to it: ubuntu-latest has four cores and the lint steps are single-threaded. Measured locally on the real step bodies, a 5s lane alongside 5s of foreground work took 7s total, not 10s. Absorbing a narrower-triggered job into an unfiltered one can only reduce the slots a commit takes. The absorbed work now runs on commits that would not have triggered it, on a runner that was going to exist anyway. Three things about this were not obvious and are worth recording: * Each lane needs its own venv. The load-orchestrator lane installs fastapi and friends while the foreground steps use the interpreter Lint CI's own pip install populated, and two concurrent installs into one site-packages is a real race. * The launch has to detach from the step's stdout and stderr. A background child inherits those pipes and the step is not considered finished while a writer holds them: launching a 4s lane took 4.0s before the redirect and 0.0s after. Nothing would have gone red, the overlap would just have silently disappeared. * A missing exit-status file has to be a failure. A lane that never started is otherwise indistinguishable from one that passed. The payloads live in .github/scripts/lane-*.sh, called by both the lane and the standalone workflow, so there is one definition. studio-load-orchestrator-ci.yml keeps workflow_dispatch as an escape hatch; its push trigger had no paths filter at all, so it was running on every commit to main regardless of the four paths its PR trigger listed. lockfile-audit.yml keeps its nightly schedule, which is a different check: it re-reads the lockfiles against advisories published since the last commit, which no commit-triggered run can do. tests/studio/test_absorbed_lanes_still_run.py covers the silent failure modes. Eight mutations verified red, including an uncollected lane, a missing sentinel read as success, and a launch that stops detaching. * Run the lane guard on workflow-only PRs The guard covers work moved off its own runner into a background lane, and every way that regresses -- a lane launched but never collected, a missing exit status read as success, a launch that stops detaching -- is an edit to lint-ci.yml or to one of the two absorbed workflows. No job's paths filter matches a workflow-only change: workflow-trigger-lint enumerates specific test modules, and Backend CI matches tests/** and its own YAML but not the workflow files guarded here. So the suite would first be collected by Backend CI's unfiltered push on main, after the change had merged. That matters more than usual because every failure mode here is silent. Nothing goes red when a lane stops being collected; the absorbed suites just quietly stop being able to fail anything. * Run the workflow guards in one pytest, and run the ten that ran nowhere workflow-trigger-lint is the only job in the repo with no paths filter, which makes it the only job a workflow-only PR is guaranteed to start. Every other job filters on source paths: Backend CI matches studio/**, tests/**, scripts/** and its own YAML, but not arbitrary workflow files. So a guard that reads a workflow file and is not run by this job has a specific silent hole. The edit it exists to reject is by definition a workflow-only edit, and on such a PR it is never collected. It is collected later by Backend CI's unfiltered push on main, after the change has merged. The guard still works, it just stops being able to block anything. Ten modules were in that state at once, including the indicator-parallelism guard, the GGUF phase-independence guard and the Windows small-checks guard, each of which reads a workflow and can only be broken by editing one. Three separate review rounds reported one instance each before the pattern was visible. They now run in ONE pytest invocation rather than a step per module. Measured over these 18 modules: 53.9s as a single call against 300.8s as one each, because this repo's conftest is expensive to import and a step per module pays it every time. That also removes the reason the list stopped growing: adding a guard no longer means adding a step. tests/studio/test_workflow_guards_run_unfiltered.py keeps the list honest, with two exemptions carrying their reason (PIL and a local utils helper, neither of which that job installs). It caught itself on its first run, being a workflow-reading module that was not yet in the list. Three mutations verified red: a guard dropped from the list, the invocation split back into two steps, and the job gaining a paths filter. * Give the consolidated guard job the cores and the budget it now needs Absorbing 12 more guard modules into one step made that step the job, and the job still carried a 5 minute budget sized for the old set. Serial the 21 modules are 209s on a 192-core box, which would have failed on timeout on a slower runner rather than on anything real. So: pytest-xdist, `-n 4`, and 15 minutes. Pinned to 4 rather than `auto` on purpose. ubuntu-latest is a 4-core runner so on CI the two are identical, but `auto` scales to the host and every xdist worker re-imports this repo's expensive conftest. Measured here: `auto` spawned 192 workers and took 327s, worse than running serially, while `-n 4` is 42s. A fixed width behaves the same everywhere and cannot be made pathological by the machine it lands on. 304 tests, identical results serial and parallel.	2026-08-18 05:27:55 -07:00
Daniel Han	54b6ca4c3f	Stop spending the Actions cache budget on caches nothing reads (#9151) ... * Stop spending the Actions cache budget on caches nothing reads The repo holds 50.0 GB of Actions cache against a 10 GB limit, so LRU eviction runs continuously and the entries CI actually depends on are the ones being thrown away. Measured: 28.9 GB 60 entries setup-python pip 58% of the budget, only 18 distinct keys 7.2 GB 8 entries v0-rust 11.8 GB 5 entries the GGUF / HF model caches CI depends on 25.9 GB 214 entries on PR refs, restorable only by re-runs of that same PR Two doors let that happen, and both are silent: nothing goes red when a cache is evicted, CI just re-downloads a 4.6 GB model and everyone assumes that is the cost. setup-python derives one pip key per interpreter from dependency files across the whole repo, so dozens of unrelated jobs share it and race to save under it. The entries measure 666-715 MB. Jobs that only pip-install huggingface_hub, pytest or playwright were paying that for the 0-7s their restore step takes. Those 24 call sites give the cache back; the 9 that install torch/transformers keep it. The Playwright browser caches in studio-ui-smoke.yml used the read-write actions/cache, which saves from its post-step on every ref, at ~470 MB per browser set. They now restore always and save on main only, matching what every model cache in this repo already does. This is the same thrash loop the GGUF caches were fixed for, arriving through a different door: PR misses, downloads, saves its own copy, evicts main's, next PR misses. Windows benefits most. Its model caches are among the evicted, and a Windows cache miss costs about 3x a Linux one. tests/studio/test_cache_budget_discipline.py asserts both doors stay shut, that cold-install lanes are never warmed by a cache, and that every setup-python step still pins an interpreter -- the last because removing cache: 'pip' from an inline-flow mapping by deleting the line takes python-version with it, which this change did once before it was caught. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Correct the cache budget figure: 50 GiB, not GitHub's 10GB default The budget comments across eleven workflows quoted GitHub's 10GB default. This repo's quota is 50 GiB, so the earlier framing (and the 33.3GB / 3.3x-over measurement they carry) understated the headroom and overstated the severity. Re-measured with the correct quota: 49.63 GiB across 258 entries, 99.3% full. The conclusion does not change, only the reason. Eviction runs at the margin, and what fills the budget is redundancy rather than useful payload: 20.74 GiB the SAME key held on several refs (42% of the whole cache) 6.67 GiB 13 copies setup-python ... python-3.13.15-pip-85e247d7... 6.50 GiB 11 copies setup-python ... python-3.11.15-pip-85e247d7... 3.83 GiB 10 copies setup-python ... python-3.12.13-pip-85e247d7... 0.91 GiB 3 copies ms-playwright-Linux-1.62.0-cfw-v1 10.70 GiB 84 entries written and never read again, 10.60 GiB of it setup-python 24.01 GiB 198 entries on PR refs, restorable only by re-runs of that same PR 0.00 GiB entries unread for 7+ days: nothing is idle, the cache is churning Every duplicated key already has a copy on main, which every PR can restore from, so the PR-scoped copies buy no hit rate and evict the copy that does. Worth recording for whoever adds the next cache: the duplication is a SYMPTOM of sitting at quota, not an independent cause. A PR only writes its own copy after missing, and it only misses because main's copy was evicted. Getting back under the quota stops the loop on its own, which is why this change reclaims claimants rather than restructuring the nine jobs that legitimately cache torch. * Scope every pip cache key to what its job actually installs Five of the nine jobs that keep a pip cache had no cache-dependency-path, so setup-python hashed dependency files across the whole repo to build their key. That is the second multiplier behind the 27.05 GiB those caches occupy: 16 distinct keys appeared in a week, because one requirements edit anywhere invalidates every interpreter's entry at once and orphans the old ones at ~700MB each. The four already-scoped jobs (consolidated-tests, studio-backend) show the pattern; it just was not applied consistently. mlx-ci installs from studio/backend/requirements/studio.txt, so it now points there like its siblings. The other four pin their dependencies inline in the workflow (a torch CPU index URL, pinned transformers/trl/peft), so there is no requirements file to name and their key was describing files they never read. For those the workflow file IS the dependency spec, so the key hashes that: it moves when the install actually changes and not otherwise. notebooks-ci's step had to be expanded from the inline-flow form (with: { python-version: '3.12', cache: 'pip' }) to take the new key. The guard test now asserts every allowed pip cache scopes its key, since an unscoped one costs budget quietly rather than failing. * Save the Playwright browsers only when the download succeeded The save ran under a bare always(), so a browser install that failed part-way still stored whatever had landed on disk. The key is pinned to the resolved Playwright version and never rolls over, so every later run would restore that partial tree, report a cache hit, skip the download and run install-deps against engines that are not there. Every UI job fails until someone deletes the entry by hand, and nothing in the log points at the cache. always() stays, so browsers that did download are not thrown away because an unrelated earlier step failed; the install step now carries an id and the save checks its outcome. Both jobs in the workflow had the same condition. * Point the scoped cache keys at the nested checkout, and run the guard on workflow-only PRs Three jobs check the repo out under `unsloth/` because they need a second repo beside it: notebooks-ci api-introspect, and version-compat-ci zoo-imports-under-spoof and grpo-fake-run. The cache-dependency-path added for them was workspace-root-relative, so it matched no file. setup-python treats that as fatal ("No file in ... matched to ..."), not as a reason to skip the cache, so all three jobs would have stopped before installing anything. Prefixed with the checkout directory, and guarded: the new test resolves every cache-dependency-path against its own job's checkout paths. The discipline test is also wired into workflow-trigger-lint, which carries no paths filter. Repo tests (CPU) does collect the file, but studio-backend-ci.yml's paths do not match a workflow-only PR, so the job never starts for exactly the change this guard exists to reject. Collection is not coverage if nothing triggers the workflow that collects it. * Count setup-python's implicit save, and drop a cache path with no cache The save scan only looked for actions/cache steps, so it reported clean while nine jobs wrote a PR-scoped entry on every run. setup-python's cache: input registers a post-step (post: dist/cache-save/index.js in its own action.yml) that saves after the job on whatever ref it ran on, with no condition to gate it. Those nine are now named in PIP_CACHE_JOBS_PENDING_CONVERSION rather than skipped silently, so a tenth joining them fails the check, and the follow-up that converts them to an explicit restore plus main-only save empties the set. lint-ci.yml also kept a cache-dependency-path after its cache: 'pip' was removed. That is inert, since setup-python only reads the path when caching is on, but it reads as a scoped cache key and the next person believes the job is cached. Removed, with the comment rewritten to say why the job has no cache, and a check for the same shape elsewhere. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Read the save condition and the cache path, rather than searching them Two guards in this file passed on text that looked right rather than on the property they claim. The main-only check was a substring search for "refs/heads/main". Three conditions contain that literal while still saving on every PR: a `!=` comparison, an `||` that admits another event, and the check appearing in only one branch of one. It now splits the expression on its top-level `||` and requires a positive `github.ref == 'refs/heads/main'` in every alternative, which is conservative in the safe direction. The splitter is itself tested against nine expressions, including a `||` inside a string. The cache-dependency-path check compared prefixes and skipped root-checkout jobs entirely, so `unsloth/.github/workflows/typo.yml` read as correct and a misspelling in a root-checkout job was never examined. Each entry is now resolved against the checkout it belongs to and globbed against the tree. Paths under a checkout of a DIFFERENT repository are skipped rather than reported: notebooks-ci api-introspect checks out unslothai/notebooks beside this one, and a path under it cannot be resolved here. Both matter because setup-python does not skip a cache it cannot resolve, it fails the job with "No file in ... matched" before anything is installed. All four shapes verified red: the inverted comparison, the widened `||`, a correctly prefixed but misspelled nested path, and a misspelled root path. * Save the pip cache on main only, via a restore/save action pair (#9165) * Save the pip cache on main only, via a restore/save action pair setup-python's built-in cache: 'pip' is the read-write form. It restores in the step and saves from its own post-step on whatever ref the job ran on, and exposes no condition to stop that. An entry written on a pull_request ref can only be restored by re-runs of that same pull request, so it buys no hit rate while competing for the shared 50 GiB budget against the copy on main that every PR can read. Measured on this repo before the change: setup-python entries were 19.45 GiB across 40 entries, 15.49 GiB of it on PR refs, with four interpreter keys duplicated four to six times each for 9.60 GiB of pure waste. None of it shows up as a failure. Over quota, GitHub evicts least-recently-used, so main's copy goes, the next PR misses, downloads and writes its own, and CI simply gets slower. The repo had already diagnosed and fixed this same loop for the GGUF caches and the Playwright browsers; setup-python was left doing it because the built-in cache has no save-gating knob. Splitting restore from save is how you get one. The nine jobs that genuinely install a torch/transformers-class dependency set now use .github/actions/pip-cache-restore and .github/actions/pip-cache-save, with the save gated on refs/heads/main. Defined once as a composite pair rather than inlined nine times, following install-unsloth-local: the gate is the whole point of the change and nine copies of it would drift. The action resolves pip's cache directory with pip cache dir rather than hardcoding a path, so the macOS and Windows call sites work unchanged, and it fails loudly when key-files matches nothing, since an empty hash would silently collapse every job onto one key. The guards move with the architecture. The save scan now reads composite actions too, which is where the save now lives; a job using the built-in cache at all is now a failure rather than an allowlist question; and each of the nine is checked to restore and save as a wired pair. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Point the local action references at the nested checkout `uses: ./...` resolves from GITHUB_WORKSPACE, not from the workflow file, and three jobs check the repo out under `unsloth/` because they need a second repo beside it. The unprefixed path is a directory that does not exist there, so all six references would have failed with "Can't find 'action.yml', 'action.yaml' or 'Dockerfile'" before installing anything. Same root cause as the cache key paths, one level out: those were fixed for these jobs and the action paths were not. The guard now resolves every local action reference against its own job's checkout paths. * Discover pip cache users, rather than only iterating the allowlist Replacing setup-python's built-in `cache: 'pip'` removed the mechanism that found claimants. Every remaining check is parametrized over PIP_CACHE_JOBS, so a new job adding the restore/save pair was never visited: it would get a ~700MB entry with no scoping check, no wiring check and no justification, and this file would stay green. The allowlist stopped being enforced and became a list to iterate. Two checks now read the workflows instead. One rejects any job using either half of the pair without being listed. The other re-applies the heavy-install requirement to the discovered set, so a job that keeps its cache after its torch-class install moves elsewhere is reported rather than grandfathered. Verified red by giving workflow-trigger-lint, which installs nothing heavy, a pip-cache-restore step. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 04:47:22 -07:00
Daniel Han	aa32c1861c	Studio CI: make a failing browser smoke say why, and stop it skipping the build gates (#8983) ... * Fix two module resolution failures in the frontend test suite * Studio CI: make a failing browser smoke say why, and stop it skipping the build gates * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix the third Windows-only path failure in the frontend test suite * Drop the #8980 content this branch no longer needs * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Snapshot the vite tail before printing it, and put the startup bundle gate ahead of the smokes The failure dump iterated the live deque the drain thread is still appending to, while writing each line to stdout, so a vite server that was still talking during the dump raised deque mutated during iteration and dropped the tail in exactly the noisy failure the dump was added for. list() of a deque is atomic; take it first. Startup bundle budget still ran after the browser smokes, and every step carries an implicit success(), so a red smoke skipped it. It only needs dist/, so it moves up with the other build gates. * Tighten comments in the smoke diagnostics changes * Upload the failing smoke's own report, not every report but that one The failure upload globbed logs/playwright-*, which four of the five browser smokes write. playwright_settings_tabs.py writes logs/settings_tabs_report.json and, for the blocked-chunk arm, logs/settings_tabs_blocked_report.json. So when either settings smoke failed the artifact contained the reports of the smokes that had passed and not the one that had just failed, which is the opposite of what this upload is for. Add both names to the upload path, and guard it: the new test reads every logs/ path the wired-up smokes actually write out of their own source and fails if one is not matched by an upload pattern. Shown red on the bare glob first, naming logs/settings_tabs_report.json. * Upload the non-blocking smoke's report on the runs where it is the point The stream-pacing smoke is continue-on-error, which rewrites its CONCLUSION to success while leaving its OUTCOME as failure. The artifact upload was gated on a bare failure(), so on the runs where that smoke was the only thing that failed -- exactly the runs where its report is the whole point -- the upload was skipped and logs/playwright-stream-pacing went nowhere. Give the step an id and OR its raw outcome into the upload condition. Guarded: the new test walks every continue-on-error browser smoke and fails if it has no id, or if its outcome is not named in the upload condition. Shown red two independent ways first, reverting the condition to bare failure() and separately deleting the step id, each naming the step. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com>	2026-08-18 04:42:41 -07:00
Daniel Han	58403dd001	Studio: measure where a heavy thread stalls, across engines and thread size (#9016) ... * Studio: measure where a heavy thread stalls, as a curve over thread content Users report Studio and Desktop going sluggish after long generations with code cells and text. That is a statement about content volume, so the new harness varies characters of thread content rather than message count, and the fixture carries the mix the report names: prose, large code fences, tool calls with collapsible output, code-execution result panes, HTML and canvas artifacts, and inline images. The primary metrics are DOM-observable and wall-clock, because Unsloth Desktop is a Tauri webview and not Chromium. PerformanceObserver accepts type longtask on WebKit 26.5 and Firefox 153 without throwing and then never fires, so support is read from supportedEntryTypes; CDP counters are recorded alongside and labelled Chromium-only. * Settle the highlighter between repetitions and tolerate constant engine chatter Re-opening the thread throws away every highlighted fence, so repetitions 2 and 3 were measuring a thread that was still building itself: on Chromium at 300K the scroll gesture read 667ms on the first repetition and 1100ms on the two after it, and the difference was the re-highlighting. Firefox 153 emits exactly two scroll-anchoring notices per run at every size. A warning count that grows with the thread still fails; a constant one does not, or the harness could never report a Gecko number. * Measure time to settle from the start of the action, and stop crediting floored metrics Measured from the end of the gesture, time to settle reads ~50ms at every size on every engine, because the answer is then three frames, which is the minimum the loop can return. From the start of the action it is what a user waits. A count that goes 0 to 4 has answered the question and counts as discriminating. A floored timing that is zero or negative at the smallest size has not: it says the action resolves inside one frame there, which is a metric with no room to move. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Record a crashed cell instead of losing the whole matrix A WebKit page that ran out of memory at 300K on a loaded machine took eight good measurements down with it. The cell is now recorded as crashed, the run continues, and the verdict still fails on it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Say precisely what the Long Tasks observer does off Chromium * Say how to bound a wedged engine, having measured that nothing in-process can On a macos-14 runner Chromium finished all three sizes in 90 seconds and then Playwright's WebKit wedged at the smallest size and never came back, which cost the whole matrix. page.evaluate and browser.new_page have no timeout, and SIGALRM does not help: the sync API blocks the main thread inside a greenlet, so the exception lands in the driver and the caller never resumes. The process boundary is the only bound that works, so the docstring says to drive one engine per invocation under an external timeout. * macOS runners have no timeout(1), so give the portable bound instead * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Load the crypto polyfill on the heavy-thread smoke page Same defect as the thread-weight page: the smoke page this branch adds was missing <script src="/crypto-boot.js"></script>, so crypto-uuid-boot.test.ts fails with "smoke-heavy-thread.html must load /crypto-boot.js". It matters more here than on a normal smoke page. This harness is what the perf numbers are measured on, and a page that lacks the polyfill differs from production in exactly the kind of way that makes a measurement mean something other than it claims. Verified: the named assertion fails before the change, and all four tests in the file pass after it. * Measure what the labels say in the heavy-thread harness Every repetition deleted a message and nothing put it back, so repetitions 2 and 3 ran on a smaller thread than the census recorded before the loop. An instrumented run at 25K read 20, 19, 18 messages at the start of each repetition; the smoke page now exposes restore() and it reads 20, 20, 20. One cycle is 20 messages against 10 content kinds, so at 25K those deletions were taking a whole kind each time. The re-open window closed on three calm frames, which held 7 rAF samples for an action taking up to 1.4s, and the leftover highlighting was absorbed by the untimed gate at the top of the next repetition. It now settles on no long frame and no new highlighted token for a grace period, and reports the time of the last activity so the grace is not added to both ends of every ratio. The token probe is polled rather than read per frame because it is a document-wide query whose cost would otherwise grow with the signal. The recorder decided ownership from a shared running flag, so a callback scheduled by the previous action ran once more under the next one and both loops appended to the same array. It carries a generation token now. Both settle() calls in the menu script compare a MutationObserver flag before the observer microtask has run, so each waits out a full double rAF. The growth axis carries a count of those floors instead of a flag, and the menu total carries two. median() dropped None, so a repetition where the menu never opened was averaged away and the null checks downstream never saw it; it now returns None if any repetition did, and a key that was null throughout stays present. A scroll, jump or re-open that never settled is a harness failure rather than an axis reading not recorded. Expanding the tool panes after waiting for the highlighter mounted two fresh unhighlighted fences per cycle whose work landed in the keystroke window, the next thing timed. Seeding and repetitions share one build_fixture() now. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run the measurement integrity tests in CI The 22 tests added with the harness fixes were registered in no workflow, so they ran nowhere. A test that cannot fail is not a guard, and these are the ones that keep a timed-out repetition from being published as a median of three. * Stub the fork count with a real zero, and only that endpoint getForkCount returns data.count and the badge's guard is count <= 0. An empty object body makes that undefined, and undefined <= 0 is false, so the badge rendered on every assistant message with a title reading undefined forks from this message. Measured at 25000 chars: 10 badges and 4031 DOM nodes before, 0 badges and 3981 after. That is DOM in proportion to thread size, added to the axis this harness exists to measure. The stub also matched every url containing /api/, so any other request a measured interaction made was resolved locally before Playwright emitted it and measure_cell's listener never counted it. The harness could report zero stray API requests while fanning out, which is the thing it claims to catch. It matches the fork count endpoint alone now. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Answer the requests the harness provokes from an allowlist Narrowing the fetch stub to the fork-count endpoint was right, and it broke the run: the delete action persists through syncExportedRepositoryToBackend whenever remoteId is truthy, which the synthetic __LOCALID_ id always is, so three requests reached the wire inside the delete measurement and the stray check failed every size. CI showed it as HARNESS-BROKEN, 5 requests, at both 25000 and 100000 chars. The stub is an explicit allowlist now rather than either a blanket match or a single endpoint. Each entry answers a request the harness itself provokes, with the body that endpoint really returns, so no round trip lands in a timed region, and anything unlisted still goes to the network and trips the counter. Narrowing it is also what made two requests visible that the blanket match had been hiding since the beginning: re-opening a thread asks for the project list and the knowledge bases. Those are app fan-out rather than fixture upkeep, so they are stubbed to keep the network out of the reopen window but counted and printed as stubbed api requests. Answering a request must not delete it from the record. The fork-count guard now pins the body to its own allowlist entry rather than scanning the whole file, since another entry legitimately answers with an empty object and a file-wide check would fail on it while saying nothing about fork counts. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add seedCompactTail and gapMetrics for the viewport gap measurement #9058 measures the empty band below the last mounted row and needs two things this harness did not expose. Landing them here rather than having that branch duplicate 767 lines of an unmerged harness. seedCompactTail(targetChars, tailMessages) builds the heavy fixture with the same buildThread call seed() makes, then appends N one-word messages, so the first mount commit lands entirely on compact rows. That is the worst case for a fixed size initial window. Census parity is the point: seedCompactTail (25000, 16) reports 36 messages against seed(25000)'s 20, a tail of exactly 16, with every other count unchanged. gapMetrics() reports the band below the last row, measured against the viewport's bottom edge rather than scrollHeight, so the viewport's own bottom spacer counts as the gap it always was and the caller subtracts spacerHeight to get the part the mount window owns. Computed any other way the numbers stop being comparable across sizes. Both degenerate returns are kept: no viewport gives ok 0 alone, and a viewport with no rows gives ok 0 with mountedRows and clientHeight. The contract test is separate from the rest because what it protects is on another branch: #9058's probe preflights for these exact names and keys, so a rename here breaks a probe nothing in this repo would otherwise exercise. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Gate the tool expand on a count that a closed thread fails The wait after expandTools() read collapsibleOutputs, which is the Radix collapsible CONTENT ELEMENT. Radix keeps that element in the tree for its collapse animation, so it is present whether the card is open or shut. Measured at 25000 chars before any expandTools() call: collapsibleOutputs 2 of the 2 expected while codeExecutionPanes was 0, and after expanding, collapsibleOutputs was still 2. The gate could not fail, so the ordering that build_fixture exists to guarantee, expand and then wait for the highlighter, was never enforced. It reads codeExecutionPanes now, the pane's own pre, which is 0 collapsed and 22 expanded at 300000 chars on all three engines. The comment on collapsibleOutputs claimed Radix mounts the content only while open; that is measurably false and it now says what the count really is, with a warning not to gate on it. Also records what the timed windows scan, since two review items asked. Each window scans a fixed number of times rather than once per frame, and the cost was measured rather than argued: the re-open makes two messageCount passes, 0.4ms of a 2292ms re-open at 300000 chars and 0.0ms of 363ms at 25000, and the menu scans four times for 2.7ms of 3208ms against 0.3ms of 375ms. The share is 0.017 and 0.08 percent, the same at both ends of the axis. * Run the gap contract in frontend CI I added test_heavy_thread_gap_contract.py and registered it in neither the path filter nor the pytest step, so the guard that keeps #9058's probe working ran nowhere. That is the third time this round something was added that could not fail, and this one was mine, two commits after fixing the same class twice. In both lists now. * Count the reopen paint floor instead of declaring it zero growth() subtracts one ~33ms double-rAF vsync floor per wait a metric is clocked across, and the count was a hand-declared integer in GROWTH_AXES. reopen ms declared 0. Reopening is driven by a React state update, so the count check immediately after openThread() always still sees the unmounted tree and the loop always pays at least one __nextPaint() before it can observe the rebuilt messages. That is the same floor already subtracted from jump painted ms and delete ms. Leaving it at 0 kept a full floor of constant baseline in both ends of the ratio, which drags the ratio towards 1 and can report a real reopen curve as flat, worst when the smallest fixture rebuilds near the vsync floor. Rather than change the constant and leave the next one to be declared by hand, REOPEN_JS now counts the waits it actually pays and returns them, and floor_declaration_problems compares the declaration against the count for every engine and size, from harness_failures, so a mismatch fails the run instead of being published. FLOOR_COUNTERS is keyed on the exact axis name, since a prefix would let a later reopen settle ms axis be checked against this one's declaration. A cell that reports no count is a failure rather than a skip; leaving it silent is how the check would quietly stop checking. The clean-cell fixture in the integrity tests gained the field, and it caught the change: with the axis back at 0 the existing clean-cell test goes red too. Eight assertions added, each made to fail on its own targeted broken tree before being kept: the axis back at 0, the checker short-circuited, a missing count skipped rather than reported, the crashed-cell guard removed, the checker unwired from harness_failures, the counter removed from the loop, and mismatches accepted. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Do not report an action that did not run as an unverified paint floor Self-review of the floor check. An action that never ran carries no paintWaits, so the new check reported its floor as unverified on top of the failure harness_failures already raises for the action itself, with the reason. Two failures for one cause, and the second buries the first. Skipped only when the action explicitly reports ran false. An action that DID run and still has no count is still a failure, since that is the case where the subtraction is genuinely unverified, and there is a test on that side too so the skip cannot be widened into a blanket exemption. Both proven red: removing the skip, and widening it to always. * Measure the wall floor per window, and stop filing exceptions as chatter Two things. The generated wall ms axes declared zero double-rAF waits for every action, while the explicit axes declared theirs by hand. MENU_JS opens the recorder before opening the menu and closes it after closing it, so it crosses the same two waits menu open+close ms correctly declares, and menu wall ms was subtracting none of them. That leaves roughly two vsync floors of constant baseline in both ends of the ratio, which compresses it towards 1 and can label a primary metric flat on a small or fast fixture. Rather than hand-declare a second set of numbers, the recorder now counts the waits each window is clocked across and reports them, and the wall axes read that count from the row. growth() accepts a callable floor for this. Waits taken outside a recorder window, ACTION_SETUPS among them, are excluded by construction rather than by remembering to exclude them, and begin() zeroes the counter so a window cannot inherit the waits of the ones before it. Separately, console.error and uncaught pageerror went into the same list as Firefox's two scroll-anchoring notices and were tolerated by the same > 4 allowance. Engine chatter is the engine describing itself; an application exception is not that, and a single one inside a measured interaction means the interaction did not do what the row says. Severity is preserved now, the allowance applies to warnings only, and any error in seeding or in the measured actions fails the run. The counts and the first message are printed either way. Ten assertions added and each proven red on its own broken tree. Two did not go red on the first attempt: the severity assertion pinned an exact one-line expression and stayed green when the predicate was moved to its own line, and the counter-reset break did not apply at all because its needle was mis-indented. Both are fixed and both now fail as they should. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Require a meaningful rise before a zero-based counter counts as an answer No ratio can be formed against zero, so DISCRIMINATION_RATIO never applied to the counter axes at all and large > small was the entire test. The CI workflow runs one repetition on Chromium, so there is no median to smooth a stray dropped frame, and harness_failures accepts any ONE discriminating axis: 0 missed frames at 25K and 1 at 100K was marked as discriminating and could carry the whole liveness verdict while every latency axis was flat or broken. ZERO_BASED_MIN_RISE is what such a counter has to reach. 5 because these axes are dropped frames and long tasks: at twelve times the content a real curve produces them in quantity, while one or two is what an unloaded machine produces on its own. Absolute rather than a ratio because there is nothing to divide by, and the reason string now distinguishes a counter that rose too little from one that never moved, so a reader can see which happened. Five assertions, each proven red on its own broken tree: back to a bare large > small, the threshold lowered to 1, and every zero-based counter rejected outright. The last of those covers the control, since a check that rejects every counter would leave the harness unable to report a live run at all. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Say which axes are counts instead of inferring it from the paint floor The zero branch keyed on floored, which only identifies a timing that had a paint floor subtracted. An unfloored timing does not have one: longest stall ms and worst frame ms read zero at the smallest size whenever the action resolves before the recorder produces a sample, and were then judged as dropped-frame counters, so a noisy 5ms at the largest size read as a rise of 5 and discriminated. harness_failures accepts any single discriminating axis, so that stray millisecond could carry a run in which every valid latency curve was flat. COUNTER_AXES states which axes are counts. Only frames over 33ms is one; everything else is milliseconds. A timing that reads zero at the smallest size is now reported as having no rise to measure rather than being given a counter's credit. Three assertions, each proven red on its own broken tree: timings judged as counters again, no axis classified as a counter at all, and a timing axis classified as a count. The middle one matters because emptying the set would silently turn every counter into a timing and remove the only zero-based axis the liveness verdict has. One correction: the first version of the set assertion required a counter's name not to end in ms, which is wrong, since the counter axis is called frames over 33ms and does. It names the set exactly now, which is the point of classifying it explicitly. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Put the floor COUNT in the report, not the thing that computes it Making the wall floor a callable put the lambda itself into the growth report. main() attaches that report to results and json.dumps it, so a complete run raised Object of type function is not JSON serializable after every measurement had already been taken, including the new CI smoke. That is my regression from the previous commit and it broke every full run. resolve_floor returns an int, growth uses it, and the report stores the resolved count at each end of the ratio rather than a boolean. A boolean would serialise and say nothing; the counts let a reader check the subtraction instead of trusting it. The reason no test caught this is that none of them serialised the report, so three assertions now do, and one of them covers the callable-floor axis specifically since that is the case that broke. Both proven red: the callable put back into the report, and a boolean marker in place of the count. * Keep fractional paint floors, and hold counters to the noise floor at any baseline Two follow-ons from the same review. resolve_floor cast to int. summarise takes a median across repetitions, so a run whose repetitions paid 1 and 2 waits reports 1.5, and truncating that left half a vsync floor in the wall axis. The documented two-repetition configurations are precisely the ones that produce halves. The median is kept as a float, which serialises fine. The noise floor only applied when a counter started at exactly zero. A dropped-frame count going 1 to 2 is a ratio of 2.0, cleared DISCRIMINATION_RATIO, and since harness_failures accepts any single discriminating axis, one incidental frame could carry the CI smoke while every latency axis was flat. A ratio on a counter is only meaningful once there are enough events for it to be about the content rather than about one frame either way, so the floor now applies whatever the baseline, and the reason string says which of the two rules rejected the axis. The floor stays a count of events and is NOT applied to timings, which would silently reject real latency curves that happen to sit at low absolute values. That has its own test. Five assertions, each proven red on its own broken tree: the floor truncated again, the noise floor skipped for nonzero baselines, the noise floor applied to timings as well, and every counter treated as noise, which is what covers the control. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Subtract the whole window's floors from the axes that span it quiet() and quietUntilIdle() return the elapsed time since this.startedAt, not the time they themselves took, and gestureMs is computed from startedAt as well. All three therefore span the entire recorder window and contain every double-rAF wait in it, and all three declared zero. For the scroll that is twenty vsync floors left in both ends of the ratio, which compresses it hard enough to report a real size-dependent regression as flat. scroll gesture ms, scroll settle ms and jump settle ms now take the measured paint_waits. Counted at runtime rather than declared, because the twenty come from a loop: the literal nextPaint count in the source is one, so any number written in here would have been wrong the same way the zero was. Deliberately NOT applied to everything. jump painted ms starts at a mark taken after begin() and spans one wait while the jump's window holds two, and MENU_JS awaits no paint at all, so its window count is zero while its two floors are real, coming from settle() reading the pre-MutationObserver state on entry for open and again for close. Giving either the window count would subtract a floor the number never contained, or drop one that it did. The rule is that an axis measured from startedAt takes the measured count and an axis measured from a later mark keeps a declared one. Seven assertions, each proven red on its own broken tree: each of the three axes back to zero, jump painted given the whole-window floor, and menu given a window count of zero. That last break also turned two PRE-EXISTING menu tests red, which independently confirms the menu axis really does carry both of its floors. There is an end-to-end case too: with the floors left in, a 16x scroll curve reads as 1.86x. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com> Co-authored-by: danielhanchen <danielhanchen@users.noreply.github.com>	2026-08-18 04:33:11 -07:00
Daniel Han	3a7c5ddfde	Time the install from CI, without changing the installers (#9153) ... * Make the installer say where its time goes Install Unsloth (--local, --no-torch) is the largest step in every Windows job: 260s of Windows API CI's 374s, 291s of Windows UI CI's 715s, 281s of Windows Update CI's 794s. The same install on Linux is 88s. Across the ~11 Windows cells a commit triggers that is roughly 50 minutes of Windows runner time per commit spent installing the same thing. Which phase spends it could not be answered from a CI log. Neither setup.ps1 nor setup.sh emits a timestamp anywhere, and the one Stopwatch in setup.ps1 is inside the llama.cpp source-build branch that CI never takes. Guessing would have been misleading: unsloth studio update over an already-complete install costs 297s, more than the 281s full install it follows, which is the opposite of what a download-bound install does. That number is the reason this lands before any caching work rather than after it. UNSLOTH_INSTALL_TIMING=1 prefixes every step/substep line with seconds since the script started, in both the PowerShell and bash installers, so one run of any install turns into a phase breakdown. Off by default and output is then byte-identical, which the tests check by running the bash helper rather than reading it: PowerShell treats every non-empty string as true, so a bare [bool]:... would have made UNSLOTH_INSTALL_TIMING=0 mean on. Enabled on the five Windows install steps, so the breakdown is in the logs from now on rather than needing another PR the next time this is asked. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the installer print helpers self-contained tests/python/test_windows_setup_output_encoding.py dot-sources Get-StudioAnsi, Write-StudioLine, Write-StudioStdoutMirror, step and substep on their own and runs them, so a call out of step to a helper defined elsewhere in setup.ps1 is a hard failure rather than a warning. Inline the elapsed-time prefix in both installers and drop the helper. The PowerShell side reads its state through Test-Path so an unset script variable is empty rather than fatal under a caller's Set-StrictMode, and the guard test now asserts the two print helpers call nothing the probe does not dot-source alongside them. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Skip the setup.sh execution tests where bash is the WSL launcher The cross-platform parity job runs this file on windows-latest, where `bash` resolves to the WSL stub: it ignores the script, prints a UTF-16 "no distributions installed" notice and exits 1. That is not a finding about setup.sh, which is not the installer Windows uses. Probed rather than keyed off sys.platform, so a Windows box with a working git-bash still runs them, plus a sentinel test that fails on any platform that does ship bash. Without it a probe that quietly started returning False would skip both tests everywhere and stay green. * Time the outer Windows installer too, and the update runs Two gaps the instrumentation left, both of which kept the numbers it was added to explain out of reach. install.ps1 is what the Windows jobs actually run, and it does the uv bootstrap and the whole Unsloth dependency install itself before handing off to studio/setup.ps1. Only the child was instrumented, so the larger half of the 260-291s stayed untimed and the child's clock restarted at the handoff. install.ps1 now carries the same opt-in prefix and publishes its start as UTC ticks; setup.ps1 continues from that instead of counting from its own zero, and falls back to a local start when the value is absent or unparseable. The prefix is inlined and Test-Path guarded in install.ps1 for the same reason as in setup.ps1: tests/python/test_windows_setup_output_encoding.py slices those helpers out of this file as well and runs them alone. UNSLOTH_INSTALL_TIMING was also scoped to the install step, while the two `unsloth studio update --local` steps declare their own env. Those runs are the sharpest anomaly on record, a 297s no-op update after a 281s full install, and they were producing no breakdown at all. Set at job scope so both are covered, along with anything added later. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Bounds-check the timing handoff, restore it, and time the POSIX installer too Three follow-ups on the phase timing. The tick handoff was parsed with TryParse alone, which accepts -1 and 9223372036854775807. Both are outside DateTime's range, so the constructor threw, and under $ErrorActionPreference = "Stop" that made inherited junk a fatal installer startup error rather than the documented fall back to a local clock. It also ran when timing was disabled. Now gated on the switch and bounds-checked against DateTime.MinValue.Ticks and MaxValue.Ticks; -1, near-long-max, non-numeric and empty all fall back. The origin was exported and never restored. The documented `irm ... | iex` entry point runs in the caller's process, so it outlived the install: the next run in that session kept the old origin and a later `unsloth studio update` inherited it, both then reporting time since the first install. Saved and restored in the existing finally, like every other handoff variable in this script, and removed when there was no previous value. install.sh had the same gap install.ps1 did. It bootstraps uv and installs the dependencies before launching studio/setup.sh, and its own step/substep were untimed, so a Linux or macOS run had the same unattributed first half. Both POSIX halves now carry the prefix and share one origin, off by default and identical in shape to the Windows pair. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Set the timing origin at the handoff, and validate it on POSIX Restoring the origin in a finally was not enough. Install-UnslothStudio has several early returns above that block, including the install-lock failure and the "another install is already running" path, and they exit before any try begins, so a run that hit one still left UNSLOTH_INSTALL_TIMING_T0 behind in the caller's process. That matters because the documented irm | iex entry point runs there. The variable is no longer written at the top of the function at all. The origin is computed there, and exported only beside the other handoff variables immediately before the child launch, then restored in the finally that already covers them. That is the one region an early return cannot skip, so nothing between entry and handoff can leak. On POSIX the inherited value landed inside an arithmetic expansion, and $(( )) evaluates a bare word as a variable name: with set -u, UNSLOTH_INSTALL_TIMING_T0=junk aborted the installer with "junk: unbound variable", and "1;rm" was an arithmetic syntax error, so an unrelated outer process could stop an install that merely asked for timing. Both halves now accept only a plain non-negative integer and fall back to the local clock otherwise, including when the arithmetic yields a negative elapsed. Same reasoning as the tick bounds check already on the PowerShell side, which this had fallen a platform behind. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Time the install from CI, and leave the installers alone The previous approach put the clock inside install.sh, install.ps1 and studio/setup.*. That is a lot of surface for a log detail: it needed a UNSLOTH_INSTALL_TIMING switch with a different off-by-default rule per shell (PowerShell treats every non-empty string as true, so "0" enabled it), plus a UNSLOTH_INSTALL_TIMING_T0 epoch handed from the outer installer to the inner one, which then had to be bounds-checked in both dialects and unset again on every early return. Two of those hazards were reported on this PR. None of it is needed. Every step that runs an installer already pipes its output, so CI can prefix elapsed seconds as a pipeline stage and the installers stay exactly as they were. * POSIX: one stage in .github/actions/install-unsloth-local, which is the single definition behind 40 jobs. * Windows: the five install.ps1 pipelines, plus the two `unsloth studio update --local` steps in the update workflow. That update is the number worth explaining: a no-op update over a complete install costs 297s, more than the 281s full install it follows. The filter sits downstream of the log write, so logs/install.log keeps byte-for-byte what the installer produced and the ~30 steps that read that artifact are unaffected. interrupted-install-ci.yml matches ^\[TAURI:STEP\] anchored at line start against one of them, which is the reader that a prefix in the file would break silently rather than loudly. tests/studio/test_install_phase_timing.py asserts both halves: that the four installer scripts carry no timing machinery, and that in every prefixing step the log write comes first. Nine mutations were checked red, including two that were green on the first cut of the tests: an ordering check that read Tee-Object out of the explanatory comment above the pipeline, and a $LASTEXITCODE check that stayed green when deleted because the same name appears inside the child command string. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 04:23:09 -07:00
Daniel Han	fc325f431a	Stop running every macOS workflow on every commit to main (#9174) ... * Stop running every macOS workflow on every commit to main GitHub caps macOS at five concurrent jobs account-wide, across every repository, which makes a macOS runner slot the scarcest thing in this repo's CI. Four workflows scoped their pull_request trigger carefully and then declared `push: branches: [main]` with no paths filter at all, so after merge they ran on everything. Commit 6371f46a changes README.md and nothing else. It started Mac Studio GGUF CI, Mac Studio UI + API + Update CI, Mac Studio Install Matrix CI and Unsloth Tauri CI: seven macOS legs, 40 percent over the entire account cap, for a documentation typo. Every one of them then queued behind the others. Replaying each workflow's own pull_request filter against the last 40 commits to main puts the cost at 280 macOS job-slots today against 94 with the filters mirrored, and drops the peak on a single commit from 7 to 3, under the cap. Most of that is one workflow: Mac Studio Install Matrix fired on 40 of 40 sampled commits and would fire on none of them, at 4 slots each, and it also carries the worst queue in the set at a 200 minute median for a 2m38s job. No coverage changes. Each push list is its own pull_request list copied verbatim, which is the same question asked twice, and the post-merge backstop still runs whenever a commit can affect what the workflow tests. clean-machine-install-ci.yml and mlx-ci.yml already do exactly this and say why; this brings the other four in line. tests/studio/test_macos_slots_per_commit.py asserts no macOS workflow has an unfiltered push trigger, that the two filters do not drift apart in either direction, and that a README-only commit starts nothing. Its first cut reported workflow-trigger-lint and studio-inference-smoke as macOS workflows, because it scanned whole jobs and both merely mention macOS in a step; it now reads runs-on and resolves matrix expressions. Three mutations verified red. * List the helpers these workflows execute, now that the trigger is scoped Scoping a trigger is only safe if its path list is complete, and these were not. Five helper scripts and one auditor are invoked by name from a run step and matched no pattern in either list: studio-mac-inference-smoke assert-llama-loads.sh, hf-download-with-retry.sh, studio_smoke/multi_turn_chat.py studio-mac-ui-smoke assert-llama-loads.sh, hf-download-with-retry.sh studio-tauri-smoke scripts/lockfile_supply_chain_audit.py The gap predates this PR, but it was invisible while the push trigger was unfiltered, because every commit ran everything after merge. Narrowing the trigger is what turns it into a real hole: editing assert-llama-loads.sh would stop running the workflows that assert with it. studio-tauri-smoke also gains studio/package.json and studio/package-lock.json. Its Linux job installs and runs the Tauri CLI out of that package root (npm install --prefix studio, npx --prefix studio), so those manifests decide what it builds with. They are listed by hand rather than found, because that dependency does not go through a file path in a run step. test_every_helper_a_workflow_executes_is_in_its_trigger asserts the rest, from the run bodies rather than a fixed list, for both triggers. Verified red by dropping assert-llama-loads.sh again. * Cover the install matrix's transitive inputs, and run the guard on workflow-only PRs studio-mac-install-matrix listed install.sh and studio/setup.sh but not what they reach: setup.sh:1444 runs install_python_stack.py, which imports install_manifest.py, and install_llama_prebuilt.py imports prebuilt_core.py at line 55. A commit touching only one of those changed exactly what this matrix asserts on and did not start it. Not studio/backend/requirements/** or pyproject.toml, which install_python_stack.py also consumes. Generic install breakage on macOS is clean-machine-install-ci's job, and it lists both on either trigger with seven macOS legs; duplicating them here would re-spend the slots this filter exists to save. That is a judgement rather than an oversight, so it is written in the filter next to the paths it excludes. test_a_listed_python_input_brings_its_sibling_imports asserts the mechanical half: a listed .py must bring the modules it imports from its own directory. The full transitive closure of an installer is most of the repo, and chasing it is how a filter stops saving anything, so anything deeper stays hand-listed with a reason. The guard found install_manifest.py on its first run, which is one hop further out than the report that prompted it. The guard itself is now wired into workflow-trigger-lint, the one job with no paths filter. Every way it can regress is a workflow-only edit, which no other job matches, so it would otherwise have been collected first by Backend CI's unfiltered push -- after the change had merged.	2026-08-18 04:22:15 -07:00
Daniel Han	571a99b72b	Studio: stop the overlay rail going click-through while it is scrolling (#9150) ... * Studio: stop the overlay rail going click-through while it is scrolling `Chat UI Tests (banner)` has been red on main since #9132, on the WebKit leg of "Update banner layout, other engines", at 921x534 on New chat: FAIL the card keeps its full width whatever the scrollbar does cardWidth=430 want=448 railGutter=0 scrolls=True FAIL the rail takes pointer input exactly when it scrolls scrolls=True pointerEvents=none Both assertions are from #8367 and both are right. The second one describes a state a user can actually be in, and the shard's own screenshot shows it: the rail is capped, the llama.cpp card is sliced off at the fold, the Unsloth update card is entirely above it, and the rail is `pointer-events: none`, so there is no scrollbar and no drag that brings it back. The suite's reachability checks pass because they call scrollIntoView, which a reader has no way to do. What #9132 changed is not the product, it is the state the suite finds. The banner scripts used to run on the machine and the Studio home the chat and extra shards had already been through; they now get their own runner and a pristine `~/.unsloth/studio`, since boot-studio-api-only.sh wipes only `auth`. The last green artifact is dark-themed with "Chat C" and "Chat B" in Recents; the first red one is default-themed with "No chats yet". Same commit range, different starting state, so the rail is capped now where it was not before. #9132 is correct; it exposed this rather than caused it, and the same failure reproduces on unrelated PR branches. The defect is that `overflowing` is a prediction. `floorRoom > maxHeight` asks whether the cards WOULD fit if they collapsed to the floor that was measured for them under a max-height of 0. When they stop short of that floor the box overflows a cap the prediction says it fits inside, and the rail is left click-through while it scrolls, which is the failing pair exactly. So the reading is kept and the actual state is or-ed into it, taken off the node in the same synchronous block that already lifts and restores the cap, with the real cap back on and the layout just flushed. It is re-read on every pass and never remembered, which is what the derived value was there to protect against: the observers watch the rail and every descendant, so a placement that grows to fit clears the flag on the same pass that applied it. Being an `||`, it can only add the case the prediction misses and can never take pointer input away from a rail that already had it. Verified on this box, WebKit and Chromium, against a build actually served from this tree (`unsloth studio --frontend`; the venv's installed copy is what gets served otherwise, which is worth knowing before trusting a local Studio run): - Broken deliberately, `overflowing` forced false: the full Chromium suite is 1433 checks, 1 failed, and the one is `320x480 at 20px: the rail takes pointer input exactly when it scrolls scrolls=True pointerEvents=none`. - Fixed: 1433 checks, 0 failed. Firefox and WebKit spot: 221, 0 failed each. - Unchanged where the prediction was already right. Sweeping WebKit down the height axis at 921 wide, the rail and the flag agree at every step both before and after: no scroll and `none` at 534 through 364, scroll and `auto` from 360 down, card 448 throughout. The width half is not fixed here, because I could not reproduce it and will not guess at it. 448 - 430 is 18px of scrollbar, but `offsetWidth - clientWidth` reports 0 in the same breath, so on that runner the bar takes width out of the content box without showing up in the gutter the assertion reads. This box never reproduces it: WebKit here keeps the card at 448 whether the rail scrolls or not. `scrollbar-gutter: stable` is the obvious candidate and is deliberately not taken on a guess, since it reserves the gutter permanently and would shift the cards left by the scrollbar width on Chromium, where the card is already correct. Instead both failing assertions now print the measurements that would settle it: the card's transform and border box, its computed and max width, innerWidth and documentElement.clientWidth, the rail's offset, client and content widths, its scroll and client heights, its cap, and the children's heights. Nothing is relaxed; the same two checks fail on the same conditions, with enough attached to say which of the two mechanisms it is. That output is what the next run of this shard is for. * Take the rail's overflow reading after the transition is restored The DOM reading was inserted between the flush of the restored cap and the line that puts the eased transition back, which is the one place the store must keep adjacent: update-banner-flex-priority asserts on it, and the assertion went red on both the Linux and the Windows frontend unit jobs. The reading does not need to sit there. The cap change is already committed under the suppression by the flush above it, and transition is not itself a transitionable property, so no reflow the read forces can hand that cap to an animation. Moving it below the restore keeps the invariant and the reading. * Measure the banner card's layout width, not its painted width The width half of the banner failure is the card's enter animation, not a scrollbar. The diagnostics added earlier in this branch settled it on the WebKit leg: transform matrix(0.96, 0, 0, 0.96, 0, 12), which is exactly the initial state in components/web/update-banner.tsx, with borderBox 448, cssWidth 448px, railContentW 448 and railGutter 0. 448 * 0.96 is 430.08, and 430 is what the assertion read. Nothing had taken any width; the card had simply not been painted at full size yet. So the assertion was measuring the painted box for a question about layout. getBoundingClientRect includes transforms and offsetWidth does not, so the reading moves to offsetWidth, which a scrollbar taking width out of the rail's content box still shrinks. The painted width is still reported next to it, since the gap between the two is the diagnosis. Checked on chromium, firefox and webkit that this is red exactly when it should be: with the transform applied and no width lost, the old reading fails and the new one passes; with 18px actually taken out of the rail's content box the new one fails on all three, with or without the transform also applied; on a settled healthy box both pass.	2026-08-18 01:41:05 -07:00
Daniel Han	9ccc64d16e	Unblock Mac Studio GGUF CI, red on every main run since #8883 (#9155) ... * Unblock Mac Studio GGUF CI, red on every main run since #8883 Mac Studio GGUF CI has failed on every main run from ee68d9e2a onward, the merge of #8883 (refuse a gguf that cannot fit in free vram plus available ram). Bisected over 90 completed main runs: green through 31c42e872, red from ee68d9e2a, no recovery since. The guard is not wrong. From the failing run's own server log: Metal device looks virtualised (Apple Paravirtual device) ... GGUF inference will run on CPU Forcing gpu_layers=0 for gemma-4-E2B-it-UD-Q4_K_XL.gguf Error loading model: About 3 GB of this model does not fit in GPU memory and would run from system RAM. Only about 4 GB is available and 2 GB of that is kept free for the rest of the system, leaving about 2 GB usable. GitHub's macOS runners expose a paravirtual Metal device, and Unsloth already refuses to offload to one because paravirtual Apple GPUs return corrupt output. So the launch is --gpu-layers 0 --device none and the WHOLE model is a host mapping, not the partial spill the guard was written to price. It then measures honestly: gemma-4-E2B UD-Q4_K_XL (~2.4 GB) plus mmproj-F16 (~780 MB) does not fit in 2 GB usable. The load returns HTTP 400 and the phase fails several layers away from the cause. The workflow is what was wrong. It had been getting away with a model the runner cannot hold because the prompts are tiny and the mapping is paged, which is exactly the gamble the guard stops taking on a user's machine. CI now takes it knowingly with UNSLOTH_ALLOW_HOST_OFFLOAD, the escape hatch the refusal names, rather than silently swapping in a smaller model and quietly changing what mac vision inference is tested against. Set at job level, not on the vision phase: every phase there is CPU-only for the same reason, so a runner image with slightly less free RAM would take out the tool-calling phase next. tests/studio/test_mac_host_offload_optin.py keeps the blast radius honest. The opt-out disables a real safety net, so it asserts the reason stays written down next to it, that the Linux and Windows GGUF workflows never acquire it -- there a silenced guard would let a genuine host-offload regression pass green -- and that the guard keeps its own unit tests. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run the host-offload guard on workflow-only PRs, and check workflow-level env The guard rejects a PR that removes UNSLOTH_ALLOW_HOST_OFFLOAD from the macOS GGUF job or adds it to the Linux or Windows one. Such a PR edits a workflow and nothing else, which Backend CI's paths filter does not match, so the assertion was never collected for the one change it exists to reject. Wired into workflow-trigger-lint, which carries no paths filter and already hosts the other workflow invariants for this reason. The blast-radius check also read only job and step env. GitHub propagates a top-level env to every job, so one line at the top of the Linux or Windows workflow would have disabled the guard across all of them while this test stayed green. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 01:32:35 -07:00
Daniel Han	68fec34d73	Run the three loaded-models-indicator engines at once (#9158) ... * Run the three loaded-models-indicator engines at once Loaded-models indicator (cross-browser) is the longest Linux job in CI at about 1000s, and about 870s of that is one step running the same Playwright suite three times in a row, once per engine. The runs are disjoint: each boots its own server and drives its own browser, so the serialisation bought wall-clock and nothing else. Two things were shared, and each is split rather than serialised. Each engine gets its own port, so three servers coexist, and its own UNSLOTH_STUDIO_HOME. The second one is the reason this was not already done: run-studio-indicator-browser.sh wipes $studio_home/auth so the boot mints a fresh .bootstrap_password, then reads that file back, and on a shared home one engine's wipe lands between another's mint and its read. Running the current step body concurrently against one home has two of the three engines read a password another engine minted. A per-engine home is cheap because UNSLOTH_STUDIO_HOME selects a data root only: unsloth on PATH still resolves the installed venv, the frontend is served from a package-relative path rather than from studio_root(), and the suite is API-only with the status endpoints stubbed via page.route, so a fresh home needs no model, no GPU and no llama.cpp build. The step now waits on all three engines before failing, so one engine's breakage no longer hides another's, and each engine's output is echoed in its own log group. test_the_linux_job_still_drives_all_three_browser_engines matched the literal '...sh 18899 <engine>' call form, which the loop no longer produces. It now asserts the property instead, scoped to the steps that invoke the helper so that the browser-install step naming all three engines cannot stand in as coverage. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Point each isolated indicator home at the installed studio venv UNSLOTH_STUDIO_HOME selects the CLI's install root, not just a data root, so the per-engine directories were empty installs: unsloth_cli resolves $UNSLOTH_STUDIO_HOME/unsloth_studio/ bin/python and exits "Unsloth Studio not set up. Run install.sh first." before binding a port. All three engines failed identically, which reads like a broken suite rather than broken isolation. Each per-engine home now symlinks the one venv install.sh already built and owns only the mutable state beside it, so the auth wipe still cannot race while nothing is copied or rebuilt. The step also checks the installed venv is there first, so a missing install is one clear error instead of three misleading ones. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-18 00:53:30 -07:00
Daniel Han	2073e0fe4d	Studio: load the settings tab panels when they are shown, not at launch (#8966) ... * Studio: load the settings tab panels when they are shown, not at launch * Studio: keep a settings panel that fails to load from taking the app down A panel is fetched the first time it is shown, so it can now fail where it could not before: offline, or a page whose entry bundle predates an in-place rewrite of dist/ and still names chunks that have been replaced. The dialog is mounted at the app root and nothing above it catches, so the throw unmounted the whole of Studio rather than one panel. Blocking a panel's module in a browser reproduced it: the dialog, its nav and the rest of the page went. The panel area now sits in an error boundary that offers a reload, and the Suspense fallback is a delayed loading line rather than an empty pane, so a slow first open shows something and a prompt one still shows no flash. Reload rather than retry: React caches a lazy rejection for the life of the page and the browser's module map caches the failed import, so re-importing the same URL rethrows without a new request. index.html is served no-store, so a reload does pick up the current chunk names. tests/settings-tab-panel-loading.test.ts gains a case that walks the JSX and asserts every panel Suspense is inside a class that defines getDerivedStateFromError. tests/studio/playwright_settings_tabs.py drives the real dialog in a browser: all twelve tabs, deep-open, the search jump, and the blocked-module case. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: consume a failed settings panel prefetch The idle prefetch warms every panel once the dialog opens, so a chunk it cannot fetch reached the page as an unhandled rejection for a tab nobody had asked for. Reproduced by blocking one panel's module in a browser: the rejection landed on window even though the boundary handled the panel that was actually on screen. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: typecheck the settings smoke entry, and stop the harness settling on a placeholder tsconfig.app.json lists the three existing smoke entries explicitly, so the new one was outside the project and npm run typecheck skipped it. Added, and the harness handle it installs on window is now optional, since app code sits in the same project and must not be able to reach a handle only the harness page installs. tsc --listFiles now names the file. The Playwright harness settled on whatever held still for 600ms. The panel renders from a deferred value, so a switch keeps the outgoing content up until the incoming panel is ready, and on a loaded machine that hand-off outlives the window: one run read the placeholder as the final panel and called a correct recovery a failure. It now refuses to settle on something with almost no content. A run that dies on a cold dev server also writes its report instead of leaving none. * Tighten the settings lazy-panel comments * Name the encoding when the settings harness writes its report tests/test_source_read_encoding.py holds every checked-in file read and write in the test trees to an explicit utf-8, so it does not depend on the platform default and break on Windows the day the file gains a non-ASCII byte. The report write was the one that did not. * Let the select's keyboard scroll settle before the font-scale wheel check Pre-existing flake in this step, not something this branch introduced. The step reads scrollTop straight after keyboard.press, but Radix scrolls the highlighted item into view off the back of that keypress, so the value is a mid-scroll sample: instrumented on the ubuntu CI image the viewport went on to settle 24-35px further down in 20 runs out of 20, on this branch and on its merge base alike. Two things break as a result. The stale sample is not the floor the wheel has to beat, which is why the failure reads '20 -> 44' as though the viewport had moved the wrong way when 44 is simply where the keyboard scroll ended up. And a wheel dispatched into a scroll Chromium is still animating can be swallowed outright, which is the actual failure: at a maximum scrollTop of 243 a working -400 wheel lands on 0 every time. So wait for the scroll to stop before taking the floor, keep the pointer inside a viewport that is not always 40px tall, and re-send the wheel on a bounded retry. A viewport that genuinely refuses the wheel still never moves and still fails, just after more tries. * Clear an unconsumed archive deep-open when settings navigates away * Run the settings tab-panel browser smoke in frontend CI * Keep an archive deep-open when the navigation lands back on Data * Keep a settings scroll target when its own tab is reselected * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Load the crypto polyfill on the settings smoke page * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Trim comments in the settings lazy-loading changes * Hold only the Data panel's own module in the settings deep-open test The abandoned-deep-open step routed every request through a handler that sleeps 2.5s, and that sleep runs on the driver thread, so the whole page's module load queued behind it and arrived at the main thread in one go. On a two-core runner sharing the box with a live Studio that pushed the reopen past its 15s timeout, which reads as a settings dialog that would not open when nothing was wrong with it. Route the Data module alone. The assertion is unchanged and still goes red on the pre-change store: the next ordinary visit to Data reopens the archive listing. * Name the cause when the settings smoke page has navigated away Vite dev proxies /api to 127.0.0.1:8888. With a Studio listening there and no token those calls answer 401, the app's auth handling navigates, and the harness window goes with it, after which every step times out waiting for a dialog that cannot exist. It happens on main too, where the harness is gone before the first open, so it says nothing about the panels. Report it. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com> Co-authored-by: oobabooga <112222186+oobabooga@users.noreply.github.com>	2026-08-18 00:10:58 -07:00
Daniel Han	1f09a17868	Studio: keep the streaming render harness the perf PRs kept rebuilding (#8969) ... * Studio: keep the streaming render harness the perf PRs kept rebuilding * Studio: close the false-green paths in the stream pacing harness The long-task total is the metric the budgets turn on, and it reads 0 both when the render is free and when the observer never ran. observe({type: "longtask"}) is specified to abort silently on an engine that lacks the entry type rather than throw, so the try/catch around it never fired: under firefox or webkit the harness scored a perfect zero and exited 0. Detect support with PerformanceObserver.supportedEntryTypes, record it, and fail the run when no long tasks were seen or when throttling was disabled. Also: - add the entry to tsconfig.app.json, which lists the smoke entries one by one, so npm run typecheck actually covers the 270 lines it was reported against - write the JSON under logs/ like every sibling harness instead of dropping an untracked stream-pacing.json in the repo root, and create the directory - treat an exported-but-empty SMOKE_BASE_URL as unset, matching the siblings; it drove "" as the base URL and burned the full readiness timeout - register the harness in the two contract tests, which is what surfaced the SMOKE_BASE_URL bug, and pin the new guards there - record the second mutation: reverting #7892 moves the longest stall 4-5x while leaving the long-task total inside the clean range, the exact opposite of reverting #8750, so both budgets are load-bearing * Studio: budget only the long tasks the stream itself caused buffered: true replays whatever the performance timeline already held, so module evaluation and the first React render landed in the budgeted total: one entry, ~140ms, about 2.6% of a clean run here, and larger on a cold or loaded runner. Nothing filtered by startTime and run() reset nothing, so a slow page load read as a slow renderer. Open the measurement window in run() and drop entries that began before it. Pre-stream share goes 2.6% to 0.00% while the stream's own tasks are unchanged (60 and 52 entries over two clean runs), and reverting #8750 still fails the budget at 52,465ms. * Studio: tighten the stream pacing harness comments Comments and docstrings only, no code change. Every measured number, PR reference and causal reason is kept verbatim. * Studio: check the reply that settled, not the peak it once reached paintedChars is a high-water mark and only ever climbs, so a completion render that truncated the bubble would leave the peak behind and the 90% workload floor would still pass on a DOM that no longer held the reply. Record what is on screen at settlement and check that too. Measured equal to the peak today (24,033 both), so this is a guard rather than a live discrepancy, and it is pinned in the harness contract test. Also count slow frames only inside the measurement window and reset the counter in run(), the same rule long tasks now follow. Contamination measured at 0 of 286 here, but an external server or a slower box need not be 0 and the number is meant to be comparable across them. * Studio: record a stall that never ends, and keep the task that starts the stream Two holes left by the measurement window added in the previous commits. A long task carries the start time of its whole task, so appending the user message in the same task that assigned measureFrom stamped runtime startup and the first publish as earlier than the window and dropped them as page load. Hand the append to a later task so the work that begins the stream sorts inside it. longestStallMs was only ever written when a later paint closed the stall, so a freeze that ran to the end of the stream was never recorded: the tail can go missing inside the 90% floor and the quiet-frame loop then calls it settled. Measure the stall in progress while text is still arriving, which is what the number means, and not afterwards, where the settle window's own quiet frames would read as a freeze. Clean runs unchanged (stall 933 to 1,050ms, long tasks 4,749 to 5,159ms over three) and both mutations still caught: #7892 reverted fails the stall at 5,233ms, #8750 reverted fails long tasks at 52,263ms. * Studio: tighten the comments added since the first pass Comments only, no code change. Every measured number and every causal reason is kept. * Studio: record a freeze that spans the end of the stream The stall in progress was measured only while text was still arriving. A freeze that spans the moment the stream ends blocks the frame loop across it, so the first frame afterwards already observes a non-null streamEndedAtMs and the whole frozen interval was skipped. With the lost tail able to hide inside the 90% workload floor, thirty quiet frames then settled the reply and the run reported a short longest stall, which is the one shape this number exists to catch. Cap the interval at the absolute stream-end timestamp instead. A freeze across that moment is recorded in full, and the stall stops growing once there is no more text to wait for, so the settle check's own quiet frames are still not counted as a freeze. The rule moves into smoke-stream-pacing-stall.ts so it can be tested without importing the harness entry, which mounts React on import. The new tests cover the spanning freeze, the settle-window bound, idempotence and late tail paint; restoring the previous rule fails two of the five. Clean runs unchanged (stall 967 to 983ms, long tasks 5,442 to 5,842ms) and both mutations still caught: #7892 reverted fails the stall at 5,017ms, #8750 reverted fails long tasks at 63,687ms. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Make the stream pacing smoke page load again Merging main brought in ASSISTANT_PART_COMPONENTS, which thread.tsx builds at module scope with Text: MarkdownText. Entering the markdown-text -> features/chat -> chat-page -> thread cycle from markdown-text runs that object literal while the MarkdownText binding is still in its temporal dead zone, so the page died with Cannot access MarkdownText before initialization and rendered nothing. Import the chat barrel first, as the app's entry does. The page is also a new HTML entry, so it has to load the crypto polyfill before its module entry like every other one. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com>	2026-08-17 22:38:56 -07:00
Daniel Han	9eb614207a	Give the small Windows checks one box per image instead of one box per check (#9143) ... * Bundle Unsloth GGUF CI onto one runner, matching Windows and macOS The three ubuntu-latest jobs differed only in model, port and test body; checkout, apt, setup-node, setup-python and install.sh --local --no-torch were identical in all three. Windows and macOS already run these three phases in a single job. This is the Linux port of that layout, step bodies unchanged. studio-windows-inference-smoke.yml carried a note saying the trade was not worth making on ubuntu, because setup there is ~1.9 min against ~3.0 min of tests and the sequential wall time would cost more than the slots are worth. The arithmetic was right; the premise that the three jobs start together is not what the runners do. Run 32089506294 started two jobs at 02:40:11 and the third at 02:46:52, then finished at 02:52:57: a 12m46s wall for about 6 min of work. Run 32089558062 staggered all three. Across 18 recent multi-job runs on main the median gap between a run's first and last job start was 175s, and 13 of the 18 exceeded 60s. Bundled is about 7m35 every time. When all three do get slots at once that is ~1m30 slower than the 6m05 longest job; when they do not it is 2-5 min faster. It also returns 2 of the 60 concurrent slots, which is what makes the contended case rarer for every other workflow. Each phase keeps its own model cache directory, HF_HOME, port, server log, artifact and step timeouts, and gates on the shared preamble rather than on the phase before it, so a phase-1 failure still lets phases 2 and 3 report. Phase 3 moves to hf-cache-vision with a key bump because phase 1 keeps hf-cache, whose key is byte-shared with the macOS and Windows gemma phases. tests/studio/test_gguf_smoke_phases_stay_independent.py asserts all of that from the workflow rather than from a list, since every one of these regressions is silent rather than red. * Use github.workspace for the xet scratch dir; runner is not a job-env context Actions rejects the whole workflow before scheduling a job, so the first staging push produced a run with zero jobs and only 'This run likely failed because of a workflow file issue'. Job-level env can read github, needs, strategy, matrix, vars, inputs and secrets, not runner. * Record the measured staging timing for the bundled job Green end to end on a staging repo with an empty actions cache, so every model downloaded cold: 6m03s for all three phases, of which 105s is the now-shared setup. The phases themselves are 26s / 179s / 36s. That is at or under the 6m05 the longest of the three jobs took on its own, on a third of the runners, so the earlier estimate of ~7m35 was pessimistic. * Give the small Windows checks one box per image instead of one box per check pester, no-vs-gpu-resolve and the two cells each of vs-integration and vcredist-clean-box are six Windows job-runs that execute for 16-34s apiece. On this repo's Windows pool that is not what they cost. Measured over recent main runs, every Windows job waits 2600-3400s for a slot regardless of what it then does: exec queue job 16s 2606s real-VS detection (VS 2022) 22s 3315s real-VS detection (VS 2026) 24s 3170s VC++ round-trip (windows-latest) 26s 3358s VC++ round-trip (windows-2025-vs2026) 33s 3311s setup.ps1 unit tests 34s 3391s GPU prebuilt resolves without VS 1108s 3164s Chat UI Tests So six slots deliver 155s of work, and the queue those slots help create is what the 18-minute Chat UI job waits in. Merged by runner image they are three job-runs and nothing moves platform: windows-2022 keeps its own box for VS 2022 detection, windows-2025-vs2026 keeps its own for VS 2026, and windows-latest carries the three checks that only need a stock Windows box. Ordering inside a box is load-bearing. The VC++ phase uninstalls the runtime and restores the registry in a finally, so it runs last everywhere. The phases before it install a PowerShell module, pip packages and a simulated no-build-tools tree under the workspace; none writes the VC++ registry keys the round-trip reads as ground truth, so its clean-box precondition still holds. The two long jobs (inference-smoke 718s, no-vs-cpu 464s) stay on their own runners: their cost is execution, not slot occupancy. tests/studio/test_windows_small_checks_stay_on_their_image.py pins the phase-to-image contract as data rather than reading it back out of the workflow, because a check running on the wrong Windows image mostly still passes. Stacked on #9139, which edits a comment in this file. Not verifiable locally: needs a cross-platform staging run. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Find the Pester phase by content, not by job id test_pester_bootstrap_hardening.py keyed on jobs['pester'], so grouping the small Windows checks by runner image turned all 8 of its assertions red on a rename that changed nothing they assert. The staged run caught it: 'Guard the Pester bootstrap' failed while every phase after it passed. Looks up whichever job installs Pester instead. Worth noting that the gating held exactly as designed under a real failure: the guard failed and the no-VS resolve and VC++ round-trip phases on the same box still ran and passed. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 21:13:48 -07:00
Daniel Han	7298bb9568	Bundle Unsloth GGUF CI onto one runner, matching Windows and macOS (#9139) ... * Bundle Unsloth GGUF CI onto one runner, matching Windows and macOS The three ubuntu-latest jobs differed only in model, port and test body; checkout, apt, setup-node, setup-python and install.sh --local --no-torch were identical in all three. Windows and macOS already run these three phases in a single job. This is the Linux port of that layout, step bodies unchanged. studio-windows-inference-smoke.yml carried a note saying the trade was not worth making on ubuntu, because setup there is ~1.9 min against ~3.0 min of tests and the sequential wall time would cost more than the slots are worth. The arithmetic was right; the premise that the three jobs start together is not what the runners do. Run 32089506294 started two jobs at 02:40:11 and the third at 02:46:52, then finished at 02:52:57: a 12m46s wall for about 6 min of work. Run 32089558062 staggered all three. Across 18 recent multi-job runs on main the median gap between a run's first and last job start was 175s, and 13 of the 18 exceeded 60s. Bundled is about 7m35 every time. When all three do get slots at once that is ~1m30 slower than the 6m05 longest job; when they do not it is 2-5 min faster. It also returns 2 of the 60 concurrent slots, which is what makes the contended case rarer for every other workflow. Each phase keeps its own model cache directory, HF_HOME, port, server log, artifact and step timeouts, and gates on the shared preamble rather than on the phase before it, so a phase-1 failure still lets phases 2 and 3 report. Phase 3 moves to hf-cache-vision with a key bump because phase 1 keeps hf-cache, whose key is byte-shared with the macOS and Windows gemma phases. tests/studio/test_gguf_smoke_phases_stay_independent.py asserts all of that from the workflow rather than from a list, since every one of these regressions is silent rather than red. * Use github.workspace for the xet scratch dir; runner is not a job-env context Actions rejects the whole workflow before scheduling a job, so the first staging push produced a run with zero jobs and only 'This run likely failed because of a workflow file issue'. Job-level env can read github, needs, strategy, matrix, vars, inputs and secrets, not runner. * Record the measured staging timing for the bundled job Green end to end on a staging repo with an empty actions cache, so every model downloaded cold: 6m03s for all three phases, of which 105s is the now-shared setup. The phases themselves are 26s / 179s / 36s. That is at or under the 6m05 the longest of the three jobs took on its own, on a third of the runners, so the earlier estimate of ~7m35 was pessimistic. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 21:13:01 -07:00
Daniel Han	e463194583	Split Chat UI Tests into four shards on the Studio boundaries (#9132) ... * Split Chat UI Tests into four shards on the Studio boundaries Chat UI Tests was the largest job in the repo: 2,514 minutes over a week, 22.1 minutes on average, 11 Playwright scripts run one after another against four Studios booted one after another. Timed on a real run, 17.6 minutes of which the setup every shard has to repeat is about 2.6 (checkout, Linux deps, Install Unsloth, Playwright browsers). The rest is the scripts, and the two banner engines alone are 7.25 of it, which is why they are a shard of their own rather than sitting with their neighbours. The boundaries are the Studio instances, not an even division of scripts: a boot, its health wait, its bootstrap password and the scripts that drive it have to stay on one machine. chat takes 18892 plus the cross-browser permission pass, extra and banner each boot their own 18894, picker takes 18898 and 18896. In-machine parallelism is deliberately not used. These are separate runners, so each shard still boots one Studio at a time and nothing competes for RAM, which is the constraint that rules out simply running the scripts concurrently on one box. Runner minutes go up and wall-clock comes down, which is the trade being made on purpose. The risk worth guarding is not a broken shard, which is loud. It is a step whose if: names no shard, or names one not in the matrix, or a shard left with nothing to do: the step runs nowhere, all four shards are green, and a regression suite has quietly stopped existing. The new test asserts coverage from the workflow itself rather than from a list, checks that a script and the Studio it talks to land on the same shard, and pins the count of driving steps at 11. All three mutations fail it: renaming a shard in one if:, dropping a gate, and moving a script away from its Studio. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Cache the Playwright browser downloads Three engines, downloaded on every job, on every run, uncached. It was 0.75 minutes when one job needed them; with four shards it is four times that, and it sits on the critical path of each. Keyed on the RESOLVED Playwright version rather than on the '>=1.45' spec, because that is what decides which browser builds are wanted: a floating spec that resolves higher must not hit a cache holding the older engines. --with-deps stays unconditional on a miss and becomes install-deps on a hit. The apt half installs system libraries outside the cached directory, so restoring the cache without it would give browsers that cannot start, which is the failure this kind of change usually ships with. Only the download is skipped. * Give each shard its own artifact name, and run this guard on workflow edits Two from review, both real, and the first would have made the change worse than what it replaced. Artifacts are immutable within a workflow run, so four cells uploading studio-ui-smoke-artifacts means the first to finish creates it and the other three fail on the conflict. The upload step carries if: always() and no continue-on-error, so a UI run where every test passed reports red on three cells out of four. The name now carries the shard, and the new assertion is written for any matrix job in this workflow rather than for this one by name, since the next job to be sharded inherits the same trap. Removing the shard from the name fails it. The second is the same gap this repo keeps finding: the guard reads studio-ui-smoke.yml, so the edit it exists to catch is by definition a workflow-only edit. Backend CI's paths filter does not match one, and the UI smoke job it protects is precisely the thing that would silently stop running, so it cannot be the one to notice. It joins the four guards already running in workflow-trigger-lint.yml, which carries no paths filter and sees every pull request. * Keep the 30 minute budget per shard I cut this to 20 on the reasoning that a quarter of the job needs a quarter of the budget. Staging then cancelled three of the four shards at exactly 20m0s, all of them inside "Linux deps", an apt step that takes 13 SECONDS in the org repo. The work was not slow, the runner was. A timeout is there to catch a hang, not to police the mean. Sizing it to the expected duration converts an infrastructure stall into a red build on a shard where every test would have passed, which is a worse failure than the one the tighter number was meant to prevent, because it looks like the change broke something. * Capture the server logs on every shard, not just the last one The copy of ~/.unsloth/studio/logs lived inside the step that stops the third Studio, under a comment saying all three Studios share the directory. That was true when they shared a RUNNER. Each cell now has its own machine and its own logs directory, so gating it to picker uploaded three artifacts with no server-side traceback in them, which is the first thing anyone debugging a failed shard opens. The copy is now its own always() step on every cell; the IME process cleanup stays picker-specific, since that PID only exists there. Re-gating the copy to one shard fails the new assertion. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 18:49:10 -07:00
Daniel Han	2748b15eb3	One interpreter leg on a pull request, and a floor lint that reads more than syntax (#9100) ... * One interpreter leg on a pull request, and a floor lint that reads more than syntax A pull request ran 3.10 and 3.13. It now runs 3.13 only. Main still runs all four, so anything that needs a real run is caught at merge rather than never. The leg that goes is worth something, so this pays for it rather than dropping it. What a dropped floor leg actually stops catching is not syntax: it is reaching for a stdlib name that does not exist yet. core/research_runs.py already uses anext, which is 3.10, and that parses perfectly on every version and fails only when the line runs, so the existing ast.parse floor check would not have seen it. scripts/lint_backend_python_floor.py asks vermin instead, which reads syntax AND stdlib API availability, and takes its target from the workflow's own matrix rather than a number written in the script. Adding a call to itertools.batched, which is 3.12, fails it in seconds. It runs from workflow-trigger-lint.yml, which carries no paths filter, so it sees the pull requests that touch only backend source -- the ones that most need it now. The single leg has to be the NEWEST. Removals and deprecations land on the newest interpreter first and on the oldest never, so running only the oldest would be the wrong single choice; the guard asserts which end it is. What is genuinely given up, kept visible rather than deleted along with the old guard: the backend has version_info branches at 3.10, 3.11 and 3.12 boundaries, and a pull request no longer takes both sides of any of them. Nothing static covers that -- a parse reads both sides and runs neither. test_the_boundaries_the_subset_stops_executing_are_still_run_on_main lists them and fails if main ever stops running the full matrix, at which point this stops being a trade and becomes a straight loss. Mutation tested: making the single leg the floor fails one test, dropping the lint invocation fails another, removing vermin from the install fails it too, and taking Backend CI off push-to-main fails two. That vermin check needed a second pass: the first version looked for the string anywhere in the workflow and was satisfied by a comment mentioning it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Scan the backend tree, not a list of packages I remembered The floor lint named core, utils and routes, and silently missed 116 shipped files: all of hub, plugins, models, storage, auth, picker and state, plus _platform_compat.py, which main.py imports directly. It also named "loggers.py", which is a directory, so that entry matched nothing at all. With the 3.10 leg dropped this lint is the only thing looking at the floor before a merge, and a check that covers most of a tree reads exactly like one that covers all of it. It now scans studio/backend and excludes only tests and vendored code, which takes it from 307 files to 422. Verified by putting an itertools.batched call, which is 3.12, into each of hub, auth, picker, state, storage, models, plugins and _platform_compat.py in turn: every one is caught now, and none of them was before. Widening it immediately found something real, which is the point: locale.getencoding is 3.11 and the floor is 3.10. It turns out to be correctly guarded, in a try/except AttributeError whose fallback is locale.getpreferredencoding(False), commented "Python < 3.11". vermin reads names rather than control flow, so a guarded attribute lookup is indistinguishable from an unguarded one. That file is exempt with its reason printed on every run, and an exemption naming a path that no longer exists fails the lint, so it cannot outlive the guard it was written for. The guard test counts what the lint would hand to vermin against what is on disk, so narrowing the input back to a package list fails rather than quietly shrinking coverage. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Suppress the guarded call, not the file it lives in Excluding state_store.py wholesale left everything else in it permanently unchecked, which is the package-allowlist mistake from the previous commit one level down: a new unguarded 3.12 call anywhere in that module would have passed the floor lint on a pull request that runs only 3.13. The suppression moves to the site. vermin honours a `# novermin` annotation, so comment parsing is on now and the one guarded call carries the annotation with a note saying the except below IS the guard and that vermin reads names rather than control flow. The file is back in the scan, which is 422 files again rather than 421, and adding an unguarded itertools.batched call elsewhere in it now fails. The coverage test no longer permits any file-level exemption at all, rather than permitting a recorded one, so reintroducing the exclusion fails it. Separately: the assertion that the lint step exists was only collected by Backend CI, whose paths cover its own YAML and studio/**, not .github/workflows/**. A pull request editing only workflow-trigger-lint.yml could therefore delete the step without failing anything, which is the one change the assertion exists to reject. It runs from that unfiltered workflow now, alongside the three guards already there for the same reason. * Scan unsloth_cli on the floor as well, since the matrix runs it studio-backend-ci lists unsloth_cli/** in its own paths filter and runs pytest unsloth_cli/tests as a step on every leg, so the 3.10 leg this replaces was executing shipped CLI code on the floor interpreter, not only backend code. A lint aimed at studio/backend alone covers part of that while reading like it covers all of it, which is the same shape as the package allowlist the previous round removed, one level up. ROOTS is now both trees and targets() walks each of them, 441 files rather than 422, and the run is still clean at 3.10. The guard asserts on what the lint would actually hand to vermin rather than on its source, and dropping unsloth_cli back out fails it. * Lint the test code at the floor too, since the matrix executes it The first version dropped tests on the theory that they are not shipped. Shipping is not the question, execution is: studio-backend-ci runs pytest tests/ from studio/backend on every leg, so a 3.11 API in a test file is executed by the 3.10 leg exactly as one in a shipped module is. With the pull request down to a single 3.13 leg, that leg and this lint would both pass and the failure would arrive on the push to main, which is the gap this exists to close. Only vendored code comes out now, pinned to its own support range. 1093 files rather than 441, still clean at 3.10, so this costs nothing today and closes the hole. Putting tests back into EXCLUDE_PARTS fails the new guard. * Run one interpreter and defend the floor statically The 3.10, 3.11 and 3.12 legs are gone from Backend CI, on pull requests and on main alike. Measured on one runner over the same tree, the four legs collected the same 26,320 tests and differed by exactly one: the >= 3.12 gate on test_demonstrates_the_underlying_stdlib_regression. 3.10 and 3.11 reported 26193 passed / 127 skipped, 3.12 and 3.13 reported 26194 / 126. That is 97 runner-minutes per push to run one identical suite four times and learn the value of a single skip marker, into a queue that has been observed 195 deep, and queue depth is wall-clock for every other workflow in the repo. What the older legs were really defending is that nothing reaches for a symbol newer than the floor, which is static. scripts/lint_backend_python_floor.py now checks exactly that, on every pull request, in seconds, across 1093 shipped and executed files, reading stdlib API availability rather than syntax alone. The floor is DECLARED, as PYTHON_FLOOR in the workflow, next to where the legs used to be. Deriving it from the matrix was right while the matrix ran several interpreters and becomes self-defeating with one: a 3.13-only matrix would move the floor to 3.13 and leave the lint asserting that code written for 3.13 runs on 3.13. 3.10 rather than the 3.9 pyproject.toml declares, because 3.9 is not true today. unsloth/models/_utils.py already uses dataclasses.dataclass(kw_only) and tempfile.TemporaryDirectory(ignore_cleanup_errors), both 3.10, so a 3.9 target fails on the tree as it stands. Either the declaration or those two call sites has to give, and that is worth its own change; this lint is what made the mismatch visible rather than what hides it. The cost is stated rather than buried. A static check does not run anything, so the sys.version_info branches in sitecustomize.py, native_path_leases.py, third_party_source.py and worker.py are now covered by reading and by the lint's view of the names they use, not by execution. The guard that used to assert main still ran them asserts instead that every file carrying such a branch is inside the lint's scan, since that is the only check left on them. * Keep executing the pre-3.12 branches, and pin the ceiling by name Two from review. The first is the honest objection to a 3.13-only matrix on push as well as on pull requests: a break in a supported older runtime path that uses no newer stdlib name passes the lint and is then executed nowhere. So the branches were counted rather than argued about. Seven backend files carry a sys.version_info comparison, at 3.10, 3.12 and 3.14. The 3.10 ones were never straddled even by the old matrix, whose oldest leg WAS 3.10, so every leg took the same side of them and dropping legs loses nothing there. 3.14 is above every leg there has ever been. What is genuinely lost is the pre-3.12 side of three files, and that is small enough to keep running: a second matrix entry on 3.11, the newest version that still takes that side, running those three files and nothing else. 57 tests in under seven seconds, beside the full leg rather than in front of it, so the critical path is the full leg either way. It is not a second copy of the suite, and the four legs it replaces are still gone. The second is that asserting the sole leg is merely above the floor let 3.11 or 3.12 satisfy it, which would give up the removals-and-deprecations coverage that is the entire reason the single leg is the newest one. The ceiling is now written down and compared by name, so moving it is a decision somebody makes and defends in the same change. Both new assertions fail when mutated: pointing the full leg at 3.12 fails the ceiling test, and pointing the spot-check leg at 3.13 fails the pre-3.12 test because it would then re-test what the full leg already covers. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 18:48:26 -07:00
Daniel Han	af2fa437a2	Give each xdist worker its own torch.compile cache directory (#9134) ... Inductor's on-disk caches default to one directory per USER, not per process, so the four workers under pytest -n 4 share /tmp/torchinductor_<user> and its fxgraph, aotautograd and Triton subtrees. The upstream recipe is explicit that a common TORCHINDUCTOR_CACHE_DIR is what makes processes SHARE compiled artifacts, so a different value per worker is how they are kept apart. Sharing is not obviously wrong, since the entries are content-addressed. It is still a write-write interaction between processes that nothing here controls, and a cache is exactly the sort of thing that turns a deterministic suite into an intermittent one. This removes the interaction for the price of some recompilation. What it buys, measured rather than assumed, so nobody has to guess later: a full unsloth_zoo run against an empty dedicated cache directory took 578s and left ZERO entries in it. That suite never populates the on-disk cache, so for that step this is neither a saving nor a cost. It is insurance for the suites that do compile, applied wherever tests run in parallel rather than only where a problem has already been seen. It has to run before torch is imported, so it is a module-level import in the two conftests rather than a fixture. TRITON_CACHE_DIR is set explicitly rather than left to follow, because it only derives from the inductor directory when unset and an environment exporting it would keep all four workers on one Triton cache. An explicit TORCHINDUCTOR_CACHE_DIR is split underneath rather than replaced, so a CI path chosen on purpose is respected. A single-process run is left alone entirely. Verified under real xdist, not only in unit tests: four workers report four distinct directories, and the parallel suites still pass.	2026-08-17 18:48:11 -07:00
Daniel Han	019ede777d	Stop the banner layout suite waiting out the update timer on every boot (#9105) ... * Stop the banner layout suite waiting out the update timer on every boot Update banner layout regression is the largest step in Chat UI Tests at 4.98 of the job's 17.6 minutes, and the second banner step adds 2.27 more. Together that is 41 percent of a job that runs on Linux, Windows and macOS. Where it goes: the suite boots a fresh page for each case it measures, and the card it is measuring is mounted by the app's 5s update-check timer, so wait_for_selector on it cannot return sooner than that. At 33 boots the timer alone is over two and a half minutes. The remaining per-boot time is the 900ms animation settle and settle_stack, both of which are render waits, both of which this leaves exactly as they are. Measured earlier as 33 gaps of about 7.5s each, which is 5 for the timer plus 0.9 plus roughly 1.6 of stability polling: the stability poll is converging, not exhausting, so it is not the thing to change. So the delay becomes overridable from a global that the hook reads AT MOUNT, and the suite sets it in the init script it already installs, to 150ms. Not zero: a card that mounts on the first frame would stop exercising the late-mount reflow this file exists to measure. Production is untouched, since the global is undefined in every real browser and the 5000ms constant stands, and it is a global rather than an env var so no build can bake the short delay in. The two sides are connected by nothing but the spelling, and a typo on either would fail in the worst way available: the timer quietly stays at 5s, the step goes back to being slow, and every assertion still passes. The new test asserts the spelling, that the suite sets it before the app runs rather than after navigation, and that the production constant is still 5000. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 18:48:01 -07:00
Daniel Han	a14c289a40	Delete a test that never ran, and stop another appearing (#9135) ... TestParser.test_xml_param_preserves_leading_indentation was defined twice in test_safetensors_tool_loop.py, at lines 120 and 156. Python keeps the last definition, so the first was overwritten before pytest ever collected it: present in the file, maintained, reviewed, and never run. Nothing reports that. It is not a failure, not a skip, not an error. The test is absent, and absence is what green looks like. The two bodies were equivalent, so no coverage is lost by removing the dead one. The surviving copy relies on the module-level json import rather than a local one, which is why only it works standalone. Found by fingerprinting all 23,895 test bodies in the repo and looking for duplicates. Exactly one shadowed definition in the whole tree, which is a good result and cheap to keep at one, so the sweep is now a test. It looks at module and class scope, which is where pytest collects from; a function nested inside a function is not collected either way and is not its business. Worth recording from the same sweep, since it argues against a change rather than for one: 72 test bodies out of 23,895 are byte-identical to another, 0.3 percent, and almost all are two-statement tests applying one assertion to different subjects from different classes. There is no CI time in de-duplicating them. The first version of that sweep ignored decorators and reported 104, including three tests in test_control_markup_neutralize_7066.py whose bodies are identical and whose @pytest.mark.parametrize supplies different schemas. Deleting two of those would have removed real coverage with every remaining test still green, so the fingerprint now includes decorators.	2026-08-17 18:47:38 -07:00
Daniel Han	019a4819d4	Ask the repo, not sys.path, whether a stubbed name is third-party (#9133) ... * Ask the repo, not sys.path, whether a stubbed name is third-party The stub-shadowing guard I added is failing Repo tests (CPU) on main: assert not {'test_llama_cpp_wait_for_vram_settle.py': ['loggers']} loggers is the backend's OWN module. Stubbing it shadows nothing, and the guard is about third-party libraries. The bug is the reasoning in its own docstring, which said an in-repo name resolves only with studio/backend on sys.path, "which this test does not have and should not add". That is untrue in the job that runs it: under pytest tests/ -n 4 from the repo root, studio/backend does end up on sys.path, find_spec("loggers") resolves, and the guard fires on a stub that is doing nothing wrong. It passed locally, where the path differs, which is the worst shape a CI-only assertion can have and the reason it reached main. So the question is answered from the tree, which is the same everywhere: if studio/backend defines the name, it is repo code and a stub for it is substitution rather than shadowing. importlib is consulted only for the rest. Reproduced both ways under the runner's path: PYTHONPATH=studio/backend fails on the old code with exactly the CI message and passes on the new. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 18:43:27 -07:00
Daniel Han	96cf275d71	Run the Backend CI matrix in parallel, like its sibling job already does (#9095) ... * Run the Backend CI matrix in parallel, like its sibling job already does The matrix leg is the longest job in the repo at 23.3 minutes, and it was the only large pytest run still serial. repo-cpu-tests has run -n 4 since it measured 806s -> 220s. Measured over the same tree in the same environment, serial against -n 4: 1322.6s -> 343.0s, and the results are identical -- 51 failed / 26190 passed serial, 51 failed / 26188 passed parallel, with the two failure sets compared name for name and equal. So nothing in the backend suite depends on the order it runs in. (Those 51 are one local environment missing peft and a diffusers pin. The point is that the two modes agree.) This is CPU bound and not memory bound: the suite loads no model, unlike the inference smoke workflows where four workers on one runner would not fit. The existing isolation guard caught this change, correctly: it assumed exactly one parallel pytest run, and the two now run over different trees. The repo-root job runs tests/ from the checkout, the matrix job runs the backend's own suite from studio/backend, so tests/studio/load_freeze is not a path that exists for it and demanding those ignores would be nonsense. The guard now tells the two apart and applies the isolation rules to the repo-root run only, and a new test pins the matrix leg as parallel so losing the flag shows up as a failure rather than as CI slowly getting slower again. * Keep the relative-timing tests off the parallel workers Staging caught what the local comparison could not: the 3.10 leg reported 'early markup cost 1.354s against the reference's 0.854s' and 'incremental cost grew 7.0x vs the reference's 11.5x', while 3.13 passed the same commit in 9 minutes against the 23 it used to take. test_streaming_stripper times itself against a reference implementation measured in the same process. Under four workers on four vCPUs one side of that ratio gets descheduled and the other does not, so the comparison stops being between two implementations. It is the same reason repo-cpu-tests already keeps load_freeze out of its parallel run, and it does not reproduce on a machine with cores to spare, which is why the local run agreed serially and in parallel. So it is ignored from the parallel run and runs again in its own serial step, and the isolation guard now holds that pair together for the backend run the way it already does for the repo-root one: putting the file back in the parallel run fails one test, deleting the serial step fails another. * Find the tight elapsed-time bounds by scanning, not by remembering Two more files assert ABSOLUTE elapsed time, and tightly: 50ms for a short-circuit that should not run the probe at all, and 100ms for a regex backtracking guard. Bounds that small sit inside one scheduler quantum, so under four workers on four vCPUs they measure the scheduler as much as the code. Both passed on staging, which is the problem: they would have flaked later, on somebody else's change. Twenty-two backend files assert some elapsed bound, and serialising all of them would give back most of what -n 4 buys. So the line is drawn at 0.1s, where the measurement stops being about the code, and the three files at or below it are ignored from the parallel run and rerun serially, which costs 2.2s and 1.7s. The guard now finds them by scanning rather than by listing. It reads with ast, so the name has to be assigned from a difference of two clock readings: grepping for '< 0.05' would match a float tolerance, and grepping for 'elapsed' would match anything. A new test asserting a 20ms bound fails that guard instead of buying a flake, which is verified by adding one. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run the Backend CI matrix in parallel, like its sibling job already does The matrix leg is the longest job in the repo at 23.3 minutes, and it was the only large pytest run still serial. repo-cpu-tests has run -n 4 since it measured 806s -> 220s. Measured over the same tree in the same environment, serial against -n 4: 1322.6s -> 343.0s, and the results are identical -- 51 failed / 26190 passed serial, 51 failed / 26188 passed parallel, with the two failure sets compared name for name and equal. So nothing in the backend suite depends on the order it runs in. (Those 51 are one local environment missing peft and a diffusers pin. The point is that the two modes agree.) This is CPU bound and not memory bound: the suite loads no model, unlike the inference smoke workflows where four workers on one runner would not fit. The existing isolation guard caught this change, correctly: it assumed exactly one parallel pytest run, and the two now run over different trees. The repo-root job runs tests/ from the checkout, the matrix job runs the backend's own suite from studio/backend, so tests/studio/load_freeze is not a path that exists for it and demanding those ignores would be nonsense. The guard now tells the two apart and applies the isolation rules to the repo-root run only, and a new test pins the matrix leg as parallel so losing the flag shows up as a failure rather than as CI slowly getting slower again. * Keep the relative-timing tests off the parallel workers Staging caught what the local comparison could not: the 3.10 leg reported 'early markup cost 1.354s against the reference's 0.854s' and 'incremental cost grew 7.0x vs the reference's 11.5x', while 3.13 passed the same commit in 9 minutes against the 23 it used to take. test_streaming_stripper times itself against a reference implementation measured in the same process. Under four workers on four vCPUs one side of that ratio gets descheduled and the other does not, so the comparison stops being between two implementations. It is the same reason repo-cpu-tests already keeps load_freeze out of its parallel run, and it does not reproduce on a machine with cores to spare, which is why the local run agreed serially and in parallel. So it is ignored from the parallel run and runs again in its own serial step, and the isolation guard now holds that pair together for the backend run the way it already does for the repo-root one: putting the file back in the parallel run fails one test, deleting the serial step fails another. * Find the tight elapsed-time bounds by scanning, not by remembering Two more files assert ABSOLUTE elapsed time, and tightly: 50ms for a short-circuit that should not run the probe at all, and 100ms for a regex backtracking guard. Bounds that small sit inside one scheduler quantum, so under four workers on four vCPUs they measure the scheduler as much as the code. Both passed on staging, which is the problem: they would have flaked later, on somebody else's change. Twenty-two backend files assert some elapsed bound, and serialising all of them would give back most of what -n 4 buys. So the line is drawn at 0.1s, where the measurement stops being about the code, and the three files at or below it are ignored from the parallel run and rerun serially, which costs 2.2s and 1.7s. The guard now finds them by scanning rather than by listing. It reads with ast, so the name has to be assigned from a difference of two clock readings: grepping for '< 0.05' would match a float tolerance, and grepping for 'elapsed' would match anything. A new test asserting a 20ms bound fails that guard instead of buying a flake, which is verified by adding one. * Teach the timing scan the two shapes it was blind to The scan only recognised a comparison whose left operand was a name assigned from a clock difference. Two forms in this suite are written differently and were silently skipped: time.monotonic() - started < 0.2 the difference inline (test_stt_download_followups) _elapsed(big) < 8 * _elapsed(small) a helper returning one (test_diffusion_checkpoint_resume) The second is not a near miss. It compares two wall-clock measurements taken in sequence, so descheduling one side and not the other breaks it at ANY magnitude, with no threshold to be under, which is the same reason test_streaming_stripper came out of the parallel run. It was still running under -n 4. So the scan now asks whether an expression IS a duration, however it was spelled: a name assigned from a difference, a difference written inline, or a call to a function that returns one, found by walking for a return of a clock difference at any nesting depth -- the helper in question is defined inside its own test. And a relative comparison is fragile regardless of magnitude, while an absolute one still has to be at or below the threshold. test_diffusion_checkpoint_resume joins the serial step, costing 8.9s. Adding either shape to a file that is not isolated fails the guard, both verified. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow the clock value through a container, and record what is benign The scan tracked names assigned a clock DIFFERENCE. test_tool_output_streaming compares first_seen_at[0] - started against finished - started - 0.5, where every term is an instant, no single name ever holds a duration, and one of them is parked in a list by a callback. Nothing in it looked timed, so the file kept running under -n 4 while asserting that a callback fired at least 0.5s before the child exited -- which collapses if the worker is descheduled while the child sleeps. So instants count now, not just differences, including one appended to a container, and the check walks the expression rather than reading its top node. That widened net found four more files, and only two are real: test_web_fetch_extraction compares parse time at two input sizes, and test_tool_output_streaming is the above. The other three are not performance claims at all. A sandwich, "before <= recorded <= after", cannot be falsified by widening the gap; a poll deadline inside a wait-for-condition loop is the pattern that replaces a guessed sleep; and "stamp < 0.0" compares against a sentinel. Those are in BENIGN_TIMING with their reasons, keyed on the enclosing function so an edit above them does not move the exemption onto something else. Keeping the net wide means a new benign pattern lands here too, so the failure message now says which of the three ways out applies rather than assuming the test is wrong. Verified: a stored-instant comparison added to an unisolated file fails the guard, and the two new files cost 39.5s and 13.3s in the serial step. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow a duration back through the name a helper returned it in The helper check required the return expression itself to read a clock. test_tool_call_parser_strict has def best_ms(depth): best = float("inf") for _ in range(5): t0 = time.perf_counter() ... best = min(best, time.perf_counter() - t0) return best t200 = best_ms(200) t400 = best_ms(400) assert t400 < t200 * 3.0 where the return reads no clock, and neither does the assignment that holds the result. Two links were missing, not one: a function counts as a timing helper if it returns any of its OWN timed names, and a name counts as timed if it was assigned from a call to such a helper. Helpers are resolved first for that reason, and to a fixpoint, so a helper built on another helper is found on the next pass rather than missed. Worth noting as a check on the scan rather than on this test: test_streaming_stripper is now found by the scan on its own, having been in the isolation list by hand since the run that started this. The rule and the list agree where before only the list knew. test_tool_call_parser_strict joins the serial step at 2.0s. A helper returning a duration through a local name, added to a file that is not isolated, fails the guard. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Isolate the keepalive test staging caught, and say what the scan cannot see test_tunnel_safe_long_post::test_the_route_still_pads_the_same_slow_load failed on a staging 3.13 leg that had been green. It patches the keepalive threshold to 0.05s and makes the work sleep 0.2s, then asserts the response starts with padding. Whether it passes turns on which of two timers fired first, and four times the threshold was not enough margin under four workers on four vCPUs. The scan did not find it and structurally cannot: it looks for assertions COMPARING clock-derived values, and this one has no clock in it at all. The dependency is implicit, between a patched constant and a sleep, and the assertion is on the result. Ten backend files pair a sub-second sleep with a small threshold. Since 4x margin already proved insufficient, the ratio is not a usable rule, and flagging all ten would serialise a large part of the suite on a guess. So that class stays with staging, which is the only thing that has ever caught one, and the limit is written next to BACKEND_ISOLATED rather than left for the next person to discover the same way. The file joins the serial step at 3.9s. * Isolate the heartbeat-count test, and correct the note about how this class is found test_scan_loras_off_event_loop counts how many times a heartbeat coroutine ticked during a 0.3s sleep and requires at least three. Descheduling the worker costs ticks without the scan being wrong, so it fails for a reason that has nothing to do with the code under test. Same class as the keepalive test in the previous commit, from the other direction: there the assertion was on a result, here it is on a COUNT, and either way there is no duration in the expression for the scan to find. The previous commit said this class was left to staging, "the only thing that has ever detected one". That was true when written and is not now: this one came from review, on a file staging had not yet failed on. Found by reading, not by scanning, is the accurate statement, and the note says that instead. The file joins the serial step at 1.6s. * Isolate the anthropic keepalive counts from the xdist workers Codex found test_scan_loras_off_event_loop by reading, not by running: it counts heartbeats across a 0.3s sleep, and the assertion compares a count rather than anything clock-derived, so the AST scan in this file is structurally unable to see it. That is a class, not a one-off, so I read the rest of the suite for the same shape instead of waiting for staging to hit it. One more: test_anthropic_messages asserts len(keepalives) >= 3 and >= 2 across a _time.sleep(0.24) stall past several shortened keepalive windows. A descheduled worker loses keepalives exactly as the heartbeat test loses ticks. It costs 2.2s to run serially. One false positive worth naming, because the grep that finds these is crude: test_diffusion_backend asserts len(staged) > 1 near a 0.2s sleep, but staged is a list comprehension over cached filenames with no timing in it. It also costs 152s, so matching on the pattern alone would have been expensive as well as wrong. * Read every link of a chained comparison, not just the first A tight bound is often written as a sandwich, and a sandwich is one Compare node whose leftmost operand is the literal floor. Requiring that operand to be timed made the scan skip the upper link entirely, so a file could hold an assertion of the shape it exists to find and stay in the -n 4 run with the guard green. test_llama_cpp_wait_for_vram_settle already writes a bound that way. Verified by running the scan over a file with 0.3 <= elapsed < 0.05, which it now reports and previously did not. The walk also reads Gt and GtE by swapping the operands, since a bound written backwards bounds the same thing. That turned up one live case, an access token asserted to expire after the wall clock. The margin is 600 seconds, so reading both sides late by whole seconds still leaves it true, and it goes in BENIGN_TIMING rather than into the serial step. * Stop the test stubs shadowing httpx once the suite runs in small pieces The 3.10 leg failed collection on two of the ten files in the new serial step, on module 'httpx' has no attribute 'Response', and it is worth being precise about what that is: httpx is installed on that job. Thirteen backend modules build a fake one and install it with sys.modules.setdefault, which reads as deferring to the real library and does not. sys.modules holds what has been IMPORTED, not what is installed, so in a process where nothing has touched httpx yet the stub wins and shadows it for the rest of the session. The stubs have no Response, starlette.testclient reads httpx.Response at import, and everything collected afterwards that reaches fastapi.testclient or routes.inference dies. In a 26,000-test run something always imports httpx before any of them is collected, which is why this has been invisible for as long as the suite ran as one process. Splitting the timing tests out removed the accident rather than introduced the bug, and any future split would have found the same landmine. All thirteen now try the real import first, the form test_llama_cpp_placement.py already uses. Reproduced before the change by collecting wait_for_vram_settle and diffusion_checkpoint_resume together, which errored, and after, which collects 154 tests. The full suite still collects 26391. The guard is scoped to the isolated files. Around fifty other modules stub structlog the same way and are load-bearing in a run that also imports the real one, so rewriting those is a separate change with its own risk. What has to hold here is that anything moved OUT of the parallel run stands on its own. * Propagate helpers through assigned results, and isolate one more tick count Two from review, both real. The fixpoint over timing helpers called _timed_names without the helpers it had already found, so a wrapper that assigns value = base() and returns value never learned that value was timed. base was discovered, the wrapper was not, and any relative benchmark built on the wrapper stayed invisible. The pass that learns a helper is not the pass that reads its callers, which is the whole reason this runs to a fixpoint, so the set has to go in as well as come out. Verified on a base/wrapper pair the scan now reports and did not before. test_profile_stats counts event-loop ticks during a 0.5s blocking call and needs more than ten of the roughly fifty that fit. That is the same shape as the two tick counts already isolated: no clock appears in the assertion, so the scan cannot see it, and a descheduled worker loses ticks. 12.8s serially. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>	2026-08-17 08:23:36 -07:00