fix: Backup audit trail retention - #7231
Open
saurabh6790 wants to merge 13 commits into
Open
Conversation
saurabh6790
requested review from
balamurali27,
siduck and
tanmoysrt
as code owners
August 14, 2026 12:57
Five of the eight errors were the BACKUP_TYPES alias, which mypy reads as a plain variable unless it is annotated and unless Literal comes from typing rather than through site.py. The rest: take_offsite took datetime.date, which is the method and not the type; get_site_time was annotated as a plain dict but reads attributes off a frappe._dict; and backup() fell out of its except block returning None where the round robin expects a bool. No behaviour change — None was already falsy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rotation only flipped files_availability to Unavailable, so months later a day whose backup was deleted on schedule and a day nothing ran on look the same. That is the question an audit asks, so stamp the time the files went and the rule that took them: Daily, Weekly, Monthly or Yearly under GFS, Rolling under FIFO, Local for onsite copies, Snapshot for physical ones and Dropped when a team drops its backups. The tier is derived from the backup's own date rather than written when the backup runs. It is the same arithmetic the expiry query already does, and one place to change if the policy is ever retuned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Site Backup rows are pruned after a few months, and the audit trail asks about a year. What survives is the bucket, where retention has already deleted everything but the site config, so old days come back as a successful backup of nothing. So roll each day into one compact document per site per month: status, sizes, and what retention did with the files. It is written nightly from the backups that changed that day, which catches both the backup and the expiry that lands a week later, and it costs one document a month rather than a row a day. A patch enqueues a backfill over the last year of records. It only reaches as far back as the rows still exist; anything older is the bucket's word or nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A day whose backup ran and was then deleted on schedule looked exactly like a day nothing ran on: Success, and every size blank. Blank meant four different things and the row never said which. So every day now carries what became of its files — Stored, Deleted with the date and rule, None, or Unknown — where its sizes came from, and whether it knows them at all. Sizes are read off the Site Backup record itself rather than the Remote File rows, so they survive the objects being deleted, and the monthly summaries answer for days whose records are gone. A day holding nothing but the site config is reported as deleted rather than as a backup of nothing, which is what the trail showed for anything past the retention window. Records now answer for failed backups too, so a day Press already knows failed no longer waits on the server. The bucket reading moves to its own module: it deals in object keys and sizes and knows nothing about a day of the trail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two columns and a day's detail, off the back of what the API now answers. Files says Stored, Deleted with the date and the rule that took them, None or Unknown; Evidence says which source answered for that day. A size the objects no longer back is shown as recorded rather than current, and a day whose sizes were never kept reads "Not recorded" instead of blank. The CSV carries the deletion date, the rule and the evidence, so the export stands on its own once it leaves the dashboard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Picking a date fired a request, so setting a range cost two builds and the first one was for a range nobody asked for. A build walks the buckets, so that is not free. Both dates now only change what will be asked for, and refresh asks. Which made the second half worse: the server rejects a range wider than a year, and the page swallowed it. frappe.throw comes back as a server message the dashboard never shows, so the trail just sat there on the old range. Surface it as a toast. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saurabh6790
force-pushed
the
backup-audit-trail-retention
branch
from
August 14, 2026 13:00
b82ff9d to
79a1311
Compare
Contributor
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported concurrent summary-rollup path no longer exists. Reviews (3): Last reviewed commit: "fix(site-backup): Stop the page and the ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+73
to
+87
| summary = frappe.db.get_value( | ||
| "Site Backup Summary", summary_name(site, month), ["name", "days"], as_dict=True | ||
| ) | ||
| if not summary: | ||
| frappe.get_doc({"doctype": "Site Backup Summary", "site": site, "month": month, "days": days}).insert( | ||
| ignore_permissions=True | ||
| ) | ||
| return | ||
|
|
||
| frappe.db.set_value( | ||
| "Site Backup Summary", | ||
| summary.name, | ||
| "days", | ||
| frappe.as_json((frappe.parse_json(summary.days) or {}) | days), | ||
| ) |
Contributor
There was a problem hiding this comment.
If the migration backfill overlaps the nightly job, both execute the non-atomic summary insert or JSON read-modify-write, causing one rollup to fail or overwrite the other's days; once the source backup rows are pruned, that audit history cannot be reconstructed.
Context Used: Guidelines for reviewing Frappe Framework applicat... (source)
Knowledge Base Used: Backup and Restore
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7231 +/- ##
===========================================
+ Coverage 59.66% 59.82% +0.16%
===========================================
Files 1019 1020 +1
Lines 93493 93926 +433
Branches 1212 1220 +8
===========================================
+ Hits 55780 56190 +410
- Misses 37690 37713 +23
Partials 23 23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The month is text, and frappe reads a between on it as numbers: the bounds became 0.0 and the site filter was folded into the upper bound, so the query was `month between 0 and (site = 'x')`. That answered with whatever months existed, for every site — one team's audit trail could show another's backup days — and on CI it answered with nothing. List the months the range touches and match on them instead. The test that catches it fails against the old filter with another site's day in the answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A build that raised set no answer and published nothing, so the page sat on "Putting the trail together" for as long as the tab stayed open, and every refresh started another build that died the same way. Nothing in the flow could ever end it. Cache a Broken answer for the short partial window and publish it from a finally, so the page hears about the failure, stops spinning, and a refresh retries a minute later. The exception still leaves the worker, so the error log keeps the traceback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The backfill runs for hours and the nightly pass starts while it is still going, so both can land on the same site-month. Merging was a read and then a write with nothing between them: the second writer dropped whatever the first had just added, and both could insert the month and one would fail outright. Once the backups behind those days are pruned there is nothing left to rebuild them from. Take the row lock for the length of the merge, and let a month another rollup created first be merged into rather than rejected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The patch would have walked every site with a backup in the last year to fill history nobody had asked for — 28k sites with as many backups again — and it did it by enqueuing, which a patch cannot rely on anyway. The nightly pass covers every day from here on. For the months before it, backfill_backup_summaries now takes the sites to do, run by hand for an audit that actually needs them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The trail reads Site Backup, then asks the server for the days records cannot answer, then the bucket for what is left. That is the flow we agreed on, and a fourth store of the same facts was never part of it — it would hold a copy of every site's backup history for good, for tens of thousands of sites, to answer a page. What made the trail readable stays: the deletion date and rule on the Site Backup row, sizes read off the record rather than the Remote File, and a bucket day holding only the site config reported as deleted. The cost is that a day whose Site Backup row has been pruned has no sizes to show. It reads Deleted with the bucket as its evidence, which is the truth of what is left by then. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A bench with an expired or wrong access key answered ListObjectsV2 with InvalidAccessKeyId, the error came straight out of the build, and the page was left saying the trail was being put together — for as long as the tab was open, with every refresh starting another build that died the same way. The buckets are the last source asked, and by then the records and the server have already said what they know. So log the bucket that could not be read, carry on with the rest, and mark the days nothing answered for as unconfirmed, the way an unreachable server already does. A build can still be killed rather than raise — a worker out of memory, a restart mid-build — and then nothing says why. Remember that a range was attempted: an attempt with no answer and no build still running is a build that died, and the page is told so instead of waiting on it. Tests put Press Settings back afterwards. Their fake S3 credentials had been committed onto the bench by an earlier run, which is what made every build fail there in the first place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A finished build tells the page to look again, and every look was allowed to start a build. So a build that ended without leaving an answer — the bucket unreadable, the worker killed, anything — put the page in a loop: look, build, publish, look, at whatever rate the build took. Staging counted fifty requests and was still going. A look that comes from a publish now reads only. If the build it was told about left nothing behind, it says so instead of starting another one. Refresh and the first page load still build. The server's backup jobs answer arriving does its own rebuild rather than dropping the trail and asking the page to rebuild it, which is the same loop by another route. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.