ci: add macOS to the test matrix and split unit tests from integration tests

.github/workflows/test.yml

@@ -27,12 +27,14 @@ permissions:
 
 jobs:
   tests:
-    runs-on: ubuntu-latest
-    timeout-minutes: 30
+    runs-on: ${{ matrix.os }}
+    # macOS devel runs take about 30 minutes; Linux finishes in about 15.
+    timeout-minutes: ${{ matrix.os == 'macOS-latest' && 60 || 30 }}
     strategy:
       matrix:
         os:
           - 'ubuntu-latest'
+          - 'macOS-latest'
         nim-version:
           - '2.2.4'
           - 'stable'
@@ -72,8 +74,10 @@ jobs:
       - name: Generate test certificates
         run: bash tests/gen_certs.sh
 
+      # macOS runners have no Docker.
       - name: Start PostgreSQL
         id: start-psql
+        if: runner.os == 'Linux'
         run: docker compose up -d --wait
         background: true
 
@@ -92,9 +96,17 @@ jobs:
 
       - name: Run tests
         id: run-tests
+        if: runner.os == 'Linux'
         run: nimble test -y
         background: true
 
+      # No PostgreSQL on macOS: unit and mock-server tests only.
+      - name: Run unit tests
+        id: run-unit-tests
+        if: runner.os != 'Linux'
+        run: nimble testUnit -y
+        background: true
+
       # tests/config.nims always adds -d:ssl, so no test build covers this.
       - name: Check build without TLS (asyncdispatch, no -d:ssl)
         run: nim check --hints:off --warningAsError:UnreachableCode --warningAsError:UnusedImport async_postgres.nim
@@ -105,9 +117,11 @@ jobs:
       - name: Compile examples (chronos)
         run: for f in examples/*.nim; do nim c -d:asyncBackend=chronos "$f"; done
 
+      # Platform-independent, so generate once.
       - name: Gen docs
+        if: runner.os == 'Linux'
         run: |
           nim doc --project --index:on --outdir:./htmldocs ./async_postgres.nim
 
-      - name: Wait setup
-        wait: [run-tests]
+      - name: Wait tests
+        wait: [run-tests, run-unit-tests]

async_postgres.nimble

@@ -27,10 +27,17 @@ task parseGuard, "check that stdlib text parsers are only called from the gramma
 task privateAccessGuard, "check that privateAccess is only used from tests":
   exec "nim c -r --hints:off tools/private_access_guard.nim"
 
-task test, "test":
+proc runSuite(file: string) =
+  ## Build and run `file` once per async backend.
+  exec "bash tests/gen_certs.sh"
+  exec "nim c -d:asyncBackend=asyncdispatch -r " & file
+  exec "nim c -d:asyncBackend=chronos -r " & file
+
+task test, "run the full suite (requires a live PostgreSQL on 127.0.0.1:15432)":
   apiSurfaceTask()
   parseGuardTask()
   privateAccessGuardTask()
-  exec "bash tests/gen_certs.sh"
-  exec "nim c -d:asyncBackend=asyncdispatch -r tests/all_tests.nim"
-  exec "nim c -d:asyncBackend=chronos -r tests/all_tests.nim"
+  runSuite "tests/all_tests.nim"
+
+task testUnit, "run unit and mock-server tests only (no PostgreSQL required)":
+  runSuite "tests/all_tests_unit.nim"

async_postgres/async_backend.nim

@@ -210,7 +210,10 @@ elif hasAsyncDispatch:
     ## release other resources the orphan holds. When ``onOrphan`` is omitted
     ## the default handler drains the orphan's outcome (clears a late failure)
     ## but does **not** release other resources. Pass an explicit ``onOrphan``
-    ## when the orphan owns a connection or other live resource.
+    ## when the orphan owns a connection or other live resource. The default
+    ## clears the error on ``fut`` itself, so anything else that awaits ``fut``
+    ## after the timeout reads a late failure as a default-valued success; pass
+    ## an explicit ``onOrphan`` (a no-op will do) in that case too.
     ##
     ## Under chronos the ``onOrphan`` argument is accepted but never called —
     ## futures are properly cancelled on timeout and no orphan remains.

async_postgres/pg_connection/buffer_io.nim

@@ -109,9 +109,10 @@ proc oneLine*(msg: string): string =
 type DialFailure = tuple[target: string, err: ref CatchableError]
 
 when defined(posix):
-  func isTransientErrno(code: int32): bool =
+  proc isTransientErrno(code: int32): bool {.raises: [].} =
     ## Whether an OS error may clear (``ENOENT``: a Unix socket not created yet).
-    # Qualified: chronos exports same-named OSErrorCode constants.
+    # Qualified: chronos exports same-named OSErrorCode constants. A proc, not
+    # a func: macOS's posix errno constants are importc vars, not consts.
     code in [
       posix.ECONNREFUSED, posix.ECONNRESET, posix.ECONNABORTED, posix.ETIMEDOUT,
       posix.EHOSTUNREACH, posix.ENETUNREACH, posix.ENETDOWN, posix.EADDRNOTAVAIL,

async_postgres/pg_connection/lifecycle.nim

@@ -461,14 +461,10 @@ proc connectToHostImpl(
     when defined(posix):
       if not isUnix:
         try:
-          when defined(nimdoc):
-            # nim doc resolves nativesockets.SocketHandle to winlean on some
-            # setups, so cast explicitly to satisfy the doc-time type check.
-            configureTcpNoDelay(posix.SocketHandle(sock.getFd()))
-            configureKeepalive(posix.SocketHandle(sock.getFd()), config)
-          else:
-            configureTcpNoDelay(sock.getFd())
-            configureKeepalive(sock.getFd(), config)
+          # Cast explicitly: nim doc resolves nativesockets.SocketHandle to
+          # winlean on some setups.
+          configureTcpNoDelay(posix.SocketHandle(sock.getFd()))
+          configureKeepalive(posix.SocketHandle(sock.getFd()), config)
         except CatchableError as e:
           sock.close()
           raise e

async_postgres/pg_pool_cluster.nim

@@ -157,6 +157,11 @@ proc fireReadFallback(
   if cluster.onReadFallback != nil:
     cluster.onReadFallback(reason, err)
 
+proc keepOrphanOutcome(fut: Future[PgConnection]) {.gcsafe.} =
+  ## No-op orphan hook: the default one clears a late failure, which would make
+  ## `drainAbandonedAcquire` read a nil connection out of the failed future.
+  discard
+
 proc drainAbandonedAcquire(acquireFut: Future[PgConnection]) {.async.} =
   ## Reclaim the connection from a pool acquire abandoned by `fallbackTimeout`.
   ##
@@ -170,9 +175,10 @@ proc drainAbandonedAcquire(acquireFut: Future[PgConnection]) {.async.} =
   ## resolves immediately without a connection.
   try:
     let conn = await acquireFut
-    # Not `release()`: the pool is taking its own abandoned acquire back, not the
-    # application returning a borrow.
-    conn.releaseReclaimed()
+    if conn != nil:
+      # Not `release()`: the pool is taking its own abandoned acquire back, not
+      # the application returning a borrow.
+      conn.releaseReclaimed()
   except CatchableError:
     discard # a failed/cancelled acquire cleans up its own pool accounting
 
@@ -198,7 +204,7 @@ proc acquireRead(
   if cluster.fallbackTimeout > ZeroDuration:
     let replicaFut = cluster.replica.acquire()
     try:
-      let conn = await replicaFut.wait(cluster.fallbackTimeout)
+      let conn = await replicaFut.wait(cluster.fallbackTimeout, keepOrphanOutcome)
       return (conn, cluster.replica)
     except AsyncTimeoutError as e:
       asyncSpawn drainAbandonedAcquire(replicaFut)
@@ -229,7 +235,7 @@ proc acquireRead(
   if cluster.fallbackTimeout > ZeroDuration:
     let primaryFut = cluster.primary.acquire()
     try:
-      let conn = await primaryFut.wait(cluster.fallbackTimeout)
+      let conn = await primaryFut.wait(cluster.fallbackTimeout, keepOrphanOutcome)
       return (conn, cluster.primary)
     except AsyncTimeoutError:
       asyncSpawn drainAbandonedAcquire(primaryFut)

tests/all_tests.nim

@@ -1,17 +1,4 @@
+## Full suite; needs a live PostgreSQL (docker-compose.yml).
 {.push warning[UnusedImport]: off.}
-import
-  test_abandonment_e2e, test_advisory_lock, test_aggregate, test_async_backend,
-  test_auth, test_bytes, test_cache, test_cancel_e2e, test_conn_types, test_copy_race,
-  test_dsn, test_e2e_arrays, test_e2e_connection, test_e2e_convenience, test_e2e_copy,
-  test_e2e_cursor, test_e2e_listen, test_e2e_misc, test_e2e_pool, test_e2e_query,
-  test_e2e_transaction, test_e2e_types, test_errors, test_fill_recvbuf, test_keepalive,
-  test_largeobject, test_largeobject_parse, test_listen_reconnect, test_network_failure,
-  test_physical_replication, test_pool, test_private_access_guard, test_protocol,
-  test_protocol_fuzz, test_replication, test_replication_auto_confirm,
-  test_replication_keepalive, test_replication_restart, test_rowdata, test_saslprep,
-  test_sendbuf_seal, test_server_error, test_session_attrs, test_source_scan, test_sql,
-  test_ssl, test_tls_error_paths, test_tracing, test_transaction_cancel,
-  test_tx_cleanup_defect, test_type_lookup, test_types_array, test_types_inline,
-  test_types_misc, test_types_numeric, test_types_range, test_types_scalar,
-  test_types_temporal, test_types_user_defined, test_types_validation, test_pool_cluster
+import all_tests_unit, all_tests_integration
 {.pop.}

tests/all_tests_integration.nim

@@ -0,0 +1,8 @@
+## Tests that need a live PostgreSQL at 127.0.0.1:15432 (docker-compose.yml).
+{.push warning[UnusedImport]: off.}
+import
+  test_abandonment_e2e, test_advisory_lock, test_cancel_e2e, test_e2e_arrays,
+  test_e2e_connection, test_e2e_convenience, test_e2e_copy, test_e2e_cursor,
+  test_e2e_listen, test_e2e_misc, test_e2e_pool, test_e2e_query, test_e2e_transaction,
+  test_e2e_types, test_largeobject, test_tracing
+{.pop.}

tests/all_tests_unit.nim

@@ -0,0 +1,15 @@
+## Tests that need no PostgreSQL (pure logic or in-process mock servers).
+{.push warning[UnusedImport]: off.}
+import
+  test_aggregate, test_async_backend, test_auth, test_bytes, test_cache,
+  test_conn_types, test_copy_race, test_dsn, test_errors, test_fill_recvbuf,
+  test_keepalive, test_largeobject_parse, test_listen_reconnect, test_network_failure,
+  test_physical_replication, test_pool, test_pool_cluster, test_private_access_guard,
+  test_protocol, test_protocol_fuzz, test_replication, test_replication_auto_confirm,
+  test_replication_keepalive, test_replication_restart, test_rowdata, test_saslprep,
+  test_sendbuf_seal, test_server_error, test_session_attrs, test_source_scan, test_sql,
+  test_ssl, test_tls_error_paths, test_transaction_cancel, test_tx_cleanup_defect,
+  test_type_lookup, test_types_array, test_types_inline, test_types_misc,
+  test_types_numeric, test_types_range, test_types_scalar, test_types_temporal,
+  test_types_user_defined, test_types_validation
+{.pop.}

tests/mock_pg_server.nim

@@ -180,6 +180,31 @@ elif hasAsyncDispatch:
     if data.len > 0:
       await client.send(cast[string](data))
 
+when defined(posix):
+  from std/posix import nil
+
+  proc sendSegmentsNow*(client: MockClient, segments: openArray[seq[byte]]) =
+    ## Send each of `segments` as its own TCP segment before returning, so the
+    ## peer cannot run between them. Consecutive `sendBytes` calls do not
+    ## guarantee this: asyncdispatch defers each write to a later poll, and
+    ## Nagle may hold a later segment until the peer ACKs an earlier one.
+    when hasChronos:
+      let fd = posix.SocketHandle(client.fd)
+    elif hasAsyncDispatch:
+      let fd = posix.SocketHandle(client.getFd())
+    var one: cint = 1
+    doAssert posix.setsockopt(
+      fd,
+      cint(posix.IPPROTO_TCP),
+      posix.TCP_NODELAY,
+      addr one,
+      posix.SockLen(sizeof(one)),
+    ) == 0, "setsockopt(TCP_NODELAY) failed"
+    for seg in segments:
+      doAssert seg.len > 0
+      let n = posix.send(fd, unsafeAddr seg[0], seg.len, posix.MSG_NOSIGNAL)
+      doAssert n == seg.len, "short or failed send"
+
 # Message-building helpers
 
 proc buildBackendMsg*(msgType: char, body: openArray[byte]): seq[byte] =

tests/test_keepalive.nim

@@ -17,14 +17,18 @@ suite "configureKeepalive":
     doAssert fd != SocketHandle(-1), "socket() failed"
     fd
 
+  proc keepaliveEnabled(fd: SocketHandle): bool =
+    # macOS/BSD report the flag bit (8) instead of 1.
+    getIntSockOpt(fd, SOL_SOCKET, SO_KEEPALIVE) != 0
+
   test "keepAlive=false does not set SO_KEEPALIVE":
     let fd = makeSocket()
     defer:
       discard close(fd)
     var config = ConnConfig()
     config.keepAlive = false
     configureKeepalive(fd, config)
-    check getIntSockOpt(fd, SOL_SOCKET, SO_KEEPALIVE) == 0
+    check not keepaliveEnabled(fd)
 
   test "keepAlive=true sets SO_KEEPALIVE":
     let fd = makeSocket()
@@ -33,7 +37,7 @@ suite "configureKeepalive":
     var config = ConnConfig()
     config.keepAlive = true
     configureKeepalive(fd, config)
-    check getIntSockOpt(fd, SOL_SOCKET, SO_KEEPALIVE) == 1
+    check keepaliveEnabled(fd)
 
   test "keepAlive with idle/interval/count":
     let fd = makeSocket()
@@ -45,7 +49,7 @@ suite "configureKeepalive":
     config.keepAliveInterval = 7
     config.keepAliveCount = 3
     configureKeepalive(fd, config)
-    check getIntSockOpt(fd, SOL_SOCKET, SO_KEEPALIVE) == 1
+    check keepaliveEnabled(fd)
     when defined(linux):
       check getIntSockOpt(fd, cint(posix.IPPROTO_TCP), TCP_KEEPIDLE) == 42
       check getIntSockOpt(fd, cint(posix.IPPROTO_TCP), TCP_KEEPINTVL) == 7
@@ -65,7 +69,7 @@ suite "configureKeepalive":
     config.keepAliveInterval = 0
     config.keepAliveCount = 0
     configureKeepalive(fd, config)
-    check getIntSockOpt(fd, SOL_SOCKET, SO_KEEPALIVE) == 1
+    check keepaliveEnabled(fd)
 
   test "keepAlive=false with timing params does not set SO_KEEPALIVE":
     let fd = makeSocket()
@@ -77,7 +81,7 @@ suite "configureKeepalive":
     config.keepAliveInterval = 10
     config.keepAliveCount = 3
     configureKeepalive(fd, config)
-    check getIntSockOpt(fd, SOL_SOCKET, SO_KEEPALIVE) == 0
+    check not keepaliveEnabled(fd)
 
   test "partial timing (idle only)":
     let fd = makeSocket()
@@ -87,7 +91,7 @@ suite "configureKeepalive":
     config.keepAlive = true
     config.keepAliveIdle = 99
     configureKeepalive(fd, config)
-    check getIntSockOpt(fd, SOL_SOCKET, SO_KEEPALIVE) == 1
+    check keepaliveEnabled(fd)
     when defined(linux):
       check getIntSockOpt(fd, cint(posix.IPPROTO_TCP), TCP_KEEPIDLE) == 99
     elif defined(macosx):

tests/test_pool_cluster.nim

@@ -475,6 +475,26 @@ suite "Fallback":
       check cluster.replica.idle.len == 1
       check cluster.replica.idle.peekFirst().conn == lateConn
 
+    test "an abandoned replica acquire that later fails does not crash the drain":
+      # The drain must not release a nil connection out of the failed future.
+      let cluster =
+        makeCluster(fallback = fallbackPrimary, fallbackTimeout = milliseconds(20))
+      cluster.replica.active = cluster.replica.config.maxSize
+      cluster.replica.config.acquireTimeout = milliseconds(60)
+
+      let primaryConn = mockConn()
+      cluster.primary.idle.addLast(
+        PooledConn(conn: primaryConn, lastUsedAt: Moment.now())
+      )
+      let (acquired, pool) = waitFor acquireRead(cluster)
+      check acquired == primaryConn
+      check pool == cluster.primary
+
+      # Let the replica acquire hit its own acquireTimeout and the drain run.
+      waitFor sleepMsAsync(150)
+
+      check cluster.replica.waiterCount == 0
+
     test "a drained late connection is closed by the pool, not by the application":
       # The replica pool shuts down mid-acquire. `drainAbandonedAcquire` must
       # reclaim through the pool's own path: a plain `release()` would stamp

tests/test_ssl.nim

@@ -1,7 +1,7 @@
 import std/[unittest, strutils, os]
 
 import cert_fixtures
-from mock_pg_server import buildPreV3Error
+from mock_pg_server import buildPreV3Error, sendSegmentsNow
 import ../async_postgres/[async_backend, pg_bytes, pg_protocol]
 from ../async_postgres/pg_auth import computeTlsServerEndpoint
 
@@ -622,8 +622,10 @@ suite "SSL negotiation - pre-TLS byte injection":
     check securityRefusal
 
   test "split-write injection after 'S' response is rejected (CVE-2021-23214 family)":
-    # Two writes: caught by `socketHasPendingData` or, if they coalesce into
-    # chronos's read, by the `n > 1` path.
+    # Two segments: caught by `socketHasPendingData` or, if they coalesce into
+    # chronos's read, by the `n > 1` path. Both must be sent before the client
+    # reads 'S': an injection arriving after the check is left to fail the TLS
+    # handshake instead (libpq checks the same way).
     var raised = false
     var msgMatches = false
     var securityRefusal = false
@@ -635,8 +637,7 @@ suite "SSL negotiation - pre-TLS byte injection":
         let st = await ms.accept()
         try:
           discard await readN(st, 8) # SSLRequest
-          await sendBytes(st, @[byte('S')])
-          await sendBytes(st, @[byte('X'), byte('Y'), byte('Z')])
+          sendSegmentsNow(st, [@[byte('S')], @[byte('X'), byte('Y'), byte('Z')]])
         except CatchableError:
           discard
         await closeClient(st)

tests/test_tls_error_paths.nim

@@ -405,7 +405,8 @@ suite "TLS handshake failure path":
     # `connect` folds every per-host failure into PgConnectionError, so the type
     # says nothing here; only the wording checked below rules out a leak.
     when hasAsyncDispatch:
-      check "closed by peer" in msg
+      # Closing with our ClientHello unread may send an RST instead of a FIN.
+      check "closed by peer" in msg or "reset by peer" in msg
     elif hasChronos:
       check "TLS handshake failed" in msg