From 1302dee84d6453d0bfe7117540c6e5af4f89ae45 Mon Sep 17 00:00:00 2001 From: Mathijs Smit Date: Fri, 5 Jun 2026 14:54:11 +0200 Subject: [PATCH 1/3] Add Session daemon state restore after transport reconnect. Replay EVENT_REGISTER from registration refcounts via ReconnectableTransportInterface, retry request() once on connection loss, and harden streamed command cleanup. --- README.md | 8 +- src/Session.php | 153 +++++++++++--- .../ReconnectableTransportInterface.php | 20 ++ src/Transport/ReconnectingTransport.php | 12 +- src/Transport/UnixSocketTransport.php | 14 +- tests/Integration/FileSocketViciServer.php | 20 ++ .../SessionRestoreIntegrationTest.php | 188 ++++++++++++++++++ 7 files changed, 378 insertions(+), 37 deletions(-) create mode 100644 src/Transport/ReconnectableTransportInterface.php create mode 100644 tests/Integration/SessionRestoreIntegrationTest.php diff --git a/README.md b/README.md index 4870c3c..12dd415 100644 --- a/README.md +++ b/README.md @@ -89,11 +89,13 @@ while (true) { `ReconnectingTransport` is opt-in; `new Session()` alone still uses a plain `UnixSocketTransport` with no automatic recovery. -**v1 limitations** +When the transport implements {@see \Bk203\Vici\Transport\ReconnectableTransportInterface} (including `ReconnectingTransport`), `Session` automatically replays daemon-side `EVENT_REGISTER` calls after each reconnect. -- Retry covers one transport I/O call. Multi-packet commands (`streamedRequest()`, `EventListener::listen()`) can still fail mid-operation; catch `ConnectionException` and restart the command or listener loop. -- Daemon-side `EVENT_REGISTER` state is not replayed after reconnect. Re-register events or wait for a future Session-level restore helper. +**Limitations** + +- `request()` / `requireSuccess()` retry once on `ConnectionException`. Multi-packet commands (`streamedRequest()`, `EventListener::listen()`) do not auto-resume mid-operation; catch `ConnectionException` and restart the command or listener loop. - `TimeoutException` is not retried (slow charon is not treated as a dead socket). +- Custom transports can implement `ReconnectableTransportInterface` and receive the same restore hook via `setOnReconnect()`. ## Common workflows diff --git a/src/Session.php b/src/Session.php index b7be676..0fffdd3 100644 --- a/src/Session.php +++ b/src/Session.php @@ -6,6 +6,7 @@ use Bk203\Vici\Exception\CommandException; use Bk203\Vici\Exception\CommandUnknownException; +use Bk203\Vici\Exception\ConnectionException; use Bk203\Vici\Exception\EventRegistrationException; use Bk203\Vici\Exception\ProtocolException; use Bk203\Vici\Message\MessageDecoder; @@ -13,6 +14,8 @@ use Bk203\Vici\Protocol\Packet; use Bk203\Vici\Protocol\PacketCodec; use Bk203\Vici\Protocol\PacketType; +use Bk203\Vici\Transport\ReconnectableTransportInterface; +use Bk203\Vici\Transport\ReconnectingTransport; use Bk203\Vici\Transport\TransportInterface; use Bk203\Vici\Transport\UnixSocketTransport; use Generator; @@ -49,6 +52,10 @@ public function __construct( $this->packetCodec = new PacketCodec(); $this->messageEncoder = new MessageEncoder(); $this->messageDecoder = new MessageDecoder(); + + if ($this->transport instanceof ReconnectableTransportInterface) { + $this->transport->setOnReconnect($this->restoreDaemonState(...)); + } } public function transport(): TransportInterface @@ -96,20 +103,7 @@ public function registerEvent(string $event): void return; } - $this->writePacket(Packet::eventRegister($event)); - - $reply = $this->readUntilControlPacket([ - PacketType::EVENT_CONFIRM, - PacketType::EVENT_UNKNOWN, - ]); - - if ($reply->type === PacketType::EVENT_UNKNOWN) { - throw new EventRegistrationException( - \sprintf('VICI daemon does not know event "%s".', $event), - $event, - ); - } - + $this->registerEventOnDaemon($event); $this->registrationRefcount[$event] = 1; } @@ -142,22 +136,13 @@ public function unregisterEvent(string $event): void */ public function request(string $command, array $message = []): array { - $payload = $this->messageEncoder->encode($message); - $this->writePacket(Packet::cmdRequest($command, $payload)); - - $reply = $this->readUntilControlPacket([ - PacketType::CMD_RESPONSE, - PacketType::CMD_UNKNOWN, - ]); + try { + return $this->executeRequest($command, $message); + } catch (ConnectionException) { + $this->recoverViaTransportReconnect(); - if ($reply->type === PacketType::CMD_UNKNOWN) { - throw new CommandUnknownException(\sprintf( - 'VICI daemon does not implement command "%s".', - $command, - )); + return $this->executeRequest($command, $message); } - - return $this->messageDecoder->decode($reply->payload); } /** @@ -241,10 +226,21 @@ public function streamedRequest(string $command, string $streamEvent, array $mes )); } } finally { - if (!$commandCompleted) { - $this->drainStreamRemainder($streamEvent); + try { + if (!$commandCompleted) { + $this->drainStreamRemainder($streamEvent); + } + $this->unregisterEvent($streamEvent); + } catch (ConnectionException) { + try { + $this->recoverViaTransportReconnect(); + if (!$commandCompleted) { + $this->drainStreamRemainder($streamEvent); + } + $this->unregisterEvent($streamEvent); + } catch (ConnectionException) { + } } - $this->unregisterEvent($streamEvent); } } @@ -353,14 +349,107 @@ private function dispatchEvent(string $event, array $message): void } } + /** + * @param array $message + * @return array + */ + private function executeRequest(string $command, array $message): array + { + $payload = $this->messageEncoder->encode($message); + $this->writePacket(Packet::cmdRequest($command, $payload)); + + $reply = $this->readUntilControlPacket([ + PacketType::CMD_RESPONSE, + PacketType::CMD_UNKNOWN, + ]); + + if ($reply->type === PacketType::CMD_UNKNOWN) { + throw new CommandUnknownException(\sprintf( + 'VICI daemon does not implement command "%s".', + $command, + )); + } + + return $this->messageDecoder->decode($reply->payload); + } + + private function registerEventOnDaemon(string $event): void + { + $this->writePacket(Packet::eventRegister($event)); + + $reply = $this->readUntilControlPacket([ + PacketType::EVENT_CONFIRM, + PacketType::EVENT_UNKNOWN, + ]); + + if ($reply->type === PacketType::EVENT_UNKNOWN) { + throw new EventRegistrationException( + \sprintf('VICI daemon does not know event "%s".', $event), + $event, + ); + } + } + + private function restoreDaemonState(): void + { + foreach ($this->registrationRefcount as $event => $count) { + if ($count < 1) { + continue; + } + $this->registerEventOnDaemon($event); + } + } + + private function recoverViaTransportReconnect(): void + { + if (!$this->transport instanceof ReconnectableTransportInterface) { + throw new ConnectionException('VICI transport does not support reconnection.'); + } + + $this->transport->reconnect(); + } + + private function usesSessionIoRecovery(): bool + { + return $this->transport instanceof ReconnectableTransportInterface + && !$this->transport instanceof ReconnectingTransport; + } + private function writePacket(Packet $packet): void { - $this->transport->send($this->packetCodec->encode($packet)); + $bytes = $this->packetCodec->encode($packet); + + try { + $this->transport->send($bytes); + } catch (ConnectionException $e) { + if (!$this->usesSessionIoRecovery()) { + throw $e; + } + + $this->recoverViaTransportReconnect(); + $this->transport->send($bytes); + } } private function readPacket(): Packet + { + try { + return $this->decodeReceivedPacket(); + } catch (ConnectionException $e) { + if (!$this->usesSessionIoRecovery()) { + throw $e; + } + + $this->recoverViaTransportReconnect(); + + return $this->decodeReceivedPacket(); + } + } + + private function decodeReceivedPacket(): Packet { $bytes = $this->transport->receive(); + return $this->packetCodec->decode($bytes); } diff --git a/src/Transport/ReconnectableTransportInterface.php b/src/Transport/ReconnectableTransportInterface.php new file mode 100644 index 0000000..a49e076 --- /dev/null +++ b/src/Transport/ReconnectableTransportInterface.php @@ -0,0 +1,20 @@ +inner->getStream(); } + public function reconnect(): void + { + $this->inner->reconnect(); + } + + public function setOnReconnect(?callable $callback): void + { + $this->inner->setOnReconnect($callback); + } + /** * @template T * diff --git a/src/Transport/UnixSocketTransport.php b/src/Transport/UnixSocketTransport.php index 70cfec0..96385df 100644 --- a/src/Transport/UnixSocketTransport.php +++ b/src/Transport/UnixSocketTransport.php @@ -9,10 +9,13 @@ /** * Connects to a charon VICI Unix domain socket. */ -final class UnixSocketTransport extends SocketTransport +final class UnixSocketTransport extends SocketTransport implements ReconnectableTransportInterface { public const string DEFAULT_PATH = '/var/run/charon.vici'; + /** @var (callable(): void)|null */ + private $onReconnect = null; + public function __construct( public readonly string $path = self::DEFAULT_PATH, public readonly float $connectTimeout = 5.0, @@ -26,6 +29,15 @@ public function reconnect(): void { $this->close(); $this->connect(); + + if ($this->onReconnect !== null) { + ($this->onReconnect)(); + } + } + + public function setOnReconnect(?callable $callback): void + { + $this->onReconnect = $callback; } protected function connect(): void diff --git a/tests/Integration/FileSocketViciServer.php b/tests/Integration/FileSocketViciServer.php index ab31e9b..1dee978 100644 --- a/tests/Integration/FileSocketViciServer.php +++ b/tests/Integration/FileSocketViciServer.php @@ -117,6 +117,13 @@ public function expectCommand(string $command, float $timeout = 1.0): array return $packet->payload === '' ? [] : $this->decoder->decode($packet->payload); } + public function expectEventRegister(string $event, float $timeout = 1.0): void + { + $packet = $this->readPacket($timeout); + Assert::assertSame(PacketType::EVENT_REGISTER, $packet->type); + Assert::assertSame($event, $packet->name); + } + /** * @param array $message */ @@ -125,6 +132,19 @@ public function sendCmdResponse(array $message = []): void $this->writePacket(Packet::cmdResponse($this->encoder->encode($message))); } + public function sendEventConfirm(): void + { + $this->writePacket(Packet::eventConfirm()); + } + + /** + * @param array $message + */ + public function sendEvent(string $event, array $message = []): void + { + $this->writePacket(Packet::event($event, $this->encoder->encode($message))); + } + private function readPacket(float $timeout = 1.0): Packet { if (!\is_resource($this->serverStream)) { diff --git a/tests/Integration/SessionRestoreIntegrationTest.php b/tests/Integration/SessionRestoreIntegrationTest.php new file mode 100644 index 0000000..036e1ba --- /dev/null +++ b/tests/Integration/SessionRestoreIntegrationTest.php @@ -0,0 +1,188 @@ +server = new FileSocketViciServer(); + + $this->session = new Session(new ReconnectingTransport( + path: $this->server->getPath(), + connectTimeout: 2.0, + readTimeout: 2.0, + maxReconnectAttempts: 3, + reconnectDelayMs: 50, + )); + $this->server->acceptClient(); + } + + protected function tearDown(): void + { + $this->session->close(); + $this->server->close(); + } + + public function testRestoreDaemonStateReplaysEventRegistrationAfterRestart(): void + { + if (!\function_exists('pcntl_fork')) { + self::markTestSkipped('pcntl extension required to accept during reconnect.'); + } + + $this->server->sendEventConfirm(); + $this->session->registerEvent(Event::LOG); + $this->server->expectEventRegister(Event::LOG); + + $this->server->simulateRestart(); + + $server = $this->server; + $pid = pcntl_fork(); + if ($pid === -1) { + self::markTestSkipped('pcntl_fork() failed.'); + } + if ($pid === 0) { + $server->acceptClient(5.0); + $server->expectEventRegister(Event::LOG); + $server->sendEventConfirm(); + $server->expectCommand('version'); + $server->sendCmdResponse([ + 'daemon' => 'charon', + 'version' => '5.9.13', + 'sysname' => 'Linux', + 'release' => '6.1.0', + 'machine' => 'x86_64', + ]); + exit(0); + } + + $version = $this->session->version(); + pcntl_waitpid($pid, $status); + + self::assertSame('5.9.13', $version['version']); + self::assertSame(0, pcntl_wexitstatus($status)); + } + + public function testRequestRetriesAfterMidCommandDisconnect(): void + { + if (!\function_exists('pcntl_fork')) { + self::markTestSkipped('pcntl extension required to accept during reconnect.'); + } + + $this->server->simulateRestart(); + + $server = $this->server; + $pid = pcntl_fork(); + if ($pid === -1) { + self::markTestSkipped('pcntl_fork() failed.'); + } + if ($pid === 0) { + $server->acceptClient(5.0); + $server->expectCommand('version'); + $server->sendCmdResponse([ + 'daemon' => 'charon', + 'version' => '5.9.14', + 'sysname' => 'Linux', + 'release' => '6.1.0', + 'machine' => 'x86_64', + ]); + exit(0); + } + + $version = $this->session->version(); + pcntl_waitpid($pid, $status); + + self::assertSame('5.9.14', $version['version']); + self::assertSame(0, pcntl_wexitstatus($status)); + } + + public function testEventDeliveryAfterRestore(): void + { + if (!\function_exists('pcntl_fork')) { + self::markTestSkipped('pcntl extension required to accept during reconnect.'); + } + + $received = []; + $this->session->onEvent(Event::LOG, static function (string $name, array $msg) use (&$received): void { + $received[] = [$name, $msg]; + }); + + $this->server->sendEventConfirm(); + $this->session->registerEvent(Event::LOG); + $this->server->expectEventRegister(Event::LOG); + + $this->server->simulateRestart(); + + $server = $this->server; + $pid = pcntl_fork(); + if ($pid === -1) { + self::markTestSkipped('pcntl_fork() failed.'); + } + if ($pid === 0) { + $server->acceptClient(5.0); + $server->expectEventRegister(Event::LOG); + $server->sendEventConfirm(); + $server->expectCommand('version'); + $server->sendEvent(Event::LOG, [ + 'group' => 'IKE', + 'level' => '1', + 'msg' => 'restored', + ]); + $server->sendCmdResponse([ + 'daemon' => 'charon', + 'version' => '5.9.13', + 'sysname' => 'Linux', + 'release' => '6.1.0', + 'machine' => 'x86_64', + ]); + exit(0); + } + + $version = $this->session->version(); + pcntl_waitpid($pid, $status); + + self::assertSame('5.9.13', $version['version']); + self::assertCount(1, $received); + self::assertSame(Event::LOG, $received[0][0]); + self::assertSame('restored', $received[0][1]['msg']); + self::assertSame(0, pcntl_wexitstatus($status)); + } + + public function testStreamedRequestDoesNotAutoResumeOnDisconnect(): void + { + $mock = new MockViciServer(); + $session = new Session($mock->getClientTransport()); + + $gen = null; + try { + $mock->sendEventConfirm(); + $mock->sendEvent(Event::LIST_SA, ['gw' => ['uniqueid' => '1']]); + + $gen = $session->listSas(); + self::assertSame('1', $gen->current()['gw']['uniqueid']); + + $mock->simulateRestart(); + + try { + $gen->next(); + self::fail('Expected ConnectionException.'); + } catch (ConnectionException) { + } + } finally { + unset($gen); + $session->close(); + $mock->close(); + } + } +} From 88f8fb1e60c0c3060d5fd5d2525703b75e2dfa32 Mon Sep 17 00:00:00 2001 From: Mathijs Smit Date: Fri, 5 Jun 2026 15:50:40 +0200 Subject: [PATCH 2/3] Add ConnectionException diagnostics for transport failures. Capture stream metadata, endpoint state, partial I/O progress, and PHP errors in ConnectionFailureContext so stale socket failures are easier to diagnose. --- README.md | 17 ++- src/Exception/ConnectionException.php | 16 +++ src/Exception/ConnectionFailureContext.php | 68 ++++++++++ src/Transport/ReconnectingTransport.php | 8 ++ src/Transport/SocketTransport.php | 123 ++++++++++++++++-- src/Transport/TcpSocketTransport.php | 28 +++- src/Transport/UnixSocketTransport.php | 28 +++- .../Exception/ConnectionExceptionTest.php | 46 +++++++ tests/Unit/Transport/StreamTransportTest.php | 5 +- .../Transport/UnixSocketTransportTest.php | 10 +- 10 files changed, 323 insertions(+), 26 deletions(-) create mode 100644 src/Exception/ConnectionFailureContext.php create mode 100644 tests/Unit/Exception/ConnectionExceptionTest.php diff --git a/README.md b/README.md index 12dd415..10878c1 100644 --- a/README.md +++ b/README.md @@ -192,13 +192,28 @@ All exceptions extend `Bk203\Vici\Exception\ViciException`: | Exception | Thrown when | | ---------------------------- | ----------- | -| `ConnectionException` | Underlying socket cannot connect, closes mid-stream, or `stream_select()` fails | +| `ConnectionException` | Underlying socket cannot connect, closes mid-stream, or `stream_select()` fails. Exposes `->context` (`ConnectionFailureContext`) and `->getDetailedMessage()` with stream metadata, endpoint, partial I/O progress, and PHP error text | | `TimeoutException` | Read/connect timeout elapses | | `ProtocolException` | Framing or message-encoding violation on the wire | | `CommandUnknownException` | Server replies with `CMD_UNKNOWN` | | `CommandException` | Command completes with `success = no`; exposes `->command` and `->response` | | `EventRegistrationException` | Server replies with `EVENT_UNKNOWN` to `EVENT_REGISTER` / `EVENT_UNREGISTER` | +For long-lived loops, log the detailed form when a connection fails: + +```php +use Bk203\Vici\Exception\ConnectionException; + +try { + $session->version(); +} catch (ConnectionException $e) { + error_log($e->getDetailedMessage()); + // Inspect $e->context?->endpoint for "socket file exists" vs stale fd + // Inspect $e->context?->streamMeta['eof'] and $e->context?->phpError + throw $e; +} +``` + ## Architecture - `Bk203\Vici\Transport\TransportInterface` — 32-bit length-prefixed framing (max 512 KiB), implemented by `UnixSocketTransport`, `TcpSocketTransport`, and `StreamTransport`. diff --git a/src/Exception/ConnectionException.php b/src/Exception/ConnectionException.php index 76b7a83..7450c12 100644 --- a/src/Exception/ConnectionException.php +++ b/src/Exception/ConnectionException.php @@ -9,4 +9,20 @@ */ final class ConnectionException extends ViciException { + public function __construct( + string $message, + public readonly ?ConnectionFailureContext $context = null, + ?\Throwable $previous = null, + ) { + parent::__construct($message, 0, $previous); + } + + public function getDetailedMessage(): string + { + if ($this->context === null || $this->context->format() === '') { + return $this->getMessage(); + } + + return $this->getMessage() . ' | ' . $this->context->format(); + } } diff --git a/src/Exception/ConnectionFailureContext.php b/src/Exception/ConnectionFailureContext.php new file mode 100644 index 0000000..7913f8d --- /dev/null +++ b/src/Exception/ConnectionFailureContext.php @@ -0,0 +1,68 @@ +|null $streamMeta + */ + public function __construct( + public readonly ?string $operation = null, + public readonly ?string $endpoint = null, + public readonly ?array $streamMeta = null, + public readonly ?int $expectedBytes = null, + public readonly ?int $receivedBytes = null, + public readonly ?int $errno = null, + public readonly ?string $phpError = null, + ) { + } + + public function format(): string + { + $parts = []; + + if ($this->operation !== null) { + $parts[] = 'operation=' . $this->operation; + } + if ($this->endpoint !== null) { + $parts[] = 'endpoint=' . $this->endpoint; + } + if ($this->expectedBytes !== null) { + $parts[] = 'expected_bytes=' . $this->expectedBytes; + } + if ($this->receivedBytes !== null) { + $parts[] = 'received_bytes=' . $this->receivedBytes; + } + if ($this->streamMeta !== null) { + if (\array_key_exists('eof', $this->streamMeta)) { + $parts[] = 'stream_eof=' . ($this->streamMeta['eof'] ? '1' : '0'); + } + if (\array_key_exists('timed_out', $this->streamMeta)) { + $parts[] = 'stream_timed_out=' . ($this->streamMeta['timed_out'] ? '1' : '0'); + } + if (\array_key_exists('unread_bytes', $this->streamMeta)) { + $parts[] = 'stream_unread_bytes=' . $this->streamMeta['unread_bytes']; + } + if (\array_key_exists('blocked', $this->streamMeta)) { + $parts[] = 'stream_blocked=' . ($this->streamMeta['blocked'] ? '1' : '0'); + } + if (\array_key_exists('stream_type', $this->streamMeta)) { + $parts[] = 'stream_type=' . $this->streamMeta['stream_type']; + } + } + if ($this->errno !== null) { + $parts[] = 'errno=' . $this->errno; + } + if ($this->phpError !== null) { + $parts[] = 'php_error=' . $this->phpError; + } + + return implode(' ', $parts); + } +} diff --git a/src/Transport/ReconnectingTransport.php b/src/Transport/ReconnectingTransport.php index 3407643..aec780b 100644 --- a/src/Transport/ReconnectingTransport.php +++ b/src/Transport/ReconnectingTransport.php @@ -102,6 +102,14 @@ private function withReconnect(callable $operation): mixed } } + if ($last !== $first) { + throw new ConnectionException( + $last->getMessage(), + $last->context, + $first, + ); + } + throw $last; } } diff --git a/src/Transport/SocketTransport.php b/src/Transport/SocketTransport.php index dc81e11..ea8dee6 100644 --- a/src/Transport/SocketTransport.php +++ b/src/Transport/SocketTransport.php @@ -5,6 +5,7 @@ namespace Bk203\Vici\Transport; use Bk203\Vici\Exception\ConnectionException; +use Bk203\Vici\Exception\ConnectionFailureContext; use Bk203\Vici\Exception\ProtocolException; use Bk203\Vici\Exception\TimeoutException; @@ -72,7 +73,14 @@ final public function hasData(float $timeout = 0.0): bool $ready = @stream_select($read, $write, $except, $sec, $usec); if ($ready === false) { - $this->connectionFailed('stream_select() failed on VICI transport.'); + $this->connectionFailed( + 'stream_select() failed on VICI transport.', + $this->buildFailureContext( + operation: 'select', + stream: $stream, + phpError: $this->capturePhpError(), + ), + ); } return $ready > 0; @@ -101,13 +109,64 @@ protected function invalidate(): void $this->close(); } + protected function getEndpointDescription(): string + { + return ''; + } + /** * @return never */ - protected function connectionFailed(string $message): void + protected function connectionFailed(string $message, ?ConnectionFailureContext $context = null): void { $this->invalidate(); - throw new ConnectionException($message); + throw new ConnectionException($message, $context); + } + + /** + * @param resource|null $stream + */ + protected function buildFailureContext( + string $operation, + $stream = null, + ?int $expectedBytes = null, + ?int $receivedBytes = null, + ?int $errno = null, + ?string $phpError = null, + ): ConnectionFailureContext { + $endpoint = $this->getEndpointDescription(); + + return new ConnectionFailureContext( + operation: $operation, + endpoint: $endpoint !== '' ? $endpoint : null, + streamMeta: \is_resource($stream) ? $this->captureStreamMeta($stream) : null, + expectedBytes: $expectedBytes, + receivedBytes: $receivedBytes, + errno: $errno, + phpError: $phpError, + ); + } + + protected function capturePhpError(): ?string + { + $error = error_get_last(); + if ($error === null) { + return null; + } + + return $error['message']; + } + + /** + * @param resource $stream + * @return array + */ + protected function captureStreamMeta($stream): array + { + /** @var array $meta */ + $meta = stream_get_meta_data($stream); + + return $meta; } private function writeAll(string $data): void @@ -124,9 +183,26 @@ private function writeAll(string $data): void throw new TimeoutException('Timed out writing to VICI socket.'); } if (feof($stream)) { - $this->connectionFailed('VICI socket closed during write.'); + $this->connectionFailed( + 'VICI socket closed during write.', + $this->buildFailureContext( + operation: 'write', + stream: $stream, + expectedBytes: $total - $written, + receivedBytes: $written, + ), + ); } - $this->connectionFailed('Failed to write to VICI socket.'); + $this->connectionFailed( + 'Failed to write to VICI socket.', + $this->buildFailureContext( + operation: 'write', + stream: $stream, + expectedBytes: $total - $written, + receivedBytes: $written, + phpError: $this->capturePhpError(), + ), + ); } $written += $chunk; } @@ -151,7 +227,16 @@ private function readAll(int $length, ?float $timeout): string $usec = (int) round(($remaining - $sec) * 1_000_000); $ready = @stream_select($read, $write, $except, $sec, $usec); if ($ready === false) { - $this->connectionFailed('stream_select() failed on VICI transport.'); + $this->connectionFailed( + 'stream_select() failed on VICI transport.', + $this->buildFailureContext( + operation: 'select', + stream: $stream, + expectedBytes: $length, + receivedBytes: \strlen($buffer), + phpError: $this->capturePhpError(), + ), + ); } if ($ready === 0) { throw new TimeoutException('Timed out reading from VICI socket.'); @@ -162,11 +247,28 @@ private function readAll(int $length, ?float $timeout): string \assert($need > 0); $chunk = @fread($stream, $need); if ($chunk === false) { - $this->connectionFailed('Failed to read from VICI socket.'); + $this->connectionFailed( + 'Failed to read from VICI socket.', + $this->buildFailureContext( + operation: 'read', + stream: $stream, + expectedBytes: $need, + receivedBytes: \strlen($buffer), + phpError: $this->capturePhpError(), + ), + ); } if ($chunk === '') { if (feof($stream)) { - $this->connectionFailed('VICI socket closed during read.'); + $this->connectionFailed( + 'VICI socket closed during read.', + $this->buildFailureContext( + operation: 'read', + stream: $stream, + expectedBytes: $need, + receivedBytes: \strlen($buffer), + ), + ); } $meta = stream_get_meta_data($stream); if ($meta['timed_out']) { @@ -187,7 +289,10 @@ private function readAll(int $length, ?float $timeout): string private function requireStream() { if (!\is_resource($this->stream)) { - $this->connectionFailed('VICI transport is not connected.'); + $this->connectionFailed( + 'VICI transport is not connected.', + $this->buildFailureContext(operation: 'require_stream'), + ); } return $this->stream; } diff --git a/src/Transport/TcpSocketTransport.php b/src/Transport/TcpSocketTransport.php index f59fe25..841f49e 100644 --- a/src/Transport/TcpSocketTransport.php +++ b/src/Transport/TcpSocketTransport.php @@ -5,6 +5,7 @@ namespace Bk203\Vici\Transport; use Bk203\Vici\Exception\ConnectionException; +use Bk203\Vici\Exception\ConnectionFailureContext; /** * Connects to a charon VICI TCP socket (used when charon is configured with a @@ -22,6 +23,11 @@ public function __construct( $this->connect(); } + protected function getEndpointDescription(): string + { + return \sprintf('tcp://%s:%d', $this->host, $this->port); + } + private function connect(): void { $errno = 0; @@ -35,13 +41,21 @@ private function connect(): void ); if ($stream === false) { - throw new ConnectionException(\sprintf( - 'Failed to connect to VICI TCP %s:%d: [%d] %s', - $this->host, - $this->port, - $errno, - $errstr, - )); + throw new ConnectionException( + \sprintf( + 'Failed to connect to VICI TCP %s:%d: [%d] %s', + $this->host, + $this->port, + $errno, + $errstr, + ), + new ConnectionFailureContext( + operation: 'connect', + endpoint: $this->getEndpointDescription(), + errno: $errno, + phpError: $errstr !== '' ? $errstr : null, + ), + ); } stream_set_blocking($stream, true); diff --git a/src/Transport/UnixSocketTransport.php b/src/Transport/UnixSocketTransport.php index 96385df..f04560f 100644 --- a/src/Transport/UnixSocketTransport.php +++ b/src/Transport/UnixSocketTransport.php @@ -5,6 +5,7 @@ namespace Bk203\Vici\Transport; use Bk203\Vici\Exception\ConnectionException; +use Bk203\Vici\Exception\ConnectionFailureContext; /** * Connects to a charon VICI Unix domain socket. @@ -59,18 +60,33 @@ protected function connect(): void ); if ($stream === false) { - throw new ConnectionException(\sprintf( - 'Failed to connect to VICI socket %s: [%d] %s', - $this->path, - $errno, - $errstr, - )); + throw new ConnectionException( + \sprintf( + 'Failed to connect to VICI socket %s: [%d] %s', + $this->path, + $errno, + $errstr, + ), + new ConnectionFailureContext( + operation: 'connect', + endpoint: $this->getEndpointDescription(), + errno: $errno, + phpError: $errstr !== '' ? $errstr : $this->capturePhpError(), + ), + ); } $this->applyStreamOptions($stream); $this->stream = $stream; } + protected function getEndpointDescription(): string + { + $state = file_exists($this->path) ? 'socket file exists' : 'socket file missing'; + + return \sprintf('unix://%s (%s)', $this->path, $state); + } + /** * @param resource $stream */ diff --git a/tests/Unit/Exception/ConnectionExceptionTest.php b/tests/Unit/Exception/ConnectionExceptionTest.php new file mode 100644 index 0000000..b158442 --- /dev/null +++ b/tests/Unit/Exception/ConnectionExceptionTest.php @@ -0,0 +1,46 @@ +getMessage()); + self::assertNull($exception->context); + self::assertSame('Failed to read from VICI socket.', $exception->getDetailedMessage()); + } + + public function testDetailedMessageIncludesDiagnostics(): void + { + $exception = new ConnectionException( + 'Failed to read from VICI socket.', + new ConnectionFailureContext( + operation: 'read', + endpoint: 'unix:///var/run/charon.vici (socket file exists)', + streamMeta: ['eof' => false, 'timed_out' => false], + expectedBytes: 4, + receivedBytes: 0, + phpError: 'fread(): Broken pipe', + ), + ); + + self::assertSame('Failed to read from VICI socket.', $exception->getMessage()); + self::assertStringContainsString('operation=read', $exception->getDetailedMessage()); + self::assertStringContainsString('endpoint=unix:///var/run/charon.vici (socket file exists)', $exception->getDetailedMessage()); + self::assertStringContainsString('expected_bytes=4', $exception->getDetailedMessage()); + self::assertStringContainsString('received_bytes=0', $exception->getDetailedMessage()); + self::assertStringContainsString('php_error=fread(): Broken pipe', $exception->getDetailedMessage()); + } +} diff --git a/tests/Unit/Transport/StreamTransportTest.php b/tests/Unit/Transport/StreamTransportTest.php index 3dd2e1b..28b115e 100644 --- a/tests/Unit/Transport/StreamTransportTest.php +++ b/tests/Unit/Transport/StreamTransportTest.php @@ -147,9 +147,12 @@ public function testConnectionFailureInvalidatesStream(): void try { $transport->receive(0.5); self::fail('Expected ConnectionException.'); - } catch (\Bk203\Vici\Exception\ConnectionException) { + } catch (\Bk203\Vici\Exception\ConnectionException $e) { self::assertFalse($transport->isConnected()); self::assertNull($transport->getStream()); + self::assertNotNull($e->context); + self::assertSame('read', $e->context->operation); + self::assertTrue($e->context->streamMeta['eof'] ?? false); } } diff --git a/tests/Unit/Transport/UnixSocketTransportTest.php b/tests/Unit/Transport/UnixSocketTransportTest.php index 54c48ec..02183db 100644 --- a/tests/Unit/Transport/UnixSocketTransportTest.php +++ b/tests/Unit/Transport/UnixSocketTransportTest.php @@ -72,8 +72,14 @@ public function testReconnectThrowsWhenSocketFileMissing(): void $this->closeListener(); - $this->expectException(ConnectionException::class); - $transport->reconnect(); + try { + $transport->reconnect(); + self::fail('Expected ConnectionException.'); + } catch (ConnectionException $e) { + self::assertNotNull($e->context); + self::assertSame('connect', $e->context->operation); + self::assertStringContainsString('socket file missing', (string) $e->context->endpoint); + } } private function startListener(): void From 096ffedec8d7e4ccfe58ad9165958f16d2626a1c Mon Sep 17 00:00:00 2001 From: Mathijs Smit Date: Mon, 8 Jun 2026 11:24:19 +0200 Subject: [PATCH 3/3] Document VICI socket lock from stacked short-timeout initiate calls. --- README.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/README.md b/README.md index 10878c1..8c2c5a9 100644 --- a/README.md +++ b/README.md @@ -214,6 +214,20 @@ try { } ``` +## Known issues + +### Stacked `initiate` commands with short client timeouts can lock the VICI socket + +`initiate` can run for a long time on the charon side while IKE negotiation retries play out. That sequence has its own timeout (the `timeout` field in the command message, in milliseconds), independent of the transport read timeout on your `Session`. + +If the transport read timeout is shorter than that whole charon-side sequence, the client raises `TimeoutException` before charon sends `CMD_RESPONSE`. Catching that exception and immediately sending another `initiate` — or any other command — on the same socket leaves charon still busy with the earlier command. Repeating this pattern desynchronizes the VICI control channel over time: the socket stops responding, every caller cascades into `TimeoutException`, and the lock affects **all** clients on that socket, including `swanctl` and other tools. + +**Mitigations** + +- Set transport read timeouts well above the `initiate` message `timeout`, or omit a read timeout for long-running control commands. +- Do not retry `initiate` on the same `Session` after a client-side timeout; treat a wedged socket as requiring a new connection or a charon restart. +- Keep at most one in-flight `initiate` per connection; wait for charon to finish (success, failure, or its own timeout) before trying again. + ## Architecture - `Bk203\Vici\Transport\TransportInterface` — 32-bit length-prefixed framing (max 512 KiB), implemented by `UnixSocketTransport`, `TcpSocketTransport`, and `StreamTransport`.