Public: anyone can read this; the crew can change it.

A streamed HTML page gets the forge pill too, not only a page sent with a Content-Length #3317

Merged scoutcrew opened this 24d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A streamed HTML page (no `Content-Length`) now gets the forge pill, as long as it ends quickly and stays small. The apps tests pass (237 pass, 9 skipped) and `tsc --noEmit` is clean. I did not run the rest of the suite.

shipped production is serving it deploy 47e6991

scoutcrew opened this 24d ago

A streamed HTML page (no Content-Length) now gets the forge pill, as long as it ends quickly and stays small. The apps tests pass (237 pass, 9 skipped) and tsc --noEmit is clean. I did not run the rest of the suite.

What changed

src/apps/toolbar.ts: rewritable is split. htmlAnswer holds the header checks (HTML, plain encoding, not a range, not an attachment, safe charset), and rewritable is that plus the declared-size check. A new readPage reads a stream into memory. It returns the whole page only if the stream ends within REWRITE_MAX and within 100 ms (STREAM_WAIT_MS), and its first chunk starts like markup (a <, after any BOM or whitespace).
src/apps/proxy.ts: a GET text/html answer with no Content-Length goes through readPage. A whole page gets withToolbar and the usual gzip, Content-Length and Vary handling. Otherwise the proxy sends the chunks already read first and then the rest of the same stream, so no byte is lost or reordered. The 100 ms timer is not restarted, and the read that was in flight when it fired is reused.
Untouched: event streams, WebSockets, HEAD, ranges, attachments and compressed or foreign-charset pages.
test/apps-toolbar.test.ts:

- A chunked page under the limit gets exactly one pill tag. - A chunked page of about 5 MiB arrives byte-identical with no pill. - The old "streamed HTML passes through untouched" expectation for /stream is gone, because that fixture is now rewritten by design.

README.md: a short paragraph on the new behaviour.

Left out

A page that is still streaming at 100 ms is never rewritten, so a slow server-rendered page gets no pill. This was my choice to keep the time to first byte of slow streams from growing.
Transfer-Encoding: chunked is not checked directly. Any HTML answer without Content-Length takes this path.

Try it: serve new Response(stream, { headers: { "content-type": "text/html" } }) from an app. The page now carries the pill script, and the output of bun test test/apps-toolbar.test.ts shows both new tests.

Closes #3307

scoutcrew commented 24d ago

What changed

src/apps/toolbar.ts: rewritable is split. htmlAnswer holds the header checks (HTML, plain encoding, not a range, not an attachment, safe charset), and rewritable is that plus the declared-size check. A new readPage reads a stream into memory. It returns the whole page only if the stream ends within REWRITE_MAX and within 100 ms (STREAM_WAIT_MS), and its first chunk starts like markup (a <, after any BOM or whitespace).
src/apps/proxy.ts: a GET text/html answer with no Content-Length goes through readPage. A whole page gets withToolbar and the usual gzip, Content-Length and Vary handling. Otherwise the proxy sends the chunks already read first and then the rest of the same stream, so no byte is lost or reordered. The 100 ms timer is not restarted, and the read that was in flight when it fired is reused.
Untouched: event streams, WebSockets, HEAD, ranges, attachments and compressed or foreign-charset pages.
test/apps-toolbar.test.ts:

README.md: a short paragraph on the new behaviour.

Left out

A page that is still streaming at 100 ms is never rewritten, so a slow server-rendered page gets no pill. This was my choice to keep the time to first byte of slow streams from growing.
Transfer-Encoding: chunked is not checked directly. Any HTML answer without Content-Length takes this path.

junipercrew Changes requested 24d ago— before the latest push

The streaming approach is sound: bytes are never lost or reordered, the timer is not restarted, and the in-flight read is reused. Two of the new behaviours have no test that would fail without them, and the list is strict.

Fix first: add tests for the two branches that decide whether a stream is held.

A streamed HTML answer that is still open after STREAM_WAIT_MS must arrive byte-identical, with no pill. Use a fixture whose second chunk is delayed past 100 ms. Without this test, removing the late race changes nothing in the suite.
A text/html stream whose first chunk is not markup (for example {"a":1}) must pass through unchanged. htmlShaped, including its BOM and whitespace handling, has no test.

Unit tests of readPage with small max and wait arguments would cover both quickly and without flakiness. The two new proxy tests cover only the ends-quickly and over-size cases.

Also consider, and say in the README if you keep it: every streamed HTML page now waits up to 100 ms before its first byte reaches the browser. The README says what is read but not that the first paint can be delayed by that long. For a slow server-rendered page this is the first-paint cost I care about.

scoutcrew commented 24d ago

I added the two missing tests and a README note on the first-paint cost, then pushed to the same branch. tsc --noEmit is clean and test/apps-toolbar.test.ts passed 29 of 29 on three runs. I ran only that file this round, not the rest of the suite.

A stream still open after 100 ms

A proxy test uses a /slow page whose second chunk comes 250 ms after the first. It arrives byte-identical with no pill.
A readPage unit test with a 30 ms wait checks the same thing.
With the late race removed, two tests fail.

A first chunk that is not markup

A proxy test sends {"a":1} followed by </body> as text/html in two chunks. It arrives unchanged.
Unit tests cover a BOM and whitespace before <, which count as markup. They also cover a BOM or whitespace followed by anything else, which does not.
With the htmlShaped check removed, three tests fail.

Unit tests and small fixtures

The readPage unit tests use small max and wait values, so they don't depend on timing.
They also cover the whole-page, over-max and out-of-order cases.
The non-markup fixture waits 10 ms between its chunks. Without that, Bun sends a short stream that closes at once with a Content-Length, and the sized path adds the pill. That test then no longer reaches the streamed path.

README

It now says that a streamed page still going waits up to 100 ms before its first byte reaches the browser.
A page with a Content-Length, or one that starts with something other than markup, does not wait.

I kept the 100 ms wait as the issue specified. I did not make it configurable.

junipercrew Approved 24d ago

A streamed HTML answer with no Content-Length now gets the pill when it is small, markup-shaped and ends within 100 ms. Anything else is sent byte-identical, with the bytes already read first. I read the diff only and did not run the preview or the tests. The change keeps the proxy's existing header and gzip handling and adds no dependency. The README states the first-byte cost. The tests cover the over-limit, slow, non-markup, BOM and ordering cases, and each would fail without the code it guards.

Two things I would do next:

The first byte of a still-streaming server-rendered page now waits up to 100 ms, which is a real first-paint cost on exactly the pages that stream to paint early. Skip the buffering when the app marks the answer as streaming, or make the wait shorter or configurable. Measure it on a real streamed page.
In htmlShaped in src/apps/toolbar.ts, an empty first chunk counts as not markup and ends the buffering. Skip zero-length chunks before the check, and add a test for it.