fix(backup): clean up staging/temp files on abnormal exit and tighten disk-space pre-check by mrrobot47 · Pull Request #482 · EasyEngine/site-command

mrrobot47 · 2026-06-29T16:59:39Z

Summary

Resource-cleanup, disk-estimate accuracy, and minor-correctness hardening for the backup/restore flow.

Changes

Staging-dir / partial-download cleanup on abnormal exit. A backup's half-built archive (EE_BACKUP_DIR/<site>) and a restore's downloaded archive were removed only on success; any error exit left them to fill the disk (and a stale partial download could be reused). A register_shutdown_function now purges the staging dir on abnormal exit, with the tracked path cleared after the success-path remove so it only fires on failure. For restore, the tracking is armed in pre_restore_check() right after the per-site lock is acquired, before the download. Any leftover staging dir from an earlier failed or killed run is wiped at that point, so a restore always downloads the requested backup (previously a leftover metadata.json/archive was silently reused, even with a different --id). Nothing is armed or removed before the lock is held, so a lock-conflict or bad-id early exit can't delete a concurrent run's dir. The cleanup handler is registered in the constructor, so it runs before the lock-release and dash handlers. PHP fatals and uncaught exceptions still skip it (the site command's shutdown handler exits first); the wipe at the next run's start covers that.
Temp query-file cleanup. query.sql / db_size_query.sql written into the live web root are removed inline; a shutdown handler is a safety net for an interrupt between write and remove. Both cleanup handlers are wrapped in try/catch so a failed unlink (Symfony's Filesystem::remove() throws) can never abort the other shutdown handlers (lock release, dash callback).
Disk-space estimate accuracy. The pre-check now sizes the full archived set (app/ + config/, not just app/htdocs), guards false === disk_free_space() and a missing app/, and warns (instead of silently counting 0) when a file or the DB size can't be read. Required space is ceil(total_source * 1.1) (DB counted once — it's already in the source total — plus 10% slack), avoiding both the old under-count and a DB double-count that would false-reject DB-heavy sites.
dir_size() robustness. Built with CATCH_GET_CHILD | LEAVES_ONLY so an untraversable subdirectory is skipped rather than throwing UnexpectedValueException and aborting the whole backup; a missing dir returns 0 (some archived dirs are optional). A file that disappears mid-scan is counted as unsized instead of aborting with RuntimeException, and broken symlinks are skipped silently.
cleanup_old_backups() off-by-one. Retention triggered at N+1; now triggers at N to match the array_slice that keeps N.
list_remote_backups() empty result. explode(PHP_EOL, trim('')) yields [''], making the "No remote backups found" branch unreachable; now filters blank lines.

Verification

A failed upload, a restore type mismatch and a failed download leave no staging dir behind; a leftover staging dir is ignored and the backup is downloaded fresh; a lock held by another run leaves that run's staging dir alone; a 5 TB sparse file under app/ makes the pre-check fail fast with no staging dir or lock left; --list with no backups prints "No remote backups found."; with no-of-backups: 2, --dash-auth retention keeps 2 (previously 3).

Tested on Ubuntu 26.04 with EasyEngine 4.12.0 (WordPress and PHP-with-DB sites), on a real rclone remote for the happy path and a backup/restore round trip, and on a local rclone remote for the failure cases.

Merge note

Conflicts with #480 (flock lock). When reconciling, replace each try_remove_site_lock() call with #480's release_site_backup_lock() and delete try_remove_site_lock() (never unlink a flocked lock file). Keep this PR's constructor registration of cleanup_staging_dir. Error codes 3003 (site app directory not found) and 3004 (unable to determine free disk space) are new here, and no other open PR uses them: #481 adds 4003/4004 and #483 adds 3005/4005. The full list of new backup error codes is in #483.

Resource-cleanup, disk-estimate accuracy and minor correctness fixes in Site_Backup_Restore, all scoped to non-lock/non-archive concerns: - Register shutdown handlers that purge the local staging dir (EE_BACKUP_DIR/<site_url>) and the leftover temp SQL query files on any abnormal exit. The success path clears the tracked state so the handlers no-op; previously an error/crash leaked the half-built archive (disk fill) or a stale partial download a later restore could reuse. - pre_backup_check(): size the full set of dirs actually archived per site type (app/ + config/, not just app/htdocs) plus DB, add headroom for the transient uncompressed dump and growing archive, and fail up front when free space is undeterminable instead of treating it as 0. - dir_size()/get_db_size(): warn instead of silently counting an unreadable/unparseable size as 0, so the estimate isn't quietly low. - cleanup_old_backups(): drop the off-by-one (`> N + 1`) so retention triggers at exactly N, matching the array_slice that keeps N. - list_remote_backups(): filter blank lines so the empty-listing ("No remote backups found") path is actually reachable.

Address review findings on the cleanup/robustness changes: - restore(): set $staging_dir and register cleanup_staging_dir only AFTER pre_restore_check() has acquired this site's lock (mirroring backup()). Registering it earlier let an early exit (invalid backup id, or a lock held by another process) delete the staging dir a concurrent backup/restore of the same site was actively writing into -- data loss. - cleanup_staging_dir()/cleanup_temp_query_files(): wrap each shutdown safety-net body in try/catch ( \Throwable ). Symfony Filesystem::remove() throws IOException on a failed unlink (e.g. permission denied under the www-data-owned web root); a throw in the first shutdown handler would abort the remaining handlers, leaking the global lock and skipping the EasyDash failure callback. - pre_backup_check(): count the DB size once. $site_size already includes it, so multiplying ( $site_size + $db_size ) over-counted the DB ~2.2x and false-rejected DB-heavy sites; require ceil( $site_size * 1.1 ) instead. - dir_size(): build the recursive iterator with CATCH_GET_CHILD (and LEAVES_ONLY) so an untraversable subdirectory is skipped instead of throwing UnexpectedValueException and aborting the whole backup.

Copilot

Pull request overview

This PR hardens the backup/restore helper by ensuring staging artifacts and temporary query files are cleaned up on abnormal exits, and by improving disk-space preflight estimation/robustness so failures don’t leave behind large partial state.

Changes:

Add shutdown-time cleanup for per-site staging directories and temporary SQL query files to prevent disk fill and stale partial artifacts after crashes/interrupts.
Tighten disk-space pre-check sizing (include app/ + config/, add 10% headroom, guard disk_free_space()), and make dir_size() resilient to missing/untraversable paths.
Fix minor correctness issues in remote backup listing (empty output) and retention logic (off-by-one).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mrrobot47 · 2026-06-29T17:59:30Z

+			$this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' );
+			EE::error( "Site app directory does not exist: $site_root/app" );


Fixed in e92c356 — per-site lock removals now go through a best-effort try_remove_site_lock() (try/catch), so a Filesystem::remove() failure on an error path can no longer mask the intended EE::error().

mrrobot47 · 2026-06-29T17:59:32Z

+			$this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' );
+			EE::error( 'Unable to determine free disk space for backup directory.' );


Fixed in e92c356 — per-site lock removals now go through a best-effort try_remove_site_lock() (try/catch), so a Filesystem::remove() failure on an error path can no longer mask the intended EE::error().

mrrobot47 · 2026-06-29T17:59:33Z

+		$db_size   = 0;
+		$site_size = $this->dir_size( $site_root . '/app' );
+		$site_size += $this->dir_size( $site_root . '/config' );
+
+		EE::debug( 'Site size (files): ' . $site_size );


Fixed in e92c356 — inlined the single-use $db_size directly into $site_size.

Addresses code-review feedback: Filesystem::remove() throws on a failed unlink, so removing the lock on an error path could raise an uncaught exception and mask the intended EE::error(). Route all per-site lock removals through a best-effort try_remove_site_lock() helper. Also inline the single-use $db_size into $site_size.

…ore lock release The restore download (rclone copy) and the metadata checks run inside pre_restore_check(), before restore() armed the staging-dir cleanup, so a failed download or a site-type/DB/public-dir mismatch still left the archive in EE_BACKUP_DIR/<site>, and the next restore reused it without downloading (even with a different --id). Arm the tracking in pre_restore_check() right after the site lock is taken, and drop any leftover from an earlier run there, as backup() already does. Register cleanup_staging_dir() in the constructor so it runs before any lock-release shutdown handler (e.g. the flock-based per-site lock), otherwise a run acquiring the freed lock could have its fresh staging dir deleted.

…r_size() SplFileInfo::getSize() throws RuntimeException when stat fails (e.g. a file removed during the scan) rather than returning false, so the false check never ran and the exception aborted the backup. Catch it and count the file as unsized. Skip broken symlinks without counting them, so they don't trigger the "could not size" warning on every backup.

3003 and 4003 are already taken by EasyEngine#482 (site app directory not found) and EasyEngine#481 (orphaned remote backup delete failure). Use the next free codes: 3005 for the archive integrity failure and 4005 for the dump staging failure.

mrrobot47 added 2 commits June 29, 2026 22:08

mrrobot47 requested a review from Copilot June 29, 2026 17:46

Copilot started reviewing on behalf of mrrobot47 June 29, 2026 17:47 View session

Copilot AI reviewed Jun 29, 2026

View reviewed changes

mrrobot47 added 3 commits June 29, 2026 23:28

mrrobot47 mentioned this pull request Sep 24, 2026

fix(backup): verify DB dump and archive integrity to prevent silent corrupt backups #483

Open

Navigation Menu

fix(backup): clean up staging/temp files on abnormal exit and tighten disk-space pre-check - #482

fix(backup): clean up staging/temp files on abnormal exit and tighten disk-space pre-check#482
mrrobot47 wants to merge 5 commits into
EasyEngine:developfrom
mrrobot47:fix/backup-cleanup-and-robustness

mrrobot47 commented Jun 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

mrrobot47 Jun 29, 2026

Uh oh!

mrrobot47 Jun 29, 2026

Uh oh!

mrrobot47 Jun 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		$this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' );
		EE::error( "Site app directory does not exist: $site_root/app" );

		$this->fs->remove( EE_BACKUP_DIR . '/' . $this->site_data['site_url'] . '.lock' );
		EE::error( 'Unable to determine free disk space for backup directory.' );

Conversation

mrrobot47 commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Verification

Merge note

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mrrobot47 Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

mrrobot47 Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

mrrobot47 Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mrrobot47 commented Jun 29, 2026 •

edited

Loading