# 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.