From 83bcdc8a5b1057dff0861c15f7e2ec1611ef4809 Mon Sep 17 00:00:00 2001 From: Shuu Date: Mon, 28 Sep 2026 23:42:42 +0900 Subject: [PATCH 1/3] ci: add macOS to the test matrix and split unit tests from integration tests --- .github/workflows/test.yml | 22 ++++++++++++++++++---- async_postgres.nimble | 15 +++++++++++---- async_postgres/async_backend.nim | 5 ++++- async_postgres/pg_pool_cluster.nim | 16 +++++++++++----- tests/all_tests.nim | 17 ++--------------- tests/all_tests_integration.nim | 8 ++++++++ tests/all_tests_unit.nim | 15 +++++++++++++++ tests/test_keepalive.nim | 16 ++++++++++------ tests/test_pool_cluster.nim | 20 ++++++++++++++++++++ tests/test_tls_error_paths.nim | 3 ++- 10 files changed, 101 insertions(+), 36 deletions(-) create mode 100644 tests/all_tests_integration.nim create mode 100644 tests/all_tests_unit.nim diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1da31813..892f652f 100644 --- a/.github/workflows/test.yml +++ b/.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] diff --git a/async_postgres.nimble b/async_postgres.nimble index 1ebeecaf..b1c34c9f 100644 --- a/async_postgres.nimble +++ b/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" diff --git a/async_postgres/async_backend.nim b/async_postgres/async_backend.nim index c010ef00..853a3da0 100644 --- a/async_postgres/async_backend.nim +++ b/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. diff --git a/async_postgres/pg_pool_cluster.nim b/async_postgres/pg_pool_cluster.nim index c2654a12..75c117e2 100644 --- a/async_postgres/pg_pool_cluster.nim +++ b/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) diff --git a/tests/all_tests.nim b/tests/all_tests.nim index 868df462..3db8a44a 100644 --- a/tests/all_tests.nim +++ b/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.} diff --git a/tests/all_tests_integration.nim b/tests/all_tests_integration.nim new file mode 100644 index 00000000..03a29d64 --- /dev/null +++ b/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.} diff --git a/tests/all_tests_unit.nim b/tests/all_tests_unit.nim new file mode 100644 index 00000000..d7a374d1 --- /dev/null +++ b/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.} diff --git a/tests/test_keepalive.nim b/tests/test_keepalive.nim index 4e368479..0dccfca6 100644 --- a/tests/test_keepalive.nim +++ b/tests/test_keepalive.nim @@ -17,6 +17,10 @@ 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: @@ -24,7 +28,7 @@ suite "configureKeepalive": 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): diff --git a/tests/test_pool_cluster.nim b/tests/test_pool_cluster.nim index e3987ca2..94dc7d42 100644 --- a/tests/test_pool_cluster.nim +++ b/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 diff --git a/tests/test_tls_error_paths.nim b/tests/test_tls_error_paths.nim index cb6c8ad9..79683759 100644 --- a/tests/test_tls_error_paths.nim +++ b/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 From 8de11ed74817068d9b3ab3efc6301b402c84e618 Mon Sep 17 00:00:00 2001 From: Shuu Date: Tue, 29 Sep 2026 00:19:06 +0900 Subject: [PATCH 2/3] fix --- async_postgres/pg_connection/buffer_io.nim | 5 +++-- async_postgres/pg_connection/lifecycle.nim | 12 ++++-------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/async_postgres/pg_connection/buffer_io.nim b/async_postgres/pg_connection/buffer_io.nim index 497f2ad0..c12e96de 100644 --- a/async_postgres/pg_connection/buffer_io.nim +++ b/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, diff --git a/async_postgres/pg_connection/lifecycle.nim b/async_postgres/pg_connection/lifecycle.nim index 6c0d03c4..4631da0d 100644 --- a/async_postgres/pg_connection/lifecycle.nim +++ b/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 From fd72ae77446a76a5dc7aa9f1288cd4d396de9e47 Mon Sep 17 00:00:00 2001 From: Shuu Date: Tue, 29 Sep 2026 00:47:15 +0900 Subject: [PATCH 3/3] fix --- tests/mock_pg_server.nim | 25 +++++++++++++++++++++++++ tests/test_ssl.nim | 11 ++++++----- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/tests/mock_pg_server.nim b/tests/mock_pg_server.nim index 87dcb8ff..4c9b8c9c 100644 --- a/tests/mock_pg_server.nim +++ b/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] = diff --git a/tests/test_ssl.nim b/tests/test_ssl.nim index 86598087..f09c8439 100644 --- a/tests/test_ssl.nim +++ b/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)