diff --git a/Makefile.docker b/Makefile.docker index d8fb42372..1d1c90aaf 100644 --- a/Makefile.docker +++ b/Makefile.docker @@ -34,7 +34,17 @@ BASE ?= ghcr.io/hapostgres/pg_auto_failover/pgaf-base:bookworm CONTAINER_NAME = pg_auto_failover TEST_CONTAINER_NAME = pg_auto_failover_test -DOCKER_RUN_OPTS = --privileged --rm + +# --init runs Docker's built-in tini as PID 1 instead of the test's own +# `make` invocation. Without it, `make` (which installs its own SIGHUP +# handler to clean up partial targets on interrupt) is PID 1 itself, which +# strips away the kernel's usual "PID 1 ignores signals with no explicit +# handler" protection -- any stray SIGHUP reaching the container then kills +# the whole test run outright ("make: *** [Makefile:67: test] Hangup"), +# with no resilience at all. Seen recurring on pytest/single (PG19) across +# unrelated PRs; tini reaping/forwarding signals properly as a real PID 1 +# is the standard fix for this class of Docker footgun. +DOCKER_RUN_OPTS = --init --privileged --rm # DOCKER BUILDS # diff --git a/src/monitor/health_check_worker.c b/src/monitor/health_check_worker.c index 34be3895e..3034e379a 100644 --- a/src/monitor/health_check_worker.c +++ b/src/monitor/health_check_worker.c @@ -132,6 +132,7 @@ static List * CreateHealthChecks(List *nodeHealthList); static HealthCheck * CreateHealthCheck(NodeHealth *nodeHealth); static void DoHealthChecks(List *healthCheckList); static void ManageHealthCheck(HealthCheck *healthCheck, struct timeval currentTime); +static void FinishHealthCheckConnection(PGconn *connection); static int WaitForEvent(List *healthCheckList); static int CompareTimes(struct timeval *leftTime, struct timeval *rightTime); static int SubtractTimes(struct timeval base, struct timeval subtract); @@ -923,7 +924,7 @@ ManageHealthCheck(HealthCheck *healthCheck, struct timeval currentTime) { struct timeval nextTryTime = { 0, 0 }; - PQfinish(connection); + FinishHealthCheckConnection(connection); nextTryTime = AddTimeMillis(currentTime, HealthCheckRetryDelay); @@ -943,7 +944,7 @@ ManageHealthCheck(HealthCheck *healthCheck, struct timeval currentTime) if (pollingStatus == PGRES_POLLING_OK) { - PQfinish(connection); + FinishHealthCheckConnection(connection); SetNodeHealthState(healthCheck->node->nodeId, healthCheck->node->nodeName, @@ -960,7 +961,7 @@ ManageHealthCheck(HealthCheck *healthCheck, struct timeval currentTime) { struct timeval nextTryTime = { 0, 0 }; - PQfinish(connection); + FinishHealthCheckConnection(connection); nextTryTime = AddTimeMillis(currentTime, HealthCheckRetryDelay); @@ -988,6 +989,35 @@ ManageHealthCheck(HealthCheck *healthCheck, struct timeval currentTime) } +/* + * FinishHealthCheckConnection closes a health check connection. + * + * By the time we decide to close a health check connection, the target + * Postgres server may have already sent us a few bytes we haven't read yet + * (e.g. trailing ParameterStatus/BackendKeyData/ReadyForQuery messages that + * arrived in a TCP segment separate from the one that got us to our current + * polling status). Closing a socket that still has unread data sitting in + * its receive buffer makes the kernel send the peer an RST instead of a + * plain FIN, which the target then logs as "could not receive data from + * client: Connection reset by peer" -- confusing operators into thinking + * something is actually wrong (see issue #916), even though the health + * check itself completed (or failed) exactly as expected. + * + * A single non-blocking PQconsumeInput() call drains whatever is currently + * sitting in the socket's receive buffer into libpq's own memory before we + * close it, which is enough to avoid the spurious RST in the common case. + * It never blocks: on a non-blocking connection it just reads whatever the + * kernel already has buffered and returns immediately either way. + */ +static void +FinishHealthCheckConnection(PGconn *connection) +{ + (void) PQconsumeInput(connection); + + PQfinish(connection); +} + + /* * CompareTime compares two timeval structs. *