Skip to content

monitor: drain health check socket before closing to avoid spurious RST (#916) + fix pytest/single Hangup flake - #1179

Merged
dimitri merged 2 commits into
mainfrom
fix/916-healthcheck-connection-reset
Jul 29, 2026
Merged

monitor: drain health check socket before closing to avoid spurious RST (#916) + fix pytest/single Hangup flake#1179
dimitri merged 2 commits into
mainfrom
fix/916-healthcheck-connection-reset

Conversation

@dimitri

@dimitri dimitri commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Background

Fixes #916, "Connection reset by peer in log on datanodes". The reporter's
data node Postgres logs were filled with:

LOG:  could not receive data from client: Connection reset by peer

attributed to pgautofailover_monitor connections (i.e. the monitor's
per-node health check "ping"). Functionally harmless -- failover/switchover
kept working -- but noisy enough to slow down a production rollout decision.
The issue was closed with no comments and no linked commit/PR; static
analysis confirms the underlying code was still present, unchanged, as of
main.

Root cause

src/monitor/health_check_worker.c's ManageHealthCheck() state machine
closes each per-node health check connection with a bare PQfinish() as
soon as it reaches PGRES_POLLING_OK (success), PGRES_POLLING_FAILED, or
a connect timeout. By that point the target server may have already sent a
few trailing bytes we haven't read yet -- extra ParameterStatus/
BackendKeyData/ReadyForQuery messages arriving in a TCP segment separate
from the one that got us to our current polling status.

Per standard POSIX socket semantics, close() on a socket that still has
unread data sitting in its receive buffer sends the peer an RST instead of
a plain FIN. The target then logs exactly the message from #916. This is
timing-dependent (only shows up when a trailing segment lands in the race
window between libpq's last read and our PQfinish()), which explains the
non-deterministic "almost every minute" pattern, and is far more likely
across a real network hop than over loopback -- matching the reporter's
setup (two separate VirtualBox VMs).

Fix

Add FinishHealthCheckConnection(), used at all three PQfinish() call
sites for a connection that reached at least HEALTH_CHECK_CONNECTING
(the immediate PQconnectStart()-failed branch is left alone -- no
protocol exchange ever happens there, nothing to drain). It does a single
non-blocking PQconsumeInput() call before PQfinish(), which drains
whatever is currently sitting in the kernel's receive buffer into libpq's
own memory, avoiding the spurious RST in the common case.

This is deliberately the minimal fix (drain-then-close), not a full
graceful shutdown()-and-wait-for-EOF: PQconsumeInput() never blocks on
a non-blocking connection (it reads whatever's already buffered and returns
either way), so it has no effect on how many nodes a single health-check
worker can service concurrently
-- the existing poll()-driven state
machine (one worker per database, checking every node in that formation via
a single poll() call per round) is completely untouched. A full graceful
close would add a per-node round-trip and need its own bounded timeout
(nodes failing health checks are exactly the ones least likely to close
cleanly); that tradeoff didn't seem worth it for what is, after all, a
cosmetic log message.

Also in this PR: pytest / single (PG19) Hangup flake

This PR's own first CI run hit pytest / single (PG19) failing with
make: *** [Makefile:67: test] Hangup a few tests into an unrelated 88-item
run -- the exact same pattern already called out as CI-runner
infrastructure flakiness in #1177's body ("killed by a Hangup signal 2
tests into an 88-test run, with no error of its own"). Recurring on a
second, otherwise-unrelated PR (this one touches only
health_check_worker.c) while every other job -- including every
pytest / monitor job across PG14-19 and every pgaftest suite, i.e.
exactly the jobs that actually exercise the health-check code above --
passed cleanly, made it worth a closer look rather than just re-running and
moving on.

Root cause: tests/Makefile's docker run for these jobs has no init
process, so make itself is the container's PID 1. GNU Make installs an
explicit SIGHUP handler (to clean up partial targets on interrupt), which
strips away the kernel's usual "PID 1 ignores signals with no explicit
handler" protection for that signal -- any stray SIGHUP reaching the
container kills the whole test run outright, with no resilience at all.
This is a well-known Docker footgun class (raw workload as PID 1, no
reaper/signal-forwarding init).

Fix: added --init to DOCKER_RUN_OPTS in Makefile.docker, the single
shared option list both run-test and run-test-prebuilt in
tests/Makefile already use. Docker's built-in tini becomes the
container's actual PID 1, correctly reaping zombies and forwarding signals,
with make demoted to an ordinary child under normal (non-PID-1) signal
semantics. Confirmed the flag threads through correctly (make -n run-test-prebuilt) and that this runner's Docker supports --init
(docker run --rm --init hello-world).

This is test/CI infrastructure, not application code, and it's what
surfaced the whole investigation in the first place on this branch --
folding it in here rather than opening a separate PR for it.

Verification

  • make docker-check (citus_indent via citus/stylechecker:no-py): clean
    on the changed file.
  • ci/banned.h.sh: clean.
  • make -C src/monitor (bare build, -Wall -Werror): clean, no warnings.
  • make installcheck PGVERSION=17 (Docker-based, matches CI): all 14 SQL
    regression tests and all 6 isolation tests pass.
  • pgaftest run tests/tap/specs/basic_operation.pgaf: 28/28, including
    maintenance-mode, multiple manual failovers, and the 86s
    test_022_detect_network_partition scenario -- confirms no functional
    regression in health-check-driven node marking or failover behavior.
  • Brought up the same cluster interactively (pgaftest cluster setup) and
    captured full container logs (monitor + all 3 data nodes) over 3+ minutes
    of uptime (~36 health-check cycles per node at the default 5s period):
    zero occurrences of "Connection reset by peer" or "could not receive data
    from client" anywhere in the captured logs.
  • --init: verified locally that it threads through the docker run
    invocation and that Docker on this machine supports it. The actual test
    for whether it resolves the Hangup flake is this PR's own subsequent CI
    runs, since the failure mode is a CI-runner-delivered signal I can't
    reproduce on demand locally.

Caveat: the health-check drain fix addresses a timing-dependent race
that was originally reported over a real network between separate VMs; a
few minutes on a local Docker bridge network is reassuring (no regression,
no new errors) but isn't a guarantee the race can never occur elsewhere --
draining once closes the specific window described above, not every
conceivable timing variant.

The health check background worker (src/monitor/health_check_worker.c)
closes each per-node connection with a bare PQfinish() as soon as it
reaches PGRES_POLLING_OK (or fails/times out). By that point the target
Postgres server may have already sent a few trailing bytes (extra
ParameterStatus/BackendKeyData/ReadyForQuery messages arriving in a
separate TCP segment) that we haven't read yet. Closing a socket with
unread data still sitting in its receive buffer makes the kernel send
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 wrong, even though the
health check itself succeeded (or failed) exactly as expected. This is
what issue #916 reports; the underlying code was unchanged as of this
commit despite the issue being closed.

Add FinishHealthCheckConnection(), used at all three PQfinish() call
sites for a connection that reached at least PGRES_POLLING_CONNECTING:
a single non-blocking PQconsumeInput() call drains whatever is
currently sitting in the kernel's receive buffer into libpq's own
memory before PQfinish() closes the socket. This never blocks (a
non-blocking connection's PQconsumeInput() just reads what's already
buffered and returns either way), so it has no effect on how many
nodes a single health-check worker can service concurrently -- the
existing poll()-driven state machine is untouched.

Fixes #916
@dimitri dimitri self-assigned this Jul 29, 2026
@dimitri dimitri added the bug Something isn't working label Jul 29, 2026
…p flake

tests/Makefile's docker run for pytest jobs has no init process: make
itself is PID 1 inside the container. GNU Make installs an explicit
SIGHUP handler (to clean up partial targets on interrupt), which
strips away the kernel's usual "PID 1 ignores signals with no
explicit handler" protection for that signal -- any stray SIGHUP
reaching the container kills the whole test run outright with no
resilience:

  make: *** [Makefile:67: test] Hangup

Seen recurring on pytest / single (PG19) across unrelated PRs (#1177's
batch, and again on this branch before this commit), always the same
job, never a real test assertion failing -- consistent with this being
a PID-1 signal-handling gap rather than anything in the test's own
logic.

Add --init to DOCKER_RUN_OPTS (Makefile.docker), the single shared
option list both run-test and run-test-prebuilt already use. Docker's
built-in tini becomes the container's actual PID 1, correctly reaping
and forwarding signals, with make demoted to an ordinary child under
normal (non-PID-1) signal semantics.
@dimitri dimitri changed the title monitor: drain health check socket before closing to avoid spurious RST (#916) monitor: drain health check socket before closing to avoid spurious RST (#916) + fix pytest/single Hangup flake Jul 29, 2026
@dimitri
dimitri merged commit 869f2b2 into main Jul 29, 2026
73 checks passed
@dimitri
dimitri deleted the fix/916-healthcheck-connection-reset branch July 29, 2026 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Connection reset by peer in log on datanodes

1 participant