Merge pull request #2854 from webmin/dev/fix-dnf-check-update-failure

Fix failed DNF update checks showing no updates

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,49 @@ 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+$//;
+		$out =~ 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>) {

t/software-yum.t

@@ -8,7 +8,8 @@ use File::Spec;
 use Cwd qw(abs_path);
 
 our (%config, %packages, %text);
-our ($yum_command, $supports_dnf_versionlock, $dnf_version);
+our ($yum_command, $supports_dnf_versionlock, $dnf_version,
+     $update_system_error);
 
 sub has_command
 {
@@ -232,22 +233,26 @@ no warnings qw(once redefine);
 my $dnf_output =
 	"bash.aarch64 5.1.8-10.el9 baseos\n".
 	"coreutils.aarch64 8.32-40.el9 baseos\n";
+my $dnf_stderr = '';
+my $dnf_status = 100;
 my @commands;
 local *supports_update_system_holds = sub { return 1; };
 local *list_update_system_holds = sub { return ('bash'); };
 local *set_yum_security_field = sub { };
 local *get_dnf_version = sub { return 4; };
-local *open_execute_command = sub {
-	my ($fh, $command) = @_;
+local *transname = sub { return 'simulated-stderr-file'; };
+local *read_file_contents = sub { return $dnf_stderr; };
+local *execute_command = sub {
+	my ($command, $stdin, $stdout) = @_;
 	push(@commands, $command);
-	no strict 'refs';
-	open(ref($fh) ? $fh : \*{$fh}, '<', \$dnf_output)
-		or die "open simulated DNF updates: $!";
+	$$stdout = $dnf_output;
+	return $dnf_status << 8;
 	};
 
 my @normal = update_system_updates(0);
 is_deeply([ map { $_->{'name'} } @normal ], [ 'coreutils' ],
 	'DNF 4 regular updates exclude held packages');
+is($update_system_error, undef, 'a successful check reports no error');
 my @with_holds = update_system_updates(1);
 is_deeply([ map { $_->{'name'} } @with_holds ],
 	[ 'bash', 'coreutils' ], 'DNF 4 held-update query includes locks');
@@ -260,6 +265,39 @@ local *get_dnf_version = sub { return 5; };
 like($commands[0], qr/--setopt=disable_excludes=\\\* check-update/,
 	'DNF 5 disables excludes for held-update discovery');
 ok($with_holds[0]->{'held'}, 'marks a DNF 5 locked update as held');
+
+$dnf_output = '';
+$dnf_status = 0;
+is_deeply([ update_system_updates(0) ], [ ],
+	'DNF exit status 0 means no updates');
+is($update_system_error, undef, 'no updates is not reported as an error');
+
+$dnf_output = "Some repo   0.0  B/s |   0  B     00:00\n";
+$dnf_stderr =
+	"Errors during downloading metadata for repository 'x':\n".
+	"Error: Failed to download metadata for repo 'x'\n";
+$dnf_status = 1;
+is_deeply([ update_system_updates(0) ], [ ],
+	'a failed check returns no updates');
+is($update_system_error,
+	"Errors during downloading metadata for repository 'x':\n".
+	"Error: Failed to download metadata for repo 'x'",
+	'a failed check reports the DNF error output');
+
+$dnf_stderr = '';
+update_system_updates(0);
+is($update_system_error, 'Some repo   0.0  B/s |   0  B     00:00',
+	'a failed check without error output reports its normal output');
+
+$dnf_output = '';
+update_system_updates(0);
+is($update_system_error, '/usr/bin/dnf check-update exited with status 1',
+	'a silent failure reports the exit status');
+
+$dnf_status = 137;
+update_system_updates(0);
+is($update_system_error, '/usr/bin/dnf check-update exited with status 137',
+	'a killed check is reported as a failure');
 }
 
 {