Skip to content

fix(proxy): bound connection memory and admission#25858

Open
XuPeng-SH wants to merge 11 commits into
mainfrom
fix/proxy-bounded-connection-memory
Open

fix(proxy): bound connection memory and admission#25858
XuPeng-SH wants to merge 11 commits into
mainfrom
fix/proxy-bounded-connection-memory

Conversation

@XuPeng-SH

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes #25856.

Depends on #25857. This is a stacked PR and should be retargeted to main after #25857 merges.

What this PR does / why we need it:

The Proxy previously paid two eager 1 MiB frontend.IOSession buffers per live proxied connection, created one managed allocator per connection, retained an unbounded login packet for migration, and had no global or per-tenant admission boundary. Raising a memory limit alone would only delay exhaustion.

This PR closes the resource lifecycle at multiple layers:

  • reduces the two retained Proxy protocol buffers from about 2 MiB to 32 KiB per active connection (16 KiB on each side);
  • shares one size-bounded protocol allocator across client, backend, migration, and cached backend sessions;
  • adds global and per-tenant live-connection admission with idempotent leases;
  • rejects global overload before per-connection protocol allocation with MySQL error 1040;
  • caps retained client login packets at 64 KiB and rejects them from the packet header before decoding;
  • bounds the complete unauthenticated handshake lifecycle, including TLS, and clears the read deadline before entering the raw tunnel;
  • rejects duplicate PROXY protocol headers so they cannot extend the handshake or grow recursion;
  • validates connection, protocol-memory, handshake-size, and handshake-timeout configuration as one coherent budget;
  • records expected overload as reject metrics without error-log amplification.

The SQL/tunnel data path is unchanged: the limiter lock is touched only during acquire, tenant bind, and release, and no query/transaction idle deadline is introduced. This avoids regressions for long HTAP queries.

Test matrix

  • full ./pkg/frontend and ./pkg/proxy unit suites;
  • go build ./pkg/frontend ./pkg/proxy;
  • go vet ./pkg/frontend ./pkg/proxy;
  • new boundary/concurrency tests repeated 100 times;
  • full ./pkg/proxy race suite;
  • focused frontend race tests.

Covered unhappy paths include allocator exhaustion and recovery, exact-fill and oversized packets, global and tenant saturation, concurrent bind/release, double release, constructor/TLS failure cleanup, silent-client timeout, deadline clearing after successful login, standard client-visible errors, and bounded rejection writes.

@XuPeng-SH
XuPeng-SH requested a review from gouhongshen as a code owner July 17, 2026 22:07
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@mergify mergify Bot added kind/bug Something isn't working kind/enhancement kind/test-ci labels Jul 17, 2026
@mergify

mergify Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@XuPeng-SH
XuPeng-SH force-pushed the fix/frontend-configurable-protocol-memory branch from 3e05081 to da2e56c Compare July 18, 2026 00:37
@XuPeng-SH
XuPeng-SH force-pushed the fix/proxy-bounded-connection-memory branch from ff4127a to f259642 Compare July 18, 2026 00:40
@LeftHandCold
LeftHandCold self-requested a review July 18, 2026 03:30

@LeftHandCold LeftHandCold 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.

Requesting changes for one availability bypass and three concrete overload/protocol regressions. I reviewed only the stacked delta from da2e56c to f259642. The full proxy test suite, focused race tests, build, and vet pass; additional focused diagnostics reproduced the absolute-deadline bypass and the incorrect ERR packet sequence.

Comment thread pkg/proxy/client_conn.go
Comment thread pkg/proxy/client_conn.go Outdated
Comment thread pkg/proxy/handler.go Outdated
Comment thread pkg/proxy/handshake.go Outdated
@XuPeng-SH
XuPeng-SH force-pushed the fix/frontend-configurable-protocol-memory branch from da2e56c to 6788d7c Compare July 18, 2026 04:26
@XuPeng-SH
XuPeng-SH force-pushed the fix/proxy-bounded-connection-memory branch from f259642 to 2728840 Compare July 18, 2026 04:28
@XuPeng-SH
XuPeng-SH force-pushed the fix/proxy-bounded-connection-memory branch from 2728840 to 2c57d30 Compare July 18, 2026 04:57
@XuPeng-SH
XuPeng-SH requested a review from LeftHandCold July 18, 2026 04:58

@LeftHandCold LeftHandCold 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-reviewed the latest head. The four findings from my previous review are fixed. Requesting changes for one newly confirmed PROXY v2 protocol regression. The full proxy/frontend test suites, the full proxy race suite, build, vet, and diff checks pass; a focused diagnostic reproduces this failure deterministically.

Comment thread pkg/proxy/ppv2.go
Base automatically changed from fix/frontend-configurable-protocol-memory to main July 18, 2026 05:28

@gouhongshen gouhongshen 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.

Codex automated review

The new admission and memory limits have a concrete OOM gap, and duplicate PROXY v2 detection is incomplete.

P1 - Include retained handshake storage in the memory budget (pkg/proxy/config.go:348)

Validation accounts only for the shared 16 KiB client/backend session buffers. The goetty input buffer and the copied handshakePack.Payload are outside that allocator but remain retained for authenticated connections. With the defaults, 10,000 connections may each retain a valid 64 KiB login twice, already exceeding 1 GiB before the fixed session buffers and tunnel state, so ProtocolMemoryLimit does not prevent OOM.

P2 - Mark consumed LOCAL/UNSPEC PROXY frames for duplicate detection (pkg/proxy/ppv2.go:194)

For valid unspec/unix PROXY v2 frames, the parser consumes the frame but returns ok=false. The wrapper then delegates to the SQL codec, and readPacketBefore never sets proxyHeaderReceived. A LOCAL/UNSPEC frame followed by an address-bearing frame in a later read therefore bypasses duplicate rejection and can overwrite origin metadata used for internal classification and routing.

@LeftHandCold LeftHandCold 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-reviewed the latest head and approving. The previously reported fragmented PROXY v2 regression is fixed: matching signature prefixes, partial fixed headers, and partial declared bodies now remain buffered without consumption, while definite mismatches still delegate to the bounded MySQL codec. I found no new blocker in the updated delta. The focused fragmentation test passed 100 iterations; the full proxy suite, full proxy race suite, build, vet, and diff checks also pass.

@XuPeng-SH
XuPeng-SH requested a review from gouhongshen July 18, 2026 11:04
@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

Deep-review blocker (GitHub does not allow the PR author to submit a formal REQUEST_CHANGES review on their own PR):

[P1] Reject handshake packet limits that cannot admit a final login

minimumClientHandshakePacketLimit is currently 32 because a protocol 4.1 SSLRequest is 32 bytes, and Config.Validate therefore accepts limits of 32 or 33. However, 32 bytes is only the fixed protocol-4.1 prefix. A final authentication response must additionally contain at least the NUL-terminated username and an auth length/terminator. On TLS connections the 32-byte SSLRequest is followed by that larger final login, and the same SQL codec limit applies to both phases.

I reproduced this at the wire level on the current head: a config with client-handshake-packet-limit = 32 passes FillDefault + Validate, then a valid login is rejected with frontend.ErrPacketTooLarge. Thus an explicitly accepted configuration deterministically makes the proxy unable to authenticate any real client.

Please model the minimum for a complete login rather than the SSLRequest-only size (or otherwise reject unusable limits), and add a boundary test that the smallest configuration accepted by Validate can pass the packet-size gate for a final protocol-4.1 login.

@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

@gouhongshen Addressed the latest handshake-limit blocker and the related phase-model gaps in a569dfa; the branch is also merged with the latest main at 80668d9.

Root-cause classification:

  • The 32-byte minimum was a configuration-contract bug: 32 bytes describes only the intermediate protocol-4.1 SSLRequest, not a usable terminal login. The accepted minimum is now derived as 32-byte fixed prefix + one-byte username + NUL + one auth length/terminator byte (35 bytes), with exact min/max wire and validation boundaries.
  • The recursive TLS path exposed a broader protocol-state-machine gap. Handshake processing is now an explicit bounded phase loop: pre-TLS -> at most one TLS upgrade -> final login. A repeated SSLRequest is rejected instead of recursively layering TLS/stack/CPU work.
  • Validation now mirrors runtime composition: plugin routing disables the connection cache, so that mode no longer reserves 1,024 cache sessions that can never exist.

Unhappy-path ownership coverage now proves:

  • the intermediate SSLRequest allocation is released before both successful and failed TLS negotiation;
  • a partially parsed final login transfers cleanup ownership and is released exactly once by Close;
  • duplicate TLS requests are rejected promptly without recursion;
  • plaintext and TLS logins at the exact accepted minimum complete parsing;
  • allocator, admission, timeout, pipelined-prefix, double-close, and concurrent reader/Close paths remain covered.

Performance: the production delta is limited to startup validation and the unauthenticated handshake. The recursion-to-loop change adds only a stack boolean and performs the same packet parse/allocation work for the normal one- or two-packet flow. It does not touch the established tunnel/query hot path.

Fresh validation after merging latest main:

  • focused phase/config tests: 20x
  • full pkg/proxy
  • full pkg/proxy with -race
  • go build -mod=readonly ./pkg/proxy
  • go vet -mod=readonly ./pkg/proxy
  • make static-check
  • git diff --check

During the unhappy-path test addition, it also exposed and fixed the existing "TSL handshake error" typo so production diagnostics now consistently say TLS.

@XuPeng-SH

XuPeng-SH commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Deep self-review found and fixed one additional admission trust-boundary issue in 584ff05.

Root cause:

  • The global slot is intentionally acquired before expensive per-connection state, but the per-tenant slot was previously bound as soon as the login packet revealed a tenant name.
  • Before backend authentication that name is only an untrusted client claim. An unauthenticated client could repeatedly claim another tenant and occupy the target tenant quota for the backend-auth window, causing targeted admission denial without owning any credentials.

The lifecycle is now:

global admission -> parse claimed tenant -> authenticate via fresh backend or cache authenticator -> bind tenant admission -> expose backend OK to client

This preserves the global memory/work bound during authentication while making tenant fairness depend on an authenticated identity. On tenant saturation, a freshly authenticated backend or cache hit is closed before any OK packet reaches the client. Backend authentication failures consume no tenant quota. Expected admission rejection remains debug-level and uses the existing reject metric, avoiding error-log amplification.

Added regression coverage for:

  • two unauthenticated connections claiming the same tenant without consuming its authoritative quota;
  • fresh-backend success binding before client OK;
  • fresh-backend tenant rejection closing the backend before client OK;
  • backend auth failure leaving tenant quota untouched;
  • authenticated cache-hit rejection and cleanup.

Validation on the pushed commit:

  • focused admission/lifecycle tests: 100x;
  • focused race tests: 20x;
  • full pkg/proxy;
  • full pkg/proxy with -race;
  • go build -mod=readonly ./pkg/proxy;
  • go vet -mod=readonly ./pkg/proxy;
  • make static-check;
  • git diff --check.

Performance remains outside the established tunnel/query hot path: one existing limiter mutex operation moves from pre-auth parsing to the successful authentication boundary; rejected unauthenticated claims no longer contend on per-tenant counters.

@LeftHandCold LeftHandCold 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-reviewed the latest head carefully and approving.\n\nI re-audited the new handshake lifecycle and admission boundary from first principles:\n- retained auth payload ownership is closed: backend connects hold a read lease, timeout closes the transport and joins the worker before returning, and client Close frees the allocator-backed payload exactly once;\n- coalesced plaintext/TLS bytes are handed to the tunnel without loss or stale ByteBuf access;\n- SSLRequest is phase-local, duplicate TLS requests are bounded/rejected, and the final login remains subject to the absolute handshake deadline and packet limit;\n- per-tenant admission is bound only after fresh-backend or cache authentication and before the backend OK is exposed; rejection closes the accepted backend and does not consume unauthenticated tenant quota;\n- PROXY v2 fragmentation/LOCAL/UNSPEC handling, oversized ERR sequencing, shared-memory accounting, cache and migration paths have no new reachable regression found.\n\nFresh validation on this exact head:\n- focused lifecycle/admission/TLS/PROXY/timeout tests: 10x;\n- focused ownership tests with : 10x;\n- full FAIL github.com/matrixorigin/matrixone/pkg/proxy [build failed]
FAIL;\n- full FAIL github.com/matrixorigin/matrixone/pkg/proxy [build failed]
FAIL;\n- ;\n- ;\n- dependent frontend SQL codec test;\n- .\n\nNo new realistic blocker found.

@LeftHandCold LeftHandCold 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.

Correction to my immediately preceding approval: its Markdown command names were expanded by the local shell while submitting the review body. The two displayed build-failed lines were submission-formatting artifacts, not validation results.

This approval is for head 584ff05. The actual fresh validation all passed:

  • focused lifecycle, admission, TLS, PROXY, timeout, and packet-sequence tests, 10 iterations;
  • focused ownership tests under the race detector, 10 iterations;
  • full pkg/proxy test suite;
  • full pkg/proxy suite under the race detector;
  • pkg/proxy build and vet;
  • dependent frontend SQL codec test;
  • diff check.

The code-review conclusion is unchanged: retained handshake ownership, timeout worker termination, pipelined plaintext and TLS handoff, authenticated tenant binding, packet bounds, cache and migration interactions form a closed lifecycle. I found no new realistic blocker on this head.

@gouhongshen gouhongshen 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.

Codex automated review

Valid configurations can fail normal migration/control-plane operations because peak shared allocations exceed the validated budget. The fragmented PROXY-v2 path also bypasses the configured handshake memory limit.

P1 - Account for the transient third protocol session (pkg/proxy/config.go:357)

Validation reserves only maxConnections*2 fixed sessions, but migration allocates the replacement backend session before replaceServerConn closes the old one (tunnel.go:491-496, tunnel.go:371-374). The same third-session peak occurs for connAndExec during upgrade/kill. A configuration accepted by the exact-boundary test can therefore reach the allocator limit and return out of space during legitimate operations. Reserve transient headroom or release/coordinate the old session before allocating its replacement.

P2 - Bound fragmented PROXY-v2 bodies by the handshake budget (pkg/proxy/ppv2.go:158)

The parser now waits for the complete declared body before delegating to the SQL codec. Length is a uint16, so an unauthenticated client can send an UNSPEC frame with a 65535-byte body and make goetty retain roughly 64 KiB outside the shared allocator; line 175 also creates a second temporary copy once complete. ClientHandshakePacketLimit can legally be as low as 35, but it is never applied to this frame, so the new fragmented-header path bypasses the configured handshake memory bound until timeout.

@aptend aptend 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-review of head 584ff05: the protocol-memory bound is still not closed, and the fragmented PROXY-v2 path still bypasses it.

[P1] Account for all peak shared protocol allocations

Config.Validate accepts an exact budget of two 16 KiB IOSessions plus one retained login payload per admitted connection. That is below several reachable peaks. A valid 32 KiB login fills the accepted client + retained payload + backend budget, then forwarding that login to the backend needs an additional dynamic write block and can fail with out of space. Established connections also need a third IOSession while migration creates the replacement backend before closing the old one; connAndExec has the same peak for upgrade/kill operations. Pipelined handshake read-ahead adds another allocator-backed prefix that is also omitted. Consequently, configurations accepted by validation can reject normal login, migration, and control-plane work. Please reserve the complete peak or add explicit admission/reservation that guarantees these operations can allocate.

[P2] Bound PROXY-v2 bodies within the handshake memory budget

parseProxyHeaderV2 waits for the complete uint16-declared body before the SQL codec runs, so an unauthenticated client can retain up to 65,535 bytes in goetty input memory that is outside the shared SessionAllocator and unaffected by ClientHandshakePacketLimit. Once complete, the parser also allocates and copies another full body. This violates ProtocolMemoryLimit even when the configured handshake limit is only 35 bytes. Please reject bodies above an explicit accounted limit, or bring the goetty input allocation and parser copy under the same shared bound.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working kind/enhancement kind/test-ci size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Proxy deterministically runs out of memory as idle connections grow without bounds

5 participants