diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index a7f1a7eeb9..343184d01a 100644 --- a/frappe/tests/test_commands.py +++ b/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() diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index 7845d814c6..3e70f719da 100644 --- a/frappe/utils/__init__.py +++ b/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 diff --git a/frappe/utils/backups.py b/frappe/utils/backups.py index b5722def4f..2a2eeb8efc 100644 --- a/frappe/utils/backups.py +++ b/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): """