Skip to content

Apply readBufferSize to the copy loop and split it per direction - #36

Merged
kriszyp merged 5 commits into
mainfrom
kris/wire-read-buffer-size
Jul 30, 2026
Merged

Apply readBufferSize to the copy loop and split it per direction#36
kriszyp merged 5 commits into
mainfrom
kris/wire-read-buffer-size

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What

readBufferSize has been accepted by the config and plumbed all the way into ProxyConnConfig for several releases without anything reading it. The copy was copy_bidirectional, which uses tokio's hard-coded 8 KiB per direction. http_proxy.rs doesn't read it either — that path uses fixed stack buffers — so the setting has never had any effect on any port, and the 64 KiB default documented in the README has never applied.

This PR makes it live, per direction.

Why it matters

Not throughput — connection count. Both copy buffers are allocated for the whole life of every connection whether or not it is transferring anything, so the size multiplies straight into per-connection memory:

buffer bytes = 2 × readBufferSize × connections

At the 8192 default that is 16 KiB/conn — 4.0 GiB at 262k connections, 5.1 GiB at 333k. For a fleet of mostly-idle MQTT subscribers those are gigabytes of buffers nothing ever reads. Dropping MQTT to 1024/4096 is 5 KiB/conn, freeing ~3.75 GB at 333k connections at no CPU cost (a payload larger than the buffer is just more loop iterations, and on a TLS-terminating listener those aren't even syscalls — the reads come out of rustls's already-decrypted buffer).

Context: Customer load test, where a 3-node cluster is being sized for ~1M concurrent MQTT connections.

Changes

  • Wire the configured size through to copy_bidirectional_with_sizes.
  • Lower the default 65536 → 8192. This is a compatibility requirement, not a preference: 8192 is what the copy loop has actually been using all along, so plumbing the old default through would have taken every deployment that leaves the field unset from 16 KiB to 128 KiB per connection — an 8× regression arriving as a "no-op wiring fix".
  • Add clientReadBufferSize / upstreamReadBufferSize. MQTT is strongly asymmetric — after SUBSCRIBE a client sends almost nothing but PINGREQ while the broker carries the fan-out — so 1024/4096 beats a symmetric 2048 on both memory and downstream headroom.
  • Clamp to [512, 1 MiB] and log anything out of range, naming the key the operator actually set. Zero is the correctness case: the copy loop would read into an empty slice and take the resulting Ok(0) for EOF, closing the connection instead of proxying it.
  • symphony-server: widen the recreate-vs-hot-swap signature (listenerSig → constructionSig) to cover the proxy-level fields that are frozen at construction. Without this, editing only a buffer size in config.json left the signature unchanged, so the reload took the route-only hot-swap branch, reported success, and kept running the old value. workerThreads had the same pre-existing hole and is included.
  • README: new "Copy buffers and per-connection memory" section with per-traffic-shape recommendations (small for MQTT, default for HTTPS/ops, leave or raise for replication), the two scope limits, and an upgrade note.

Scope limits worth knowing

  • Header-rewriting routes are not covered. When sourceAddressHeader: 'xForwardedFor' or a header-carried forwardFingerprint is set, the flow takes proxy_http1_rewriting, which frames with its own fixed 8 KiB buffers. The new default equals that, so only a raised or lowered value is a no-op there. The MQTT target uses PROXY protocol, so it takes the plain path. Wiring those buffers too is a reasonable follow-up; the README no longer implies the setting governs every proxied byte.
  • "Behavior-preserving" applies to configs that leave the field unset. One that explicitly set 65536 (plausibly copied from the old documented default) was getting 8 KiB while the field was inert and now gets what it asked for — 128 KiB/conn. There's an upgrade note in the README. host-manager never sets the field, so the Harper fleet is on the default path.

Testing

  • Rust unit tests: default equals the copy loop's historical value, and clamping (0 → floor, u32::MAX → ceiling, in-range passthrough).
  • New __test__/copy-buffers.spec.ts: round-trips a 512 KiB patterned payload (1024× the minimum buffer) through TLS termination with a 512-byte buffer, with asymmetric buffers, and with a clamped 0 — asserting byte-for-byte integrity. A too-small buffer must only mean more iterations, never truncation or a stall. Registered in the npm test file list.
  • Instrumented Rust tests (CapacityRecorder) record the ReadBuf capacity the copy loop uses in each direction, so a config that is accepted-then-ignored fails, and a transposed pair of directions fails — neither of which a byte-for-byte round-trip can detect. Added in response to review; forward delegates to copy_both_ways to make the copy step reachable without a napi env.

Cross-model review

Ran thorough mode: Codex + Gemini + an experimental Grok leg + a Harper-domain pass, adjudicated. No blockers. Four significant concerns, all addressed in the second commit:

  1. Standalone-server reload silently ignored the new fields (all three legs) — fixed via constructionSig above.
  2. Doc-comment arithmetic wrong by ~15× (Codex + Grok) — "~7 GiB" for the 8→64 KiB delta at 1M connections is actually ~107 GiB (2 × 56 KiB × 1e6). Corrected. The README's own numbers verified correct; two roundings tightened.
  3. Buffer settings don't reach the header-rewriting path (all three legs) — README claim scoped, see above.
  4. The is_http1_alpn gate was inert (domain pass + Codex) — reverted. cfg.alpn_protocols is set at one site, to [h2, http/1.1], and only when a route sets http2 (src/tls.rs:107), so rustls can only ever negotiate None, h2, or http/1.1 — never mqtt. The gate was behavior-identical to the old != h2 in every reachable state, and real terminated MQTT negotiates no ALPN at all, so it is still classified HTTP/1. The first commit message claimed a stall was fixed that was not. The real root cause needs a route-level protocol declaration and a design call, so it's filed as Header rewriting can be applied to non-HTTP streams: ALPN cannot distinguish MQTT from an HTTPS client that offers no ALPN #38 rather than papered over here.

Plus one from the Grok leg that the other legs and I both missed: the upgrade-migration case for configs that set readBufferSize explicitly (scope limit #2 above).

Review round 2 (dispatched codex/gpt-5.6-sol, verdict CHANGES)

Five comments, all addressed; threads replied to and resolved.

# Point Resolution
1 Standalone reload ignores the new fields Already fixed in 0035db8 (constructionSig)
2 Sizes don't reach the rewrite path Took the "narrow the contract + docs" option; threading filed as #39
3 Specs can't detect ignored or swapped sizes Fixed in 4ed6304 — instrumented per-direction capacity assertions
4 The ALPN change is unreachable; remove it Already reverted in 0035db8; root cause filed as #38
5 Memory equation only valid for symmetric sizing Fixed in 4ed6304 — general form, in all four places it appeared

Reviewer notes

🤖 Generated with Claude Code (Opus 5)

@kriszyp @claude
Apply readBufferSize to the copy loop; split it per direction
6b84f55 
`readBufferSize` has been plumbed from the JS config into `ProxyConnConfig` for
several releases without anything reading it: the copy is `copy_bidirectional`,
which uses tokio's hard-coded 8 KiB per direction. `http_proxy.rs` doesn't read
it either — that path uses fixed stack buffers — so the setting has never had
any effect on any port, and the documented 64 KiB default has never applied.

This matters at high connection counts, not high throughput. Both buffers are
allocated for the whole life of every connection whether or not it is
transferring anything, so the size multiplies straight into per-connection
memory: 2 x size x connections. At a few hundred thousand mostly-idle MQTT
subscribers that is gigabytes of buffers nothing ever reads.

- Wire the value through to `copy_bidirectional_with_sizes`.
- Lower the default 65536 -> 8192. This is a compatibility requirement rather
  than a preference: 8192 is what the copy loop has actually been using, so
  wiring the old default through would have taken every existing deployment
  from 16 KiB to 128 KiB per connection.
- Add `clientReadBufferSize` / `upstreamReadBufferSize`. MQTT is strongly
  asymmetric — after SUBSCRIBE a client sends almost nothing but PINGREQ while
  the broker carries the fan-out — so 1024/4096 beats a symmetric 2048 at both
  memory and downstream headroom.
- Clamp to [512, 1 MiB] and log out-of-range values. Zero is the correctness
  case: the copy loop would read into an empty slice and take the `Ok(0)` for
  EOF, closing the connection instead of proxying it.

Also narrows the header-rewrite gate. `l7_http1` was "ALPN is not h2", so a
terminated MQTT connection on a route configured with `sourceAddressHeader:
'xForwardedFor'` sent the stream through the HTTP/1 rewriter, which would wait
for a `\r\n\r\n` that never arrives and stall until the idle timeout. A
negotiated ALPN is now taken at its word; an absent one stays permissive,
because HTTPS clients that offer no ALPN are real and still need their
X-Forwarded-For. A native MQTT client that negotiates no ALPN is still
indistinguishable from one of those, so the complete fix is a route-level
protocol declaration — noted in the function docs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Jul 29, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces per-direction copy buffer configurations (clientReadBufferSize and upstreamReadBufferSize) to override the base readBufferSize, which has been reduced to a default of 8192 bytes. It integrates tokio::io::copy_bidirectional_with_sizes to apply these sizes, adds clamping bounds to prevent invalid buffer sizes, and refines ALPN protocol checks to avoid header rewriting on non-HTTP/1 streams like MQTT. Comprehensive unit and integration tests, along with detailed documentation, have been added to support these changes. I have no feedback to provide as there are no review comments to evaluate.

@kriszyp
kriszyp marked this pull request as ready for review July 29, 2026 23:14
This was referenced Jul 29, 2026
Open
Closed
kriszyp and others added 3 commits July 29, 2026 17:31
@kriszyp @claude
Address cross-model review: reload gap, wrong arithmetic, inert ALPN …
0035db8 
…gate

Cross-model review (Codex + Gemini + Grok-experiment + a Harper domain pass)
found three real problems and one overclaim in the first commit.

Standalone-server reload silently ignored the new fields. `symphony-server`
decided recreate-vs-hot-swap from a signature over listeners only, but the
buffer sizes are frozen at construction on the Rust side and `updateConfig`
reaches routes and protection only. Editing just a buffer size left the
signature unchanged, so the reload took the hot-swap branch, reported success,
and kept running the old value — the exact "config masquerading as applied"
that the clamp warning exists to prevent. The signature now covers the
proxy-level construction-frozen fields, `workerThreads` included; it had the
same pre-existing hole.

The doc comment's memory figure was wrong by ~15x. Going from 8 KiB to 64 KiB
per direction across a million connections is 2 x 56 KiB x 1e6, which is
~107 GiB, not the ~7 GiB claimed. An operator sizing a node from that comment
would have under-planned RSS by an order of magnitude. The README's own numbers
checked out; two roundings there are tightened (5.2 -> 5.1 GiB, 3.6 -> 3.75 GB).

Reverted the `is_http1_alpn` gate as inert. `cfg.alpn_protocols` is set at one
site, to `[h2, http/1.1]`, and only when a route sets `http2` (tls.rs:107), so
rustls can only ever negotiate None, h2, or http/1.1 — never `mqtt`. The new
gate was therefore behavior-identical to the old `!= h2` in every reachable
state, and real terminated MQTT (which negotiates no ALPN at all) is still
classified as HTTP/1. It bought an unreachable branch plus a test asserting an
impossible state, and the commit message claimed a stall was fixed that was
not. The actual root cause needs a route-level protocol declaration, filed as
 #38 with a design question rather than papered over here.

Scoped two overclaims. The buffer settings do not reach header-rewriting
routes, whose framing uses its own fixed 8 KiB buffers, so the README no longer
implies they govern every proxied byte. And "behavior-preserving" holds only
for configs that leave `readBufferSize` unset: one that explicitly set 65536
was getting 8 KiB while the field was inert and now gets what it asked for, so
the README carries an upgrade note. host-manager never sets the field, so the
Harper fleet is on the default path.

Also label the clamp warning with the key the operator actually set — the
fallback previously reported a per-direction field name for an out-of-range
`readBufferSize`, so a grep for the key they wrote found nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kriszyp @claude
Keep the copy-buffer sizing numbers in one unit
fcf677d 
The section's totals are GiB but the MQTT saving was written in decimal GB, so
a reader comparing 3.75 against 5.1 in the line above is comparing different
units. Same quantity, stated as ~3.5 GiB.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kriszyp @claude
Prove each configured size reaches its own direction; fix the memory …
4ed6304 
…equation

Review noted the specs prove only that large payloads round-trip: they pass
just as well if the new fields are accepted and never read, which is the exact
regression this change exists to prevent, and they cannot tell whether the two
directions were transposed.

Closed that with an instrumented Rust test. `CapacityRecorder` records the
capacity of every `ReadBuf` the copy loop hands it, and that capacity IS the
configured size for its direction, so the assertions distinguish "the size
reached this half" from "the size was accepted and dropped". A second case
feeds the same two values exchanged, so a transposed implementation fails even
though it would satisfy the first case's value set.

Getting there needed the copy step reachable from a unit test: `ConnContext`
holds an `Arc<ThreadsafeFunction>`, which cannot be constructed without a napi
env, so `forward` now delegates to `copy_both_ways(client, upstream,
client_size, upstream_size)`. That leaves exactly one untested step — the
field-to-argument mapping on the call line, directly above the tested function.

Also corrects the memory equation, which was only valid for symmetric sizing.
`2 × readBufferSize × connections` does not describe the recommended MQTT
setting at all: 1024 + 4096 is 5 KiB, not twice either value. The general form
is `(client + upstream) × connections` with each unset override falling back to
`readBufferSize`, stated now in the README, the napi struct docs, ts/types.ts,
and the addon typings, with the symmetric shorthand kept as the special case.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kriszyp
kriszyp commented Jul 30, 2026
View reviewed changes

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Submitting the dispatched codex/gpt-5.6-sol review (verdict CHANGES) so its findings are on the record rather than sitting in a draft. Replies to each point follow; two were already addressed by the review-fix commit, three are addressed in 4ed6304.

Comment thread ts/server.ts
Comment thread src/proxy_conn.rs
Comment thread __test__/copy-buffers.spec.ts
Comment thread src/proxy_conn.rs Outdated
Comment thread README.md Outdated
This was referenced Jul 30, 2026
Open
Closed
kriszyp added a commit that referenced this pull request Jul 30, 2026
@kriszyp @claude
Scale copy-buffer memory with concurrent transfers, not connection co…
8195a10 
…unt (#37)

Replaces copy_bidirectional_with_sizes with a hand-rolled copy_bidirectional_lazy
(src/copy.rs). tokio::io::CopyBuffer allocates its per-direction buffer once and
holds it for the connection's whole life, whether or not it is transferring — at
a million mostly-idle MQTT subscribers, readBufferSize x 2 held forever per
connection is dead weight.

pump() instead starts each direction at a small fixed floor (512 B) and escalates
to the full configured max only once a read proves a sustained burst (a read that
exactly fills the current buffer), dropping back down the first time a read comes
back under capacity. Every read is a single ordinary blocking .await; an earlier
version tried a non-blocking opportunistic drain via a manual poll_read with a
noop Waker to batch up "whatever's already queued" without an extra iteration,
and under load it silently stranded a connection's wakeup (reproduced
empirically). The final design has no manual polling at all, so every step is
provably deadlock-free at the cost of one extra small-buffer round trip per
burst. tokio::try_join! (not join!) on the two directions preserves
copy_bidirectional's error semantics: an error on either side ends the whole
copy immediately.

readBufferSize/client|upstreamReadBufferSize become a per-transfer maximum
rather than a permanent allocation; default sizes and the config surface are
unchanged.

Measured (see __test__/bench-copy-memory.ts, __test__/bench-copy-throughput.ts):
- 15,000 connections, 64 KiB configured buffer, each sending one full-buffer
  burst then going idle: 305,985 B/conn (base) -> 233,240 B/conn (this branch),
  ~24% less resident memory per connection.
- Bulk throughput (4 connections, sustained high volume): no regression,
  ~1700-1900 MiB/s on both, within run-to-run noise.

Based on kris/wire-read-buffer-size (PR #36, not yet merged), which introduces
readBufferSize and the per-direction client/upstreamReadBufferSize fields this
work builds on.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Devin-Holland
Devin-Holland approved these changes Jul 30, 2026
View reviewed changes

@Devin-Holland Devin-Holland left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified locally

The reviewer notes say the toolchain wasn't available, so I re-ran the parts that were CI-only. There is a Rust toolchain on this box (cargo 1.94.1):

  • cargo test --lib — 105 passed, 0 failed, including the four new tests.
  • cargo clippy --all-targets — no errors. The 9 warnings are all pre-existing dead-code ones on lines this diff doesn't touch (rt field, JsEvent::Error variant, http_proxy helpers).
  • CI is green on 4ed6304 across all 11 checks.

I also checked the two load-bearing claims against the tokio source rather than taking them on trust:

  • DEFAULT_COPY_BUFFER_SIZE = 8 * 1024 is exactly right. tokio-1.53.1/src/io/util/mod.rs:88 — const DEFAULT_BUF_SIZE: usize = 8 * 1024, and copy_bidirectional.rs:83-84 builds both CopyBuffers from it. So "unset configs keep the footprint they already had" holds.
  • The per-direction mapping is not transposed. copy_bidirectional_with_sizes(a, b, a_to_b, b_to_a) passes a_to_b to CopyBuffer::new for the a→b half, so with a = client, clientReadBufferSize really does size reads from the client. The CapacityRecorder tests are a good way to pin that down — nice touch making the two values asymmetric so a transposition can't pass.
  • The eager-allocation premise is real: CopyBuffer::new does vec![0; buf_size].into_boxed_slice(), so both buffers are allocated up front and held for the connection's life. The memory arithmetic checks out — 2 × 57344 × 1e6 = 106.8 GiB for the 64 KiB-vs-8 KiB comparison in the doc comment, and 2 × 8192 × 333_000 = 5.08 GiB.
  • host-manager never sets readBufferSize — grepped src/, no hits — so the default-lowering is a no-op for the Harper fleet, as the body claims.

Approving. Two things inline, neither blocking.

Claude (Opus 5)

Comment thread README.md Outdated
Comment thread src/proxy.rs
@kriszyp @Devin-Holland
Apply suggestions from code review
ce1c8bc 
Co-authored-by: Devin Holland <50112339+Devin-Holland@users.noreply.github.com>
@kriszyp
kriszyp merged commit 72609d0 into main Jul 30, 2026
7 checks passed
kriszyp added a commit that referenced this pull request Jul 30, 2026
@kriszyp @claude
Scale copy-buffer memory with concurrent transfers, not connection co…
ffd42d1 
…unt (#37)

Replaces copy_bidirectional_with_sizes with a hand-rolled copy_bidirectional_lazy
(src/copy.rs). tokio::io::CopyBuffer allocates its per-direction buffer once and
holds it for the connection's whole life, whether or not it is transferring — at
a million mostly-idle MQTT subscribers, readBufferSize x 2 held forever per
connection is dead weight.

pump() instead starts each direction at a small fixed floor (512 B) and escalates
to the full configured max only once a read proves a sustained burst (a read that
exactly fills the current buffer), dropping back down the first time a read comes
back under capacity. Every read is a single ordinary blocking .await; an earlier
version tried a non-blocking opportunistic drain via a manual poll_read with a
noop Waker to batch up "whatever's already queued" without an extra iteration,
and under load it silently stranded a connection's wakeup (reproduced
empirically). The final design has no manual polling at all, so every step is
provably deadlock-free at the cost of one extra small-buffer round trip per
burst. tokio::try_join! (not join!) on the two directions preserves
copy_bidirectional's error semantics: an error on either side ends the whole
copy immediately.

readBufferSize/client|upstreamReadBufferSize become a per-transfer maximum
rather than a permanent allocation; default sizes and the config surface are
unchanged.

Measured (see __test__/bench-copy-memory.ts, __test__/bench-copy-throughput.ts):
- 15,000 connections, 64 KiB configured buffer, each sending one full-buffer
  burst then going idle: 305,985 B/conn (base) -> 233,240 B/conn (this branch),
  ~24% less resident memory per connection.
- Bulk throughput (4 connections, sustained high volume): no regression,
  ~1700-1900 MiB/s on both, within run-to-run noise.

Based on kris/wire-read-buffer-size (PR #36, not yet merged), which introduces
readBufferSize and the per-direction client/upstreamReadBufferSize fields this
work builds on.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Jul 30, 2026
@kriszyp @claude
Gate escalating copy buffers on active connection count
f1a0d16 
The escalate/park/shrink behaviour saves memory in proportion to connection
count, but costs two allocations plus a zeroing `vec![0u8; n]` per burst
regardless of it. A replication port-set carrying six bulk streams paid that on
every burst to reclaim a few hundred KiB it was never short of.

`copy::LazyBufferGate` decides at each resize point from the proxy-wide
`GlobalMetrics::active_connections` gauge, configured per proxy by
`lazyCopyBufferThreshold` (default 1000). Below it, each direction allocates its
full configured buffer once and never resizes — byte-for-byte the
`copy_bidirectional_with_sizes` behaviour this module replaced, so a small
port-set pays none of the churn rather than a reduced amount. `0` engages
always; a value above peak concurrency disables it.

The gauge is re-read at every resize point rather than latched per connection:
a connection established while the proxy was quiet would otherwise hold a
full-size buffer for its whole life however busy the proxy later became, and
long-lived connections accumulating while idle is the shape this exists for.

Also adds the reload regression test that was asked for on #36 and never landed,
now covering all four construction-frozen proxy fields, with a route-only
control so it can't pass against a server that recreates on every write.
Writing it turned up a wrong claim in the README: a recreate does NOT drop
established connections. `stop()` ends the accept loops and sleeps 100ms but
never aborts connection tasks, and the runtime lives in the napi wrap until GC,
so established sessions keep running on the old buffer sizes until they close.
That is now documented and pinned by a test.

Benchmarks take the threshold as an argument and pin it to 0 by default, so a
run below the default threshold can't silently measure the static path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Devin-Holland Devin-Holland Devin-Holland approved these changes
@harper-joseph harper-joseph Awaiting requested review from harper-joseph
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kriszyp @Devin-Holland