Bug 40901: (QA follow-up) Harden the service abstraction in koha-functions.sh

- koha_init_backend relied on "systemctl list-unit-files" exiting
  non-zero when nothing matches. That behaviour is version dependent,
  and on a systemd host running koha-sysv a false positive would send
  every koha-* helper to units that do not exist. Test for the
  koha-plack@.service unit file instead: unambiguous, and no systemctl
  fork per call. The _KOHA_INIT_BACKEND cache is dropped; every caller
  ran the function in a $(...) subshell so it never took effect anyway.
- The _sysv_worker and _sysv_z3950 code moved here from bash-only
  scripts brought "((error_count++))" and "[[ ! $VAR ]]" into a
  #!/bin/sh library that koha-disable, koha-enable, koha-remove,
  koha-list and the init script source under dash. Use POSIX
  equivalents.
- _sysv_restart_zebra and _sysv_restart_sip lost the explicit
  "return 0" of the original quiet not-running branch, so a start
  failure now aborted the caller's whole instance loop under set -e
  (including the init script's restart across all instances). Restore
  it.
- systemctl status is run with --no-pager from koha_service_ctl and
  koha-systemd-ctl so the helpers never block on a pager.
- koha-plack --enable/--disable ran systemctl before editing the Apache
  configuration, so a systemctl failure under set -e left the vhost
  untouched. Do the systemd half after the Apache edits and warn
  instead of aborting.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>

debian/scripts/koha-functions.sh

@@ -170,19 +170,21 @@ is_sitemap_enabled()
 # --- Service management abstraction ---
 # Systemd is the primary path. Legacy daemon/start-stop-daemon is the fallback.
 
-_KOHA_INIT_BACKEND=""
-
+# Prints "systemd" when systemd is PID 1 *and* the koha-systemd package's
+# units are installed, "sysv" otherwise. A systemd host running koha-sysv
+# must take the SysV path, so the presence of the unit files is what
+# decides, not the init system alone. Callers use $(koha_init_backend), so
+# keep this cheap: plain file tests, no systemctl round trip.
 koha_init_backend()
 {
-    if [ -z "$_KOHA_INIT_BACKEND" ]; then
     if [ -d /run/systemd/system ] && \
-           systemctl list-unit-files koha-plack@.service >/dev/null 2>&1; then
-            _KOHA_INIT_BACKEND="systemd"
+       { [ -f /lib/systemd/system/koha-plack@.service ] || \
+         [ -f /usr/lib/systemd/system/koha-plack@.service ] || \
+         [ -f /etc/systemd/system/koha-plack@.service ]; }; then
+        echo "systemd"
     else
-            _KOHA_INIT_BACKEND="sysv"
+        echo "sysv"
     fi
-    fi
-    echo "$_KOHA_INIT_BACKEND"
 }
 
 # Map service + queue to systemd unit name
@@ -210,7 +212,7 @@ koha_service_ctl()
         reload)
             systemctl reload-or-restart "$unit" ;;
         status)
-            systemctl status "$unit" ;;
+            systemctl --no-pager status "$unit" ;;
     esac
 }
 
@@ -787,8 +789,11 @@ _sysv_restart_zebra()
     else
         if [ "$verbose" != "no" ]; then
             log_warning_msg "Zebra not running for ${name}."
-        fi
             _sysv_start_zebra ${name}
+        else
+            _sysv_start_zebra ${name}
+            return 0
+        fi
     fi
 }
 
@@ -838,11 +843,11 @@ _sysv_start_worker()
 
             echo "Starting Koha worker daemon for ${name} (${queue})"
             if ! daemon $DAEMONOPTS -- "$worker_DAEMON" --queue "$queue"; then
-                ((error_count++))
+                error_count=$((error_count+1))
             fi
         else
             echo "Error: worker already running for ${name} (${queue})"
-            ((error_count++))
+            error_count=$((error_count+1))
         fi
     done
     log_end_msg $error_count
@@ -868,11 +873,11 @@ _sysv_stop_worker()
 
             echo "Stopping Koha worker daemon for ${name} (${queue})"
             if ! daemon $DAEMONOPTS --stop -- "$worker_DAEMON" --queue "$queue"; then
-                ((error_count++))
+                error_count=$((error_count+1))
             fi
         else
             echo "Error: worker not running for ${name} (${queue})"
-            ((error_count++))
+            error_count=$((error_count+1))
         fi
     done
     log_end_msg $error_count
@@ -898,7 +903,7 @@ _sysv_restart_worker()
 
             echo "Restarting Koha worker daemon for ${name} (${queue})"
             if ! daemon $DAEMONOPTS --restart -- "$worker_DAEMON" --queue "$queue"; then
-                ((error_count++))
+                error_count=$((error_count+1))
             fi
         else
             echo "Worker not running for ${name} (${queue})."
@@ -1199,8 +1204,11 @@ _sysv_restart_sip()
     else
         if [ "$verbose" != "no" ]; then
             log_warning_msg "Warning: SIP server not running for ${name}."
-        fi
             _sysv_start_sip ${name}
+        else
+            _sysv_start_sip ${name}
+            return 0
+        fi
     fi
 }
 
@@ -1264,7 +1272,7 @@ _sysv_start_z3950()
     if ! is_z3950_running ${instancename}; then
         export KOHA_CONF="/etc/koha/sites/${instancename}/koha-conf.xml"
 
-        if [[ ! $Z3950_ADDITIONAL_OPTS ]]; then
+        if [ -z "$Z3950_ADDITIONAL_OPTS" ]; then
             Z3950_ADDITIONAL_OPTS="$( xmlstarlet sel -t -v 'yazgfs/config/z3950_responder_options' "$CONFIGDIR/config.xml" || true )"
         fi
 

debian/scripts/koha-plack

@@ -124,10 +124,6 @@ enable_plack()
     local instancename=$1
     local instancefile=$(get_apache_config_for "$instancename")
 
-    if [ "$(koha_init_backend)" = "systemd" ]; then
-        koha_service_ctl enable plack "$instancename"
-    fi
-
     alreadyopac=0
     alreadyintra=0
     failopac=0
@@ -159,6 +155,14 @@ enable_plack()
         alreadyintra=1
     fi
 
+    # Under systemd the unit has to be enabled as well, whatever the Apache
+    # side looked like. Done after the config edits so a systemctl failure
+    # cannot leave them half-applied under set -e.
+    if [ "$(koha_init_backend)" = "systemd" ]; then
+        koha_service_ctl enable plack "$instancename" || \
+            warn "Could not enable koha-plack@${instancename}.service"
+    fi
+
     # Fail if it was already plack enabled.
     if [ $alreadyopac -eq 1 ] && [ $alreadyintra -eq 1 ] ; then
         return 1
@@ -179,10 +183,6 @@ disable_plack()
     local instancename=$1
     local instancefile=$(get_apache_config_for "$instancename")
 
-    if [ "$(koha_init_backend)" = "systemd" ]; then
-        koha_service_ctl disable plack "$instancename"
-    fi
-
     alreadyopac=0
     alreadyintra=0
     failopac=0
@@ -214,6 +214,13 @@ disable_plack()
         alreadyintra=1
     fi
 
+    # Under systemd the unit has to be disabled as well; done after the
+    # Apache edits for the same reason as in enable_plack.
+    if [ "$(koha_init_backend)" = "systemd" ]; then
+        koha_service_ctl disable plack "$instancename" || \
+            warn "Could not disable koha-plack@${instancename}.service"
+    fi
+
     # Fail if it was already plack disabled.
     if [ $alreadyopac -eq 1 ] &&  [ $alreadyintra -eq 1 ] ; then
         return 1

debian/systemd/koha-systemd-ctl

@@ -60,7 +60,10 @@ else
 fi
 
 case "$CMD" in
-    enable|disable|start|stop|restart|status)
+    status)
+        systemctl --no-pager "$CMD" "$UNIT"
+        ;;
+    enable|disable|start|stop|restart)
         systemctl "$CMD" "$UNIT"
         ;;
     *)