SocketHttpServer never closes the client when the HTTP driver returns normally (fd leak, can pin a core at 100%) #390

Summary

SocketHttpServer::handleClient() only closes the client on three paths: a failed TLS handshake, a not-yet-started server, or an exception reaching the catch block. When $driver->handleClient() returns normally, the finally block just unsets the driver from the map and the socket is never closed:

try {
    $driver->handleClient($client, $socket, $socket);
} finally {
    unset($this->drivers[$id]);   // <- no $client->close()
}

Nothing releases the descriptor afterwards. Neither ReadableResourceStream::close() nor WritableResourceStream::close() calls fclose() on a socket — each does stream_socket_shutdown(SHUT_RD/SHUT_WR) and drops its own reference, so the fd is freed only once both halves free it and the refcount reaches zero. With no close() at all, the descriptor and its event-loop watcher are retained for the lifetime of the process.

Version

amphp/http-server 3.4.5 (also present in 3.x main as far as I can tell), amphp/byte-stream 2.1.2, PHP 8.4.

Reproduction

Deterministic — no timing dependency. A trivial HttpDriver that returns from handleClient() reproduces the same control flow a real driver takes at end-of-connection:

$noopDriver = new class implements HttpDriver {
    public ?Client $seen = null;
    public function handleClient(Client $client, ReadableStream $r, WritableStream $w): void {
        $this->seen = $client;   // return normally, as at end-of-connection
    }
    public function getPendingRequestCount(): int { return 0; }
    public function getApplicationLayerProtocols(): array { return []; }
    public function stop(): void {}
};

$server = new SocketHttpServer(new NullLogger(), new ResourceServerSocketFactory(),
    new SocketClientFactory(new NullLogger()), httpDriverFactory: $factoryReturning($noopDriver));
$server->expose(SocketAddress\fromString('127.0.0.1:0'));
$server->start($requestHandler, new DefaultErrorHandler());

$client = Socket\connect($server->getServers()[0]->getAddress()->toString());
$client->write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");
\Amp\delay(0.3);
$client->close();
\Amp\delay(0.3);

var_dump($noopDriver->seen->isClosed());   // expected true

Output:

driver received a client:            yes
client closed after driver returned: NO
RESULT: LEAKED - SocketHttpServer never closed the client

Impact

Beyond the descriptor leak, whether the leak is merely wasteful or actively burns a core depends on the write queue at the moment of abandonment. In WritableResourceStream's watcher finally:

• write queue empty → EventLoop::disable() → idle leak, no CPU cost
• write queue non-empty → the watcher is left enabled

A dead socket is permanently "writable" as far as epoll is concerned, so in the second case the watcher retries a failing write forever. Measured at roughly 30k EPIPE writes/second, per leaked descriptor, indefinitely.

In a production deployment this surfaced as two cluster workers pinned at 100% CPU continuously for 19 days, holding 35 and 37 orphaned descriptors between them. The real-world trigger is a peer that RSTs a connection sitting idle in HTTP/1.1 keep-alive — i.e. ordinary internet scanner traffic against a public listener. Roughly 60% of such connections leaked in our testing.

Suggested fix

             try {
                 $driver->handleClient($client, $socket, $socket);
             } finally {
+                $client->close();
                 unset($this->drivers[$id]);
             }

With that applied the reproduction above prints client closed after driver returned: yes, and the production symptom disappears: 15 runs of the triggering sequence produce zero orphaned descriptors and 0% idle CPU.

For what it's worth, this is what Amp\Cluster users are most exposed to, and it is also worth noting that at least one downstream project which modelled its own socket server on this method (Flyokai\DataService\DataServer\SocketDataServer::handleClient()) does call $client->close() in the equivalent finally — which is why its non-HTTP listener never exhibited the leak while the HTTP one did.