fix: three backportable bug fixes in pg_autoctl (#1127)

* fix: SIGHUP race in reload, multi_async report_lsn timing, drop-primary timeout

Three CI fixes found by running pgaftest:

1. cli_common.c: add signal(SIGHUP, SIG_IGN) before kill() in
   cli_pg_autoctl_reload().  The daemon's supervisor_reload_services()
   broadcasts SIGHUP to all supervised children.  A freshly exec'd
   one-shot command (e.g. 'pg_autoctl set node metadata') can receive
   that SIGHUP via PID reuse in the container's small PID namespace;
   since one-shot commands don't install a SIGHUP handler, the default
   disposition (terminate) kills the command before it returns,
   producing exit code 129 (128+SIGHUP).  SIG_IGN is safe here because
   all callers are one-shot CLI commands.

2. multi_async.pgaf test_011: change 'wait until node4 assigned-state =
   report_lsn' to 'wait until node4 state is report_lsn'.  The old
   check fires as soon as the monitor assigns the goal state, before
   node4 has stopped Postgres and reported its LSN.  Disconnecting node4
   at that point leaves the monitor waiting for a LSN report from a node
   that can no longer talk to it, blocking the entire promotion.
   'state is' waits for the convergence event (reported == goal), so
   node4 has already reported its LSN when we disconnect it.

3. basic_operation.pgaf test_025: increase 'wait until node3 stopped'
   from 30s to 60s.  Dropping the primary requires stopping Postgres
   (including WAL senders to the secondary), writing DROPPED twice to
   the monitor, and supervisor teardown — this regularly exceeds 30s
   in CI but fits comfortably within 60s.

* fix: drop node succeeds when service already cleaned up state file

The service_keeper.c changes on this branch delete the state file when
the keeper exits after completing the DROP protocol (two confirmed
node_active contacts in DROPPED state).  This is correct: it prevents
a restarted process from trying to re-register a dropped node.

However, 'pg_autoctl drop node' (without --no-wait) also calls
keeper_ensure_node_has_been_dropped() AFTER waiting for the running
service to stop.  That function reads the state file to get the node ID
and then queries the monitor.  If the service already exited cleanly and
deleted the state file, the read fails and drop reports EXIT_CODE_MONITOR.

Fix: after keeper_ensure_node_has_been_dropped() fails, check whether
the state file is simply missing and we had a running service (pid != 0).
The absence of the state file in that path is authoritative — the service
only removes it after successfully completing the DROPPED protocol — so
treat the drop as successful.

* fix hba-lan: defer pgSetup init to after config file exists

cli_common_pgsetup_init() calls ProbeConfigurationFileRole() which
FAILs with FATAL when the keeper config file does not yet exist
(node still initializing when setup block runs in parallel).

Rewrite keeper_cli_pgsetup_hba_lan() to:
- use keeperOptions.pgSetup.pgdata directly (no config file needed)
- wait up to 60 s for pg_hba.conf to appear before writing rules
- determine hostname from: keeper config → gethostname() → pghost
- use &keeperOptions.pgSetup for pg_setup_wait_until_is_ready()
- use keeperOptions.pgSetup.pg_ctl for pg_ctl_reload()

gethostname() inside Docker returns the container service name
(e.g. "node2") which resolves correctly on the Docker LAN network,
covering the race window before the keeper config file is written.

Add #include <unistd.h> for gethostname() on Linux.

* style: fix citus_indent formatting in cli_do_misc.c

* fix: add missing includes and functions for hba-lan cherry-pick

The cherry-pick of the hba-lan fix introduced calls to three symbols that
did not exist on main:

- pghba_ensure_ident_map_entry (pghba.c/h): appends a missing entry to
  pg_ident.conf; used by cert auth to map the autoctl_node CN to the
  pgautofailover_replicator role.
- pg_ctl_reload (pgctl.c/h): runs pg_ctl reload -D <pgdata>.
- pghba.h, pgsetup.h, pgsql.h: missing includes in cli_do_misc.c.

All implementations ported from pgaftest-infra:src/bin/pg_autoctl/ and
src/bin/common/.

Author: dimitri

src/bin/pg_autoctl/cli_common.c

@@ -1744,6 +1744,18 @@ cli_pg_autoctl_reload(const char *pidfile)
 {
 	pid_t pid;
 
+	/*
+	 * Ignore SIGHUP in this process before sending it to the daemon.  The
+	 * daemon's supervisor_reload_services() broadcasts SIGHUP to all its
+	 * service children.  A freshly exec'd one-shot command can share a PID
+	 * with a recently-exited service (PID reuse), and since one-shot
+	 * commands don't install a SIGHUP handler, the default disposition
+	 * (terminate) would kill us before we even return.  SIG_IGN is safe
+	 * here because all callers are one-shot CLI commands that exit shortly
+	 * after this call anyway.
+	 */
+	signal(SIGHUP, SIG_IGN);
+
 	if (read_pidfile(pidfile, &pid))
 	{
 		if (pid <= 0)

src/bin/pg_autoctl/cli_do_misc.c

@@ -12,6 +12,7 @@
 #include <getopt.h>
 #include <inttypes.h>
 #include <signal.h>
+#include <unistd.h>
 
 #include "postgres_fe.h"
 #include "pqexpbuffer.h"
@@ -30,6 +31,9 @@
 #include "monitor.h"
 #include "monitor_config.h"
 #include "pgctl.h"
+#include "pghba.h"
+#include "pgsetup.h"
+#include "pgsql.h"
 #include "pgtuning.h"
 #include "primary_standby.h"
 #include "string_utils.h"
@@ -393,6 +397,169 @@ keeper_cli_pgsetup_tune(int argc, char **argv)
 }
 
 
+/*
+ * keeper_cli_pgsetup_hba_lan appends LAN CIDR trust rules to pg_hba.conf
+ * without connecting to Postgres (safe when TCP HBA hasn't been set up yet),
+ * then reloads the configuration via pg_ctl.
+ *
+ * Intended for --skip-pg-hba setups where the operator needs to seed the
+ * initial trust rules before pg_autoctl or replication can connect via TCP.
+ *
+ * Sequence:
+ *   1. Wait up to 60 s for $PGDATA/pg_hba.conf to appear.
+ *   2. Append "host all all <LAN-CIDR> trust" and
+ *      "host replication all <LAN-CIDR> trust" directly to the file.
+ *   3. Wait for Postgres to be running (reads postmaster.pid, no TCP).
+ *   4. pg_ctl reload -D $PGDATA.
+ */
+void
+keeper_cli_pgsetup_hba_lan(int argc, char **argv)
+{
+	/*
+	 * Derive pgdata from command-line options directly, without calling
+	 * cli_common_pgsetup_init().  cli_common_pgsetup_init() calls
+	 * ProbeConfigurationFileRole() which fatals when the keeper config file
+	 * does not exist yet (e.g. node still initializing).  We do not need the
+	 * full pgSetup here — only pgdata, hostname, auth method, and ssl.
+	 */
+	const char *pgdata = keeperOptions.pgSetup.pgdata;
+
+	if (IS_EMPTY_STRING_BUFFER(pgdata))
+	{
+		log_fatal("Please provide --pgdata or set PGDATA");
+		exit(EXIT_CODE_BAD_ARGS);
+	}
+
+	char hbaFile[MAXPGPATH];
+	sformat(hbaFile, MAXPGPATH, "%s/pg_hba.conf", pgdata);
+
+	/* Step 1: wait for pg_hba.conf to appear (up to 60 s) */
+	{
+		int timeout = 60;
+		time_t deadline = time(NULL) + timeout;
+		while (!file_exists(hbaFile) && time(NULL) < deadline)
+		{
+			log_debug("Waiting for \"%s\" to appear", hbaFile);
+			pg_usleep(500 * 1000);
+		}
+		if (!file_exists(hbaFile))
+		{
+			log_error("Timed out waiting for \"%s\" to appear", hbaFile);
+			exit(EXIT_CODE_PGCTL);
+		}
+	}
+
+	/*
+	 * Step 2: determine the hostname for LAN CIDR lookups.
+	 *
+	 * Preference order:
+	 *   a) keeper config file hostname (written early during pg_autoctl init)
+	 *   b) OS hostname via gethostname() — inside a Docker container this is
+	 *      the service name (e.g. "node2") which resolves correctly on the LAN
+	 *   c) keeperOptions.pgSetup.pghost (last resort; may be a Unix socket
+	 *      path like /var/run/postgresql that does not resolve as a hostname)
+	 *
+	 * The keeper config file is created before postgres initialises, so it is
+	 * normally available by the time pg_hba.conf appears.  All three options
+	 * are tried so the command works even when called very early.
+	 */
+	char hostname[_POSIX_HOST_NAME_MAX] = "";
+
+	/* option a: keeper config */
+	{
+		KeeperConfig hbaConfig = keeperOptions;
+		if (keeper_config_set_pathnames_from_pgdata(&hbaConfig.pathnames,
+													pgdata) &&
+			keeper_config_read_file(&hbaConfig,
+									false /* missingPgdataIsOk */,
+									true /* pgIsNotRunningIsOk */,
+									true /* monitorDisabledIsOk */) &&
+			!IS_EMPTY_STRING_BUFFER(hbaConfig.hostname))
+		{
+			strlcpy(hostname, hbaConfig.hostname, sizeof(hostname));
+		}
+	}
+
+	/* option b: OS hostname */
+	if (IS_EMPTY_STRING_BUFFER(hostname))
+	{
+		if (gethostname(hostname, sizeof(hostname)) == 0)
+		{
+			log_debug("hba-lan: using OS hostname \"%s\"", hostname);
+		}
+		else
+		{
+			hostname[0] = '\0';
+		}
+	}
+
+	/* option c: pghost (may be a socket path — last resort) */
+	if (IS_EMPTY_STRING_BUFFER(hostname))
+	{
+		strlcpy(hostname, keeperOptions.pgSetup.pghost, sizeof(hostname));
+	}
+
+	/* --auth <method> defaults to "trust"; --ssl enables hostssl rules */
+	const char *authMethod = keeperOptions.pgSetup.authMethod;
+	if (IS_EMPTY_STRING_BUFFER(authMethod))
+	{
+		authMethod = "trust";
+	}
+	bool useSSL = keeperOptions.pgSetup.ssl.active;
+
+	/* cert auth for replication needs an ident map */
+	bool isCert = (strcmp(authMethod, "cert") == 0);
+	const char *replAuth = isCert ? "cert map=pgautofailover" : authMethod;
+
+	if (!pghba_enable_lan_cidr(NULL, useSSL,
+							   HBA_DATABASE_ALL, NULL,
+							   hostname, NULL, authMethod,
+							   HBA_EDIT_MINIMAL,
+							   pgdata))
+	{
+		log_error("Failed to add LAN CIDR HBA rule for \"all\" databases");
+		exit(EXIT_CODE_PGCTL);
+	}
+
+	if (!pghba_enable_lan_cidr(NULL, useSSL,
+							   HBA_DATABASE_REPLICATION, NULL,
+							   hostname, NULL, replAuth,
+							   HBA_EDIT_MINIMAL,
+							   pgdata))
+	{
+		log_error("Failed to add LAN CIDR HBA rule for replication");
+		exit(EXIT_CODE_PGCTL);
+	}
+
+	/* cert auth requires an ident map entry in pg_ident.conf */
+	if (isCert)
+	{
+		if (!pghba_ensure_ident_map_entry(pgdata,
+										  "pgautofailover",
+										  PG_AUTOCTL_MONITOR_USERNAME,
+										  PG_AUTOCTL_REPLICA_USERNAME))
+		{
+			log_error("Failed to add cert ident map entry to pg_ident.conf");
+			exit(EXIT_CODE_PGCTL);
+		}
+	}
+
+	/* Step 3: wait for Postgres to be running (PID file, no TCP needed) */
+	if (!pg_setup_wait_until_is_ready(&keeperOptions.pgSetup, 60, LOG_INFO))
+	{
+		log_error("Postgres did not become ready within 60 s");
+		exit(EXIT_CODE_PGCTL);
+	}
+
+	/* Step 4: reload so the new HBA rules take effect */
+	log_info("Reloading Postgres configuration in \"%s\"", pgdata);
+	if (!pg_ctl_reload(keeperOptions.pgSetup.pg_ctl, pgdata))
+	{
+		exit(EXIT_CODE_PGCTL);
+	}
+}
+
+
 /*
  * keeper_cli_init_standby initializes a standby
  */

src/bin/pg_autoctl/cli_drop_node.c

@@ -39,6 +39,7 @@
 #include "service_monitor.h"
 #include "service_monitor_init.h"
 #include "signals.h"
+#include "file_utils.h"
 #include "string_utils.h"
 
 /*
@@ -680,6 +681,22 @@ cli_drop_local_node(KeeperConfig *config, bool dropAndDestroy)
 				 config->formation,
 				 config->groupId);
 	}
+	else if (pid != 0 && !file_exists(config->pathnames.state))
+	{
+		/*
+		 * The running service stopped and deleted its own state file.  The
+		 * service only removes the state file after completing the DROPPED
+		 * protocol (two confirmed monitor contacts in DROPPED state), so the
+		 * absence of the state file is authoritative: the drop succeeded.
+		 */
+		log_info("This node with id %lld in formation \"%s\" and group %d "
+				 "has been dropped from the monitor "
+				 "(confirmed by service clean-up)",
+				 (long long) keeperState->current_node_id,
+				 config->formation,
+				 config->groupId);
+		dropped = true;
+	}
 	else
 	{
 		log_fatal("Failed to ensure that the local node with id %lld "

src/bin/pg_autoctl/pgctl.c

@@ -2025,6 +2025,32 @@ pg_ctl_status(const char *pg_ctl, const char *pgdata, bool log_output)
 }
 
 
+/*
+ * pg_ctl_reload reloads PostgreSQL configuration by running "pg_ctl reload".
+ */
+bool
+pg_ctl_reload(const char *pg_ctl, const char *pgdata)
+{
+	Program program = run_program(pg_ctl, "-D", pgdata, "reload", NULL);
+	int returnCode = program.returnCode;
+
+	if (program.stdErr != NULL)
+	{
+		log_debug("%s", program.stdErr);
+	}
+
+	free_program(&program);
+
+	if (returnCode != 0)
+	{
+		log_error("pg_ctl reload -D %s failed (exit %d)", pgdata, returnCode);
+		return false;
+	}
+
+	return true;
+}
+
+
 /*
  * pg_ctl_promote promotes a standby by running "pg_ctl promote"
  */

src/bin/pg_autoctl/pgctl.h

@@ -57,6 +57,7 @@ bool pg_ctl_postgres(const char *pg_ctl, const char *pgdata, int pgport,
 bool pg_log_startup(const char *pgdata, int logLevel);
 bool pg_log_recovery_setup(const char *pgdata, int logLevel);
 bool pg_ctl_stop(const char *pg_ctl, const char *pgdata);
+bool pg_ctl_reload(const char *pg_ctl, const char *pgdata);
 int pg_ctl_status(const char *pg_ctl, const char *pgdata, bool log_output);
 bool pg_ctl_promote(const char *pg_ctl, const char *pgdata);
 

src/bin/pg_autoctl/pghba.c

@@ -700,6 +700,68 @@ pghba_enable_lan_cidr(PGSQL *pgsql,
 }
 
 
+/*
+ * pghba_ensure_ident_map_entry ensures that pg_ident.conf contains the given
+ * ident map entry, appending it if missing.  Used with cert auth so that the
+ * autoctl_node client certificate CN maps to the pgautofailover_replicator
+ * PostgreSQL role for replication connections.
+ */
+bool
+pghba_ensure_ident_map_entry(const char *pgdata,
+							 const char *mapName,
+							 const char *systemUser,
+							 const char *pgUser)
+{
+	char identFilePath[MAXPGPATH];
+	char *contents = NULL;
+	long size = 0L;
+
+	sformat(identFilePath, MAXPGPATH, "%s/pg_ident.conf", pgdata);
+
+	if (!read_file(identFilePath, &contents, &size))
+	{
+		log_error("Failed to read \"%s\"", identFilePath);
+		return false;
+	}
+
+	/* check whether this exact entry already exists */
+	PQExpBuffer needle = createPQExpBuffer();
+	if (!needle)
+	{
+		free(contents);
+		log_error("Failed to allocate memory");
+		return false;
+	}
+	appendPQExpBuffer(needle, "%s %s %s", mapName, systemUser, pgUser);
+
+	if (strstr(contents, needle->data) != NULL)
+	{
+		/* already present */
+		destroyPQExpBuffer(needle);
+		free(contents);
+		return true;
+	}
+	destroyPQExpBuffer(needle);
+
+	/* append the new entry */
+	FILE *f = fopen(identFilePath, "a"); /* IGNORE-BANNED */
+	if (!f)
+	{
+		log_error("Failed to open \"%s\" for appending: %m", identFilePath);
+		free(contents);
+		return false;
+	}
+	fformat(f, "%s %s %s%s\n",
+			mapName, systemUser, pgUser, HBA_LINE_COMMENT);
+	fclose(f);
+	free(contents);
+
+	log_info("Added ident map entry \"%s %s %s\" to \"%s\"",
+			 mapName, systemUser, pgUser, identFilePath);
+	return true;
+}
+
+
 /*
  * hba_check_hostname returns true when the DNS setting looks compatible with
  * Postgres expectations for an HBA hostname entry.

src/bin/pg_autoctl/pghba.h

@@ -49,6 +49,11 @@ bool pghba_enable_lan_cidr(PGSQL *pgsql,
 						   HBAEditLevel hbaLevel,
 						   const char *pgdata);
 
+bool pghba_ensure_ident_map_entry(const char *pgdata,
+								  const char *mapName,
+								  const char *systemUser,
+								  const char *pgUser);
+
 bool pghba_check_hostname(const char *hostname, char *ipaddr, size_t size,
 						  bool *useHostname);