fix(site-update): Make site update recovery resilient and recoverable

[balamurali27]

Problem

When a site update (Pull/Migrate onto a new bench) fails, recovery could end up Fatal with no automatic path back and no clear signal to the user:

• Recovery migrate queries on large sites could exceed the database server's max_statement_time and get killed mid-query, turning a recoverable failure into a fatal one.
• A migrate recovery that failed mid table-restore had no automatic follow-up — the site was left Broken even though its tables could still be restored from the backup. Re-running the full recovery doesn't work: the agent's move_site is not idempotent (the site has already been moved back), so it fails at "Move Site".
• An update run with backups skipped that failed gave a generic notification, leaving the user unsure it needs manual intervention and how to do it.

Changes

max_statement_time bump before a recovery migrate (and revert after)

• Before a recovery migrate on a large database (over LARGE_DATABASE_SIZE, 2 GB), bump max_statement_time by an hour (dynamic MariaDB variable, no restart), recorded as a comment on the Site Update. Smaller databases finish well within the timeout and are skipped.
• The pre-bump value is stashed on the Site Update (previous_max_statement_time) and restored once recovery finishes — at the recover job's terminal state, or, when the fallback table restore runs, at that job's callback. Without this the value ratcheted up an hour on every recovery.

One-shot Restore Site Tables fallback when a migrate recovery fails — but only when safe

Press makes one automatic attempt to bring the site back up, only when all hold:

• it was a Recover Failed Site Migrate;
• its "Move Site" step succeeded (so the site is back on the source bench — otherwise restoring tables would target the wrong bench); and
• it failed due to a transient DB error (MySQL server has gone away / Lost connection to MySQL server, detected from the job output/traceback and step output). Other failures are genuine problems left Fatal for manual attention.

Since the failed recovery has already moved the site back, only the table restore is left undone, so Press re-issues just that job (linked on the Site Update as a comment for traceability).

On a successful fallback Restore Site Tables, the update stays Fatal

The site becomes Active again and the fatal update's cause of failure is marked resolved (set_cause_of_failure_is_resolved, which also clears the site's fatal_site_update). The update itself still failed and is recorded as such, rather than being silently flipped to Recovered.

Actionable notification for skipped-backups failures

Replaces the generic message with one telling the user the site can't be recovered automatically, linking to the SSH docs to fix it manually. (Skipped-backups failures go straight to Fatal — no recovery, no table restore, since there's no backup.)

Notes

Site Usage stores sizes in MB, not bytes (despite unitless Int fields) — so the 2 GB threshold is 2048, not 2048**3. Documented in a new Site Usage README.

While here, all agent-job failure notification messages were flattened to single lines via implicit string concatenation — the fix dialog renders them with whitespace-pre-wrap, so source indentation and mid-paragraph line breaks were showing as literal whitespace.

Docs

• docs/code/site-update/index.md — the recovery flow, status lifecycle, the transient-only restore fallback, the max_statement_time bump/revert, and the constants.
• press/press/doctype/site_update/README.md — Fatal-state behavior with the transient-error fallback.
• press/press/doctype/site_usage/README.md — the MB-not-bytes gotcha.
• Removed the redundant top-level guide-to-testing.md in favour of docs/code/testing/.

Tests

bench --site <site> run-tests --app press --module press.press.doctype.site_update.test_site_update — passing, including:

• a transient-error migrate recovery triggers Restore Site Tables and, on success, leaves the update Fatal with its cause of failure resolved and the site Active
• a transient-error recovery followed by a failed table restore goes Fatal with fatal_site_update set
• a non-transient recovery failure does not restore tables (stays Fatal)
• a recovery that fails before "Move Site" does not restore tables
• a successful recovery does not restore tables (stays Recovered)
• a skipped-backups update failure goes straight to Fatal with no recovery/restore, and surfaces the actionable SSH notification
• the recovery-migrate max_statement_time bump is stashed and restored to its pre-bump value
• increase_max_statement_time bumps the DB server variable by an hour; Site.database_size returns the latest Site Usage value (in MB)

Also verified end-to-end on a local Vagrant Frappe Cloud: real Update Site Migrate → Recover Failed Site Migrate (killed mid-restore) → Restore Site Tables success, ending Fatal with cause resolved and the site Active.

🤖 Generated with Claude Code

[greptile-apps]

Confidence Score: 3/5

The recovery flow has correctness gaps that could leave site state inconsistent after a fallback restore; hold for fixes before merging.

In process_restore_tables_job_update, reset_previous_status persists the site as Active before the fatal_site_update / cause_of_failure_is_resolved cleanup runs. Any exception in that cleanup chain leaves the site Active but with fatal_site_update still set — a state contradiction the old unconditional clear prevented. Additionally, restore_tables_after_failed_recovery has no error handling around restore_tables(); a raised exception there silently drops the fallback with no comment added.

press/press/doctype/site/site.py (process_restore_tables_job_update) and press/press/doctype/site_update/site_update.py (restore_tables_after_failed_recovery)

Important Files Changed

Filename	Overview
press/press/doctype/site_update/site_update.py	Core recovery logic: adds max_statement_time bump/restore, transient-error fallback to Restore Site Tables, and skipped-backups fatal path. Several correctness edge cases around exception ordering and fallback triggering remain.
press/press/doctype/site/site.py	Adds database_size property, increase/set_max_statement_time helpers, restore_tables return value, and updated process_restore_tables_job_update. Removes the unconditional fatal_site_update clear, introducing an inconsistency if cleanup raises after site is Active.
press/press/doctype/agent_job/agent_job_notifications.py	Adds skipped-backups failure notification with SSH link; reformats existing message strings to single-line implicit concatenation. Logic is correct.
press/press/doctype/site_update/site_update.json	Adds previous_max_statement_time Int field; the field definition includes hide_days/hide_seconds which are Duration-field properties and have no effect on an Int field.
press/press/doctype/site_update/test_site_update.py	Comprehensive test coverage for all recovery paths: transient vs. non-transient, move-site guard, skipped-backups, max_statement_time stash/restore, and successful recovery. Well structured.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Update Site Migrate] -->|Failure| B{Skipped backups?}
    B -->|Yes| C[Fatal — SSH notification]
    B -->|No| D[Recover Failed Site Migrate]
    D -->|Success| E[Recovered]
    E --> F[restore_max_statement_time]
    D -->|Failure| G{Move Site step succeeded?}
    G -->|No| H[Fatal — no fallback]
    G -->|Yes| I{Transient DB error?}
    I -->|No| H
    I -->|Yes| J[Restore Site Tables]
    J -->|Success| K[Site Active, update stays Fatal]
    J -->|Failure| L[Site Broken, fatal_site_update set]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Update Site Migrate] -->|Failure| B{Skipped backups?}
    B -->|Yes| C[Fatal — SSH notification]
    B -->|No| D[Recover Failed Site Migrate]
    D -->|Success| E[Recovered]
    E --> F[restore_max_statement_time]
    D -->|Failure| G{Move Site step succeeded?}
    G -->|No| H[Fatal — no fallback]
    G -->|Yes| I{Transient DB error?}
    I -->|No| H
    I -->|Yes| J[Restore Site Tables]
    J -->|Success| K[Site Active, update stays Fatal]
    J -->|Failure| L[Site Broken, fatal_site_update set]

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
press/press/doctype/site/site.py:5029-5038
**Unconditional `fatal_site_update` clear replaced by error-prone conditional**

The old code always ran `frappe.db.set_value("Site", job.site, "fatal_site_update", None)` after `reset_previous_status`. Now the clear depends on `set_cause_of_failure_is_resolved()` completing successfully. If `frappe.get_doc("Site Update", fatal_update)` raises (e.g. the SiteUpdate was deleted) or `restore_max_statement_time()` raises (e.g. the database server is unreachable), the site will be left `Active` (status already persisted by `reset_previous_status` → `self.save()`) with `fatal_site_update` still set and `cause_of_failure_is_resolved` still `0`. Adding a `try/finally` that calls `frappe.db.set_value("Site", job.site, "fatal_site_update", None)` — mirroring the old unconditional clear — would prevent this inconsistency.

_{Reviews (10): Last reviewed commit: "test(site-update): A transient update-jo..." | Re-trigger Greptile}

[greptile-apps]

User-visible strings not wrapped with _() for i18n. details["title"] and all literal text in details["message"] should be translated per the project's internationalisation rules.

Suggested change

	details["title"] = "Site update failed and cannot be recovered automatically"
	details["message"] = f"""<p>The update for site <b>{job.site}</b> failed.</p>
	<p>Because this update was run with backups skipped, there is no backup to restore from and the
	site cannot be recovered automatically. The database may be left in a partially migrated state.</p>
	<p>Please connect to the bench over SSH and fix the site manually.</p>
	"""
	details["title"] = _("Site update failed and cannot be recovered automatically")
	details["message"] = f"""<p>{_("The update for site")} <b>{job.site}</b> {_("failed.")}</p>
	<p>{_("Because this update was run with backups skipped, there is no backup to restore from and the site cannot be recovered automatically. The database may be left in a partially migrated state.")}</p>
	<p>{_("Please connect to the bench over SSH and fix the site manually.")}</p>
	"""

Context Used: This is a Frappe Framework application (Python bac... (source)

Prompt To Fix With AI

This is a comment left during a code review.
Path: press/press/doctype/agent_job/agent_job_notifications.py
Line: 203-208

Comment:
User-visible strings not wrapped with `_()` for i18n. `details["title"]` and all literal text in `details["message"]` should be translated per the project's internationalisation rules.

```suggestion
	details["title"] = _("Site update failed and cannot be recovered automatically")
	details["message"] = f"""<p>{_("The update for site")} <b>{job.site}</b> {_("failed.")}</p>
	<p>{_("Because this update was run with backups skipped, there is no backup to restore from and the site cannot be recovered automatically. The database may be left in a partially migrated state.")}</p>
	<p>{_("Please connect to the bench over SSH and fix the site manually.")}</p>
	"""
```

**Context Used:** This is a Frappe Framework application (Python bac... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

How can I resolve this? If you propose a fix, please make it concise.

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!

[greptile-apps]

Inconsistent retry-limit semantics vs MAX_RECOVERY_RETRIES

> MAX_STATEMENT_TIMEOUT_RETRIES (strict greater-than) lets the function retry when failed_attempts is 1, 2, or 3, giving 3 retries and 4 total attempts before stopping. The symmetrical guard in should_retry_recovery uses < MAX_RECOVERY_RETRIES (strict less-than), which allows only 2 retries and 3 total attempts before going Fatal. Both constants equal 3, but the effective limits differ by one attempt. The docs for this method say "up to MAX_STATEMENT_TIMEOUT_RETRIES times" (i.e. 3 retries = 4 total), so the > comparison is internally correct — but future readers will naturally read both guards as symmetric and may accidentally normalise one to match the other, silently changing the intended limit.

Prompt To Fix With AI

This is a comment left during a code review.
Path: press/press/doctype/site/site.py
Line: 1224

Comment:
**Inconsistent retry-limit semantics vs `MAX_RECOVERY_RETRIES`**

`> MAX_STATEMENT_TIMEOUT_RETRIES` (strict greater-than) lets the function retry when `failed_attempts` is 1, 2, **or 3**, giving 3 retries and 4 total attempts before stopping. The symmetrical guard in `should_retry_recovery` uses `< MAX_RECOVERY_RETRIES` (strict less-than), which allows only 2 retries and 3 total attempts before going Fatal. Both constants equal 3, but the effective limits differ by one attempt. The docs for this method say "up to `MAX_STATEMENT_TIMEOUT_RETRIES` times" (i.e. 3 retries = 4 total), so the `>` comparison is internally correct — but future readers will naturally read both guards as symmetric and may accidentally normalise one to match the other, silently changing the intended limit.

How can I resolve this? If you propose a fix, please make it concise.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.92713% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.91%. Comparing base (3a677a9) to head (a390615).
⚠️ Report is 76 commits behind head on develop.

Files with missing lines	Patch %	Lines
...press/doctype/agent_job/agent_job_notifications.py	60.00%	8 Missing ⚠️
press/press/doctype/site/site.py	85.71%	5 Missing ⚠️
press/press/doctype/site_update/site_update.py	97.05%	1 Missing ⚠️
...ress/press/doctype/site_update/test_site_update.py	99.36%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #6729       +/-   ##
============================================
- Coverage    90.90%   56.91%   -34.00%     
============================================
  Files          117      994      +877     
  Lines        18093    83995    +65902     
  Branches       678      682        +4     
============================================
+ Hits         16448    47803    +31355     
- Misses        1610    36159    +34549     
+ Partials        35       33        -2     

Flag	Coverage Δ
dashboard	90.92% <ø> (+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.

[greptile-apps]

database_size silently returns 0 when no Site Usage row exists yet, causing the > LARGE_DATABASE_SIZE guard in trigger_recovery_job to be False for any site without usage history. A large site whose usage was never recorded will have max_statement_time skipped, re-exposing the exact timeout risk this feature aims to prevent. Returning None and handling the sentinel explicitly in the caller makes the "no data" case visible rather than silently treating it as "small".

Suggested change

	return (
	frappe.db.get_value("Site Usage", {"site": self.name}, "database", order_by="creation desc") or 0
	)
	return (
	frappe.db.get_value("Site Usage", {"site": self.name}, "database", order_by="creation desc") or None
	)

Prompt To Fix With AI

This is a comment left during a code review.
Path: press/press/doctype/site/site.py
Line: 1184-1186

Comment:
`database_size` silently returns `0` when no Site Usage row exists yet, causing the `> LARGE_DATABASE_SIZE` guard in `trigger_recovery_job` to be `False` for any site without usage history. A large site whose usage was never recorded will have `max_statement_time` skipped, re-exposing the exact timeout risk this feature aims to prevent. Returning `None` and handling the sentinel explicitly in the caller makes the "no data" case visible rather than silently treating it as "small".

```suggestion
		return (
			frappe.db.get_value("Site Usage", {"site": self.name}, "database", order_by="creation desc") or None
		)
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Silent exception swallows the restore-tables fallback

If restore_tables() raises (e.g. the agent is unreachable), the exception propagates out of process_update_site_recover_job_update. The site is already marked Broken with fatal_site_update set (the two frappe.db.set_value calls on lines 1214-1215 run before this function), but no Restore Site Tables job is created and no comment is added. The fallback silently disappears: the operator sees a broken site with no indication that an automatic restore was even attempted or failed. Wrapping the call in a try/except and logging (or adding a comment on failure) would make the failure visible and leave the site in a clearly actionable state.

Prompt To Fix With AI

This is a comment left during a code review.
Path: press/press/doctype/site_update/site_update.py
Line: 1172-1182

Comment:
**Silent exception swallows the restore-tables fallback**

If `restore_tables()` raises (e.g. the agent is unreachable), the exception propagates out of `process_update_site_recover_job_update`. The site is already marked `Broken` with `fatal_site_update` set (the two `frappe.db.set_value` calls on lines 1214-1215 run before this function), but no `Restore Site Tables` job is created and no comment is added. The fallback silently disappears: the operator sees a broken site with no indication that an automatic restore was even attempted or failed. Wrapping the call in a try/except and logging (or adding a comment on failure) would make the failure visible and leave the site in a clearly actionable state.

How can I resolve this? If you propose a fix, please make it concise.

[balamurali27]

Note for reviewers — review the net diff, not the commit history.

The early commits of this PR (roughly 7ff506d9…b5365565) were over-ambitious and no longer reflect the final design — please don't review them commit-by-commit.

They built a full retry mechanism around recovery: up to MAX_RECOVERY_RETRIES (3) recover-job retries on transient DB errors, a statement-timeout retry loop around restore tables, flipping the update to Recovered after a restore, etc. That machinery was later torn back out in favour of a minimal one-shot fallback: when a migrate recovery fails after moving the site back due to a transient DB error, re-issue just Restore Site Tables once; everything else stays Fatal. The max_statement_time bump survived (now also reverted after recovery), and the threshold moved to 2 GB.

So most of those starting commits are effectively superseded. The parts that carried through unchanged are the tests (the scaffolding/helpers in test_site_update.py) and the SSH/skipped-backups notification. The current behaviour is best read from the PR description and the final state of the files, not the intermediate commits.

[greptile-apps]

"Delivery Failure" skips restore_max_statement_time

The guard job.status == "Failure" misses "Delivery Failure". The pre-existing updated_status dict also lacks a "Delivery Failure" key (so it would KeyError before reaching this line today), but once that pre-existing mapping gap is fixed, restore_max_statement_time would still be silently skipped — leaving max_statement_time permanently bumped on the database server. Every analogous handler in this file (process_update_site_recover_job_update, etc.) handles "Delivery Failure" explicitly. Change to job.status in ("Failure", "Delivery Failure").

Suggested change

	# A failed recovery fallback ends here; put back any bumped max_statement_time.
	if job.status == "Failure" and (
	fatal_update := frappe.db.get_value("Site", job.site, "fatal_site_update")
	):
	frappe.get_doc("Site Update", fatal_update).restore_max_statement_time()
	# A failed recovery fallback ends here; put back any bumped max_statement_time.
	if job.status in ("Failure", "Delivery Failure") and (
	fatal_update := frappe.db.get_value("Site", job.site, "fatal_site_update")
	):
	frappe.get_doc("Site Update", fatal_update).restore_max_statement_time()

Prompt To Fix With AI

This is a comment left during a code review.
Path: press/press/doctype/site/site.py
Line: 5043-5047

Comment:
**`"Delivery Failure"` skips `restore_max_statement_time`**

The guard `job.status == "Failure"` misses `"Delivery Failure"`. The pre-existing `updated_status` dict also lacks a `"Delivery Failure"` key (so it would `KeyError` before reaching this line today), but once that pre-existing mapping gap is fixed, `restore_max_statement_time` would still be silently skipped — leaving `max_statement_time` permanently bumped on the database server. Every analogous handler in this file (`process_update_site_recover_job_update`, etc.) handles `"Delivery Failure"` explicitly. Change to `job.status in ("Failure", "Delivery Failure")`.

```suggestion
			# A failed recovery fallback ends here; put back any bumped max_statement_time.
			if job.status in ("Failure", "Delivery Failure") and (
				fatal_update := frappe.db.get_value("Site", job.site, "fatal_site_update")
			):
				frappe.get_doc("Site Update", fatal_update).restore_max_statement_time()
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Unconditional fatal_site_update clear replaced by error-prone conditional

The old code always ran frappe.db.set_value("Site", job.site, "fatal_site_update", None) after reset_previous_status. Now the clear depends on set_cause_of_failure_is_resolved() completing successfully. If frappe.get_doc("Site Update", fatal_update) raises (e.g. the SiteUpdate was deleted) or restore_max_statement_time() raises (e.g. the database server is unreachable), the site will be left Active (status already persisted by reset_previous_status → self.save()) with fatal_site_update still set and cause_of_failure_is_resolved still 0. Adding a try/finally that calls frappe.db.set_value("Site", job.site, "fatal_site_update", None) — mirroring the old unconditional clear — would prevent this inconsistency.

Prompt To Fix With AI

This is a comment left during a code review.
Path: press/press/doctype/site/site.py
Line: 5029-5038

Comment:
**Unconditional `fatal_site_update` clear replaced by error-prone conditional**

The old code always ran `frappe.db.set_value("Site", job.site, "fatal_site_update", None)` after `reset_previous_status`. Now the clear depends on `set_cause_of_failure_is_resolved()` completing successfully. If `frappe.get_doc("Site Update", fatal_update)` raises (e.g. the SiteUpdate was deleted) or `restore_max_statement_time()` raises (e.g. the database server is unreachable), the site will be left `Active` (status already persisted by `reset_previous_status` → `self.save()`) with `fatal_site_update` still set and `cause_of_failure_is_resolved` still `0`. Adding a `try/finally` that calls `frappe.db.set_value("Site", job.site, "fatal_site_update", None)` — mirroring the old unconditional clear — would prevent this inconsistency.

How can I resolve this? If you propose a fix, please make it concise.