# TcpServer admission race — analysis and two measured fixes **Repo:** CodexCoder21Organization/UrlProtocol · **Base:** `e804219c` · **Defect arrived in:** [PR #493](https://github.com/CodexCoder21Organization/UrlProtocol/pull/493) · **Fix PR:** [#498](https://github.com/CodexCoder21Organization/UrlProtocol/pull/498) `TcpServer` reports its concurrent-handler limit to callers that are within that limit. The cause is settled and reproducible. What I want challenged is the fix: closing it completely costs a platform-thread bound that was deliberately established one commit earlier, and I believe that trade is unavoidable rather than a failure of imagination. **That belief is the thing to attack.** --- ## 1. What is observably wrong A caller that completes one request and immediately issues the next receives: ``` RpcError(code=CONCURRENT_HANDLER_LIMIT, message=TcpServer on port 41571 is at its concurrent request-handler limit of 2; request recovered-request for method 'status' was not started) ``` At the moment of that refusal, **no handler is running**. The statement is false, and it is user-visible: the caller is told to back off by a server that is idle. This surfaced as a red `kotlin.build (remote)` check — `testTcpServerRejectsRequestsAtConcurrentHandlerLimitAndRecovers`, 1 failure in 1372 tests, retries exhausted. It passes on an unloaded local machine and fails on the loaded CI droplet, which is the signature of a timing window rather than a broken assertion. --- ## 2. Mechanism Admission was never counted. It was *inferred* from a thread pool: ```kotlin ThreadPoolExecutor( 0, // corePoolSize concurrentHandlerLimit, // maximumPoolSize REQUEST_THREAD_KEEP_ALIVE_SECONDS, TimeUnit.SECONDS, SynchronousQueue(), threadFactory, ThreadPoolExecutor.AbortPolicy(), ) ``` A `SynchronousQueue` has no capacity of its own. `execute()` succeeds only if it can hand the task to a worker **currently parked in `poll()`**, or if it may create a new thread. Once `poolSize` has reached `maximumPoolSize`, the second option is gone — so capacity means *"a worker is parked right now"*, not *"a handler slot is free"*. Those two things diverge for as long as it takes the scheduler to run a worker that has just finished. Ordered by happens-before: | # | Actor | What happens | |---|---|---| | 1 | reader thread | Reads request *i*, calls `execute()`. A worker takes it. | | 2 | worker | Runs the handler and writes the response to the socket. | | **3** | **worker — the window** | **Returns from the task and begins unwinding toward `poll()`. It is not yet parked, and `poolSize == maximumPoolSize`.** | | **4** | **client** | **Has already read the response from step 2. Sends request *i+1*.** | | **5** | **reader thread** | **`execute()` finds no parked worker and may not create one. `RejectedExecutionException` → `CONCURRENT_HANDLER_LIMIT`.** | | 6 | worker | Finally parks. Capacity "returns", having never actually been used. | Steps 3–5 are the defect. CPU contention widens the gap between 3 and 6, which is exactly why it reproduces on a loaded CI droplet and hides on an idle laptop. --- ## 3. Reproducer `tests/testTcpServerDoesNotReportHandlerLimitBelowIt.kts` The assertion is designed to be machine-independent. *N* client lanes each issue requests strictly sequentially, waiting for each response before sending the next, and there are exactly `concurrentHandlerLimit` lanes. At most `concurrentHandlerLimit` requests can be in flight at any instant, so the server is never genuinely saturated and **no** response may carry `CONCURRENT_HANDLER_LIMIT`. One such response is a failure. The iteration count samples the window; it is not load, and it is not a threshold. Nothing is tuned to a machine — a faster host changes how often step 4 lands inside steps 3–6, not whether the assertion is correct. > **Why the failure distribution confirms the mechanism** > > On unfixed `main`, **every lane failed at request index 1** — the very first request after its > first response. Request 0 creates the worker; the worker finishes and writes; request 1 arrives > before it has re-parked. That is step 3 exactly, and it is the reason I stopped looking for a load > explanation. --- ## 4. Two variants, measured Both replace the inferred capacity with an explicit `Semaphore(concurrentHandlerLimit)` acquired before dispatch, and let the pool grow unbounded so it can never reject an admitted request. They differ only in **when the permit is released**. Same reproducer, same host, 1600 requests. | Variant | Permit released | Spurious refusals | PR #493 test | |---|---|---|---| | Unfixed `main` | n/a — capacity inferred from the pool | **23 / 1600** | fails on CI | | **A** — semaphore | after the response is written | 1 / 1600 | passes | | **B** — semaphore | after the handler returns, before the write | **0 / 1600** | passes | Variant A removes the scheduler from the accounting and fixes the reported CI failure, but leaves a narrower instance of the same class: the client reads the response before the handler thread executes `release()`. Variant B releases after the handler returns and before any byte can reach the caller, so a caller holding a response is guaranteed the slot that served it is already free. **Variant B is what shipped in [#498](https://github.com/CodexCoder21Organization/UrlProtocol/pull/498).** --- ## 5. The trade, and why I think it is inherent This is the part I most want a second opinion on, because the argument is short enough to be suspicious. > **The argument** > > For the server never to refuse a caller that respects the limit, nothing that gates admission may > still be held while the response is being written — otherwise a caller holding its response can > still be refused, which is the bug. But the *thread* is held through the write, by construction. > Therefore threads cannot be the gate. If that holds, variant B's cost is not an oversight but the price: with the permit released before the write, a peer that pipelines requests while not reading its responses is no longer refused, and its handlers accumulate threads instead — one writing, the rest blocked on the connection's `output` monitor. Previously the held slot supplied that backpressure. That backpressure is what PR #493 established one commit earlier, specifically to bound platform threads. My reading is that bounding a stalled peer belongs at the socket layer — per-connection in-flight response limits, or write deadlines — not in handler-slot accounting, and that refusing unrelated well-behaved callers is the wrong way to get it. But I hold that loosely, and a redesign that bounds writes properly would dominate both variants. > **Evidence that partly cuts against my own concern** > > Under variant B all 26 existing TCP tests pass, including > `testTcpPushIsBoundedWhenClientStopsReading` — the repository's own test for the stalled-reader > case I am worried about. So the concern is **not** currently demonstrated by any test in the > suite. Either the existing coverage does not reach the accumulation path, or the path is bounded > by something I have not identified. I could not tell which, and that uncertainty is why this > document exists rather than just a merged PR. --- ## 6. What I would like attacked 1. Is the inherency argument in §5 actually sound, or is there a formulation where admission is gated by something released before the write while threads stay bounded — a bounded writer stage, non-blocking writes, or per-connection in-flight caps? 2. Is *"a caller within the limit is never refused"* the right contract at all? The alternative is that a slot is occupied until its response is fully written, which makes **variant A correct** and makes my stricter reproducer wrong rather than the server. 3. Variant B changes behaviour under a hostile or stalled peer, in a subsystem hardened for exactly that one commit earlier. Is shipping it without the socket-layer bound in place too risky to do in one step? 4. Is 1600 requests on one host adequate evidence that B is 0 rather than merely rarer than A? I did not run a power analysis, and A's residual was 1 in 1600. --- ## Appendix — provenance of the numbers - Measurements are from a single 16-core host, one run per variant, using the same reproducer and the same `concurrentHandlerLimit = 4` with 4 lanes × 400 sequential requests. - The CI observation is from buildtest run `a44e4416`, read from the executor's `test-events.jsonl` on the host rather than the frontend, which was returning HTTP 500/503 at the time. - Post-fix CI: run `d2e49e12`, `kotlin.build (remote)` passed in 15m14s.