Skip to content

patching: Rewriter: actually delete empty patch files on rewrite #9209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rpardini wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −3
Open

patching: Rewriter: actually delete empty patch files on rewrite #9209

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions lib/tools/patching.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,11 @@
parent.rewrite_patch_file(patches)
FAILED_PATCHES = [one_patch for one_patch in VALID_PATCHES if (not one_patch.applied_ok) or (one_patch.rewritten_patch is None)]
for failed_patch in FAILED_PATCHES:
log.info(
f"Consider removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); "
f"it was not applied successfully.")
log.warning(
f"Removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); "
f" it failed to apply/was not rewritten.")
os.remove(failed_patch.parent.full_file_path())
Comment on lines 377 to +381
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Duplicate file deletion attempts will crash the script.

This code will attempt to delete the same patch file multiple times if that file contains multiple patches (mbox format) that both fail to apply or weren't rewritten. The second os.remove() call will raise a FileNotFoundError, terminating the script.

Additionally, there's no error handling around os.remove(), so any deletion failure (permissions, file locked, etc.) will also crash the script.

🐛 Proposed fix: deduplicate parent files and add error handling 
 		FAILED_PATCHES = [one_patch for one_patch in VALID_PATCHES if (not one_patch.applied_ok) or (one_patch.rewritten_patch is None)]
+		# Deduplicate parent files to avoid attempting to delete the same file multiple times
+		failed_patch_files_to_delete = set()
 		for failed_patch in FAILED_PATCHES:
-			log.warning(
-				f"Removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); "
-				f" it failed to apply/was not rewritten.")
-			os.remove(failed_patch.parent.full_file_path())
+			failed_patch_files_to_delete.add(failed_patch.parent.full_file_path())
+		
+		for patch_file_path in failed_patch_files_to_delete:
+			log.warning(f"Removing {patch_file_path}; it failed to apply/was not rewritten.")
+			try:
+				os.remove(patch_file_path)
+			except FileNotFoundError:
+				log.debug(f"File {patch_file_path} already removed.")
+			except OSError as e:
+				log.error(f"Failed to remove {patch_file_path}: {e}")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for failed_patch in FAILED_PATCHES:
log.info(
f"Consider removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); "
f"it was not applied successfully.")
log.warning(
f"Removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); "
f" it failed to apply/was not rewritten.")
os.remove(failed_patch.parent.full_file_path())
FAILED_PATCHES = [one_patch for one_patch in VALID_PATCHES if (not one_patch.applied_ok) or (one_patch.rewritten_patch is None)]
# Deduplicate parent files to avoid attempting to delete the same file multiple times
failed_patch_files_to_delete = set()
for failed_patch in FAILED_PATCHES:
failed_patch_files_to_delete.add(failed_patch.parent.full_file_path())
for patch_file_path in failed_patch_files_to_delete:
log.warning(f"Removing {patch_file_path}; it failed to apply/was not rewritten.")
try:
os.remove(patch_file_path)
except FileNotFoundError:
log.debug(f"File {patch_file_path} already removed.")
except OSError as e:
log.error(f"Failed to remove {patch_file_path}: {e}")
🤖 Prompt for AI Agents 
In @lib/tools/patching.py around lines 377 - 381, The loop over FAILED_PATCHES
may call os.remove(failed_patch.parent.full_file_path()) multiple times for the
same file and has no error handling; change the logic to collect unique parent
paths (use failed_patch.parent.full_file_path() or a set of parent identifiers)
and iterate once per unique path, call log.warning once per path (including the
failing patch counters if desired), and wrap os.remove(...) in a try/except that
catches OSError/FileNotFoundError to log an error via log.exception or log.error
instead of letting the script crash.

Copy link
Member

@igorpecovnik igorpecovnik Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpardini Is this concern real?

Copy link
Contributor

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member Author

@rpardini rpardini Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see it like that.


Comment on lines 376 to +382
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: File deletion logic has multiple bugs that can cause data loss and runtime errors.

Several issues with this implementation:

  1. Duplicate os.remove() calls: If a patch file contains multiple patches and several fail, each failed patch triggers os.remove() on the same path. The first succeeds; subsequent calls raise FileNotFoundError.

  2. Deletes successfully rewritten files: If a file has 3 patches where 2 succeed and 1 fails, the file is first rewritten with the 2 successful patches (line 375), then deleted here because of the 1 failed patch.

  3. Deletes files skipped intentionally: When rewrite_only_patches_needing_rebase=True (lines 364-367), patches not needing rebase are skipped and have rewritten_patch=None. These would incorrectly be treated as failed and trigger file deletion.

Proposed fix: Track parents that were successfully rewritten and use unique paths 
+		# Get parents that were successfully rewritten
+		successful_parents = set(patch_files_by_parent.keys())
+		
+		# Collect unique parent file paths that should be deleted (only if ALL patches failed)
+		parents_to_delete = set()
 		FAILED_PATCHES = [one_patch for one_patch in VALID_PATCHES if (not one_patch.applied_ok) or (one_patch.rewritten_patch is None)]
 		for failed_patch in FAILED_PATCHES:
-			log.warning(
-				f"Removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); "
-				f" it failed to apply/was not rewritten.")
-			os.remove(failed_patch.parent.full_file_path())
-
+			# Only delete if this parent had no successfully rewritten patches
+			if failed_patch.parent not in successful_parents:
+				parents_to_delete.add(failed_patch.parent)
+		
+		for parent in parents_to_delete:
+			log.warning(f"Removing {parent.full_file_path()}; all patches failed to apply/were not rewritten.")
+			os.remove(parent.full_file_path())
🤖 Prompt for AI Agents 
In @lib/tools/patching.py around lines 376 - 382, The deletion logic is unsafe:
it builds FAILED_PATCHES from VALID_PATCHES and calls
os.remove(failed_patch.parent.full_file_path()) for each failed patch, causing
duplicate removes, deleting files that were actually rewritten, and treating
skipped patches (rewritten_patch is None when
rewrite_only_patches_needing_rebase=True) as failures; fix by computing a unique
set of parent paths that truly need deletion by (a) tracking which parent paths
were successfully rewritten (e.g., collect parent.full_file_path() when
Patch.rewritten_patch is not None and the rewrite completed successfully), (b)
excluding patches skipped due to rewrite_only_patches_needing_rebase (do not
treat rewritten_patch==None as failure if the patch was intentionally skipped),
(c) building a set of failed_paths = {parent.full_file_path() for patches that
applied_ok is False and were not skipped} and then iterating over the sorted
unique failed_paths to remove each once with os.remove inside a try/except
FileNotFoundError to avoid crashing; use the symbols VALID_PATCHES,
FAILED_PATCHES, Patch.rewritten_patch, Patch.applied_ok,
parent.full_file_path(), and rewrite_only_patches_needing_rebase to locate and
implement the change.


# Create markdown about the patches
readme_markdown: "str | None" = None
Expand Down