Fix failed DNF update checks showing no updates

This PR fixes the Package Updates module reporting "no updates" when dnf check-update fails.

DNF prints its error to stderr and exits with status 1, but the module only read stdout, so a failed check looked like an empty list and was cached for hours. Users could go weeks or months without seeing that updates were blocked!

The module now checks the exit status. When a check fails, it keeps the last known list, shows the error on the module and refresh pages, and retries after 15 minutes instead of caching an empty result.

Scheduled checks email the failure instead of acting on the old list, and in new-only mode the failure is emailed once until a check succeeds again.

Fixes https://forum.virtualmin.com/t/no-warning-of-dnf-error-suppressing-update/138052

package-updates/index.cgi

@@ -51,6 +51,16 @@ print &ui_hidden("mode", $in{'mode'});
 print &ui_grid_table(\@grid, 2),"<p>\n";
 print &ui_form_end();
 
+# Warn if the last check for updates failed, as the list may be stale
+$updates_error = &get_updates_error();
+if ($updates_error) {
+	print &ui_tag('div', &ui_details({
+		'title' => $text{'index_updateserr'},
+		'content' => &ui_tag('tt', &html_escape($updates_error)),
+		'class' => 'error',
+		'html' => 1 }, 1), {'style' => 'margin-bottom: 0.333em;'});
+	}
+
 # Work out what packages to show
 @current = &list_current(1);
 

package-updates/lang/en

@@ -35,6 +35,7 @@ index_action_note=Updates may also be installed by $1; disable external updates
 index_action_disable=Disable external updates
 index_err=Failed to fetch package list
 index_refresh=Refresh Available Packages
+index_updateserr=The last check for updates failed, so the list below may be out of date
 index_noupdate=No update exists from version $1
 index_caninstall=Available for installation
 index_webmin=Webmin module $1
@@ -138,6 +139,7 @@ refresh_clearing=Clearing package caches ..
 refresh_done=.. done
 refresh_available=Re-fetching available packages ..
 refresh_done3=.. found $1 possible updates
+refresh_failed=.. failed, so the last known list of updates is kept :
 
 view_title=Package Details
 view_header=Updatable package information

package-updates/package-updates-lib.pl

@@ -18,10 +18,14 @@ eval "use WebminCore;";
 	"dnf-automatic-notifyonly.timer",
 );
 
+# Cache files for the package lists and the error from a failed check for
+# updates
 $available_cache_file = &cache_file_path("available.cache");
 $current_cache_file = &cache_file_path("current.cache");
 $updates_cache_file = &cache_file_path("updates.cache");
 $held_updates_cache_file = &cache_file_path("held-updates.cache");
+$updates_error_file = &cache_file_path("updates.error");
+$failed_check_retry_mins = 15;	# Minutes before a failed check is retried
 $cron_cmd = "$module_config_directory/update.pl";
 
 $yum_cache_file = &cache_file_path("yumcache");
@@ -222,6 +226,32 @@ if (-r $file) {
 return ( );
 }
 
+# save_updates_error([error])
+# Saves the error from a failed check for updates, or removes the saved
+# error if none is given
+sub save_updates_error
+{
+my ($err) = @_;
+if ($err) {
+	# Save it for the module pages and the scheduled job to report
+	&open_tempfile(ERR, ">$updates_error_file");
+	&print_tempfile(ERR, $err);
+	&close_tempfile(ERR);
+	}
+else {
+	# No error, so stop warning about an earlier failure
+	unlink($updates_error_file);
+	}
+}
+
+# get_updates_error()
+# Returns the error if the last check for updates failed, or undef
+sub get_updates_error
+{
+return undef if (!-r $updates_error_file);
+return &read_file_contents($updates_error_file);
+}
+
 # compare_versions(&pkg1, &pkg2)
 # Returns -1 if the version of pkg1 is older than pkg2, 1 if newer, 0 if same.
 sub compare_versions
@@ -356,25 +386,47 @@ return $text{'hold_enotsupported'} if (!&supports_package_holds());
 return &software::update_system_hold($packages, $hold);
 }
 
-# updates_available(no-cache, [include-held])
+# updates_available([no-cache], [include-held])
 # Returns an array of hash refs of package updates available, according to
-# the update system, with caching.
+# the update system, with caching. If the check fails, returns the last
+# known list and saves the error for get_updates_error.
 sub updates_available
 {
 my ($nocache, $include_held) = @_;
+# Held updates have their own cache file and in-memory list
 my $cache_file = $include_held ? $held_updates_cache_file :
 				$updates_cache_file;
 my $cache = $include_held ? \@held_updates_available_cache :
 				\@updates_available_cache;
+# Load the list, unless a non-empty one is already in memory
 if (!scalar(@$cache)) {
 	if ($nocache || &cache_expired($cache_file)) {
 		# Get from original source
 		@$cache = &software::update_system_updates($include_held);
+		if ($software::update_system_error) {
+			# The check failed, so keep the last known list rather
+			# than caching an empty one, and save the error
+			@$cache = &read_cache_file($cache_file);
+			&save_updates_error($software::update_system_error);
+			}
+		else {
+			# The check worked, so clear any earlier error
+			&save_updates_error(undef);
+			}
+		# Add the fields callers use, and cache the list
 		foreach my $a (@$cache) {
 			$a->{'update'} = $a->{'name'};
 			$a->{'system'} = $software::update_system;
 			}
 		&write_cache_file($cache_file, $cache);
+		if ($software::update_system_error) {
+			# Expire the cache after $failed_check_retry_mins
+			# minutes, so a failed check is retried soon but not on
+			# every page load. A shorter cache time is left alone.
+			my $retry = time() - $config{'cache_time'}*60*60 +
+				    $failed_check_retry_mins*60;
+			utime($retry, $retry, $cache_file) if ($retry < time());
+			}
 		}
 	else {
 		# Use on-disk cache
@@ -750,14 +802,23 @@ if ($pkg->{'system'} eq 'yum') {
 return undef;
 }
 
+# flush_package_caches()
+# Clears the package list caches, so the next lookups fetch fresh data. Also
+# removes any saved error from a failed check for updates.
 sub flush_package_caches
 {
 unlink($current_cache_file);
-unlink($updates_cache_file);
-unlink($held_updates_cache_file);
 unlink($available_cache_file);
+# Expire the update lists rather than deleting them, so a failed check can
+# still fall back to the last known list
+foreach my $f ($updates_cache_file, $held_updates_cache_file) {
+	utime(0, 0, $f) if (-e $f);
+	}
+# Delete the remaining cache files and the saved error
 unlink($available_cache_file.'0');
 unlink($available_cache_file.'1');
+unlink($updates_error_file);
+# Clear the lists held in memory by this process
 @packages_available_cache = ( );
 @updates_available_cache = ( );
 @held_updates_available_cache = ( );

package-updates/update.cgi

@@ -62,7 +62,16 @@ elsif ($in{'refresh'} || $in{'refresh_top'}) {
 	# Force re-fetch
 	print $text{'refresh_available'},"<br>\n";
 	@avail = &list_possible_updates();
-	print &text('refresh_done3', scalar(@avail)),"<p>\n";
+	$err = &get_updates_error();
+	if ($err) {
+		# The check failed, so show the error instead of the count
+		print $text{'refresh_failed'},"<br>\n";
+		print "<pre>",&html_escape($err),"</pre>\n";
+		}
+	else {
+		# The check worked, so show how many updates were found
+		print &text('refresh_done3', scalar(@avail)),"<p>\n";
+		}
 
 	&webmin_log("refresh");
 	&ui_print_footer($redir, $redirdesc);

package-updates/update.pl

@@ -13,6 +13,11 @@ if ($ARGV[0] eq "--debug" || $ARGV[0] eq "-debug") {
 &clear_repository_cache();
 @todo = &list_possible_updates();
 
+# If the check for updates failed, the list is only the last known one, so
+# ignore it and report the failure instead
+$checkerr = &get_updates_error();
+@todo = ( ) if ($checkerr);
+
 # Install packages that are needed
 $tellcount = 0;
 %already = ( );
@@ -41,6 +46,15 @@ $newcount = 0;
 $tellbody = "";
 %notified = ( );
 %pending = ( );
+
+# A failed check for updates counts as a failure, so the "any update fails"
+# mode emails it. It is also tracked like a pending update, so new-only mode
+# emails it once until a check works again. Its key is fixed, because DNF's
+# error text varies between runs.
+if ($checkerr) {
+	$fcount++;
+	$pending{'error'} = 1;
+	}
 foreach $t (@todo) {
 	next if ($already{$t->{'update'}});
 	my $umsg = $t->{'security'} ? "security update" : "update";
@@ -104,6 +118,13 @@ else {
 	$body .= $tellbody;
 	}
 
+# Report a failed check for updates first, unless only new updates are
+# being reported and the failure was already reported
+if ($checkerr && (!$newonly || $newcount)) {
+	$body = "Checking for updates failed :\n".
+		"$checkerr\n\n".$body;
+	}
+
 if (@updated && $config{'sched_post_script'}) {
 	my @unique_updated = &unique(@updated);
 	my ($post_ok, $post_body) = &run_update_script(

software/yum-lib.pl

@@ -505,19 +505,48 @@ close(PKG);
 
 # update_system_updates([include-holds])
 # Returns available package updates, optionally including version-locked ones.
+# On DNF, a failed check returns an empty list and sets $update_system_error
+# to the error output. Any other result clears it, so callers can tell a
+# failure from no updates.
 sub update_system_updates
 {
 my ($include_holds) = @_;
 local @rv;
 local %done;
+$update_system_error = undef;
 if ($yum_command =~ /dnf/) {
+	# With DNF, add the flag that also lists version-locked updates if
+	# asked, escaping * for the shell
 	my $holdflag = $include_holds && &supports_update_system_holds() ?
 			" ".&update_system_hold_flags() : "";
 	$holdflag =~ s/\*/\\*/g;
-	&open_execute_command(PKG,
-		"$yum_command$holdflag check-update 2>/dev/null", 1, 1);
+
+	# Capture the exit status: DNF exits with 0 for no updates, 100 for
+	# updates found and any other status on failure. Stderr goes to a file
+	# rather than a second pipe, which could fill up and block DNF while
+	# stdout is still being read. Stdin comes from /dev/null so a prompt,
+	# such as for a repository key import, cannot hang.
+	my $out = "";
+	my $errfile = &transname();
+	my $ex = &execute_command("$yum_command$holdflag check-update",
+				  "/dev/null", \$out, $errfile, undef, 1);
+	my $code = $ex >> 8;
+	my $err = &read_file_contents($errfile);
+	unlink($errfile);
+	if ($code != 0 && $code != 100) {
+		# The check failed, so return no updates and set the error from
+		# stderr, or from stdout or the exit status if stderr is empty
+		$err =~ s/\s+$//;
+		$update_system_error = $err || $out ||
+			"$yum_command check-update exited with status $code";
+		return ( );
+		}
+	# Parse the captured output with the shared loop below
+	open(PKG, "<", \$out);
 	}
 else {
+	# YUM wraps long entries onto indented lines, so join them first.
+	# Failed YUM checks are not detected.
 	&open_execute_command(PKG, "$yum_command check-update 2>/dev/null | tr '\n' '#' | sed -e 's/# / /g' | tr '#' '\n'", 1, 1);
 	}
 while(<PKG>) {