From 2717504b5915be723bdbcb4003778672bfaea864 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@frappe.io>
Date: Wed, 28 Sep 2022 14:29:38 +0530
Subject: [PATCH] fix: throw exception if backup failed (#18230)

(cherry picked from commit abeed354612ca034eeb09b5ad4a1f419853bdcd0)
---
 frappe/tests/test_commands.py | 15 ++++++++++++++-
 frappe/utils/__init__.py      | 11 ++++++++---
 frappe/utils/backups.py       | 13 ++++++++-----
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py
index cc71922e7ede..50a35723c59e 100644
--- a/frappe/tests/test_commands.py
+++ b/frappe/tests/test_commands.py
@@ -15,7 +15,7 @@
 import frappe.recorder
 from frappe.installer import add_to_installed_apps, remove_app
 from frappe.utils import add_to_date, get_bench_relative_path, now
-from frappe.utils.backups import fetch_latest_backups
+from frappe.utils.backups import BackupGenerator, fetch_latest_backups
 
 
 # TODO: check frappe.cli.coloured_output to set coloured output!
@@ -317,6 +317,19 @@ def test_partial_restore(self):
 		self.assertEquals(self.returncode, 0)
 		self.assertEquals(frappe.db.count("ToDo"), todo_count)
 
+	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_recorder(self):
 		frappe.recorder.stop()
 
diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py
index d73a92ad0612..aa3ac75ef2cd 100644
--- a/frappe/utils/__init__.py
+++ b/frappe/utils/__init__.py
@@ -391,7 +391,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
@@ -404,7 +404,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()
@@ -412,12 +412,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
 
 
diff --git a/frappe/utils/backups.py b/frappe/utils/backups.py
index 1842146f1da3..84d9f520f771 100644
--- a/frappe/utils/backups.py
+++ b/frappe/utils/backups.py
@@ -389,8 +389,9 @@ def take_dump(self):
 				)
 
 			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:
@@ -405,8 +406,10 @@ def take_dump(self):
 				)
 
 			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}"
 			)
 
@@ -426,7 +429,7 @@ def take_dump(self):
 		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):
 		"""