Skip to content

fix(http): stream Brotli room exports incrementally - #865

Merged
sv merged 4 commits into
flop-labs:mainfrom
ShalyX:fix/export-brotli-streaming-v2
Sep 23, 2026
Merged

sv merged 4 commits into
flop-labs:mainfrom
ShalyX:fix/export-brotli-streaming-v2

Conversation

@ShalyX

@ShalyX ShalyX commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Register application/x-ndjson with streaming=True in starlette-compress.
  • Add an ASGI regression that proves Brotli sends bytes before the export generator reaches EOF.

Fixes #861.

Why this matters

/r/<room>/export is a StreamingResponse emitted in 64 KiB chunks. With the existing registration, Brotli's non-streaming path buffered the export until 3,735,552 of 4,180,345 bytes had been consumed in the measured corpus. That delays the first byte, increases time-to-first-byte, and defeats back-pressure for the largest response lane. streaming=True preserves incremental delivery while keeping the byte-exact export payload unchanged.

Evidence

  • Targeted compression suite: 6 passed in 3.76s
  • Full suite: 776 passed, 1 skipped in 208.64s
  • Coverage: 97.97%
  • Benchmark (uv run python bench/compression.py): streamed br-4 1,189,695 bytes at 66.5 MB/s, versus gzip-4 1,463,246 bytes at 53.8 MB/s.
  • Regression fails on the pre-fix registration (first body observed only after all 4 chunks were consumed) and passes with the fix.
  • ruff check, ruff format --check, and ty check pass; git diff --check passes.

Known repository check

uv run sz.py --check currently reports pre-existing core/app.py 1020 -> 1023 growth on upstream main@542790b (the merged compression PR) against sz-baseline.json; this change does not add further measured code lines beyond that upstream state. I did not modify the maintainer-regenerated baseline.

@WIZARDspace

Copy link
Copy Markdown
Contributor

Checked at efc08de (on main@542790b):

  • tests/http/test_compression.py: 6 passed. With src/app.py reverted to add_compress_type("application/x-ndjson"), test_brotli_export_starts_before_the_whole_ring_is_read fails with assert 4 < 4, so it tells the two registrations apart.
  • With the fix, the first http.response.body arrives after 1 of the 4 chunks has been read (13 B of compressed output).

One suggestion: first_body[2] < len(chunks) still passes if the first body arrives after 3 of 4 chunks, i.e. with 196,608 B held back. The bug in #861 was a partial buffer too (57 of 64 blocks), so a smaller partial regression would get through. assert first_body[2] == 1 matches what streaming=True actually does here, and would catch that.

This is the fix I offered on #861, so I won't open a competing PR.

@Minh3132 Minh3132 mentioned this pull request Sep 17, 2026
6 tasks done
@ShalyX
ShalyX force-pushed the fix/export-brotli-streaming-v2 branch from efc08de to a0616b8 Compare September 17, 2026 18:53
@ShalyX

ShalyX commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Updated the regression on current main: the ASGI harness now requires the first compressed body message after exactly one source chunk, so buffering three chunks still fails. Local full checks pass (783 passed, 1 skipped); pushed as f3d7547.

@WIZARDspace WIZARDspace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-verified at f3d7547 (rebased onto main@e4c4f73 today). My 09-16 check was at efc08de, so these numbers are fresh.

The regression test does its job. Reverting the one-line change, test_brotli_export_starts_before_the_whole_ring_is_read fails with assert 4 == 1 — "Brotli must emit after the first source chunk". Restored, the file is 6 passed. uv run --group dev just check exits 0 on this head: 783 passed, 1 skipped.

Measured on a real export — 6,000,000 B of /r/lobby/export (18,498 records) fed through the actual ASGI app in 65,536-byte EXPORT_CHUNK blocks:

Accept-Encoding: br first body message source consumed first body msgs total
without streaming=True 1,002,606 B 38 of 92 chunks (2,490,368 B) 3 2,401,671 B
with streaming=True 29,414 B 1 of 92 chunks (65,536 B) 93 2,410,729 B

Cost: +9,058 B, +0.377%. That is middleware against middleware. A comparison against one-shot brotli.compress() suggests ~9%, but the middleware never does that, so that figure does not apply here.

The buffering threshold is output-side. Without the fix the encoder emits in ~1 MB compressed blocks, so time-to-first-byte is however much input it takes to fill one: 2,490,368 B on this sample, 3,735,552 B on the production export in #861. Same behavior, not two different numbers.

gzip was never affected, so the brotli-only test is correctly scoped. Same harness with Accept-Encoding: gzip: 1 of 92 chunks to first byte both with and without the change, 93 messages either way. streaming=True costs gzip +1,414 B (+0.054%) and changes nothing else.

No blockers.

@sv
sv merged commit 6571499 into flop-labs:main Sep 23, 2026
9 checks passed
@sv sv mentioned this pull request Sep 23, 2026
5 tasks done
sv added a commit that referenced this pull request Sep 23, 2026
## What

Cuts **0.14.2**. Like #893, it dates the CHANGELOG section, bumps the
version in the files the service, the MCP wrapper and the worker share,
and re-ratchets the measured block of `sz-baseline.json`. No cap moves.

## Why

**Main CI is red.** Since #163, every push to main fails `sz.py --check`
(`core/limit.py 183 -> 186`, under its cap of 193). Because that step
fails, the steps after it are skipped on main: the example e2e, the MCP
wrapper build, and the image build and smoke. This PR's CI runs those
steps against #163 and #126 for the first time.

There are three fixes since v0.14.1, all PATCH, and nothing on the HTTP
surface moves:

- #865: a brotli `/r/<room>/export` streams again instead of buffering
to near-EOF.
- #163: the per-IP token bucket's read-modify-write runs under a lock,
so concurrent threadpool requests can't overspend it.
- #126: room and note readers treat a file reaped between the check and
`open()` as absent, where they used to return a 500.

After merge: tag `v0.14.2` on the squash commit, which publishes
`ghcr.io/flop-labs/technocore-chat:0.14.2`.

## Checks

- [x] `uv run coverage run -m pytest tests -q && uv run coverage
report`: 812 passed, 98.08%
- [x] `uv run ruff check . && uv run ruff format --check .` and `uv run
ty check`: clean
- [x] `uv lock --check` (root, and `mcp/worker` after `uv build
--project mcp` built the 0.14.2 wheel), `sz.py --caps`, `sz.py --check`,
and both `release.yml` gates
- [x] Docs that would now be wrong are updated: `CHANGELOG.md` is the
change
- [x] New surface on a world-writable service: nothing new
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Brotli /r/<room>/export is not streamed: no body bytes until 3,735,552 of 4,180,345 B are consumed

3 participants