electric-sql / electric-sql/electric
AdmissionControl permits leak when HTTP/2 stream processes terminate with their connection
- Dominant language
- TypeScript
- Stars
- 10.4k
- Forks
- 375
- Avg merge
- 3d 1h
- Merged PRs (30d)
- 18
Description
**Versions**
- Electric: current `main`
- HTTP server: Bandit 1.12.0
- Protocol: HTTP/2
**Bug description**
AdmissionControl permits are released by an `after` block in `Electric.Plug.ServeShapePlug.call/2`:
```elixir
try do
# Run the shape request
after
release_admission_permit()
end
```
The acquired permit is stored only in the request process dictionary:
```elixir
Process.put(@admission_permit_key, {stack_id, kind})
```
This handles normal returns, exceptions, throws, and exits raised within the request process. It does not handle the request process being terminated by an external exit signal, because the process stops without executing the `after` block.
This matters under Bandit's HTTP/2 process model:
1. Each HTTP/2 stream runs its Plug pipeline in a separate `Bandit.HTTP2.StreamProcess`.
2. The stream process is linked to the HTTP/2 connection process.
3. The stream process does not trap exits.
4. When the connection process terminates with a non-normal reason, its active stream processes terminate immediately.
5. Any AdmissionControl permits held by those processes remain counted in the AdmissionControl ETS table.
HTTP/2 connection termination can happen when:
- The peer closes the connection while streams are active.
- The socket encounters an error.
- The server closes the connection.
- The HTTP/2 `max_requests` behavior described in #4771 sends GOAWAY and closes the connection.
Electric's live requests may spend up to `long_poll_timeout` waiting for changes, so it is normal for an HTTP/2 connection to have active request processes when the connection disappears.
Because the AdmissionControl ETS table is owned independently of the request and connection processes, leaked counts do not recover when those processes terminate. Repeated connection closures can ratchet the `:initial` or `:existing` counter upward until the configured concurrency limit is exhausted. At that point, subsequent requests receive `503 concurrent_request_limit_exceeded` despite there being fewer—or no—corresponding live request processes.
The counters only recover when AdmissionControl or the VM restarts.
**Expected behavior**
An AdmissionControl permit should be released exactly once when its owning request process terminates, regardless of whether the request:
- Returns normally.
- Raises or exits internally.
- Is terminated by an external exit signal.
- Is killed because its HTTP/2 connection closes.
AdmissionControl's counters should represent live permit holders rather than relying exclusively on cooperative cleanup by those holders.
**Suggested fix**
Associate each acquired permit with its owning request process and monitor that process.
Both normal release and owner-process termination should attempt to atomically claim the permit lease before decrementing the counter. Only the path that successfully claims the lease should perform the decrement. This provides exactly-once release when normal cleanup races with a `:DOWN` notification.
The current direct ETS counter updates are intentionally cheap, so the implementation should avoid serializing every stack through one global GenServer. Possible implementations include partitioned lease monitors or another ownership mechanism that preserves concurrency across stacks.
Making HTTP/2 stream processes trap connection exits would be a narrower workaround, but it would only defer cleanup until the request returns and would couple Electric to Bandit's process model. Owner-monitored permits address the underlying AdmissionControl invariant regardless of why a request process terminates.
**Suggested regression test**
1. Start Bandit with HTTP/2 enabled.
2. Configure an AdmissionControl limit of one.
3. Open a live request that remains in its long-poll wait and verify the counter is one.
4. Close the underlying HTTP/2 connection while the stream remains active.
5. Wait for the stream process to terminate.
6. Assert that the AdmissionControl counter returns to zero.
7. Assert that another request can acquire the permit.
A second test should cover the GOAWAY path from #4771 by configuring a very small HTTP/2 `max_requests` value.
The test should also verify that normal request completion does not cause a double release if process-death cleanup races with the existing `after` block.
**Acceptance criteria**
- Admission permits are associated with their owning request processes.
- A permit is reclaimed when its owner terminates unexpectedly.
- Normal completion and owner-death cleanup cannot release the same permit twice.
- HTTP/2 connection closure with active streams does not leave AdmissionControl counters elevated.
- A regression test covers externally terminated HTTP/2 stream processes.
- #4771 remains the mitigation for avoiding request-count-driven HTTP/2 connection cycling; this issue does not reintroduce an HTTP/2 request cap.
Contributor guide
Assessment
This issue has not been assessed yet.