From ab9f94cc28674017a24779d7ed66566c90772a2a Mon Sep 17 00:00:00 2001 From: Mathieu Benoit Date: Fri, 31 Jul 2026 23:22:02 -0400 Subject: [PATCH] [FIX] migration: stop recording failed steps as successful MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The database migration recorded steps as done even when they had failed, so a resume skipped them and the loop kept going on a broken database. execute.py - exec_command_live() left exit_code = None when an exception was raised. None is falsy, so « if not status: » marked the step done and « if status and wait_at_error » skipped the error prompt: a crashed command was reported as a success. Both except blocks now set exit_code = 1. - The « no Odoo version installed » path returned a bare -1 while callers unpack a tuple (status, cmd), raising ValueError instead of surfacing the failure. It now returns the same shape the caller asked for. todo_upgrade.py - todo_upgrade_execute(): treat a None status as a failure (defence in depth). - Database migration (OpenUpgrade): the return code was discarded, with an explicit « TODO detect error », and the state was written unconditionally. It is now captured; on failure the loop stops instead of migrating the next version on top of a half-migrated database. - Neutralization: the flag was set before the « if not status » test, making that test dead code. Removed the unconditional assignment. - Clone and fix-migration hook: their return codes were never captured, so the steps were marked done whatever happened. Both are now checked. Co-Authored-By: Claude Opus 4.8 (1M context) --- script/execute/execute.py | 17 +++++++++++- script/todo/todo_upgrade.py | 55 ++++++++++++++++++++++++++++++++----- 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/script/execute/execute.py b/script/execute/execute.py index 62ddcb8..c10bf5d 100644 --- a/script/execute/execute.py +++ b/script/execute/execute.py @@ -109,7 +109,16 @@ class Execute: _logger.error( f"You cannot execute Odoo command if no version is installed. Command : {command}" ) - return -1 + # Return the SAME shape the caller asked for. A bare int here + # made callers doing « status, cmd = exec_command_live(...) » + # crash with ValueError instead of seeing the failure. + if return_status_and_output_and_command: + return 1, command, [] + if return_status_and_command: + return 1, command + if return_status_and_output: + return 1, [] + return 1 command = f"source ./.venv.{source_odoo}/bin/activate && {command}" if new_window and self.cmd_source_default: command = self.cmd_source_default % command @@ -154,7 +163,12 @@ class Execute: if process.returncode != 0 and not quiet: print("Command returned error code:" f" {process.returncode}") + # An exception MUST report a failure. exit_code stays None otherwise, + # and None is falsy: callers testing « if not status: » would mark the + # step as done, and « if status and wait_at_error » would skip the error + # prompt. A crashed command was therefore recorded as a success. except FileNotFoundError: + exit_code = 1 if not quiet: if "password" in command: print( @@ -164,6 +178,7 @@ class Execute: else: print(f"Error: Command '{command}' not found.") except Exception as e: + exit_code = 1 if not quiet: print(f"An error occurred: {e}") process_end_time = time.time() diff --git a/script/todo/todo_upgrade.py b/script/todo/todo_upgrade.py index 84da783..b34ea9a 100755 --- a/script/todo/todo_upgrade.py +++ b/script/todo/todo_upgrade.py @@ -810,8 +810,9 @@ class TodoUpgrade: # ./script/database/migrate/process_backup_file.py --path_backup_zip image_db/db.zip --path_output_zip image_db/dbFIX.zip --word_to_delete discuss_channel_channel_type_not_null # Puis faire un retry de la commande, sinon rien - self.dct_progression["state_1_neutralize_database"] = True - self.write_config() + # Only record the step when it actually succeeded. The previous + # unconditional assignment made the test below dead code: a failed + # neutralization was remembered as done and skipped on resume. if not status: self.dct_progression["state_1_neutralize_database"] = True self.write_config() @@ -1029,7 +1030,20 @@ class TodoUpgrade: # Duplicate database cmd_clone_database = f"./odoo_bin.sh db --clone --from_database {last_database_name} --database {database_name_upgrade}" - self.todo_upgrade_execute(cmd_clone_database) + status, cmd_executed = self.todo_upgrade_execute( + cmd_clone_database + ) + + # Everything downstream runs against this clone: if it failed, + # do not mark it done (a rerun would skip the clone and migrate + # a missing or truncated database). + if status: + print( + f"❌ -> Clone to Odoo{next_version} FAILED (status" + f" {status}). Stopping: '{database_name_upgrade}' is" + " not usable." + ) + return lst_clone_odoo[index] = True self.dct_progression["state_4_clone_odoo_lst"] = lst_clone_odoo @@ -1508,11 +1522,22 @@ class TodoUpgrade: f"fix_migration_odoo{(next_version-1)*10}_to_odoo{next_version*10}.py", ) if os.path.exists(file_path_fix_migration): - self.todo_upgrade_execute( + status, cmd_executed = self.todo_upgrade_execute( f"cat ./{file_path_fix_migration} | ./odoo{next_version}.0/odoo/odoo-bin shell -d {database_name_upgrade}", single_source_odoo=True, ) + # A fix that did not run must not be recorded as applied, + # otherwise the rerun skips it and OpenUpgrade hits the very + # problem the fix exists to prevent. + if status: + print( + f"❌ -> Fix migration Odoo{next_version} FAILED" + f" (status {status}):" + f" {file_path_fix_migration}" + ) + return + lst_fix_migration_odoo[index] = file_path_fix_migration self.dct_progression["state_4_fix_migration_odoo_lst"] = ( lst_fix_migration_odoo @@ -1571,13 +1596,26 @@ class TodoUpgrade: cmd_upgrade = f"./run.sh --upgrade-path=./odoo{next_version}.0/OCA_OpenUpgrade/openupgrade_scripts/scripts --update all -c config.conf --stop-after-init --no-http --load=base,web,openupgrade_framework -d {database_name_upgrade}" lst_upgrade_odoo[index] = cmd_upgrade - self.todo_upgrade_execute( + status, cmd_executed = self.todo_upgrade_execute( cmd_upgrade, new_env={ "OPENUPGRADE_TARGET_VERSION": f"{next_version}.0" }, ) - # TODO detect error + + # This is THE data migration. Recording it as done when it + # failed used to send the loop to the next version on top of a + # half-migrated database. Stop here instead: the state stays + # unset, so a rerun replays this version. + if status: + print( + f"❌ -> Database migration to Odoo{next_version} FAILED" + f" (status {status}). Stopping before version" + f" {next_version + 1} to avoid migrating a broken" + " database. Fix the cause, then relaunch: this version" + " will be replayed." + ) + return self.dct_progression["state_4_upgrade_odoo_lst"] = ( lst_upgrade_odoo @@ -1920,7 +1958,10 @@ class TodoUpgrade: self.lst_command_executed.append(cmd_executed) self.dct_progression["command_executed"] = self.lst_command_executed self.write_config() - if status and wait_at_error: + # None means « the command never reported a status » -> treat it as a + # failure, never as a success (defence in depth: exec_command_live now + # always sets one, but a silent None must not skip this prompt). + if (status is None or status) and wait_at_error: print("[1] to redo the command") wait_status = ( input(