Skip to content

fix(agent-job): Tell the team to remove folders from private/backups - #7998

Open
Shariqk3 wants to merge 1 commit into
frappe:developfrom
Shariqk3:fix/backup-dir-in-backups-folder
Open

Shariqk3 wants to merge 1 commit into
frappe:developfrom
Shariqk3:fix/backup-dir-in-backups-folder

Conversation

@Shariqk3

@Shariqk3 Shariqk3 commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

A "Backup Site" job fails when the private/backups folder of a site contains a folder. Before each backup, Frappe deletes old files in private/backups. Frappe uses os.remove, which cannot delete a folder, so the backup stops with this error:

File "apps/frappe/frappe/utils/backups.py", line 662, in delete_temp_backups
    os.remove(this_file_path)
IsADirectoryError: [Errno 21] Is a directory: './ropierpnext.frappe.cloud/private/backups/uom-night-20261102'

The customer sees only the raw traceback in the job. The traceback does not tell the customer what to do. Usually a custom app or a script of the customer makes the folder, so only the customer can remove it.

Change

This PR adds a handler to agent_job_notifications.py. The handler starts when the traceback or the output of the job contains IsADirectoryError and /private/backups/. The failed job page then shows a red banner. The "Fix" button on the banner opens a dialog that tells the customer to do these steps:

  1. Delete the folders in private/backups, or move them to a different location.
  2. Take the backup again.
  3. If an app or a script makes these folders, change it to use a different location.

The dialog shows the path of the folder from the traceback. The dialog has no Help link, because no page on docs.frappe.io covers this error.

A fix in Frappe (skip folders in delete_temp_backups) can prevent the failure. But sites on older Frappe versions continue to fail until they update, so this banner is still necessary.

Screenshots

The screenshots show a failed "Backup Site" job on a local demo site. The job went through the real failure path, send_job_failure_notification, once with the code from develop and once with the code from this PR.

Before After
No banner on the failed job Banner on the failed job
The open step shows only the traceback The banner and the open step with the traceback
No dialog The dialog that the Fix button opens

Tests

  • test_folder_in_backups_folder_names_the_folder_to_delete: the banner is actionable and shows the path of the folder.
  • test_folder_error_outside_backups_folder_is_not_actionable: an IsADirectoryError for a different folder gets no banner.

I did these checks:

  1. I ran bench backup on a local site with a folder in private/backups. The backup failed at delete_temp_backups, line 662, as in the example job.
  2. I ran the tests in test_agent_job. 39 of 41 tests pass. The 2 tests that fail are test_fail_old_jobs_*. They also fail on develop locally, because the local database does not have their sites.
  3. I ran the tests in test_site_backup. All 13 tests pass.
  4. Ruff, semgrep, and mypy found no problems.

🤖 Generated with Claude Code

Frappe deletes old files in private/backups before each backup. If a
folder is in private/backups, os.remove raises IsADirectoryError and
the backup fails. The team saw only "Job Failure" and did not know the
cause, because a custom app or a script made the folder.

The banner on the failed job now names the folder and tells the team to
remove it. It has no Help link, because no doc page covers this error.
The real fix is in frappe (skip folders), but sites on older versions
continue to fail until then.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Shariqk3 Shariqk3 added the backport-master For mergify backport to master label Nov 5, 2026
@codecov-commenter

codecov-commenter commented Nov 5, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.08%. Comparing base (fdd16c9) to head (7904ca9).
⚠️ Report is 23 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7998      +/-   ##
===========================================
+ Coverage    67.00%   67.08%   +0.08%     
===========================================
  Files         1178     1178              
  Lines       118284   118551     +267     
  Branches      2383     2379       -4     
===========================================
+ Hits         79256    79535     +279     
+ Misses       38961    38949      -12     
  Partials        67       67              
Flag Coverage Δ
dashboard 85.44% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mergify

mergify Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-master For mergify backport to master

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants