fix: throw exception if backup failed (#18230)

frappe/tests/test_commands.py

@@ -31,7 +31,7 @@ from frappe.query_builder.utils import db_type_is
 from frappe.tests.test_query_builder import run_only_if
 from frappe.tests.utils import FrappeTestCase
 from frappe.utils import add_to_date, get_bench_path, get_bench_relative_path, now
-from frappe.utils.backups import fetch_latest_backups
+from frappe.utils.backups import BackupGenerator, fetch_latest_backups
 from frappe.utils.jinja_globals import bundled_asset
 
 _result: Result | None = None
@@ -515,6 +515,19 @@ class TestBackups(BaseTestCommands):
 		self.assertIn("successfully completed", self.stdout)
 		self.assertNotEqual(before_backup["database"], after_backup["database"])
 
+	def test_backup_fails_with_exit_code(self):
+		"""Provide incorrect options to check if exit code is 1"""
+		odb = BackupGenerator(
+			frappe.conf.db_name,
+			frappe.conf.db_name,
+			frappe.conf.db_password + "INCORRECT PASSWORD",
+			db_host=frappe.db.host,
+			db_port=frappe.db.port,
+			db_type=frappe.conf.db_type,
+		)
+		with self.assertRaises(Exception):
+			odb.take_dump()
+
 	def test_backup_with_files(self):
 		"""Take a backup with files (--with-files)"""
 		before_backup = fetch_latest_backups()

frappe/utils/__init__.py

@@ -415,7 +415,7 @@ def unesc(s, esc_chars):
 	return s
 
 
-def execute_in_shell(cmd, verbose=0, low_priority=False):
+def execute_in_shell(cmd, verbose=False, low_priority=False, check_exit_code=False):
 	# using Popen instead of os.system - as recommended by python docs
 	import tempfile
 	from subprocess import Popen
@@ -427,7 +427,7 @@ def execute_in_shell(cmd, verbose=0, low_priority=False):
 			kwargs["preexec_fn"] = lambda: os.nice(10)
 
 		p = Popen(cmd, **kwargs)
-		p.wait()
+		exit_code = p.wait()
 
 		stdout.seek(0)
 		out = stdout.read()
@@ -435,12 +435,17 @@ def execute_in_shell(cmd, verbose=0, low_priority=False):
 		stderr.seek(0)
 		err = stderr.read()
 
-	if verbose:
+	failed = check_exit_code and exit_code
+
+	if verbose or failed:
 		if err:
 			print(err)
 		if out:
 			print(out)
 
+	if failed:
+		raise Exception("Command failed")
+
 	return err, out
 
 

frappe/utils/backups.py

@@ -420,8 +420,9 @@ class BackupGenerator:
 				)
 
 			cmd_string = (
-				"{db_exc} postgres://{user}:{password}@{db_host}:{db_port}/{db_name}"
-				" {include} {exclude} | {gzip} >> {backup_path_db}"
+				"self=$$; "
+				"( {db_exc} postgres://{user}:{password}@{db_host}:{db_port}/{db_name}"
+				" {include} {exclude} || kill $self ) | {gzip} >> {backup_path_db}"
 			)
 
 		else:
@@ -433,8 +434,10 @@ class BackupGenerator:
 				)
 
 			cmd_string = (
-				"{db_exc} --single-transaction --quick --lock-tables=false -u {user}"
-				" -p{password} {db_name} -h {db_host} -P {db_port} {include} {exclude}"
+				# Remember process of this shell and kill it if mysqldump exits w/ non-zero code
+				"self=$$; "
+				" ( {db_exc} --single-transaction --quick --lock-tables=false -u {user}"
+				" -p{password} {db_name} -h {db_host} -P {db_port} {include} {exclude} || kill $self ) "
 				" | {gzip} >> {backup_path_db}"
 			)
 
@@ -454,7 +457,7 @@ class BackupGenerator:
 		if self.verbose:
 			print(command.replace(args.password, "*" * 10) + "\n")
 
-		frappe.utils.execute_in_shell(command, low_priority=True)
+		frappe.utils.execute_in_shell(command, low_priority=True, check_exit_code=True)
 
 	def send_email(self):
 		"""