Skip to content

fix(backup): Add the site name to streamed backup file names #613

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

Merged
regdocs merged 1 commit into from
Oct 8, 2026
Merged

fix(backup): Add the site name to streamed backup file names #613

Show file tree
Hide file tree
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
14 changes: 10 additions & 4 deletions agent/site.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,11 @@ def backup_encryption_enabled(self):
return True
return value == "1"

def streamed_backup_prefix(self, todays_dt: str) -> str:

@greptile-apps greptile-apps Bot Sep 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Public method lacks docstring The new streamed_backup_prefix method has no docstring. The repository’s Frappe review guideline requires public functions to be documented; add one before merging.

Context Used: Guidelines for reviewing Frappe Framework applications. (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: agent/site.py
Line: 1005

Comment:
**Public method lacks docstring** The new `streamed_backup_prefix` method has no docstring. The repository’s Frappe review guideline requires public functions to be documented; add one before merging.

**Context Used:** Guidelines for reviewing Frappe Framework applications. ([source](https://github.com/frappe/skills/blob/main/skills/quality-code-review/SKILL.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

# Frappe's site slug goes before the marker, so its "*-{slug}-database.sql.gz"
# glob for recent backups never matches one of these FIFOs.
return f"{todays_dt}-{self.name.replace('.', '_')}-%stream%"

@job("Backup Site", priority="backup")
def backup_job( # noqa: C901
self,
Expand Down Expand Up @@ -1038,10 +1043,11 @@ def backup_job( # noqa: C901
{"traceback": "Streaming backup requires rclone, but it was not found on PATH."}
)
todays_dt = datetime.now().strftime("%Y%m%d_%H%M%S")
public_file = todays_dt + "-%stream%-files.tar"
db_file = todays_dt + "-%stream%-database.sql.gz"
private_file = todays_dt + "-%stream%-private-files.tar"
config_file = todays_dt + "-%stream%-site_config_backup.json"
file_prefix = self.streamed_backup_prefix(todays_dt)
public_file = file_prefix + "-files.tar"
db_file = file_prefix + "-database.sql.gz"
private_file = file_prefix + "-private-files.tar"
config_file = file_prefix + "-site_config_backup.json"

files_to_stream = [config_file, db_file]
if with_files:
Expand Down
12 changes: 12 additions & 0 deletions agent/tests/test_site.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import fnmatch
import json
import os
import random
Expand Down Expand Up @@ -375,6 +376,17 @@ def test_rename_site_works_for_existing_site(self):
self.assertTrue(os.path.exists(os.path.join(self.sites_directory, new_name)))
self.assertFalse(os.path.exists(os.path.join(self.sites_directory, old_name)))

def test_streamed_backup_names_carry_site_slug_and_stay_out_of_frappe_recent_backup_glob(self):
site_name = "streamed.site.test"
self._create_test_site(site_name)
with patch.object(Site, "config"):
site = Site(site_name, self._get_test_bench())

db_file = site.streamed_backup_prefix("20260929_101500") + "-database.sql.gz"

self.assertEqual(db_file, "20260929_101500-streamed_site_test-%stream%-database.sql.gz")
self.assertFalse(fnmatch.fnmatch(db_file, "*-streamed_site_test-database.sql.gz"))
Comment on lines +385 to +388

@greptile-apps greptile-apps Bot Sep 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 S3 filename remains untested This test checks the FIFO name but not the resulting S3 object name. Assert that the %stream%- marker becomes stream- in the object name so a regression in that conversion cannot pass unnoticed.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: agent/tests/test_site.py
Line: 385-388

Comment:
**S3 filename remains untested** This test checks the FIFO name but not the resulting S3 object name. Assert that the `%stream%-` marker becomes `stream-` in the object name so a regression in that conversion cannot pass unnoticed.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.


@unittest.skip("fails with 'Server' has no attr 'job'")
def test_valid_sites_property_of_bench_throws_if_site_config_is_corrupt(
self,
Expand Down
Loading