From 6005d67c875539e301f8659ff42adce31cc55c66 Mon Sep 17 00:00:00 2001
From: "David E. Wheeler" <david@justatheory.com>
Date: Fri, 16 Feb 2024 17:16:27 -0500
Subject: [PATCH] Fix PID

---
 Changes                      |  4 ++++
 lib/PGXN/Manager/Consumer.pm | 12 +++++------
 t/consumer.t                 | 40 +++++++++++++++++++++---------------
 3 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/Changes b/Changes
index b30445c..edb0fa0 100644
--- a/Changes
+++ b/Changes
@@ -8,6 +8,10 @@ Revision history for Perl extension PGXN::Manager
        - Fixed invalid license example in the META spec.
        - Added a logger to the Consumer and the Mastodon and Twitter handlers,
          so that they now log debug and info messages about what's being sent.
+       - Moved PID file cleanup from the `DEMOLISH` method to the `run` method,
+         where it should always execute. This will hopefully fix the issue
+         where the consumer mysteriously ceases running and doesn't remove its
+         PID file, so never restarts.
 
 0.31.1   2023-10-07T21:40:53Z
        - Restored the writing of the pgxn_consumer PID file, accidentally
diff --git a/lib/PGXN/Manager/Consumer.pm b/lib/PGXN/Manager/Consumer.pm
index 017ce51..f435f73 100644
--- a/lib/PGXN/Manager/Consumer.pm
+++ b/lib/PGXN/Manager/Consumer.pm
@@ -89,13 +89,6 @@ sub _signal_handler {
     };
 }
 
-sub DEMOLISH {
-    my $self = shift;
-    if (my $path = $self->pid_file) {
-        unlink $path;
-    }
-}
-
 sub run {
     my $self = shift;
     $self->log(INFO => sprintf "Starting %s %s", __PACKAGE__, __PACKAGE__->VERSION);
@@ -114,6 +107,11 @@ sub run {
         sleep($self->interval);
     }
 
+    if (my $path = $self->pid_file) {
+        $self->log(DEBUG => "Unlinking PID file ", $self->pid_file);
+        unlink $path;
+    }
+
     $self->log(INFO => 'Shutting down');
     return 0;
 }
diff --git a/t/consumer.t b/t/consumer.t
index 2cbadbd..0f0d7ee 100644
--- a/t/consumer.t
+++ b/t/consumer.t
@@ -109,19 +109,6 @@ LOGFILE: {
 
 my $chans = join(', ', PGXN::Manager::Consumer::CHANNELS);
 
-##############################################################################
-# Test pid file cleanup.
-PID: {
-    my $tmp = File::Temp->new(UNLINK => 0);
-    my $fn = $tmp->filename;
-    do {
-        my $consumer = $CLASS->new(logger => $logger, pid_file => $fn);
-        file_exists_ok $fn, 'PID file should exist';
-    };
-    file_not_exists_ok $fn, 'PID file be gone';
-    is output(), '', 'Should have no output';
-}
-
 ##############################################################################
 # Load the test environment configuration.
 sub load_config {
@@ -327,22 +314,41 @@ RUN: {
     $db_mocker->mock(do => sub { shift; push @done => \@_ });
     my $exp_done = [map { ["LISTEN pgxn_$_"] } PGXN::Manager::Consumer::CHANNELS ];
 
-    # Run it.
+    # Set up a PID file.
+    my $tmp = File::Temp->new(UNLINK => 0);
+    my $fn = $tmp->filename;
+
+    # Set up the config.
     local @ARGV = qw(--env test --interval 0);
     my $cfg = $CLASS->_config;
     $cfg->{logger} = $logger;
+    $cfg->{pid_file} = $fn;
+
+    # Instantiate.
     my $consumer = $CLASS->new($cfg);
     is $consumer->interval, 0, 'Should have interval 0';
     is $consumer->continue, 1, 'Should have default continue 1';
     is $consumer->verbose, 0, 'Should have default verbose 0';
     is $consumer->logger, $logger, 'Should have set logger';
+    is $consumer->pid_file, $fn, 'Should have set pid_file';
+    file_exists_ok $fn, 'PID file should exist';
+
+    # Run it.
+    $logger->{verbose} = 2;
     is $consumer->run, 0, 'Run consumer';
+    file_not_exists_ok $fn, 'PID file should no longer exist';
+    $logger->{verbose} = 1;
 
     is_deeply output(), join("\n",
         "$logtime - INFO: Starting $CLASS " . $CLASS->VERSION,
+        "$logtime - DEBUG: Loading PGXN::Manager::Consumer::mastodon",
+        "$logtime - DEBUG: Configuring PGXN::Manager::Consumer::mastodon for release",
+        "$logtime - DEBUG: Loading PGXN::Manager::Consumer::twitter",
+        "$logtime - DEBUG: Configuring PGXN::Manager::Consumer::twitter for release",
+        "$logtime - DEBUG: Unlinking PID file $fn",
         "$logtime - INFO: Shutting down",
         '',
-    ), 'Should have startup and shutdown log entries';
+    ), 'Should have startup, loading, PID, and shutdown log entries';
     is_deeply \@done, $exp_done, 'Should have listened to all channels';
     ok $consumer->conn->dbh->{Callbacks}{connected},
         'Should have configured listening in connected callback';
@@ -412,7 +418,7 @@ CONSUME: {
     # Set up a notification.
     my $json1 = '{"name": "Julie ❤️"}';
     my $payload1 = {name => 'Julie ❤️'};
-    my $consumer = $new_consumer->(verbose => 2, logger => $logger);
+    my $consumer = $new_consumer->(verbose => 2);
     $consumer->conn->run(sub {
         $_->do("SELECT pg_notify(?, ?)", undef, 'pgxn_release', $json1);
     });
@@ -456,7 +462,7 @@ CONSUME: {
     my $json3 = '{"drop": "out"}';
     my $payload3 = {drop => 'out'};
     $logger->{verbose} = 0;
-    $consumer = $new_consumer->(verbose => 0, logger => $logger);
+    $consumer = $new_consumer->(verbose => 0);
     $consumer->conn->run(sub {
         $_->do("SELECT pg_notify(?, ?)", undef, 'pgxn_drop', $json3);
     });