From 8e64894ccbb83985d766c88744da647fea15dff9 Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Thu, 15 Oct 2026 20:33:15 +0000 Subject: [PATCH 1/2] Handle the AI removing every mozilla patch when resolving conflicts. Fixes #430 If the AI resolves a patch conflict by dropping every patch (e.g. they were all upstreamed), the library no longer has patches, but we decided what to do next from the moz.yaml as it was at the start of the run. The re-apply then failed with "there are no vendored patches defined", and had it got past that, committing the (empty) patches and the phabricator revision count would have failed too. This happened to libsrtp on the dev instance. Re-read the moz.yaml after the AI succeeds; if no patches remain, skip the re-apply and the patches commit and submit one fewer revision. --- components/libraryprovider.py | 8 +++++ tasktypes/vendoring.py | 29 ++++++++++++------ tests/functionality_all_platforms.py | 46 ++++++++++++++++++++++++++++ tests/mock_libraryprovider.py | 4 +++ 4 files changed, 78 insertions(+), 9 deletions(-) diff --git a/components/libraryprovider.py b/components/libraryprovider.py index 656a6b9f..a0139fca 100644 --- a/components/libraryprovider.py +++ b/components/libraryprovider.py @@ -197,6 +197,14 @@ def get_libraries(self, gecko_path): return self._libraries + def library_has_patches(self, gecko_path, library): + # Re-read the moz.yaml rather than trusting library.has_patches, which + # reflects the file as it was when the run started (the AI conflict + # resolver may since have removed every patch). + with open(os.path.join(gecko_path, library.yaml_path), "r") as mozyaml: + contents = yaml.load(mozyaml.read(), Loader=yaml.BaseLoader) + return not not contents.get('vendoring', {}).get('patches') + @staticmethod def validate_library(yaml_contents, yaml_path): library = yaml.load(yaml_contents, Loader=yaml.BaseLoader) diff --git a/tasktypes/vendoring.py b/tasktypes/vendoring.py index be929c7c..3b0cc1d8 100644 --- a/tasktypes/vendoring.py +++ b/tasktypes/vendoring.py @@ -186,6 +186,9 @@ def _process_new_job(self, library, task, new_version, timestamp, most_recent_jo raise e ai_resolved_conflicts = False + # Whether there's a separate commit applying the mozilla patches. Normally + # the same as library.has_patches, but the AI may remove every patch. + has_patches_commit = library.has_patches if library.has_patches: # Apply Patches ------------------- try: @@ -217,17 +220,25 @@ def _process_new_job(self, library, task, new_version, timestamp, most_recent_jo CommentTemplates.AI_RESOLVED_PATCH_CONFLICTS(outcome, ai_details), needinfo=needinfo) - # Re-apply the now-updated patches so the vendored tree is patched. - try: - self.vendorProvider.patch(library, new_version) - except Exception as e2: - self.dbProvider.update_job_status(created_job, JOBSTATUS.DONE, JOBOUTCOME.COULD_NOT_PATCH) - self.bugzillaProvider.comment_on_bug(created_job.bugzilla_id, CommentTemplates.COULD_NOT_GENERAL_ERROR("apply the mozilla patches even after AI conflict resolution.", errormessage=_patch_exception_message(e2)), needinfo=library.maintainer_bz) - return + # If the AI removed every patch (e.g. because they were all + # upstreamed), there is nothing left to re-apply or commit. + has_patches_commit = self.libraryProvider.library_has_patches( + self.config['General'].get('gecko-path'), library) + if has_patches_commit: + # Re-apply the now-updated patches so the vendored tree is patched. + try: + self.vendorProvider.patch(library, new_version) + except Exception as e2: + self.dbProvider.update_job_status(created_job, JOBSTATUS.DONE, JOBOUTCOME.COULD_NOT_PATCH) + self.bugzillaProvider.comment_on_bug(created_job.bugzilla_id, CommentTemplates.COULD_NOT_GENERAL_ERROR("apply the mozilla patches even after AI conflict resolution.", errormessage=_patch_exception_message(e2)), needinfo=library.maintainer_bz) + return + else: + self.logger.log("The AI removed every mozilla patch, so there are none to re-apply or commit.", level=LogLevel.Info) # The AI added its own commit for the .patch/moz.yaml fixes, so # there is now an extra commit to submit to Phabricator. ai_resolved_conflicts = True + if has_patches_commit: # Commit Patches ------------------ try: self.mercurialProvider.commit_patches(library, created_job.bugzilla_id, new_version) @@ -254,14 +265,14 @@ def _process_new_job(self, library, task, new_version, timestamp, most_recent_jo try: # One commit for the vendoring, plus one for the patches (if any), # plus one more if the AI added a commit resolving patch conflicts. - num_commits = 1 + (1 if library.has_patches else 0) + (1 if ai_resolved_conflicts else 0) + num_commits = 1 + (1 if has_patches_commit else 0) + (1 if ai_resolved_conflicts else 0) phab_revisions = self.phabricatorProvider.submit_patches(created_job.bugzilla_id, num_commits) assert len(phab_revisions) == num_commits, "We don't have the correct number of phabricator patches; we have %s, expected %s" % (len(phab_revisions), num_commits) # Revisions come back bottom-up, matching the order the commits were made. purposes = ['vendoring commit'] if ai_resolved_conflicts: purposes.append('patch conflict resolution commit') - if library.has_patches: + if has_patches_commit: purposes.append('patches commit') for revision, purpose in zip(phab_revisions, purposes): self.dbProvider.add_phab_revision(created_job, revision, purpose) diff --git a/tests/functionality_all_platforms.py b/tests/functionality_all_platforms.py index 9f45184c..d9be39a1 100755 --- a/tests/functionality_all_platforms.py +++ b/tests/functionality_all_platforms.py @@ -470,6 +470,52 @@ def phab_submit(): finally: self._cleanup(u, expected_values) + # patch fails -> AI resolves it by removing every patch (e.g. all upstreamed) -> + # nothing to re-apply or commit, so only the vendoring and AI commits go to phabricator + @logEntryExitHeaderLine + def testPatchConflictAIRemovedAllPatches(self): + patch_calls = [0] + + def patch_callback(): + patch_calls[0] += 1 + if patch_calls[0] == 1: + raise_(Exception("Could not apply mozilla.patch, possible reasons:")) + raise_(Exception("Patch import was specified but there are no vendored patches defined.")) + + arc_calls = [0] + + def phab_submit(): + arc_calls[0] += 1 + v = 92000 + arc_calls[0] + return ARC_OUTPUT % (v, v) + + library_filter = 'png' + (u, expected_values, _check_jobs) = self._setup( + library_filter, + lambda b: ["try_rev|2021-02-09 15:30:04 -0500|2021-02-12 17:40:01 +0000"], + lambda: 55, # get_filed_bug_id_func + lambda b: {}, # filed_bug_ids_func + AssertFalse, # treeherder_response + command_callbacks={'patch': patch_callback, 'phab_submit': phab_submit}, + ai_config={'ai_outcome': 'trivial success', + 'ai_details': ['The only patch is already applied upstream, so I removed it.']}, + ) + u.libraryProvider.config['patches_after_ai'] = False + try: + u.run(library_filter=library_filter) + + self.assertEqual(u.aiProvider.resolve_call_count, 1, "The AI conflict resolver should have been called once") + self.assertEqual(patch_calls[0], 1, "With no patches left, they should not have been re-applied") + + lib = [lib for lib in u.libraryProvider.get_libraries(u.config_dictionary['General']['gecko-path']) if library_filter in lib.name][0] + j = u.dbProvider.get_job(lib, expected_values.library_new_version_id()) + self.assertEqual(JOBSTATUS.AWAITING_SECOND_PLATFORMS_TRY_RESULTS, j.status) + self.assertEqual(JOBOUTCOME.PENDING, j.outcome) + self.assertEqual([p.purpose for p in j.phab_revisions], + ['vendoring commit', 'patch conflict resolution commit']) + finally: + self._cleanup(u, expected_values) + # patch fails -> AI claims success -> but the patches STILL fail to re-apply -> COULD_NOT_PATCH @logEntryExitHeaderLine def testPatchConflictAIResolvedButReapplyStillFails(self): diff --git a/tests/mock_libraryprovider.py b/tests/mock_libraryprovider.py index 08f679a0..a7d25ef1 100644 --- a/tests/mock_libraryprovider.py +++ b/tests/mock_libraryprovider.py @@ -18,6 +18,10 @@ class MockLibraryProvider(BaseProvider, INeedsCommandProvider, INeedsLoggingProv def __init__(self, config): self.config = config + def library_has_patches(self, gecko_path, library): + # Tests set 'patches_after_ai' to simulate the AI removing every patch. + return self.config.get('patches_after_ai', library.has_patches) + def get_libraries(self, gecko_path): return [ Library({ From 83fa3ae21b14978b2a662c7809fb6c109b4b059b Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Mon, 19 Oct 2026 16:37:32 +0000 Subject: [PATCH 2/2] Handle a broken moz.yaml after AI conflict resolution Re-reading the moz.yaml happened outside the error handling for re-applying the patches, so if the AI left it missing or malformed the exception escaped, the job stayed in CREATED, and the maintainer wasn't told. Before re-reading was added, the same problem surfaced in the re-apply and was handled. Move it inside that handler. Also treat a moz.yaml without a vendoring section as broken rather than as having no patches. --- components/libraryprovider.py | 8 +++++-- tasktypes/vendoring.py | 28 +++++++++++++------------ tests/functionality_all_platforms.py | 31 ++++++++++++++++++++++++++++ tests/library.py | 18 ++++++++++++++++ tests/mock_libraryprovider.py | 5 ++++- 5 files changed, 74 insertions(+), 16 deletions(-) diff --git a/components/libraryprovider.py b/components/libraryprovider.py index a0139fca..c505901a 100644 --- a/components/libraryprovider.py +++ b/components/libraryprovider.py @@ -201,9 +201,13 @@ def library_has_patches(self, gecko_path, library): # Re-read the moz.yaml rather than trusting library.has_patches, which # reflects the file as it was when the run started (the AI conflict # resolver may since have removed every patch). - with open(os.path.join(gecko_path, library.yaml_path), "r") as mozyaml: + yaml_path = os.path.join(gecko_path, library.yaml_path) + with open(yaml_path, "r") as mozyaml: contents = yaml.load(mozyaml.read(), Loader=yaml.BaseLoader) - return not not contents.get('vendoring', {}).get('patches') + # A moz.yaml without a vendoring section is broken, not patch-free. + if not isinstance(contents, dict) or not isinstance(contents.get('vendoring'), dict): + raise Exception("%s has no vendoring section" % yaml_path) + return not not contents['vendoring'].get('patches') @staticmethod def validate_library(yaml_contents, yaml_path): diff --git a/tasktypes/vendoring.py b/tasktypes/vendoring.py index 3b0cc1d8..bcbf71e6 100644 --- a/tasktypes/vendoring.py +++ b/tasktypes/vendoring.py @@ -220,20 +220,22 @@ def _process_new_job(self, library, task, new_version, timestamp, most_recent_jo CommentTemplates.AI_RESOLVED_PATCH_CONFLICTS(outcome, ai_details), needinfo=needinfo) - # If the AI removed every patch (e.g. because they were all - # upstreamed), there is nothing left to re-apply or commit. - has_patches_commit = self.libraryProvider.library_has_patches( - self.config['General'].get('gecko-path'), library) - if has_patches_commit: - # Re-apply the now-updated patches so the vendored tree is patched. - try: + try: + # If the AI removed every patch (e.g. because they were all + # upstreamed), there is nothing left to re-apply or commit. + # Re-reading the moz.yaml is inside this handler because the + # AI may have left it broken. + has_patches_commit = self.libraryProvider.library_has_patches( + self.config['General'].get('gecko-path'), library) + if has_patches_commit: + # Re-apply the now-updated patches so the vendored tree is patched. self.vendorProvider.patch(library, new_version) - except Exception as e2: - self.dbProvider.update_job_status(created_job, JOBSTATUS.DONE, JOBOUTCOME.COULD_NOT_PATCH) - self.bugzillaProvider.comment_on_bug(created_job.bugzilla_id, CommentTemplates.COULD_NOT_GENERAL_ERROR("apply the mozilla patches even after AI conflict resolution.", errormessage=_patch_exception_message(e2)), needinfo=library.maintainer_bz) - return - else: - self.logger.log("The AI removed every mozilla patch, so there are none to re-apply or commit.", level=LogLevel.Info) + else: + self.logger.log("The AI removed every mozilla patch, so there are none to re-apply or commit.", level=LogLevel.Info) + except Exception as e2: + self.dbProvider.update_job_status(created_job, JOBSTATUS.DONE, JOBOUTCOME.COULD_NOT_PATCH) + self.bugzillaProvider.comment_on_bug(created_job.bugzilla_id, CommentTemplates.COULD_NOT_GENERAL_ERROR("apply the mozilla patches even after AI conflict resolution.", errormessage=_patch_exception_message(e2)), needinfo=library.maintainer_bz) + return # The AI added its own commit for the .patch/moz.yaml fixes, so # there is now an extra commit to submit to Phabricator. diff --git a/tests/functionality_all_platforms.py b/tests/functionality_all_platforms.py index d9be39a1..a69d2708 100755 --- a/tests/functionality_all_platforms.py +++ b/tests/functionality_all_platforms.py @@ -516,6 +516,37 @@ def phab_submit(): finally: self._cleanup(u, expected_values) + # patch fails -> AI claims success -> but leaves the moz.yaml unreadable -> COULD_NOT_PATCH + @logEntryExitHeaderLine + def testPatchConflictAILeftMozYamlBroken(self): + patch_calls = [0] + + def patch_callback(): + patch_calls[0] += 1 + raise_(Exception("Could not apply mozilla.patch, possible reasons:")) + + library_filter = 'png' + (u, expected_values, _check_jobs) = self._setup( + library_filter, + lambda b: ["try_rev|2021-02-09 15:30:04 -0500|2021-02-12 17:40:01 +0000"], + lambda: 57, # get_filed_bug_id_func + lambda b: {}, # filed_bug_ids_func + AssertFalse, # treeherder_response + command_callbacks={'patch': patch_callback}, + ai_config={'ai_outcome': 'trivial success', 'ai_details': ['I edited the moz.yaml.']}, + ) + u.libraryProvider.config['patches_after_ai'] = 'broken' + try: + u.run(library_filter=library_filter) + self.assertEqual(u.aiProvider.resolve_call_count, 1) + self.assertEqual(patch_calls[0], 1, "The patches should not be re-applied when the moz.yaml can't be read") + lib = [lib for lib in u.libraryProvider.get_libraries(u.config_dictionary['General']['gecko-path']) if library_filter in lib.name][0] + j = u.dbProvider.get_job(lib, expected_values.library_new_version_id()) + self.assertEqual(JOBSTATUS.DONE, j.status) + self.assertEqual(JOBOUTCOME.COULD_NOT_PATCH, j.outcome) + finally: + self._cleanup(u, expected_values) + # patch fails -> AI claims success -> but the patches STILL fail to re-apply -> COULD_NOT_PATCH @logEntryExitHeaderLine def testPatchConflictAIResolvedButReapplyStillFails(self): diff --git a/tests/library.py b/tests/library.py index ae50ac23..3405c773 100755 --- a/tests/library.py +++ b/tests/library.py @@ -7,11 +7,13 @@ import sys import unittest import os +import tempfile sys.path.append(".") sys.path.append("..") from components.libraryprovider import LibraryProvider, Library +from components.utilities import Struct from tests.mock_commandprovider import TestCommandProvider from components.logging import SimpleLoggerConfig @@ -150,6 +152,22 @@ def setUpClass(cls): }) cls.libraryprovider.update_config(additional_config) + def testLibraryHasPatches(self): + def has_patches(contents): + with tempfile.TemporaryDirectory() as gecko: + if contents is not None: + with open(os.path.join(gecko, "moz.yaml"), "w") as f: + f.write(contents) + return self.libraryprovider.library_has_patches(gecko, Struct(yaml_path='moz.yaml')) + + self.assertTrue(has_patches("vendoring:\n url: https://example.invalid\n patches:\n - mozilla.patch\n")) + self.assertFalse(has_patches("vendoring:\n url: https://example.invalid\n patches: []\n")) + self.assertFalse(has_patches("vendoring:\n url: https://example.invalid\n")) + # A broken moz.yaml is an error, not a library without patches. + for broken in ["origin:\n name: foo\n", "", "vendoring: [unclosed\n", None]: + with self.assertRaises(Exception, msg=repr(broken)): + has_patches(broken) + def testLibraryFindAndImport(self): libs = self.libraryprovider.get_libraries(os.getcwd()) self.maxDiff = 999999 diff --git a/tests/mock_libraryprovider.py b/tests/mock_libraryprovider.py index a7d25ef1..88609ae3 100644 --- a/tests/mock_libraryprovider.py +++ b/tests/mock_libraryprovider.py @@ -19,7 +19,10 @@ def __init__(self, config): self.config = config def library_has_patches(self, gecko_path, library): - # Tests set 'patches_after_ai' to simulate the AI removing every patch. + # Tests set 'patches_after_ai' to simulate the AI removing every patch, + # or to 'broken' to simulate it leaving the moz.yaml unreadable. + if self.config.get('patches_after_ai') == 'broken': + raise Exception("moz.yaml has no vendoring section") return self.config.get('patches_after_ai', library.has_patches) def get_libraries(self, gecko_path):