Multi protocol proxy

[abnegate]

Summary by CodeRabbit

• New Features

  • Multi-service Docker setup and runnable proxy examples; Resolver-based dynamic backend routing; read/write split for DB proxies with transaction pinning; TLS/mTLS termination for TCP.

• Documentation

  • Major README overhaul with Resolver/benchmark guidance; new benchmarks suite and runner docs; removed legacy PERFORMANCE.md.

• Tests

  • Extensive unit, integration, and performance test suites added.

• Chores

  • CI workflows (lint, static analysis, tests); environment example; PHP bumped to 8.4 and Swoole to 6.0.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a89cfb30-1bc5-4240-adc6-ec80f1313db2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

The PR performs a large-scale refactor and feature expansion: renames package/PSR-4 namespaces; replaces the ConnectionManager-based resolution with a new Resolver and Adapter abstraction; adds Protocol enum and Resolver result/exception types; implements new Swoole and Swoole Coroutine HTTP/TCP/SMTP servers and TCP TLS support (TlsContext/TLS); introduces TCP adapters with read/write split and an HTTP Adapter; removes legacy ConnectionManager, Resource, and ResourceStatus implementations; adds extensive benchmarking scripts, Docker and CI workflows, linting/format configs, many example/proxy entrypoints, and a broad suite of unit, integration, and performance tests. Several files were added, modified, or deleted across src/, proxies/, benchmarks/, tests/, and CI/docker configs.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch dev

[coderabbitai]

Actionable comments posted: 9

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

README.md (1)

1-3: ⚠️ Potential issue | 🟡 Minor

Title inconsistent with package rename.

The README title says "Appwrite Protocol Proxy" but composer.json renames the package to utopia-php/protocol-proxy. Consider updating the title to match the new branding.

📝 Suggested title update

-# Appwrite Protocol Proxy
+# Utopia Protocol Proxy

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 1 - 3, Update the README title string "Appwrite
Protocol Proxy" to match the package rename in composer.json (package name
"utopia-php/protocol-proxy"); locate the README header and replace the title
with the new branding (e.g., "Utopia Protocol Proxy" or
"utopia-php/protocol-proxy") so README and composer.json use consistent package
naming.

src/Server/SMTP/Swoole.php (3)

193-205: ⚠️ Potential issue | 🟠 Major

Close the previous backend before replacing it on a repeated EHLO/HELO.

If the client sends EHLO/HELO again, $conn['backend'] is overwritten and only the most recent client is closed in onClose(). The earlier backend sockets are leaked for the lifetime of the worker.

💡 Close the old backend before reconnecting

         if (preg_match('/^(EHLO|HELO)\s+([^\s]+)/i', $data, $matches)) {
             $domain = $matches[2];
             $conn['domain'] = $domain;
+
+            if ($conn['backend'] instanceof Client) {
+                $conn['backend']->close();
+                $conn['backend'] = null;
+            }
 
             // Route to backend using adapter
             $result = $this->adapter->route($domain);

Also applies to: 269-274

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/SMTP/Swoole.php` around lines 193 - 205, When assigning a new
backend client in handleHelo (and the similar block around the 269-274 region),
ensure you close any existing backend socket stored in $conn['backend'] before
overwriting it to prevent leaking connections: check if $conn['backend'] exists
and is a valid/active client, call the appropriate close/disconnect method on
that object (the same one used in onClose()), then replace $conn['backend'] with
the newly returned value from connectToBackend; do the same cleanup in the other
repeated-EHLO/HELO handling path so old backend sockets are always closed before
being replaced.

---

157-179: ⚠️ Potential issue | 🔴 Critical

Handle SMTP DATA mode separately from command parsing.

$conn['state'] is initialized but never used. After the backend replies 354, the next bytes are message content until \r\n.\r\n, not SMTP verbs. Right now headers/body lines fall into the default branch and get 500 Unknown command, so a normal mail submission cannot complete.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/SMTP/Swoole.php` around lines 157 - 179, The SMTP DATA body is
being parsed as commands because $conn['state'] isn't used; modify the
receive/parse block in Swoole.php to check $conn['state'] before extracting a
4-char verb and, if $conn['state']==='DATA', treat incoming bytes as message
content (accumulate and forward to backend) until the terminator "\r\n.\r\n" is
seen, then clear the state and handle the end-of-data reply; update
forwardToBackend (or create a new handleData method) to set
$conn['state']='DATA' when the backend responds with 354 and to accept raw data
chunks thereafter so headers/body lines are not run through the default command
switch, finally reset $conn['state'] after the message is completed.

---

220-238: ⚠️ Potential issue | 🟠 Major

Do not spawn independent recv() coroutines per command.

A single recv(8192) is not enough for multiline replies like EHLO, and launching a detached read coroutine per command lets multiple coroutines read from the same backend socket. That can strand reply bytes or associate them with the wrong frontend command.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/SMTP/Swoole.php` around lines 220 - 238, forwardToBackend
currently sends data then creates a detached Coroutine::create that calls
backendClient->recv(8192), which can cause multiple coroutines to read the same
backend socket and drop/misassociate multiline replies (e.g., EHLO). Replace
this by removing the per-call Coroutine::create/recv from forwardToBackend;
instead implement/ensure a single persistent reader coroutine for each backend
connection (spawned when backendClient is attached) that continuously reads
(loop recv until socket closed), buffers full SMTP replies and then dispatches
them to the correct frontend fd(s) (use a pending-response queue or mapping
keyed by backend connection stored on the connection object). Keep
forwardToBackend to only backendClient->send($data) and register the frontend fd
in the backend's pending-request structure so the shared reader can relay
complete multiline responses back via $server->send($fd, $response).

🟠 Major comments (18)

.dockerignore-1-8 (1)

1-8: ⚠️ Potential issue | 🟠 Major

Keep local secrets and volume data out of the Docker build context.

.env and /docker-volumes/ are now ignored by Git, but they are still sent to docker build because this file does not exclude them. That is an avoidable secret-leak risk and can massively inflate build context size on developer machines.

🛡️ Proposed fix

 .git
 .gitignore
 .idea
 vendor
 *.md
+.env
+/docker-volumes/
 .dockerignore
 Dockerfile
 docker-compose.yml

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.dockerignore around lines 1 - 8, The .dockerignore currently omits .env and
docker volume directories, so sensitive local secrets and large /docker-volumes/
data are being sent to docker build; update the .dockerignore to explicitly
exclude .env (and any .env.* patterns you use) and the docker-volumes directory
(e.g., docker-volumes/ or /docker-volumes/) so those files are not included in
the build context; locate the .dockerignore file and add entries for ".env",
".env.*" (optional) and "docker-volumes/" to fix the leak and reduce build
context size.

.github/workflows/lint.yml-18-25 (1)

18-25: ⚠️ Potential issue | 🟠 Major

Install the required PHP extensions before Composer runs.

The lint job installs dependencies before running Pint, but does not provision the native extensions required by the project. The composer.json declares ext-swoole (>=6.0) and ext-redis as platform requirements. Without these extensions, composer install will fail and the job will never reach Pint.

🔧 Proposed fix

       - name: Setup PHP
         uses: shivammathur/setup-php@v2
         with:
           php-version: '8.4'
+          extensions: swoole, redis
           tools: composer:v2

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/lint.yml around lines 18 - 25, The workflow installs PHP
then runs composer before provisioning required native extensions, causing
composer install to fail because ext-swoole and ext-redis are declared in
composer.json; update the "Setup PHP" step (the one using
shivammathur/setup-php@v2) to install the required extensions by adding the
extensions/pecl entries for swoole (>=6.0) and redis (e.g., extensions: redis,
swoole or pecl: swoole) so the environment satisfies the platform requirements
before the "Install dependencies" step runs.

benchmarks/compare-http-servers.sh-8-10 (1)

8-10: ⚠️ Potential issue | 🟠 Major

COMPARE_HOST/COMPARE_PORT never reach the proxy process.

These knobs currently only change the health check and benchmarks/http.php. start_proxy() still launches proxies/http.php without a matching listen-address override, so any non-default host/port points the probe and benchmark at a different endpoint than the proxy you just started.

Also applies to: 52-77, 83-86

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/compare-http-servers.sh` around lines 8 - 10, The env vars
COMPARE_HOST/COMPARE_PORT (host/port) are only used for the probe and benchmark
but not passed to the proxy; update start_proxy() so the proxy process is
launched with the same listen address by exporting or passing
COMPARE_HOST/COMPARE_PORT into the PHP proxy invocation (proxies/http.php) or by
adding CLI args to start_proxy() to forward host and port, then use those inside
proxies/http.php to bind the server; ensure every proxy launch (all
start_proxy() calls) is updated so the started proxy actually listens on
host/port used by the health check and benchmark.

benchmarks/http.php-52-67 (1)

52-67: ⚠️ Potential issue | 🟠 Major

Validate BENCH_SAMPLE_EVERY before using it.

Right now BENCH_SAMPLE_EVERY=0 will throw at Line 154, and values larger than $requests can leave $samples empty so the latency goals report 0.00ms and pass with no real percentile data.

🩹 Proposed fix

     if ($concurrent < 1) {
         echo "Invalid concurrency.\n";

         return;
     }
+    if ($sampleEvery < 1) {
+        echo "Invalid sample interval.\n";
+
+        return;
+    }
+    if ($sampleEvery > $requests) {
+        $sampleEvery = $requests;
+    }

Also applies to: 154-155, 218-250

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/http.php` around lines 52 - 67, The code doesn't validate
BENCH_SAMPLE_EVERY (and indirectly BENCH_SAMPLE_TARGET) so a zero or
out-of-range value can cause a divide-by-zero or produce an empty $samples set;
fix by sanitizing and clamping the sampler variables: ensure $sampleTarget =
max(1, $envInt('BENCH_SAMPLE_TARGET', 200000)), then compute $sampleEvery =
max(1, min($sampleEvery, $requests)) (or derive $sampleEvery = max(1, (int)
ceil($requests / max(1, $sampleTarget)))) and when building $samples ensure at
least one sample (e.g. compute sample count with max(1, (int) ceil($requests /
$sampleEvery))). Update references to $sampleTarget, $sampleEvery and $samples
where used (including the code paths around $requests checks and later uses at
the percentile/latency reporting) so zero or too-large values are clamped and
never produce division by zero or empty sample lists.

benchmarks/bootstrap-droplet.sh-116-126 (1)

116-126: ⚠️ Potential issue | 🟠 Major

Don't delete the workdir before deciding whether to reuse it.

Line 117 runs before the "already in the repo" check, so rerunning this script from /tmp/protocol-proxy-bench can wipe the checkout it is about to use.

🧯 Proposed fix

 WORKDIR="/tmp/protocol-proxy-bench"
-rm -rf "$WORKDIR"

 if [ -f "composer.json" ] && grep -q "protocol-proxy" composer.json 2>/dev/null; then
     # Already in the repo
     WORKDIR="$(pwd)"
     echo "  - Using current directory"
 else
+    rm -rf "$WORKDIR"
     git clone --depth 1 -b dev https://github.com/utopia-php/protocol-proxy.git "$WORKDIR" 2>/dev/null
     cd "$WORKDIR"
     echo "  - Cloned to $WORKDIR"
 fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/bootstrap-droplet.sh` around lines 116 - 126, The script currently
does rm -rf "$WORKDIR" before checking whether we are already running inside
that repo; move or conditionally perform the removal so you don't wipe an
existing checkout: first set WORKDIR="/tmp/protocol-proxy-bench" and then test
if [ -f "composer.json" ] && grep -q "protocol-proxy" composer.json to detect
reuse (setting WORKDIR="$(pwd)" when true) and only run rm -rf "$WORKDIR" in the
else branch before git clone (or skip removal entirely when WORKDIR was set to
the current directory); update the logic around WORKDIR and the clone branch
that contains the git clone and cd so removal happens only when cloning a fresh
copy.

benchmarks/bootstrap-droplet.sh-47-48 (1)

47-48: ⚠️ Potential issue | 🟠 Major

Bootstrap pins PHP 8.3, but project requires PHP 8.4.

composer.json specifies PHP >=8.4 and 8.4, but the bootstrap script hard-codes PHP 8.3 packages at lines 47-48, 70, and 99. On a fresh host, this will install the wrong PHP version, breaking composer install and potentially loading incorrect package versions.

Affected lines

47-48:  apt-get install -y -qq php8.3-cli php8.3-dev php8.3-xml php8.3-curl \
            php8.3-mbstring php8.3-zip php-pear git unzip curl > /dev/null 2>&1
70:     apt-get install -y -qq php8.3-swoole > /dev/null 2>&1 || {
99:     echo "  apt-get install php8.3-swoole"

Update package versions to PHP 8.4.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/bootstrap-droplet.sh` around lines 47 - 48, The bootstrap script
pins PHP packages to 8.3; update all occurrences of the php8.3 package names to
php8.4 so the installed PHP matches composer.json; specifically replace package
strings like "php8.3-cli", "php8.3-dev", "php8.3-xml", "php8.3-curl",
"php8.3-mbstring", "php8.3-zip", and "php8.3-swoole" with their "php8.4-*"
equivalents and update the echo that references "apt-get install php8.3-swoole"
to "php8.4-swoole" so the install commands and error messages reflect PHP 8.4.

benchmarks/bootstrap-droplet.sh-13-14 (1)

13-14: ⚠️ Potential issue | 🟠 Major

The automated benchmark path skips the backend startup.

The usage example above starts benchmarks/tcp-backend.php before invoking benchmarks/tcp.php, but the actual run phase never does that. On a fresh droplet these benchmark commands are now running without the listener this script documents as required.

Also applies to: 165-183

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/bootstrap-droplet.sh` around lines 13 - 14, The example run in
benchmarks/bootstrap-droplet.sh documents starting benchmarks/tcp-backend.php
before running benchmarks/tcp.php but the actual automated run omits starting
the backend; update the script so any invocation that runs benchmarks/tcp.php
first launches benchmarks/tcp-backend.php in the background (e.g., with proper
stdout/stderr redirection), waits for it to be ready (sleep or a health-check
loop), and ensures it is killed/cleaned up after the benchmark completes;
specifically modify the sections that reference benchmarks/tcp-backend.php and
benchmarks/tcp.php (also the repeated block around lines 165-183) to start the
backend first, verify it started, then run the benchmark client.

composer.json-12-17 (1)

12-17: ⚠️ Potential issue | 🟠 Major

Path repository will cause installation failure for external consumers.

The path repository pointing to ../query will cause Composer to abort during installation if the local directory does not exist. Since utopia-php/query is already published on Packagist, the path repository is not necessary and will break installations for users without this monorepo structure. Remove the path repository from composer.json, or move it to a development-only configuration that is not committed to version control.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@composer.json` around lines 12 - 17, The composer.json contains a
"repositories" entry with a path repository ("type": "path", "url": "../query")
that will abort installation for external consumers if that local directory
doesn't exist; remove this path repository from composer.json (or move it into a
non-committed dev-only composer config) so Packagist's published package
(utopia-php/query) is used instead; update or delete the "repositories" array
entry referencing "../query" and commit the cleaned composer.json.

composer.json-25-27 (1)

25-27: ⚠️ Potential issue | 🟠 Major

Wildcard versions for dev dependencies may cause CI instability.

Using * for phpstan/phpstan and laravel/pint could pull incompatible major versions, causing unexpected CI failures. Additionally, PHPUnit 12.* is outdated—PHPUnit 13 is the current stable major version.

📌 Pin to major versions

     "require-dev": {
-        "phpunit/phpunit": "12.*",
-        "phpstan/phpstan": "*",
-        "laravel/pint": "*"
+        "phpunit/phpunit": "^13.0",
+        "phpstan/phpstan": "^2.0",
+        "laravel/pint": "^1.0"
     },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@composer.json` around lines 25 - 27, The dev-dependency versions are too
permissive and PHPUnit is pinned to an older major; update composer.json so
"phpunit/phpunit" is bumped from "12.*" to a current major constraint like
"^13.0" and replace the wildcard "*" entries for "phpstan/phpstan" and
"laravel/pint" with explicit major constraints (e.g., "^1" or the correct
current major for your project) to avoid pulling breaking changes; edit the
package entries for phpstan/phpstan and laravel/pint accordingly and run
composer update / composer.lock regeneration to lock compatible versions.

src/Server/TCP/SwooleCoroutine.php-183-225 (1)

183-225: ⚠️ Potential issue | 🟠 Major

Potential race condition between forwarding coroutines.

When the backend→client coroutine (lines 183-195) exits and closes $clientSocket, the main client→backend loop (lines 208-216) may still be waiting on recv(). While this will eventually return false, there's a window where:

1. B2C coroutine closes the backend socket externally (not shown but backend may close)
2. B2C closes client socket (line 194)
3. Main loop exits and tries to close $backendSocket (line 219) which may already be in a broken state

Consider using a shared cancellation flag or Coroutine::cancel() for cleaner shutdown coordination.

Suggested approach using atomic flag

+        $closed = new \Swoole\Atomic(0);
+
         // Start backend -> client forwarding in separate coroutine
-        Coroutine::create(function () use ($clientSocket, $backendSocket, $bufferSize, $adapter, $databaseId): void {
+        Coroutine::create(function () use ($clientSocket, $backendSocket, $bufferSize, $adapter, $databaseId, $closed): void {
             while (true) {
                 $data = $backendSocket->recv($bufferSize);
                 if ($data === false || $data === '') {
                     break;
                 }
                 $adapter->recordBytes($databaseId, 0, \strlen($data));
                 if ($clientSocket->sendAll($data) === false) {
                     break;
                 }
             }
-            $clientSocket->close();
+            $closed->set(1);
         });
 
         // ... main loop ...
 
-        $adapter->notifyClose($databaseId);
-        $backendSocket->close();
+        if ($closed->cmpset(0, 1)) {
+            // We initiated close
+        }
+        $adapter->notifyClose($databaseId);
+        @$backendSocket->close();
+        @$clientSocket->close();
         $adapter->closeBackendConnection($databaseId, $clientId);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/TCP/SwooleCoroutine.php` around lines 183 - 225, The
backend->client Coroutine spawned with Coroutine::create can close $clientSocket
while the main client->backend loop is blocked on $clientSocket->recv(), causing
a race and potential double/invalid close of $backendSocket; fix by introducing
a shared cancellation flag (e.g., $cancelled) or use Coroutine::cancel() on the
spawned coroutine and check that flag in both loops: set the flag before closing
sockets in the B2C coroutine, have the main loop break immediately if the flag
is set (and skip closing $backendSocket if already closed), and guard any
close() calls on $backendSocket/$clientSocket with the flag or an isClosed check
and ensure adapter->notifyClose($databaseId) /
adapter->closeBackendConnection($databaseId, $clientId) are called exactly once.

tests/QueryParserTest.php-620-654 (1)

620-654: ⚠️ Potential issue | 🟠 Major

Keep the microbenchmarks out of the default PHPUnit suite.

These 100k-iteration tests assert sub-microsecond timings, so they'll flap on slower CI runners and add avoidable time to every regular test run. Move them behind an opt-in env flag or a dedicated benchmark group instead of making them mandatory unit tests.

💡 One simple opt-in pattern

 public function test_parse_performance(): void
 {
+    if (getenv('RUN_PERFORMANCE_TESTS') !== '1') {
+        self::markTestSkipped('Performance tests are opt-in');
+    }
+
     $pgData = $this->buildPgQuery('SELECT * FROM users WHERE id = 1');
     $mysqlData = $this->buildMySQLQuery('SELECT * FROM users WHERE id = 1');

 public function test_classify_sql_performance(): void
 {
+    if (getenv('RUN_PERFORMANCE_TESTS') !== '1') {
+        self::markTestSkipped('Performance tests are opt-in');
+    }
+
     $queries = [
         'SELECT * FROM users WHERE id = 1',

Also applies to: 656-681

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/QueryParserTest.php` around lines 620 - 654, The performance
microbenchmark in test_parse_performance (and the similar test at lines 656-681)
runs 100_000 iterations calling $this->pgParser->parse(...) and
$this->mysqlParser->parse(...), asserting sub-microsecond timings which flaps CI
and slows regular test runs; update these tests to be opt-in by either skipping
when an env var is not set (e.g. if (!getenv('RUN_BENCHMARKS'))
$this->markTestSkipped('Benchmarks disabled')) or by adding a PHPUnit group
annotation (e.g. `@group` benchmark) and document running them separately, leaving
the loop/measurement code and assertions unchanged so the benchmarks still
execute only when opted into.

benchmarks/compare-tcp-servers.sh-68-71 (1)

68-71: ⚠️ Potential issue | 🟠 Major

Track child PIDs instead of killing by command pattern.

These pkill -f calls will tear down any matching proxy/backend on the host, not just the ones this script spawned. That's especially risky when COMPARE_BACKEND_START=false and another benchmark backend is already running.

💡 Safer cleanup pattern

+proxy_pids=()
+backend_pid=
+
 cleanup() {
-  pkill -f "proxies/tcp.php" >/dev/null 2>&1 || true
-  pkill -f "php benchmarks/tcp-backend.php" >/dev/null 2>&1 || true
+  for pid in "${proxy_pids[@]:-}"; do
+    kill "$pid" >/dev/null 2>&1 || true
+  done
+  if [ -n "${backend_pid:-}" ]; then
+    kill "$backend_pid" >/dev/null 2>&1 || true
+  fi
 }

   nohup env BACKEND_HOST="${backend_host}" BACKEND_PORT="${backend_port}" \
     php benchmarks/tcp-backend.php > /tmp/tcp-backend.log 2>&1 &
+  backend_pid=$!

   nohup env \
     TCP_SERVER_IMPL="${impl}" \
     TCP_BACKEND_ENDPOINT="${backend_host}:${backend_port}" \
     ...
     php -d memory_limit=1G proxies/tcp.php > /tmp/tcp-proxy.log 2>&1 &
+  proxy_pids+=($!)

Also applies to: 79-79, 100-100

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/compare-tcp-servers.sh` around lines 68 - 71, The cleanup()
function currently uses pkill -f with patterns ("proxies/tcp.php" and "php
benchmarks/tcp-backend.php") which can kill unrelated processes; instead, when
spawning the proxy and backend capture and store their PIDs (e.g., assign $! to
variables or write to a temp pidfile from the start logic that launches the
processes referenced by proxies/tcp.php and php benchmarks/tcp-backend.php),
update cleanup() to kill only those stored PIDs (kill <pid> || true) and remove
the pidfile, and ensure a trap EXIT invokes cleanup so processes started by this
script are the only ones torn down.

benchmarks/compare-tcp-servers.sh-162-169 (1)

162-169: ⚠️ Potential issue | 🟠 Major

Fail fast when the backend never becomes reachable.

If COMPARE_BACKEND_START=false and nothing is listening on ${backend_host}:${backend_port}, this loop falls through and the script still prints benchmark rows against a dead target. Exit once the retries are exhausted.

💡 Abort on exhausted retries

+backend_ready=false
 for _ in {1..10}; do
   if php -r '$s=@stream_socket_client("tcp://'"${backend_host}:${backend_port}"'", $errno, $errstr, 0.5); if ($s) { fclose($s); exit(0);} exit(1);' >/dev/null 2>&1; then
+    backend_ready=true
     break
   fi
   sleep 0.5
 done
+
+if [ "$backend_ready" != "true" ]; then
+  echo "Backend is not reachable at ${backend_host}:${backend_port}" >&2
+  exit 1
+fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/compare-tcp-servers.sh` around lines 162 - 169, The readiness loop
started by start_backend may exhaust retries and still continue benchmarking
against a dead target; modify the loop around the PHP stream_socket_client probe
(which checks backend_host and backend_port) to detect when all retries fail and
then exit non‑zero with an error message; e.g., set a success flag or test the
probe result after the for {1..10} loop and if unreachable (and
COMPARE_BACKEND_START is false or however start_backend is gated) call echo to
stderr and exit 1 so the script fails fast instead of running benchmarks against
a nonresponsive backend.

tests/Performance/PerformanceTest.php-791-807 (1)

791-807: ⚠️ Potential issue | 🟠 Major

readResponse() is truncating benchmark traffic.

A single fread(..., 65536) is not a full PostgreSQL response. Large-payload tests and repeated-query loops leave unread bytes on the socket, so later iterations measure leftovers instead of the next request/response pair.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Performance/PerformanceTest.php` around lines 791 - 807, The current
readResponse function reads only a single fread(…, 65536) which leaves unread
bytes on the socket for large Postgres responses; update readResponse to loop
and accumulate chunks until the full response is consumed: keep using
stream_set_timeout and repeatedly call fread inside a loop, appending each
non-empty chunk to a buffer; on an empty chunk call stream_get_meta_data($sock)
and if meta['timed_out'] return false, otherwise check meta['unread_bytes'] (or
feof) and only break the loop when unread_bytes == 0 (or EOF) to ensure the
whole response is drained before returning the buffer; reference readResponse,
stream_set_timeout, fread, and stream_get_meta_data to locate where to implement
the loop and accumulation.

src/Adapter.php-244-277 (1)

244-277: ⚠️ Potential issue | 🟠 Major

The SSRF guard can be bypassed via DNS rebinding.

validateEndpoint() checks one DNS result, but the actual connection later re-resolves the original hostname. A rebinding hostname can validate against a public address and then dial private space at connect time. If this check is meant to be authoritative, the code needs to validate all resolved addresses and pin the address it actually connects to.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Adapter.php` around lines 244 - 277, validateEndpoint currently uses
gethostbyname to check a single DNS result, which allows DNS rebinding; instead
resolve all A and AAAA records (e.g. via dns_get_record or equivalent) and
validate every returned IP against the private/reserved ranges in the
validateEndpoint function, and then ensure the code pins and uses one of those
validated IPs for the actual connection (i.e. return or store the resolved IP
chosen by validateEndpoint and modify the later connection code to use that
returned IP rather than re-resolving the hostname). Ensure both IPv4 and IPv6
results are checked and the same private/reserved checks (ip2long ranges and
IPv6 prefix checks) are applied to every resolved address.

src/Server/TCP/Config.php-12-39 (1)

12-39: ⚠️ Potential issue | 🟠 Major

Fail fast on an empty or invalid port list.

src/Server/TCP/Swoole.php immediately uses $this->config->ports[0]. If ports is empty, startup dies with a runtime error instead of a clear config failure, and out-of-range ports are only caught much later by bind/connect code.

Patch sketch

     ) {
+        if ($ports === []) {
+            throw new \InvalidArgumentException('At least one TCP port is required.');
+        }
+
+        foreach ($ports as $port) {
+            if ($port < 1 || $port > 65535) {
+                throw new \InvalidArgumentException("Invalid TCP port: {$port}");
+            }
+        }
+
         $this->reactorNum = $reactorNum ?? swoole_cpu_num() * 2;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/TCP/Config.php` around lines 12 - 39, The TCP Config constructor
currently accepts any array for $ports, causing downstream crashes when
Swoole.php accesses $this->config->ports[0]; update the Config::__construct to
validate $ports immediately: ensure the array is non-empty, each entry is an
integer within 1..65535 (or castable to int if desired), and throw a clear
InvalidArgumentException (or custom ConfigException) with a descriptive message
if validation fails; reference the Config class and its __construct and the
$ports property so the check is placed there to fail fast on invalid or empty
port lists.

tests/Integration/EdgeIntegrationTest.php-442-446 (1)

442-446: ⚠️ Potential issue | 🟠 Major

This validates cache expiry, not cache invalidation.

purge() in the mock is effectively a no-op, and the test then waits longer than the routing cache TTL before routing again. That means the assertion still passes if invalidation is broken, because natural expiry forces the re-resolve anyway. Re-route before the TTL elapses, or make invalidation observable from the adapter side.

Also applies to: 760-763

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Integration/EdgeIntegrationTest.php` around lines 442 - 446, The test
currently validates cache expiry, not invalidation: replace the sleep-based
approach by exercising re-route immediately after calling
$resolver->purge('invaldb') and asserting that invalidation occurred;
specifically, update the EdgeIntegrationTest to (a) call the routing operation
(the same method that triggers resolution) immediately after resolver->purge,
(b) either make the resolver mock's purge() mutate observable state in the
adapter/mock (so the adapter sees a cleared cache) or add an expectation on the
adapter mock that purge()/invalidate was invoked, and (c) remove the sleep(2) so
the test fails if purge() is a no-op. Reference the resolver->purge call and the
routing invocation in the test and ensure assertions verify that the adapter
observed the invalidation rather than waiting for TTL expiry.

tests/Integration/EdgeIntegrationTest.php-492-525 (1)

492-525: ⚠️ Potential issue | 🟠 Major

These "concurrent" cases never execute concurrently.

Both methods only perform serial route() calls in a single execution context. They verify multi-database routing, but they do not exercise concurrent access to the adapter or resolver state. Please either run them in parallel or rename them so the coverage claim matches reality.

Also applies to: 530-566

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Integration/EdgeIntegrationTest.php` around lines 492 - 525, The test
test_concurrent_resolution_of_multiple_databases() (and the similar 530-566
case) does not perform parallel execution — it calls
EdgeMockResolver->registerDatabase and TCPAdapter->route() serially — so either
make it actually concurrent or change the name; simplest fix: rename the test(s)
and any references from "concurrent" to "serial" or "multiple" (e.g.,
test_resolution_of_multiple_databases) and update assertions/comments
accordingly, keeping the same use of EdgeMockResolver, TCPAdapter, route(), and
getStats() so the behavior and assertions remain unchanged.

🟡 Minor comments (9)

docker-compose.yml-70-71 (1)

70-71: ⚠️ Potential issue | 🟡 Minor

Potential port conflict with MariaDB.

Both mariadb (line 13) and tcp-proxy (line 71) map to host port 3306. When running the base compose file without overrides, this will cause a port binding conflict. Consider using a different default port for TCP_MYSQL_PORT or documenting that these services shouldn't run simultaneously on the same host.

-      - "${TCP_MYSQL_PORT:-3306}:3306"
+      - "${TCP_MYSQL_PORT:-33060}:3306"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docker-compose.yml` around lines 70 - 71, The docker-compose mapping for
tcp-proxy uses the same host port as the mariadb service, causing a port bind
conflict; update the default mapping for the tcp-proxy MYSQL port (the
TCP_MYSQL_PORT environment variable referenced in the tcp-proxy service) to a
different host port (e.g., change default from 3306 to another unused port) or
add a note in the compose file/service description that mariadb and tcp-proxy
must not run simultaneously; modify the tcp-proxy service's port entry that uses
"${TCP_MYSQL_PORT:-3306}:3306" (and/or the TCP_POSTGRES_PORT if applicable) to a
new default or document the conflict so users can override TCP_MYSQL_PORT when
needed.

benchmarks/wrk2.sh-4-7 (1)

4-7: ⚠️ Potential issue | 🟡 Minor

Error message references unused WRK2_BIN variable.

The error suggests setting WRK2_BIN, but the script never uses this variable - it just calls wrk2 directly.

 if ! command -v wrk2 >/dev/null 2>&1; then
-  echo "wrk2 not found. Install wrk2 or set WRK2_BIN." >&2
+  echo "wrk2 not found. Please install wrk2 first." >&2
   exit 1
 fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/wrk2.sh` around lines 4 - 7, The error message incorrectly
references WRK2_BIN even though the script always calls wrk2; update the script
to honor WRK2_BIN by using a resolved binary variable (e.g.,
WRK2_CMD="${WRK2_BIN:-wrk2}") and use WRK2_CMD in the check and later
invocations (replace direct calls to wrk2 and the command -v check with
"${WRK2_CMD}"/command -v "${WRK2_CMD}" ), or if you don't want a configurable
path, remove the WRK2_BIN mention from the echo and keep the unconditional wrk2
usage; adjust the block around the existing check (the if ! command -v wrk2 ...
fi) accordingly.

benchmarks/test-bootstrap.sh-16-24 (1)

16-24: ⚠️ Potential issue | 🟡 Minor

dnf detected but never used - script is apt-only.

The script detects dnf availability but all subsequent package installation commands use apt-get. This will fail on Fedora/RHEL systems.

🐛 Suggested fix: either implement dnf support or remove the misleading check

 echo "[1/6] Testing package manager..."
-if command -v apt-get > /dev/null 2>&1; then
-    echo "  OK: apt-get available"
-    apt-get update -qq
-elif command -v dnf > /dev/null 2>&1; then
-    echo "  OK: dnf available"
-else
-    echo "  FAIL: No supported package manager"
+if ! command -v apt-get > /dev/null 2>&1; then
+    echo "  FAIL: apt-get required (Debian/Ubuntu only)"
     exit 1
 fi
+echo "  OK: apt-get available"
+apt-get update -qq

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/test-bootstrap.sh` around lines 16 - 24, The conditional currently
detects dnf but never uses it, causing failures on Fedora/RHEL; update the
installation logic in the top-level package manager check to either remove the
dnf test or add a dnf branch that sets a package manager variable (e.g., PM) and
maps install/update flags appropriately, then replace hard-coded apt-get calls
with that variable (or implement equivalent dnf commands) so the script works on
both apt and dnf systems; modify the if/elif block and subsequent apt-get usage
to reference the chosen package manager decision.

benchmarks/wrk.sh-4-7 (1)

4-7: ⚠️ Potential issue | 🟡 Minor

Honor WRK_BIN consistently.

The script tells users to set WRK_BIN, but both the presence check and the final exec still hard-code wrk, so a custom binary path will fail here.

🔧 Proposed fix

+wrk_bin="${WRK_BIN:-wrk}"
-if ! command -v wrk >/dev/null 2>&1; then
-  echo "wrk not found. Install wrk or set WRK_BIN." >&2
+if ! command -v "$wrk_bin" >/dev/null 2>&1; then
+  echo "${wrk_bin} not found. Install wrk or set WRK_BIN." >&2
   exit 1
 fi
@@
-echo "Running: wrk -t${threads} -c${connections} -d${duration} ${extra_args[*]} ${url}"
-exec wrk -t"${threads}" -c"${connections}" -d"${duration}" "${extra_args[@]}" "${url}"
+echo "Running: ${wrk_bin} -t${threads} -c${connections} -d${duration} ${extra_args[*]} ${url}"
+exec "$wrk_bin" -t"${threads}" -c"${connections}" -d"${duration}" "${extra_args[@]}" "${url}"

Also applies to: 35-36

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/wrk.sh` around lines 4 - 7, The script currently hard-codes "wrk"
for the existence check and when executing, ignoring the WRK_BIN override;
change the checks and invocation to respect WRK_BIN by using a variable (e.g.,
WRK_BIN=${WRK_BIN:-wrk}) and replace occurrences of literal wrk with that
variable (update the command -v check and the exec call, and the other
occurrences noted around lines 35-36) so the script verifies and executes the
configured binary rather than always using "wrk".

benchmarks/tcp-sustained.php-207-216 (1)

207-216: ⚠️ Potential issue | 🟡 Minor

Keep-alive "PING" may not be valid for database protocols.

The handshake uses protocol-specific formats (MySQL/PostgreSQL), but the keep-alive loop sends a raw "PING" string. Real PostgreSQL/MySQL connections don't recognize this command, which could cause protocol errors or connection termination.

For accurate testing, consider using protocol-appropriate keep-alive messages or simply relying on TCP keep-alive without application-level pings.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/tcp-sustained.php` around lines 207 - 216, The loop currently
sends a raw "PING" string over $client (in the while block using $running,
Coroutine::sleep, $client->send, $client->recv, $stats['requests']->add), which
is invalid for DB protocols; remove the application-level $client->send("PING")
(and its recv expectation) and either rely on TCP keep-alive at the socket level
or implement protocol-specific keep-alive outside this loop (e.g., a MySQL
COM_PING packet or PostgreSQL-compatible message); update the loop to only sleep
and count requests (or perform a proper protocol ping when configured) so you no
longer send raw "PING" on a DB connection.

PERFORMANCE.md-164-168 (1)

164-168: ⚠️ Potential issue | 🟡 Minor

TCP benchmark command uses incorrect flag syntax.

The command php benchmarks/tcp.php --port=3306 uses double-dash flag syntax, but the script reads configuration from environment variables (BENCH_PORT), not CLI arguments.

📝 Fix the command to use environment variable

 # MySQL connections
-php benchmarks/tcp.php --port=3306
+BENCH_PORT=3306 php benchmarks/tcp.php

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@PERFORMANCE.md` around lines 164 - 168, The TCP benchmark command in
PERFORMANCE.md is using CLI flag syntax that the script doesn't read; update the
example to set the BENCH_PORT environment variable before running the script
(referencing benchmarks/tcp.php and the BENCH_PORT env var) so the script picks
up the port correctly; replace the `--port=3306` invocation with an
environment-variable invocation using BENCH_PORT=3306 followed by the existing
php benchmarks/tcp.php command.

benchmarks/tcp.php-396-397 (1)

396-397: ⚠️ Potential issue | 🟡 Minor

Handshake send result not checked in non-persistent mode.

Unlike the persistent mode which checks send() return values, the non-persistent mode ignores the result of $client->send($handshake). Failed sends won't be counted as errors, potentially skewing benchmark results.

🔧 Add send result check

-                $client->send($handshake);
-                $response = $client->recv(8192);
+                if ($client->send($handshake) === false) {
+                    $errors++;
+                    $client->close();
+                    continue;
+                }
+                $response = $client->recv(8192);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/tcp.php` around lines 396 - 397, In non-persistent mode the result
of $client->send($handshake) is not checked, so failed sends are not counted;
update the block around $client->send and $client->recv to capture the return
value of $client->send($handshake), and if it returns false (or a non-positive
value) increment the same error counter used elsewhere (e.g., $errors++), close
or cleanup $client as appropriate, and skip calling $client->recv so the failure
is recorded consistently with the persistent-mode handling.

examples/http-proxy.php-59-64 (1)

59-64: ⚠️ Potential issue | 🟡 Minor

Connection count can become negative.

The decrement logic only checks if the key exists but doesn't prevent the count from going negative if onDisconnect is called more times than onConnect for a given resource.

Proposed fix

     public function onDisconnect(string $resourceId, array $metadata = []): void
     {
-        if (isset($this->connectionCounts[$resourceId])) {
+        if (isset($this->connectionCounts[$resourceId]) && $this->connectionCounts[$resourceId] > 0) {
             $this->connectionCounts[$resourceId]--;
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@examples/http-proxy.php` around lines 59 - 64, The onDisconnect method can
drive connectionCounts[resourceId] negative if disconnects outnumber connects;
modify onDisconnect (in the class where connectionCounts is maintained) to check
that the current count is greater than zero before decrementing and, after
decrementing, remove the key or set it to zero when it reaches zero to keep
counts non-negative and avoid stale entries. Ensure you reference
connectionCounts and onDisconnect when making the change.

proxies/smtp.php-14-18 (1)

14-18: ⚠️ Potential issue | 🟡 Minor

Use an unprivileged default port for the standalone example.

The documented php ... flow binds to port 25, which typically needs elevated privileges. Most contributors won't be able to run the example as written. Default to something like 2525 and keep 25 opt-in via env.

💡 Small tweak

 $config = [
     // Server settings
     'host' => '0.0.0.0',
-    'port' => 25,
+    'port' => (int) (getenv('SMTP_PORT') ?: 2525),
     'workers' => swoole_cpu_num() * 2,

Also applies to: 65-69

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@proxies/smtp.php` around lines 14 - 18, Update the standalone SMTP example to
use an unprivileged default port (2525) instead of 25: change any hardcoded
references and documentation strings ("php examples/smtp.php" usage and "telnet
localhost 25") to use 2525, and modify the code that sets the bind port (e.g.,
the $port or PORT variable) to read an environment variable like SMTP_PORT with
a default of 2525 so port 25 remains opt-in via env.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7e8dba5d-6ac1-407c-a016-c55a51b61b37

📥 Commits

Reviewing files that changed from the base of the PR and between 2b9a550 and 1d91e37.

📒 Files selected for processing (77)

• .dockerignore
• .env.example
• .github/workflows/integration.yml
• .github/workflows/lint.yml
• .github/workflows/static-analysis.yml
• .github/workflows/tests.yml
• .gitignore
• Dockerfile
• Dockerfile.test
• PERFORMANCE.md
• README.md
• benchmarks/README.md
• benchmarks/bootstrap-droplet.sh
• benchmarks/compare-http-servers.sh
• benchmarks/compare-tcp-servers.sh
• benchmarks/http-backend.php
• benchmarks/http-benchmark.php
• benchmarks/http.php
• benchmarks/setup-linux-production.sh
• benchmarks/setup-linux.sh
• benchmarks/stress-max.sh
• benchmarks/tcp-backend.php
• benchmarks/tcp-benchmark.php
• benchmarks/tcp-sustained.php
• benchmarks/tcp.php
• benchmarks/test-bootstrap.sh
• benchmarks/wrk.sh
• benchmarks/wrk2.sh
• composer.json
• docker-compose.dev.yml
• docker-compose.integration.yml
• docker-compose.yml
• examples/http-edge-integration.php
• examples/http-proxy.php
• examples/smtp-proxy.php
• examples/tcp-proxy.php
• phpunit.xml
• pint.json
• proxies/http.php
• proxies/smtp.php
• proxies/tcp.php
• src/Adapter.php
• src/Adapter/TCP.php
• src/ConnectionManager.php
• src/ConnectionResult.php
• src/Http/HttpConnectionManager.php
• src/Http/HttpServer.php
• src/Protocol.php
• src/Resolver.php
• src/Resolver/Exception.php
• src/Resolver/ReadWriteResolver.php
• src/Resolver/Result.php
• src/Resource.php
• src/ResourceStatus.php
• src/Server/HTTP/Swoole.php
• src/Server/HTTP/SwooleCoroutine.php
• src/Server/SMTP/Swoole.php
• src/Server/TCP/Config.php
• src/Server/TCP/Swoole.php
• src/Server/TCP/SwooleCoroutine.php
• src/Server/TCP/TLS.php
• src/Server/TCP/TlsContext.php
• src/Smtp/SmtpConnectionManager.php
• src/Tcp/TcpConnectionManager.php
• src/Tcp/TcpServer.php
• tests/AdapterActionsTest.php
• tests/AdapterMetadataTest.php
• tests/AdapterStatsTest.php
• tests/ConnectionResultTest.php
• tests/Integration/EdgeIntegrationTest.php
• tests/MockReadWriteResolver.php
• tests/MockResolver.php
• tests/Performance/PerformanceTest.php
• tests/QueryParserTest.php
• tests/ReadWriteSplitTest.php
• tests/ResolverTest.php
• tests/TCPAdapterTest.php

💤 Files with no reviewable changes (12)

• benchmarks/http-benchmark.php
• src/Smtp/SmtpConnectionManager.php
• src/Http/HttpServer.php
• src/ConnectionManager.php
• examples/tcp-proxy.php
• examples/smtp-proxy.php
• src/Resource.php
• src/Http/HttpConnectionManager.php
• src/Tcp/TcpConnectionManager.php
• benchmarks/tcp-benchmark.php
• src/ResourceStatus.php
• src/Tcp/TcpServer.php

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Build failing: composer.lock not found.

The CI pipeline is failing because composer.lock doesn't exist in the repository. Either commit the lock file or modify the COPY command.

🐛 Proposed fix

Option 1 - If composer.lock exists but isn't tracked:

git add composer.lock && git commit -m "Add composer.lock"

Option 2 - If you intentionally don't want to track the lock file:

-COPY composer.json composer.lock ./
+COPY composer.json ./
+RUN composer install --no-dev --optimize-autoloader \

🧰 Tools

🪛 GitHub Actions: Tests

[error] 28-28: Build failed during COPY: failed to compute cache key: failed to calculate checksum of ref ... "/composer.lock": not found

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` at line 28, The Dockerfile's COPY step references composer.lock
which does not exist causing CI to fail; either add and commit composer.lock to
the repo (git add composer.lock && git commit) so the existing COPY
composer.json composer.lock ./ works, or update the Dockerfile to stop copying
composer.lock (change the COPY line to only COPY composer.json ./ or add a
conditional/copy pattern that won't fail if composer.lock is absent) so builds
no longer error; locate the COPY composer.json composer.lock ./ line in the
Dockerfile to apply the chosen fix.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

The MySQL startup parser is looking at the wrong packet type.

This method is used during connection setup, but a normal MySQL client sends its default schema in the handshake response packet, not in COM_INIT_DB. Rejecting everything except command 0x02 means standard clients still won't be routable even after the server-first greeting exists.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Adapter/TCP.php` around lines 312 - 318, The parser in
parseMySQLDatabaseId is incorrectly treating the packet as COM_INIT_DB (0x02);
instead, detect and parse the MySQL Handshake Response (client response) by
reading the 4-byte little-endian capability flags at the start of $data, check
the CLIENT_CONNECT_WITH_DB flag (0x00000008), then locate the database string by
advancing past the username and auth-response fields according to the handshake
response format (skip username NUL, read auth-response length/bytes, then the
optional null-terminated database); throw on missing CLIENT_CONNECT_WITH_DB or
malformed layout and return the extracted database name.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Use the writer route for the primary backend socket.

When read/write split is enabled, this still resolves the long-lived backend connection with route(). If resolve() is not identical to resolveWrite(), every write and transaction-pinned query can be sent to the wrong backend.

Patch sketch

-        $result = $this->route($databaseId);
+        $result = $this->readWriteSplit && $this->resolver instanceof ReadWriteResolver
+            ? $this->routeWrite($databaseId)
+            : $this->route($databaseId);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Adapter/TCP.php` around lines 430 - 443, getBackendConnection currently
always uses the generic route() result for the long-lived backend socket, which
can send writes/transaction-pinned queries to the wrong host when read/write
split is enabled; change it to choose the writer/primary endpoint (e.g., call
the resolver that returns the write/primary route such as resolveWrite() or
routeWrite()) when the connection will be used for writes or when a transaction
is pinned, otherwise keep using the read route; update getBackendConnection to
detect read/write-split mode and transaction-pinned state and call the
write-resolving method (reference getBackendConnection, route(),
resolveWrite()/routeWrite(), and resolve()) so the cached backendConnections use
the correct primary socket for writes.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Preserve the query string when proxying.

Both forwarders rebuild the backend target from the path only. A request like /search?q=foo is forwarded as /search, so the proxy changes the resource being fetched.

Patch sketch

-        $path = $requestServer['request_uri'] ?? '/';
+        $path = $requestServer['request_uri'] ?? '/';
+        $query = $requestServer['query_string'] ?? '';
+        if ($query !== '') {
+            $path .= '?'.$query;
+        }

Apply the same change in the raw forwarder before composing $requestLine.

Also applies to: 447-451

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/HTTP/Swoole.php` around lines 321 - 324, The proxy currently drops
the query string because $path is built from $requestServer['request_uri']
without preserving the query; update the logic in Swoole.php where $path is
derived (the block setting $requestServer, $method, $path) to include the query
string when present (e.g., use REQUEST_URI or combine path and query) and apply
the same fix in the raw forwarder before composing $requestLine (the code around
lines that build $requestLine) so forwarded requests like /search?q=foo keep
?q=foo; ensure both forwarder paths reference the same $path variable that
includes the query string.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Preserve the query string when proxying.

Both forwarders rebuild the backend target from the path only. A request like /search?q=foo is forwarded as /search, so the proxy changes the resource being fetched.

Patch sketch

-        $path = $requestServer['request_uri'] ?? '/';
+        $path = $requestServer['request_uri'] ?? '/';
+        $query = $requestServer['query_string'] ?? '';
+        if ($query !== '') {
+            $path .= '?'.$query;
+        }

Apply the same change in the raw forwarder before composing $requestLine.

Also applies to: 425-429

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/HTTP/SwooleCoroutine.php` around lines 299 - 302, The proxy
currently drops the query string when reconstructing the backend target from
$requestServer['request_uri']; fix by appending the query string when present
(e.g., if (!empty($requestServer['query_string'])) { $path .= '?' .
$requestServer['query_string']; }) where $path is set (around the $method/$path
assignment) and apply the same change in the raw forwarder before composing
$requestLine so both forwarders preserve queries (refer to $requestServer,
$path, and the code that builds $requestLine).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

3306 never emits the required MySQL greeting.

The proxy waits for client bytes and then tries to parse a database ID, but MySQL is server-first. A normal client will stall at connect because it expects the proxy to send the initial handshake packet before any routable payload exists.

Also applies to: 289-295

🧰 Tools

🪛 PHPMD (2.15.0)

[warning] 193-193: Avoid unused parameters such as '$reactorId'. (undefined)

(UnusedFormalParameter)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/TCP/Swoole.php` around lines 193 - 204, The onConnect handler
currently only records client port and logs the connection but does not emit the
MySQL server handshake, causing clients (which expect a server-first greeting)
to stall; update onConnect (and the similar code for lines 289-295) to build and
send a proper MySQL initial handshake packet immediately after storing
$this->clientPorts[$fd] (using $server->send($fd, $handshakePayload) or the
appropriate Swoole send call) — include server version, connection id,
capability flags, charset, and auth-salt bytes consistent with MySQL protocol so
clients receive the greeting before any client-side payload is parsed.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Don't attach one frontend session to two backend sessions.

The startup packet is sent to both the primary and replica, and both backends start streaming responses into the same client FD. That duplicates startup/auth frames and lets later read/write replies race out of order, which breaks database session semantics.

Also applies to: 334-339

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/TCP/Swoole.php` around lines 297 - 320, The code currently assigns
the same frontend session FD to both write and read backends by storing
$this->readBackendClients[$fd], sending the startup packet
($readClient->send($data)), and calling $this->startForwarding($server, $fd,
$readClient); instead only create/connect the read replica socket but do NOT
attach it to the frontend FD, do NOT send the initial startup/auth packet to it,
and do NOT call startForwarding for it; instead keep the read client privately
managed (or indexed by a replica-specific key) and route read queries explicitly
through it from your proxy logic so only the primary write backend is streaming
responses into the frontend FD (apply the same change for the other occurrence
at the 334-339 block).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how minProtocol is defined in the TLS class
ast-grep --pattern $'class TLS {
  $$$
}'

Repository: utopia-php/protocol-proxy

Length of output: 6982

---

🏁 Script executed:

# Search for Swoole ssl_protocols usage or documentation in the codebase
rg -i "ssl_protocols" --type php -A 2 -B 2

Repository: utopia-php/protocol-proxy

Length of output: 474

---

🌐 Web query:

Swoole ssl_protocols configuration parameter format TLS

💡 Result:

ssl_protocols in Swoole/OpenSwoole is an integer bitmask: you bitwise-OR (|) the protocol constants you want to allow (not a string like "TLSv1.2 TLSv1.3"). By default, all protocols are enabled. [1]

Example (allow only TLS 1.2 + TLS 1.3):

$server->set([
  'ssl_protocols' => SWOOLE_SSL_TLSv1_2 | SWOOLE_SSL_TLSv1_3,
]);

Supported values include: SWOOLE_SSL_TLSv1, SWOOLE_SSL_TLSv1_1, SWOOLE_SSL_TLSv1_2, SWOOLE_SSL_TLSv1_3, SWOOLE_SSL_SSLv2. [1]
(These constants are also listed in the PHP manual, e.g. SWOOLE_SSL_TLSv1_2.) [2]

Sources: [1] OpenSwoole docs – Swoole Server ssl_protocols configuration, [2] PHP manual – Swoole predefined constants.

---

Fix ssl_protocols to allow TLS 1.2 and above, not only TLS 1.2.

Swoole's ssl_protocols expects a bitmask of allowed protocol constants. Currently, $this->tls->minProtocol is set to a single constant (SWOOLE_SSL_TLSv1_2), which disables TLS 1.3. For a "minimum" version, use a bitmask like SWOOLE_SSL_TLSv1_2 | SWOOLE_SSL_TLSv1_3 (or include other versions as needed).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Server/TCP/TlsContext.php` around lines 42 - 64, The ssl_protocols value
in toSwooleConfig currently uses $this->tls->minProtocol as a single constant
(e.g., SWOOLE_SSL_TLSv1_2) which excludes newer protocols; change to provide a
bitmask of allowed protocols instead (e.g., combine SWOOLE_SSL_TLSv1_2 |
SWOOLE_SSL_TLSv1_3 or build the mask programmatically based on
$this->tls->minProtocol) so that toSwooleConfig returns ssl_protocols as a
bitwise OR of supported SWOOLE_SSL_* constants rather than a single constant.

[coderabbitai]

Actionable comments posted: 8

🧹 Nitpick comments (2)

Dockerfile (1)

30-30: Pin the Composer image instead of using latest.

composer:latest makes this build non-reproducible and can break on upstream image changes. Please pin a Composer version or digest here.

Proposed change

-COPY --from=composer:latest /usr/bin/composer /usr/bin/composer
+COPY --from=composer:2 /usr/bin/composer /usr/bin/composer

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` at line 30, The Dockerfile currently references the moving tag
"composer:latest" in the COPY --from stage; replace that floating tag with a
pinned Composer image tag or digest (e.g., a specific version tag or sha256
digest) so the build is reproducible; update the COPY --from source to use the
chosen pinned reference and, if desired, add a build ARG like COMPOSER_VERSION
to centralize the version choice for future updates.

tests/AdapterStatsTest.php (1)

29-32: Avoid synchronizing this test to the wall clock.

This adds up to a second of delay per run and still relies on scheduler timing around the cache TTL boundary. A controllable clock/timestamp seam in the adapter/router would make the cache-hit path deterministic and keep the suite fast.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/AdapterStatsTest.php` around lines 29 - 32, The test currently syncs to
the wall clock using time() and usleep() which adds delays and flakiness; remove
this loop and make the timestamp deterministic by introducing a controllable
clock/seam: add or use a Clock/TimeProvider interface in the adapter/router (or
a timestamp parameter on the cache-check method used in AdapterStatsTest) and in
the test inject a fixed/mock clock that returns the desired times so the
cache-hit/path and TTL behavior are exercised deterministically without
sleeping; update AdapterStatsTest to use the mock Clock/TimeProvider (or pass
explicit timestamps) instead of calling time() and usleep().

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Dockerfile`:
- Around line 36-40: The container currently runs as root at runtime; create and
switch to a non-root user in the Dockerfile, ensure application files are owned
by that user, and switch to that user before the CMD that launches the proxy
(referencing the EXPOSE lines and CMD ["php", "proxies/http.php"]).
Specifically: add a non-root user/group (e.g., create user "app" or "proxy"),
chown the application directory and any runtime directories to that user, and
add a USER instruction prior to the CMD so the proxy runs as the dedicated user
instead of root; no capability changes are needed because the proxy binds only
to unprivileged ports (8080, 8081, 8025).
- Around line 31-34: Remove the --ignore-platform-reqs flag from the Composer
install command in the Dockerfile so Composer validates required PHP extensions
(ext-swoole, ext-redis) at build time; update the RUN composer install
invocation (the existing "RUN composer install \ --no-dev \
--optimize-autoloader \ --ignore-platform-reqs" command) to omit
--ignore-platform-reqs and ensure the image installs/ENABLED the ext-swoole and
ext-redis extensions before running that Composer step so platform checks
succeed.

In `@tests/Integration/EdgeIntegrationTest.php`:
- Around line 492-525: The test testConcurrentResolutionOfMultipleDatabases is
currently invoking $adapter->route() sequentially in a loop so it doesn't
exercise concurrency; change the test to invoke TCPAdapter::route concurrently
(e.g., spawn N parallel tasks/runtimes/workers or use an async framework)
against the same EdgeMockResolver registrations, collect/join all results before
asserting, then perform the same endpoint/protocol and getStats() assertions to
verify cache behavior under true concurrent resolution; apply the same change to
the other similar test referenced (the block around 530-566) so both actually
run route() in parallel rather than one-by-one.
- Around line 442-452: The test currently proves TTL expiry, not invalidation,
because Adapter::route has its own 1s routing cache; instead of sleeping, change
the test to exercise the cache owner's invalidation path directly: after calling
EdgeMockResolver::purge('invaldb') assert that the resolver recorded the purge
(e.g., check whatever log/record EdgeMockResolver stores) and then force the
adapter to bypass/clear its internal routing cache before calling
Adapter::route('invaldb') (use the adapter's public cache-clear/invalidate
method or add one if missing) and finally assert getResolveCount() increased to
2; alternatively remove the sleep and assert the resolver-side purge effect
directly by inspecting EdgeMockResolver's recorded purges and that a subsequent
resolution used the resolver rather than relying on TTL expiry.
- Around line 909-920: The resolve method currently catches all
ResolverException and fails over every error; change it to only fail over on
transient availability errors (e.g., UNAVAILABLE, TIMEOUT) and rethrow for
definitive errors (e.g., NOT_FOUND, FORBIDDEN). In the resolve(string
$resourceId): Result implementation, after catching ResolverException $e,
inspect the exception's status/code (from the ResolverException API) and if it
indicates a transient availability issue set $this->failedOver = true and return
$this->secondary->resolve($resourceId); otherwise rethrow $e so primary->resolve
and secondary->resolve behavior is preserved for definitive errors. Ensure
$this->failedOver is only set when actually failing over.
- Around line 928-960: The wrapper currently records
onConnect/onDisconnect/track calls into local arrays but never forwards them to
the actual resolver that served resolve(), so child resolvers' cleanup/metrics
are bypassed; modify resolve() to record which underlying resolver instance
(primary or secondary) was chosen for each resourceId (e.g. persist a map like
$this->resolvedMap[$resourceId] = $chosenResolver) and update onConnect,
onDisconnect, track, purge to look up the resolver from that map and delegate
the call to that resolver (fall back to existing local arrays only if no mapping
exists); ensure purge also removes the mapping and still calls
$this->primary->purge/$this->secondary->purge as needed and keep getStats()
returning child resolvers' stats while optionally including wrapper counters.

In `@tests/QueryParserTest.php`:
- Around line 620-682: The micro-benchmarks testParsePerformance and
testClassifySqlPerformance are flaky in CI; move them out of the blocking
PHPUnit suite by making them opt-in: either add a PHPUnit group annotation
(e.g., `@group` performance) on the test methods (testParsePerformance,
testClassifySqlPerformance) so they can be excluded from default runs, or gate
their execution at runtime by checking an env var (e.g., if
(!getenv('RUN_PERF_TESTS')) $this->markTestSkipped('Performance tests
disabled');). Ensure references to pgParser->parse and pgParser->classifySQL
(and mysqlParser->parse) remain intact so the benchmarks still run when
explicitly enabled.
- Around line 575-579: The test testClassifyCteNoFinalKeyword is asserting that
pgParser->classifySQL('WITH x AS (SELECT 1)') returns QueryType::Read, but that
WITH-only input is incomplete and should be classified as QueryType::Unknown to
match the runtime safety in src/Adapter/TCP (which treats unknown as
primary-only); update the expectation in testClassifyCteNoFinalKeyword to assert
QueryType::Unknown from pgParser->classifySQL instead of QueryType::Read.

---

Nitpick comments:
In `@Dockerfile`:
- Line 30: The Dockerfile currently references the moving tag "composer:latest"
in the COPY --from stage; replace that floating tag with a pinned Composer image
tag or digest (e.g., a specific version tag or sha256 digest) so the build is
reproducible; update the COPY --from source to use the chosen pinned reference
and, if desired, add a build ARG like COMPOSER_VERSION to centralize the version
choice for future updates.

In `@tests/AdapterStatsTest.php`:
- Around line 29-32: The test currently syncs to the wall clock using time() and
usleep() which adds delays and flakiness; remove this loop and make the
timestamp deterministic by introducing a controllable clock/seam: add or use a
Clock/TimeProvider interface in the adapter/router (or a timestamp parameter on
the cache-check method used in AdapterStatsTest) and in the test inject a
fixed/mock clock that returns the desired times so the cache-hit/path and TTL
behavior are exercised deterministically without sleeping; update
AdapterStatsTest to use the mock Clock/TimeProvider (or pass explicit
timestamps) instead of calling time() and usleep().

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 08062e47-f934-4dd5-a61e-fac924f3ebad

📥 Commits

Reviewing files that changed from the base of the PR and between 1d91e37 and e2c66a7.

📒 Files selected for processing (11)

• Dockerfile
• PERFORMANCE.md
• tests/AdapterActionsTest.php
• tests/AdapterMetadataTest.php
• tests/AdapterStatsTest.php
• tests/ConnectionResultTest.php
• tests/Integration/EdgeIntegrationTest.php
• tests/QueryParserTest.php
• tests/ReadWriteSplitTest.php
• tests/ResolverTest.php
• tests/TCPAdapterTest.php

💤 Files with no reviewable changes (1)

• PERFORMANCE.md

🚧 Files skipped from review as they are similar to previous changes (5)

• tests/ResolverTest.php
• tests/TCPAdapterTest.php
• tests/ConnectionResultTest.php
• tests/AdapterActionsTest.php
• tests/ReadWriteSplitTest.php

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

git ls-files | grep -i dockerfile | head -20

Repository: utopia-php/protocol-proxy

Length of output: 79

---

🏁 Script executed:

cat -n Dockerfile | head -50

Repository: utopia-php/protocol-proxy

Length of output: 1089

---

🏁 Script executed:

cat composer.json

Repository: utopia-php/protocol-proxy

Length of output: 1895

---

Remove --ignore-platform-reqs to validate required extensions.

The Dockerfile installs ext-swoole and ext-redis before running Composer, and composer.json explicitly requires both extensions. Using --ignore-platform-reqs suppresses Composer's platform validation, masking installation failures and deferring errors to runtime. Remove the flag to ensure Composer verifies that the required extensions are actually available.

Proposed change

 RUN composer install \
     --no-dev \
-    --optimize-autoloader \
-    --ignore-platform-reqs
+    --optimize-autoloader

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	RUN composer install \
	--no-dev \
	--optimize-autoloader \
	--ignore-platform-reqs
	RUN composer install \
	--no-dev \
	--optimize-autoloader

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 31 - 34, Remove the --ignore-platform-reqs flag from
the Composer install command in the Dockerfile so Composer validates required
PHP extensions (ext-swoole, ext-redis) at build time; update the RUN composer
install invocation (the existing "RUN composer install \ --no-dev \
--optimize-autoloader \ --ignore-platform-reqs" command) to omit
--ignore-platform-reqs and ensure the image installs/ENABLED the ext-swoole and
ext-redis extensions before running that Composer step so platform checks
succeed.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Drop root before starting the proxy.

This container still runs as root at runtime. The proxy only binds to unprivileged ports, so it should run as a dedicated user instead of keeping full container privileges.

Proposed change

+RUN addgroup -S app && adduser -S -G app app
+
-COPY . .
+COPY --chown=app:app . .
+RUN chown -R app:app /app
 
 EXPOSE 8080 8081 8025
 
+USER app
 CMD ["php", "proxies/http.php"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile` around lines 36 - 40, The container currently runs as root at
runtime; create and switch to a non-root user in the Dockerfile, ensure
application files are owned by that user, and switch to that user before the CMD
that launches the proxy (referencing the EXPOSE lines and CMD ["php",
"proxies/http.php"]). Specifically: add a non-root user/group (e.g., create user
"app" or "proxy"), chown the application directory and any runtime directories
to that user, and add a USER instruction prior to the CMD so the proxy runs as
the dedicated user instead of root; no capability changes are needed because the
proxy binds only to unprivileged ports (8080, 8081, 8025).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

This test passes even if invalidation is broken.

src/Adapter/TCP.php:490-520 keeps a separate 1-second routing cache inside the adapter, and sleep(2) guarantees that cache expires before the second route() call. On top of that, EdgeMockResolver::purge() only records the resource ID. So this assertion proves TTL expiry, not cache invalidation. Exercise the cache owner's invalidation path directly, or assert the resolver-side purge effect instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Integration/EdgeIntegrationTest.php` around lines 442 - 452, The test
currently proves TTL expiry, not invalidation, because Adapter::route has its
own 1s routing cache; instead of sleeping, change the test to exercise the cache
owner's invalidation path directly: after calling
EdgeMockResolver::purge('invaldb') assert that the resolver recorded the purge
(e.g., check whatever log/record EdgeMockResolver stores) and then force the
adapter to bypass/clear its internal routing cache before calling
Adapter::route('invaldb') (use the adapter's public cache-clear/invalidate
method or add one if missing) and finally assert getResolveCount() increased to
2; alternatively remove the sleep and assert the resolver-side purge effect
directly by inspecting EdgeMockResolver's recorded purges and that a subsequent
resolution used the resolver rather than relying on TTL expiry.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

These “concurrent” tests are fully sequential.

Both methods call route() one request at a time in a plain loop/try-catch, so there is no overlapping work here. That leaves the concurrency path untested while the test names claim otherwise.

Also applies to: 530-566

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Integration/EdgeIntegrationTest.php` around lines 492 - 525, The test
testConcurrentResolutionOfMultipleDatabases is currently invoking
$adapter->route() sequentially in a loop so it doesn't exercise concurrency;
change the test to invoke TCPAdapter::route concurrently (e.g., spawn N parallel
tasks/runtimes/workers or use an async framework) against the same
EdgeMockResolver registrations, collect/join all results before asserting, then
perform the same endpoint/protocol and getStats() assertions to verify cache
behavior under true concurrent resolution; apply the same change to the other
similar test referenced (the block around 530-566) so both actually run route()
in parallel rather than one-by-one.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Fail over only on transient availability errors.

Catching every ResolverException means primary NOT_FOUND or FORBIDDEN responses are retried against the secondary. That can turn a definitive miss/deny into a successful route and makes the failover tests assert the wrong contract. Restrict failover to transient availability failures such as UNAVAILABLE/TIMEOUT and rethrow the rest.

Suggested guard

     try {
         return $this->primary->resolve($resourceId);
     } catch (ResolverException $e) {
+        if (!\in_array($e->getCode(), [ResolverException::UNAVAILABLE, ResolverException::TIMEOUT], true)) {
+            throw $e;
+        }
+
         $this->failedOver = true;
 
         // Try secondary; let its exception propagate if it also fails
         return $this->secondary->resolve($resourceId);
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	public function resolve(string $resourceId): Result
	{
	$this->failedOver = false;
	try {
	return $this->primary->resolve($resourceId);
	} catch (ResolverException $e) {
	$this->failedOver = true;
	// Try secondary; let its exception propagate if it also fails
	return $this->secondary->resolve($resourceId);
	}
	public function resolve(string $resourceId): Result
	{
	$this->failedOver = false;
	try {
	return $this->primary->resolve($resourceId);
	} catch (ResolverException $e) {
	if (!\in_array($e->getCode(), [ResolverException::UNAVAILABLE, ResolverException::TIMEOUT], true)) {
	throw $e;
	}
	$this->failedOver = true;
	// Try secondary; let its exception propagate if it also fails
	return $this->secondary->resolve($resourceId);
	}
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Integration/EdgeIntegrationTest.php` around lines 909 - 920, The
resolve method currently catches all ResolverException and fails over every
error; change it to only fail over on transient availability errors (e.g.,
UNAVAILABLE, TIMEOUT) and rethrow for definitive errors (e.g., NOT_FOUND,
FORBIDDEN). In the resolve(string $resourceId): Result implementation, after
catching ResolverException $e, inspect the exception's status/code (from the
ResolverException API) and if it indicates a transient availability issue set
$this->failedOver = true and return $this->secondary->resolve($resourceId);
otherwise rethrow $e so primary->resolve and secondary->resolve behavior is
preserved for definitive errors. Ensure $this->failedOver is only set when
actually failing over.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Lifecycle callbacks stop at the wrapper.

src/Adapter.php:75-142 sends connect/close/track events to the resolver wired into the adapter. Here those callbacks are only appended to local arrays, while getStats() reports the child resolvers' stats instead of these local counters. So the resolver that actually served resolve() never sees lifecycle callbacks and its cleanup/metrics logic is bypassed. Persist the chosen resolver per resourceId during resolve() and delegate subsequent lifecycle calls to it.

One way to wire the callbacks through

 class EdgeFailoverResolver implements Resolver
 {
     protected bool $failedOver = false;
+    /** `@var` array<string, Resolver> */
+    protected array $activeResolvers = [];
@@
     public function resolve(string $resourceId): Result
     {
         $this->failedOver = false;
 
         try {
-            return $this->primary->resolve($resourceId);
+            $result = $this->primary->resolve($resourceId);
+            $this->activeResolvers[$resourceId] = $this->primary;
+
+            return $result;
         } catch (ResolverException $e) {
             $this->failedOver = true;
 
             // Try secondary; let its exception propagate if it also fails
-            return $this->secondary->resolve($resourceId);
+            $result = $this->secondary->resolve($resourceId);
+            $this->activeResolvers[$resourceId] = $this->secondary;
+
+            return $result;
         }
     }
@@
     public function onConnect(string $resourceId, array $metadata = []): void
     {
         $this->connects[] = ['resourceId' => $resourceId, 'metadata' => $metadata];
+        ($this->activeResolvers[$resourceId] ?? $this->primary)->onConnect($resourceId, $metadata);
     }
@@
     public function onDisconnect(string $resourceId, array $metadata = []): void
     {
         $this->disconnects[] = ['resourceId' => $resourceId, 'metadata' => $metadata];
+        ($this->activeResolvers[$resourceId] ?? $this->primary)->onDisconnect($resourceId, $metadata);
+        unset($this->activeResolvers[$resourceId]);
     }
@@
     public function track(string $resourceId, array $metadata = []): void
     {
         $this->activities[] = ['resourceId' => $resourceId, 'metadata' => $metadata];
+        ($this->activeResolvers[$resourceId] ?? $this->primary)->track($resourceId, $metadata);
     }
@@
     public function purge(string $resourceId): void
     {
         $this->invalidations[] = $resourceId;
+        unset($this->activeResolvers[$resourceId]);
         $this->primary->purge($resourceId);
         $this->secondary->purge($resourceId);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Integration/EdgeIntegrationTest.php` around lines 928 - 960, The
wrapper currently records onConnect/onDisconnect/track calls into local arrays
but never forwards them to the actual resolver that served resolve(), so child
resolvers' cleanup/metrics are bypassed; modify resolve() to record which
underlying resolver instance (primary or secondary) was chosen for each
resourceId (e.g. persist a map like $this->resolvedMap[$resourceId] =
$chosenResolver) and update onConnect, onDisconnect, track, purge to look up the
resolver from that map and delegate the call to that resolver (fall back to
existing local arrays only if no mapping exists); ensure purge also removes the
mapping and still calls $this->primary->purge/$this->secondary->purge as needed
and keep getStats() returning child resolvers' stats while optionally including
wrapper counters.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't lock an incomplete WITH statement to the read path.

WITH x AS (SELECT 1) is syntactically incomplete. Asserting QueryType::Read here blesses an unsafe fallback that can route malformed SQL to replicas, even though src/Adapter/TCP.php:179-215 intentionally keeps QueryType::Unknown on primary for safety.

Suggested expectation

     public function testClassifyCteNoFinalKeyword(): void
     {
-        // Bare WITH with no recognizable final statement - defaults to READ
+        // Incomplete CTE should stay on the primary path for safety.
         $sql = 'WITH x AS (SELECT 1)';
-        $this->assertSame(QueryType::Read, $this->pgParser->classifySQL($sql));
+        $this->assertSame(QueryType::Unknown, $this->pgParser->classifySQL($sql));
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	public function testClassifyCteNoFinalKeyword(): void
	{
	// Bare WITH with no recognizable final statement - defaults to READ
	$sql = 'WITH x AS (SELECT 1)';
	$this->assertSame(QueryType::Read, $this->pgParser->classifySQL($sql));
	public function testClassifyCteNoFinalKeyword(): void
	{
	// Incomplete CTE should stay on the primary path for safety.
	$sql = 'WITH x AS (SELECT 1)';
	$this->assertSame(QueryType::Unknown, $this->pgParser->classifySQL($sql));
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/QueryParserTest.php` around lines 575 - 579, The test
testClassifyCteNoFinalKeyword is asserting that pgParser->classifySQL('WITH x AS
(SELECT 1)') returns QueryType::Read, but that WITH-only input is incomplete and
should be classified as QueryType::Unknown to match the runtime safety in
src/Adapter/TCP (which treats unknown as primary-only); update the expectation
in testClassifyCteNoFinalKeyword to assert QueryType::Unknown from
pgParser->classifySQL instead of QueryType::Read.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Move these micro-benchmarks out of the blocking PHPUnit suite.

The < 1us / < 2us thresholds depend heavily on CPU speed, CI contention, coverage settings, and thermal throttling, so these tests will fail intermittently without any parser regression. Keep them as opt-in benchmarks or a dedicated performance job instead of regular pass/fail unit tests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/QueryParserTest.php` around lines 620 - 682, The micro-benchmarks
testParsePerformance and testClassifySqlPerformance are flaky in CI; move them
out of the blocking PHPUnit suite by making them opt-in: either add a PHPUnit
group annotation (e.g., `@group` performance) on the test methods
(testParsePerformance, testClassifySqlPerformance) so they can be excluded from
default runs, or gate their execution at runtime by checking an env var (e.g.,
if (!getenv('RUN_PERF_TESTS')) $this->markTestSkipped('Performance tests
disabled');). Ensure references to pgParser->parse and pgParser->classifySQL
(and mysqlParser->parse) remain intact so the benchmarks still run when
explicitly enabled.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@README.md`:
- Around line 391-426: The fenced ASCII-art diagram block in README.md lacks a
language tag; update the opening fence for the diagram (the triple-backtick that
precedes the ASCII art starting with the box drawing "┌────────────────") to
include a language tag (e.g., "text") so it becomes ```text and leave the
closing ``` unchanged to satisfy markdownlint MD040.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9f91c034-ee55-4389-a163-df83ac4d3b81

📥 Commits

Reviewing files that changed from the base of the PR and between e2c66a7 and 27d1e9e.

📒 Files selected for processing (1)

• README.md

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add a language tag to the architecture diagram fence.

The block starting at Line 391 is missing a fenced-code language, which matches the markdownlint MD040 warning.

Suggested fix

-```
+```text
 ┌─────────────────────────────────────────────────────────────────┐
 │                        Protocol Proxy                           │
 ├─────────────────────────────────────────────────────────────────┤
@@
 └──────────────────────────────────────────────────────────────────┘

</details>

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

```suggestion

🧰 Tools

🪛 markdownlint-cli2 (0.21.0)

[warning] 391-391: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 391 - 426, The fenced ASCII-art diagram block in
README.md lacks a language tag; update the opening fence for the diagram (the
triple-backtick that precedes the ASCII art starting with the box drawing
"┌────────────────") to include a language tag (e.g., "text") so it becomes
```text and leave the closing ``` unchanged to satisfy markdownlint MD040.