fix(frontend): promptly cancel requests after client disconnect - #27634
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head fd50bd36c1f93b253e59bbefffe12e364fe50382 against merge base 1891117aedce0419a81630b3011906b560b93d0a completed.
The correctness/lifecycle direction is sound: only active requests are considered, the Unix probe is non-consuming, beginClose seals admission before cancellation, and the existing cleanup remains the sole rollback/resource owner. The real disconnect scenario demonstrates the lock is released promptly. One production performance blocker remains.
[P1 performance] Do not run two socket syscalls per active request every second without a bounded-capacity design and evidence.
This changes the monitor from once per 5 seconds and only after a 30-second grace period to once per second for every active request. For the common workload of many 1–30 second queries, the old path performed zero socket probes while this head serially performs poll(2) plus recvfrom(MSG_PEEK|MSG_DONTWAIT) for each request on one monitor goroutine, in addition to scanning every connection and allocating the active slice. A temporary local macOS probe using the same live-TCP SyscallConn.Control / Poll / Recvfrom sequence took 31–32ms per pass for 2,000 live connections; 10,000 active connections is roughly 160ms of one core per tick before manager overhead, and the frontend allows max_connections up to 100,000. At higher occupancy the monitor can consume material CPU or fall behind its own interval.
Please make the no-event live path materially cheaper (for example, avoid Recvfrom when Poll reports no relevant event, and/or batch/stagger/cap probes rather than issuing two syscalls for every request in one tick), then attach before/after benchmarks for realistic 10k and upper-bound connection populations with active ratios, allocations, and a stated CPU/tick budget. Retain the first-tick disconnect latency regression and false-positive/TLS/unread-byte coverage. Also rate-limit/deduplicate persistent probe-error diagnostics if the new cadence can emit the same error every second.
Exact-head CI is green; this is independent of CI correctness.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head 92e43943f35e7687700ced7d2f83d10d6e7c373f against merge base b0ae66c04d9a67175d2dfb8bf1c777fb888e1430 completed.
The previous performance blocker is closed. The common live-socket path now performs one zero-timeout poll, calls recv(MSG_PEEK | MSG_DONTWAIT) only for readable events, reuses both per-connection callback state and the manager snapshot, and has a zero-allocation regression test. The supplied 10k distinct-FD and 100k upper-bound measurements establish a bounded per-tick CPU cost under the existing connection cap, while persistent probe errors use a process-wide bounded event.
I also re-audited correctness and lifecycle behavior: the probe is non-consuming; Linux POLLRDHUP and Darwin POLLHUP cover disconnects even with unread bytes; syscall.RawConn.Control keeps the descriptor valid during the callback; TLS replacement refreshes the cached probe; stale/closing routines are not re-probed; and beginClose only seals/cancels lifecycle work while the existing single cleanup owner performs rollback and connection teardown. The first-tick policy and real explicit-transaction FOR UPDATE regression demonstrate that issue #27595 is addressed without introducing a new wait cycle or unbounded logging path.
Exact-head CI is green, including frontend UT and BVT. No blocking correctness, performance, or unhappy-path issue found.
Merge Queue Status
This pull request spent 4 minutes 54 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #27595
What this PR does / why we need it:
Probe active frontend requests for a disconnected client from the first one-second monitor tick. This promptly cancels the request context and lets the existing connection cleanup roll back an open transaction, so locks are not retained after a direct client disconnect.
The liveness hot path is bounded by the configured
max_connectionslimit (100k): it reuses the request snapshot and a cached non-owningRawConn, performs only a zero-timeoutpollfor the common no-event case, and callsrecv(MSG_PEEK | MSG_DONTWAIT)only when the socket is readable. Probe errors use a process-wide rate-limited event. TLS and unread protocol bytes remain covered.Steady-state benchmark medians (
-benchtime=5x -count=5) show zero allocations for the new path. A realistic 10k-distinct-socket scan at 100% active takes about 7.36 ms per one-second tick (0.74% of one core); the 100k upper-bound shared-fd case takes about 55.1 ms (5.5% of one core). At 100% active, the reviewed implementation took about 5.61 ms / 2.5 MB / 50k allocs for 10k connections and 70.3 ms / 27.3 MB / 500k allocs for 100k connections; this update takes about 2.98 ms / 0 allocs and 55.1 ms / 0 allocs in the same shared-fd cases.Add policy, socket, allocation, TLS, unread-byte, and probe-refresh coverage, plus a reproducible end-to-end scenario covering an explicit transaction that holds a
FOR UPDATElock while a long-running statement is active.Validation:
make buildgo build,go vet, and full tests for./pkg/frontendwith the required CGO environment-racerun; seven lifecycle/probe tests also pass individually with-race -count=100