Studio: keep the streaming render harness the perf PRs kept rebuilding - #8969
Conversation
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
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.
Comments and docstrings only, no code change. Every measured number, PR reference and causal reason is kept verbatim.
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
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.
… 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.
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Comments only, no code change. Every measured number and every causal reason is kept.
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.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64baedbaf7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| const painted = paintedChars(); | ||
| if (painted > state.paintedChars) { | ||
| const stall = now - lastGrowthAt; |
There was a problem hiding this comment.
Cap late-growth stalls at the stream end
When the stream ends during a main-thread freeze and the first frame afterward observes newly painted text, this growth branch records now - lastGrowthAt, bypassing the stream-end cap used by the no-growth branch below. The resulting longestStallMs includes arbitrary post-stream delay even though the metric and its 2,500 ms budget are defined only while text is arriving, so a late final paint can produce a false budget failure; apply the same stream-end cap when closing a stall on growth.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Measured this both ways and the cap is what would break the number, not what fixes it, so I am leaving the growth branch uncapped.
The asymmetry is deliberate. The no-growth branch has to cap at stream end or the 30 quiet frames the settle check needs would be reported as a stall on every healthy run. The growth branch must not, because a growth event is a real paint: the interval it closes ends at the moment arrived text finally reached the screen. Capping it would make never painted score better than painted late.
Numbers, Chromium at 6x throttle. In a burst config, chunkChars 4000, the last text arrives at 2547ms and the bubble does not grow again until 4521ms. The branch as written reports 2017ms. The proposed cap reports 967ms and discards 1974ms of arrived-but-unpainted text, which is exactly the freeze class this metric exists to catch. In the shipped 96-char config the cap changes longestStallMs by 0.0ms, 1200.1ms either way, because the maximum came from 1060 to 2260ms, mid-stream, with 43.2ms of post-stream exposure against a 2500ms budget.
There is also no idle-time vector: post-stream, paintedChars only rises when the DOM textContent grows, so the growth branch cannot fire while idle, and the driver stops polling as soon as done flips.
Separately, your item made me re-run the harness end to end, which turned up that the page had stopped loading at all after the main merge. Fixed in 3d0bbce.
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.
|
@codex review |

Four merged PRs moved the chat streaming render path, and each built a throwaway harness to prove it:
Nothing was left behind that would notice the next regression, and each measurement had to rediscover the same methodology. #8845 needed four attempts before its numbers meant anything: a real model gave the two sides different essays, generating the stream in-page competed for the throttled CPU budget, a CSP silently blocked the helper server, and a wall-clock quiet window declared a reply finished in the middle of a freeze.
This keeps the harness.
What it runs
smoke-stream-pacing.htmlmounts the realMarkdownTextinside a real assistant-ui local runtime, in the same shape as the existingsmoke-research.htmlandsmoke-autoscroll.html: a vite entry, no backend, no auth, no GPU, no model. A local runtime rather than a bare component becauseMarkdownTextreads its part from assistant-ui context and itsBlockComponentreadsuseAuiState, and because assistant-ui's own update scheduling is where #7892's starvation lived.tests/studio/playwright_stream_pacing.pydrives it under CPU throttling and reports longest stall, long-task total and count, frames over 33 ms, and time to fully painted.Three methodology points are carried over deliberately, each from a way a measurement went wrong before:
Choosing the budgets
A budget picked by feel is not a gate. Two merged fixes were reverted in this harness and measured on two different machines, and they move the two numbers in opposite directions:
Reverting #8750 (incremental Markdown parsing) blows up the long-task total and leaves the longest stall alone. Reverting #7892 (the Streamdown
animatedconfiguration that keeps block updates out of an interruptible transition) does the exact opposite: the stall goes 4 to 5x while the long-task total stays inside the clean range.So both budgets are load-bearing, and a single headline metric would have missed one of these two outright. Every mutated run exits 1 on the budget you would expect and every clean run exits 0, over sixteen clean runs, six with #8750 reverted and five with #7892 reverted.
Machine spread, and why this does not gate yet
Clean long-task readings are 5,029 and 5,901ms on one machine and 6,687 to 8,003ms over five runs on another. So the number varies around 60% across boxes even though a single box repeats to within around 15%, and the headroom over the 10,000ms budget is 1.7x on the first machine but only 1.25x on the second.
The budget is deliberately not raised to buy that headroom back, because the same two machines read the #8750 revert as 13,059ms and 74,353ms. A budget loose enough to be comfortable on the slower box stops catching that regression on the faster one. That tension is the whole reason the step keeps
continue-on-error: true: tighten it from observed runner numbers, not from any one machine.The longest-stall budget needs no such hedging. 2,500ms sits at 2.0x above the worst clean run and 2.0x below the mildest #7892-reverted run, and the stall metric repeats to within 4% on a single box.
What stops it reading a false pass
A harness that cannot fail is worse than nothing, so every number the budgets rest on is asserted to have measured something, and every number is scoped to the stream rather than to the page:
paintedCharsis a high-water mark and only ever climbs, so a completion render that truncated the bubble would leave the peak behind and pass the floor on a DOM that no longer held the reply. The length present at settlement is recorded separately and checked against the same floor.observe({type: "longtask"})is specified to abort silently on an engine that lacks the type rather than throw, so atry/catcharound it never fires:longtaskis Chromium-only, and under firefox or webkit the long-task total stayed at 0 and sailed under the budget. Support is detected withPerformanceObserver.supportedEntryTypes, recorded, and failed on.smoke-stream-pacing-stall.tswith its own tests, two of which fail if the previous rule is put back.Long tasks and slow frames are counted only from the moment the stream is asked for, so page load and module evaluation are not budgeted as render cost. That was measured at one entry of about 140ms, roughly 2.6% of a clean total. A long task carries the start time of its whole task, so the message append is handed to a later task, or runtime startup and the first publish would sort before the window and be dropped as page load.
A broken page fails fast rather than hanging: with the fixture deliberately crashed on load the driver exits 1 in 65s on the readiness wait, well before the 300s settle deadline.
Chromium only, and deliberately
Both of the things that make this a measurement are Chromium-only:
Emulation.setCPUThrottlingRateis reached over CDP, which Playwright exposes for Chromium alone, andlongtaskentries exist in no other engine (the Gecko bug is open, WebKit has never shipped them). Verified against all three engines: the page renders fine under firefox and webkit,new_cdp_sessionraisesCDP session is only available in Chromium, andsupportedEntryTypesexcludeslongtaskwithoutobserve()throwing. Hence the explicit support check rather than acatch.This step will not execute yet
The step is added after
Browser smoke for ANSI tool output, which is currently failing onmainand does not carrycontinue-on-error. A failing step without it skips every step after it in the job, so this one reports asskippedrather than running, which is what happened on a replicated CI run of this branch. That is being handled separately and is not fixed here, but it is worth stating plainly: until it is resolved, a green Frontend CI is not evidence that this harness ran.It is also why the budgets are still calibrated from developer machines rather than from runner numbers.
Limitation
createStreamPublishGatefrom #8845 lives in the Studio chat adapter, not in this render path, so this harness does not exercise it. What it covers is everything downstream of the yield: the Streamdown configuration, the incremental Markdown cache, the fence tokenizer, and assistant-ui's update scheduling.A separate finding, not fixed here
The entry opens with a deliberate side-effect import of the sidebar organization store. Without it the harness renders nothing and the page throws
Cannot access 'SIDEBAR_ORGANIZATION_STORAGE_KEY' before initialization.The underlying cycle is real app code, not a harness artefact: the chat barrel re-exports
chat-page, which reaches the tour barrel, whose confetti helper imports the settings barrel, which imports the General tab, which importsSIDEBAR_ORGANIZATION_STORAGE_KEYback out of the chat barrel and reads it at module evaluation time in a top-levelconst PREFS_KEYSarray. The app survives only becauseapp-sidebar.tsxhappens to import the theme toggler, and so the settings barrel, above its own chat import. Moving that one import block up reproduces the same white screen in the real app.So this is a latent app bug worth its own change, not something to fix in a test-only PR. #8966 makes the settings tabs lazy, which cuts that edge and makes the workaround here unnecessary; the harness was verified to load clean with #8966 applied and the workaround removed. Whichever lands first, the root fix is for the General tab to read the constant from the store module rather than through the barrel.
Testing
python3 tests/studio/playwright_stream_pacing.py: sixteen clean runs, six with Fix Studio CPU saturation on long streaming replies #8750 reverted, five with Studio: prevent long streaming Markdown stalls #7892 reverted, numbers abovelongtaskavailabilitynpm test(2,787 passed, including 5 new for the stall rule),npm run typecheck,npm run builddist/confirmed to contain no reference to the smoke entry, and the entry is not inindex.html's import closure, so neither the bundle nor the 75 MB budget movespytest tests/studio/test_playwright_server_lifecycle.py tests/studio/test_autoscroll_harness_contract.py(27 passed) with the harness registered in bothruff checkon the driver,npx eslinton the harness entry, cleanThe harness had stopped loading, and CI could not tell us
Recorded here because it is the kind of thing that comes back silently.
After this branch merged
main, the smoke page stopped loading entirely. Loadingsmoke-stream-pacing.htmlon the branch head produced an empty#rootand a single page error:and the driver then died on a 60 second
wait_for_functiontimeout. Every other smoke page on the same tree loaded fine, so it was specific to this one.The cause is a second edge in the cycle described above, added by
mainrather than by this branch.#9014introduced a module-scope literal inthread.tsx:This harness enters the graph at
markdown-text.tsx, which imports the chat barrel, which reacheschat-page, which reachesthread.tsx, which readsMarkdownTextwhile that binding is still in its temporal dead zone. Module bodies evaluate once and live bindings start uninitialized, so a cycle is only harmless while every access is deferred into a function body, and this one is not. The workaround already at the top of the entry broke the first edge;mainadded a second and the trick went stale.Fixed by importing the chat barrel before
MarkdownText, mirroring the app's own entry order.The part worth stating plainly: CI stayed green through all of it. The step that runs this harness carries
continue-on-error: true, so a page that could not load, and a driver that timed out after 60 seconds, both reported as a passing job. The budgets this PR exists to enforce were not being enforced; nothing was being measured at all.That argues for distinguishing the two failure kinds at the exit-code level, so that "the page is dead" can gate the job while the timing budgets stay advisory. That is a change to the workflow rather than to this harness, so it is not made here, but the current arrangement means a green Frontend CI is not evidence that this harness ran, let alone passed.
Separately,
smoke-stream-pacing.htmlis a new HTML entry and did not load/crypto-boot.js, which#9075added a test for. That was the live red on this PR and is fixed in the same commit.